From: "Jason A. Donenfeld" <Jason@zx2c4.com> Date: 2015-06-12 13:30:35
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(+)
From: David Miller <davem@davemloft.net> Date: 2015-06-12 21:21:02
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.
From: "Jason A. Donenfeld" <Jason@zx2c4.com> Date: 2016-10-05 00:52:35
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 [off-list ref] wrote:
quoted hunk
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(+)
From: "Jason A. Donenfeld" <Jason@zx2c4.com> Date: 2017-07-10 03:20:09
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(+)
@@ -2030,26 +2030,37 @@ void dev_net_set(struct net_device *dev, struct net *net)}/***netdev_priv-accessnetworkdeviceprivatedata*@dev:networkdevice**Getnetworkdeviceprivatedata*/staticinlinevoid*netdev_priv(conststructnet_device*dev){return(char*)dev+ALIGN(sizeof(structnet_device),NETDEV_ALIGN);}+/**+*netdev_pub-accessnetworkdevicefromprivatepointer+*@priv:privatedatapointerofnetworkdevice+*+*Getnetworkdevicefromanetworkdeviceprivatedatapointer+*/+staticinlinestructnet_device*netdev_pub(void*priv)+{+return(structnet_device*)((char*)priv-ALIGN(sizeof(structnet_device),NETDEV_ALIGN));+}+/* Set the sysfs physical device reference for the network logical device*ifsetpriortoregistrationwillcauseasymlinkduringinitialization.*/#define SET_NETDEV_DEV(net, pdev) ((net)->dev.parent = (pdev))/* Set the sysfs device type for the network logical device to allow*fine-grainedidentificationofdifferentnetworkdevicetypes.For*exampleEthernet,WirelessLAN,Bluetooth,WiMAXetc.*/#define SET_NETDEV_DEVTYPE(net, devtype) ((net)->dev.type = (devtype))/* Default NAPI poll() weight*Devicedriversarestronglyadvisedtonotusebiggervalue
From: "Jason A. Donenfeld" <Jason@zx2c4.com> Date: 2017-07-10 03:20:41
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(-)
@@ -86,31 +86,26 @@ struct ioc3_private {inttx_ci;/* TX consumer index */inttx_pi;/* TX producer index */inttxqlen;u32emcr,ehar_h,ehar_l;spinlock_tioc3_lock;structmii_if_infomii;structpci_dev*pdev;/* Members used by autonegotiation */structtimer_listioc3_timer;};-staticinlinestructnet_device*priv_netdev(structioc3_private*dev)-{-return(void*)dev-((sizeof(structnet_device)+31)&~31);-}-staticintioc3_ioctl(structnet_device*dev,structifreq*rq,intcmd);staticvoidioc3_set_multicast_list(structnet_device*dev);staticintioc3_start_xmit(structsk_buff*skb,structnet_device*dev);staticvoidioc3_timeout(structnet_device*dev);staticinlineunsignedintioc3_hash(constunsignedchar*addr);staticinlinevoidioc3_stop(structioc3_private*ip);staticvoidioc3_init(structnet_device*dev);staticconstcharioc3_str[]="IOC3 Ethernet";staticconststructethtool_opsioc3_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,thisishosedbydesign.It'snecessarytoknowwhatmachinethe*NICisininordertoknowhowtoreadtheNICaddress.Wealsohave*toknowifit'saPCIcardoraNICinonthenodeboard...*/staticvoidioc3_get_eaddr(structioc3_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);}staticvoid__ioc3_set_mac_address(structnet_device*dev){structioc3_private*ip=netdev_priv(dev);structioc3*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]);}staticintioc3_set_mac_address(structnet_device*dev,void*addr)
@@ -780,27 +775,27 @@ static void ioc3_timer(unsigned long data)add_timer(&ip->ioc3_timer);}/**TrytofindaPHY.ThereisnoapparentrelationbetweentheMIIaddresses*intheSGIdocumentationandwhatwefindinreality,sowesimplyprobe*forthePHY.ItseemsIOC3PHYsusuallyliveonaddress31.Oneofmy*onboardIOC3shasthespecialodditythatprobingdoesn'tseemtofindit*yettheinterfaceseemstoworkfine,soifprobingfailswefornowwill*simplydefaulttoPHY31insteadofbailingout.*/staticintioc3_mii_init(structioc3_private*ip){-structnet_device*dev=priv_netdev(ip);+structnet_device*dev=netdev_pub(ip);inti,found=0,res=0;intioc3_phy_workaround=1;u16word;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 */}}
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.
From: "Jason A. Donenfeld" <Jason@zx2c4.com> Date: 2017-07-10 11:52:50
On Mon, Jul 10, 2017 at 10:04 AM, David Miller [off-list ref] 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