From: Claudiu Beznea <claudiu.beznea.uj@bp.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@vger.kernel.org
Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
Changes in v2:
- collected tags
drivers/phy/renesas/phy-rcar-gen3-usb2.c | 49 +++++++++++++-----------
1 file changed, 26 insertions(+), 23 deletions(-)
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.0On Tue, Feb 25, 2025 at 11:01 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>
> From: Claudiu Beznea <claudiu.beznea.uj@bp.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@vger.kernel.org
> Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.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@bp.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
>
>
© 2016 - 2026 Red Hat, Inc.