netdev_ops retraction

netdev_ops retraction

[Matthew Wilcox]

So I've prototyped netdev_ops over a few drivers, and I don't like it.
Here's why I don't like it:

+struct netdev_ops {
+       struct net_device_stats * (*get_stats)(struct net_device *dev);
+       void    (*uninit)(struct net_device *dev);
+       void    (*destructor)(struct net_device *dev);
+       int     (*open)(struct net_device *dev);
+       int     (*stop)(struct net_device *dev);
+       int     (*hard_start_xmit)(struct sk_buff *skb, struct net_device *dev);
+       int     (*poll)(struct net_device *dev, int *quota);
+       int     (*hard_header)(struct sk_buff *skb, struct net_device *dev,
+                               unsigned short type, void *daddr,
+                               void *saddr, unsigned len);
+       int     (*rebuild_header)(struct sk_buff *skb);
+       void    (*set_multicast_list)(struct net_device *dev);
+       int     (*set_mac_address)(struct net_device *dev, void *addr);
+       int     (*do_ioctl)(struct net_device *dev, struct ifreq *ifr, int cmd);
+       int     (*set_config)(struct net_device *dev, struct ifmap *map);
+       int     (*hard_header_cache)(struct neighbour *neigh, struct hh_cache *hh);
+       void    (*header_cache_update)(struct hh_cache *hh,
+                       struct net_device *dev, unsigned char * haddr);
+       int     (*change_mtu)(struct net_device *dev, int new_mtu);
+       void    (*tx_timeout)(struct net_device *dev);
+       void    (*vlan_rx_register)(struct net_device *dev, struct vlan_group *grp);
+       void    (*vlan_rx_add_vid)(struct net_device *dev, unsigned short vid);
+       void    (*vlan_rx_kill_vid)(struct net_device *dev, unsigned short vid);
+       int     (*hard_header_parse)(struct sk_buff *skb, unsigned char *haddr);
+       int     (*neigh_setup)(struct net_device *dev, struct neigh_parms *);
+       int     (*accept_fastpath)(struct net_device *, struct dst_entry*);
+
+       int     (*get_settings)(struct net_device *, struct ethtool_cmd *);
+       int     (*set_settings)(struct net_device *, struct ethtool_cmd *);
+       void    (*get_drvinfo)(struct net_device *, struct ethtool_drvinfo *);
+       int     (*get_regs_len)(struct net_device *);
+       void    (*get_regs)(struct net_device *, struct ethtool_regs *, void *);
+       void    (*get_wol)(struct net_device *, struct ethtool_wolinfo *);
+       int     (*set_wol)(struct net_device *, struct ethtool_wolinfo *);
+       u32     (*get_msglevel)(struct net_device *);
+       void    (*set_msglevel)(struct net_device *, u32);
+       int     (*nway_reset)(struct net_device *);
+       u32     (*get_link)(struct net_device *);
+       int     (*get_eeprom)(struct net_device *, u32 offset, u32 len, u8 *, u32 magic);
+       int     (*set_eeprom)(struct net_device *, u32 offset, u32 len, u8 *, u32 magic);
+       int     (*get_coalesce)(struct net_device *, struct ethtool_coalesce *);
+       int     (*set_coalesce)(struct net_device *, struct ethtool_coalesce *);
+       void    (*get_ringparam)(struct net_device *, struct ethtool_ringparam *);
+       int     (*set_ringparam)(struct net_device *, struct ethtool_ringparam *);
+       void    (*get_pauseparam)(struct net_device *, struct ethtool_pauseparam *);
+       int     (*set_pauseparam)(struct net_device *, struct ethtool_pauseparam *);
+       u32     (*get_rx_csum)(struct net_device *);
+       int     (*set_rx_csum)(struct net_device *, u32);
+       u32     (*get_tx_csum)(struct net_device *);
+       int     (*set_tx_csum)(struct net_device *, u32);
+       u32     (*get_sg)(struct net_device *);
+       int     (*set_sg)(struct net_device *, u32);
+       int     (*self_test_count)(struct net_device *);
+       void    (*self_test)(struct net_device *, struct ethtool_test *, u64 *);
+       void    (*get_strings)(struct net_device *, u32 stringset, u8 *);
+       int     (*phys_id)(struct net_device *, u32);
+       int     (*get_stats_count)(struct net_device *);
+       void    (*get_ethtool_stats)(struct net_device *, struct ethtool_stats *, u64 *);
+};

Never mind the additional pointer dereference in the code, this is just
a horribly large data structure.  Unless someone convinces me otherwise,
I'm going to drop this idea and revert to doing ethtool_ops as a separate
data structure.  I think there's still scope for a netdev_ops patch,
but it's of dubious value and more of a 2.7 project.

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Re: netdev_ops retraction

