[PATCH] net: mdio-gpio: support access that may sleep

[PATCH] net: mdio-gpio: support access that may sleep

[Vivien Didelot]

Some systems using mdio-gpio may use gpio on message based busses, which
require sleeping (e.g. gpio from an I2C I/O expander).

Since this driver does not use IRQ handler, it is safe to use the
_cansleep suffixed gpio accessors.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/phy/mdio-gpio.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c
index 49ce7ec..c9cb486c 100644
--- a/drivers/net/phy/mdio-gpio.c
+++ b/drivers/net/phy/mdio-gpio.c
@@ -80,7 +80,8 @@ static void mdio_dir(struct mdiobb_ctrl *ctrl, int dir)
 		 * assume the pin serves as pull-up. If direction is
 		 * output, the default value is high.
 		 */
-		gpio_set_value(bitbang->mdo, 1 ^ bitbang->mdo_active_low);
+		gpio_set_value_cansleep(bitbang->mdo,
+					1 ^ bitbang->mdo_active_low);
 		return;
 	}
 
@@ -96,7 +97,8 @@ static int mdio_get(struct mdiobb_ctrl *ctrl)
 	struct mdio_gpio_info *bitbang =
 		container_of(ctrl, struct mdio_gpio_info, ctrl);
 
-	return gpio_get_value(bitbang->mdio) ^ bitbang->mdio_active_low;
+	return gpio_get_value_cansleep(bitbang->mdio) ^
+		bitbang->mdio_active_low;
 }
 
 static void mdio_set(struct mdiobb_ctrl *ctrl, int what)
@@ -105,9 +107,11 @@ static void mdio_set(struct mdiobb_ctrl *ctrl, int what)
 		container_of(ctrl, struct mdio_gpio_info, ctrl);
 
 	if (bitbang->mdo)
-		gpio_set_value(bitbang->mdo, what ^ bitbang->mdo_active_low);
+		gpio_set_value_cansleep(bitbang->mdo,
+					what ^ bitbang->mdo_active_low);
 	else
-		gpio_set_value(bitbang->mdio, what ^ bitbang->mdio_active_low);
+		gpio_set_value_cansleep(bitbang->mdio,
+					what ^ bitbang->mdio_active_low);
 }
 
 static void mdc_set(struct mdiobb_ctrl *ctrl, int what)
@@ -115,7 +119,7 @@ static void mdc_set(struct mdiobb_ctrl *ctrl, int what)
 	struct mdio_gpio_info *bitbang =
 		container_of(ctrl, struct mdio_gpio_info, ctrl);
 
