| Message ID | 20240606085234.565551-10-ms@dev.tdt.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: lantiq_gswip: code improvements | expand |
On Thu, Jun 06, 2024 at 10:52:30AM +0200, Martin Schiller wrote: > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > Calling gswip_add_single_port_br() with the CPU port would be a bug > because then only the CPU port could talk to itself. Add the CPU port to > the validation at the beginning of gswip_add_single_port_br(). > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- > drivers/net/dsa/lantiq_gswip.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c > index ee8296d5b901..d2195271ffe9 100644 > --- a/drivers/net/dsa/lantiq_gswip.c > +++ b/drivers/net/dsa/lantiq_gswip.c > @@ -657,7 +657,7 @@ static int gswip_add_single_port_br(struct gswip_priv *priv, int port, bool add) > unsigned int max_ports = priv->hw_info->max_ports; > int err; > > - if (port >= max_ports) { > + if (port >= max_ports || dsa_is_cpu_port(priv->ds, port)) { > dev_err(priv->dev, "single port for %i supported\n", port); > return -EIO; > } > -- > 2.39.2 > Isn't the new check effectively dead code?
On 2024-06-07 13:26, Vladimir Oltean wrote: > On Thu, Jun 06, 2024 at 10:52:30AM +0200, Martin Schiller wrote: >> From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> >> >> Calling gswip_add_single_port_br() with the CPU port would be a bug >> because then only the CPU port could talk to itself. Add the CPU port >> to >> the validation at the beginning of gswip_add_single_port_br(). >> >> Signed-off-by: Martin Blumenstingl >> <martin.blumenstingl@googlemail.com> >> --- >> drivers/net/dsa/lantiq_gswip.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/net/dsa/lantiq_gswip.c >> b/drivers/net/dsa/lantiq_gswip.c >> index ee8296d5b901..d2195271ffe9 100644 >> --- a/drivers/net/dsa/lantiq_gswip.c >> +++ b/drivers/net/dsa/lantiq_gswip.c >> @@ -657,7 +657,7 @@ static int gswip_add_single_port_br(struct >> gswip_priv *priv, int port, bool add) >> unsigned int max_ports = priv->hw_info->max_ports; >> int err; >> >> - if (port >= max_ports) { >> + if (port >= max_ports || dsa_is_cpu_port(priv->ds, port)) { >> dev_err(priv->dev, "single port for %i supported\n", port); >> return -EIO; >> } >> -- >> 2.39.2 >> > > Isn't the new check effectively dead code? As long as the dsa_switch_ops .port_bridge_join and .port_bridge_leave are not executed for the cpu port, I agree with you.
On Fri, Jun 07, 2024 at 03:31:57PM +0200, Martin Schiller wrote: > On 2024-06-07 13:26, Vladimir Oltean wrote: > > On Thu, Jun 06, 2024 at 10:52:30AM +0200, Martin Schiller wrote: > > > From: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > > > > > Calling gswip_add_single_port_br() with the CPU port would be a bug > > > because then only the CPU port could talk to itself. Add the CPU > > > port to > > > the validation at the beginning of gswip_add_single_port_br(). > > > > > > Signed-off-by: Martin Blumenstingl > > > <martin.blumenstingl@googlemail.com> > > > --- > > > drivers/net/dsa/lantiq_gswip.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/dsa/lantiq_gswip.c > > > b/drivers/net/dsa/lantiq_gswip.c > > > index ee8296d5b901..d2195271ffe9 100644 > > > --- a/drivers/net/dsa/lantiq_gswip.c > > > +++ b/drivers/net/dsa/lantiq_gswip.c > > > @@ -657,7 +657,7 @@ static int gswip_add_single_port_br(struct > > > gswip_priv *priv, int port, bool add) > > > unsigned int max_ports = priv->hw_info->max_ports; > > > int err; > > > > > > - if (port >= max_ports) { > > > + if (port >= max_ports || dsa_is_cpu_port(priv->ds, port)) { > > > dev_err(priv->dev, "single port for %i supported\n", port); > > > return -EIO; > > > } > > > -- > > > 2.39.2 > > > > > > > Isn't the new check effectively dead code? > > As long as the dsa_switch_ops .port_bridge_join and .port_bridge_leave are not > executed for the cpu port, I agree with you. They aren't. The primary trigger for dsa_port_bridge_join() is dsa_user_changeupper(), along with other code paths that replay the operation in certain circumstances, again only for user ports.
diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c index ee8296d5b901..d2195271ffe9 100644 --- a/drivers/net/dsa/lantiq_gswip.c +++ b/drivers/net/dsa/lantiq_gswip.c @@ -657,7 +657,7 @@ static int gswip_add_single_port_br(struct gswip_priv *priv, int port, bool add) unsigned int max_ports = priv->hw_info->max_ports; int err; - if (port >= max_ports) { + if (port >= max_ports || dsa_is_cpu_port(priv->ds, port)) { dev_err(priv->dev, "single port for %i supported\n", port); return -EIO; }