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

[Jan Hoffmann]

On 26.10.22 at 23:39, Jan Hoffmann wrote:
> On 26.10.22 at 00:20, Jan Hoffmann wrote:
>> L2 learning on the CPU port is currently not consistently configured and
>> relies on the default configuration of the device. On RTL83xx, it is
>> disabled for packets transmitted with a TX header, as hardware learning
>> corrupts the forwarding table otherwise. As a result, unneeded flooding
>> of traffic for the CPU port can already happen on some devices now. It
>> is also likely that similar issues exist on RTL93xx, which doesn't have
>> a field to disable learning in the TX header.
>>
>> To address this, disable hardware learning for the CPU port globally on
>> all devices. Instead, enable assisted learning to let DSA write FDB
>> entries to the switch.
>>
>> For now, this does not sync local/bridge entries to the switch. However,
>> support for that was added in Linux 5.14, so the next switch to a newer
>> kernel version is going to fix this.
>>
>> Signed-off-by: Jan Hoffmann <jan at 3e8.eu>
>> ---
>>   .../files-5.10/drivers/net/dsa/rtl83xx/dsa.c     | 16 ++++++++++++++++
>>   .../files-5.10/drivers/net/dsa/rtl83xx/rtl838x.h |  6 ++++++
>>   2 files changed, 22 insertions(+)
>>
>> diff --git 
>> 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
>> index 474d540945d1..319f171eaacc 100644
>> --- 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.


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.)


>> +
>> +    sw_w32_mask(SALRN_MODE_MASK << shift, val << shift,
>> +            priv->r->l2_port_new_salrn(port));
>> +}
>> +
>>   static int rtl83xx_setup(struct dsa_switch *ds)
>>   {
>>       int i;
>> @@ -205,6 +215,9 @@ static int rtl83xx_setup(struct dsa_switch *ds)
>>       priv->r->l2_learning_setup();
>> +    rtl83xx_port_set_salrn(priv, priv->cpu_port, false);
>> +    ds->assisted_learning_on_cpu_port = true;
>> +
>>       /*
>>        *  Make sure all frames sent to the switch's MAC are trapped to 
>> the CPU-port
>>        *  0: FWD, 1: DROP, 2: TRAP2CPU
>> @@ -263,6 +276,9 @@ static int rtl93xx_setup(struct dsa_switch *ds)
>>       priv->r->l2_learning_setup();
>> +    rtl83xx_port_set_salrn(priv, priv->cpu_port, false);
>> +    ds->assisted_learning_on_cpu_port = true;
>> +
>>       rtl83xx_enable_phy_polling(priv);
>>       priv->r->pie_init(priv);
>> diff --git 
>> a/target/linux/realtek/files-5.10/drivers/net/dsa/rtl83xx/rtl838x.h 
>> b/target/linux/realtek/files-5.10/drivers/net/dsa/rtl83xx/rtl838x.h
>> index e2b82a4975f0..10913dacef42 100644
>> --- a/target/linux/realtek/files-5.10/drivers/net/dsa/rtl83xx/rtl838x.h
>> +++ b/target/linux/realtek/files-5.10/drivers/net/dsa/rtl83xx/rtl838x.h
>> @@ -245,6 +245,12 @@
>>   #define RTL839X_L2_PORT_NEW_SALRN(p)        (0x38F0 + (((p >> 4) << 
>> 2)))
>>   #define RTL930X_L2_PORT_SALRN(p)        (0x8FEC + (((p >> 4) << 2)))
>>   #define RTL931X_L2_PORT_NEW_SALRN(p)        (0xC820 + (((p >> 4) << 
>> 2)))
>> +
>> +#define SALRN_PORT_SHIFT(p)            ((p % 16) * 2)
>> +#define SALRN_MODE_MASK                0x3
>> +#define SALRN_MODE_HARDWARE            0
>> +#define SALRN_MODE_DISABLED            2
>> +
>>   #define RTL838X_L2_PORT_NEW_SA_FWD(p)        (0x3294 + (((p >> 4) << 
>> 2)))
>>   #define RTL839X_L2_PORT_NEW_SA_FWD(p)        (0x3900 + (((p >> 4) << 
>> 2)))
>>   #define RTL930X_L2_PORT_NEW_SA_FWD(p)        (0x8FF4 + (((p / 10) << 
>> 2)))