Re: [PATCH] usb: gadget: udc: renesas_usb3: Fix RZ/V2M {modprobe,bind} error

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@bp.renesas.com>
> > > ---
> > >  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