* Re: [net-next 4/5] bnx2x: improve memory handling, low memory recovery flows
From: David Miller @ 2011-05-03 23:07 UTC (permalink / raw)
To: dmitry; +Cc: netdev, eilong
In-Reply-To: <1304429337.19456.5.camel@lb-tlvb-dmitry>
From: "Dmitry Kravkov" <dmitry@broadcom.com>
Date: Tue, 3 May 2011 16:28:57 +0300
>
> +/**
> + * zeros the contents of the bp->fp[index].
> + * Makes sure the contents of the bp->fp[index]. napi is kept
> + * intact.
> + *
> + * @param bp
> + * @param index
> + */
I should have caught this in the past but I've only noticed it right
now.
Do not use your own, custom, format for documenting functions and
their arguments.
Use the standard DocBook comment format we use in the entire kernel.
The last thing we need is every single driver using their own custom
format.
Fix up the existing cases, or remove them completely.
Thanks.
^ permalink raw reply
* Re: [PATCH V2] net/stmmac: Move "#include <linux/platform_device.h>" to linux/stmmac.h
From: David Miller @ 2011-05-03 23:09 UTC (permalink / raw)
To: peppe.cavallaro
Cc: viresh.kumar, netdev, shiraz.hashim, armando.visconti,
deepak.sikri, viresh.linux
In-Reply-To: <4DBF9477.5070302@st.com>
From: Giuseppe CAVALLARO <peppe.cavallaro@st.com>
Date: Tue, 03 May 2011 07:36:55 +0200
> On 5/3/2011 6:36 AM, Viresh Kumar wrote:
>> stmmac.h uses struct platform_device and doesn't include
>> <linux/platform_device.h>. Whereas drivers/net/stmmac/stmmac.h includes it, but
>> doesn't directly use it. And so we get following compilation warning while using
>> this file:
>> warning: ‘struct platform_device’ declared inside parameter list
>>
>> This patch includes <linux/platform_device.h> in linux/stmmac.h and removes it
>> from drivers/net/stmmac/stmmac.h
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
>
> Acked-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Applied to net-next-2.6, thanks everyone.
^ permalink raw reply
* Re: A race in register_netdevice()
From: Kalle Valo @ 2011-05-03 23:18 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, linux-wireless
In-Reply-To: <20110428165237.0c1eddbc@nehalam>
Hi Stephen,
Stephen Hemminger <shemminger@vyatta.com> writes:
> On Fri, 29 Apr 2011 01:36:37 +0300
> Kalle Valo <kvalo@adurom.com> wrote:
>
>> there seems to be a race in register_netdevice(), which is reported here:
>>
>> https://bugzilla.kernel.org/show_bug.cgi?id=15606
>>
>> This is visible at least with flimflam and ath6kl. Basically what
>> happens is this:
>>
>> Apr 29 00:21:35 roska flimflamd[2598]: src/udev.c:add_net_device()
>> Apr 29 00:21:35 roska flimflamd[2598]: connman_inet_ifname: SIOCGIFNAME(index
>> 4): No such device
>> Apr 29 00:21:45 roska flimflamd[2598]: src/rtnl.c:rtnl_message() buf
>> 0xbfefda3c len 1004
>> Apr 29 00:21:45 roska flimflamd[2598]: src/rtnl.c:rtnl_message()
>> NEWLINK len 1004 type 16 flags 0x0000 seq 0
[...]
>> I have confirmed that both of these patches fix the issue. Now I'm
>> wondering which one is the best way forward. Or is there a better way
>> to fix this?
>>
>
> I see no problem with moving this.
> SIOCGIFNAME should not need to hold rtnl.
I'm having difficulties of fixing the race and exploring other
options. Is there any particular issue why SIOCGIFNAME should not take
rtnl?
--
Kalle Valo
^ permalink raw reply
* Re: [ethtool PATCH 4/4] v5 Add RX packet classification interface
From: Dimitris Michailidis @ 2011-05-03 23:23 UTC (permalink / raw)
To: Alexander Duyck; +Cc: davem, jeffrey.t.kirsher, bhutchings, netdev
In-Reply-To: <20110503161226.29251.40838.stgit@gitlad.jf.intel.com>
On 05/03/2011 09:12 AM, Alexander Duyck wrote:
[...]
> +static int rmgr_add(struct ethtool_rx_flow_spec *fsp)
> +{
> + __u32 loc = fsp->location;
Unneeded assignment. Couple lines later you assign a new value to loc.
> + __u32 slot_num;
> +
> + /* start at the end of the list since it is lowest priority */
> + loc = rmgr.size - 1;
> +
> + /* locate the first slot a rule can be placed in */
> + slot_num = loc / BITS_PER_LONG;
> +
> + /*
> + * Avoid testing individual bits by inverting the word and checking
> + * to see if any bits are left set, if so there are empty spots. By
> + * moving 1 + loc % BITS_PER_LONG we align ourselves to the last bit
> + * in the previous word.
> + *
> + * If loc rolls over it should be greater than or equal to rmgr.size
> + * and as such we know we have reached the end of the list.
> + */
> + if (!~(rmgr.slot[slot_num] | (~1UL << rmgr.size % BITS_PER_LONG))) {
> + loc -= 1 + (loc % BITS_PER_LONG);
> + slot_num--;
> + }
> +
> + /*
> + * Now that we are aligned with the last bit in each long we can just
> + * go though and eliminate all the longs with no free bits
> + */
> + while (loc < rmgr.size && !~(rmgr.slot[slot_num])) {
> + loc -= BITS_PER_LONG;
> + slot_num--;
> + }
> +
> + /*
> + * If we still are inside the range, test individual bits as one is
> + * likely available for our use.
> + */
> + while (loc < rmgr.size && test_bit(loc, rmgr.slot))
> + loc--;
> +
> + /* location found, insert rule */
> + if (loc < rmgr.size) {
> + fsp->location = loc;
> + return rmgr_ins(loc);
> + }
> +
> + /* No space to add this rule */
> + fprintf(stderr, "rmgr: Cannot find appropriate slot to insert rule\n");
> +
> + return -1;
> +}
[...]
> +int rxclass_rule_ins(int fd, struct ifreq *ifr,
> + struct ethtool_rx_flow_spec *fsp)
> +{
> + struct ethtool_rxnfc nfccmd;
> + __u32 loc = fsp->location;
> + int err;
> +
> + /*
> + * if location is unspecified pull rules from device
> + * and allocate a free rule for our use
> + */
> + if (loc == RX_CLS_LOC_UNSPEC) {
> + /* init table of available rules */
> + err = rmgr_init(fd, ifr);
> + if (err < 0)
> + return err;
> +
> + /* verify rule location */
> + err = rmgr_add(fsp);
> + if (err < 0)
> + return err;
> +
> + /* cleanup table and free resources */
> + rmgr_cleanup();
> + }
This logic where ethtool tries to select a filter slot when a user provides
RX_CLS_LOC_UNSPEC does not work in general. It assumes that all slots are
equal and a new filter can go into any available slot. But a device may have
restrictions on where a filter may go that ethtool doesn't know. I
mentioned during a previous review that for cxgb4 some filters require a
slot number that is a multiple of 4. There are some other constraints as
well depending on the type of filter being added. For such a device ethtool
doesn't know enough to handle RX_CLS_LOC_UNSPEC correctly.
I think RX_CLS_LOC_UNSPEC should be passed to the driver, where there is
enough knowledge to pick an appropriate slot. So I'd remove the
if (loc == RX_CLS_LOC_UNSPEC)
block above, let the driver pick a slot, and then pass the selected location
back for ethtool to report.
^ permalink raw reply
* Re: [PATCH] pktgen: use %pI6c for printing IPv6 addresses
From: Joe Perches @ 2011-05-03 23:25 UTC (permalink / raw)
To: Alexey Dobriyan; +Cc: davem, netdev, Robert Olsson
In-Reply-To: <20110503212340.GA25293@p183>
On Wed, 2011-05-04 at 00:23 +0300, Alexey Dobriyan wrote:
> I don't know why %pI6 doesn't compress, but the format specifier is
> kernel-standard, so use it.
Hi again Alexey.
I doubt it matters but the old routine compresses the first
0 it finds rather than the longest consecutive 0 match so
the output could be a bit different.
given: "0:1:0:0:0:0:0:7"
old: "::1:0:0:0:0:0:7"
new: "0:1::7"
I think the patch is an improvement.
> -static unsigned int fmt_ip6(char *s, const char ip[16])
> -{
> - unsigned int len;
> - unsigned int i;
> - unsigned int temp;
> - unsigned int compressing;
> - int j;
> -
> - len = 0;
> - compressing = 0;
> - for (j = 0; j < 16; j += 2) {
> -
> -#ifdef V4MAPPEDPREFIX
> - if (j == 12 && !memcmp(ip, V4mappedprefix, 12)) {
> - inet_ntoa_r(*(struct in_addr *)(ip + 12), s);
> - temp = strlen(s);
> - return len + temp;
> - }
> -#endif
> - temp = ((unsigned long)(unsigned char)ip[j] << 8) +
> - (unsigned long)(unsigned char)ip[j + 1];
> - if (temp == 0) {
> - if (!compressing) {
> - compressing = 1;
> - if (j == 0) {
> - *s++ = ':';
> - ++len;
> - }
> - }
> - } else {
> - if (compressing) {
> - compressing = 0;
> - *s++ = ':';
> - ++len;
> - }
> - i = fmt_xlong(s, temp);
> - len += i;
> - s += i;
> - if (j < 14) {
> - *s++ = ':';
> - ++len;
> - }
> - }
> - }
> - if (compressing) {
> - *s++ = ':';
> - ++len;
> - }
> - *s = 0;
> - return len;
> -}
^ permalink raw reply
* Re: [ethtool PATCH 4/4] v5 Add RX packet classification interface
From: Ben Hutchings @ 2011-05-03 23:34 UTC (permalink / raw)
To: Dimitris Michailidis; +Cc: Alexander Duyck, davem, jeffrey.t.kirsher, netdev
In-Reply-To: <4DC08E7B.7070906@chelsio.com>
On Tue, 2011-05-03 at 16:23 -0700, Dimitris Michailidis wrote:
> On 05/03/2011 09:12 AM, Alexander Duyck wrote:
[...]
> > +int rxclass_rule_ins(int fd, struct ifreq *ifr,
> > + struct ethtool_rx_flow_spec *fsp)
> > +{
> > + struct ethtool_rxnfc nfccmd;
> > + __u32 loc = fsp->location;
> > + int err;
> > +
> > + /*
> > + * if location is unspecified pull rules from device
> > + * and allocate a free rule for our use
> > + */
> > + if (loc == RX_CLS_LOC_UNSPEC) {
> > + /* init table of available rules */
> > + err = rmgr_init(fd, ifr);
> > + if (err < 0)
> > + return err;
> > +
> > + /* verify rule location */
> > + err = rmgr_add(fsp);
> > + if (err < 0)
> > + return err;
> > +
> > + /* cleanup table and free resources */
> > + rmgr_cleanup();
> > + }
>
> This logic where ethtool tries to select a filter slot when a user provides
> RX_CLS_LOC_UNSPEC does not work in general. It assumes that all slots are
> equal and a new filter can go into any available slot. But a device may have
> restrictions on where a filter may go that ethtool doesn't know.
I agree. And if filter lookup is largely hash-based (as it is in
Solarflare hardware) the user will also find it very difficult to
specify the right location!
> I mentioned during a previous review that for cxgb4 some filters require a
> slot number that is a multiple of 4. There are some other constraints as
> well depending on the type of filter being added. For such a device ethtool
> doesn't know enough to handle RX_CLS_LOC_UNSPEC correctly.
>
> I think RX_CLS_LOC_UNSPEC should be passed to the driver, where there is
> enough knowledge to pick an appropriate slot. So I'd remove the
>
> if (loc == RX_CLS_LOC_UNSPEC)
>
> block above, let the driver pick a slot, and then pass the selected location
> back for ethtool to report.
But first we have to specify this in the ethtool API. So please propose
a patch to ethtool.h.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: A race in register_netdevice()
From: Stephen Hemminger @ 2011-05-03 23:41 UTC (permalink / raw)
To: Kalle Valo
Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
linux-wireless-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <878vungyq4.fsf-5ukZ45wKbUHoml4zekdYB16hYfS7NtTn@public.gmane.org>
On Wed, 04 May 2011 02:18:11 +0300
Kalle Valo <kvalo-BkwN83ws05HQT0dZR+AlfA@public.gmane.org> wrote:
> Hi Stephen,
>
> Stephen Hemminger <shemminger-ZtmgI6mnKB3QT0dZR+AlfA@public.gmane.org> writes:
>
> > On Fri, 29 Apr 2011 01:36:37 +0300
> > Kalle Valo <kvalo-BkwN83ws05HQT0dZR+AlfA@public.gmane.org> wrote:
> >
> >> there seems to be a race in register_netdevice(), which is reported here:
> >>
> >> https://bugzilla.kernel.org/show_bug.cgi?id=15606
> >>
> >> This is visible at least with flimflam and ath6kl. Basically what
> >> happens is this:
> >>
> >> Apr 29 00:21:35 roska flimflamd[2598]: src/udev.c:add_net_device()
> >> Apr 29 00:21:35 roska flimflamd[2598]: connman_inet_ifname: SIOCGIFNAME(index
> >> 4): No such device
> >> Apr 29 00:21:45 roska flimflamd[2598]: src/rtnl.c:rtnl_message() buf
> >> 0xbfefda3c len 1004
> >> Apr 29 00:21:45 roska flimflamd[2598]: src/rtnl.c:rtnl_message()
> >> NEWLINK len 1004 type 16 flags 0x0000 seq 0
>
> [...]
>
> >> I have confirmed that both of these patches fix the issue. Now I'm
> >> wondering which one is the best way forward. Or is there a better way
> >> to fix this?
> >>
> >
> > I see no problem with moving this.
> > SIOCGIFNAME should not need to hold rtnl.
>
> I'm having difficulties of fixing the race and exploring other
> options. Is there any particular issue why SIOCGIFNAME should not take
> rtnl?
None really, but the answer given by SIOCGIFNAME is going to race
anyway. I.e if ioctl returns a value, by the time user space sees it
the result may have changed.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [net-next-2.6 PATCH] ethtool: Support to take FW dump
From: Ben Hutchings @ 2011-05-03 23:53 UTC (permalink / raw)
To: anirban.chakraborty; +Cc: netdev, --no-chain-reply-to, davem
In-Reply-To: <1304378957-24123-2-git-send-email-anirban.chakraborty@qlogic.com>
On Mon, 2011-05-02 at 16:29 -0700, anirban.chakraborty@qlogic.com wrote:
> From: Anirban Chakraborty <anirban.chakraborty@qlogic.com>
>
> Added code to take FW dump via ethtool. A pair of set and get functions are
> added to configure dump level and fetch it from the driver respectively. A
> third function is added to retrieve the dumped FW data from the driver.
>
> Signed-off-by: Anirban Chakraborty <anirban.chakraborty@qlogic.com>
> ---
> include/linux/ethtool.h | 20 ++++++++++++
> net/core/ethtool.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 96 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 9de3127..3dd91a5 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -595,6 +595,19 @@ struct ethtool_flash {
> char data[ETHTOOL_FLASH_MAX_FILENAME];
> };
>
> +/**
> + * struct ethtool_dump - used for retrieving, setting device dump
> + * @flag: flag for dump setting
> + * @len: length of dump data
> + * @data: data collected for this command
> + */
> +struct ethtool_dump {
> + __u32 cmd;
> + __u32 flag;
> + __u32 len;
> + u8 data[0];
> +};
> +
What are the legal values of flags? Are they expected to be entirely
driver-dependent?
Shouldn't there be a version field, as for register dumps?
> /* for returning and changing feature sets */
>
> /**
> @@ -926,6 +939,10 @@ struct ethtool_ops {
> const struct ethtool_rxfh_indir *);
> void (*get_channels)(struct net_device *, struct ethtool_channels *);
> int (*set_channels)(struct net_device *, struct ethtool_channels *);
> + int (*set_dump)(struct net_device *, struct ethtool_dump *);
> + int (*get_dump)(struct net_device *, struct ethtool_dump *);
> + int (*get_dump_data)(struct net_device *,
> + struct ethtool_dump *, void *);
These need to be properly documented in the header comment.
> };
> #endif /* __KERNEL__ */
> @@ -997,6 +1014,9 @@ struct ethtool_ops {
> #define ETHTOOL_SFEATURES 0x0000003b /* Change device offload settings */
> #define ETHTOOL_GCHANNELS 0x0000003c /* Get no of channels */
> #define ETHTOOL_SCHANNELS 0x0000003d /* Set no of channels */
> +#define ETHTOOL_SET_DUMP 0x0000003e /* Set dump settings */
> +#define ETHTOOL_GET_DUMP 0x0000003f /* Get dump settings */
> +#define ETHTOOL_GET_DUMP_DATA 0x00000040 /* Get dump data */
>
> /* compatibility with older code */
> #define SPARC_ETH_GSET ETHTOOL_GSET
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index d8b1a8d..dce547c 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -1827,6 +1827,73 @@ static noinline_for_stack int ethtool_flash_device(struct net_device *dev,
> return dev->ethtool_ops->flash_device(dev, &efl);
> }
>
> +static int ethtool_set_dump(struct net_device *dev,
> + void __user *useraddr)
> +{
> + struct ethtool_dump dump;
> +
> + if (!dev->ethtool_ops->set_dump)
> + return -EOPNOTSUPP;
> +
> + if (copy_from_user(&dump, useraddr, sizeof(dump)))
> + return -EFAULT;
> +
> + return dev->ethtool_ops->set_dump(dev, &dump);
> +}
> +
> +static int ethtool_get_dump(struct net_device *dev,
> + void __user *useraddr)
> +{
> + struct ethtool_dump dump;
> + const struct ethtool_ops *ops = dev->ethtool_ops;
> +
> + if (!dev->ethtool_ops->get_dump)
> + return -EOPNOTSUPP;
> +
> + if (copy_from_user(&dump, useraddr, sizeof(dump)))
> + return -EFAULT;
> +
> + if (ops->get_dump(dev, &dump))
> + return -EFAULT;
Why does this change the error code?
> + if (copy_to_user(useraddr, &dump, sizeof(dump)))
> + return -EFAULT;
> + return 0;
> +}
> +
> +static int ethtool_get_dump_data(struct net_device *dev,
> + void __user *useraddr)
> +{
> + int ret;
> + void *data;
> + struct ethtool_dump dump;
> + const struct ethtool_ops *ops = dev->ethtool_ops;
> +
> + if (!dev->ethtool_ops->get_dump_data)
> + return -EOPNOTSUPP;
> +
> + if (copy_from_user(&dump, useraddr, sizeof(dump)))
> + return -EFAULT;
> + data = vzalloc(dump.len);
I think we ought to query the driver to find out the maximum dump
length, rather than using the length passed by the user (up to 4GB). We
already check that the user has the CAP_NET_ADMIN capability but that
should not mean the ability to evade memory limits.
> + if (!data)
> + return -ENOMEM;
> + ret = ops->get_dump_data(dev, &dump, data);
> + if (ret) {
> + ret = -EFAULT;
Why does this change the error code?
> + goto out;
> + }
> + if (copy_to_user(useraddr, &dump, sizeof(dump))) {
> + ret = -EFAULT;
> + goto out;
> + }
> + useraddr += offsetof(struct ethtool_dump, data);
> + if (copy_to_user(useraddr, data, dump.len))
> + ret = -EFAULT;
> +out:
> + vfree(data);
> + return ret;
> +}
> +
> /* The main entry point in this file. Called from net/core/dev.c */
>
> int dev_ethtool(struct net *net, struct ifreq *ifr)
> @@ -2043,6 +2110,15 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
> case ETHTOOL_SCHANNELS:
> rc = ethtool_set_channels(dev, useraddr);
> break;
> + case ETHTOOL_SET_DUMP:
> + rc = ethtool_set_dump(dev, useraddr);
> + break;
> + case ETHTOOL_GET_DUMP:
> + rc = ethtool_get_dump(dev, useraddr);
> + break;
> + case ETHTOOL_GET_DUMP_DATA:
> + rc = ethtool_get_dump_data(dev, useraddr);
> + break;
> default:
> rc = -EOPNOTSUPP;
> }
--
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [ethtool PATCH 4/4] v5 Add RX packet classification interface
From: Alexander Duyck @ 2011-05-04 0:29 UTC (permalink / raw)
To: Ben Hutchings
Cc: Dimitris Michailidis, davem@davemloft.net, Kirsher, Jeffrey T,
netdev@vger.kernel.org
In-Reply-To: <1304465684.2873.26.camel@bwh-desktop>
On 5/3/2011 4:34 PM, Ben Hutchings wrote:
> On Tue, 2011-05-03 at 16:23 -0700, Dimitris Michailidis wrote:
>> On 05/03/2011 09:12 AM, Alexander Duyck wrote:
> [...]
>>> +int rxclass_rule_ins(int fd, struct ifreq *ifr,
>>> + struct ethtool_rx_flow_spec *fsp)
>>> +{
>>> + struct ethtool_rxnfc nfccmd;
>>> + __u32 loc = fsp->location;
>>> + int err;
>>> +
>>> + /*
>>> + * if location is unspecified pull rules from device
>>> + * and allocate a free rule for our use
>>> + */
>>> + if (loc == RX_CLS_LOC_UNSPEC) {
>>> + /* init table of available rules */
>>> + err = rmgr_init(fd, ifr);
>>> + if (err< 0)
>>> + return err;
>>> +
>>> + /* verify rule location */
>>> + err = rmgr_add(fsp);
>>> + if (err< 0)
>>> + return err;
>>> +
>>> + /* cleanup table and free resources */
>>> + rmgr_cleanup();
>>> + }
>>
>> This logic where ethtool tries to select a filter slot when a user provides
>> RX_CLS_LOC_UNSPEC does not work in general. It assumes that all slots are
>> equal and a new filter can go into any available slot. But a device may have
>> restrictions on where a filter may go that ethtool doesn't know.
>
> I agree. And if filter lookup is largely hash-based (as it is in
> Solarflare hardware) the user will also find it very difficult to
> specify the right location!
The thing to keep in mind is that the index doesn't have to be a
hardware index. In ixgbe we have a field in the hardware which is meant
to just be a unique software index and that is what I am using as the
location field for our filters. All the location information in the
rules really provides is a logical way of tracking rules. It doesn't
necessarily have to represent the physical location of the rule in hardware.
>> I mentioned during a previous review that for cxgb4 some filters require a
>> slot number that is a multiple of 4. There are some other constraints as
>> well depending on the type of filter being added. For such a device ethtool
>> doesn't know enough to handle RX_CLS_LOC_UNSPEC correctly.
>>
>> I think RX_CLS_LOC_UNSPEC should be passed to the driver, where there is
>> enough knowledge to pick an appropriate slot. So I'd remove the
>>
>> if (loc == RX_CLS_LOC_UNSPEC)
>>
>> block above, let the driver pick a slot, and then pass the selected location
>> back for ethtool to report.
>
> But first we have to specify this in the ethtool API. So please propose
> a patch to ethtool.h.
>
> Ben.
The other thing to keep in mind with this is that it doesn't lock you
into the network flow classifier configuration. If you want to be able
to specify a rule without having any location information included there
is always the option of ntuple which accepts almost all the same fields
but doesn't include any specific location information.
Thanks,
Alex
^ permalink raw reply
* Re: [net-next-2.6 PATCH] ethtool: Support to take FW dump
From: Anirban Chakraborty @ 2011-05-04 0:29 UTC (permalink / raw)
To: Ben Hutchings; +Cc: netdev, David Miller
In-Reply-To: <1304466797.2873.38.camel@bwh-desktop>
On May 3, 2011, at 4:53 PM, Ben Hutchings wrote:
> On Mon, 2011-05-02 at 16:29 -0700, anirban.chakraborty@qlogic.com wrote:
>> From: Anirban Chakraborty <anirban.chakraborty@qlogic.com>
>>
>> Added code to take FW dump via ethtool. A pair of set and get functions are
>> added to configure dump level and fetch it from the driver respectively. A
>> third function is added to retrieve the dumped FW data from the driver.
>>
>> Signed-off-by: Anirban Chakraborty <anirban.chakraborty@qlogic.com>
>> ---
>> include/linux/ethtool.h | 20 ++++++++++++
>> net/core/ethtool.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 96 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>> index 9de3127..3dd91a5 100644
>> --- a/include/linux/ethtool.h
>> +++ b/include/linux/ethtool.h
>> @@ -595,6 +595,19 @@ struct ethtool_flash {
>> char data[ETHTOOL_FLASH_MAX_FILENAME];
>> };
>>
>> +/**
>> + * struct ethtool_dump - used for retrieving, setting device dump
>> + * @flag: flag for dump setting
>> + * @len: length of dump data
>> + * @data: data collected for this command
>> + */
>> +struct ethtool_dump {
>> + __u32 cmd;
>> + __u32 flag;
>> + __u32 len;
>> + u8 data[0];
>> +};
>> +
>
> What are the legal values of flags? Are they expected to be entirely
> driver-dependent?
Yes, the flag could be completely driver dependent. For example, in the qlcnic
driver this would indicate the level of the dump that the driver should take. Also,
by specifying a magic key via the flag, the driver could be instructed to take a FW dump
forcibly.
>
> Shouldn't there be a version field, as for register dumps?
I was thinking that the dump data itself could contain the version field specifying the FW version. However,
we can have an additional field if that turns out to be useful.
>
>> /* for returning and changing feature sets */
>>
>> /**
>> @@ -926,6 +939,10 @@ struct ethtool_ops {
>> const struct ethtool_rxfh_indir *);
>> void (*get_channels)(struct net_device *, struct ethtool_channels *);
>> int (*set_channels)(struct net_device *, struct ethtool_channels *);
>> + int (*set_dump)(struct net_device *, struct ethtool_dump *);
>> + int (*get_dump)(struct net_device *, struct ethtool_dump *);
>> + int (*get_dump_data)(struct net_device *,
>> + struct ethtool_dump *, void *);
>
> These need to be properly documented in the header comment.
WIll do.
>
>> };
>> #endif /* __KERNEL__ */
>> @@ -997,6 +1014,9 @@ struct ethtool_ops {
>> #define ETHTOOL_SFEATURES 0x0000003b /* Change device offload settings */
>> #define ETHTOOL_GCHANNELS 0x0000003c /* Get no of channels */
>> #define ETHTOOL_SCHANNELS 0x0000003d /* Set no of channels */
>> +#define ETHTOOL_SET_DUMP 0x0000003e /* Set dump settings */
>> +#define ETHTOOL_GET_DUMP 0x0000003f /* Get dump settings */
>> +#define ETHTOOL_GET_DUMP_DATA 0x00000040 /* Get dump data */
>>
>> /* compatibility with older code */
>> #define SPARC_ETH_GSET ETHTOOL_GSET
>> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
>> index d8b1a8d..dce547c 100644
>> --- a/net/core/ethtool.c
>> +++ b/net/core/ethtool.c
>> @@ -1827,6 +1827,73 @@ static noinline_for_stack int ethtool_flash_device(struct net_device *dev,
>> return dev->ethtool_ops->flash_device(dev, &efl);
>> }
>>
>> +static int ethtool_set_dump(struct net_device *dev,
>> + void __user *useraddr)
>> +{
>> + struct ethtool_dump dump;
>> +
>> + if (!dev->ethtool_ops->set_dump)
>> + return -EOPNOTSUPP;
>> +
>> + if (copy_from_user(&dump, useraddr, sizeof(dump)))
>> + return -EFAULT;
>> +
>> + return dev->ethtool_ops->set_dump(dev, &dump);
>> +}
>> +
>> +static int ethtool_get_dump(struct net_device *dev,
>> + void __user *useraddr)
>> +{
>> + struct ethtool_dump dump;
>> + const struct ethtool_ops *ops = dev->ethtool_ops;
>> +
>> + if (!dev->ethtool_ops->get_dump)
>> + return -EOPNOTSUPP;
>> +
>> + if (copy_from_user(&dump, useraddr, sizeof(dump)))
>> + return -EFAULT;
>> +
>> + if (ops->get_dump(dev, &dump))
>> + return -EFAULT;
>
> Why does this change the error code?
It should not. Will fix.
>
>> + if (copy_to_user(useraddr, &dump, sizeof(dump)))
>> + return -EFAULT;
>> + return 0;
>> +}
>> +
>> +static int ethtool_get_dump_data(struct net_device *dev,
>> + void __user *useraddr)
>> +{
>> + int ret;
>> + void *data;
>> + struct ethtool_dump dump;
>> + const struct ethtool_ops *ops = dev->ethtool_ops;
>> +
>> + if (!dev->ethtool_ops->get_dump_data)
>> + return -EOPNOTSUPP;
>> +
>> + if (copy_from_user(&dump, useraddr, sizeof(dump)))
>> + return -EFAULT;
>> + data = vzalloc(dump.len);
>
> I think we ought to query the driver to find out the maximum dump
> length, rather than using the length passed by the user (up to 4GB). We
> already check that the user has the CAP_NET_ADMIN capability but that
> should not mean the ability to evade memory limits.
That's right. The user makes a call to get_dump (ETHTOOL_GET_DUMP)
first to get the size of the collected dump data for the current
dump level (flag). Then it makes the call to get the dump data (ETHTOOL_GET_DUMP_DATA),
as in above where it specifies the dump size.
The patch ref-ed below demonstrated that.
http://patchwork.ozlabs.org/patch/93729/
>
>> + if (!data)
>> + return -ENOMEM;
>> + ret = ops->get_dump_data(dev, &dump, data);
>> + if (ret) {
>> + ret = -EFAULT;
>
> Why does this change the error code?
Will fix it.
>
>> + goto out;
>> + }
>> + if (copy_to_user(useraddr, &dump, sizeof(dump))) {
>> + ret = -EFAULT;
>> + goto out;
>> + }
>> + useraddr += offsetof(struct ethtool_dump, data);
>> + if (copy_to_user(useraddr, data, dump.len))
>> + ret = -EFAULT;
>> +out:
>> + vfree(data);
>> + return ret;
>> +}
>> +
>> /* The main entry point in this file. Called from net/core/dev.c */
>>
>> int dev_ethtool(struct net *net, struct ifreq *ifr)
>> @@ -2043,6 +2110,15 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
>> case ETHTOOL_SCHANNELS:
>> rc = ethtool_set_channels(dev, useraddr);
>> break;
>> + case ETHTOOL_SET_DUMP:
>> + rc = ethtool_set_dump(dev, useraddr);
>> + break;
>> + case ETHTOOL_GET_DUMP:
>> + rc = ethtool_get_dump(dev, useraddr);
>> + break;
>> + case ETHTOOL_GET_DUMP_DATA:
>> + rc = ethtool_get_dump_data(dev, useraddr);
>> + break;
>> default:
>> rc = -EOPNOTSUPP;
>> }
>
Thanks for your feedback. I'll respin the patch incorporating your comments and submit it again.
-Anirban
^ permalink raw reply
* Re: [net-next-2.6 PATCH] ethtool: Support to take FW dump
From: Ben Hutchings @ 2011-05-04 1:06 UTC (permalink / raw)
To: Anirban Chakraborty; +Cc: netdev, David Miller
In-Reply-To: <2941C562-0D47-40DF-B2C0-6292381097AE@qlogic.com>
On Tue, 2011-05-03 at 17:29 -0700, Anirban Chakraborty wrote:
> On May 3, 2011, at 4:53 PM, Ben Hutchings wrote:
>
> > On Mon, 2011-05-02 at 16:29 -0700, anirban.chakraborty@qlogic.com wrote:
[...]
> >> +static int ethtool_get_dump_data(struct net_device *dev,
> >> + void __user *useraddr)
> >> +{
> >> + int ret;
> >> + void *data;
> >> + struct ethtool_dump dump;
> >> + const struct ethtool_ops *ops = dev->ethtool_ops;
> >> +
> >> + if (!dev->ethtool_ops->get_dump_data)
> >> + return -EOPNOTSUPP;
> >> +
> >> + if (copy_from_user(&dump, useraddr, sizeof(dump)))
> >> + return -EFAULT;
> >> + data = vzalloc(dump.len);
> >
> > I think we ought to query the driver to find out the maximum dump
> > length, rather than using the length passed by the user (up to 4GB). We
> > already check that the user has the CAP_NET_ADMIN capability but that
> > should not mean the ability to evade memory limits.
> That's right. The user makes a call to get_dump (ETHTOOL_GET_DUMP)
> first to get the size of the collected dump data for the current
> dump level (flag). Then it makes the call to get the dump data (ETHTOOL_GET_DUMP_DATA),
> as in above where it specifies the dump size.
> The patch ref-ed below demonstrated that.
> http://patchwork.ozlabs.org/patch/93729/
[...]
I understand that the userland application will need to get the size
that way. I'm saying that this code in the kernel should also get the
size from the driver, so that a malicious application cannot make the
kernel allocate an excessively large buffer.
Also, you'll need to submit your implementation along with this, as
David won't accept an extension to the API without a driver that
implements it.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* [PATCH 1/2] net: Allow ethtool to set interface in loopback mode.
From: Mahesh Bandewar @ 2011-05-04 1:18 UTC (permalink / raw)
To: Matt Carlson, David Miller
Cc: netdev, Michael Chan, Ben Hutchings, Michał Mirosław,
Tom Herbert, Mahesh Bandewar
In-Reply-To: <1304471935-402-1-git-send-email-maheshb@google.com>
This patch enables ethtool to set the loopback mode on a given interface.
By configuring the interface in loopback mode in conjunction with a policy
route / rule, a userland application can stress the egress / ingress path
exposing the flows of the change in progress and potentially help developer(s)
understand the impact of those changes without even sending a packet out
on the network.
Following set of commands illustrates one such example -
a) ip -4 addr add 192.168.1.1/24 dev eth1
b) ip -4 rule add from all iif eth1 lookup 250
c) ip -4 route add local 0/0 dev lo proto kernel scope host table 250
d) arp -Ds 192.168.1.100 eth1
e) arp -Ds 192.168.1.200 eth1
f) sysctl -w net.ipv4.ip_nonlocal_bind=1
g) sysctl -w net.ipv4.conf.all.accept_local=1
# Assuming that the machine has 8 cores
h) taskset 000f netserver -L 192.168.1.200
i) taskset 00f0 netperf -t TCP_CRR -L 192.168.1.100 -H 192.168.1.200 -l 30
Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
include/linux/netdevice.h | 3 ++-
net/core/ethtool.c | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index d5de66a..e7244ed 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1067,6 +1067,7 @@ struct net_device {
#define NETIF_F_RXHASH (1 << 28) /* Receive hashing offload */
#define NETIF_F_RXCSUM (1 << 29) /* Receive checksumming offload */
#define NETIF_F_NOCACHE_COPY (1 << 30) /* Use no-cache copyfromuser */
+#define NETIF_F_LOOPBACK (1 << 31) /* Enable loopback */
/* Segmentation offload features */
#define NETIF_F_GSO_SHIFT 16
@@ -1082,7 +1083,7 @@ struct net_device {
/* = all defined minus driver/device-class-related */
#define NETIF_F_NEVER_CHANGE (NETIF_F_VLAN_CHALLENGED | \
NETIF_F_LLTX | NETIF_F_NETNS_LOCAL)
-#define NETIF_F_ETHTOOL_BITS (0x7f3fffff & ~NETIF_F_NEVER_CHANGE)
+#define NETIF_F_ETHTOOL_BITS (0xff3fffff & ~NETIF_F_NEVER_CHANGE)
/* List of features with software fallbacks. */
#define NETIF_F_GSO_SOFTWARE (NETIF_F_TSO | NETIF_F_TSO_ECN | \
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index d8b1a8d..f26649d 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -362,7 +362,7 @@ static const char netdev_features_strings[ETHTOOL_DEV_FEATURE_WORDS * 32][ETH_GS
/* NETIF_F_RXHASH */ "rx-hashing",
/* NETIF_F_RXCSUM */ "rx-checksum",
/* NETIF_F_NOCACHE_COPY */ "tx-nocache-copy"
- "",
+ /* NETIF_F_LOOPBACK */ "loopback",
};
static int __ethtool_get_sset_count(struct net_device *dev, int sset)
--
1.7.3.1
^ permalink raw reply related
* [PATCHv3 0/2] Loopback
From: Mahesh Bandewar @ 2011-05-04 1:18 UTC (permalink / raw)
To: Matt Carlson, David Miller
Cc: netdev, Michael Chan, Ben Hutchings, Michał Mirosław,
Tom Herbert, Mahesh Bandewar
First patch is the repost of the earlier loopback patch. tg3 implementation / patch demonstrates one such usage and I have incarporated changes suggested by Matt and Joe.
Mahesh Bandewar (2):
net: Allow ethtool to set interface in loopback mode.
tg3: Allow ethtool to enable/disable loopback.
drivers/net/tg3.c | 57 ++++++++++++++++++++++++++++++++++++++++
include/linux/netdevice.h | 3 +-
net/core/ethtool.c | 2 +-
3 files changed, 59 insertions(+), 2 deletions(-)
--
1.7.3.1
^ permalink raw reply
* [PATCHv3 2/2] tg3: Allow ethtool to enable/disable loopback.
From: Mahesh Bandewar @ 2011-05-04 1:18 UTC (permalink / raw)
To: Matt Carlson, David Miller
Cc: netdev, Michael Chan, Ben Hutchings, Michał Mirosław,
Tom Herbert, Mahesh Bandewar
In-Reply-To: <1304471935-402-2-git-send-email-maheshb@google.com>
This patch adds tg3_set_features() to handle loopback mode. Currently the
capability is added for the devices which support internal MAC loopback mode.
So when enabled, it enables internal-MAC loopback.
Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
Changes since v2:
Implemtned Joe Perches's style change.
Changes since v1:
Implemented Matt Carlson's suggestions.
(a) Enable this capability on the devices which are capable of MAC-loopback
mode.
(b) check if the device is running before making changes.
(c) check bits before making changes.
drivers/net/tg3.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 57 insertions(+), 0 deletions(-)
diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 7c7c9a8..46de633 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -6319,6 +6319,51 @@ static u32 tg3_fix_features(struct net_device *dev, u32 features)
return features;
}
+static int tg3_set_features(struct net_device *dev, u32 features)
+{
+ struct tg3 *tp = netdev_priv(dev);
+ u32 cur_mode = 0;
+ int err = 0;
+
+ if (!netif_running(dev)) {
+ err = -EAGAIN;
+ goto sfeatures_out;
+ }
+
+ spin_lock_bh(&tp->lock);
+ cur_mode = tr32(MAC_MODE);
+
+ if (features & NETIF_F_LOOPBACK) {
+ if (cur_mode & MAC_MODE_PORT_INT_LPBACK)
+ goto sfeatures_unlock;
+
+ /*
+ * Clear MAC_MODE_HALF_DUPLEX or you won't get packets back in
+ * loopback mode if Half-Duplex mode was negotiated earlier.
+ */
+ cur_mode &= ~MAC_MODE_HALF_DUPLEX;
+
+ /* Enable internal MAC loopback mode */
+ tw32(MAC_MODE, cur_mode | MAC_MODE_PORT_INT_LPBACK);
+ netif_carrier_on(tp->dev);
+ netdev_info(dev, "Internal MAC loopback mode enabled.\n");
+ } else {
+ if (!(cur_mode & MAC_MODE_PORT_INT_LPBACK))
+ goto sfeatures_unlock;
+
+ /* Disable internal MAC loopback mode */
+ tw32(MAC_MODE, cur_mode & ~MAC_MODE_PORT_INT_LPBACK);
+ /* Force link status check */
+ tg3_setup_phy(tp, 1);
+ netdev_info(dev, "Internal MAC loopback mode disabled.\n");
+ }
+sfeatures_unlock:
+ spin_unlock_bh(&tp->lock);
+
+sfeatures_out:
+ return err;
+}
+
static inline void tg3_set_mtu(struct net_device *dev, struct tg3 *tp,
int new_mtu)
{
@@ -15029,6 +15074,7 @@ static const struct net_device_ops tg3_netdev_ops = {
.ndo_tx_timeout = tg3_tx_timeout,
.ndo_change_mtu = tg3_change_mtu,
.ndo_fix_features = tg3_fix_features,
+ .ndo_set_features = tg3_set_features,
#ifdef CONFIG_NET_POLL_CONTROLLER
.ndo_poll_controller = tg3_poll_controller,
#endif
@@ -15045,6 +15091,7 @@ static const struct net_device_ops tg3_netdev_ops_dma_bug = {
.ndo_do_ioctl = tg3_ioctl,
.ndo_tx_timeout = tg3_tx_timeout,
.ndo_change_mtu = tg3_change_mtu,
+ .ndo_set_features = tg3_set_features,
#ifdef CONFIG_NET_POLL_CONTROLLER
.ndo_poll_controller = tg3_poll_controller,
#endif
@@ -15242,6 +15289,16 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
dev->features |= hw_features;
dev->vlan_features |= hw_features;
+ /*
+ * Add loopback capability only for a subset of devices that support
+ * MAC-LOOPBACK. Eventually this need to be enhanced to allow INT-PHY
+ * loopback for the remaining devices.
+ */
+ if (GET_ASIC_REV(tp->pci_chip_rev_id) != ASIC_REV_5780 ||
+ !tg3_flag(tp, CPMU_PRESENT))
+ /* Add the loopback capability */
+ dev->hw_features |= NETIF_F_LOOPBACK;
+
if (tp->pci_chip_rev_id == CHIPREV_ID_5705_A1 &&
!tg3_flag(tp, TSO_CAPABLE) &&
!(tr32(TG3PCI_PCISTATE) & PCISTATE_BUS_SPEED_HIGH)) {
--
1.7.3.1
^ permalink raw reply related
* [PATCHv1] bnx2x: Allow ethtool to enable/disable loopback.
From: Mahesh Bandewar @ 2011-05-04 1:20 UTC (permalink / raw)
To: Eilon Greenstein, David Miller; +Cc: netdev, Tom Herbert, Mahesh Bandewar
This patch updates the bnx2x_set_features() to handle loopback mode.
When enabled; it sets internal-MAC loopback.
Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
drivers/net/bnx2x/bnx2x_cmn.c | 16 ++++++++++++++++
drivers/net/bnx2x/bnx2x_main.c | 3 +++
2 files changed, 19 insertions(+), 0 deletions(-)
diff --git a/drivers/net/bnx2x/bnx2x_cmn.c b/drivers/net/bnx2x/bnx2x_cmn.c
index 8729061..e944d2a 100644
--- a/drivers/net/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/bnx2x/bnx2x_cmn.c
@@ -2506,15 +2506,31 @@ int bnx2x_set_features(struct net_device *dev, u32 features)
{
struct bnx2x *bp = netdev_priv(dev);
u32 flags = bp->flags;
+ bool bnx2x_reload = false;
if (features & NETIF_F_LRO)
flags |= TPA_ENABLE_FLAG;
else
flags &= ~TPA_ENABLE_FLAG;
+ if (features & NETIF_F_LOOPBACK) {
+ if (bp->link_params.loopback_mode != LOOPBACK_BMAC) {
+ bp->link_params.loopback_mode = LOOPBACK_BMAC;
+ bnx2x_reload = true;
+ }
+ } else {
+ if (bp->link_params.loopback_mode != LOOPBACK_NONE) {
+ bp->link_params.loopback_mode = LOOPBACK_NONE;
+ bnx2x_reload = true;
+ }
+ }
+
if (flags ^ bp->flags) {
bp->flags = flags;
+ bnx2x_reload = true;
+ }
+ if (bnx2x_reload) {
if (bp->recovery_state == BNX2X_RECOVERY_DONE)
return bnx2x_reload_if_running(dev);
/* else: bnx2x_nic_load() will be called at end of recovery */
diff --git a/drivers/net/bnx2x/bnx2x_main.c b/drivers/net/bnx2x/bnx2x_main.c
index bfd7ac9..2c6b5e2 100644
--- a/drivers/net/bnx2x/bnx2x_main.c
+++ b/drivers/net/bnx2x/bnx2x_main.c
@@ -9435,6 +9435,9 @@ static int __devinit bnx2x_init_dev(struct pci_dev *pdev,
if (bp->flags & USING_DAC_FLAG)
dev->features |= NETIF_F_HIGHDMA;
+ /* Add Loopback capability to the device */
+ dev->hw_features |= NETIF_F_LOOPBACK;
+
#ifdef BCM_DCBNL
dev->dcbnl_ops = &bnx2x_dcbnl_ops;
#endif
--
1.7.3.1
^ permalink raw reply related
* [PATCHv1] forcedeth: Allow ethtool to enable/disable loopback.
From: Mahesh Bandewar @ 2011-05-04 1:22 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Tom Herbert, Mahesh Bandewar
This patch updates nv_set_features() to handle loopback mode.
When enabled; it sets internal-PHY loopback.
Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
drivers/net/forcedeth.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 55 insertions(+), 1 deletions(-)
diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
index d09e8b0..10d522e 100644
--- a/drivers/net/forcedeth.c
+++ b/drivers/net/forcedeth.c
@@ -4474,6 +4474,53 @@ static int nv_set_pauseparam(struct net_device *dev, struct ethtool_pauseparam*
return 0;
}
+static int nv_set_loopback(struct net_device *dev, u32 features)
+{
+ struct fe_priv *np = netdev_priv(dev);
+ unsigned long flags;
+ u32 miicontrol;
+ int err, retval = 0;
+
+ spin_lock_irqsave(&np->lock, flags);
+ miicontrol = mii_rw(dev, np->phyaddr, MII_BMCR, MII_READ);
+ if (features & NETIF_F_LOOPBACK) {
+ if (miicontrol & BMCR_LOOPBACK) {
+ spin_unlock_irqrestore(&np->lock, flags);
+ return retval;
+ }
+ nv_disable_irq(dev);
+ /* Turn on loopback mode */
+ miicontrol |= BMCR_LOOPBACK | BMCR_FULLDPLX;
+ err = mii_rw(dev, np->phyaddr, MII_BMCR, miicontrol);
+ if (err) {
+ retval = -EAGAIN;
+ spin_unlock_irqrestore(&np->lock, flags);
+ phy_init(dev);
+ } else {
+ /* Force link up */
+ netif_carrier_on(dev);
+ spin_unlock_irqrestore(&np->lock, flags);
+ netdev_info(dev, "Internal PHY loopback mode enabled.\n");
+ }
+ } else {
+ if (!(miicontrol & BMCR_LOOPBACK)) {
+ spin_unlock_irqrestore(&np->lock, flags);
+ return retval;
+ }
+ nv_disable_irq(dev);
+ /* Turn off loopback */
+ spin_unlock_irqrestore(&np->lock, flags);
+ netdev_info(dev, "Internal PHY loopback mode disabled.\n");
+ phy_init(dev);
+ }
+ msleep(500);
+ spin_lock_irqsave(&np->lock, flags);
+ nv_enable_irq(dev);
+ spin_unlock_irqrestore(&np->lock, flags);
+
+ return retval;
+}
+
static u32 nv_fix_features(struct net_device *dev, u32 features)
{
/* vlan is dependent on rx checksum offload */
@@ -4488,6 +4535,7 @@ static int nv_set_features(struct net_device *dev, u32 features)
struct fe_priv *np = netdev_priv(dev);
u8 __iomem *base = get_hwbase(dev);
u32 changed = dev->features ^ features;
+ int retval = 0;
if (changed & NETIF_F_RXCSUM) {
spin_lock_irq(&np->lock);
@@ -4502,8 +4550,11 @@ static int nv_set_features(struct net_device *dev, u32 features)
spin_unlock_irq(&np->lock);
}
+ if ((changed & NETIF_F_LOOPBACK) && netif_running(dev)) {
+ retval = nv_set_loopback(dev, features);
+ }
- return 0;
+ return retval;
}
static int nv_get_sset_count(struct net_device *dev, int sset)
@@ -5341,6 +5392,9 @@ static int __devinit nv_probe(struct pci_dev *pci_dev, const struct pci_device_i
dev->features |= dev->hw_features;
}
+ /* Add loopback capability to the device. */
+ dev->hw_features |= NETIF_F_LOOPBACK;
+
np->vlanctl_bits = 0;
if (id->driver_data & DEV_HAS_VLAN) {
np->vlanctl_bits = NVREG_VLANCONTROL_ENABLE;
--
1.7.3.1
^ permalink raw reply related
* Re: [ethtool PATCH 4/4] v5 Add RX packet classification interface
From: Dimitris Michailidis @ 2011-05-04 1:35 UTC (permalink / raw)
To: Alexander Duyck
Cc: Ben Hutchings, davem@davemloft.net, Kirsher, Jeffrey T,
netdev@vger.kernel.org
In-Reply-To: <4DC09DE0.8070102@intel.com>
On 05/03/2011 05:29 PM, Alexander Duyck wrote:
> On 5/3/2011 4:34 PM, Ben Hutchings wrote:
>> On Tue, 2011-05-03 at 16:23 -0700, Dimitris Michailidis wrote:
>>> On 05/03/2011 09:12 AM, Alexander Duyck wrote:
>> [...]
>>>> +int rxclass_rule_ins(int fd, struct ifreq *ifr,
>>>> + struct ethtool_rx_flow_spec *fsp)
>>>> +{
>>>> + struct ethtool_rxnfc nfccmd;
>>>> + __u32 loc = fsp->location;
>>>> + int err;
>>>> +
>>>> + /*
>>>> + * if location is unspecified pull rules from device
>>>> + * and allocate a free rule for our use
>>>> + */
>>>> + if (loc == RX_CLS_LOC_UNSPEC) {
>>>> + /* init table of available rules */
>>>> + err = rmgr_init(fd, ifr);
>>>> + if (err< 0)
>>>> + return err;
>>>> +
>>>> + /* verify rule location */
>>>> + err = rmgr_add(fsp);
>>>> + if (err< 0)
>>>> + return err;
>>>> +
>>>> + /* cleanup table and free resources */
>>>> + rmgr_cleanup();
>>>> + }
>>>
>>> This logic where ethtool tries to select a filter slot when a user
>>> provides
>>> RX_CLS_LOC_UNSPEC does not work in general. It assumes that all
>>> slots are
>>> equal and a new filter can go into any available slot. But a device
>>> may have
>>> restrictions on where a filter may go that ethtool doesn't know.
>>
>> I agree. And if filter lookup is largely hash-based (as it is in
>> Solarflare hardware) the user will also find it very difficult to
>> specify the right location!
>
> The thing to keep in mind is that the index doesn't have to be a
> hardware index. In ixgbe we have a field in the hardware which is meant
> to just be a unique software index and that is what I am using as the
> location field for our filters. All the location information in the
> rules really provides is a logical way of tracking rules. It doesn't
> necessarily have to represent the physical location of the rule in
> hardware.
I appreciate the intent but there are couple problems.
a) ethtool.h documents location as
* @location: Index of filter in hardware table
i.e., physical location. But we could change that.
b) for TCAMs physical location is important and if ethtool offers to provide
only a logical index and massages the original user input to do so where
will a driver get the physical location it ultimately needs? For a device
with physical indices a multiple of 4 the logical index ethtool picks will
very frequently be illegal as physical location. E.g., if location 1 is
available ethtool will keep selecting it and the driver will need to deal
with these requests without the benefit of knowing what the user really
asked for.
Another problem with ethtool selecting locations is it assumes it's the sole
allocator (I think there's a comment in the code pointing this out). But
this isn't a valid assumption, e.g., HW RFS comes to mind as another allocator.
^ permalink raw reply
* Re: [PATCHv3 2/2] tg3: Allow ethtool to enable/disable loopback.
From: Matt Carlson @ 2011-05-04 1:52 UTC (permalink / raw)
To: Mahesh Bandewar
Cc: Matthew Carlson, David Miller, netdev, Michael Chan,
Ben Hutchings, Micha? Miros?aw, Tom Herbert
In-Reply-To: <1304471935-402-3-git-send-email-maheshb@google.com>
On Tue, May 03, 2011 at 06:18:55PM -0700, Mahesh Bandewar wrote:
> + /*
> + * Add loopback capability only for a subset of devices that support
> + * MAC-LOOPBACK. Eventually this need to be enhanced to allow INT-PHY
> + * loopback for the remaining devices.
> + */
> + if (GET_ASIC_REV(tp->pci_chip_rev_id) != ASIC_REV_5780 ||
> + !tg3_flag(tp, CPMU_PRESENT))
> + /* Add the loopback capability */
> + dev->hw_features |= NETIF_F_LOOPBACK;
s/||/&&/
^ permalink raw reply
* Re: [RFC v3 02/10] Revert "lsm: Remove the socket_post_accept() hook"
From: Tetsuo Handa @ 2011-05-04 2:28 UTC (permalink / raw)
To: paul.moore, sam
Cc: linux-security-module, linux-kernel, netdev, netfilter-devel,
hadi, kaber, zbr, root
In-Reply-To: <201105031802.34724.paul.moore@hp.com>
Paul Moore wrote:
> On Tuesday, May 03, 2011 10:24:15 AM Samir Bellabes wrote:
> > snet needs to reintroduce this hook, as it was designed to be: a hook for
> > updating security informations on objects.
>
> Looking at this and 5/10 again, it seems that you should be able to do what
> you need with the sock_graft() hook. Am I missing something?
>
> My apologies if we've already discussed this approach previously ...
static void snet_socket_post_accept(struct socket *sock, struct socket *newsock)
{
static void snet_do_send_event(struct snet_info *info)
{
int snet_nl_send_event(struct snet_info *info)
{
skb_rsp = genlmsg_new(size, GFP_KERNEL);
genlmsg_unicast()
}
}
}
First problem with using snet_do_send_event() from security_sock_graft() is
that we have to use GFP_ATOMIC rather than GFP_KERNEL because we are inside
write_lock_bh()/write_unlock_bh().
static inline int genlmsg_unicast(struct net *net, struct sk_buff *skb, u32 pid)
{
static inline int nlmsg_unicast(struct sock *sk, struct sk_buff *skb, u32 pid)
{
int netlink_unicast(struct sock *ssk, struct sk_buff *skb,
u32 pid, MSG_DONTWAIT)
{
int netlink_attachskb(struct sock *sk, struct sk_buff *skb,
long *timeo, struct sock *ssk)
{
if (!*timeo) {
return -EAGAIN;
}
}
}
}
Second problem is that genlmsg_unicast() might return -EAGAIN because we can't
sleep inside write_lock_bh()/write_unlock_bh().
Third problem (though independent with security_sock_graft()) is that
snet_do_send_event() ignores snet_nl_send_event() failure.
^ permalink raw reply
* Re: [ethtool PATCH 4/4] v5 Add RX packet classification interface
From: Alexander Duyck @ 2011-05-04 3:10 UTC (permalink / raw)
To: Dimitris Michailidis
Cc: Alexander Duyck, Ben Hutchings, davem@davemloft.net,
Kirsher, Jeffrey T, netdev@vger.kernel.org
In-Reply-To: <4DC0AD7B.7070009@chelsio.com>
On Tue, May 3, 2011 at 6:35 PM, Dimitris Michailidis <dm@chelsio.com> wrote:
> On 05/03/2011 05:29 PM, Alexander Duyck wrote:
>> The thing to keep in mind is that the index doesn't have to be a hardware
>> index. In ixgbe we have a field in the hardware which is meant to just be a
>> unique software index and that is what I am using as the location field for
>> our filters. All the location information in the rules really provides is a
>> logical way of tracking rules. It doesn't necessarily have to represent the
>> physical location of the rule in hardware.
>
> I appreciate the intent but there are couple problems.
> a) ethtool.h documents location as
>
> * @location: Index of filter in hardware table
>
> i.e., physical location. But we could change that.
I will probably want to change that. The fact is as I recall even niu
was using a hash in addition to the location index. As such it isn't
really the true location in the hardware since there is a second piece
to determining the actual location in hardware.
> b) for TCAMs physical location is important and if ethtool offers to provide
> only a logical index and massages the original user input to do so where
> will a driver get the physical location it ultimately needs? For a device
> with physical indices a multiple of 4 the logical index ethtool picks will
> very frequently be illegal as physical location. E.g., if location 1 is
> available ethtool will keep selecting it and the driver will need to deal
> with these requests without the benefit of knowing what the user really
> asked for.
>
> Another problem with ethtool selecting locations is it assumes it's the sole
> allocator (I think there's a comment in the code pointing this out). But
> this isn't a valid assumption, e.g., HW RFS comes to mind as another
> allocator.
The other thing to keep in mind is that this doesn't preclude the
option of adding an ethtool command at some point in the future for
handling the determination of filter location. The fact is all that
would need to be done is to add an extra ioctl call to determine the
location based on the filter and if that returns op not supported we
fall back to the current rule manager.
The way I have things setup now provides a good foundation in
user-space for managing the rules. I fully admit it doesn't fit all
solutions, and I welcome follow-on patches to add extra functionality,
but at this time I really would prefer avoiding adding extra
functionality for yet to be implemented features in device drivers.
The ntuple display functionality provides a good example of why I
would prefer to avoid it. Everything looked like it should have
worked when get_rx_ntuple was implemented in the device drivers, but
as soon as I implemented a get_rx_ntuple for ixgbe I quickly
discovered it didn't work and as such I am now stuck moving everything
over to network flow classifier for ixgbe.
Thanks,
Alex
^ permalink raw reply
* Re: [PATCH v4 2/5] can: add rtnetlink support
From: Kurt Van Dijck @ 2011-05-04 4:20 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <4DBD9586.7080908-fJ+pQTUTwRTk1uMJSBkQmQ@public.gmane.org>
> Besides some minor adaptions to support the SVN i left out some infrastructure
> changes you made in patch 2/5 to constify this ...
>
> > -static struct can_proto *proto_tab[CAN_NPROTO] __read_mostly;
> > +static const struct can_proto *proto_tab[CAN_NPROTO] __read_mostly;
>
> and this ...
>
> > -static struct can_proto *can_try_module_get(int protocol)
> > +static const struct can_proto *can_try_module_get(int protocol)
>
> (..)
>
> > +static inline void can_put_proto(const struct can_proto *cp)
> > +{
> > + module_put(cp->prot->owner);
> > +}
>
> These infrastructure changes did not really belong to the rtnetlink support.
I see. I see I've made a mistake in the patch set here.
I'll go for a seperate 'make it const' patch.
Thanks,
Kurt
^ permalink raw reply
* [net-next-2.6 PATCH] can: make struct can_proto const
From: Kurt Van Dijck @ 2011-05-04 4:40 UTC (permalink / raw)
To: socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
netdev-u79uwXL29TY76Z2rM5mHXA
commit 53914b67993c724cec585863755c9ebc8446e83b had the
same message. That commit did put everything in place but
did not make can_proto const itself.
Signed-off-by: Kurt Van Dijck <kurt.van.dijck-/BeEPy95v10@public.gmane.org>
diff --git a/include/linux/can/core.h b/include/linux/can/core.h
index 6f70a6d..5ce6b5d 100644
--- a/include/linux/can/core.h
+++ b/include/linux/can/core.h
@@ -44,8 +44,8 @@ struct can_proto {
/* function prototypes for the CAN networklayer core (af_can.c) */
-extern int can_proto_register(struct can_proto *cp);
-extern void can_proto_unregister(struct can_proto *cp);
+extern int can_proto_register(const struct can_proto *cp);
+extern void can_proto_unregister(const struct can_proto *cp);
extern int can_rx_register(struct net_device *dev, canid_t can_id,
canid_t mask,
diff --git a/net/can/af_can.c b/net/can/af_can.c
index a8dcaa4..5b52762 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -84,7 +84,7 @@ static DEFINE_SPINLOCK(can_rcvlists_lock);
static struct kmem_cache *rcv_cache __read_mostly;
/* table of registered CAN protocols */
-static struct can_proto *proto_tab[CAN_NPROTO] __read_mostly;
+static const struct can_proto *proto_tab[CAN_NPROTO] __read_mostly;
static DEFINE_MUTEX(proto_tab_lock);
struct timer_list can_stattimer; /* timer for statistics update */
@@ -115,9 +115,9 @@ static void can_sock_destruct(struct sock *sk)
skb_queue_purge(&sk->sk_receive_queue);
}
-static struct can_proto *can_try_module_get(int protocol)
+static const struct can_proto *can_try_module_get(int protocol)
{
- struct can_proto *cp;
+ const struct can_proto *cp;
rcu_read_lock();
cp = rcu_dereference(proto_tab[protocol]);
@@ -132,7 +132,7 @@ static int can_create(struct net *net, struct socket *sock, int protocol,
int kern)
{
struct sock *sk;
- struct can_proto *cp;
+ const struct can_proto *cp;
int err = 0;
sock->state = SS_UNCONNECTED;
@@ -691,7 +691,7 @@ drop:
* -EBUSY protocol already in use
* -ENOBUF if proto_register() fails
*/
-int can_proto_register(struct can_proto *cp)
+int can_proto_register(const struct can_proto *cp)
{
int proto = cp->protocol;
int err = 0;
@@ -728,7 +728,7 @@ EXPORT_SYMBOL(can_proto_register);
* can_proto_unregister - unregister CAN transport protocol
* @cp: pointer to CAN protocol structure
*/
-void can_proto_unregister(struct can_proto *cp)
+void can_proto_unregister(const struct can_proto *cp)
{
int proto = cp->protocol;
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 8a6a05e..cced806 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1601,7 +1601,7 @@ static struct proto bcm_proto __read_mostly = {
.init = bcm_init,
};
-static struct can_proto bcm_can_proto __read_mostly = {
+static const struct can_proto bcm_can_proto = {
.type = SOCK_DGRAM,
.protocol = CAN_BCM,
.ops = &bcm_ops,
diff --git a/net/can/raw.c b/net/can/raw.c
index 0eb39a7..dea99a6 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -774,7 +774,7 @@ static struct proto raw_proto __read_mostly = {
.init = raw_init,
};
-static struct can_proto raw_can_proto __read_mostly = {
+static const struct can_proto raw_can_proto = {
.type = SOCK_RAW,
.protocol = CAN_RAW,
.ops = &raw_ops,
^ permalink raw reply related
* [net-next-2.6 PATCH] can: rename can_try_module_get to can_get_proto
From: Kurt Van Dijck @ 2011-05-04 4:42 UTC (permalink / raw)
To: socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
netdev-u79uwXL29TY76Z2rM5mHXA
can: rename can_try_module_get to can_get_proto
can_try_module_get does return a struct can_proto.
The name explains what is done in so much detail that a caller
may not notice that a struct can_proto is locked/unlocked.
Signed-off-by: Kurt Van Dijck <kurt.van.dijck-/BeEPy95v10@public.gmane.org>
diff --git a/net/can/af_can.c b/net/can/af_can.c
index 5b52762..094fc53 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -115,7 +115,7 @@ static void can_sock_destruct(struct sock *sk)
skb_queue_purge(&sk->sk_receive_queue);
}
-static const struct can_proto *can_try_module_get(int protocol)
+static const struct can_proto *can_get_proto(int protocol)
{
const struct can_proto *cp;
@@ -128,6 +128,11 @@ static const struct can_proto *can_try_module_get(int protocol)
return cp;
}
+static inline void can_put_proto(const struct can_proto *cp)
+{
+ module_put(cp->prot->owner);
+}
+
static int can_create(struct net *net, struct socket *sock, int protocol,
int kern)
{
@@ -143,7 +148,7 @@ static int can_create(struct net *net, struct socket *sock, int protocol,
if (!net_eq(net, &init_net))
return -EAFNOSUPPORT;
- cp = can_try_module_get(protocol);
+ cp = can_get_proto(protocol);
#ifdef CONFIG_MODULES
if (!cp) {
@@ -160,7 +165,7 @@ static int can_create(struct net *net, struct socket *sock, int protocol,
printk(KERN_ERR "can: request_module "
"(can-proto-%d) failed.\n", protocol);
- cp = can_try_module_get(protocol);
+ cp = can_get_proto(protocol);
}
#endif
@@ -195,7 +200,7 @@ static int can_create(struct net *net, struct socket *sock, int protocol,
}
errout:
- module_put(cp->prot->owner);
+ can_put_proto(cp);
return err;
}
^ permalink raw reply related
* Re: [net-next-2.6 PATCH] ethtool: Support to take FW dump
From: Anirban Chakraborty @ 2011-05-04 5:22 UTC (permalink / raw)
To: Ben Hutchings; +Cc: netdev, David Miller
In-Reply-To: <1304471194.3203.5.camel@localhost>
On May 3, 2011, at 6:06 PM, Ben Hutchings wrote:
> <snip>
>
> I understand that the userland application will need to get the size
> that way. I'm saying that this code in the kernel should also get the
> size from the driver, so that a malicious application cannot make the
> kernel allocate an excessively large buffer.
Makes sense. Will do so.
>
> Also, you'll need to submit your implementation along with this, as
> David won't accept an extension to the API without a driver that
> implements it.
I have implemented it in qlcnic driver. Will submit it along with the v2 patches.
Thanks.
-Anirban
^ permalink raw reply
* Re: [PATCH] usbnet: Transfer of maintainership
From: Richard Cochran @ 2011-05-04 5:45 UTC (permalink / raw)
To: Oliver Neukum
Cc: davem-fT/PcQaiUtIeIZ0/mPfg9Q, netdev-u79uwXL29TY76Z2rM5mHXA,
USB list
In-Reply-To: <201104291419.04498.oneukum-l3A5Bk7waGM@public.gmane.org>
On Fri, Apr 29, 2011 at 02:19:04PM +0200, Oliver Neukum wrote:
> USB "USBNET" DRIVER FRAMEWORK
> -M: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> +M: Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org>
Oliver,
We have been looking at usbnet and a have question.
Usbnet doesn't use either phylib or napi, but I think the reason is
probably purely historical. Is there a technical reason why phylib or
napi won't work with usbnet devices?
If not, I would like to convert them in the near future.
Thanks,
Richard
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox