Re: rtl8821ae.
Hi [ CC'ing the relevant parties ] On Sunday 02 February 2014, Dave Jones wrote: > On Sun, Feb 02, 2014 at 03:41:27AM -0800, scan-ad...@coverity.com wrote: > > > > Please find the latest report on new defect(s) introduced to Linux found > with Coverity Scan. > > > > Defect(s) Reported-by: Coverity Scan > > Showing 20 of 83 defect(s) > > Ugh, this is even worse than the usual realtek drivers. (With the exception > of rtl8188eu) > All 83 of those new defects came from this new driver, and while there's > a bunch of "who cares" type things in there, there's a load of stuff that > needs fixing a lot more urgently than CodingStyle issues or anything else in > the TODO > for that driver. > > A bigger problem though, is what is the plan for these realtek drivers ? > They've been in staging forever. rtl8187se has been there for _five_ years > with > no indication it's ever getting promoted to first class status. Actually rtl8187se (aka rtl8185b) seems to have gotten some attention recently: http://lkml.kernel.org/r/can8yu5pgkx9s9dewpfto_ztdr-+wdd5cx2jrv1zd1m1q0bp...@mail.gmail.com > The git logs are littered mostly with CodingStyle cleanups, sparse cleanups > and such, > meanwhile for five years they've had out of bounds reads, overflows, and such > for this whole time. Even worse, when one of the drivers gets fixes for > actual > problems like this, they never make it back to Realtek, who clone the same > old shitty driver they shipped last time, and reintroduce new variants of the > same damn bugs, and then we import the new turd into staging and start all > over again. > > I get the whole "a shit driver is better than no driver", but there's no > discernable > effort to ever improve this pile, just to keep adding to it. > > Dave I think there are mostly two major problems with these drivers, besides RealTek still working on a non-mac80211 codebase for USB based devices. The sheer number of slightly different RealTek drivers for similar chipsets, for which RealTek as forked off a dedicated driver each, rather than extending the existing ones. With the other, probably even larger, problem being that it isn't possible to port wireless drivers from non-mac80111 to mac80211 in a gradual fashion, it's always a parallel re-implementation. Just look at the recent history of staging wireless drivers: the successful ones: - csr --> /dev/null - otus --> ar9170 --> carl9170 - ( rt2870 && rt3070 ) --> rt2800usb - rt2870 --> rt2800pci - [ at76c503a --> ] at76_usb --> at76c50x-usb [*] not in staging the pending ones - rtl8187se [ --> rtl8180 ] [*] hopefully soon - rtl8188eu --> ? - [ rtl8192du --> ? ] [*] not in staging, [1] - rtl8192e --> ? - rtl8192u --> ? - rtl8192su --> rtl8712 --> ? [ r92su[2] would add cfg80211 support, but it being a fullmac like re-implementation doesn't get it anywhere ] - rtl8821ae [ --> mac80211 port planned for 3.15[3]? ] these devices are, besides rtl8187se (802.11g) and rtl8821ae (802.11ac), all 802.11n compatible, but were quickly EOLed by the vendor, probably making it hard to get enough traction for a proper mac80211 port. Coincidentally these chipsets are also very popular, rtl8187se being the chipset of the early netbook craze, rtl8188eu pretty ubiquitous on embedded platforms, the others making the bulk of aftermarket USB devices. ancient hardware, probably not going anywhere: - vt6655 --> ? - vt6656 --> ? - wlags49_h2 --> ? - wlags49_h25 --> ? - wlan-ng --> ? This likely leaves staging wireless drivers to small corrections and bugfixes. In the hope that the devices will get enough traction that someone takes up the effort of doing a parallel re-implementation of a proper mac80211 based driver, using the staging source only as reference. Regards Stefan Lippers-Hollmann [1] https://github.com/lwfinger/rtl8192du [2] https://github.com/chunkeey/rtl8192su/tree/master/r92su [3] http://lkml.kernel.org/r/52e066a1.9010...@lwfinger.net http://lkml.kernel.org/r/52e7d9f6.5070...@lwfinger.net ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: rtl8821ae.
On Sun, 2014-02-02 at 18:07 +, Stefan Lippers-Hollmann wrote: > Hi > > [ CC'ing the relevant parties ] > > On Sunday 02 February 2014, Dave Jones wrote: > > On Sun, Feb 02, 2014 at 03:41:27AM -0800, scan-ad...@coverity.com wrote: > > > > > > Please find the latest report on new defect(s) introduced to Linux found > > with Coverity Scan. > > > > > > Defect(s) Reported-by: Coverity Scan > > > Showing 20 of 83 defect(s) > > > > Ugh, this is even worse than the usual realtek drivers. (With the exception > > of rtl8188eu) > > All 83 of those new defects came from this new driver, and while there's > > a bunch of "who cares" type things in there, there's a load of stuff that > > needs fixing a lot more urgently than CodingStyle issues or anything else > > in the TODO > > for that driver. > > > > A bigger problem though, is what is the plan for these realtek drivers ? > > They've been in staging forever. rtl8187se has been there for _five_ years > > with > > no indication it's ever getting promoted to first class status. > > Actually rtl8187se (aka rtl8185b) seems to have gotten some attention > recently: > > http://lkml.kernel.org/r/can8yu5pgkx9s9dewpfto_ztdr-+wdd5cx2jrv1zd1m1q0bp...@mail.gmail.com > > > The git logs are littered mostly with CodingStyle cleanups, sparse cleanups > > and such, > > meanwhile for five years they've had out of bounds reads, overflows, and > > such > > for this whole time. Even worse, when one of the drivers gets fixes for > > actual > > problems like this, they never make it back to Realtek, who clone the same > > old shitty driver they shipped last time, and reintroduce new variants of > > the > > same damn bugs, and then we import the new turd into staging and start all > > over again. > > > > I get the whole "a shit driver is better than no driver", but there's no > > discernable > > effort to ever improve this pile, just to keep adding to it. > > > > Dave > > I think there are mostly two major problems with these drivers, besides > RealTek still working on a non-mac80211 codebase for USB based devices. > > The sheer number of slightly different RealTek drivers for similar > chipsets, for which RealTek as forked off a dedicated driver each, > rather than extending the existing ones. With the other, probably even > larger, problem being that it isn't possible to port wireless drivers > from non-mac80111 to mac80211 in a gradual fashion, it's always a > parallel re-implementation. Just look at the recent history of staging > wireless drivers: > > the successful ones: > - csr --> /dev/null > - otus --> ar9170 --> carl9170 > - ( rt2870 && rt3070 ) --> rt2800usb > - rt2870 --> rt2800pci > - [ at76c503a --> ] at76_usb --> at76c50x-usb [*] not in staging > > the pending ones > - rtl8187se [ --> rtl8180 ] [*] hopefully soon > - rtl8188eu --> ? > - [ rtl8192du --> ? ] [*] not in staging, [1] > - rtl8192e --> ? > - rtl8192u --> ? > - rtl8192su --> rtl8712 --> ? [ r92su[2] would add cfg80211 support, > but it being a fullmac like > re-implementation doesn't get it > anywhere ] > - rtl8821ae [ --> mac80211 port planned for 3.15[3]? ] > > these devices are, besides rtl8187se (802.11g) and rtl8821ae > (802.11ac), all 802.11n compatible, but were quickly EOLed by the > vendor, probably making it hard to get enough traction for a proper > mac80211 port. Coincidentally these chipsets are also very popular, > rtl8187se being the chipset of the early netbook craze, rtl8188eu > pretty ubiquitous on embedded platforms, the others making the bulk > of aftermarket USB devices. > > ancient hardware, probably not going anywhere: The below devices are still been sold new > - vt6655 --> ? > - vt6656 --> ? to mac80211 I have already done the conversion, just some minor things todo LED/ host implementation Should be ready to merge next + 3-4. I will update the TODO file shortly. > - wlags49_h2 --> ? > - wlags49_h25 --> ? > - wlan-ng --> ? > > This likely leaves staging wireless drivers to small corrections and > bugfixes. In the hope that the devices will get enough traction that > someone takes up the effort of doing a parallel re-implementation of a > proper mac80211 based driver, using the staging source only as > reference. > For my part, it is an educational exercise. However, I do wonder why I don't simply submit a new driver. There is very little of the staging code left. Regards Malcolm ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: rtl8821ae.
On 02/02/2014 12:07 PM, Stefan Lippers-Hollmann wrote: Hi [ CC'ing the relevant parties ] On Sunday 02 February 2014, Dave Jones wrote: On Sun, Feb 02, 2014 at 03:41:27AM -0800, scan-ad...@coverity.com wrote: > > Please find the latest report on new defect(s) introduced to Linux found with Coverity Scan. > > Defect(s) Reported-by: Coverity Scan > Showing 20 of 83 defect(s) Ugh, this is even worse than the usual realtek drivers. (With the exception of rtl8188eu) All 83 of those new defects came from this new driver, and while there's a bunch of "who cares" type things in there, there's a load of stuff that needs fixing a lot more urgently than CodingStyle issues or anything else in the TODO for that driver. A bigger problem though, is what is the plan for these realtek drivers ? They've been in staging forever. rtl8187se has been there for _five_ years with no indication it's ever getting promoted to first class status. Actually rtl8187se (aka rtl8185b) seems to have gotten some attention recently: http://lkml.kernel.org/r/can8yu5pgkx9s9dewpfto_ztdr-+wdd5cx2jrv1zd1m1q0bp...@mail.gmail.com The git logs are littered mostly with CodingStyle cleanups, sparse cleanups and such, meanwhile for five years they've had out of bounds reads, overflows, and such for this whole time. Even worse, when one of the drivers gets fixes for actual problems like this, they never make it back to Realtek, who clone the same old shitty driver they shipped last time, and reintroduce new variants of the same damn bugs, and then we import the new turd into staging and start all over again. I get the whole "a shit driver is better than no driver", but there's no discernable effort to ever improve this pile, just to keep adding to it. Dave I think there are mostly two major problems with these drivers, besides RealTek still working on a non-mac80211 codebase for USB based devices. The sheer number of slightly different RealTek drivers for similar chipsets, for which RealTek as forked off a dedicated driver each, rather than extending the existing ones. With the other, probably even larger, problem being that it isn't possible to port wireless drivers from non-mac80111 to mac80211 in a gradual fashion, it's always a parallel re-implementation. Just look at the recent history of staging wireless drivers: the successful ones: - csr --> /dev/null - otus --> ar9170 --> carl9170 - ( rt2870 && rt3070 ) --> rt2800usb - rt2870 --> rt2800pci - [ at76c503a --> ] at76_usb --> at76c50x-usb [*] not in staging the pending ones - rtl8187se [ --> rtl8180 ] [*] hopefully soon - rtl8188eu --> ? - [ rtl8192du --> ? ] [*] not in staging, [1] - rtl8192e --> ? - rtl8192u --> ? - rtl8192su --> rtl8712 --> ? [ r92su[2] would add cfg80211 support, but it being a fullmac like re-implementation doesn't get it anywhere ] - rtl8821ae [ --> mac80211 port planned for 3.15[3]? ] these devices are, besides rtl8187se (802.11g) and rtl8821ae (802.11ac), all 802.11n compatible, but were quickly EOLed by the vendor, probably making it hard to get enough traction for a proper mac80211 port. Coincidentally these chipsets are also very popular, rtl8187se being the chipset of the early netbook craze, rtl8188eu pretty ubiquitous on embedded platforms, the others making the bulk of aftermarket USB devices. ancient hardware, probably not going anywhere: - vt6655 --> ? - vt6656 --> ? - wlags49_h2 --> ? - wlags49_h25 --> ? - wlan-ng --> ? This likely leaves staging wireless drivers to small corrections and bugfixes. In the hope that the devices will get enough traction that someone takes up the effort of doing a parallel re-implementation of a proper mac80211 based driver, using the staging source only as reference. Regards Stefan Lippers-Hollmann [1] https://github.com/lwfinger/rtl8192du [2] https://github.com/chunkeey/rtl8192su/tree/master/r92su [3] http://lkml.kernel.org/r/52e066a1.9010...@lwfinger.net http://lkml.kernel.org/r/52e7d9f6.5070...@lwfinger.net There are many issues here. I will try to address them without writing something so long that no one reads it. Just to be clear, my work with Realtek drivers is as a volunteer and is done to provide a service to Linux users. The only thing I get from Realtek is advance copies of drivers (sometimes), and sample cards for PCI devices. With one exception, the USB devices in my possession have have been purchased with my own funds. 1. Coverity errors in rtl8821ae: This driver was added to staging with essentially no cleanup for several reasons. The first was as a favor to GregKH, Linus, and roughly 1000 other users who recently got this hardware and want a driver in 3.14. The second reason is that I do not yet have this hardware and did not want to make any changes as all testing would have to be conducted by Greg, and I think he has enou
Re: rtl8821ae.
> >On Sunday 02 February 2014, Dave Jones wrote: > >>The git logs are littered mostly with CodingStyle cleanups, sparse cleanups > >>and such, > >>meanwhile for five years they've had out of bounds reads, overflows, and > >>such > >>for this whole time. Really, any sort of cleanup is good, though... There are bugs I would fix if the code didn't look like puke. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: rtl8821ae.
On Mon, Feb 03, 2014 at 11:05:42AM -0600, Larry Finger wrote: > A combined driver would require very many branches based on chip > number and would certainly execute much more slowly. I seriously doubt there are performance issues with merging the drivers. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: rtl8821ae.
On Mon, Feb 3, 2014 at 12:12 PM, Dan Carpenter wrote: > On Mon, Feb 03, 2014 at 11:05:42AM -0600, Larry Finger wrote: >> A combined driver would require very many branches based on chip >> number and would certainly execute much more slowly. > > I seriously doubt there are performance issues with merging the drivers. Quite frankly, merging drivers can be a f*cking pain. I would seriously suggest avoiding it *unless* the hardware is literally almost identical, or the drivers have been written with multi-chip support from the ground up by people who actually understood the hardware. But the reason isn't performance - it's subtle breakage. Merged drivers have a tendency to break support for chip A when you fix something for chip B. And the end result is a driver that nobody understands, and nobody can sanely test. Sometimes it's simply better to leave old drivers alone. Linus ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: rtl8821ae.
On Mon, Feb 03, 2014 at 11:05:42AM -0600, Larry Finger wrote: > I am trying to improve the quality of this pile of , but I am only one > person who was a life-long Fortran-coding scientist until I retired 15 years > ago. Since then, I have been learning C and kernel-style coding practices. > Certainly there is both a lot to do and a lot to learn. I greatly appreciate all of the work you have done here, with this almost impossible task (no specs, random code drops from external people, limited feedback loop from the company, etc.) I blame the company for the situation here, not you at all, and its great that the PCI division is doing much better now. Hopefully the USB side will come around over time. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: rtl8821ae.
Nice. Thanks for doing that. Do you have some patches we could review of the merged driver? regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: rtl8821ae.
On 02/04/2014 08:41 AM, Andrea Merello wrote: Yes, I attach a cumulative patch for latest version of my work. When I will send this asking for merge in kernel, I will split it in some patches, because I also also included fix and improvements for other already supported cards (not directly related to rtl8187se support). About this, I will produce separate patches for rtl8180 driver if finally the drivers will be merged and also in case I'll split them.. Any suggestion/advice will be appreciated :) Thanks Andrea On Tue, Feb 4, 2014 at 10:28 AM, Dan Carpenter wrote: Nice. Thanks for doing that. Do you have some patches we could review of the merged driver? Andrea, In the case of the RTL8187SE driver, you only have three chips to deal with, two of them are already in the driver, 802.11g devices are rather simple, and it is extremely unlikely that any new devices will be added. For those reasons, I think it is appropriate to make a combined driver. As there has been quite a bit of testing on the current version, my vote is to stay with it. I will test the latest revision soon. I'm not sure my laptop will hibernate, but I will test that operation if I can. Larry ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: rtl8821ae.
Andrea, After applying the latest patch, I get the following warning and errors when building the USB driver rtl8187: CC drivers/video/fbmon.o drivers/net/wireless/rtl818x/rtl8187/dev.c: In function ‘rtl8187_set_anaparam’: drivers/net/wireless/rtl818x/rtl8187/dev.c:595:3: warning: passing argument 2 of ‘rtl818x_iowrite8’ from incompatible pointer type [enabled by default] rtl818x_iowrite8(priv, &priv->map->ANAPARAM3, anaparam3); ^ In file included from drivers/net/wireless/rtl818x/rtl8187/dev.c:31:0: drivers/net/wireless/rtl818x/rtl8187/rtl8187.h:237:20: note: expected ‘u8 *’ but argument is of type ‘__le16 *’ static inline void rtl818x_iowrite8(struct rtl8187_priv *priv, u8 *addr, u8 val) ^ drivers/net/wireless/rtl818x/rtl8187/dev.c: In function ‘rtl8187b_init_hw’: drivers/net/wireless/rtl818x/rtl8187/dev.c:788:9: error: ‘RTL818X_CW_CONF_PERPACKET_RETRY_SHIFT’ undeclared (first use in this function) reg |= RTL818X_CW_CONF_PERPACKET_RETRY_SHIFT; ^ drivers/net/wireless/rtl818x/rtl8187/dev.c:788:9: note: each undeclared identifier is reported only once for each function it appears in drivers/net/wireless/rtl818x/rtl8187/dev.c: In function ‘rtl8187_start’: drivers/net/wireless/rtl818x/rtl8187/dev.c:946:11: error: ‘RTL818X_TX_AGC_CTL_PERPACKET_GAIN_SHIFT’ undeclared (first use in this function) reg &= ~RTL818X_TX_AGC_CTL_PERPACKET_GAIN_SHIFT; ^ drivers/net/wireless/rtl818x/rtl8187/dev.c:947:11: error: ‘RTL818X_TX_AGC_CTL_PERPACKET_ANTSEL_SHIFT’ undeclared (first use in this function) reg &= ~RTL818X_TX_AGC_CTL_PERPACKET_ANTSEL_SHIFT; ^ drivers/net/wireless/rtl818x/rtl8187/dev.c:952:7: error: ‘RTL818X_TX_CONF_HW_SEQNUM’ undeclared (first use in this function) RTL818X_TX_CONF_HW_SEQNUM | ^ drivers/net/wireless/rtl818x/rtl8187/dev.c:989:10: error: ‘RTL818X_CW_CONF_PERPACKET_CW_SHIFT’ undeclared (first use in this function) reg &= ~RTL818X_CW_CONF_PERPACKET_CW_SHIFT; ^ drivers/net/wireless/rtl818x/rtl8187/dev.c:990:9: error: ‘RTL818X_CW_CONF_PERPACKET_RETRY_SHIFT’ undeclared (first use in this function) reg |= RTL818X_CW_CONF_PERPACKET_RETRY_SHIFT; The warning is fixed by Index: wireless-testing/drivers/net/wireless/rtl818x/rtl8187/dev.c === --- wireless-testing.orig/drivers/net/wireless/rtl818x/rtl8187/dev.c +++ wireless-testing/drivers/net/wireless/rtl818x/rtl8187/dev.c @@ -593,7 +593,7 @@ static void rtl8187_set_anaparam(struct rtl818x_iowrite32(priv, &priv->map->ANAPARAM, anaparam); rtl818x_iowrite32(priv, &priv->map->ANAPARAM2, anaparam2); if (priv->is_rtl8187b) - rtl818x_iowrite8(priv, &priv->map->ANAPARAM3, anaparam3); + rtl818x_iowrite16(priv, &priv->map->ANAPARAM3, anaparam3); reg &= ~RTL818X_CONFIG3_ANAPARAM_WRITE; rtl818x_iowrite8(priv, &priv->map->CONFIG3, reg); rtl818x_iowrite8(priv, &priv->map->EEPROM_CMD, For testing purposes, I have turned on the build for rtl8187. Larry ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: rtl8821ae.
On Mon, Feb 3, 2014 at 6:22 PM, Andrea Merello wrote: > Right now I have merged rtl8187se support in rtl8180 driver. But I must > admit that I ever been in doubt about whether to produce a separate driver > rather than going on merging. If you've already merged them, and you think it works, keep it that way. I was just saying that merging drivers isn't necessarily always the right thing to do, and can be painful as hell. Linus ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: rtl8821ae.
Sorry, my fault.. you had already reported me the iowrite16 vs iowrite8 for anaparam3 issue, but I forgot to fix it :( If you are reasonably sure that a 16bits-wide write is OK on the rtl8187b hardware (In reference code I have here only 8bits are written), then I agree applying your fix, and I will leave unchanged my modifications. Otherwise, if you are unsure, I will revert thing in the common rtl818x struct (switching back to 8bit type) and I simply make a cast on rtl8187se, or I will use an union if possible, so the rtl8187b driver remains unchanged. On the other hand, for the remaining issues with changed names for bitfield (the _SHIFT), I wil vote to fix the rtl8187 driver: That names IMHO are wrong (misleading) and while fixing them should help avoid future confusion, the fix is trivial enough that I'm really confident not to broke anything. I had already sent a patch for it: http://marc.info/?l=linux-wireless&m=139033180125824&w=2 Whit this mail I intend to try to push it once more. If it could be applied I prefer by far, otherwise, if it is too hassle to handle it for the trivial thing it actually does, then I will change back in rtl8180 driver.. Thank you Andrea On Tue, Feb 4, 2014 at 6:57 PM, Larry Finger wrote: > Andrea, > > After applying the latest patch, I get the following warning and errors when > building the USB driver rtl8187: > > CC drivers/video/fbmon.o > drivers/net/wireless/rtl818x/rtl8187/dev.c: In function > 'rtl8187_set_anaparam': > drivers/net/wireless/rtl818x/rtl8187/dev.c:595:3: warning: passing argument > 2 of 'rtl818x_iowrite8' from incompatible pointer type [enabled by default] >rtl818x_iowrite8(priv, &priv->map->ANAPARAM3, anaparam3); >^ > In file included from drivers/net/wireless/rtl818x/rtl8187/dev.c:31:0: > drivers/net/wireless/rtl818x/rtl8187/rtl8187.h:237:20: note: expected 'u8 *' > but argument is of type '__le16 *' > static inline void rtl818x_iowrite8(struct rtl8187_priv *priv, u8 *addr, u8 > val) > ^ > drivers/net/wireless/rtl818x/rtl8187/dev.c: In function 'rtl8187b_init_hw': > drivers/net/wireless/rtl818x/rtl8187/dev.c:788:9: error: > 'RTL818X_CW_CONF_PERPACKET_RETRY_SHIFT' undeclared (first use in this > function) > reg |= RTL818X_CW_CONF_PERPACKET_RETRY_SHIFT; > ^ > drivers/net/wireless/rtl818x/rtl8187/dev.c:788:9: note: each undeclared > identifier is reported only once for each function it appears in > drivers/net/wireless/rtl818x/rtl8187/dev.c: In function 'rtl8187_start': > drivers/net/wireless/rtl818x/rtl8187/dev.c:946:11: error: > 'RTL818X_TX_AGC_CTL_PERPACKET_GAIN_SHIFT' undeclared (first use in this > function) >reg &= ~RTL818X_TX_AGC_CTL_PERPACKET_GAIN_SHIFT; >^ > drivers/net/wireless/rtl818x/rtl8187/dev.c:947:11: error: > 'RTL818X_TX_AGC_CTL_PERPACKET_ANTSEL_SHIFT' undeclared (first use in this > function) >reg &= ~RTL818X_TX_AGC_CTL_PERPACKET_ANTSEL_SHIFT; >^ > drivers/net/wireless/rtl818x/rtl8187/dev.c:952:7: error: > 'RTL818X_TX_CONF_HW_SEQNUM' undeclared (first use in this function) >RTL818X_TX_CONF_HW_SEQNUM | >^ > drivers/net/wireless/rtl818x/rtl8187/dev.c:989:10: error: > 'RTL818X_CW_CONF_PERPACKET_CW_SHIFT' undeclared (first use in this function) > reg &= ~RTL818X_CW_CONF_PERPACKET_CW_SHIFT; > ^ > drivers/net/wireless/rtl818x/rtl8187/dev.c:990:9: error: > 'RTL818X_CW_CONF_PERPACKET_RETRY_SHIFT' undeclared (first use in this > function) > reg |= RTL818X_CW_CONF_PERPACKET_RETRY_SHIFT; > > The warning is fixed by > > Index: wireless-testing/drivers/net/wireless/rtl818x/rtl8187/dev.c > === > --- wireless-testing.orig/drivers/net/wireless/rtl818x/rtl8187/dev.c > +++ wireless-testing/drivers/net/wireless/rtl818x/rtl8187/dev.c > @@ -593,7 +593,7 @@ static void rtl8187_set_anaparam(struct > rtl818x_iowrite32(priv, &priv->map->ANAPARAM, anaparam); > rtl818x_iowrite32(priv, &priv->map->ANAPARAM2, anaparam2); > if (priv->is_rtl8187b) > - rtl818x_iowrite8(priv, &priv->map->ANAPARAM3, anaparam3); > + rtl818x_iowrite16(priv, &priv->map->ANAPARAM3, anaparam3); > reg &= ~RTL818X_CONFIG3_ANAPARAM_WRITE; > rtl818x_iowrite8(priv, &priv->map->CONFIG3, reg); > rtl818x_iowrite8(priv, &priv->map->EEPROM_CMD, > > For testing purposes, I have turned on the build for rtl8187. > > Larry > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: rtl8821ae.
On 02/04/2014 01:57 PM, Andrea Merello wrote:
Sorry, my fault.. you had already reported me the iowrite16 vs
iowrite8 for anaparam3 issue, but I forgot to fix it :(
If you are reasonably sure that a 16bits-wide write is OK on the
rtl8187b hardware (In reference code I have here only 8bits are
written), then I agree applying your fix, and I will leave unchanged
my modifications.
Otherwise, if you are unsure, I will revert thing in the common
rtl818x struct (switching back to 8bit type) and I simply make a cast
on rtl8187se, or I will use an union if possible, so the rtl8187b
driver remains unchanged.
On the other hand, for the remaining issues with changed names for
bitfield (the _SHIFT), I wil vote to fix the rtl8187 driver:
That names IMHO are wrong (misleading) and while fixing them should
help avoid future confusion, the fix is trivial enough that I'm really
confident not to broke anything.
I had already sent a patch for it:
Somehow I missed this patch. Do you know if John Linville picked it up?
I'm not sure if the ANAPARAM3 register could handle a 16-bit write on an
RTL8187B. To be cautious, I wrote and have tested the attached patch using a
union. The patch includes a fix for an undefined symbol
(RTL818X_TX_CONF_HW_SEQNUM). With this patch my RTL8187SE and RTL8187B devices
both work.
Larry
Index: wireless-testing/drivers/net/wireless/rtl818x/rtl8187/dev.c
===
--- wireless-testing.orig/drivers/net/wireless/rtl818x/rtl8187/dev.c
+++ wireless-testing/drivers/net/wireless/rtl818x/rtl8187/dev.c
@@ -592,7 +592,7 @@ static void rtl8187_set_anaparam(struct
rtl818x_iowrite32(priv, &priv->map->ANAPARAM, anaparam);
rtl818x_iowrite32(priv, &priv->map->ANAPARAM2, anaparam2);
if (priv->is_rtl8187b)
- rtl818x_iowrite8(priv, &priv->map->ANAPARAM3, anaparam3);
+ rtl818x_iowrite8(priv, &priv->map->ANAPARAM3A, anaparam3);
reg &= ~RTL818X_CONFIG3_ANAPARAM_WRITE;
rtl818x_iowrite8(priv, &priv->map->CONFIG3, reg);
rtl818x_iowrite8(priv, &priv->map->EEPROM_CMD,
@@ -949,7 +949,7 @@ static int rtl8187_start(struct ieee8021
rtl818x_iowrite8(priv, &priv->map->TX_AGC_CTL, reg);
rtl818x_iowrite32(priv, &priv->map->TX_CONF,
- RTL818X_TX_CONF_HW_SEQNUM |
+ RTL818X_TX_CONF_SW_SEQNUM |
RTL818X_TX_CONF_DISREQQSIZE |
(RETRY_COUNT << 8 /* short retry limit */) |
(RETRY_COUNT << 0 /* long retry limit */) |
Index: wireless-testing/drivers/net/wireless/rtl818x/rtl818x.h
===
--- wireless-testing.orig/drivers/net/wireless/rtl818x/rtl818x.h
+++ wireless-testing/drivers/net/wireless/rtl818x/rtl818x.h
@@ -309,7 +309,10 @@ struct rtl818x_csr {
__le32 RDSAR; /* 0xe4 */
__le16 TID_AC_MAP; /* 0xe8 */
u8 reserved_20[4];
- __le16 ANAPARAM3; /* 0xee */
+ union {
+ __le16 ANAPARAM3; /* 0xee */
+ u8 ANAPARAM3A; /* for rtl8187 */
+ };
#define AC_PARAM_TXOP_LIMIT_SHIFT 16
#define AC_PARAM_ECW_MAX_SHIFT 12
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: rtl8821ae.
> > Somehow I missed this patch. Do you know if John Linville picked it up? > AFAIK no.. > I'm not sure if the ANAPARAM3 register could handle a 16-bit write on an > RTL8187B. To be cautious, I wrote and have tested the attached patch using a > union. The patch includes a fix for an undefined symbol > (RTL818X_TX_CONF_HW_SEQNUM). With this patch my RTL8187SE and RTL8187B > devices both work. Excellent, thank you ! > Larry > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: rtl8821ae.
> Looks good. It would be best to submit the bugfixes first like where
> you add error checking for pci_map_single().
Yes, this is exactly what I started to do this morning :)
I'm preparing a patchseries where I extracting fixes or improvements
unrelated to rtl8187se, including pci_map_single fixes
> You add too many blank lines btw, you should never have two
> consecutive blank lines. Don't add a blank line in the middle of the
> declaration block or before a close curly brace '}' line.
OK
> After that
> maybe the patch to change "if (priv->r8185)" to
> "if (priv->chip_family == RTL818X_CHIP_FAMILY_RTL8185)". Then the patch
> to add rtl8225se support.
Yes, this is also my plan :)
> Some minor things below.
Ok, I have reviewed all your advices.
I agree with them.
I also commended the ones where you asked something about.
> regards,
> dan carpenter
Thank you a lot for your review!
Andrea
>> +#include "rtl8225se.h"
>> +
>
> Don't include this twice. Only put one blank line, not two consecutive
> blank lines.
OK
> This comment is out of date. RTL8180_NR_TX_QUEUES is 2. I haven't
> really understood the changes to rtl8180_queues_map[]. It used to have
> 4 elements and now it only has 2. Is this change needed to support
> rtl8225se or is it a separate bugfix which should go in a separate
> patch?
It's for rtl8187se.
That card uses more queues to enable WMM
> Also this change which I have pulled out of context:
>
> + if (ring->entries - skb_queue_len(&ring->queue) < 2) {
> + if (prio != dev->queues)
> + ieee80211_stop_queue(dev, prio);
> + else
> + priv->beacon_queue_stop = 1;
> + }
>
> This is a separate bugfix?
Yes, the old driver uses ieee80211_stop_queue and ieee80211_wake_queue
on the beacon queue, that is indeed handled in the driver and not by
ieee80211
This one of the things that I will include in patches for
rtl8180/rtl8185 fixes before merging real rtl8187se code
>> +static void rtl8180_write_phy_(struct ieee80211_hw *dev, u8 addr, u32 data)
>
> This is a temporary name to not conflight with rtl8180_write_phy()?
No, this is the aux function that does the real work, but should not
be called directly in the driver code, as rtl8180_write_phy() should
be called.
Maybe I have to move the underscore at the beginning of the name.
>
> These don't go over the 80 character limit so we don't need the line
> breaks. The casts are not needed either.
OK
>> + if ((addr == 0xa) && (
>> + priv->chip_family == RTL818X_CHIP_FAMILY_RTL8185))
>> + rb_mask = 0x7f;
>
> Write it like this:
>
> if (addr == 0xa && priv->chip_family == RTL818X_CHIP_FAMILY_RTL8185)
> rb_mask = 0x7f;
>
> If you really want to add parenthesis then do this:
>
> if ((addr == 0xa) &&
> (priv->chip_family == RTL818X_CHIP_FAMILY_RTL8185))
> rb_mask = 0x7f;
OK
>> - buf = (data << 8) | addr;
>> + if (tmp & 0x80)
>> + printk(KERN_WARNING
>> +"rtl818x failed to write BB (busy) adr:%x data:%x\n",
>> +addr, data);
>
> Checkpatch.pl will complain that you should use netdev_warn() or
> something.
OK
>
> Don't put a blank line here in the middle of the declaration block.
>
OK
>> + frame_duration = priv->ack_time + le16_to_cpu(
>> + ieee80211_generic_frame_duration(dev, priv->vif,
>> + IEEE80211_BAND_2GHZ, skb->len,
>> + ieee80211_get_tx_rate(dev, info)));
>
> Use a temporary variable:
>
> __le16 duration;
I agree. Will do.
>> + /* must be written last */
>> entry->flags = cpu_to_le32(tx_flags);
>
> Why must this be written last? The CPU can re-order it anyway unless
> there is some locking.
Because that marks the descriptor as good for the NIC to be processed.
I suppose a wmb() is needed before this line to ensure it is the last written
and another wmb() is needed to ensure that the descriptor is fully
written before polling the NIC for performing DMA (I think that,
depending by the situation, the HW may wait for this register write on
not).
>> + /* Enable DA10 TX power saving */
>> + rtl818x_iowrite8(priv, &priv->map->PHY_PR,
>> + rtl818x_ioread8(priv, &priv->map->PHY_PR) | 0x04);
>
> Use a temporary variable:
>
> reg = rtl818x_ioread8(priv, &priv->map->PHY_PR);
> rtl818x_iowrite8(priv, &priv->map->PHY_PR, reg | 0x40);
>
OK
>> +static void rtl8187se_set_antenna_config(struct ieee80211_hw *dev, u8
>> def_ant,
>> + bool use_diver)
>
> Could you rename "use_diver" to "diversity"? I misread it every time.
It makes sense.
>> +#if 0
>> +static void rtl8187se_power_tracking(struct ieee80211_hw *dev)
>> +{
>> + struct rtl8180_priv *priv = dev->priv;
>> + u8 tmp = rtl818x_ioread8(priv
Re: rtl8821ae: patch for mac80211 regulatory changes
On 07/15, Kevin Folz wrote: > Hello, > > > > I noticed you two have worked on the staging rtl8821ae driver. > > > > I wrote to the mailing list last month, but have yet to receive a > response. > > http://permalink.gmane.org/gmane.linux.kernel.wireless.general/125235 > > > > > I downloaded the latest linux kernel, 3.16.0rc2+, and created a patch > (attached) based on changes to rtl8192ee > > mac80211 had a change to regulatory defines, so I ported the changes to > your driver. > > > > rtl8821ae driver now successfully initializes on boot. > > > > I've never submitted to the Linux kernel before, I hope you can apply > this patch. > > > > Thanks, > Kevin Folz > Hello Kevin, first of all thanks for your submission and the changes that you have made. Now, since I'm not the right person to recieve such patches I'm going to use this opportunity and show you how you can do better. You can learn how to send patches by reading Documentation/SendPatches document. This is good place to start because it covers everything that is needed to make a successfull submission. In short you should send your patch inlined in the mail body to the mantainer of the staging subsystem and the 'driver devel' mailing list. Yes, you can include developer such as me that have contributed to the portion of kernel that you are working on however the important thing is to include the mantainers and the mailing list. This is because each patch before it is accepted is reviewed by multiple people and changed according to their comments. This is how the Linux kernel development is taking place. Before you send your patch make sure that your changes are compliant with the coding standard that the Linux kernel developers are following. You can learn it by reading Documentation/CodingStyle document and check for issues in your patches by using scripts/checkpatch.pl script. When your patch is good and contains no coding style issues you can use the scripts/get_mantainer.pl script to list the people and mailing lists that are interested in seeing changes to this code area. The last thing that you should do is to send it. When you work witch patches the easiest thing is to use git for it. Just read about the git send-email command. I know that it is a bit of reading however I assure you that the whole process despite of being precisely documented is not difficult at all. I suggest that you learn the process, send your patch again now in the proper manner and we will take it from there. Good Luck PS: I forward this message to the driver devel mailing list where the other people might help you too. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
