linux-kernel - RE: [PATCH RFT 2/5] phy: renesas: rcar-gen3-usb2: Move IRQ request in probe

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <TYCPR01MB11040051BFF6ECA0F45FD6038D8C72@TYCPR01MB11040.jpnprd01.prod.outlook.com>
Date: Fri, 21 Feb 2025 02:10:41 +0000
From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
To: Claudiu.Beznea <claudiu.beznea@...on.dev>, "vkoul@...nel.org"
	<vkoul@...nel.org>, "kishon@...nel.org" <kishon@...nel.org>,
	"horms+renesas@...ge.net.au" <horms+renesas@...ge.net.au>,
	"fabrizio.castro@...renesas.com" <fabrizio.castro@...renesas.com>,
	"robh@...nel.org" <robh@...nel.org>
CC: Claudiu.Beznea <claudiu.beznea@...on.dev>,
	"linux-renesas-soc@...r.kernel.org" <linux-renesas-soc@...r.kernel.org>,
	"linux-phy@...ts.infradead.org" <linux-phy@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Claudiu Beznea
	<claudiu.beznea.uj@...renesas.com>
Subject: RE: [PATCH RFT 2/5] phy: renesas: rcar-gen3-usb2: Move IRQ request in
 probe

Hello Claudiu-san,

> From: Claudiu, Sent: Thursday, February 20, 2025 1:08 AM
> 
> From: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
> 
> Commit 08b0ad375ca6 ("phy: renesas: rcar-gen3-usb2: move IRQ registration
> to init") moved the IRQ request operation from probe to

I don't know why the checkpatch.pl said the following ERROR though,
as I sent my Reviewed-by on the top of this email thread, this patch is good,
I think.
---
ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'Commit 08b0ad375ca6 ("phy: renesas: rcar-gen3-usb2: move irq registration to init")'
#6:
Commit 08b0ad375ca6 ("phy: renesas: rcar-gen3-usb2: move IRQ registration
to init") moved the IRQ request operation from probe to
---

Best regards,
Yoshihiro Shimoda

> struct phy_ops::phy_init API to avoid triggering interrupts (which lead to
> register accesses) while the PHY clocks (enabled through runtime PM APIs)
> are not active. If this happens, it results in a synchronous abort.
> 
> One way to reproduce this issue is by enabling CONFIG_DEBUG_SHIRQ, which
> calls free_irq() on driver removal.
> 
> Move the IRQ request and free operations back to probe, and take the
> runtime PM state into account in IRQ handler. This commit is preparatory
> for the subsequent fixes in this series.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
> ---
>  drivers/phy/renesas/phy-rcar-gen3-usb2.c | 46 +++++++++++++-----------
>  1 file changed, 26 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> index 46afba2fe0dc..826c9c4dd4c0 100644
> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> @@ -120,7 +120,6 @@ struct rcar_gen3_chan {
>  	struct work_struct work;
>  	struct mutex lock;	/* protects rphys[...].powered */
>  	enum usb_dr_mode dr_mode;
> -	int irq;
>  	u32 obint_enable_bits;
>  	bool extcon_host;
>  	bool is_otg_channel;
> @@ -428,16 +427,25 @@ 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);
> +	struct device *dev = ch->dev;
>  	irqreturn_t ret = IRQ_NONE;
> +	u32 status;
> 
> +	pm_runtime_get_noresume(dev);
> +
> +	if (pm_runtime_suspended(dev))
> +		goto rpm_put;
> +
> +	status = readl(usb2_base + USB2_OBINTSTA);
>  	if (status & ch->obint_enable_bits) {
> -		dev_vdbg(ch->dev, "%s: %08x\n", __func__, status);
> +		dev_vdbg(dev, "%s: %08x\n", __func__, status);
>  		writel(ch->obint_enable_bits, usb2_base + USB2_OBINTSTA);
>  		rcar_gen3_device_recognition(ch);
>  		ret = IRQ_HANDLED;
>  	}
> 
> +rpm_put:
> +	pm_runtime_put_noidle(dev);
>  	return ret;
>  }
> 
> @@ -447,17 +455,6 @@ static int rcar_gen3_phy_usb2_init(struct phy *p)
>  	struct rcar_gen3_chan *channel = rphy->ch;
>  	void __iomem *usb2_base = channel->base;
>  	u32 val;
> -	int ret;
> -
> -	if (!rcar_gen3_is_any_rphy_initialized(channel) && channel->irq >= 0) {
> -		INIT_WORK(&channel->work, rcar_gen3_phy_usb2_work);
> -		ret = request_irq(channel->irq, rcar_gen3_phy_usb2_irq,
> -				  IRQF_SHARED, dev_name(channel->dev), channel);
> -		if (ret < 0) {
> -			dev_err(channel->dev, "No irq handler (%d)\n", channel->irq);
> -			return ret;
> -		}
> -	}
> 
>  	/* Initialize USB2 part */
>  	val = readl(usb2_base + USB2_INT_ENABLE);
> @@ -490,9 +487,6 @@ static int rcar_gen3_phy_usb2_exit(struct phy *p)
>  		val &= ~USB2_INT_ENABLE_UCOM_INTEN;
>  	writel(val, usb2_base + USB2_INT_ENABLE);
> 
> -	if (channel->irq >= 0 && !rcar_gen3_is_any_rphy_initialized(channel))
> -		free_irq(channel->irq, channel);
> -
>  	return 0;
>  }
> 
> @@ -698,7 +692,7 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct rcar_gen3_chan *channel;
>  	struct phy_provider *provider;
> -	int ret = 0, i;
> +	int ret = 0, i, irq;
> 
>  	if (!dev->of_node) {
>  		dev_err(dev, "This driver needs device tree\n");
> @@ -714,8 +708,6 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
>  		return PTR_ERR(channel->base);
> 
>  	channel->obint_enable_bits = USB2_OBINT_BITS;
> -	/* get irq number here and request_irq for OTG in phy_init */
> -	channel->irq = platform_get_irq_optional(pdev, 0);
>  	channel->dr_mode = rcar_gen3_get_dr_mode(dev->of_node);
>  	if (channel->dr_mode != USB_DR_MODE_UNKNOWN) {
>  		channel->is_otg_channel = true;
> @@ -784,6 +776,20 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
>  		channel->vbus = NULL;
>  	}
> 
> +	irq = platform_get_irq_optional(pdev, 0);
> +	if (irq == -EPROBE_DEFER) {
> +		ret = -EPROBE_DEFER;
> +		goto error;
> +	} else if (irq >= 0) {
> +		INIT_WORK(&channel->work, rcar_gen3_phy_usb2_work);
> +		ret = devm_request_irq(dev, irq, rcar_gen3_phy_usb2_irq,
> +				       IRQF_SHARED, dev_name(dev), channel);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to request irq (%d)\n", irq);
> +			goto error;
> +		}
> +	}
> +
>  	provider = devm_of_phy_provider_register(dev, rcar_gen3_phy_usb2_xlate);
>  	if (IS_ERR(provider)) {
>  		dev_err(dev, "Failed to register PHY provider\n");
> --
> 2.43.0