Actually, in the real code, there are printk's showing this function flow
all over it. I just removed them to leave the code clean and show only the
logic.
Just for information, though, this part of the code is never reached.
> > if (test_and_set_bit(0, (void*)&dev->tbusy) != 0) {
> > return 1;
> >
> This kind of locking is legacy code (still present in lots of drivers,
> it was necessary before linux 1.2.9, but strictly depraced now as it might
> be subject to race conditions with linux 2.3.x multithreaded linux
> network core).
>
> Leave it out. tbusy should only be set in the dev->hard_start_xmit()
> thread when an xmitter-busy condition is detected (in your driver, this
> is probably at those location where your cpc_queue_xmit() returns 1).
> Other threads (e.g. interrupt handlers) are only allowed to clear tbusy.
This is _not_ the interrupt handler, this is the function to which
dev->hard_start_xmit() points to. Furthermore, isn't the test_and_set_bit
call the one that sets tbusy to 1 ?? At least that's what I thought. Also,
I thought that [set/clear]_bit functions were used because they are
atomic, which ensures that they work fine even in SMP systems (as oposed
to using 'dev->tbusy = 1' directly). Isn't that correct?
> tbusy is only to tell the upper layer whether the device is willing to
> accept more data for output. It should not be [ab]used for setting
> driver-internal locking state.
I only have to tell the upper layer that it can't write more data to the
device during the DMA write, and that's why I don't need to turn tbusy to
0 _only_ when the TX completion interrupt happens (this board is capable
of transferring several packets at once, with only one interrupt back at
the end of the whole DMA transfer). Once I wrote to the DMA buffer, the
device is ready to receive more data from the upper layer, and that's
when I set tbusy back to 0 (still in the queue_xmit function).
> > /* Write buffer to on-board DMA buffers */
> > CPC_LOCK(card, flags);
> > if(dma_buf_write(card, chan, (ucchar *)skb->data, skb->len) != 0) {
> > CPC_UNLOCK(card, flags);
> > stats->tx_dropped++;
> > dev_kfree_skb(skb);
> > return 1;
> > }
> > CPC_UNLOCK(card, flags);
> >
> > stats->tx_bytes += skb->len;
> > dev_kfree_skb(skb);
> > stats->tx_packets++;
> > dev->trans_start = jiffies;
> > dev->tbusy = 0;
> > mark_bh(NET_BH);
> >
> > /* Start transmission */
> > tx_dma_start(card, chan);
> >
> > return 0;
> >}
> >
> >Note that mark_bh is called just after dev->tbusy is set to 0 (before you
> >ask, there are no dropped packets, so I know that the other places where
> >tbusy could be set to 0 are not reached).
> >
> >Furthermore, the only other places where the tbusy value is changed are
> >the open (tbusy = 0) and close (tbusy = 1) functions.
> >
> Tbusy usually should be cleared whenever the driver detects that
> the device has become ready to accept a new frame for transmission.
> This usually occurs in the driverīs interrupt handler when it detects
> that the last frame has successfully been send out because after that, the
> device is usually willing to accept the next frame for transmission.
> (some other code parts, e.g. error recovery and initialization routines, might
> do so as well).
_BUT_ for this device, as it has _large_ DMA buffers (you can queue up to
32KB of TX data per channel), the driver doesn't need to wait until the
board has actually physically placed the data in the line to tell the
upper layer that it's ready to receive more data. Once the data from the
upper layer is in the DMA buffer, the driver is ready to receive more data
from the upper layer. That's why tbusy is _set_ and _cleared_ in the same
function (cpc_queue_xmit, which is dev->hard_start_xmit).
Did you see the difference? Is this a wrong thing to do? Please let me
know. And thanks for your helpful reply.
Regards,
Ivan
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/