netdev - Re: [PATCH net-next 1/2] net: dsa: add a dsa_port_is

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] [day] [month] [year] [list]

Message-ID: <CA+h21hqn5BH5u+6ornb--haE2yad1Mqe_B9L5UMEBuUAdiNazw@mail.gmail.com>
Date:   Thu, 19 Mar 2020 22:14:50 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Vivien Didelot <vivien.didelot@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>, Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH net-next 1/2] net: dsa: add a dsa_port_is_enabled helper function

Hi Andrew, Vivien,

On Thu, 19 Mar 2020 at 21:17, Vivien Didelot <vivien.didelot@...il.com> wrote:
>
> Hi Vladimir,
>
> On Thu, 19 Mar 2020 20:56:19 +0200, Vladimir Oltean <olteanv@...il.com> wrote:
> > From: Vladimir Oltean <vladimir.oltean@....com>
> >
> > Sometimes drivers need to do per-port operation outside the port DSA
> > methods, and in that case they typically iterate through their port list
> > themselves.
> >
> > Give them an aid to skip ports that are disabled in the device tree
> > (which the DSA core already skips).
> >
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> > ---
> >  include/net/dsa.h |  2 ++
> >  net/dsa/dsa2.c    | 29 +++++++++++++++++++++++++++++
> >  2 files changed, 31 insertions(+)
> >
> > diff --git a/include/net/dsa.h b/include/net/dsa.h
> > index beeb81a532e3..813792e6f0be 100644
> > --- a/include/net/dsa.h
> > +++ b/include/net/dsa.h
> > @@ -376,6 +376,8 @@ static inline bool dsa_port_is_vlan_filtering(const struct dsa_port *dp)
> >               return dp->vlan_filtering;
> >  }
> >
> > +bool dsa_port_is_enabled(struct dsa_switch *ds, int port);
> > +
> >  typedef int dsa_fdb_dump_cb_t(const unsigned char *addr, u16 vid,
> >                             bool is_static, void *data);
> >  struct dsa_switch_ops {
> > diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
> > index e7c30b472034..752f21273bd6 100644
> > --- a/net/dsa/dsa2.c
> > +++ b/net/dsa/dsa2.c
> > @@ -727,6 +727,35 @@ static int dsa_switch_parse_ports_of(struct dsa_switch *ds,
> >       return err;
> >  }
> >
> > +bool dsa_port_is_enabled(struct dsa_switch *ds, int port)
> > +{
> > +     struct device_node *dn = ds->dev->of_node;
> > +     struct device_node *ports, *port_node;
> > +     bool found = false;
> > +     int reg, err;
> > +
> > +     ports = of_get_child_by_name(dn, "ports");
> > +     if (!ports) {
> > +             dev_err(ds->dev, "no ports child node found\n");
> > +             return false;
> > +     }
> > +
> > +     for_each_available_child_of_node(ports, port_node) {
> > +             err = of_property_read_u32(port_node, "reg", &reg);
> > +             if (err)
> > +                     goto out_put_node;
> > +
> > +             if (reg == port) {
> > +                     found = true;
> > +                     break;
> > +             }
> > +     }
> > +
> > +out_put_node:
> > +     of_node_put(ports);
> > +     return found;
> > +}
>
> Why do you need to iterate on the device tree? Ideally we could make
> abstraction of that (basic platform data is supported too) and use pure DSA
> helpers, given that DSA core has already peeled all that for us.
>
> Your helper above doesn't even look at the port's state, only if it is declared
> or not. Looking at patch 2, Using !dsa_is_unused_port(ds, port) seems enough.
>

Huh, this one was too smart for me.
I wrote the patch a while ago and did not really look deep enough to
spot this one. Actually I remember I did notice the
DSA_PORT_TYPE_UNUSED keyword, but I hadn't seen the ports being set
anywhere to that value. I realized just now that it's zero so they're
unused by default.
That's exactly why code review is amazing! Thank you, I've sent a v2
with your suggestion, it works a treat.

>
> Thanks,
>
>         Vivien

Thanks,
-Vladimir