All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] netdevice: add netdev_pub helper function
@ 2015-06-12 13:30 Jason A. Donenfeld
  2015-06-12 21:20 ` David Miller
  2016-10-05  0:52 ` [PATCH] " Jason A. Donenfeld
  0 siblings, 2 replies; 7+ messages in thread
From: Jason A. Donenfeld @ 2015-06-12 13:30 UTC (permalink / raw)
  To: David S. Miller, netdev, linux-kernel; +Cc: Jason A. Donenfeld

Being able to utilize this makes much code a lot simpler and cleaner.
It's a nice convenience function.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 include/linux/netdevice.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 05b9a69..f85be18 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1871,6 +1871,17 @@ static inline void *netdev_priv(const struct net_device *dev)
 	return (char *)dev + ALIGN(sizeof(struct net_device), NETDEV_ALIGN);
 }
 
+/**
+ * 	netdev_pub - access network device from private pointer
+ * 	@priv: private data pointer of network device
+ *
+ * Get network device from a network device private data pointer
+ */
+static inline struct net_device *netdev_pub(void *priv)
+{
+	return (struct net_device *)((char *)priv - ALIGN(sizeof(struct net_device), NETDEV_ALIGN));
+}
+
 /* Set the sysfs physical device reference for the network logical device
  * if set prior to registration will cause a symlink during initialization.
  */
-- 
2.4.2


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] netdevice: add netdev_pub helper function
  2015-06-12 13:30 [PATCH] netdevice: add netdev_pub helper function Jason A. Donenfeld
@ 2015-06-12 21:20 ` David Miller
  2017-07-10  3:19   ` [PATCH 1/2] " Jason A. Donenfeld
  2016-10-05  0:52 ` [PATCH] " Jason A. Donenfeld
  1 sibling, 1 reply; 7+ messages in thread
From: David Miller @ 2015-06-12 21:20 UTC (permalink / raw)
  To: Jason; +Cc: netdev, linux-kernel

From: "Jason A. Donenfeld" <Jason@zx2c4.com>
Date: Fri, 12 Jun 2015 15:30:29 +0200

> Being able to utilize this makes much code a lot simpler and cleaner.
> It's a nice convenience function.
> 
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

Please do not ever submit patches adding new interfaces without
also submitting changes showing actual uses of the new interface.

Otherwise it's impossible to see how really useful it actually
is.

I'm not applying this until you do so, thanks.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] netdevice: add netdev_pub helper function
  2015-06-12 13:30 [PATCH] netdevice: add netdev_pub helper function Jason A. Donenfeld
  2015-06-12 21:20 ` David Miller
@ 2016-10-05  0:52 ` Jason A. Donenfeld
  1 sibling, 0 replies; 7+ messages in thread
From: Jason A. Donenfeld @ 2016-10-05  0:52 UTC (permalink / raw)
  To: David S. Miller, Netdev, LKML

Hey David,

The use of this function is going from the private member to the
public netdev struct. The usage is desired from the following coding
pattern.

You're implementing a netdevice. You've got ndo_init, ndo_uninit,
ndo_open, ndo_stop, ndo_start_xmit, and maybe even ndo_do_ioctl. All
of these functions basically follow the flow: get some information out
of struct netdev, then call netdev_priv(), and pass that specific
pointer onto the rest of the driver. The rest of the driver, 99% of
the time, only deals with your private member. Very very occasionally
it might want to check into how some piece of public data is doing.
For example, is the interface up? In this case, it's very convenient
to have the netdev_pub function, as in this patch.

    if (netdev_pub(priv)->flags & IFF_UP) {
        ...
    }

Then, after shortly using the public members, the driver gets on its
way dealing again exclusively with the private part.

I posted this patch a year ago, and let it languish after your initial
comment, because I wasn't confident that this was necessarily
something everybody could benefit from. 18 months later, after reading
quite a few netdevice-based drivers, it seems like this is indeed a
very useful code pattern, that makes things a bit more clear, a bit
less verbose, and helps maintain type safety throughout a driver.

So, I resubmit this to you for inclusion.

Regards,
Jason


On Fri, Jun 12, 2015 at 3:30 PM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> Being able to utilize this makes much code a lot simpler and cleaner.
> It's a nice convenience function.
>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  include/linux/netdevice.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 05b9a69..f85be18 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1871,6 +1871,17 @@ static inline void *netdev_priv(const struct net_device *dev)
>         return (char *)dev + ALIGN(sizeof(struct net_device), NETDEV_ALIGN);
>  }
>
> +/**
> + *     netdev_pub - access network device from private pointer
> + *     @priv: private data pointer of network device
> + *
> + * Get network device from a network device private data pointer
> + */
> +static inline struct net_device *netdev_pub(void *priv)
> +{
> +       return (struct net_device *)((char *)priv - ALIGN(sizeof(struct net_device), NETDEV_ALIGN));
> +}
> +
>  /* Set the sysfs physical device reference for the network logical device
>   * if set prior to registration will cause a symlink during initialization.
>   */
> --
> 2.4.2
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/2] netdevice: add netdev_pub helper function
  2015-06-12 21:20 ` David Miller
