netdev - Re: [PATCH net 01/15] docs: net: dsa: update probing documentation

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: <20220717233940.dsiskh6etvr23ivl@skbuf>
Date:   Sun, 17 Jul 2022 23:39:41 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH net 01/15] docs: net: dsa: update probing documentation

On Sun, Jul 17, 2022 at 05:14:34PM +0200, Andrew Lunn wrote:
> Hi Vladimir
> 
> > +Switch registration from the perspective of a driver means passing a valid
> > +``struct dsa_switch`` pointer to ``dsa_register_switch()``, usually from the
> > +switch driver's probing function. The following members must be valid in the
> > +provided structure:
> 
> > +
> > +- ``ds->priv``: backpointer to a driver-private data structure which can be
> > +  retrieved in all further DSA method callbacks.
> 
> I'm not sure this is strictly true. The DSA core itself should not use
> ds->priv. And the driver could use other means, like
> dev_get_platdata(ds->dev->pdev) to get at is private data. But in

dev_get_platdata or dev_get_drvdata? I think platform_data is usually
for read-only information and thus not really appropriate as the
stateful private structure.

Also, "struct device" (ds->dev) doesn't have a "pdev" member.

> practice it is true.
> 
> Reviewed-by: Andrew Lunn <andrew@...n.ch>
> 
>     Andrew

While it isn't strictly true technically speaking, I'd probably still
tell someone during review to do what everybody else does, if it comes
to it. It's also shorter to write "priv = ds->priv" than
"priv = dev_get_drvdata(ds->dev)", plus it goes through fewer pointers.

Worth changing or not worth changing all drivers from ds->priv to
dev_get_drvdata? If not worth changing, I guess it's not worth
mentioning either.