Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH] net/ipv6/exthdrs.c et al: Optional strict PadN option checking
From: David Miller @ 2012-04-12 21:35 UTC (permalink / raw)
  To: eldad; +Cc: kuznet, jmorris, yoshfuji, kaber, netdev, linux-kernel
In-Reply-To: <CABe5r8JJPuhq3r=JnfLA=_U46-SFmK+g3XqC3MaRdLxwVXx9Qw@mail.gmail.com>

From: Eldad Zack <eldad@fogrefinery.com>
Date: Thu, 12 Apr 2012 23:31:47 +0200

> That's the way I see it, as was my initial intent. Then I got
> concerned with the possibility that a communication with
> slightly-broken stack implementation (e.g., unsanitized buffers) would
> fail without the user being able to control it at runtime.
> Do you consider this a non-issue?

I think it's a non-issue.

^ permalink raw reply

* [PATCH] ks8851: Fix missing mutex_lock/unlock
From: mjr @ 2012-04-12 21:28 UTC (permalink / raw)
  To: fbl; +Cc: davem, sboyd, ben, netdev, Matt Renzelmann

From: Matt Renzelmann <mjr@cs.wisc.edu>

All calls to ks8851_rdreg* and ks8851_wrreg* should be protected with
the driver's lock mutex.  A spurious interrupt may otherwise cause a
crash.  Thanks to Stephen Boyd and Flavio Leitner for feedback.

Signed-off-by: Matt Renzelmann <mjr@cs.wisc.edu>
---

Here's the next revision.  This is basically Flavio Leitner's latest
version without the mutex.  Please let me know if I'm missing
anything and I can re-submit it.

 drivers/net/ethernet/micrel/ks8851.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index c722aa6..6528d66 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -1418,6 +1418,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
 	struct net_device *ndev;
 	struct ks8851_net *ks;
 	int ret;
+	unsigned cider;
 
 	ndev = alloc_etherdev(sizeof(struct ks8851_net));
 	if (!ndev)
@@ -1484,8 +1485,8 @@ static int __devinit ks8851_probe(struct spi_device *spi)
 	ks8851_soft_reset(ks, GRR_GSR);
 
 	/* simple check for a valid chip being connected to the bus */
-
-	if ((ks8851_rdreg16(ks, KS_CIDER) & ~CIDER_REV_MASK) != CIDER_ID) {
+	cider = CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER));
+	if ((cider & ~CIDER_REV_MASK) != CIDER_ID) {
 		dev_err(&spi->dev, "failed to read device ID\n");
 		ret = -ENODEV;
 		goto err_id;
@@ -1516,8 +1517,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
 	}
 
 	netdev_info(ndev, "revision %d, MAC %pM, IRQ %d, %s EEPROM\n",
-		    CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER)),
-		    ndev->dev_addr, ndev->irq,
+		    CIDER_REV_GET(cider), ndev->dev_addr, ndev->irq,
 		    ks->rc_ccr & CCR_EEPROM ? "has" : "no");
 
 	return 0;
-- 
1.7.5.4

^ permalink raw reply related

* [PATCH] net/ipv6/exthdrs.c: Strict PadN option checking
From: Eldad Zack @ 2012-04-12 21:31 UTC (permalink / raw)
  To: David S. Miller, Alexey Kuznetsov, James Morris,
	Hideaki YOSHIFUJI, Patrick McHardy
  Cc: netdev, linux-kernel, Eldad Zack

Added strict checking of PadN, as PadN can be used to increase header
size and thus push the protocol header into the 2nd fragment.

PadN is used to align the options within the Hop-by-Hop or
Destination Options header to 64-bit boundaries. The maximum valid
size is thus 7 bytes.
RFC 4942 recommends to actively check the "payload" itself and
ensure that it contains only zeroes.

