[1591] in linux-net channel archive
Re: Network device setup (was Re: unresolved register_netdev() in 52)
daemon@ATHENA.MIT.EDU (Avery Pennarun)
Thu Jan 4 19:49:11 1996
Date: Wed, 3 Jan 1996 02:40:55 -0500 (EST)
From: Avery Pennarun <apenwarr@foxnet.net>
To: Paul Gortmaker <gpg109@rsphy1.anu.edu.au>
cc: linux-net@vger.rutgers.edu
In-Reply-To: <9601020437.AA23646@rsphy6.anu.edu.au>
On Tue, 2 Jan 1996, Paul Gortmaker wrote:
> Yeah, I like a solution similar to register_netdev() as well, but what I
> was trying to get across is that all the "static struct device ..." in
> Space.c just to hook into the probe are ugly (with the exception of
> eth0->3).
It's probably just my attachment to the arcnet driver, but I personally am
not very happy with the way ethernet cards get "special treatment" in so
many cases. This is a good example: there can just as easily be three
arcnet cards as three ethernets installed, but I need to write special code
to allocate arc0-arc2. With eth0-eth2 it happens automatically. (I suppose
I could lazily just use eth0-eth2 for arcnet, since it's so similar, but
that just isn't as pretty.)
On the other hand, I think I can see how not changing the way ethernet
devices work would be a real timesaver for the poor programmer who has to go
and change all the init functions. See (rather far) below for a possible
interim solution to this.
> I was thinking along the line of getting rid of those non-ether
> "static struct device ..." and then these drivers would (upon detecting a
> valid physical device) do an init_netdev() (similar to part of
> the present init_etherdev(), but w/o touching the list) which would create
> a device struct, and fill in some of the fields, The driver would then
> fill in the rest, make sure everything came up allright, and then call
> add_to_list() or whatever to get itself into the linked list. Not
> horribly different from the register_netdev(), but if all drivers
> (i.e. non ethN ones) used it, Space.c wouldn't be so ugggggly.
It seems to be that there would be no problem with just:
struct device *dev=init_netdev();
[...fill in device-specific fields...]
register_netdev(dev);
I don't think there needs to be a specific add_to_list function. If all
drivers did this, there would be no need for two separate initialization
functions (one for modules and one for non-modules) - this would be fine
with me!
I think init_netdev should be a bit more flexible than the current scheme,
though. In particular, it should accept a prefix to give to the device
name. eg. for arcnet:
dev=init_netdev("arc");
It could then auto-generate names for arcnet, slip, token ring, etc as well
as ethernet without all the code duplication. (Up to now I've been too lazy
to even bother with auto-naming for arcnet - the driver requires you to give
the number yourself when loading the module). I guess init_netdev should
accept a null prefix as well to let the driver name the device itself - that
way I could keep using "arc0e" and "arc0s" which are auto-generated by arc0
upon ifconfig.
Hmm, and move the skb_queue_head_init loop from eth_setup, tr_setup, etc
here (or in register_netdev) as well. The loop is the same every time and
belongs in a common function.
> > Of course, you still need something like Space.c to do the probing
> > before register_netdev will ever be called, if modules aren't being
> > used. It's hard to win :)
>
> Exactly. So the aforementioned devices that handle their own dev
> structures dynamically (arcnet, slip, ppp, etc.) go into a
> function similar to ethif_probe(), lets say netif_probe() which is
> also in Space.c and called once from net/core/dev.c - something like:
>
> [...code deleted...]
>
> You get the idea. No struct device is passed in or allocated in Space.c
> for these devices. They would allocate them themselves, be it one,
> two or twenty, and do so as they are required. In the case of the above,
> arcnet_probe() would be responsible for finding all arcnet cards, and so
> one may wish to consider setting up an "arcnet=" command line, so users
> could do "arcnet=5,0x280,0xd0000,arc0 arcnet=9,0x300,0xd8000,arc1" etc.
> It would also be responsible for inserting all the valid arcnet device
> structures into the linked list, using the hypothetical add_to_list(dev);
The problem with this is that important information is currently being
passed via 'dev'. (ie. the "ether" portion of the kernel command line.)
Assuming again that all drivers are created equal, perhaps something like
this would be best:
netdev=0x300,9,0xd0000,...,arcnet (not arc0)
(with the added advantage of putting the io address before the IRQ like it
should have been all along :))
The idea is that a function, say parse_netdev(), would receive the kernel's
command line as usual like this:
parse_netdev(char *line,int *ints);
where "line" is the actual command line given, and "ints" is the
kernel-parsed list of comma-separated numbers as usual. parse_netdev could
do a little more processing, ie. to separate the 'arc' parameter from the
end of "line", and then call each probe like this:
arcnet_probe(line,ints,name);
A driver which doesn't recognize the "arcnet" name will just choose not to
probe for the device. The actual "ints" provided don't need to be in a
certain order all the time; it would depend on the device probe used.
There's no real need to provide the device name on the command line
anyway, what with the new auto-registration features :) The order of
entries on the command line could still make sure eth0 and eth1 were always
the same devices.
We could even allow the last parameter to be empty, which means all devices
should try the probe. In fact, the probe functions will need to accept NULL
values for all parameters, for use if no kernel parameters are given.
> > Oh, by the way, whatever happened to the patch that added a "kick me"
> > function call to all the network drivers? I remember there was a patch
> > for this in 1.2.x during the old 1.3.6-or-so days.
>
> That was/is a brutal editing job. If I could convince net driver authors
> to separate out their individual Tx timeout code into a separate
> function, then it would be easy. For example:
Hmm, I recently got annoyed with the entire check-this-check-that opening
section of the transmitter function (I had to duplicate it between all three
"virtual transmitters" in the arcnet driver) and separated it out already.
Until I did that, splitting out a "kickme" function would have been rather
painful, but now it should be as easy as with any network driver.
Really, though, wouldn't it make sense to add support for the kickme
function to the higher levels, and just revert to the old behaviour if it's
not used (ie. dev->kickme==NULL)? I think that's what the original patch
did anyway. That way drivers could be changed a bit at a time.
> [...some code deleted...]
>
> to:
> ----------------------------------------------------------------
> /* Routine to handle Tx timeout for Foo Bar net device */
> static int foo_timeout(dev)
> {
> printk("%s: transmit timed out, oops\n",dev->name);
> ....
> [transmit timeout fixup code here]
> ....
> }
>
> [...]
>
> if (dev->tbusy) {
> int tickssofar = jiffies - dev->trans_start;
> if (tickssofar < TX_TIMEOUT)
> return 1;
> foo_timeout(dev);
> }
> ----------------------------------------------------------------
What happens when the higher-level calls the kickme function, though? Where
is TX_TIMEOUT checked? As an experiment, I tried taking out the "tickssofar
< TX_TIMEOUT" business in the arcnet driver and the results were very
unpleasant :)
I'm also suspicious that arcnet needs a higher timeout value than ethernets,
simply because arcnet cards don't blindly transmit a packet regardless of
everyone else, but actually have to wait for permission. (I _think_ I read
that a very big, very busy arcnet network can have latency up to 86ms).
Currently I'm using a TX_TIMEOUT of 20 which should handle all
possibilities, and considering the unlikeliness of an actual arcnet timeout,
it should work fine.
> As you can see this type of change is not rocket science, and makes
> the driver source easier to read anyway. I don't have the time to
> do this again for all 5 zillion linux net devices, however.
As all the error-checking is already in a separate function in arcnet.c, I
doubt this would improve readability all that much. I'll still do it if it
helps, though :)
I imagine redesigning Space.c (and correspondingly, all autoprobe routines,
yech) is probably even more work. Maybe an interim function, say a modified
ethif_probe, that works like the new probe functions "should" and
auto-allocates a struct device by itself, fills in the values with any
kernel parameters, and then calls the old-style ethernet probe functions as
usual:
ethif_probe(char *line,int *ints,char *name)
{
[...handle parameters, if any...]
dev=init_netdev("eth");
if (1
&& seeq8005_probe(dev)
&& hp100_probe(dev)
&& ultra_probe(dev)
...
&& 1) {
return 1;
}
return 0;
}
As ethernet devices were "converted" they could be moved out of ethif_probe
to the generic net_probe.
Hmm. If no one objects to my probably-too-big plans I may try to start this
myself. Seems like it would be fairly easy to do - and if I don't try to
convert all the ethernet drivers at once, not too tedious either. (with the
added bonus that I don't have to write the "arc" version of "automatically
choose a device name").
Of course, unless I get truly inspired one day I don't think I want to try
adding "kickme" support to the latest kernel. (It's new territory for me
:))
Have fun.
Avery
