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