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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <49f01c3d-7149-299e-d191-7ffdfb975039@gmail.com>
Date:   Wed, 17 Mar 2021 14:20:58 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Rafał Miłecki <zajec5@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        bcm-kernel-feedback-list@...adcom.com,
        Rafał Miłecki <rafal@...ecki.pl>
Subject: Re: [PATCH 1/2] net: dsa: bcm_sf2: add function finding RGMII
 register



On 3/17/2021 7:37 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@...ecki.pl>
> 
> Simple macro like REG_RGMII_CNTRL_P() is insufficient as:
> 1. It doesn't validate port argument
> 2. It doesn't support chipsets with non-lineral RGMII regs layout
> 
> Missing port validation could result in getting register offset from out
> of array. Random memory -> random offset -> random reads/writes. It
> affected e.g. BCM4908 for REG_RGMII_CNTRL_P(7).

That is entirely fair, however as a bug fix this is not necessarily the
simplest way to approach this.

> 
> Fixes: a78e86ed586d ("net: dsa: bcm_sf2: Prepare for different register layouts")
> Signed-off-by: Rafał Miłecki <rafal@...ecki.pl>
> ---
>  drivers/net/dsa/bcm_sf2.c      | 51 ++++++++++++++++++++++++++++++----
>  drivers/net/dsa/bcm_sf2_regs.h |  2 --
>  2 files changed, 45 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
> index ba5d546d06aa..942773bcb7e0 100644
> --- a/drivers/net/dsa/bcm_sf2.c
> +++ b/drivers/net/dsa/bcm_sf2.c
> @@ -32,6 +32,30 @@
>  #include "b53/b53_priv.h"
>  #include "b53/b53_regs.h"
>  
> +static u16 bcm_sf2_reg_rgmii_cntrl(struct bcm_sf2_priv *priv, int port)

This is not meant to be used outside the file, so I would be keen on
removing the bcm_sf2_ prefix to make the name shorter and closer to the
original macro name.

