Skip to content

Commit 55a387e

Browse files
claudiubezneavinodkoul
authored andcommitted
phy: renesas: rcar-gen3-usb2: Lock around hardware registers and driver data
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: f3b5a8d ("phy: rcar-gen3-usb2: Add R-Car Gen3 USB2 PHY driver") Cc: [email protected] Reviewed-by: Yoshihiro Shimoda <[email protected]> Tested-by: Yoshihiro Shimoda <[email protected]> Reviewed-by: Lad Prabhakar <[email protected]> Signed-off-by: Claudiu Beznea <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Vinod Koul <[email protected]>
1 parent de76809 commit 55a387e

File tree

1 file changed

+26
-23
lines changed

1 file changed

+26
-23
lines changed

drivers/phy/renesas/phy-rcar-gen3-usb2.c

Lines changed: 26 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
* Copyright (C) 2014 Cogent Embedded, Inc.
1010
*/
1111

12+
#include <linux/cleanup.h>
1213
#include <linux/extcon-provider.h>
1314
#include <linux/interrupt.h>
1415
#include <linux/io.h>
@@ -118,7 +119,7 @@ struct rcar_gen3_chan {
118119
struct regulator *vbus;
119120
struct reset_control *rstc;
120121
struct work_struct work;
121-
struct mutex lock; /* protects rphys[...].powered */
122+
spinlock_t lock; /* protects access to hardware and driver data structure. */
122123
enum usb_dr_mode dr_mode;
123124
u32 obint_enable_bits;
124125
bool extcon_host;
@@ -348,6 +349,8 @@ static ssize_t role_store(struct device *dev, struct device_attribute *attr,
348349
bool is_b_device;
349350
enum phy_mode cur_mode, new_mode;
350351

352+
guard(spinlock_irqsave)(&ch->lock);
353+
351354
if (!ch->is_otg_channel || !rcar_gen3_is_any_otg_rphy_initialized(ch))
352355
return -EIO;
353356

@@ -415,7 +418,7 @@ static void rcar_gen3_init_otg(struct rcar_gen3_chan *ch)
415418
val = readl(usb2_base + USB2_ADPCTRL);
416419
writel(val | USB2_ADPCTRL_IDPULLUP, usb2_base + USB2_ADPCTRL);
417420
}
418-
msleep(20);
421+
mdelay(20);
419422

420423
writel(0xffffffff, usb2_base + USB2_OBINTSTA);
421424
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)
436439
if (pm_runtime_suspended(dev))
437440
goto rpm_put;
438441

439-
status = readl(usb2_base + USB2_OBINTSTA);
440-
if (status & ch->obint_enable_bits) {
441-
dev_vdbg(dev, "%s: %08x\n", __func__, status);
442-
writel(ch->obint_enable_bits, usb2_base + USB2_OBINTSTA);
443-
rcar_gen3_device_recognition(ch);
444-
ret = IRQ_HANDLED;
442+
scoped_guard(spinlock, &ch->lock) {
443+
status = readl(usb2_base + USB2_OBINTSTA);
444+
if (status & ch->obint_enable_bits) {
445+
dev_vdbg(dev, "%s: %08x\n", __func__, status);
446+
writel(ch->obint_enable_bits, usb2_base + USB2_OBINTSTA);
447+
rcar_gen3_device_recognition(ch);
448+
ret = IRQ_HANDLED;
449+
}
445450
}
446451

447452
rpm_put:
@@ -456,6 +461,8 @@ static int rcar_gen3_phy_usb2_init(struct phy *p)
456461
void __iomem *usb2_base = channel->base;
457462
u32 val;
458463

464+
guard(spinlock_irqsave)(&channel->lock);
465+
459466
/* Initialize USB2 part */
460467
val = readl(usb2_base + USB2_INT_ENABLE);
461468
val |= USB2_INT_ENABLE_UCOM_INTEN | rphy->int_enable_bits;
@@ -479,6 +486,8 @@ static int rcar_gen3_phy_usb2_exit(struct phy *p)
479486
void __iomem *usb2_base = channel->base;
480487
u32 val;
481488

489+
guard(spinlock_irqsave)(&channel->lock);
490+
482491
rphy->initialized = false;
483492

484493
val = readl(usb2_base + USB2_INT_ENABLE);
@@ -498,16 +507,17 @@ static int rcar_gen3_phy_usb2_power_on(struct phy *p)
498507
u32 val;
499508
int ret = 0;
500509

501-
mutex_lock(&channel->lock);
502-
if (!rcar_gen3_are_all_rphys_power_off(channel))
503-
goto out;
504-
505510
if (channel->vbus) {
506511
ret = regulator_enable(channel->vbus);
507512
if (ret)
508-
goto out;
513+
return ret;
509514
}
510515

516+
guard(spinlock_irqsave)(&channel->lock);
517+
518+
if (!rcar_gen3_are_all_rphys_power_off(channel))
519+
goto out;
520+
511521
val = readl(usb2_base + USB2_USBCTR);
512522
val |= USB2_USBCTR_PLL_RST;
513523
writel(val, usb2_base + USB2_USBCTR);
@@ -517,7 +527,6 @@ static int rcar_gen3_phy_usb2_power_on(struct phy *p)
517527
out:
518528
/* The powered flag should be set for any other phys anyway */
519529
rphy->powered = true;
520-
mutex_unlock(&channel->lock);
521530

522531
return 0;
523532
}
@@ -528,18 +537,12 @@ static int rcar_gen3_phy_usb2_power_off(struct phy *p)
528537
struct rcar_gen3_chan *channel = rphy->ch;
529538
int ret = 0;
530539

531-
mutex_lock(&channel->lock);
532-
rphy->powered = false;
533-
534-
if (!rcar_gen3_are_all_rphys_power_off(channel))
535-
goto out;
540+
scoped_guard(spinlock_irqsave, &channel->lock)
541+
rphy->powered = false;
536542

537543
if (channel->vbus)
538544
ret = regulator_disable(channel->vbus);
539545

540-
out:
541-
mutex_unlock(&channel->lock);
542-
543546
return ret;
544547
}
545548

@@ -750,7 +753,7 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
750753
if (phy_data->no_adp_ctrl)
751754
channel->obint_enable_bits = USB2_OBINT_IDCHG_EN;
752755

753-
mutex_init(&channel->lock);
756+
spin_lock_init(&channel->lock);
754757
for (i = 0; i < NUM_OF_PHYS; i++) {
755758
channel->rphys[i].phy = devm_phy_create(dev, NULL,
756759
phy_data->phy_usb2_ops);

0 commit comments

Comments
 (0)