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

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

[Biju Das]

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??

> 
> > 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

Cheers,
Biju

> 
> >  		if (ret < 0)
> 
> --
> Regards,
> 
> Laurent Pinchart

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

[Laurent Pinchart]

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

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

[Marc Zyngier]

On Mon, 29 May 2023 09:56:56 +0100,
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> 
> 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.

Well, it registers it itself, but pretending to be another device.
Which is wrong on many levels.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

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

[Biju Das]

Hi Laurent and Marc,

Thanks for your feedback.

> Subject: Re: [PATCH] usb: gadget: udc: renesas_usb3: Fix RZ/V2M
> {modprobe,bind} error
> 
> On Mon, 29 May 2023 09:56:56 +0100,
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> >
> > 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.
> 
> Well, it registers it itself, but pretending to be another device.
> Which is wrong on many levels.

OK, Will register the handler in usb3drd driver and expose register/unregister handler API
with cb function to avoid duplication of the code with renesas_usb3 driver.

Basically, there will be 2 exported functions + 1 spinlock introduced in usb3drd driver

USB3DRD driver register the IRQ handler

Probe/remove from renesas_usb3 driver register/unregister the cb function. Whenever there is drd irq
cb function will be called.

I will send next version with these changes.

Cheers,
Biju

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

[Geert Uytterhoeven]

Hi Biju,

On Tue, May 30, 2023 at 2:37 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH] usb: gadget: udc: renesas_usb3: Fix RZ/V2M
> > {modprobe,bind} error
> >
> > On Mon, 29 May 2023 09:56:56 +0100,
> > Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> > >
> > > 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.
> >
> > Well, it registers it itself, but pretending to be another device.
> > Which is wrong on many levels.
>
> OK, Will register the handler in usb3drd driver and expose register/unregister handler API
> with cb function to avoid duplication of the code with renesas_usb3 driver.
>
> Basically, there will be 2 exported functions + 1 spinlock introduced in usb3drd driver
>
> USB3DRD driver register the IRQ handler
>
> Probe/remove from renesas_usb3 driver register/unregister the cb function. Whenever there is drd irq
> cb function will be called.

Please don't make it more complicated: if the parent device does not
use that interrupt, there is no need to move its handling to the parent
device driver.

Your patch looks fine to me, just replace the second ddata->dev, too.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Biju Das]

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Tuesday, May 30, 2023 2:17 PM
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: Marc Zyngier <maz@kernel.org>; Laurent Pinchart
> <laurent.pinchart@ideasonboard.com>; linux-kernel@vger.kernel.org; Greg
> Kroah-Hartman <gregkh@linuxfoundation.org>; Zheng Wang
> <zyytlz.wz@163.com>; Geert Uytterhoeven <geert+renesas@glider.be>;
> Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>; Wolfram Sang
> <wsa+renesas@sang-engineering.com>; Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org>; linux-usb@vger.kernel.org; Prabhakar
> Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>; linux-renesas-
> soc@vger.kernel.org
> Subject: Re: [PATCH] usb: gadget: udc: renesas_usb3: Fix RZ/V2M
> {modprobe,bind} error
> 
> Hi Biju,
> 
> On Tue, May 30, 2023 at 2:37 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > Subject: Re: [PATCH] usb: gadget: udc: renesas_usb3: Fix RZ/V2M
> > > {modprobe,bind} error
> > >
> > > On Mon, 29 May 2023 09:56:56 +0100,
> > > Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> > > >
> > > > 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.
> > >
> > > Well, it registers it itself, but pretending to be another device.
> > > Which is wrong on many levels.
> >
> > OK, Will register the handler in usb3drd driver and expose
> > register/unregister handler API with cb function to avoid duplication
> of the code with renesas_usb3 driver.
> >
> > Basically, there will be 2 exported functions + 1 spinlock introduced
> > in usb3drd driver
> >
> > USB3DRD driver register the IRQ handler
> >
> > Probe/remove from renesas_usb3 driver register/unregister the cb
> > function. Whenever there is drd irq cb function will be called.
> 
> Please don't make it more complicated: if the parent device does not use
> that interrupt, there is no need to move its handling to the parent
> device driver.
> 
> Your patch looks fine to me, just replace the second ddata->dev, too.

Agreed, will replace "dev_name(ddata->dev)"->dev_name(&pdev->dev).

With Interrupt ID "274" vs "278", we can differentiate "drd" vs "peri" interrupt.

root@rzv2m:~# cat /proc/interrupts | grep usb
 22:          0     GICv2 274 Level     85070000.usb3peri
 28:          0     GICv2 278 Level     85070000.usb3peri

Cheers,
Biju

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

[Marc Zyngier]

On Mon, 29 May 2023 09:42:34 +0100,
Biju Das <biju.das.jz@bp.renesas.com> 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, and the interrupt is not configured with the
SHARED flag. 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.

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.

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.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

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

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

[Laurent Pinchart]

