Re: [patch v4 0/4] Cypress c67x00 (EZ-Host/EZ-OTG) support

["Grant Likely" <grant.likely@secretlab.ca>]

On 1/23/08, David Brownell <david-b@pacbell.net> wrote:
> On Wednesday 23 January 2008, Grant Likely wrote:
> > The question is about the device structure which used to be provided
> > by the platform device instances and now there just uses the c67x00's
> > device struct.  I was under the impression that each USB HCD needs to
> > have it's own struct device.  I take it that's not true?
>
> Each root hub necessarily is a unique device, representing a set
> of downstream links.  Unless Peter didn't test something relevant,
> it would seem we have observational proof that two root hubs can
> share the same device node for an upstream link.
>
> I can't think of a reason to demand multiple upstream links, though
> sharing them between root hubs like that isn't a common structure.

I've now merged the driver into my tree and tested it.

The good news is that using only one device node is absolutely no
problem.  It works without error or complaint and sysfs looks sane.

The bad news is that I discovered a race condition on initialization.
The problem is that IRQs are enabled before the SIEs are configured.
The interrupt handler is firing immediately and tries to get the
status of each SIE.  Since the SIE probe has not yet occured, sie->dev
is NULL and we get a kernel oops when c67x00_ll_get_siemsg() tries to
dereference it.

However, register_irq() cannot be easily moved to after the SIE probe
because the current probe code depends on interrupts being enabled.
Below is an ugly workaround that solves the problem to me, but there
is probably a better solution.

Cheers,
g.


diff --git a/drivers/usb/c67x00/c67x00-drv.c
b/drivers/usb/c67x00/c67x00-drv.c index 360bae5..4051502 100644
--- a/drivers/usb/c67x00/c67x00-drv.c
+++ b/drivers/usb/c67x00/c67x00-drv.c
@@ -105,6 +105,8 @@ static irqreturn_t c67x00_irq(int irq, void *__dev)
                        }
                        if (sie->irq)
                                sie->irq(sie, int_status, msg);
+                       else if (int_status & SOFEOP_FLG(sie->sie_num))
+                               c67x00_ll_usb_clear_status(sie,
SOF_EOP_IRQ_FLG);
                }
                int_status = c67x00_ll_hpi_status(c67x00);
        }
@@ -168,6 +170,11 @@ static int __devinit c67x00_drv_probe(struct
platform_device *pdev)
                goto request_irq_failed;
        }

+       for (i = 0; i < C67X00_SIES; i++) {
+               c67x00->sie[i].sie_num = i;
+               c67x00->sie[i].dev = c67x00;
+       }
+
        ret = c67x00_ll_reset(c67x00);
        if (ret) {
                dev_err(&pdev->dev, "Device reset failed\n");

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.