Netdev List
 help / color / mirror / Atom feed
* [PATCH 01/13] mm: Serialize access to min_free_kbytes
From: Mel Gorman @ 2011-04-27 16:07 UTC (permalink / raw)
  To: Linux-MM, Linux-Netdev
  Cc: LKML, David Miller, Neil Brown, Peter Zijlstra, Mel Gorman
In-Reply-To: <1303920491-25302-1-git-send-email-mgorman@suse.de>

There is a race between the min_free_kbytes sysctl, memory hotplug
and transparent hugepage support enablement.  Memory hotplug uses a
zonelists_mutex to avoid a race when building zonelists. Reuse it to
serialise watermark updates.

[a.p.zijlstra@chello.nl: Older patch fixed the race with spinlock]
Signed-off-by: Mel Gorman <mgorman@suse.de>
---
 mm/page_alloc.c |   23 +++++++++++++++--------
 1 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 1d5c189..a93013a 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4990,14 +4990,7 @@ static void setup_per_zone_lowmem_reserve(void)
 	calculate_totalreserve_pages();
 }
 
-/**
- * setup_per_zone_wmarks - called when min_free_kbytes changes
- * or when memory is hot-{added|removed}
- *
- * Ensures that the watermark[min,low,high] values for each zone are set
- * correctly with respect to min_free_kbytes.
- */
-void setup_per_zone_wmarks(void)
+static void __setup_per_zone_wmarks(void)
 {
 	unsigned long pages_min = min_free_kbytes >> (PAGE_SHIFT - 10);
 	unsigned long lowmem_pages = 0;
@@ -5052,6 +5045,20 @@ void setup_per_zone_wmarks(void)
 	calculate_totalreserve_pages();
 }
 