@ 2017-07-10  3:19   ` Jason A. Donenfeld
  2017-07-10  3:19     ` [PATCH 2/2] ioc3-eth: use netdev_pub instead of handrolling alignment Jason A. Donenfeld
  2017-07-10  8:04     ` [PATCH 1/2] netdevice: add netdev_pub helper function David Miller
  0 siblings, 2 replies; 7+ messages in thread
From: Jason A. Donenfeld @ 2017-07-10  3:19 UTC (permalink / raw)
  To: davem, netdev, linux-kernel; +Cc: Jason A. Donenfeld

Being able to utilize this makes code a lot simpler and cleaner. It's
easier in many cases for drivers to pass around their private data
structure, while occationally needing to dip into net_device, rather
than the other way around, which results in tons of calls to netdev_priv
in the top of every single function, which makes everything confusing
and less clear. Additionally, this enables a "correct" way of doing such
a thing, instead of having drivers attempt to reinvent the wheel and
screw it up.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 include/linux/netdevice.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 779b23595596..83d58504e5c4 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2030,26 +2030,37 @@ void dev_net_set(struct net_device *dev, struct net *net)
 }
 
 /**
  *	netdev_priv - access network device private data
  *	@dev: network device
  *
  * Get network device private data
  */
 static inline void *netdev_priv(const struct net_device *dev)
 {
 	return (char *)dev + ALIGN(sizeof(struct net_device), NETDEV_ALIGN);
 }
 
+/**
+ * 	netdev_pub - access network device from private pointer
+ * 	@priv: private data pointer of network device
+ *
+ * Get network device from a network device private data pointer
+ */
+static inline struct net_device *netdev_pub(void *priv)
+{
+	return (struct net_device *)((char *)priv - ALIGN(sizeof(struct net_device), NETDEV_ALIGN));
+}
+
 /* Set the sysfs physical device reference for the network logical device
  * if set prior to registration will cause a symlink during initialization.
  */
 #define SET_NETDEV_DEV(net, pdev)	((net)->dev.parent = (pdev))
 
 /* Set the sysfs device type for the network logical device to allow
  * fine-grained identification of different network device types. For
  * example Ethernet, Wireless LAN, Bluetooth, WiMAX etc.
  */
 #define SET_NETDEV_DEVTYPE(net, devtype)	((net)->dev.type = (devtype))
 
 /* Default NAPI poll() weight
  * Device drivers are strongly advised to not use bigger value
-- 
2.13.2

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/2] ioc3-eth: use netdev_pub instead of handrolling alignment
  2017-07-10  3:19   ` [PATCH 1/2] " Jason A. Donenfeld
@ 2017-07-10  3:19     ` Jason A. Donenfeld
  2017-07-10  8:04     ` [PATCH 1/2] netdevice: add netdev_pub helper function David Miller
  1 sibling, 0 replies; 7+ messages in thread
From: Jason A. Donenfeld @ 2017-07-10  3:19 UTC (permalink / raw)
  To: davem, netdev, linux-kernel; +Cc: Jason A. Donenfeld

