Re: [PATCH v2 08/14] rt2x00: rt2800mmio: add a workaround for spurious TX_FIFO_STATUS interrupts

[Stanislaw Gruszka <sgruszka@redhat.com>]

On Wed, Jan 18, 2017 at 03:44:46PM +0100, Stanislaw Gruszka wrote:
> On Mon, Jan 16, 2017 at 04:05:47AM +0100, Daniel Golle wrote:
> >  irqreturn_t rt2800mmio_interrupt(int irq, void *dev_instance)
> >  {
> >  	struct rt2x00_dev *rt2x00dev = dev_instance;
> >  	u32 reg, mask;
> > +	u32 txstatus = 0;
> >  
> > -	/* Read status and ACK all interrupts */
> > +	/* Read status */
> >  	rt2x00mmio_register_read(rt2x00dev, INT_SOURCE_CSR, &reg);
> > +
> > +	if (rt2x00_get_field32(reg, INT_SOURCE_CSR_TX_FIFO_STATUS)) {
> > +		/* Due to unknown reason the hardware generates a
> > +		 * TX_FIFO_STATUS interrupt before the TX_STA_FIFO
> > +		 * register contain valid data. Read the TX status
> > +		 * here to see if we have to process the actual
> > +		 * request.
> > +		 */
> > +		rt2x00mmio_register_read(rt2x00dev, TX_STA_FIFO, &txstatus);
> > +		if (rt2800mmio_txstatus_is_spurious(rt2x00dev, txstatus)) {
> > +			/* Remove the TX_FIFO_STATUS bit so it won't be
> > +			 * processed in this turn. The hardware will
> > +			 * generate another IRQ for us.
> > +			 */
> > +			rt2x00_set_field32(&reg,
> > +					   INT_SOURCE_CSR_TX_FIFO_STATUS, 0);
> > +		}
> > +	}
> > +
> > +	/* ACK interrupts */
> >  	rt2x00mmio_register_write(rt2x00dev, INT_SOURCE_CSR, reg);
> 
> I think spurious TX_STA_FIFO problem happen because we first ACK
> interrupt and then read in bulk all statuses from TX_STA_FIFO
> register, while hardware generate new interrupt (as previous
> was ACKed), then in new interrupt we have no more statues to
> read.
> 
> This is inherently racy situation and first ACK interrupt and
> then read statuses is safer than reverse order which make risk we
> will have pending status and not get interrupt about that.
> 
> Hence I think we should not apply this patch.

Actually patch is safe in regard that we first ACK interrupt
and then read all statuses TX_STA_FIFO. We only do not ACK 
interrupt if we do not have valid status in TX_STA_FIFO .

However I don't really see point of the patch, we should get
next interrupt when new status will be places in TX_STA_FIFO
regardless we ACK this interrupt or don't and if we don't 
ACK we possibly can get series of spurious interrupts.

Stanislaw