| Message ID | 20191228203358.23490-14-digetx@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | NVIDIA Tegra USB2 drivers clean up | expand |
On Sat, Dec 28, 2019 at 11:33:55PM +0300, Dmitry Osipenko wrote: > There is no good reason for disabling of CPU interrupts in order to > protect the utmip_pad_count modification. Since there are sleeping functions called outside of the locked sections, this should be a mutex instead. OTOH if the spin_lock is to protect register write against IRQ handler, then the patch is wrong. [...] > - spin_unlock_irqrestore(&utmip_pad_lock, flags); > + spin_unlock(&utmip_pad_lock); > > clk_disable_unprepare(phy->pad_clk); Best Regards, Michał Mirosław
30.12.2019 23:36, Michał Mirosław пишет: > On Sat, Dec 28, 2019 at 11:33:55PM +0300, Dmitry Osipenko wrote: >> There is no good reason for disabling of CPU interrupts in order to >> protect the utmip_pad_count modification. > > Since there are sleeping functions called outside of the locked sections, > this should be a mutex instead. OTOH if the spin_lock is to protect register > write against IRQ handler, then the patch is wrong. > > [...] >> - spin_unlock_irqrestore(&utmip_pad_lock, flags); >> + spin_unlock(&utmip_pad_lock); >> >> clk_disable_unprepare(phy->pad_clk); Hello Michał, This spinlock isn't for protecting from the IRQ handler, it's used solely to protect modification of the registers that are shared by all USB controllers. It's possible to use mutex instead of spinlock here, but it doesn't bring any benefits because mutex is more useful when protected code could block for a long time due to sleep or whatever, while spinlock is much more efficient when protected code doesn't sleep and takes no more than dozens microseconds to execute (which is the case here). In this particular case of the Tegra USB PHY driver, the chance of getting a block on taking the utmip_pad_lock is zero unless USB controller drivers will start to use asynchronous probing. So this patch does a very minor clean-up change and nothing more.
On Thu, Jan 02, 2020 at 05:40:48PM +0300, Dmitry Osipenko wrote: > 30.12.2019 23:36, Michał Mirosław пишет: > > On Sat, Dec 28, 2019 at 11:33:55PM +0300, Dmitry Osipenko wrote: > >> There is no good reason for disabling of CPU interrupts in order to > >> protect the utmip_pad_count modification. > > > > Since there are sleeping functions called outside of the locked sections, > > this should be a mutex instead. OTOH if the spin_lock is to protect register > > write against IRQ handler, then the patch is wrong. > > > > [...] > >> - spin_unlock_irqrestore(&utmip_pad_lock, flags); > >> + spin_unlock(&utmip_pad_lock); > >> > >> clk_disable_unprepare(phy->pad_clk); > > Hello Michał, > > This spinlock isn't for protecting from the IRQ handler, it's used > solely to protect modification of the registers that are shared by all > USB controllers. > > It's possible to use mutex instead of spinlock here, but it doesn't > bring any benefits because mutex is more useful when protected code > could block for a long time due to sleep or whatever, while spinlock is > much more efficient when protected code doesn't sleep and takes no more > than dozens microseconds to execute (which is the case here). > > In this particular case of the Tegra USB PHY driver, the chance of > getting a block on taking the utmip_pad_lock is zero unless USB > controller drivers will start to use asynchronous probing. So this patch > does a very minor clean-up change and nothing more. I was concerned that this change allows the kernel to switch away to another task, but I can see now that spin_lock() does disable preemtion. So it looks OK after all. Would be nice to see the explanation in the commit message (that the spinlock is only used to serialize probe()s). Best Regards, Michał Mirosław
diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c index 72825d207840..b0c4f304a5ba 100644 --- a/drivers/usb/phy/phy-tegra-usb.c +++ b/drivers/usb/phy/phy-tegra-usb.c @@ -313,7 +313,6 @@ static int utmip_pad_power_on(struct tegra_usb_phy *phy) { struct tegra_utmip_config *config = phy->config; void __iomem *base = phy->pad_regs; - unsigned long flags; u32 val; int err; @@ -321,7 +320,7 @@ static int utmip_pad_power_on(struct tegra_usb_phy *phy) if (err) return err; - spin_lock_irqsave(&utmip_pad_lock, flags); + spin_lock(&utmip_pad_lock); if (utmip_pad_count++ == 0) { val = readl_relaxed(base + UTMIP_BIAS_CFG0); @@ -339,7 +338,7 @@ static int utmip_pad_power_on(struct tegra_usb_phy *phy) writel_relaxed(val, base + UTMIP_BIAS_CFG0); } - spin_unlock_irqrestore(&utmip_pad_lock, flags); + spin_unlock(&utmip_pad_lock); clk_disable_unprepare(phy->pad_clk); @@ -349,7 +348,6 @@ static int utmip_pad_power_on(struct tegra_usb_phy *phy) static int utmip_pad_power_off(struct tegra_usb_phy *phy) { void __iomem *base = phy->pad_regs; - unsigned long flags; u32 val; int ret; @@ -357,7 +355,7 @@ static int utmip_pad_power_off(struct tegra_usb_phy *phy) if (ret) return ret; - spin_lock_irqsave(&utmip_pad_lock, flags); + spin_lock(&utmip_pad_lock); if (!utmip_pad_count) { dev_err(phy->u_phy.dev, "UTMIP pad already powered off\n"); @@ -371,7 +369,7 @@ static int utmip_pad_power_off(struct tegra_usb_phy *phy) writel_relaxed(val, base + UTMIP_BIAS_CFG0); } ulock: - spin_unlock_irqrestore(&utmip_pad_lock, flags); + spin_unlock(&utmip_pad_lock); clk_disable_unprepare(phy->pad_clk);
There is no good reason for disabling of CPU interrupts in order to protect the utmip_pad_count modification. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/usb/phy/phy-tegra-usb.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)