[PATCH v2 2/2] realtek: use assisted learning on CPU port

[Sander Vanheule]

On Fri, 2022-10-28 at 20:26 +0200, Jan Hoffmann wrote:
> 
> On 26.10.22 at 23:39, Jan Hoffmann wrote:
> > On 26.10.22 at 00:20, Jan Hoffmann wrote:
> > > --- a/target/linux/realtek/files-5.10/drivers/net/dsa/rtl83xx/dsa.c
> > > +++ b/target/linux/realtek/files-5.10/drivers/net/dsa/rtl83xx/dsa.c
> > > @@ -162,6 +162,16 @@ static void rtl83xx_setup_bpdu_traps(struct 
> > > rtl838x_switch_priv *priv)
> > >           priv->r->set_receive_management_action(i, BPDU, COPY2CPU);
> > >   }
> > > +static void rtl83xx_port_set_salrn(struct rtl838x_switch_priv *priv,
> > > +                   int port, bool enable)
> > > +{
> > > +    int shift = SALRN_PORT_SHIFT(port);
> > > +    int val = enable ? SALRN_MODE_HARDWARE : SALRN_MODE_DISABLED;
> > 
> > While looking at the patch again, I noticed that the type of these 
> > variables should probably be u32 (to avoid possible undefined behaviour 
> > when shift is 30).
> > 
> > As the patch is already accepted now, I'll send a separate fix.
> 
> So I looked into this a bit further. In standard C, there would still be 
> undefined behaviour even after changing the variable type. This is 
> because the part "SALRN_MODE_MASK << shift" is still signed because of 
> the constant. To also avoid that, it would be necessary to either add a 
> cast, or to define the value as 0x3U.
> 
> However, as the kernel is built with -fno-strict-overflow none of this 
> actually matters. With that flag, UBSAN also doesn't emit the warning it 
> otherwise would for this code.
> 
> So in the end, there isn't really an issue. Using an int for val still 
> doesn't quite look right to me, but at this point I'm not sure if that 
> warrants an additional commit to change it.

Unless you already have something you can fold this change into, I'm fine with a
small fixup patch.


> However, looking at this was useful in any case, because UBSAN also 
> found a real bug in another place:
> 
> [    2.880000] UBSAN: shift-out-of-bounds in 
> drivers/net/phy/mdio_bus.c:577:27
> [    2.910000] shift exponent 32 is too large for 32-bit type 'int'
> 
> The issue here isn't just that shift. The patch 
> "705-include-linux-phy-increase-phy-address-number-for-rtl839x.patch" 
> increases PHY_MAX_ADDR from 32 to 64, but it it doesn't adjust any of 
> the 32 bit PHY mask fields.
> 
> It looks like this is going to at least require changes to phy_mask and 
> phy_ignore_ta_mask in struct mii_bus and struct mdio_gpio_platform_data, 
> as well as phys_mii_mask in struct dsa_switch (and of course all of the 
> places where those are used also need to be checked).
> 
> (On the HPE 1920-48G (RTL8393) I've seen a few other UBSAN warnings, but 
> at least some of them look like they are due to the same issue, so it is 
> probably best to check again after this is fixed.)
> 
> 

The driver should rather stop pretending MDIO addresses have 6 bits on Realtek
switches. MDIO frames have 5 address bits, so PHY_MAX_ADDR *is* 32.

Instead of trying to adjust the kernel so it can live in the same alternative
MDIO reality, the driver should be adjusted to provide two (virtual) MDIO busses
IMHO.

For example, the RTL8214FC has 5 strapping pins to configure the base MDIO
address. You appear to have these configured at phy addresses 48-51 on the HPE
1920-48 [1]. Having only five strapping pins, those high addresses don't make
much sense to me.

[1] https://github.com/janh/openwrt/commit/f280d2417d10

Best,
Sander