-	gpio_set_value(bitbang->mdc, what ^ bitbang->mdc_active_low);
+	gpio_set_value_cansleep(bitbang->mdc, what ^ bitbang->mdc_active_low);
 }
 
 static struct mdiobb_ops mdio_gpio_ops = {
-- 
2.3.5

Re: [PATCH] net: mdio-gpio: support access that may sleep

[David Miller]

From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Wed, 22 Apr 2015 13:06:54 -0400

> Some systems using mdio-gpio may use gpio on message based busses, which
> require sleeping (e.g. gpio from an I2C I/O expander).
> 
> Since this driver does not use IRQ handler, it is safe to use the
> _cansleep suffixed gpio accessors.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Since this is down underneath the layer of an MII bus, you cannot
universally say that these routines are always called in a sleepable
context.

The PHY layer, and the driver itself above that, might call these
routines from timers, interruptes etc.

In fact, since the whole point of this driver is to provide a specific
implementation for programming registers over an MII bus, it's quite
rediculuous to say that just because interrupts are not used in this
implementation it means that sleeping is always valid.

You have to look at all of the (real and potential) users, all the way
up into the specific ethernet drivers.

Sorry I'm not applying this.

Re: [PATCH] net: mdio-gpio: support access that may sleep

[Florian Fainelli]

On 24/04/15 08:04, David Miller wrote:
> From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> Date: Wed, 22 Apr 2015 13:06:54 -0400
> 
>> Some systems using mdio-gpio may use gpio on message based busses, which
>> require sleeping (e.g. gpio from an I2C I/O expander).
>>
>> Since this driver does not use IRQ handler, it is safe to use the
>> _cansleep suffixed gpio accessors.
>>
>> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> 
> Since this is down underneath the layer of an MII bus, you cannot
> universally say that these routines are always called in a sleepable
> context.
> 
> The PHY layer, and the driver itself above that, might call these
> routines from timers, interruptes etc.

The PHY library calls these routines from its state machine workqueue
for that reason, or from process context (when invoked via ethtool
ioctl). The only special case is phy_mac_interrupt() which is callable
from interrupt context, but schedules the state machine workqueue anyway
to circumvent the "in-interrupt" context.

If we were not doing that, there would be a number of things broken, for
instance the per-MDIO bus mutex would not protect us from anything.

> 
> In fact, since the whole point of this driver is to provide a specific
> implementation for programming registers over an MII bus, it's quite
> rediculuous to say that just because interrupts are not used in this
> implementation it means that sleeping is always valid.
> 
> You have to look at all of the (real and potential) users, all the way
> up into the specific ethernet drivers.

It seems to me like this patch in itself is ok, but if there are
particular drivers you believe are at risk, then yes, we definitively
need to audit those.
-- 
Florian

Re: [PATCH] net: mdio-gpio: support access that may sleep

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Fri, 24 Apr 2015 08:56:34 -0700

> On 24/04/15 08:04, David Miller wrote:
>> From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>> Date: Wed, 22 Apr 2015 13:06:54 -0400
>> 
>>> Some systems using mdio-gpio may use gpio on message based busses, which
>>> require sleeping (e.g. gpio from an I2C I/O expander).
>>>
>>> Since this driver does not use IRQ handler, it is safe to use the
>>> _cansleep suffixed gpio accessors.
>>>
>>> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>> 
>> Since this is down underneath the layer of an MII bus, you cannot
>> universally say that these routines are always called in a sleepable
>> context.
>> 
>> The PHY layer, and the driver itself above that, might call these
>> routines from timers, interruptes etc.
> 
> The PHY library calls these routines from its state machine workqueue
> for that reason, or from process context (when invoked via ethtool
> ioctl). The only special case is phy_mac_interrupt() which is callable
> from interrupt context, but schedules the state machine workqueue anyway
> to circumvent the "in-interrupt" context.
> 
> If we were not doing that, there would be a number of things broken, for
> instance the per-MDIO bus mutex would not protect us from anything.

Does the link state polling timer use a workqueue in this manner as
well?

Re: [PATCH] net: mdio-gpio: support access that may sleep

[Florian Fainelli]

On 24/04/15 09:01, David Miller wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Fri, 24 Apr 2015 08:56:34 -0700
> 
>> On 24/04/15 08:04, David Miller wrote:
>>> From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>>> Date: Wed, 22 Apr 2015 13:06:54 -0400
>>>
>>>> Some systems using mdio-gpio may use gpio on message based busses, which
>>>> require sleeping (e.g. gpio from an I2C I/O expander).
>>>>
>>>> Since this driver does not use IRQ handler, it is safe to use the
>>>> _cansleep suffixed gpio accessors.
>>>>
>>>> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>>>
>>> Since this is down underneath the layer of an MII bus, you cannot
>>> universally say that these routines are always called in a sleepable
>>> context.
>>>
>>> The PHY layer, and the driver itself above that, might call these
>>> routines from timers, interruptes etc.
>>
>> The PHY library calls these routines from its state machine workqueue
>> for that reason, or from process context (when invoked via ethtool
>> ioctl). The only special case is phy_mac_interrupt() which is callable
>> from interrupt context, but schedules the state machine workqueue anyway
>> to circumvent the "in-interrupt" context.
>>
>> If we were not doing that, there would be a number of things broken, for
>> instance the per-MDIO bus mutex would not protect us from anything.
> 
> Does the link state polling timer use a workqueue in this manner as
> well?

Yes, the state machine re-schedules its own delayed workqueue at the end
of its state processing, no timer/hrtimer is used.
-- 
Florian

Re: [PATCH] net: mdio-gpio: support access that may sleep

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Fri, 24 Apr 2015 09:19:55 -0700

> On 24/04/15 09:01, David Miller wrote:
>> From: Florian Fainelli <f.fainelli@gmail.com>
>> Date: Fri, 24 Apr 2015 08:56:34 -0700
>> 
>>> On 24/04/15 08:04, David Miller wrote:
>>>> From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>>>> Date: Wed, 22 Apr 2015 13:06:54 -0400
>>>>
>>>>> Some systems using mdio-gpio may use gpio on message based busses, which
>>>>> require sleeping (e.g. gpio from an I2C I/O expander).
>>>>>
>>>>> Since this driver does not use IRQ handler, it is safe to use the
>>>>> _cansleep suffixed gpio accessors.
>>>>>
>>>>> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>>>>
>>>> Since this is down underneath the layer of an MII bus, you cannot
>>>> universally say that these routines are always called in a sleepable
>>>> context.
>>>>
>>>> The PHY layer, and the driver itself above that, might call these
>>>> routines from timers, interruptes etc.
>>>
>>> The PHY library calls these routines from its state machine workqueue
>>> for that reason, or from process context (when invoked via ethtool
>>> ioctl). The only special case is phy_mac_interrupt() which is callable
>>> from interrupt context, but schedules the state machine workqueue anyway
>>> to circumvent the "in-interrupt" context.
>>>
>>> If we were not doing that, there would be a number of things broken, for
>>> instance the per-MDIO bus mutex would not protect us from anything.
>> 
>> Does the link state polling timer use a workqueue in this manner as
>> well?
> 
> Yes, the state machine re-schedules its own delayed workqueue at the end
> of its state processing, no timer/hrtimer is used.

Ok, thanks for explaining.

I'm apply this patch, therefore, thanks.

Re: [PATCH] net: mdio-gpio: support access that may sleep

[Sergei Shtylyov]

On 04/24/2015 06:56 PM, Florian Fainelli wrote:

>>> Some systems using mdio-gpio may use gpio on message based busses, which
>>> require sleeping (e.g. gpio from an I2C I/O expander).

>>> Since this driver does not use IRQ handler, it is safe to use the
>>> _cansleep suffixed gpio accessors.

>>> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

>> Since this is down underneath the layer of an MII bus, you cannot
>> universally say that these routines are always called in a sleepable
>> context.

>> The PHY layer, and the driver itself above that, might call these
>> routines from timers, interruptes etc.

> The PHY library calls these routines from its state machine workqueue
> for that reason, or from process context (when invoked via ethtool
> ioctl). The only special case is phy_mac_interrupt() which is callable
> from interrupt context,

    It is not (as we have discussed recently) -- cancel_work_sync() may sleep.

WBR, Sergei

Re: [PATCH] net: mdio-gpio: support access that may sleep

[Florian Fainelli]

On 24/04/15 10:25, Sergei Shtylyov wrote:
> On 04/24/2015 06:56 PM, Florian Fainelli wrote:
> 
>>>> Some systems using mdio-gpio may use gpio on message based busses,
>>>> which
>>>> require sleeping (e.g. gpio from an I2C I/O expander).
> 
>>>> Since this driver does not use IRQ handler, it is safe to use the
>>>> _cansleep suffixed gpio accessors.
> 
>>>> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> 
>>> Since this is down underneath the layer of an MII bus, you cannot
>>> universally say that these routines are always called in a sleepable
>>> context.
> 
>>> The PHY layer, and the driver itself above that, might call these
>>> routines from timers, interruptes etc.
> 
>> The PHY library calls these routines from its state machine workqueue
>> for that reason, or from process context (when invoked via ethtool
>> ioctl). The only special case is phy_mac_interrupt() which is callable
>> from interrupt context,
> 
>    It is not (as we have discussed recently) -- cancel_work_sync() may
> sleep.

True, but that does not invalidate my comment, I meant to write that
this is the only function that you *might* potentially want to call from
interrupt context, and yet it does not trigger low-level I/O accesses to
the underlying MDIO bus, but instead uses the PHY library state machine
workqueue to do that.

Thanks for the reminder though, that needs fixing ;)
--
Florian

