netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 net-next] net: dsa: Allow only up to two HSR HW offloaded ports for KSZ9477
@ 2024-06-18 13:04 Lukasz Majewski
  2024-06-18 13:42 ` Dan Carpenter
  0 siblings, 1 reply; 11+ messages in thread
From: Lukasz Majewski @ 2024-06-18 13:04 UTC (permalink / raw)
  To: Vladimir Oltean, Jakub Kicinski, netdev, Paolo Abeni
  Cc: Eric Dumazet, David S. Miller, Oleksij Rempel, Tristram.Ha,
	Sebastian Andrzej Siewior, Simon Horman, Dan Carpenter,
	Ricardo B. Marliere, Casper Andersson, linux-kernel, Woojung Huh,
	UNGLinuxDriver, Andrew Lunn, Lukasz Majewski

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(+)

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);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 net-next] net: dsa: Allow only up to two HSR HW offloaded ports for KSZ9477
  2024-06-18 13:04 [PATCH v1 net-next] net: dsa: Allow only up to two HSR HW offloaded ports for KSZ9477 Lukasz Majewski
@ 2024-06-18 13:42 ` Dan Carpenter
  2024-06-18 13:49   ` Lukasz Majewski
                     ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Dan Carpenter @ 2024-06-18 13:42 UTC (permalink / raw)
  To: Lukasz Majewski
  Cc: Vladimir Oltean, Jakub Kicinski, netdev, Paolo Abeni,
	Eric Dumazet, David S. Miller, Oleksij Rempel, Tristram.Ha,
	Sebastian Andrzej Siewior, Simon Horman, Ricardo B. Marliere,
	Casper Andersson, linux-kernel, Woojung Huh, UNGLinuxDriver,
	Andrew Lunn

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.
 	 */



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 net-next] net: dsa: Allow only up to two HSR HW offloaded ports for KSZ9477
  2024-06-18 13:42 ` Dan Carpenter
@ 2024-06-18 13:49   ` Lukasz Majewski
  2024-06-18 13:52   ` Andrew Lunn
  2024-06-18 14:31   ` Lukasz Majewski
  2 siblings, 0 replies; 11+ messages in thread
From: Lukasz Majewski @ 2024-06-18 13:49 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Vladimir Oltean, Jakub Kicinski, netdev, Paolo Abeni,
	Eric Dumazet, David S. Miller, Oleksij Rempel, Tristram.Ha,
	Sebastian Andrzej Siewior, Simon Horman, Ricardo B. Marliere,
	Casper Andersson, linux-kernel, Woojung Huh, UNGLinuxDriver,
	Andrew Lunn

[-- Attachment #1: Type: text/plain, Size: 2776 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 net-next] net: dsa: Allow only up to two HSR HW offloaded ports for KSZ9477
  2024-06-18 13:42 ` Dan Carpenter
  2024-06-18 13:49   ` Lukasz Majewski
@ 2024-06-18 13:52   ` Andrew Lunn
  2024-06-18 14:06     ` Dan Carpenter
  2024-06-18 14:31   ` Lukasz Majewski
  2 siblings, 1 reply; 11+ messages in thread
From: Andrew Lunn @ 2024-06-18 13:52 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Lukasz Majewski, Vladimir Oltean, Jakub Kicinski, netdev,
	Paolo Abeni, Eric Dumazet, David S. Miller, Oleksij Rempel,
	Tristram.Ha, Sebastian Andrzej Siewior, Simon Horman,
	Ricardo B. Marliere, Casper Andersson, linux-kernel, Woojung Huh,
	UNGLinuxDriver

> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 net-next] net: dsa: Allow only up to two HSR HW offloaded ports for KSZ9477
  2024-06-18 13:52   ` Andrew Lunn
@ 2024-06-18 14:06     ` Dan Carpenter
  2024-06-18 14:31       ` Woojung.Huh
  0 siblings, 1 reply; 11+ messages in thread
From: Dan Carpenter @ 2024-06-18 14:06 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Lukasz Majewski, Vladimir Oltean, Jakub Kicinski, netdev,
	Paolo Abeni, Eric Dumazet, David S. Miller, Oleksij Rempel,
	Tristram.Ha, Sebastian Andrzej Siewior, Simon Horman,
	Ricardo B. Marliere, Casper Andersson, linux-kernel, Woojung Huh,
	UNGLinuxDriver

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 net-next] net: dsa: Allow only up to two HSR HW offloaded ports for KSZ9477
  2024-06-18 13:42 ` Dan Carpenter
  2024-06-18 13:49   ` Lukasz Majewski
  2024-06-18 13:52   ` Andrew Lunn
