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

[Biju Das]

Hi Marc Zyngier,

Thanks for the feedback.

> Subject: Re: [PATCH] usb: gadget: udc: renesas_usb3: Fix RZ/V2M
> {modprobe,bind} error
> 
> On Mon, 29 May 2023 09:42:34 +0100,
> Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> >
> > Hi Laurent,
> >
> > Thanks for the feedback.
> >
> > > 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??
> 
> You get shouted at because you are registering an interrupt handler for
> the same IRQ twice,

This not true. It is registering only one IRQ, but with parent device handle.

 and the interrupt is not configured with the SHARED
> flag.

I haven't added SHARED flag as there is only one IRQ registration.

 If, as I understand it, you only have a single device using this
> interrupt, then it means your driver is not freeing its interrupt on
> unload.

You mean devm_request_irq(ddata->dev..)  doesn't free the resource as
we have unloaded only child device rather than parent.

But while parent is active, why genirq is giving error during reload?
It should show same behaviour like initial probe.

> 
> And that's probably because the device object used when requesting the
> interrupt isn't the one you load/unload, as indicated by the message.
> On the first load of "usb3peri", you register an interrupt with
> "usb3drd" as the requester device. You then unload "usb3peri", which of
> course has no effect whatsoever on the interrupt.
> 
> You could simply have done a "cat /proc/interrupt" and see that
> interrupt was still there after unload.

Yes, interrupt still there after unload.

With devm_request_irq(ddata->dev..), after unload
=================================================

root@rzv2m:~# cat /proc/interrupts | grep usb
 22:          0     GICv2 274 Level     85070400.usb3drd
 28:          0     GICv2 278 Level     85070000.usb3peri
root@rzv2m:~# lsmod
Module                  Size  Used by
hd3ss3220              12288  0
typec                  73728  1 hd3ss3220
renesas_usb3           32768  1
i2c_rzv2m              12288  0
crct10dif_ce           12288  1
ipv6                  450560  16
root@rzv2m:~# rmmod hd3ss3220
root@rzv2m:~# rmmod renesas_usb3
root@rzv2m:~# cat /proc/interrupts | grep usb
 22:          0     GICv2 274 Level     85070400.usb3drd
root@rzv2m:~#

With devm_request_irq(&pdev->dev..), after unload
================================================

root@rzv2m:~# cat /proc/interrupts | grep usb
 22:          0     GICv2 274 Level     85070400.usb3drd
 28:          0     GICv2 278 Level     85070000.usb3peri
root@rzv2m:~# lsmod
Module                  Size  Used by
hd3ss3220              12288  0
typec                  73728  1 hd3ss3220
renesas_usb3           32768  1
crct10dif_ce           12288  1
i2c_rzv2m              12288  0
ipv6                  450560  16
root@rzv2m:~# rmmod hd3ss3220
root@rzv2m:~# rmmod renesas_usb3
root@rzv2m:~# cat /proc/interrupts | grep usb
root@rzv2m:~#

> 
> So the only bug here is in the handling of the interrupt request. And
> that bug firmly lies in your code. My "expert" advise is to debug the
> problem rather than suspecting some random failure modes.

With devm_request_irq(&pdev->dev..) the above issue is fixed.

Or

the correct way is passing SHARED flag with devm_request_irq(ddata ->dev..), 
as the resource is owned by the parent??

Cheers,
Biju