Hi Gertjan
 
On Wed, Aug 22, 2012 at 10:41:42PM +0200, Gertjan van Wingerde wrote:

quoted

IIRC this is basically what I proposed, except without setting
rxdesc->size, unlikely() and rx_pkt_len == 0 check. It will work as
rxdesc->size will be 0. But I think it would be better, if WARNING on
rt2x00lib_rxdone() will print actual corrupted size instead of 0.
Having unlikely is good too - this must be unlikely situation.

OK, I agree with the use of unlikely(). An rx_pkt_len of 0 doesn't seem to happen in practice, so testing for it seems superfluous, but may be providing some extra safety. Don't really care about that. I really don't think we should set rxdesc->size, as we cannot determine it. If we want to print the value, then do it in this function. Adding a hack to having it printed somewhere else doesn't seem right to me.
Also, it should be an error log message, not a WARNING. There's no need to have a stack trace as the buffer is filled by HW, not by some other function. So, the use of WARNING, with its stack dumping functionality is overkill.

quoted

BTW: would be good to fix reason of that corruption if possible
(as long this is not a H/W or F/W bug). But for now, let just
stop kernel crashing. Printing WARNING on this situation will
help to identify there is something wrong if someone will observe
performance problems or similar.

I think this is a HW issue. As mentioned above, I believe a WARNING here is overkill, as we don't need the stack trace.

I was talking about this WARNING in rt2x00lib_rxdone() (which is not
WARN_ON() or WARN() - so no stactrace):

	/*
	 * Check for valid size in case we get corrupted descriptor from
	 * hardware.
	 */
	if (unlikely(rxdesc.size == 0 ||
		     rxdesc.size > entry->queue->data_size)) {
		WARNING(rt2x00dev, "Wrong frame size %d max %d.\n",
			rxdesc.size, entry->queue->data_size);
		dev_kfree_skb(entry->skb);
		goto renew_skb;
	}


I agree that using ERROR is better.
 

Anyway, Sergei, would you be able to modify the patch along the lines of the discussion?

I hope so :-)

Thanks
Stanislaw