[Jeff Garzik]

On Wed, Jul 30, 2003 at 07:44:16PM +0100, Matthew Wilcox wrote:
> Never mind the additional pointer dereference in the code, this is just
> a horribly large data structure.  Unless someone convinces me otherwise,
> I'm going to drop this idea and revert to doing ethtool_ops as a separate
> data structure.  I think there's still scope for a netdev_ops patch,
> but it's of dubious value and more of a 2.7 project.

Definitely OK with me.

And I think this is probably the best route for the moment, especially.

	Jeff

Re: netdev_ops retraction

[Arnaldo Carvalho de Melo]

Em Wed, Jul 30, 2003 at 07:44:16PM +0100, Matthew Wilcox escreveu:
> I think there's still scope for a netdev_ops patch, but it's of dubious value
> and more of a 2.7 project.

OK with me, I mentioned this in a brainstorm and thought of it as a 2.7 thing
anyway.

- Arnaldo

Re: netdev_ops retraction

[David S. Miller]

On Wed, 30 Jul 2003 20:35:42 -0300
Arnaldo Carvalho de Melo <acme@conectiva.com.br> wrote:

> Em Wed, Jul 30, 2003 at 07:44:16PM +0100, Matthew Wilcox escreveu:
> > I think there's still scope for a netdev_ops patch, but it's of dubious value
> > and more of a 2.7 project.
> 
> OK with me, I mentioned this in a brainstorm and thought of it as a 2.7 thing
> anyway.

I'm ok with the simplified ethtool-only version too.

Although I'm confused about what kind of problem there is with
netdev_ops being such a "large structure".

This is the kind of thing there'd be _ONE_ copy of in each driver,
ala.

struct netdev_ops tg3_netdev_ops {
 ...
	.foo	=	tg3_foo,
 ...
};

...
	tp->dev->netdev_ops = &tg3_netdev_ops;
...

Right?

Re: netdev_ops retraction

[Matthew Wilcox]

On Wed, Jul 30, 2003 at 04:41:08PM -0700, David S. Miller wrote:
> I'm ok with the simplified ethtool-only version too.
> 
> Although I'm confused about what kind of problem there is with
> netdev_ops being such a "large structure".

Hard to understand/keep track of is my main concern.  There was also
a namespace collision -- two functions called get_stats.  It's now
get_ethtool_stats.

> This is the kind of thing there'd be _ONE_ copy of in each driver,
> ala.
> 
> struct netdev_ops tg3_netdev_ops {
>  ...
> 	.foo	=	tg3_foo,
>  ...
> };
> 
> ...
> 	tp->dev->netdev_ops = &tg3_netdev_ops;
> ...
> 
> Right?

I'm glad you mentioned tg3 as an example, since it's one of the ones where
this isn't true.

        if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5700)
                tp->dev->hard_start_xmit = tg3_start_xmit_4gbug;
        else
                tp->dev->hard_start_xmit = tg3_start_xmit;

I fixed this with two netdev_ops structs, but imagine a driver with two
or three more cases like this and everything starts to look quite nasty.
I do think we want to do netdev_ops, but not now, and let's keep
netdev_ops and ethtool_ops separate.

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

multiple unicast mac address (was Re: netdev_ops retraction)

[Rick Payne]

--On Wednesday, July 30, 2003 7:44 pm +0100 Matthew Wilcox 
<willy@debian.org> wrote:

> +       void    (*set_multicast_list)(struct net_device *dev);
> +       int     (*set_mac_address)(struct net_device *dev, void *addr);

Talking of which - is there any appetite for a patch that allows multiple 
unicast mac addresses to be set on an ethernet interface? Its certainly 
much neater for things like VRRP and HA stuff if an ethernet device is able 
to continue accepting packets for its original MAC address, as well as the 
'virtual MAC address'.

Obviously I'm not talking about generated packets (they will still take the 
MAC address from dev->dev_addr) - I'm talking about the hardware filter on 
the ethernet cards themselves. (In some cases, the software concerned may 
want to set_mac_address - thus updating dev->dev_addr, and then also add 
the original mac address to the 'unicast accept list' for instance).

Some ethernet cards seem to support this and don't care what MAC addresses 
get put in the multicast list - and I've used that technique before (on 
cards such as the eepro100 for instance). Others may have a different, not 
currently used method to set multiple unicast MAC addresses. Finally, as a 
worst last case - a card could go into promiscuous mode and filter in 
software.

Should I just start on a patch and submit it here for comment?

Rick

