* [PATCH] phy: sun4i-usb: Use spinlock to guard phyctl register access
@ 2016-09-08 3:14 Chen-Yu Tsai
2016-09-08 10:16 ` Hans de Goede
0 siblings, 1 reply; 4+ messages in thread
From: Chen-Yu Tsai @ 2016-09-08 3:14 UTC (permalink / raw)
To: Kishon Vijay Abraham I, Maxime Ripard
Cc: Chen-Yu Tsai, linux-arm-kernel, linux-kernel, Hans de Goede, stable
The musb driver calls into this phy driver to disable/enable squelch
detection. This function was introduced in 24fe86a617c5 ("phy: sun4i-usb:
Add a sunxi specific function for setting squelch-detect"). This
function in turn calls sun4i_usb_phy_write, which uses a mutex to
guard the common access register. Unfortunately musb does this
in atomic context, which results in the following warning with lock
debugging enabled:
BUG: sleeping function called from invalid context at kernel/locking/mutex.c:97
in_atomic(): 1, irqs_disabled(): 128, pid: 96, name: kworker/0:2
CPU: 0 PID: 96 Comm: kworker/0:2 Not tainted 4.8.0-rc4-00181-gd502f8ad1c3e #13
Hardware name: Allwinner sun8i Family
Workqueue: events musb_deassert_reset
[<c010bc01>] (unwind_backtrace) from [<c0109237>] (show_stack+0xb/0xc)
[<c0109237>] (show_stack) from [<c02a669b>] (dump_stack+0x67/0x74)
[<c02a669b>] (dump_stack) from [<c05d68c9>] (mutex_lock+0x15/0x2c)
[<c05d68c9>] (mutex_lock) from [<c02c3589>] (sun4i_usb_phy_write+0x39/0xec)
[<c02c3589>] (sun4i_usb_phy_write) from [<c03e6327>] (musb_port_reset+0xfb/0x184)
[<c03e6327>] (musb_port_reset) from [<c03e4917>] (musb_deassert_reset+0x1f/0x2c)
[<c03e4917>] (musb_deassert_reset) from [<c012ecb5>] (process_one_work+0x129/0x2b8)
[<c012ecb5>] (process_one_work) from [<c012f5e3>] (worker_thread+0xf3/0x424)
[<c012f5e3>] (worker_thread) from [<c0132dbd>] (kthread+0xa1/0xb8)
[<c0132dbd>] (kthread) from [<c0105f31>] (ret_from_fork+0x11/0x20)
Since the register access is mmio, we can use a spinlock to guard this
specific access, rather than the mutex that guards the entire phy.
Fixes: ba4bdc9e1dc0 ("PHY: sunxi: Add driver for sunxi usb phy")
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: stable@vger.kernel.org
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
drivers/phy/phy-sun4i-usb.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
index fcf4d95ecc6d..ae4ac5457c64 100644
--- a/drivers/phy/phy-sun4i-usb.c
+++ b/drivers/phy/phy-sun4i-usb.c
@@ -40,6 +40,7 @@
#include <linux/power_supply.h>
#include <linux/regulator/consumer.h>
#include <linux/reset.h>
+#include <linux/spinlock.h>
#include <linux/usb/of.h>
#include <linux/workqueue.h>
@@ -115,6 +116,7 @@ struct sun4i_usb_phy_data {
const struct sun4i_usb_phy_cfg *cfg;
enum usb_dr_mode dr_mode;
struct mutex mutex;
+ spinlock_t reg_lock; /* guard access to phyctl reg */
struct sun4i_usb_phy {
struct phy *phy;
void __iomem *pmu;
@@ -183,7 +185,7 @@ static void sun4i_usb_phy_write(struct sun4i_usb_phy *phy, u32 addr, u32 data,
void __iomem *phyctl = phy_data->base + phy_data->cfg->phyctl_offset;
int i;
- mutex_lock(&phy_data->mutex);
+ spin_lock(&phy_data->reg_lock);
if (phy_data->cfg->type == sun8i_a33_phy ||
phy_data->cfg->type == sun50i_a64_phy) {
@@ -221,7 +223,8 @@ static void sun4i_usb_phy_write(struct sun4i_usb_phy *phy, u32 addr, u32 data,
data >>= 1;
}
- mutex_unlock(&phy_data->mutex);
+
+ spin_unlock(&phy_data->reg_lock);
}
static void sun4i_usb_phy_passby(struct sun4i_usb_phy *phy, int enable)
@@ -583,6 +586,7 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
return -ENOMEM;
mutex_init(&data->mutex);
+ spin_lock_init(&data->reg_lock);
INIT_DELAYED_WORK(&data->detect, sun4i_usb_phy0_id_vbus_det_scan);
dev_set_drvdata(dev, data);
data->cfg = of_device_get_match_data(dev);
--
2.9.3
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] phy: sun4i-usb: Use spinlock to guard phyctl register access
2016-09-08 3:14 [PATCH] phy: sun4i-usb: Use spinlock to guard phyctl register access Chen-Yu Tsai
@ 2016-09-08 10:16 ` Hans de Goede
2016-09-08 10:24 ` Chen-Yu Tsai
0 siblings, 1 reply; 4+ messages in thread
From: Hans de Goede @ 2016-09-08 10:16 UTC (permalink / raw)
To: Chen-Yu Tsai, Kishon Vijay Abraham I, Maxime Ripard
Cc: linux-arm-kernel, linux-kernel, stable
Hi,
On 08-09-16 05:14, Chen-Yu Tsai wrote:
> The musb driver calls into this phy driver to disable/enable squelch
> detection. This function was introduced in 24fe86a617c5 ("phy: sun4i-usb:
> Add a sunxi specific function for setting squelch-detect"). This
> function in turn calls sun4i_usb_phy_write, which uses a mutex to
> guard the common access register. Unfortunately musb does this
> in atomic context, which results in the following warning with lock
> debugging enabled:
>
> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:97
> in_atomic(): 1, irqs_disabled(): 128, pid: 96, name: kworker/0:2
> CPU: 0 PID: 96 Comm: kworker/0:2 Not tainted 4.8.0-rc4-00181-gd502f8ad1c3e #13
> Hardware name: Allwinner sun8i Family
> Workqueue: events musb_deassert_reset
> [<c010bc01>] (unwind_backtrace) from [<c0109237>] (show_stack+0xb/0xc)
> [<c0109237>] (show_stack) from [<c02a669b>] (dump_stack+0x67/0x74)
> [<c02a669b>] (dump_stack) from [<c05d68c9>] (mutex_lock+0x15/0x2c)
> [<c05d68c9>] (mutex_lock) from [<c02c3589>] (sun4i_usb_phy_write+0x39/0xec)
> [<c02c3589>] (sun4i_usb_phy_write) from [<c03e6327>] (musb_port_reset+0xfb/0x184)
> [<c03e6327>] (musb_port_reset) from [<c03e4917>] (musb_deassert_reset+0x1f/0x2c)
> [<c03e4917>] (musb_deassert_reset) from [<c012ecb5>] (process_one_work+0x129/0x2b8)
> [<c012ecb5>] (process_one_work) from [<c012f5e3>] (worker_thread+0xf3/0x424)
> [<c012f5e3>] (worker_thread) from [<c0132dbd>] (kthread+0xa1/0xb8)
> [<c0132dbd>] (kthread) from [<c0105f31>] (ret_from_fork+0x11/0x20)
>
> Since the register access is mmio, we can use a spinlock to guard this
> specific access, rather than the mutex that guards the entire phy.
>
> Fixes: ba4bdc9e1dc0 ("PHY: sunxi: Add driver for sunxi usb phy")
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
Good catch, but you're actually replacing the locking calls in the only
user of sun4i_usb_phy_data.mutex, so after your patch it is no longer
used anywhere. Can you do a v2 just outright replacing it with a
spinlock please ?
Thanks,
Hans
> ---
> drivers/phy/phy-sun4i-usb.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
> index fcf4d95ecc6d..ae4ac5457c64 100644
> --- a/drivers/phy/phy-sun4i-usb.c
> +++ b/drivers/phy/phy-sun4i-usb.c
> @@ -40,6 +40,7 @@
> #include <linux/power_supply.h>
> #include <linux/regulator/consumer.h>
> #include <linux/reset.h>
> +#include <linux/spinlock.h>
> #include <linux/usb/of.h>
> #include <linux/workqueue.h>
>
> @@ -115,6 +116,7 @@ struct sun4i_usb_phy_data {
> const struct sun4i_usb_phy_cfg *cfg;
> enum usb_dr_mode dr_mode;
> struct mutex mutex;
> + spinlock_t reg_lock; /* guard access to phyctl reg */
> struct sun4i_usb_phy {
> struct phy *phy;
> void __iomem *pmu;
> @@ -183,7 +185,7 @@ static void sun4i_usb_phy_write(struct sun4i_usb_phy *phy, u32 addr, u32 data,
> void __iomem *phyctl = phy_data->base + phy_data->cfg->phyctl_offset;
> int i;
>
> - mutex_lock(&phy_data->mutex);
> + spin_lock(&phy_data->reg_lock);
>
> if (phy_data->cfg->type == sun8i_a33_phy ||
> phy_data->cfg->type == sun50i_a64_phy) {
> @@ -221,7 +223,8 @@ static void sun4i_usb_phy_write(struct sun4i_usb_phy *phy, u32 addr, u32 data,
>
> data >>= 1;
> }
> - mutex_unlock(&phy_data->mutex);
> +
> + spin_unlock(&phy_data->reg_lock);
> }
>
> static void sun4i_usb_phy_passby(struct sun4i_usb_phy *phy, int enable)
> @@ -583,6 +586,7 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
> return -ENOMEM;
>
> mutex_init(&data->mutex);
> + spin_lock_init(&data->reg_lock);
> INIT_DELAYED_WORK(&data->detect, sun4i_usb_phy0_id_vbus_det_scan);
> dev_set_drvdata(dev, data);
> data->cfg = of_device_get_match_data(dev);
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] phy: sun4i-usb: Use spinlock to guard phyctl register access
2016-09-08 10:16 ` Hans de Goede
@ 2016-09-08 10:24 ` Chen-Yu Tsai
2016-09-08 11:57 ` Hans de Goede
0 siblings, 1 reply; 4+ messages in thread
From: Chen-Yu Tsai @ 2016-09-08 10:24 UTC (permalink / raw)
To: Hans de Goede
Cc: Chen-Yu Tsai, Kishon Vijay Abraham I, Maxime Ripard,
linux-arm-kernel, linux-kernel, stable
On Thu, Sep 8, 2016 at 6:16 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 08-09-16 05:14, Chen-Yu Tsai wrote:
>>
>> The musb driver calls into this phy driver to disable/enable squelch
>> detection. This function was introduced in 24fe86a617c5 ("phy: sun4i-usb:
>> Add a sunxi specific function for setting squelch-detect"). This
>> function in turn calls sun4i_usb_phy_write, which uses a mutex to
>> guard the common access register. Unfortunately musb does this
>> in atomic context, which results in the following warning with lock
>> debugging enabled:
>>
>> BUG: sleeping function called from invalid context at
>> kernel/locking/mutex.c:97
>> in_atomic(): 1, irqs_disabled(): 128, pid: 96, name: kworker/0:2
>> CPU: 0 PID: 96 Comm: kworker/0:2 Not tainted 4.8.0-rc4-00181-gd502f8ad1c3e
>> #13
>> Hardware name: Allwinner sun8i Family
>> Workqueue: events musb_deassert_reset
>> [<c010bc01>] (unwind_backtrace) from [<c0109237>] (show_stack+0xb/0xc)
>> [<c0109237>] (show_stack) from [<c02a669b>] (dump_stack+0x67/0x74)
>> [<c02a669b>] (dump_stack) from [<c05d68c9>] (mutex_lock+0x15/0x2c)
>> [<c05d68c9>] (mutex_lock) from [<c02c3589>]
>> (sun4i_usb_phy_write+0x39/0xec)
>> [<c02c3589>] (sun4i_usb_phy_write) from [<c03e6327>]
>> (musb_port_reset+0xfb/0x184)
>> [<c03e6327>] (musb_port_reset) from [<c03e4917>]
>> (musb_deassert_reset+0x1f/0x2c)
>> [<c03e4917>] (musb_deassert_reset) from [<c012ecb5>]
>> (process_one_work+0x129/0x2b8)
>> [<c012ecb5>] (process_one_work) from [<c012f5e3>]
>> (worker_thread+0xf3/0x424)
>> [<c012f5e3>] (worker_thread) from [<c0132dbd>] (kthread+0xa1/0xb8)
>> [<c0132dbd>] (kthread) from [<c0105f31>] (ret_from_fork+0x11/0x20)
>>
>> Since the register access is mmio, we can use a spinlock to guard this
>> specific access, rather than the mutex that guards the entire phy.
>>
>> Fixes: ba4bdc9e1dc0 ("PHY: sunxi: Add driver for sunxi usb phy")
>> Cc: Hans de Goede <hdegoede@redhat.com>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>
>
> Good catch, but you're actually replacing the locking calls in the only
> user of sun4i_usb_phy_data.mutex, so after your patch it is no longer
> used anywhere. Can you do a v2 just outright replacing it with a
> spinlock please ?
Ah. I misread the other use of mutex_lock, which is actually using
the mutex in struct phy. Sure I'll just replace it.
After sending this patch I wondered if I should have used
spin_lock_irqsave though.
ChenYu
>
> Thanks,
>
> Hans
>
>
>
>
>> ---
>> drivers/phy/phy-sun4i-usb.c | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
>> index fcf4d95ecc6d..ae4ac5457c64 100644
>> --- a/drivers/phy/phy-sun4i-usb.c
>> +++ b/drivers/phy/phy-sun4i-usb.c
>> @@ -40,6 +40,7 @@
>> #include <linux/power_supply.h>
>> #include <linux/regulator/consumer.h>
>> #include <linux/reset.h>
>> +#include <linux/spinlock.h>
>> #include <linux/usb/of.h>
>> #include <linux/workqueue.h>
>>
>> @@ -115,6 +116,7 @@ struct sun4i_usb_phy_data {
>> const struct sun4i_usb_phy_cfg *cfg;
>> enum usb_dr_mode dr_mode;
>> struct mutex mutex;
>> + spinlock_t reg_lock; /* guard access to phyctl reg */
>> struct sun4i_usb_phy {
>> struct phy *phy;
>> void __iomem *pmu;
>> @@ -183,7 +185,7 @@ static void sun4i_usb_phy_write(struct sun4i_usb_phy
>> *phy, u32 addr, u32 data,
>> void __iomem *phyctl = phy_data->base +
>> phy_data->cfg->phyctl_offset;
>> int i;
>>
>> - mutex_lock(&phy_data->mutex);
>> + spin_lock(&phy_data->reg_lock);
>>
>> if (phy_data->cfg->type == sun8i_a33_phy ||
>> phy_data->cfg->type == sun50i_a64_phy) {
>> @@ -221,7 +223,8 @@ static void sun4i_usb_phy_write(struct sun4i_usb_phy
>> *phy, u32 addr, u32 data,
>>
>> data >>= 1;
>> }
>> - mutex_unlock(&phy_data->mutex);
>> +
>> + spin_unlock(&phy_data->reg_lock);
>> }
>>
>> static void sun4i_usb_phy_passby(struct sun4i_usb_phy *phy, int enable)
>> @@ -583,6 +586,7 @@ static int sun4i_usb_phy_probe(struct platform_device
>> *pdev)
>> return -ENOMEM;
>>
>> mutex_init(&data->mutex);
>> + spin_lock_init(&data->reg_lock);
>> INIT_DELAYED_WORK(&data->detect, sun4i_usb_phy0_id_vbus_det_scan);
>> dev_set_drvdata(dev, data);
>> data->cfg = of_device_get_match_data(dev);
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] phy: sun4i-usb: Use spinlock to guard phyctl register access
2016-09-08 10:24 ` Chen-Yu Tsai
@ 2016-09-08 11:57 ` Hans de Goede
0 siblings, 0 replies; 4+ messages in thread
From: Hans de Goede @ 2016-09-08 11:57 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Kishon Vijay Abraham I, Maxime Ripard, linux-arm-kernel,
linux-kernel, stable
Hi,
On 08-09-16 12:24, Chen-Yu Tsai wrote:
> On Thu, Sep 8, 2016 at 6:16 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>>
>> On 08-09-16 05:14, Chen-Yu Tsai wrote:
>>>
>>> The musb driver calls into this phy driver to disable/enable squelch
>>> detection. This function was introduced in 24fe86a617c5 ("phy: sun4i-usb:
>>> Add a sunxi specific function for setting squelch-detect"). This
>>> function in turn calls sun4i_usb_phy_write, which uses a mutex to
>>> guard the common access register. Unfortunately musb does this
>>> in atomic context, which results in the following warning with lock
>>> debugging enabled:
>>>
>>> BUG: sleeping function called from invalid context at
>>> kernel/locking/mutex.c:97
>>> in_atomic(): 1, irqs_disabled(): 128, pid: 96, name: kworker/0:2
>>> CPU: 0 PID: 96 Comm: kworker/0:2 Not tainted 4.8.0-rc4-00181-gd502f8ad1c3e
>>> #13
>>> Hardware name: Allwinner sun8i Family
>>> Workqueue: events musb_deassert_reset
>>> [<c010bc01>] (unwind_backtrace) from [<c0109237>] (show_stack+0xb/0xc)
>>> [<c0109237>] (show_stack) from [<c02a669b>] (dump_stack+0x67/0x74)
>>> [<c02a669b>] (dump_stack) from [<c05d68c9>] (mutex_lock+0x15/0x2c)
>>> [<c05d68c9>] (mutex_lock) from [<c02c3589>]
>>> (sun4i_usb_phy_write+0x39/0xec)
>>> [<c02c3589>] (sun4i_usb_phy_write) from [<c03e6327>]
>>> (musb_port_reset+0xfb/0x184)
>>> [<c03e6327>] (musb_port_reset) from [<c03e4917>]
>>> (musb_deassert_reset+0x1f/0x2c)
>>> [<c03e4917>] (musb_deassert_reset) from [<c012ecb5>]
>>> (process_one_work+0x129/0x2b8)
>>> [<c012ecb5>] (process_one_work) from [<c012f5e3>]
>>> (worker_thread+0xf3/0x424)
>>> [<c012f5e3>] (worker_thread) from [<c0132dbd>] (kthread+0xa1/0xb8)
>>> [<c0132dbd>] (kthread) from [<c0105f31>] (ret_from_fork+0x11/0x20)
>>>
>>> Since the register access is mmio, we can use a spinlock to guard this
>>> specific access, rather than the mutex that guards the entire phy.
>>>
>>> Fixes: ba4bdc9e1dc0 ("PHY: sunxi: Add driver for sunxi usb phy")
>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>>
>>
>> Good catch, but you're actually replacing the locking calls in the only
>> user of sun4i_usb_phy_data.mutex, so after your patch it is no longer
>> used anywhere. Can you do a v2 just outright replacing it with a
>> spinlock please ?
>
> Ah. I misread the other use of mutex_lock, which is actually using
> the mutex in struct phy. Sure I'll just replace it.
>
> After sending this patch I wondered if I should have used
> spin_lock_irqsave though.
Good point, yes please use spin_lock_irqsave, I believe this
code path may get called outside of irq handling so using
spin_lock_irqsave is the right thing todo.
Regards,
Hans
>
> ChenYu
>
>>
>> Thanks,
>>
>> Hans
>>
>>
>>
>>
>>> ---
>>> drivers/phy/phy-sun4i-usb.c | 8 ++++++--
>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
>>> index fcf4d95ecc6d..ae4ac5457c64 100644
>>> --- a/drivers/phy/phy-sun4i-usb.c
>>> +++ b/drivers/phy/phy-sun4i-usb.c
>>> @@ -40,6 +40,7 @@
>>> #include <linux/power_supply.h>
>>> #include <linux/regulator/consumer.h>
>>> #include <linux/reset.h>
>>> +#include <linux/spinlock.h>
>>> #include <linux/usb/of.h>
>>> #include <linux/workqueue.h>
>>>
>>> @@ -115,6 +116,7 @@ struct sun4i_usb_phy_data {
>>> const struct sun4i_usb_phy_cfg *cfg;
>>> enum usb_dr_mode dr_mode;
>>> struct mutex mutex;
>>> + spinlock_t reg_lock; /* guard access to phyctl reg */
>>> struct sun4i_usb_phy {
>>> struct phy *phy;
>>> void __iomem *pmu;
>>> @@ -183,7 +185,7 @@ static void sun4i_usb_phy_write(struct sun4i_usb_phy
>>> *phy, u32 addr, u32 data,
>>> void __iomem *phyctl = phy_data->base +
>>> phy_data->cfg->phyctl_offset;
>>> int i;
>>>
>>> - mutex_lock(&phy_data->mutex);
>>> + spin_lock(&phy_data->reg_lock);
>>>
>>> if (phy_data->cfg->type == sun8i_a33_phy ||
>>> phy_data->cfg->type == sun50i_a64_phy) {
>>> @@ -221,7 +223,8 @@ static void sun4i_usb_phy_write(struct sun4i_usb_phy
>>> *phy, u32 addr, u32 data,
>>>
>>> data >>= 1;
>>> }
>>> - mutex_unlock(&phy_data->mutex);
>>> +
>>> + spin_unlock(&phy_data->reg_lock);
>>> }
>>>
>>> static void sun4i_usb_phy_passby(struct sun4i_usb_phy *phy, int enable)
>>> @@ -583,6 +586,7 @@ static int sun4i_usb_phy_probe(struct platform_device
>>> *pdev)
>>> return -ENOMEM;
>>>
>>> mutex_init(&data->mutex);
>>> + spin_lock_init(&data->reg_lock);
>>> INIT_DELAYED_WORK(&data->detect, sun4i_usb_phy0_id_vbus_det_scan);
>>> dev_set_drvdata(dev, data);
>>> data->cfg = of_device_get_match_data(dev);
>>>
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-09-08 11:58 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-08 3:14 [PATCH] phy: sun4i-usb: Use spinlock to guard phyctl register access Chen-Yu Tsai
2016-09-08 10:16 ` Hans de Goede
2016-09-08 10:24 ` Chen-Yu Tsai
2016-09-08 11:57 ` Hans de Goede
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).