netdev - Re: [PATCH net-next v2 5/5] net: dsa: Stop accessing ds->dst->cpu

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: <87bmpz8f4x.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me>
Date:   Wed, 07 Jun 2017 21:03:26 -0400
From:   Vivien Didelot <vivien.didelot@...oirfairelinux.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     netdev <netdev@...r.kernel.org>, Andrew Lunn <andrew@...n.ch>,
        John Crispin <john@...ozen.org>,
        David Miller <davem@...emloft.net>,
        Sean Wang <sean.wang@...iatek.com>
Subject: Re: [PATCH net-next v2 5/5] net: dsa: Stop accessing ds->dst->cpu_dp in drivers

Hi Florian,

Florian Fainelli <f.fainelli@...il.com> writes:

>> Also I do not think that it is a good thing that a DSA driver play much
>> in dsa_port structures (they are ideally DSA core only specific data).
>> They only seem to need the master interface, so what I see is:
>> 
>>     static inline struct net_device *dsa_get_master(struct dsa_switch *ds, int port)
>>     {
>>             struct dsa_port *dp = &ds->ports[port];
>> 
>>             if (!dsa_is_cpu_port(ds, port))
>>                     dp = dp->cpu_dp;
>> 
>>             return dp->netdev;
>>     }
>
> The port parameter is kind of pointless, that is what I was trying to
> say, see below.
>
> MT7530_CPU_PORT = 6 and there is a define above for 7 ports, so it is
> presumably the default CPU port that the switch uses. With Broadcom
> switches you could have port 5, 7 or 8 as CPU ports but 8 is still the
> default for most 9-ports capable switches.
>
>> Now let's think that you can have several CPU ports (as with mv88e6xxx).
>> I think it is the driver responsibility to iterate over CPU capable
>> ports and inspect the master devices if they need to, instead of having
>> DSA core return an arbitrary one (which might be the wrong one.)
>
> OK, then are you okay if I do this in mt7530.c instead:
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 1e46418a3b74..d5b63958dd85 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -907,17 +907,26 @@ static int
>  mt7530_setup(struct dsa_switch *ds)
>  {
>         struct mt7530_priv *priv = ds->priv;
> +       struct dsa_port *port;
>         int ret, i;
>         u32 id, val;
>         struct device_node *dn;
>         struct mt7530_dummy_poll p;
>
> -       /* The parent node of cpu_dp->netdev which holds the common system
> -        * controller also is the container for two GMACs nodes representing
> -        * as two netdev instances.
> -        */
> -       dn = ds->dst->cpu_dp->netdev->dev.of_node->parent;
> -       priv->ethernet = syscon_node_to_regmap(dn);
> +       for (i = 0; i < ds->num_ports; i++) {
> +               port = &ds->ports[i];
> +               /* The parent node of cpu_dp->netdev which holds the common
> +                * system controller also is the container for two GMACs
> nodes
> +                * representing as two netdev instances.
> +                */
> +               if (dsa_is_cpu_port(ds, i)) {
> +                       dn = port->netdev->dev.of_node->parent;
> +                       if (priv->ethernet)
> +                               return -EEXIST;
> +                       priv->ethernet = syscon_node_to_regmap(dn);
> +               }
> +       }
> +
>         if (IS_ERR(priv->ethernet))
>                 return PTR_ERR(priv->ethernet);

I think it is the correct way to do this.

This was the reason why I suggested a dsa_get_master() alternative, even
though it is not necessary per-se, it was just to reduce boilerplate and
avoid exposing too much the dsa_port structures.

dsa_ds_get_cpu_dp() doesn't make much sense anymore, right? If there are
2 CPU ports in use, it is better that the driver iterates on them itself
as you did, instead of having DSA return the first one it knows about.

Thanks,

        Vivien