+/**
+ * setup_per_zone_wmarks - called when min_free_kbytes changes
+ * or when memory is hot-{added|removed}
+ *
+ * Ensures that the watermark[min,low,high] values for each zone are set
+ * correctly with respect to min_free_kbytes.
+ */
+void setup_per_zone_wmarks(void)
+{
+	mutex_lock(&zonelists_mutex);
+	__setup_per_zone_wmarks();
+	mutex_unlock(&zonelists_mutex);
+}
+
 /*
  * The inactive anon list should be small enough that the VM never has to
  * do too much work, but large enough that each inactive page has a chance
-- 
1.7.3.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply related

* [PATCH 00/13] Swap-over-NBD without deadlocking v3
From: Mel Gorman @ 2011-04-27 16:07 UTC (permalink / raw)
  To: Linux-MM, Linux-Netdev
  Cc: LKML, David Miller, Neil Brown, Peter Zijlstra, Mel Gorman

Changelog since V2
  o Document that __GFP_NOMEMALLOC overrides __GFP_MEMALLOC		(Neil)
  o Use wait_event_interruptible					(Neil)
  o Use !! when casting to bool to avoid any possibilitity of type
    truncation								(Neil)
  o Nicer logic when using skb_pfmemalloc_protocol			(Neil)

Changelog since V1
  o Rebase on top of mmotm
  o Use atomic_t for memalloc_socks		(David Miller)
  o Remove use of sk_memalloc_socks in vmscan	(Neil Brown)
  o Check throttle within prepare_to_wait	(Neil Brown)
  o Add statistics on throttling instead of printk

Swapping over NBD is something that is technically possible but not
often advised. While there are number of guides on the internet
on how to configure it and nbd-client supports a -swap switch to
"prevent deadlocks", the fact of the matter is a machine using NBD
for swap can be locked up within minutes if swap is used intensively.

The problem is that network block devices do not use mempools like
normal block devices do. As the host cannot control where they receive
packets from, they cannot reliably work out in advance how much memory
they might need.

Some years ago, Peter Ziljstra developed a series of patches that
supported swap over an NFS that some distributions are carrying in
their kernels. This patch series borrows very heavily from Peter's work
to support swapping over NBD (the relatively straight-forward case)
and uses throttling instead of dynamically resized memory reserves
so the series is not too unwieldy for review.

Patch 1 serialises access to min_free_kbytes. It's not strictly needed
	by this series but as the series cares about watermarks in
	general, it's a harmless fix. It could be merged independently.

Patch 2 adds knowledge of the PFMEMALLOC reserves to SLAB and SLUB to
	preserve access to pages allocated under low memory situations
	to callers that are freeying memory.

Patch 3 introduces __GFP_MEMALLOC to allow access to the PFMEMALLOC
	reserves without setting PFMEMALLOC.

Patch 4 opens the possibility for softirqs to use PFMEMALLOC reserves
	for later use by network packet processing.

Patch 5 ignores memory policies when ALLOC_NO_WATERMARKS is set.

Patches 6-9 allows network processing to use PFMEMALLOC reserves when
	the socket has been marked as being used by the VM to clean
	pages. If packets are received and stored in pages that were
	allocated under low-memory situations and are unrelated to
	the VM, the packets are dropped.

Patch 10 is a micro-optimisation to avoid a function call in the
	common case.

Patch 11 tags NBD sockets as being SOCK_MEMALLOC so they can use
	PFMEMALLOC if necessary.

Patch 12 notes that it is still possible for the PFMEMALLOC reserve
	to be depleted. To prevent this, direct reclaimers get
	throttled on a waitqueue if 50% of the PFMEMALLOC reserves are
	depleted.  It is expected that kswapd and the direct reclaimers
	already running will clean enough pages for the low watermark
	to be reached and the throttled processes are woken up.

Patch 13 adds a statistic to track how often processes get throttled

Some basic performance testing was run using kernel builds, netperf
on loopback for UDP and TCP, hackbench (pipes and sockets), iozone
and sysbench. Each of them were expected to use the sl*b allocators
reasonably heavily but there did not appear to be significant
performance variances. Here is the results from netperf using
slab as an example

NETPERF UDP
                   netperf-udp       udp-swapnbd
                  vanilla-slab        v1r17-slab
      64   178.06 ( 0.00%)*   189.46 ( 6.02%) 
             1.02%             1.00%        
     128   355.06 ( 0.00%)    370.75 ( 4.23%) 
     256   662.47 ( 0.00%)    721.62 ( 8.20%) 
    1024  2229.39 ( 0.00%)   2567.04 (13.15%) 
    2048  3974.20 ( 0.00%)   4114.70 ( 3.41%) 
    3312  5619.89 ( 0.00%)   5800.09 ( 3.11%) 
    4096  6460.45 ( 0.00%)   6702.45 ( 3.61%) 
    8192  9580.24 ( 0.00%)   9927.97 ( 3.50%) 
   16384 13259.14 ( 0.00%)  13493.88 ( 1.74%) 
MMTests Statistics: duration
User/Sys Time Running Test (seconds)       2960.17   2540.14
Total Elapsed Time (seconds)               3554.10   3050.10

NETPERF TCP
                   netperf-tcp       tcp-swapnbd
                  vanilla-slab        v1r17-slab
      64  1230.29 ( 0.00%)   1273.17 ( 3.37%) 
     128  2309.97 ( 0.00%)   2375.22 ( 2.75%) 
     256  3659.32 ( 0.00%)   3704.87 ( 1.23%) 
    1024  7267.80 ( 0.00%)   7251.02 (-0.23%) 
    2048  8358.26 ( 0.00%)   8204.74 (-1.87%) 
    3312  8631.07 ( 0.00%)   8637.62 ( 0.08%) 
    4096  8770.95 ( 0.00%)   8704.08 (-0.77%) 
    8192  9749.33 ( 0.00%)   9769.06 ( 0.20%) 
   16384 11151.71 ( 0.00%)  11135.32 (-0.15%) 
MMTests Statistics: duration
User/Sys Time Running Test (seconds)       1245.04   1619.89
Total Elapsed Time (seconds)               1250.66   1622.18

Here is the equivalent test for SLUB

NETPERF UDP
                   netperf-udp       udp-swapnbd
                  vanilla-slub        v1r17-slub
      64   180.83 ( 0.00%)    183.68 ( 1.55%) 
     128   357.29 ( 0.00%)    367.11 ( 2.67%) 
     256   679.64 ( 0.00%)*   724.03 ( 6.13%) 
             1.15%             1.00%        
    1024  2343.40 ( 0.00%)*  2610.63 (10.24%) 
             1.68%             1.00%        
    2048  3971.53 ( 0.00%)   4102.21 ( 3.19%)*
             1.00%             1.40%        
    3312  5677.04 ( 0.00%)   5748.69 ( 1.25%) 
    4096  6436.75 ( 0.00%)   6549.41 ( 1.72%) 
    8192  9698.56 ( 0.00%)   9808.84 ( 1.12%) 
   16384 13337.06 ( 0.00%)  13404.38 ( 0.50%) 
MMTests Statistics: duration
User/Sys Time Running Test (seconds)       2880.15   2180.13
Total Elapsed Time (seconds)               3458.10   2618.09

NETPERF TCP
                   netperf-tcp       tcp-swapnbd
                  vanilla-slub        v1r17-slub
      64  1256.79 ( 0.00%)   1287.32 ( 2.37%) 
     128  2308.71 ( 0.00%)   2371.09 ( 2.63%) 
     256  3672.03 ( 0.00%)   3771.05 ( 2.63%) 
    1024  7245.08 ( 0.00%)   7261.60 ( 0.23%) 
    2048  8315.17 ( 0.00%)   8244.14 (-0.86%) 
    3312  8611.43 ( 0.00%)   8616.90 ( 0.06%) 
    4096  8711.64 ( 0.00%)   8695.97 (-0.18%) 
    8192  9795.71 ( 0.00%)   9774.11 (-0.22%) 
   16384 11145.48 ( 0.00%)  11225.70 ( 0.71%) 
MMTests Statistics: duration
User/Sys Time Running Test (seconds)       1345.05   1425.06
Total Elapsed Time (seconds)               1350.61   1430.66

Time to completion varied a lot but this can happen with netperf as
it tries to find results within a sufficiently high confidence. I
wouldn't read too much into the performance gains of netperf-udp
as it can sometimes be affected by code just shuffling around for
whatever reason.

For testing swap-over-NBD, a machine was booted with 2G of RAM with a
swapfile backed by NBD. 16*NUM_CPU processes were started that create
anonymous memory mappings and read them linearly in a loop. The total
size of the mappings were 4*PHYSICAL_MEMORY to use swap heavily under
memory pressure. Without the patches, the machine locks up within
minutes and runs to completion with them applied.

 drivers/block/nbd.c           |    7 +-
 include/linux/gfp.h           |   13 ++-
 include/linux/mm_types.h      |    8 ++
 include/linux/mmzone.h        |    1 +
 include/linux/sched.h         |    7 ++
 include/linux/skbuff.h        |   19 +++-
 include/linux/slub_def.h      |    1 +
 include/linux/vm_event_item.h |    1 +
 include/net/sock.h            |   19 ++++
 kernel/softirq.c              |    3 +
 mm/page_alloc.c               |   57 ++++++++--
 mm/slab.c                     |  240 +++++++++++++++++++++++++++++++++++------
 mm/slub.c                     |   35 +++++-
 mm/vmscan.c                   |   55 ++++++++++
 mm/vmstat.c                   |    1 +
 net/core/dev.c                |   48 ++++++++-
 net/core/filter.c             |    8 ++
 net/core/skbuff.c             |   95 ++++++++++++++---
 net/core/sock.c               |   42 +++++++
 net/ipv4/tcp.c                |    3 +-
 net/ipv4/tcp_output.c         |   13 ++-
 net/ipv6/tcp_ipv6.c           |   12 ++-
 22 files changed, 601 insertions(+), 87 deletions(-)

-- 
1.7.3.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply

* Re: [ethtool PATCH 4/6] Add support for __be64 and bitops to ethtool
From: Ben Hutchings @ 2011-04-27 15:54 UTC (permalink / raw)
  To: Alexander Duyck; +Cc: davem, jeffrey.t.kirsher, netdev
In-Reply-To: <20110421204035.23054.6918.stgit@gitlad.jf.intel.com>

On Thu, 2011-04-21 at 13:40 -0700, Alexander Duyck wrote:
> This change is meant to add support for __be64 values and bitops to
> ethtool.  These changes will be needed in order to support network flow
> classifier rule configuration.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---
> 
>  ethtool-bitops.h |   25 +++++++++++++++++++++++++
>  ethtool-util.h   |   30 ++++++++++++++++++++++++++----
>  ethtool.c        |    7 -------
>  3 files changed, 51 insertions(+), 11 deletions(-)
>  create mode 100644 ethtool-bitops.h
> 
> diff --git a/ethtool-bitops.h b/ethtool-bitops.h
> new file mode 100644
> index 0000000..0ff14f1
> --- /dev/null
> +++ b/ethtool-bitops.h
> @@ -0,0 +1,25 @@
> +#ifndef ETHTOOL_BITOPS_H__
> +#define ETHTOOL_BITOPS_H__
> +
> +#define BITS_PER_BYTE		8
> +#define BITS_PER_LONG		(BITS_PER_BYTE * sizeof(long))
> +#define DIV_ROUND_UP(n, d)	(((n) + (d) - 1) / (d))
> +#define BITS_TO_LONGS(nr)	DIV_ROUND_UP(nr, BITS_PER_LONG)
> +
> +static inline void set_bit(int nr, unsigned long *addr)
> +{
> +	addr[nr / BITS_PER_LONG] |= 1UL << (nr % BITS_PER_LONG);
> +}
> +
> +static inline void clear_bit(int nr, unsigned long *addr)
> +{
> +	addr[nr / BITS_PER_LONG] &= ~(1UL << (nr % BITS_PER_LONG));
> +}
> +
> +static __always_inline int test_bit(unsigned int nr, const unsigned long *addr)
> +{
> +	return !!((1UL << (nr % BITS_PER_LONG)) &
> +		  (((unsigned long *)addr)[nr / BITS_PER_LONG]));
> +}

Where is __always_inline supposed to be defined?

> +#endif
> diff --git a/ethtool-util.h b/ethtool-util.h
> index f053028..3d46faf 100644
> --- a/ethtool-util.h
> +++ b/ethtool-util.h
> @@ -5,15 +5,18 @@
>  
>  #include <sys/types.h>
>  #include <endian.h>
> +#include <sys/ioctl.h>
> +#include <net/if.h>
> +#include "ethtool-config.h"
> +#include "ethtool-copy.h"
>  
>  /* ethtool.h expects these to be defined by <linux/types.h> */
>  #ifndef HAVE_BE_TYPES
>  typedef __uint16_t __be16;
>  typedef __uint32_t __be32;
> +typedef unsigned long long __be64;
>  #endif
>  
> -#include "ethtool-copy.h"
> -

You can't move the inclusion of ethtool-copy.h; that defeats the whole
purpose of the HAVE_BE_TYPES feature test.

