On 17.03.2021 22:20, Florian Fainelli wrote:
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.
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 <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.
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!
