* [PATCH] net/bonding: fix link properties with autoneg @ 2018-02-13 22:54 Chas Williams 2018-02-13 23:03 ` Thomas Monjalon 2018-04-16 8:06 ` Matan Azrad 0 siblings, 2 replies; 7+ messages in thread From: Chas Williams @ 2018-02-13 22:54 UTC (permalink / raw) To: dev; +Cc: declan.doherty, Chas Williams From: Chas Williams <chas3@att.com> If a link is carrier down and using autonegotiation, then the PMD may not have detected a speed yet. In this case the best we can do is ignore the link speed and duplex since they aren't valid. To be completely correct, there should be additional checks to prevent a slave that negotiates a different speed from being activated. Signed-off-by: Chas Williams <chas3@att.com> --- drivers/net/bonding/rte_eth_bond_pmd.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c index 92ad688..5559879 100644 --- a/drivers/net/bonding/rte_eth_bond_pmd.c +++ b/drivers/net/bonding/rte_eth_bond_pmd.c @@ -1545,9 +1545,10 @@ link_properties_valid(struct rte_eth_dev *ethdev, if (bond_ctx->mode == BONDING_MODE_8023AD) { struct rte_eth_link *bond_link = &bond_ctx->mode4.slave_link; - if (bond_link->link_duplex != slave_link->link_duplex || - bond_link->link_autoneg != slave_link->link_autoneg || - bond_link->link_speed != slave_link->link_speed) + if (bond_link->link_autoneg != slave_link->link_autoneg || + (bond_link->link_autoneg != ETH_LINK_AUTONEG && + (bond_link->link_duplex != slave_link->link_duplex || + bond_link->link_speed != slave_link->link_speed))) return -1; } -- 2.9.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] net/bonding: fix link properties with autoneg 2018-02-13 22:54 [PATCH] net/bonding: fix link properties with autoneg Chas Williams @ 2018-02-13 23:03 ` Thomas Monjalon 2018-04-16 8:06 ` Matan Azrad 1 sibling, 0 replies; 7+ messages in thread From: Thomas Monjalon @ 2018-02-13 23:03 UTC (permalink / raw) To: Chas Williams, Chas Williams; +Cc: dev, declan.doherty 13/02/2018 23:54, Chas Williams: > From: Chas Williams <chas3@att.com> > > If a link is carrier down and using autonegotiation, then the PMD may not > have detected a speed yet. In this case the best we can do is ignore the > link speed and duplex since they aren't valid. To be completely correct, > there should be additional checks to prevent a slave that negotiates a > different speed from being activated. > > Signed-off-by: Chas Williams <chas3@att.com> Please add Fixes line to all your fix patches. Thanks ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net/bonding: fix link properties with autoneg 2018-02-13 22:54 [PATCH] net/bonding: fix link properties with autoneg Chas Williams 2018-02-13 23:03 ` Thomas Monjalon @ 2018-04-16 8:06 ` Matan Azrad 2018-04-16 16:44 ` Chas Williams 1 sibling, 1 reply; 7+ messages in thread From: Matan Azrad @ 2018-04-16 8:06 UTC (permalink / raw) To: Chas Williams, dev; +Cc: declan.doherty, Chas Williams Hi Chas From: Chas Williams, Wednesday, February 14, 2018 12:55 AM > If a link is carrier down and using autonegotiation, then the PMD may not > have detected a speed yet. In this case the best we can do is ignore the link > speed and duplex since they aren't valid. Ok for this. > To be completely correct, there > should be additional checks to prevent a slave that negotiates a different > speed from being activated. Looks like every changing in the link properties should cause LSC interrupt. In the bonding LCS interrupt you could handle and to deactivate the device. Also you should deal with the case of the first slave, what is happen if the first slave has invalid link properties? How can you know that the speed\duplex_mode is invalid? Are we sure LACP mode can run with auto negotiation? > > Signed-off-by: Chas Williams <chas3@att.com> > --- > drivers/net/bonding/rte_eth_bond_pmd.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c > b/drivers/net/bonding/rte_eth_bond_pmd.c > index 92ad688..5559879 100644 > --- a/drivers/net/bonding/rte_eth_bond_pmd.c > +++ b/drivers/net/bonding/rte_eth_bond_pmd.c > @@ -1545,9 +1545,10 @@ link_properties_valid(struct rte_eth_dev > *ethdev, > if (bond_ctx->mode == BONDING_MODE_8023AD) { > struct rte_eth_link *bond_link = &bond_ctx- > >mode4.slave_link; > > - if (bond_link->link_duplex != slave_link->link_duplex || > - bond_link->link_autoneg != slave_link->link_autoneg > || > - bond_link->link_speed != slave_link->link_speed) > + if (bond_link->link_autoneg != slave_link->link_autoneg || > + (bond_link->link_autoneg != ETH_LINK_AUTONEG && > + (bond_link->link_duplex != slave_link->link_duplex || > + bond_link->link_speed != slave_link->link_speed))) > return -1; > } > > -- > 2.9.5 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net/bonding: fix link properties with autoneg 2018-04-16 8:06 ` Matan Azrad @ 2018-04-16 16:44 ` Chas Williams 2018-04-16 19:09 ` Matan Azrad 0 siblings, 1 reply; 7+ messages in thread From: Chas Williams @ 2018-04-16 16:44 UTC (permalink / raw) To: Matan Azrad; +Cc: dev, declan.doherty, Chas Williams On Mon, Apr 16, 2018 at 4:06 AM, Matan Azrad <matan@mellanox.com> wrote: > Hi Chas > > From: Chas Williams, Wednesday, February 14, 2018 12:55 AM >> If a link is carrier down and using autonegotiation, then the PMD may not >> have detected a speed yet. In this case the best we can do is ignore the link >> speed and duplex since they aren't valid. > > Ok for this. > >> To be completely correct, there >> should be additional checks to prevent a slave that negotiates a different >> speed from being activated. > > Looks like every changing in the link properties should cause LSC interrupt. > In the bonding LCS interrupt you could handle and to deactivate the device. > Also you should deal with the case of the first slave, what is happen if the first slave has invalid link properties? > How can you know that the speed\duplex_mode is invalid? > Are we sure LACP mode can run with auto negotiation? Yes, I am pretty sure bonding doesn't get this right when the interfaces aren't link up. While what bonding is doing is likely wrong, it doesn't mean that the behavior of the PMDs are correct in leaving the link_status unset until the first LSC interrupt. I plan to get around to looking at this bonding problem in a little bit. Luckily it seems that we always tend to get matched links and even if bonding is advertising the wrong aggregate speed and duplex we are find for now. It wouldn't pass close inspection by a protocol analyzer though. >> >> Signed-off-by: Chas Williams <chas3@att.com> >> --- >> drivers/net/bonding/rte_eth_bond_pmd.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c >> b/drivers/net/bonding/rte_eth_bond_pmd.c >> index 92ad688..5559879 100644 >> --- a/drivers/net/bonding/rte_eth_bond_pmd.c >> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c >> @@ -1545,9 +1545,10 @@ link_properties_valid(struct rte_eth_dev >> *ethdev, >> if (bond_ctx->mode == BONDING_MODE_8023AD) { >> struct rte_eth_link *bond_link = &bond_ctx- >> >mode4.slave_link; >> >> - if (bond_link->link_duplex != slave_link->link_duplex || >> - bond_link->link_autoneg != slave_link->link_autoneg >> || >> - bond_link->link_speed != slave_link->link_speed) >> + if (bond_link->link_autoneg != slave_link->link_autoneg || >> + (bond_link->link_autoneg != ETH_LINK_AUTONEG && >> + (bond_link->link_duplex != slave_link->link_duplex || >> + bond_link->link_speed != slave_link->link_speed))) >> return -1; >> } >> >> -- >> 2.9.5 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net/bonding: fix link properties with autoneg 2018-04-16 16:44 ` Chas Williams @ 2018-04-16 19:09 ` Matan Azrad 2018-06-14 17:04 ` Ferruh Yigit 0 siblings, 1 reply; 7+ messages in thread From: Matan Azrad @ 2018-04-16 19:09 UTC (permalink / raw) To: Chas Williams; +Cc: dev, declan.doherty, Chas Williams Hi Chas From: Chas Williams, Monday, April 16, 2018 7:44 PM > On Mon, Apr 16, 2018 at 4:06 AM, Matan Azrad <matan@mellanox.com> > wrote: > > Hi Chas > > > > From: Chas Williams, Wednesday, February 14, 2018 12:55 AM > >> If a link is carrier down and using autonegotiation, then the PMD may > >> not have detected a speed yet. In this case the best we can do is > >> ignore the link speed and duplex since they aren't valid. > > > > Ok for this. > > > >> To be completely correct, there > >> should be additional checks to prevent a slave that negotiates a > >> different speed from being activated. > > > > Looks like every changing in the link properties should cause LSC interrupt. > > In the bonding LCS interrupt you could handle and to deactivate the device. > > Also you should deal with the case of the first slave, what is happen if the > first slave has invalid link properties? > > How can you know that the speed\duplex_mode is invalid? > > Are we sure LACP mode can run with auto negotiation? > > Yes, I am pretty sure bonding doesn't get this right when the interfaces > aren't link up. While what bonding is doing is likely wrong, it doesn't mean > that the behavior of the PMDs are correct in leaving the link_status unset > until the first LSC interrupt. > > I plan to get around to looking at this bonding problem in a little bit. Luckily it > seems that we always tend to get matched links and even if bonding is > advertising the wrong aggregate speed and duplex we are find for now. It > wouldn't pass close inspection by a protocol analyzer though. > So, Are you going to fix it, If no, I think you can open a bug in Bugzilla. > >> > >> Signed-off-by: Chas Williams <chas3@att.com> > >> --- > >> drivers/net/bonding/rte_eth_bond_pmd.c | 7 ++++--- > >> 1 file changed, 4 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c > >> b/drivers/net/bonding/rte_eth_bond_pmd.c > >> index 92ad688..5559879 100644 > >> --- a/drivers/net/bonding/rte_eth_bond_pmd.c > >> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c > >> @@ -1545,9 +1545,10 @@ link_properties_valid(struct rte_eth_dev > >> *ethdev, > >> if (bond_ctx->mode == BONDING_MODE_8023AD) { > >> struct rte_eth_link *bond_link = &bond_ctx- > >> >mode4.slave_link; > >> > >> - if (bond_link->link_duplex != slave_link->link_duplex || > >> - bond_link->link_autoneg != slave_link->link_autoneg > >> || > >> - bond_link->link_speed != slave_link->link_speed) > >> + if (bond_link->link_autoneg != slave_link->link_autoneg || > >> + (bond_link->link_autoneg != ETH_LINK_AUTONEG && > >> + (bond_link->link_duplex != slave_link->link_duplex || > >> + bond_link->link_speed != > >> + slave_link->link_speed))) > >> return -1; > >> } > >> > >> -- > >> 2.9.5 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net/bonding: fix link properties with autoneg 2018-04-16 19:09 ` Matan Azrad @ 2018-06-14 17:04 ` Ferruh Yigit 2018-06-16 17:29 ` Chas Williams 0 siblings, 1 reply; 7+ messages in thread From: Ferruh Yigit @ 2018-06-14 17:04 UTC (permalink / raw) To: Matan Azrad, Chas Williams; +Cc: dev, declan.doherty, Chas Williams On 4/16/2018 8:09 PM, Matan Azrad wrote: > Hi Chas > > From: Chas Williams, Monday, April 16, 2018 7:44 PM >> On Mon, Apr 16, 2018 at 4:06 AM, Matan Azrad <matan@mellanox.com> >> wrote: >>> Hi Chas >>> >>> From: Chas Williams, Wednesday, February 14, 2018 12:55 AM >>>> If a link is carrier down and using autonegotiation, then the PMD may >>>> not have detected a speed yet. In this case the best we can do is >>>> ignore the link speed and duplex since they aren't valid. >>> >>> Ok for this. >>> >>>> To be completely correct, there >>>> should be additional checks to prevent a slave that negotiates a >>>> different speed from being activated. >>> >>> Looks like every changing in the link properties should cause LSC interrupt. >>> In the bonding LCS interrupt you could handle and to deactivate the device. >>> Also you should deal with the case of the first slave, what is happen if the >> first slave has invalid link properties? >>> How can you know that the speed\duplex_mode is invalid? >>> Are we sure LACP mode can run with auto negotiation? >> >> Yes, I am pretty sure bonding doesn't get this right when the interfaces >> aren't link up. While what bonding is doing is likely wrong, it doesn't mean >> that the behavior of the PMDs are correct in leaving the link_status unset >> until the first LSC interrupt. >> >> I plan to get around to looking at this bonding problem in a little bit. Luckily it >> seems that we always tend to get matched links and even if bonding is >> advertising the wrong aggregate speed and duplex we are find for now. It >> wouldn't pass close inspection by a protocol analyzer though. >> > > So, Are you going to fix it, > If no, I think you can open a bug in Bugzilla. Hi Matan, Chas, What is the latest status of the patch? And I guess there is another issue as well discussed here, is it still valid? Thanks, ferruh ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net/bonding: fix link properties with autoneg 2018-06-14 17:04 ` Ferruh Yigit @ 2018-06-16 17:29 ` Chas Williams 0 siblings, 0 replies; 7+ messages in thread From: Chas Williams @ 2018-06-16 17:29 UTC (permalink / raw) To: Ferruh Yigit; +Cc: Matan Azrad, dev, Declan Doherty, Chas Williams On Thu, Jun 14, 2018 at 1:04 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote: > On 4/16/2018 8:09 PM, Matan Azrad wrote: > > Hi Chas > > > > From: Chas Williams, Monday, April 16, 2018 7:44 PM > >> On Mon, Apr 16, 2018 at 4:06 AM, Matan Azrad <matan@mellanox.com> > >> wrote: > >>> Hi Chas > >>> > >>> From: Chas Williams, Wednesday, February 14, 2018 12:55 AM > >>>> If a link is carrier down and using autonegotiation, then the PMD may > >>>> not have detected a speed yet. In this case the best we can do is > >>>> ignore the link speed and duplex since they aren't valid. > >>> > >>> Ok for this. > >>> > >>>> To be completely correct, there > >>>> should be additional checks to prevent a slave that negotiates a > >>>> different speed from being activated. > >>> > >>> Looks like every changing in the link properties should cause LSC > interrupt. > >>> In the bonding LCS interrupt you could handle and to deactivate the > device. > >>> Also you should deal with the case of the first slave, what is happen > if the > >> first slave has invalid link properties? > >>> How can you know that the speed\duplex_mode is invalid? > >>> Are we sure LACP mode can run with auto negotiation? > >> > >> Yes, I am pretty sure bonding doesn't get this right when the interfaces > >> aren't link up. While what bonding is doing is likely wrong, it > doesn't mean > >> that the behavior of the PMDs are correct in leaving the link_status > unset > >> until the first LSC interrupt. > >> > >> I plan to get around to looking at this bonding problem in a little > bit. Luckily it > >> seems that we always tend to get matched links and even if bonding is > >> advertising the wrong aggregate speed and duplex we are find for now. > It > >> wouldn't pass close inspection by a protocol analyzer though. > >> > > > > So, Are you going to fix it, > > If no, I think you can open a bug in Bugzilla. > > Hi Matan, Chas, > > What is the latest status of the patch? > And I guess there is another issue as well discussed here, is it still > valid? > > Thanks, > ferruh > I think this issue is better addressed by http://dpdk.org/dev/patchwork/patch/40572/ There's just a little more cleanup that needs to be done in that patch. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-06-16 17:29 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-13 22:54 [PATCH] net/bonding: fix link properties with autoneg Chas Williams 2018-02-13 23:03 ` Thomas Monjalon 2018-04-16 8:06 ` Matan Azrad 2018-04-16 16:44 ` Chas Williams 2018-04-16 19:09 ` Matan Azrad 2018-06-14 17:04 ` Ferruh Yigit 2018-06-16 17:29 ` Chas Williams
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.