Re: [PATCH v1 21/26] net: dsa: microchip: ksz8_r_sta_mac_table(): do not use error code for empty entries
From: Oleksij Rempel
Date: Tue Nov 29 2022 - 00:56:39 EST
On Tue, Nov 29, 2022 at 05:25:55AM +0000, Arun.Ramadoss@xxxxxxxxxxxxx wrote:
> 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.
This should not be done in this patch.
> > + 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);
I would love to do this, but in this case, complete driver should be
splittet in to KSZ88X3 and KSZ879X portions. FIELD_GET can't work with
dynamic masks and shifts.
I would say, it is not for this patch set.
> > +
> > + *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.
Is it a new coding style requirement? I can't find it here:
https://www.kernel.org/doc/html/v6.0/process/coding-style.html
> >
> > 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
> >
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |