Re: [PATCH v1 21/26] net: dsa: microchip: ksz8_r_sta_mac_table(): do not use error code for empty entries

[Arun.Ramadoss]

On Mon, 2022-11-28 at 12:59 +0100, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> This is a preparation for the next patch, to make use of read/write
> errors.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> ---
>  drivers/net/dsa/microchip/ksz8795.c | 94 +++++++++++++++++--------
> ----
>  1 file changed, 54 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8795.c
> b/drivers/net/dsa/microchip/ksz8795.c
> index 1c08103c9f50..b7487be91f67 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -457,7 +457,7 @@ static int ksz8_r_dyn_mac_table(struct ksz_device
> *dev, u16 addr, u8 *mac_addr,
>  }
> 
>  static int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
> -                               struct alu_struct *alu)
> +                               struct alu_struct *alu, bool *valid)
>  {
>         u32 data_hi, data_lo;
>         const u8 *shifts;
> @@ -470,28 +470,32 @@ static int ksz8_r_sta_mac_table(struct
> ksz_device *dev, u16 addr,
>         ksz8_r_table(dev, TABLE_STATIC_MAC, addr, &data);
>         data_hi = data >> 32;
>         data_lo = (u32)data;
> -       if (data_hi & (masks[STATIC_MAC_TABLE_VALID] |
> -                       masks[STATIC_MAC_TABLE_OVERRIDE])) {
> -               alu->mac[5] = (u8)data_lo;
> -               alu->mac[4] = (u8)(data_lo >> 8);
> -               alu->mac[3] = (u8)(data_lo >> 16);
> -               alu->mac[2] = (u8)(data_lo >> 24);
> -               alu->mac[1] = (u8)data_hi;
> -               alu->mac[0] = (u8)(data_hi >> 8);
> -               alu->port_forward =
> -                       (data_hi & masks[STATIC_MAC_TABLE_FWD_PORTS])
> >>
> -                               shifts[STATIC_MAC_FWD_PORTS];
> -               alu->is_override =
> -                       (data_hi & masks[STATIC_MAC_TABLE_OVERRIDE])
> ? 1 : 0;
> -               data_hi >>= 1;
> -               alu->is_static = true;
> -               alu->is_use_fid =
> -                       (data_hi & masks[STATIC_MAC_TABLE_USE_FID]) ?
> 1 : 0;
> -               alu->fid = (data_hi & masks[STATIC_MAC_TABLE_FID]) >>
> -                               shifts[STATIC_MAC_FID];
> +
> +       if (!(data_hi & (masks[STATIC_MAC_TABLE_VALID] |
> +                        masks[STATIC_MAC_TABLE_OVERRIDE]))) {
> +               *valid = false;
>                 return 0;
>         }
> -       return -ENXIO;
> +
> +       alu->mac[5] = (u8)data_lo;
> +       alu->mac[4] = (u8)(data_lo >> 8);
> +       alu->mac[3] = (u8)(data_lo >> 16);
> +       alu->mac[2] = (u8)(data_lo >> 24);
> +       alu->mac[1] = (u8)data_hi;
> +       alu->mac[0] = (u8)(data_hi >> 8);

u64_to_ether_addr macro can be used to store address into array.

> +       alu->port_forward =
> +               (data_hi & masks[STATIC_MAC_TABLE_FWD_PORTS]) >>
> +                       shifts[STATIC_MAC_FWD_PORTS];



> +       alu->is_override = (data_hi &
> masks[STATIC_MAC_TABLE_OVERRIDE]) ? 1 : 0;
> +       data_hi >>= 1;
> +       alu->is_static = true;
> +       alu->is_use_fid = (data_hi & masks[STATIC_MAC_TABLE_USE_FID])
> ? 1 : 0;
> +       alu->fid = (data_hi & masks[STATIC_MAC_TABLE_FID]) >>
> +               shifts[STATIC_MAC_FID];

Instead of masks and shifts, you consider using
FIELD_GET(STATIC_MAC_TABLE_FID, data_hi);

> +
> +       *valid = true;
> +
> +       return 0;
>  }
> 
>  void ksz8_w_sta_mac_table(struct ksz_device *dev, u16 addr,
> @@ -969,12 +973,13 @@ int ksz8_fdb_dump(struct ksz_device *dev, int
> port,
> 
>         for (i = 0; i  < dev->info->num_statics; i++) {
>                 struct alu_struct alu;
> +               bool valid;
> 
> -               ret = ksz8_r_sta_mac_table(dev, i, &alu);
> -               if (ret == -ENXIO)
> -                       continue;
> +               ret = ksz8_r_sta_mac_table(dev, i, &alu, &valid);
>                 if (ret)
>                         return ret;
> +               if (!valid)
> +                       continue;
> 
>                 if (!(alu.port_forward & BIT(port)))
>                         continue;
> @@ -1010,20 +1015,25 @@ static int ksz8_add_sta_mac(struct ksz_device
> *dev, int port,
>                             const unsigned char *addr, u16 vid)
>  {
>         struct alu_struct alu;
> -       int index;
> +       int index, ret;
>         int empty = 0;
> 
>         alu.port_forward = 0;
>         for (index = 0; index < dev->info->num_statics; index++) {
> -               if (!ksz8_r_sta_mac_table(dev, index, &alu)) {
> -                       /* Found one already in static MAC table. */
> -                       if (!memcmp(alu.mac, addr, ETH_ALEN) &&
> -                           alu.fid == vid)
> -                               break;
> -               /* Remember the first empty entry. */
> -               } else if (!empty) {
> -                       empty = index + 1;
> +               bool valid;
> +
> +               ret = ksz8_r_sta_mac_table(dev, index, &alu, &valid);
> +               if (ret)
> +                       return ret;
> +               if (!valid) {
> +                       /* Remember the first empty entry. */
> +                       if (!empty)
> +                               empty = index + 1;
> +                       continue;
>                 }
> +
> +               if (!memcmp(alu.mac, addr, ETH_ALEN) && alu.fid ==
> vid)
> +                       break;
>         }
> 
>         /* no available entry */
> @@ -1053,15 +1063,19 @@ static int ksz8_del_sta_mac(struct ksz_device
> *dev, int port,
>                             const unsigned char *addr, u16 vid)
>  {
>         struct alu_struct alu;
> -       int index;
> +       int index, ret;

Variable declaration in separate line.

> 
>         for (index = 0; index < dev->info->num_statics; index++) {
> -               if (!ksz8_r_sta_mac_table(dev, index, &alu)) {
> -                       /* Found one already in static MAC table. */
> -                       if (!memcmp(alu.mac, addr, ETH_ALEN) &&
> -                           alu.fid == vid)
> -                               break;
> -               }
> +               bool valid;
> +
> +               ret = ksz8_r_sta_mac_table(dev, index, &alu, &valid);
> +               if (ret)
> +                       return ret;
> +               if (!valid)
> +                       continue;
> +
> +               if (!memcmp(alu.mac, addr, ETH_ALEN) && alu.fid ==
> vid)
> +                       break;
>         }
> 
>         /* no available entry */
> --
> 2.30.2
>