[...]
> +#ifndef ARRAY_SIZE
> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> +#endif
> +
> +#ifndef SIOCETHTOOL
> +#define SIOCETHTOOL     0x8946
>  #endif
>  
>  /* National Semiconductor DP83815, DP83816 */
> diff --git a/ethtool.c b/ethtool.c
> index 9ad7000..15af86a 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -45,16 +45,9 @@
>  #include <linux/sockios.h>
>  #include "ethtool-util.h"
>  
> -
> -#ifndef SIOCETHTOOL
> -#define SIOCETHTOOL     0x8946
> -#endif
>  #ifndef MAX_ADDR_LEN
>  #define MAX_ADDR_LEN	32
>  #endif
> -#ifndef ARRAY_SIZE
> -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> -#endif
>  
>  #ifndef HAVE_NETIF_MSG
>  enum {
> 

Presumably this is needed by the next patch, but it has nothing to do
with what the commit message says.

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: [ethtool PATCH 2/6] Add support for NFC flow classifier extensions
From: Ben Hutchings @ 2011-04-27 15:48 UTC (permalink / raw)
  To: Alexander Duyck; +Cc: davem, jeffrey.t.kirsher, netdev
In-Reply-To: <20110421204025.23054.3310.stgit@gitlad.jf.intel.com>

On Thu, 2011-04-21 at 13:40 -0700, Alexander Duyck wrote:
> This change makes it so that we can add VLAN Ethertype, TCI, and 64bits of
> driver defined data to a network flow classifier allowing us to handle a
> n-tuple flow contained within a network flow classifier.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
[...]

This does not correspond to any version of ethtool.h in kernel history.
I sync'd from current net-next-2.6 instead.

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: [ethtool PATCH 1/6] Add support for ESP as a separate protocol from AH
From: Ben Hutchings @ 2011-04-27 15:47 UTC (permalink / raw)
  To: Alexander Duyck; +Cc: davem, jeffrey.t.kirsher, netdev
In-Reply-To: <20110421204020.23054.60822.stgit@gitlad.jf.intel.com>

On Thu, 2011-04-21 at 13:40 -0700, Alexander Duyck wrote:
> This change is mostly cosmetic.  NIU had supported AH and ESP seperately.
> As such it is possible that a return value of ESP or AH may be returned
> for a has request instead of the AH_ESP combined value.  To resolve that
> the inputs are combined for AH and ESP into the AH_ESP value and return
> values for AH and ESP will display the combined string info.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>

Applied, but I moved this into a separate commit:

[...]
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -32,7 +32,6 @@
>  #include <sys/ioctl.h>
>  #include <sys/stat.h>
>  #include <stdio.h>
> -#include <string.h>
>  #include <errno.h>
>  #include <net/if.h>
>  #include <sys/utsname.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: [RFC PATCH] netlink: Increase netlink dump skb message size
From: Steve Hodgson @ 2011-04-27 15:46 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Rose, Gregory V, David Miller, netdev@vger.kernel.org,
	bhutchings@solarflare.com
In-Reply-To: <1303834864.3358.58.camel@edumazet-laptop>

On 04/27/2011 04:24 PM, Eric Dumazet wrote:
> Le mardi 26 avril 2011 à 09:12 -0700, Rose, Gregory V a écrit :
>
>> I'm fine with however you folks want to approach this, just give me some direction.
>
> I would just try following patch :
>

This allows the sfc driver to use 102 VFs, up from the current limit of 
45 VFs.

It's unfortunate that this patch isn't sufficient to allow all 127 VFs 
to be used, but whilst we wait for a new netlink api this is an 
improvement worth having.

I'd like to see this patch committed.

Thanks Eric,

- Steve

^ permalink raw reply

* RE: [PATCH] igb: restore EEPROM 16kB access limit
From: Wyborny, Carolyn @ 2011-04-27 15:01 UTC (permalink / raw)
  To: Andy Gospodarek
  Cc: Stefan Assmann, netdev@vger.kernel.org,
	e1000-devel@lists.sourceforge.net, Kirsher, Jeffrey T,
	Pieper, Jeffrey E, Ronciak, John
In-Reply-To: <20110427141542.GB21309@gospo.rdu.redhat.com>



>-----Original Message-----
>From: Andy Gospodarek [mailto:andy@greyhouse.net]
>Sent: Wednesday, April 27, 2011 7:16 AM
>To: Wyborny, Carolyn
>Cc: Andy Gospodarek; Stefan Assmann; netdev@vger.kernel.org; e1000-
>devel@lists.sourceforge.net; Kirsher, Jeffrey T; Pieper, Jeffrey E;
>Ronciak, John
>Subject: Re: [PATCH] igb: restore EEPROM 16kB access limit
>
>On Tue, Apr 26, 2011 at 08:12:20AM -0700, Wyborny, Carolyn wrote:
>[...]
>> Part of the problem you are seeing is an apparently widespread EEPROM
>problem where the size word in the EEPROM is invalid.  Since we didn't
>really check it before it didn't cause a problem.  I have a patch coming
>that addresses this by messaging the user that the size is invalid but
>setting it to a default and continuing.
>>
>
>It wasn't really a problem for me until the commit Stefan mentioned
>4322e561a93ec7ee034b603a6c610e7be90d4e8a was applied.
>
>I'm glad you are planning a fix for it, but I hope it can be out soon
>and not held up for too long by other patches planned for the next
>update.

Yes, the problem wasn't there before because of a bug in the code.  The bad EEPROM's have apparently been out there a while and are now being exposed, now that the code is fixed.  We didn't see one in our test of the fix originally or know there were out there until the reports starting coming in.

I'm pushing the fix through as soon as possible.  Its in test now.  I apologize for the delay.

Thanks,

Carolyn

Carolyn Wyborny
Linux Development
LAN Access Division
Intel Corporation



^ permalink raw reply

* Re: [PATCH net-next-2.6] ipv4,ipv6,bonding: Restore control over number of peer notifications
From: Brian Haley @ 2011-04-27 14:21 UTC (permalink / raw)
  To: Ben Hutchings
  Cc: Jay Vosburgh, Andy Gospodarek, David Miller, Patrick McHardy,
	netdev
In-Reply-To: <1303870446.3032.399.camel@localhost>

On 04/26/2011 10:14 PM, Ben Hutchings wrote:
> On Tue, 2011-04-26 at 22:09 -0400, Brian Haley wrote:
>> On 04/26/2011 09:25 PM, Ben Hutchings wrote:
>>> For backward compatibility, we should retain the module parameters and
>>> sysfs attributes to control the number of peer notifications
>>> (gratuitous ARPs and unsolicited NAs) sent after bonding failover.
>>> Also, it is possible for failover to take place even though the new
>>> active slave does not have link up, and in that case the peer
>>> notification should be deferred until it does.
>>>
>>> Change ipv4 and ipv6 so they do not automatically send peer
>>> notifications on bonding failover.
>>>
>>> Change the bonding driver to send separate NETDEV_NOTIFY_PEERS
>>> notifications when the link is up, as many times as requested.  Since
>>> it does not directly control which protocols send notifications, make
>>> num_grat_arp and num_unsol_na aliases for a single parameter.  Bump
>>> the bonding version number and update its documentation.
>>>
>>> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
>>
>> Signed-off-by: Brian Haley <brian.haley@hp.com>
> 
> I'm not sure what you mean by this.  You didn't write any of it and
> you're not a maintainer with your own repository.  Did you mean to say
> 'Reviewed-by' or 'Acked-by'?

Sorry, yes:

Acked-by: Brian Haley <brian.haley@hp.com>

^ permalink raw reply

* Re: [PATCH] igb: restore EEPROM 16kB access limit
From: Andy Gospodarek @ 2011-04-27 14:15 UTC (permalink / raw)
  To: Wyborny, Carolyn
  Cc: e1000-devel@lists.sourceforge.net, netdev@vger.kernel.org,
	Stefan Assmann, Ronciak, John, Andy Gospodarek
In-Reply-To: <EDC0E76513226749BFBC9C3FB031318F016AF39010@orsmsx508.amr.corp.intel.com>

On Tue, Apr 26, 2011 at 08:12:20AM -0700, Wyborny, Carolyn wrote:
[...]
> Part of the problem you are seeing is an apparently widespread EEPROM problem where the size word in the EEPROM is invalid.  Since we didn't really check it before it didn't cause a problem.  I have a patch coming that addresses this by messaging the user that the size is invalid but setting it to a default and continuing.  
> 

It wasn't really a problem for me until the commit Stefan mentioned
4322e561a93ec7ee034b603a6c610e7be90d4e8a was applied.

I'm glad you are planning a fix for it, but I hope it can be out soon
and not held up for too long by other patches planned for the next
update.


------------------------------------------------------------------------------
WhatsUp Gold - Download Free Network Management Software
The most intuitive, comprehensive, and cost-effective network 
management toolset available today.  Delivers lowest initial 
acquisition cost and overall TCO of any competing solution.
http://p.sf.net/sfu/whatsupgold-sd
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

^ permalink raw reply

* Re: [PATCH] Applying inappropriate ioctl operation on socket should return ENOTTY
From: Lifeng Sun @ 2011-04-27 13:54 UTC (permalink / raw)
  To: Alan Cox; +Cc: linux-kernel, netdev
In-Reply-To: <20110427130904.47331a9f@lxorguk.ukuu.org.uk>

On 13:09 Wed 04/27/11 Apr, Alan Cox wrote:
> > diff --git a/drivers/char/applicom.c b/drivers/char/applicom.c
> > index 25373df..50c09e4 100644
> > --- a/drivers/char/applicom.c
> > +++ b/drivers/char/applicom.c
> > @@ -838,6 +838,6 @@ static long ac_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> >  	Dummy = readb(apbs[IndexCard].RamIO + VERS);
> >  	kfree(adgl);
> >  	mutex_unlock(&ac_mutex);
> > -	return 0;
> > +	return ret;
> >  }
> 
> This one in fact is a bug fix where 0 gets returned not an error code it
> ought to be submitted separately and described as such.

