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

[Scott Branden <scott.branden@broadcom.com>]

Hi Michal,

Thanks - one question below hopefully someone can help with.


On 17-12-05 02:29 PM, Michal Kubecek wrote:
> 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
In making the improvement I found a bug in the bnxt_en kernel driver.
The bnxt_en driver currently doesn't clear any of the component flags on 
success so I need to send in a fix for that.

Although in one case (RESET_ALL) in the driver it doesn't actually 
execute the reset until all necessary drivers are unloaded to prevent 
the PCIe bus from hanging.
So question: should the flags be cleared if the reset is "pending" but 
hasn't actually happened yet, but will reset once all the drivers are 
all properly unloaded?
>
> Michal Kubecek
>