Can we have GET_NETDEV_DEV?

Can we have GET_NETDEV_DEV?

[Pavel Roskin]

Hello!

gregkh-driver-network-class_device-to-device.patch, which briefly
appeared in Linux 2.6.18-rc1-mm1 broke MadWifi, which is copying the
physical device information from the master network device to the
virtual network devices:

SET_NETDEV_DEV(dev, mdev->class_dev.dev);

The same code exists in hostap.  The patch is gone from 2.6.18-rc1-mm2,
but I'd like to be prepared if it reappears.

An easy solution would be to have GET_NETDEV_DEV macro.  Then the
drivers could do this:

SET_NETDEV_DEV(dev, GET_NETDEV_DEV(mdev));

without having to worry about the internals of struct net_device.  It
should be done before class_dev is removed or in the same time.

Should I send a patch?

-- 
Regards,
Pavel Roskin

Re: Can we have GET_NETDEV_DEV?

[Stephen Hemminger]

On Tue, 25 Jul 2006 00:52:25 -0400
Pavel Roskin <proski@gnu.org> wrote:

> Hello!
> 
> gregkh-driver-network-class_device-to-device.patch, which briefly
> appeared in Linux 2.6.18-rc1-mm1 broke MadWifi, which is copying the
> physical device information from the master network device to the
> virtual network devices:
> 
> SET_NETDEV_DEV(dev, mdev->class_dev.dev);
>

I would rather see SET_NETDEV_DEV go away. It was done for source
compatibility between 2.4 and 2.6 network device drivers. This is
no longer really important.

It would be better to either access the network device directly.
BUT, there are plans to get rid of class_dev so that would mean
source changes to all the drivers again...

So how about these wrappers.

--- a/include/linux/netdevice.h	2006-07-24 10:56:59.000000000 -0700
+++ b/include/linux/netdevice.h	2006-07-25 10:18:12.000000000 -0700
@@ -535,10 +535,23 @@
 }
 
 #define SET_MODULE_OWNER(dev) do { } while (0)
