netdev - Re: [PATCH net-next v1] ethtool: ethtool_get_drvinfo: populate drvinfo fields even if callback exits

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: <Y20zQ2XQL6QJfAke@unreal>
Date:   Thu, 10 Nov 2022 19:22:11 +0200
From:   Leon Romanovsky <leon@...nel.org>
To:     Vincent MAILHOL <mailhol.vincent@...adoo.fr>
Cc:     Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Guangbin Huang <huangguangbin2@...wei.com>,
        Sean Anderson <sean.anderson@...o.com>,
        Tom Rix <trix@...hat.com>,
        Tonghao Zhang <xiangxia.m.yue@...il.com>,
        Wolfram Sang <wsa+renesas@...g-engineering.com>,
        Marco Bonelli <marco@...eim.net>
Subject: Re: [PATCH net-next v1] ethtool: ethtool_get_drvinfo: populate
 drvinfo fields even if callback exits

On Fri, Nov 11, 2022 at 12:53:16AM +0900, Vincent MAILHOL wrote:
> On Thu. 10 Nov. 2022 at 21:00, Leon Romanovsky <leon@...nel.org> wrote:
> > On Thu, Nov 10, 2022 at 08:43:25PM +0900, Vincent MAILHOL wrote:
> > > On Thu. 10 Nov. 2022 at 18:11, Leon Romanovsky <leon@...nel.org> wrote:
> > > > On Thu, Nov 10, 2022 at 05:34:55PM +0900, Vincent MAILHOL wrote:
> > > > > On Thu. 10 nov. 2022 at 05:26, Jakub Kicinski <kuba@...nel.org> wrote:
> > > > > > On Wed, 9 Nov 2022 19:52:13 +0200 Leon Romanovsky wrote:
> > > > > > > On Tue, Nov 08, 2022 at 12:57:54PM +0900, Vincent Mailhol wrote:
> > > > > > > > If ethtool_ops::get_drvinfo() callback isn't set,
> > > > > > > > ethtool_get_drvinfo() will fill the ethtool_drvinfo::name and
> > > > > > > > ethtool_drvinfo::bus_info fields.
> > > > > > > >
> > > > > > > > However, if the driver provides the callback function, those two
> > > > > > > > fields are not touched. This means that the driver has to fill these
> > > > > > > > itself.
> > > > > > >
> > > > > > > Can you please point to such drivers?
> > > > > >
> > > > > > What you mean by "such drivers" is not clear from the quoted context,
> > > > > > at least to me.
> > > > >
> > > > > An example:
> > > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/broadcom/bnx2.c#L7041
> > > > >
> > > > > This driver wants to set fw_version but needs to also fill the driver
> > > > > name and bus_info. My patch will enable *such drivers* to only fill
> > > > > the fw_version and delegate the rest to the core.
> > > >
> > > > Sorry for being misleading, It looks like I typed only part of the sentence
> > > > which I had in my mind. I wanted to see if any driver exists which prints
> > > > drv_name and bus_info different from default.
> > > >
> > > > >
> > > > > > > One can argue that they don't need to touch these fields in a first
> > > > > > > place and ethtool_drvinfo should always overwrite them.
> > > > > >
> > > > > > Quite likely most driver prints to .driver and .bus_info can be dropped
> > > > > > with this patch in place. Then again, I'm suspecting it's a bit of a
> > > > > > chicken and an egg problem with people adding new drivers not having
> > > > > > an incentive to add the print in the core and people who want to add
> > > > > > the print in the core not having any driver that would benefit.
> > > > > > Therefore I'd lean towards accepting Vincent's patch as is even if
> > > > > > the submission can likely be more thorough and strict.
> > > > >
> > > > > If we can agree that no drivers should ever print .driver and
> > > > > .bus_info, then I am fine to send a clean-up patch to remove all this
> > > > > after this one gets accepted. However, I am not willing to invest time
> > > > > for nothing. So would one of you be ready to sign-off such a  clean-up
> > > > > patch?
> > > >
> > > > I will be happy to see such patch and will review it, but can't add sign-off
> > > > as I'm not netdev maintainer.
> > >
> > > Well, if you want to review, just have a look at:
> > >   $ git grep -W "get_drvinfo(struct"
> >
> > BTW, in some of the callbacks, if driver doesn't exists, they print "N/A",
> > while in your patch it will be empty string.
> 
> Indeed and this is inconsistent. For example, no one sets
> .erom_version to "N/A". So you will have some of the fields set to
> "N/A" and some others set to an empty string. The "N/A" thing was a
> mistake to begin with. I will not change my patch.

I don't see anyone asking you to change your patch drastically. This
discussion is more about next steps.

Thanks