Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH v2 net 1/2] net sched actions: fix access to uninitialized data
From: Daniel Borkmann @ 2017-04-19  9:02 UTC (permalink / raw)
  To: Wolfgang Bumiller, netdev; +Cc: Jamal Hadi Salim, David S. Miller, Cong Wang
In-Reply-To: <20170418101322.27666-1-w.bumiller@proxmox.com>

Hi Wolfgang,

On 04/18/2017 12:13 PM, Wolfgang Bumiller wrote:
[...]

Since this patch goes to -net tree and is a bug fix, please
also add a proper analysis into the commit log instead of
just an empty commit message.

A Fixes tag to your bug fix would also be appropriate, so
that this helps tracking follow-ups to the original commit
that introduced the code. For this one, should be:

Fixes: 1045ba77a596 ("net sched actions: Add support for user cookies")

> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> Acked-by: Cong Wang <xiyou.wangcong@gmail.com>
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

Thanks,
Daniel

^ permalink raw reply

* Re: [PATCH] dp83640: don't recieve time stamps twice
From: Sørensen, Stefan @ 2017-04-19  9:16 UTC (permalink / raw)
  To: dan.carpenter@oracle.com, richardcochran@gmail.com
  Cc: netdev@vger.kernel.org, f.fainelli@gmail.com, andrew@lunn.ch,
	kernel-janitors@vger.kernel.org
In-Reply-To: <20170418191426.GA17838@mwanda>

On Tue, 2017-04-18 at 22:14 +0300, Dan Carpenter wrote:
> This patch is prompted by a static checker warning about a potential
> use after free.  The concern is that netif_rx_ni() can free "skb" and
> we
> call it twice.
> 
> When I look at the commit that added this, it looks like some stray
> lines were added accidentally.  It doesn't make sense to me that we
> would recieve the same data two times.  I asked the author but never
> recieved a response.
> 
> I can't test this code, but I'm pretty sure my patch is correct.

Sorry for not getting back earlier, I have swamped with other stuff.

You are correct that these lines was added accidentally.

Acked-by: Stefan Sørensen <stefan.sorensen@spectralink.com>

> 
> Fixes: 4b063258ab93 ("dp83640: Delay scheduled work.")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
> index e2460a57e4b1..ed0d10f54f26 100644
> --- a/drivers/net/phy/dp83640.c
> +++ b/drivers/net/phy/dp83640.c
> @@ -1438,8 +1438,6 @@ static bool dp83640_rxtstamp(struct phy_device
> *phydev,
>  		skb_info->tmo = jiffies + SKB_TIMESTAMP_TIMEOUT;
>  		skb_queue_tail(&dp83640->rx_queue, skb);
>  		schedule_delayed_work(&dp83640->ts_work,
> SKB_TIMESTAMP_TIMEOUT);
> -	} else {
> -		netif_rx_ni(skb);
>  	}
>  
>  	return true;

^ permalink raw reply

* [PATCH] mdio_bus: Issue GPIO RESET to PHYs.
From: Roger Quadros @ 2017-04-19  9:24 UTC (permalink / raw)
  To: davem, Andrew Lunn, Florian Fainelli
  Cc: tony, nsekhar, jsarha, netdev, linux-omap, linux-kernel, rogerq
In-Reply-To: <1491381237-24635-1-git-send-email-rogerq@ti.com>

Some boards [1] leave the PHYs at an invalid state
during system power-up or reset thus causing unreliability
issues with the PHY which manifests as PHY not being detected
or link not functional. To fix this, these PHYs need to be RESET
via a GPIO connected to the PHY's RESET pin.

Some boards have a single GPIO controlling the PHY RESET pin of all
PHYs on the bus whereas some others have separate GPIOs controlling
individual PHY RESETs.

In both cases, the RESET de-assertion cannot be done in the PHY driver
as the PHY will not probe till its reset is de-asserted.
So do the RESET de-assertion in the MDIO bus driver.

