On Thu, 13 Feb 2003, Jeff Garzik wrote:
> I'm curious about the ring-wrapping code... the comments indicate you
> may not have fully investigated the ring-wrapping semantics?
I think you're misreading the code...
The problem with the starfire is that you can only wrap the ring after the
_first_ descriptor in a SG skbuff. That's why the code is more complicated
than it would otherwise be.
> Neither style requires any special handling of "wrap" cases, which your
> patch adds.. Your patch adds things like arbitrary padding of 4 tx
> slots, where you might as well add a comment "/* for luck! */".
Again, I think you're misinterpreting the 4 (which is, as I mentioned,
hardcoded and somewhat arbitrary).
All I'm doing there is making sure there are at least 4 slots available
before waking up the Tx queue. Why? because otherwise you might end up not
having enough slots free for all the descriptors needed for a SG skbuff.
Otherwise, the Becker-style cur_tx <-> dirty_tx handling is in effect, and
works as expected.
> Why not
> actually nail down the problems the code is obvious working around?
Because there aren't any? :-)
There were two big problems in the old driver:
- it was still using the Becker-era tx_full flag, which was non-atomic and
therefore had a race between start_tx and intr_handler.
- it was failing to stop the DMA engines on ifdown, which could end up
corrupting random memory (also inherited from the Becker driver)
The reason all the Tx handling has changed is due to the 64-bit support.
The starfire does have a SG descriptor, but it can only handle 32-bit
buffers. If you want 64-bit, you have to use chained single-buffer
descriptors. So I just changed the code to always use chained
single-buffer descriptors, for both 32-bit and 64-bit buffers -- but
that's again the reason the driver now reserves more than one slot before
turning Tx back on.
> A minor style point too, "s/struct foodesc/foodesc/" is going the
> opposite of preferred.
The alternative is a lot of #ifdef's all over the place. Thanks but no
thanks, I'm no fan of typedef's but ifdef's are worse.
> This is why I have not applied your patch when it was sent to me...
Well, you didn't say much at the time... :-) so I just assumed you were
mostly happy with it.
Thanks,
Ion
--
It is better to keep your mouth shut and be thought a fool,
than to open it and remove all doubt.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Sat Feb 15 2003 - 22:00:51 EST