| Message ID | 20210317143706.30809-2-zajec5@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] net: dsa: bcm_sf2: add function finding RGMII register | expand |
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) >
On 17.03.2021 22:20, Florian Fainelli wrote: > 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. I'm not sure if I understand. Should I fix it in some totally different way? Or should I just follow your inline suggestions? >> 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. Most or all local functions use such a prefix. E.g.: bcm_sf2_num_active_ports() bcm_sf2_recalc_clock() bcm_sf2_imp_setup() bcm_sf2_gphy_enable_set() bcm_sf2_port_intr_enable() bcm_sf2_port_intr_disable() bcm_sf2_port_setup() bcm_sf2_port_disable() It would be inconsistent to have RGMII reg function not follow that. >> +{ >> + 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? Great, thanks!
On 3/18/2021 12:30 AM, Rafał Miłecki wrote: > On 17.03.2021 22:20, Florian Fainelli wrote: >> 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. > > I'm not sure if I understand. Should I fix it in some totally different > way? Or should I just follow your inline suggestions? What I meant is that for a bug fix you could just mangled the offset of the register such that REG_RGMII_CNTRL_P(7) would resole to the right offset. That would be lying a little bit, but for a bug fix, that would work. Not that it matters since the changes are still fresh in net/net-next. -- Florian
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) +{ + 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; +} + /* 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)