Re: [net-next PATCH v4 7/8] net: dsa: realtek: rtl8365mb: add FDB support

[Luiz Angelo Daros de Luca]

> > +
> > +struct rtl8365mb_l2_mc_key {
> > +     u8 mac_addr[ETH_ALEN];
> > +     union {
> > +             u16 vid; /* IVL */
> > +             u16 fid; /* SVL */
> > +     };
> > +     bool ivl;
> > +};
> > +
> > +struct rtl8365mb_l2_mc {
> > +     struct rtl8365mb_l2_mc_key key;
> > +     u16 member;
> > +     u8 priority;
> > +     u8 igmpidx;
> > +
> > +     bool is_static;
> > +     bool fwd_pri;
> > +     bool igmp_asic;
> > +};
> > +
> > +static void rtl8365mb_l2_data_to_uc(const u16 *data, struct rtl8365mb_l2_uc *uc)
> > +{
> > +     u32 val;
> > +
> > +     uc->key.mac_addr[5] = FIELD_GET(RTL8365MB_L2_UC_D0_MAC5_MSK, data[0]);
> > +     uc->key.mac_addr[4] = FIELD_GET(RTL8365MB_L2_UC_D0_MAC4_MSK, data[0]);
> > +     uc->key.mac_addr[3] = FIELD_GET(RTL8365MB_L2_UC_D1_MAC3_MSK, data[1]);
> > +     uc->key.mac_addr[2] = FIELD_GET(RTL8365MB_L2_UC_D1_MAC2_MSK, data[1]);
> > +     uc->key.mac_addr[1] = FIELD_GET(RTL8365MB_L2_UC_D2_MAC1_MSK, data[2]);
> > +     uc->key.mac_addr[0] = FIELD_GET(RTL8365MB_L2_UC_D2_MAC0_MSK, data[2]);
> > +     uc->key.efid = FIELD_GET(RTL8365MB_L2_UC_D4_EFID_MSK, data[4]);
> > +     uc->key.vid = FIELD_GET(RTL8365MB_L2_UC_D3_VID_MSK, data[3]);
> > +     uc->key.ivl = FIELD_GET(RTL8365MB_L2_UC_D3_IVL_MSK, data[3]);
> > +     uc->key.fid = FIELD_GET(RTL8365MB_L2_UC_D4_FID_MSK, data[4]);
> > +     uc->age = FIELD_GET(RTL8365MB_L2_UC_D4_AGE_MSK, data[4]);
> > +     uc->auth = FIELD_GET(RTL8365MB_L2_UC_D4_AUTH_MSK, data[4]);
> > +
>
> The problem with overwriting uc->key.fid and uc->key.vid values in the union still exists.

Hello Mieczysław,

Thanks for the review. Please note that rtl8365mb_l2_data_to_uc
operates on the unicast structure (struct rtl8365mb_l2_uc), rather
than the multicast structure (struct rtl8365mb_l2_mc) quoted in your
comment.

For reference, here is the definition of rtl8365mb_l2_uc and its key:

+struct rtl8365mb_l2_uc_key {
+       u8 mac_addr[ETH_ALEN];
+       u16 vid;
+       u16 fid;
+       bool ivl;
+       u16 efid;
+};
+
+struct rtl8365mb_l2_uc {
+       struct rtl8365mb_l2_uc_key key;
+       u8 port;
+       u8 age;
+       u8 priority;
+
+       bool sa_block;
+       bool da_block;
+       bool auth;
+       bool is_static;
+       bool sa_pri;
+       bool fwd_pri;
+};

As you can see, there is no union in the unicast key. The hardware
table entry has independent fields for efid, fid, and vid (though in
practice, vendor examples always show one of fid or vid set to 0).

The multicast entry (struct rtl8365mb_l2_mc) is the only one that uses
a union for the dual-purpose field, which is interpreted as either fid
or vid depending on the ivl value.

Regards,

Luiz