Re: [PATCH] Fix rtl8187 multicast reception

[Larry Finger <Larry.Finger@lwfinger.net>]

On 02/19/2017 07:29 AM, Kalle Valo wrote:
> Nils Holland <nholland@tisys.org> writes:
>
>> On Sun, Feb 19, 2017 at 09:46:16AM +0200, Kalle Valo wrote:
>>> Larry Finger <Larry.Finger@lwfinger.net> writes:
>>>
>>>> On 02/18/2017 07:35 PM, Nils Holland wrote:
>>>>
>>>> I would prefer a module parameter that would allow this change to be
>>>> implemented only if the user takes special action. I suspect that you
>>>> will have no difficulty preparing such a change. If that is not true,
>>>> I would be happy to help.
>>>
>>> I understand why you prefer having a module parameter but the thing is
>>> that being able to receive multicast frames is really basic
>>> functionality. We should not hide it under a module parameter.
>>
>> Well, this is a tough question, I have to admit that I have a somewhat
>> weird feeling making a change that also applies to other hardware that
>> I cannot test on, so I don't know about any negative consequences that
>> might arise there, especially when the change isn't based on some
>> official information from some documentation, but rather a result of
>> trial & error. So I can fully understand Larry's concerns and do in
>> fact think that a module parameter could be a nice solution, so that
>> by default the behavior if the driver does not change.
>
> There are lots of hardware variations that cannot be tested when a patch
> is commited. If we followed the same methdology with all patches we
> would have thousands of module parameters in kernel in no time :)
>
>> From an end-user standpoint, it's probably always worse to see
>> something break which has always worked before than it is to have
>> something not work properly right from the start, but being able to
>> easily find some parameter to fix this.
>
> Sure. But if there's a report about this patch breaking something, it's
> easy to revert it.
>
>> On the other hand, use of this particular USB wifi card is probably
>> not so common these days so relatively few people would notice at all.
>> Tough! I guess I'll submit a module parameter based v2 of the patch
>> later today, just as Larry suggested!
>
> Also remember to add a prefix to the title:
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#subject
>
>>> Isn't there any other option, for example does anyone have hw to test
>>> this with other hw? (what exactly?) Or maybe we just take the risk and
>>> take it as is and revert if problems arise?
>>
>> Of course, if someone has other hw, more testing would be nice! Any
>> situation where the card is supposed to receive multicast frames
>> should be suitable as a test case - in my case, just connecting to my
>> local network and expecting to see the card pick up RAs and acquire an
>> IPv6 address is the easiest test case. This works nicely on several
>> other machines with completely different wifi hardware as well as
>> wired machines, but fails without the fix on the rtl8187b. It would,
>> for example, be interesting to see if a non-b 8187 behaves the same or
>> if things work there out of the box.
>
> I'm not familiar with the driver so when you say "non-b 8187" what do
> you mean exactly? What kind of hardware are we talking about? How many
> different hardware versions are there that this patch affects? Is the
> firmware/hardware really so different that the chances are high that
> this breaks something?
>
>> In that case, instead of doing a module parameter, I could also change
>> the fix so that it only does something different on the b-variants of
>> the card and doesn't change behavior at all on non-b.
>
> Now that's much better option than adding a module parameter.

There are three variants of the chip, the RTL8187, RTL8187L, and RTL8187B. 
Programming for the first two are the same. The RTL8187B has a number of newer 
features such as priority queues and a more complete set of parameters in the 
descriptors.

OK, for the moment I agree to this patch with a respin of the commit message. I 
will test with my 8187L device. If those fail, I will request that it be limited 
to the 8187B.

If we later get complaints, then we can decide to revert the patch, or add a 
module parameter to selectively disable it.

Larry