Re: [PATCH] net: mdio-gpio: support access that may sleep

[Sergei Shtylyov]

On 04/24/2015 08:36 PM, Florian Fainelli wrote:

>>>>> Some systems using mdio-gpio may use gpio on message based busses,
>>>>> which
>>>>> require sleeping (e.g. gpio from an I2C I/O expander).

>>>>> Since this driver does not use IRQ handler, it is safe to use the
>>>>> _cansleep suffixed gpio accessors.

>>>>> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

>>>> Since this is down underneath the layer of an MII bus, you cannot
>>>> universally say that these routines are always called in a sleepable
>>>> context.

>>>> The PHY layer, and the driver itself above that, might call these
>>>> routines from timers, interruptes etc.

>>> The PHY library calls these routines from its state machine workqueue
>>> for that reason, or from process context (when invoked via ethtool
>>> ioctl). The only special case is phy_mac_interrupt() which is callable
>>> from interrupt context,

>>     It is not (as we have discussed recently) -- cancel_work_sync() may
>> sleep.

> True, but that does not invalidate my comment, I meant to write that
> this is the only function that you *might* potentially want to call from
> interrupt context,

    Sure, I did want that. :-)

> and yet it does not trigger low-level I/O accesses to
> the underlying MDIO bus, but instead uses the PHY library state machine
> workqueue to do that.

    That's so.

> Thanks for the reminder though, that needs fixing ;)

    Hopefully it's fixable... I had to use GPIO interrupt for AVB_PHY_INT pin 
since I didn't want to create a thread just to call phy_mac_interrupt().

> --
> Florian

WBR, Sergei