Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH v2 4/6] can: sja1000: fuse of_platform into platform
From: Marc Kleine-Budde @ 2014-01-31 12:33 UTC (permalink / raw)
  To: Florian Vaussard, Wolfgang Grandegger; +Cc: linux-can, netdev, linux-kernel
In-Reply-To: <1391164513-11529-5-git-send-email-florian.vaussard@epfl.ch>

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

On 01/31/2014 11:35 AM, Florian Vaussard wrote:
> The OpenFirmware probe can be merged into the standard platform
> probe to leverage common code.

Good work, as we want to replace the existing driver, I'm quite picky on
this patch, see more comments inline.

Please don't delete of of_platform driver, yet.

> Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
> ---
>  drivers/net/can/sja1000/Kconfig               |  13 +-
>  drivers/net/can/sja1000/Makefile              |   1 -
>  drivers/net/can/sja1000/sja1000_of_platform.c | 218 --------------------------
>  drivers/net/can/sja1000/sja1000_platform.c    | 141 +++++++++++++----
>  4 files changed, 116 insertions(+), 257 deletions(-)
>  delete mode 100644 drivers/net/can/sja1000/sja1000_of_platform.c
[...]

> diff --git a/drivers/net/can/sja1000/sja1000_platform.c b/drivers/net/can/sja1000/sja1000_platform.c
> index 779679a..96a92a1 100644
> --- a/drivers/net/can/sja1000/sja1000_platform.c
> +++ b/drivers/net/can/sja1000/sja1000_platform.c
> @@ -27,12 +27,16 @@
>  #include <linux/can/dev.h>
>  #include <linux/can/platform/sja1000.h>
>  #include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
>  
>  #include "sja1000.h"
>  
>  #define DRV_NAME "sja1000_platform"
> +#define SJA1000_OFP_CAN_CLOCK  (16000000 / 2)

This driver uses "SP" short for sja100_platform as namespace, please
convert.

>  
>  MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
> +MODULE_AUTHOR("Wolfgang Grandegger <wg@grandegger.com>");
>  MODULE_DESCRIPTION("Socket-CAN driver for SJA1000 on the platform bus");
>  MODULE_ALIAS("platform:" DRV_NAME);
>  MODULE_LICENSE("GPL v2");
> @@ -67,24 +71,98 @@ static void sp_write_reg32(const struct sja1000_priv *priv, int reg, u8 val)
>  	iowrite8(val, priv->reg_base + reg * 4);
>  }
>  
> -static int sp_probe(struct platform_device *pdev)
> +static void sp_populate(struct sja1000_priv *priv,
> +			struct sja1000_platform_data *pdata,
> +			unsigned long resource_mem_flags)
> +{
> +	/* The CAN clock frequency is half the oscillator clock frequency */
> +	priv->can.clock.freq = pdata->osc_freq / 2;
> +	priv->ocr = pdata->ocr;
> +	priv->cdr = pdata->cdr;
> +
> +	switch (resource_mem_flags & IORESOURCE_MEM_TYPE_MASK) {
> +	case IORESOURCE_MEM_32BIT:
> +		priv->read_reg = sp_read_reg32;
> +		priv->write_reg = sp_write_reg32;
> +		break;
> +	case IORESOURCE_MEM_16BIT:
> +		priv->read_reg = sp_read_reg16;
> +		priv->write_reg = sp_write_reg16;
> +		break;
> +	case IORESOURCE_MEM_8BIT:
> +	default:
> +		priv->read_reg = sp_read_reg8;
> +		priv->write_reg = sp_write_reg8;
> +		break;
> +	}
> +}
> +
> +#if defined(CONFIG_OF)

The driver also compiles on systems without CONFIG_OF without these
defines. Please remove.

