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: <42b76996-b351-4dd4-741d-879e156d1c5c@broadcom.com>
Date:   Tue, 5 Dec 2017 14:06:09 -0800
From:   Scott Branden <scott.branden@...adcom.com>
To:     Michal Kubecek <mkubecek@...e.cz>
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

Hi Michal,

thanks for review - see comments

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.
Whichever convention is decided for the --reset command I will change 
the patch to though.

>   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 ?
>
>> +static int do_reset(struct cmd_context *ctx)
>> +{
>> +	struct ethtool_value resetinfo;
>> +	int argc = ctx->argc;
>> +	char **argp = ctx->argp;
>> +	int i;
>> +
>> +	if (argc == 0)
>> +		exit_bad_args();
>> +
>> +	resetinfo.data = 0;
>> +
>> +	for (i = 0; i < argc; i++) {
>> +		if (!strcmp(argp[i], "flags")) {
>> +			__u32 flags;
>> +
>> +			i++;
>> +			if (i >= argc)
>> +				exit_bad_args();
>> +			flags = strtoul(argp[i], NULL, 0);
>> +			if (flags == 0)
>> +				exit_bad_args();
>> +			else
>> +				resetinfo.data |= flags;
>> +		} else if (!strcmp(argp[i], "mgmt")) {
>> +			resetinfo.data |= ETH_RESET_MGMT;
>> +		} else if (!strcmp(argp[i], "irq")) {
>> +			resetinfo.data |= ETH_RESET_IRQ;
>> +		} else if (!strcmp(argp[i], "dma")) {
>> +			resetinfo.data |= ETH_RESET_DMA;
>> +		} else if (!strcmp(argp[i], "filter")) {
>> +			resetinfo.data |= ETH_RESET_FILTER;
>> +		} else if (!strcmp(argp[i], "offload")) {
>> +			resetinfo.data |= ETH_RESET_OFFLOAD;
>> +		} else if (!strcmp(argp[i], "mac")) {
>> +			resetinfo.data |= ETH_RESET_MAC;
>> +		} else if (!strcmp(argp[i], "phy")) {
>> +			resetinfo.data |= ETH_RESET_PHY;
>> +		} else if (!strcmp(argp[i], "ram")) {
>> +			resetinfo.data |= ETH_RESET_RAM;
>> +		} else if (!strcmp(argp[i], "ap")) {
>> +			resetinfo.data |= ETH_RESET_AP;
>> +		} else if (!strcmp(argp[i], "dedicated")) {
>> +			resetinfo.data |= ETH_RESET_DEDICATED;
>> +		} else if (!strcmp(argp[i], "all")) {
>> +			resetinfo.data |= ETH_RESET_ALL;
>> +		} else {
>> +			exit_bad_args();
>> +		}
>> +	}
>> +
>> +	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.
> Michal Kubecek