> +{
> +	switch (priv->type) {
> +	case BCM4908_DEVICE_ID:
> +		/* TODO */
> +		break;
> +	default:
> +		switch (port) {
> +		case 0:
> +			return REG_RGMII_0_CNTRL;
> +		case 1:
> +			return REG_RGMII_1_CNTRL;
> +		case 2:
> +			return REG_RGMII_2_CNTRL;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	WARN_ONCE(1, "Unsupported port %d\n", port);
> +
> +	return 0;

maybe return -1 or -EINVAL just in case 0 happens to be a valid offset
in the future. Checking the return value is not necessarily going to be
helpful as it needs immediate fixing, so what we could do is keep the
WARN_ON, and return the offset of REG_SWITCH_STATUS which is a read-only
register. This will trigger the bus arbiter logic to return an error
because a write was attempted from a read-only register.

What do you think?

> +}
> +
>  /* Return the number of active ports, not counting the IMP (CPU) port */
>  static unsigned int bcm_sf2_num_active_ports(struct dsa_switch *ds)
>  {
> @@ -647,6 +671,7 @@ static void bcm_sf2_sw_mac_config(struct dsa_switch *ds, int port,
>  {
>  	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
>  	u32 id_mode_dis = 0, port_mode;
> +	u32 reg_rgmii_ctrl;
>  	u32 reg;
>  
>  	if (port == core_readl(priv, CORE_IMP0_PRT_ID))
> @@ -670,10 +695,14 @@ static void bcm_sf2_sw_mac_config(struct dsa_switch *ds, int port,
>  		return;
>  	}
>  
> +	reg_rgmii_ctrl = bcm_sf2_reg_rgmii_cntrl(priv, port);
> +	if (!reg_rgmii_ctrl)
> +		return;
> +
>  	/* Clear id_mode_dis bit, and the existing port mode, let
>  	 * RGMII_MODE_EN bet set by mac_link_{up,down}
>  	 */
> -	reg = reg_readl(priv, REG_RGMII_CNTRL_P(port));
> +	reg = reg_readl(priv, reg_rgmii_ctrl);
>  	reg &= ~ID_MODE_DIS;
>  	reg &= ~(PORT_MODE_MASK << PORT_MODE_SHIFT);
>  
> @@ -681,13 +710,14 @@ static void bcm_sf2_sw_mac_config(struct dsa_switch *ds, int port,
>  	if (id_mode_dis)
>  		reg |= ID_MODE_DIS;
>  
> -	reg_writel(priv, reg, REG_RGMII_CNTRL_P(port));
> +	reg_writel(priv, reg, reg_rgmii_ctrl);
>  }
>  
>  static void bcm_sf2_sw_mac_link_set(struct dsa_switch *ds, int port,
>  				    phy_interface_t interface, bool link)
>  {
>  	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
> +	u32 reg_rgmii_ctrl;
>  	u32 reg;
>  
>  	if (!phy_interface_mode_is_rgmii(interface) &&
> @@ -695,13 +725,17 @@ static void bcm_sf2_sw_mac_link_set(struct dsa_switch *ds, int port,
>  	    interface != PHY_INTERFACE_MODE_REVMII)
>  		return;
>  
> +	reg_rgmii_ctrl = bcm_sf2_reg_rgmii_cntrl(priv, port);
> +	if (!reg_rgmii_ctrl)
> +		return;
> +
>  	/* If the link is down, just disable the interface to conserve power */
> -	reg = reg_readl(priv, REG_RGMII_CNTRL_P(port));
> +	reg = reg_readl(priv, reg_rgmii_ctrl);
>  	if (link)
>  		reg |= RGMII_MODE_EN;
>  	else
>  		reg &= ~RGMII_MODE_EN;
> -	reg_writel(priv, reg, REG_RGMII_CNTRL_P(port));
> +	reg_writel(priv, reg, reg_rgmii_ctrl);
>  }
>  
>  static void bcm_sf2_sw_mac_link_down(struct dsa_switch *ds, int port,
> @@ -735,8 +769,13 @@ static void bcm_sf2_sw_mac_link_up(struct dsa_switch *ds, int port,
>  {
>  	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
>  	struct ethtool_eee *p = &priv->dev->ports[port].eee;
> +	u32 reg_rgmii_ctrl;
>  	u32 reg, offset;
>  
> +	reg_rgmii_ctrl = bcm_sf2_reg_rgmii_cntrl(priv, port);
> +	if (!reg_rgmii_ctrl)
> +		return;
> +
>  	bcm_sf2_sw_mac_link_set(ds, port, interface, true);
>  
>  	if (port != core_readl(priv, CORE_IMP0_PRT_ID)) {
> @@ -750,7 +789,7 @@ static void bcm_sf2_sw_mac_link_up(struct dsa_switch *ds, int port,
>  		    interface == PHY_INTERFACE_MODE_RGMII_TXID ||
>  		    interface == PHY_INTERFACE_MODE_MII ||
>  		    interface == PHY_INTERFACE_MODE_REVMII) {
> -			reg = reg_readl(priv, REG_RGMII_CNTRL_P(port));
> +			reg = reg_readl(priv, reg_rgmii_ctrl);
>  			reg &= ~(RX_PAUSE_EN | TX_PAUSE_EN);
>  
>  			if (tx_pause)
> @@ -758,7 +797,7 @@ static void bcm_sf2_sw_mac_link_up(struct dsa_switch *ds, int port,
>  			if (rx_pause)
>  				reg |= RX_PAUSE_EN;
>  
> -			reg_writel(priv, reg, REG_RGMII_CNTRL_P(port));
> +			reg_writel(priv, reg, reg_rgmii_ctrl);
>  		}
>  
>  		reg = SW_OVERRIDE | LINK_STS;
> diff --git a/drivers/net/dsa/bcm_sf2_regs.h b/drivers/net/dsa/bcm_sf2_regs.h
> index 1d2d55c9f8aa..c7783cb45845 100644
> --- a/drivers/net/dsa/bcm_sf2_regs.h
> +++ b/drivers/net/dsa/bcm_sf2_regs.h
> @@ -48,8 +48,6 @@ enum bcm_sf2_reg_offs {
>  #define  PHY_PHYAD_SHIFT		8
>  #define  PHY_PHYAD_MASK			0x1F
>  
> -#define REG_RGMII_CNTRL_P(x)		(REG_RGMII_0_CNTRL + (x))
> -
>  /* Relative to REG_RGMII_CNTRL */
>  #define  RGMII_MODE_EN			(1 << 0)
>  #define  ID_MODE_DIS			(1 << 1)
> 

-- 
Florian

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