[v1,21/26] net: dsa: microchip: ksz8_r_sta_mac_table(): do not use error code for empty entries
Commit Message
This is a preparation for the next patch, to make use of read/write errors.
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
drivers/net/dsa/microchip/ksz8795.c | 94 +++++++++++++++++------------
1 file changed, 54 insertions(+), 40 deletions(-)
Comments
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@pengutronix.de>
> ---
> 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
>
On Tue, Nov 29, 2022 at 05:25:55AM +0000, Arun.Ramadoss@microchip.com 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@pengutronix.de>
> > ---
> > 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
> >
@@ -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);
+ 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];
+
+ *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;
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 */