+
 /* 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)->class_dev.dev = (pdev))
+static inline void netdev_set_pdev(struct net_device *dev, struct device *pdev)
+{
+	dev->class_dev.dev = pdev;
+}
+
+static inline struct device *netdev_get_pdev(struct net_device *dev)
+{
+	return dev->class_dev.dev;
+}
+
+
+/* old style macro for compatiablity */
+#define SET_NETDEV_DEV(net, pdev)	netdev_set_pdev(net, pdev)
 
 struct packet_type {
 	__be16			type;	/* This is really htons(ether_type). */

Re: Can we have GET_NETDEV_DEV?

[Pavel Roskin]

Hello, Stephen!

On Tue, 2006-07-25 at 10:20 -0700, Stephen Hemminger wrote:

> So how about these wrappers.

> +static inline void netdev_set_pdev(struct net_device *dev, struct device *pdev)
> +{
> +	dev->class_dev.dev = pdev;
> +}
> +
> +static inline struct device *netdev_get_pdev(struct net_device *dev)
> +{
> +	return dev->class_dev.dev;
> +}

The positive effect of a macro is that if would allow MadWifi to prepare
in advance by testing if GET_NETDEV_DEV is defined.

The functions may or may not be useful for the code in the kernel, but
I'll not be able to prepare MadWifi unless I know the kernel version in
which they will appear.

Considering the drivers that are already in the kernel, you may prefer
to have a more high-level function that would clone the network device
by copying most of the net_device structure.  I think netdev_get_pdev()
would be mostly used for such cloning if implemented.

-- 
Regards,
Pavel Roskin

Re: Can we have GET_NETDEV_DEV?

[Stephen Hemminger]

On Tue, 25 Jul 2006 17:08:11 -0400
Pavel Roskin <proski@gnu.org> wrote:

> Hello, Stephen!
> 
> On Tue, 2006-07-25 at 10:20 -0700, Stephen Hemminger wrote:
> 
> > So how about these wrappers.
> 
> > +static inline void netdev_set_pdev(struct net_device *dev, struct device *pdev)
> > +{
> > +	dev->class_dev.dev = pdev;
> > +}
> > +
> > +static inline struct device *netdev_get_pdev(struct net_device *dev)
> > +{
> > +	return dev->class_dev.dev;
> > +}
> 
> The positive effect of a macro is that if would allow MadWifi to prepare
> in advance by testing if GET_NETDEV_DEV is defined.
> 
> The functions may or may not be useful for the code in the kernel, but
> I'll not be able to prepare MadWifi unless I know the kernel version in
> which they will appear.
> 
> Considering the drivers that are already in the kernel, you may prefer
> to have a more high-level function that would clone the network device
> by copying most of the net_device structure.  I think netdev_get_pdev()
> would be mostly used for such cloning if implemented.
> 

What is the wireless tree using for this?

Re: Can we have GET_NETDEV_DEV?

[Pavel Roskin]

On Tue, 2006-07-25 at 14:26 -0700, Stephen Hemminger wrote:
> On Tue, 25 Jul 2006 17:08:11 -0400
> Pavel Roskin <proski@gnu.org> wrote:
> > Considering the drivers that are already in the kernel, you may prefer
> > to have a more high-level function that would clone the network device
> > by copying most of the net_device structure.  I think netdev_get_pdev()
> > would be mostly used for such cloning if implemented.
> > 
> 
> What is the wireless tree using for this?

Suppose there is a wireless device that can be used on different buses,
e.g. PCI and PCMCIA.  The driver consists of the common part that works
with the chip and the bus specific parts that allocate the resources.

The bus specific parts call SET_NETDEV_DEV to associate the network
device with the hardware device.

Now, suppose the user requests creating a WDS interface.  WDS interfaces
use 4-address headers, which allows bridging.  As it stands now, the way
to support a WDS interface is to make is a separate network device.  I
wish it wouldn't be necessary, but we would need to change quite a few
code inside and outside the kernel to understand WDS addressing (I hope
to be wrong about that).

There is nothing hardware specific about WDS interfaces.  They are just
extensions of the main interface with a fixed "receive address" and
4-address headers enabled.  So it makes sense to create WDS interface in
the common (i.e. bus-agnostic) part of the driver.

In the (possibly misguided) attempt to make WDS network devices look
like real network interfaces, the "class_dev" field is copied from the
master network device, because it's something that can be done in the
bus-agnostic way:

SET_NETDEV_DEV(dev, mdev->class_dev.dev);

This is how it's done in HostAP (in the kernel) and MadWifi (outside the
kernel).

Sure, this value can be stored in the private part of the driver, so
it's a minor issue.

(Actually, the main (AP or STA) interface is also made a virtual network
device, so that the the master interface appears as the parent of the
main and the WDS devices and as the single point for monitoring of all
wireless traffic.)

-- 
Regards,
Pavel Roskin

Re: Can we have GET_NETDEV_DEV?

[Greg KH]

On Tue, Jul 25, 2006 at 10:20:05AM -0700, Stephen Hemminger wrote:
> On Tue, 25 Jul 2006 00:52:25 -0400
> Pavel Roskin <proski@gnu.org> wrote:
> 
> > Hello!
> > 
> > gregkh-driver-network-class_device-to-device.patch, which briefly
> > appeared in Linux 2.6.18-rc1-mm1 broke MadWifi, which is copying the
> > physical device information from the master network device to the
> > virtual network devices:
> > 
> > SET_NETDEV_DEV(dev, mdev->class_dev.dev);
> >
> 
> I would rather see SET_NETDEV_DEV go away. It was done for source
> compatibility between 2.4 and 2.6 network device drivers. This is
> no longer really important.

It did make my change much simpler to do, which I appreciated :)

> It would be better to either access the network device directly.
> BUT, there are plans to get rid of class_dev so that would mean
> source changes to all the drivers again...
> 
> So how about these wrappers.

They look good to me.

BTW, I'll be getting the patch back into -mm that does the class_device
-> device conversion after I fix the SuSE 10.0 release that doesn't like
the change and get that pushed out to users.  After that happens, there
shouldn't be any userspace problems with this patch.

thanks,

greg k-h