From: Jeff Garzik <jeff@garzik•org>
To: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel•com>,
"Kirsher, Jeffrey T" <jeffrey.t.kirsher@intel•com>
Cc: "davem@davemloft•net" <davem@davemloft•net>,
"netdev@vger•kernel.org" <netdev@vger•kernel.org>,
"gospo@redhat•com" <gospo@redhat•com>,
Ben Hutchings <bhutchings@solarflare•com>
Subject: Re: [net-next-2.6 PATCH] ethtool: Add n-tuple string length to drvinfo and return it
Date: Sat, 27 Feb 2010 01:31:08 -0500 [thread overview]
Message-ID: <4B88BC2C.1030304@garzik.org> (raw)
In-Reply-To: <1267228162.2224.38.camel@localhost>
On 02/26/2010 06:49 PM, Peter P Waskiewicz Jr wrote:
> On Fri, 2010-02-26 at 05:56 -0800, Jeff Garzik wrote:
>> On 02/26/2010 06:54 AM, Jeff Kirsher wrote:
>>> From: Peter Waskiewicz<peter.p.waskiewicz.jr@intel•com>
>>>
>>> The drvinfo struct should include the number of strings that
>>> get_rx_ntuple will return. It will be variable if an underlying
>>> driver implements its own get_rx_ntuple routine, so userspace
>>> needs to know how much data is coming.
>>>
>>> Signed-off-by: Peter P Waskiewicz Jr<peter.p.waskiewicz.jr@intel•com>
>>> Signed-off-by: Jeff Kirsher<jeffrey.t.kirsher@intel•com>
>>> ---
>>>
>>> include/linux/ethtool.h | 1 +
>>> net/core/ethtool.c | 3 +++
>>> 2 files changed, 4 insertions(+), 0 deletions(-)
>>
>> (resending reply, standard patch-sending box is having trouble sending
>> to vger)
>>
>>
>> As noted in the other email, your patch breaks ABI. The proper path is
>> to decrease the size of reserved struct member, and NOT shift the offset
>> of other members.
>>
>>
>>
>> However, perhaps consider the following patch for returning n-tuple
>> count, for four reasons:
>>
>> 1) space in ethtool_drvinfo is limited
>>
>> 2) the patch below permits trivial string set addition, without
>> ABI changes beyond adding a new ETH_SS_xxx constant.
>>
>> 3) the patch below permits direct access to ops->get_sset_count(),
>> rather than implicit access via ethtool_drvinfo
>>
>> 4) ethtool_drvinfo interface does not permit indication of
>> ops->get_sset_count() failure, versus returning zero value. The
>> patch below does so, via output sset_mask.
>>
>> WARNING: this patch is compile-tested only.
>>
>> NOTE: I added a cosmetic fix to ETHTOOL_[GS]RXNTUPLE constants, making
>> their indentation consistent with the rest of the list of constants.
>>
>> Signed-off-by: Jeff Garzik<jgarzik@redhat•com>
>
> I'm updating your patch since I found an issue. The mask is passing in
> the ETH_SS_* flags, but then they're treated as bits, not enumerated
> flags. I'm thinking of the best non-intrusive way to correct it.
As Ben noted, you cannot change those enumerated values, as they are
already part of the ABI.
For ETHTOOL_GSSET_INFO, you initialize the sset_mask like this:
info.sset_mask = (1ULL << ETH_SS_NTUPLE_FILTERS);
A multiple initialization would look like this:
info.sset_mask = (1ULL << ETH_SS_NTUPLE_FILTERS) |
(1ULL << ETH_SS_STATS) |
(1ULL << ETH_SS_PRIV_FLAGS);
Do you still see an issue in my suggested code, now that sset_mask
confusion is cleared up?
Regards,
Jeff
next prev parent reply other threads:[~2010-02-27 6:31 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-02-26 11:54 [net-next-2.6 PATCH] ethtool: Add n-tuple string length to drvinfo and return it Jeff Kirsher
2010-02-26 12:20 ` David Miller
2010-02-26 13:05 ` Jeff Garzik
2010-02-26 13:11 ` David Miller
2010-02-26 20:08 ` Peter P Waskiewicz Jr
2010-02-26 13:44 ` [PATCH] " Jeff Garzik
2010-02-26 13:56 ` Jeff Garzik
2010-02-26 13:59 ` Jeff Garzik
2010-02-26 23:49 ` Peter P Waskiewicz Jr
2010-02-27 6:31 ` Jeff Garzik [this message]
2010-02-27 7:25 ` Jeff Garzik
2010-02-27 20:28 ` Waskiewicz Jr, Peter P
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4B88BC2C.1030304@garzik.org \
--to=jeff@garzik$(echo .)org \
--cc=bhutchings@solarflare$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=gospo@redhat$(echo .)com \
--cc=jeffrey.t.kirsher@intel$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=peter.p.waskiewicz.jr@intel$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox