netdev - Re: [PATCH v2 3/3] ethtool: Add ETHTOOL_RESET support via --reset command

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: <20171205222920.3szmrvrhbu72wpf2@unicorn.suse.cz>
Date:   Tue, 5 Dec 2017 23:29:20 +0100
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Scott Branden <scott.branden@...adcom.com>
Cc:     "John W. Linville" <linville@...driver.com>,
        BCM Kernel Feedback <bcm-kernel-feedback-list@...adcom.com>,
        Steve Lin <steven.lin1@...adcom.com>,
        Michael Chan <michael.chan@...adcom.com>,
        netdev@...r.kernel.org, Paul Greenwalt <paul.greenwalt@...el.com>,
        Stephen Hemminger <shemminger@...tta.com>
Subject: Re: [PATCH v2 3/3] ethtool: Add ETHTOOL_RESET support via --reset
 command

On Tue, Dec 05, 2017 at 02:06:09PM -0800, Scott Branden wrote:
> On 17-12-05 01:30 PM, Michal Kubecek wrote:
> > On Tue, Dec 05, 2017 at 12:53:23PM -0800, Scott Branden wrote:
> > > Add ETHTOOL_RESET support via --reset command.
> > > 
> > > ie.  ethtool --reset DEVNAME <flagname(s)>
> > > 
> > > flagnames currently match the ETH_RESET_xxx names:
> > > mgmt,irq,dma,filter,offload,mac,phy,ram,dedicated,all
> > > 
> > > Alternatively, you can specific component bitfield directly using
> > > ethtool --reset DEVNAME flags %x
> > IMHO it would be more consistent with e.g. msglvl without the keyword
> > "flags".
> I don't see the consistency in ethtool of specifying a number without a
> keyword in front of it.
> I can only find --set-dump specify a number?
> Others have keyword and number.  msglvl is the keyword after specifying -s -
> same as flags is the keyword I use after specifying --reset.

What I meant is that you can write

    ethtool -s eth0 msglvl drv on probe off
    ethtool -s eth0 msglvl 0x7

i.e. either number or names (with on/off in this case) while your patch
has

    ethtool --reset eth0 mgmg,irq
    ethtool --reset eth0 flags 0x3

i.e. an extra keyword if a number is used.

But it's not really important, it doesn't seem I would be able to share
a parser for this with any other subcommand or parameter anyway.

> >   It would be also nice to provide a symbolic way to specify the
> > shared flags.
> 
> I'll change to allow -shared to be added to the end of each component
> specified to use the shared bit.
>  IE. mgmt-shared, irq-shared, dma-shared ?

Sounds good to me.

> > > +	resetinfo.cmd = ETHTOOL_RESET;
> > > +
> > > +	if (send_ioctl(ctx, &resetinfo)) {
> > > +		perror("Cannot issue RESET");
> > > +		return 1;
> > > +	}
> > > +	fprintf(stdout, "RESET 0x%x issued\n", resetinfo.data);
> > 
> > According to documentation, driver is supposed to clear the flags
> > corresponding to components which were reset so that what is left are
> > those which were _not_ reset.
> 
> I'll move the print above the send_ioctl.

It might be even more useful if ethtool informed user what actually
happened, i.e. either change the message to saying these are bits for
components not reset (if resetinfo.data is not zero) or save the
original value of resetinfo.data and show  saved_data & ~resetinfo.data

Michal Kubecek