Will do.

> 
> > diff --git a/drivers/char/dtlk.c b/drivers/char/dtlk.c
> > index 85156dd..2d116d5 100644
> > --- a/drivers/char/dtlk.c
> > +++ b/drivers/char/dtlk.c
> > @@ -289,7 +289,7 @@ static long dtlk_ioctl(struct file *file,
> >  		return put_user(portval, argp);
> >  
> >  	default:
> > -		return -EINVAL;
> > +		return -ENOTTY;
> >  	}
> >  }
> 
> This one looks good (and the driver has another error in the ioctl
> handler too that wants fixing where it returnds -EINVAL not -EFAULT)

Will fix.

> > diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
> > index d72433f..4ba9b9f 100644
> > --- a/drivers/char/i8k.c
> > +++ b/drivers/char/i8k.c
> > @@ -370,7 +370,7 @@ i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg)
> >  		break;
> >  
> >  	default:
> > -		return -EINVAL;
> > +		return -ENOTTY;
> 
> This one is incomplete - the driver also has a bogus check for arg being
> non zero. That means ioctl(fd, BOGUS, 0) will return the wrong error code
> still.

Will fix.

> > diff --git a/drivers/char/ipmi/ipmi_devintf.c b/drivers/char/ipmi/ipmi_devintf.c
> > index 2aa3977..bc8af5a 100644
> > --- a/drivers/char/ipmi/ipmi_devintf.c
> > +++ b/drivers/char/ipmi/ipmi_devintf.c
> > @@ -232,7 +232,7 @@ static int ipmi_ioctl(struct file   *file,
> >  		      unsigned int  cmd,
> >  		      unsigned long data)
> >  {
> > -	int                      rv = -EINVAL;
> > +	int                      rv = -ENOTTY;
> >  	struct ipmi_file_private *priv = file->private_data;
> >  	void __user *arg = (void __user *)data;
> 
> No - there are cases that should return -EINVAL that this will break - a
> default case needs adding

All execution paths overwrite the return value except those should
return -ENOTTY, but it's more clear to add a default case.

Will do.

> > diff --git a/drivers/char/viotape.c b/drivers/char/viotape.c
> > index ad6e64a..a427d40 100644
> > --- a/drivers/char/viotape.c
> > +++ b/drivers/char/viotape.c
> > @@ -529,7 +529,7 @@ static int viotap_ioctl(struct inode *inode, struct file *file,
> >  
> >  	down(&reqSem);
> >  
> > -	ret = -EINVAL;
> > +	ret = -ENOTTY;
> 
> Again this messes up the returns because code assumes the initial
> default.

Likewise, except the unsupported MTIOCPOS command. SuSv4 has two
appropriate errno's for this unsupported case and the one below
returns EOPNOTSUPP,

 [ENOTSUP]
     Not supported (may be the same value as [EOPNOTSUPP]).

 [EOPNOTSUPP]
     Operation not supported on socket (may be the same value as
     [ENOTSUP]).

but the manpage of ioctl disagree. I am wondering how to handle
unsupported ioctl operations. Maybe following the manpage is a better
choice though it's not exact.


> The original code also has bugs too (wrong error off copy_*_user()
> again)

Will fix.

> > diff --git a/fs/pipe.c b/fs/pipe.c
> > index da42f7d..fe7ffe4 100644
> > --- a/fs/pipe.c
> > +++ b/fs/pipe.c
> > @@ -665,7 +665,7 @@ static long pipe_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> >  
> >  			return put_user(count, (int __user *)arg);
> >  		default:
> > -			return -EINVAL;
> > +			return -ENOTTY;
> >  	}
> 
> Looks good - but this one really does want to be a patch on its own as if
> anything causes compatibility funnies it will be this, and we need to be
> sure we can bisect nicely to it should this occur.

will submit serparately.

> > @@ -5041,7 +5041,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)
> >  		/* Set the per device memory buffer space.
> >  		 * Not applicable in our case */
> >  	case SIOCSIFLINK:
> > -		return -EINVAL;
> > +		return -EOPNOTSUPP;
> 
> This change seems unrelated to anything in your description and outside
> of anything SuS cares about or demands.

As stated above. I would submit separately.


- Lifeng

-- 

^ permalink raw reply

* Re: Question for canutils
From: Marc Kleine-Budde @ 2011-04-27 13:50 UTC (permalink / raw)
  To: Tomoya MORINAGA
  Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
	netdev-u79uwXL29TY76Z2rM5mHXA,
	toshiharu-linux-ECg8zkTtlr0C6LszWs/t0g,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	'Wolfgang Grandegger'
In-Reply-To: <02D6556ECE7A41859777EB7300D12394-c0cKtqp5df7I9507bXv2FdBPR1lH4CV8@public.gmane.org>


[-- Attachment #1.1: Type: text/plain, Size: 1937 bytes --]

On 04/27/2011 10:36 AM, Tomoya MORINAGA wrote:
> I have 2 questions for canutils.
> 
> 1) Build issue
> I downloaded the latest canutils and libsocketcan.
>    - canutils-4.0.6
>    - libsocketcan-0.0.8
> 
> Firstly, I installed libsocketcan-0.0.8

Into which location?

Please send me the output of:

ls -l /usr/local/lib
ls -l /usr/local/lib/pkgconfig

> Secondly, I tried to install canutils-4.0.6.
> But it failed like below.
> 
> [root@localhost canutils-4.0.6]# ./configure
> ...snip...
> checking whether lstat correctly handles trailing slash... yes
> checking whether stat accepts an empty string... no
> checking for socket... yes
> checking for strchr... yes
> checking for strtoul... yes
> checking for pkg-config... /usr/bin/pkg-config
> checking pkg-config is at least version 0.9.0... yes
> checking for libsocketcan... no
> configure: error: *** libsocketcan version above 0.0.8 not found on your system
> [root@localhost canutils-4.0.6]# 
> 
> Do you have any information about the fail?

> BTW, Using canutils-3.0.2, I could confirm build becomes success.

3.x doesn't work with the mainline configuration interface.

> 2) How to use
> Executing candump like following, I see the following message.(Of course, pch_can have already installed)
> [root@localhost morinaga]# candump can0
> interface = can0, family = 29, type = 3, proto = 1
> read: Network is down
> [root@localhost morinaga]#
> 
> Would you tell me how to active can0 interface.

First build the canutils, then configure the bitrate, e.g. 250 kbit/s:
    $ canconfig can0 bitrate 250000
    $ ifconfig can0 up