It's safer to use the generic library function for this, rather than
reinventing it here with hard-coded alignment values.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/net/ethernet/sgi/ioc3-eth.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/sgi/ioc3-eth.c b/drivers/net/ethernet/sgi/ioc3-eth.c
index b607936e1b3e..514eca163ea5 100644
--- a/drivers/net/ethernet/sgi/ioc3-eth.c
+++ b/drivers/net/ethernet/sgi/ioc3-eth.c
@@ -86,31 +86,26 @@ struct ioc3_private {
 	int tx_ci;			/* TX consumer index */
 	int tx_pi;			/* TX producer index */
 	int txqlen;
 	u32 emcr, ehar_h, ehar_l;
 	spinlock_t ioc3_lock;
 	struct mii_if_info mii;
 
 	struct pci_dev *pdev;
 
 	/* Members used by autonegotiation  */
 	struct timer_list ioc3_timer;
 };
 
-static inline struct net_device *priv_netdev(struct ioc3_private *dev)
-{
-	return (void *)dev - ((sizeof(struct net_device) + 31) & ~31);
-}
-
 static int ioc3_ioctl(struct net_device *dev, struct ifreq *rq, int cmd);
 static void ioc3_set_multicast_list(struct net_device *dev);
 static int ioc3_start_xmit(struct sk_buff *skb, struct net_device *dev);
 static void ioc3_timeout(struct net_device *dev);
 static inline unsigned int ioc3_hash(const unsigned char *addr);
 static inline void ioc3_stop(struct ioc3_private *ip);
 static void ioc3_init(struct net_device *dev);
 
 static const char ioc3_str[] = "IOC3 Ethernet";
 static const struct ethtool_ops ioc3_ethtool_ops;
 
 /* We use this to acquire receive skb's that we can DMA directly into. */
 
@@ -417,39 +412,39 @@ static void ioc3_get_eaddr_nic(struct ioc3_private *ip)
 		printk("Failed to read MAC address\n");
 		return;
 	}
 
 	/* Read Memory.  */
 	nic_write_byte(ioc3, 0xf0);
 	nic_write_byte(ioc3, 0x00);
 	nic_write_byte(ioc3, 0x00);
 
 	for (i = 13; i >= 0; i--)
 		nic[i] = nic_read_byte(ioc3);
 
 	for (i = 2; i < 8; i++)
-		priv_netdev(ip)->dev_addr[i - 2] = nic[i];
+		netdev_pub(ip)->dev_addr[i - 2] = nic[i];
 }
 
 /*
  * Ok, this is hosed by design.  It's necessary to know what machine the
  * NIC is in in order to know how to read the NIC address.  We also have
  * to know if it's a PCI card or a NIC in on the node board ...
  */
 static void ioc3_get_eaddr(struct ioc3_private *ip)
 {
 	ioc3_get_eaddr_nic(ip);
 
-	printk("Ethernet address is %pM.\n", priv_netdev(ip)->dev_addr);
+	printk("Ethernet address is %pM.\n", netdev_pub(ip)->dev_addr);
 }
 
 static void __ioc3_set_mac_address(struct net_device *dev)
 {
 	struct ioc3_private *ip = netdev_priv(dev);
 	struct ioc3 *ioc3 = ip->regs;
 
 	ioc3_w_emar_h((dev->dev_addr[5] <<  8) | dev->dev_addr[4]);
 	ioc3_w_emar_l((dev->dev_addr[3] << 24) | (dev->dev_addr[2] << 16) |
 	              (dev->dev_addr[1] <<  8) | dev->dev_addr[0]);
 }
 
 static int ioc3_set_mac_address(struct net_device *dev, void *addr)
