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