regards, Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[-- Attachment #2: Type: text/plain, Size: 188 bytes --]

_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core

^ permalink raw reply

* Re: [PATCH v4 1/1] can: add pruss CAN driver.
From: Wolfgang Grandegger @ 2011-04-27 13:34 UTC (permalink / raw)
  To: Subhasish Ghosh
  Cc: sachi-EvXpCiN+lbve9wHmmfpqLFaTQe2KTcn/,
	davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/,
	Netdev-u79uwXL29TY76Z2rM5mHXA, nsekhar-l0cyMroinI0, open list,
	CAN NETWORK DRIVERS, Marc Kleine-Budde, m-watkins-l0cyMroinI0,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <4DB81A12.1000006-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>

On 04/27/2011 03:28 PM, Wolfgang Grandegger wrote:
> On 04/27/2011 03:08 PM, Subhasish Ghosh wrote:
>>>
>>> - Use just *one* value per sysfs file
>>
>> SG - I felt adding entry for each mbx_id will clutter the sysfs.
>>        Is it ok to do that.
> 
> No, see:
> 
> http://lxr.linux.no/#linux+v2.6.38/Documentation/filesystems/sysfs.txt#L56

s/No/Yes/

Sorry for the confusion.

Wolfgang.

^ permalink raw reply

* Re: [PATCH v4 1/1] can: add pruss CAN driver.
From: Wolfgang Grandegger @ 2011-04-27 13:28 UTC (permalink / raw)
  To: Subhasish Ghosh
  Cc: sachi-EvXpCiN+lbve9wHmmfpqLFaTQe2KTcn/,
	davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/,
	Netdev-u79uwXL29TY76Z2rM5mHXA, nsekhar-l0cyMroinI0, open list,
	CAN NETWORK DRIVERS, Marc Kleine-Budde, m-watkins-l0cyMroinI0,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <46D523E49EFF489F9B088AE7B9CD7286@subhasishg>

On 04/27/2011 03:08 PM, Subhasish Ghosh wrote:
>>
>> - Use just *one* value per sysfs file
> 
> SG - I felt adding entry for each mbx_id will clutter the sysfs.
>        Is it ok to do that.

No, see:

http://lxr.linux.no/#linux+v2.6.38/Documentation/filesystems/sysfs.txt#L56

>>>> +static u32 pruss_intc_init[19][3] = {
>>>> + {PRUSS_INTC_POLARITY0, PRU_INTC_REGMAP_MASK, 0xFFFFFFFF},
>>>> + {PRUSS_INTC_POLARITY1, PRU_INTC_REGMAP_MASK, 0xFFFFFFFF},
>>>> + {PRUSS_INTC_TYPE0, PRU_INTC_REGMAP_MASK, 0x1C000000},
>>>> + {PRUSS_INTC_TYPE1, PRU_INTC_REGMAP_MASK, 0},
>>>> + {PRUSS_INTC_GLBLEN, 0, 1},
>>>> + {PRUSS_INTC_HOSTMAP0, PRU_INTC_REGMAP_MASK, 0x03020100},
>>>> + {PRUSS_INTC_HOSTMAP1, PRU_INTC_REGMAP_MASK, 0x07060504},
>>>> + {PRUSS_INTC_HOSTMAP2, PRU_INTC_REGMAP_MASK, 0x0000908},
>>>> + {PRUSS_INTC_CHANMAP0, PRU_INTC_REGMAP_MASK, 0},
>>>> + {PRUSS_INTC_CHANMAP8, PRU_INTC_REGMAP_MASK, 0x00020200},
>>>> + {PRUSS_INTC_STATIDXCLR, 0, 32},
>>>> + {PRUSS_INTC_STATIDXCLR, 0, 19},
>>>> + {PRUSS_INTC_ENIDXSET, 0, 19},
>>>> + {PRUSS_INTC_STATIDXCLR, 0, 18},
>>>> + {PRUSS_INTC_ENIDXSET, 0, 18},
>>>> + {PRUSS_INTC_STATIDXCLR, 0, 34},
>>>> + {PRUSS_INTC_ENIDXSET, 0, 34},
>>>> + {PRUSS_INTC_ENIDXSET, 0, 32},
>>>> + {PRUSS_INTC_HOSTINTEN, 0, 5}
>>>
>>> please add ","
>>
>> Also a struct to describe each entry would improve readability.
>> Then you could also use ARRAY_SIZE.
> 
> SG _ I could not follow this, are you recommending that I create a
> structure with three variables and then create
>            an array for it.
> something like:
> 
> const static struct [] = {
>    {
>    unsigned int reg_base;
>    unsigned int reg_mask;
>    unsigned int reg_val;
>    },
> ...
> };

Yes:

  struct s_name {
	unsigned int base;
	unsigned int mask;
	unsigned int val;
  };

  const static struct s_name array[] = {
	...
  };

> 
>>>> + value = (PRUSS_CAN_GPIO_SETUP_DELAY *
>>>> + (priv->clk_freq_pru / 1000000) / 1000) /
>>>> + PRUSS_CAN_DELAY_LOOP_LENGTH;
>>
>> This calculation looks delicate. 64-bit math would be safer.
> 
> SG - This one works fine. I am dividing it twice to avoid the problem.

Yes, but what if the frequency increases with the next generation of the
hardware?

Wolfgang.

^ permalink raw reply

* Re: [PATCH v4 1/1] can: add pruss CAN driver.
From: Arnd Bergmann @ 2011-04-27 13:25 UTC (permalink / raw)
  To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: sachi-EvXpCiN+lbve9wHmmfpqLFaTQe2KTcn/,
	davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/,
	Subhasish Ghosh, nsekhar-l0cyMroinI0, open list,
	CAN NETWORK DRIVERS, Marc Kleine-Budde,
	Netdev-u79uwXL29TY76Z2rM5mHXA, m-watkins-l0cyMroinI0,
	Wolfgang Grandegger
In-Reply-To: <46D523E49EFF489F9B088AE7B9CD7286@subhasishg>

On Wednesday 27 April 2011, Subhasish Ghosh wrote:
> >
> > - Use just one value per sysfs file
> 
> SG - I felt adding entry for each mbx_id will clutter the sysfs.
>         Is it ok to do that.

That is probably not much better either.

Note also that every sysfs file needs to come with associated
documentation in Documentation/ABI/*/ to make sure that users
will know exactly how the file is meant to work. 

Why do you need to export these values in the first place? Is
it just for debugging or do you expect all CAN user space
to look at this?

If it's for debugging, please don't export the files through sysfs.
Depending on how useful the data is to regular users, you can
still export it through a debugfs file in that case, which has
much less strict rules.

If the file is instead meant as part of the regular operation of
the device, it should not be in debugfs but probably be integrated
into the CAN socket interface, so that users don't need to work
with two different ways of getting to the device (socket and sysfs).

	Arnd

^ permalink raw reply

* Re: [PATCH v4 1/1] can: add pruss CAN driver.
From: Marc Kleine-Budde @ 2011-04-27 13:21 UTC (permalink / raw)
  To: Subhasish Ghosh
  Cc: sachi-EvXpCiN+lbve9wHmmfpqLFaTQe2KTcn/,
	davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/,
	Netdev-u79uwXL29TY76Z2rM5mHXA, nsekhar-l0cyMroinI0, open list,
	CAN NETWORK DRIVERS,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	m-watkins-l0cyMroinI0, Wolfgang Grandegger
In-Reply-To: <46D523E49EFF489F9B088AE7B9CD7286@subhasishg>


[-- Attachment #1.1: Type: text/plain, Size: 2178 bytes --]

On 04/27/2011 03:08 PM, Subhasish Ghosh wrote:

>>>> +static u32 pruss_intc_init[19][3] = {
>>>> + {PRUSS_INTC_POLARITY0, PRU_INTC_REGMAP_MASK, 0xFFFFFFFF},
>>>> + {PRUSS_INTC_POLARITY1, PRU_INTC_REGMAP_MASK, 0xFFFFFFFF},
>>>> + {PRUSS_INTC_TYPE0, PRU_INTC_REGMAP_MASK, 0x1C000000},
>>>> + {PRUSS_INTC_TYPE1, PRU_INTC_REGMAP_MASK, 0},
>>>> + {PRUSS_INTC_GLBLEN, 0, 1},
>>>> + {PRUSS_INTC_HOSTMAP0, PRU_INTC_REGMAP_MASK, 0x03020100},
>>>> + {PRUSS_INTC_HOSTMAP1, PRU_INTC_REGMAP_MASK, 0x07060504},
>>>> + {PRUSS_INTC_HOSTMAP2, PRU_INTC_REGMAP_MASK, 0x0000908},
>>>> + {PRUSS_INTC_CHANMAP0, PRU_INTC_REGMAP_MASK, 0},
>>>> + {PRUSS_INTC_CHANMAP8, PRU_INTC_REGMAP_MASK, 0x00020200},
>>>> + {PRUSS_INTC_STATIDXCLR, 0, 32},
>>>> + {PRUSS_INTC_STATIDXCLR, 0, 19},
>>>> + {PRUSS_INTC_ENIDXSET, 0, 19},
>>>> + {PRUSS_INTC_STATIDXCLR, 0, 18},
>>>> + {PRUSS_INTC_ENIDXSET, 0, 18},
>>>> + {PRUSS_INTC_STATIDXCLR, 0, 34},
>>>> + {PRUSS_INTC_ENIDXSET, 0, 34},
>>>> + {PRUSS_INTC_ENIDXSET, 0, 32},
>>>> + {PRUSS_INTC_HOSTINTEN, 0, 5}
>>>
>>> please add ","
>>
>> Also a struct to describe each entry would improve readability.
>> Then you could also use ARRAY_SIZE.
> 
> SG _ I could not follow this, are you recommending that I create a
> structure with three variables and then create
>            an array for it.
> something like:
> 
> const static struct [] = {
>    {
>    unsigned int reg_base;
>    unsigned int reg_mask;
>    unsigned int reg_val;
>    },
> ...
> };

I think this isn't valid C. It should look like this:

struct pruss_intc_init {
	unsigned long reg_base;
	u32 reg_mask;
	u32 reg+val;
};

static const struct pruss_intc_init pruss_initc_init[] = {
	{ .reg_base = 0xdeadbeef, .reg_mask = 0xaa, .reg_val = 0x55 },
	...
};

I'm not sure about the datatype of reg_base. I haven't looked at the
code that uses this array.

cheers, Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[-- Attachment #2: Type: text/plain, Size: 188 bytes --]

_______________________________________________
Socketcan-core mailing list
Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
https://lists.berlios.de/mailman/listinfo/socketcan-core

^ permalink raw reply

* Re: [PATCH v4 1/1] can: add pruss CAN driver.
From: Subhasish Ghosh @ 2011-04-27 13:08 UTC (permalink / raw)
  To: Wolfgang Grandegger, Marc Kleine-Budde
  Cc: sachi-EvXpCiN+lbve9wHmmfpqLFaTQe2KTcn/,
	davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/,
	Netdev-u79uwXL29TY76Z2rM5mHXA, nsekhar-l0cyMroinI0, open list,
	CAN NETWORK DRIVERS, m-watkins-l0cyMroinI0,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <4DB5D452.9050500-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>

>
> - Use just *one* value per sysfs file

SG - I felt adding entry for each mbx_id will clutter the sysfs.
        Is it ok to do that.


>>> +static u32 pruss_intc_init[19][3] = {
>>> + {PRUSS_INTC_POLARITY0, PRU_INTC_REGMAP_MASK, 0xFFFFFFFF},
>>> + {PRUSS_INTC_POLARITY1, PRU_INTC_REGMAP_MASK, 0xFFFFFFFF},
>>> + {PRUSS_INTC_TYPE0, PRU_INTC_REGMAP_MASK, 0x1C000000},
>>> + {PRUSS_INTC_TYPE1, PRU_INTC_REGMAP_MASK, 0},
>>> + {PRUSS_INTC_GLBLEN, 0, 1},
>>> + {PRUSS_INTC_HOSTMAP0, PRU_INTC_REGMAP_MASK, 0x03020100},
>>> + {PRUSS_INTC_HOSTMAP1, PRU_INTC_REGMAP_MASK, 0x07060504},
>>> + {PRUSS_INTC_HOSTMAP2, PRU_INTC_REGMAP_MASK, 0x0000908},
>>> + {PRUSS_INTC_CHANMAP0, PRU_INTC_REGMAP_MASK, 0},
>>> + {PRUSS_INTC_CHANMAP8, PRU_INTC_REGMAP_MASK, 0x00020200},
>>> + {PRUSS_INTC_STATIDXCLR, 0, 32},
>>> + {PRUSS_INTC_STATIDXCLR, 0, 19},
>>> + {PRUSS_INTC_ENIDXSET, 0, 19},
>>> + {PRUSS_INTC_STATIDXCLR, 0, 18},
>>> + {PRUSS_INTC_ENIDXSET, 0, 18},
>>> + {PRUSS_INTC_STATIDXCLR, 0, 34},
>>> + {PRUSS_INTC_ENIDXSET, 0, 34},
>>> + {PRUSS_INTC_ENIDXSET, 0, 32},
>>> + {PRUSS_INTC_HOSTINTEN, 0, 5}
>>
>> please add ","
>
> Also a struct to describe each entry would improve readability.
> Then you could also use ARRAY_SIZE.

SG _ I could not follow this, are you recommending that I create a structure 
with three variables and then create
            an array for it.
something like:

const static struct [] = {
    {
    unsigned int reg_base;
    unsigned int reg_mask;
    unsigned int reg_val;
    },
...
};


>>> + value = (PRUSS_CAN_GPIO_SETUP_DELAY *
>>> + (priv->clk_freq_pru / 1000000) / 1000) /
>>> + PRUSS_CAN_DELAY_LOOP_LENGTH;
>
> This calculation looks delicate. 64-bit math would be safer.

SG - This one works fine. I am dividing it twice to avoid the problem.


>>> + pru_can_mask_ints(priv->dev, PRUSS_CAN_TX_PRU_1, false);
>>> + pru_can_mask_ints(priv->dev, PRUSS_CAN_RX_PRU_0, false);
>>> + can_bus_off(ndev);
>>> + dev_dbg(priv->ndev->dev.parent, "Bus off mode\n");
>>> + }
>>> +
>>> + netif_rx(skb);
>
> You should use netif_receive_skb(skb) here as well.

SG - Ok, Will do.

>
> if (PRUSS_CAN_ISR_BIT_ESI &
>     priv->can_rx_cntx.intr_stat) {
>
> Is more readable.

SG - Ok, Will do.

>
>
>>> + pru_can_gbl_stat_get(priv->dev,
>>> + &priv->can_rx_cntx);
>>> + pru_can_err(ndev,
>>> + priv->can_rx_cntx.intr_stat,
>>> + priv->can_rx_cntx.gbl_stat);
>
> Please fix bogous indention.

SG - Ok, Will do.


>>> +
>>> + pdata = dev->platform_data;
>>> + if (!pdata) {
>>> + dev_err(&pdev->dev, "platform data not found\n");
>>> + return -EINVAL;
>>> + }
>>> + (pdata->setup)();
>>
>> no need fot the ( )

SG - Ok, Will do.

>>> + }
>>> +
>>> + priv->ndev = ndev;
>>> + priv->dev = dev;
>>> +
>>> + priv->can.bittiming_const = &pru_can_bittiming_const;
>>> + priv->can.do_set_bittiming = pru_can_set_bittiming;
>>> + priv->can.do_set_mode = pru_can_set_mode;
>>> + priv->can.do_get_state = pru_can_get_state;
>
> Please remove that callback. It's not needed as state changes are
> handled properly.
>

SG -- Ok, Will do

^ permalink raw reply

* Re: Maximum no of bytes Ethernet can transfer at a time ??
From: Neil Horman @ 2011-04-27 13:01 UTC (permalink / raw)
  To: Ajit; +Cc: netdev
In-Reply-To: <loom.20110427T140651-957@post.gmane.org>

On Wed, Apr 27, 2011 at 12:11:35PM +0000, Ajit wrote:
> Guys,
> 
> I have developed a code which uses raw sockets to transfer files. My code skips
> all the upper layer protocols,I have designed a small protocol of my own.
> 
> Now to problem is, whenever I transfer a large file it creates a problem. If
> transfer a file of suppose 100kb or more, only 97.9 Kb is received, unlike in
> the case of files smaller that 97.9.
> 
> What can be the problem ?? Does continuously sending and receiving of frames
> creates a problem ??
> 
> If any one is interested I can give you my code..
> 
> Thank you. 
> 
What transport layer are you using (UDP/TCP/SCTP/etc)?  Does a simmilar transfer
work if you use an actual TCP/UDP socket, rather than your raw one?  If you're
consistently getting 97.9k transferred, that almost sounds like some sort of
firewall type connection limitation.  Take a TCPDump from the peer to get a
better idea of whats going on when the connection fails
Neil

> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply

* Re: Maximum no of bytes Ethernet can transfer at a time ??
From: Ajit @ 2011-04-27 12:27 UTC (permalink / raw)
  To: netdev
In-Reply-To: <1303906910.3166.50.camel@edumazet-laptop>

Eric Dumazet <eric.dumazet <at> gmail.com> writes:

> Sure, check your syscall returns values, and search for SO_RCVBUF &
> SO_SNDBUF   (man 7 socket)
> 
> --

okies..I dont know exactly how to use those but I will google and try it..
will post my result after some time.

Thank you



^ permalink raw reply

* Re: Maximum no of bytes Ethernet can transfer at a time ??
From: Eric Dumazet @ 2011-04-27 12:21 UTC (permalink / raw)
  To: Ajit; +Cc: netdev
In-Reply-To: <loom.20110427T140651-957@post.gmane.org>

Le mercredi 27 avril 2011 à 12:11 +0000, Ajit a écrit :
> Guys,
> 
> I have developed a code which uses raw sockets to transfer files. My code skips
> all the upper layer protocols,I have designed a small protocol of my own.
> 
> Now to problem is, whenever I transfer a large file it creates a problem. If
> transfer a file of suppose 100kb or more, only 97.9 Kb is received, unlike in
> the case of files smaller that 97.9.
> 
> What can be the problem ?? Does continuously sending and receiving of frames
> creates a problem ??

Sure, check your syscall returns values, and search for SO_RCVBUF &
SO_SNDBUF   (man 7 socket)





^ permalink raw reply

* Re: [Bugme-new] [Bug 33842] New: NULL pointer dereference in ip_fragment
From: Eric Dumazet @ 2011-04-27 12:17 UTC (permalink / raw)
  To: Tomas Carnecky; +Cc: Bandan Das, David Miller, netdev, akpm
In-Reply-To: <4DB8037E.7010207@dbservice.com>

Le mercredi 27 avril 2011 à 13:52 +0200, Tomas Carnecky a écrit :
> On 4/27/11 9:41 AM, Eric Dumazet wrote:
> > netconsole=4444@192.168.20.108/eth0,4444@192.168.20.112/00:1e:0b:ec:c3:e4
> I'm not having any luck with the netconsole. The last message I see on 
> the target host is "Freeing unused kernel memory: 100k freed". I don't 
> see any messages after that. Do I need to configure the same IP address 
> in netconsole as is later configured by userspace? I set netconsole to 
> use 192.168.0.50 while the init scripts set br0 to have 192.168.0.82. 
> And would netconsole even work, as the bug is in the networking code itself?
> 
> 

It should work yes, even for a bug in networking stack.

Yes, you should take the source address you're supposed to have once
machine running. I suspect it could work with another IP address, but
using the normal one makes sure you dont hit some anti spoofing rule in
your LAN.

To check if netconsole works (after boot), you can try

dmesg -n 8
modprobe pktgen

You should see on remote machine :
pktgen: Packet Generator for packet performance testing. Version: 2.74




^ permalink raw reply

* Maximum no of bytes Ethernet can transfer at a time ??
From: Ajit @ 2011-04-27 12:11 UTC (permalink / raw)
  To: netdev

Guys,

I have developed a code which uses raw sockets to transfer files. My code skips
all the upper layer protocols,I have designed a small protocol of my own.

Now to problem is, whenever I transfer a large file it creates a problem. If
transfer a file of suppose 100kb or more, only 97.9 Kb is received, unlike in
the case of files smaller that 97.9.

What can be the problem ?? Does continuously sending and receiving of frames
creates a problem ??

If any one is interested I can give you my code..

Thank you. 


^ permalink raw reply

* Re: [PATCH] Applying inappropriate ioctl operation on socket should return ENOTTY
From: Alan Cox @ 2011-04-27 12:09 UTC (permalink / raw)
  To: Lifeng Sun; +Cc: linux-kernel, netdev
In-Reply-To: <1303882625-28115-1-git-send-email-lifongsun@gmail.com>

> diff --git a/drivers/char/applicom.c b/drivers/char/applicom.c
> index 25373df..50c09e4 100644
> --- a/drivers/char/applicom.c
> +++ b/drivers/char/applicom.c
> @@ -838,6 +838,6 @@ static long ac_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  	Dummy = readb(apbs[IndexCard].RamIO + VERS);
>  	kfree(adgl);
>  	mutex_unlock(&ac_mutex);
> -	return 0;
> +	return ret;
>  }

This one in fact is a bug fix where 0 gets returned not an error code it
ought to be submitted separately and described as such.

> diff --git a/drivers/char/dtlk.c b/drivers/char/dtlk.c
> index 85156dd..2d116d5 100644
> --- a/drivers/char/dtlk.c
> +++ b/drivers/char/dtlk.c
> @@ -289,7 +289,7 @@ static long dtlk_ioctl(struct file *file,
>  		return put_user(portval, argp);
>  
>  	default:
> -		return -EINVAL;
> +		return -ENOTTY;
>  	}
>  }

This one looks good (and the driver has another error in the ioctl
handler too that wants fixing where it returnds -EINVAL not -EFAULT)

>  
> diff --git a/drivers/char/generic_nvram.c b/drivers/char/generic_nvram.c
> index 0e941b5..95278e9 100644
> --- a/drivers/char/generic_nvram.c
> +++ b/drivers/char/generic_nvram.c
> @@ -111,7 +111,7 @@ static int nvram_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  		nvram_sync();
>  		break;
>  	default:
> -		return -EINVAL;
> +		return -ENOTTY;
>  	}

Looks good

>  	return 0;
> diff --git a/drivers/char/genrtc.c b/drivers/char/genrtc.c
> index f773a9d..6f4c3da 100644
> --- a/drivers/char/genrtc.c
> +++ b/drivers/char/genrtc.c
> @@ -330,7 +330,7 @@ static int gen_rtc_ioctl(struct file *file,
>  	    }
>  	}
>  
> -	return -EINVAL;
> +	return -ENOTTY;
>  }

Likewise


>  static long gen_rtc_unlocked_ioctl(struct file *file, unsigned int cmd,
> diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
> index 7066e80..720de66 100644
> --- a/drivers/char/hpet.c
> +++ b/drivers/char/hpet.c
> @@ -575,7 +575,7 @@ hpet_ioctl_common(struct hpet_dev *devp, int cmd, unsigned long arg,
>  	case HPET_IE_ON:
>  		return hpet_ioctl_ieon(devp);
>  	default:
> -		return -EINVAL;
> +		return -ENOTTY;
>  	}

Ok

>  	err = 0;
> diff --git a/drivers/char/i8k.c b/drivers/char/i8k.c
> index d72433f..4ba9b9f 100644
> --- a/drivers/char/i8k.c
> +++ b/drivers/char/i8k.c
> @@ -370,7 +370,7 @@ i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg)
>  		break;
>  
>  	default:
> -		return -EINVAL;
> +		return -ENOTTY;

This one is incomplete - the driver also has a bogus check for arg being
non zero. That means ioctl(fd, BOGUS, 0) will return the wrong error code
still.

>  	}
>  
>  	if (val < 0)
> diff --git a/drivers/char/ipmi/ipmi_devintf.c b/drivers/char/ipmi/ipmi_devintf.c
> index 2aa3977..bc8af5a 100644
> --- a/drivers/char/ipmi/ipmi_devintf.c
> +++ b/drivers/char/ipmi/ipmi_devintf.c
> @@ -232,7 +232,7 @@ static int ipmi_ioctl(struct file   *file,
>  		      unsigned int  cmd,
>  		      unsigned long data)
>  {
> -	int                      rv = -EINVAL;
> +	int                      rv = -ENOTTY;
>  	struct ipmi_file_private *priv = file->private_data;
>  	void __user *arg = (void __user *)data;

No - there are cases that should return -EINVAL that this will break - a
default case needs adding

> diff --git a/drivers/char/lp.c b/drivers/char/lp.c
> index 97c3edb..2ff32c8 100644
> --- a/drivers/char/lp.c
> +++ b/drivers/char/lp.c
> @@ -650,7 +650,7 @@ static int lp_do_ioctl(unsigned int minor, unsigned int cmd,
>  			break;
>  
>  		default:
> -			retval = -EINVAL;
> +			retval = -ENOTTY;
>  	}
>  	return retval;

Looks good

>  }
> diff --git a/drivers/char/nwflash.c b/drivers/char/nwflash.c
> index a12f524..45b7a7a 100644
> --- a/drivers/char/nwflash.c
> +++ b/drivers/char/nwflash.c
> @@ -115,7 +115,7 @@ static long flash_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
>  		gbWriteBase64Enable = 0;
>  		gbWriteEnable = 0;
>  		mutex_unlock(&flash_mutex);
> -		return -EINVAL;
> +		return -ENOTTY;

Ok

>  	}
>  	mutex_unlock(&flash_mutex);
>  	return 0;
> diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c
> index f176dba..8dce214 100644
> --- a/drivers/char/ppdev.c
> +++ b/drivers/char/ppdev.c
> @@ -622,7 +622,7 @@ static int pp_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  
>  	default:
>  		pr_debug(CHRDEV "%x: What? (cmd=0x%x)\n", minor, cmd);
> -		return -EINVAL;
> +		return -ENOTTY;
>  	}

Looks good

>  
>  	/* Keep the compiler happy */
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index d4ddeba..40aad1c 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1157,7 +1157,7 @@ static long random_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
>  		rand_initialize();
>  		return 0;
>  	default:
> -		return -EINVAL;
> +		return -ENOTTY;

Ok
>  	}
>  }
>  
> diff --git a/drivers/char/raw.c b/drivers/char/raw.c
> index b4b9d5a..a992bf1 100644
> --- a/drivers/char/raw.c
> +++ b/drivers/char/raw.c
> @@ -231,7 +231,7 @@ static long raw_ctl_ioctl(struct file *filp, unsigned int command,
>  		return 0;
>  	}
>  
> -	return -EINVAL;
> +	return -ENOTTY;

