| Message ID | 20240528092010.439089-1-xiaolei.wang@windriver.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [net] net: stmmac: update priv->speed to SPEED_UNKNOWN when link down | expand |
On Tue, May 28, 2024 at 05:20:10PM +0800, Xiaolei Wang wrote: > The CBS parameter can still be configured when the port is > currently disconnected and link down. This is unreasonable. This sounds like a generic problem. Can the core check the carrier status and error out there? Maybe return a useful extack message. If you do need to return an error code, ENETDOWN seems more appropriate. Andrew
On 5/28/24 21:20, Andrew Lunn wrote: > CAUTION: This email comes from a non Wind River email account! > Do not click links or open attachments unless you recognize the sender and know the content is safe. > > On Tue, May 28, 2024 at 05:20:10PM +0800, Xiaolei Wang wrote: >> The CBS parameter can still be configured when the port is >> currently disconnected and link down. This is unreasonable. > This sounds like a generic problem. Can the core check the carrier > status and error out there? Maybe return a useful extack message. > > If you do need to return an error code, ENETDOWN seems more Currently cbs does not check link status. If ops->ndo_setup_tc() returns failure, there will only be an output of "Specified device failed to setup cbs hardware offload". thanks xiaolei > appropriate. > > Andrew
On Wed, May 29, 2024 at 08:22:01AM +0800, xiaolei wang wrote: > > On 5/28/24 21:20, Andrew Lunn wrote: > > CAUTION: This email comes from a non Wind River email account! > > Do not click links or open attachments unless you recognize the sender and know the content is safe. > > > > On Tue, May 28, 2024 at 05:20:10PM +0800, Xiaolei Wang wrote: > > > The CBS parameter can still be configured when the port is > > > currently disconnected and link down. This is unreasonable. > > This sounds like a generic problem. Can the core check the carrier > > status and error out there? Maybe return a useful extack message. > > > > If you do need to return an error code, ENETDOWN seems more > > Currently cbs does not check link status. If ops->ndo_setup_tc() returns > failure, there will only be an output of "Specified device failed to setup > cbs hardware offload". So it sounds like we should catch this in the core then, not the driver. And cbs_enable_offload() takes an extack, so you can report a user friendly reason for failing, the at the carrier is off. Andrew --- pw-bot: cr
On Tue, May 28, 2024 at 05:20:10PM +0800, Xiaolei Wang wrote: > The CBS parameter can still be configured when the port is > currently disconnected and link down. This is unreasonable. > The current speed_div and ptr parameters depend on the negotiated > speed after uplinking. So When the link is down, update priv->speed > to SPEED_UNKNOWN and an error log should be added. > > Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com> So what happens if stmmac is connected to a PHY that can negotiate with the link partner, it has link up at e.g. 1G speed, one configures CBS, and then the link goes down and comes up at a different speed? I can't see any way in the stmmac driver that this is handled, which makes this feature way more buggy than you're referring to here. It also means that with your patch, if one attempts to configure CBS when the link is down, it will fail. To me, commit 1f705bc61aee ("net: stmmac: Add support for CBS QDISC") just looks very buggy.
On Wed, May 29, 2024 at 02:57:27AM +0200, Andrew Lunn wrote: > On Wed, May 29, 2024 at 08:22:01AM +0800, xiaolei wang wrote: > > > > On 5/28/24 21:20, Andrew Lunn wrote: > > > CAUTION: This email comes from a non Wind River email account! > > > Do not click links or open attachments unless you recognize the sender and know the content is safe. > > > > > > On Tue, May 28, 2024 at 05:20:10PM +0800, Xiaolei Wang wrote: > > > > The CBS parameter can still be configured when the port is > > > > currently disconnected and link down. This is unreasonable. > > > This sounds like a generic problem. Can the core check the carrier > > > status and error out there? Maybe return a useful extack message. > > > > > > If you do need to return an error code, ENETDOWN seems more > > > > Currently cbs does not check link status. If ops->ndo_setup_tc() returns > > failure, there will only be an output of "Specified device failed to setup > > cbs hardware offload". > > So it sounds like we should catch this in the core then, not the > driver. And cbs_enable_offload() takes an extack, so you can report a > user friendly reason for failing, the at the carrier is off. It's worse than that (see my other reply.) If the link speed changes, there's nothing that deals with updating the CBS configuration for the new speed. CBS here is basically buggy - unless one reconfigures CBS each time the link comes up.
On 5/29/24 16:46, Russell King (Oracle) wrote: > CAUTION: This email comes from a non Wind River email account! > Do not click links or open attachments unless you recognize the sender and know the content is safe. > > On Tue, May 28, 2024 at 05:20:10PM +0800, Xiaolei Wang wrote: >> The CBS parameter can still be configured when the port is >> currently disconnected and link down. This is unreasonable. >> The current speed_div and ptr parameters depend on the negotiated >> speed after uplinking. So When the link is down, update priv->speed >> to SPEED_UNKNOWN and an error log should be added. >> >> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com> > So what happens if stmmac is connected to a PHY that can negotiate with > the link partner, it has link up at e.g. 1G speed, one configures CBS, > and then the link goes down and comes up at a different speed? > > I can't see any way in the stmmac driver that this is handled, which > makes this feature way more buggy than you're referring to here. It > also means that with your patch, if one attempts to configure CBS > when the link is down, it will fail. If there is no connection at the beginning, we still cannot configure, but after linking up again, and then linking down, we can configure again. This is very confusing. I think it makes sense to give a prompt for the stmmac driver after linking down. > > To me, commit 1f705bc61aee ("net: stmmac: Add support for CBS QDISC") > just looks very buggy. This makes sense. I think it is necessary to update the parameters after linking up. Does anyone have a better suggestion? thanks xiaolei > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Wed, May 29, 2024 at 06:55:21PM +0800, xiaolei wang wrote: > On 5/29/24 16:46, Russell King (Oracle) wrote: > > To me, commit 1f705bc61aee ("net: stmmac: Add support for CBS QDISC") > > just looks very buggy. > > This makes sense. I think it is necessary to update the parameters after > linking up. > > Does anyone have a better suggestion? Any setup that a phylink-based MAC driver does which is dependent on the negotiated media parameters (e.g. speed, duplex etc) _should_ always be done from the .mac_link_up method. So, from a phylink perspective, what you propose is the correct and only way.
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index b3afc7cb7d72..604e2e053852 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -995,6 +995,7 @@ static void stmmac_mac_link_down(struct phylink_config *config, priv->tx_lpi_enabled = false; priv->eee_enabled = stmmac_eee_init(priv); stmmac_set_eee_pls(priv, priv->hw, false); + priv->speed = SPEED_UNKNOWN; if (priv->dma_cap.fpesel) stmmac_fpe_link_state_handle(priv, false); diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c index 222540b55480..1e60033c6fbb 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c @@ -378,6 +378,7 @@ static int tc_setup_cbs(struct stmmac_priv *priv, speed_div = 100000; break; default: + dev_err(priv->device, "Link speed is not known"); return -EOPNOTSUPP; }
The CBS parameter can still be configured when the port is currently disconnected and link down. This is unreasonable. The current speed_div and ptr parameters depend on the negotiated speed after uplinking. So When the link is down, update priv->speed to SPEED_UNKNOWN and an error log should be added. Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com> --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 1 + drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c | 1 + 2 files changed, 2 insertions(+)