[Top] [All Lists]

Re: 3c59x.c

To: andrewm@xxxxxxxxxx (Andrew Morton)
Subject: Re: 3c59x.c
From: kuznet@xxxxxxxxxxxxx
Date: Thu, 30 Mar 2000 19:29:29 +0400 (MSK DST)
Cc: netdev@xxxxxxxxxxx
In-reply-to: <> from "Andrew Morton" at Mar 28, 0 03:01:43 pm
Sender: owner-netdev@xxxxxxxxxxx

> mm..  The problem with this approach is that it doesn't scale.  _I_
> learn the answer, and then the next person comes along...

I do not know better one, I am sorry.

> - Why do some drivers statically allocate their struct net_device
> (eepro, cs89x0) whereas others call init_etherdev()?  What's the right
> thing to do?

By historical reasons. This mess was supposed to be rectified
some time ago in 2.3 (when file drivers/net/setup.c was created),
but the cleanup was not completed, unfortunately.

I think right way is not to use static net_device structures at all.
But it can (and do) result in problems with passing kernel boot options
to driver, which are unsolved until now.

> - Why do some go (in probe1):
>   if (dev == NULL)
>       init_etherdev()
> whereas others do not test for null?  Is the test necessary?

It is consequence of the first. There were three major kinds of device

1. Devices which were in initial "boot" chain were allocated statically
   (or declared statically in module), and probe1() got a struct and
   allocated private data itself.
   (it was the case when check dev == NULL was made before calling
2. Device is not in initial "boot" chain, but its "eth*" label
   is found in initial chain, so that only private data are allocated.
3. Device is not in boot chain and allocated as whole with private
   data piggybacked to main struct.

To be honest, I do not know what way is correct. This three-fold way
was just shit, new one is apparently cleaner, but it is incomplete,
at least passing options via kernel command line does not work now,
which is bad. I am not an expert here, alas.

> - The manipulation of the pci_root_buses and pci_devices lists in pci.c
> has no SMP or IRQ protection.  The net drivers call into pci.c to
> add/remove things from these lists but provide no race avoidance.  Is
> there something higher up which guarantees that these list operations
> are serialised wrt some random soundcard driver or is this a bug?

I do not know, I am not an expert here too. Seems, all this code is supposed
to be executed only under kernel lock and never executed from an
interrupt context. Then it should be valid.

> - As far as I can tell, many Ethernet chips separate the Tx and Rx
> functions sufficiently well for the driver author to be able to let the
> Tx and Rx threads operate independently.  And the orginal architecture
> allowed this to occur.

It did not change.

> But the recommended practice of locking the whole driver within
> hard_start_xmit() will penalise the Rx threads.

This practice was not recommended, certainly!

If you mean "controller" lock, it is TX lock as rule.
RX locking is made automatically by IRQ serialization logic,
so that the question occurs only when driver needs to make something
serialized with RX interrupts itself. No help from top level
is possible here, it is an internal driver problem and driver
have to create its private locks, if it is necessary,
or, alternatively, to disable device irqs, while doing dangerous job.

> If this is correct, should we be using separate rx and tx device-private
> locks?

If driver needs them.

> - Should we be hardwiring ISAPNP databases into the drivers?  Shouldn't
> these be on disk? (this question is rhetorical - I don't think anyone's
> interested in ISA any more).

I have no idea. I know about this even less than about PCI interface,
i.e. less then nothing.

> A lot of these questions would be perfectly answered if someone such as
> yourself were to give the skeleton driver an overhaul.  Then if any
> drivers deviate from its recipe we need to understand why.

I can provide only that part of the skeleton, which interfaces to linux/net/.
PCI (or ISA PNP) interface is terra incognita for me. Such skeleton requires
collective work.

> BTW: a while back I noticed that the driver's probe1() is being called a
> huge number of times at bootup from net/dev.c.  Is this known about?

No... Could you give more information? For what device does this occur?


<Prev in Thread] Current Thread [Next in Thread>