Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH net-next-2.6 v4 1/1] can: c_can: Added support for Bosch C_CAN controller
From: Marc Kleine-Budde @ 2011-01-12  9:30 UTC (permalink / raw)
  To: Wolfgang Grandegger
  Cc: Bhupesh SHARMA, Socketcan-core@lists.berlios.de,
	netdev@vger.kernel.org, David Miller
In-Reply-To: <4D2D6F95.8030502@grandegger.com>

[-- Attachment #1: Type: text/plain, Size: 2129 bytes --]

On 01/12/2011 10:08 AM, Wolfgang Grandegger wrote:
> On 01/12/2011 09:51 AM, Bhupesh SHARMA wrote:
>> Hi Marc,
>>
>> Thanks for your review.
>> Please see my comments inline:
>>
>>> -----Original Message-----
>>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
>>> On 01/11/2011 12:49 PM, Bhupesh Sharma wrote:
> ...
>>>> +static int c_can_close(struct net_device *dev) {
>>>> +   struct c_can_priv *priv = netdev_priv(dev);
>>>> +
>>>> +   netif_stop_queue(dev);
>>>> +   napi_disable(&priv->napi);
>>>> +   c_can_stop(dev);
>>>> +   free_irq(dev->irq, dev);
>>>> +   close_candev(dev);
>>>> +
>>>> +   return 0;
>>>> +}
>>>> +
>>>> +struct net_device *alloc_c_can_dev(void)
>>>
>>> Please model after alloc_sja1000_dev:
>>>
>>> struct net_device *alloc_sja1000dev(int sizeof_priv)
>>>
>>> The private for the _user_ of alloc_c_can_dev is behind the sja1000
>>> private, so you can get rid of the void *priv member in the struct
>>> c_can_priv. (see below)
>>
>> Ok.
>> But Wolfgang also suggested to use the *priv* member inside c_can_priv struct
>> for board specific details. In my c_can platform driver I use it to
>> store the *clk* variable. Do I need to change that as well?
> 
> Marc is referring to:
> 
> http://lxr.linux.no/#linux+v2.6.37/drivers/net/can/sja1000/sja1000.c#L582
> 
> But also there "priv->priv" is used to store a pointer to the board
> specific details. Looking to the SJA1000 drivers, only *one* uses
> "sizeof_priv > 0", but many other attach a separately allocated
> structure to "priv->priv". For that reason, I'm fine with your current
> implementation.

Okay fine with me.

A nice cleanup might be to introduce something like this:

static inline void * give_me_my_priv_from_sja1000_priv
	(struct sja1000_priv *priv)
{
	return (void *)(priv + 1);
}

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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

^ permalink raw reply

* [PATCH net-next-2.6] netdev: bfin_mac: Use is_multicast_ether_addr helper
From: Tobias Klauser @ 2011-01-12  9:30 UTC (permalink / raw)
  To: Michael Hennerich, uclinux-dist-devel, netdev

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
 drivers/net/bfin_mac.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c
index 0b9fc51..fe75e7a 100644
--- a/drivers/net/bfin_mac.c
+++ b/drivers/net/bfin_mac.c
@@ -1293,7 +1293,7 @@ static void bfin_mac_multicast_hash(struct net_device *dev)
 		addrs = ha->addr;
 
 		/* skip non-multicast addresses */
-		if (!(*addrs & 1))
+		if (!is_multicast_ether_addr(addrs))
 			continue;
 
 		crc = ether_crc(ETH_ALEN, addrs);
-- 
1.7.0.4


^ permalink raw reply related

* [PATCH net-next-2.6] netdev: ucc_geth: Use is_multicast_ether_addr helper
From: Tobias Klauser @ 2011-01-12  9:31 UTC (permalink / raw)
  To: Li Yang, netdev; +Cc: linuxppc-dev

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
 drivers/net/ucc_geth.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
index 73a3e0d..715e7b4 100644
--- a/drivers/net/ucc_geth.c
+++ b/drivers/net/ucc_geth.c
@@ -2032,7 +2032,7 @@ static void ucc_geth_set_multi(struct net_device *dev)
 			netdev_for_each_mc_addr(ha, dev) {
 				/* Only support group multicast for now.
 				 */
-				if (!(ha->addr[0] & 1))
+				if (!is_multicast_ether_addr(ha->addr))
 					continue;
 
 				/* Ask CPM to run CRC and set bit in
-- 
1.7.0.4


^ permalink raw reply related

* [PATCH net-next-2.6] netdev: tilepro: Use is_multicast_ether_addr helper
From: Tobias Klauser @ 2011-01-12  9:31 UTC (permalink / raw)
  To: Chris Metcalf, netdev

Use is_multicast_ether_addr from linux/etherdevice.h instead of a custom
macro. Also remove the broadcast address check, as it is considered a
multicast address too.

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
 drivers/net/tile/tilepro.c |   10 +---------
 1 files changed, 1 insertions(+), 9 deletions(-)

diff --git a/drivers/net/tile/tilepro.c b/drivers/net/tile/tilepro.c
index 0e6bac5..8d8b67b 100644
--- a/drivers/net/tile/tilepro.c
+++ b/drivers/net/tile/tilepro.c
@@ -142,14 +142,6 @@
 MODULE_AUTHOR("Tilera");
 MODULE_LICENSE("GPL");
 
-
-#define IS_MULTICAST(mac_addr) \
-	(((u8 *)(mac_addr))[0] & 0x01)
-
-#define IS_BROADCAST(mac_addr) \
-	(((u16 *)(mac_addr))[0] == 0xffff)
-
-
 /*
  * Queue of incoming packets for a specific cpu and device.
  *
@@ -795,7 +787,7 @@ static bool tile_net_poll_aux(struct tile_net_cpu *info, int index)
 		/*
 		 * FIXME: Implement HW multicast filter.
 		 */
-		if (!IS_MULTICAST(buf) && !IS_BROADCAST(buf)) {
+		if (!is_multicast_ether_addr(buf)) {
 			/* Filter packets not for our address. */
 			const u8 *mine = dev->dev_addr;
 			filter = compare_ether_addr(mine, buf);
-- 
1.7.0.4


^ permalink raw reply related

* [PATCH] ipv4: devconf: start IPV4_DEVCONF_* from 0
From: Lucian Adrian Grijincu @ 2011-01-12 10:19 UTC (permalink / raw)
  To: netdev, Thomas Graf
  Cc: David S. Miller, Alexey Kuznetsov, Pekka Savola (ipv6),
	James Morris, Hideaki YOSHIFUJI, Patrick McHardy,
	Octavian Purdila, Vlad Dogaru

[-- Attachment #1: Type: text/plain, Size: 1529 bytes --]

The IPV4_DEVCONF_* enums are never exposed to the userspace and it
would make code simpler to remove all the useless (-1) adjustments.

-----

Thomas Graf added an interface that exposes the enums (counting from 1)
for libnl which seems to be the only user of the interface:

commit 9f0f7272ac9506f4c8c05cc597b7e376b0b9f3e4
Author: Thomas Graf <tgraf@infradead.org>
Date:   Tue Nov 16 04:32:48 2010 +0000

    ipv4: AF_INET link address family

The development branch of libnl[0] redefines the enums in userspace
counting from 1 (to mimic the 2.6.37 kernel).

The stable[1] version of libnl does not define or use these enums.

This patch adjusts the number received from userspace to not break the
ABI, but given these circumstances, could we consider the 2.6.37 ABI
unstable and change the only user to start counting from 0?

[0] http://git.kernel.org/?p=libs/netlink/libnl.git
[1] http://git.kernel.org/?p=libs/netlink/libnl-stable.git

-----

    And the moral of the story is that we had better regard — after
    all those centuries! — zero as a most natural number.

    http://www.cs.utexas.edu/users/EWD/transcriptions/EWD08xx/EWD831.html

CC: Edsger Wybe Dijkstra <dijkstra@cs.utexas.edu>
CC: Thomas Graf <tgraf@infradead.org>
Signed-off-by: Lucian Adrian Grijincu <lucian.grijincu@gmail.com>
---
 include/linux/inetdevice.h |    9 +++------
 net/ipv4/devinet.c         |   44 ++++++++++++++++++++++++++------------------
 2 files changed, 29 insertions(+), 24 deletions(-)

[-- Attachment #2: 0003-ipv4-devconf-start-IPV4_DEVCONF_-from-0.patch --]
[-- Type: text/x-patch, Size: 5017 bytes --]

diff --git a/include/linux/inetdevice.h b/include/linux/inetdevice.h
index ae8fdc5..e4d93b2 100644
--- a/include/linux/inetdevice.h
+++ b/include/linux/inetdevice.h
@@ -13,7 +13,7 @@
 
 enum
 {
-	IPV4_DEVCONF_FORWARDING=1,
+	IPV4_DEVCONF_FORWARDING,
 	IPV4_DEVCONF_MC_FORWARDING,
 	IPV4_DEVCONF_PROXY_ARP,
 	IPV4_DEVCONF_ACCEPT_REDIRECTS,
@@ -38,10 +38,9 @@ enum
 	IPV4_DEVCONF_ACCEPT_LOCAL,
 	IPV4_DEVCONF_SRC_VMARK,
 	IPV4_DEVCONF_PROXY_ARP_PVLAN,
-	__IPV4_DEVCONF_MAX
+	IPV4_DEVCONF_MAX
 };
 
-#define IPV4_DEVCONF_MAX (__IPV4_DEVCONF_MAX - 1)
 
 struct ipv4_devconf {
 	void	*sysctl;
@@ -72,20 +71,18 @@ struct in_device {
 	struct rcu_head		rcu_head;
 };
 
-#define IPV4_DEVCONF(cnf, attr) ((cnf).data[IPV4_DEVCONF_ ## attr - 1])
+#define IPV4_DEVCONF(cnf, attr) ((cnf).data[IPV4_DEVCONF_ ## attr])
 #define IPV4_DEVCONF_ALL(net, attr) \
 	IPV4_DEVCONF((*(net)->ipv4.devconf_all), attr)
 
 static inline int ipv4_devconf_get(struct in_device *in_dev, int index)
 {
-	index--;
 	return in_dev->cnf.data[index];
 }
 
 static inline void ipv4_devconf_set(struct in_device *in_dev, int index,
 				    int val)
 {
-	index--;
 	set_bit(index, in_dev->cnf.state);
 	in_dev->cnf.data[index] = val;
 }
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 748cb5b..181d558 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -65,20 +65,20 @@
 
 static struct ipv4_devconf ipv4_devconf = {
 	.data = {
-		[IPV4_DEVCONF_ACCEPT_REDIRECTS - 1] = 1,
-		[IPV4_DEVCONF_SEND_REDIRECTS - 1] = 1,
-		[IPV4_DEVCONF_SECURE_REDIRECTS - 1] = 1,
-		[IPV4_DEVCONF_SHARED_MEDIA - 1] = 1,
+		[IPV4_DEVCONF_ACCEPT_REDIRECTS] = 1,
+		[IPV4_DEVCONF_SEND_REDIRECTS]   = 1,
+		[IPV4_DEVCONF_SECURE_REDIRECTS] = 1,
+		[IPV4_DEVCONF_SHARED_MEDIA]     = 1,
 	},
 };
 
 static struct ipv4_devconf ipv4_devconf_dflt = {
 	.data = {
-		[IPV4_DEVCONF_ACCEPT_REDIRECTS - 1] = 1,
-		[IPV4_DEVCONF_SEND_REDIRECTS - 1] = 1,
-		[IPV4_DEVCONF_SECURE_REDIRECTS - 1] = 1,
-		[IPV4_DEVCONF_SHARED_MEDIA - 1] = 1,
-		[IPV4_DEVCONF_ACCEPT_SOURCE_ROUTE - 1] = 1,
+		[IPV4_DEVCONF_ACCEPT_REDIRECTS]    = 1,
+		[IPV4_DEVCONF_SEND_REDIRECTS]      = 1,
+		[IPV4_DEVCONF_SECURE_REDIRECTS]    = 1,
+		[IPV4_DEVCONF_SHARED_MEDIA]        = 1,
+		[IPV4_DEVCONF_ACCEPT_SOURCE_ROUTE] = 1,
 	},
 };
 
@@ -1304,12 +1304,15 @@ static int inet_validate_link_af(const struct net_device *dev,
 
 	if (tb[IFLA_INET_CONF]) {
 		nla_for_each_nested(a, tb[IFLA_INET_CONF], rem) {
-			int cfgid = nla_type(a);
+			/* Userspace indexes these ids starting from 1.
+			 * This was the way the kernel indexed elements too,
+			 * but now it counts from 0 */
+			int cfgid = nla_type(a) - 1;
 
 			if (nla_len(a) < 4)
 				return -EINVAL;
 
-			if (cfgid <= 0 || cfgid > IPV4_DEVCONF_MAX)
+			if (cfgid < 0 || cfgid >= IPV4_DEVCONF_MAX)
 				return -EINVAL;
 		}
 	}
@@ -1330,8 +1333,13 @@ static int inet_set_link_af(struct net_device *dev, const struct nlattr *nla)
 		BUG();
 
 	if (tb[IFLA_INET_CONF]) {
-		nla_for_each_nested(a, tb[IFLA_INET_CONF], rem)
-			ipv4_devconf_set(in_dev, nla_type(a), nla_get_u32(a));
+		nla_for_each_nested(a, tb[IFLA_INET_CONF], rem) {
+			/* Userspace indexes these ids starting from 1.
+			 * This was the way the kernel indexed elements too,
+			 * but now it counts from 0 */
+			int cfgid = nla_type(a) - 1;
+			ipv4_devconf_set(in_dev, cfgid, nla_get_u32(a));
+		}
 	}
 
 	return 0;
@@ -1449,7 +1457,7 @@ static int ipv4_doint_and_flush(ctl_table *ctl, int write,
 	{ \
 		.procname	= name, \
 		.data		= ipv4_devconf.data + \
-				  IPV4_DEVCONF_ ## attr - 1, \
+				  IPV4_DEVCONF_ ## attr, \
 		.maxlen		= sizeof(int), \
 		.mode		= mval, \
 		.proc_handler	= proc, \
@@ -1470,7 +1478,7 @@ static int ipv4_doint_and_flush(ctl_table *ctl, int write,
 
 static struct devinet_sysctl_table {
 	struct ctl_table_header *sysctl_header;
-	struct ctl_table devinet_vars[__IPV4_DEVCONF_MAX];
+	struct ctl_table devinet_vars[IPV4_DEVCONF_MAX + 1];
 	char *dev_name;
 } devinet_sysctl = {
 	.devinet_vars = {
@@ -1505,6 +1513,7 @@ static struct devinet_sysctl_table {
 					      "force_igmp_version"),
 		DEVINET_SYSCTL_FLUSHING_ENTRY(PROMOTE_SECONDARIES,
 					      "promote_secondaries"),
+		{ }
 	},
 };
 
@@ -1590,8 +1599,7 @@ static void devinet_sysctl_unregister(struct in_device *idev)
 static struct ctl_table ctl_forward_entry[] = {
 	{
 		.procname	= "ip_forward",
-		.data		= &ipv4_devconf.data[
-					IPV4_DEVCONF_FORWARDING - 1],
+		.data		= &ipv4_devconf.data[IPV4_DEVCONF_FORWARDING],
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
 		.proc_handler	= devinet_sysctl_forward,
@@ -1635,7 +1643,7 @@ static __net_init int devinet_init_net(struct net *net)
 		if (tbl == NULL)
 			goto err_alloc_ctl;
 
-		tbl[0].data = &all->data[IPV4_DEVCONF_FORWARDING - 1];
+		tbl[0].data = &all->data[IPV4_DEVCONF_FORWARDING];
 		tbl[0].extra1 = all;
 		tbl[0].extra2 = net;
 #endif

^ permalink raw reply related

* Re: [PATCH V8 08/13] posix clocks: cleanup the CLOCK_DISPTACH macro
From: Thomas Gleixner @ 2011-01-12 11:16 UTC (permalink / raw)
  To: Richard Cochran
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-api-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
	Alan Cox, Arnd Bergmann, Christoph Lameter, David Miller,
	John Stultz, Krzysztof Halasa, Peter Zijlstra, Rodolfo Giometti
In-Reply-To: <20110112073728.GA2935-7KxsofuKt4IfAd9E5cN8NEzG7cXyKsk/@public.gmane.org>

On Wed, 12 Jan 2011, Richard Cochran wrote:

> On Tue, Jan 11, 2011 at 01:57:23PM +0100, Thomas Gleixner wrote:
> > I wonder whether we should be a bit more clever here and create an
> > extra entry for posix_cpu_timers in the posix_clocks array and do the
> > following:
> ...
> > That reduces the code significantly and the MAX_CLOCKS check in
> > clock_get_array_id() replaces the invalid_clock() check in the
> > syscalls as well. It does not matter whether we check this before or
> > after copying stuff from user.
> 
> Well, this does reduce the number of LOC, but the number of
> comparisons is the same. I think the code size would be also no
> different.

Right, It's about lines of code and ever repeated if / else constructs
in the dispatch functions. The number of comparisons is probably the
same, but I'm sure that the binary size will be smaller.

We probably can remove the dispatch inlines that way completely and do
the call directly from the syscall function.
 
> > Adding your new stuff requires just another entry in the array, the
> > setup of the function pointers and the CLOCKFD check in
> > clock_get_array_id(). Everything else just falls in place.
> 
> For me, I am not sure if either version is really very pretty or easy
> to understand.

Well, if we document clock_get_array_id() proper, then it's easier to
follow than 10 dispatch functions which have all the same checks and
comparisons inside.
 
> My instinct is to keep the posix_cpu_X and pc_X functions out of the
> array of static clock id functions, if only to make the distinction
> between the legacy static ids and new dynamic ids clear.
> 
> The conclusion from the "dynamic clock as file" discussion was that we
> don't want to add any more static clock ids, and new clocks should use
> the new, dynamic clock API. So, we should discourage any future
> attempt to add to that function array!

These IDs are not public, they are helpers to make the code simpler,
nothing else. I agree, that we don't want to extend the static ids for
public consumption, but implementation wise we can do that confined to
posix-timers.c.

> Having said that, if you insist on it, I won't mind reworking the
> dispatch code as you suggested.

I'm not insisting. I just saw the repeated if/else constructs and
added the clockfd check mentally :) That's where I started to wonder
about a way to do the same thing with way less lines of code.

> > >  clock_nanosleep_restart(struct restart_block *restart_block)
> > >  {
> > > -	clockid_t which_clock = restart_block->arg0;
> > > -
> > 
> > How does that compile ?
> 
> Because the CLOCK_DISPATCH macro, above, makes no use of the first

Missed that, sorry.

> argument! I have removed the macro for the next round.

Cool.

Thanks,

	tglx

^ permalink raw reply

* Re: [PATCH V8 08/13] posix clocks: cleanup the CLOCK_DISPTACH macro
From: Richard Cochran @ 2011-01-12 12:17 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: linux-kernel, linux-api, netdev, Alan Cox, Arnd Bergmann,
	Christoph Lameter, David Miller, John Stultz, Krzysztof Halasa,
	Peter Zijlstra, Rodolfo Giometti
In-Reply-To: <alpine.LFD.2.00.1101121201080.12146@localhost6.localdomain6>

On Wed, Jan 12, 2011 at 12:16:10PM +0100, Thomas Gleixner wrote:
> 
> We probably can remove the dispatch inlines that way completely and do
> the call directly from the syscall function.

Right, okay. I also wanted to put the whole logic into the syscall
itself. I'll try it that way.

Thanks,

Richard

^ permalink raw reply

* Re: rndis gadget: Inconsistent locking
From: Jarek Poplawski @ 2011-01-12 12:28 UTC (permalink / raw)
  To: David Brownell
  Cc: =?ISO-8859-2?Q?Micha=B3_Nazarewicz?=, Neil Jones, linux-usb,
	netdev
In-Reply-To: <466657.40223.qm@web180306.mail.gq1.yahoo.com>

On 2011-01-10 13:14, David Brownell wrote:
> 
> 
>> I have just retested Michals patch but I have found another
>> lockdep failure.
>>
>> It looks like rndis_msg_parser() can call dev_get_stats> too:
> 
> Can someone provide a fully working patch then?
> Or maybe just revert the one that caused all the
> breakage??
> 
> Rememeber that this driver was working fine for
> years before netdev changes added multiple bugs
> because (at least) they didn't update all callers.

Maybe I miss something, but if it's like Michal described something
like this should be enough (not tested nor compiled).

Jarek P.
---

 drivers/usb/gadget/f_phonet.c |    6 ++++++
 drivers/usb/gadget/u_ether.c  |    6 ++++++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/gadget/f_phonet.c b/drivers/usb/gadget/f_phonet.c
index 3c6e1a0..bef4622 100644
--- a/drivers/usb/gadget/f_phonet.c
+++ b/drivers/usb/gadget/f_phonet.c
@@ -207,6 +207,11 @@ static int pn_net_close(struct net_device *dev)
 	return 0;
 }
 
+static struct net_device_stats *pn_net_stats(struct net_device *dev)
+{
+	return &dev->stats;
+}
+
 static void pn_tx_complete(struct usb_ep *ep, struct usb_request *req)
 {
 	struct f_phonet *fp = ep->driver_data;
@@ -279,6 +284,7 @@ static int pn_net_mtu(struct net_device *dev, int new_mtu)
 static const struct net_device_ops pn_netdev_ops = {
 	.ndo_open	= pn_net_open,
 	.ndo_stop	= pn_net_close,
+	.ndo_get_stats	= pn_net_stats,
 	.ndo_start_xmit	= pn_net_xmit,
 	.ndo_change_mtu	= pn_net_mtu,
 };
diff --git a/drivers/usb/gadget/u_ether.c b/drivers/usb/gadget/u_ether.c
index 1eda968..f21520f 100644
--- a/drivers/usb/gadget/u_ether.c
+++ b/drivers/usb/gadget/u_ether.c
@@ -702,6 +702,11 @@ static int eth_stop(struct net_device *net)
 	return 0;
 }
 
+static struct net_device_stats *eth_get_stats(struct net_device *net)
+{
+	return &net->stats;
+}
+
 /*-------------------------------------------------------------------------*/
 
 /* initial value, changed by "ifconfig usb0 hw ether xx:xx:xx:xx:xx:xx" */
@@ -740,6 +745,7 @@ static struct eth_dev *the_dev;
 static const struct net_device_ops eth_netdev_ops = {
 	.ndo_open		= eth_open,
 	.ndo_stop		= eth_stop,
+	.ndo_get_stats		= eth_get_stats,
 	.ndo_start_xmit		= eth_start_xmit,
 	.ndo_change_mtu		= ueth_change_mtu,
 	.ndo_set_mac_address 	= eth_mac_addr,

^ permalink raw reply related

* Re: rndis gadget: Inconsistent locking
From: Eric Dumazet @ 2011-01-12 13:07 UTC (permalink / raw)
  To: Jarek Poplawski
  Cc: David Brownell, Michał Nazarewicz, Neil Jones,
	linux-usb-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20110112122811.GA9513-8HppEYmqbBCE+EvaaNYduQ@public.gmane.org>

Le mercredi 12 janvier 2011 à 12:28 +0000, Jarek Poplawski a écrit :
> On 2011-01-10 13:14, David Brownell wrote:
> > 
> > 
> >> I have just retested Michals patch but I have found another
> >> lockdep failure.
> >>
> >> It looks like rndis_msg_parser() can call dev_get_stats> too:
> > 
> > Can someone provide a fully working patch then?
> > Or maybe just revert the one that caused all the
> > breakage??
> > 
> > Rememeber that this driver was working fine for
> > years before netdev changes added multiple bugs
> > because (at least) they didn't update all callers.
> 
> Maybe I miss something, but if it's like Michal described something
> like this should be enough (not tested nor compiled).
> 
> Jarek P.
> ---
> 
>  drivers/usb/gadget/f_phonet.c |    6 ++++++
>  drivers/usb/gadget/u_ether.c  |    6 ++++++
>  2 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/gadget/f_phonet.c b/drivers/usb/gadget/f_phonet.c
> index 3c6e1a0..bef4622 100644
> --- a/drivers/usb/gadget/f_phonet.c
> +++ b/drivers/usb/gadget/f_phonet.c
> @@ -207,6 +207,11 @@ static int pn_net_close(struct net_device *dev)
>  	return 0;
>  }
>  
> +static struct net_device_stats *pn_net_stats(struct net_device *dev)
> +{
> +	return &dev->stats;
> +}
> +
>  static void pn_tx_complete(struct usb_ep *ep, struct usb_request *req)
>  {
>  	struct f_phonet *fp = ep->driver_data;
> @@ -279,6 +284,7 @@ static int pn_net_mtu(struct net_device *dev, int new_mtu)
>  static const struct net_device_ops pn_netdev_ops = {
>  	.ndo_open	= pn_net_open,
>  	.ndo_stop	= pn_net_close,
> +	.ndo_get_stats	= pn_net_stats,
>  	.ndo_start_xmit	= pn_net_xmit,
>  	.ndo_change_mtu	= pn_net_mtu,
>  };
> diff --git a/drivers/usb/gadget/u_ether.c b/drivers/usb/gadget/u_ether.c
> index 1eda968..f21520f 100644
> --- a/drivers/usb/gadget/u_ether.c
> +++ b/drivers/usb/gadget/u_ether.c
> @@ -702,6 +702,11 @@ static int eth_stop(struct net_device *net)
>  	return 0;
>  }
>  
> +static struct net_device_stats *eth_get_stats(struct net_device *net)
> +{
> +	return &net->stats;
> +}
> +
>  /*-------------------------------------------------------------------------*/
>  
>  /* initial value, changed by "ifconfig usb0 hw ether xx:xx:xx:xx:xx:xx" */
> @@ -740,6 +745,7 @@ static struct eth_dev *the_dev;
>  static const struct net_device_ops eth_netdev_ops = {
>  	.ndo_open		= eth_open,
>  	.ndo_stop		= eth_stop,
> +	.ndo_get_stats		= eth_get_stats,
>  	.ndo_start_xmit		= eth_start_xmit,
>  	.ndo_change_mtu		= ueth_change_mtu,
>  	.ndo_set_mac_address 	= eth_mac_addr,
> --

Hmm... 

So all net devices in gen_ndis_query_resp() should have a
ndo_get_stats() or ndo_get_stats64() method, not allowed to use
spin_lock_bh() / spin_unlock_bh()

If yes, we should add big fat comments to pn_net_stats()/eth_get_stats() so that nobody
tries to revert your patch ;)



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

^ permalink raw reply

* Re: rndis gadget: Inconsistent locking
From: Jarek Poplawski @ 2011-01-12 13:23 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: David Brownell, Michał Nazarewicz, Neil Jones, linux-usb,
	netdev
In-Reply-To: <1294837632.3981.18.camel@edumazet-laptop>

On Wed, Jan 12, 2011 at 02:07:12PM +0100, Eric Dumazet wrote:
...
> 
> Hmm... 
> 
> So all net devices in gen_ndis_query_resp() should have a
> ndo_get_stats() or ndo_get_stats64() method, not allowed to use
> spin_lock_bh() / spin_unlock_bh()
> 
> If yes, we should add big fat comments to pn_net_stats()/eth_get_stats() so that nobody
> tries to revert your patch ;)

I'm not sure I got your point: my patch could be replaced with
ndo_get_stats64() implementing irq safe locking or by changing
gen_ndis_query_resp() calling context. It's intended as a fast
(compatible) fix.

Jarek P.

^ permalink raw reply

* Re: rndis gadget: Inconsistent locking
From: Eric Dumazet @ 2011-01-12 13:32 UTC (permalink / raw)
  To: Jarek Poplawski
  Cc: David Brownell, Michał Nazarewicz, Neil Jones, linux-usb,
	netdev
In-Reply-To: <20110112132314.GA9920@ff.dom.local>

Le mercredi 12 janvier 2011 à 13:23 +0000, Jarek Poplawski a écrit :
> On Wed, Jan 12, 2011 at 02:07:12PM +0100, Eric Dumazet wrote:
> ...
> > 
> > Hmm... 
> > 
> > So all net devices in gen_ndis_query_resp() should have a
> > ndo_get_stats() or ndo_get_stats64() method, not allowed to use
> > spin_lock_bh() / spin_unlock_bh()
> > 
> > If yes, we should add big fat comments to pn_net_stats()/eth_get_stats() so that nobody
> > tries to revert your patch ;)
> 
> I'm not sure I got your point: my patch could be replaced with
> ndo_get_stats64() implementing irq safe locking or by changing
> gen_ndis_query_resp() calling context. It's intended as a fast
> (compatible) fix.

I was mentioning that we tried in past months to remove useless
ndo_get_stats() methods that were only doing :

return &net->stats;

random commit : b27d50a9ff5cf2775b7a4daf5

Another possibility would be to use u64_stats_sync.h for these txq
counters.

(no locking needed to read counters, only a seqcount fetch/retry)

As a bonus, no overhead on 64bit arches.



^ permalink raw reply

* Re: rndis gadget: Inconsistent locking
From: Jarek Poplawski @ 2011-01-12 13:41 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: David Brownell, Michał Nazarewicz, Neil Jones,
	linux-usb-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1294839140.3981.23.camel@edumazet-laptop>

On Wed, Jan 12, 2011 at 02:32:20PM +0100, Eric Dumazet wrote:
> Le mercredi 12 janvier 2011 ?? 13:23 +0000, Jarek Poplawski a écrit :
> > On Wed, Jan 12, 2011 at 02:07:12PM +0100, Eric Dumazet wrote:
> > ...
> > > 
> > > Hmm... 
> > > 
> > > So all net devices in gen_ndis_query_resp() should have a
> > > ndo_get_stats() or ndo_get_stats64() method, not allowed to use
> > > spin_lock_bh() / spin_unlock_bh()
> > > 
> > > If yes, we should add big fat comments to pn_net_stats()/eth_get_stats() so that nobody
> > > tries to revert your patch ;)
> > 
> > I'm not sure I got your point: my patch could be replaced with
> > ndo_get_stats64() implementing irq safe locking or by changing
> > gen_ndis_query_resp() calling context. It's intended as a fast
> > (compatible) fix.
> 
> I was mentioning that we tried in past months to remove useless
> ndo_get_stats() methods that were only doing :
> 
> return &net->stats;
> 
> random commit : b27d50a9ff5cf2775b7a4daf5
> 
> Another possibility would be to use u64_stats_sync.h for these txq
> counters.
> 
> (no locking needed to read counters, only a seqcount fetch/retry)
> 
> As a bonus, no overhead on 64bit arches.

Well, I'm OK with anything, but this thread gets older and older ;-)

Jarek P.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: rndis gadget: Inconsistent locking
From: Eric Dumazet @ 2011-01-12 13:52 UTC (permalink / raw)
  To: Jarek Poplawski
  Cc: David Brownell, Michał Nazarewicz, Neil Jones,
	linux-usb-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1294839140.3981.23.camel@edumazet-laptop>

Le mercredi 12 janvier 2011 à 14:32 +0100, Eric Dumazet a écrit :
> Le mercredi 12 janvier 2011 à 13:23 +0000, Jarek Poplawski a écrit :
> > On Wed, Jan 12, 2011 at 02:07:12PM +0100, Eric Dumazet wrote:
> > ...
> > > 
> > > Hmm... 
> > > 
> > > So all net devices in gen_ndis_query_resp() should have a
> > > ndo_get_stats() or ndo_get_stats64() method, not allowed to use
> > > spin_lock_bh() / spin_unlock_bh()
> > > 
> > > If yes, we should add big fat comments to pn_net_stats()/eth_get_stats() so that nobody
> > > tries to revert your patch ;)
> > 
> > I'm not sure I got your point: my patch could be replaced with
> > ndo_get_stats64() implementing irq safe locking or by changing
> > gen_ndis_query_resp() calling context. It's intended as a fast
> > (compatible) fix.
> 
> I was mentioning that we tried in past months to remove useless
> ndo_get_stats() methods that were only doing :
> 
> return &net->stats;
> 
> random commit : b27d50a9ff5cf2775b7a4daf5
> 
> Another possibility would be to use u64_stats_sync.h for these txq
> counters.
> 
> (no locking needed to read counters, only a seqcount fetch/retry)
> 
> As a bonus, no overhead on 64bit arches.
> 

Or even better, remove these counters since there is no users left but
ixgbe.

(vlans, tunnels, ... now use percpu stats)



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

^ permalink raw reply

* Re: panic in tg3 driver
From: Stephen Clark @ 2011-01-12 13:53 UTC (permalink / raw)
  To: Matt Carlson; +Cc: Linux Kernel Network Developers, Michael Chan
In-Reply-To: <20110112030652.GA27164@mcarlson.broadcom.com>

On 01/11/2011 10:06 PM, Matt Carlson wrote:
> lspci -vvv -xxx -s 81:00.0

Linux Z1010.netwolves.com 2.6.37 #9 SMP PREEMPT Wed Jan 5 11:14:46 EST 
2011 i686
i686 i386 GNU/Linux

[root@Z1010 ~]# lspci -vvv -xxx -s 81:00.0
81:00.0 Ethernet controller: Broadcom Corporation NetLink BCM5906M Fast 
Ethernet
PCI Express (rev 02)
         Subsystem: Broadcom Corporation Unknown device 9713
         Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- 
ParErr- Stepp
ing- SERR- FastB2B-
         Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- 
<TAbort- <
MAbort- >SERR- <PERR-
         Latency: 0, Cache Line Size: 64 bytes
         Interrupt: pin A routed to IRQ 10
         Region 0: Memory at cfbf0000 (64-bit, non-prefetchable) [size=64K]
         Capabilities: [48] Power Management version 3
                 Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA 
PME(D0-,D1-,D2-,D3hot+
,D3cold+)
                 Status: D0 PME-Enable- DSel=0 DScale=0 PME-
         Capabilities: [50] Vital Product Data
         Capabilities: [58] Vendor Specific Information
         Capabilities: [e8] Message Signalled Interrupts: 64bit+ 
Queue=0/0 Enable-
                 Address: 00000000fee0100c  Data: 4169
         Capabilities: [d0] Express Endpoint IRQ 0
                 Device: Supported: MaxPayload 128 bytes, PhantFunc 0, 
ExtTag+
                 Device: Latency L0s <4us, L1 unlimited
                 Device: AtnBtn- AtnInd- PwrInd-
                 Device: Errors: Correctable- Non-Fatal- Fatal- Unsupported-
                 Device: RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
                 Device: MaxPayload 128 bytes, MaxReadReq 512 bytes
                 Link: Supported Speed 2.5Gb/s, Width x1, ASPM L0s L1, 
Port 0
                 Link: Latency L0s <4us, L1 <64us
                 Link: ASPM Disabled RCB 64 bytes CommClk- ExtSynch-
                 Link: Speed 2.5Gb/s, Width x1
         Capabilities: [100] Advanced Error Reporting
         Capabilities: [13c] Virtual Channel
         Capabilities: [160] Device Serial Number 39-d1-36-fe-ff-b6-02-00
00: e4 14 13 17 06 00 10 00 02 00 00 02 10 00 00 00
10: 04 00 bf cf 00 00 00 00 00 00 00 00 00 00 00 00
20: 00 00 00 00 00 00 00 00 00 00 00 00 e4 14 13 97
30: 00 00 00 00 48 00 00 00 00 00 00 00 0a 01 00 00
40: 00 00 00 00 00 00 00 00 01 50 03 c0 08 00 00 00
50: 03 58 fc 00 78 00 00 00 09 e8 78 00 64 2f 72 64
60: 00 00 00 00 00 00 00 00 00 00 02 c0 00 00 00 00
70: 12 12 00 00 a0 00 00 00 00 00 00 00 00 00 00 00
80: 00 00 00 00 00 00 00 00 00 00 00 00 fe 50 08 24
90: 01 92 00 00 00 00 00 00 00 00 00 00 00 00 00 00
a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
d0: 10 00 01 00 a0 8f 64 00 00 20 10 00 11 6c 03 00
e0: 00 00 11 10 00 00 00 00 05 d0 80 00 0c 10 e0 fe
f0: 00 00 00 00 69 41 00 00 00 00 00 00 00 00 00 00

modprobe tg3

dmesg output:
tg3.c:v3.115 (October 14, 2010)
tg3 0000:81:00.0: PCI INT A -> GSI 16 (level, low) -> IRQ 16
tg3 0000:81:00.0: setting latency timer to 64
tg3 0000:81:00.0: PCI: Disallowing DAC for device
tg3 0000:81:00.0: eth2: Tigon3 [partno(BCM95906) rev c002] (PCI Express) 
MAC addr
ess 00:02:b6:36:d1:39
tg3 0000:81:00.0: eth2: attached PHY is 5906 (10/100Base-TX Ethernet) 
(WireSpeed[
0])
tg3 0000:81:00.0: eth2: RXcsums[1] LinkChgREG[0] MIirq[0] ASF[0] TSOcap[1]
tg3 0000:81:00.0: eth2: dma_rwctrl[76180000] dma_mask[32-bit]
tg3 0000:82:00.0: PCI INT A -> GSI 16 (level, low) -> IRQ 16
tg3 0000:82:00.0: setting latency timer to 64
tg3 0000:82:00.0: PCI: Disallowing DAC for device
tg3 0000:82:00.0: eth3: Tigon3 [partno(BCM95906) rev c002] (PCI Express) 
MAC addr
ess 00:02:b6:36:d1:3a
tg3 0000:82:00.0: eth3: attached PHY is 5906 (10/100Base-TX Ethernet) 
(WireSpeed[
0])
tg3 0000:82:00.0: eth3: RXcsums[1] LinkChgREG[0] MIirq[0] ASF[0] TSOcap[1]
tg3 0000:82:00.0: eth3: dma_rwctrl[76180000] dma_mask[32-bit]

[root@Z1010 ~]# ethtool -i eth2
driver: tg3
version: 3.115
firmware-version: sb v3.03
bus-info: 0000:81:00.0

[root@Z1010 ~]# cat /proc/interrupts
            CPU0
   0:        173   IO-APIC-edge      timer
   1:          2   IO-APIC-edge      i8042
   4:       2864   IO-APIC-edge      serial
   6:          2   IO-APIC-edge      floppy
   8:          0   IO-APIC-edge      rtc0
   9:          0   IO-APIC-fasteoi   acpi
  14:          0   IO-APIC-edge      pata_via
  15:       8100   IO-APIC-edge      pata_via
  16:        984   IO-APIC-fasteoi   eth0
  17:        104   IO-APIC-fasteoi   eth1
  20:          0   IO-APIC-fasteoi   uhci_hcd:usb2
  21:          0   IO-APIC-fasteoi   uhci_hcd:usb4, sata_via
  22:          0   IO-APIC-fasteoi   ehci_hcd:usb1, uhci_hcd:usb3
  23:          0   IO-APIC-fasteoi   uhci_hcd:usb5
NMI:          0   Non-maskable interrupts
LOC:     101963   Local timer interrupts
SPU:          0   Spurious interrupts
PMI:          0   Performance monitoring interrupts
IWI:          0   IRQ work interrupts
RES:          0   Rescheduling interrupts
CAL:          0   Function call interrupts
TLB:          0   TLB shootdowns
TRM:          0   Thermal event interrupts
THR:          0   Threshold APIC interrupts
MCE:          0   Machine check exceptions
MCP:          0   Machine check polls
ERR:          0
MIS:          0

The b44 interfaces are working great.

[root@Z1010 ~]# ifconfig eth2 up
do_IRQ: 0.64 No irq handler for vector (irq -1)

system becomes unresponsive then ususally
reboots.

but it didn't this last time just has become really doggy in
responding
[root@Z1010 ~]#
[root@Z1010 ~]#
[root@Z1010 ~]# ifconfig
eth0      Link encap:Ethernet  HWaddr 00:02:B6:36:D1:37
           inet addr:10.0.129.4  Bcast:10.0.255.255  Mask:255.255.128.0
           inet6 addr: fe80::202:b6ff:fe36:d137/64 Scope:Link
           UP BROADCAST RUNNING MULTICAST  MTU:1500  Metric:1
           RX packets:1025 errors:0 dropped:12 overruns:0 frame:0
           TX packets:6 errors:0 dropped:0 overruns:0 carrier:0
           collisions:0 txqueuelen:1000
           RX bytes:185675 (181.3 KiB)  TX bytes:492 (492.0 b)
           Interrupt:16

eth1      Link encap:Ethernet  HWaddr 00:02:B6:36:D1:38
           inet6 addr: fe80::202:b6ff:fe36:d138/64 Scope:Link
           UP BROADCAST RUNNING MULTICAST  MTU:1500  Metric:1
           RX packets:35 errors:0 dropped:0 overruns:0 frame:0
           TX packets:41 errors:0 dropped:0 overruns:0 carrier:0
           collisions:0 txqueuelen:1000
           RX bytes:2612 (2.5 KiB)  TX bytes:4014 (3.9 KiB)
           Interrupt:17

eth2      Link encap:Ethernet  HWaddr 00:02:B6:36:D1:39
           UP BROADCAST MULTICAST  MTU:1500  Metric:1
           RX packets:0 errors:0 dropped:0 overruns:0 frame:0
           TX packets:0 errors:0 dropped:0 overruns:0 carrier:0
           collisions:0 txqueuelen:1000
           RX bytes:0 (0.0 b)  TX bytes:0 (0.0 b)
           Interrupt:16

lo        Link encap:Local Loopback
           inet addr:127.0.0.1  Mask:255.0.0.0
           inet6 addr: ::1/128 Scope:Host
           UP LOOPBACK RUNNING  MTU:16436  Metric:1
           RX packets:5298 errors:0 dropped:0 overruns:0 frame:0
           TX packets:5298 errors:0 dropped:0 overruns:0 carrier:0
           collisions:0 txqueuelen:0
           RX bytes:475525 (464.3 KiB)  TX bytes:475525 (464.3 KiB)


Message from syslogd@ at Wed Jan 12 08:44:17 2011 ...
localhost kernel: do_IRQ: 0.192 No irq handler for vector (irq -1)

Message from syslogd@ at Wed Jan 12 08:44:17 2011 ...
localhost kernel: do_IRQ: 0.64 No irq handler for vector (irq -1)
[root@Z1010 ~]# cat /proc/interrupts
            CPU0
   0:        173   IO-APIC-edge      timer
   1:          2   IO-APIC-edge      i8042
   4:        821   IO-APIC-edge      serial
   6:          2   IO-APIC-edge      floppy
   8:          0   IO-APIC-edge      rtc0
   9:          2   IO-APIC-fasteoi   acpi
  14:          0   IO-APIC-edge      pata_via
  15:      19522   IO-APIC-edge      pata_via
  16:        256   IO-APIC-fasteoi   eth0, eth2
  17:         54   IO-APIC-fasteoi   eth1
  20:          0   IO-APIC-fasteoi   uhci_hcd:usb2
  21:          0   IO-APIC-fasteoi   uhci_hcd:usb4, sata_via
  22:          0   IO-APIC-fasteoi   ehci_hcd:usb1, uhci_hcd:usb3
  23:          0   IO-APIC-fasteoi   uhci_hcd:usb5
NMI:          0   Non-maskable interrupts
LOC:     116090   Local timer interrupts
SPU:          0   Spurious interrupts
PMI:          0   Performance monitoring interrupts
IWI:          0   IRQ work interrupts
RES:          0   Rescheduling interrupts
CAL:          0   Function call interrupts
TLB:          0   TLB shootdowns
TRM:          0   Thermal event interrupts
THR:          0   Threshold APIC interrupts
MCE:          0   Machine check exceptions
MCP:          0   Machine check polls
ERR:         38
MIS:          2
[root@Z1010 ~]# arp -an

the system has now lost ethernet connectivity via the b44 ports


This is a test system and I can recompile the kernel if there are
any patches you would like me to try out.








^ permalink raw reply

* Re: [PATCH v4 08/10] ARM: mxs: add ocotp read function
From: Sascha Hauer @ 2011-01-12 14:50 UTC (permalink / raw)
  To: Shawn Guo
  Cc: davem, gerg, baruch, eric, bryan.wu, r64343, B32542,
	u.kleine-koenig, lw, w.sang, jamie, jamie, netdev,
	linux-arm-kernel
In-Reply-To: <20110112064711.GG2888@freescale.com>

On Wed, Jan 12, 2011 at 02:47:12PM +0800, Shawn Guo wrote:
> Hi Sascha,
> 
> On Tue, Jan 11, 2011 at 02:31:37PM +0100, Sascha Hauer wrote:
> > On Thu, Jan 06, 2011 at 03:13:16PM +0800, Shawn Guo wrote:
> > > Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> > > ---
> > > Changes for v4:
> > >  - Call cpu_relax() during polling
> > > 
> > > Changes for v2:
> > >  - Add mutex locking for mxs_read_ocotp()
> > >  - Use type size_t for count and i
> > >  - Add comment for clk_enable/disable skipping
> > >  - Add ERROR bit clearing and polling step
> > > 
> > >  arch/arm/mach-mxs/Makefile              |    2 +-
> > >  arch/arm/mach-mxs/include/mach/common.h |    1 +
> > >  arch/arm/mach-mxs/ocotp.c               |   79 +++++++++++++++++++++++++++++++
> > >  3 files changed, 81 insertions(+), 1 deletions(-)
> > >  create mode 100644 arch/arm/mach-mxs/ocotp.c
> > > 
> > > diff --git a/arch/arm/mach-mxs/Makefile b/arch/arm/mach-mxs/Makefile
> > > index 39d3f9c..f23ebbd 100644
> > > --- a/arch/arm/mach-mxs/Makefile
> > > +++ b/arch/arm/mach-mxs/Makefile
> > > @@ -1,5 +1,5 @@
> > >  # Common support
> > > -obj-y := clock.o devices.o gpio.o icoll.o iomux.o system.o timer.o
> > > +obj-y := clock.o devices.o gpio.o icoll.o iomux.o ocotp.o system.o timer.o
> > >  
> > >  obj-$(CONFIG_SOC_IMX23) += clock-mx23.o mm-mx23.o
> > >  obj-$(CONFIG_SOC_IMX28) += clock-mx28.o mm-mx28.o
> > > diff --git a/arch/arm/mach-mxs/include/mach/common.h b/arch/arm/mach-mxs/include/mach/common.h
> > > index 59133eb..cf02552 100644
> > > --- a/arch/arm/mach-mxs/include/mach/common.h
> > > +++ b/arch/arm/mach-mxs/include/mach/common.h
> > > @@ -13,6 +13,7 @@
> > >  
> > >  struct clk;
> > >  
> > > +extern int mxs_read_ocotp(int offset, int count, u32 *values);
> > >  extern int mxs_reset_block(void __iomem *);
> > >  extern void mxs_timer_init(struct clk *, int);
> > >  
> > > diff --git a/arch/arm/mach-mxs/ocotp.c b/arch/arm/mach-mxs/ocotp.c
> > > new file mode 100644
> > > index 0000000..e2d39aa
> > > --- /dev/null
> > > +++ b/arch/arm/mach-mxs/ocotp.c
> > > @@ -0,0 +1,79 @@
> > > +/*
> > > + * Copyright 2010 Freescale Semiconductor, Inc. All Rights Reserved.
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License as published by
> > > + * the Free Software Foundation; either version 2 of the License, or
> > > + * (at your option) any later version.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + * GNU General Public License for more details.
> > > + */
> > > +
> > > +#include <linux/delay.h>
> > > +#include <linux/err.h>
> > > +#include <linux/mutex.h>
> > > +
> > > +#include <mach/mxs.h>
> > > +
> > > +#define BM_OCOTP_CTRL_BUSY		(1 << 8)
> > > +#define BM_OCOTP_CTRL_ERROR		(1 << 9)
> > > +#define BM_OCOTP_CTRL_RD_BANK_OPEN	(1 << 12)
> > > +
> > > +static DEFINE_MUTEX(ocotp_mutex);
> > > +
> > > +int mxs_read_ocotp(unsigned offset, size_t count, u32 *values)
> > > +{
> > > +	void __iomem *ocotp_base = MXS_IO_ADDRESS(MXS_OCOTP_BASE_ADDR);
> > > +	int timeout = 0x400;
> > > +	size_t i;
> > > +
> > > +	mutex_lock(&ocotp_mutex);
> > > +
> > > +	/*
> > > +	 * clk_enable(hbus_clk) for ocotp can be skipped
> > > +	 * as it must be on when system is running.
> > > +	 */
> > > +
> > > +	/* try to clear ERROR bit */
> > > +	__mxs_clrl(BM_OCOTP_CTRL_ERROR, ocotp_base);
> > 
> > This operation does not try to clear the error bit but actually clears
> > it...
> > 
> > > +
> > > +	/* check both BUSY and ERROR cleared */
> > > +	while ((__raw_readl(ocotp_base) &
> > > +		(BM_OCOTP_CTRL_BUSY | BM_OCOTP_CTRL_ERROR)) && --timeout)
> > > +		cpu_relax();
> > 
> > ...which means you do not have to poll the error bit here...
> > 
> > > +
> > > +	if (unlikely(!timeout))
> > > +		goto error_unlock;
> > > +
> > > +	/* open OCOTP banks for read */
> > > +	__mxs_setl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
> > > +
> > > +	/* approximately wait 32 hclk cycles */
> > > +	udelay(1);
> > > +
> > > +	/* poll BUSY bit becoming cleared */
> > > +	timeout = 0x400;
> > > +	while ((__raw_readl(ocotp_base) & BM_OCOTP_CTRL_BUSY) && --timeout)
> > > +		cpu_relax();
> > 
> > ...which means you can factor out a ocotp_wait_busy function and let the
> > code speak instead of the comments.
> > 
> > > +
> > > +	if (unlikely(!timeout))
> > > +		goto error_unlock;
> > > +
> > > +	for (i = 0; i < count; i++, offset += 4)
> > > +		*values++ = __raw_readl(ocotp_base + offset);
> > 
> > The registers in the ocotp are 16 byte aligned. Does it really make
> > sense to provide a function allowing to read the gaps between the
> > registers?
> > 
> Good catch.  The count was added to ease the consecutive otp word
> reading, as there is bank open/close cost for otp read.  What about
> the following changes?
> 
> int mxs_read_ocotp(unsigned offset, size_t otp_word_cnt, u32 *values)
> {
> 	......
> 
> 	for (i = 0; i < otp_word_cnt; i++, offset += 0x10)
> 		*values++ = __raw_readl(ocotp_base + offset);
> 
> 	......
> }

I would rather make a function like this:

static u32 ocotp[0x27];

const u32 *mxs_get_ocotp(void)
{
	static int once = 0;

	if (once)
		return ocotp

	/* bank open */

	for (i = 0; i < 0x27; i++)
		ocotp[i] = readl(ocotp_base + 0x20 + i * 0x10)

	/* bank_close */

	once = 1;

	return ocotp;
}


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply

* [PATCH] net: remove dev_txq_stats_fold()
From: Eric Dumazet @ 2011-01-12 15:02 UTC (permalink / raw)
  To: Jarek Poplawski, David Miller
  Cc: David Brownell, Michał Nazarewicz, Neil Jones, linux-usb,
	netdev, Alexander Duyck, Jeff Kirsher
In-Reply-To: <1294840379.3981.31.camel@edumazet-laptop>

Le mercredi 12 janvier 2011 à 14:52 +0100, Eric Dumazet a écrit :

> Or even better, remove these counters since there is no users left but
> ixgbe.
> 
> (vlans, tunnels, ... now use percpu stats)
> 
> 

Thanks Jarek for the reminder :)

[PATCH] net: remove dev_txq_stats_fold()

After recent changes, (percpu stats on vlan/tunnels...), we dont need
anymore per struct netdev_queue tx_bytes/tx_packets/tx_dropped counters.

Only remaining users are ixgbe, sch_teql & macvlan :

1) ixgbe can be converted to use existing tx_ring counters.

2) macvlan incremented txq->tx_dropped, it can use the
dev->stats.tx_dropped counter.

3) sch_teql : almost revert ab35cd4b8f42 (Use net_device internal stats)
    Now we have ndo_get_stats64(), use it.

This removes a lockdep warning (and possible lockup) in rndis gadget,
calling dev_get_stats() from hard IRQ context.

Ref: http://www.spinics.net/lists/netdev/msg149202.html

Reported-by: Neil Jones <neiljay@gmail.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: Jarek Poplawski <jarkao2@gmail.com>
CC: Alexander Duyck <alexander.h.duyck@intel.com>
CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ixgbe/ixgbe_main.c |   23 ++++++++++++++++-------
 drivers/net/macvtap.c          |    2 +-
 include/linux/netdevice.h      |    5 -----
 net/core/dev.c                 |   29 -----------------------------
 net/sched/sch_teql.c           |   26 +++++++++++++++++++++-----
 5 files changed, 38 insertions(+), 47 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index a060610..602078b 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -6667,8 +6667,6 @@ netdev_tx_t ixgbe_xmit_frame_ring(struct sk_buff *skb,
 			  struct ixgbe_adapter *adapter,
 			  struct ixgbe_ring *tx_ring)
 {
-	struct net_device *netdev = tx_ring->netdev;
-	struct netdev_queue *txq;
 	unsigned int first;
 	unsigned int tx_flags = 0;
 	u8 hdr_len = 0;
@@ -6765,9 +6763,6 @@ netdev_tx_t ixgbe_xmit_frame_ring(struct sk_buff *skb,
 		/* add the ATR filter if ATR is on */
 		if (test_bit(__IXGBE_TX_FDIR_INIT_DONE, &tx_ring->state))
 			ixgbe_atr(tx_ring, skb, tx_flags, protocol);
-		txq = netdev_get_tx_queue(netdev, tx_ring->queue_index);
-		txq->tx_bytes += skb->len;
-		txq->tx_packets++;
 		ixgbe_tx_queue(tx_ring, tx_flags, count, skb->len, hdr_len);
 		ixgbe_maybe_stop_tx(tx_ring, DESC_NEEDED);
 
@@ -6925,8 +6920,6 @@ static struct rtnl_link_stats64 *ixgbe_get_stats64(struct net_device *netdev,
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);
 	int i;
 
-	/* accurate rx/tx bytes/packets stats */
-	dev_txq_stats_fold(netdev, stats);
 	rcu_read_lock();
 	for (i = 0; i < adapter->num_rx_queues; i++) {
 		struct ixgbe_ring *ring = ACCESS_ONCE(adapter->rx_ring[i]);
@@ -6943,6 +6936,22 @@ static struct rtnl_link_stats64 *ixgbe_get_stats64(struct net_device *netdev,
 			stats->rx_bytes   += bytes;
 		}
 	}
+
+	for (i = 0; i < adapter->num_tx_queues; i++) {
+		struct ixgbe_ring *ring = ACCESS_ONCE(adapter->tx_ring[i]);
+		u64 bytes, packets;
+		unsigned int start;
+
+		if (ring) {
+			do {
+				start = u64_stats_fetch_begin_bh(&ring->syncp);
+				packets = ring->stats.packets;
+				bytes   = ring->stats.bytes;
+			} while (u64_stats_fetch_retry_bh(&ring->syncp, start));
+			stats->tx_packets += packets;
+			stats->tx_bytes   += bytes;
+		}
+	}
 	rcu_read_unlock();
 	/* following stats updated by ixgbe_watchdog_task() */
 	stats->multicast	= netdev->stats.multicast;
diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 21845af..5933621 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -585,7 +585,7 @@ err:
 	rcu_read_lock_bh();
 	vlan = rcu_dereference(q->vlan);
 	if (vlan)
-		netdev_get_tx_queue(vlan->dev, 0)->tx_dropped++;
+		vlan->dev->stats.tx_dropped++;
 	rcu_read_unlock_bh();
 
 	return err;
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index be4957c..d971346 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -520,9 +520,6 @@ struct netdev_queue {
 	 * please use this field instead of dev->trans_start
 	 */
 	unsigned long		trans_start;
-	u64			tx_bytes;
-	u64			tx_packets;
-	u64			tx_dropped;
 } ____cacheline_aligned_in_smp;
 
 static inline int netdev_queue_numa_node_read(const struct netdev_queue *q)
@@ -2265,8 +2262,6 @@ extern void		dev_load(struct net *net, const char *name);
 extern void		dev_mcast_init(void);
 extern struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
 					       struct rtnl_link_stats64 *storage);
-extern void		dev_txq_stats_fold(const struct net_device *dev,
-					   struct rtnl_link_stats64 *stats);
 
 extern int		netdev_max_backlog;
 extern int		netdev_tstamp_prequeue;
diff --git a/net/core/dev.c b/net/core/dev.c
index a3ef808..83507c2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5523,34 +5523,6 @@ void netdev_run_todo(void)
 	}
 }
 
-/**
- *	dev_txq_stats_fold - fold tx_queues stats
- *	@dev: device to get statistics from
- *	@stats: struct rtnl_link_stats64 to hold results
- */
-void dev_txq_stats_fold(const struct net_device *dev,
-			struct rtnl_link_stats64 *stats)
-{
-	u64 tx_bytes = 0, tx_packets = 0, tx_dropped = 0;
-	unsigned int i;
-	struct netdev_queue *txq;
-
-	for (i = 0; i < dev->num_tx_queues; i++) {
-		txq = netdev_get_tx_queue(dev, i);
-		spin_lock_bh(&txq->_xmit_lock);
-		tx_bytes   += txq->tx_bytes;
-		tx_packets += txq->tx_packets;
-		tx_dropped += txq->tx_dropped;
-		spin_unlock_bh(&txq->_xmit_lock);
-	}
-	if (tx_bytes || tx_packets || tx_dropped) {
-		stats->tx_bytes   = tx_bytes;
-		stats->tx_packets = tx_packets;
-		stats->tx_dropped = tx_dropped;
-	}
-}
-EXPORT_SYMBOL(dev_txq_stats_fold);
-
 /* Convert net_device_stats to rtnl_link_stats64.  They have the same
  * fields in the same order, with only the type differing.
  */
@@ -5594,7 +5566,6 @@ struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
 		netdev_stats_to_stats64(storage, ops->ndo_get_stats(dev));
 	} else {
 		netdev_stats_to_stats64(storage, &dev->stats);
-		dev_txq_stats_fold(dev, storage);
 	}
 	storage->rx_dropped += atomic_long_read(&dev->rx_dropped);
 	return storage;
diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c
index af9360d..8b82c13 100644
--- a/net/sched/sch_teql.c
+++ b/net/sched/sch_teql.c
@@ -59,6 +59,10 @@ struct teql_master
 	struct net_device *dev;
 	struct Qdisc *slaves;
 	struct list_head master_list;
+	u64	tx_bytes;
+	u64	tx_packets;
+	u64	tx_errors;
+	u64	tx_dropped;
 };
 
 struct teql_sched_data
@@ -274,7 +278,6 @@ static inline int teql_resolve(struct sk_buff *skb,
 static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct teql_master *master = netdev_priv(dev);
-	struct netdev_queue *txq = netdev_get_tx_queue(dev, 0);
 	struct Qdisc *start, *q;
 	int busy;
 	int nores;
@@ -314,8 +317,8 @@ restart:
 					__netif_tx_unlock(slave_txq);
 					master->slaves = NEXT_SLAVE(q);
 					netif_wake_queue(dev);
-					txq->tx_packets++;
-					txq->tx_bytes += length;
+ 					master->tx_packets++;
+ 					master->tx_bytes += length;
 					return NETDEV_TX_OK;
 				}
 				__netif_tx_unlock(slave_txq);
@@ -342,10 +345,10 @@ restart:
 		netif_stop_queue(dev);
 		return NETDEV_TX_BUSY;
 	}
-	dev->stats.tx_errors++;
+	master->tx_errors++;
 
 drop:
-	txq->tx_dropped++;
+	master->tx_dropped++;
 	dev_kfree_skb(skb);
 	return NETDEV_TX_OK;
 }
@@ -398,6 +401,18 @@ static int teql_master_close(struct net_device *dev)
 	return 0;
 }
 
+static struct rtnl_link_stats64 *teql_master_stats64(struct net_device *dev,
+						     struct rtnl_link_stats64 *stats)
+{
+	struct teql_master *m = netdev_priv(dev);
+
+	stats->tx_packets	= m->tx_packets;
+	stats->tx_bytes		= m->tx_bytes;
+	stats->tx_errors	= m->tx_errors;
+	stats->tx_dropped	= m->tx_dropped;
+	return stats;
+}
+
 static int teql_master_mtu(struct net_device *dev, int new_mtu)
 {
 	struct teql_master *m = netdev_priv(dev);
@@ -422,6 +437,7 @@ static const struct net_device_ops teql_netdev_ops = {
 	.ndo_open	= teql_master_open,
 	.ndo_stop	= teql_master_close,
 	.ndo_start_xmit	= teql_master_xmit,
+	.ndo_get_stats64 = teql_master_stats64,
 	.ndo_change_mtu	= teql_master_mtu,
 };
 



^ permalink raw reply related

* [patch] bridge-utils: show selected bridge
From: Anton Danilov @ 2011-01-12 15:29 UTC (permalink / raw)
  To: netdev

diff --git a/brctl/brctl_cmd.c b/brctl/brctl_cmd.c
index d37e99c..d95fe54 100644
--- a/brctl/brctl_cmd.c
+++ b/brctl/brctl_cmd.c
@@ -338,8 +338,14 @@ static int show_bridge(const char *name, void *arg)

 static int br_cmd_show(int argc, char *const* argv)
 {
+       int i = 0;
+
        printf("bridge name\tbridge id\t\tSTP enabled\tinterfaces\n");
-       br_foreach_bridge(show_bridge, NULL);
+       if (argc == 1)
+               br_foreach_bridge(show_bridge, NULL);
+       else
+               for(i = 2; i <= argc; i++)
+                       show_bridge(argv[i - 1], NULL);
        return 0;
 }

@@ -454,7 +460,8 @@ static const struct command commands[] = {
          "<bridge> <port> <cost>\tset path cost" },
        { 3, "setportprio", br_cmd_setportprio,
          "<bridge> <port> <prio>\tset port priority" },
-       { 0, "show", br_cmd_show, "\t\t\tshow a list of bridges" },
+       { 0, "show", br_cmd_show,
+         "[ <bridge> ]\t\tshow a list of bridges" },
        { 1, "showmacs", br_cmd_showmacs,
          "<bridge>\t\tshow a list of mac addrs"},
        { 1, "showstp", br_cmd_showstp,
-- 
Anton.

^ permalink raw reply related

* Re: [PATCH v2 1/2] net_device: add support for network device groups
From: jamal @ 2011-01-12 15:40 UTC (permalink / raw)
  To: Vlad Dogaru; +Cc: netdev, Octavian Purdila
In-Reply-To: <1294763724-9927-2-git-send-email-ddvlad@rosedu.org>

On Tue, 2011-01-11 at 18:35 +0200, Vlad Dogaru wrote:
> Net devices can now be grouped, enabling simpler manipulation from
> userspace. This patch adds a group field to the net_device strucure, as
                                                             ^^^^^
typo

> well as rtnetlink support to query and modify it.
> 


> +	unsigned int group;

Should that be int?

cheers,
jamal


^ permalink raw reply

* Re: [PATCH v2 2/2] netlink: support setting devgroup parameters
From: jamal @ 2011-01-12 15:44 UTC (permalink / raw)
  To: Vlad Dogaru; +Cc: netdev, Octavian Purdila
In-Reply-To: <1294763724-9927-3-git-send-email-ddvlad@rosedu.org>

On Tue, 2011-01-11 at 18:35 +0200, Vlad Dogaru wrote:
> If a rtnetlink request specifies a negative or zero ifindex and has no
> interface name attribute, but has a group attribute, then the chenges
> are made to all the interfaces belonging to the specified group.
> 
> Signed-off-by: Vlad Dogaru <ddvlad@rosedu.org>

Looks good. Please just do some basic tests like setting a few
interfaces to the same group, changing their MTU and setting admin
up then down. If it works add my ACKed-by on both patches.
The only thing that will still generate a lot of noise is the netlink
events that will be generated afterwards for each change on a netdev in
a group i.e a single netlink message..

Maybe we could batch those in a future patch

cheers,
jamal


^ permalink raw reply

* Re: [PATCH v4 08/10] ARM: mxs: add ocotp read function
From: Uwe Kleine-König @ 2011-01-12 16:01 UTC (permalink / raw)
  To: Sascha Hauer
  Cc: Shawn Guo, davem, gerg, baruch, eric, bryan.wu, r64343, B32542,
	lw, w.sang, jamie, jamie, netdev, linux-arm-kernel
In-Reply-To: <20110112145036.GY12078@pengutronix.de>

Hello Sascha,

On Wed, Jan 12, 2011 at 03:50:36PM +0100, Sascha Hauer wrote:
> On Wed, Jan 12, 2011 at 02:47:12PM +0800, Shawn Guo wrote:
> > On Tue, Jan 11, 2011 at 02:31:37PM +0100, Sascha Hauer wrote:
> > > On Thu, Jan 06, 2011 at 03:13:16PM +0800, Shawn Guo wrote:
> > > > Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> > > > ---
> > > > Changes for v4:
> > > >  - Call cpu_relax() during polling
> > > > 
> > > > Changes for v2:
> > > >  - Add mutex locking for mxs_read_ocotp()
> > > >  - Use type size_t for count and i
> > > >  - Add comment for clk_enable/disable skipping
> > > >  - Add ERROR bit clearing and polling step
> > > > 
> > > >  arch/arm/mach-mxs/Makefile              |    2 +-
> > > >  arch/arm/mach-mxs/include/mach/common.h |    1 +
> > > >  arch/arm/mach-mxs/ocotp.c               |   79 +++++++++++++++++++++++++++++++
> > > >  3 files changed, 81 insertions(+), 1 deletions(-)
> > > >  create mode 100644 arch/arm/mach-mxs/ocotp.c
> > > > 
> > > > diff --git a/arch/arm/mach-mxs/Makefile b/arch/arm/mach-mxs/Makefile
> > > > index 39d3f9c..f23ebbd 100644
> > > > --- a/arch/arm/mach-mxs/Makefile
> > > > +++ b/arch/arm/mach-mxs/Makefile
> > > > @@ -1,5 +1,5 @@
> > > >  # Common support
> > > > -obj-y := clock.o devices.o gpio.o icoll.o iomux.o system.o timer.o
> > > > +obj-y := clock.o devices.o gpio.o icoll.o iomux.o ocotp.o system.o timer.o
> > > >  
> > > >  obj-$(CONFIG_SOC_IMX23) += clock-mx23.o mm-mx23.o
> > > >  obj-$(CONFIG_SOC_IMX28) += clock-mx28.o mm-mx28.o
> > > > diff --git a/arch/arm/mach-mxs/include/mach/common.h b/arch/arm/mach-mxs/include/mach/common.h
> > > > index 59133eb..cf02552 100644
> > > > --- a/arch/arm/mach-mxs/include/mach/common.h
> > > > +++ b/arch/arm/mach-mxs/include/mach/common.h
> > > > @@ -13,6 +13,7 @@
> > > >  
> > > >  struct clk;
> > > >  
> > > > +extern int mxs_read_ocotp(int offset, int count, u32 *values);
> > > >  extern int mxs_reset_block(void __iomem *);
> > > >  extern void mxs_timer_init(struct clk *, int);
> > > >  
> > > > diff --git a/arch/arm/mach-mxs/ocotp.c b/arch/arm/mach-mxs/ocotp.c
> > > > new file mode 100644
> > > > index 0000000..e2d39aa
> > > > --- /dev/null
> > > > +++ b/arch/arm/mach-mxs/ocotp.c
> > > > @@ -0,0 +1,79 @@
> > > > +/*
> > > > + * Copyright 2010 Freescale Semiconductor, Inc. All Rights Reserved.
> > > > + *
> > > > + * This program is free software; you can redistribute it and/or modify
> > > > + * it under the terms of the GNU General Public License as published by
> > > > + * the Free Software Foundation; either version 2 of the License, or
> > > > + * (at your option) any later version.
> > > > + *
> > > > + * This program is distributed in the hope that it will be useful,
> > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > > + * GNU General Public License for more details.
> > > > + */
> > > > +
> > > > +#include <linux/delay.h>
> > > > +#include <linux/err.h>
> > > > +#include <linux/mutex.h>
> > > > +
> > > > +#include <mach/mxs.h>
> > > > +
> > > > +#define BM_OCOTP_CTRL_BUSY		(1 << 8)
> > > > +#define BM_OCOTP_CTRL_ERROR		(1 << 9)
> > > > +#define BM_OCOTP_CTRL_RD_BANK_OPEN	(1 << 12)
> > > > +
> > > > +static DEFINE_MUTEX(ocotp_mutex);
> > > > +
> > > > +int mxs_read_ocotp(unsigned offset, size_t count, u32 *values)
> > > > +{
> > > > +	void __iomem *ocotp_base = MXS_IO_ADDRESS(MXS_OCOTP_BASE_ADDR);
> > > > +	int timeout = 0x400;
> > > > +	size_t i;
> > > > +
> > > > +	mutex_lock(&ocotp_mutex);
> > > > +
> > > > +	/*
> > > > +	 * clk_enable(hbus_clk) for ocotp can be skipped
> > > > +	 * as it must be on when system is running.
> > > > +	 */
> > > > +
> > > > +	/* try to clear ERROR bit */
> > > > +	__mxs_clrl(BM_OCOTP_CTRL_ERROR, ocotp_base);
> > > 
> > > This operation does not try to clear the error bit but actually clears
> > > it...
> > > 
> > > > +
> > > > +	/* check both BUSY and ERROR cleared */
> > > > +	while ((__raw_readl(ocotp_base) &
> > > > +		(BM_OCOTP_CTRL_BUSY | BM_OCOTP_CTRL_ERROR)) && --timeout)
> > > > +		cpu_relax();
> > > 
> > > ...which means you do not have to poll the error bit here...
> > > 
> > > > +
> > > > +	if (unlikely(!timeout))
> > > > +		goto error_unlock;
> > > > +
> > > > +	/* open OCOTP banks for read */
> > > > +	__mxs_setl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
> > > > +
> > > > +	/* approximately wait 32 hclk cycles */
> > > > +	udelay(1);
> > > > +
> > > > +	/* poll BUSY bit becoming cleared */
> > > > +	timeout = 0x400;
> > > > +	while ((__raw_readl(ocotp_base) & BM_OCOTP_CTRL_BUSY) && --timeout)
> > > > +		cpu_relax();
> > > 
> > > ...which means you can factor out a ocotp_wait_busy function and let the
> > > code speak instead of the comments.
> > > 
> > > > +
> > > > +	if (unlikely(!timeout))
> > > > +		goto error_unlock;
> > > > +
> > > > +	for (i = 0; i < count; i++, offset += 4)
> > > > +		*values++ = __raw_readl(ocotp_base + offset);
> > > 
> > > The registers in the ocotp are 16 byte aligned. Does it really make
> > > sense to provide a function allowing to read the gaps between the
> > > registers?
> > > 
> > Good catch.  The count was added to ease the consecutive otp word
> > reading, as there is bank open/close cost for otp read.  What about
> > the following changes?
> > 
> > int mxs_read_ocotp(unsigned offset, size_t otp_word_cnt, u32 *values)
> > {
> > 	......
> > 
> > 	for (i = 0; i < otp_word_cnt; i++, offset += 0x10)
> > 		*values++ = __raw_readl(ocotp_base + offset);
> > 
> > 	......
> > }
> 
> I would rather make a function like this:
> 
> static u32 ocotp[0x27];
> 
> const u32 *mxs_get_ocotp(void)
> {
> 	static int once = 0;
> 
> 	if (once)
> 		return ocotp
> 
> 	/* bank open */
> 
> 	for (i = 0; i < 0x27; i++)
> 		ocotp[i] = readl(ocotp_base + 0x20 + i * 0x10)
> 
> 	/* bank_close */
> 
> 	once = 1;
> 
> 	return ocotp;
which is save on UP when it's not called from irq context.

Additionally I suggest a #define for 0x27 and 0x20.

Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

^ permalink raw reply

* [PATCH ethtool 0/7] Documentation fixes
From: Ben Hutchings @ 2011-01-12 16:21 UTC (permalink / raw)
  To: netdev; +Cc: linux-net-drivers

Some minor fixes to the online help and manual page.

Ben Hutchings (7):
  ethtool: Fix spelling and spacing in online help
  ethtool.8: Update date, version, web site reference
  ethtool.8: Substitute version at configure time
  ethtool.8: Fix obvious spelling errors
  ethtool.8: Generalise references to network devices, not Ethernet
  ethtool.8: Reword synopses for consistency and style
  ethtool.8: Fix capitalisation

 configure.ac              |    2 +-
 ethtool.8 => ethtool.8.in |  104 ++++++++++++++++++++++++++++++--------------
 ethtool.c                 |    4 +-
 3 files changed, 74 insertions(+), 36 deletions(-)
 rename ethtool.8 => ethtool.8.in (84%)

-- 
1.7.3.4


-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.


^ permalink raw reply

* [PATCH ethtool 1/7] ethtool: Fix spelling and spacing in online help
From: Ben Hutchings @ 2011-01-12 16:22 UTC (permalink / raw)
  To: netdev; +Cc: linux-net-drivers, Kelly Anderson
In-Reply-To: <1294849266.3946.0.camel@bwh-desktop>

Kelly Anderson <kelly@silka.with-linux.com> pointed out that
the help for --show-nfc was missing a space between two words.

I checked the rest of the help text with aspell and found one
other error.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>

diff --git a/ethtool.c b/ethtool.c
index 63e0ead..1afdfe4 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -224,13 +224,13 @@ static struct option {
 		"		[ offset N ]\n"
 		"		[ length N ]\n"
 		"		[ value N ]\n" },
-    { "-r", "--negotiate", MODE_NWAY_RST, "Restart N-WAY negotation" },
+    { "-r", "--negotiate", MODE_NWAY_RST, "Restart N-WAY negotiation" },
     { "-p", "--identify", MODE_PHYS_ID, "Show visible port identification (e.g. blinking)",
                 "               [ TIME-IN-SECONDS ]\n" },
     { "-t", "--test", MODE_TEST, "Execute adapter self test",
                 "               [ online | offline ]\n" },
     { "-S", "--statistics", MODE_GSTATS, "Show adapter statistics" },
-    { "-n", "--show-nfc", MODE_GNFC, "Show Rx network flow classification"
+    { "-n", "--show-nfc", MODE_GNFC, "Show Rx network flow classification "
 		"options",
 		"		[ rx-flow-hash tcp4|udp4|ah4|sctp4|"
 		"tcp6|udp6|ah6|sctp6 ]\n" },
-- 
1.7.3.4



-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
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 related

* [PATCH ethtool 2/7] ethtool.8: Update date, version, web site reference
From: Ben Hutchings @ 2011-01-12 16:22 UTC (permalink / raw)
  To: netdev; +Cc: linux-net-drivers
In-Reply-To: <1294849266.3946.0.camel@bwh-desktop>

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>

diff --git a/ethtool.8 b/ethtool.8
index 1760924..8c2137b 100644
--- a/ethtool.8
+++ b/ethtool.8
@@ -1,6 +1,7 @@
 .\" -*- nroff -*-
 .\" Copyright 1999 by David S. Miller.  All Rights Reserved.
 .\" Portions Copyright 2001 Sun Microsystems
+.\" Portions Copyright 2007, 2009 Free Software Foundation, Inc.
 .\" This file may be copied under the terms of the GNU Public License.
 .\" 
 .\"	.An - list of n alternative values as in "flav vanilla|strawberry"
@@ -48,7 +49,43 @@
 .\"	\(*HO - hash options
 .\"
 .ds HO \fBm\fP|\fBv\fP|\fBt\fP|\fBs\fP|\fBd\fP|\fBf\fP|\fBn\fP|\fBr\fP...
-.TH ETHTOOL 8 "July 2007" "Ethtool version 6"
+.\" Start URL.
+.de UR
+.  ds m1 \\$1\"
+.  nh
+.  if \\n(mH \{\
+.    \" Start diversion in a new environment.
+.    do ev URL-div
+.    do di URL-div
+.  \}
+..
+.\" End URL.
+.de UE
+.  ie \\n(mH \{\
+.    br
+.    di
+.    ev
+.
+.    \" Has there been one or more input lines for the link text?
+.    ie \\n(dn \{\
+.      do HTML-NS "<a href=""\\*(m1"">"
+.      \" Yes, strip off final newline of diversion and emit it.
+.      do chop URL-div
+.      do URL-div
+\c
+.      do HTML-NS </a>
+.    \}
+.    el \
+.      do HTML-NS "<a href=""\\*(m1"">\\*(m1</a>"
+\&\\$*\"
+.  \}
+.  el \
+\\*(la\\*(m1\\*(ra\\$*\"
+.
+.  hy \\n(HY
+..
+
+.TH ETHTOOL 8 "January 2011" "Ethtool version 2.6.37"
 .SH NAME
 ethtool \- Display or change ethernet card settings
 .SH SYNOPSIS
@@ -777,6 +814,6 @@ Scott Feldman,
 Andi Kleen.
 .SH AVAILABILITY
 .B ethtool
-is available over the Web on the SourceForge site at
-http://sourceforge.net/projects/gkernel/
-
+is available from
+.UR http://www.kernel.org/pub/software/network/ethtool/
+.UE
-- 
1.7.3.4



-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
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 related

* [PATCH ethtool 3/7] ethtool.8: Substitute version at configure time
From: Ben Hutchings @ 2011-01-12 16:22 UTC (permalink / raw)
  To: netdev; +Cc: linux-net-drivers
In-Reply-To: <1294849266.3946.0.camel@bwh-desktop>

Rename ethtool.8 to ethtool.8.in and let autoconf set the version.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>

diff --git a/configure.ac b/configure.ac
index 9bc8c26..a96fd4d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -29,5 +29,5 @@ dnl Checks for library functions.
 AC_HEADER_STDC
 AC_CHECK_FUNCS(socket strtol)
 
-AC_CONFIG_FILES([Makefile ethtool.spec])
+AC_CONFIG_FILES([Makefile ethtool.spec ethtool.8])
 AC_OUTPUT
diff --git a/ethtool.8 b/ethtool.8.in
similarity index 99%
rename from ethtool.8
rename to ethtool.8.in
index 8c2137b..75f63ba 100644
--- a/ethtool.8
+++ b/ethtool.8.in
@@ -85,7 +85,7 @@
 .  hy \\n(HY
 ..
 
-.TH ETHTOOL 8 "January 2011" "Ethtool version 2.6.37"
+.TH ETHTOOL 8 "January 2011" "Ethtool version @VERSION@"
 .SH NAME
 ethtool \- Display or change ethernet card settings
 .SH SYNOPSIS
-- 
1.7.3.4



-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
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 related

* [PATCH ethtool 4/7] ethtool.8: Fix obvious spelling errors
From: Ben Hutchings @ 2011-01-12 16:22 UTC (permalink / raw)
  To: netdev; +Cc: linux-net-drivers
In-Reply-To: <1294849266.3946.0.camel@bwh-desktop>

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>

diff --git a/ethtool.8.in b/ethtool.8.in
index 75f63ba..4554ca0 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -491,12 +491,12 @@ Selects device port.
 .TP
 .A2 autoneg on off
 Specifies whether autonegotiation should be enabled. Autonegotiation 
-is enabled by deafult, but in some network devices may have trouble
+is enabled by default, but in some network devices may have trouble
 with it, so you can disable it if really necessary. 
 .TP
 .BI advertise \ N
 Sets the speed and duplex advertised by autonegotiation.  The argument is
-a hexidecimal value using one or a combination of the following values:
+a hexadecimal value using one or a combination of the following values:
 .RS
 .PD 0
 .TP 3
-- 
1.7.3.4



-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
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 related


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