| Message ID | 20211126081252.32254-1-luizluca@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net: dsa: realtek-smi: fix indirect reg access for ports>3 | expand |
On Fri, Nov 26, 2021 at 05:12:52AM -0300, luizluca@gmail.com wrote: > From: Luiz Angelo Daros de Luca <luizluca@gmail.com> > > This switch family can have up to 8 ports {0..7}. However, > INDIRECT_ACCESS_ADDRESS_PHYNUM_MASK was using 2 bits instead of 3, > dropping the most significant bit during indirect register reads and > writes. Reading or writing ports 4, 5, 6, and 7 registers was actually > manipulating, respectively, ports 0, 1, 2, and 3 registers. > > rtl8365mb_phy_{read,write} will now returns -EINVAL if phy is greater > than 7. > > v2: > - fix affected ports in commit message > > Fixes: 4af2950c50c8 ("net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC") > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > --- > drivers/net/dsa/rtl8365mb.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) <formletter> This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly. </formletter>
Hi Luiz, Thanks for your patch. You needn't cc the stable list, since rtl8365mb doesn't exist in any stable release just yet. If you send a v3, just target net: [PATCH net v3] net: dsa: ... Then the fix will land in 5.16. :-) On 11/26/21 09:12, luizluca@gmail.com wrote: > From: Luiz Angelo Daros de Luca <luizluca@gmail.com> > > This switch family can have up to 8 ports {0..7}. However, > INDIRECT_ACCESS_ADDRESS_PHYNUM_MASK was using 2 bits instead of 3, > dropping the most significant bit during indirect register reads and > writes. Reading or writing ports 4, 5, 6, and 7 registers was actually > manipulating, respectively, ports 0, 1, 2, and 3 registers. Nice catch. Out of curiosity can you share what switch you are testing? So far the driver only advertises support for RTL8365MB-VC. Since that switch only uses PHY addresses 0~3, it shouldn't be affected by a narrower (2-bit) PHYNUM_MASK, right? Are you able to add words to the effect of "... now this fixes the driver to work with RTL83xxxx"? The change is OK except for some comments below: > > rtl8365mb_phy_{read,write} will now returns -EINVAL if phy is greater > than 7. I don't think this is really necessary: a valid (device tree) configuration should never specify a PHY with address greater than 7. Or am I missing something? > > v2: > - fix affected ports in commit message The changelog shouldn't end up in the final commit message - please move it out in v3. > > Fixes: 4af2950c50c8 ("net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC") > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > --- > drivers/net/dsa/rtl8365mb.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/dsa/rtl8365mb.c b/drivers/net/dsa/rtl8365mb.c > index baaae97283c5..f4414ac74b61 100644 > --- a/drivers/net/dsa/rtl8365mb.c > +++ b/drivers/net/dsa/rtl8365mb.c > @@ -107,6 +107,7 @@ > #define RTL8365MB_LEARN_LIMIT_MAX_8365MB_VC 2112 > > /* Family-specific data and limits */ > +#define RTL8365MB_PHYADDRMAX 7 > #define RTL8365MB_NUM_PHYREGS 32 > #define RTL8365MB_PHYREGMAX (RTL8365MB_NUM_PHYREGS - 1) > #define RTL8365MB_MAX_NUM_PORTS (RTL8365MB_CPU_PORT_NUM_8365MB_VC + 1) > @@ -176,7 +177,7 @@ > #define RTL8365MB_INDIRECT_ACCESS_STATUS_REG 0x1F01 > #define RTL8365MB_INDIRECT_ACCESS_ADDRESS_REG 0x1F02 > #define RTL8365MB_INDIRECT_ACCESS_ADDRESS_OCPADR_5_1_MASK GENMASK(4, 0) > -#define RTL8365MB_INDIRECT_ACCESS_ADDRESS_PHYNUM_MASK GENMASK(6, 5) > +#define RTL8365MB_INDIRECT_ACCESS_ADDRESS_PHYNUM_MASK GENMASK(7, 5) > #define RTL8365MB_INDIRECT_ACCESS_ADDRESS_OCPADR_9_6_MASK GENMASK(11, 8) > #define RTL8365MB_PHY_BASE 0x2000 > #define RTL8365MB_INDIRECT_ACCESS_WRITE_DATA_REG 0x1F03 > @@ -679,6 +680,8 @@ static int rtl8365mb_phy_read(struct realtek_smi *smi, int phy, int regnum) > u16 val; > int ret; > > + if (phy > RTL8365MB_PHYADDRMAX) > + return -EINVAL; > if (regnum > RTL8365MB_PHYREGMAX) If you decide to keep these check, please add a newline after your returns. > return -EINVAL; > > @@ -704,6 +707,8 @@ static int rtl8365mb_phy_write(struct realtek_smi *smi, int phy, int regnum, > u32 ocp_addr; > int ret; > > + if (phy > RTL8365MB_PHYADDRMAX) > + return -EINVAL; > if (regnum > RTL8365MB_PHYREGMAX) > return -EINVAL; > >
> Hi Luiz, > > Thanks for your patch. > > You needn't cc the stable list, since rtl8365mb doesn't exist in any > stable release just yet. If you send a v3, just target net: > > [PATCH net v3] net: dsa: ... > > Then the fix will land in 5.16. :-) > OK. Thanks. Newbie mistakes... > On 11/26/21 09:12, luizluca@gmail.com wrote: > > From: Luiz Angelo Daros de Luca <luizluca@gmail.com> > > > > This switch family can have up to 8 ports {0..7}. However, > > INDIRECT_ACCESS_ADDRESS_PHYNUM_MASK was using 2 bits instead of 3, > > dropping the most significant bit during indirect register reads and > > writes. Reading or writing ports 4, 5, 6, and 7 registers was actually > > manipulating, respectively, ports 0, 1, 2, and 3 registers. > > Nice catch. Out of curiosity can you share what switch you are testing? > So far the driver only advertises support for RTL8365MB-VC. Since that > switch only uses PHY addresses 0~3, it shouldn't be affected by a > narrower (2-bit) PHYNUM_MASK, right? Are you able to add words to the > effect of "... now this fixes the driver to work with RTL83xxxx"? RTL8365MB-VC is one of the smallest one. It was not affected by this issue as all 4 UTP ports are below 4. I have a RTL8367S with 5 ports+CPU. It took me some days to figure out why wan port (4) was not working and why indirect register access was mirroring the status from port 0. I'm finishing two improvements to the realtek switch driver: 1) realtek mdio interface 2) RTL8367S support (modifying RTL8365MB-VC driver) Here is my current status: https://github.com/luizluca/linux/tree/realtek_mdio_refactor (it is a moving target as I occasionally do forced pushes) I still don't know how to relate cpu_port to ext_port. I have port 7 and ext_int 2 but that same port 7 would be an UTP port in another variant. Is there a fixed relation between ext interfaces and port numbers for each variant? Or is this something defined by the board configuration? For now, I'm using DSA to determine the CPU port and I added a new DT property to define the external interface. rtl8367c_phy_mode_supported also needs to be fixed. For now, it simply returns true as the default mode is what I needed. There are also dozens of initializations that the original realtek device uses like disabling EEE and Gigabit lite that I'm not sure if we need. I was thinking about adding an DT array property to allow extra initializations without a kernel rebuild. With a little more effort, this driver could be able to support other switch variants only providing extra DT properties. While looking for the cause of wan port not working, I was mapping each init table value to a readable defined value but I'm not sure if it is desired to expand the code with that much information. This commit is outside my tree for now. > The change is OK except for some comments below: > > > > > rtl8365mb_phy_{read,write} will now returns -EINVAL if phy is greater > > than 7. > > I don't think this is really necessary: a valid (device tree) > configuration should never specify a PHY with address greater than 7. Or > am I missing something? No, you are correct. A correct DT will never access those register out of range. However, DT is written by humans. This family supports up to 10 ports (8 UTP + 2 EXT). If someone define a port 8 or 9 in device tree as an UTP port, the function will silently return port 0 data for port 8. The error returned here could save some hours. I'll keep that as it is a cheap test for a low traffic function. Maybe we should also check if the port is not, according to device tree, an ext port (fixed-link?) and fail right if it is port 8 or 9. > > > > > v2: > > - fix affected ports in commit message > > The changelog shouldn't end up in the final commit message - please move > it out in v3. OK > > > > > Fixes: 4af2950c50c8 ("net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC") > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > > --- > > drivers/net/dsa/rtl8365mb.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/dsa/rtl8365mb.c b/drivers/net/dsa/rtl8365mb.c > > index baaae97283c5..f4414ac74b61 100644 > > --- a/drivers/net/dsa/rtl8365mb.c > > +++ b/drivers/net/dsa/rtl8365mb.c > > @@ -107,6 +107,7 @@ > > #define RTL8365MB_LEARN_LIMIT_MAX_8365MB_VC 2112 > > > > /* Family-specific data and limits */ > > +#define RTL8365MB_PHYADDRMAX 7 > > #define RTL8365MB_NUM_PHYREGS 32 > > #define RTL8365MB_PHYREGMAX (RTL8365MB_NUM_PHYREGS - 1) > > #define RTL8365MB_MAX_NUM_PORTS (RTL8365MB_CPU_PORT_NUM_8365MB_VC + 1) > > @@ -176,7 +177,7 @@ > > #define RTL8365MB_INDIRECT_ACCESS_STATUS_REG 0x1F01 > > #define RTL8365MB_INDIRECT_ACCESS_ADDRESS_REG 0x1F02 > > #define RTL8365MB_INDIRECT_ACCESS_ADDRESS_OCPADR_5_1_MASK GENMASK(4, 0) > > -#define RTL8365MB_INDIRECT_ACCESS_ADDRESS_PHYNUM_MASK GENMASK(6, 5) > > +#define RTL8365MB_INDIRECT_ACCESS_ADDRESS_PHYNUM_MASK GENMASK(7, 5) > > #define RTL8365MB_INDIRECT_ACCESS_ADDRESS_OCPADR_9_6_MASK GENMASK(11, 8) > > #define RTL8365MB_PHY_BASE 0x2000 > > #define RTL8365MB_INDIRECT_ACCESS_WRITE_DATA_REG 0x1F03 > > @@ -679,6 +680,8 @@ static int rtl8365mb_phy_read(struct realtek_smi *smi, int phy, int regnum) > > u16 val; > > int ret; > > > > + if (phy > RTL8365MB_PHYADDRMAX) > > + return -EINVAL; > > if (regnum > RTL8365MB_PHYREGMAX) > > If you decide to keep these check, please add a newline after your returns. > > > return -EINVAL; > > > > @@ -704,6 +707,8 @@ static int rtl8365mb_phy_write(struct realtek_smi *smi, int phy, int regnum, > > u32 ocp_addr; > > int ret; > > > > + if (phy > RTL8365MB_PHYADDRMAX) > > + return -EINVAL; > > if (regnum > RTL8365MB_PHYREGMAX) > > return -EINVAL; > > > > >
diff --git a/drivers/net/dsa/rtl8365mb.c b/drivers/net/dsa/rtl8365mb.c index baaae97283c5..f4414ac74b61 100644 --- a/drivers/net/dsa/rtl8365mb.c +++ b/drivers/net/dsa/rtl8365mb.c @@ -107,6 +107,7 @@ #define RTL8365MB_LEARN_LIMIT_MAX_8365MB_VC 2112 /* Family-specific data and limits */ +#define RTL8365MB_PHYADDRMAX 7 #define RTL8365MB_NUM_PHYREGS 32 #define RTL8365MB_PHYREGMAX (RTL8365MB_NUM_PHYREGS - 1) #define RTL8365MB_MAX_NUM_PORTS (RTL8365MB_CPU_PORT_NUM_8365MB_VC + 1) @@ -176,7 +177,7 @@ #define RTL8365MB_INDIRECT_ACCESS_STATUS_REG 0x1F01 #define RTL8365MB_INDIRECT_ACCESS_ADDRESS_REG 0x1F02 #define RTL8365MB_INDIRECT_ACCESS_ADDRESS_OCPADR_5_1_MASK GENMASK(4, 0) -#define RTL8365MB_INDIRECT_ACCESS_ADDRESS_PHYNUM_MASK GENMASK(6, 5) +#define RTL8365MB_INDIRECT_ACCESS_ADDRESS_PHYNUM_MASK GENMASK(7, 5) #define RTL8365MB_INDIRECT_ACCESS_ADDRESS_OCPADR_9_6_MASK GENMASK(11, 8) #define RTL8365MB_PHY_BASE 0x2000 #define RTL8365MB_INDIRECT_ACCESS_WRITE_DATA_REG 0x1F03 @@ -679,6 +680,8 @@ static int rtl8365mb_phy_read(struct realtek_smi *smi, int phy, int regnum) u16 val; int ret; + if (phy > RTL8365MB_PHYADDRMAX) + return -EINVAL; if (regnum > RTL8365MB_PHYREGMAX) return -EINVAL; @@ -704,6 +707,8 @@ static int rtl8365mb_phy_write(struct realtek_smi *smi, int phy, int regnum, u32 ocp_addr; int ret; + if (phy > RTL8365MB_PHYADDRMAX) + return -EINVAL; if (regnum > RTL8365MB_PHYREGMAX) return -EINVAL;