| Message ID | 1429722414-18173-1-git-send-email-vivien.didelot@savoirfairelinux.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 = {
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(-)