Hi Gertjan
On Wed, Aug 22, 2012 at 10:41:42PM +0200, Gertjan van Wingerde wrote:
> > 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.
>
>
> >
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
