* Re: [PATCH net-next-2.6 2/2] be2net: fix mbox polling for signal reception
From: David Miller @ 2011-05-12 21:29 UTC (permalink / raw)
To: bhutchings; +Cc: sathya.perla, netdev
In-Reply-To: <1305204546.4065.395.camel@localhost>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Thu, 12 May 2011 13:49:06 +0100
> On Thu, 2011-05-12 at 11:41 +0530, Sathya Perla wrote:
>> Sending mbox cmds require multiple steps of writing to the DB register and polling
>> for an ack. Gettting interrupted in the middle by a signal breaks the mbox protocol.
>> So, set the task to UNINTERRUPTIBLE for mbox polling.
>>
>> Signed-off-by: Sathya Perla <sathya.perla@emulex.com>
>> ---
>> drivers/net/benet/be_cmds.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/net/benet/be_cmds.c b/drivers/net/benet/be_cmds.c
>> index bff41ed..55c8301 100644
>> --- a/drivers/net/benet/be_cmds.c
>> +++ b/drivers/net/benet/be_cmds.c
>> @@ -297,7 +297,7 @@ static int be_mbox_db_ready_wait(struct be_adapter *adapter, void __iomem *db)
>> return -1;
>> }
>>
>> - set_current_state(TASK_INTERRUPTIBLE);
>> + set_current_state(TASK_UNINTERRUPTIBLE);
>> schedule_timeout(msecs_to_jiffies(1));
>
> msleep(1) is a lot more readable.
Agreed.
Sathya, please make this change and resubmit this patch series.
Thanks.
^ permalink raw reply
* Re: [PATCH net-next-2.6] l2tp: fix potential rcu race
From: David Miller @ 2011-05-12 21:27 UTC (permalink / raw)
To: paulmck; +Cc: eric.dumazet, netdev, jchapman
In-Reply-To: <20110512142641.GO2258@linux.vnet.ibm.com>
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date: Thu, 12 May 2011 07:26:41 -0700
> On Thu, May 12, 2011 at 06:22:36AM +0200, Eric Dumazet wrote:
>> While trying to remove useless synchronize_rcu() calls, I found l2tp is
>> indeed incorrectly using two of such calls, but also bumps tunnel
>> refcount after list insertion.
>>
>> tunnel refcount must be incremented before being made publically visible
>> by rcu readers.
>>
>> This fix can be applied to 2.6.35+ and might need a backport for older
>> kernels, since things were shuffled in commit fd558d186df2c
>> (l2tp: Split pppol2tp patch into separate l2tp and ppp parts)
>
> Ouch! Good catch, Eric!
>
> Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Applied, thanks Eric.
^ permalink raw reply
* [PATCH 1/2 v2] sctp: sctp_sendmsg: Don't initialize default_sinfo
From: Joe Perches @ 2011-05-12 21:27 UTC (permalink / raw)
To: David Miller; +Cc: vladislav.yasevich, sri, linux-sctp, netdev, linux-kernel
In-Reply-To: <20110512.170603.549349521517995900.davem@davemloft.net>
This variable only needs initialization when cmsgs.info
is NULL.
Use memset to ensure padding is also zeroed so
kernel doesn't leak any data.
Signed-off-by: Joe Perches <joe@perches.com>
---
On Thu, 2011-05-12 at 17:06 -0400, David Miller wrote:
From: Joe Perches <joe@perches.com>
> Date: Thu, 12 May 2011 12:19:09 -0700
> > This variable only needs initialization when cmsgs.info
> > is NULL.
> > Don't use memset, just initialize every struct member.
> > Signed-off-by: Joe Perches <joe@perches.com>
> I don't think you do this, this structure has padding holes on pretty
> much every architecture.
> It starts with 3 u16's, then there is a u32, so there is a 2-byte
> piece of padding after the 3rd u16.
> Can you prove that these uninitialized portions never make it to
> userspace? If you can, that proof belongs in the commit message.
Thanks David. I didn't notice it went to userspace.
> I think it's too risky.
It is. I like memset.
The current initialization isn't guaranteed by c90 standard
to zero all padding either. In practice it does though.
The idea was to avoid doing a (non-memset) struct foo bar = {}
when unnecessary for every packet as it's only needed when
cmsgs.info is NULL.
net/sctp/socket.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 33d9ee6..d4b8db1 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -1496,7 +1496,7 @@ SCTP_STATIC int sctp_sendmsg(struct kiocb *iocb, struct sock *sk,
struct sctp_chunk *chunk;
union sctp_addr to;
struct sockaddr *msg_name = NULL;
- struct sctp_sndrcvinfo default_sinfo = { 0 };
+ struct sctp_sndrcvinfo default_sinfo;
struct sctp_sndrcvinfo *sinfo;
struct sctp_initmsg *sinit;
sctp_assoc_t associd = 0;
@@ -1760,6 +1760,7 @@ SCTP_STATIC int sctp_sendmsg(struct kiocb *iocb, struct sock *sk,
/* If the user didn't specify SNDRCVINFO, make up one with
* some defaults.
*/
+ memset(&default_sinfo, 0, sizeof(default_sinfo));
default_sinfo.sinfo_stream = asoc->default_stream;
default_sinfo.sinfo_flags = asoc->default_flags;
default_sinfo.sinfo_ppid = asoc->default_ppid;
^ permalink raw reply related
* Re: [PATCH] net: af_packet: Don't initialize vnet_hdr
From: David Miller @ 2011-05-12 21:26 UTC (permalink / raw)
To: joe; +Cc: linux-kernel, netdev
In-Reply-To: <18a010357eb63dfd50751c5eb529f6261cf90ecd.1305232529.git.joe@perches.com>
From: Joe Perches <joe@perches.com>
Date: Thu, 12 May 2011 13:36:10 -0700
> Save a memset, initialize only the portion necessary.
>
> packet_snd either gets this structure completely from
> memcpy_fromiovec or uses only the hdr_len set to 0,
> so don't always initialize the structure to 0.
>
> Signed-off-by: Joe Perches <joe@perches.com>
On ARM this won't be tightly packed, therefore you'll leave
uninitialized pieces of padding in this structure and this
thing is an "on-the-wire" network header.
I'm not applying this.
^ permalink raw reply
* Re: Kernel crash after using new Intel NIC (igb)
From: Eric Dumazet @ 2011-05-12 21:15 UTC (permalink / raw)
To: Arun Sharma
Cc: Maximilian Engelhardt, linux-kernel, netdev, StuStaNet Vorstand
In-Reply-To: <20110512211033.GA3468@dev1756.snc6.facebook.com>
Le jeudi 12 mai 2011 à 14:10 -0700, Arun Sharma a écrit :
> On Wed, Apr 27, 2011 at 01:51:59PM +0200, Maximilian Engelhardt wrote:
> >
> > thank you for this information. I updated the kernel of the affected server to
> > version 2.6.38.4 yesterday. I'll report when there are still crashes, but it
> > might take a while, as in the past they only happened within the interval of
> > weeks to month.
>
> I've seen this panic on 2.6.38.y as well. It's rare (< 1%), but the
> panic consistently happens at the same place.
>
> Are any of the inetpeer.c commits in the 2.6.39 tree, which are not in -stable yet,
> good candidates to try?
>
Probably not.
What gives slub_nomerge=1 for you ?
^ permalink raw reply
* Re: Kernel crash after using new Intel NIC (igb)
From: Arun Sharma @ 2011-05-12 21:10 UTC (permalink / raw)
To: Maximilian Engelhardt
Cc: Eric Dumazet, linux-kernel, netdev, StuStaNet Vorstand
In-Reply-To: <201104271352.00601.maxi@daemonizer.de>
On Wed, Apr 27, 2011 at 01:51:59PM +0200, Maximilian Engelhardt wrote:
>
> thank you for this information. I updated the kernel of the affected server to
> version 2.6.38.4 yesterday. I'll report when there are still crashes, but it
> might take a while, as in the past they only happened within the interval of
> weeks to month.
I've seen this panic on 2.6.38.y as well. It's rare (< 1%), but the
panic consistently happens at the same place.
Are any of the inetpeer.c commits in the 2.6.39 tree, which are not in -stable yet,
good candidates to try?
-Arun
^ permalink raw reply
* Re: [PATCH 1/2] sctp: sctp_sendmsg: Don't initialize default_sinfo
From: David Miller @ 2011-05-12 21:06 UTC (permalink / raw)
To: joe; +Cc: vladislav.yasevich, sri, linux-sctp, netdev, linux-kernel
In-Reply-To: <a4e846ed62d7f9222dc9b09ca00b5356a1eb9b73.1305227620.git.joe@perches.com>
From: Joe Perches <joe@perches.com>
Date: Thu, 12 May 2011 12:19:09 -0700
> This variable only needs initialization when cmsgs.info
> is NULL.
>
> Don't use memset, just initialize every struct member.
>
> Signed-off-by: Joe Perches <joe@perches.com>
I don't think you do this, this structure has padding holes on pretty
much every architecture.
It starts with 3 u16's, then there is a u32, so there is a 2-byte
piece of padding after the 3rd u16.
Can you prove that these uninitialized portions never make it to
userspace? If you can, that proof belongs in the commit message.
I think it's too risky.
^ permalink raw reply
* Re: [PATCH 0/3] Fix 8390 regressions
From: David Miller @ 2011-05-12 21:02 UTC (permalink / raw)
To: geert; +Cc: shemminger, ysato, linux, fthain, netdev, linux-kernel,
linux-m68k
In-Reply-To: <1305227500-15595-1-git-send-email-geert@linux-m68k.org>
From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: Thu, 12 May 2011 19:11:37 +0000
> These patches fix regressions in 3 8390-based network drivers:
> [1/3] zorro8390: Fix regression caused during net_device_ops conversion
> [2/3] hydra: Fix regression caused during net_device_ops conversion
> [3/3] ne-h8300: Fix regression caused during net_device_ops conversion
>
> The first one, for zorro8390, has been tested.
> The second one, for hydra, has been compile-tested only.
>
> Based on commits 8cfd9e923be54ef66ce174a93f4592b444b96407 ("[ARM] RiscPC: Fix
> etherh oops") and 217cbfa856dc1cbc2890781626c4032d9e3ec59f ("mac8390: fix
> regression caused during net_device_ops conversion"), and the patch for
> zorro8390, we have good reasons to believe hydra and ne-h8300 are affected
> as well, as they all include lib8390.c.
> Hence patches for those are also included, although I could not test them.
All applied, thanks Geert, and I made the "From: " removals you requested.
^ permalink raw reply
* Re: [PATCHv3 net-next-2.6 2/3] qlcnic: Take FW dump via ethtool
From: Ben Hutchings @ 2011-05-12 21:02 UTC (permalink / raw)
To: Anirban Chakraborty; +Cc: netdev, David Miller
In-Reply-To: <135A6631-C557-4C01-9FDD-F9E58F555168@qlogic.com>
On Thu, 2011-05-12 at 13:54 -0700, Anirban Chakraborty wrote:
> On May 12, 2011, at 12:18 PM, Ben Hutchings wrote:
[...]
> > What prevents this sequence:
> >
> > 1. Driver detects firmware dump is required, and creates the dump
> > (length L1).
> > 2. User changes firmware dump flags through ethtool.
> > 3. User starts to save firmware dump through ethtool:
> > a. ethtool utility reads dump length (= L1) and allocates user buffer
> > b. ethtool utility reads dump:
> > c. ethtool core reads dump length (L1) and allocates kernel buffer
> > 4. Meanwhile, driver detects firmware dump is required again, and
> > creates a new dump (length L2, since dump flags changed)
>
> This step won't happen as driver ensures that if a dump is taken that is not
> cleared yet by ethtool utility, it is not going to take any further dump. So, until
> the get_dump_data() has been called to fetch the dump data, changing dump
> flag (and hence dump size) will not have any effect.
> In 3 above, ethtool core created a buffer of L1 size but hasn't yet called
> get_dump_data() of the driver, so driver is still holding onto its previously
> dumped data even though capture mask has been changed and the driver
> encountered a situation where it ought to take the dump.
OK, that sounds safe.
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: [PATCHv3 net-next-2.6 2/3] qlcnic: Take FW dump via ethtool
From: Anirban Chakraborty @ 2011-05-12 20:54 UTC (permalink / raw)
To: Ben Hutchings; +Cc: netdev, David Miller
In-Reply-To: <1305227938.5214.48.camel@bwh-desktop>
On May 12, 2011, at 12:18 PM, Ben Hutchings wrote:
> On Thu, 2011-05-12 at 11:53 -0700, Anirban Chakraborty wrote:
>> On May 12, 2011, at 10:27 AM, Ben Hutchings wrote:
>>
>>> On Wed, 2011-05-11 at 15:54 -0700, Anirban Chakraborty wrote:
>>>> Driver checks if the previous dump has been cleared before taking the dump.
>>>> It doesn't take the dump if it is not cleared.
>>>>
>>>> Changes from v2:
>>>> Added lock to protect dump data structures from being mangled while
>>>> dumping or setting them via ethtool.
>>>
>>> Unfortunately it still seems to be possible for the dump length to
>>> change between the ethtool core calling qlcnic_get_dump_flag() and
>>> qlcnic_get_dump_data().
>>
>> dump length is serialized via the driver lock. dump length is a static
>> entity for a given capture mask and it can only be changed when there
>> is a different capture mask set in the driver (via calling set_dump()
>> from ethtool core).
>
> OK.
>
>> Actual dump size is determined during the initial steps of FW dump
>> which takes the driver lock to start with. So, I am not sure how the
>> dump length could be changed between the calls to get_dump_flag and
>> get_dump_data from within the ethtool core without a call to
>> set_dump() in between.
> [...]
>
> What prevents this sequence:
>
> 1. Driver detects firmware dump is required, and creates the dump
> (length L1).
> 2. User changes firmware dump flags through ethtool.
> 3. User starts to save firmware dump through ethtool:
> a. ethtool utility reads dump length (= L1) and allocates user buffer
> b. ethtool utility reads dump:
> c. ethtool core reads dump length (L1) and allocates kernel buffer
> 4. Meanwhile, driver detects firmware dump is required again, and
> creates a new dump (length L2, since dump flags changed)
This step won't happen as driver ensures that if a dump is taken that is not
cleared yet by ethtool utility, it is not going to take any further dump. So, until
the get_dump_data() has been called to fetch the dump data, changing dump
flag (and hence dump size) will not have any effect.
In 3 above, ethtool core created a buffer of L1 size but hasn't yet called
get_dump_data() of the driver, so driver is still holding onto its previously
dumped data even though capture mask has been changed and the driver
encountered a situation where it ought to take the dump.
-Anirban
^ permalink raw reply
* [PATCH] net: af_packet: Don't initialize vnet_hdr
From: Joe Perches @ 2011-05-12 20:36 UTC (permalink / raw)
To: linux-kernel; +Cc: David S. Miller, netdev
Save a memset, initialize only the portion necessary.
packet_snd either gets this structure completely from
memcpy_fromiovec or uses only the hdr_len set to 0,
so don't always initialize the structure to 0.
Signed-off-by: Joe Perches <joe@perches.com>
---
net/packet/af_packet.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 549527b..ac88df9 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1123,7 +1123,7 @@ static int packet_snd(struct socket *sock,
__be16 proto;
unsigned char *addr;
int ifindex, err, reserve = 0;
- struct virtio_net_hdr vnet_hdr = { 0 };
+ struct virtio_net_hdr vnet_hdr;
int offset = 0;
int vnet_hdr_len;
struct packet_sock *po = pkt_sk(sk);
@@ -1206,7 +1206,9 @@ static int packet_snd(struct socket *sock,
goto out_unlock;
}
- }
+ } else
+ vnet_hdr.hdr_len = 0;
+
err = -EMSGSIZE;
if (!gso_type && (len > dev->mtu + reserve + VLAN_HLEN))
--
1.7.5.rc3.dirty
^ permalink raw reply related
* Re: pull request: sfc-2.6 2011-05-12
From: David Miller @ 2011-05-12 20:30 UTC (permalink / raw)
To: bhutchings; +Cc: netdev, linux-net-drivers
In-Reply-To: <1305216428.5214.13.camel@bwh-desktop>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Thu, 12 May 2011 17:07:08 +0100
> The following changes since commit ff538818f4a82c4cf02d2d6bd6ac5c7360b9d41d:
>
> sysctl: net: call unregister_net_sysctl_table where needed (2011-05-02 16:12:14 -0700)
>
> are available in the git repository at:
> git://git.kernel.org/pub/scm/linux/kernel/git/bwh/sfc-2.6.git sfc-2.6.39
>
> Ben Hutchings (1):
> sfc: Always map MCDI shared memory as uncacheable
>
> Sorry for sending this so late. I was really hoping to get to the
> bottom of this issue and find a simple fix.
Pulled, thanks Ben.
^ permalink raw reply
* [PATCH 2/2] sctp: sctp_sendmsg: Don't test known non-null sinfo
From: Joe Perches @ 2011-05-12 19:19 UTC (permalink / raw)
To: Vlad Yasevich, Sridhar Samudrala
Cc: David S. Miller, linux-sctp, netdev, linux-kernel
In-Reply-To: <cover.1305227620.git.joe@perches.com>
It's already known non-null above.
Signed-off-by: Joe Perches <joe@perches.com>
---
net/sctp/socket.c | 10 ++++------
1 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index a5d3560..10ebcd7 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -1793,12 +1793,10 @@ SCTP_STATIC int sctp_sendmsg(struct kiocb *iocb, struct sock *sk,
goto out_free;
}
- if (sinfo) {
- /* Check for invalid stream. */
- if (sinfo->sinfo_stream >= asoc->c.sinit_num_ostreams) {
- err = -EINVAL;
- goto out_free;
- }
+ /* Check for invalid stream. */
+ if (sinfo->sinfo_stream >= asoc->c.sinit_num_ostreams) {
+ err = -EINVAL;
+ goto out_free;
}
timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
--
1.7.5.rc3.dirty
^ permalink raw reply related
* [PATCH 1/2] sctp: sctp_sendmsg: Don't initialize default_sinfo
From: Joe Perches @ 2011-05-12 19:19 UTC (permalink / raw)
To: Vlad Yasevich, Sridhar Samudrala
Cc: David S. Miller, linux-sctp, netdev, linux-kernel
In-Reply-To: <cover.1305227620.git.joe@perches.com>
This variable only needs initialization when cmsgs.info
is NULL.
Don't use memset, just initialize every struct member.
Signed-off-by: Joe Perches <joe@perches.com>
---
net/sctp/socket.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 33d9ee6..a5d3560 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -1496,8 +1496,8 @@ SCTP_STATIC int sctp_sendmsg(struct kiocb *iocb, struct sock *sk,
struct sctp_chunk *chunk;
union sctp_addr to;
struct sockaddr *msg_name = NULL;
- struct sctp_sndrcvinfo default_sinfo = { 0 };
struct sctp_sndrcvinfo *sinfo;
+ struct sctp_sndrcvinfo default_sinfo;
struct sctp_initmsg *sinit;
sctp_assoc_t associd = 0;
sctp_cmsgs_t cmsgs = { NULL };
@@ -1761,10 +1761,13 @@ SCTP_STATIC int sctp_sendmsg(struct kiocb *iocb, struct sock *sk,
* some defaults.
*/
default_sinfo.sinfo_stream = asoc->default_stream;
+ default_sinfo.sinfo_ssn = 0;
default_sinfo.sinfo_flags = asoc->default_flags;
default_sinfo.sinfo_ppid = asoc->default_ppid;
default_sinfo.sinfo_context = asoc->default_context;
default_sinfo.sinfo_timetolive = asoc->default_timetolive;
+ default_sinfo.sinfo_tsn = 0;
+ default_sinfo.sinfo_cumtsn = 0;
default_sinfo.sinfo_assoc_id = sctp_assoc2id(asoc);
sinfo = &default_sinfo;
}
--
1.7.5.rc3.dirty
^ permalink raw reply related
* [PATCH 0/2] sctp: socket cleanups
From: Joe Perches @ 2011-05-12 19:19 UTC (permalink / raw)
To: linux-sctp; +Cc: netdev, linux-kernel
Size trivially reduced as well.
$ size net/sctp/socket.o
text data bss dec hex filename
55720 442 14988 71150 115ee net/sctp/socket.o.new
55730 442 14996 71168 11600 net/sctp/socket.o.old
Joe Perches (2):
sctp: sctp_sendmsg: Don't initialize default_sinfo
sctp: sctp_sendmsg: Don't test known non-null sinfo
net/sctp/socket.c | 15 ++++++++-------
1 files changed, 8 insertions(+), 7 deletions(-)
--
1.7.5.rc3.dirty
^ permalink raw reply
* Re: [PATCHv3 net-next-2.6 2/3] qlcnic: Take FW dump via ethtool
From: Ben Hutchings @ 2011-05-12 19:18 UTC (permalink / raw)
To: Anirban Chakraborty; +Cc: netdev, David Miller
In-Reply-To: <F9877D83-226B-4C7A-AD01-E9AA441AD1CD@qlogic.com>
On Thu, 2011-05-12 at 11:53 -0700, Anirban Chakraborty wrote:
> On May 12, 2011, at 10:27 AM, Ben Hutchings wrote:
>
> > On Wed, 2011-05-11 at 15:54 -0700, Anirban Chakraborty wrote:
> >> Driver checks if the previous dump has been cleared before taking the dump.
> >> It doesn't take the dump if it is not cleared.
> >>
> >> Changes from v2:
> >> Added lock to protect dump data structures from being mangled while
> >> dumping or setting them via ethtool.
> >
> > Unfortunately it still seems to be possible for the dump length to
> > change between the ethtool core calling qlcnic_get_dump_flag() and
> > qlcnic_get_dump_data().
>
> dump length is serialized via the driver lock. dump length is a static
> entity for a given capture mask and it can only be changed when there
> is a different capture mask set in the driver (via calling set_dump()
> from ethtool core).
OK.
> Actual dump size is determined during the initial steps of FW dump
> which takes the driver lock to start with. So, I am not sure how the
> dump length could be changed between the calls to get_dump_flag and
> get_dump_data from within the ethtool core without a call to
> set_dump() in between.
[...]
What prevents this sequence:
1. Driver detects firmware dump is required, and creates the dump
(length L1).
2. User changes firmware dump flags through ethtool.
3. User starts to save firmware dump through ethtool:
a. ethtool utility reads dump length (= L1) and allocates user buffer
b. ethtool utility reads dump:
c. ethtool core reads dump length (L1) and allocates kernel buffer
4. Meanwhile, driver detects firmware dump is required again, and
creates a new dump (length L2, since dump flags changed)
5. (Continuation of 3)
d. ethtool core calls driver to read firmware dump
e. Driver copies new dump (length L2) into buffer of length L1
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: AAARGH bisection is hard (Re: [2.6.39 regression] X locks up hard right after logging in)
From: Linus Torvalds @ 2011-05-12 19:17 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Andrew Lutomirski, linux-kernel, netdev, git
In-Reply-To: <4DCC2CFD.4010807@kdbg.org>
On Thu, May 12, 2011 at 11:54 AM, Johannes Sixt <j6t@kdbg.org> wrote:
>
> Except that you should git reset --hard; git bisect reset gets you out
> of bisect-mode, no?
Yeah, sorry, my bad.
Linus
^ permalink raw reply
* Re: [PATCH 3/3] ne-h8300: Fix regression caused during net_device_ops conversion
From: Geert Uytterhoeven @ 2011-05-12 19:16 UTC (permalink / raw)
To: David S. Miller, Stephen Hemminger, Yoshinori Sato
Cc: Russell King, Finn Thain, netdev, linux-kernel, linux-m68k,
Geert Uytterhoeven, stable
In-Reply-To: <1305227500-15595-4-git-send-email-geert@linux-m68k.org>
On Thu, May 12, 2011 at 21:11, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> From: Geert Uytterhoeven <geert@chicken.sonytel.be>
Sorry, the above line is bogus. Please remove it.
> Changeset dcd39c90290297f6e6ed8a04bb20da7ac2b043c5 ("ne-h8300: convert to
> net_device_ops") broke ne-h8300 by adding 8390.o to the link. That
> meant that lib8390.c was included twice, once in ne-h8300.c and once in
> 8390.c, subject to different macros. This patch reverts that by
> avoiding the wrappers in 8390.c.
>
> Fix based on commits 217cbfa856dc1cbc2890781626c4032d9e3ec59f ("mac8390:
> fix regression caused during net_device_ops conversion") and
> 4e0168fa4842e27795a75b205a510f25b62181d9 ("mac8390: fix build with
> NET_POLL_CONTROLLER").
>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: stable@kernel.org
> ---
> drivers/net/Makefile | 2 +-
> drivers/net/ne-h8300.c | 16 ++++++++--------
> 2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> index 4d2f094..e5a7375 100644
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -144,7 +144,7 @@ obj-$(CONFIG_NE3210) += ne3210.o 8390.o
> obj-$(CONFIG_SB1250_MAC) += sb1250-mac.o
> obj-$(CONFIG_B44) += b44.o
> obj-$(CONFIG_FORCEDETH) += forcedeth.o
> -obj-$(CONFIG_NE_H8300) += ne-h8300.o 8390.o
> +obj-$(CONFIG_NE_H8300) += ne-h8300.o
> obj-$(CONFIG_AX88796) += ax88796.o
> obj-$(CONFIG_BCM63XX_ENET) += bcm63xx_enet.o
> obj-$(CONFIG_FTMAC100) += ftmac100.o
> diff --git a/drivers/net/ne-h8300.c b/drivers/net/ne-h8300.c
> index 30be8c6..7298a34 100644
> --- a/drivers/net/ne-h8300.c
> +++ b/drivers/net/ne-h8300.c
> @@ -167,7 +167,7 @@ static void cleanup_card(struct net_device *dev)
> #ifndef MODULE
> struct net_device * __init ne_probe(int unit)
> {
> - struct net_device *dev = alloc_ei_netdev();
> + struct net_device *dev = ____alloc_ei_netdev(0);
> int err;
>
> if (!dev)
> @@ -197,15 +197,15 @@ static const struct net_device_ops ne_netdev_ops = {
> .ndo_open = ne_open,
> .ndo_stop = ne_close,
>
> - .ndo_start_xmit = ei_start_xmit,
> - .ndo_tx_timeout = ei_tx_timeout,
> - .ndo_get_stats = ei_get_stats,
> - .ndo_set_multicast_list = ei_set_multicast_list,
> + .ndo_start_xmit = __ei_start_xmit,
> + .ndo_tx_timeout = __ei_tx_timeout,
> + .ndo_get_stats = __ei_get_stats,
> + .ndo_set_multicast_list = __ei_set_multicast_list,
> .ndo_validate_addr = eth_validate_addr,
> - .ndo_set_mac_address = eth_mac_addr,
> + .ndo_set_mac_address = eth_mac_addr,
> .ndo_change_mtu = eth_change_mtu,
> #ifdef CONFIG_NET_POLL_CONTROLLER
> - .ndo_poll_controller = ei_poll,
> + .ndo_poll_controller = __ei_poll,
> #endif
> };
>
> @@ -637,7 +637,7 @@ int init_module(void)
> int err;
>
> for (this_dev = 0; this_dev < MAX_NE_CARDS; this_dev++) {
> - struct net_device *dev = alloc_ei_netdev();
> + struct net_device *dev = ____alloc_ei_netdev(0);
> if (!dev)
> break;
> if (io[this_dev]) {
> --
> 1.7.0.4
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH 2/3] hydra: Fix regression caused during net_device_ops conversion
From: Geert Uytterhoeven @ 2011-05-12 19:15 UTC (permalink / raw)
To: David S. Miller, Stephen Hemminger, Yoshinori Sato
Cc: Russell King, Finn Thain, netdev, linux-kernel, linux-m68k,
Geert Uytterhoeven, Geert Uytterhoeven, stable
In-Reply-To: <1305227500-15595-3-git-send-email-geert@linux-m68k.org>
On Thu, May 12, 2011 at 21:11, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> From: Geert Uytterhoeven <geert@chicken.sonytel.be>
Sorry, the above line is bogus. Please remove it.
> Changeset 5618f0d1193d6b051da9b59b0e32ad24397f06a4 ("hydra: convert to
> net_device_ops") broke hydra by adding 8390.o to the link. That
> meant that lib8390.c was included twice, once in hydra.c and once in
> 8390.c, subject to different macros. This patch reverts that by
> avoiding the wrappers in 8390.c.
>
> Fix based on commits 217cbfa856dc1cbc2890781626c4032d9e3ec59f ("mac8390:
> fix regression caused during net_device_ops conversion") and
> 4e0168fa4842e27795a75b205a510f25b62181d9 ("mac8390: fix build with
> NET_POLL_CONTROLLER").
>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: stable@kernel.org
> ---
> drivers/net/Makefile | 2 +-
> drivers/net/hydra.c | 14 +++++++-------
> 2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> index c64675f..4d2f094 100644
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -231,7 +231,7 @@ obj-$(CONFIG_SGI_IOC3_ETH) += ioc3-eth.o
> obj-$(CONFIG_DECLANCE) += declance.o
> obj-$(CONFIG_ATARILANCE) += atarilance.o
> obj-$(CONFIG_A2065) += a2065.o
> -obj-$(CONFIG_HYDRA) += hydra.o 8390.o
> +obj-$(CONFIG_HYDRA) += hydra.o
> obj-$(CONFIG_ARIADNE) += ariadne.o
> obj-$(CONFIG_CS89x0) += cs89x0.o
> obj-$(CONFIG_MACSONIC) += macsonic.o
> diff --git a/drivers/net/hydra.c b/drivers/net/hydra.c
> index c5ef62c..1cd481c 100644
> --- a/drivers/net/hydra.c
> +++ b/drivers/net/hydra.c
> @@ -98,15 +98,15 @@ static const struct net_device_ops hydra_netdev_ops = {
> .ndo_open = hydra_open,
> .ndo_stop = hydra_close,
>
> - .ndo_start_xmit = ei_start_xmit,
> - .ndo_tx_timeout = ei_tx_timeout,
> - .ndo_get_stats = ei_get_stats,
> - .ndo_set_multicast_list = ei_set_multicast_list,
> + .ndo_start_xmit = __ei_start_xmit,
> + .ndo_tx_timeout = __ei_tx_timeout,
> + .ndo_get_stats = __ei_get_stats,
> + .ndo_set_multicast_list = __ei_set_multicast_list,
> .ndo_validate_addr = eth_validate_addr,
> - .ndo_set_mac_address = eth_mac_addr,
> + .ndo_set_mac_address = eth_mac_addr,
> .ndo_change_mtu = eth_change_mtu,
> #ifdef CONFIG_NET_POLL_CONTROLLER
> - .ndo_poll_controller = ei_poll,
> + .ndo_poll_controller = __ei_poll,
> #endif
> };
>
> @@ -125,7 +125,7 @@ static int __devinit hydra_init(struct zorro_dev *z)
> 0x10, 0x12, 0x14, 0x16, 0x18, 0x1a, 0x1c, 0x1e,
> };
>
> - dev = alloc_ei_netdev();
> + dev = ____alloc_ei_netdev(0);
> if (!dev)
> return -ENOMEM;
>
> --
> 1.7.0.4
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH 3/3] ne-h8300: Fix regression caused during net_device_ops conversion
From: Geert Uytterhoeven @ 2011-05-12 19:11 UTC (permalink / raw)
To: David S. Miller, Stephen Hemminger, Yoshinori Sato
Cc: linux-m68k, Russell King, netdev, linux-kernel, Finn Thain,
stable, Geert Uytterhoeven, Geert Uytterhoeven
In-Reply-To: <1305227500-15595-1-git-send-email-geert@linux-m68k.org>
From: Geert Uytterhoeven <geert@chicken.sonytel.be>
Changeset dcd39c90290297f6e6ed8a04bb20da7ac2b043c5 ("ne-h8300: convert to
net_device_ops") broke ne-h8300 by adding 8390.o to the link. That
meant that lib8390.c was included twice, once in ne-h8300.c and once in
8390.c, subject to different macros. This patch reverts that by
avoiding the wrappers in 8390.c.
Fix based on commits 217cbfa856dc1cbc2890781626c4032d9e3ec59f ("mac8390:
fix regression caused during net_device_ops conversion") and
4e0168fa4842e27795a75b205a510f25b62181d9 ("mac8390: fix build with
NET_POLL_CONTROLLER").
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: stable@kernel.org
---
drivers/net/Makefile | 2 +-
drivers/net/ne-h8300.c | 16 ++++++++--------
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index 4d2f094..e5a7375 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -144,7 +144,7 @@ obj-$(CONFIG_NE3210) += ne3210.o 8390.o
obj-$(CONFIG_SB1250_MAC) += sb1250-mac.o
obj-$(CONFIG_B44) += b44.o
obj-$(CONFIG_FORCEDETH) += forcedeth.o
-obj-$(CONFIG_NE_H8300) += ne-h8300.o 8390.o
+obj-$(CONFIG_NE_H8300) += ne-h8300.o
obj-$(CONFIG_AX88796) += ax88796.o
obj-$(CONFIG_BCM63XX_ENET) += bcm63xx_enet.o
obj-$(CONFIG_FTMAC100) += ftmac100.o
diff --git a/drivers/net/ne-h8300.c b/drivers/net/ne-h8300.c
index 30be8c6..7298a34 100644
--- a/drivers/net/ne-h8300.c
+++ b/drivers/net/ne-h8300.c
@@ -167,7 +167,7 @@ static void cleanup_card(struct net_device *dev)
#ifndef MODULE
struct net_device * __init ne_probe(int unit)
{
- struct net_device *dev = alloc_ei_netdev();
+ struct net_device *dev = ____alloc_ei_netdev(0);
int err;
if (!dev)
@@ -197,15 +197,15 @@ static const struct net_device_ops ne_netdev_ops = {
.ndo_open = ne_open,
.ndo_stop = ne_close,
- .ndo_start_xmit = ei_start_xmit,
- .ndo_tx_timeout = ei_tx_timeout,
- .ndo_get_stats = ei_get_stats,
- .ndo_set_multicast_list = ei_set_multicast_list,
+ .ndo_start_xmit = __ei_start_xmit,
+ .ndo_tx_timeout = __ei_tx_timeout,
+ .ndo_get_stats = __ei_get_stats,
+ .ndo_set_multicast_list = __ei_set_multicast_list,
.ndo_validate_addr = eth_validate_addr,
- .ndo_set_mac_address = eth_mac_addr,
+ .ndo_set_mac_address = eth_mac_addr,
.ndo_change_mtu = eth_change_mtu,
#ifdef CONFIG_NET_POLL_CONTROLLER
- .ndo_poll_controller = ei_poll,
+ .ndo_poll_controller = __ei_poll,
#endif
};
@@ -637,7 +637,7 @@ int init_module(void)
int err;
for (this_dev = 0; this_dev < MAX_NE_CARDS; this_dev++) {
- struct net_device *dev = alloc_ei_netdev();
+ struct net_device *dev = ____alloc_ei_netdev(0);
if (!dev)
break;
if (io[this_dev]) {
--
1.7.0.4
_______________________________________________
stable mailing list
stable@linux.kernel.org
http://linux.kernel.org/mailman/listinfo/stable
^ permalink raw reply related
* [PATCH 2/3] hydra: Fix regression caused during net_device_ops conversion
From: Geert Uytterhoeven @ 2011-05-12 19:11 UTC (permalink / raw)
To: David S. Miller, Stephen Hemminger, Yoshinori Sato
Cc: linux-m68k, Russell King, netdev, linux-kernel, Finn Thain,
stable, Geert Uytterhoeven, Geert Uytterhoeven
In-Reply-To: <1305227500-15595-1-git-send-email-geert@linux-m68k.org>
From: Geert Uytterhoeven <geert@chicken.sonytel.be>
Changeset 5618f0d1193d6b051da9b59b0e32ad24397f06a4 ("hydra: convert to
net_device_ops") broke hydra by adding 8390.o to the link. That
meant that lib8390.c was included twice, once in hydra.c and once in
8390.c, subject to different macros. This patch reverts that by
avoiding the wrappers in 8390.c.
Fix based on commits 217cbfa856dc1cbc2890781626c4032d9e3ec59f ("mac8390:
fix regression caused during net_device_ops conversion") and
4e0168fa4842e27795a75b205a510f25b62181d9 ("mac8390: fix build with
NET_POLL_CONTROLLER").
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: stable@kernel.org
---
drivers/net/Makefile | 2 +-
drivers/net/hydra.c | 14 +++++++-------
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index c64675f..4d2f094 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -231,7 +231,7 @@ obj-$(CONFIG_SGI_IOC3_ETH) += ioc3-eth.o
obj-$(CONFIG_DECLANCE) += declance.o
obj-$(CONFIG_ATARILANCE) += atarilance.o
obj-$(CONFIG_A2065) += a2065.o
-obj-$(CONFIG_HYDRA) += hydra.o 8390.o
+obj-$(CONFIG_HYDRA) += hydra.o
obj-$(CONFIG_ARIADNE) += ariadne.o
obj-$(CONFIG_CS89x0) += cs89x0.o
obj-$(CONFIG_MACSONIC) += macsonic.o
diff --git a/drivers/net/hydra.c b/drivers/net/hydra.c
index c5ef62c..1cd481c 100644
--- a/drivers/net/hydra.c
+++ b/drivers/net/hydra.c
@@ -98,15 +98,15 @@ static const struct net_device_ops hydra_netdev_ops = {
.ndo_open = hydra_open,
.ndo_stop = hydra_close,
- .ndo_start_xmit = ei_start_xmit,
- .ndo_tx_timeout = ei_tx_timeout,
- .ndo_get_stats = ei_get_stats,
- .ndo_set_multicast_list = ei_set_multicast_list,
+ .ndo_start_xmit = __ei_start_xmit,
+ .ndo_tx_timeout = __ei_tx_timeout,
+ .ndo_get_stats = __ei_get_stats,
+ .ndo_set_multicast_list = __ei_set_multicast_list,
.ndo_validate_addr = eth_validate_addr,
- .ndo_set_mac_address = eth_mac_addr,
+ .ndo_set_mac_address = eth_mac_addr,
.ndo_change_mtu = eth_change_mtu,
#ifdef CONFIG_NET_POLL_CONTROLLER
- .ndo_poll_controller = ei_poll,
+ .ndo_poll_controller = __ei_poll,
#endif
};
@@ -125,7 +125,7 @@ static int __devinit hydra_init(struct zorro_dev *z)
0x10, 0x12, 0x14, 0x16, 0x18, 0x1a, 0x1c, 0x1e,
};
- dev = alloc_ei_netdev();
+ dev = ____alloc_ei_netdev(0);
if (!dev)
return -ENOMEM;
--
1.7.0.4
_______________________________________________
stable mailing list
stable@linux.kernel.org
http://linux.kernel.org/mailman/listinfo/stable
^ permalink raw reply related
* [PATCH 1/3] zorro8390: Fix regression caused during net_device_ops conversion
From: Geert Uytterhoeven @ 2011-05-12 19:11 UTC (permalink / raw)
To: David S. Miller, Stephen Hemminger, Yoshinori Sato
Cc: linux-m68k, Russell King, netdev, linux-kernel, Finn Thain,
Geert Uytterhoeven, stable
In-Reply-To: <1305227500-15595-1-git-send-email-geert@linux-m68k.org>
Changeset b6114794a1c394534659f4a17420e48cf23aa922 ("zorro8390: convert to
net_device_ops") broke zorro8390 by adding 8390.o to the link. That
meant that lib8390.c was included twice, once in zorro8390.c and once in
8390.c, subject to different macros. This patch reverts that by
avoiding the wrappers in 8390.c.
Fix based on commits 217cbfa856dc1cbc2890781626c4032d9e3ec59f ("mac8390:
fix regression caused during net_device_ops conversion") and
4e0168fa4842e27795a75b205a510f25b62181d9 ("mac8390: fix build with
NET_POLL_CONTROLLER").
Reported-by: Christian T. Steigies <cts@debian.org>
Suggested-by: Finn Thain <fthain@telegraphics.com.au>
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
Tested-by: Christian T. Steigies <cts@debian.org>
Cc: stable@kernel.org
---
drivers/net/Makefile | 2 +-
drivers/net/zorro8390.c | 12 ++++++------
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index 01b604a..c64675f 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -219,7 +219,7 @@ obj-$(CONFIG_SC92031) += sc92031.o
obj-$(CONFIG_LP486E) += lp486e.o
obj-$(CONFIG_ETH16I) += eth16i.o
-obj-$(CONFIG_ZORRO8390) += zorro8390.o 8390.o
+obj-$(CONFIG_ZORRO8390) += zorro8390.o
obj-$(CONFIG_HPLANCE) += hplance.o 7990.o
obj-$(CONFIG_MVME147_NET) += mvme147.o 7990.o
obj-$(CONFIG_EQUALIZER) += eql.o
diff --git a/drivers/net/zorro8390.c b/drivers/net/zorro8390.c
index b78a38d..8c7c522 100644
--- a/drivers/net/zorro8390.c
+++ b/drivers/net/zorro8390.c
@@ -126,7 +126,7 @@ static int __devinit zorro8390_init_one(struct zorro_dev *z,
board = z->resource.start;
ioaddr = board+cards[i].offset;
- dev = alloc_ei_netdev();
+ dev = ____alloc_ei_netdev(0);
if (!dev)
return -ENOMEM;
if (!request_mem_region(ioaddr, NE_IO_EXTENT*2, DRV_NAME)) {
@@ -146,15 +146,15 @@ static int __devinit zorro8390_init_one(struct zorro_dev *z,
static const struct net_device_ops zorro8390_netdev_ops = {
.ndo_open = zorro8390_open,
.ndo_stop = zorro8390_close,
- .ndo_start_xmit = ei_start_xmit,
- .ndo_tx_timeout = ei_tx_timeout,
- .ndo_get_stats = ei_get_stats,
- .ndo_set_multicast_list = ei_set_multicast_list,
+ .ndo_start_xmit = __ei_start_xmit,
+ .ndo_tx_timeout = __ei_tx_timeout,
+ .ndo_get_stats = __ei_get_stats,
+ .ndo_set_multicast_list = __ei_set_multicast_list,
.ndo_validate_addr = eth_validate_addr,
.ndo_set_mac_address = eth_mac_addr,
.ndo_change_mtu = eth_change_mtu,
#ifdef CONFIG_NET_POLL_CONTROLLER
- .ndo_poll_controller = ei_poll,
+ .ndo_poll_controller = __ei_poll,
#endif
};
--
1.7.0.4
_______________________________________________
stable mailing list
stable@linux.kernel.org
http://linux.kernel.org/mailman/listinfo/stable
^ permalink raw reply related
* [PATCH 0/3] Fix 8390 regressions
From: Geert Uytterhoeven @ 2011-05-12 19:11 UTC (permalink / raw)
To: David S. Miller, Stephen Hemminger, Yoshinori Sato
Cc: Russell King, Finn Thain, netdev, linux-kernel, linux-m68k
These patches fix regressions in 3 8390-based network drivers:
[1/3] zorro8390: Fix regression caused during net_device_ops conversion
[2/3] hydra: Fix regression caused during net_device_ops conversion
[3/3] ne-h8300: Fix regression caused during net_device_ops conversion
The first one, for zorro8390, has been tested.
The second one, for hydra, has been compile-tested only.
Based on commits 8cfd9e923be54ef66ce174a93f4592b444b96407 ("[ARM] RiscPC: Fix
etherh oops") and 217cbfa856dc1cbc2890781626c4032d9e3ec59f ("mac8390: fix
regression caused during net_device_ops conversion"), and the patch for
zorro8390, we have good reasons to believe hydra and ne-h8300 are affected
as well, as they all include lib8390.c.
Hence patches for those are also included, although I could not test them.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: AAARGH bisection is hard (Re: [2.6.39 regression] X locks up hard right after logging in)
From: Johannes Sixt @ 2011-05-12 18:54 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Andrew Lutomirski, linux-kernel, netdev, git
In-Reply-To: <BANLkTi=YDZa+BRaG90vJsjrT9VxgySrDRQ@mail.gmail.com>
Am 12.05.2011 19:37, schrieb Linus Torvalds:
> If you think it's networking, for example, and you've bisected into
> there but aren't sure, do "gitk --bisect", find the point where I
> merge, and pick that (and my parent), and "git bisect reset" those
> points.
Except that you should git reset --hard; git bisect reset gets you out
of bisect-mode, no?
-- Hannes
^ permalink raw reply
* Re: [PATCHv3 net-next-2.6 2/3] qlcnic: Take FW dump via ethtool
From: Anirban Chakraborty @ 2011-05-12 18:53 UTC (permalink / raw)
To: Ben Hutchings; +Cc: netdev, David Miller
In-Reply-To: <1305221242.5214.36.camel@bwh-desktop>
On May 12, 2011, at 10:27 AM, Ben Hutchings wrote:
> On Wed, 2011-05-11 at 15:54 -0700, Anirban Chakraborty wrote:
>> Driver checks if the previous dump has been cleared before taking the dump.
>> It doesn't take the dump if it is not cleared.
>>
>> Changes from v2:
>> Added lock to protect dump data structures from being mangled while
>> dumping or setting them via ethtool.
>
> Unfortunately it still seems to be possible for the dump length to
> change between the ethtool core calling qlcnic_get_dump_flag() and
> qlcnic_get_dump_data().
dump length is serialized via the driver lock. dump length is a static entity for a given capture mask
and it can only be changed when there is a different capture mask set in the driver (via calling
set_dump() from ethtool core).
Actual dump size is determined during the initial steps of FW dump which takes the driver lock
to start with. So, I am not sure how the dump length could be changed between the calls to
get_dump_flag and get_dump_data from within the ethtool core without a call to set_dump()
in between.
>
> So I think qlcnic_get_dump_data() will need to double-check the length
> after taking the internal lock:
>
> [...]
>> +static int
>> +qlcnic_get_dump_data(struct net_device *netdev, struct ethtool_dump *dump,
>> + void *buffer)
>> +{
>> + int i, copy_sz;
>> + u32 *hdr_ptr, *data;
>> + struct qlcnic_adapter *adapter = netdev_priv(netdev);
>> + struct qlcnic_fw_dump *fw_dump = &adapter->ahw->fw_dump;
>> +
>> + if (qlcnic_api_lock(adapter))
>> + return -EIO;
> [...]
>
> if (dump->len < fw_dump->tmpl_hdr->size + fw_dump->size) {
> qlcnic_api_unlock(adapter);
> return -EINVAL;
> }
>
> I'm not sure about the error code... and I'm really not happy about the
> need to check lengths in both the ethtool core and the driver.
I can put the check in here but don't think it is required really.
>
> Can't you change the function that actually makes a dump to acquire the
> RTNL lock? (You'll need to do that *before* acquiring the driver's own
> lock.)
We can't do that because the driver lock is taken at much higher level where it does
some hardware specific things even before attempting to take FW dump.
Thanks.
-Anirban
^ 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