Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 29 May 2023 11:56:56 +0300
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     Biju Das <biju.das.jz@bp.renesas.com>
Cc:     Marc Zyngier <maz@kernel.org>,
        "linux-kernel@vger.kernel.org" <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" <linux-usb@vger.kernel.org>,
        Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>,
        "linux-renesas-soc@vger.kernel.org" 
        <linux-renesas-soc@vger.kernel.org>
Subject: Re: [PATCH] usb: gadget: udc: renesas_usb3: Fix RZ/V2M
 {modprobe,bind} error
Message-ID: <20230529085656.GL25984@pendragon.ideasonboard.com>
References: <20230526143615.372338-1-biju.das.jz@bp.renesas.com>
 <20230529061714.GA25984@pendragon.ideasonboard.com>
 <OS0PR01MB592296756992262EC6D382D0864A9@OS0PR01MB5922.jpnprd01.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <OS0PR01MB592296756992262EC6D382D0864A9@OS0PR01MB5922.jpnprd01.prod.outlook.com>
Precedence: bulk

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