On Mon, May 29, 2023 at 09:39:50AM +0000, Biju Das wrote:
> > 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 wrote:
> > >
> > > > Subject: Re: [PATCH] usb: gadget: udc: renesas_usb3: Fix RZ/V2M
> > > > {modprobe,bind} error
> > > >
> > > > 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.

It uses devm_request_irq() with the parent device, so the interrupt
handler won't be unregistered when the usb3-peri device is unbound. The
next probe will register the same interrupt handler a second time. This
has nothing to do with genirq, it's related to devm_*.

> > 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??

No you shouldn't pass the SHARED flag. This patch is a step in the right
direction, but the proper fix would be to register the interrupt handler
in the usb3drd driver.

-- 
Regards,

Laurent Pinchart

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

[Biju Das]

Hi Laurent,

Thanks for the feedback.

> Subject: Re: [PATCH] usb: gadget: udc: renesas_usb3: Fix RZ/V2M
> {modprobe,bind} error
> 
> On Mon, May 29, 2023 at 09:39:50AM +0000, Biju Das wrote:
> > > 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 wrote:
> > > >
> > > > > Subject: Re: [PATCH] usb: gadget: udc: renesas_usb3: Fix RZ/V2M
> > > > > {modprobe,bind} error
> > > > >
> > > > > 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.
> 
> It uses devm_request_irq() with the parent device, so the interrupt
> handler won't be unregistered when the usb3-peri device is unbound. The
> next probe will register the same interrupt handler a second time. This
> has nothing to do with genirq, it's related to devm_*.

I agree.

> 
> > > 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??
> 
> No you shouldn't pass the SHARED flag. This patch is a step in the right
> direction, but the proper fix would be to register the interrupt handler
> in the usb3drd driver.

I believe, if we register the interrupt handler in the usb3drd driver, we need to
duplicate the code to avoid cross dependencies.

Currently USB3drd driver expose reset API for both host/peri.

If we want to reuse the code, means we need to export a call from peri driver
this will result in cross dependency.

That is the reason for reusing the code, this handler is managed by the
peri driver.

Cheers,
Biju

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

[Marc Zyngier]

On Mon, 29 May 2023 10:39:50 +0100,
Biju Das <biju.das.jz@bp.renesas.com> wrote:
> 
> 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@bp.renesas.com> 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 you're doing that *TWICE*. Once per load of this driver.

>
>  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.

Do you understand the meaning of the "dev" parameter you pass to
devm_request_irq()?

	M.

-- 
Without deviation from the norm, progress is not possible.

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 10:39:50 +0100,
> Biju Das <biju.das.jz@bp.renesas.com> wrote:
> >
> > 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@bp.renesas.com> 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 you're doing that *TWICE*. Once per load of this driver.

I got it from Laurent's feedback.

> 
> >
> >  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.
> 
> Do you understand the meaning of the "dev" parameter you pass to
> devm_request_irq()?

Yes, the resource is managed with particular device.

I should not use devm_request_irq here. rather should use
request_irq and free_irq during unload with parent device handle.

Cheers,
Biju

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

[Marc Zyngier]

On Mon, 29 May 2023 11:03:27 +0100,
Biju Das <biju.das.jz@bp.renesas.com> wrote:
> 
> > Do you understand the meaning of the "dev" parameter you pass to
> > devm_request_irq()?
> 
> Yes, the resource is managed with particular device.

So what does it tell you about the life cycle of the interrupt you
request with the *wrong* device?

> I should not use devm_request_irq here. rather should use
> request_irq and free_irq during unload with parent device handle.

No, that's just papering over the real issue. You should just get the
driver that handles the interrupt to request it. Anything else is a
design bug.

	M.

-- 
Without deviation from the norm, progress is not possible.

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

[Biju Das]

Hi Marc,

> Subject: Re: [PATCH] usb: gadget: udc: renesas_usb3: Fix RZ/V2M
> {modprobe,bind} error
> 
> On Mon, 29 May 2023 11:03:27 +0100,
> Biju Das <biju.das.jz@bp.renesas.com> wrote:
> >
> > > Do you understand the meaning of the "dev" parameter you pass to
> > > devm_request_irq()?
> >
> > Yes, the resource is managed with particular device.
> 
> So what does it tell you about the life cycle of the interrupt you
> request with the *wrong* device?

It is not *wrong* device, it just parent device, which is telling
its child to manage it.

I agree, using parent device is wrong here.

> 
> > I should not use devm_request_irq here. rather should use request_irq
> > and free_irq during unload with parent device handle.
> 
> No, that's just papering over the real issue. You should just get the
> driver that handles the interrupt to request it. Anything else is a
> design bug.

Got it, but I guess, by using devm_request_irq() with child device is not a design bug

In a real life situation, for eg: A parent owns a property, 

Parent is thinking it is efficient to handle the property by its child
rather than him. So it can tell its child to manage the property. 
The parent share the property details and child manages it efficiently.

Cheers,
Biju