[1] - am572x-idk, am571x-idk, a437x-idk

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/net/phy/mdio_bus.c | 26 ++++++++++++++++++++++++++
 drivers/of/of_mdio.c       |  4 ++++
 include/linux/phy.h        |  5 +++++
 3 files changed, 35 insertions(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index fa7d51f..25fda2b 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -22,8 +22,11 @@
 #include <linux/init.h>
 #include <linux/delay.h>
 #include <linux/device.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/of_device.h>
 #include <linux/of_mdio.h>
+#include <linux/of_gpio.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 #include <linux/skbuff.h>
@@ -43,6 +46,8 @@
 
 #include "mdio-boardinfo.h"
 
+#define DEFAULT_GPIO_RESET_DELAY	10	/* in microseconds */
+
 int mdiobus_register_device(struct mdio_device *mdiodev)
 {
 	if (mdiodev->bus->mdio_map[mdiodev->addr])
@@ -307,6 +312,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 {
 	struct mdio_device *mdiodev;
 	int i, err;
+	struct gpio_desc *gpiod;
 
 	if (NULL == bus || NULL == bus->name ||
 	    NULL == bus->read || NULL == bus->write)
@@ -333,6 +339,26 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 	if (bus->reset)
 		bus->reset(bus);
 
+	/* de-assert bus level PHY GPIO resets */
+	for (i = 0; i < bus->num_reset_gpios; i++) {
+		gpiod = devm_gpiod_get_index(&bus->dev, "reset", i,
+					     GPIOD_OUT_LOW);
+		if (IS_ERR(gpiod)) {
+			err = PTR_ERR(gpiod);
+			if (err != -ENOENT) {
+				pr_err("mii_bus %s couldn't get reset GPIO\n",
+				       bus->id);
+				return err;
+			}
+		} else {
+			gpiod_set_value_cansleep(gpiod, 1);
+			if (!bus->reset_delay_us)
+				bus->reset_delay_us = DEFAULT_GPIO_RESET_DELAY;
+			udelay(bus->reset_delay_us);
+			gpiod_set_value_cansleep(gpiod, 0);
+		}
+	}
+
 	for (i = 0; i < PHY_MAX_ADDR; i++) {
 		if ((bus->phy_mask & (1 << i)) == 0) {
 			struct phy_device *phydev;
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 0b29798..83a62e4 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -221,6 +221,10 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 
 	mdio->dev.of_node = np;
 
+	/* Get bus level PHY reset GPIO details */
+	of_property_read_u32(np, "reset-delay-us", &mdio->reset_delay_us);
+	mdio->num_reset_gpios = of_gpio_named_count(np, "reset-gpios");
+
 	/* Register the MDIO bus */
 	rc = mdiobus_register(mdio);
 	if (rc)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 43a7748..80a6574 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -217,6 +217,11 @@ struct mii_bus {
 	 * matching its address
 	 */
 	int irq[PHY_MAX_ADDR];
+
+	/* GPIO reset pulse width in uS */
+	int reset_delay_us;
+	/* Number of reset GPIOs */
+	int num_reset_gpios;
 };
 #define to_mii_bus(d) container_of(d, struct mii_bus, dev)
 
-- 
2.7.4

^ permalink raw reply related

* Re: [PATCH 2/2] sparc64: Add eBPF JIT.
From: Daniel Borkmann @ 2017-04-19  9:35 UTC (permalink / raw)
  To: David Miller, sparclinux; +Cc: netdev, ast
In-Reply-To: <20170418.145823.444134784458713460.davem@davemloft.net>

On 04/18/2017 08:58 PM, David Miller wrote:
>
> This is an eBPF JIT for sparc64.  All major features are supported
> except for tail calls.
>
> test_bpf passes with no failures and all tests are JIT'd, both with
> and without hardening enabled.
>
> Signed-off-by: David S. Miller <davem@davemloft.net>
[...]

While going over the code again, I noticed two minor
things that could still be changed before applying:

> +struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
> +{
> +	struct bpf_prog *tmp, *orig_prog = prog;
> +	struct bpf_binary_header *header;
> +	bool tmp_blinded = false;
> +	struct jit_ctx ctx;
> +	u32 image_size;
> +	u8 *image_ptr;
> +	int pass;
> +
> +	if (!bpf_jit_enable)
> +		return orig_prog;
> +
> +	if (!prog || !prog->len)
> +		return orig_prog;

This condition can be removed, see also 93a73d442d37
("bpf, x86/arm64: remove useless checks on prog"), since
there's no way we could land here under such circumstance.

> +	tmp = bpf_jit_blind_constants(prog);
> +	/* If blinding was requested and we failed during blinding,
> +	 * we must fall back to the interpreter.
> +	 */
> +	if (IS_ERR(tmp))
> +		return orig_prog;
> +	if (tmp != prog) {
> +		tmp_blinded = true;
> +		prog = tmp;
> +	}
> +
> +	memset(&ctx, 0, sizeof(ctx));
> +	ctx.prog = prog;
> +
> +	ctx.offset = kcalloc(prog->len, sizeof(unsigned int), GFP_KERNEL);
> +	if (ctx.offset == NULL) {
> +		prog = orig_prog;
> +		goto out;
> +	}
> +
> +	/* Fake pass to detect features used, and get an accurate assessment
> +	 * of what the final image size will be.
> +	 */
> +	if (build_body(&ctx)) {
> +		prog = orig_prog;
> +		goto out_off;
> +	}
> +	build_prologue(&ctx);
> +	build_epilogue(&ctx);
> +
> +	/* Now we know the actual image size. */
> +	image_size = sizeof(u32) * ctx.idx;
> +	header = bpf_jit_binary_alloc(image_size, &image_ptr,
> +				      sizeof(u32), jit_fill_hole);
> +	if (header == NULL) {
> +		prog = orig_prog;
> +		goto out_off;
> +	}
> +
> +	ctx.image = (u32 *)image_ptr;
> +
> +	for (pass = 1; pass < 3; pass++) {
> +		ctx.idx = 0;
> +
> +		build_prologue(&ctx);
> +
> +		if (build_body(&ctx)) {
> +			bpf_jit_binary_free(header);
> +			prog = orig_prog;
> +			goto out_off;
> +		}
> +
> +		build_epilogue(&ctx);
> +
> +		if (bpf_jit_enable > 1)
> +			pr_info("Pass %d: shrink = %d, seen = [%c%c%c%c%c]\n", pass,
> +				image_size - (ctx.idx * 4),
> +				ctx.tmp_1_used ? '1' : ' ',
> +				ctx.tmp_2_used ? '2' : ' ',
> +				ctx.tmp_3_used ? '3' : ' ',
> +				ctx.saw_ld_abs_ind ? 'L' : ' ',
> +				ctx.saw_frame_pointer ? 'F' : ' ');
> +	}
> +
> +	if (bpf_jit_enable > 1)
> +		bpf_jit_dump(prog->len, image_size, pass, ctx.image);
> +	bpf_flush_icache(ctx.image, ctx.image + image_size);

Since remaining parts were filled through jit_fill_hole(),
it would be better / more correct to flush the whole buffer,
see also the recent ppc64 commit 10528b9c45cf ("powerpc/bpf:
Flush the entire JIT buffer") that fixed it for their jit.

> +	bpf_jit_binary_lock_ro(header);
> +
> +	prog->bpf_func = (void *)ctx.image;
> +	prog->jited = 1;
> +
> +out_off:
> +	kfree(ctx.offset);

Thanks a lot,
Daniel

^ permalink raw reply

* Re: [PATCH v2] smsc95xx: Use skb_cow_head to deal with cloned skbs
From: James Hughes @ 2017-04-19  9:40 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Woojung.Huh, netdev, Steve Glendinning,
	Microchip Linux Driver Support
In-Reply-To: <CAE_XsMJhCVU4eHtiO7gs3=mRvA9vN4kiuh2JhXf+F_YgVXcWhQ@mail.gmail.com>

On 19 April 2017 at 09:33, James Hughes <james.hughes@raspberrypi.org> wrote:
> On 18 April 2017 at 23:46, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> On Tue, 2017-04-18 at 22:09 +0000, Woojung.Huh@microchip.com wrote:
>>> > > @@ -2067,13 +2067,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct
>>> > usbnet *dev,
>>> > >   /* We do not advertise SG, so skbs should be already linearized */
>>> > >   BUG_ON(skb_shinfo(skb)->nr_frags);
>>> > >
>>> > > - if (skb_headroom(skb) < overhead) {
>>> > > -         struct sk_buff *skb2 = skb_copy_expand(skb,
>>> > > -                 overhead, 0, flags);
>>> > > -         dev_kfree_skb_any(skb);
>>> > > -         skb = skb2;
>>> > > -         if (!skb)
>>> > > -                 return NULL;
>>> > > + /* Make writable and expand header space by overhead if required
>>> > */
>>> > > + if (skb_cow_head(skb, overhead)) {
>>> >
>>> > I believe you still need to
>>> >             dev_kfree_skb_any(skb);
>>> >
>>> I think caller of usbnet_start_xmit() takes care of free when return NULL.
>>
>> I do not think so. Here is the code in usbnet_start_xmit() :
>>
>> if (info->tx_fixup) {
>>    skb = info->tx_fixup (dev, skb, GFP_ATOMIC);
>>
>>    if (!skb) { // Note that skb is NULL now
>>
>>        if (info->flags & FLAG_MULTI_PACKET)
>>              goto not_drop;
>>         netif_dbg(dev, tx_err, dev->net, "can't tx_fixup skb\n");
>>         goto drop;
>>
>>
>> If you really think about it (even before double checking the code in
>> usbnet_start_xmit()), that would have been a very serious bug.
>>
>> Calling dev_kfree_skb_any(skb) in this fixup code, then later from
>> usbnet_start_xmit() would have been a double free, or use after free.
>>
>>
>
> So, I still need to return NULL (as per the code this is replacing) to
> indicate failure, but need to free the skb prior to return, as per
> fragment below.
>
> /* Make writable and expand header space by overhead if required */
> if (skb_cow_head(skb, overhead)) {
>    dev_kfree_skb_any(skb);
>    return NULL;
> }
>
> Once confirmed, I'll do a another patch.

Actually, having checked the code path, doing the free here does look
like the correct operation. Returning NULL indicates to the caller
that the call has failed, but also means the skb is not deallocated
later on. It's seems rather a clunky mechanism for the driver to have
to deallocate, but I guess that's 'just the way it is'.

I'll do another patch.

^ permalink raw reply

* [PATCH net-next] qede: allocate enough data for ->arfs_fltr_bmap
From: Dan Carpenter @ 2017-04-19  9:54 UTC (permalink / raw)
  To: Yuval Mintz, Chopra, Manish
  Cc: Ariel Elior, everest-linux-l2, netdev, kernel-janitors

We've got the number of longs, yes, but we should multiply by
sizeof(long) to get the number of bytes needed.

Fixes: e4917d46a653 ("qede: Add aRFS support")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Btw, this driver use vmalloc() all over the place but kmalloc() is more
appropriate when you're allocating tiny ammounts.

diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c b/drivers/net/ethernet/qlogic/qede/qede_filter.c
index 8c594a3ca63b..34473fbac798 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c
@@ -267,7 +267,8 @@ int qede_alloc_arfs(struct qede_dev *edev)
 		return -ENOMEM;
 	}
 
-	edev->arfs->arfs_fltr_bmap = vzalloc(BITS_TO_LONGS(QEDE_RFS_MAX_FLTR));
+	edev->arfs->arfs_fltr_bmap = vzalloc(BITS_TO_LONGS(QEDE_RFS_MAX_FLTR) *
+					     sizeof(long));
 	if (!edev->arfs->arfs_fltr_bmap) {
 		free_irq_cpu_rmap(edev->ndev->rx_cpu_rmap);
 		edev->ndev->rx_cpu_rmap = NULL;

^ permalink raw reply related

* [PATCH net-next] net/mlx5e: IPoIB, Fix error handling in mlx5_rdma_netdev_alloc()
From: Dan Carpenter @ 2017-04-19  9:59 UTC (permalink / raw)
  To: Saeed Mahameed
  Cc: Matan Barak, Leon Romanovsky, netdev, linux-rdma, kernel-janitors

The labels were out of order, so it either could result in an Oops or a
leak.

Fixes: 48935bbb7ae8 ("net/mlx5e: IPoIB, Add netdevice profile skeleton")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/ipoib.c b/drivers/net/ethernet/mellanox/mlx5/core/ipoib.c
index 001d2953cb6d..ec78e637840f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/ipoib.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/ipoib.c
@@ -471,10 +471,11 @@ struct net_device *mlx5_rdma_netdev_alloc(struct mlx5_core_dev *mdev,
 	 */
 	return netdev;
 
-free_mdev_resources:
-	mlx5e_destroy_mdev_resources(mdev);
 err_free_netdev:
 	free_netdev(netdev);
+free_mdev_resources:
+	mlx5e_destroy_mdev_resources(mdev);
+
 	return NULL;
 }
 EXPORT_SYMBOL(mlx5_rdma_netdev_alloc);

^ permalink raw reply related

* [PATCH v3] smsc95xx: Use skb_cow_head to deal with cloned skbs
From: James Hughes @ 2017-04-19 10:07 UTC (permalink / raw)
  To: netdev, Steve Glendinning, Microchip Linux Driver Support; +Cc: James Hughes

The driver was failing to check that the SKB wasn't cloned
before adding checksum data.
Replace existing handling to extend/copy the header buffer
with skb_cow_head.

Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
---
Change for V3
- Added the dev_kfree_skb_any, otherwise the skb would not 
  be deallocated due to returning NULL.

 drivers/net/usb/smsc95xx.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index df60c98..4cb8144 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -2067,13 +2067,13 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
 	/* We do not advertise SG, so skbs should be already linearized */
 	BUG_ON(skb_shinfo(skb)->nr_frags);
 
-	if (skb_headroom(skb) < overhead) {
-		struct sk_buff *skb2 = skb_copy_expand(skb,
-			overhead, 0, flags);
+	/* Make writable and expand header space by overhead if required */
+	if (skb_cow_head(skb, overhead)) {
+		/* Must reallocate here as returning NULL to indicate error
+		 * means the skb won't be deallocated in the caller.
+		 */
 		dev_kfree_skb_any(skb);
-		skb = skb2;
-		if (!skb)
-			return NULL;
+		return NULL;
 	}
 
 	if (csum) {
-- 
2.9.3

^ permalink raw reply related

* Re: [PATCH net-next] net/mlx5e: IPoIB, Fix error handling in mlx5_rdma_netdev_alloc()
From: Leon Romanovsky @ 2017-04-19 10:11 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Saeed Mahameed, Matan Barak, netdev, linux-rdma, kernel-janitors
In-Reply-To: <20170419095914.GB4266@mwanda>

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

On Wed, Apr 19, 2017 at 12:59:15PM +0300, Dan Carpenter wrote:
> The labels were out of order, so it either could result in an Oops or a
> leak.
>
> Fixes: 48935bbb7ae8 ("net/mlx5e: IPoIB, Add netdevice profile skeleton")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>

Thanks,
Acked-by: Leon Romanovsky <leonro@mellanox.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply

* RE: [PATCH net-next] qede: allocate enough data for ->arfs_fltr_bmap
From: Mintz, Yuval @ 2017-04-19 10:12 UTC (permalink / raw)
  To: Dan Carpenter, Chopra, Manish
  Cc: Elior, Ariel, netdev@vger.kernel.org,
	kernel-janitors@vger.kernel.org, Chopra@mwanda
In-Reply-To: <20170419095432.GA4266@mwanda>

> We've got the number of longs, yes, but we should multiply by
> sizeof(long) to get the number of bytes needed.
> 
> Fixes: e4917d46a653 ("qede: Add aRFS support")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Thanks Dan.

Acked-by: Yuval Mintz <Yuval.Mintz@cavium.com>

^ permalink raw reply

* [PATCH v4] smsc95xx: Use skb_cow_head to deal with cloned skbs
From: James Hughes @ 2017-04-19 10:13 UTC (permalink / raw)
  To: netdev, Steve Glendinning, Microchip Linux Driver Support; +Cc: James Hughes

The driver was failing to check that the SKB wasn't cloned
before adding checksum data.
Replace existing handling to extend/copy the header buffer
with skb_cow_head.

Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
---
Changes 
v3 
 - Added the dev_kfree_skb_any call to ensure skb is deallocated 
   as when returning NULL the caller won't deallocate
v4 
 - Fix typo in comment (reallocate to deallocate) 

 drivers/net/usb/smsc95xx.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index df60c98..f6661e3 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -2067,13 +2067,13 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
 	/* We do not advertise SG, so skbs should be already linearized */
 	BUG_ON(skb_shinfo(skb)->nr_frags);
 
-	if (skb_headroom(skb) < overhead) {
-		struct sk_buff *skb2 = skb_copy_expand(skb,
-			overhead, 0, flags);
+	/* Make writable and expand header space by overhead if required */
+	if (skb_cow_head(skb, overhead)) {
+		/* Must deallocate here as returning NULL to indicate error
+		 * means the skb won't be deallocated in the caller.
+		 */
 		dev_kfree_skb_any(skb);
-		skb = skb2;
-		if (!skb)
-			return NULL;
+		return NULL;
 	}
 
 	if (csum) {
-- 
2.9.3

^ permalink raw reply related

* [PATCH net 1/4] qed: Fix possible error in populating max_tc field.
From: Sudarsana Reddy Kalluru @ 2017-04-19 10:19 UTC (permalink / raw)
  To: davem; +Cc: netdev, Yuval.Mintz
In-Reply-To: <20170419101955.26444-1-sudarsana.kalluru@cavium.com>

Some adapters may not publish the max_tc value. Populate the default
value for max_tc field in case the mfw didn't provide one.

Signed-off-by: Sudarsana Reddy Kalluru <Sudarsana.Kalluru@cavium.com>
Signed-off-by: Yuval Mintz <Yuval.Mintz@cavium.com>
---
 drivers/net/ethernet/qlogic/qed/qed_dcbx.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_dcbx.c b/drivers/net/ethernet/qlogic/qed/qed_dcbx.c
index 5bd36a4..c24436c 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_dcbx.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_dcbx.c
@@ -583,6 +583,13 @@ static int qed_dcbx_process_mib_info(struct qed_hwfn *p_hwfn)
 		   p_params->ets_cbs,
 		   p_ets->pri_tc_tbl[0], p_params->max_ets_tc);
 
+	if (p_params->ets_enabled && !p_params->max_ets_tc) {
+		p_params->max_ets_tc = QED_MAX_PFC_PRIORITIES;
+		DP_VERBOSE(p_hwfn, QED_MSG_DCB,
+			   "ETS params: max_ets_tc is forced to %d\n",
+		p_params->max_ets_tc);
+	}
+
 	/* 8 bit tsa and bw data corresponding to each of the 8 TC's are
 	 * encoded in a type u32 array of size 2.
 	 */
-- 
1.8.3.1

^ permalink raw reply related

* [PATCH net 0/4] qed: Dcbx bug fixes
From: Sudarsana Reddy Kalluru @ 2017-04-19 10:19 UTC (permalink / raw)
  To: davem; +Cc: netdev, Yuval.Mintz, Sudarsana Reddy Kalluru

From: Sudarsana Reddy Kalluru <Sudarsana.Kalluru@cavium.com>

The series has set of bug fixes for dcbx implementation of qed driver.
Please consider applying this to 'net' branch.

Sudarsana Reddy Kalluru (4):
  qed: Fix possible error in populating max_tc field.
  qed: Fix sending an invalid PFC error mask to MFW.
  qed: Fix possible system hang in the dcbnl-getdcbx() path.
  qed: Fix issue in populating the PFC config paramters.

 drivers/net/ethernet/qlogic/qed/qed_dcbx.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

-- 
1.8.3.1

^ permalink raw reply

* [PATCH net 2/4] qed: Fix sending an invalid PFC error mask to MFW.
From: Sudarsana Reddy Kalluru @ 2017-04-19 10:19 UTC (permalink / raw)
  To: davem; +Cc: netdev, Yuval.Mintz
In-Reply-To: <20170419101955.26444-1-sudarsana.kalluru@cavium.com>

PFC error-mask value is not supported by MFW, but this bit could be
set in the pfc bit-map of the operational parameters if remote device
supports it. These operational parameters are used as basis for
populating the dcbx config parameters. User provided configs will be
applied on top of these parameters and then send them to MFW when
requested. Driver need to clear the error-mask bit before sending the
config parameters to MFW.

Signed-off-by: Sudarsana Reddy Kalluru <Sudarsana.Kalluru@cavium.com>
Signed-off-by: Yuval Mintz <Yuval.Mintz@cavium.com>
---
 drivers/net/ethernet/qlogic/qed/qed_dcbx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_dcbx.c b/drivers/net/ethernet/qlogic/qed/qed_dcbx.c
index c24436c..ff058a3 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_dcbx.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_dcbx.c
@@ -1008,6 +1008,8 @@ static int qed_dcbx_query_params(struct qed_hwfn *p_hwfn,
 	u8 pfc_map = 0;
 	int i;
 
+	*pfc &= ~DCBX_PFC_ERROR_MASK;
+
 	if (p_params->pfc.willing)
 		*pfc |= DCBX_PFC_WILLING_MASK;
 	else
-- 
1.8.3.1

^ permalink raw reply related

* [PATCH net 4/4] qed: Fix issue in populating the PFC config paramters.
From: Sudarsana Reddy Kalluru @ 2017-04-19 10:19 UTC (permalink / raw)
  To: davem; +Cc: netdev, Yuval.Mintz
In-Reply-To: <20170419101955.26444-1-sudarsana.kalluru@cavium.com>

Change ieee_setpfc() callback implementation to populate traffic class
count with the user provided value.

Signed-off-by: Sudarsana Reddy Kalluru <Sudarsana.Kalluru@cavium.com>
Signed-off-by: Yuval Mintz <Yuval.Mintz@cavium.com>
---
 drivers/net/ethernet/qlogic/qed/qed_dcbx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_dcbx.c b/drivers/net/ethernet/qlogic/qed/qed_dcbx.c
index 8f0783a..a6e2bbe 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_dcbx.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_dcbx.c
@@ -2082,6 +2082,8 @@ static int qed_dcbnl_ieee_setpfc(struct qed_dev *cdev, struct ieee_pfc *pfc)
 	for (i = 0; i < QED_MAX_PFC_PRIORITIES; i++)
 		dcbx_set.config.params.pfc.prio[i] = !!(pfc->pfc_en & BIT(i));
 
+	dcbx_set.config.params.pfc.max_tc = pfc->pfc_cap;
+
 	ptt = qed_ptt_acquire(hwfn);
 	if (!ptt)
 		return -EINVAL;
-- 
1.8.3.1

^ permalink raw reply related

* [PATCH net 3/4] qed: Fix possible system hang in the dcbnl-getdcbx() path.
From: Sudarsana Reddy Kalluru @ 2017-04-19 10:19 UTC (permalink / raw)
  To: davem; +Cc: netdev, Yuval.Mintz
In-Reply-To: <20170419101955.26444-1-sudarsana.kalluru@cavium.com>

qed_dcbnl_get_dcbx() API uses kmalloc in GFT_KERNEL mode. The API gets
invoked in the interrupt context by qed_dcbnl_getdcbx callback. Need
to invoke this kmalloc in atomic mode.

Signed-off-by: Sudarsana Reddy Kalluru <Sudarsana.Kalluru@cavium.com>
Signed-off-by: Yuval Mintz <Yuval.Mintz@cavium.com>
---
 drivers/net/ethernet/qlogic/qed/qed_dcbx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_dcbx.c b/drivers/net/ethernet/qlogic/qed/qed_dcbx.c
index ff058a3..8f0783a 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_dcbx.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_dcbx.c
@@ -1264,7 +1264,7 @@ static struct qed_dcbx_get *qed_dcbnl_get_dcbx(struct qed_hwfn *hwfn,
 {
 	struct qed_dcbx_get *dcbx_info;
 
-	dcbx_info = kzalloc(sizeof(*dcbx_info), GFP_KERNEL);
+	dcbx_info = kmalloc(sizeof(*dcbx_info), GFP_ATOMIC);
 	if (!dcbx_info)
 		return NULL;
 
-- 
1.8.3.1

^ permalink raw reply related

* Re: [PATCH] dp83640: don't recieve time stamps twice
From: Richard Cochran @ 2017-04-19 10:31 UTC (permalink / raw)
  To: Sørensen, Stefan
  Cc: dan.carpenter@oracle.com, netdev@vger.kernel.org,
	f.fainelli@gmail.com, andrew@lunn.ch,
	kernel-janitors@vger.kernel.org
In-Reply-To: <1492593414.3108.1.camel@spectralink.com>

On Wed, Apr 19, 2017 at 09:16:56AM +0000, Sørensen, Stefan wrote:
> You are correct that these lines was added accidentally.

Can we please fix this in another way?  There is no need to hold the
spin lock during the callback into the networking stack.  Instead, how
about the following diff, which also fixes the other call site...

Thanks,
Richard

---
diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index e2460a5..593e533 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -874,14 +874,15 @@ static void decode_rxts(struct dp83640_private *dp83640,
 			shhwtstamps = skb_hwtstamps(skb);
 			memset(shhwtstamps, 0, sizeof(*shhwtstamps));
 			shhwtstamps->hwtstamp = ns_to_ktime(rxts->ns);
-			netif_rx_ni(skb);
 			list_add(&rxts->list, &dp83640->rxpool);
 			break;
 		}
 	}
 	spin_unlock(&dp83640->rx_queue.lock);
 
-	if (!shhwtstamps)
+	if (shhwtstamps)
+		netif_rx_ni(skb);
+	else
 		list_add_tail(&rxts->list, &dp83640->rxts);
 out:
 	spin_unlock_irqrestore(&dp83640->rx_lock, flags);
@@ -1425,7 +1426,6 @@ static bool dp83640_rxtstamp(struct phy_device *phydev,
 			shhwtstamps = skb_hwtstamps(skb);
 			memset(shhwtstamps, 0, sizeof(*shhwtstamps));
 			shhwtstamps->hwtstamp = ns_to_ktime(rxts->ns);
-			netif_rx_ni(skb);
 			list_del_init(&rxts->list);
 			list_add(&rxts->list, &dp83640->rxpool);
 			break;

^ permalink raw reply related

* Re: [PATCH net-next 1/1] net sched actions: add time filter for action dumping
From: Jamal Hadi Salim @ 2017-04-19 10:44 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: davem, netdev, xiyou.wangcong, eric.dumazet, jiri
In-Reply-To: <20170418205525.7d8d18a1@cakuba.lan>

On 17-04-18 11:55 PM, Jakub Kicinski wrote:
> On Tue, 18 Apr 2017 21:37:12 -0400, Jamal Hadi Salim wrote:
>> On 17-04-18 06:12 PM, Jakub Kicinski wrote:

[..]
>> I see both being fine from that perspective - you dont need 100%
>> accuracy. Just something that is within reason of a small delta
>> of time.
>
> I'm just referring to the theoretical possibility that if the dumping
> process gets preempted for long enough you may loose samples.  Just
> because the dumping process cannot control when kernel executes this
> line:
>
> 	jiffy_wanted = jiffies - jiffy_msecs;
>
> It could in theory be few seconds after the request was made.  Perhaps
> using timestamps from a proper time sources instead of the notion of
> "last X seconds" would solve that?


Good point which i didnt mention as part of the drawbacks. This
is a tradeoff. We dont need to be 100% accurate[1].
The timestamps on the action entries in the kernel are in jiffies;
a lot simpler to do jiffy comparison. If you used a different
timestamp source you'd need to convert for every comparison you
make (i am not sure how costly is when you have many actions).

In use cases i am familiar with, there is a user process app which
opens the socket once and issues dumps every X seconds (ranging from
5-120 seconds). So we will re-issue the dump regardless. Yes
it would be an issue if said application keeps getting pre-empted
and that jiffy computation was always off - but not sure under
what circumstances that could be a common scenario.

cheers,
jamal

[1] As an example, dumps are never 100% accurate you could iterate
  something that then changes while you are in the middle of dumping
  which then renders an already dumped entity obsolete.

^ permalink raw reply

* Re: [PATCH] dp83640: don't recieve time stamps twice
From: Dan Carpenter @ 2017-04-19 10:53 UTC (permalink / raw)
  To: Richard Cochran
  Cc: Sørensen, Stefan, netdev@vger.kernel.org,
	f.fainelli@gmail.com, andrew@lunn.ch,
	kernel-janitors@vger.kernel.org
In-Reply-To: <20170419103135.GA1670@localhost.localdomain>

On Wed, Apr 19, 2017 at 12:31:35PM +0200, Richard Cochran wrote:
> On Wed, Apr 19, 2017 at 09:16:56AM +0000, Sørensen, Stefan wrote:
> > You are correct that these lines was added accidentally.
> 
> Can we please fix this in another way?  There is no need to hold the
> spin lock during the callback into the networking stack.  Instead, how
> about the following diff, which also fixes the other call site...
> 

I'm fine with this.  I hated my changelog, as well because it sounds
like guess work.  Could you send it with a:

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter



^ permalink raw reply

* Re: [PATCH net-next 1/2 v2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Jamal Hadi Salim @ 2017-04-19 11:02 UTC (permalink / raw)
  To: Roopa Prabhu; +Cc: davem, netdev, eric.dumazet, jiri, xiyou.wangcong
In-Reply-To: <58F6EDA4.6090601@cumulusnetworks.com>

On 17-04-19 12:55 AM, Roopa Prabhu wrote:
> On 4/18/17, 6:14 PM, Jamal Hadi Salim wrote:

>>
>> A new top level TLV space is introduced. An attribute
>> TCAA_ACT_FLAGS is used to carry the flags indicating the user
>> is capable of processing these large dumps. Older user space which
>> doesnt set this flag doesnt get the large (than 32) batches.
>> The kernel uses the TCAA_ACT_COUNT attribute to tell the user how many
>> actions are put in a single batch. As such user space app knows how long
>> to iterate (independent of the type of action being dumped)
>> instead of hardcoded maximum of 32.
> Is the count attribute really needed ?. user space knows how to iterate multiple
> attributes of the same type till the end of msg. eg. fdb dumps don't use count.
>

fdb dumping is different. One nlmsg contains one fdb entry.
So the boundary for checking for iproute2 is the nlmsg header.
So you wouldnt need such a counter.

OTOH, actions are packed, many per nlmsg:
I can put X or more policers in one nlmsg and batch many nlmsgs;
all constrained by the dump skb size.
Back in the day it was considered that a filter having more than
32 actions in its pipeline was probably enough. So in a dump, the
kernel would not send more than 32(TCA_ACT_MAX_PRIO) at a time.
This patch basically makes it fit as many as possible.

The other difference is there is only one type of fdb. There are
many types of actions - each with different sizes and different
optionally appearing entities. So if you are writing generic code
to print for example, you can play acrobatics in user space and
look at TLV lengths, deduce where the boundaries are for each type
and then for each type of action do speacial handling.
If the kernel tells you how many actions are packed then a major
change in iproute2 is:
-       for (i = 0; i < TCA_ACT_MAX_PRIO; i++) {
+       for (i = 0; i < tot_acts_from_kernel; i++) {

cheers,
jamal

PS: I think fdb is a good candidate for time filtering since
it keeps track of lastused time. You dont really have to change it to
be as  efficient as actions in terms of packing but it would
definetely improve status quo since you now have stats.

^ permalink raw reply

* Re: [PATCH net-next 1/2 v2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Jamal Hadi Salim @ 2017-04-19 11:06 UTC (permalink / raw)
  To: Cong Wang
  Cc: Eric Dumazet, David Miller, Linux Kernel Network Developers,
	Jiri Pirko
In-Reply-To: <CAM_iQpVbgNB53L-fVwtSfJSfBmSDua4EttqoHALzNiuuJOUT3A@mail.gmail.com>

On 17-04-19 02:12 AM, Cong Wang wrote:
> On Tue, Apr 18, 2017 at 7:32 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> On 17-04-18 09:49 PM, Eric Dumazet wrote:

>> With this change I ask the kernel to fit as many actions as
>> possible in the 32K (all these 128 will fit in one batch).
>> Then it has to tell user space how many are in that batch
>> using TCAA_ACT_COUNT attribute.
>>
>> Make sense?
>
> Hmm? How do tc filters do dumping? There is no max and no
> COUNT attribute either, IIUC.
>

They dont have this hard coded 32 entries limit. So standard
dumping works fine. Note: count is a very speacial attribute to
make sure we can distinguish between old kernels which send
only 32 and new kernels which could send more.

cheers,
jamal

^ permalink raw reply

* RE: [PATCH] gso: Validate assumption of frag_list segementation
From: Ilan Tayari @ 2017-04-19 11:17 UTC (permalink / raw)
  To: David Miller
  Cc: netdev@vger.kernel.org, alexander.h.duyck@intel.com,
	eric.dumazet@gmail.com, steffen.klassert@secunet.com,
	Boris Pismenny, Ilya Lesokhin
In-Reply-To: <20170417.153259.1519976990479511796.davem@davemloft.net>

> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> 
> > From: Ilan Tayari <ilant@mellanox.com>
> >
> > Commit 07b26c9454a2 ("gso: Support partial splitting at the frag_list
> > pointer") assumes that all SKBs in a frag_list (except maybe the last
> > one) contain the same amount of GSO payload.
> >
> > This assumption is not always correct, resulting in the following
> > warning message in the log:
> >     skb_segment: too many frags
> >
> > For example, mlx5 driver in Striding RQ mode creates some RX SKBs with
> > one frag, and some with 2 frags.
> > After GRO, the frag_list SKBs end up having different amounts of
> payload.
> > If this frag_list SKB is then forwarded, the aforementioned assumption
> > is violated.
> >
> > Validate the assumption, and fall back to software GSO if it not true.
> >
> > Fixes: 07b26c9454a2 ("gso: Support partial splitting at the frag_list
> > pointer")
> > Signed-off-by: Ilan Tayari <ilant@mellanox.com>
> > Signed-off-by: Ilya Lesokhin <ilyal@mellanox.com>
> > ---
> 
> Your commit message mentions a fixes tag which make this change seem
> relevant for 'net', but your patch depends upon things in 'net-next'
> and therefore only applies there.
> 
> I've added this change to net-next, but I want an explanation of why this
> change is not targettting 'net' if it seems to fix a problem there.
> 
> Thanks.

Sorry, my mistake.

This fix does belong to 'net', but this patch needs a modification to be
applied there.

I will send a patch for 'net' as well.

Thanks

^ permalink raw reply

* Re: [PATCH net-next 1/2 v2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Jamal Hadi Salim @ 2017-04-19 11:24 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: davem, netdev, jiri, xiyou.wangcong
In-Reply-To: <1492571845.10587.161.camel@edumazet-glaptop3.roam.corp.google.com>

On 17-04-18 11:17 PM, Eric Dumazet wrote:
> On Tue, 2017-04-18 at 22:32 -0400, Jamal Hadi Salim wrote:
>> On 17-04-18 09:49 PM, Eric Dumazet wrote:
>>> On Tue, 2017-04-18 at 21:14 -0400, Jamal Hadi Salim wrote:

>>
>> Make sense?
>
> What if we have 1024 actions, and user provides a 4KB buffer ?
>

No problem - we will fit as many per batch to consume 4KB
and will send them as long as user calls recvmsg.

> Normally multiple recvmsg() calls would be needed, but I do not see how
> the nla_put_u32(skb, TCAA_ACT_COUNT, cb->args[1]) can always succeed.

Oh, I see the cross-talk Eric;->
We dont pack the actions in TCAA_ACT_COUNT - we put them in
TCAA_ACT_TAB.

Here's some strace capture that best describes what happened before
and after which i hope will make sense. Granted tc uses 32KB from
user space and not 4KB you mention.

We have 400 actions in the kernel at this point:

tc with no changes (doesnt for this large dump):
---
recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, 
msg_iov(1)=[{"\240\16\0\0002\0\2\0^6\367XE\3\0\0\0\0\0\0\204\16\1\0\200\0\0\0\t\0\1\0"..., 
32768,g}], msg_controllen=0, msg_flags=0}, 0) = 3744

===> total actions dumped 29

recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, 
msg_iov(1)=[{" 
\20\0\0002\0\2\0s6\367XI\3\0\0\0\0\0\0\4\20\1\0\200\0\0\0\t\0\1\0"..., 
32768}], msg_controllen=0, msg_flags=0}, 0) = 4128

==> total actions dumped 32

recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, 
msg_iov(1)=[{" 
\20\0\0002\0\2\0s6\367XI\3\0\0\0\0\0\0\4\20\1\0\200\0\0\0\t\0\1\0"..., 
32768}], msg_controllen=0, msg_flags=0}, 0) = 4128

==> total actions dumped 32
....
.....
.........
recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, 
msg_iov(1)=[{"\24\0\0\0\3\0\2\0s6\367XI\3\0\0\0\0\0\0", 32768}], 
msg_controllen=0, msg_flags=0}, 0) = 20
-----

Goes on a few times until we get all 400 entries - last recvmsg (with
20B) has no actions and indicates dump is complete.
Issue: The kernel is refusing to add more than 32 entries in the skb
even though we get allocated 32KB for the skb.

So now lets see what happens with this change:
------
recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, 
msg_iov(1)=[{"\240\16\0\0002\0\2\0^6\367XE\3\0\0\0\0\0\0\204\16\1\0\200\0\0\0\t\0\1\0"..., 
32768,g}], msg_controllen=0, msg_flags=0}, 0) = 3744

==> total actions dumped 29

recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, 
msg_iov(1)=[{"\240~\0\0002\0\2\0^6\367XE\3\0\0\0\0\0\0\204~\1\0\200\0\0\0\t\0\1\0"..., 
32768,g}], msg_controllen=0, msg_flags=0}, 0) = 32416

==> total actions dumped 253

recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, 
msg_iov(1)=[{" 
;\0\0002\0\2\0^6\367XE\3\0\0\0\0\0\0\4;\1\0\200\0\0\0\t\0\1\0"..., 
32768,g}], msg_controllen=0, msg_flags=0}, 0) = 15136

==> total actions dumped 118

recvmsg(3, {msg_name(12)={sa_family=AF_NETLINK, pid=0, groups=00000000}, 
msg_iov(1)=[{"\24\0\0\0\3\0\2\0s6\367XI\3\0\0\0\0\0\0", 32768}], 
msg_controllen=0, msg_flags=0}, 0) = 20

--------

We got all 400 in 3 requests. Imagine what we have to deal with
when we have 2M actions (the improvement is about 10x).

cheers,
jamal

^ permalink raw reply

* Re: [PATCH] dp83640: don't recieve time stamps twice
From: Sørensen, Stefan @ 2017-04-19 11:28 UTC (permalink / raw)
  To: richardcochran@gmail.com
  Cc: netdev@vger.kernel.org, dan.carpenter@oracle.com,
	f.fainelli@gmail.com, andrew@lunn.ch,
	kernel-janitors@vger.kernel.org
In-Reply-To: <20170419103135.GA1670@localhost.localdomain>

On Wed, 2017-04-19 at 12:31 +0200, Richard Cochran wrote:
> Can we please fix this in another way?  There is no need to hold the
> spin lock during the callback into the networking stack.  Instead,
> how about the following diff, which also fixes the other call site...


I agree that this is a better way to fix it - I think that this was the
purpose of the patch that got halfway mixed in.

@@ -874,14 +874,15 @@ static void decode_rxts(struct dp83640_private
> *dp83640,
>  			shhwtstamps = skb_hwtstamps(skb);
>  			memset(shhwtstamps, 0,
> sizeof(*shhwtstamps));
>  			shhwtstamps->hwtstamp = ns_to_ktime(rxts-
> >ns);
> -			netif_rx_ni(skb);
>  			list_add(&rxts->list, &dp83640->rxpool);
>  			break;
>  		}
>  	}
>  	spin_unlock(&dp83640->rx_queue.lock);
>  
> -	if (!shhwtstamps)
> +	if (shhwtstamps)
> +		netif_rx_ni(skb);
> +	else
>  		list_add_tail(&rxts->list, &dp83640->rxts);
>  out:
>  	spin_unlock_irqrestore(&dp83640->rx_lock, flags);

Here we still hold rx_lock while during the callback, wouldn't it be
beneficial to release that first?

Stefan

^ permalink raw reply

* Re: [PATCH net-next 1/2] Add Cong Wang as TC subsystem co-maintainer
From: Jamal Hadi Salim @ 2017-04-19 11:29 UTC (permalink / raw)
  To: Cong Wang
  Cc: David Miller, Linux Kernel Network Developers, Eric Dumazet,
	Jiri Pirko
In-Reply-To: <CAM_iQpXt8pd=t1=S+PaANyRw2Po2Q04k3FJnZXd4UsBsHc_4gA@mail.gmail.com>

On 17-04-19 02:01 AM, Cong Wang wrote:
> On Tue, Apr 18, 2017 at 6:23 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> From: Jamal Hadi Salim <jhs@mojatatu.com>
>>
>> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>
> I am happy and honored to review net sched patches,
>

Thank you for all your contributions so far!

cheers,
jamal

^ permalink raw reply


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