| Message ID | 20240618130433.1111485-1-lukma@denx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v1,net-next] net: dsa: Allow only up to two HSR HW offloaded ports for KSZ9477 | expand |
On Tue, Jun 18, 2024 at 03:04:33PM +0200, Lukasz Majewski wrote: > The KSZ9477 allows HSR in-HW offloading for any of two selected ports. > This patch adds check if one tries to use more than two ports with > HSR offloading enabled. > > Signed-off-by: Lukasz Majewski <lukma@denx.de> Is this a bug fix? What is the impact for the user? Fixes tag? Add this information to the commit message when you resend. > --- > drivers/net/dsa/microchip/ksz_common.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c > index 2818e24e2a51..0d68f0a5bf19 100644 > --- a/drivers/net/dsa/microchip/ksz_common.c > +++ b/drivers/net/dsa/microchip/ksz_common.c > @@ -3913,6 +3913,9 @@ static int ksz_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr, > if (ret) > return ret; > > + if (dev->chip_id == KSZ9477_CHIP_ID && hweight8(dev->hsr_ports) > 1) > + return -EOPNOTSUPP; Put this condition before the ksz_switch_macaddr_get(). Otherwise we'd need to do a ksz_switch_macaddr_put(). If dev->chip_id != KSZ9477_CHIP_ID then we would have already returned. Really, that should be the first check in this function. The hsr_get_version() should be moved to right before we use the version. (But that's a separate issue, not related to this patch so ignore it). So do something like this but write a better error message. regards, dan carpenter diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index 2818e24e2a51..181e81af3a78 100644 --- a/drivers/net/dsa/microchip/ksz_common.c +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -3906,6 +3906,11 @@ static int ksz_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr, return -EOPNOTSUPP; } + if (hweight8(dev->hsr_ports) > 1) { + NL_SET_ERR_MSG_MOD(extack, "Cannot offload more than two ports (in use=0x%x)", dev->hsr_ports); + return -EOPNOTSUPP; + } + /* Self MAC address filtering, to avoid frames traversing * the HSR ring more than once. */
Hi Dan, > On Tue, Jun 18, 2024 at 03:04:33PM +0200, Lukasz Majewski wrote: > > The KSZ9477 allows HSR in-HW offloading for any of two selected > > ports. This patch adds check if one tries to use more than two > > ports with HSR offloading enabled. > > > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > Is this a bug fix? This seems to be fixing stuff, which was added earlier to next-next. > What is the impact for the user? Impact is that this board with this particular setup can just malfunction. > Fixes tag? Ok. > Add > this information to the commit message when you resend. I will wait a few days for input and then send v2. Thanks for the input. > > > --- > > drivers/net/dsa/microchip/ksz_common.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/net/dsa/microchip/ksz_common.c > > b/drivers/net/dsa/microchip/ksz_common.c index > > 2818e24e2a51..0d68f0a5bf19 100644 --- > > a/drivers/net/dsa/microchip/ksz_common.c +++ > > b/drivers/net/dsa/microchip/ksz_common.c @@ -3913,6 +3913,9 @@ > > static int ksz_hsr_join(struct dsa_switch *ds, int port, struct > > net_device *hsr, if (ret) return ret; > > > > + if (dev->chip_id == KSZ9477_CHIP_ID && > > hweight8(dev->hsr_ports) > 1) > > + return -EOPNOTSUPP; > > Put this condition before the ksz_switch_macaddr_get(). Otherwise > we'd need to do a ksz_switch_macaddr_put(). > > If dev->chip_id != KSZ9477_CHIP_ID then we would have already > returned. Really, that should be the first check in this function. > The hsr_get_version() should be moved to right before we use the > version. (But that's a separate issue, not related to this patch so > ignore it). > > So do something like this but write a better error message. > > regards, > dan carpenter > > diff --git a/drivers/net/dsa/microchip/ksz_common.c > b/drivers/net/dsa/microchip/ksz_common.c index > 2818e24e2a51..181e81af3a78 100644 --- > a/drivers/net/dsa/microchip/ksz_common.c +++ > b/drivers/net/dsa/microchip/ksz_common.c @@ -3906,6 +3906,11 @@ > static int ksz_hsr_join(struct dsa_switch *ds, int port, struct > net_device *hsr, return -EOPNOTSUPP; } > > + if (hweight8(dev->hsr_ports) > 1) { > + NL_SET_ERR_MSG_MOD(extack, "Cannot offload more than > two ports (in use=0x%x)", dev->hsr_ports); > + return -EOPNOTSUPP; > + } > + > /* Self MAC address filtering, to avoid frames traversing > * the HSR ring more than once. > */ > > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c > index 2818e24e2a51..181e81af3a78 100644 > --- a/drivers/net/dsa/microchip/ksz_common.c > +++ b/drivers/net/dsa/microchip/ksz_common.c > @@ -3906,6 +3906,11 @@ static int ksz_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr, > return -EOPNOTSUPP; > } > > + if (hweight8(dev->hsr_ports) > 1) { > + NL_SET_ERR_MSG_MOD(extack, "Cannot offload more than two ports (in use=0x%x)", dev->hsr_ports); > + return -EOPNOTSUPP; > + } Hi Dan I don't know HSR to well, but this is offloading to hardware, to accelerate what Linux is already doing in software. It should be, if the hardware says it cannot do it, software will continue to do the job. So the extack message should never be seen. Andrew
On Tue, Jun 18, 2024 at 03:52:23PM +0200, Andrew Lunn wrote: > > diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c > > index 2818e24e2a51..181e81af3a78 100644 > > --- a/drivers/net/dsa/microchip/ksz_common.c > > +++ b/drivers/net/dsa/microchip/ksz_common.c > > @@ -3906,6 +3906,11 @@ static int ksz_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr, > > return -EOPNOTSUPP; > > } > > > > + if (hweight8(dev->hsr_ports) > 1) { > > + NL_SET_ERR_MSG_MOD(extack, "Cannot offload more than two ports (in use=0x%x)", dev->hsr_ports); > > + return -EOPNOTSUPP; > > + } > > Hi Dan > > I don't know HSR to well, but this is offloading to hardware, to > accelerate what Linux is already doing in software. It should be, if > the hardware says it cannot do it, software will continue to do the > job. So the extack message should never be seen. Ah. Okay. However the rest of the function prints similar messages and so probably we could remove those error messages as well. To be honest, I just wanted something which functioned as a comment and mentioned "two ports". Perhaps the condition would be more clear as >= 2 instead of > 1? regards, dan carpenter
On Tue, 18 Jun 2024 16:42:51 +0300 Dan Carpenter <dan.carpenter@linaro.org> wrote: > On Tue, Jun 18, 2024 at 03:04:33PM +0200, Lukasz Majewski wrote: > > The KSZ9477 allows HSR in-HW offloading for any of two selected > > ports. This patch adds check if one tries to use more than two > > ports with HSR offloading enabled. > > > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > Is this a bug fix? What is the impact for the user? Fixes tag? Add > this information to the commit message when you resend. > > > --- > > drivers/net/dsa/microchip/ksz_common.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/net/dsa/microchip/ksz_common.c > > b/drivers/net/dsa/microchip/ksz_common.c index > > 2818e24e2a51..0d68f0a5bf19 100644 --- > > a/drivers/net/dsa/microchip/ksz_common.c +++ > > b/drivers/net/dsa/microchip/ksz_common.c @@ -3913,6 +3913,9 @@ > > static int ksz_hsr_join(struct dsa_switch *ds, int port, struct > > net_device *hsr, if (ret) return ret; > > > > + if (dev->chip_id == KSZ9477_CHIP_ID && > > hweight8(dev->hsr_ports) > 1) > > + return -EOPNOTSUPP; > > Put this condition before the ksz_switch_macaddr_get(). Otherwise > we'd need to do a ksz_switch_macaddr_put(). > > If dev->chip_id != KSZ9477_CHIP_ID then we would have already > returned. Really, that should be the first check in this function. > The hsr_get_version() should be moved to right before we use the > version. (But that's a separate issue, not related to this patch so > ignore it). > > So do something like this but write a better error message. Ok. > > regards, > dan carpenter > > diff --git a/drivers/net/dsa/microchip/ksz_common.c > b/drivers/net/dsa/microchip/ksz_common.c index > 2818e24e2a51..181e81af3a78 100644 --- > a/drivers/net/dsa/microchip/ksz_common.c +++ > b/drivers/net/dsa/microchip/ksz_common.c @@ -3906,6 +3906,11 @@ > static int ksz_hsr_join(struct dsa_switch *ds, int port, struct > net_device *hsr, return -EOPNOTSUPP; } > > + if (hweight8(dev->hsr_ports) > 1) { > + NL_SET_ERR_MSG_MOD(extack, "Cannot offload more than > two ports (in use=0x%x)", dev->hsr_ports); > + return -EOPNOTSUPP; > + } > + > /* Self MAC address filtering, to avoid frames traversing > * the HSR ring more than once. > */ > > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Dan & Andrew, > On Tue, Jun 18, 2024 at 03:52:23PM +0200, Andrew Lunn wrote: > > > diff --git a/drivers/net/dsa/microchip/ksz_common.c > b/drivers/net/dsa/microchip/ksz_common.c > > > index 2818e24e2a51..181e81af3a78 100644 > > > --- a/drivers/net/dsa/microchip/ksz_common.c > > > +++ b/drivers/net/dsa/microchip/ksz_common.c > > > @@ -3906,6 +3906,11 @@ static int ksz_hsr_join(struct dsa_switch *ds, > int port, struct net_device *hsr, > > > return -EOPNOTSUPP; > > > } > > > > > > + if (hweight8(dev->hsr_ports) > 1) { > > > + NL_SET_ERR_MSG_MOD(extack, "Cannot offload more than two > ports (in use=0x%x)", dev->hsr_ports); > > > + return -EOPNOTSUPP; > > > + } > > > > Hi Dan > > > > I don't know HSR to well, but this is offloading to hardware, to > > accelerate what Linux is already doing in software. It should be, if > > the hardware says it cannot do it, software will continue to do the > > job. So the extack message should never be seen. > > Ah. Okay. However the rest of the function prints similar messages > and so probably we could remove those error messages as well. To be > honest, I just wanted something which functioned as a comment and > mentioned "two ports". Perhaps the condition would be more clear as > >= 2 instead of > 1? > I'm not a HSR expert and so could be a dummy question. I think this case (upto 2 HSR port offload) is different from other offload error. Others are checking whether offload is possible or not, so SW HSR can kick in when -EOPNOTSUPP returns. However, this happens when joining 3rd (2+) port with hardware offload is enabled. It is still working two ports are in HW HSR offload and next ports are in SW HSR? Thanks. Woojung
Hi Dan, Andrew, Woojung > Hi Dan & Andrew, > > > On Tue, Jun 18, 2024 at 03:52:23PM +0200, Andrew Lunn wrote: > > > > diff --git a/drivers/net/dsa/microchip/ksz_common.c > > b/drivers/net/dsa/microchip/ksz_common.c > > > > index 2818e24e2a51..181e81af3a78 100644 > > > > --- a/drivers/net/dsa/microchip/ksz_common.c > > > > +++ b/drivers/net/dsa/microchip/ksz_common.c > > > > @@ -3906,6 +3906,11 @@ static int ksz_hsr_join(struct > > > > dsa_switch *ds, > > int port, struct net_device *hsr, > > > > return -EOPNOTSUPP; > > > > } > > > > > > > > + if (hweight8(dev->hsr_ports) > 1) { > > > > + NL_SET_ERR_MSG_MOD(extack, "Cannot offload more > > > > than two > > ports (in use=0x%x)", dev->hsr_ports); > > > > + return -EOPNOTSUPP; > > > > + } > > > > > > Hi Dan > > > > > > I don't know HSR to well, but this is offloading to hardware, to > > > accelerate what Linux is already doing in software. It should be, > > > if the hardware says it cannot do it, software will continue to > > > do the job. So the extack message should never be seen. > > > > Ah. Okay. However the rest of the function prints similar messages > > and so probably we could remove those error messages as well. To be > > honest, I just wanted something which functioned as a comment and > > mentioned "two ports". Perhaps the condition would be more clear > > as > > >= 2 instead of > 1? > > > > I'm not a HSR expert and so could be a dummy question. > > I think this case (upto 2 HSR port offload) is different from other > offload error. It is not so different. In this case when we'd call: ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 interlink lan3 supervision 45 version 1 lan1 and lan2 are correctly configured as ports, which can use HSR offloading on ksz9477. However, when we do already have two bits set in hsr_ports, we need to return (-ENOTSUPP), so the interlink port (HSR-SAN) would be used with SW based HSR interlink support. Otherwise, I do see some strange behaviour, as some HSR frames are visible on HSR-SAN network and vice versa causing switch to drop frames. Also conceptually - the interlink (i.e. HSR-SAN port) shall be only SW supported as it is also possible to use ksz9477 with only SW based HSR (i.e. port0/1 -> hsr0 with offloading, port2 -> HSR-SAN/interlink, port4/5 -> hsr1 with SW based HSR). > Others are checking whether offload is possible or > not, so SW HSR can kick in when -EOPNOTSUPP returns. Yes, this is exactly the case. > However, this > happens when joining 3rd (2+) port with hardware offload is enabled. > It is still working two ports are in HW HSR offload and next ports > are in SW HSR? As written above, it seems like the in-chip VLAN register is modified and some frames are passed between HSR and SAN networks, which is wrong. Best would be to have only two ports with HSR offloading enabled and then others with SW based HSR if required. For me the: NL_SET_ERR_MSG_MOD(extack, "Cannot offload more than two ports (in use=0x%x)", dev->hsr_ports); is fine - as it informs that no more HSR offloading is possible (and allows to SW based RedBox/HSR-SAN operation). > > Thanks. > Woojung Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Lukasz, Thanks for detail explanation. > Hi Dan, Andrew, Woojung > > > Hi Dan & Andrew, > > > > > On Tue, Jun 18, 2024 at 03:52:23PM +0200, Andrew Lunn wrote: > > > > > diff --git a/drivers/net/dsa/microchip/ksz_common.c > > > b/drivers/net/dsa/microchip/ksz_common.c > > > > > index 2818e24e2a51..181e81af3a78 100644 > > > > > --- a/drivers/net/dsa/microchip/ksz_common.c > > > > > +++ b/drivers/net/dsa/microchip/ksz_common.c > > > > > @@ -3906,6 +3906,11 @@ static int ksz_hsr_join(struct > > > > > dsa_switch *ds, > > > int port, struct net_device *hsr, > > > > > return -EOPNOTSUPP; > > > > > } > > > > > > > > > > + if (hweight8(dev->hsr_ports) > 1) { > > > > > + NL_SET_ERR_MSG_MOD(extack, "Cannot offload more > > > > > than two > > > ports (in use=0x%x)", dev->hsr_ports); > > > > > + return -EOPNOTSUPP; > > > > > + } > > > > > > > > Hi Dan > > > > > > > > I don't know HSR to well, but this is offloading to hardware, to > > > > accelerate what Linux is already doing in software. It should be, > > > > if the hardware says it cannot do it, software will continue to > > > > do the job. So the extack message should never be seen. > > > > > > Ah. Okay. However the rest of the function prints similar messages > > > and so probably we could remove those error messages as well. To be > > > honest, I just wanted something which functioned as a comment and > > > mentioned "two ports". Perhaps the condition would be more clear > > > as > > > >= 2 instead of > 1? > > > > > > > I'm not a HSR expert and so could be a dummy question. > > > > I think this case (upto 2 HSR port offload) is different from other > > offload error. > > It is not so different. > > In this case when we'd call: > ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 interlink lan3 > supervision 45 version 1 > > lan1 and lan2 are correctly configured as ports, which can use HSR > offloading on ksz9477. > > However, when we do already have two bits set in hsr_ports, we need to > return (-ENOTSUPP), so the interlink port (HSR-SAN) would be used with > SW based HSR interlink support. > > Otherwise, I do see some strange behaviour, as some HSR frames are > visible on HSR-SAN network and vice versa causing switch to drop frames. > > Also conceptually - the interlink (i.e. HSR-SAN port) shall be only SW > supported as it is also possible to use ksz9477 with only SW based HSR > (i.e. port0/1 -> hsr0 with offloading, port2 -> HSR-SAN/interlink, > port4/5 -> hsr1 with SW based HSR). Got the point. Didn't think separate HSR (port 0/1 & port 4/5). Thought the case of port 0/1 (offload) + port 4/.. (SW HSR) > > > Others are checking whether offload is possible or > > not, so SW HSR can kick in when -EOPNOTSUPP returns. > > Yes, this is exactly the case. > > > However, this > > happens when joining 3rd (2+) port with hardware offload is enabled. > > It is still working two ports are in HW HSR offload and next ports > > are in SW HSR? > > As written above, it seems like the in-chip VLAN register is modified > and some frames are passed between HSR and SAN networks, which is wrong. > > Best would be to have only two ports with HSR offloading enabled and > then others with SW based HSR if required. > > For me the: > > NL_SET_ERR_MSG_MOD(extack, "Cannot offload more than two ports (in > use=0x%x)", dev->hsr_ports); > > is fine - as it informs that no more HSR offloading is possible (and > allows to SW based RedBox/HSR-SAN operation). > Having message looks good to me too. Woojung
> For me the: > > NL_SET_ERR_MSG_MOD(extack, "Cannot offload more than two ports (in > use=0x%x)", dev->hsr_ports); > > is fine - as it informs that no more HSR offloading is possible (and > allows to SW based RedBox/HSR-SAN operation). Does user space actually get to see it? I would expect the HSR code sees the EOPNOTSUPP, does not consider it an fatal error, and return 0 to user space. If userspace does see it, maybe we should make it clearer it is not an actually error. "Cannot offload more than two ports, using software bridging" so something similar. Andrew
Hi Andrew, > > For me the: > > > > NL_SET_ERR_MSG_MOD(extack, "Cannot offload more than two ports (in > > use=0x%x)", dev->hsr_ports); > > > > is fine - as it informs that no more HSR offloading is possible (and > > allows to SW based RedBox/HSR-SAN operation). > > Does user space actually get to see it? I would expect the HSR code > sees the EOPNOTSUPP, does not consider it an fatal error, and return 0 > to user space. > > If userspace does see it, maybe we should make it clearer it is not an > actually error. > > "Cannot offload more than two ports, using software bridging" > > so something similar. > Exactly - this is useful information - not error indication. (The same case is when we do want to set the MAC address already "taken" by ksz9477 HSR configuration.) > Andrew Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index 2818e24e2a51..0d68f0a5bf19 100644 --- a/drivers/net/dsa/microchip/ksz_common.c +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -3913,6 +3913,9 @@ static int ksz_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr, if (ret) return ret; + if (dev->chip_id == KSZ9477_CHIP_ID && hweight8(dev->hsr_ports) > 1) + return -EOPNOTSUPP; + ksz9477_hsr_join(ds, port, hsr); dev->hsr_dev = hsr; dev->hsr_ports |= BIT(port);
The KSZ9477 allows HSR in-HW offloading for any of two selected ports. This patch adds check if one tries to use more than two ports with HSR offloading enabled. Signed-off-by: Lukasz Majewski <lukma@denx.de> --- drivers/net/dsa/microchip/ksz_common.c | 3 +++ 1 file changed, 3 insertions(+)