> +static void sp_populate_of(struct sja1000_priv *priv, struct device_node *of)
>  {
>  	int err;
> +	u32 prop;
> +
> +	priv->read_reg = sp_read_reg8;
> +	priv->write_reg = sp_write_reg8;
> +
> +	err = of_property_read_u32(of, "nxp,external-clock-frequency", &prop);
> +	if (!err)
> +		priv->can.clock.freq = prop / 2;
> +	else
> +		priv->can.clock.freq = SJA1000_OFP_CAN_CLOCK; /* default */
> +
> +	err = of_property_read_u32(of, "nxp,tx-output-mode", &prop);
> +	if (!err)
> +		priv->ocr |= prop & OCR_MODE_MASK;
> +	else
> +		priv->ocr |= OCR_MODE_NORMAL; /* default */
> +
> +	err = of_property_read_u32(of, "nxp,tx-output-config", &prop);
> +	if (!err)
> +		priv->ocr |= (prop << OCR_TX_SHIFT) & OCR_TX_MASK;
> +	else
> +		priv->ocr |= OCR_TX0_PULLDOWN; /* default */
> +
> +	err = of_property_read_u32(of, "nxp,clock-out-frequency", &prop);
> +	if (!err && prop) {
> +		u32 divider = priv->can.clock.freq * 2 / prop;
> +
> +		if (divider > 1)
> +			priv->cdr |= divider / 2 - 1;
> +		else
> +			priv->cdr |= CDR_CLKOUT_MASK;
> +	} else {
> +		priv->cdr |= CDR_CLK_OFF; /* default */
> +	}
> +
> +	if (!of_property_read_bool(of, "nxp,no-comparator-bypass"))
> +		priv->cdr |= CDR_CBP; /* default */
> +}
> +#else
> +static void sp_populate_of(struct sja1000_priv *priv, device_node *of)
> +{
> +}
> +#endif
> +
> +static int sp_probe(struct platform_device *pdev)
> +{
> +	int err, irq = 0;
>  	void __iomem *addr;
>  	struct net_device *dev;
>  	struct sja1000_priv *priv;
> -	struct resource *res_mem, *res_irq;
> +	struct resource *res_mem, *res_irq = 0;

Please use NULL to initialize pointers.

>  	struct sja1000_platform_data *pdata;
> +	struct device_node *of = pdev->dev.of_node;
>  
>  	pdata = dev_get_platdata(&pdev->dev);
> -	if (!pdata) {
> +	if (!pdata && !of) {
>  		dev_err(&pdev->dev, "No platform data provided!\n");
>  		return -ENODEV;
>  	}
>  
>  	res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> -	if (!res_mem || !res_irq)
> +	if (!res_mem)
>  		return -ENODEV;
>  
>  	if (!devm_request_mem_region(&pdev->dev, res_mem->start,
> @@ -96,36 +174,34 @@ static int sp_probe(struct platform_device *pdev)
>  	if (!addr)
>  		return -ENOMEM;
>  
> +	if (of)
> +		irq = irq_of_parse_and_map(of, 0);
> +	else
> +		res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +
> +	if (!irq && !res_irq)
> +		return -ENODEV;
> +
>  	dev = alloc_sja1000dev(0);
>  	if (!dev)
>  		return -ENOMEM;
>  	priv = netdev_priv(dev);
>  
> -	dev->irq = res_irq->start;
> -	priv->irq_flags = res_irq->flags & IRQF_TRIGGER_MASK;
> -	if (res_irq->flags & IORESOURCE_IRQ_SHAREABLE)
> -		priv->irq_flags |= IRQF_SHARED;
> +	if (res_irq) {
> +		irq = res_irq->start;
> +		priv->irq_flags = res_irq->flags & IRQF_TRIGGER_MASK;
> +		if (res_irq->flags & IORESOURCE_IRQ_SHAREABLE)
> +			priv->irq_flags |= IRQF_SHARED;
> +	} else
> +		priv->irq_flags = IRQF_SHARED;

Please add { } for else, too.

> +
> +	dev->irq = irq;
>  	priv->reg_base = addr;
> -	/* The CAN clock frequency is half the oscillator clock frequency */
> -	priv->can.clock.freq = pdata->osc_freq / 2;
> -	priv->ocr = pdata->ocr;
> -	priv->cdr = pdata->cdr;
>  
> -	switch (res_mem->flags & IORESOURCE_MEM_TYPE_MASK) {
> -	case IORESOURCE_MEM_32BIT:
> -		priv->read_reg = sp_read_reg32;
> -		priv->write_reg = sp_write_reg32;
> -		break;
> -	case IORESOURCE_MEM_16BIT:
> -		priv->read_reg = sp_read_reg16;
> -		priv->write_reg = sp_write_reg16;
> -		break;
> -	case IORESOURCE_MEM_8BIT:
> -	default:
> -		priv->read_reg = sp_read_reg8;
> -		priv->write_reg = sp_write_reg8;
> -		break;
> -	}
> +	if (of)
> +		sp_populate_of(priv, of);
> +	else
> +		sp_populate(priv, pdata, res_mem->flags);
>  
>  	platform_set_drvdata(pdev, dev);
>  	SET_NETDEV_DEV(dev, &pdev->dev);
> @@ -156,12 +232,21 @@ static int sp_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +#if defined(CONFIG_OF)

Please remove the ifdef.

> +static struct of_device_id sja1000_ofp_table[] = {

Please rename into sp_of_table or something similar.

> +	{.compatible = "nxp,sja1000"},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, sja1000_ofp_table);
> +#endif
> +
>  static struct platform_driver sp_driver = {
>  	.probe = sp_probe,
>  	.remove = sp_remove,
>  	.driver = {
>  		.name = DRV_NAME,
>  		.owner = THIS_MODULE,
> +		.of_match_table = of_match_ptr(sja1000_ofp_table),

The of_match_ptr is not needed anymore, please remove.

>  	},
>  };
>  
> 

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: 242 bytes --]

^ permalink raw reply

* Re: [PATCH v2 5/6] Documentation: devicetree: sja1000: add reg-io-width binding
From: Marc Kleine-Budde @ 2014-01-31 12:29 UTC (permalink / raw)
  To: Florian Vaussard, Wolfgang Grandegger
  Cc: linux-can, netdev, linux-kernel, Grant Likely, Rob Herring,
	Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree
In-Reply-To: <1391164513-11529-6-git-send-email-florian.vaussard@epfl.ch>

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

On 01/31/2014 11:35 AM, Florian Vaussard wrote:
> Add the reg-io-width property to describe the width of the memory
> accesses.
> 
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: Kumar Gala <galak@codeaurora.org>
> Cc: devicetree@vger.kernel.org
> Acked-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>

I think it makes sense to squash into patch 6.

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: 242 bytes --]

^ permalink raw reply

* Re: [PATCH v2 6/6] can: sja1000: of: add reg-io-width property for 8, 16 and 32-bit register access
From: Marc Kleine-Budde @ 2014-01-31 12:28 UTC (permalink / raw)
  To: Florian Vaussard, Wolfgang Grandegger; +Cc: linux-can, netdev, linux-kernel
In-Reply-To: <1391164513-11529-7-git-send-email-florian.vaussard@epfl.ch>

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

On 01/31/2014 11:35 AM, Florian Vaussard wrote:
> Add the 'reg-io-width' property for 8, 16 and 32-bit access, like
> what is currently done with IORESOURCE_MEM_{8,16,32}BIT for non-OF
> boot.
> 
> Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
> ---
>  drivers/net/can/sja1000/sja1000_platform.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/sja1000/sja1000_platform.c b/drivers/net/can/sja1000/sja1000_platform.c
> index 96a92a1..62ebc3d 100644
> --- a/drivers/net/can/sja1000/sja1000_platform.c
> +++ b/drivers/net/can/sja1000/sja1000_platform.c
> @@ -103,8 +103,17 @@ static void sp_populate_of(struct sja1000_priv *priv, struct device_node *of)
>  	int err;
>  	u32 prop;
>  
> -	priv->read_reg = sp_read_reg8;
> -	priv->write_reg = sp_write_reg8;
> +	of_property_read_u32(of, "reg-io-width", &prop);

The new property might not be available in the DT, please check the
return value of of_property_read_u32(). Use 8 bit wide as default.

> +	if (prop == 4) {
> +		priv->read_reg = sp_read_reg32;
> +		priv->write_reg = sp_write_reg32;
> +	} else if (prop == 2) {
> +		priv->read_reg = sp_read_reg16;
> +		priv->write_reg = sp_write_reg16;
> +	} else {
> +		priv->read_reg = sp_read_reg8;
> +		priv->write_reg = sp_write_reg8;
> +	}
>  
>  	err = of_property_read_u32(of, "nxp,external-clock-frequency", &prop);
>  	if (!err)
> 

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: 242 bytes --]

^ permalink raw reply

* Re: Suspected Copy Paste error in rtnl_bridge_notify
From: Tejun Heo @ 2014-01-31 11:47 UTC (permalink / raw)
  To: Yogesh Gaur; +Cc: akpm, torvalds, linux-kernel, David S. Miller, netdev
In-Reply-To: <CAG6enPyV9bvYSwDrMp6X1LY6PXrmrP5MtjNk0N+7qs1LDY5pRg@mail.gmail.com>

Hey,

Seems suspicious but I don't know the code at all.  cc'ing David and
netdev and quoting the whole body for them.

Thanks.

On Fri, Jan 31, 2014 at 11:32:55AM +0530, Yogesh Gaur wrote:
> Hello All,
> 
> I want to clarify about following code-snippet from
> file: "net/core/rtnetlink.c"
> func: rtnl_bridge_notify
> 
> ----------------------------------------------------------------------------
> |    if ((!flags || (flags & BRIDGE_FLAGS_MASTER)) && |
> |        br_dev && br_dev->netdev_ops->ndo_bridge_getlink) {
> |
> |        err = br_dev->netdev_ops->ndo_bridge_getlink(skb, 0, 0, dev, 0);
> |
> |        if (err < 0)
> |
> |            goto errout;
> |
> |    }
> |
> |
> |
> |    if ((flags & BRIDGE_FLAGS_SELF) &&
> |
> |        dev->netdev_ops->ndo_bridge_getlink) {
> |
> |        err = dev->netdev_ops->ndo_bridge_getlink(skb, 0, 0, dev, 0);
> |
> |        if (err < 0)
> |
> |            goto errout;
> |
> ----------------------------------------------------------------------------
> 
> In above code isn't code line:
> err = br_dev->netdev_ops->ndo_bridge_getlink(skb, 0, 0, dev, 0);
> should be
> err = br_dev->netdev_ops->ndo_bridge_getlink(skb, 0, 0, br_dev, 0);
> 
> If this needs to be changed then please review attached patch adding this
> change.

-- 
tejun

^ permalink raw reply

* Re: IGMP joins come from the wrong SA/interface
From: Hannes Frederic Sowa @ 2014-01-31 11:32 UTC (permalink / raw)
  To: Julian Anastasov; +Cc: Steinar H. Gunderson, netdev
In-Reply-To: <alpine.LFD.2.11.1401310948270.1576@ja.home.ssi.bg>

On Fri, Jan 31, 2014 at 10:51:53AM +0200, Julian Anastasov wrote:
> 	To summarize: IGMP will use the same interface as
> selected from route to the joined multicast group but
> source can be from another device if for some reason there
> is no address on this interface.

Exactly that was my conclusion. As eth0.11 (as said in the second mail)
is a leg of a bridge I concluded that there is no ip address on that
interface.

Greetings,

  Hannes

^ permalink raw reply

* [PATCH v2 5/6] Documentation: devicetree: sja1000: add reg-io-width binding
From: Florian Vaussard @ 2014-01-31 10:35 UTC (permalink / raw)
  To: Wolfgang Grandegger, Marc Kleine-Budde
  Cc: linux-can, netdev, linux-kernel, florian.vaussard, Grant Likely,
	Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
	devicetree
In-Reply-To: <1391164513-11529-1-git-send-email-florian.vaussard@epfl.ch>

Add the reg-io-width property to describe the width of the memory
accesses.

Cc: Grant Likely <grant.likely@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Kumar Gala <galak@codeaurora.org>
Cc: devicetree@vger.kernel.org
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
---
 Documentation/devicetree/bindings/net/can/sja1000.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/can/sja1000.txt b/Documentation/devicetree/bindings/net/can/sja1000.txt
index f2105a4..b4a6d53 100644
--- a/Documentation/devicetree/bindings/net/can/sja1000.txt
+++ b/Documentation/devicetree/bindings/net/can/sja1000.txt
@@ -12,6 +12,10 @@ Required properties:
 
 Optional properties:
 
+- reg-io-width : Specify the size (in bytes) of the IO accesses that
+	should be performed on the device.  Valid value is 1, 2 or 4.
+	Default to 1 (8 bits).
+
 - nxp,external-clock-frequency : Frequency of the external oscillator
 	clock in Hz. Note that the internal clock frequency used by the
 	SJA1000 is half of that value. If not specified, a default value
-- 
1.8.1.2

^ permalink raw reply related

* [PATCH v2 3/6] can: sja1000: platform: use devm_* APIs
From: Florian Vaussard @ 2014-01-31 10:35 UTC (permalink / raw)
  To: Wolfgang Grandegger, Marc Kleine-Budde
  Cc: linux-can, netdev, linux-kernel, florian.vaussard
In-Reply-To: <1391164513-11529-1-git-send-email-florian.vaussard@epfl.ch>

Simplify probe and remove functions by converting most of the resources
to use devm_* APIs.

Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
---
 drivers/net/can/sja1000/sja1000_platform.c | 46 ++++++++----------------------
 1 file changed, 12 insertions(+), 34 deletions(-)

diff --git a/drivers/net/can/sja1000/sja1000_platform.c b/drivers/net/can/sja1000/sja1000_platform.c
index 29f9b63..779679a 100644
--- a/drivers/net/can/sja1000/sja1000_platform.c
+++ b/drivers/net/can/sja1000/sja1000_platform.c
@@ -79,34 +79,26 @@ static int sp_probe(struct platform_device *pdev)
 	pdata = dev_get_platdata(&pdev->dev);
 	if (!pdata) {
 		dev_err(&pdev->dev, "No platform data provided!\n");
-		err = -ENODEV;
-		goto exit;
+		return -ENODEV;
 	}
 
 	res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-	if (!res_mem || !res_irq) {
-		err = -ENODEV;
-		goto exit;
-	}
+	if (!res_mem || !res_irq)
+		return -ENODEV;
 
-	if (!request_mem_region(res_mem->start, resource_size(res_mem),
-				DRV_NAME)) {
-		err = -EBUSY;
-		goto exit;
-	}
+	if (!devm_request_mem_region(&pdev->dev, res_mem->start,
+				     resource_size(res_mem), DRV_NAME))
+		return -EBUSY;
 
-	addr = ioremap_nocache(res_mem->start, resource_size(res_mem));
-	if (!addr) {
-		err = -ENOMEM;
-		goto exit_release;
-	}
+	addr = devm_ioremap_nocache(&pdev->dev, res_mem->start,
+				    resource_size(res_mem));
+	if (!addr)
+		return -ENOMEM;
 
 	dev = alloc_sja1000dev(0);
-	if (!dev) {
-		err = -ENOMEM;
-		goto exit_iounmap;
-	}
+	if (!dev)
+		return -ENOMEM;
 	priv = netdev_priv(dev);
 
 	dev->irq = res_irq->start;
@@ -151,28 +143,14 @@ static int sp_probe(struct platform_device *pdev)
 
  exit_free:
 	free_sja1000dev(dev);
- exit_iounmap:
-	iounmap(addr);
- exit_release:
-	release_mem_region(res_mem->start, resource_size(res_mem));
- exit:
 	return err;
 }
 
 static int sp_remove(struct platform_device *pdev)
 {
 	struct net_device *dev = platform_get_drvdata(pdev);
-	struct sja1000_priv *priv = netdev_priv(dev);
-	struct resource *res;
 
 	unregister_sja1000dev(dev);
-
-	if (priv->reg_base)
-		iounmap(priv->reg_base);
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	release_mem_region(res->start, resource_size(res));
-
 	free_sja1000dev(dev);
 
 	return 0;
-- 
1.8.1.2

^ permalink raw reply related

* [PATCH v2 6/6] can: sja1000: of: add reg-io-width property for 8, 16 and 32-bit register access
From: Florian Vaussard @ 2014-01-31 10:35 UTC (permalink / raw)
  To: Wolfgang Grandegger, Marc Kleine-Budde
  Cc: linux-can, netdev, linux-kernel, florian.vaussard
In-Reply-To: <1391164513-11529-1-git-send-email-florian.vaussard@epfl.ch>

Add the 'reg-io-width' property for 8, 16 and 32-bit access, like
what is currently done with IORESOURCE_MEM_{8,16,32}BIT for non-OF
boot.

Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
---
 drivers/net/can/sja1000/sja1000_platform.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/sja1000/sja1000_platform.c b/drivers/net/can/sja1000/sja1000_platform.c
index 96a92a1..62ebc3d 100644
--- a/drivers/net/can/sja1000/sja1000_platform.c
+++ b/drivers/net/can/sja1000/sja1000_platform.c
@@ -103,8 +103,17 @@ static void sp_populate_of(struct sja1000_priv *priv, struct device_node *of)
 	int err;
 	u32 prop;
 
-	priv->read_reg = sp_read_reg8;
-	priv->write_reg = sp_write_reg8;
+	of_property_read_u32(of, "reg-io-width", &prop);
+	if (prop == 4) {
+		priv->read_reg = sp_read_reg32;
+		priv->write_reg = sp_write_reg32;
+	} else if (prop == 2) {
+		priv->read_reg = sp_read_reg16;
+		priv->write_reg = sp_write_reg16;
+	} else {
+		priv->read_reg = sp_read_reg8;
+		priv->write_reg = sp_write_reg8;
+	}
 
 	err = of_property_read_u32(of, "nxp,external-clock-frequency", &prop);
 	if (!err)
-- 
1.8.1.2

^ permalink raw reply related

* [PATCH v2 4/6] can: sja1000: fuse of_platform into platform
From: Florian Vaussard @ 2014-01-31 10:35 UTC (permalink / raw)
  To: Wolfgang Grandegger, Marc Kleine-Budde
  Cc: linux-can, netdev, linux-kernel, florian.vaussard
In-Reply-To: <1391164513-11529-1-git-send-email-florian.vaussard@epfl.ch>

The OpenFirmware probe can be merged into the standard platform
probe to leverage common code.

Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
---
 drivers/net/can/sja1000/Kconfig               |  13 +-
 drivers/net/can/sja1000/Makefile              |   1 -
 drivers/net/can/sja1000/sja1000_of_platform.c | 218 --------------------------
 drivers/net/can/sja1000/sja1000_platform.c    | 141 +++++++++++++----
 4 files changed, 116 insertions(+), 257 deletions(-)
 delete mode 100644 drivers/net/can/sja1000/sja1000_of_platform.c

diff --git a/drivers/net/can/sja1000/Kconfig b/drivers/net/can/sja1000/Kconfig
index ff2ba86..4b18b87 100644
--- a/drivers/net/can/sja1000/Kconfig
+++ b/drivers/net/can/sja1000/Kconfig
@@ -17,16 +17,9 @@ config CAN_SJA1000_PLATFORM
 	  the "platform bus" (Linux abstraction for directly to the
 	  processor attached devices).  Which can be found on various
 	  boards from Phytec (http://www.phytec.de) like the PCM027,
-	  PCM038.
-
-config CAN_SJA1000_OF_PLATFORM
-	tristate "Generic OF Platform Bus based SJA1000 driver"
-	depends on OF
-	---help---
-	  This driver adds support for the SJA1000 chips connected to
-	  the OpenFirmware "platform bus" found on embedded systems with
-	  OpenFirmware bindings, e.g. if you have a PowerPC based system
-	  you may want to enable this option.
+	  PCM038. It also provides the OpenFirmware "platform bus" found
+	  on embedded systems with OpenFirmware bindings, e.g. if you
+	  have a PowerPC based system you may want to enable this option.
 
 config CAN_EMS_PCMCIA
 	tristate "EMS CPC-CARD Card"
diff --git a/drivers/net/can/sja1000/Makefile b/drivers/net/can/sja1000/Makefile
index b3d05cb..531d5fc 100644
--- a/drivers/net/can/sja1000/Makefile
+++ b/drivers/net/can/sja1000/Makefile
@@ -5,7 +5,6 @@
 obj-$(CONFIG_CAN_SJA1000) += sja1000.o
 obj-$(CONFIG_CAN_SJA1000_ISA) += sja1000_isa.o
 obj-$(CONFIG_CAN_SJA1000_PLATFORM) += sja1000_platform.o
-obj-$(CONFIG_CAN_SJA1000_OF_PLATFORM) += sja1000_of_platform.o
 obj-$(CONFIG_CAN_EMS_PCMCIA) += ems_pcmcia.o
 obj-$(CONFIG_CAN_EMS_PCI) += ems_pci.o
 obj-$(CONFIG_CAN_KVASER_PCI) += kvaser_pci.o
diff --git a/drivers/net/can/sja1000/sja1000_of_platform.c b/drivers/net/can/sja1000/sja1000_of_platform.c
deleted file mode 100644
index 2f29eb9..0000000
--- a/drivers/net/can/sja1000/sja1000_of_platform.c
+++ /dev/null
@@ -1,218 +0,0 @@
-/*
- * Driver for SJA1000 CAN controllers on the OpenFirmware platform bus
- *
- * Copyright (C) 2008-2009 Wolfgang Grandegger <wg@grandegger.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the version 2 of the GNU General Public License
- * as published by the Free Software Foundation
- *
- * 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.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software Foundation,
- * Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
- */
-
-/* This is a generic driver for SJA1000 chips on the OpenFirmware platform
- * bus found on embedded PowerPC systems. You need a SJA1000 CAN node
- * definition in your flattened device tree source (DTS) file similar to:
- *
- *   can@3,100 {
- *           compatible = "nxp,sja1000";
- *           reg = <3 0x100 0x80>;
- *           interrupts = <2 0>;
- *           interrupt-parent = <&mpic>;
- *           nxp,external-clock-frequency = <16000000>;
- *   };
- *
- * See "Documentation/devicetree/bindings/net/can/sja1000.txt" for further
- * information.
- */
-
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/interrupt.h>
-#include <linux/netdevice.h>
-#include <linux/delay.h>
-#include <linux/io.h>
-#include <linux/can/dev.h>
-
-#include <linux/of_platform.h>
-#include <linux/of_address.h>
-#include <linux/of_irq.h>
-
-#include "sja1000.h"
-
-#define DRV_NAME "sja1000_of_platform"
-
-MODULE_AUTHOR("Wolfgang Grandegger <wg@grandegger.com>");
-MODULE_DESCRIPTION("Socket-CAN driver for SJA1000 on the OF platform bus");
-MODULE_LICENSE("GPL v2");
-
-#define SJA1000_OFP_CAN_CLOCK  (16000000 / 2)
-
-static u8 sja1000_ofp_read_reg(const struct sja1000_priv *priv, int reg)
-{
-	return ioread8(priv->reg_base + reg);
-}
-
-static void sja1000_ofp_write_reg(const struct sja1000_priv *priv,
-				  int reg, u8 val)
-{
-	iowrite8(val, priv->reg_base + reg);
-}
-
-static int sja1000_ofp_remove(struct platform_device *ofdev)
-{
-	struct net_device *dev = platform_get_drvdata(ofdev);
-	struct sja1000_priv *priv = netdev_priv(dev);
-	struct device_node *np = ofdev->dev.of_node;
-	struct resource res;
-
-	unregister_sja1000dev(dev);
-	free_sja1000dev(dev);
-	iounmap(priv->reg_base);
-	irq_dispose_mapping(dev->irq);
-
-	of_address_to_resource(np, 0, &res);
-	release_mem_region(res.start, resource_size(&res));
-
-	return 0;
-}
-
-static int sja1000_ofp_probe(struct platform_device *ofdev)
-{
-	struct device_node *np = ofdev->dev.of_node;
-	struct net_device *dev;
-	struct sja1000_priv *priv;
-	struct resource res;
-	u32 prop;
-	int err, irq, res_size;
-	void __iomem *base;
-
-	err = of_address_to_resource(np, 0, &res);
-	if (err) {
-		dev_err(&ofdev->dev, "invalid address\n");
-		return err;
-	}
-
-	res_size = resource_size(&res);
-
-	if (!request_mem_region(res.start, res_size, DRV_NAME)) {
-		dev_err(&ofdev->dev, "couldn't request %pR\n", &res);
-		return -EBUSY;
-	}
-
-	base = ioremap_nocache(res.start, res_size);
-	if (!base) {
-		dev_err(&ofdev->dev, "couldn't ioremap %pR\n", &res);
-		err = -ENOMEM;
-		goto exit_release_mem;
-	}
-
-	irq = irq_of_parse_and_map(np, 0);
-	if (irq == 0) {
-		dev_err(&ofdev->dev, "no irq found\n");
-		err = -ENODEV;
-		goto exit_unmap_mem;
-	}
-
-	dev = alloc_sja1000dev(0);
-	if (!dev) {
-		err = -ENOMEM;
-		goto exit_dispose_irq;
-	}
-
-	priv = netdev_priv(dev);
-
-	priv->read_reg = sja1000_ofp_read_reg;
-	priv->write_reg = sja1000_ofp_write_reg;
-
-	err = of_property_read_u32(np, "nxp,external-clock-frequency", &prop);
-	if (!err)
-		priv->can.clock.freq = prop / 2;
-	else
-		priv->can.clock.freq = SJA1000_OFP_CAN_CLOCK; /* default */
-
-	err = of_property_read_u32(np, "nxp,tx-output-mode", &prop);
-	if (!err)
-		priv->ocr |= prop & OCR_MODE_MASK;
-	else
-		priv->ocr |= OCR_MODE_NORMAL; /* default */
-
-	err = of_property_read_u32(np, "nxp,tx-output-config", &prop);
-	if (!err)
-		priv->ocr |= (prop << OCR_TX_SHIFT) & OCR_TX_MASK;
-	else
-		priv->ocr |= OCR_TX0_PULLDOWN; /* default */
-
-	err = of_property_read_u32(np, "nxp,clock-out-frequency", &prop);
-	if (!err && prop) {
-		u32 divider = priv->can.clock.freq * 2 / prop;
-
-		if (divider > 1)
-			priv->cdr |= divider / 2 - 1;
-		else
-			priv->cdr |= CDR_CLKOUT_MASK;
-	} else {
-		priv->cdr |= CDR_CLK_OFF; /* default */
-	}
-
-	if (!of_property_read_bool(np, "nxp,no-comparator-bypass"))
-		priv->cdr |= CDR_CBP; /* default */
-
-	priv->irq_flags = IRQF_SHARED;
-	priv->reg_base = base;
-
-	dev->irq = irq;
-
-	dev_info(&ofdev->dev,
-		 "reg_base=0x%p irq=%d clock=%d ocr=0x%02x cdr=0x%02x\n",
-		 priv->reg_base, dev->irq, priv->can.clock.freq,
-		 priv->ocr, priv->cdr);
-
-	platform_set_drvdata(ofdev, dev);
-	SET_NETDEV_DEV(dev, &ofdev->dev);
-
-	err = register_sja1000dev(dev);
-	if (err) {
-		dev_err(&ofdev->dev, "registering %s failed (err=%d)\n",
-			DRV_NAME, err);
-		goto exit_free_sja1000;
-	}
-
-	return 0;
-
-exit_free_sja1000:
-	free_sja1000dev(dev);
-exit_dispose_irq:
-	irq_dispose_mapping(irq);
-exit_unmap_mem:
-	iounmap(base);
-exit_release_mem:
-	release_mem_region(res.start, res_size);
-
-	return err;
-}
-
-static struct of_device_id sja1000_ofp_table[] = {
-	{.compatible = "nxp,sja1000"},
-	{},
-};
-MODULE_DEVICE_TABLE(of, sja1000_ofp_table);
-
-static struct platform_driver sja1000_ofp_driver = {
-	.driver = {
-		.owner = THIS_MODULE,
-		.name = DRV_NAME,
-		.of_match_table = sja1000_ofp_table,
-	},
-	.probe = sja1000_ofp_probe,
-	.remove = sja1000_ofp_remove,
-};
-
-module_platform_driver(sja1000_ofp_driver);
diff --git a/drivers/net/can/sja1000/sja1000_platform.c b/drivers/net/can/sja1000/sja1000_platform.c
index 779679a..96a92a1 100644
--- a/drivers/net/can/sja1000/sja1000_platform.c
+++ b/drivers/net/can/sja1000/sja1000_platform.c
@@ -27,12 +27,16 @@
 #include <linux/can/dev.h>
 #include <linux/can/platform/sja1000.h>
 #include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
 
 #include "sja1000.h"
 
 #define DRV_NAME "sja1000_platform"
+#define SJA1000_OFP_CAN_CLOCK  (16000000 / 2)
 
 MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
+MODULE_AUTHOR("Wolfgang Grandegger <wg@grandegger.com>");
 MODULE_DESCRIPTION("Socket-CAN driver for SJA1000 on the platform bus");
 MODULE_ALIAS("platform:" DRV_NAME);
 MODULE_LICENSE("GPL v2");
@@ -67,24 +71,98 @@ static void sp_write_reg32(const struct sja1000_priv *priv, int reg, u8 val)
 	iowrite8(val, priv->reg_base + reg * 4);
 }
 
-static int sp_probe(struct platform_device *pdev)
+static void sp_populate(struct sja1000_priv *priv,
+			struct sja1000_platform_data *pdata,
+			unsigned long resource_mem_flags)
+{
+	/* The CAN clock frequency is half the oscillator clock frequency */
+	priv->can.clock.freq = pdata->osc_freq / 2;
+	priv->ocr = pdata->ocr;
+	priv->cdr = pdata->cdr;
+
+	switch (resource_mem_flags & IORESOURCE_MEM_TYPE_MASK) {
+	case IORESOURCE_MEM_32BIT:
+		priv->read_reg = sp_read_reg32;
+		priv->write_reg = sp_write_reg32;
+		break;
+	case IORESOURCE_MEM_16BIT:
+		priv->read_reg = sp_read_reg16;
+		priv->write_reg = sp_write_reg16;
+		break;
+	case IORESOURCE_MEM_8BIT:
+	default:
+		priv->read_reg = sp_read_reg8;
+		priv->write_reg = sp_write_reg8;
+		break;
+	}
+}
+
+#if defined(CONFIG_OF)
+static void sp_populate_of(struct sja1000_priv *priv, struct device_node *of)
 {
 	int err;
+	u32 prop;
+
+	priv->read_reg = sp_read_reg8;
+	priv->write_reg = sp_write_reg8;
+
+	err = of_property_read_u32(of, "nxp,external-clock-frequency", &prop);
+	if (!err)
+		priv->can.clock.freq = prop / 2;
+	else
+		priv->can.clock.freq = SJA1000_OFP_CAN_CLOCK; /* default */
+
+	err = of_property_read_u32(of, "nxp,tx-output-mode", &prop);
+	if (!err)
+		priv->ocr |= prop & OCR_MODE_MASK;
+	else
+		priv->ocr |= OCR_MODE_NORMAL; /* default */
+
+	err = of_property_read_u32(of, "nxp,tx-output-config", &prop);
+	if (!err)
+		priv->ocr |= (prop << OCR_TX_SHIFT) & OCR_TX_MASK;
+	else
+		priv->ocr |= OCR_TX0_PULLDOWN; /* default */
+
+	err = of_property_read_u32(of, "nxp,clock-out-frequency", &prop);
+	if (!err && prop) {
+		u32 divider = priv->can.clock.freq * 2 / prop;
+
+		if (divider > 1)
+			priv->cdr |= divider / 2 - 1;
+		else
+			priv->cdr |= CDR_CLKOUT_MASK;
+	} else {
+		priv->cdr |= CDR_CLK_OFF; /* default */
+	}
+
+	if (!of_property_read_bool(of, "nxp,no-comparator-bypass"))
+		priv->cdr |= CDR_CBP; /* default */
+}
+#else
+static void sp_populate_of(struct sja1000_priv *priv, device_node *of)
+{
+}
+#endif
+
+static int sp_probe(struct platform_device *pdev)
+{
+	int err, irq = 0;
 	void __iomem *addr;
 	struct net_device *dev;
 	struct sja1000_priv *priv;
-	struct resource *res_mem, *res_irq;
+	struct resource *res_mem, *res_irq = 0;
 	struct sja1000_platform_data *pdata;
+	struct device_node *of = pdev->dev.of_node;
 
 	pdata = dev_get_platdata(&pdev->dev);
-	if (!pdata) {
+	if (!pdata && !of) {
 		dev_err(&pdev->dev, "No platform data provided!\n");
 		return -ENODEV;
 	}
 
 	res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-	if (!res_mem || !res_irq)
+	if (!res_mem)
 		return -ENODEV;
 
 	if (!devm_request_mem_region(&pdev->dev, res_mem->start,
@@ -96,36 +174,34 @@ static int sp_probe(struct platform_device *pdev)
 	if (!addr)
 		return -ENOMEM;
 
+	if (of)
+		irq = irq_of_parse_and_map(of, 0);
+	else
+		res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+
+	if (!irq && !res_irq)
+		return -ENODEV;
+
 	dev = alloc_sja1000dev(0);
 	if (!dev)
 		return -ENOMEM;
 	priv = netdev_priv(dev);
 
-	dev->irq = res_irq->start;
-	priv->irq_flags = res_irq->flags & IRQF_TRIGGER_MASK;
-	if (res_irq->flags & IORESOURCE_IRQ_SHAREABLE)
-		priv->irq_flags |= IRQF_SHARED;
+	if (res_irq) {
+		irq = res_irq->start;
+		priv->irq_flags = res_irq->flags & IRQF_TRIGGER_MASK;
+		if (res_irq->flags & IORESOURCE_IRQ_SHAREABLE)
+			priv->irq_flags |= IRQF_SHARED;
+	} else
+		priv->irq_flags = IRQF_SHARED;
+
+	dev->irq = irq;
 	priv->reg_base = addr;
-	/* The CAN clock frequency is half the oscillator clock frequency */
-	priv->can.clock.freq = pdata->osc_freq / 2;
-	priv->ocr = pdata->ocr;
-	priv->cdr = pdata->cdr;
 
-	switch (res_mem->flags & IORESOURCE_MEM_TYPE_MASK) {
-	case IORESOURCE_MEM_32BIT:
-		priv->read_reg = sp_read_reg32;
-		priv->write_reg = sp_write_reg32;
-		break;
-	case IORESOURCE_MEM_16BIT:
-		priv->read_reg = sp_read_reg16;
-		priv->write_reg = sp_write_reg16;
-		break;
-	case IORESOURCE_MEM_8BIT:
-	default:
-		priv->read_reg = sp_read_reg8;
-		priv->write_reg = sp_write_reg8;
-		break;
-	}
+	if (of)
+		sp_populate_of(priv, of);
+	else
+		sp_populate(priv, pdata, res_mem->flags);
 
 	platform_set_drvdata(pdev, dev);
 	SET_NETDEV_DEV(dev, &pdev->dev);
@@ -156,12 +232,21 @@ static int sp_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#if defined(CONFIG_OF)
+static struct of_device_id sja1000_ofp_table[] = {
+	{.compatible = "nxp,sja1000"},
+	{},
+};
+MODULE_DEVICE_TABLE(of, sja1000_ofp_table);
+#endif
+
 static struct platform_driver sp_driver = {
 	.probe = sp_probe,
 	.remove = sp_remove,
 	.driver = {
 		.name = DRV_NAME,
 		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(sja1000_ofp_table),
 	},
 };
 
-- 
1.8.1.2

^ permalink raw reply related

* [PATCH v2 2/6] can: sja1000: convert printk to use netdev API
From: Florian Vaussard @ 2014-01-31 10:35 UTC (permalink / raw)
  To: Wolfgang Grandegger, Marc Kleine-Budde
  Cc: linux-can, netdev, linux-kernel, florian.vaussard
In-Reply-To: <1391164513-11529-1-git-send-email-florian.vaussard@epfl.ch>

Use netdev_* where applicable.

Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
---
 drivers/net/can/sja1000/sja1000.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index f17c301..55cce47 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -106,8 +106,7 @@ static int sja1000_probe_chip(struct net_device *dev)
 	struct sja1000_priv *priv = netdev_priv(dev);
 
 	if (priv->reg_base && sja1000_is_absent(priv)) {
-		printk(KERN_INFO "%s: probing @0x%lX failed\n",
-		       DRV_NAME, dev->base_addr);
+		netdev_err(dev, "probing failed\n");
 		return 0;
 	}
 	return -1;
-- 
1.8.1.2

^ permalink raw reply related

* [PATCH v2 1/6] can: sja1000: remove unused defines
From: Florian Vaussard @ 2014-01-31 10:35 UTC (permalink / raw)
  To: Wolfgang Grandegger, Marc Kleine-Budde
  Cc: linux-can, netdev, linux-kernel, florian.vaussard
In-Reply-To: <1391164513-11529-1-git-send-email-florian.vaussard@epfl.ch>

Remove unused defines for the OF platform.

Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
---
 drivers/net/can/sja1000/sja1000_of_platform.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/can/sja1000/sja1000_of_platform.c b/drivers/net/can/sja1000/sja1000_of_platform.c
index 047accd..2f29eb9 100644
--- a/drivers/net/can/sja1000/sja1000_of_platform.c
+++ b/drivers/net/can/sja1000/sja1000_of_platform.c
@@ -55,9 +55,6 @@ MODULE_LICENSE("GPL v2");
 
 #define SJA1000_OFP_CAN_CLOCK  (16000000 / 2)
 
-#define SJA1000_OFP_OCR        OCR_TX0_PULLDOWN
-#define SJA1000_OFP_CDR        (CDR_CBP | CDR_CLK_OFF)
-
 static u8 sja1000_ofp_read_reg(const struct sja1000_priv *priv, int reg)
 {
 	return ioread8(priv->reg_base + reg);
-- 
1.8.1.2

^ permalink raw reply related

* [PATCH v2 0/6] can: sja1000: cleanups and new OF property
From: Florian Vaussard @ 2014-01-31 10:35 UTC (permalink / raw)
  To: Wolfgang Grandegger, Marc Kleine-Budde
  Cc: linux-can, netdev, linux-kernel, florian.vaussard

Hello,

Changes sinces v1:
- Merge sja1000_of_platform.c into sja1000_platform.c (patch 4)

The first part of this series performs serveral small cleanups
(patches 1 to 3).

Patch 4 merges sja1000_of_platform.c into sja1000_platform.c.
Changes are pretty conservatives (mostly copy/paste/move). IRQ
is treated differently in the OF and non-OF versions, thus this
is where the fused version differs the most.

The final part introduces the 'reg-io-width' binding (already used
by some other drivers) to perform a similar job as what was done
with IORESOURCE_MEM_XXBIT. This is needed on my system to correctly
take into account the aliasing of the address bus.

All patches were tested using OF boot on my OMAP3 system with a
memory-mapped SJA1000. Thus, the non-OF path is not tested, as
I do not have a platform data at hand.

Regards,
Florian

Florian Vaussard (6):
  can: sja1000: remove unused defines
  can: sja1000: convert printk to use netdev API
  can: sja1000: platform: use devm_* APIs
  can: sja1000: fuse of_platform into platform
  Documentation: devicetree: sja1000: add reg-io-width binding
  can: sja1000: of: add reg-io-width property for 8, 16 and 32-bit
    register access

 .../devicetree/bindings/net/can/sja1000.txt        |   4 +
 drivers/net/can/sja1000/Kconfig                    |  13 +-
 drivers/net/can/sja1000/Makefile                   |   1 -
 drivers/net/can/sja1000/sja1000.c                  |   3 +-
 drivers/net/can/sja1000/sja1000_of_platform.c      | 221 ---------------------
 drivers/net/can/sja1000/sja1000_platform.c         | 194 ++++++++++++------
 6 files changed, 141 insertions(+), 295 deletions(-)
 delete mode 100644 drivers/net/can/sja1000/sja1000_of_platform.c

-- 
1.8.1.2


^ permalink raw reply

* RE: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
From: David Laight @ 2014-01-31 10:14 UTC (permalink / raw)
  To: 'Sarah Sharp'
  Cc: linux-usb@vger.kernel.org, netdev@vger.kernel.org,
	Greg Kroah-Hartman, David Miller, Dan Williams, Nyman, Mathias,
	Mark Lord, Alan Stern, Freddy Xin
In-Reply-To: <20140130211816.GB3787@xanatos>

From: Sarah Sharp
> We need to step back and reassess the larger picture here, instead of
> trying to fire-fight all the regressions caused by the link TRB commit
> (35773dac5f86 "usb: xhci: Link TRB must not occur within a USB payload
> burst").

Some of the breakage seems to have been related to the PM and readq/writeq
changes.

The main problem with that patch is that it limited the number of
fragments.

> We shouldn't need to make userspace start to worry about alignment at
> all.  libusb worked in the past, before the link TRB fix went in.  We
> *cannot* break userspace USB drivers.  The breakage needs to be fixed in
> the USB core or the xHCI driver.

Userspace doesn't care since everything gets copied into aligned
kernel fragments - otherwise the other usb controllers wouldn't work.

> Commit 35773dac5f86 was meant to be a short-term bandaid fix, but it's
> already caused at least four different regressions.  Some we've fixed,
> some have proposed solutions that David has sent.
> 
> The storage layer is getting borked because it submits scatter-gather
> lists longer than what will fit on a segment, and now libusb has the
> same issue.  One xHCI host controller stopped responding to commands,
> and reverting the bandaid fix helped.  The implications of this change
> just keep coming in, and I'm not comfortable wall-papering over the
> issues.

The transfers from the storage layer are actually all 'suitably aligned'.
Fragments can cross 64k boundaries, but they all start on 4k boundaries.

> On the flip side, it seems that the only devices that have been helped
> by the bandaid fix patch are USB ethernet devices using the ax88179_178a
> driver.  (Mark Lord still needs to confirm which device he uses.)  I
> have not seen any other reports that other USB ethernet chipsets were
> broken in 3.12 by the USB networking layer adding scatter-gather
> support.

That is the only usbnet driver for which SG support is enabled.
I believe it was all enabled because the ax88179_178a is the only
one that can be expected to saturate Ge and supports TCP segmentation
offload (TSO) - where the buffer is almost always fragmented.
With TSO transmits are almost certainly single fragments.

> It should not matter what alignment or length of scatter-gather list the
> upper layers pass the xHCI driver, it should just work.  I want to do
> this fix right, by changing the fundamental way we queue TRBs to the
> rings to fit the TD rules.  We should break each TD into fragment-sized
> chunks, and add a link TRB in the middle of a segment where necessary.

There will always be some transfer requests that make this impossible
(without using a bounce buffer).
In practise nothing will send such bad transfers.
To avoid excessive work you need to be told whether the transfer is
aligned (everything from the block layer and libusb is) or misaligned
(potentially everything from usbnet).
The limits on the number length of SG list has to be different for the
two types of request.

> Let's do this fix the right way, instead of wall papering over the
> issue.  Here's what we should do:
> 
> 1. Disable scatter-gather for the ax88179_178a driver when it's under an
>    xHCI host.

That can be done by simple not setting the flag on the xhci driver.
However you also need to double check that this disables TSO.

> 2. Revert the following commits:
>    f2d9b991c549 xhci: Set scatter-gather limit to avoid failed block writes.
>    d6c9ea9069af xhci: Avoid infinite loop when sg urb requires too many trbs
>    35773dac5f86 usb: xhci: Link TRB must not occur within a USB payload burst
> 
> 3. Dan and Mathias can work together to come up with an overall plan to
>    change the xHCI driver architecture to be fully compliant with the TD
>    fragment rules.  That can be done over the next few kernel releases.

Don't forget that these rules can affect isoc transfers as well.
Even without SG, the buffer can cross a 64k boundary and thus
need splitting into separate TRB - which might need to be in
different ring segments.
Easily fixable by writing the LINK early (expect that the TRB writing
code looks for LINK TRBs).
 
> The end result is that we don't destabilize storage or break userspace
> USB drivers, we don't break people's xHCI host controllers,
> the ax88179_178a USB ethernet devices still work under xHCI (a bit with
> worse performance), and other USB ethernet devices still get the
> performance improvement introduced in 3.12.

^ permalink raw reply

* Re: [PATCH] net: set default DEVTYPE for all ethernet based devices
From: Veaceslav Falico @ 2014-01-31 10:07 UTC (permalink / raw)
  To: Tom Gundersen
  Cc: netdev, LKML, Stephen Hemminger, Avinash Kumar, Simon Horman,
	Marcel Holtmann, Greg KH, Kay Sievers
In-Reply-To: <CAG-2HqVH5akkCY_d7=v-EECFzYTfT531t4g6zG9euC4+VgFLWw@mail.gmail.com>

On Fri, Jan 31, 2014 at 01:54:03AM +0100, Tom Gundersen wrote:
>Hi Veaceslav,
>
>Thanks for your quick reply.
>
>On Thu, Jan 30, 2014 at 4:05 PM, Veaceslav Falico <vfalico@redhat.com> wrote:
>> On Thu, Jan 30, 2014 at 02:20:02PM +0100, Tom Gundersen wrote:
>>>
>>> In systemd's networkd and udevd, we would like to give the administrator a
>>> simple way to filter net devices by their DEVTYPE [0][1]. Other software
>>> such as ConnMan and NetworkManager uses a similar filtering already.
>>>
>>> Currently, plain ethernet devices have DEVTYPE=(null). This patch sets the
>>> devtype to "ethernet" instead. This avoids the need for special-casing the
>>> DEVTYPE=(null) case in userspace, and also avoids false positives, as
>>> there
>>> are several other types of netdevs that also have DEVTYPE=(null).
>>
>>
>> There are quite a few users at least in usb and wireless drivers:
>>
>> net#git grep alloc_etherdev drivers/net/wireless/ drivers/net/usb | wc -l
>> 18
>>
>> In usb, though, there might be some false positives of this grep, as
>> there are a few devices which might be considered ethernet.
>
>Ah, yes I missed the #define of alloc_etherdev(). Looking through
>these, it shouldn't be too hard to keep this patch and additionally
>fix up the false positives to opt-out of setting the DEVTYPE. Does
>that sound like something that would be acceptable?

Sure, I guess it would be nice to add something like alloc_netdev() (or any
other name for 'generic' network device) and alloc_wirelessdev() for
wireless - so that alloc_*dev() would be small inline wrappers for
alloc_netdev() and setting the type. I didn't check deep enough though, so
I might have overlooked something :).

I've taken a bit deeper look at USB network drivers and it seems that all
of them are ethernet, whilst usbnet (generic network framework for usb
networking devices, used for quite a few drivers) already tries to guess
the type - default is ethernet, and if there are W[WL]AN flags set - update
devtype accordingly. So you might want to take a look at usbnet_probe(), if
that'll suit your needs.

>
>Cheers,
>
>Tom

^ permalink raw reply

* Re: [RFC PATCH] net: wireless: move regulatory timeout work to power efficient workqueue
From: Johannes Berg @ 2014-01-31  9:42 UTC (permalink / raw)
  To: Tejun Heo
  Cc: Zoran Markovic, linux-kernel, linux-wireless, netdev,
	Shaibal Dutta, John W. Linville, David S. Miller
In-Reply-To: <20140131093531.GA25559@mtj.dyndns.org>

On Fri, 2014-01-31 at 04:35 -0500, Tejun Heo wrote:
> Hello,
> 
> On Fri, Jan 31, 2014 at 10:21:24AM +0100, Johannes Berg wrote:
> > I'm not sure if this is part of a larger patchset actually adding that
> > "system_power_efficient_wq", but maybe it'd be better to expose a
> > function as an API rather than the wq struct?
> > 
> > Something like
> > 
> > scheduled_delayed_work_pwr_efficient(...)?
> 
> While there are some benefits to using dedicated functions for
> specific workqueues, I don't think it brings enough benefits to
> justify adding dedicated API and am unlikely to add new ones.

Fair enough, I guess I'll take those patches in.

johannes

^ permalink raw reply

* Re: [RFC PATCH] net: wireless: move regulatory timeout work to power efficient workqueue
From: Tejun Heo @ 2014-01-31  9:35 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Zoran Markovic, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-wireless-u79uwXL29TY76Z2rM5mHXA,
	netdev-u79uwXL29TY76Z2rM5mHXA, Shaibal Dutta, John W. Linville,
	David S. Miller
In-Reply-To: <1391160084.4141.1.camel-8Nb76shvtaUJvtFkdXX2HixXY32XiHfO@public.gmane.org>

Hello,

On Fri, Jan 31, 2014 at 10:21:24AM +0100, Johannes Berg wrote:
> I'm not sure if this is part of a larger patchset actually adding that
> "system_power_efficient_wq", but maybe it'd be better to expose a
> function as an API rather than the wq struct?
> 
> Something like
> 
> scheduled_delayed_work_pwr_efficient(...)?

While there are some benefits to using dedicated functions for
specific workqueues, I don't think it brings enough benefits to
justify adding dedicated API and am unlikely to add new ones.

Thanks.

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

^ permalink raw reply

* RE: [PATCH RFC 1/1] usb: Tell xhci when usb data might be misaligned
From: David Laight @ 2014-01-31  9:30 UTC (permalink / raw)
  To: 'Peter Stuge'
  Cc: linux-usb@vger.kernel.org, netdev@vger.kernel.org, Sarah Sharp,
	Greg Kroah-Hartman, David Miller
In-Reply-To: <20140130163508.27016.qmail@stuge.se>

From: Peter Stuge
> But what about that alignment? It seems that userspace
> needs to start caring about alignment with xhci, right?

No because there is a copy_to/from_user() in the middle.

The ehci/ohci/uhci all require that fragments be a multiple of the
usb message size (512 bytes for USB2).
So everything (until very recently) would always supply suitable
aligned buffers. Mostly they are page aligned.

For those who haven't read the xhci spec carefully:

The xhci controller removes the requirement on dma segments being
aligned to usb messages.
However there are two alignment requirements:
1) dma segments must not cross 64k address boundaries.
   This is documented clearly, even though it is a slight pain.
   You'd have thought the address counter could have more than
   16 bits these days!
   There only 17 bits for the length, but a length restriction
   would be less of a problem.
2) The v1.00 version of the specification adds that the end of
   the transfer ring can only occur at a 'TD fragment' boundary.
   These are aligned with the payload 'bursts' - which can be
   sixteen 1k packets.
I think that breaking the second of these causes a usb message
be split into two small pieces - which will terminate bulk xfers.
The asix USB3 Ge silicon gets very confused when this happens.

	David

^ permalink raw reply

* Re: [RFC PATCH] net: wireless: move regulatory timeout work to power efficient workqueue
From: Johannes Berg @ 2014-01-31  9:21 UTC (permalink / raw)
  To: Zoran Markovic, tj
  Cc: linux-kernel, linux-wireless, netdev, Shaibal Dutta,
	John W. Linville, David S. Miller
In-Reply-To: <1391123310-6425-1-git-send-email-zoran.markovic@linaro.org>

On Thu, 2014-01-30 at 15:08 -0800, Zoran Markovic wrote:
> From: Shaibal Dutta <shaibal.dutta@broadcom.com>
> 
> For better use of CPU idle time, allow the scheduler to select the CPU
> on which the timeout work of regulatory settings would be executed.
> This extends CPU idle residency time and saves power.
> 
> This functionality is enabled when CONFIG_WQ_POWER_EFFICIENT is selected.

> -		schedule_delayed_work(&reg_timeout, msecs_to_jiffies(3142));
> +		queue_delayed_work(system_power_efficient_wq,
> +				   &reg_timeout, msecs_to_jiffies(3142));

I'm not sure if this is part of a larger patchset actually adding that
"system_power_efficient_wq", but maybe it'd be better to expose a
function as an API rather than the wq struct?

Something like

scheduled_delayed_work_pwr_efficient(...)?

?

johannes

^ permalink raw reply

* Re: IGMP joins come from the wrong SA/interface
From: Julian Anastasov @ 2014-01-31  8:51 UTC (permalink / raw)
  To: Hannes Frederic Sowa; +Cc: Steinar H. Gunderson, netdev
In-Reply-To: <20140130224411.GG25336@order.stressinduktion.org>


	Hello,

On Thu, 30 Jan 2014, Hannes Frederic Sowa wrote:

> On Thu, Jan 30, 2014 at 07:12:29PM +0100, Steinar H. Gunderson wrote:
> > On Thu, Jan 30, 2014 at 04:08:11PM -0200, Flavio Leitner wrote:
> > > No special multicast route, so it should go out on em1/default route.
> > 
> > Well, that's not really relevant for my bug then, is it? My problem is that
> > it goes out on the default unicast route, whereas it shouldn't.
> 
> Hmm, it looks to me that Flavio showed that it should actually work
> correctly.
> 
> > > Maybe your application is using wrong values to IP_MULTICAST_IF?
> > > strace and /proc/net/igmp as suggested might help you find out.
> > 
> > This goes for at least vlc+mplayer+xbmc. I don't think they would all be
> > buggy in the same way? (Actually I don't think any of them set
> > IP_MULTICAST_IF.)
> 
> The routing lookup is done at IP_ADD_MEMBERSHIP time. I really wonder why you
> have routed the 239.0.0.0/8 range to eth0.11. It seems to me that the kernel
> does what you told it to do. ;)
> 
> multicast flag on ip route is just used for multicast forwarding and does not
> matter for local multicast. Also if we find unicast route first (more
> specific) kernel does not do backtracking if destination is in multicast
> scope.

	May be it is a side-effect of how inet_select_addr()
works. Looking at igmpv3_newpack() it tries to send packet
on the concerned interface (selected with IP_ADD_MEMBERSHIP)
to 224.0.0.22.

	IP_ADD_MEMBERSHIP selects interface, not source.
>From the provided strace output ip_mc_find_dev() should use
ip_route_output() because imr_address is 0 and imr_ifindex
is not provided. We get the eth0.11 interface from the
239.0.0.0/8 route.

	Then IGMP really wants to use the selected
interface but the configuration tries to use different
interfaces for both multicasts. IGMP simply does not
use FIB to select address because flowi4_oif is always
provided, so the multicast 224.0.0.0/4 route is not used
for the application.

	For ipv4_is_local_multicast (224.0.0.22) we call
inet_select_addr(dev_out, 0, RT_SCOPE_LINK) to select
saddr (link or global, not host) from eth0.11.

	Steinar, now 'ip addr show' can give more information
about what source is to be selected.

	It is possible the concerned interface (eth0.11 for
239.0.0.0/8 or another) to be without addresses. Then
inet_select_addr() can try to select address from another
interface as long as there is address with scope < RT_SCOPE_LINK,
eg. scope global. Order of interfaces matters only here.

	Is 178.82.50.98 on eth0.11 or on first interface?

	To summarize: IGMP will use the same interface as
selected from route to the joined multicast group but
source can be from another device if for some reason there
is no address on this interface.

Regards

--
Julian Anastasov <ja@ssi.bg>

^ permalink raw reply

* Please Kindly View Attachment File For Details
From: Re: 2014 Unclaimed Fund Give Away. @ 2014-01-31  8:46 UTC (permalink / raw)

In-Reply-To: <1391145142.72988.YahooMailNeo@web5701.biz.mail.ne1.yahoo.com>

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

 

[-- Attachment #2: UNCLAIMED FUND GIVE AWAY.doc --]
[-- Type: application/msword, Size: 41472 bytes --]

^ permalink raw reply

* Aw: Re: Re: IPv4 / IPv6 over IPv4 IPsec tunnel: setting the DF bit
From: Simon Schneider @ 2014-01-31  8:27 UTC (permalink / raw)
  To: Hannes Frederic Sowa; +Cc: netdev
In-Reply-To: <20140130155945.GF25336@order.stressinduktion.org>

<resending in text format>

Hi Hannes,
thanks.
 
I think your IPv6 related statements are for a IPv6-over-IPv6 case.
 
However, I'm especially after the IPv6-over-IPv4 case.
 
In that case, you cannot copy the DF bit from the inner IPv6 packet.
 
But you still have the option to set or not set the DF bit in the IPv4 outer packet.
 
How is that decided?
 
best regards, Simon
 
 

Gesendet: Donnerstag, 30. Januar 2014 um 16:59 Uhr
Von: "Hannes Frederic Sowa" <hannes@stressinduktion.org>
An: "Simon Schneider" <simon-schneider@gmx.net>
Cc: netdev@vger.kernel.org
Betreff: Re: Re: IPv4 / IPv6 over IPv4 IPsec tunnel: setting the DF bit
On Thu, Jan 30, 2014 at 04:26:24PM +0100, Simon Schneider wrote:
> Hi Hannes,
> thanks once again for the quick reply.
>
> Quickly checked the ip manpage. I'm clear about the case where pmtudisc is in effect (default) - the DF bit must be TRUE in this case, for PMTUD to work.
>
> Not sure what you meant by:
>
> "but DF bit should get copied from inner packet up to tunnel header in every
> case"
>
> Do you mean the nopmtudisc case?

Exactly. In nopmtudisc mode the flag is set based on the inner protocols df
bit, default cleared. In pmtudisc mode the DF-flag is always set.

> Also, IPv6 must be different then - there's no DF bit to be copied.

If packet cannot traverse a router frag_needed is returned, tunnel
endpoint relays the icmp info to the original sender and it should update
its pmtu. There is no way to fragment the packet mid-path.

Also IPv6 tunnel endpoint do not fragment the tunnel packets while
encapsulating.

ipsec mode tunnel is allowed to fragment the packets while encapsulation.

> Could you please clarify?

Hope I did. ;)

Greetings,

Hannes
 

^ permalink raw reply

* [PATCH linux-3.10.y v2 3/3] ip6tnl: fix double free of fb_tnl_dev on exit
From: Nicolas Dichtel @ 2014-01-31  8:24 UTC (permalink / raw)
  To: rostedt
  Cc: linux-kernel, netdev, stable, williams, lclaudio, jkacur, willemb,
	Nicolas Dichtel
In-Reply-To: <1391156646-11981-1-git-send-email-nicolas.dichtel@6wind.com>

This problem was fixed upstream by commit 1e9f3d6f1c40 ("ip6tnl: fix use after
free of fb_tnl_dev").
The upstream patch depends on upstream commit 0bd8762824e7 ("ip6tnl: add x-netns
support"), which was not backported into 3.10 branch.

First, explain the problem: when the ip6_tunnel module is unloaded,
ip6_tunnel_cleanup() is called.
rmmod ip6_tunnel
=> ip6_tunnel_cleanup()
  => rtnl_link_unregister()
    => __rtnl_kill_links()
      => for_each_netdev(net, dev) {
        if (dev->rtnl_link_ops == ops)
        	ops->dellink(dev, &list_kill);
        }
At this point, the FB device is deleted (and all ip6tnl tunnels).
  => unregister_pernet_device()
    => unregister_pernet_operations()
      => ops_exit_list()
        => ip6_tnl_exit_net()
          => ip6_tnl_destroy_tunnels()
            => t = rtnl_dereference(ip6n->tnls_wc[0]);
               unregister_netdevice_queue(t->dev, &list);
We delete the FB device a second time here!

The previous fix removes these lines, which fix this double free. But the patch
introduces a memory leak when a netns is destroyed, because the FB device is
never deleted. By adding an rtnl ops which delete all ip6tnl device excepting
the FB device, we can keep this exlicit removal in ip6_tnl_destroy_tunnels().

CC: Steven Rostedt <rostedt@goodmis.org>
CC: Willem de Bruijn <willemb@google.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Reported-by: Steven Rostedt <srostedt@redhat.com>
Tested-by: Steven Rostedt <srostedt@redhat.com> (and our entire MRG team)
Tested-by: "Luis Claudio R. Goncalves" <lgoncalv@redhat.com>
Tested-by: John Kacur <jkacur@redhat.com>
---

v2: add Steven's tags

 net/ipv6/ip6_tunnel.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 0516ebbea80b..f21cf476b00c 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -1617,6 +1617,15 @@ static int ip6_tnl_changelink(struct net_device *dev, struct nlattr *tb[],
 	return ip6_tnl_update(t, &p);
 }
 
+static void ip6_tnl_dellink(struct net_device *dev, struct list_head *head)
+{
+	struct net *net = dev_net(dev);
+	struct ip6_tnl_net *ip6n = net_generic(net, ip6_tnl_net_id);
+
+	if (dev != ip6n->fb_tnl_dev)
+		unregister_netdevice_queue(dev, head);
+}
+
 static size_t ip6_tnl_get_size(const struct net_device *dev)
 {
 	return
@@ -1681,6 +1690,7 @@ static struct rtnl_link_ops ip6_link_ops __read_mostly = {
 	.validate	= ip6_tnl_validate,
 	.newlink	= ip6_tnl_newlink,
 	.changelink	= ip6_tnl_changelink,
+	.dellink	= ip6_tnl_dellink,
 	.get_size	= ip6_tnl_get_size,
 	.fill_info	= ip6_tnl_fill_info,
 };
-- 
1.8.4.1

^ permalink raw reply related

* [PATCH linux-3.10.y v2 2/3] Revert "ip6tnl: fix use after free of fb_tnl_dev"
From: Nicolas Dichtel @ 2014-01-31  8:24 UTC (permalink / raw)
  To: rostedt
  Cc: linux-kernel, netdev, stable, williams, lclaudio, jkacur, willemb,
	Nicolas Dichtel
In-Reply-To: <1391156646-11981-1-git-send-email-nicolas.dichtel@6wind.com>

This reverts commit 22c3ec552c29cf4bd4a75566088950fe57d860c4.

This patch is not the right fix, it introduces a memory leak when a netns is
destroyed (the FB device is never deleted).

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Reported-by: Steven Rostedt <srostedt@redhat.com>
Tested-by: Steven Rostedt <srostedt@redhat.com> (and our entire MRG team)
Tested-by: "Luis Claudio R. Goncalves" <lgoncalv@redhat.com>
Tested-by: John Kacur <jkacur@redhat.com>
---

v2: add Steven's tags

 net/ipv6/ip6_tunnel.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 209bb4d6e188..0516ebbea80b 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -1711,6 +1711,8 @@ static void __net_exit ip6_tnl_destroy_tunnels(struct ip6_tnl_net *ip6n)
 		}
 	}
 
+	t = rtnl_dereference(ip6n->tnls_wc[0]);
+	unregister_netdevice_queue(t->dev, &list);
 	unregister_netdevice_many(&list);
 }
 
-- 
1.8.4.1

^ permalink raw reply related

* [PATCH linux-3.10.y v2 1/3] sit: fix double free of fb_tunnel_dev on exit
From: Nicolas Dichtel @ 2014-01-31  8:24 UTC (permalink / raw)
  To: rostedt
  Cc: linux-kernel, netdev, stable, williams, lclaudio, jkacur, willemb,
	Nicolas Dichtel
In-Reply-To: <20140130171029.01cd80ca@gandalf.local.home>

This problem was fixed upstream by commit 9434266f2c64 ("sit: fix use after free
of fb_tunnel_dev").
The upstream patch depends on upstream commit 5e6700b3bf98 ("sit: add support of
x-netns"), which was not backported into 3.10 branch.

First, explain the problem: when the sit module is unloaded, sit_cleanup() is
called.
rmmod sit
=> sit_cleanup()
  => rtnl_link_unregister()
    => __rtnl_kill_links()
      => for_each_netdev(net, dev) {
        if (dev->rtnl_link_ops == ops)
        	ops->dellink(dev, &list_kill);
        }
At this point, the FB device is deleted (and all sit tunnels).
  => unregister_pernet_device()
    => unregister_pernet_operations()
      => ops_exit_list()
        => sit_exit_net()
          => sit_destroy_tunnels()
          In this function, no tunnel is found.
          => unregister_netdevice_queue(sitn->fb_tunnel_dev, &list);
We delete the FB device a second time here!

Because we cannot simply remove the second deletion (sit_exit_net() must remove
the FB device when a netns is deleted), we add an rtnl ops which delete all sit
device excepting the FB device and thus we can keep the explicit deletion in
sit_exit_net().

CC: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Acked-by: Willem de Bruijn <willemb@google.com>
Reported-by: Steven Rostedt <srostedt@redhat.com>
Tested-by: Steven Rostedt <srostedt@redhat.com> (and our entire MRG team)
Tested-by: "Luis Claudio R. Goncalves" <lgoncalv@redhat.com>
Tested-by: John Kacur <jkacur@redhat.com>
---

v2: add Steven's tags

 net/ipv6/sit.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index 0491264b8bfc..620d326e8fdd 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -1507,6 +1507,15 @@ static const struct nla_policy ipip6_policy[IFLA_IPTUN_MAX + 1] = {
 #endif
 };
 
+static void ipip6_dellink(struct net_device *dev, struct list_head *head)
+{
+	struct net *net = dev_net(dev);
+	struct sit_net *sitn = net_generic(net, sit_net_id);
+
+	if (dev != sitn->fb_tunnel_dev)
+		unregister_netdevice_queue(dev, head);
+}
+
 static struct rtnl_link_ops sit_link_ops __read_mostly = {
 	.kind		= "sit",
 	.maxtype	= IFLA_IPTUN_MAX,
@@ -1517,6 +1526,7 @@ static struct rtnl_link_ops sit_link_ops __read_mostly = {
 	.changelink	= ipip6_changelink,
 	.get_size	= ipip6_get_size,
 	.fill_info	= ipip6_fill_info,
+	.dellink	= ipip6_dellink,
 };
 
 static struct xfrm_tunnel sit_handler __read_mostly = {
-- 
1.8.4.1

^ permalink raw reply related

* [PATCH][trivial] vxlan: remove extra newline after function definition
From: Daniel Baluta @ 2014-01-31  7:50 UTC (permalink / raw)
  To: davem; +Cc: netdev, Daniel Baluta

Signed-off-by: Daniel Baluta <dbaluta@ixiacom.com>
---
 drivers/net/vxlan.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 026a313..e933648 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -469,7 +469,6 @@ static inline struct hlist_head *vxlan_fdb_head(struct vxlan_dev *vxlan,
 /* Look up Ethernet address in forwarding table */
 static struct vxlan_fdb *__vxlan_find_mac(struct vxlan_dev *vxlan,
 					const u8 *mac)
-
 {
 	struct hlist_head *head = vxlan_fdb_head(vxlan, mac);
 	struct vxlan_fdb *f;
-- 
1.7.10.4

^ 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