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: <CAKR-sGeJ3FWM_0FyuSF=esuvrSDX5w9zwfs7Peof1XDx=sHSpA@mail.gmail.com>
Date: Thu, 12 Jun 2025 11:47:03 +0200
From: Álvaro Fernández Rojas <noltari@...il.com>
To: Jonas Gorski <jonas.gorski@...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

Hi Jonas,

El jue, 12 jun 2025 a las 11:17, Jonas Gorski
(<jonas.gorski@...il.com>) escribió:
>
> 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?

Nope, I don't have any device 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.

Since Florian suggested that we should be able to differentiate them I
assumed we were supporting it, but if it doesn't make sense to support
it at all we can drop this patch entirely and not check for 5325m in
B53_VLAN_ID_IDX access of the next patch (5/14).

>
> > +               .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.

So the correct would be:
val = b53_phy_read16(dev->ds, 0, MII_PHYSID2);
phy_id |= (val & 0xfff0);
Right?

>
> > +
> > +                       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).

That makes sense.
I can rework this patch with a new variant field if differentiating
the BCM5325M is finally needed.

>
> Regards,
> Jonas

Best regards,
Álvaro.