Ok

>  }
>  
>  #ifdef CONFIG_COMPAT
> @@ -273,7 +273,7 @@ static long raw_ctl_compat_ioctl(struct file *file, unsigned int cmd,
>  		return 0;
>  	}
>  
> -	return -EINVAL;
> +	return -ENOTTY;
>  }
>  #endif

Ok

>  
> diff --git a/drivers/char/viotape.c b/drivers/char/viotape.c
> index ad6e64a..a427d40 100644
> --- a/drivers/char/viotape.c
> +++ b/drivers/char/viotape.c
> @@ -529,7 +529,7 @@ static int viotap_ioctl(struct inode *inode, struct file *file,
>  
>  	down(&reqSem);
>  
> -	ret = -EINVAL;
> +	ret = -ENOTTY;

Again this messes up the returns because code assumes the initial
default. The original code also has bugs too (wrong error off
copy_*_user() again)

>  
>  	switch (cmd) {
>  	case MTIOCTOP:
> diff --git a/fs/pipe.c b/fs/pipe.c
> index da42f7d..fe7ffe4 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -665,7 +665,7 @@ static long pipe_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  
>  			return put_user(count, (int __user *)arg);
>  		default:
> -			return -EINVAL;
> +			return -ENOTTY;
>  	}

Looks good - but this one really does want to be a patch on its own as if
anything causes compatibility funnies it will be this, and we need to be
sure we can bisect nicely to it should this occur.

>  }
>  
> diff --git a/net/core/dev.c b/net/core/dev.c
> index c2ac599..b93c76d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4773,7 +4773,7 @@ static int dev_ifsioc_locked(struct net *net, struct ifreq *ifr, unsigned int cm
>  		 * is never reached
>  		 */
>  		WARN_ON(1);
> -		err = -EINVAL;
> +		err = -ENOTTY;

This case doesn't really matter - it can't happen anyway so might as well
change
>  		break;
>  
>  	}
> @@ -5041,7 +5041,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)
>  		/* Set the per device memory buffer space.
>  		 * Not applicable in our case */
>  	case SIOCSIFLINK:
> -		return -EINVAL;
> +		return -EOPNOTSUPP;

