Skip to contentSkip to conversation

&[Question] netdevice: Use netdev_priv()

PATCH In review Wang Chen 3 participants 10 messages Aug 1, 2008 – Aug 6, 2008

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Wang Chen

author
#0 lore raw

PATCH 1/1 &[Question] netdevice: Use netdev_priv()

Dave

This mail is a question mail more than patch mail.
In your commit "netdev: netdev_priv() can now be sane again.",
you said netdev_priv() != netdev->priv, because that time,
netdev->priv = ((char *)dev +((sizeof(struct net_device) +(sizeof(struct net_device_subqueue) *
(queue_count - 1)) + NETDEV_ALIGN_CONST) & ~NETDEV_ALIGN_CONST))
But now, after you do TX multiqueue improvement.
netdev->priv == netdev_priv() again.

That make me think, what the use of netdev->priv?
We have netdev_priv() and use it flexibly. Why don't kill netdev->priv?

---

Quoted 1 line

From be26ade29c6a3d0002492f7aab25258cb78ab597 Mon Sep 17 00:00:00 2001

From: Wang Chen <[email protected]>
Date: Fri, 1 Aug 2008 16:59:59 +0800
Subject: [PATCH] netdevice: Use netdev_priv()

dev->priv is now point to private data again.

Signed-off-by Wang Chen <[email protected]>
---
 net/core/dev.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)
net/core/dev.c +2 -5
index 63d6bcd..9b73624 100644
@@ -4255,11 +4255,8 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
 	dev->num_tx_queues = queue_count;
 	dev->real_num_tx_queues = queue_count;
 
-	if (sizeof_priv) {
-		dev->priv = ((char *)dev +
-			     ((sizeof(struct net_device) + NETDEV_ALIGN_CONST)
-			      & ~NETDEV_ALIGN_CONST));
-	}
+	if (sizeof_priv)
+		dev->priv = netdev_priv(dev);
 
 	dev->gso_max_size = GSO_MAX_SIZE;
Signature · 1 line
1.5.3.4
maintainer
#1 lore raw

From: Wang Chen <[email protected]>
Date: Fri, 01 Aug 2008 17:50:15 +0800

Quoted 2 lines

That make me think, what the use of netdev->priv?
We have netdev_priv() and use it flexibly. Why don't kill netdev->priv?

Way too many old drivers still reference netdev->priv directly.

Once we convert all of them to netdev_priv(), yes we can kill netdev->priv

author
#2 lore raw

David Miller said the following on 2008-8-1 18:08:

Quoted 11 lines

From: Wang Chen <[email protected]>
Date: Fri, 01 Aug 2008 17:50:15 +0800

Quoted 2 lines

That make me think, what the use of netdev->priv?
We have netdev_priv() and use it flexibly. Why don't kill netdev->priv?

Way too many old drivers still reference netdev->priv directly.

Once we convert all of them to netdev_priv(), yes we can kill netdev->priv

Thanks for the explanation.

Then next question comes.
We *have to* convert the drivers to use netdev_priv(), right? Because, netdev->priv == netdev_priv() maybe change again in future, just as before.

maintainer
#3 lore raw

From: Wang Chen <[email protected]>
Date: Fri, 01 Aug 2008 18:16:32 +0800

Quoted 2 lines

Then next question comes.
We *have to* convert the drivers to use netdev_priv(), right?

No, we don't.

Quoted 2 lines

Because, netdev->priv == netdev_priv() maybe change again in future, just as before.

We will work to prevent that. I should not have allowed it to happen in the first place. :-)

author
#4 lore raw

David Miller said the following on 2008-8-1 18:24:

Quoted 6 lines

From: Wang Chen <[email protected]>
Date: Fri, 01 Aug 2008 18:16:32 +0800

Quoted 2 lines

Then next question comes.
We *have to* convert the drivers to use netdev_priv(), right?

Quoted 8 lines

No, we don't.

Quoted 2 lines

Because, netdev->priv == netdev_priv() maybe change again in future, just as before.

We will work to prevent that. I should not have allowed it to happen in the first place. :-)

How about add a comment to netdev->priv

void *priv; /* pointer to private data, referent me by netdev_priv()*/

maintainer
#5 lore raw

From: Wang Chen <[email protected]>
Date: Fri, 01 Aug 2008 18:35:24 +0800

Quoted 3 lines

How about add a comment to netdev->priv

void *priv; /* pointer to private data, referent me by netdev_priv()*/

Sure, but I'd much rather apply a patch that converts all the direct references :-)

author
#6 lore raw

David Miller said the following on 2008-8-1 19:05:

Quoted 9 lines

From: Wang Chen <[email protected]>
Date: Fri, 01 Aug 2008 18:35:24 +0800

Quoted 3 lines

How about add a comment to netdev->priv

void *priv; /* pointer to private data, referent me by netdev_priv()*/

Sure, but I'd much rather apply a patch that converts all the direct references :-)

I just checked some drivers' source.
And, maybe some direct references of net_device->priv are needed. For example:
init_one() of cxgb2 makes several net_device to share one private data. For this purpose, direct reference is necessary.

maintainer
#7 lore raw

From: Wang Chen <[email protected]>
Date: Tue, 05 Aug 2008 17:55:27 +0800

Quoted 2 lines

init_one() of cxgb2 makes several net_device to share one private data. For this purpose, direct reference is necessary.

It should use netdev->ml_priv which is created for this purpose.

Every direct netdev->priv usage is a bug.

#8 lore raw
Quoted 9 lines · David Miller

From: Wang Chen <[email protected]>
Date: Tue, 05 Aug 2008 17:55:27 +0800

Quoted 2 lines

init_one() of cxgb2 makes several net_device to share one private data. For this purpose, direct reference is necessary.

It should use netdev->ml_priv which is created for this purpose.

Every direct netdev->priv usage is a bug.

cxgb3 was fixed in this regard, cxgb2 needs the same treatment ...

cheers,
Divy

author
#9 lore raw

Divy Le Ray said the following on 2008-8-6 1:51:

Quoted 12 lines · David Miller
Quoted 8 lines

From: Wang Chen <[email protected]>
Date: Tue, 05 Aug 2008 17:55:27 +0800

Quoted 2 lines

init_one() of cxgb2 makes several net_device to share one private data. For this purpose, direct reference is necessary.

It should use netdev->ml_priv which is created for this purpose.

Every direct netdev->priv usage is a bug.

cxgb3 was fixed in this regard, cxgb2 needs the same treatment ...

Thanks Dave and Divy.
I am making the big patch for fixing all direct reference of netdev->priv.