Patchwork [RFC,net-next] mlx4_en: Do not set dev_id to port number

login
register
mail settings
Submitter Narendra K
Date June 14, 2013, 3:06 p.m.
Message ID <20130614150624.GA1316@fedora-17-guest.blr.amer.dell.com>
Download mbox | patch
Permalink /patch/251447/
State RFC
Delegated to: David Miller
Headers show

Comments

Narendra K - June 14, 2013, 3:06 p.m.
Port number should not be stored in dev_id. 'dev_id' field was
intended to be used to differentiate between multiple devices
which share the same MAC address.

Signed-off-by: Narendra K <narendra_k@dell.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 1 -
 1 file changed, 1 deletion(-)
Or Gerlitz - June 14, 2013, 7:55 p.m.
On Fri, Jun 14, 2013 at 6:06 PM, <Narendra_K@dell.com> wrote:
> Port number should not be stored in dev_id. 'dev_id' field was
> intended to be used to differentiate between multiple devices
> which share the same MAC address.

Maybe you want to say that dev_id is **now** intended to differentiate
between multiple devices which share the same MAC address? the reason
that mlx4_en and IPoIB use this field is

1. the Mellanox exposes one PCI function but has two ports with a
netdevice set on each

2. there was no standard way to expose the port number used by two
netdevices that share the same PCI function

3. here http://patchwork.ozlabs.org/patch/53547/ we were suggested by
the netdev maintainers to use that field

This patch introduces a regression and can't get in without a
replacment mechanism.

Or.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - June 14, 2013, 8:14 p.m.
On Fri, 2013-06-14 at 22:55 +0300, Or Gerlitz wrote:
> On Fri, Jun 14, 2013 at 6:06 PM, <Narendra_K@dell.com> wrote:
> > Port number should not be stored in dev_id. 'dev_id' field was
> > intended to be used to differentiate between multiple devices
> > which share the same MAC address.
> 
> Maybe you want to say that dev_id is **now** intended to differentiate
> between multiple devices which share the same MAC address?

That was the original intent but it wasn't well-documented.  (Hence
several of us driver writers have made a similar mistake.)

> the reason that mlx4_en and IPoIB use this field is
> 
> 1. the Mellanox exposes one PCI function but has two ports with a
> netdevice set on each
> 
> 2. there was no standard way to expose the port number used by two
> netdevices that share the same PCI function
> 
> 3. here http://patchwork.ozlabs.org/patch/53547/ we were suggested by
> the netdev maintainers to use that field
> 
> This patch introduces a regression and can't get in without a
> replacment mechanism.

If you have userland tools relying on this then I agree it should be
left alone, but maybe add a comment explaining why the field is used in
an unusual way.

Ben.
Or Gerlitz - June 14, 2013, 8:18 p.m.
On Fri, Jun 14, 2013 at 11:14 PM, Ben Hutchings
<bhutchings@solarflare.com> wrote:

> If you have userland tools relying on this then I agree it should be
> left alone, but maybe add a comment explaining why the field is used in
> an unusual way.

we were not aware that it used in unsual way, just used that b/c were told
too, not sure what to put in the comment. As for userland tools, I know
about internal regression tools and some clould related scripts that use
that.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index ade276c..ca618e0 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -2138,7 +2138,6 @@  int mlx4_en_init_netdev(struct mlx4_en_dev *mdev, int port,
 	netif_set_real_num_rx_queues(dev, prof->rx_ring_num);
 
 	SET_NETDEV_DEV(dev, &mdev->dev->pdev->dev);
-	dev->dev_id =  port - 1;
 
 	/*
 	 * Initialize driver private data