This change seems unrelated to anything in your description and outside
of anything SuS cares about or demands.
>  
>  	/*
>  	 *	Unknown or private ioctl.
> @@ -5062,7 +5062,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)
>  		/* Take care of Wireless Extensions */
>  		if (cmd >= SIOCIWFIRST && cmd <= SIOCIWLAST)
>  			return wext_handle_ioctl(net, &ifr, cmd, arg);
> -		return -EINVAL;
> +		return -ENOTTY;

and this one looks right.

Alan

^ permalink raw reply

* Re: Kernel crash after using new Intel NIC (igb)
From: Eric Dumazet @ 2011-04-27 12:04 UTC (permalink / raw)
  To: Maximilian Engelhardt
  Cc: Wyborny, Carolyn, linux-kernel@vger.kernel.org,
	netdev@vger.kernel.org, StuStaNet Vorstand,
	e1000-devel@lists.sourceforge.net
In-Reply-To: <201104271346.34431.maxi@daemonizer.de>

Le mercredi 27 avril 2011 à 13:46 +0200, Maximilian Engelhardt a écrit :

> Hello Carolyn,
> 
> Thanks for your response.
> 
> I have opened a issue at
> https://sourceforge.net/tracker/?func=detail&aid=3293703&group_id=42302&atid=447449
> and also posted all information there.
> 
> 
> Please not that yesterday I updated the kernel, so I'm now running 2.6.38.4.
> Eric Dumazet mentioned on the LKML that this might be a memory corruption that 
> my be solved with kernel 2.6.38.
> 

