* [PATCH 0/2] net: dsa: bcm_sf2: refactor & update RGMII regs
@ 2021-03-17 14:37 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 14:37 ` [PATCH 2/2] net: dsa: bcm_sf2: fix BCM4908 RGMII reg(s) Rafał Miłecki
0 siblings, 2 replies; 6+ messages in thread
From: Rafał Miłecki @ 2021-03-17 14:37 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean
Cc: David S . Miller, Jakub Kicinski, netdev,
bcm-kernel-feedback-list, Rafał Miłecki
From: Rafał Miłecki <rafal@milecki.pl>
The first patch fixes some generic problem but it seems so device other
than BCM4908 SoC were affected. So this probably isn't critical.
I think it's OK / enough to take it for the net-next.git.
Rafał Miłecki (2):
net: dsa: bcm_sf2: add function finding RGMII register
net: dsa: bcm_sf2: fix BCM4908 RGMII reg(s)
drivers/net/dsa/bcm_sf2.c | 60 +++++++++++++++++++++++++++++-----
drivers/net/dsa/bcm_sf2_regs.h | 3 +-
2 files changed, 52 insertions(+), 11 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] net: dsa: bcm_sf2: add function finding RGMII register
2021-03-17 14:37 [PATCH 0/2] net: dsa: bcm_sf2: refactor & update RGMII regs Rafał Miłecki
@ 2021-03-17 14:37 ` Rafał Miłecki
2021-03-17 21:20 ` Florian Fainelli
2021-03-17 14:37 ` [PATCH 2/2] net: dsa: bcm_sf2: fix BCM4908 RGMII reg(s) Rafał Miłecki
1 sibling, 1 reply; 6+ messages in thread
From: Rafał Miłecki @ 2021-03-17 14:37 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean
Cc: David S . Miller, Jakub Kicinski, netdev,
bcm-kernel-feedback-list, Rafał Miłecki
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).
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)
+{
+ 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)
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] net: dsa: bcm_sf2: fix BCM4908 RGMII reg(s)
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 14:37 ` Rafał Miłecki
1 sibling, 0 replies; 6+ messages in thread
From: Rafał Miłecki @ 2021-03-17 14:37 UTC (permalink / raw)
To: Andrew Lunn, Vivien Didelot, Florian Fainelli, Vladimir Oltean
Cc: David S . Miller, Jakub Kicinski, netdev,
bcm-kernel-feedback-list, Rafał Miłecki
From: Rafał Miłecki <rafal@milecki.pl>
BCM4908 has only 1 RGMII reg for controlling port 7.
Fixes: 73b7a6047971 ("net: dsa: bcm_sf2: support BCM4908's integrated switch")
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
drivers/net/dsa/bcm_sf2.c | 11 +++++++----
drivers/net/dsa/bcm_sf2_regs.h | 1 +
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 942773bcb7e0..5dc02dbb7094 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -36,7 +36,12 @@ static u16 bcm_sf2_reg_rgmii_cntrl(struct bcm_sf2_priv *priv, int port)
{
switch (priv->type) {
case BCM4908_DEVICE_ID:
- /* TODO */
+ switch (port) {
+ case 7:
+ return REG_RGMII_11_CNTRL;
+ default:
+ break;
+ }
break;
default:
switch (port) {
@@ -1183,9 +1188,7 @@ static const u16 bcm_sf2_4908_reg_offsets[] = {
[REG_PHY_REVISION] = 0x14,
[REG_SPHY_CNTRL] = 0x24,
[REG_CROSSBAR] = 0xc8,
- [REG_RGMII_0_CNTRL] = 0xe0,
- [REG_RGMII_1_CNTRL] = 0xec,
- [REG_RGMII_2_CNTRL] = 0xf8,
+ [REG_RGMII_11_CNTRL] = 0x014c,
[REG_LED_0_CNTRL] = 0x40,
[REG_LED_1_CNTRL] = 0x4c,
[REG_LED_2_CNTRL] = 0x58,
diff --git a/drivers/net/dsa/bcm_sf2_regs.h b/drivers/net/dsa/bcm_sf2_regs.h
index c7783cb45845..9e141d1a0b07 100644
--- a/drivers/net/dsa/bcm_sf2_regs.h
+++ b/drivers/net/dsa/bcm_sf2_regs.h
@@ -21,6 +21,7 @@ enum bcm_sf2_reg_offs {
REG_RGMII_0_CNTRL,
REG_RGMII_1_CNTRL,
REG_RGMII_2_CNTRL,
+ REG_RGMII_11_CNTRL,
REG_LED_0_CNTRL,
REG_LED_1_CNTRL,
REG_LED_2_CNTRL,
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] net: dsa: bcm_sf2: add function finding RGMII register
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
2021-03-18 7:30 ` Rafał Miłecki
0 siblings, 1 reply; 6+ messages in thread
From: Florian Fainelli @ 2021-03-17 21:20 UTC (permalink / raw)
To: Rafał Miłecki, Andrew Lunn, Vivien Didelot,
Florian Fainelli, Vladimir Oltean
Cc: David S . Miller, Jakub Kicinski, netdev,
bcm-kernel-feedback-list, Rafał Miłecki
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] net: dsa: bcm_sf2: add function finding RGMII register
2021-03-17 21:20 ` Florian Fainelli
@ 2021-03-18 7:30 ` Rafał Miłecki
2021-03-18 16:16 ` Florian Fainelli
0 siblings, 1 reply; 6+ messages in thread
From: Rafał Miłecki @ 2021-03-18 7:30 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, Vivien Didelot, Vladimir Oltean
Cc: David S . Miller, Jakub Kicinski, netdev,
bcm-kernel-feedback-list, Rafał Miłecki
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!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] net: dsa: bcm_sf2: add function finding RGMII register
2021-03-18 7:30 ` Rafał Miłecki
@ 2021-03-18 16:16 ` Florian Fainelli
0 siblings, 0 replies; 6+ messages in thread
From: Florian Fainelli @ 2021-03-18 16:16 UTC (permalink / raw)
To: Rafał Miłecki, Andrew Lunn, Vivien Didelot, Vladimir Oltean
Cc: David S . Miller, Jakub Kicinski, netdev,
bcm-kernel-feedback-list, Rafał Miłecki
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-18 16:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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
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.