Hi Biju,
On Mon, May 29, 2023 at 08:42:34AM +0000, Biju Das wrote:
> > Subject: Re: [PATCH] usb: gadget: udc: renesas_usb3: Fix RZ/V2M
> > {modprobe,bind} error
> >
> > Hi Biju,
> >
> > Thank you for the patch.
> >
> > On Fri, May 26, 2023 at 03:36:15PM +0100, Biju Das wrote:
> > > Currently {modprobe, bind} after {rmmod, unbind} results in probe
> > failure.
> > >
> > > genirq: Flags mismatch irq 22. 00000004 (85070400.usb3drd) vs.
> > > 00000004 (85070400.usb3drd)
> > > renesas_usb3: probe of 85070000.usb3peri failed with error -16
> > >
> > > Fix this issue by replacing "parent dev"->"dev" as the irq resource is
> > > managed by this driver.
> >
> > If the dev pointer passed to devm_request_irq() is not the correct one,
> > how does it work the first time the driver is loaded ?
>
> + Marc/ Kernel.org to give some feedback on this issue
>
> I believe there may be a bug in the genirq (kernel/irq) driver.
> first time it works ok. Maybe this driver is caching on unload
> with null value and comparing with actual one (irq 22) during reload??
>
> Maybe genirq expert can comment what went wrong here??
I'm curious to understand this (an update to the commit message would
then be nice), but regardless, I think the code change is fine.
> > > Fixes: 9cad72dfc556 ("usb: gadget: Add support for RZ/V2M USB3DRD
> > driver"
> >
> > There's a missing ')' at the end of the line.
>
> Oops missed it.
>
> > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > > ---
> > > drivers/usb/gadget/udc/renesas_usb3.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/gadget/udc/renesas_usb3.c
> > > b/drivers/usb/gadget/udc/renesas_usb3.c
> > > index aac8bc185afa..4a37b2e4b9b3 100644
> > > --- a/drivers/usb/gadget/udc/renesas_usb3.c
> > > +++ b/drivers/usb/gadget/udc/renesas_usb3.c
> > > @@ -2877,7 +2877,7 @@ static int renesas_usb3_probe(struct platform_device *pdev)
> > > struct rzv2m_usb3drd *ddata = dev_get_drvdata(pdev->dev.parent);
> > >
> > > usb3->drd_reg = ddata->reg;
> > > - ret = devm_request_irq(ddata->dev, ddata->drd_irq,
> > > + ret = devm_request_irq(&pdev->dev, ddata->drd_irq,
> > > renesas_usb3_otg_irq, 0,
> > > dev_name(ddata->dev), usb3);
> >
> > Shouldn't you use dev_name(&pdev->dev) too ?
>
> This irq resource belongs to usb3drd driver and is managed by renesas_usb3 driver.
> It is just representation of irqname and cat /proc/interrupts shows the correct
> irq resource name. with dev_name(ddata->dev), it displays correct resource name
> associated with the handler.
>
> root@rzv2m:~# cat /proc/interrupts | grep usb
> 22: 0 GICv2 274 Level 85070400.usb3drd
> 23: 353 GICv2 277 Level xhci-hcd:usb1
> 28: 0 GICv2 278 Level 85070000.usb3peri
The name is just informative so I suppose it's ok. It makes me wonder,
though, if the usb3drd driver shouldn't register the interrupt handler
itself.
> > > if (ret < 0)
--
Regards,
Laurent Pinchart
