netdev - Re: [RFC PATCH net-next 03/11] net: dsa: microchip: lan937x: enable cascade port

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230203232442.hhfgebj4rppmh5nn@skbuf>
Date:   Sat, 4 Feb 2023 01:24:42 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Rakesh Sankaranarayanan <rakesh.sankaranarayanan@...rochip.com>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        andrew@...n.ch, f.fainelli@...il.com, davem@...emloft.net,
        edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
        woojung.huh@...rochip.com, UNGLinuxDriver@...rochip.com,
        linux@...linux.org.uk
Subject: Re: [RFC PATCH net-next 03/11] net: dsa: microchip: lan937x: enable
 cascade port

On Thu, Feb 02, 2023 at 06:29:22PM +0530, Rakesh Sankaranarayanan wrote:
> diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
> index aab60f2587bf..c3c3eee178f4 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -147,6 +147,7 @@ struct ksz_device {
>  	u32 chip_id;
>  	u8 chip_rev;
>  	int cpu_port;			/* port connected to CPU */
> +	int dsa_port;                   /* Port used as cascaded port */

nitpick: since "dsa_port" is typically a keyword that I use for the DSA
framework's generic port structure (struct dsa_port *dp), I would appreciate
if you could name this in some other way, like "cascade_port". I don't
believe the explanatory comment would even be necessary in that case.
And btw, the comment is inconsistent in alignment (tabs vs spaces) with
the line immediately above it.

>  	u32 smi_index;
>  	int phy_port_cnt;
>  	phy_interface_t compat_interface;
> @@ -358,6 +359,7 @@ struct ksz_dev_ops {
>  	void (*setup_rgmii_delay)(struct ksz_device *dev, int port);
>  	int (*tc_cbs_set_cinc)(struct ksz_device *dev, int port, u32 val);
>  	void (*config_cpu_port)(struct dsa_switch *ds);
> +	void (*config_dsa_port)(struct dsa_switch *ds);
>  	int (*enable_stp_addr)(struct ksz_device *dev);
>  	int (*reset)(struct ksz_device *dev);
>  	int (*init)(struct ksz_device *dev);
> diff --git a/drivers/net/dsa/microchip/lan937x.h b/drivers/net/dsa/microchip/lan937x.h
> index 3388d91dbc44..ef84abc31556 100644
> --- a/drivers/net/dsa/microchip/lan937x.h
> +++ b/drivers/net/dsa/microchip/lan937x.h
> @@ -11,6 +11,7 @@ int lan937x_setup(struct dsa_switch *ds);
>  void lan937x_teardown(struct dsa_switch *ds);
>  void lan937x_port_setup(struct ksz_device *dev, int port, bool cpu_port);
>  void lan937x_config_cpu_port(struct dsa_switch *ds);
> +void lan937x_config_dsa_port(struct dsa_switch *ds);
>  int lan937x_switch_init(struct ksz_device *dev);
>  void lan937x_switch_exit(struct ksz_device *dev);
>  int lan937x_r_phy(struct ksz_device *dev, u16 addr, u16 reg, u16 *data);
> diff --git a/drivers/net/dsa/microchip/lan937x_main.c b/drivers/net/dsa/microchip/lan937x_main.c
> index 399a3905e6ca..5108a3f4bf76 100644
> --- a/drivers/net/dsa/microchip/lan937x_main.c
> +++ b/drivers/net/dsa/microchip/lan937x_main.c
> @@ -205,11 +205,42 @@ void lan937x_port_setup(struct ksz_device *dev, int port, bool cpu_port)
>  	dev->dev_ops->cfg_port_member(dev, port, member);
>  }
>  
> +void lan937x_config_dsa_port(struct dsa_switch *ds)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	struct dsa_port *dp;
> +
> +	dev->dsa_port = 0xFF;
> +
> +	dsa_switch_for_each_port(dp, ds) {
> +		if (dsa_is_dsa_port(ds, dp->index)) {

Would be good if you introduced dsa_switch_for_each_dsa_port() as a
separate patch, and used it here first.

Not sure if you realize this, but dsa_is_dsa_port() contains a list
iteration hidden in it, in dsa_to_port(). So dsa_switch_for_each_port()
-> dsa_is_dsa_port() effectively does an O(n^2) list walk (apart from
uselessly increasing the code indentation).

> +			ksz_rmw32(dev, REG_SW_CASCADE_MODE_CTL,
> +				  CASCADE_PORT_SEL, dp->index);
> +			dev->dsa_port = dp->index;
> +
> +			/* Tail tag should be enabled for switch 0
> +			 * in cascaded connection.
> +			 */
> +			if (dev->smi_index == 0) {
> +				lan937x_port_cfg(dev, dp->index, REG_PORT_CTRL_0,
> +						 PORT_TAIL_TAG_ENABLE, true);
> +			}
> +
> +			/* Frame check length should be disabled for cascaded ports */
> +			lan937x_port_cfg(dev, dp->index, REG_PORT_MAC_CTRL_0,
> +					 PORT_CHECK_LENGTH, false);

break?

> +		}
> +	}
> +}
> +
>  void lan937x_config_cpu_port(struct dsa_switch *ds)
>  {
>  	struct ksz_device *dev = ds->priv;
>  	struct dsa_port *dp;
>  
> +	/* Initializing cpu_port parameter into invalid value */
> +	dev->cpu_port = 0xFF;
> +
>  	dsa_switch_for_each_cpu_port(dp, ds) {
>  		if (dev->info->cpu_ports & (1 << dp->index)) {
>  			dev->cpu_port = dp->index;
> diff --git a/drivers/net/dsa/microchip/lan937x_reg.h b/drivers/net/dsa/microchip/lan937x_reg.h
> index 45b606b6429f..4f30bc12f7a9 100644
> --- a/drivers/net/dsa/microchip/lan937x_reg.h
> +++ b/drivers/net/dsa/microchip/lan937x_reg.h
> @@ -32,6 +32,9 @@
>  #define REG_SW_PORT_INT_STATUS__4	0x0018
>  #define REG_SW_PORT_INT_MASK__4		0x001C
>  
> +#define REG_SW_CASCADE_MODE_CTL         0x0030
> +#define CASCADE_PORT_SEL                7
> +
>  /* 1 - Global */
>  #define REG_SW_GLOBAL_OUTPUT_CTRL__1	0x0103
>  #define SW_CLK125_ENB			BIT(1)
> -- 
> 2.34.1
>