| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 18 Feb 2022 12:26:28 +0000
From: Alvin Šipraga <ALSI@...g-olufsen.dk>
To: Luiz Angelo Daros de Luca <luizluca@...il.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linus.walleij@...aro.org" <linus.walleij@...aro.org>,
"andrew@...n.ch" <andrew@...n.ch>,
"vivien.didelot@...il.com" <vivien.didelot@...il.com>,
"f.fainelli@...il.com" <f.fainelli@...il.com>,
"olteanv@...il.com" <olteanv@...il.com>,
"davem@...emloft.net" <davem@...emloft.net>,
"kuba@...nel.org" <kuba@...nel.org>,
"arinc.unal@...nc9.com" <arinc.unal@...nc9.com>
Subject: Re: [PATCH net-next v2 2/2] net: dsa: realtek: rtl8365mb: add support
for rtl8_4t
Luiz Angelo Daros de Luca <luizluca@...il.com> writes:
> The trailing tag is also supported by this family. The default is still
> rtl8_4 but now the switch supports changing the tag to rtl8_4t.
>
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@...il.com>
> ---
> drivers/net/dsa/realtek/rtl8365mb.c | 78 ++++++++++++++++++++++++-----
> 1 file changed, 66 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index 2ed592147c20..043cac34e906 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> @@ -524,9 +524,7 @@ enum rtl8365mb_cpu_rxlen {
> * @mask: port mask of ports that parse should parse CPU tags
> * @trap_port: forward trapped frames to this port
> * @insert: CPU tag insertion mode in switch->CPU frames
> - * @position: position of CPU tag in frame
> * @rx_length: minimum CPU RX length
> - * @format: CPU tag format
> *
> * Represents the CPU tagging and CPU port configuration of the switch. These
> * settings are configurable at runtime.
> @@ -536,9 +534,7 @@ struct rtl8365mb_cpu {
> u32 mask;
> u32 trap_port;
> enum rtl8365mb_cpu_insert insert;
> - enum rtl8365mb_cpu_position position;
> enum rtl8365mb_cpu_rxlen rx_length;
> - enum rtl8365mb_cpu_format format;
This struct is meant to represent the whole CPU config register. Rather
than pulling it out and adding tag_protocol to struct rtl8365mb, can you
instead do something like:
- keep these members of _cpu
- put back the cpu member of struct rtl8365mb (I don't know why it was removed...)
- in get_tag_protocol: return mb->cpu.position == AFTER_SA ? RTL8_4 : RTL8_4T;
- in change_tag_protocol: just update mb->cpu.position and call
rtl8365mb_cpu_config again
- avoid the arcane call to rtl8365mb_change_tag_protocol in _setup
- avoid the need to do regmap_update_bits instead of a clean
regmap_write in one place
The reason I'm saying this is because, in the original version of the
driver, CPU configuration was in a single place. Now it is scattered. I
would kindly ask that you try to respect the existing design because I
can already see that things are starting to get a bit messy.
If we subsequently want to configure other CPU parameters on the fly, it
will be as easy as updating the cpu struct and calling cpu_config
again. This register is also non-volatile so the state we keep will
always conform with the switch configuration.
Sorry if you find the feedback too opinionated - I don't mean anything
personally. But the original design was not by accident, so I would
appreciate if we can keep it that way unless there is a good reason to
change it.
> };
>
> /**
> @@ -566,6 +562,7 @@ struct rtl8365mb_port {
> * @chip_ver: chip silicon revision
> * @port_mask: mask of all ports
> * @learn_limit_max: maximum number of L2 addresses the chip can learn
> + * @tag_protocol: current switch CPU tag protocol
> * @mib_lock: prevent concurrent reads of MIB counters
> * @ports: per-port data
> * @jam_table: chip-specific initialization jam table
> @@ -580,6 +577,7 @@ struct rtl8365mb {
> u32 chip_ver;
> u32 port_mask;
> u32 learn_limit_max;
> + enum dsa_tag_protocol tag_protocol;
> struct mutex mib_lock;
> struct rtl8365mb_port ports[RTL8365MB_MAX_NUM_PORTS];
> const struct rtl8365mb_jam_tbl_entry *jam_table;
> @@ -770,7 +768,54 @@ static enum dsa_tag_protocol
> rtl8365mb_get_tag_protocol(struct dsa_switch *ds, int port,
> enum dsa_tag_protocol mp)
> {
> - return DSA_TAG_PROTO_RTL8_4;
> + struct realtek_priv *priv = ds->priv;
> + struct rtl8365mb *chip_data;
Please stick to the convention and call this struct rtl8365mb pointer mb.
> +
> + chip_data = priv->chip_data;
> +
> + return chip_data->tag_protocol;
> +}
> +
> +static int rtl8365mb_change_tag_protocol(struct dsa_switch *ds, int cpu,
> + enum dsa_tag_protocol proto)
> +{
> + struct realtek_priv *priv = ds->priv;
> + struct rtl8365mb *chip_data;
s/chip_data/mb/ per convention
> + int tag_position;
> + int tag_format;
> + int ret;
> +
> + switch (proto) {
> + case DSA_TAG_PROTO_RTL8_4:
> + tag_format = RTL8365MB_CPU_FORMAT_8BYTES;
> + tag_position = RTL8365MB_CPU_POS_AFTER_SA;
> + break;
> + case DSA_TAG_PROTO_RTL8_4T:
> + tag_format = RTL8365MB_CPU_FORMAT_8BYTES;
> + tag_position = RTL8365MB_CPU_POS_BEFORE_CRC;
> + break;
> + /* The switch also supports a 4-byte format, similar to rtl4a but with
> + * the same 0x04 8-bit version and probably 8-bit port source/dest.
> + * There is no public doc about it. Not supported yet.
> + */
> + default:
> + return -EPROTONOSUPPORT;
> + }
> +
> + ret = regmap_update_bits(priv->map, RTL8365MB_CPU_CTRL_REG,
> + RTL8365MB_CPU_CTRL_TAG_POSITION_MASK |
> + RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK,
> + FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK,
> + tag_position) |
> + FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK,
> + tag_format));
> + if (ret)
> + return ret;
> +
> + chip_data = priv->chip_data;
nit: I would put this assignment up top like in the rest of the driver,
respecting reverse-christmass-tree order. It's nice to stick to the
existing style.
> + chip_data->tag_protocol = proto;
> +
> + return 0;
> }
>
> static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port,
> @@ -1739,13 +1784,18 @@ static int rtl8365mb_cpu_config(struct realtek_priv *priv, const struct rtl8365m
>
> val = FIELD_PREP(RTL8365MB_CPU_CTRL_EN_MASK, cpu->enable ? 1 : 0) |
> FIELD_PREP(RTL8365MB_CPU_CTRL_INSERTMODE_MASK, cpu->insert) |
> - FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK, cpu->position) |
> FIELD_PREP(RTL8365MB_CPU_CTRL_RXBYTECOUNT_MASK, cpu->rx_length) |
> - FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, cpu->format) |
> FIELD_PREP(RTL8365MB_CPU_CTRL_TRAP_PORT_MASK, cpu->trap_port & 0x7) |
> FIELD_PREP(RTL8365MB_CPU_CTRL_TRAP_PORT_EXT_MASK,
> cpu->trap_port >> 3 & 0x1);
> - ret = regmap_write(priv->map, RTL8365MB_CPU_CTRL_REG, val);
> +
> + ret = regmap_update_bits(priv->map, RTL8365MB_CPU_CTRL_REG,
> + RTL8365MB_CPU_CTRL_EN_MASK |
> + RTL8365MB_CPU_CTRL_INSERTMODE_MASK |
> + RTL8365MB_CPU_CTRL_RXBYTECOUNT_MASK |
> + RTL8365MB_CPU_CTRL_TRAP_PORT_MASK |
> + RTL8365MB_CPU_CTRL_TRAP_PORT_EXT_MASK,
> + val);
> if (ret)
> return ret;
>
> @@ -1827,6 +1877,11 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
> dev_info(priv->dev, "no interrupt support\n");
>
> /* Configure CPU tagging */
> + ret = rtl8365mb_change_tag_protocol(priv->ds, -1, DSA_TAG_PROTO_RTL8_4);
> + if (ret) {
> + dev_err(priv->dev, "failed to set default tag protocol: %d\n", ret);
> + return ret;
> + }
> cpu.trap_port = RTL8365MB_MAX_NUM_PORTS;
> dsa_switch_for_each_cpu_port(cpu_dp, priv->ds) {
> cpu.mask |= BIT(cpu_dp->index);
> @@ -1834,13 +1889,9 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
> if (cpu.trap_port == RTL8365MB_MAX_NUM_PORTS)
> cpu.trap_port = cpu_dp->index;
> }
> -
> cpu.enable = cpu.mask > 0;
> cpu.insert = RTL8365MB_CPU_INSERT_TO_ALL;
> - cpu.position = RTL8365MB_CPU_POS_AFTER_SA;
> cpu.rx_length = RTL8365MB_CPU_RXLEN_64BYTES;
> - cpu.format = RTL8365MB_CPU_FORMAT_8BYTES;
Like I said above, I think it would be nice to put this cpu struct back
in the rtl8365mb private data.
> -
> ret = rtl8365mb_cpu_config(priv, &cpu);
> if (ret)
> goto out_teardown_irq;
> @@ -1982,6 +2033,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
> mb->learn_limit_max = RTL8365MB_LEARN_LIMIT_MAX;
> mb->jam_table = rtl8365mb_init_jam_8365mb_vc;
> mb->jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc);
> + mb->tag_protocol = DSA_TAG_PROTO_RTL8_4;
>
> break;
> default:
> @@ -1996,6 +2048,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
>
> static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = {
> .get_tag_protocol = rtl8365mb_get_tag_protocol,
> + .change_tag_protocol = rtl8365mb_change_tag_protocol,
> .setup = rtl8365mb_setup,
> .teardown = rtl8365mb_teardown,
> .phylink_get_caps = rtl8365mb_phylink_get_caps,
> @@ -2014,6 +2067,7 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = {
>
> static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = {
> .get_tag_protocol = rtl8365mb_get_tag_protocol,
> + .change_tag_protocol = rtl8365mb_change_tag_protocol,
> .setup = rtl8365mb_setup,
> .teardown = rtl8365mb_teardown,
> .phylink_get_caps = rtl8365mb_phylink_get_caps,
Powered by blists - more mailing lists