* [PATCH] Fix rtl8187 multicast reception @ 2017-02-19 1:35 Nils Holland 2017-02-19 3:26 ` Larry Finger 0 siblings, 1 reply; 9+ messages in thread From: Nils Holland @ 2017-02-19 1:35 UTC (permalink / raw) To: linux-wireless The rtl8187 doesn't seem to receive multicast data, which, among other thinks, make it fail to receive RAs in IPv6 networks. The cause seems to be that the RTL818X_RX_CONF_MULTICAST flag doesn't have any effect at all. Fix this issue by setting RTL818X_RX_CONF_MONITOR instead, which puts the card into monitor mode, and fixes the problem. Signed-off-by: Nils Holland <nholland@tisys.org> --- The problem and solution have been tested on an rtl8187b (0bda:8197), but the fix changes behavior on other cards supported by the driver as well (like non-b 8187's). Due to lack of hardware, I unfortunately cannot say if the issue exists on these cards in the first place, or if the fix has any unwanted consequences there. If people consider it a bad idea to just always put the card into monitor mode (for example, for performance reasons), I could imagine rewriting this patch so that a module parameter controls this behavior instead. drivers/net/wireless/realtek/rtl818x/rtl8187/dev.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/drivers/net/wireless/realtek/rtl818x/rtl8187/dev.c b/drivers/net/wireless/realtek/rtl818x/rtl8187/dev.c index 231f84db9ab0..56a8686cd367 100644 --- a/drivers/net/wireless/realtek/rtl818x/rtl8187/dev.c +++ b/drivers/net/wireless/realtek/rtl818x/rtl8187/dev.c @@ -946,8 +946,7 @@ static int rtl8187_start(struct ieee80211_hw *dev) (7 << 13 /* RX FIFO threshold NONE */) | (7 << 10 /* MAX RX DMA */) | RTL818X_RX_CONF_RX_AUTORESETPHY | - RTL818X_RX_CONF_ONLYERLPKT | - RTL818X_RX_CONF_MULTICAST; + RTL818X_RX_CONF_ONLYERLPKT; priv->rx_conf = reg; rtl818x_iowrite32(priv, &priv->map->RX_CONF, reg); @@ -1319,12 +1318,11 @@ static void rtl8187_configure_filter(struct ieee80211_hw *dev, priv->rx_conf ^= RTL818X_RX_CONF_FCS; if (changed_flags & FIF_CONTROL) priv->rx_conf ^= RTL818X_RX_CONF_CTRL; - if (changed_flags & FIF_OTHER_BSS) - priv->rx_conf ^= RTL818X_RX_CONF_MONITOR; - if (*total_flags & FIF_ALLMULTI || multicast > 0) - priv->rx_conf |= RTL818X_RX_CONF_MULTICAST; + if (*total_flags & FIF_OTHER_BSS || + *total_flags & FIF_ALLMULTI || multicast > 0) + priv->rx_conf |= RTL818X_RX_CONF_MONITOR; else - priv->rx_conf &= ~RTL818X_RX_CONF_MULTICAST; + priv->rx_conf &= ~RTL818X_RX_CONF_MONITOR; *total_flags = 0; @@ -1332,10 +1330,10 @@ static void rtl8187_configure_filter(struct ieee80211_hw *dev, *total_flags |= FIF_FCSFAIL; if (priv->rx_conf & RTL818X_RX_CONF_CTRL) *total_flags |= FIF_CONTROL; - if (priv->rx_conf & RTL818X_RX_CONF_MONITOR) + if (priv->rx_conf & RTL818X_RX_CONF_MONITOR) { *total_flags |= FIF_OTHER_BSS; - if (priv->rx_conf & RTL818X_RX_CONF_MULTICAST) *total_flags |= FIF_ALLMULTI; + } rtl818x_iowrite32_async(priv, &priv->map->RX_CONF, priv->rx_conf); } -- 2.11.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix rtl8187 multicast reception 2017-02-19 1:35 [PATCH] Fix rtl8187 multicast reception Nils Holland @ 2017-02-19 3:26 ` Larry Finger 2017-02-19 7:46 ` Kalle Valo 0 siblings, 1 reply; 9+ messages in thread From: Larry Finger @ 2017-02-19 3:26 UTC (permalink / raw) To: Nils Holland, linux-wireless On 02/18/2017 07:35 PM, Nils Holland wrote: > The rtl8187 doesn't seem to receive multicast data, which, among other > thinks, make it fail to receive RAs in IPv6 networks. > > The cause seems to be that the RTL818X_RX_CONF_MULTICAST flag doesn't > have any effect at all. Fix this issue by setting > RTL818X_RX_CONF_MONITOR instead, which puts the card into monitor mode, > and fixes the problem. > > Signed-off-by: Nils Holland <nholland@tisys.org> > --- > The problem and solution have been tested on an rtl8187b (0bda:8197), but > the fix changes behavior on other cards supported by the driver as well > (like non-b 8187's). Due to lack of hardware, I unfortunately cannot say > if the issue exists on these cards in the first place, or if the fix has > any unwanted consequences there. > > If people consider it a bad idea to just always put the card into monitor > mode (for example, for performance reasons), I could imagine rewriting this > patch so that a module parameter controls this behavior instead. I would hate to make such a change in the behavior of the driver, and have it be applied without the user having any say. The fact that setting RTL818X_RX_CONF_MULTICAST does not have the desired effect may be due to a firmware error; however, there is no chance of making a change there as these devices have embedded/fixed fw. 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. Larry > > drivers/net/wireless/realtek/rtl818x/rtl8187/dev.c | 16 +++++++--------- > 1 file changed, 7 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtl818x/rtl8187/dev.c b/drivers/net/wireless/realtek/rtl818x/rtl8187/dev.c > index 231f84db9ab0..56a8686cd367 100644 > --- a/drivers/net/wireless/realtek/rtl818x/rtl8187/dev.c > +++ b/drivers/net/wireless/realtek/rtl818x/rtl8187/dev.c > @@ -946,8 +946,7 @@ static int rtl8187_start(struct ieee80211_hw *dev) > (7 << 13 /* RX FIFO threshold NONE */) | > (7 << 10 /* MAX RX DMA */) | > RTL818X_RX_CONF_RX_AUTORESETPHY | > - RTL818X_RX_CONF_ONLYERLPKT | > - RTL818X_RX_CONF_MULTICAST; > + RTL818X_RX_CONF_ONLYERLPKT; > priv->rx_conf = reg; > rtl818x_iowrite32(priv, &priv->map->RX_CONF, reg); > > @@ -1319,12 +1318,11 @@ static void rtl8187_configure_filter(struct ieee80211_hw *dev, > priv->rx_conf ^= RTL818X_RX_CONF_FCS; > if (changed_flags & FIF_CONTROL) > priv->rx_conf ^= RTL818X_RX_CONF_CTRL; > - if (changed_flags & FIF_OTHER_BSS) > - priv->rx_conf ^= RTL818X_RX_CONF_MONITOR; > - if (*total_flags & FIF_ALLMULTI || multicast > 0) > - priv->rx_conf |= RTL818X_RX_CONF_MULTICAST; > + if (*total_flags & FIF_OTHER_BSS || > + *total_flags & FIF_ALLMULTI || multicast > 0) > + priv->rx_conf |= RTL818X_RX_CONF_MONITOR; > else > - priv->rx_conf &= ~RTL818X_RX_CONF_MULTICAST; > + priv->rx_conf &= ~RTL818X_RX_CONF_MONITOR; > > *total_flags = 0; > > @@ -1332,10 +1330,10 @@ static void rtl8187_configure_filter(struct ieee80211_hw *dev, > *total_flags |= FIF_FCSFAIL; > if (priv->rx_conf & RTL818X_RX_CONF_CTRL) > *total_flags |= FIF_CONTROL; > - if (priv->rx_conf & RTL818X_RX_CONF_MONITOR) > + if (priv->rx_conf & RTL818X_RX_CONF_MONITOR) { > *total_flags |= FIF_OTHER_BSS; > - if (priv->rx_conf & RTL818X_RX_CONF_MULTICAST) > *total_flags |= FIF_ALLMULTI; > + } > > rtl818x_iowrite32_async(priv, &priv->map->RX_CONF, priv->rx_conf); > } > -- If I was stranded on an island and the only way to get off the island was to make a pretty UI, I’d die there. Linus Torvalds ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix rtl8187 multicast reception 2017-02-19 3:26 ` Larry Finger @ 2017-02-19 7:46 ` Kalle Valo 2017-02-19 9:41 ` Nils Holland 0 siblings, 1 reply; 9+ messages in thread From: Kalle Valo @ 2017-02-19 7:46 UTC (permalink / raw) To: Larry Finger; +Cc: Nils Holland, linux-wireless Larry Finger <Larry.Finger@lwfinger.net> writes: > On 02/18/2017 07:35 PM, Nils Holland wrote: >> The rtl8187 doesn't seem to receive multicast data, which, among other >> thinks, make it fail to receive RAs in IPv6 networks. >> >> The cause seems to be that the RTL818X_RX_CONF_MULTICAST flag doesn't >> have any effect at all. Fix this issue by setting >> RTL818X_RX_CONF_MONITOR instead, which puts the card into monitor mode, >> and fixes the problem. >> >> Signed-off-by: Nils Holland <nholland@tisys.org> >> --- >> The problem and solution have been tested on an rtl8187b (0bda:8197), but >> the fix changes behavior on other cards supported by the driver as well >> (like non-b 8187's). Due to lack of hardware, I unfortunately cannot say >> if the issue exists on these cards in the first place, or if the fix has >> any unwanted consequences there. BTW, this is good info to have in the actual commit log. No need put it under "---" line. >> If people consider it a bad idea to just always put the card into monitor >> mode (for example, for performance reasons), I could imagine rewriting this >> patch so that a module parameter controls this behavior instead. > > I would hate to make such a change in the behavior of the driver, and > have it be applied without the user having any say. The fact that > setting RTL818X_RX_CONF_MULTICAST does not have the desired effect may > be due to a firmware error; however, there is no chance of making a > change there as these devices have embedded/fixed fw. Or it could be also a problem how we configure the firmware. > 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. 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? -- Kalle Valo ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix rtl8187 multicast reception 2017-02-19 7:46 ` Kalle Valo @ 2017-02-19 9:41 ` Nils Holland 2017-02-19 13:29 ` Kalle Valo 0 siblings, 1 reply; 9+ messages in thread From: Nils Holland @ 2017-02-19 9:41 UTC (permalink / raw) To: Kalle Valo; +Cc: Larry Finger, linux-wireless 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: > >> The rtl8187 doesn't seem to receive multicast data, which, among other > >> thinks, make it fail to receive RAs in IPv6 networks. > >> > >> The cause seems to be that the RTL818X_RX_CONF_MULTICAST flag doesn't > >> have any effect at all. Fix this issue by setting > >> RTL818X_RX_CONF_MONITOR instead, which puts the card into monitor mode, > >> and fixes the problem. > >> > >> Signed-off-by: Nils Holland <nholland@tisys.org> > >> --- > >> The problem and solution have been tested on an rtl8187b (0bda:8197), but > >> the fix changes behavior on other cards supported by the driver as well > >> (like non-b 8187's). Due to lack of hardware, I unfortunately cannot say > >> if the issue exists on these cards in the first place, or if the fix has > >> any unwanted consequences there. > > BTW, this is good info to have in the actual commit log. No need put it > under "---" line. Thanks for the hint, I'll do that better the next time! :-) > > 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. 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. 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! > 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. 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. Greetings Nils ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix rtl8187 multicast reception 2017-02-19 9:41 ` Nils Holland @ 2017-02-19 13:29 ` Kalle Valo 2017-02-19 16:25 ` Nils Holland 2017-02-19 18:11 ` Larry Finger 0 siblings, 2 replies; 9+ messages in thread From: Kalle Valo @ 2017-02-19 13:29 UTC (permalink / raw) To: Nils Holland; +Cc: Larry Finger, linux-wireless 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. -- Kalle Valo ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix rtl8187 multicast reception 2017-02-19 13:29 ` Kalle Valo @ 2017-02-19 16:25 ` Nils Holland 2017-02-19 18:11 ` Larry Finger 1 sibling, 0 replies; 9+ messages in thread From: Nils Holland @ 2017-02-19 16:25 UTC (permalink / raw) To: Kalle Valo; +Cc: Larry Finger, linux-wireless On Sun, Feb 19, 2017 at 03:29:12PM +0200, 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 :) Right, that's certainly true as well, and would be a problem, so too much should be avoided. > > 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 Another helpful hint! Thanks, will keep that in mind for the next submission! :-) > >> 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? The driver in question seems to apply to, all in all, 21 different usb ids, so 21 different USB wifi cards. The existing code classifies these into two different categories: rtl8187 and rtl8187b, which seem to be different versions / revisions of a realtek usb wifi chip. Depending on this classification - if a card is a rtl8187 or rtl8187b - the existing code does things a bit different in several cases, for example during initialization of the card (however, not during the routine that sets the filters and where I've made my changes). The card I have here and on which I noticed the problem and verified the fix is one that the driver classifies as rtl8187b. All such classified cards should use the same chip and thus likely behave the same regarding the problem and fix. But as far as those cards classified as rtl8187 are concerned, I don't have an idea how they behave in practice. All I know is that my current patch would change behavior on them as well. If this sounds good to Larry, too, I might in fact - instead of using a module parameter - change the patch in such a way that it only changes behavior on cards classified as rtl8187b, which should behave like mine and thus benefit from the change, and leave the code affecting rtl8187 cards, which I honestly cannot say anything about, alone. Probably sounds like a safer thing to do. Greetings Nils ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix rtl8187 multicast reception 2017-02-19 13:29 ` Kalle Valo 2017-02-19 16:25 ` Nils Holland @ 2017-02-19 18:11 ` Larry Finger 2017-02-19 18:53 ` Nils Holland 1 sibling, 1 reply; 9+ messages in thread From: Larry Finger @ 2017-02-19 18:11 UTC (permalink / raw) To: Kalle Valo, Nils Holland; +Cc: linux-wireless 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix rtl8187 multicast reception 2017-02-19 18:11 ` Larry Finger @ 2017-02-19 18:53 ` Nils Holland 2017-02-19 21:23 ` Larry Finger 0 siblings, 1 reply; 9+ messages in thread From: Nils Holland @ 2017-02-19 18:53 UTC (permalink / raw) To: Larry Finger; +Cc: Kalle Valo, linux-wireless On Sun, Feb 19, 2017 at 12:11:50PM -0600, Larry Finger wrote: > 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. Sounds good, so I guess I'll wait for your report regarding behavior on your 8187l. Then, I'll either re-submit the patch as a v2 with a reworked commit message and proper tag in the title, or, if the fix as-is causes problems on the 8187l, re-submit the patch with limiting the scope of its change to the 8187b only. Sounds good? :-) Greetings Nils ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix rtl8187 multicast reception 2017-02-19 18:53 ` Nils Holland @ 2017-02-19 21:23 ` Larry Finger 0 siblings, 0 replies; 9+ messages in thread From: Larry Finger @ 2017-02-19 21:23 UTC (permalink / raw) To: Nils Holland; +Cc: Kalle Valo, linux-wireless On 02/19/2017 12:53 PM, Nils Holland wrote: > On Sun, Feb 19, 2017 at 12:11:50PM -0600, Larry Finger wrote: >> 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. > > Sounds good, so I guess I'll wait for your report regarding behavior > on your 8187l. Then, I'll either re-submit the patch as a v2 with a > reworked commit message and proper tag in the title, or, if the fix > as-is causes problems on the 8187l, re-submit the patch with limiting > the scope of its change to the 8187b only. Sounds good? :-) The 8187L is working as expected. The only downside of your patch is that the NIC is put into monitor mode silently, which is probably a security hole. As Kalle is opposed to a module parameter, then I guess that is an acceptable risk. Go ahead with V2 with the revised tag and commit message. Larry ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-02-19 21:23 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-19 1:35 [PATCH] Fix rtl8187 multicast reception Nils Holland 2017-02-19 3:26 ` Larry Finger 2017-02-19 7:46 ` Kalle Valo 2017-02-19 9:41 ` Nils Holland 2017-02-19 13:29 ` Kalle Valo 2017-02-19 16:25 ` Nils Holland 2017-02-19 18:11 ` Larry Finger 2017-02-19 18:53 ` Nils Holland 2017-02-19 21:23 ` Larry Finger
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.