[PATCH] phy: renesas: rcar-gen3-usb2: fix SError happen if DEBUG_SHIRQ is enabled

[PATCH] phy: renesas: rcar-gen3-usb2: fix SError happen if DEBUG_SHIRQ is enabled

[Yoshihiro Shimoda]

If CONFIG_DEBUG_SHIRQ was enabled, r8a77951-salvator-xs could boot
correctly. If we appended "earlycon keep_bootcon" to the kernel
command like, we could get kernel log like below.

    SError Interrupt on CPU0, code 0xbf000002 -- SError
    CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.8.0-rc3-salvator-x-00505-g6c843129e6faaf01 #785
    Hardware name: Renesas Salvator-X 2nd version board based on r8a77951 (DT)
    pstate: 60400085 (nZCv daIf +PAN -UAO BTYPE=--)
    pc : rcar_gen3_phy_usb2_irq+0x14/0x54
    lr : free_irq+0xf4/0x27c

This means free_irq() calls the interrupt handler while PM runtime
is not getting if DEBUG_SHIRQ is enabled and rcar_gen3_phy_usb2_probe()
failed. To fix the issue, add a condition into the interrupt
handler to avoid register access if any phys are not initialized.

Note that rcar_gen3_is_any_rphy_initialized() was introduced on v5.2.
So, if we backports this patch to v5.1 or less, we need to make
other way.

Reported-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
Fixes: 9f391c574efc ("phy: rcar-gen3-usb2: add runtime ID/VBUS pin detection")
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/phy/renesas/phy-rcar-gen3-usb2.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
index bfb22f8..91c732d 100644
--- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
+++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
@@ -507,9 +507,13 @@ static irqreturn_t rcar_gen3_phy_usb2_irq(int irq, void *_ch)
 {
 	struct rcar_gen3_chan *ch = _ch;
 	void __iomem *usb2_base = ch->base;
-	u32 status = readl(usb2_base + USB2_OBINTSTA);
+	u32 status;
 	irqreturn_t ret = IRQ_NONE;
 
+	if (!rcar_gen3_is_any_rphy_initialized(ch))
+		return ret;
+
+	status = readl(usb2_base + USB2_OBINTSTA);
 	if (status & USB2_OBINT_BITS) {
 		dev_vdbg(ch->dev, "%s: %08x\n", __func__, status);
 		writel(USB2_OBINT_BITS, usb2_base + USB2_OBINTSTA);
-- 
2.7.4

Re: [PATCH] phy: renesas: rcar-gen3-usb2: fix SError happen if DEBUG_SHIRQ is enabled

[Vinod Koul]

Hi Yoshihiro,

On 09-07-20, 19:36, Yoshihiro Shimoda wrote:
> If CONFIG_DEBUG_SHIRQ was enabled, r8a77951-salvator-xs could boot
> correctly. If we appended "earlycon keep_bootcon" to the kernel
> command like, we could get kernel log like below.
> 
>     SError Interrupt on CPU0, code 0xbf000002 -- SError
>     CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.8.0-rc3-salvator-x-00505-g6c843129e6faaf01 #785
>     Hardware name: Renesas Salvator-X 2nd version board based on r8a77951 (DT)
>     pstate: 60400085 (nZCv daIf +PAN -UAO BTYPE=--)
>     pc : rcar_gen3_phy_usb2_irq+0x14/0x54
>     lr : free_irq+0xf4/0x27c
> 
> This means free_irq() calls the interrupt handler while PM runtime
> is not getting if DEBUG_SHIRQ is enabled and rcar_gen3_phy_usb2_probe()
> failed. To fix the issue, add a condition into the interrupt
> handler to avoid register access if any phys are not initialized.
> 
> Note that rcar_gen3_is_any_rphy_initialized() was introduced on v5.2.
> So, if we backports this patch to v5.1 or less, we need to make
> other way.

Should we really check phy is initialized? I think the issue here is
that you register irq first, so your handler can be invoked. Right fix
for this would be to move the irq registration to later in the probe
when we are ready to handle interrupts

> 
> Reported-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Fixes: 9f391c574efc ("phy: rcar-gen3-usb2: add runtime ID/VBUS pin detection")
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/phy/renesas/phy-rcar-gen3-usb2.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> index bfb22f8..91c732d 100644
> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> @@ -507,9 +507,13 @@ static irqreturn_t rcar_gen3_phy_usb2_irq(int irq, void *_ch)
>  {
>  	struct rcar_gen3_chan *ch = _ch;
>  	void __iomem *usb2_base = ch->base;
> -	u32 status = readl(usb2_base + USB2_OBINTSTA);
> +	u32 status;
>  	irqreturn_t ret = IRQ_NONE;
>  
> +	if (!rcar_gen3_is_any_rphy_initialized(ch))
> +		return ret;
> +
> +	status = readl(usb2_base + USB2_OBINTSTA);
>  	if (status & USB2_OBINT_BITS) {
>  		dev_vdbg(ch->dev, "%s: %08x\n", __func__, status);
>  		writel(USB2_OBINT_BITS, usb2_base + USB2_OBINTSTA);
> -- 
> 2.7.4

-- 
~Vinod

RE: [PATCH] phy: renesas: rcar-gen3-usb2: fix SError happen if DEBUG_SHIRQ is enabled

[Yoshihiro Shimoda]

Hi Vinod,

> From: Vinod Koul, Sent: Monday, July 13, 2020 2:17 PM
> 
> Hi Yoshihiro,
> 
> On 09-07-20, 19:36, Yoshihiro Shimoda wrote:
> > If CONFIG_DEBUG_SHIRQ was enabled, r8a77951-salvator-xs could boot
> > correctly. If we appended "earlycon keep_bootcon" to the kernel
> > command like, we could get kernel log like below.
> >
> >     SError Interrupt on CPU0, code 0xbf000002 -- SError
> >     CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.8.0-rc3-salvator-x-00505-g6c843129e6faaf01 #785
> >     Hardware name: Renesas Salvator-X 2nd version board based on r8a77951 (DT)
> >     pstate: 60400085 (nZCv daIf +PAN -UAO BTYPE=--)
> >     pc : rcar_gen3_phy_usb2_irq+0x14/0x54
> >     lr : free_irq+0xf4/0x27c
> >
> > This means free_irq() calls the interrupt handler while PM runtime
> > is not getting if DEBUG_SHIRQ is enabled and rcar_gen3_phy_usb2_probe()
> > failed. To fix the issue, add a condition into the interrupt
> > handler to avoid register access if any phys are not initialized.
> >
> > Note that rcar_gen3_is_any_rphy_initialized() was introduced on v5.2.
> > So, if we backports this patch to v5.1 or less, we need to make
> > other way.
> 
> Should we really check phy is initialized? I think the issue here is
> that you register irq first, so your handler can be invoked. Right fix
> for this would be to move the irq registration to later in the probe
> when we are ready to handle interrupts

Thank you for your review! I think we need to check whether phy is initialized
even if the irq registration moves to rcar_gen3_phy_usb2_init() because the current
driver will have multiple phy instances. However, moving the irq registration can be
easy to backport than this patch, I think. So, I'll modify this patch as you suggested.

Best regards,
Yoshihiro Shiomda