Re: [PATCH v24 06/34] usb: host: xhci-mem: Allow for interrupter clients to choose specific index

On 8/1/2024 3:17 AM, Wesley Cheng wrote:

Some clients may operate only on a specific XHCI interrupter instance. Allow for the associated class driver to request for the interrupter that it requires.

Signed-off-by: Wesley Cheng quic_wcheng@quicinc.com

drivers/usb/host/xhci-mem.c | 29 ++++++++++++++++------------- drivers/usb/host/xhci-sideband.c | 4 ++-- drivers/usb/host/xhci.h | 2 +- include/linux/usb/xhci-sideband.h | 2 +- 4 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 60dfc59260d8..997e8f27acb8 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -2340,7 +2340,7 @@ xhci_add_interrupter(struct xhci_hcd *xhci, struct xhci_interrupter *ir,

struct xhci_interrupter * xhci_create_secondary_interrupter(struct usb_hcd *hcd, unsigned int segs,

• 			u32 imod_interval)
  

• 			u32 imod_interval, int intr_num)
  

  { struct xhci_hcd *xhci = hcd_to_xhci(hcd); struct xhci_interrupter *ir;

@@ -2355,29 +2355,32 @@ xhci_create_secondary_interrupter(struct usb_hcd *hcd, unsigned int segs, return NULL;

spin_lock_irq(&xhci->lock);

• /* Find available secondary interrupter, interrupter 0 is reserved for primary */ for (i = 1; i < xhci->max_interrupters; i++) {

• if (xhci->interrupters[i] == NULL) {
  

• 	err = xhci_add_interrupter(xhci, ir, i);
  

• 	break;
  

• if ((intr_num > 0 && i == intr_num) || intr_num <= 0) {
  

• 	if (xhci->interrupters[i] == NULL) {
  

• 		err = xhci_add_interrupter(xhci, ir, i);
  

• 		if (err) {
  

• 			spin_unlock_irq(&xhci->lock);
  

• 			goto free_ir;
  

• 		}
  

• 		break;
  

• 	}
  

  } }

When intr_num is set, you don't really need to iterate to search for free slot, as you already know which one you want to use?

Wouldn't something like: /* Find available secondary interrupter, interrupter 0 is reserved for primary */ if (!intr_num) { for (i = 1; i < xhci->max_interrupters; i++) { if (xhci->interrupters[i] == NULL) { err = xhci_add_interrupter(xhci, ir, i); break; } } } else { if (xhci->interrupters[intr_num] == NULL) err = xhci_add_interrupter(xhci, ir, i); } make more sense, also make intr_num unsigned int, as it should never be negative as it is index into a table.

• spin_unlock_irq(&xhci->lock);

• if (err) {

• xhci_warn(xhci, "Failed to add secondary interrupter, max interrupters %d\n",
  

• 	  xhci->max_interrupters);
  

• xhci_free_interrupter(xhci, ir);
  

• return NULL;
  

• }

• xhci_set_interrupter_moderation(ir, imod_interval);

• xhci_dbg(xhci, "Add secondary interrupter %d, max interrupters %d\n", i, xhci->max_interrupters);

  return ir;

+free_ir:

• xhci_free_interrupter(xhci, ir);
• return NULL; } EXPORT_SYMBOL_GPL(xhci_create_secondary_interrupter);

(...)