| Message ID | 20230526143615.372338-1-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | usb: gadget: udc: renesas_usb3: Fix RZ/V2M {modprobe,bind} error | expand |
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 ? > Fixes: 9cad72dfc556 ("usb: gadget: Add support for RZ/V2M USB3DRD driver" There's a missing ')' at the end of the line. > 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 ? > if (ret < 0)
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
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)
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.
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.
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
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.
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.
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
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
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.
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
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
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
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
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); if (ret < 0)
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. Fixes: 9cad72dfc556 ("usb: gadget: Add support for RZ/V2M USB3DRD driver" 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(-)