@@ -780,27 +775,27 @@ static void ioc3_timer(unsigned long data)
 	add_timer(&ip->ioc3_timer);
 }
 
 /*
  * Try to find a PHY.  There is no apparent relation between the MII addresses
  * in the SGI documentation and what we find in reality, so we simply probe
  * for the PHY.  It seems IOC3 PHYs usually live on address 31.  One of my
  * onboard IOC3s has the special oddity that probing doesn't seem to find it
  * yet the interface seems to work fine, so if probing fails we for now will
  * simply default to PHY 31 instead of bailing out.
  */
 static int ioc3_mii_init(struct ioc3_private *ip)
 {
-	struct net_device *dev = priv_netdev(ip);
+	struct net_device *dev = netdev_pub(ip);
 	int i, found = 0, res = 0;
 	int ioc3_phy_workaround = 1;
 	u16 word;
 
 	for (i = 0; i < 32; i++) {
 		word = ioc3_mdio_read(dev, i, MII_PHYSID1);
 
 		if (word != 0xffff && word != 0x0000) {
 			found = 1;
 			break;			/* Found a PHY		*/
 		}
 	}
 
-- 
2.13.2

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] netdevice: add netdev_pub helper function
  2017-07-10  3:19   ` [PATCH 1/2] " Jason A. Donenfeld
  2017-07-10  3:19     ` [PATCH 2/2] ioc3-eth: use netdev_pub instead of handrolling alignment Jason A. Donenfeld
@ 2017-07-10  8:04     ` David Miller
  2017-07-10 11:52       ` Jason A. Donenfeld
  1 sibling, 1 reply; 7+ messages in thread
From: David Miller @ 2017-07-10  8:04 UTC (permalink / raw)
  To: Jason; +Cc: netdev, linux-kernel

From: "Jason A. Donenfeld" <Jason@zx2c4.com>
Date: Mon, 10 Jul 2017 05:19:58 +0200

> Being able to utilize this makes code a lot simpler and cleaner. It's
> easier in many cases for drivers to pass around their private data
> structure, while occationally needing to dip into net_device, rather
> than the other way around, which results in tons of calls to netdev_priv
> in the top of every single function, which makes everything confusing
> and less clear. Additionally, this enables a "correct" way of doing such
> a thing, instead of having drivers attempt to reinvent the wheel and
> screw it up.
> 
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

I disagree.  Assuming one can go from the driver private to the netdev
object trivially is a worse assumption than the other way around, and
locks us into the current implementation of how the netdev and driver
private memory is allocated.

If you want to style your driver such that the private is passed
around instead of the netdev, put a pointer back to the netdev object
in your private data structure.

Which is exactly what the ioc3-eth driver ought to be doing.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] netdevice: add netdev_pub helper function
  2017-07-10  8:04     ` [PATCH 1/2] netdevice: add netdev_pub helper function David Miller
@ 2017-07-10 11:52       ` Jason A. Donenfeld
  0 siblings, 0 replies; 7+ messages in thread
From: Jason A. Donenfeld @ 2017-07-10 11:52 UTC (permalink / raw)
  To: David Miller; +Cc: Netdev, LKML

On Mon, Jul 10, 2017 at 10:04 AM, David Miller <davem@davemloft.net> wrote:
> I disagree.  Assuming one can go from the driver private to the netdev
> object trivially is a worse assumption than the other way around, and
> locks us into the current implementation of how the netdev and driver
> private memory is allocated.
>
> If you want to style your driver such that the private is passed
> around instead of the netdev, put a pointer back to the netdev object
> in your private data structure.

I'm surprised you're okay with the memory waste of that, but you bring
up the ability to change the interface later, which is a great point.
I'll submit a patch for that random driver, and I'll also refactor
WireGuard to do the same. Thanks for the guidance.

Jason

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2017-07-10 11:52 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-12 13:30 [PATCH] netdevice: add netdev_pub helper function Jason A. Donenfeld
2015-06-12 21:20 ` David Miller
2017-07-10  3:19   ` [PATCH 1/2] " Jason A. Donenfeld
2017-07-10  3:19     ` [PATCH 2/2] ioc3-eth: use netdev_pub instead of handrolling alignment Jason A. Donenfeld
2017-07-10  8:04     ` [PATCH 1/2] netdevice: add netdev_pub helper function David Miller
2017-07-10 11:52       ` Jason A. Donenfeld
2016-10-05  0:52 ` [PATCH] " Jason A. Donenfeld

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.