| 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: Mon, 10 Jan 2022 13:39:53 +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>,
"arinc.unal@...nc9.com" <arinc.unal@...nc9.com>,
"frank-w@...lic-files.de" <frank-w@...lic-files.de>
Subject: Re: [PATCH net-next v4 11/11] net: dsa: realtek: rtl8365mb: multiple
cpu ports, non cpu extint
Luiz Angelo Daros de Luca <luizluca@...il.com> writes:
> Now CPU port is not limited to a single port. Also, extint can be used
> as non-cpu ports, as long as it defines relatek,ext-int. The last cpu
> port will be used as trap_port.
>
> The CPU information was dropped from chip data as it was not used
> outside setup. The only other place it was used is when it wrongly
> checks for CPU port when it should check for extint.
>
> realtek_priv->cpu_port is now only used by rtl8366rb.c
Great work with this series! If I understood correctly from your last
emails, you weren't actually able to test this due to hardware
constraints. While I think this change is not going to introduce any
surprises, I think you should still mention that it is not tested.
Some more comments below but in general the change makes sense to me.
>
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@...il.com>
> ---
> drivers/net/dsa/realtek/rtl8365mb.c | 53 +++++++++++++++--------------
> 1 file changed, 27 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index 59e08b192c06..6a00a162b2ac 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> @@ -556,7 +556,6 @@ 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
> - * @cpu: CPU tagging and CPU port configuration for this chip
> * @mib_lock: prevent concurrent reads of MIB counters
> * @ports: per-port data
> * @jam_table: chip-specific initialization jam table
> @@ -571,7 +570,6 @@ struct rtl8365mb {
> u32 chip_ver;
> u32 port_mask;
> u32 learn_limit_max;
> - struct rtl8365mb_cpu cpu;
> struct mutex mib_lock;
> struct rtl8365mb_port ports[RTL8365MB_MAX_NUM_PORTS];
> const struct rtl8365mb_jam_tbl_entry *jam_table;
> @@ -769,17 +767,20 @@ static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port,
> u32 val;
> int ret;
>
> - if (port != priv->cpu_port) {
> - dev_err(priv->dev, "only one EXT interface is currently supported\n");
> + mb = priv->chip_data;
> + p = &mb->ports[port];
> + ext_int = p->ext_int;
> +
> + if (ext_int == RTL8365MB_NOT_EXT) {
> + dev_err(priv->dev,
> + "Port %d is not identified as extenal interface.\n",
Maybe just a warning?
also: s/as extenal/as an external/
> + port);
> return -EINVAL;
> }
>
> dp = dsa_to_port(priv->ds, port);
> dn = dp->dn;
>
> - mb = priv->chip_data;
> - p = &mb->ports[port];
> - ext_int = p->ext_int;
>
> /* Set the RGMII TX/RX delay
> *
> @@ -859,15 +860,17 @@ static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port,
> int val;
> int ret;
>
> - if (port != priv->cpu_port) {
> - dev_err(priv->dev, "only one EXT interface is currently supported\n");
> - return -EINVAL;
> - }
> -
> mb = priv->chip_data;
> p = &mb->ports[port];
> ext_int = p->ext_int;
>
> + if (ext_int == RTL8365MB_NOT_EXT) {
> + dev_err(priv->dev,
> + "Port %d is not identified as extenal interface.\n",
ditto
> + port);
> + return -EINVAL;
> + }
> +
> if (link) {
> /* Force the link up with the desired configuration */
> r_link = 1;
> @@ -1734,10 +1737,8 @@ static void rtl8365mb_irq_teardown(struct realtek_priv *priv)
> }
> }
>
> -static int rtl8365mb_cpu_config(struct realtek_priv *priv)
> +static int rtl8365mb_cpu_config(struct realtek_priv *priv, struct rtl8365mb_cpu *cpu)
const struct rtl8365mb_cpu?
> {
> - struct rtl8365mb *mb = priv->chip_data;
> - struct rtl8365mb_cpu *cpu = &mb->cpu;
> u32 val;
> int ret;
>
> @@ -1839,11 +1840,17 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
> dev_info(priv->dev, "no interrupt support\n");
>
> /* Configure CPU tagging */
> + cpu.mask = 0;
I guess the unused cpu variable in the earlier patch belongs in this
one, in which case you can just initialize it = { 0 } so that you don't
need to explicitly set cpu.mask = 0.
> dsa_switch_for_each_cpu_port(cpu_dp, priv->ds) {
> - priv->cpu_port = cpu_dp->index;
> - mb->cpu.mask = BIT(priv->cpu_port);
> - mb->cpu.trap_port = priv->cpu_port;
> - ret = rtl8365mb_cpu_config(priv);
> + cpu.enable = 1;
> + 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;
> + cpu.trap_port = cpu_dp->index;
If you are going to do this, perhaps it's better specified as a device
tree property like the external interface index? Making the "last" CPU
port the trap port is not incorrect, but it seems quite arbitrary.
> + cpu.mask |= BIT(cpu_dp->index);
> +
> + ret = rtl8365mb_cpu_config(priv, &cpu);
Shouldn't this go outside the loop to avoid potentially calling it twice
in a row?
> if (ret)
> goto out_teardown_irq;
>
> @@ -1862,7 +1869,7 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
> dn = dsa_to_port(priv->ds, i)->dn;
>
> /* Forward only to the CPU */
> - ret = rtl8365mb_port_set_isolation(priv, i, BIT(priv->cpu_port));
> + ret = rtl8365mb_port_set_isolation(priv, i, cpu.mask);
> if (ret)
> goto out_teardown_irq;
>
> @@ -2003,12 +2010,6 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
> mb->jam_table = rtl8365mb_init_jam_8365mb_vc;
> mb->jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc);
>
> - mb->cpu.enable = 1;
> - mb->cpu.insert = RTL8365MB_CPU_INSERT_TO_ALL;
> - mb->cpu.position = RTL8365MB_CPU_POS_AFTER_SA;
> - mb->cpu.rx_length = RTL8365MB_CPU_RXLEN_64BYTES;
> - mb->cpu.format = RTL8365MB_CPU_FORMAT_8BYTES;
> -
> break;
> default:
> dev_err(priv->dev,
Powered by blists - more mailing lists