Comment 34 for bug 659143

@Wolfgang:

> The usb drivers use REGISTER_TIMEOUT32(entry->skb->len), evidently
> basing the timeout value on the length of the data written. Based on
> that it would now be REGISTER_TIMEOUT32(entry->skb->len + padding_len).

Yes, I overlooked that, thanks for pointing it out.

> However, that time seems to be rather long already, my beacon e.g. would
> have a timeout of 11500ms. padding_len would only add a maximum of
> 375ms. I tried to find some indication of how long that timeout really
> has to be. It feels like people are using various incarnations of
> "plenty".

That's a long time, but an extra 375ms is insignificant. I guess it
would probably be better to include the extra bytes in the timeout
calculation (if we continue to carry the usb changes, more about that
below).

> It would have been possible to just backport the beacon padding for the
> two pci drivers, since only they use the changed register_multiwrite()
> from the second patch in 2.6.35, so only they need it as preparation for
> that patch. But the usb beacons should be padded anyway, its nice to
> have.

Hmm, that appears to be true. For some reason I thought I had identified
a connection, but looking now that doesn't seem to make sense at all. I
was probably mixing up what I saw in the upstream code with what I saw
in the maverick tree. I'm probably going to strip the usb changes out
then, since this was only reported against pci and as such those changes
are unrelated.

> Regarding the missing return value check. Good question. No, not sure
> that it will always be successful. Normal beacons should always have
> enough tailroom so that skb_pad() does not even have to ask for more
> memory, but we are not sure that this will always be the case. If an
> extra long beacon should need to be padded in an out of memory situation
> skb_pad() might fail, not getting the memory. I tried but could not
> provoke that. Seems linux does like to give skb_pad() what it asks for
> even while oom-killing. If it does happen however skb_pad() would free
> the skb and the call to free it again with dev_kfree_skb_any() produces
> a hard panic that will leave the user without anything useful to report
> (unless he already lay in wait before the console with an HD video
> camera). That, plus seeing that this is checked everywhere else gives me
> second thoughts, so I am considering proposing a patch upstream (for
> linux-next). I'd appreciate your thoughts on this issue.

I'd certainly prefer to see the checks there. I'll add a patch to
introduce the checks when I refresh the series with the other changes.

> I successfully tried your test kernel with rt2800pci. It now loads the
> firmware (and also works after that in station mode). Testing against
> regressions introduced by the padding patch is trickier as it involves
> four different drivers and this code does only get used in ad hoc mode.

Thanks for the review and testing! Agree about the regression testing
being dificult, it frequently is :)

I'll post new patches and test builds as soon as I can get to them,
hopefully later today.