Message-ID: <OS0PR01MB5922B52C4B2E5A55B6E4B6DD864A9@OS0PR01MB5922.jpnprd01.prod.outlook.com>
Date:   Mon, 29 May 2023 09:59:23 +0000
From:   Biju Das <biju.das.jz@...renesas.com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
CC:     Marc Zyngier <maz@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Zheng Wang <zyytlz.wz@....com>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
        Wolfram Sang <wsa+renesas@...g-engineering.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@...renesas.com>,
        "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>
Subject: RE: [PATCH] usb: gadget: udc: renesas_usb3: Fix RZ/V2M
 {modprobe,bind} error

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@...2m:~# cat /proc/interrupts | grep usb
> >  22:          0     GICv2 274 Level     85070400.usb3drd
> >  28:          0     GICv2 278 Level     85070000.usb3peri
> > root@...2m:~# 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@...2m:~# rmmod hd3ss3220
> > root@...2m:~# rmmod renesas_usb3
> > root@...2m:~# cat /proc/interrupts | grep usb
> >  22:          0     GICv2 274 Level     85070400.usb3drd
> > root@...2m:~#
> >
> > With devm_request_irq(&pdev->dev..), after unload
> > ================================================
> >
> > root@...2m:~# cat /proc/interrupts | grep usb
> >  22:          0     GICv2 274 Level     85070400.usb3drd
> >  28:          0     GICv2 278 Level     85070000.usb3peri
> > root@...2m:~# 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@...2m:~# rmmod hd3ss3220
> > root@...2m:~# rmmod renesas_usb3
> > root@...2m:~# cat /proc/interrupts | grep usb root@...2m:~#
> >
> > > 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

Powered by blists - more mailing lists