2023-03-22 14:36:58

by Oleksij Rempel

[permalink] [raw]
Subject: [PATCH net v1 2/6] net: dsa: microchip: ksz8: fix ksz8_fdb_dump() to extract all 1024 entries

Current ksz8_fdb_dump() is able to extract only max 249 entries on
the ksz8863/ksz8873 series of switches. This happened due to wrong
bit mask and offset calculation.

This commit corrects the issue and allows for the complete extraction of
all 1024 entries.

Fixes: d23a5e18606c ("net: dsa: microchip: move ksz8->masks to ksz_common")
Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/net/dsa/microchip/ksz_common.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 7fc2155d93d6..3a1afc9f4621 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -407,10 +407,10 @@ static const u32 ksz8863_masks[] = {
[STATIC_MAC_TABLE_FID] = GENMASK(29, 26),
[STATIC_MAC_TABLE_OVERRIDE] = BIT(20),
[STATIC_MAC_TABLE_FWD_PORTS] = GENMASK(18, 16),
- [DYNAMIC_MAC_TABLE_ENTRIES_H] = GENMASK(5, 0),
+ [DYNAMIC_MAC_TABLE_ENTRIES_H] = GENMASK(1, 0),
[DYNAMIC_MAC_TABLE_MAC_EMPTY] = BIT(7),
[DYNAMIC_MAC_TABLE_NOT_READY] = BIT(7),
- [DYNAMIC_MAC_TABLE_ENTRIES] = GENMASK(31, 28),
+ [DYNAMIC_MAC_TABLE_ENTRIES] = GENMASK(31, 24),
[DYNAMIC_MAC_TABLE_FID] = GENMASK(19, 16),
[DYNAMIC_MAC_TABLE_SRC_PORT] = GENMASK(21, 20),
[DYNAMIC_MAC_TABLE_TIMESTAMP] = GENMASK(23, 22),
@@ -420,7 +420,7 @@ static u8 ksz8863_shifts[] = {
[VLAN_TABLE_MEMBERSHIP_S] = 16,
[STATIC_MAC_FWD_PORTS] = 16,
[STATIC_MAC_FID] = 22,
- [DYNAMIC_MAC_ENTRIES_H] = 3,
+ [DYNAMIC_MAC_ENTRIES_H] = 8,
[DYNAMIC_MAC_ENTRIES] = 24,
[DYNAMIC_MAC_FID] = 16,
[DYNAMIC_MAC_TIMESTAMP] = 24,
--
2.30.2


2023-03-23 22:49:51

by Jakub Kicinski

[permalink] [raw]
Subject: Re: [PATCH net v1 2/6] net: dsa: microchip: ksz8: fix ksz8_fdb_dump() to extract all 1024 entries

On Wed, 22 Mar 2023 15:31:26 +0100 Oleksij Rempel wrote:
> Fixes: d23a5e18606c ("net: dsa: microchip: move ksz8->masks to ksz_common")

The code move broke it? Looks like it was 5,0 before and 5,0 after
the change? We need a real tag, pointing to where the code was first
added.

Any reason you didn't CC Arun, just an omission or they're no longer
@microchip?

Arun, would you be able to review this series?

2023-03-24 04:06:28

by Arun Ramadoss

[permalink] [raw]
Subject: Re: [PATCH net v1 2/6] net: dsa: microchip: ksz8: fix ksz8_fdb_dump() to extract all 1024 entries

Hi Oleksij,

On Wed, 2023-03-22 at 15:31 +0100, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> Current ksz8_fdb_dump() is able to extract only max 249 entries on
> the ksz8863/ksz8873 series of switches. This happened due to wrong
> bit mask and offset calculation.
>
> This commit corrects the issue and allows for the complete extraction
> of
> all 1024 entries.
>
> Fixes: d23a5e18606c ("net: dsa: microchip: move ksz8->masks to
> ksz_common")
> Signed-off-by: Oleksij Rempel <[email protected]>
> ---
> drivers/net/dsa/microchip/ksz_common.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/dsa/microchip/ksz_common.c
> b/drivers/net/dsa/microchip/ksz_common.c
> index 7fc2155d93d6..3a1afc9f4621 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -407,10 +407,10 @@ static const u32 ksz8863_masks[] = {
> [STATIC_MAC_TABLE_FID] = GENMASK(29, 26),
> [STATIC_MAC_TABLE_OVERRIDE] = BIT(20),
> [STATIC_MAC_TABLE_FWD_PORTS] = GENMASK(18, 16),
> - [DYNAMIC_MAC_TABLE_ENTRIES_H] = GENMASK(5, 0),
> + [DYNAMIC_MAC_TABLE_ENTRIES_H] = GENMASK(1, 0),
> [DYNAMIC_MAC_TABLE_MAC_EMPTY] = BIT(7),
> [DYNAMIC_MAC_TABLE_NOT_READY] = BIT(7),
> - [DYNAMIC_MAC_TABLE_ENTRIES] = GENMASK(31, 28),
> + [DYNAMIC_MAC_TABLE_ENTRIES] = GENMASK(31, 24),
> [DYNAMIC_MAC_TABLE_FID] = GENMASK(19, 16),
> [DYNAMIC_MAC_TABLE_SRC_PORT] = GENMASK(21, 20),
> [DYNAMIC_MAC_TABLE_TIMESTAMP] = GENMASK(23, 22),
> @@ -420,7 +420,7 @@ static u8 ksz8863_shifts[] = {
> [VLAN_TABLE_MEMBERSHIP_S] = 16,
> [STATIC_MAC_FWD_PORTS] = 16,
> [STATIC_MAC_FID] = 22,
> - [DYNAMIC_MAC_ENTRIES_H] = 3,
> + [DYNAMIC_MAC_ENTRIES_H] = 8,
> [DYNAMIC_MAC_ENTRIES] = 24,
> [DYNAMIC_MAC_FID] = 16,
> [DYNAMIC_MAC_TIMESTAMP] = 24,

Cross verified the above entries with datasheet.

As Jakub mentioned, above fix commit is just code movement from
ksz8795.c to ksz_common.

Other than Fix commit, patch Looks good to me.

Acked-by: Arun Ramadoss <[email protected]>

> --
> 2.30.2
>

2023-03-24 06:10:53

by Oleksij Rempel

[permalink] [raw]
Subject: Re: [PATCH net v1 2/6] net: dsa: microchip: ksz8: fix ksz8_fdb_dump() to extract all 1024 entries

On Thu, Mar 23, 2023 at 03:41:01PM -0700, Jakub Kicinski wrote:
> On Wed, 22 Mar 2023 15:31:26 +0100 Oleksij Rempel wrote:
> > Fixes: d23a5e18606c ("net: dsa: microchip: move ksz8->masks to ksz_common")
>
> The code move broke it? Looks like it was 5,0 before and 5,0 after
> the change? We need a real tag, pointing to where the code was first
> added.

ack. will fix it.

> Any reason you didn't CC Arun, just an omission or they're no longer
> @microchip?

He is not in MAINTAINERS for drivers/net/dsa/microchip/* even if he is
practically maintaining it .. :)

> Arun, would you be able to review this series?

Regards,
Oleksij
--
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 |

2023-03-24 16:51:04

by Jakub Kicinski

[permalink] [raw]
Subject: Re: [PATCH net v1 2/6] net: dsa: microchip: ksz8: fix ksz8_fdb_dump() to extract all 1024 entries

On Fri, 24 Mar 2023 06:35:12 +0100 Oleksij Rempel wrote:
> > Any reason you didn't CC Arun, just an omission or they're no longer
> > @microchip?
>
> He is not in MAINTAINERS for drivers/net/dsa/microchip/* even if he is
> practically maintaining it .. :)

get_maintainer is occasionally useful in pointing out people who wrote
the code but mostly the authors of code under Fixes. I use this little
script usually:


#!/usr/bin/env python3

import argparse
import fileinput
import subprocess
import tempfile
import sys
import os
import re

emailpat = re.compile(r'([^ <"]*@[^ >"]*)')
skip = {'[email protected]',
'[email protected]',
'[email protected]',
'[email protected]',
'[email protected]',
'[email protected]'}


def do(lines):
ret = ['---']

for line in lines:
line = line.strip()
if not line:
continue

ret.append('# ' + line)

if "moderated" in line:
ret.append('# skip, moderated')
continue

match = emailpat.search(line)
if match:
addr = match.group(1)
if addr in skip:
ret.append('# skip, always-cc')

else:
ret.append('CC: ' + addr)
else:
ret.append('# Bad line')

return ret


def run(cmd):
p = subprocess.run(cmd, capture_output=True, check=True)
return p.stdout.decode("utf-8").strip()


def git_commit_msg():
return run(["git", "show", "--format=%B", "--no-patch"])


def git_commit(filename):
return run(["git", "commit", "--amend", "-F", filename])


def git_patch_format():
return run(["git", "format-patch", "HEAD~", "-o", "/tmp/"])


def get_maint(patch_file):
return run(["./scripts/get_maintainer.pl",
"--git-min-percent", "30", patch_file])


def main():
parser = argparse.ArgumentParser()
parser.add_argument('--stdin',
help="Read the get_maintainer output from stdin",
action='store_true')
parser.add_argument('--inline', help="Amend HEAD directly",
action='store_true')
args = parser.parse_args()

if args.stdin:
out = do(sys.stdin.readlines())
elif args.inline:
msg = git_commit_msg()

patch_file = git_patch_format()
maint = get_maint(patch_file)
os.unlink(patch_file)

out = do(maint.split("\n"))
out = [l for l in out if l[0] != '#']

tmpf = tempfile.NamedTemporaryFile(mode='w+', encoding="utf-8")
tmpf.write(msg + '\n')
tmpf.write('\n'.join(out))
tmpf.flush()
git_commit(tmpf.name)
tmpf.close()
out = ["Updated inline: " + msg.split("\n")[0]]
else:
patch_file = git_patch_format()
maint = get_maint(patch_file)
os.remove(patch_file)

out = do(maint.split("\n"))

print('\n'.join(out))

if __name__ == '__main__':
sys.exit(main())