| 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
| ||
|
Message-ID: <CA+V-a8sS5m7jtKj6S3BaPN0f43Q4wCZ8vgMEY+A204HuzhqYgg@mail.gmail.com>
Date: Tue, 25 Feb 2025 21:09:30 +0000
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Claudiu <claudiu.beznea@...on.dev>
Cc: yoshihiro.shimoda.uh@...esas.com, vkoul@...nel.org, kishon@...nel.org,
horms+renesas@...ge.net.au, fabrizio.castro.jz@...esas.com,
linux-renesas-soc@...r.kernel.org, linux-phy@...ts.infradead.org,
linux-kernel@...r.kernel.org,
Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH v2 2/5] phy: renesas: rcar-gen3-usb2: Move IRQ request in probe
Hi Claudiu,
Thank you for the patch.
On Tue, Feb 25, 2025 at 11:00 AM Claudiu <claudiu.beznea@...on.dev> wrote:
>
> 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
> 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.
>
> Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
> Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
> ---
>
> Changes in v2:
> - collected tags
>
> 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);
You may want to propagate the error and fail in case of errors like below:
if (irq < 0 && irq != -ENXIO) {
ret = irq;
goto error;
}
Rest lgtm,
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Cheers,
Prabhakar
> + 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
>
>
Powered by blists - more mailing lists