| Message ID | 506ae10e-5d93-f64e-a615-c320010a5529@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | ethtool: add support for Fast Link Down as new PHY tunable | expand |
On Sun, Mar 24, 2019 at 04:02:43PM +0100, Heiner Kallweit wrote: > This adds support for Fast Link Down as new PHY tunable. > Fast Link Down reduces the time until a link down event is reported > for 1000BaseT. According to the standard it's 750ms what is too long > for several use cases. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > include/uapi/linux/ethtool.h | 4 ++++ > net/core/ethtool.c | 2 ++ > 2 files changed, 6 insertions(+) > > diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h > index 3652b239d..2c7136adc 100644 > --- a/include/uapi/linux/ethtool.h > +++ b/include/uapi/linux/ethtool.h > @@ -252,9 +252,13 @@ struct ethtool_tunable { > #define DOWNSHIFT_DEV_DEFAULT_COUNT 0xff > #define DOWNSHIFT_DEV_DISABLE 0 > > +#define ETHTOOL_PHY_FAST_LINK_DOWN_ON 0 > +#define ETHTOOL_PHY_FAST_LINK_DOWN_OFF 0xff > + > enum phy_tunable_id { > ETHTOOL_PHY_ID_UNSPEC, > ETHTOOL_PHY_DOWNSHIFT, > + ETHTOOL_PHY_FAST_LINK_DOWN, > /* > * Add your fresh new phy tunable attribute above and remember to update > * phy_tunable_strings[] in net/core/ethtool.c It would be nice to have a short summary around here explaining how is the value interpreted. While it's obvious from the second patch, one shouldn't have to go into driver specific implementation to find out. I also wonder if the range 0-254 ms is sufficient. Would it be possible that there is some other hardware which would support e.g. 300 ms? Michal Kubecek > diff --git a/net/core/ethtool.c b/net/core/ethtool.c > index b1eb32419..387d67eb7 100644 > --- a/net/core/ethtool.c > +++ b/net/core/ethtool.c > @@ -136,6 +136,7 @@ static const char > phy_tunable_strings[__ETHTOOL_PHY_TUNABLE_COUNT][ETH_GSTRING_LEN] = { > [ETHTOOL_ID_UNSPEC] = "Unspec", > [ETHTOOL_PHY_DOWNSHIFT] = "phy-downshift", > + [ETHTOOL_PHY_FAST_LINK_DOWN] = "phy-fast-link-down", > }; > > static int ethtool_get_features(struct net_device *dev, void __user *useraddr) > @@ -2432,6 +2433,7 @@ static int ethtool_phy_tunable_valid(const struct ethtool_tunable *tuna) > { > switch (tuna->id) { > case ETHTOOL_PHY_DOWNSHIFT: > + case ETHTOOL_PHY_FAST_LINK_DOWN: > if (tuna->len != sizeof(u8) || > tuna->type_id != ETHTOOL_TUNABLE_U8) > return -EINVAL; > -- > 2.21.0 > >
On 25.03.2019 18:49, Michal Kubecek wrote: > On Sun, Mar 24, 2019 at 04:02:43PM +0100, Heiner Kallweit wrote: >> This adds support for Fast Link Down as new PHY tunable. >> Fast Link Down reduces the time until a link down event is reported >> for 1000BaseT. According to the standard it's 750ms what is too long >> for several use cases. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> include/uapi/linux/ethtool.h | 4 ++++ >> net/core/ethtool.c | 2 ++ >> 2 files changed, 6 insertions(+) >> >> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h >> index 3652b239d..2c7136adc 100644 >> --- a/include/uapi/linux/ethtool.h >> +++ b/include/uapi/linux/ethtool.h >> @@ -252,9 +252,13 @@ struct ethtool_tunable { >> #define DOWNSHIFT_DEV_DEFAULT_COUNT 0xff >> #define DOWNSHIFT_DEV_DISABLE 0 >> >> +#define ETHTOOL_PHY_FAST_LINK_DOWN_ON 0 >> +#define ETHTOOL_PHY_FAST_LINK_DOWN_OFF 0xff >> + >> enum phy_tunable_id { >> ETHTOOL_PHY_ID_UNSPEC, >> ETHTOOL_PHY_DOWNSHIFT, >> + ETHTOOL_PHY_FAST_LINK_DOWN, >> /* >> * Add your fresh new phy tunable attribute above and remember to update >> * phy_tunable_strings[] in net/core/ethtool.c > > It would be nice to have a short summary around here explaining how is > the value interpreted. While it's obvious from the second patch, one > shouldn't have to go into driver specific implementation to find out. > OK > I also wonder if the range 0-254 ms is sufficient. Would it be possible > that there is some other hardware which would support e.g. 300 ms? > The relevant use cases (according to the Marvell spec) require link down recognition in <50ms. Something >200ms I think wouldn't be considered as "fast" in general. And for something completely different, all mails to John are bounced currently: rejected because 209.85.128.65 is in a black list at 0spam.fusionzero.com Received: from mail-wm1-f65.google.com I tried from another email address, but same result. > Michal Kubecek > Heiner >> diff --git a/net/core/ethtool.c b/net/core/ethtool.c >> index b1eb32419..387d67eb7 100644 >> --- a/net/core/ethtool.c >> +++ b/net/core/ethtool.c >> @@ -136,6 +136,7 @@ static const char >> phy_tunable_strings[__ETHTOOL_PHY_TUNABLE_COUNT][ETH_GSTRING_LEN] = { >> [ETHTOOL_ID_UNSPEC] = "Unspec", >> [ETHTOOL_PHY_DOWNSHIFT] = "phy-downshift", >> + [ETHTOOL_PHY_FAST_LINK_DOWN] = "phy-fast-link-down", >> }; >> >> static int ethtool_get_features(struct net_device *dev, void __user *useraddr) >> @@ -2432,6 +2433,7 @@ static int ethtool_phy_tunable_valid(const struct ethtool_tunable *tuna) >> { >> switch (tuna->id) { >> case ETHTOOL_PHY_DOWNSHIFT: >> + case ETHTOOL_PHY_FAST_LINK_DOWN: >> if (tuna->len != sizeof(u8) || >> tuna->type_id != ETHTOOL_TUNABLE_U8) >> return -EINVAL; >> -- >> 2.21.0 >> >> >
> > +#define ETHTOOL_PHY_FAST_LINK_DOWN_ON 0 > > +#define ETHTOOL_PHY_FAST_LINK_DOWN_OFF 0xff > > + > > enum phy_tunable_id { > > ETHTOOL_PHY_ID_UNSPEC, > > ETHTOOL_PHY_DOWNSHIFT, > > + ETHTOOL_PHY_FAST_LINK_DOWN, > > /* > > * Add your fresh new phy tunable attribute above and remember to update > > * phy_tunable_strings[] in net/core/ethtool.c > > It would be nice to have a short summary around here explaining how is > the value interpreted. While it's obvious from the second patch, one > shouldn't have to go into driver specific implementation to find out. > > I also wonder if the range 0-254 ms is sufficient. Would it be possible > that there is some other hardware which would support e.g. 300 ms? The default, as defined by the 802.3 standard, is i think 750ms. The Marvel PHY also supports 50ms, 20ms and 0ms, if i remember correctly. One problem we have here is discovery. How does the user find out the values the driver supports. For a netlink socket API, extended errors could be used to pass back a string indicating the supported values. For the old ethtool, i think all we have is -EINVAL, which is not very helpful. Andrew
On Tue, Mar 26, 2019 at 09:24:38AM +0100, Andrew Lunn wrote: > > > +#define ETHTOOL_PHY_FAST_LINK_DOWN_ON 0 > > > +#define ETHTOOL_PHY_FAST_LINK_DOWN_OFF 0xff > > > + > > > enum phy_tunable_id { > > > ETHTOOL_PHY_ID_UNSPEC, > > > ETHTOOL_PHY_DOWNSHIFT, > > > + ETHTOOL_PHY_FAST_LINK_DOWN, > > > /* > > > * Add your fresh new phy tunable attribute above and remember to update > > > * phy_tunable_strings[] in net/core/ethtool.c > > > > It would be nice to have a short summary around here explaining how is > > the value interpreted. While it's obvious from the second patch, one > > shouldn't have to go into driver specific implementation to find out. > > > > I also wonder if the range 0-254 ms is sufficient. Would it be possible > > that there is some other hardware which would support e.g. 300 ms? > > The default, as defined by the 802.3 standard, is i think 750ms. > > The Marvel PHY also supports 50ms, 20ms and 0ms, if i remember > correctly. The reason why I asked about this is that PHY tunables are supposed to be universal, not specific to a particular driver, and there might be other hardware supporting the feature with different set of supported values. > One problem we have here is discovery. How does the user find out the > values the driver supports. For a netlink socket API, extended errors > could be used to pass back a string indicating the supported > values. For the old ethtool, i think all we have is -EINVAL, which is > not very helpful. As supported values are determined by the driver, we would need to pass extack to ethtool_ops handler - but that is something we will want to do eventually (ideally, for all ethtool_ops handlers). AFAICS the implementation in patch 2/2 rounds user supplied value to closest value supported by hardware so that user doesn't have to guess which values are supported. But it would still deserve a warning via netlink extack, IMHO. Michal
On 26.03.2019 09:24, Andrew Lunn wrote: >>> +#define ETHTOOL_PHY_FAST_LINK_DOWN_ON 0 >>> +#define ETHTOOL_PHY_FAST_LINK_DOWN_OFF 0xff >>> + >>> enum phy_tunable_id { >>> ETHTOOL_PHY_ID_UNSPEC, >>> ETHTOOL_PHY_DOWNSHIFT, >>> + ETHTOOL_PHY_FAST_LINK_DOWN, >>> /* >>> * Add your fresh new phy tunable attribute above and remember to update >>> * phy_tunable_strings[] in net/core/ethtool.c >> >> It would be nice to have a short summary around here explaining how is >> the value interpreted. While it's obvious from the second patch, one >> shouldn't have to go into driver specific implementation to find out. >> >> I also wonder if the range 0-254 ms is sufficient. Would it be possible >> that there is some other hardware which would support e.g. 300 ms? > > The default, as defined by the 802.3 standard, is i think 750ms. > Clause 40. From what I've found this applies to 1000BaseT only. > The Marvel PHY also supports 50ms, 20ms and 0ms, if i remember > correctly. > 0, 10, 20, 40ms (at least for 88E1540 and 88E6390) > One problem we have here is discovery. How does the user find out the > values the driver supports. For a netlink socket API, extended errors > could be used to pass back a string indicating the supported > values. For the old ethtool, i think all we have is -EINVAL, which is > not very helpful. > > Andrew > . > Heiner
On 26.03.2019 10:17, Michal Kubecek wrote: > On Tue, Mar 26, 2019 at 09:24:38AM +0100, Andrew Lunn wrote: >>>> +#define ETHTOOL_PHY_FAST_LINK_DOWN_ON 0 >>>> +#define ETHTOOL_PHY_FAST_LINK_DOWN_OFF 0xff >>>> + >>>> enum phy_tunable_id { >>>> ETHTOOL_PHY_ID_UNSPEC, >>>> ETHTOOL_PHY_DOWNSHIFT, >>>> + ETHTOOL_PHY_FAST_LINK_DOWN, >>>> /* >>>> * Add your fresh new phy tunable attribute above and remember to update >>>> * phy_tunable_strings[] in net/core/ethtool.c >>> >>> It would be nice to have a short summary around here explaining how is >>> the value interpreted. While it's obvious from the second patch, one >>> shouldn't have to go into driver specific implementation to find out. >>> >>> I also wonder if the range 0-254 ms is sufficient. Would it be possible >>> that there is some other hardware which would support e.g. 300 ms? >> >> The default, as defined by the 802.3 standard, is i think 750ms. >> >> The Marvel PHY also supports 50ms, 20ms and 0ms, if i remember >> correctly. > > The reason why I asked about this is that PHY tunables are supposed to > be universal, not specific to a particular driver, and there might be > other hardware supporting the feature with different set of supported > values. > >> One problem we have here is discovery. How does the user find out the >> values the driver supports. For a netlink socket API, extended errors >> could be used to pass back a string indicating the supported >> values. For the old ethtool, i think all we have is -EINVAL, which is >> not very helpful. > > As supported values are determined by the driver, we would need to pass > extack to ethtool_ops handler - but that is something we will want to do > eventually (ideally, for all ethtool_ops handlers). > > AFAICS the implementation in patch 2/2 rounds user supplied value to > closest value supported by hardware so that user doesn't have to guess > which values are supported. But it would still deserve a warning via > netlink extack, IMHO. > Not to confuse Dave with the discussion: This is not about whether the patch is wrong or right, but about how a future architecture based on ethtool-nl could look like. Like Michael said, based on the current ethtool architecture it's simple: Driver will choose closest supported value. When reading back the setting you will get the exact value chosen by the driver. > Michal > . > Heiner
> Not to confuse Dave with the discussion: > This is not about whether the patch is wrong or right, but about how > a future architecture based on ethtool-nl could look like. > > Like Michael said, based on the current ethtool architecture it's simple: > Driver will choose closest supported value. When reading back the setting > you will get the exact value chosen by the driver. Hi Heiner Yes, i read you actual patch later. Rounding down to the nearest seems sensible. When you write the man page patch for ethtool, please document this. Andrew
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h index 3652b239d..2c7136adc 100644 --- a/include/uapi/linux/ethtool.h +++ b/include/uapi/linux/ethtool.h @@ -252,9 +252,13 @@ struct ethtool_tunable { #define DOWNSHIFT_DEV_DEFAULT_COUNT 0xff #define DOWNSHIFT_DEV_DISABLE 0 +#define ETHTOOL_PHY_FAST_LINK_DOWN_ON 0 +#define ETHTOOL_PHY_FAST_LINK_DOWN_OFF 0xff + enum phy_tunable_id { ETHTOOL_PHY_ID_UNSPEC, ETHTOOL_PHY_DOWNSHIFT, + ETHTOOL_PHY_FAST_LINK_DOWN, /* * Add your fresh new phy tunable attribute above and remember to update * phy_tunable_strings[] in net/core/ethtool.c diff --git a/net/core/ethtool.c b/net/core/ethtool.c index b1eb32419..387d67eb7 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -136,6 +136,7 @@ static const char phy_tunable_strings[__ETHTOOL_PHY_TUNABLE_COUNT][ETH_GSTRING_LEN] = { [ETHTOOL_ID_UNSPEC] = "Unspec", [ETHTOOL_PHY_DOWNSHIFT] = "phy-downshift", + [ETHTOOL_PHY_FAST_LINK_DOWN] = "phy-fast-link-down", }; static int ethtool_get_features(struct net_device *dev, void __user *useraddr) @@ -2432,6 +2433,7 @@ static int ethtool_phy_tunable_valid(const struct ethtool_tunable *tuna) { switch (tuna->id) { case ETHTOOL_PHY_DOWNSHIFT: + case ETHTOOL_PHY_FAST_LINK_DOWN: if (tuna->len != sizeof(u8) || tuna->type_id != ETHTOOL_TUNABLE_U8) return -EINVAL;
This adds support for Fast Link Down as new PHY tunable. Fast Link Down reduces the time until a link down event is reported for 1000BaseT. According to the standard it's 750ms what is too long for several use cases. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- include/uapi/linux/ethtool.h | 4 ++++ net/core/ethtool.c | 2 ++ 2 files changed, 6 insertions(+)