Hmm, I suggested to use slub_nomerge to make sure inetpeer kmemcache is
not shared with another (possibly mem corruptor) layer.

Please note we were not able to track the bug in 2.6.37 kernels.

> I'll report if the crash happens again, but it might take some times as in the 
> past it happened within the interval of weeks to month.
> 

Thanks

^ permalink raw reply

* Re: [Bugme-new] [Bug 33842] New: NULL pointer dereference in ip_fragment
From: Tomas Carnecky @ 2011-04-27 11:52 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: Bandan Das, David Miller, netdev, akpm
In-Reply-To: <1303890067.3166.4.camel@edumazet-laptop>

On 4/27/11 9:41 AM, Eric Dumazet wrote:
> netconsole=4444@192.168.20.108/eth0,4444@192.168.20.112/00:1e:0b:ec:c3:e4
I'm not having any luck with the netconsole. The last message I see on 
the target host is "Freeing unused kernel memory: 100k freed". I don't 
see any messages after that. Do I need to configure the same IP address 
in netconsole as is later configured by userspace? I set netconsole to 
use 192.168.0.50 while the init scripts set br0 to have 192.168.0.82. 
And would netconsole even work, as the bug is in the networking code itself?



^ permalink raw reply

* Re: [PATCH] Applying inappropriate ioctl operation on socket should return ENOTTY
From: Alan Cox @ 2011-04-27 11:52 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: Lifeng Sun, linux-kernel, netdev
In-Reply-To: <1303901103.3166.30.camel@edumazet-laptop>

> Well, I wont argue the point, especially if you Ack the changes ;)
> 
> My only concern was to not break old applications, I dont know if it is
> going to break _any_ of them. Probably these old applications stick with
> old kernels.

The number of applications that actually check ioctl error codes beyond
if error perror; return is pretty small and those that do generally do so
for very narrow cases or for things like EWOULDBLOCK/EINTR stuff.

> If you ask me ENOTTY is plain wrong.
> ioctl() is not restricted to terminal devices at all.

Like the tab/space thing in Makefiles and Qwerty keyboards its now part of
how stuff all works but EINVAL is even worse because you cannot tell
between 'this ioctl isn't know/is used on the wrong fd' and 'argument
wrong to valid ioctl'

Alan

^ 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