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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230529085656.GL25984@pendragon.ideasonboard.com>
Date:   Mon, 29 May 2023 11:56:56 +0300
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Biju Das <biju.das.jz@...renesas.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 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@...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@...2m:~# 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