| 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-a8v4ZyKE+FmscDTppzqoj+qurBP=NVQqyjLZxf_RDJh96Q@mail.gmail.com>
Date: Tue, 25 Feb 2025 21:12:42 +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>, stable@...r.kernel.org
Subject: Re: [PATCH v2 3/5] phy: renesas: rcar-gen3-usb2: Lock around hardware
registers and driver data
On Tue, Feb 25, 2025 at 11:01 AM Claudiu <claudiu.beznea@...on.dev> wrote:
>
> From: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
>
> The phy-rcar-gen3-usb2 driver exposes four individual PHYs that are
> requested and configured by PHY users. The struct phy_ops APIs access the
> same set of registers to configure all PHYs. Additionally, PHY settings can
> be modified through sysfs or an IRQ handler. While some struct phy_ops APIs
> are protected by a driver-wide mutex, others rely on individual
> PHY-specific mutexes.
>
> This approach can lead to various issues, including:
> 1/ the IRQ handler may interrupt PHY settings in progress, racing with
> hardware configuration protected by a mutex lock
> 2/ due to msleep(20) in rcar_gen3_init_otg(), while a configuration thread
> suspends to wait for the delay, another thread may try to configure
> another PHY (with phy_init() + phy_power_on()); re-running the
> phy_init() goes to the exact same configuration code, re-running the
> same hardware configuration on the same set of registers (and bits)
> which might impact the result of the msleep for the 1st configuring
> thread
> 3/ sysfs can configure the hardware (though role_store()) and it can
> still race with the phy_init()/phy_power_on() APIs calling into the
> drivers struct phy_ops
>
> To address these issues, add a spinlock to protect hardware register access
> and driver private data structures (e.g., calls to
> rcar_gen3_is_any_rphy_initialized()). Checking driver-specific data remains
> necessary as all PHY instances share common settings. With this change,
> the existing mutex protection is removed and the cleanup.h helpers are
> used.
>
> While at it, to keep the code simpler, do not skip
> regulator_enable()/regulator_disable() APIs in
> rcar_gen3_phy_usb2_power_on()/rcar_gen3_phy_usb2_power_off() as the
> regulators enable/disable operations are reference counted anyway.
>
> Fixes: f3b5a8d9b50d ("phy: rcar-gen3-usb2: Add R-Car Gen3 USB2 PHY driver")
> Cc: stable@...r.kernel.org
> 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 | 49 +++++++++++++-----------
> 1 file changed, 26 insertions(+), 23 deletions(-)
>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Cheers,
Prabhakar
> diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> index 826c9c4dd4c0..5c0ceba09b67 100644
> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> @@ -9,6 +9,7 @@
> * Copyright (C) 2014 Cogent Embedded, Inc.
> */
>
> +#include <linux/cleanup.h>
> #include <linux/extcon-provider.h>
> #include <linux/interrupt.h>
> #include <linux/io.h>
> @@ -118,7 +119,7 @@ struct rcar_gen3_chan {
> struct regulator *vbus;
> struct reset_control *rstc;
> struct work_struct work;
> - struct mutex lock; /* protects rphys[...].powered */
> + spinlock_t lock; /* protects access to hardware and driver data structure. */
> enum usb_dr_mode dr_mode;
> u32 obint_enable_bits;
> bool extcon_host;
> @@ -348,6 +349,8 @@ static ssize_t role_store(struct device *dev, struct device_attribute *attr,
> bool is_b_device;
> enum phy_mode cur_mode, new_mode;
>
> + guard(spinlock_irqsave)(&ch->lock);
> +
> if (!ch->is_otg_channel || !rcar_gen3_is_any_otg_rphy_initialized(ch))
> return -EIO;
>
> @@ -415,7 +418,7 @@ static void rcar_gen3_init_otg(struct rcar_gen3_chan *ch)
> val = readl(usb2_base + USB2_ADPCTRL);
> writel(val | USB2_ADPCTRL_IDPULLUP, usb2_base + USB2_ADPCTRL);
> }
> - msleep(20);
> + mdelay(20);
>
> writel(0xffffffff, usb2_base + USB2_OBINTSTA);
> writel(ch->obint_enable_bits, usb2_base + USB2_OBINTEN);
> @@ -436,12 +439,14 @@ static irqreturn_t rcar_gen3_phy_usb2_irq(int irq, void *_ch)
> if (pm_runtime_suspended(dev))
> goto rpm_put;
>
> - status = readl(usb2_base + USB2_OBINTSTA);
> - if (status & ch->obint_enable_bits) {
> - 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;
> + scoped_guard(spinlock, &ch->lock) {
> + status = readl(usb2_base + USB2_OBINTSTA);
> + if (status & ch->obint_enable_bits) {
> + 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:
> @@ -456,6 +461,8 @@ static int rcar_gen3_phy_usb2_init(struct phy *p)
> void __iomem *usb2_base = channel->base;
> u32 val;
>
> + guard(spinlock_irqsave)(&channel->lock);
> +
> /* Initialize USB2 part */
> val = readl(usb2_base + USB2_INT_ENABLE);
> val |= USB2_INT_ENABLE_UCOM_INTEN | rphy->int_enable_bits;
> @@ -479,6 +486,8 @@ static int rcar_gen3_phy_usb2_exit(struct phy *p)
> void __iomem *usb2_base = channel->base;
> u32 val;
>
> + guard(spinlock_irqsave)(&channel->lock);
> +
> rphy->initialized = false;
>
> val = readl(usb2_base + USB2_INT_ENABLE);
> @@ -498,16 +507,17 @@ static int rcar_gen3_phy_usb2_power_on(struct phy *p)
> u32 val;
> int ret = 0;
>
> - mutex_lock(&channel->lock);
> - if (!rcar_gen3_are_all_rphys_power_off(channel))
> - goto out;
> -
> if (channel->vbus) {
> ret = regulator_enable(channel->vbus);
> if (ret)
> - goto out;
> + return ret;
> }
>
> + guard(spinlock_irqsave)(&channel->lock);
> +
> + if (!rcar_gen3_are_all_rphys_power_off(channel))
> + goto out;
> +
> val = readl(usb2_base + USB2_USBCTR);
> val |= USB2_USBCTR_PLL_RST;
> writel(val, usb2_base + USB2_USBCTR);
> @@ -517,7 +527,6 @@ static int rcar_gen3_phy_usb2_power_on(struct phy *p)
> out:
> /* The powered flag should be set for any other phys anyway */
> rphy->powered = true;
> - mutex_unlock(&channel->lock);
>
> return 0;
> }
> @@ -528,18 +537,12 @@ static int rcar_gen3_phy_usb2_power_off(struct phy *p)
> struct rcar_gen3_chan *channel = rphy->ch;
> int ret = 0;
>
> - mutex_lock(&channel->lock);
> - rphy->powered = false;
> -
> - if (!rcar_gen3_are_all_rphys_power_off(channel))
> - goto out;
> + scoped_guard(spinlock_irqsave, &channel->lock)
> + rphy->powered = false;
>
> if (channel->vbus)
> ret = regulator_disable(channel->vbus);
>
> -out:
> - mutex_unlock(&channel->lock);
> -
> return ret;
> }
>
> @@ -750,7 +753,7 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
> if (phy_data->no_adp_ctrl)
> channel->obint_enable_bits = USB2_OBINT_IDCHG_EN;
>
> - mutex_init(&channel->lock);
> + spin_lock_init(&channel->lock);
> for (i = 0; i < NUM_OF_PHYS; i++) {
> channel->rphys[i].phy = devm_phy_create(dev, NULL,
> phy_data->phy_usb2_ops);
> --
> 2.43.0
>
>
Powered by blists - more mailing lists