Skip to contentSkip to conversation

mcs7830 usb net: "scheduling while atomic" danger?

PATCH New Andreas Mohr 5 participants 13 messages Jan 18, 2010 – Jan 19, 2010

Subsystems (2)
USB USB ehci driver
lore
Share & tools
lore

Andreas Mohr

author
#0 lore raw

Hi,

drivers/net/usb/mcs7830.c does several:

        mutex_lock(&dev->phy_mutex);
        /* write the MII command */
        ret = mcs7830_set_reg(dev, HIF_REG_PHY_CMD1, 2, cmd);
        if (ret < 0)
                goto out;

        /* wait for the data to become valid, should be within < 1ms */
        for (i = 0; i < 10; i++) {
                ret = mcs7830_get_reg(dev, HIF_REG_PHY_CMD1, 2, cmd);
                if ((ret < 0) || (cmd[1] &
HIF_REG_PHY_CMD2_READY_FLAG_BIT))
                        break;
                ret = -EIO;
                msleep(1);
        }

Forgive me, but doesn't that mutex_lock()/msleep() (ab)use mean risking a "scheduling while atomic"?
(such as discussed in e.g.
http://search.luky.org/linux-kernel.2004/msg92817.html )

And, if that is the case, shouldn't all such cases simply be killed for good via a capable semantic patch?

Thanks,

Andreas Mohr

#1 lore raw
Quoted 21 lines · Andreas Mohr

Hi,

drivers/net/usb/mcs7830.c does several:

        mutex_lock(&dev->phy_mutex);
        /* write the MII command */
        ret = mcs7830_set_reg(dev, HIF_REG_PHY_CMD1, 2, cmd);
        if (ret < 0)
                goto out;

        /* wait for the data to become valid, should be within < 1ms */
        for (i = 0; i < 10; i++) {
                ret = mcs7830_get_reg(dev, HIF_REG_PHY_CMD1, 2, cmd);
                if ((ret < 0) || (cmd[1] &
HIF_REG_PHY_CMD2_READY_FLAG_BIT))
                        break;
                ret = -EIO;
                msleep(1);
        }
Quoted 8 lines

Forgive me, but doesn't that mutex_lock()/msleep() (ab)use mean risking a "scheduling while atomic"?
(such as discussed in e.g.
http://search.luky.org/linux-kernel.2004/msg92817.html )

And, if that is the case, shouldn't all such cases simply be killed for good via a capable semantic patch?

This could easily be done, if the diagnosis is correct.

julia

#2 lore raw
In reply to Andreas Mohr
Quoted 21 lines · Andreas Mohr

Hi,

drivers/net/usb/mcs7830.c does several:

        mutex_lock(&dev->phy_mutex);
        /* write the MII command */
        ret = mcs7830_set_reg(dev, HIF_REG_PHY_CMD1, 2, cmd);
        if (ret < 0)
                goto out;

        /* wait for the data to become valid, should be within < 1ms */
        for (i = 0; i < 10; i++) {
                ret = mcs7830_get_reg(dev, HIF_REG_PHY_CMD1, 2, cmd);
                if ((ret < 0) || (cmd[1] &
HIF_REG_PHY_CMD2_READY_FLAG_BIT))
                        break;
                ret = -EIO;
                msleep(1);
        }
Quoted 4 lines

Forgive me, but doesn't that mutex_lock()/msleep() (ab)use mean risking a "scheduling while atomic"?
(such as discussed in e.g.
http://search.luky.org/linux-kernel.2004/msg92817.html )

No, that's different. You are allowed to sleep with a mutex held. The thread is about spin_lock()/msleep().

spin_lock() is implicitly disabling preemption, mutex_lock() does not.

Thanks,

tglx

#3 lore raw
In reply to Andreas Mohr
Quoted 21 lines · Andreas Mohr

Hi,

drivers/net/usb/mcs7830.c does several:

        mutex_lock(&dev->phy_mutex);
        /* write the MII command */
        ret = mcs7830_set_reg(dev, HIF_REG_PHY_CMD1, 2, cmd);
        if (ret < 0)
                goto out;

        /* wait for the data to become valid, should be within < 1ms */
        for (i = 0; i < 10; i++) {
                ret = mcs7830_get_reg(dev, HIF_REG_PHY_CMD1, 2, cmd);
                if ((ret < 0) || (cmd[1] &
HIF_REG_PHY_CMD2_READY_FLAG_BIT))
                        break;
                ret = -EIO;
                msleep(1);
        }
Quoted 8 lines

Forgive me, but doesn't that mutex_lock()/msleep() (ab)use mean risking a "scheduling while atomic"?
(such as discussed in e.g.
http://search.luky.org/linux-kernel.2004/msg92817.html )

And, if that is the case, shouldn't all such cases simply be killed for good via a capable semantic patch?

The semantic match shown below finds 55 matches. All but two involve mutex_lock. Those are in the file /var/linuxes/linux-next/drivers/usb/host/ehci-hub.c
in the functions ehci_bus_suspend and ehci_hub_control.

julia

@@
@@

(
*mutex_lock(...)
|
*spin_lock(...)
|
*spin_lock_irq(...)
|
*spin_lock_irqsave(...)
)
... when != mutex_unlock(...)

    when != spin_unlock(...)
    when != spin_unlock_irq(...)
    when != spin_unlock_irqrestore(...)

*msleep(...)

#4 lore raw

Julia,

Quoted 4 lines · Julia Lawall

On Mon, 18 Jan 2010, Andreas Mohr wrote:

The semantic match shown below finds 55 matches. All but two involve mutex_lock. Those are in the file

As I said before the mutex_lock()/msleep() ones are fine.

Quoted 2 lines

/var/linuxes/linux-next/drivers/usb/host/ehci-hub.c in the functions ehci_bus_suspend and ehci_hub_control.

The msleep in ehci_hub_control() which happens with ehci->lock held and irqs disabled is definitely buggy.

I can't see anything wrong wth the msleep in ehci_bus_suspend() as it is _before_ the spin_lock_irq(&ehci->lock) region.

Thanks,

tglx

#5 lore raw
Quoted 9 lines · Thomas Gleixner

Julia,

Quoted 4 lines · Julia Lawall

On Mon, 18 Jan 2010, Andreas Mohr wrote:

The semantic match shown below finds 55 matches. All but two involve mutex_lock. Those are in the file

As I said before the mutex_lock()/msleep() ones are fine.

OK, I sem to have missed that message.

Quoted 8 lines
Quoted 2 lines

/var/linuxes/linux-next/drivers/usb/host/ehci-hub.c in the functions ehci_bus_suspend and ehci_hub_control.

The msleep in ehci_hub_control() which happens with ehci->lock held and irqs disabled is definitely buggy.

I can't see anything wrong wth the msleep in ehci_bus_suspend() as it is _before_ the spin_lock_irq(&ehci->lock) region.

There is another one later in the function:

msleep(5);/* 5ms for HCD enter low pwr mode */

In my linux-next, it is on line 181.

julia

#6 lore raw
Quoted 20 lines · Julia Lawall
Quoted 9 lines · Thomas Gleixner

Julia,

Quoted 4 lines · Julia Lawall

On Mon, 18 Jan 2010, Andreas Mohr wrote:

The semantic match shown below finds 55 matches. All but two involve mutex_lock. Those are in the file

As I said before the mutex_lock()/msleep() ones are fine.

OK, I sem to have missed that message.

Quoted 6 lines
Quoted 2 lines

/var/linuxes/linux-next/drivers/usb/host/ehci-hub.c in the functions ehci_bus_suspend and ehci_hub_control.

The msleep in ehci_hub_control() which happens with ehci->lock held and irqs disabled is definitely buggy.

Quoted 8 lines
Quoted 2 lines

I can't see anything wrong wth the msleep in ehci_bus_suspend() as it is _before_ the spin_lock_irq(&ehci->lock) region.

There is another one later in the function:

msleep(5);/* 5ms for HCD enter low pwr mode */

In my linux-next, it is on line 181.

Ooops, missed that one. Right, that's buggy as well.

Thanks,

tglx

#7 lore raw
In reply to Julia Lawall
Quoted 8 lines · Julia Lawall
Quoted 7 lines · Andreas Mohr

Forgive me, but doesn't that mutex_lock()/msleep() (ab)use mean risking a "scheduling while atomic"?
(such as discussed in e.g.
http://search.luky.org/linux-kernel.2004/msg92817.html )

Quoted 7 lines
Quoted 2 lines

And, if that is the case, shouldn't all such cases simply be killed for good via a capable semantic patch?

The semantic match shown below finds 55 matches. All but two involve mutex_lock. Those are in the file /var/linuxes/linux-next/drivers/usb/host/ehci-hub.c
in the functions ehci_bus_suspend and ehci_hub_control.

That code looks indeed broken as was added las July as part of 331ac6b288d9 "USB: EHCI: Add Intel Moorestown EHCI controller HOSTPCx extensions and support phy low power mode". The reason that this hasn't triggered is probably the lack of Moorestown machines in the field.

Arnd

#8 lore raw
Quoted 17 lines · Arnd Bergmann
Quoted 8 lines · Julia Lawall
Quoted 7 lines · Andreas Mohr

Forgive me, but doesn't that mutex_lock()/msleep() (ab)use mean risking a "scheduling while atomic"?
(such as discussed in e.g.
http://search.luky.org/linux-kernel.2004/msg92817.html )

Quoted 7 lines
Quoted 2 lines

And, if that is the case, shouldn't all such cases simply be killed for good via a capable semantic patch?

The semantic match shown below finds 55 matches. All but two involve mutex_lock. Those are in the file /var/linuxes/linux-next/drivers/usb/host/ehci-hub.c
in the functions ehci_bus_suspend and ehci_hub_control.

Quoted 4 lines

That code looks indeed broken as was added las July as part of 331ac6b288d9 "USB: EHCI: Add Intel Moorestown EHCI controller HOSTPCx extensions and support phy low power mode". The reason that this hasn't triggered is probably the lack of Moorestown machines in the field.

The fix is just msleep -> mdelay?

julia

#9 lore raw
Quoted 6 lines · Julia Lawall
Quoted 4 lines

That code looks indeed broken as was added las July as part of 331ac6b288d9 "USB: EHCI: Add Intel Moorestown EHCI controller HOSTPCx extensions and support phy low power mode". The reason that this hasn't triggered is probably the lack of Moorestown machines in the field.

The fix is just msleep -> mdelay?

No, that would just kill the warning but still leave horrible code. There should really be some way to move the sleeping operation outside of the spinlock.

Quoted 1 line

From a brief look at the code, I think the sequence could be converted

from

lock();
suspend();
sleep();
clock_disable();
unlock();

to

lock();
again:
suspend();
unlock();
sleep();
lock();
if (!suspended())
goto again;
clock_disable();
unlock();

Arnd

#10 lore raw

PATCH 1/1 ehci: phy low power mode bug fixing

This patch should fix the bug and I tested it with Moorestown platform... But the strange thing is that even the original code does not trigger the "scheduling while atomic" issue...

Quoted 1 line

From ca7833d854867e91d6aa6bccf1f3224862b3a25c Mon Sep 17 00:00:00 2001

From: Alek Du <[email protected]>
Date: Tue, 19 Jan 2010 16:05:15 +0800
Subject: [PATCH] ehci: phy low power mode bug fixing

1. There are two msleep calls inside two spin lock sections, need to unlock and lock again after msleep.
2. Save a extra status reg setting.

Signed-off-by Alek Du <[email protected]>
---
 drivers/usb/host/ehci-hub.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)
drivers/usb/host/ehci-hub.c +8 -5
index 2c6571c..7d77fbb 100644
@@ -178,7 +178,9 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
 			if (hostpc_reg) {
 				u32	t3;
 
+				spin_unlock_irq(&ehci->lock);
 				msleep(5);/* 5ms for HCD enter low pwr mode */
+				spin_lock_irq(&ehci->lock);
 				t3 = ehci_readl(ehci, hostpc_reg);
 				ehci_writel(ehci, t3 | HOSTPC_PHCD, hostpc_reg);
 				t3 = ehci_readl(ehci, hostpc_reg);

@@ -886,17 +888,18 @@ static int ehci_hub_control (
 			if ((temp & PORT_PE) == 0
 					|| (temp & PORT_RESET) != 0)
 				goto error;
-			ehci_writel(ehci, temp | PORT_SUSPEND, status_reg);
+
 			/* After above check the port must be connected.
 			 * Set appropriate bit thus could put phy into low power
 			 * mode if we have hostpc feature
 			 */
+			temp &= ~PORT_WKCONN_E;
+			temp |= PORT_WKDISC_E | PORT_WKOC_E;
+			ehci_writel(ehci, temp | PORT_SUSPEND, status_reg);
 			if (hostpc_reg) {
-				temp &= ~PORT_WKCONN_E;
-				temp |= (PORT_WKDISC_E | PORT_WKOC_E);
-				ehci_writel(ehci, temp | PORT_SUSPEND,
-							status_reg);
+				spin_unlock_irqrestore(&ehci->lock, flags);
 				msleep(5);/* 5ms for HCD enter low pwr mode */
+				spin_lock_irqsave(&ehci->lock, flags);
 				temp1 = ehci_readl(ehci, hostpc_reg);
 				ehci_writel(ehci, temp1 | HOSTPC_PHCD,
 					hostpc_reg);
Signature · 4 lines
1.6.3.3


On Tue, 19 Jan 2010 06:25:10 +0800
Quoted 36 lines · Arnd Bergmann
Quoted 7 lines · Julia Lawall
Quoted 5 lines

That code looks indeed broken as was added las July as part of 331ac6b288d9 "USB: EHCI: Add Intel Moorestown EHCI controller HOSTPCx extensions and support phy low power mode". The reason that this hasn't triggered is probably the lack of Moorestown machines in the field.

The fix is just msleep -> mdelay?

No, that would just kill the warning but still leave horrible code. There should really be some way to move the sleeping operation outside of the spinlock.

From a brief look at the code, I think the sequence could be converted from

lock();
suspend();
sleep();
clock_disable();
unlock();

to

lock();
again:
suspend();
unlock();
sleep();
lock();
if (!suspended())
goto again;
clock_disable();
unlock();

Arnd

#11 lore raw
In reply to Arnd Bergmann

I confirm that thing is bad, I need to prepare another patch to fix that.

Thanks,
Alek

Quoted 31 lines

-----Original Message-----
From: Arnd Bergmann [mailto:[email protected]]
Sent: Tuesday, January 19, 2010 5:24 AM
To: Julia Lawall
Cc: Andreas Mohr; [email protected]; Greg KH; Du, Alek; Pan, Jacob jun; Alan Stern
Subject: Re: mcs7830 usb net: "scheduling while atomic" danger?

Quoted 8 lines · Julia Lawall
Quoted 7 lines · Andreas Mohr

Forgive me, but doesn't that mutex_lock()/msleep() (ab)use mean risking a "scheduling while atomic"?
(such as discussed in e.g.
http://search.luky.org/linux-kernel.2004/msg92817.html )

Quoted 7 lines
Quoted 2 lines

And, if that is the case, shouldn't all such cases simply be killed for good via a capable semantic patch?

The semantic match shown below finds 55 matches. All but two involve mutex_lock. Those are in the file /var/linuxes/linux-next/drivers/usb/host/ehci-hub.c
in the functions ehci_bus_suspend and ehci_hub_control.

That code looks indeed broken as was added las July as part of 331ac6b288d9 "USB: EHCI: Add Intel Moorestown EHCI controller HOSTPCx extensions and support phy low power mode". The reason that this hasn't triggered is probably the lack of Moorestown machines in the field.

Arnd

#12 lore raw

So you will fix /var/linuxes/linux-next/drivers/usb/host/ehci-hub.c?

I think it is a bit out of my expertise...

julia

Quoted 36 lines · Alek

I confirm that thing is bad, I need to prepare another patch to fix that.

Thanks,
Alek

Quoted 31 lines

-----Original Message-----
From: Arnd Bergmann [mailto:[email protected]]
Sent: Tuesday, January 19, 2010 5:24 AM
To: Julia Lawall
Cc: Andreas Mohr; [email protected]; Greg KH; Du, Alek; Pan, Jacob jun; Alan Stern
Subject: Re: mcs7830 usb net: "scheduling while atomic" danger?

Quoted 8 lines · Julia Lawall
Quoted 7 lines · Andreas Mohr

Forgive me, but doesn't that mutex_lock()/msleep() (ab)use mean risking a "scheduling while atomic"?
(such as discussed in e.g.
http://search.luky.org/linux-kernel.2004/msg92817.html )

Quoted 7 lines
Quoted 2 lines

And, if that is the case, shouldn't all such cases simply be killed for good via a capable semantic patch?

The semantic match shown below finds 55 matches. All but two involve mutex_lock. Those are in the file /var/linuxes/linux-next/drivers/usb/host/ehci-hub.c
in the functions ehci_bus_suspend and ehci_hub_control.

That code looks indeed broken as was added las July as part of 331ac6b288d9 "USB: EHCI: Add Intel Moorestown EHCI controller HOSTPCx extensions and support phy low power mode". The reason that this hasn't triggered is probably the lack of Moorestown machines in the field.

Arnd