| Message ID | 20240130-realtek_reverse-v5-11-ecafd9283a07@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: realtek: variants to drivers, interfaces to a common module | expand |
On Tue, Jan 30, 2024 at 08:13:30PM -0300, Luiz Angelo Daros de Luca wrote: > To eliminate the need for a second memory allocation for dsa_switch, it > has been embedded within realtek_priv. > > Suggested-by: Alvin Šipraga <alsi@bang-olufsen.dk> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > --- Reviewed-by: Vladimir Oltean <olteanv@gmail.com> I don't think it would be bad if you could just define a local variable struct dsa_switch *ds = &priv->ds; in all functions, including those which reference &priv->ds just once. > diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h > index 864bb9a88f14..b80bfde1ad04 100644 > --- a/drivers/net/dsa/realtek/realtek.h > +++ b/drivers/net/dsa/realtek/realtek.h > @@ -61,7 +61,7 @@ struct realtek_priv { > const struct realtek_variant *variant; > > spinlock_t lock; /* Locks around command writes */ > - struct dsa_switch *ds; > + struct dsa_switch ds; > struct irq_domain *irqdomain; > bool leds_disabled; > > diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c > index 778a962727ab..9066e34e9ace 100644 > --- a/drivers/net/dsa/realtek/rtl8365mb.c > +++ b/drivers/net/dsa/realtek/rtl8365mb.c > @@ -880,7 +880,7 @@ static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port, > if (!extint) > return -ENODEV; > > - dp = dsa_to_port(priv->ds, port); > + dp = dsa_to_port(&priv->ds, port); Nitpick: I don't think it would be bad if you could just define a local variable struct dsa_switch *ds = &priv->ds; in all functions, including those which reference &priv->ds just once, like this one. > dn = dp->dn; > > /* Set the RGMII TX/RX delay > diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c > index aa998e15c42b..f65e47339d5b 100644 > --- a/drivers/net/dsa/realtek/rtl83xx.c > +++ b/drivers/net/dsa/realtek/rtl83xx.c > @@ -226,16 +226,12 @@ int rtl83xx_register_switch(struct realtek_priv *priv) > return ret; > } > > - priv->ds = devm_kzalloc(priv->dev, sizeof(*priv->ds), GFP_KERNEL); > - if (!priv->ds) > - return -ENOMEM; > + priv->ds.priv = priv; > + priv->ds.dev = priv->dev; > + priv->ds.ops = priv->variant->ds_ops; > + priv->ds.num_ports = priv->num_ports; And here, I think it would actually look better to dereference just through "ds". Also, priv->dev is dereferenced 4 times in rtl83xx_register_switch(), maybe you could add a local variable for it in the patch that introduces rtl83xx_register_switch(). > > - priv->ds->priv = priv; > - priv->ds->dev = priv->dev; > - priv->ds->ops = priv->variant->ds_ops; > - priv->ds->num_ports = priv->num_ports; > - > - ret = dsa_register_switch(priv->ds); > + ret = dsa_register_switch(&priv->ds); > if (ret) { > dev_err_probe(priv->dev, ret, "unable to register switch\n"); > return ret;
> > > I don't think it would be bad if you could just define a local variable > > struct dsa_switch *ds = &priv->ds; Yes, I'll use it. It does look better. Regards, Luiz
diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h index 864bb9a88f14..b80bfde1ad04 100644 --- a/drivers/net/dsa/realtek/realtek.h +++ b/drivers/net/dsa/realtek/realtek.h @@ -61,7 +61,7 @@ struct realtek_priv { const struct realtek_variant *variant; spinlock_t lock; /* Locks around command writes */ - struct dsa_switch *ds; + struct dsa_switch ds; struct irq_domain *irqdomain; bool leds_disabled; diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c index 778a962727ab..9066e34e9ace 100644 --- a/drivers/net/dsa/realtek/rtl8365mb.c +++ b/drivers/net/dsa/realtek/rtl8365mb.c @@ -880,7 +880,7 @@ static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port, if (!extint) return -ENODEV; - dp = dsa_to_port(priv->ds, port); + dp = dsa_to_port(&priv->ds, port); dn = dp->dn; /* Set the RGMII TX/RX delay @@ -1543,7 +1543,7 @@ static void rtl8365mb_stats_setup(struct realtek_priv *priv) for (i = 0; i < priv->num_ports; i++) { struct rtl8365mb_port *p = &mb->ports[i]; - if (dsa_is_unused_port(priv->ds, i)) + if (dsa_is_unused_port(&priv->ds, i)) continue; /* Per-port spinlock to protect the stats64 data */ @@ -1564,7 +1564,7 @@ static void rtl8365mb_stats_teardown(struct realtek_priv *priv) for (i = 0; i < priv->num_ports; i++) { struct rtl8365mb_port *p = &mb->ports[i]; - if (dsa_is_unused_port(priv->ds, i)) + if (dsa_is_unused_port(&priv->ds, i)) continue; cancel_delayed_work_sync(&p->mib_work); @@ -1963,7 +1963,7 @@ static int rtl8365mb_setup(struct dsa_switch *ds) dev_info(priv->dev, "no interrupt support\n"); /* Configure CPU tagging */ - dsa_switch_for_each_cpu_port(cpu_dp, priv->ds) { + dsa_switch_for_each_cpu_port(cpu_dp, &priv->ds) { cpu->mask |= BIT(cpu_dp->index); if (cpu->trap_port == RTL8365MB_MAX_NUM_PORTS) @@ -1978,7 +1978,7 @@ static int rtl8365mb_setup(struct dsa_switch *ds) for (i = 0; i < priv->num_ports; i++) { struct rtl8365mb_port *p = &mb->ports[i]; - if (dsa_is_unused_port(priv->ds, i)) + if (dsa_is_unused_port(&priv->ds, i)) continue; /* Forward only to the CPU */ @@ -1995,7 +1995,7 @@ static int rtl8365mb_setup(struct dsa_switch *ds) * ports will still forward frames to the CPU despite being * administratively down by default. */ - rtl8365mb_port_stp_state_set(priv->ds, i, BR_STATE_DISABLED); + rtl8365mb_port_stp_state_set(&priv->ds, i, BR_STATE_DISABLED); /* Set up per-port private data */ p->priv = priv; diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c index 54eff9cd0c03..cdc37be1ed2c 100644 --- a/drivers/net/dsa/realtek/rtl8366rb.c +++ b/drivers/net/dsa/realtek/rtl8366rb.c @@ -1675,7 +1675,7 @@ static int rtl8366rb_set_mc_index(struct realtek_priv *priv, int port, int index * not drop any untagged or C-tagged frames. Make sure to update the * filtering setting. */ - if (dsa_port_is_vlan_filtering(dsa_to_port(priv->ds, port))) + if (dsa_port_is_vlan_filtering(dsa_to_port(&priv->ds, port))) ret = rtl8366rb_drop_untagged(priv, port, !pvid_enabled); return ret; diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c index aa998e15c42b..f65e47339d5b 100644 --- a/drivers/net/dsa/realtek/rtl83xx.c +++ b/drivers/net/dsa/realtek/rtl83xx.c @@ -226,16 +226,12 @@ int rtl83xx_register_switch(struct realtek_priv *priv) return ret; } - priv->ds = devm_kzalloc(priv->dev, sizeof(*priv->ds), GFP_KERNEL); - if (!priv->ds) - return -ENOMEM; + priv->ds.priv = priv; + priv->ds.dev = priv->dev; + priv->ds.ops = priv->variant->ds_ops; + priv->ds.num_ports = priv->num_ports; - priv->ds->priv = priv; - priv->ds->dev = priv->dev; - priv->ds->ops = priv->variant->ds_ops; - priv->ds->num_ports = priv->num_ports; - - ret = dsa_register_switch(priv->ds); + ret = dsa_register_switch(&priv->ds); if (ret) { dev_err_probe(priv->dev, ret, "unable to register switch\n"); return ret; @@ -256,7 +252,7 @@ EXPORT_SYMBOL_NS_GPL(rtl83xx_register_switch, REALTEK_DSA); */ void rtl83xx_unregister_switch(struct realtek_priv *priv) { - dsa_unregister_switch(priv->ds); + dsa_unregister_switch(&priv->ds); } EXPORT_SYMBOL_NS_GPL(rtl83xx_unregister_switch, REALTEK_DSA); @@ -273,7 +269,7 @@ EXPORT_SYMBOL_NS_GPL(rtl83xx_unregister_switch, REALTEK_DSA); */ void rtl83xx_shutdown(struct realtek_priv *priv) { - dsa_switch_shutdown(priv->ds); + dsa_switch_shutdown(&priv->ds); dev_set_drvdata(priv->dev, NULL); }
To eliminate the need for a second memory allocation for dsa_switch, it has been embedded within realtek_priv. Suggested-by: Alvin Šipraga <alsi@bang-olufsen.dk> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> --- drivers/net/dsa/realtek/realtek.h | 2 +- drivers/net/dsa/realtek/rtl8365mb.c | 12 ++++++------ drivers/net/dsa/realtek/rtl8366rb.c | 2 +- drivers/net/dsa/realtek/rtl83xx.c | 18 +++++++----------- 4 files changed, 15 insertions(+), 19 deletions(-)