Re: [rt2x00-users] [PATCH 3/5] rt2x00: rt2800usb: rework txstatus code

[Jakub Kicinski <moorray@wp.pl>]

Hi!

On Mon, 19 Mar 2012 08:52:24 +0100
Stanislaw Gruszka <sgruszka@redhat.com> wrote:

> > > +	if (rt2800usb_txstatus_pending(rt2x00dev) &&
> > > +	    test_and_set_bit(TX_STATUS_READING, &rt2x00dev->flags))
> > 
> > I would put a bang before that test_and...
> I don't understand what you mean, perhaps you could post a patch
> or provide code snipset here, so I could comment.

If I understand correctly, status should be read again if there are
pending entries and no one else has set TX_STATUS_READING yet. In that
case return value of test_and_set_bit should be negated. I might be
missing something though.

> > > +	while (!kfifo_is_empty(&rt2x00dev->txstatus_fifo) ||
> > > +	       rt2800usb_txstatus_timeout(rt2x00dev)) {
> > >  
> > > -	rt2800usb_txdone_nostatus(rt2x00dev);
> > > +		rt2800usb_txdone(rt2x00dev);
> > >  
> > > -	/*
> > > -	 * The hw may delay sending the packet after DMA complete
> > > -	 * if the medium is busy, thus the TX_STA_FIFO entry is
> > > -	 * also delayed -> use a timer to retrieve it.
> > > -	 */
> > > -	if (rt2800usb_txstatus_pending(rt2x00dev))
> > > -		mod_timer(&rt2x00dev->txstatus_timer, jiffies + msecs_to_jiffies(2));
> > > +		rt2800usb_txdone_nostatus(rt2x00dev);
> > > +
> > > +		/*
> > > +		 * The hw may delay sending the packet after DMA complete
> > > +		 * if the medium is busy, thus the TX_STA_FIFO entry is
> > > +		 * also delayed -> use a timer to retrieve it.
> > > +		 */
> > > +		if (rt2800usb_txstatus_pending(rt2x00dev))
> > > +			rt2800usb_async_read_tx_status(rt2x00dev);
> > 
> > How is it possible that this call will ever start the timer? The
> > reading "thread" won't exit if rt2800usb_txstatus_pending returns true
> > and every dma_done will schedule reading itself.
> 
> I do not understand your objection here too. If rt2800usb_txstatus_pending()
> will return true and if TX_STATUS_READING bit is not set, we will run hrtimer
> to read status after 500 micro seconds. We exit the loop if kfifo is empty
> and no entry timed out waiting to get corresponding TX status.

Yes, I don't mean that this code is wrong. I just think that
rt2800usb_async_read_tx_status have no chance of actually going past
TX_STATUS_READING check. If every dma_done schedules reading and
reading stops only when all pending entries have their statuses then
call to rt2800usb_async_read_tx_status after we processed statuses is
excessive.

All that said, I haven't tested this hypothesis and may be completely
wrong (again). Also I _don't_ mean that this call should be removed,
just wanted to me sure I understand everything correctly ;-)

  -- Kuba