| Message ID | 20230516134447.193511-3-mateusz.palczewski@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fix for bond 802.3ad mode with VFs | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 11 this patch: 11 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | success | Errors and warnings before: 8 this patch: 8 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 11 this patch: 11 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 31 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
Mateusz Palczewski <mateusz.palczewski@intel.com> wrote: >From: Sebastian Basierski <sebastianx.basierski@intel.com> > >Right now bonding driver checks if link is ready once. >VF interface takes a little more time to get ready than PF, >so driver needs to wait for it to be ready. >1000ms delay was set, if VF link will not be set within given amount >of time, for sure problems should be investigated elsewhere. Why is the "updelay" mechanism that's already available insufficient for this purpose? Even without updelay, I'd expect the behavior to simply be that the carrier state flaps once or twice (because the VF is delayed in asserting carrier up). This is reflecting reality; I'm unsure why we would want to hack in an extra delay to cover that up. Regardless of whether updelay handles this case or not, adding a 1 second busy wait loop as this patch does is not a reasonable implementation. This would cause a 1 second stall in the link state check for every bond interface that is carrier down. -J >Fixes: b3c898e20b18 ("Revert "bonding: allow carrier and link status to determine link state"") >Signed-off-by: Sebastian Basierski <sebastianx.basierski@intel.com> >Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com> >--- > drivers/net/bonding/bond_main.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >index 710548dbd0c1..6d49fb25969e 100644 >--- a/drivers/net/bonding/bond_main.c >+++ b/drivers/net/bonding/bond_main.c >@@ -736,6 +736,8 @@ const char *bond_slave_link_status(s8 link) > * It'd be nice if there was a good way to tell if a driver supports > * netif_carrier, but there really isn't. > */ >+#define BOND_CARRIER_CHECK_TIMEOUT 1000 >+ > static int bond_check_dev_link(struct bonding *bond, > struct net_device *slave_dev, int reporting) > { >@@ -743,12 +745,22 @@ static int bond_check_dev_link(struct bonding *bond, > int (*ioctl)(struct net_device *, struct ifreq *, int); > struct ifreq ifr; > struct mii_ioctl_data *mii; >+ int delay; > > if (!reporting && !netif_running(slave_dev)) > return 0; > >+ for (delay = 0; delay < BOND_CARRIER_CHECK_TIMEOUT; delay++) { >+ mdelay(1); >+ >+ if (bond->params.use_carrier && >+ netif_carrier_ok(slave_dev)) { >+ return BMSR_LSTATUS; >+ } >+ } >+ > if (bond->params.use_carrier) >- return netif_carrier_ok(slave_dev) ? BMSR_LSTATUS : 0; >+ return 0; > > /* Try to get link status using Ethtool first. */ > if (slave_dev->ethtool_ops->get_link) >-- >2.31.1 > > --- -Jay Vosburgh, jay.vosburgh@canonical.com
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 710548dbd0c1..6d49fb25969e 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -736,6 +736,8 @@ const char *bond_slave_link_status(s8 link) * It'd be nice if there was a good way to tell if a driver supports * netif_carrier, but there really isn't. */ +#define BOND_CARRIER_CHECK_TIMEOUT 1000 + static int bond_check_dev_link(struct bonding *bond, struct net_device *slave_dev, int reporting) { @@ -743,12 +745,22 @@ static int bond_check_dev_link(struct bonding *bond, int (*ioctl)(struct net_device *, struct ifreq *, int); struct ifreq ifr; struct mii_ioctl_data *mii; + int delay; if (!reporting && !netif_running(slave_dev)) return 0; + for (delay = 0; delay < BOND_CARRIER_CHECK_TIMEOUT; delay++) { + mdelay(1); + + if (bond->params.use_carrier && + netif_carrier_ok(slave_dev)) { + return BMSR_LSTATUS; + } + } + if (bond->params.use_carrier) - return netif_carrier_ok(slave_dev) ? BMSR_LSTATUS : 0; + return 0; /* Try to get link status using Ethtool first. */ if (slave_dev->ethtool_ops->get_link)