Donald,
thanks for the reply - I hoped you would be listening.
I'd like to work this through a little more then summarise the outcome
as a plan of what (if anything) needs to be done. Once the intended
changes are well reviewed someone (doesn't really matter who) can cut a
patch. Sound OK?
Donald Becker wrote:
>
> On Sat, 25 Mar 2000, Andrew Morton wrote:
>
> > To: netdev <netdev@xxxxxxxxxxx>
> > Subject: 3c59x.c
> >
> > Would anyone object if I did some work on this driver? (2.3.99)
>
> You should bring this up on the linux-vortex mailing list.
> If you do not read the linux-vortex mailing list, you should not even think
> about modifying the driver until you understand more about it.
Thanks - I'll check it out.
> > - Debug code is _always_ compiled in, and debug level defaults to 1.
> > This is OK, it would be better to make it easy for 'debug' to be
> > literal "0". This reduces the driver size by 1.5k.
>
> This is highly useful information when tracking down problems.
> Most problems are not in the driver structure, but problems with the system
> (IRQs, PCI bus, and the like) or the media selection.
I wasn't proposing that any diagnostic tools be removed. I was noting
that under some circumstances, dead code elimination could be used to
remove code which is never executed.
In particular, when the driver is linked into the kernel there appears
to be no way to alter the value of 'debug'. May as well make it a
literal constant and let the compiler remove the dead code completely.
Minor issue.
> ...
>
> > - why do we independently kmalloc dev->priv? Why not let
> > init_etherdev() do it?
>
> Because of alignment requirements. Some descriptors must be longword
> aligned, and they perform much better when cache line aligned. You must
> understand caching systems before touching this. It's OK to take slight
> performance hits on rarely used systems like the Sparc, but you should
> throughly understand what the PCI bus implementation on the adapter is doing.
>
init_etherdev() aligns dev->priv on a 32 byte boundary (this was
recently fixed up). Is this not suitable?
> ...
> > - Are there MOD_INC/DEC_USE_COUNT races? What are the rules here?
>
> The locking for this is handled outside the drivers, as it should be.
Sorry, this was a comment to myself which snuck through. I am told that
there are insmod/rmmod races in various drivers but I've never seen a
description of the sorts of things to look out for.
> > - vortex_start_xmit() is racy. It is not protected from h/w
> > interrupts. It needs a spin_lock_irqsave(priv->lock) and
> > vortex_interrupt() needs a spin_lock(priv->lock);
>
> The vortex_start_xmit() is for the 3c590 series only, not the later hardware.
> The hardware only needs to be protected against simultaneous transmit
> attempts, not a receive that happens in the middle of transferring a packet
> to the card's (large!) FIFO.
Are you sure about this? That a Tx complete interrupt (or a
vortex_interrupt() call from vortex_tx_timeout()) cannot occur when
vortex_start_xmit() is executing?
> Putting a spin lock there will waste a *lot* of spin time, since the old
> 3c590 isn't especially fast, especially if we are using PIO mode.
I assume the wastage you are talking about it stalling the Rx interrupt
while the PIO Tx is in progress?
> > * vortex_start_xmit() calls netif_stop_queue() and then under some
> > circumstances (non-DMA o/p and there is room in the Tx buffer) it
> > calls netif_wake_queue(). Seems OK, as long as it's done under the
> > spin_lock_irqsave().
>
> These macros are only OK if they are trivial locking functions. When they
> are more complex functions every card without a large Tx queue will suffer
> badly.
They're quite lightweight. Probably < 100 insns for the worst case.
> > - [OT] waah! skeleton.c uses global cli()/sti()!
>
> I've had pci-skeleton.c for a long time, but it apparently wasn't worthy.
> It is now incorrect for 2.3.*.
That went over my head...
I find skeleton.c very useful. I'd be interested in pci-skeleton.c. Is
it available somewhere?
> At least parts of the ancient skeleton.c have been updated with the new
> interface.
Parts, yes. It's missing the align-IP-header-on-longword fiddle as well
as using the global cli(). There could be other things. I'll have a
closer look.
You didn't address some of my observations - I think they were pretty
straightforward things like checking returns from resource allocation
attempts. I'll included them when I revisit this later in the week.
--
-akpm-
|