Re: [PATCH v4 net-next 04/10] net/ncsi: Ethtool operation to get NCSI topology

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

On Thu, May 04, 2017 at 09:31:20AM +0000, David Laight wrote:
>From: Gavin Shan
>> Sent: 04 May 2017 07:16
>> On Wed, May 03, 2017 at 10:19:44PM -0700, Stephen Hemminger wrote:
>> >On Wed,  3 May 2017 14:44:35 +1000
>> >Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
>...
>> >> +{
>> >> +	struct ethtool_ncsi_channels *enc;
>> >> +	short nr_channels;
>> >Should be __u16 or unsigned not short.
>> >
>> 
>> Nope, It's for signed number. User expects to get number of available
>> channels when negative number is passed in. When it's positive, it's
>> going to get the channels' information.
>
>Why 16 bits?
>You are just making life hard for the compiler and possibly generating
>random padding.
>

It's because there are 256 NCSI channels to maximal degree. So 16-bits
is the minial data width to hold it in signed format. Yes, I think
__s32 would be better in this case. However, I would like to discard
the negotiation mechanism in next respin.

>I guess the user is expected to pass -1 first to get the number of
>channels, then allocate an appropriate sized array and call again
>specifying the number of channels?
>

It's correct.

>What happens if the number of channels changes between the two requests?
>

There are only one case the number changes from zero to x. In previous call,
zero is returned and userspace will get nothing. When x channels are probed,
it's stable and won't change. I don't see any problem because of it.

In next respin, I'll pass 256 entries directly. Each entry will have a flag
to indicate it's valid or not. No negotiation will be needed.

>I'd also suggest passing the size of each entry (in at least one direction).
>That way additional channel information can be added.
>

why? we have another command (ETHTOOL_GNCSICINFO) to retrieve information
about the specified channel.

Cheers,
Gavin