Re: multiple unicast mac address (was Re: netdev_ops retraction)

[Lars Marowsky-Bree]

[-- Attachment #1: Type: text/plain, Size: 803 bytes --]

On 2003-07-31T13:49:19,
   Rick Payne <rickp@rossfell.co.uk> said:

> Some ethernet cards seem to support this and don't care what MAC addresses 
> get put in the multicast list - and I've used that technique before (on 
> cards such as the eepro100 for instance). Others may have a different, not 
> currently used method to set multiple unicast MAC addresses. Finally, as a 
> worst last case - a card could go into promiscuous mode and filter in 
> software.
> 
> Should I just start on a patch and submit it here for comment?

Please do.


Sincerely,
    Lars Marowsky-Brée <lmb@suse.de>

-- 
SuSE Labs - Research & Development, SuSE Linux AG
  
"If anything can go wrong, it will." "Chance favors the prepared (mind)."
  -- Capt. Edward A. Murphy            -- Louis Pasteur

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: multiple unicast mac address (was Re: netdev_ops retraction)

[Jeff Garzik]

Rick Payne wrote:
> 
> --On Wednesday, July 30, 2003 7:44 pm +0100 Matthew Wilcox 
> <willy@debian.org> wrote:
> 
>> +       void    (*set_multicast_list)(struct net_device *dev);
>> +       int     (*set_mac_address)(struct net_device *dev, void *addr);
> 
> 
> Talking of which - is there any appetite for a patch that allows 
> multiple unicast mac addresses to be set on an ethernet interface? Its 
> certainly much neater for things like VRRP and HA stuff if an ethernet 
> device is able to continue accepting packets for its original MAC 
> address, as well as the 'virtual MAC address'.
> 
> Obviously I'm not talking about generated packets (they will still take 
> the MAC address from dev->dev_addr) - I'm talking about the hardware 
> filter on the ethernet cards themselves. (In some cases, the software 
> concerned may want to set_mac_address - thus updating dev->dev_addr, and 
> then also add the original mac address to the 'unicast accept list' for 
> instance).


This feature request comes up about once a year.  Search the archives 
for responses...

Hardware that filters N MAC addresses (unicast filtering) doesn't have a 
terribly standard interface, and the unicast filter must be adjusted at 
different times on different hardware.  Also, chip bugs lead one to 
think unicast filtering will work where it doesn't.  Also, chip limits 
for some of the more popular chips are unknown.  Also, the need for this 
feature is very uncommon, and can be achieved in other ways.

	Jeff

Re: multiple unicast mac address (was Re: netdev_ops retraction)

[Rick Payne]

--On Thursday, July 31, 2003 10:44 am -0400 Jeff Garzik <jgarzik@pobox.com> 
wrote:

> Hardware that filters N MAC addresses (unicast filtering) doesn't have a
> terribly standard interface, and the unicast filter must be adjusted at

Indeed but where its possible to support it, it can be - and those cards 
will be specified by those who need it (for HA, VRRP etc).

> different times on different hardware.  Also, chip bugs lead one to think
> unicast filtering will work where it doesn't.  Also, chip limits for some
> of the more popular chips are unknown.  Also, the need for this feature
> is very uncommon, and can be achieved in other ways.

As I said - promiscuous mode and filtering on the receive side - which is 
what you have to resort to anyway for those cards that don't support it.

Alternatively, its just another patch people need to add to make things do 
what they want - just like the ARP patch.

Rick

Re: multiple unicast mac address (was Re: netdev_ops retraction)

[jamal]

Last discussion that happened:

http://marc.theaimsgroup.com/?t=104163060100001&r=1&w=2

cheers,
jamal


On Thu, 2003-07-31 at 11:09, Rick Payne wrote:
> --On Thursday, July 31, 2003 10:44 am -0400 Jeff Garzik <jgarzik@pobox.com> 
> wrote:
> 
> > Hardware that filters N MAC addresses (unicast filtering) doesn't have a
> > terribly standard interface, and the unicast filter must be adjusted at
> 
> Indeed but where its possible to support it, it can be - and those cards 
> will be specified by those who need it (for HA, VRRP etc).
> 
> > different times on different hardware.  Also, chip bugs lead one to think
> > unicast filtering will work where it doesn't.  Also, chip limits for some
> > of the more popular chips are unknown.  Also, the need for this feature
> > is very uncommon, and can be achieved in other ways.
> 
> As I said - promiscuous mode and filtering on the receive side - which is 
> what you have to resort to anyway for those cards that don't support it.
> 
> Alternatively, its just another patch people need to add to make things do 
> what they want - just like the ARP patch.
> 
> Rick
> 
>