See also RFC 4942 section 2.1.9.5.

Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
 net/ipv6/exthdrs.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index c486b8e..63f6e87 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -153,6 +153,7 @@ static int ip6_parse_tlv(struct tlvtype_proc *procs, struct sk_buff *skb)
 
 	while (len > 0) {
 		int optlen = nh[off + 1] + 2;
+		int i;
 
 		switch (nh[off]) {
 		case IPV6_TLV_PAD0:
@@ -160,6 +161,19 @@ static int ip6_parse_tlv(struct tlvtype_proc *procs, struct sk_buff *skb)
 			break;
 
 		case IPV6_TLV_PADN:
+			/* RFC 2460 states that the purpose of PadN is
+			   to align the containing header to multiples
+			   of 8. 7 is therefore the highest valid value.
+			   See also RFC 4942, Section 2.1.9.5.*/
+			if (optlen > 7)
+				goto bad;
+			/* RFC 4942 recommends receiving hosts to
+			   actively check PadN payload to contain
+			   only zeroes. */
+			for (i = 2; i < optlen; i++) {
+				if (nh[off + i] != 0)
+					goto bad;
+			}
 			break;
 
 		default: /* Other TLV code so scan list */
-- 
1.7.9.5

^ permalink raw reply related

* Re: [PATCH] net/ipv6/exthdrs.c et al: Optional strict PadN option checking
From: Eldad Zack @ 2012-04-12 21:31 UTC (permalink / raw)
  To: David Miller; +Cc: kuznet, jmorris, yoshfuji, kaber, netdev, linux-kernel
In-Reply-To: <20120412.160040.1086391259430365718.davem@davemloft.net>

On 12 April 2012 22:00, David Miller <davem@davemloft.net> wrote:
>> Added strict checking of PadN. PadN can be used to increase header
>> size and thus push the protocol header into the 2nd fragment.
>>
>> PadN is used to align the options within the Hop-by-Hop or
>> Destination Options header to 64-bit boundaries. The maximum valid
>> size is thus 7 bytes.
>> RFC 4942 recommends to actively check the "payload" itself and
>> ensure that it contains only zeroes.
> I think you should do away with the sysctl and always perform these
> checks.
>
> At the very leat, the optlen > 7 check should always be performed.
> And frankly the pad byte being zero check makes sense to do all the
> time as far as I can tell too.

That's the way I see it, as was my initial intent. Then I got
concerned with the possibility that a communication with
slightly-broken stack implementation (e.g., unsanitized buffers) would
fail without the user being able to control it at runtime.
Do you consider this a non-issue?

If not, please apply the (soon to be sent) patch.

Thanks,
Eldad

^ permalink raw reply

* Re: linux-next: Tree for Apr 12
From: Andrew Morton @ 2012-04-12 21:24 UTC (permalink / raw)
  To: Stephen Rothwell; +Cc: linux-next, LKML, netdev
In-Reply-To: <20120412145931.9ec6f2b51655071795d60d1d@canb.auug.org.au>

On Thu, 12 Apr 2012 14:59:31 +1000
Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> I have created today's linux-next tree at
> git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git

This isn't working for me.  Some time between April 3 and April 12
someone merged something into the non-mm part of linux-next which broke
ssh.

I boot the box and everything seems to come up OK, but attemtps to ssh
into the machine fail with

X11 forwarding request failed on channel 0
Last login: Thu Apr 12 13:04:35 2012 from akpm.corp.google.com
Connection to akpm2 closed.

I took a peek in the `strace ssh' output.

Good:

17815 write(5, "Last login: Thu Apr 12 13:27:23 "..., 65) = 65
17815 select(7, [3 4], [], NULL, {120, 0}) = 1 (in [3], left {119, 770798})
17815 read(3, "\21O\200\366Mv\343\222\332\251\2403L\376Y18\2047\336\244\226p-+X\2%\2119\314\255"..., 8192) = 80
17815 select(7, [3 4], [5], NULL, {120, 0}) = 1 (out [5], left {119, 999987})
17815 write(5, "\r\33[m\17\33[27m\33[24m\33[Jakpm2:/home/ak"..., 39) = 39
17815 select(7, [3 4], [], NULL, {120, 0}) = 1 (in [4], left {118, 801111})
17815 read(4, "\4", 16384)              = 1
17815 select(7, [3 4], [3], NULL, {120, 0}) = 1 (out [3], left {119, 999991})
17815 write(3, "\235J\5\340\234\21\266\207\26e\362\327\2\332\1\267\272\200\364\267?/\320L\341\35\350{+M:\222"..., 48) = 48


Bad:

9305  write(5, "Last login: Thu Apr 12 13:02:54 "..., 65) = 65
9305  select(7, [3 4], [], NULL, {120, 0}) = 1 (in [3], left {119, 945541})
9305  read(3, "f\357\250~\260i\2259\320\3258\262)O\364;_\251\360-\314\31\374]\326\300\356\364\370S\3105"..., 8192) = 128
9305  close(5)                          = 0
9305  close(4)                          = 0

That read() is returning a lot more data.

It appears that we've done something which breaks X forwarding.  I
won't be able to look any further into this until Monday.

^ permalink raw reply

* Re: [PATCH 4/5] mac80211: Add more ethtools stats: survey, rates, etc
From: Ben Greear @ 2012-04-12 21:21 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: Florian Fainelli, linux-wireless, netdev
In-Reply-To: <1334264733.2497.35.camel@bwh-desktop.uk.solarflarecom.com>

On 04/12/2012 02:05 PM, Ben Hutchings wrote:
> On Thu, 2012-04-12 at 13:46 -0700, Ben Greear wrote:
>> On 04/12/2012 12:30 PM, Ben Hutchings wrote:
>>> On Thu, 2012-04-12 at 09:51 -0700, Ben Greear wrote:
>>>> On 04/12/2012 09:42 AM, Florian Fainelli wrote:
>>>>> Hi,
>>>>>
>>>>> Le 04/12/12 18:32, greearb@candelatech.com a écrit :
>>>>>> From: Ben Greear<greearb@candelatech.com>
>>>>>>
>>>>>> The signal and noise are forced to be positive since ethtool
>>>>>> deals in unsigned 64-bit values and this number should be human
>>>>>> readable. This gives easy access to some of the data formerly
>>>>>> exposed in the deprecated /proc/net/wireless file.
>>>>>
>>>>> Uh, that's misleading, the signal and noise values are typically negative, so one needs to think about mentally adding a minus sign if he/she wants to
>>>>> understand it. Does not ethtool know about 32-bits signed integers?
>>>>
>>>> Ethtool stats only supports u64.  I think it's easy enough for
>>>> humans or programs to add the negative sign.  Can signal or noise
>>>> ever be>   0?  If so, that could actually break something that depends
>>>> on flipping the value to negative....
>>>
>>> So far as I can see, the ethtool stats were expected to be counters,
>>> which obviously cannot become negative (or fractional).  Maybe it's time
>>> to define another command and string-set to cover other types of status
>>> information.  The ethtool utility could ask for those typed statistics
>>> as well, so 'ethtool -S' would get all of them.
>>
>> One nice thing about ethtool stats API is that it is backwards and forwards
>> compatible for a long while.
>
> Agreed.

> Actually it would be:
>
> foo:s32: 18446744073709551615
>
>> while a new one understand that the s32: prefix is special, do
>> some casting, and could show:
>> foo: -1
>>
>> Both are still at least somewhat human readable,
>
> I don't think many humans can mentally substract 2^64.

Well, if we add a new API, then anyone on older ethtool
won't see it at all, which is even more useless than a
large ugly number.

Those of us using ethtool API directly would have to add new
ioctl calls (and the performance is important to me, even
if atomicity isn't so important).  That is more work than
adding some logic to parse suffixes on the strings I think.

>> and probably wouldn't confuse anyone that is parsing the output of
>> existing ethtool output.
>
> I think you have this backwards: printing numbers in two different ways
> (old and new versions of ethtool) is likely to confuse scripts that are
> parsing and doing calculations with these numbers.  While I try to avoid
> gratuitous changes in output formatting, scripts should use the ethtool
> API if they really want interface stability.  It's not difficult (there
> are at least Python and Perl bindings available) and it's a lot more
> efficient.

If the new ethtool -S is going to nicely present things (ie, show "foo: -1"),
then the negative numbers are there anyway, so maybe the compatibility issue
for anyone parsing the output of 'ethtool -S' is moot.  Anyone parsing the
binary API sees no changes, but *could* update code to look at the suffix
if they cared.

That said, I *would* like a new 'ethool get-stats' API that took a 'verbose'
argument so that we could return more or less verbose results (dependent on the
driver to determine what that means).  That way, we could probe easy-to-obtain
information quickly and often, and if there is something more expensive to obtain,
that could be probed less often.  If this idea is worth pursuing, then perhaps
it could also include a new binary API that includes a type identifier.

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

^ permalink raw reply

* Re: [PATCH] ks8851: Fix missing mutex_lock/unlock
From: Flavio Leitner @ 2012-04-12 21:19 UTC (permalink / raw)
  To: Flavio Leitner; +Cc: Stephen Boyd, mjr, davem, ben, netdev
In-Reply-To: <20120412175945.1bbdd9bb@asterix.rh>

On Thu, 12 Apr 2012 17:59:45 -0300
Flavio Leitner <fbl@redhat.com> wrote:

> On Thu, 12 Apr 2012 13:34:32 -0700
> Stephen Boyd <sboyd@codeaurora.org> wrote:
> 
> > On 04/12/12 13:06, mjr@cs.wisc.edu wrote:
> > > From: Matt Renzelmann <mjr@cs.wisc.edu>
> > >
> > > All calls to ks8851_rdreg* and ks8851_wrreg* should be protected with
> > > the driver's lock mutex.  A spurious interrupt may otherwise cause a
> > > crash.
> > >
> > > Signed-off-by: Matt Renzelmann <mjr@cs.wisc.edu>
> > > ---
> > >
> > > Thank you, Mr. Leitner, for providing feedback.  I agree with your
> > > changes and have updated the patch to reflect them.  I apologize for
> > > missing the driver name in the title -- I've updated the patch with
> > > that information as well.  Please let me know if there is anything
> > > else I should fix/change.
> > >
> > >  drivers/net/ethernet/micrel/ks8851.c |    8 ++++++--
> > >  1 files changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
> > > index c722aa6..20237dc 100644
> > > --- a/drivers/net/ethernet/micrel/ks8851.c
> > > +++ b/drivers/net/ethernet/micrel/ks8851.c
> > > @@ -1417,6 +1417,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
> > >  {
> > >  	struct net_device *ndev;
> > >  	struct ks8851_net *ks;
> > > +	int result;
> > >  	int ret;
> > >  
> > >  	ndev = alloc_etherdev(sizeof(struct ks8851_net));
> > > @@ -1515,9 +1516,12 @@ static int __devinit ks8851_probe(struct spi_device *spi)
> > >  		goto err_netdev;
> > >  	}
> > >  
> > > +	mutex_lock(&ks->lock);
> > > +	result = CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER));
> > > +	mutex_unlock(&ks->lock);
> > > +
> > >  	netdev_info(ndev, "revision %d, MAC %pM, IRQ %d, %s EEPROM\n",
> > > -		    CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER)),
> > > -		    ndev->dev_addr, ndev->irq,
> > > +		    result, ndev->dev_addr, ndev->irq,
> > >  		    ks->rc_ccr & CCR_EEPROM ? "has" : "no");
> > >  
> > >  	return 0;
> > 
> > This register is already read in the probe function and the lock is not
> > held there so you seem to have missed a couple. I would guess it doesn't
> > really matter tha we don't grab the lock though because the device isn't
> > actively sending/receiving packets. How about this instead?
> 
> I believe that's because the IRQ isn't reserved yet, so there is no problem.

So, what about this instead:

diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index c722aa6..7137f47 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -1417,6 +1417,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
 {
 	struct net_device *ndev;
 	struct ks8851_net *ks;
+	unsigned int cider;
 	int ret;
 
 	ndev = alloc_etherdev(sizeof(struct ks8851_net));
@@ -1485,7 +1486,8 @@ static int __devinit ks8851_probe(struct spi_device *spi)
 
 	/* simple check for a valid chip being connected to the bus */
 
-	if ((ks8851_rdreg16(ks, KS_CIDER) & ~CIDER_REV_MASK) != CIDER_ID) {
+	cider = ks8851_rdreg16(ks, KS_CIDER);
+	if ((cider & ~CIDER_REV_MASK) != CIDER_ID) {
 		dev_err(&spi->dev, "failed to read device ID\n");
 		ret = -ENODEV;
 		goto err_id;
@@ -1516,7 +1518,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
 	}
 
 	netdev_info(ndev, "revision %d, MAC %pM, IRQ %d, %s EEPROM\n",
-		    CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER)),
+		    CIDER_REV_GET(cider),
 		    ndev->dev_addr, ndev->irq,
 		    ks->rc_ccr & CCR_EEPROM ? "has" : "no");
 

fbl

^ permalink raw reply related

* Re: [PATCH 4/5] mac80211: Add more ethtools stats: survey, rates, etc
From: Ben Hutchings @ 2012-04-12 21:05 UTC (permalink / raw)
  To: Ben Greear; +Cc: Florian Fainelli, linux-wireless, netdev
In-Reply-To: <4F873F10.6010207@candelatech.com>

On Thu, 2012-04-12 at 13:46 -0700, Ben Greear wrote:
> On 04/12/2012 12:30 PM, Ben Hutchings wrote:
> > On Thu, 2012-04-12 at 09:51 -0700, Ben Greear wrote:
> >> On 04/12/2012 09:42 AM, Florian Fainelli wrote:
> >>> Hi,
> >>>
> >>> Le 04/12/12 18:32, greearb@candelatech.com a écrit :
> >>>> From: Ben Greear<greearb@candelatech.com>
> >>>>
> >>>> The signal and noise are forced to be positive since ethtool
> >>>> deals in unsigned 64-bit values and this number should be human
> >>>> readable. This gives easy access to some of the data formerly
> >>>> exposed in the deprecated /proc/net/wireless file.
> >>>
> >>> Uh, that's misleading, the signal and noise values are typically negative, so one needs to think about mentally adding a minus sign if he/she wants to
> >>> understand it. Does not ethtool know about 32-bits signed integers?
> >>
> >> Ethtool stats only supports u64.  I think it's easy enough for
> >> humans or programs to add the negative sign.  Can signal or noise
> >> ever be>  0?  If so, that could actually break something that depends
> >> on flipping the value to negative....
> >
> > So far as I can see, the ethtool stats were expected to be counters,
> > which obviously cannot become negative (or fractional).  Maybe it's time
> > to define another command and string-set to cover other types of status
> > information.  The ethtool utility could ask for those typed statistics
> > as well, so 'ethtool -S' would get all of them.
> 
> One nice thing about ethtool stats API is that it is backwards and forwards
> compatible for a long while.

Agreed.

> Also, adding a new API means a second call
> into the kernel (most likely) for the other set of strings, which effectively
> doubles the cost of getting stats (and allows the stats to be updated
> out-of-sync with each other more easily).

It's generally not possible to take an atomic snapshot of all statistics
for a NIC, so that shouldn't be a consideration.

> I wonder if instead we could add a convention where we add a short
> prefix (or suffix) to a string to denote it's type (and cast the value
> into u64).  So, an old ethtool might see:
> 
> foo:s32: 4294967295

Actually it would be:

foo:s32: 18446744073709551615

> while a new one understand that the s32: prefix is special, do
> some casting, and could show:
> foo: -1
>
> Both are still at least somewhat human readable,

I don't think many humans can mentally substract 2^64.

> and probably wouldn't confuse anyone that is parsing the output of
> existing ethtool output.

I think you have this backwards: printing numbers in two different ways
(old and new versions of ethtool) is likely to confuse scripts that are
parsing and doing calculations with these numbers.  While I try to avoid
gratuitous changes in output formatting, scripts should use the ethtool
API if they really want interface stability.  It's not difficult (there
are at least Python and Perl bindings available) and it's a lot more
efficient.

Ben.

-- 
Ben Hutchings, Staff 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: [PATCH] ks8851: Fix missing mutex_lock/unlock
From: Flavio Leitner @ 2012-04-12 20:59 UTC (permalink / raw)
  To: Stephen Boyd; +Cc: mjr, davem, ben, netdev
In-Reply-To: <4F873C58.3000104@codeaurora.org>

On Thu, 12 Apr 2012 13:34:32 -0700
Stephen Boyd <sboyd@codeaurora.org> wrote:

> On 04/12/12 13:06, mjr@cs.wisc.edu wrote:
> > From: Matt Renzelmann <mjr@cs.wisc.edu>
> >
> > All calls to ks8851_rdreg* and ks8851_wrreg* should be protected with
> > the driver's lock mutex.  A spurious interrupt may otherwise cause a
> > crash.
> >
> > Signed-off-by: Matt Renzelmann <mjr@cs.wisc.edu>
> > ---
> >
> > Thank you, Mr. Leitner, for providing feedback.  I agree with your
> > changes and have updated the patch to reflect them.  I apologize for
> > missing the driver name in the title -- I've updated the patch with
> > that information as well.  Please let me know if there is anything
> > else I should fix/change.
> >
> >  drivers/net/ethernet/micrel/ks8851.c |    8 ++++++--
> >  1 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
> > index c722aa6..20237dc 100644
> > --- a/drivers/net/ethernet/micrel/ks8851.c
> > +++ b/drivers/net/ethernet/micrel/ks8851.c
> > @@ -1417,6 +1417,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
> >  {
> >  	struct net_device *ndev;
> >  	struct ks8851_net *ks;
> > +	int result;
> >  	int ret;
> >  
> >  	ndev = alloc_etherdev(sizeof(struct ks8851_net));
> > @@ -1515,9 +1516,12 @@ static int __devinit ks8851_probe(struct spi_device *spi)
> >  		goto err_netdev;
> >  	}
> >  
> > +	mutex_lock(&ks->lock);
> > +	result = CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER));
> > +	mutex_unlock(&ks->lock);
> > +
> >  	netdev_info(ndev, "revision %d, MAC %pM, IRQ %d, %s EEPROM\n",
> > -		    CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER)),
> > -		    ndev->dev_addr, ndev->irq,
> > +		    result, ndev->dev_addr, ndev->irq,
> >  		    ks->rc_ccr & CCR_EEPROM ? "has" : "no");
> >  
> >  	return 0;
> 
> This register is already read in the probe function and the lock is not
> held there so you seem to have missed a couple. I would guess it doesn't
> really matter tha we don't grab the lock though because the device isn't
> actively sending/receiving packets. How about this instead?

I believe that's because the IRQ isn't reserved yet, so there is no problem.


> diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
> index c722aa60..6f21fcd 100644
> --- a/drivers/net/ethernet/micrel/ks8851.c
> +++ b/drivers/net/ethernet/micrel/ks8851.c
> @@ -1418,6 +1418,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
>         struct net_device *ndev;
>         struct ks8851_net *ks;
>         int ret;
> +       unsigned cider;
>  
>         ndev = alloc_etherdev(sizeof(struct ks8851_net));
>         if (!ndev)
> @@ -1484,8 +1485,9 @@ static int __devinit ks8851_probe(struct spi_device *spi)
>         ks8851_soft_reset(ks, GRR_GSR);
>  
>         /* simple check for a valid chip being connected to the bus */
> -
> -       if ((ks8851_rdreg16(ks, KS_CIDER) & ~CIDER_REV_MASK) != CIDER_ID) {
> +       mutex_lock(&ks->lock);
> +       cider = ks8851_rdreg16(ks, KS_CIDER);
> +       if ((cider & ~CIDER_REV_MASK) != CIDER_ID) {
>                 dev_err(&spi->dev, "failed to read device ID\n");

If it fails here, the mutex is left locked.

>                 ret = -ENODEV;
>                 goto err_id;
> @@ -1493,6 +1495,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
>  
>         /* cache the contents of the CCR register for EEPROM, etc. */
>         ks->rc_ccr = ks8851_rdreg16(ks, KS_CCR);
> +       mutex_unlock(&ks->lock);
>  
>         if (ks->rc_ccr & CCR_EEPROM)
>                 ks->eeprom_size = 128;
> @@ -1516,8 +1519,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
>         }
>  
>         netdev_info(ndev, "revision %d, MAC %pM, IRQ %d, %s EEPROM\n",
> -                   CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER)),
> -                   ndev->dev_addr, ndev->irq,
> +                   CIDER_REV_GET(cider), ndev->dev_addr, ndev->irq,
>                     ks->rc_ccr & CCR_EEPROM ? "has" : "no");
>  
>         return 0;

I don't know the hw specs, but assuming that revision doesn't change between
those reads or that reading it doesn't do anything else... the above looks
better, indeed.

fbl

^ permalink raw reply

* RE: [PATCH] ks8851: Fix missing mutex_lock/unlock
From: Matt Renzelmann @ 2012-04-12 20:56 UTC (permalink / raw)
  To: 'Stephen Boyd', mjr; +Cc: fbl, davem, ben, netdev
In-Reply-To: <4F873C58.3000104@codeaurora.org>


> This register is already read in the probe function and the lock is not
> held there so you seem to have missed a couple. I would guess it doesn't
> really matter tha we don't grab the lock though because the device isn't
> actively sending/receiving packets. How about this instead?
> 

The reason I didn't go that way was the request_irq call is not made until near
the end of probe.  I believe only the ks8851_rdreg16 call in the netdev_info
statement actually needs the mutex protection.  That said, the approach you
posted looks reasonable as well and may be clearer.  I'm unsure as to which way
is best.

Thanks everyone for your help,
Matt

^ permalink raw reply

* Re: [PATCH 4/5] mac80211: Add more ethtools stats: survey, rates, etc
From: Ben Greear @ 2012-04-12 20:46 UTC (permalink / raw)
  To: Ben Hutchings
  Cc: Florian Fainelli, linux-wireless-u79uwXL29TY76Z2rM5mHXA,
	netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1334259053.2497.18.camel-/LGg1Z1CJKReKY3V0RtoKmatzQS1i7+A3tAM5lWOD0I@public.gmane.org>

On 04/12/2012 12:30 PM, Ben Hutchings wrote:
> On Thu, 2012-04-12 at 09:51 -0700, Ben Greear wrote:
>> On 04/12/2012 09:42 AM, Florian Fainelli wrote:
>>> Hi,
>>>
>>> Le 04/12/12 18:32, greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org a écrit :
>>>> From: Ben Greear<greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org>
>>>>
>>>> The signal and noise are forced to be positive since ethtool
>>>> deals in unsigned 64-bit values and this number should be human
>>>> readable. This gives easy access to some of the data formerly
>>>> exposed in the deprecated /proc/net/wireless file.
>>>
>>> Uh, that's misleading, the signal and noise values are typically negative, so one needs to think about mentally adding a minus sign if he/she wants to
>>> understand it. Does not ethtool know about 32-bits signed integers?
>>
>> Ethtool stats only supports u64.  I think it's easy enough for
>> humans or programs to add the negative sign.  Can signal or noise
>> ever be>  0?  If so, that could actually break something that depends
>> on flipping the value to negative....
>
> So far as I can see, the ethtool stats were expected to be counters,
> which obviously cannot become negative (or fractional).  Maybe it's time
> to define another command and string-set to cover other types of status
> information.  The ethtool utility could ask for those typed statistics
> as well, so 'ethtool -S' would get all of them.

One nice thing about ethtool stats API is that it is backwards and forwards
compatible for a long while.  Also, adding a new API means a second call
into the kernel (most likely) for the other set of strings, which effectively
doubles the cost of getting stats (and allows the stats to be updated
out-of-sync with each other more easily).

I wonder if instead we could add a convention where we add a short
prefix (or suffix) to a string to denote it's type (and cast the value
into u64).  So, an old ethtool might see:

foo:s32: 4294967295

while a new one understand that the s32: prefix is special, do
some casting, and could show:
foo: -1

Both are still at least somewhat human readable, and probably wouldn't
confuse anyone that is parsing the output of existing ethtool output.

Thanks,
Ben


-- 
Ben Greear <greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org>
Candela Technologies Inc  http://www.candelatech.com

--
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: [PATCH] ks8851: Fix missing mutex_lock/unlock
From: Flavio Leitner @ 2012-04-12 20:40 UTC (permalink / raw)
  To: mjr; +Cc: davem, sboyd, ben, netdev
In-Reply-To: <1334261204-8554-1-git-send-email-mjr@cs.wisc.edu>

On Thu, 12 Apr 2012 15:06:44 -0500
mjr@cs.wisc.edu wrote:

> From: Matt Renzelmann <mjr@cs.wisc.edu>
> 
> All calls to ks8851_rdreg* and ks8851_wrreg* should be protected with
> the driver's lock mutex.  A spurious interrupt may otherwise cause a
> crash.
> 
> Signed-off-by: Matt Renzelmann <mjr@cs.wisc.edu>
> ---
> 
> Thank you, Mr. Leitner, for providing feedback.  I agree with your
> changes and have updated the patch to reflect them.  I apologize for
> missing the driver name in the title -- I've updated the patch with
> that information as well.  Please let me know if there is anything
> else I should fix/change.

Signed-off-by: Flavio Leitner <fbl@redhat.com>

^ permalink raw reply

* Re: [PATCH] fixed coding style issue
From: David Miller @ 2012-04-12 20:36 UTC (permalink / raw)
  To: ahiliation; +Cc: netdev
In-Reply-To: <20120408160742.GA4613@debian.Jeff>

From: Jeffrin Jose <ahiliation@yahoo.co.in>
Date: Sun, 8 Apr 2012 21:37:42 +0530

> i have worked on some coding style issues
> on net/core/utils.c
> 
> please see attachment.

Applied, thanks.

^ permalink raw reply

* RE: [PATCH] ks8851: Cancel any pending IRQ work
From: Matt Renzelmann @ 2012-04-12 20:34 UTC (permalink / raw)
  To: 'Stephen Boyd'; +Cc: davem, ben, netdev, Matt Renzelmann
In-Reply-To: <4F8738EB.2060806@codeaurora.org>

> 
> Is this actually solving anything? Presumably cancel_work_sync() could
> run and then another spurious interrupt could come in after that
> function returns and we would have the same problem again. We should
> probably free the irq before unregistering the netdev so that
> ks8851_net_stop() would run after the interrupt is no longer registered,
> and the flush_work() in there would finish the last work. But then we
> have a problem where we're enabling the irq in the irq_work callback
> after the irq has been freed. Ugh.
> 
> I also see a potential deadlock in ks8851_net_stop(). ks8851_net_stop()
> holds the ks->lock while calling flush_work() which could deadlock if an
> interrupt comes and schedules an irq_work between the time
> ks8851_net_stop() grabs the mutex and calls flush_work().
> 

I agree on all counts -- the patch is buggy, though it does at least "shrink"
the window of vulnerability.  Frankly, I don't believe I'm qualified to write an
appropriate patch for this driver, at least without spending considerably more
time on it.

FWIW, I found this problem with a new driver-testing tool we've developed called
SymDrive, and my goal is primarily to determine if the bug is real or not.  The
tool is imperfect and we are trying to validate its operation.

That said, if there is an issue here, and we can come up with an appropriate
fix, then I'd be happy to write a patch for it.

^ permalink raw reply

* [PATCH net-next] net: vxge: Add MODULE_FIRMWARE
From: Tim Gardner @ 2012-04-12 20:34 UTC (permalink / raw)
  To: linux-kernel; +Cc: Tim Gardner, Jon Mason, netdev

Cc: Jon Mason <jdmason@kudzu.us>
Cc: netdev@vger.kernel.org
Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 drivers/net/ethernet/neterion/vxge/vxge-main.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/neterion/vxge/vxge-main.c b/drivers/net/ethernet/neterion/vxge/vxge-main.c
index 51387c3..dcef72d 100644
--- a/drivers/net/ethernet/neterion/vxge/vxge-main.c
+++ b/drivers/net/ethernet/neterion/vxge/vxge-main.c
@@ -4856,3 +4856,5 @@ vxge_closer(void)
 }
 module_init(vxge_starter);
 module_exit(vxge_closer);
+MODULE_FIRMWARE("vxge/X3fw-pxe.ncf");
+MODULE_FIRMWARE("vxge/X3fw.ncf");
-- 
1.7.9.5

^ permalink raw reply related

* Re: [PATCH] ks8851: Fix missing mutex_lock/unlock
From: Stephen Boyd @ 2012-04-12 20:34 UTC (permalink / raw)
  To: mjr; +Cc: fbl, davem, ben, netdev
In-Reply-To: <1334261204-8554-1-git-send-email-mjr@cs.wisc.edu>

On 04/12/12 13:06, mjr@cs.wisc.edu wrote:
> From: Matt Renzelmann <mjr@cs.wisc.edu>
>
> All calls to ks8851_rdreg* and ks8851_wrreg* should be protected with
> the driver's lock mutex.  A spurious interrupt may otherwise cause a
> crash.
>
> Signed-off-by: Matt Renzelmann <mjr@cs.wisc.edu>
> ---
>
> Thank you, Mr. Leitner, for providing feedback.  I agree with your
> changes and have updated the patch to reflect them.  I apologize for
> missing the driver name in the title -- I've updated the patch with
> that information as well.  Please let me know if there is anything
> else I should fix/change.
>
>  drivers/net/ethernet/micrel/ks8851.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
> index c722aa6..20237dc 100644
> --- a/drivers/net/ethernet/micrel/ks8851.c
> +++ b/drivers/net/ethernet/micrel/ks8851.c
> @@ -1417,6 +1417,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
>  {
>  	struct net_device *ndev;
>  	struct ks8851_net *ks;
> +	int result;
>  	int ret;
>  
>  	ndev = alloc_etherdev(sizeof(struct ks8851_net));
> @@ -1515,9 +1516,12 @@ static int __devinit ks8851_probe(struct spi_device *spi)
>  		goto err_netdev;
>  	}
>  
> +	mutex_lock(&ks->lock);
> +	result = CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER));
> +	mutex_unlock(&ks->lock);
> +
>  	netdev_info(ndev, "revision %d, MAC %pM, IRQ %d, %s EEPROM\n",
> -		    CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER)),
> -		    ndev->dev_addr, ndev->irq,
> +		    result, ndev->dev_addr, ndev->irq,
>  		    ks->rc_ccr & CCR_EEPROM ? "has" : "no");
>  
>  	return 0;

This register is already read in the probe function and the lock is not
held there so you seem to have missed a couple. I would guess it doesn't
really matter tha we don't grab the lock though because the device isn't
actively sending/receiving packets. How about this instead?

diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index c722aa60..6f21fcd 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -1418,6 +1418,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
        struct net_device *ndev;
        struct ks8851_net *ks;
        int ret;
+       unsigned cider;
 
        ndev = alloc_etherdev(sizeof(struct ks8851_net));
        if (!ndev)
@@ -1484,8 +1485,9 @@ static int __devinit ks8851_probe(struct spi_device *spi)
        ks8851_soft_reset(ks, GRR_GSR);
 
        /* simple check for a valid chip being connected to the bus */
-
-       if ((ks8851_rdreg16(ks, KS_CIDER) & ~CIDER_REV_MASK) != CIDER_ID) {
+       mutex_lock(&ks->lock);
+       cider = ks8851_rdreg16(ks, KS_CIDER);
+       if ((cider & ~CIDER_REV_MASK) != CIDER_ID) {
                dev_err(&spi->dev, "failed to read device ID\n");
                ret = -ENODEV;
                goto err_id;
@@ -1493,6 +1495,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
 
        /* cache the contents of the CCR register for EEPROM, etc. */
        ks->rc_ccr = ks8851_rdreg16(ks, KS_CCR);
+       mutex_unlock(&ks->lock);
 
        if (ks->rc_ccr & CCR_EEPROM)
                ks->eeprom_size = 128;
@@ -1516,8 +1519,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
        }
 
        netdev_info(ndev, "revision %d, MAC %pM, IRQ %d, %s EEPROM\n",
-                   CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER)),
-                   ndev->dev_addr, ndev->irq,
+                   CIDER_REV_GET(cider), ndev->dev_addr, ndev->irq,
                    ks->rc_ccr & CCR_EEPROM ? "has" : "no");
 
        return 0;


-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

^ permalink raw reply related

* Re: [net-next 3/4 (V3)] stmmac: add the Energy Efficient Ethernet support
From: David Miller @ 2012-04-12 20:32 UTC (permalink / raw)
  To: peppe.cavallaro; +Cc: netdev, bhutchings, rayagond
In-Reply-To: <1333704559-11251-4-git-send-email-peppe.cavallaro@st.com>

From: Giuseppe CAVALLARO <peppe.cavallaro@st.com>
Date: Fri,  6 Apr 2012 11:29:17 +0200

> +static int stmmac_ethtool_op_set_eee(struct net_device *dev,
> +				     struct ethtool_value *eee)
> +{
> +	struct stmmac_priv *priv = netdev_priv(dev);
> +
> +	if ((!eee->data) && (priv->eee_enabled)) {
> +		stmmac_disable_eee_mode(priv);
> +		priv->eee_enabled = eee->data;
> +	} else if ((eee->data) && (!priv->eee_enabled))
> +		/* We are asking for enabling the EEE but this
> +		 * has to be verified by invoking the eee_init function.
> +		 * For this reason we cannot set eee_enabled to
> +		 * eee->data, directly. */
> +		priv->eee_enabled = stmmac_eee_init(priv);
> +
> +	return 0;

If stmmac_eee_init() determines that it cannot enable eee, you should
return an appropriate error code here so that ethtool can report that
fact.

^ permalink raw reply

* Re: [net-next PATCH v3 1/8] net: add generic PF_BRIDGE:RTM_ FDB hooks
From: Ben Hutchings @ 2012-04-12 20:27 UTC (permalink / raw)
  To: John Fastabend
  Cc: shemminger, mst, davem, sri, hadi, jeffrey.t.kirsher, netdev,
	gregory.v.rose, krkumar2
In-Reply-To: <20120412170650.2717.56467.stgit@jf-dev1-dcblab>

On Thu, 2012-04-12 at 10:06 -0700, John Fastabend wrote:
[...]
> There is a slight complication in the case with both flags set
> when an error occurs. To resolve this the rtnl handler clears
> the NTF_ flag in the netlink ack to indicate which sets completed
> successfully. The add/del handlers will abort as soon as any
> error occurs.
[...]

Yes, this should work nicely.  Presumably these new flags are completely
ignored by the current implementation of PF_BRIDGE, and new userland
will also be able to use the flags in the response to detect the old
implementation.

Ben.

-- 
Ben Hutchings, Staff 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: [net-next PATCH v3 3/8] net: add fdb generic dump routine
From: Ben Hutchings @ 2012-04-12 20:20 UTC (permalink / raw)
  To: John Fastabend
  Cc: shemminger, mst, davem, sri, hadi, jeffrey.t.kirsher, netdev,
	gregory.v.rose, krkumar2
In-Reply-To: <20120412170702.2717.81539.stgit@jf-dev1-dcblab>

On Thu, 2012-04-12 at 10:07 -0700, John Fastabend wrote:
> This adds a generic dump routine drivers can call. It
> should be sufficient to handle any bridging model that
> uses the unicast address list. This should be most SR-IOV
> enabled NICs.
> 
> v2: return error on nlmsg_put and use -EMSGSIZE instead
>     of -ENOMEM this is inline other usages

It's still not propagated up to ndo_dflt_fdb_dump() though:

[...]
> +static int nlmsg_populate_fdb(struct sk_buff *skb,
> +			      struct netlink_callback *cb,
> +			      struct net_device *dev,
> +			      int *idx,
> +			      struct netdev_hw_addr_list *list)
> +{
> +	struct netdev_hw_addr *ha;
> +	int err;
> +	u32 pid, seq;
> +
> +	pid = NETLINK_CB(cb->skb).pid;
> +	seq = cb->nlh->nlmsg_seq;
> +
> +	list_for_each_entry(ha, &list->list, list) {
> +		if (*idx < cb->args[0])
> +			goto skip;
> +
> +		err = nlmsg_populate_fdb_fill(skb, dev, ha->addr,
> +					      pid, seq, 0, NTF_SELF);
> +		if (err < 0)
> +			break;
			return err;
> +skip:
> +		*idx += 1;
> +	}
> +	return 0;
> +}
[...]

-- 
Ben Hutchings, Staff 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: [PATCH] ks8851: Cancel any pending IRQ work
From: Stephen Boyd @ 2012-04-12 20:19 UTC (permalink / raw)
  To: mjr; +Cc: davem, ben, netdev
In-Reply-To: <1334249091-7605-1-git-send-email-mjr@cs.wisc.edu>

On 04/12/12 09:44, mjr@cs.wisc.edu wrote:
> From: Matt Renzelmann <mjr@cs.wisc.edu>
>
> An unexpected/spurious interrupt may cause the irq_work queue to
> execute during or after module unload, which can cause a crash.  It
> should be canceled.
>
> Signed-off-by: Matt Renzelmann <mjr@cs.wisc.edu>
> ---
>  drivers/net/ethernet/micrel/ks8851.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
> index c722aa6..ab46953 100644
> --- a/drivers/net/ethernet/micrel/ks8851.c
> +++ b/drivers/net/ethernet/micrel/ks8851.c
> @@ -1540,6 +1540,7 @@ static int __devexit ks8851_remove(struct spi_device *spi)
>  		dev_info(&spi->dev, "remove\n");
>  
>  	unregister_netdev(priv->netdev);
> +	cancel_work_sync(&priv->irq_work);
>  	free_irq(spi->irq, priv);
>  	free_netdev(priv->netdev);
>  

Is this actually solving anything? Presumably cancel_work_sync() could
run and then another spurious interrupt could come in after that
function returns and we would have the same problem again. We should
probably free the irq before unregistering the netdev so that
ks8851_net_stop() would run after the interrupt is no longer registered,
and the flush_work() in there would finish the last work. But then we
have a problem where we're enabling the irq in the irq_work callback
after the irq has been freed. Ugh.

I also see a potential deadlock in ks8851_net_stop(). ks8851_net_stop()
holds the ks->lock while calling flush_work() which could deadlock if an
interrupt comes and schedules an irq_work between the time
ks8851_net_stop() grabs the mutex and calls flush_work().

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

^ permalink raw reply

* Re: [PATCH] net/ipv6/ipv6_sockglue.c: Removed redundant extern
From: David Miller @ 2012-04-12 20:15 UTC (permalink / raw)
  To: eldad; +Cc: kuznet, jmorris, kaber, netdev, linux-kernel
In-Reply-To: <1334083887-5375-1-git-send-email-eldad@fogrefinery.com>

From: Eldad Zack <eldad@fogrefinery.com>
Date: Tue, 10 Apr 2012 20:51:27 +0200

> extern int sysctl_mld_max_msf is already defined in linux/ipv6.h.
> 
> Signed-off-by: Eldad Zack <eldad@fogrefinery.com>

Applied, thanks.

^ permalink raw reply

* [PATCH] ks8851: Fix missing mutex_lock/unlock
From: mjr @ 2012-04-12 20:06 UTC (permalink / raw)
  To: fbl; +Cc: davem, sboyd, ben, netdev, Matt Renzelmann

From: Matt Renzelmann <mjr@cs.wisc.edu>

All calls to ks8851_rdreg* and ks8851_wrreg* should be protected with
the driver's lock mutex.  A spurious interrupt may otherwise cause a
crash.

Signed-off-by: Matt Renzelmann <mjr@cs.wisc.edu>
---

Thank you, Mr. Leitner, for providing feedback.  I agree with your
changes and have updated the patch to reflect them.  I apologize for
missing the driver name in the title -- I've updated the patch with
that information as well.  Please let me know if there is anything
else I should fix/change.

 drivers/net/ethernet/micrel/ks8851.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index c722aa6..20237dc 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -1417,6 +1417,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
 {
 	struct net_device *ndev;
 	struct ks8851_net *ks;
+	int result;
 	int ret;
 
 	ndev = alloc_etherdev(sizeof(struct ks8851_net));
@@ -1515,9 +1516,12 @@ static int __devinit ks8851_probe(struct spi_device *spi)
 		goto err_netdev;
 	}
 
+	mutex_lock(&ks->lock);
+	result = CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER));
+	mutex_unlock(&ks->lock);
+
 	netdev_info(ndev, "revision %d, MAC %pM, IRQ %d, %s EEPROM\n",
-		    CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER)),
-		    ndev->dev_addr, ndev->irq,
+		    result, ndev->dev_addr, ndev->irq,
 		    ks->rc_ccr & CCR_EEPROM ? "has" : "no");
 
 	return 0;
-- 
1.7.5.4

^ permalink raw reply related

* Re: [PATCH] net/core: simple_strtoul cleanup
From: David Miller @ 2012-04-12 20:09 UTC (permalink / raw)
  To: shuahkhan; +Cc: netdev, linux-kernel
In-Reply-To: <1334258893.3022.7.camel@lorien2>

From: Shuah Khan <shuahkhan@gmail.com>
Date: Thu, 12 Apr 2012 13:28:13 -0600

> Changed net/core/net-sysfs.c: netdev_store() to use kstrtoul() 
> instead of obsolete simple_strtoul().
> 
>>From 84371b0ed8b277ec8005fd0efbc73165a8bd72b9 Mon Sep 17 00:00:00 2001
> From: Shuah Khan <shuahkhan@gmail.com>
> Date: Thu, 12 Apr 2012 10:36:10 -0600
> Subject: [PATCH] net/core: simple_strtoul cleanup
> 
> 
> Signed-off-by: Shuah Khan <shuahkhan@gmail.com>

Applied, thanks.

^ permalink raw reply

* Re: [PATCH 0/8] r6040: misc cleanups
From: David Miller @ 2012-04-12 20:07 UTC (permalink / raw)
  To: florian; +Cc: netdev
In-Reply-To: <1334164723-9627-1-git-send-email-florian@openwrt.org>

From: Florian Fainelli <florian@openwrt.org>
Date: Wed, 11 Apr 2012 19:18:35 +0200

> This series contains some cleanups and one small fix, they are targetted
> at net-next.

All applied, thanks Florian.

^ permalink raw reply

* Re: [PATCH 1/2] qlcnic: Add default swtich case in 'qlcnic_can_start_firmware()'
From: David Miller @ 2012-04-12 20:02 UTC (permalink / raw)
  To: santoshprasadnayak
  Cc: anirban.chakraborty, rajesh.borundia, sony.chacko, linux-driver,
	netdev, kernel-janitors
In-Reply-To: <1333951187-4395-1-git-send-email-santoshprasadnayak@gmail.com>


You haven't told me what tree these two patches should be applied to,
so I am dropping them both.

Resubmit with a proper destination tree indication.

Thanks.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox