Netdev List
 help / color / mirror / Atom feed
* RE: [PATCH linux-2.6.35-rc3] ks8842 driver
From: Choi, David @ 2010-07-08 19:01 UTC (permalink / raw)
  To: Simon Horman; +Cc: netdev, Li, Charles
In-Reply-To: <20100708075245.GA21371@verge.net.au>

Hi Simon,

Thank you for your comments.

The original ks8842 driver is designed to work on the customized bus
interface based on an FPGA. This patch is intended to address the more
commonly used generic bus interface available on the majority of SoC in
the market.

It is unlikely that for a system to use both FPGA based and generic bus
interface for ks8842, I am quite certain that those 2 devices are used
mutual exclusively.

Regards,
David J. Choi


-----Original Message-----
From: Simon Horman [mailto:horms@verge.net.au] 
Sent: Thursday, July 08, 2010 12:53 AM
To: Choi, David
Cc: netdev@vger.kernel.org; Li, Charles
Subject: Re: [PATCH linux-2.6.35-rc3] ks8842 driver

On Wed, Jul 07, 2010 at 08:24:33AM -0700, Choi, David wrote:
> To whom it may have concerned,
> 
> It seems that there are differences between Micrel ks8842 device and
Timberdale FPGA based device with generic bus interface. In order to
check the differences, I have sent several times but have not received
any response. This patch is to support ks8841/ks8842 device from Micrel.

Is it desirable that the code can never be compiled to work with both
devices?
Is this code likely to be included in a generic/distro kernel?

> 
> From: David J. Choi <david.choi@micrel.com>
> 
> Body of the explanation: 
>    -support 16bit and 32bit bus width.
>    -add device reset for ks8842/8841 Micrel device.
>    -set 100Mbps as a default for Micrel device.
>    -set MAC address in both MAC/Switch layer with different sequence
for Micrel device, 
>     as mentioned in data sheet.
> 
> Signed-off-by: David J. Choi <david.choi@micrel.com>
> 
> ---
> --- linux-2.6.35-rc3/drivers/net/ks8842.c.orig	2010-07-01
16:26:50.000000000 -0700
> +++ linux-2.6.35-rc3/drivers/net/ks8842.c	2010-07-07
07:41:03.000000000 -0700
> @@ -18,6 +18,7 @@
>  
>  /* Supports:
>   * The Micrel KS8842 behind the timberdale FPGA
> + * The genuine Micrel KS8841/42 device with ISA 16/32bit bus
interface
>   */
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> @@ -191,6 +192,12 @@ static inline u32 ks8842_read32(struct k
>  
>  static void ks8842_reset(struct ks8842_adapter *adapter)
>  {
> +#ifdef CONFIG_MICREL_KS884X
> +	ks8842_write16(adapter, 3, 1, REG_GRR);
> +	msleep(10);
> +	iowrite16(0, adapter->hw_addr + REG_GRR);
> +
> +#else
>  	/* The KS8842 goes haywire when doing softare reset
>  	 * a work around in the timberdale IP is implemented to
>  	 * do a hardware reset instead
> @@ -201,6 +208,7 @@ static void ks8842_reset(struct ks8842_a
>  	iowrite16(32, adapter->hw_addr + REG_SELECT_BANK);
>  	iowrite32(0x1, adapter->hw_addr + REG_TIMB_RST);
>  	msleep(20);
> +#endif
>  }
>  
>  static void ks8842_update_link_status(struct net_device *netdev,
> @@ -269,8 +277,11 @@ static void ks8842_reset_hw(struct ks884
>  
>  	/* restart port auto-negotiation */
>  	ks8842_enable_bits(adapter, 49, 1 << 13, REG_P1CR4);
> +
> +#ifndef CONFIG_MICREL_KS884X
>  	/* only advertise 10Mbps */
>  	ks8842_clear_bits(adapter, 49, 3 << 2, REG_P1CR4);
> +#endif
>  
>  	/* Enable the transmitter */
>  	ks8842_enable_tx(adapter);
> @@ -296,6 +307,20 @@ static void ks8842_read_mac_addr(struct 
>  	for (i = 0; i < ETH_ALEN; i++)
>  		dest[ETH_ALEN - i - 1] = ks8842_read8(adapter, 2,
REG_MARL + i);
>  
> +#ifdef CONFIG_MICREL_KS884X
> +	/*
> +		the sequence of saving mac addr between MAC and Switch
is
> +		different.
> +	*/
> +
> +	mac = ks8842_read16(adapter, 2, REG_MARL);
> +	ks8842_write16(adapter, 39, mac, REG_MACAR3);
> +	mac = ks8842_read16(adapter, 2, REG_MARM);
> +	ks8842_write16(adapter, 39, mac, REG_MACAR2);
> +	mac = ks8842_read16(adapter, 2, REG_MARH);
> +	ks8842_write16(adapter, 39, mac, REG_MACAR1);
> +#else
> +
>  	/* make sure the switch port uses the same MAC as the QMU */
>  	mac = ks8842_read16(adapter, 2, REG_MARL);
>  	ks8842_write16(adapter, 39, mac, REG_MACAR1);
> @@ -303,6 +328,7 @@ static void ks8842_read_mac_addr(struct 
>  	ks8842_write16(adapter, 39, mac, REG_MACAR2);
>  	mac = ks8842_read16(adapter, 2, REG_MARH);
>  	ks8842_write16(adapter, 39, mac, REG_MACAR3);
> +#endif
>  }
>  
>  static void ks8842_write_mac_addr(struct ks8842_adapter *adapter, u8
*mac)
> @@ -313,8 +339,13 @@ static void ks8842_write_mac_addr(struct
>  	spin_lock_irqsave(&adapter->lock, flags);
>  	for (i = 0; i < ETH_ALEN; i++) {
>  		ks8842_write8(adapter, 2, mac[ETH_ALEN - i - 1],
REG_MARL + i);
> +#ifdef CONFIG_MICREL_KS884X
> +		ks8842_write8(adapter, 39, mac[ETH_ALEN - i - 1],
> +			REG_MACAR3 + 1 - i);
> +#else
>  		ks8842_write8(adapter, 39, mac[ETH_ALEN - i - 1],
>  			REG_MACAR1 + i);
> +#endif
>  	}
>  	spin_unlock_irqrestore(&adapter->lock, flags);
>  }
> @@ -328,8 +359,12 @@ static int ks8842_tx_frame(struct sk_buf
>  {
>  	struct ks8842_adapter *adapter = netdev_priv(netdev);
>  	int len = skb->len;
> +#ifdef CONFIG_KS884X_16BIT
> +	u16 *ptr16 = (u16 *)skb->data;
> +#else
>  	u32 *ptr = (u32 *)skb->data;
>  	u32 ctrl;
> +#endif
>  
>  	dev_dbg(&adapter->pdev->dev,
>  		"%s: len %u head %p data %p tail %p end %p\n",
> @@ -340,6 +375,18 @@ static int ks8842_tx_frame(struct sk_buf
>  	if (ks8842_tx_fifo_space(adapter) < len + 8)
>  		return NETDEV_TX_BUSY;
>  
> +#ifdef CONFIG_KS884X_16BIT
> +	ks8842_write16(adapter, 17, 0x8000 | 0x100, REG_QMU_DATA_LO);
> +	ks8842_write16(adapter, 17, (u16)len, REG_QMU_DATA_HI);
> +	netdev->stats.tx_bytes += len;
> +
> +	/* copy buffer */
> +	while (len > 0) {
> +		iowrite16(*ptr16++, adapter->hw_addr + REG_QMU_DATA_LO);
> +		iowrite16(*ptr16++, adapter->hw_addr + REG_QMU_DATA_HI);
> +		len -= sizeof(u32);
> +	}
> +#else
>  	/* the control word, enable IRQ, port 1 and the length */
>  	ctrl = 0x8000 | 0x100 | (len << 16);
>  	ks8842_write32(adapter, 17, ctrl, REG_QMU_DATA_LO);
> @@ -352,6 +399,7 @@ static int ks8842_tx_frame(struct sk_buf
>  		len -= sizeof(u32);
>  		ptr++;
>  	}
> +#endif
>  
>  	/* enqueue packet */
>  	ks8842_write16(adapter, 17, 1, REG_TXQCR);
> @@ -364,10 +412,15 @@ static int ks8842_tx_frame(struct sk_buf
>  static void ks8842_rx_frame(struct net_device *netdev,
>  	struct ks8842_adapter *adapter)
>  {
> +#ifdef CONFIG_KS884X_16BIT
> +	u16 status = ks8842_read16(adapter, 17, REG_QMU_DATA_LO);
> +	int len  = (int)ks8842_read16(adapter, 17, REG_QMU_DATA_HI) &
0xffff;
> +#else
>  	u32 status = ks8842_read32(adapter, 17, REG_QMU_DATA_LO);
>  	int len = (status >> 16) & 0x7ff;
>  
>  	status &= 0xffff;
> +#endif
>  
>  	dev_dbg(&adapter->pdev->dev, "%s - rx_data: status: %x\n",
>  		__func__, status);
> @@ -379,13 +432,28 @@ static void ks8842_rx_frame(struct net_d
>  		dev_dbg(&adapter->pdev->dev, "%s, got package, len:
%d\n",
>  			__func__, len);
>  		if (skb) {
> +#ifdef CONFIG_KS884X_16BIT
> +			u16 *data16;
> +#else
>  			u32 *data;
> +#endif
>  
>  			netdev->stats.rx_packets++;
>  			netdev->stats.rx_bytes += len;
>  			if (status & RXSR_MULTICAST)
>  				netdev->stats.multicast++;
>  
> +#ifdef CONFIG_KS884X_16BIT
> +			data16 = (u16 *)skb_put(skb, len);
> +			ks8842_select_bank(adapter, 17);
> +			while (len > 0) {
> +				*data16++ = ioread16(adapter->hw_addr +
> +					REG_QMU_DATA_LO);
> +				*data16++ = ioread16(adapter->hw_addr +
> +					REG_QMU_DATA_HI);
> +				len -= sizeof(u32);
> +			}
> +#else
>  			data = (u32 *)skb_put(skb, len);
>  
>  			ks8842_select_bank(adapter, 17);
> @@ -397,6 +465,7 @@ static void ks8842_rx_frame(struct net_d
>  
>  			skb->protocol = eth_type_trans(skb, netdev);
>  			netif_rx(skb);
> +#endif
>  		} else
>  			netdev->stats.rx_dropped++;
>  	} else {
> --- linux-2.6.35-rc3/drivers/net/Kconfig.orig	2010-07-02
15:52:41.000000000 -0700
> +++ linux-2.6.35-rc3/drivers/net/Kconfig	2010-07-07
07:45:47.000000000 -0700
> @@ -1748,11 +1748,29 @@ config TLAN
>  	  Please email feedback to <torben.mathiasen@compaq.com>.
>  
>  config KS8842
> -	tristate "Micrel KSZ8842"
> +	tristate "Micrel KSZ8841/42 with generic bus interface"
>  	depends on HAS_IOMEM
>  	help
> -	  This platform driver is for Micrel KSZ8842 / KS8842
> -	  2-port ethernet switch chip (managed, VLAN, QoS).
> +	  This platform driver is for KSZ8841(1-port) / KS8842(2-port) 
> +	  ethernet switch chip (managed, VLAN, QoS) from Micrel or
> +	  Timberdale(FPGA).
> +
> +if KS8842
> +config MICREL_KS884X
> +	boolean "KSZ8841/42 device from Micrel"
> +	default n
> +	help
> +	  Say Y to use Micrel device. Otherwise Timberdale(FPGA) device
is 
> +	  selected.
> +
> +config KS884X_16BIT
> +	boolean "16bit bus width"
> +	default y
> +	help
> +	  This option specifies 16bit or 32bit bus interface. Say Y to
use 
> +	  16bit bus. Otherwise 32bit bus is selected.
> +
> +endif # KS8842
>  
>  config KS8851
>         tristate "Micrel KS8851 SPI"
> ---
> --
> 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: [PATCH v3] vlan: allow TSO setting on vlan interfaces
From: Ben Hutchings @ 2010-07-08 18:25 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: David Miller, netdev, Patrick McHardy
In-Reply-To: <1278607037.2651.57.camel@edumazet-laptop>

On Thu, 2010-07-08 at 18:37 +0200, Eric Dumazet wrote:
> When we need to shape traffic using low speeds, we need to
> disable tso on network interface :
> 
> ethtool -K eth0.2240 tso off
> 
> It seems vlan interfaces miss the set_tso() ethtool method.
> 
> Before enabling TSO, we must check real device supports 
> TSO for VLAN-tagged packets and enables TSO.
> 
> Note that a TSO change on real device propagates TSO setting
> on all vlans, even if admin selected a different TSO setting.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>

> ---
>  net/8021q/vlan_dev.c |   20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> index 7865a4c..a1b8171 100644
> --- a/net/8021q/vlan_dev.c
> +++ b/net/8021q/vlan_dev.c
> @@ -836,12 +836,32 @@ static struct rtnl_link_stats64 *vlan_dev_get_stats64(struct net_device *dev, st
>  	return stats;
>  }
>  
> +static int vlan_ethtool_set_tso(struct net_device *dev, u32 data)
> +{
> +       if (data) {
> +		struct net_device *real_dev = vlan_dev_info(dev)->real_dev;
> +
> +		/* Underlying device must support TSO for VLAN-tagged packets
> +		 * and must have TSO enabled now.
> +		 */
> +		if (!(real_dev->vlan_features & NETIF_F_TSO))
> +			return -EOPNOTSUPP;
> +		if (!(real_dev->features & NETIF_F_TSO))
> +			return -EINVAL;
> +		dev->features |= NETIF_F_TSO;
> +	} else {
> +		dev->features &= ~NETIF_F_TSO;
> +	}
> +	return 0;
> +}
> +
>  static const struct ethtool_ops vlan_ethtool_ops = {
>  	.get_settings	        = vlan_ethtool_get_settings,
>  	.get_drvinfo	        = vlan_ethtool_get_drvinfo,
>  	.get_link		= ethtool_op_get_link,
>  	.get_rx_csum		= vlan_ethtool_get_rx_csum,
>  	.get_flags		= vlan_ethtool_get_flags,
> +	.set_tso                = vlan_ethtool_set_tso,
>  };
>  
>  static const struct net_device_ops vlan_netdev_ops = {
> 
> 
-- 
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

* Re: [PATCH net-next] cxgb4vf: Implement "Unhandled Interrupts" statistic
From: Casey Leedom @ 2010-07-08 18:19 UTC (permalink / raw)
  To: netdev
In-Reply-To: <201007081107.45287.leedom@chelsio.com>

  By the way, if there are _any_ problems with the patch, please ignore it and 
ball me out.  I've tried my hardest to make it correct and follow the correct 
format but if I'm still not doing it correctly that's my problem to address.  I 
appreciate your patience as I grind my way through this learning curve!

Casey

^ permalink raw reply

* [PATCH net-next] cxgb4vf: Implement "Unhandled Interrupts" statistic
From: Casey Leedom @ 2010-07-08 18:07 UTC (permalink / raw)
  To: netdev

>From 6c9cc1b5441afb32ecfcd10ac3060452e3c4df6f Mon Sep 17 00:00:00 2001
From: Casey Leedom <leedom@chelsio.com>
Date: Thu, 8 Jul 2010 10:05:48 -0700
Subject: [PATCH net-next] cxgb4vf: Implement "Unhandled Interrupts" statistic

Implement "Unhandled Interrupts" statistic so we can detect when the
hardware tells us that it things we have work to do but we don't find
anything ...

Signed-off-by: Casey Leedom <leedom@chelsio.com>
---
 drivers/net/cxgb4vf/cxgb4vf_main.c |    8 +++++---
 drivers/net/cxgb4vf/sge.c          |    3 +++
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/cxgb4vf/cxgb4vf_main.c b/drivers/net/cxgb4vf/cxgb4vf_main.c
index bd73ff5..e988031 100644
--- a/drivers/net/cxgb4vf/cxgb4vf_main.c
+++ b/drivers/net/cxgb4vf/cxgb4vf_main.c
@@ -1790,7 +1790,7 @@ static int sge_qstats_show(struct seq_file *seq, void *v)
 		  (rxq[qs].rspq.netdev
 		   ? rxq[qs].rspq.netdev->name
 		   : "N/A"));
-		R3("u", "RspQNullInts", rspq.unhandled_irqs);
+		R3("u", "RspQNullInts:", rspq.unhandled_irqs);
 		R("RxPackets:", stats.pkts);
 		R("RxCSO:", stats.rx_cso);
 		R("VLANxtract:", stats.vlan_ex);
@@ -1814,14 +1814,16 @@ static int sge_qstats_show(struct seq_file *seq, void *v)
 		const struct sge_rspq *evtq = &adapter->sge.fw_evtq;
 
 		seq_printf(seq, "%-8s %16s\n", "QType:", "FW event queue");
-		/* no real response queue statistics available to display */
+		seq_printf(seq, "%-16s %8u\n", "RspQNullInts:",
+			   evtq->unhandled_irqs);
 		seq_printf(seq, "%-16s %8u\n", "RspQ CIdx:", evtq->cidx);
 		seq_printf(seq, "%-16s %8u\n", "RspQ Gen:", evtq->gen);
 	} else if (r == 1) {
 		const struct sge_rspq *intrq = &adapter->sge.intrq;
 
 		seq_printf(seq, "%-8s %16s\n", "QType:", "Interrupt Queue");
-		/* no real response queue statistics available to display */
+		seq_printf(seq, "%-16s %8u\n", "RspQNullInts:",
+			   intrq->unhandled_irqs);
 		seq_printf(seq, "%-16s %8u\n", "RspQ CIdx:", intrq->cidx);
 		seq_printf(seq, "%-16s %8u\n", "RspQ Gen:", intrq->gen);
 	}
diff --git a/drivers/net/cxgb4vf/sge.c b/drivers/net/cxgb4vf/sge.c
index 3a7c02f..5c6663a 100644
--- a/drivers/net/cxgb4vf/sge.c
+++ b/drivers/net/cxgb4vf/sge.c
@@ -1776,6 +1776,9 @@ static int napi_rx_handler(struct napi_struct *napi, int budget)
 	} else
 		intr_params = QINTR_TIMER_IDX(SGE_TIMER_UPD_CIDX);
 
+	if (unlikely(work_done == 0))
+		rspq->unhandled_irqs++;
+
 	t4_write_reg(rspq->adapter,
 		     T4VF_SGE_BASE_ADDR + SGE_VF_GTS,
 		     CIDXINC(work_done) |
-- 
1.7.0.4


^ permalink raw reply related

* Re: Bug handling devices with weird names
From: Stephen Hemminger @ 2010-07-08 17:27 UTC (permalink / raw)
  To: Martín Ferrari; +Cc: netdev, Mathieu Lacage
In-Reply-To: <AANLkTinBvzfdiAldJkXfH_vOkCv_cK7hBhjmH1l2vHTd@mail.gmail.com>

On Thu, 8 Jul 2010 19:08:07 +0200
Martín Ferrari <martin.ferrari@gmail.com> wrote:

> According to dev_valid_name (net/core/dev.c), a valid device name is
> one that doesn't include spaces, slashes, and is not "." or "..". But
> if I create a device called "foo:", some operations fail:
> 
> # ip link add name foo: type dummy
> # ip link list foo:
> 155: foo:: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN ioctl(SIOCGIFXQLEN)
> failed: No such device
> 
>     link/ether 92:7f:4d:0d:8f:57 brd ff:ff:ff:ff:ff:ff
> 
> Strace reveals that the problem occurs when executing an ioctl:
> 
> ioctl(4, SIOCGIFTXQLEN, {ifr_name="foo:", ???}) = -1 ENODEV (No such device)
> 
> ifconfig gets completely lost, of course:
> 
> $ /sbin/ifconfig foo:
> foo:: error fetching interface information: Device not found
> 
> ioctl(5, SIOCGIFFLAGS, {ifr_name="foo:", ???}) = -1 ENODEV (No such device)
> 
> 
> So, is this fixable or iproute should stop using the ioctl interface?
> (dunno if netlink provides everything already)

Colons are used for the old IP aliasing. IP aliasing was an older way of handling
multiple addresses per interface, and is not necessary anymore. The syntax is
retained for legacy compatibility.

^ permalink raw reply

* Bug handling devices with weird names
From: Martín Ferrari @ 2010-07-08 17:08 UTC (permalink / raw)
  To: netdev; +Cc: Mathieu Lacage

According to dev_valid_name (net/core/dev.c), a valid device name is
one that doesn't include spaces, slashes, and is not "." or "..". But
if I create a device called "foo:", some operations fail:

# ip link add name foo: type dummy
# ip link list foo:
155: foo:: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN ioctl(SIOCGIFXQLEN)
failed: No such device

    link/ether 92:7f:4d:0d:8f:57 brd ff:ff:ff:ff:ff:ff

Strace reveals that the problem occurs when executing an ioctl:

ioctl(4, SIOCGIFTXQLEN, {ifr_name="foo:", ???}) = -1 ENODEV (No such device)

ifconfig gets completely lost, of course:

$ /sbin/ifconfig foo:
foo:: error fetching interface information: Device not found

ioctl(5, SIOCGIFFLAGS, {ifr_name="foo:", ???}) = -1 ENODEV (No such device)


So, is this fixable or iproute should stop using the ioctl interface?
(dunno if netlink provides everything already)

-- 
Martín Ferrari

^ permalink raw reply

* Re: [Bugme-new] [Bug 16257] New: sysfs changes break hwsim and bnep drivers
From: Eric W. Biederman @ 2010-07-08 16:55 UTC (permalink / raw)
  To: Greg KH
  Cc: Andrew Morton, Rafael J. Wysocki, Maciej W. Rozycki, Kay Sievers,
	Johannes Berg, Greg KH, netdev
In-Reply-To: <20100622035631.GA3755@suse.de>

Greg KH <gregkh@suse.de> writes:

> Does this only show up if you enable network namespaces?  Or is it a
> problem with the "normal" kernel configuration of no network namespaces?

It is a problem with the normal kernel configuration.  In that case
we have a single network namespace.  

> The only thing that changed here was your network namespace work, what
> caused this problem to show up?  Was it bisectable down to a single
> patch?

The problem is that when we remove symlinks we look at the target of
the symlink to see which tag to remove it in it's parent directory.
Because the target of the symlink was not in a class directory because
of that crazy class on class stacking exception in get_device_parent
we fail to find the symlink when we attempt to remove it.

My one line patch to get_device_parent really does fix this.  I have
just sent you a patch to prevent the creation of these crazy symlinks
in sysfs, which at least clearly isolates this to a handful of drivers.

> Classes on top of classes should never have originally worked, I guess
> we just let them slide by accident, and we can go and fix them up as
> they are found.  But for now, for .35, it would be good to find the root
> cause of the problem here.  It might be as simple as putting a
> CONFIG_BROKEN on network namespaces until we get this working, right?

Nope, it isn't as simple as disabling network namespaces.

For the mac80211_hwsim we can just remove the device parent, and all will
be well for the moment.

For the bnep code we could also not set the parent but I don't know if
that would have undesirable complications for power management or not.
I don't have a bluetooth attached network device so I don't even know how
to test that code.

My preferred fix is the one liner I set you to get_device_parent.  It
is no worse than the magic device_is_not_partition checks then we
already have in sysfs.  Especially when it is a bug that
get_device_parent does that early return in the first place.  To fix
this we will have to introduce the missing subdirectory one way or
another.

With SYSFS_DEPRECATED enabled I don't believe this problem can actually
happen, as all network devices are placed immediately under
/sys/class/net/ in sysfs.

Eric

^ permalink raw reply

* [RFC][PATCH] mac80211_hwsim: No parent is better than an illegimate one.
From: Eric W. Biederman @ 2010-07-08 16:37 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Andrew Morton, Rafael J. Wysocki, Maciej W. Rozycki, Kay Sievers,
	Johannes Berg, Greg KH, Greg KH <gregkh@suse.de> netdev
In-Reply-To: <m1630q7x5v.fsf_-_@fess.ebiederm.org>


Don't call SET_IEEE80211_DEV.  This weakens the connections between
the phy files in sysfs slightly but otherwise it makes the driver work
in the face of tagged sysfs support.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---

Johannes does this change look usable?

 drivers/net/wireless/mac80211_hwsim.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 6f8cb3e..b387222 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -1282,7 +1282,6 @@ static int __init init_mac80211_hwsim(void)
 		}
 		data->dev->driver = &mac80211_hwsim_driver;
 
-		SET_IEEE80211_DEV(hw, data->dev);
 		addr[3] = i >> 8;
 		addr[4] = i;
 		memcpy(data->addresses[0].addr, addr, ETH_ALEN);
-- 
1.6.5.2.143.g8cc62


^ permalink raw reply related

* [PATCH v3] vlan: allow TSO setting on vlan interfaces
From: Eric Dumazet @ 2010-07-08 16:37 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: David Miller, netdev, Patrick McHardy
In-Reply-To: <1278604985.2651.35.camel@edumazet-laptop>

When we need to shape traffic using low speeds, we need to
disable tso on network interface :

ethtool -K eth0.2240 tso off

It seems vlan interfaces miss the set_tso() ethtool method.

Before enabling TSO, we must check real device supports 
TSO for VLAN-tagged packets and enables TSO.

Note that a TSO change on real device propagates TSO setting
on all vlans, even if admin selected a different TSO setting.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 net/8021q/vlan_dev.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 7865a4c..a1b8171 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -836,12 +836,32 @@ static struct rtnl_link_stats64 *vlan_dev_get_stats64(struct net_device *dev, st
 	return stats;
 }
 
+static int vlan_ethtool_set_tso(struct net_device *dev, u32 data)
+{
+       if (data) {
+		struct net_device *real_dev = vlan_dev_info(dev)->real_dev;
+
+		/* Underlying device must support TSO for VLAN-tagged packets
+		 * and must have TSO enabled now.
+		 */
+		if (!(real_dev->vlan_features & NETIF_F_TSO))
+			return -EOPNOTSUPP;
+		if (!(real_dev->features & NETIF_F_TSO))
+			return -EINVAL;
+		dev->features |= NETIF_F_TSO;
+	} else {
+		dev->features &= ~NETIF_F_TSO;
+	}
+	return 0;
+}
+
 static const struct ethtool_ops vlan_ethtool_ops = {
 	.get_settings	        = vlan_ethtool_get_settings,
 	.get_drvinfo	        = vlan_ethtool_get_drvinfo,
 	.get_link		= ethtool_op_get_link,
 	.get_rx_csum		= vlan_ethtool_get_rx_csum,
 	.get_flags		= vlan_ethtool_get_flags,
+	.set_tso                = vlan_ethtool_set_tso,
 };
 
 static const struct net_device_ops vlan_netdev_ops = {



^ permalink raw reply related

* [PATCH] sysfs: Don't allow the creation of symlinks we can't remove
From: Eric W. Biederman @ 2010-07-08 16:31 UTC (permalink / raw)
  To: Greg KH
  Cc: Andrew Morton, Rafael J. Wysocki, Maciej W. Rozycki, Kay Sievers,
	Johannes Berg, Greg KH, netdev
In-Reply-To: <20100622035631.GA3755@suse.de>


Recently my tagged sysfs support revealed a flaw in the device core
that a few rare drivers are running into such that we don't always put
network devices in a class subdirectory named net/.

Since we are not creating the class directory the network devices wind
up in a non-tagged directory, but the symlinks to the network devices
from /sys/class/net are in a tagged directory.  All of which works
until we go to remove or rename the symlink.  When we remove or rename
a symlink we look in the namespace of the target of the symlink.
Since the target of the symlink is in a non-tagged sysfs directory we
don't have a namespace to look in, and we fail to remove the symlink.

Detect this problem up front and simply don't create symlinks we won't
be able to remove later.  This prevents symlink leakage and fails in
a much clearer and more understandable way.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 fs/sysfs/symlink.c |   23 ++++++++++++++++++-----
 1 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
index f71246b..44bca5f 100644
--- a/fs/sysfs/symlink.c
+++ b/fs/sysfs/symlink.c
@@ -28,6 +28,7 @@ static int sysfs_do_create_link(struct kobject *kobj, struct kobject *target,
 	struct sysfs_dirent *target_sd = NULL;
 	struct sysfs_dirent *sd = NULL;
 	struct sysfs_addrm_cxt acxt;
+	enum kobj_ns_type ns_type;
 	int error;
 
 	BUG_ON(!name);
@@ -58,16 +59,28 @@ static int sysfs_do_create_link(struct kobject *kobj, struct kobject *target,
 	if (!sd)
 		goto out_put;
 
-	if (sysfs_ns_type(parent_sd))
+	ns_type = sysfs_ns_type(parent_sd);
+	if (ns_type)
 		sd->s_ns = target->ktype->namespace(target);
 	sd->s_symlink.target_sd = target_sd;
 	target_sd = NULL;	/* reference is now owned by the symlink */
 
 	sysfs_addrm_start(&acxt, parent_sd);
-	if (warn)
-		error = sysfs_add_one(&acxt, sd);
-	else
-		error = __sysfs_add_one(&acxt, sd);
+	/* Symlinks must be between directories with the same ns_type */
+	if (ns_type == sysfs_ns_type(sd->s_symlink.target_sd->s_parent)) {
+		if (warn)
+			error = sysfs_add_one(&acxt, sd);
+		else
+			error = __sysfs_add_one(&acxt, sd);
+	} else {
+		error = -EINVAL;
+		WARN(1, KERN_WARNING
+			"sysfs: symlink across ns_types %s/%s -> %s/%s\n",
+			parent_sd->s_name,
+			sd->s_name,
+			sd->s_symlink.target_sd->s_parent->s_name,
+			sd->s_symlink.target_sd->s_name);
+	}
 	sysfs_addrm_finish(&acxt);
 
 	if (error)
-- 
1.6.5.2.143.g8cc62


^ permalink raw reply related

* [PATCH net-next-2.6] tg3: allow TSO on vlan devices
From: Eric Dumazet @ 2010-07-08 16:14 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, Matt Carlson, Michael Chan

Similar to commit 72dccb01e8632aa (bnx2: Update vlan_features)

In order to enable TSO on vlan devices, tg3 needs to update
dev->vlan_features.

Tested on HP NC326m (aka BCM5715S (rev a3))

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 drivers/net/tg3.c |   28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 289cdc5..68329a9 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -12800,6 +12800,13 @@ done:
 
 static struct pci_dev * __devinit tg3_find_peer(struct tg3 *);
 
+static void inline vlan_features_add(struct net_device *dev, unsigned long flags)
+{
+#if TG3_VLAN_TAG_USED
+	dev->vlan_features |= flags;
+#endif
+}
+
 static int __devinit tg3_get_invariants(struct tg3 *tp)
 {
 	static struct pci_device_id write_reorder_chipsets[] = {
@@ -13032,11 +13039,13 @@ static int __devinit tg3_get_invariants(struct tg3 *tp)
 	if (tp->pci_chip_rev_id == CHIPREV_ID_5700_B0)
 		tp->tg3_flags |= TG3_FLAG_BROKEN_CHECKSUMS;
 	else {
+		unsigned long features = NETIF_F_IP_CSUM | NETIF_F_SG | NETIF_F_GRO;
+
 		tp->tg3_flags |= TG3_FLAG_RX_CHECKSUMS;
-		tp->dev->features |= NETIF_F_IP_CSUM | NETIF_F_SG;
 		if (tp->tg3_flags3 & TG3_FLG3_5755_PLUS)
-			tp->dev->features |= NETIF_F_IPV6_CSUM;
-		tp->dev->features |= NETIF_F_GRO;
+			features |= NETIF_F_IPV6_CSUM;
+		tp->dev->features |= features;
+		vlan_features_add(tp->dev, features);
 	}
 
 	/* Determine TSO capabilities */
@@ -14525,20 +14534,25 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
 	 * is off by default, but can be enabled using ethtool.
 	 */
 	if ((tp->tg3_flags2 & TG3_FLG2_HW_TSO) &&
-	    (dev->features & NETIF_F_IP_CSUM))
+	    (dev->features & NETIF_F_IP_CSUM)) {
 		dev->features |= NETIF_F_TSO;
-
+		vlan_features_add(dev, NETIF_F_TSO);
+	}
 	if ((tp->tg3_flags2 & TG3_FLG2_HW_TSO_2) ||
 	    (tp->tg3_flags2 & TG3_FLG2_HW_TSO_3)) {
-		if (dev->features & NETIF_F_IPV6_CSUM)
+		if (dev->features & NETIF_F_IPV6_CSUM) {
 			dev->features |= NETIF_F_TSO6;
+			vlan_features_add(dev, NETIF_F_TSO6);
+		}
 		if ((tp->tg3_flags2 & TG3_FLG2_HW_TSO_3) ||
 		    GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5761 ||
 		    (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5784 &&
 		     GET_CHIP_REV(tp->pci_chip_rev_id) != CHIPREV_5784_AX) ||
 			GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5785 ||
-		    GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_57780)
+		    GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_57780) {
 			dev->features |= NETIF_F_TSO_ECN;
+			vlan_features_add(dev, NETIF_F_TSO_ECN);
+		}
 	}
 
 	if (tp->pci_chip_rev_id == CHIPREV_ID_5705_A1 &&



^ permalink raw reply related

* Re: [PATCH v2] vlan: allow TSO setting on vlan interfaces
From: Eric Dumazet @ 2010-07-08 16:12 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: David Miller, netdev, Patrick McHardy
In-Reply-To: <1278605109.16013.52.camel@achroite.uk.solarflarecom.com>

Le jeudi 08 juillet 2010 à 17:05 +0100, Ben Hutchings a écrit :

> 
> Yes, sorry for the noise.

No problem, it was a good point to recheck anyway.



^ permalink raw reply

* Re: [PATCH v2] vlan: allow TSO setting on vlan interfaces
From: Ben Hutchings @ 2010-07-08 16:05 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: David Miller, netdev, Patrick McHardy
In-Reply-To: <1278604830.2651.32.camel@edumazet-laptop>

On Thu, 2010-07-08 at 18:00 +0200, Eric Dumazet wrote:
> Le jeudi 08 juillet 2010 à 16:49 +0100, Ben Hutchings a écrit :
> > On Thu, 2010-07-08 at 16:43 +0100, Ben Hutchings wrote:
> > > On Thu, 2010-07-08 at 11:39 +0200, Eric Dumazet wrote:
> > > > When we need to shape traffic with low speeds, we need to disable tso on
> > > > network interface :
> > > > 
> > > > ethtool -K eth0.2240 tso off
> > > > 
> > > > It seems vlan interfaces miss the set_tso() ethtool method.
> > > > Propagating tso changes from lower device is not always wanted, some
> > > > vlans want TSO on, others want TSO off.
> > [...]
> > > I think the vlan driver should also have a netdev notifier to handle
> > > feature changes on the underlying device.
> > 
> > To clarify, I think offload features should be disabled on a vlan device
> > if they are later disabled on the underlying device.  Propagating
> > changes to enable features, as you say, might not be wanted.
> 
> OK, but isnt it already done ?
[...]

Yes, sorry for the noise.

Ben.

-- 
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

* Re: [PATCH v2] vlan: allow TSO setting on vlan interfaces
From: Eric Dumazet @ 2010-07-08 16:04 UTC (permalink / raw)
  To: Patrick McHardy; +Cc: Ben Hutchings, David Miller, netdev
In-Reply-To: <4C35F4E4.4070606@trash.net>

Le jeudi 08 juillet 2010 à 17:55 +0200, Patrick McHardy a écrit :

> I agree. The VLAN driver should also propagate feature changes
> upwards itself.

I noticed bonding is also at fault in this area.

bond0.xxx are not noticed of TSO changes on bond0



^ permalink raw reply

* Re: [PATCH v2] vlan: allow TSO setting on vlan interfaces
From: Eric Dumazet @ 2010-07-08 16:03 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: David Miller, netdev, Patrick McHardy
In-Reply-To: <1278603804.16013.24.camel@achroite.uk.solarflarecom.com>

Le jeudi 08 juillet 2010 à 16:43 +0100, Ben Hutchings a écrit :
> On Thu, 2010-07-08 at 11:39 +0200, Eric Dumazet wrote:
> > When we need to shape traffic with low speeds, we need to disable tso on
> > network interface :
> > 
> > ethtool -K eth0.2240 tso off
> > 
> > It seems vlan interfaces miss the set_tso() ethtool method.
> > Propagating tso changes from lower device is not always wanted, some
> > vlans want TSO on, others want TSO off.
> > 
> > Before enabling TSO, we must check real device supports it.
> > 
> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> > ---
> >  net/8021q/vlan_dev.c |   13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> > index c6456cb..870bc53 100644
> > --- a/net/8021q/vlan_dev.c
> > +++ b/net/8021q/vlan_dev.c
> > @@ -838,12 +838,25 @@ static struct rtnl_link_stats64 *vlan_dev_get_stats64(struct net_device *dev)
> >  	return stats;
> >  }
> >  
> > +static int vlan_ethtool_set_tso(struct net_device *dev, u32 data)
> > +{
> > +	if (data) {
> > +		struct net_device *real_dev = vlan_dev_info(dev)->real_dev;
> > +		unsigned long rdev_vfeatures = real_dev->features & real_dev->vlan_features;
> > +
> > +		dev->features |= (NETIF_F_TSO & rdev_vfeatures);
> > +	} else
> > +		dev->features &= ~NETIF_F_TSO;
> > +	return 0;
> > +}
> [...]
> 
> This should not silently ignore attempts to enable TSO.  I think it
> should be something like:
> 
> static int vlan_ethtool_set_tso(struct net_device *dev, u32 data)
> {
> 	if (data) {
> 		struct net_device *real_dev = vlan_dev_info(dev)->real_dev;
> 
> 		/* Underlying device must support TSO for VLAN-tagged packets
> 		 * and must have TSO enabled now. */
> 		if (!(real_dev->vlan_features & NETIF_F_TSO))
> 			return -EOPNOTSUPP;
> 		if (!(real_dev->features & NETIF_F_TSO))
> 			return -EINVAL;
> 
> 		dev->features |= NETIF_F_TSO;
> 	} else {
> 		dev->features &= ~NETIF_F_TSO;
> 	}
> 
> 	return 0;
> }

I agree.

What about other TSO flags like NETIF_F_TSO6 & NETIF_F_TSO_ECN ?

Not sure if we should manipulate them in set_tso()



^ permalink raw reply

* Re: [PATCH v2] vlan: allow TSO setting on vlan interfaces
From: Eric Dumazet @ 2010-07-08 16:00 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: David Miller, netdev, Patrick McHardy
In-Reply-To: <1278604198.16013.35.camel@achroite.uk.solarflarecom.com>

Le jeudi 08 juillet 2010 à 16:49 +0100, Ben Hutchings a écrit :
> On Thu, 2010-07-08 at 16:43 +0100, Ben Hutchings wrote:
> > On Thu, 2010-07-08 at 11:39 +0200, Eric Dumazet wrote:
> > > When we need to shape traffic with low speeds, we need to disable tso on
> > > network interface :
> > > 
> > > ethtool -K eth0.2240 tso off
> > > 
> > > It seems vlan interfaces miss the set_tso() ethtool method.
> > > Propagating tso changes from lower device is not always wanted, some
> > > vlans want TSO on, others want TSO off.
> [...]
> > I think the vlan driver should also have a netdev notifier to handle
> > feature changes on the underlying device.
> 
> To clarify, I think offload features should be disabled on a vlan device
> if they are later disabled on the underlying device.  Propagating
> changes to enable features, as you say, might not be wanted.

OK, but isnt it already done ?

Check vlan_transfer_features() in net/8021q/vlan.c


# ethtool -k eth3|grep tcp-segmentation
tcp-segmentation-offload: on
# ethtool -k eth3.103|grep tcp-segmentation
tcp-segmentation-offload: on

# ethtool -K eth3 tso off

# ethtool -k eth3|grep tcp-segmentation
tcp-segmentation-offload: off
# ethtool -k eth3.103|grep tcp-segmentation
tcp-segmentation-offload: off

# ethtool -K eth3 tso on
# ethtool -k eth3.103|grep tcp-segmentation
tcp-segmentation-offload: on


We should not change it to avoid ORing TSO, it might break some setups.




^ permalink raw reply

* Re: [PATCH v2] vlan: allow TSO setting on vlan interfaces
From: Patrick McHardy @ 2010-07-08 15:55 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: Eric Dumazet, David Miller, netdev
In-Reply-To: <1278604198.16013.35.camel@achroite.uk.solarflarecom.com>

Ben Hutchings wrote:
> On Thu, 2010-07-08 at 16:43 +0100, Ben Hutchings wrote:
>   
>> On Thu, 2010-07-08 at 11:39 +0200, Eric Dumazet wrote:
>>     
>>> When we need to shape traffic with low speeds, we need to disable tso on
>>> network interface :
>>>
>>> ethtool -K eth0.2240 tso off
>>>
>>> It seems vlan interfaces miss the set_tso() ethtool method.
>>> Propagating tso changes from lower device is not always wanted, some
>>> vlans want TSO on, others want TSO off.
>>>       
> [...]
>   
>> I think the vlan driver should also have a netdev notifier to handle
>> feature changes on the underlying device.
>>     
>
> To clarify, I think offload features should be disabled on a vlan device
> if they are later disabled on the underlying device.  Propagating
> changes to enable features, as you say, might not be wanted.

I agree. The VLAN driver should also propagate feature changes
upwards itself.

^ permalink raw reply

* Re: [PATCH v2] vlan: allow TSO setting on vlan interfaces
From: Ben Hutchings @ 2010-07-08 15:49 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: David Miller, netdev, Patrick McHardy
In-Reply-To: <1278603804.16013.24.camel@achroite.uk.solarflarecom.com>

On Thu, 2010-07-08 at 16:43 +0100, Ben Hutchings wrote:
> On Thu, 2010-07-08 at 11:39 +0200, Eric Dumazet wrote:
> > When we need to shape traffic with low speeds, we need to disable tso on
> > network interface :
> > 
> > ethtool -K eth0.2240 tso off
> > 
> > It seems vlan interfaces miss the set_tso() ethtool method.
> > Propagating tso changes from lower device is not always wanted, some
> > vlans want TSO on, others want TSO off.
[...]
> I think the vlan driver should also have a netdev notifier to handle
> feature changes on the underlying device.

To clarify, I think offload features should be disabled on a vlan device
if they are later disabled on the underlying device.  Propagating
changes to enable features, as you say, might not be wanted.

Ben.

-- 
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

* Re: [PATCH v2] vlan: allow TSO setting on vlan interfaces
From: Ben Hutchings @ 2010-07-08 15:43 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: David Miller, netdev, Patrick McHardy
In-Reply-To: <1278581971.2651.10.camel@edumazet-laptop>

On Thu, 2010-07-08 at 11:39 +0200, Eric Dumazet wrote:
> When we need to shape traffic with low speeds, we need to disable tso on
> network interface :
> 
> ethtool -K eth0.2240 tso off
> 
> It seems vlan interfaces miss the set_tso() ethtool method.
> Propagating tso changes from lower device is not always wanted, some
> vlans want TSO on, others want TSO off.
> 
> Before enabling TSO, we must check real device supports it.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
>  net/8021q/vlan_dev.c |   13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> index c6456cb..870bc53 100644
> --- a/net/8021q/vlan_dev.c
> +++ b/net/8021q/vlan_dev.c
> @@ -838,12 +838,25 @@ static struct rtnl_link_stats64 *vlan_dev_get_stats64(struct net_device *dev)
>  	return stats;
>  }
>  
> +static int vlan_ethtool_set_tso(struct net_device *dev, u32 data)
> +{
> +	if (data) {
> +		struct net_device *real_dev = vlan_dev_info(dev)->real_dev;
> +		unsigned long rdev_vfeatures = real_dev->features & real_dev->vlan_features;
> +
> +		dev->features |= (NETIF_F_TSO & rdev_vfeatures);
> +	} else
> +		dev->features &= ~NETIF_F_TSO;
> +	return 0;
> +}
[...]

This should not silently ignore attempts to enable TSO.  I think it
should be something like:

static int vlan_ethtool_set_tso(struct net_device *dev, u32 data)
{
	if (data) {
		struct net_device *real_dev = vlan_dev_info(dev)->real_dev;

		/* Underlying device must support TSO for VLAN-tagged packets
		 * and must have TSO enabled now. */
		if (!(real_dev->vlan_features & NETIF_F_TSO))
			return -EOPNOTSUPP;
		if (!(real_dev->features & NETIF_F_TSO))
			return -EINVAL;

		dev->features |= NETIF_F_TSO;
	} else {
		dev->features &= ~NETIF_F_TSO;
	}

	return 0;
}

I think the vlan driver should also have a netdev notifier to handle
feature changes on the underlying device.

Ben.

-- 
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

* Re: [PATCH 2/4] ll_temac: fix memory leak
From: Grant Likely @ 2010-07-08 15:24 UTC (permalink / raw)
  To: Kulikov Vasiliy
  Cc: Kernel Janitors, David S. Miller, John Linn, Jiri Pirko,
	Brian Hill, netdev
In-Reply-To: <1278591006-3156-1-git-send-email-segooon@gmail.com>

On Thu, Jul 8, 2010 at 6:10 AM, Kulikov Vasiliy <segooon@gmail.com> wrote:
> If of_iomap() or irq_of_parse_and_map() fail then np must be freed.
>
> Signed-off-by: Kulikov Vasiliy <segooon@gmail.com>

Looks correct to me.

g.

> ---
>  drivers/net/ll_temac_main.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/ll_temac_main.c b/drivers/net/ll_temac_main.c
> index 5bca20b..a2da3d7 100644
> --- a/drivers/net/ll_temac_main.c
> +++ b/drivers/net/ll_temac_main.c
> @@ -999,19 +999,22 @@ temac_of_probe(struct of_device *op, const struct of_device_id *match)
>                        dev_dbg(&op->dev, "MEM base: %p\n", lp->sdma_regs);
>                } else {
>                        dev_err(&op->dev, "unable to map DMA registers\n");
> +                       of_node_put(np);
>                        goto err_iounmap;
>                }
>        }
>
>        lp->rx_irq = irq_of_parse_and_map(np, 0);
>        lp->tx_irq = irq_of_parse_and_map(np, 1);
> +
> +       of_node_put(np); /* Finished with the DMA node; drop the reference */
> +
>        if ((lp->rx_irq == NO_IRQ) || (lp->tx_irq == NO_IRQ)) {
>                dev_err(&op->dev, "could not determine irqs\n");
>                rc = -ENOMEM;
>                goto err_iounmap_2;
>        }
>
> -       of_node_put(np); /* Finished with the DMA node; drop the reference */
>
>        /* Retrieve the MAC address */
>        addr = of_get_property(op->dev.of_node, "local-mac-address", &size);
> --
> 1.7.0.4
>
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

^ permalink raw reply

* Re: [PATCH 3/4] ll_temac: free everything on error path
From: Kulikov Vasiliy @ 2010-07-08 14:24 UTC (permalink / raw)
  To: dkirjanov
  Cc: Kernel Janitors, David S. Miller, John Linn, Grant Likely,
	Jiri Pirko, Brian Hill, netdev
In-Reply-To: <AANLkTilMaITOMsn_9MmZ7-zHwpvTd2ZYjj4UUWOloYsn@mail.gmail.com>

> I just sent a patch that fixes this.
Hey, you forgot to CC me/kernel-janitors in your patch.

Anyway,
> /**
>+ *  * temac_dma_bd_release - Release buffer descriptor rings
>+ */
>+static void temac_dma_bd_release(struct net_device *ndev)
>+{
>+   struct temac_local *lp = netdev_priv(ndev);
>+   int i;
>+
>+   for (i = 0; i < RX_BD_NUM; i++) {
>+       if (lp->rx_skb[i]) {
            ^^^^^^^^^^
This can be NULL if kzalloc() failed.
>+           dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys,
>+                   XTE_MAX_JUMBO_FRAME_SIZE, DMA_FROM_DEVICE);
>+           dev_kfree_skb(lp->rx_skb[i]);
>+       }
>+   }
>+   dma_free_coherent(ndev->dev.parent, sizeof(*lp->rx_bd_v) * RX_BD_NUM,
>+           lp->rx_bd_v, lp->rx_bd_p);
>+   dma_free_coherent(ndev->dev.parent, sizeof(*lp->tx_bd_v) * TX_BD_NUM,
>+           lp->tx_bd_v, lp->tx_bd_p);

These two DMA's can be unallocated if dma_alloc_coherent() failed.

I wanted to introduce many labels to divide these error cases because of it.

>+   kfree(lp->rx_skb);
>+}

^ permalink raw reply

* Re: [PATCH net-next-2.6] bnx2: 64 bit stats on all arches
From: Eric Dumazet @ 2010-07-08 14:08 UTC (permalink / raw)
  To: Michael Chan; +Cc: David Miller, netdev, Matthew Carlson
In-Reply-To: <C27F8246C663564A84BB7AB3439772421B842BD250@IRVEXCHCCR01.corp.ad.broadcom.com>

Le jeudi 08 juillet 2010 à 07:00 -0700, Michael Chan a écrit :

> It seems that GET_64BIT_NET_STATS32 is no longer needed.
> Other than that,
> 
> Acked-by: Michael Chan <mchan@broadcom.com>
> 
> Thanks.

Indeed, thanks for the tip !
Here is the updated version.

[PATCH net-next-2.6] bnx2: 64 bit stats on all arches

Now core network is able to handle 64 bit netdevice stats on 32 bit
arches, we can provide them for bnx2, since hardware maintains some 64
bit counters.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Acked-by: Michael Chan <mchan@broadcom.com>
---
 drivers/net/bnx2.c |   25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index 22fa1e9..a203f39 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -6589,36 +6589,25 @@ bnx2_save_stats(struct bnx2 *bp)
 		temp_stats[i] += hw_stats[i];
 }
 
-#define GET_64BIT_NET_STATS64(ctr)				\
-	(unsigned long) ((unsigned long) (ctr##_hi) << 32) +	\
-	(unsigned long) (ctr##_lo)
+#define GET_64BIT_NET_STATS64(ctr)		\
+	(((u64) (ctr##_hi) << 32) + (u64) (ctr##_lo))
 
-#define GET_64BIT_NET_STATS32(ctr)				\
-	(ctr##_lo)
-
-#if (BITS_PER_LONG == 64)
 #define GET_64BIT_NET_STATS(ctr)				\
 	GET_64BIT_NET_STATS64(bp->stats_blk->ctr) +		\
 	GET_64BIT_NET_STATS64(bp->temp_stats_blk->ctr)
-#else
-#define GET_64BIT_NET_STATS(ctr)				\
-	GET_64BIT_NET_STATS32(bp->stats_blk->ctr) +		\
-	GET_64BIT_NET_STATS32(bp->temp_stats_blk->ctr)
-#endif
 
 #define GET_32BIT_NET_STATS(ctr)				\
 	(unsigned long) (bp->stats_blk->ctr +			\
 			 bp->temp_stats_blk->ctr)
 
-static struct net_device_stats *
-bnx2_get_stats(struct net_device *dev)
+static struct rtnl_link_stats64 *
+bnx2_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *net_stats)
 {
 	struct bnx2 *bp = netdev_priv(dev);
-	struct net_device_stats *net_stats = &dev->stats;
 
-	if (bp->stats_blk == NULL) {
+	if (bp->stats_blk == NULL)
 		return net_stats;
-	}
+
 	net_stats->rx_packets =
 		GET_64BIT_NET_STATS(stat_IfHCInUcastPkts) +
 		GET_64BIT_NET_STATS(stat_IfHCInMulticastPkts) +
@@ -8289,7 +8278,7 @@ static const struct net_device_ops bnx2_netdev_ops = {
 	.ndo_open		= bnx2_open,
 	.ndo_start_xmit		= bnx2_start_xmit,
 	.ndo_stop		= bnx2_close,
-	.ndo_get_stats		= bnx2_get_stats,
+	.ndo_get_stats64	= bnx2_get_stats64,
 	.ndo_set_rx_mode	= bnx2_set_rx_mode,
 	.ndo_do_ioctl		= bnx2_ioctl,
 	.ndo_validate_addr	= eth_validate_addr,



^ permalink raw reply related

* Re: [PATCH net-next-2.6] bnx2: 64 bit stats on all arches
From: Michael Chan @ 2010-07-08 14:00 UTC (permalink / raw)
  To: 'Eric Dumazet', David Miller; +Cc: netdev, Matthew Carlson
In-Reply-To: <1278583542.2651.19.camel@edumazet-laptop>

Eric Dumazet wrote:

> Now core network is able to handle 64 bit netdevice stats on 32 bit
> arches, we can provide them for bnx2, since hardware maintains some 64
> bit counters.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
>  drivers/net/bnx2.c |   22 +++++++---------------
>  1 file changed, 7 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
> index 22fa1e9..2975bf9 100644
> --- a/drivers/net/bnx2.c
> +++ b/drivers/net/bnx2.c
> @@ -6589,36 +6589,28 @@ bnx2_save_stats(struct bnx2 *bp)
>               temp_stats[i] += hw_stats[i];
>  }
>
> -#define GET_64BIT_NET_STATS64(ctr)                           \
> -     (unsigned long) ((unsigned long) (ctr##_hi) << 32) +    \
> -     (unsigned long) (ctr##_lo)
> +#define GET_64BIT_NET_STATS64(ctr)           \
> +     (((u64) (ctr##_hi) << 32) + (u64) (ctr##_lo))
>
>  #define GET_64BIT_NET_STATS32(ctr)                           \
>       (ctr##_lo)

It seems that GET_64BIT_NET_STATS32 is no longer needed.
Other than that,

Acked-by: Michael Chan <mchan@broadcom.com>

Thanks.

>
> -#if (BITS_PER_LONG == 64)
>  #define GET_64BIT_NET_STATS(ctr)                             \
>       GET_64BIT_NET_STATS64(bp->stats_blk->ctr) +             \
>       GET_64BIT_NET_STATS64(bp->temp_stats_blk->ctr)
> -#else
> -#define GET_64BIT_NET_STATS(ctr)                             \
> -     GET_64BIT_NET_STATS32(bp->stats_blk->ctr) +             \
> -     GET_64BIT_NET_STATS32(bp->temp_stats_blk->ctr)
> -#endif
>
>  #define GET_32BIT_NET_STATS(ctr)                             \
>       (unsigned long) (bp->stats_blk->ctr +                   \
>                        bp->temp_stats_blk->ctr)
>
> -static struct net_device_stats *
> -bnx2_get_stats(struct net_device *dev)
> +static struct rtnl_link_stats64 *
> +bnx2_get_stats64(struct net_device *dev, struct
> rtnl_link_stats64 *net_stats)
>  {
>       struct bnx2 *bp = netdev_priv(dev);
> -     struct net_device_stats *net_stats = &dev->stats;
>
> -     if (bp->stats_blk == NULL) {
> +     if (bp->stats_blk == NULL)
>               return net_stats;
> -     }
> +
>       net_stats->rx_packets =
>               GET_64BIT_NET_STATS(stat_IfHCInUcastPkts) +
>               GET_64BIT_NET_STATS(stat_IfHCInMulticastPkts) +
> @@ -8289,7 +8281,7 @@ static const struct net_device_ops
> bnx2_netdev_ops = {
>       .ndo_open               = bnx2_open,
>       .ndo_start_xmit         = bnx2_start_xmit,
>       .ndo_stop               = bnx2_close,
> -     .ndo_get_stats          = bnx2_get_stats,
> +     .ndo_get_stats64        = bnx2_get_stats64,
>       .ndo_set_rx_mode        = bnx2_set_rx_mode,
>       .ndo_do_ioctl           = bnx2_ioctl,
>       .ndo_validate_addr      = eth_validate_addr,
>
>
>
>


^ permalink raw reply

* Re: [PATCH 3/4] ll_temac: free everything on error path
From: Denis Kirjanov @ 2010-07-08 13:56 UTC (permalink / raw)
  To: Kulikov Vasiliy
  Cc: Kernel Janitors, David S. Miller, John Linn, Grant Likely,
	Jiri Pirko, Brian Hill, netdev
In-Reply-To: <20100708134651.GA27196@shinshilla>

On Thu, Jul 8, 2010 at 5:46 PM, Kulikov Vasiliy <segooon@gmail.com> wrote:
> On Thu, Jul 08, 2010 at 17:16 +0400, Denis Kirjanov wrote:
>> On Thu, Jul 8, 2010 at 4:10 PM, Kulikov Vasiliy <segooon@gmail.com> wrote:
>> > temac_dma_bd_init() must free all allocated resources: memory, dma, skbs.
>> >
>> > Signed-off-by: Kulikov Vasiliy <segooon@gmail.com>
>> > ---
>> >  drivers/net/ll_temac_main.c |   30 +++++++++++++++++++++++++-----
>> >  1 files changed, 25 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/net/ll_temac_main.c b/drivers/net/ll_temac_main.c
>> > index a2da3d7..38d658a 100644
>> > --- a/drivers/net/ll_temac_main.c
>> > +++ b/drivers/net/ll_temac_main.c
>> > @@ -200,6 +200,7 @@ static int temac_dma_bd_init(struct net_device *ndev)
>> >        struct temac_local *lp = netdev_priv(ndev);
>> >        struct sk_buff *skb;
>> >        int i;
>> > +       int tx_bd_v_size, rx_bd_v_size;
>> >
>> >        lp->rx_skb = kzalloc(sizeof(*lp->rx_skb) * RX_BD_NUM, GFP_KERNEL);
>> >        if (!lp->rx_skb) {
>> > @@ -209,21 +210,23 @@ static int temac_dma_bd_init(struct net_device *ndev)
>> >        }
>> >        /* allocate the tx and rx ring buffer descriptors. */
>> >        /* returns a virtual addres and a physical address. */
>> > +       tx_bd_v_size = sizeof(*lp->tx_bd_v) * TX_BD_NUM;
>> >        lp->tx_bd_v = dma_alloc_coherent(ndev->dev.parent,
>           ^^^^^^^^^^^
>            1st
>> > -                                        sizeof(*lp->tx_bd_v) * TX_BD_NUM,
>> > +                                       tx_bd_v_size,
>> >                                         &lp->tx_bd_p, GFP_KERNEL);
>> >        if (!lp->tx_bd_v) {
>> >                dev_err(&ndev->dev,
>> >                                "unable to allocate DMA TX buffer descriptors");
>> > -               goto out;
>> > +               goto err_rx_skb;
>> >        }
>> > +       rx_bd_v_size = sizeof(*lp->rx_bd_v) * RX_BD_NUM;
>> >        lp->rx_bd_v = dma_alloc_coherent(ndev->dev.parent,
>           ^^^^^^^^^^^
>            2nd
>> > -                                        sizeof(*lp->rx_bd_v) * RX_BD_NUM,
>> > +                                        rx_bd_v_size,
>> >                                         &lp->rx_bd_p, GFP_KERNEL);
>> >        if (!lp->rx_bd_v) {
>> >                dev_err(&ndev->dev,
>> >                                "unable to allocate DMA RX buffer descriptors");
>> > -               goto out;
>> > +               goto err_tx_bd;
>> >        }
>> >
>> >        memset(lp->tx_bd_v, 0, sizeof(*lp->tx_bd_v) * TX_BD_NUM);
>> > @@ -242,7 +245,7 @@ static int temac_dma_bd_init(struct net_device *ndev)
>> >
>> >                if (skb == 0) {
>> >                        dev_err(&ndev->dev, "alloc_skb error %d\n", i);
>> > -                       goto out;
>> > +                       goto err_dma_single;
>> >                }
>> >                lp->rx_skb[i] = skb;
>> >                /* returns physical address of skb->data */
>> > @@ -274,6 +277,23 @@ static int temac_dma_bd_init(struct net_device *ndev)
>> >
>> >        return 0;
>> >
>> > +err_dma_single:
>> > +       for (i = 0; i < RX_BD_NUM; i++) {
>> > +               if (lp->rx_skb[i] == NULL)
>> > +                       break;
>> > +
>> > +               kfree_skb(lp->rx_skb[i]);
>> > +               dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys,
>> > +                               XTE_MAX_JUMBO_FRAME_SIZE, DMA_FROM_DEVICE);
>> > +       }
>> > +
>> > +       dma_free_coherent(ndev->dev.parent, rx_bd_v_size,
>> > +                               lp->rx_bd_v, lp->rx_bd_p);
>                                    ^^^^^^^^^^^
>                                     2nd
>> > +err_tx_bd:
>> > +       dma_free_coherent(ndev->dev.parent, tx_bd_v_size,
>> > +                               lp->tx_bd_v, lp->tx_bd_p);
>                                    ^^^^^^^^^^^
>                                     1st
>> > +err_rx_skb:
>> > +       kfree(lp->rx_skb);
>> >  out:
>> >        return -ENOMEM;
>> >  }
>> > --
>> > 1.7.0.4
>> This is not enough. DMA resources also must be released on exit.
> Could you point to it? I see only two variables with allocated DMA (see above).
> --
I just sent a patch that fixes this.

> 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
>



-- 
Regards,
Denis

^ permalink raw reply

* [PATCH -net-2.6] ll_temac: fix DMA resources leak
From: Denis Kirjanov @ 2010-07-08 13:54 UTC (permalink / raw)
  To: davem; +Cc: john.linn, brian.hill, netdev

Fix DMA resources leak.

Signed-off-by: Denis Kirjanov <dkirjanov@kernel.org>
Signed-off-by: Kulikov Vasiliy <segooon@gmail.com>
---
diff --git a/drivers/net/ll_temac_main.c b/drivers/net/ll_temac_main.c
index fa303c8..afec660 100644
--- a/drivers/net/ll_temac_main.c
+++ b/drivers/net/ll_temac_main.c
@@ -193,6 +193,28 @@ static int temac_dcr_setup(struct temac_local *lp, struct of_device *op,
 #endif
 
 /**
+ *  * temac_dma_bd_release - Release buffer descriptor rings
+ */
+static void temac_dma_bd_release(struct net_device *ndev)
+{
+	struct temac_local *lp = netdev_priv(ndev);
+	int i;
+
+	for (i = 0; i < RX_BD_NUM; i++) {
+		if (lp->rx_skb[i]) {
+			dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys,
+					XTE_MAX_JUMBO_FRAME_SIZE, DMA_FROM_DEVICE);
+			dev_kfree_skb(lp->rx_skb[i]);
+		}
+	}
+	dma_free_coherent(ndev->dev.parent, sizeof(*lp->rx_bd_v) * RX_BD_NUM,
+			lp->rx_bd_v, lp->rx_bd_p);
+	dma_free_coherent(ndev->dev.parent, sizeof(*lp->tx_bd_v) * TX_BD_NUM,
+			lp->tx_bd_v, lp->tx_bd_p);
+	kfree(lp->rx_skb);
+}
+
+/**
  * temac_dma_bd_init - Setup buffer descriptor rings
  */
 static int temac_dma_bd_init(struct net_device *ndev)
@@ -275,6 +297,7 @@ static int temac_dma_bd_init(struct net_device *ndev)
 	return 0;
 
 out:
+	temac_dma_bd_release(ndev);
 	return -ENOMEM;
 }
 
@@ -858,6 +881,8 @@ static int temac_stop(struct net_device *ndev)
 		phy_disconnect(lp->phy_dev);
 	lp->phy_dev = NULL;
 
+	temac_dma_bd_release(ndev);
+
 	return 0;
 }
 

^ 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