Hi, Alan. > -----Original Message----- > From: linux-usb-owner@xxxxxxxxxxxxxxx [mailto:linux-usb- > owner@xxxxxxxxxxxxxxx] On Behalf Of Alan Stern > Sent: Friday, August 19, 2011 5:32 AM > To: Greg KH > Cc: USB list > Subject: [PATCH] USB: EHCI: remove usages of hcd->state > > This patch (as1483) improves the ehci-hcd driver family by getting rid > of the reliance on the hcd->state variable. It has no clear owner and > it isn't protected by the usual HCD locks. In its place, the patch > adds a new, private ehci->rh_state field to record the state of the > root hub. > > Along the way, the patch removes a couple of lines containing > redundant assignments to the state variable. Also, the QUIESCING > state simply gets changed to the RUNNING state, because the driver > doesn't make any distinction between them. > > Signed-off-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> Acked-by: Jingoo Han <jg1.han@xxxxxxxxxxx> I reviewed your patch and tested ehci-s5p.c. It works properly. > > --- > > drivers/usb/host/ehci-au1xxx.c | 2 +- > drivers/usb/host/ehci-dbg.c | 17 +++++++++++++++-- > drivers/usb/host/ehci-fsl.c | 4 ++-- > drivers/usb/host/ehci-hcd.c | 20 ++++++++++---------- > drivers/usb/host/ehci-hub.c | 10 ++++------ > drivers/usb/host/ehci-pci.c | 2 +- > drivers/usb/host/ehci-q.c | 22 ++++++++++------------ > drivers/usb/host/ehci-s5p.c | 2 +- > drivers/usb/host/ehci-sched.c | 11 +++++------ > drivers/usb/host/ehci.h | 7 +++++++ > 10 files changed, 56 insertions(+), 41 deletions(-) > > Index: usb-3.0/drivers/usb/host/ehci.h > =================================================================== > --- usb-3.0.orig/drivers/usb/host/ehci.h > +++ usb-3.0/drivers/usb/host/ehci.h > @@ -62,6 +62,12 @@ struct ehci_stats { > > #define EHCI_MAX_ROOT_PORTS 15 /* see HCS_N_PORTS */ > > +enum ehci_rh_state { > + EHCI_RH_HALTED, > + EHCI_RH_SUSPENDED, > + EHCI_RH_RUNNING > +}; > + > struct ehci_hcd { /* one per controller */ > /* glue to PCI and HCD framework */ > struct ehci_caps __iomem *caps; > @@ -70,6 +76,7 @@ struct ehci_hcd { /* one per controlle > > __u32 hcs_params; /* cached register copy */ > spinlock_t lock; > + enum ehci_rh_state rh_state; > > /* async schedule support */ > struct ehci_qh *async; > Index: usb-3.0/drivers/usb/host/ehci-dbg.c > =================================================================== > --- usb-3.0.orig/drivers/usb/host/ehci-dbg.c > +++ usb-3.0/drivers/usb/host/ehci-dbg.c > @@ -697,6 +697,19 @@ static ssize_t fill_periodic_buffer(stru > } > #undef DBG_SCHED_LIMIT > > +static const char *rh_state_string(struct ehci_hcd *ehci) > +{ > + switch (ehci->rh_state) { > + case EHCI_RH_HALTED: > + return "halted"; > + case EHCI_RH_SUSPENDED: > + return "suspended"; > + case EHCI_RH_RUNNING: > + return "running"; > + } > + return "?"; > +} > + > static ssize_t fill_registers_buffer(struct debug_buffer *buf) > { > struct usb_hcd *hcd; > @@ -730,11 +743,11 @@ static ssize_t fill_registers_buffer(str > temp = scnprintf (next, size, > "bus %s, device %s\n" > "%s\n" > - "EHCI %x.%02x, hcd state %d\n", > + "EHCI %x.%02x, rh state %s\n", > hcd->self.controller->bus->name, > dev_name(hcd->self.controller), > hcd->product_desc, > - i >> 8, i & 0x0ff, hcd->state); > + i >> 8, i & 0x0ff, rh_state_string(ehci)); > size -= temp; > next += temp; > > Index: usb-3.0/drivers/usb/host/ehci-hcd.c > =================================================================== > --- usb-3.0.orig/drivers/usb/host/ehci-hcd.c > +++ usb-3.0/drivers/usb/host/ehci-hcd.c > @@ -238,7 +238,7 @@ static int handshake_on_error_set_halt(s > error = handshake(ehci, ptr, mask, done, usec); > if (error) { > ehci_halt(ehci); > - ehci_to_hcd(ehci)->state = HC_STATE_HALT; > + ehci->rh_state = EHCI_RH_HALTED; > ehci_err(ehci, "force halt; handshake %p %08x %08x -> %d\n", > ptr, mask, done, error); > } > @@ -278,7 +278,7 @@ static int ehci_reset (struct ehci_hcd * > command |= CMD_RESET; > dbg_cmd (ehci, "reset", command); > ehci_writel(ehci, command, &ehci->regs->command); > - ehci_to_hcd(ehci)->state = HC_STATE_HALT; > + ehci->rh_state = EHCI_RH_HALTED; > ehci->next_statechange = jiffies; > retval = handshake (ehci, &ehci->regs->command, > CMD_RESET, 0, 250 * 1000); > @@ -307,7 +307,7 @@ static void ehci_quiesce (struct ehci_hc > u32 temp; > > #ifdef DEBUG > - if (!HC_IS_RUNNING (ehci_to_hcd(ehci)->state)) > + if (ehci->rh_state != EHCI_RH_RUNNING) > BUG (); > #endif > > @@ -356,7 +356,7 @@ static void ehci_iaa_watchdog(unsigned l > */ > if (ehci->reclaim > && !timer_pending(&ehci->iaa_watchdog) > - && HC_IS_RUNNING(ehci_to_hcd(ehci)->state)) { > + && ehci->rh_state == EHCI_RH_RUNNING) { > u32 cmd, status; > > /* If we get here, IAA is *REALLY* late. It's barely > @@ -496,7 +496,7 @@ static void ehci_work (struct ehci_hcd * > * misplace IRQs, and should let us run completely without IRQs. > * such lossage has been observed on both VT6202 and VT8235. > */ > - if (HC_IS_RUNNING (ehci_to_hcd(ehci)->state) && > + if (ehci->rh_state == EHCI_RH_RUNNING && > (ehci->async->qh_next.ptr != NULL || > ehci->periodic_sched != 0)) > timer_action (ehci, TIMER_IO_WATCHDOG); > @@ -516,7 +516,7 @@ static void ehci_stop (struct usb_hcd *h > del_timer_sync(&ehci->iaa_watchdog); > > spin_lock_irq(&ehci->lock); > - if (HC_IS_RUNNING (hcd->state)) > + if (ehci->rh_state == EHCI_RH_RUNNING) > ehci_quiesce (ehci); > > ehci_silence_controller(ehci); > @@ -741,7 +741,7 @@ static int ehci_run (struct usb_hcd *hcd > * be started before the port switching actions could complete. > */ > down_write(&ehci_cf_port_reset_rwsem); > - hcd->state = HC_STATE_RUNNING; > + ehci->rh_state = EHCI_RH_RUNNING; > ehci_writel(ehci, FLAG_CF, &ehci->regs->configured_flag); > ehci_readl(ehci, &ehci->regs->command); /* unblock posted writes */ > msleep(5); > @@ -788,7 +788,7 @@ static irqreturn_t ehci_irq (struct usb_ > > /* Shared IRQ? */ > masked_status = status & INTR_MASK; > - if (!masked_status || unlikely(hcd->state == HC_STATE_HALT)) { > + if (!masked_status || unlikely(ehci->rh_state == EHCI_RH_HALTED)) { > spin_unlock(&ehci->lock); > return IRQ_NONE; > } > @@ -952,7 +952,7 @@ static int ehci_urb_enqueue ( > static void unlink_async (struct ehci_hcd *ehci, struct ehci_qh *qh) > { > /* failfast */ > - if (!HC_IS_RUNNING(ehci_to_hcd(ehci)->state) && ehci->reclaim) > + if (ehci->rh_state != EHCI_RH_RUNNING && ehci->reclaim) > end_unlink_async(ehci); > > /* If the QH isn't linked then there's nothing we can do > @@ -1079,7 +1079,7 @@ rescan: > goto idle_timeout; > } > > - if (!HC_IS_RUNNING (hcd->state)) > + if (ehci->rh_state != EHCI_RH_RUNNING) > qh->qh_state = QH_STATE_IDLE; > switch (qh->qh_state) { > case QH_STATE_LINKED: > Index: usb-3.0/drivers/usb/host/ehci-hub.c > =================================================================== > --- usb-3.0.orig/drivers/usb/host/ehci-hub.c > +++ usb-3.0/drivers/usb/host/ehci-hub.c > @@ -236,10 +236,8 @@ static int ehci_bus_suspend (struct usb_ > } > > /* stop schedules, clean any completed work */ > - if (HC_IS_RUNNING(hcd->state)) { > + if (ehci->rh_state == EHCI_RH_RUNNING) > ehci_quiesce (ehci); > - hcd->state = HC_STATE_QUIESCING; > - } > ehci->command = ehci_readl(ehci, &ehci->regs->command); > ehci_work(ehci); > > @@ -313,7 +311,7 @@ static int ehci_bus_suspend (struct usb_ > > /* turn off now-idle HC */ > ehci_halt (ehci); > - hcd->state = HC_STATE_SUSPENDED; > + ehci->rh_state = EHCI_RH_SUSPENDED; > > if (ehci->reclaim) > end_unlink_async(ehci); > @@ -382,6 +380,7 @@ static int ehci_bus_resume (struct usb_h > > /* restore CMD_RUN, framelist size, and irq threshold */ > ehci_writel(ehci, ehci->command, &ehci->regs->command); > + ehci->rh_state = EHCI_RH_RUNNING; > > /* Some controller/firmware combinations need a delay during which > * they set up the port statuses. See Bugzilla #8190. */ > @@ -452,7 +451,6 @@ static int ehci_bus_resume (struct usb_h > } > > ehci->next_statechange = jiffies + msecs_to_jiffies(5); > - hcd->state = HC_STATE_RUNNING; > > /* Now we can safely re-enable irqs */ > ehci_writel(ehci, INTR_MASK, &ehci->regs->intr_enable); > @@ -564,7 +562,7 @@ ehci_hub_status_data (struct usb_hcd *hc > u32 ppcd = 0; > > /* if !USB_SUSPEND, root hub timers won't get shut down ... */ > - if (!HC_IS_RUNNING(hcd->state)) > + if (ehci->rh_state != EHCI_RH_RUNNING) > return 0; > > /* init status to no-changes */ > Index: usb-3.0/drivers/usb/host/ehci-q.c > =================================================================== > --- usb-3.0.orig/drivers/usb/host/ehci-q.c > +++ usb-3.0/drivers/usb/host/ehci-q.c > @@ -153,7 +153,7 @@ static void ehci_clear_tt_buffer_complet > spin_lock_irqsave(&ehci->lock, flags); > qh->clearing_tt = 0; > if (qh->qh_state == QH_STATE_IDLE && !list_empty(&qh->qtd_list) > - && HC_IS_RUNNING(hcd->state)) > + && ehci->rh_state == EHCI_RH_RUNNING) > qh_link_async(ehci, qh); > spin_unlock_irqrestore(&ehci->lock, flags); > } > @@ -425,7 +425,7 @@ qh_completions (struct ehci_hcd *ehci, s > > /* stop scanning when we reach qtds the hc is using */ > } else if (likely (!stopped > - && HC_IS_RUNNING (ehci_to_hcd(ehci)->state))) { > + && ehci->rh_state == EHCI_RH_RUNNING)) { > break; > > /* scan the whole queue for unlinks whenever it stops */ > @@ -433,7 +433,7 @@ qh_completions (struct ehci_hcd *ehci, s > stopped = 1; > > /* cancel everything if we halt, suspend, etc */ > - if (!HC_IS_RUNNING(ehci_to_hcd(ehci)->state)) > + if (ehci->rh_state != EHCI_RH_RUNNING) > last_status = -ESHUTDOWN; > > /* this qtd is active; skip it unless a previous qtd > @@ -977,9 +977,8 @@ static void qh_link_async (struct ehci_h > /* in case a clear of CMD_ASE didn't take yet */ > (void)handshake(ehci, &ehci->regs->status, > STS_ASS, 0, 150); > - cmd |= CMD_ASE | CMD_RUN; > + cmd |= CMD_ASE; > ehci_writel(ehci, cmd, &ehci->regs->command); > - ehci_to_hcd(ehci)->state = HC_STATE_RUNNING; > /* posted write need not be known to HC yet ... */ > } > } > @@ -1168,14 +1167,13 @@ static void end_unlink_async (struct ehc > > qh_completions (ehci, qh); > > - if (!list_empty (&qh->qtd_list) > - && HC_IS_RUNNING (ehci_to_hcd(ehci)->state)) > + if (!list_empty(&qh->qtd_list) && ehci->rh_state == EHCI_RH_RUNNING) > { > qh_link_async (ehci, qh); > - else { > + } else { > /* it's not free to turn the async schedule on/off; leave it > * active but idle for a while once it empties. > */ > - if (HC_IS_RUNNING (ehci_to_hcd(ehci)->state) > + if (ehci->rh_state == EHCI_RH_RUNNING > && ehci->async->qh_next.qh == NULL) > timer_action (ehci, TIMER_ASYNC_OFF); > } > @@ -1211,7 +1209,7 @@ static void start_unlink_async (struct e > /* stop async schedule right now? */ > if (unlikely (qh == ehci->async)) { > /* can't get here without STS_ASS set */ > - if (ehci_to_hcd(ehci)->state != HC_STATE_HALT > + if (ehci->rh_state != EHCI_RH_HALTED > && !ehci->reclaim) { > /* ... and CMD_IAAD clear */ > ehci_writel(ehci, cmd & ~CMD_ASE, > @@ -1237,7 +1235,7 @@ static void start_unlink_async (struct e > wmb (); > > /* If the controller isn't running, we don't have to wait for it */ > - if (unlikely(!HC_IS_RUNNING(ehci_to_hcd(ehci)->state))) { > + if (unlikely(ehci->rh_state != EHCI_RH_RUNNING)) { > /* if (unlikely (qh->reclaim != 0)) > * this will recurse, probably not much > */ > @@ -1260,7 +1258,7 @@ static void scan_async (struct ehci_hcd > enum ehci_timer_action action = TIMER_IO_WATCHDOG; > > timer_action_done (ehci, TIMER_ASYNC_SHRINK); > - stopped = !HC_IS_RUNNING(ehci_to_hcd(ehci)->state); > + stopped = (ehci->rh_state != EHCI_RH_RUNNING); > > ehci->qh_scan_next = ehci->async->qh_next.qh; > while (ehci->qh_scan_next) { > Index: usb-3.0/drivers/usb/host/ehci-sched.c > =================================================================== > --- usb-3.0.orig/drivers/usb/host/ehci-sched.c > +++ usb-3.0/drivers/usb/host/ehci-sched.c > @@ -479,7 +479,6 @@ static int enable_periodic (struct ehci_ > cmd = ehci_readl(ehci, &ehci->regs->command) | CMD_PSE; > ehci_writel(ehci, cmd, &ehci->regs->command); > /* posted write ... PSS happens later */ > - ehci_to_hcd(ehci)->state = HC_STATE_RUNNING; > > /* make sure ehci_work scans these */ > ehci->next_uframe = ehci_readl(ehci, &ehci->regs->frame_index) > @@ -677,7 +676,7 @@ static void intr_deschedule (struct ehci > > /* reschedule QH iff another request is queued */ > if (!list_empty(&qh->qtd_list) && > - HC_IS_RUNNING(ehci_to_hcd(ehci)->state)) { > + ehci->rh_state == EHCI_RH_RUNNING) { > rc = qh_schedule(ehci, qh); > > /* An error here likely indicates handshake failure > @@ -2275,7 +2274,7 @@ scan_periodic (struct ehci_hcd *ehci) > * Touches as few pages as possible: cache-friendly. > */ > now_uframe = ehci->next_uframe; > - if (HC_IS_RUNNING(ehci_to_hcd(ehci)->state)) { > + if (ehci->rh_state == EHCI_RH_RUNNING) { > clock = ehci_readl(ehci, &ehci->regs->frame_index); > clock_frame = (clock >> 3) & (ehci->periodic_size - 1); > } else { > @@ -2310,7 +2309,7 @@ restart: > union ehci_shadow temp; > int live; > > - live = HC_IS_RUNNING (ehci_to_hcd(ehci)->state); > + live = (ehci->rh_state == EHCI_RH_RUNNING); > switch (hc32_to_cpu(ehci, type)) { > case Q_TYPE_QH: > /* handle any completions */ > @@ -2435,7 +2434,7 @@ restart: > * We can't advance our scan without collecting the ISO > * transfers that are still pending in this frame. > */ > - if (incomplete && HC_IS_RUNNING(ehci_to_hcd(ehci)->state)) { > + if (incomplete && ehci->rh_state == EHCI_RH_RUNNING) { > ehci->next_uframe = now_uframe; > break; > } > @@ -2451,7 +2450,7 @@ restart: > if (now_uframe == clock) { > unsigned now; > > - if (!HC_IS_RUNNING (ehci_to_hcd(ehci)->state) > + if (ehci->rh_state != EHCI_RH_RUNNING > || ehci->periodic_sched == 0) > break; > ehci->next_uframe = now_uframe; > Index: usb-3.0/drivers/usb/host/ehci-pci.c > =================================================================== > --- usb-3.0.orig/drivers/usb/host/ehci-pci.c > +++ usb-3.0/drivers/usb/host/ehci-pci.c > @@ -439,7 +439,7 @@ static int ehci_pci_resume(struct usb_hc > /* here we "know" root ports should always stay powered */ > ehci_port_power(ehci, 1); > > - hcd->state = HC_STATE_SUSPENDED; > + ehci->rh_state = EHCI_RH_SUSPENDED; > return 0; > } > #endif > Index: usb-3.0/drivers/usb/host/ehci-au1xxx.c > =================================================================== > --- usb-3.0.orig/drivers/usb/host/ehci-au1xxx.c > +++ usb-3.0/drivers/usb/host/ehci-au1xxx.c > @@ -293,7 +293,7 @@ static int ehci_hcd_au1xxx_drv_resume(st > /* here we "know" root ports should always stay powered */ > ehci_port_power(ehci, 1); > > - hcd->state = HC_STATE_SUSPENDED; > + ehci->rh_state = EHCI_RH_SUSPENDED; > > return 0; > } > Index: usb-3.0/drivers/usb/host/ehci-fsl.c > =================================================================== > --- usb-3.0.orig/drivers/usb/host/ehci-fsl.c > +++ usb-3.0/drivers/usb/host/ehci-fsl.c > @@ -392,7 +392,7 @@ static int ehci_fsl_mpc512x_drv_suspend( > > dev_dbg(dev, "suspending...\n"); > > - hcd->state = HC_STATE_SUSPENDED; > + ehci->rh_state = EHCI_RH_SUSPENDED; > dev->power.power_state = PMSG_SUSPEND; > > /* ignore non-host interrupts */ > @@ -481,7 +481,7 @@ static int ehci_fsl_mpc512x_drv_resume(s > ehci_writel(ehci, pdata->pm_portsc, &ehci->regs->port_status[0]); > > set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags); > - hcd->state = HC_STATE_RUNNING; > + ehci->rh_state = EHCI_RH_RUNNING; > dev->power.power_state = PMSG_ON; > > tmp = ehci_readl(ehci, &ehci->regs->command); > Index: usb-3.0/drivers/usb/host/ehci-s5p.c > =================================================================== > --- usb-3.0.orig/drivers/usb/host/ehci-s5p.c > +++ usb-3.0/drivers/usb/host/ehci-s5p.c > @@ -269,7 +269,7 @@ static int s5p_ehci_resume(struct device > /* here we "know" root ports should always stay powered */ > ehci_port_power(ehci, 1); > > - hcd->state = HC_STATE_SUSPENDED; > + ehci->rh_state = EHCI_RH_SUSPENDED; > > return 0; > } > > -- ÿ淸º{.nÇ+돴윯돪†+%듚ÿ깁負¥Šwÿº{.nÇ+돴¥Š{깸ëþ)í끾èw* jgП¨ ¶‰šŽ듶¢jÿ¾?G«앶ÿ◀ ◁¦j:+v돣ŠwèjØm¶Ÿÿ ? ®w?듺þf＂·hš뤴얎ÿ녪¥
