All of lore.kernel.org
 help / color / mirror / Atom feed
From: Florian Fainelli <f.fainelli@gmail.com>
To: "Rafał Miłecki" <zajec5@gmail.com>,
	"Andrew Lunn" <andrew@lunn.ch>,
	"Vivien Didelot" <vivien.didelot@gmail.com>,
	"Florian Fainelli" <f.fainelli@gmail.com>,
	"Vladimir Oltean" <olteanv@gmail.com>
Cc: "David S . Miller" <davem@davemloft.net>,
	"Jakub Kicinski" <kuba@kernel.org>,
	netdev@vger.kernel.org, bcm-kernel-feedback-list@broadcom.com,
	"Rafał Miłecki" <rafal@milecki.pl>
Subject: Re: [PATCH 1/2] net: dsa: bcm_sf2: add function finding RGMII register
Date: Wed, 17 Mar 2021 14:20:58 -0700	[thread overview]
Message-ID: <49f01c3d-7149-299e-d191-7ffdfb975039@gmail.com> (raw)
In-Reply-To: <20210317143706.30809-2-zajec5@gmail.com>



On 3/17/2021 7:37 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.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@milecki.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

  reply	other threads:[~2021-03-17 21:21 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-17 14:37 [PATCH 0/2] net: dsa: bcm_sf2: refactor & update RGMII regs Rafał Miłecki
2021-03-17 14:37 ` [PATCH 1/2] net: dsa: bcm_sf2: add function finding RGMII register Rafał Miłecki
2021-03-17 21:20   ` Florian Fainelli [this message]
2021-03-18  7:30     ` Rafał Miłecki
2021-03-18 16:16       ` Florian Fainelli
2021-03-17 14:37 ` [PATCH 2/2] net: dsa: bcm_sf2: fix BCM4908 RGMII reg(s) Rafał Miłecki

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=49f01c3d-7149-299e-d191-7ffdfb975039@gmail.com \
    --to=f.fainelli@gmail.com \
    --cc=andrew@lunn.ch \
    --cc=bcm-kernel-feedback-list@broadcom.com \
    --cc=davem@davemloft.net \
    --cc=kuba@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=olteanv@gmail.com \
    --cc=rafal@milecki.pl \
    --cc=vivien.didelot@gmail.com \
    --cc=zajec5@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.