@ 2024-06-18 14:31   ` Lukasz Majewski
  2 siblings, 0 replies; 11+ messages in thread
From: Lukasz Majewski @ 2024-06-18 14:31 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Vladimir Oltean, Jakub Kicinski, netdev, Paolo Abeni,
	Eric Dumazet, David S. Miller, Oleksij Rempel, Tristram.Ha,
	Sebastian Andrzej Siewior, Simon Horman, Ricardo B. Marliere,
	Casper Andersson, linux-kernel, Woojung Huh, UNGLinuxDriver,
	Andrew Lunn

[-- Attachment #1: Type: text/plain, Size: 2605 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH v1 net-next] net: dsa: Allow only up to two HSR HW offloaded ports for KSZ9477
  2024-06-18 14:06     ` Dan Carpenter
@ 2024-06-18 14:31       ` Woojung.Huh
  2024-06-18 14:45         ` Lukasz Majewski
  0 siblings, 1 reply; 11+ messages in thread
From: Woojung.Huh @ 2024-06-18 14:31 UTC (permalink / raw)
  To: dan.carpenter, andrew
  Cc: lukma, olteanv, kuba, netdev, pabeni, edumazet, davem, o.rempel,
	Tristram.Ha, bigeasy, horms, ricardo, casper.casan, linux-kernel,
	UNGLinuxDriver

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 net-next] net: dsa: Allow only up to two HSR HW offloaded ports for KSZ9477
  2024-06-18 14:31       ` Woojung.Huh
@ 2024-06-18 14:45         ` Lukasz Majewski
  2024-06-18 14:56           ` Woojung.Huh
  2024-06-18 14:58           ` Andrew Lunn
  0 siblings, 2 replies; 11+ messages in thread
From: Lukasz Majewski @ 2024-06-18 14:45 UTC (permalink / raw)
  To: Woojung.Huh
  Cc: dan.carpenter, andrew, olteanv, kuba, netdev, pabeni, edumazet,
	davem, o.rempel, Tristram.Ha, bigeasy, horms, ricardo,
	casper.casan, linux-kernel, UNGLinuxDriver

[-- Attachment #1: Type: text/plain, Size: 3571 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH v1 net-next] net: dsa: Allow only up to two HSR HW offloaded ports for KSZ9477
  2024-06-18 14:45         ` Lukasz Majewski
@ 2024-06-18 14:56           ` Woojung.Huh
  2024-06-18 14:58           ` Andrew Lunn
  1 sibling, 0 replies; 11+ messages in thread
From: Woojung.Huh @ 2024-06-18 14:56 UTC (permalink / raw)
  To: lukma
  Cc: dan.carpenter, andrew, olteanv, kuba, netdev, pabeni, edumazet,
	davem, o.rempel, Tristram.Ha, bigeasy, horms, ricardo,
	casper.casan, linux-kernel, UNGLinuxDriver

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 net-next] net: dsa: Allow only up to two HSR HW offloaded ports for KSZ9477
  2024-06-18 14:45         ` Lukasz Majewski
  2024-06-18 14:56           ` Woojung.Huh
@ 2024-06-18 14:58           ` Andrew Lunn
  2024-06-18 15:02             ` Lukasz Majewski
  1 sibling, 1 reply; 11+ messages in thread
From: Andrew Lunn @ 2024-06-18 14:58 UTC (permalink / raw)
  To: Lukasz Majewski
  Cc: Woojung.Huh, dan.carpenter, olteanv, kuba, netdev, pabeni,
	edumazet, davem, o.rempel, Tristram.Ha, bigeasy, horms, ricardo,
	casper.casan, linux-kernel, UNGLinuxDriver

> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 net-next] net: dsa: Allow only up to two HSR HW offloaded ports for KSZ9477
  2024-06-18 14:58           ` Andrew Lunn
@ 2024-06-18 15:02             ` Lukasz Majewski
  0 siblings, 0 replies; 11+ messages in thread
From: Lukasz Majewski @ 2024-06-18 15:02 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Woojung.Huh, dan.carpenter, olteanv, kuba, netdev, pabeni,
	edumazet, davem, o.rempel, Tristram.Ha, bigeasy, horms, ricardo,
	casper.casan, linux-kernel, UNGLinuxDriver

[-- Attachment #1: Type: text/plain, Size: 1094 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2024-06-18 15:02 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-18 13:04 [PATCH v1 net-next] net: dsa: Allow only up to two HSR HW offloaded ports for KSZ9477 Lukasz Majewski
2024-06-18 13:42 ` Dan Carpenter
2024-06-18 13:49   ` Lukasz Majewski
2024-06-18 13:52   ` Andrew Lunn
2024-06-18 14:06     ` Dan Carpenter
2024-06-18 14:31       ` Woojung.Huh
2024-06-18 14:45         ` Lukasz Majewski
2024-06-18 14:56           ` Woojung.Huh
2024-06-18 14:58           ` Andrew Lunn
2024-06-18 15:02             ` Lukasz Majewski
2024-06-18 14:31   ` Lukasz Majewski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).