linux-kernel - Re: [PATCH net-next v3 04/14] net: dsa: b53: detect BCM5325 variants

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOiHx=kxcMNDrmzz5Bqd337YrZ23sYNWP0-nZrUynPJXdt4LLg@mail.gmail.com>
Date: Thu, 12 Jun 2025 11:17:29 +0200
From: Jonas Gorski <jonas.gorski@...il.com>
To: Álvaro Fernández Rojas <noltari@...il.com>
Cc: florian.fainelli@...adcom.com, andrew@...n.ch, olteanv@...il.com, 
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, 
	horms@...nel.org, vivien.didelot@...il.com, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, dgcbueu@...il.com
Subject: Re: [PATCH net-next v3 04/14] net: dsa: b53: detect BCM5325 variants

On Thu, Jun 12, 2025 at 10:37 AM Álvaro Fernández Rojas
<noltari@...il.com> wrote:
>
> Older BCM5325M switches lack some registers that newer BCM5325E have, so
> we need to be able to differentiate them in order to check whether the
> registers are available or not.

Did you test this with a BCM5325M?

> Signed-off-by: Álvaro Fernández Rojas <noltari@...il.com>
> ---
>  drivers/net/dsa/b53/b53_common.c | 34 ++++++++++++++++++++++++++------
>  drivers/net/dsa/b53/b53_priv.h   | 16 +++++++++++++--
>  2 files changed, 42 insertions(+), 8 deletions(-)
>
>  v3: detect BCM5325 variants as requested by Florian.
>
> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index 222107223d109..2975dab6ee0bb 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -2490,8 +2490,18 @@ struct b53_chip_data {
>
>  static const struct b53_chip_data b53_switch_chips[] = {
>         {
> -               .chip_id = BCM5325_DEVICE_ID,
> -               .dev_name = "BCM5325",
> +               .chip_id = BCM5325M_DEVICE_ID,
> +               .dev_name = "BCM5325M",
> +               .vlans = 16,

Are you sure about BCM5325M supporting VLANs at all? All the
documentation I can find implies it does not. And if it does not, not
sure if it makes sense to support it.

> +               .enabled_ports = 0x3f,
> +               .arl_bins = 2,
> +               .arl_buckets = 1024,
> +               .imp_port = 5,
> +               .duplex_reg = B53_DUPLEX_STAT_FE,
> +       },
> +       {
> +               .chip_id = BCM5325E_DEVICE_ID,
> +               .dev_name = "BCM5325E",
>                 .vlans = 16,
>                 .enabled_ports = 0x3f,
>                 .arl_bins = 2,
> @@ -2938,10 +2948,22 @@ int b53_switch_detect(struct b53_device *dev)
>                 b53_write16(dev, B53_VLAN_PAGE, B53_VLAN_TABLE_ACCESS_25, 0xf);
>                 b53_read16(dev, B53_VLAN_PAGE, B53_VLAN_TABLE_ACCESS_25, &tmp);
>
> -               if (tmp == 0xf)
> -                       dev->chip_id = BCM5325_DEVICE_ID;
> -               else
> +               if (tmp == 0xf) {
> +                       u32 phy_id;
> +                       int val;
> +
> +                       val = b53_phy_read16(dev->ds, 0, MII_PHYSID1);
> +                       phy_id = (val & 0xffff) << 16;
> +                       val = b53_phy_read16(dev->ds, 0, MII_PHYSID2);
> +                       phy_id |= (val & 0xffff);

You should ignore the least significant nibble, as it encodes the chip revision.

> +
> +                       if (phy_id == 0x0143bc30)
> +                               dev->chip_id = BCM5325E_DEVICE_ID;
> +                       else
> +                               dev->chip_id = BCM5325M_DEVICE_ID;
> +               } else {
>                         dev->chip_id = BCM5365_DEVICE_ID;
> +               }
>                 break;
>         case BCM5389_DEVICE_ID:
>         case BCM5395_DEVICE_ID:
> @@ -2975,7 +2997,7 @@ int b53_switch_detect(struct b53_device *dev)
>                 }
>         }
>
> -       if (dev->chip_id == BCM5325_DEVICE_ID)
> +       if (is5325(dev))
>                 return b53_read8(dev, B53_STAT_PAGE, B53_REV_ID_25,
>                                  &dev->core_rev);
>         else
> diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
> index a5ef7071ba07b..deea4d83f0e93 100644
> --- a/drivers/net/dsa/b53/b53_priv.h
> +++ b/drivers/net/dsa/b53/b53_priv.h
> @@ -60,7 +60,8 @@ struct b53_io_ops {
>
>  enum {
>         BCM4908_DEVICE_ID = 0x4908,
> -       BCM5325_DEVICE_ID = 0x25,
> +       BCM5325M_DEVICE_ID = 0x25,
> +       BCM5325E_DEVICE_ID = 0x25e,

Maybe we should have a b53_priv::variant_id field or so. Other chips
also can have variants, so we might want to avoid polluting the chip
id space. We currently don't care about them, but might in the future
as they have different feature support (e.g. there are bcm531x5
variants with and without CFP support).

Regards,
Jonas