[PATCH] netdevice: add netdev_pub helper function

[PATCH] netdevice: add netdev_pub helper function

[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

Re: [PATCH] netdevice: add netdev_pub helper function

[David Miller]

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.

[PATCH 1/2] netdevice: add netdev_pub helper function

[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

[PATCH 2/2] ioc3-eth: use netdev_pub instead of handrolling alignment

[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

Re: [PATCH 1/2] netdevice: add netdev_pub helper function

[David Miller]

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.

Re: [PATCH 1/2] netdevice: add netdev_pub helper function

[Jason A. Donenfeld]

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

Re: [PATCH] netdevice: add netdev_pub helper function

[Jason A. Donenfeld]

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
>