Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Jes Sorensen]

Kalle Valo  writes:
> Jes Sorensen  writes:
>
> I doubt that this will be in active enough development that a separate
> git tree is needed. wireless-drivers trees should be enough and this
> line can be removed.

 I would like to keep this line, it points to my development tree, which
 allows users to go back and look through my full development logs, as
 well as see ongoing work before it's pushed upstream.
>>>
>>> I prefer that wireless-drivers(-next).git is used as the baseline for
>>> wireless driver patches, so please remove this tag. You can document
>>> your git server in the wiki, commit log and so on.
>>
>> I will insist on keeping my branch listed as long as I maintain the
>> driver. Other drivers do the same thing, like iwlwifi. I'm happy to send
>> you patches against wireless-drivers-next.git though.
>
> To keep things simple for me I want that the patches are based on
> wireless-drivers-next.git. iwlwifi is under so active development that
> an extra tree is justified, but having a dedicated tree for a not so
> active driver is just extra work for me which I do not want to have.
>
> With rtlwifi it goes so that Larry acks the patches on the list and
> patchwork automatically adds the ack to the commit log when I apply the
> patch, which is easy and fast for everyone. The same should go with
> rtl8723au.

The rtl8xxxu branch is there because that is where I do the active
development, and work actively on the driver. If people post patches for
it and it's smaller fixes, sure just take them, if they post larger
changes, please punt them to me and I will integrate them and send them
on to you.

Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Kalle Valo]

Jes Sorensen  writes:

 I doubt that this will be in active enough development that a separate
 git tree is needed. wireless-drivers trees should be enough and this
 line can be removed.
>>>
>>> I would like to keep this line, it points to my development tree, which
>>> allows users to go back and look through my full development logs, as
>>> well as see ongoing work before it's pushed upstream.
>>
>> I prefer that wireless-drivers(-next).git is used as the baseline for
>> wireless driver patches, so please remove this tag. You can document
>> your git server in the wiki, commit log and so on.
>
> I will insist on keeping my branch listed as long as I maintain the
> driver. Other drivers do the same thing, like iwlwifi. I'm happy to send
> you patches against wireless-drivers-next.git though.

To keep things simple for me I want that the patches are based on
wireless-drivers-next.git. iwlwifi is under so active development that
an extra tree is justified, but having a dedicated tree for a not so
active driver is just extra work for me which I do not want to have.

With rtlwifi it goes so that Larry acks the patches on the list and
patchwork automatically adds the ack to the commit log when I apply the
patch, which is easy and fast for everyone. The same should go with
rtl8723au.

-- 
Kalle Valo
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Jes Sorensen]

Larry Finger  writes:
> On 09/07/2015 01:43 PM, Jes Sorensen wrote:
>> Kalle Valo  writes:
>>> Hi,
>>>
>>> replying to an old thread first.
>>>
>>> Jes Sorensen  writes:
>>>
 Kalle Valo  writes:
> jes.soren...@redhat.com writes:
>
>>   MAINTAINERS  |8 +
>>   drivers/net/wireless/Kconfig |   19 +
>>   drivers/net/wireless/Makefile|2 +
>>   drivers/net/wireless/rtl8xxxu.c | 4500
>> ++
>>   drivers/net/wireless/rtl8xxxu.h  |  497 
>>   drivers/net/wireless/rtl8xxxu_regs.h |  941 +++
>
> I think someone else already mentioned, but it would be better that has
> it's own directory. Or should this actually be under rtlwifi
> directory?

 I didn't see the need here - it's just 3 files, as long as it doesn't
 have a huge hierachy of files, a new directory doesn't add much
 value. If it becomes an issue later, we can move it into a
 subdirectory.
>>>
>>> It's easier that the driver has it's on directory as everything
>>> (makefile, kconfig etc) is cleanly separated. And actually I would like
>>> to create a new vendor directory for realtek and have this in
>>> drivers/net/wireless/realtek/rtl8xxxu/. And later we could move rtlwifi
>>> under realtek directory as well.
>>
>> If you want to create drivers/net/wireless/realtek, that is fine with
>> me, but maybe you can go ahead and create that first and move the
>> existing drivers in there?
>
> I will submit a patch to do that.

Awesome, once I see that go in, I'll move rtl8xxxu into that directory.

Cheers,
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Kalle Valo]

Larry Finger  writes:

>> It's easier that the driver has it's on directory as everything
>> (makefile, kconfig etc) is cleanly separated. And actually I would like
>> to create a new vendor directory for realtek and have this in
>> drivers/net/wireless/realtek/rtl8xxxu/. And later we could move rtlwifi
>> under realtek directory as well.
>
> I agree with the proposal for this driver to be under a separate
> directory. As this driver evolves to support more devices, it will
> likely be useful to split the current single .c file into a set of
> files, each containing the specific code needed for a given device. At
> that point, placing these files into a separate directory will be
> needed to keep the base drivers/net/wireless/ as clean as possible. We
> might as well do that now. I have no objections to moving rtlwifi
> under drivers/net/wireless/realtek/.

Great.

> We should even move rtl818x as well.

Good point, need to remember that.

-- 
Kalle Valo
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Jes Sorensen]

Kalle Valo  writes:
> Hi,
>
> replying to an old thread first.
>
> Jes Sorensen  writes:
>
>> Kalle Valo  writes:
>>> jes.soren...@redhat.com writes:
>>>
  MAINTAINERS  |8 +
  drivers/net/wireless/Kconfig |   19 +
  drivers/net/wireless/Makefile|2 +
  drivers/net/wireless/rtl8xxxu.c | 4500
 ++
  drivers/net/wireless/rtl8xxxu.h  |  497 
  drivers/net/wireless/rtl8xxxu_regs.h |  941 +++
>>>
>>> I think someone else already mentioned, but it would be better that has
>>> it's own directory. Or should this actually be under rtlwifi
>>> directory?
>>
>> I didn't see the need here - it's just 3 files, as long as it doesn't
>> have a huge hierachy of files, a new directory doesn't add much
>> value. If it becomes an issue later, we can move it into a
>> subdirectory.
>
> It's easier that the driver has it's on directory as everything
> (makefile, kconfig etc) is cleanly separated. And actually I would like
> to create a new vendor directory for realtek and have this in
> drivers/net/wireless/realtek/rtl8xxxu/. And later we could move rtlwifi
> under realtek directory as well.

If you want to create drivers/net/wireless/realtek, that is fine with
me, but maybe you can go ahead and create that first and move the
existing drivers in there?

 +T: git git://git.kernel.org/pub/scm/linux/kernel/git/jes/linux.git
 rtl8723au-mac80211
>>>
>>> I doubt that this will be in active enough development that a separate
>>> git tree is needed. wireless-drivers trees should be enough and this
>>> line can be removed.
>>
>> I would like to keep this line, it points to my development tree, which
>> allows users to go back and look through my full development logs, as
>> well as see ongoing work before it's pushed upstream.
>
> I prefer that wireless-drivers(-next).git is used as the baseline for
> wireless driver patches, so please remove this tag. You can document
> your git server in the wiki, commit log and so on.

I will insist on keeping my branch listed as long as I maintain the
driver. Other drivers do the same thing, like iwlwifi. I'm happy to send
you patches against wireless-drivers-next.git though.

>>> I'll do more detailed code review later, but my first impression was
>>> that there was a lot of #if 0 code which is frowned upon.
>>
>> Johannes already brought up the #if 0 pieces. I left those in because I
>> am not done with the driver, and this helps me map the flow of the
>> vendor driver codebase. Those #if 0 lines are not there to sit and rot,
>> but to help future development.
>
> Yeah, I guessed that. But I suspect once the driver is applied we start
> getting patches removing dead code (and rightly so). You could have a
> separate branch with these #if 0 lines if they are important to you.

Most of this is gone by now, so this part of the discussion is less
relevant by now :)

Cheers,
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Larry Finger]

On 09/07/2015 01:43 PM, Jes Sorensen wrote:

Kalle Valo  writes:

Hi,

replying to an old thread first.

Jes Sorensen  writes:


Kalle Valo  writes:

jes.soren...@redhat.com writes:


  MAINTAINERS  |8 +
  drivers/net/wireless/Kconfig |   19 +
  drivers/net/wireless/Makefile|2 +
  drivers/net/wireless/rtl8xxxu.c | 4500
++
  drivers/net/wireless/rtl8xxxu.h  |  497 
  drivers/net/wireless/rtl8xxxu_regs.h |  941 +++


I think someone else already mentioned, but it would be better that has
it's own directory. Or should this actually be under rtlwifi
directory?


I didn't see the need here - it's just 3 files, as long as it doesn't
have a huge hierachy of files, a new directory doesn't add much
value. If it becomes an issue later, we can move it into a
subdirectory.


It's easier that the driver has it's on directory as everything
(makefile, kconfig etc) is cleanly separated. And actually I would like
to create a new vendor directory for realtek and have this in
drivers/net/wireless/realtek/rtl8xxxu/. And later we could move rtlwifi
under realtek directory as well.


If you want to create drivers/net/wireless/realtek, that is fine with
me, but maybe you can go ahead and create that first and move the
existing drivers in there?


I will submit a patch to do that.

Larry


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Kalle Valo]

Hi,

replying to an old thread first.

Jes Sorensen  writes:

> Kalle Valo  writes:
>> jes.soren...@redhat.com writes:
>>
>>>  MAINTAINERS  |8 +
>>>  drivers/net/wireless/Kconfig |   19 +
>>>  drivers/net/wireless/Makefile|2 +
>>>  drivers/net/wireless/rtl8xxxu.c | 4500
>>> ++
>>>  drivers/net/wireless/rtl8xxxu.h  |  497 
>>>  drivers/net/wireless/rtl8xxxu_regs.h |  941 +++
>>
>> I think someone else already mentioned, but it would be better that has
>> it's own directory. Or should this actually be under rtlwifi
>> directory?
>
> I didn't see the need here - it's just 3 files, as long as it doesn't
> have a huge hierachy of files, a new directory doesn't add much
> value. If it becomes an issue later, we can move it into a
> subdirectory.

It's easier that the driver has it's on directory as everything
(makefile, kconfig etc) is cleanly separated. And actually I would like
to create a new vendor directory for realtek and have this in
drivers/net/wireless/realtek/rtl8xxxu/. And later we could move rtlwifi
under realtek directory as well.

>>> +T: git git://git.kernel.org/pub/scm/linux/kernel/git/jes/linux.git
>>> rtl8723au-mac80211
>>
>> I doubt that this will be in active enough development that a separate
>> git tree is needed. wireless-drivers trees should be enough and this
>> line can be removed.
>
> I would like to keep this line, it points to my development tree, which
> allows users to go back and look through my full development logs, as
> well as see ongoing work before it's pushed upstream.

I prefer that wireless-drivers(-next).git is used as the baseline for
wireless driver patches, so please remove this tag. You can document
your git server in the wiki, commit log and so on.

>> I'll do more detailed code review later, but my first impression was
>> that there was a lot of #if 0 code which is frowned upon.
>
> Johannes already brought up the #if 0 pieces. I left those in because I
> am not done with the driver, and this helps me map the flow of the
> vendor driver codebase. Those #if 0 lines are not there to sit and rot,
> but to help future development.

Yeah, I guessed that. But I suspect once the driver is applied we start
getting patches removing dead code (and rightly so). You could have a
separate branch with these #if 0 lines if they are important to you.

-- 
Kalle Valo
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Larry Finger]

On 09/06/2015 08:38 AM, Kalle Valo wrote:

Hi,

replying to an old thread first.

Jes Sorensen  writes:


Kalle Valo  writes:

jes.soren...@redhat.com writes:


  MAINTAINERS  |8 +
  drivers/net/wireless/Kconfig |   19 +
  drivers/net/wireless/Makefile|2 +
  drivers/net/wireless/rtl8xxxu.c | 4500
++
  drivers/net/wireless/rtl8xxxu.h  |  497 
  drivers/net/wireless/rtl8xxxu_regs.h |  941 +++


I think someone else already mentioned, but it would be better that has
it's own directory. Or should this actually be under rtlwifi
directory?


I didn't see the need here - it's just 3 files, as long as it doesn't
have a huge hierachy of files, a new directory doesn't add much
value. If it becomes an issue later, we can move it into a
subdirectory.


It's easier that the driver has it's on directory as everything
(makefile, kconfig etc) is cleanly separated. And actually I would like
to create a new vendor directory for realtek and have this in
drivers/net/wireless/realtek/rtl8xxxu/. And later we could move rtlwifi
under realtek directory as well.


I agree with the proposal for this driver to be under a separate directory. As 
this driver evolves to support more devices, it will likely be useful to split 
the current single .c file into a set of files, each containing the specific 
code needed for a given device. At that point, placing these files into a 
separate directory will be needed to keep the base drivers/net/wireless/ as 
clean as possible. We might as well do that now. I have no objections to moving 
rtlwifi under drivers/net/wireless/realtek/. We should even move rtl818x as well.


Larry


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Jes Sorensen]

Kalle Valo kv...@codeaurora.org writes:
 jes.soren...@redhat.com writes:

 From: Jes Sorensen jes.soren...@redhat.com

 This is an alternate driver for the Realtek 8723AU (rtl8723au) written
 from scratch utilizing the mac80211 stack.

 After spending months cleaning up the vendor provided rtl8723au
 driver, which comes with it's own 802.11 stack included, I decided to
 rewrite this driver from the bottom up.

 Where is the vendor driver available, in staging or somewhere else? It
 would be good to mention that in the commit log.

Hi Kalle,

Thanks for the feedback, the vendor driver is drivers/staging/rtl8723au/

 Many thanks to Johannes Berg for 802.11 insights and help and Larry
 Finger for help with the vendor driver.

 The full git log for the development of this driver can be found here:
 git git://git.kernel.org/pub/scm/linux/kernel/git/jes/linux.git
 branch rtl8723au-mac80211

 This driver is still experimental, but has proven to be rather stable
 for me. It lacks some features found in the staging driver, such as
 power management, AMPDU, and 40MHz channel support. In addition there
 is no AP and monitor support at this point.

 It's nice to document in the commit log what features are verified to be
 working.

 Also I see incosistencies with driver naming, in some places I see
 RTL8723au and elsewhere rtl8xxxu. And commit log title could be
 improved, for example something like rtl8xxxu: new wireless mac80211
 driver for rtl8723au chipsets

The reason for the inconsistencies is that I anticipate adding support
for more chips in the future, so the 8723au named functions are ones
that are more likely to be chip specific.

 And I would like to understand the relationship with rtlwifi, can you
 describe that a bit more? Why a separate driver instead of extending
 rtlwifi? When I look at the directory drivers/net/wireless/rtlwifi/rtl8723ae 
 I'm confused what's the bigger plan here. Larry?

I looked at rtlwifi and while it has changed a lot from the vendor
driver, it still has a strong legacy of the vendor codebase. I could
have opted to do a smaller mini-driver for rtlwifi, but I felt it was
better to start from scratch and write the driver from the bottom up for
Linux.

  MAINTAINERS  |8 +
  drivers/net/wireless/Kconfig |   19 +
  drivers/net/wireless/Makefile|2 +
  drivers/net/wireless/rtl8xxxu.c | 4500
 ++
  drivers/net/wireless/rtl8xxxu.h  |  497 
  drivers/net/wireless/rtl8xxxu_regs.h |  941 +++

 I think someone else already mentioned, but it would be better that has
 it's own directory. Or should this actually be under rtlwifi
 directory?

I didn't see the need here - it's just 3 files, as long as it doesn't
have a huge hierachy of files, a new directory doesn't add much
value. If it becomes an issue later, we can move it into a subdirectory.

 --- a/MAINTAINERS
 +++ b/MAINTAINERS
 @@ -8297,6 +8297,14 @@ S:Maintained
  F:  drivers/net/wireless/rtlwifi/
  F:  drivers/net/wireless/rtlwifi/rtl8192ce/
  
 +RTL8XXXU WIRELESS DRIVER (rtl8xxxu)
 +M:  Jes Sorensen jes.soren...@redhat.com
 +L:  linux-wireless@vger.kernel.org
 +W:  http://intellinuxwireless.org

 The link cannot be right.

That is a mistake for sure, I must have copied an entry as a template
and forgotten to remove that one.

 +T: git git://git.kernel.org/pub/scm/linux/kernel/git/jes/linux.git
 rtl8723au-mac80211

 I doubt that this will be in active enough development that a separate
 git tree is needed. wireless-drivers trees should be enough and this
 line can be removed.

I would like to keep this line, it points to my development tree, which
allows users to go back and look through my full development logs, as
well as see ongoing work before it's pushed upstream.

 I'll do more detailed code review later, but my first impression was
 that there was a lot of #if 0 code which is frowned upon.

Johannes already brought up the #if 0 pieces. I left those in because I
am not done with the driver, and this helps me map the flow of the
vendor driver codebase. Those #if 0 lines are not there to sit and rot,
but to help future development.

 And I pushed this to wireless-drivers-next.git pending branch so that
 kbuild will run it's tests with this driver.

I saw the kbuild warning, it was a false positive, which I do plan to
address down the line.

Cheers,
Jes
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Kalle Valo]

Kalle Valo kv...@codeaurora.org writes:

 And I pushed this to wireless-drivers-next.git pending branch so that
 kbuild will run it's tests with this driver.

There was one warning from kbuild, but I didn't check if it's just a
false warning or what:

drivers/net/wireless/rtl8xxxu.c: In function 'rtl8723a_phy_lc_calibrate':
drivers/net/wireless/rtl8xxxu.c:2457: warning: 'rf_amode' may be used 
uninitialized in this function

-- 
Kalle Valo
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Larry Finger]

On 04/28/2015 09:27 AM, Jes Sorensen wrote:

Kalle Valo kv...@codeaurora.org writes:

jes.soren...@redhat.com writes:


From: Jes Sorensen jes.soren...@redhat.com

This is an alternate driver for the Realtek 8723AU (rtl8723au) written
from scratch utilizing the mac80211 stack.

After spending months cleaning up the vendor provided rtl8723au
driver, which comes with it's own 802.11 stack included, I decided to
rewrite this driver from the bottom up.


Where is the vendor driver available, in staging or somewhere else? It
would be good to mention that in the commit log.


Hi Kalle,

Thanks for the feedback, the vendor driver is drivers/staging/rtl8723au/


Many thanks to Johannes Berg for 802.11 insights and help and Larry
Finger for help with the vendor driver.

The full git log for the development of this driver can be found here:
git git://git.kernel.org/pub/scm/linux/kernel/git/jes/linux.git
 branch rtl8723au-mac80211

This driver is still experimental, but has proven to be rather stable
for me. It lacks some features found in the staging driver, such as
power management, AMPDU, and 40MHz channel support. In addition there
is no AP and monitor support at this point.


It's nice to document in the commit log what features are verified to be
working.

Also I see incosistencies with driver naming, in some places I see
RTL8723au and elsewhere rtl8xxxu. And commit log title could be
improved, for example something like rtl8xxxu: new wireless mac80211
driver for rtl8723au chipsets


The reason for the inconsistencies is that I anticipate adding support
for more chips in the future, so the 8723au named functions are ones
that are more likely to be chip specific.


And I would like to understand the relationship with rtlwifi, can you
describe that a bit more? Why a separate driver instead of extending
rtlwifi? When I look at the directory drivers/net/wireless/rtlwifi/rtl8723ae
I'm confused what's the bigger plan here. Larry?


I looked at rtlwifi and while it has changed a lot from the vendor
driver, it still has a strong legacy of the vendor codebase. I could
have opted to do a smaller mini-driver for rtlwifi, but I felt it was
better to start from scratch and write the driver from the bottom up for
Linux.


  MAINTAINERS  |8 +
  drivers/net/wireless/Kconfig |   19 +
  drivers/net/wireless/Makefile|2 +
  drivers/net/wireless/rtl8xxxu.c | 4500
++
  drivers/net/wireless/rtl8xxxu.h  |  497 
  drivers/net/wireless/rtl8xxxu_regs.h |  941 +++


I think someone else already mentioned, but it would be better that has
it's own directory. Or should this actually be under rtlwifi
directory?


I didn't see the need here - it's just 3 files, as long as it doesn't
have a huge hierachy of files, a new directory doesn't add much
value. If it becomes an issue later, we can move it into a subdirectory.


--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8297,6 +8297,14 @@ S:   Maintained
  F:drivers/net/wireless/rtlwifi/
  F:drivers/net/wireless/rtlwifi/rtl8192ce/

+RTL8XXXU WIRELESS DRIVER (rtl8xxxu)
+M: Jes Sorensen jes.soren...@redhat.com
+L: linux-wireless@vger.kernel.org
+W: http://intellinuxwireless.org


The link cannot be right.


That is a mistake for sure, I must have copied an entry as a template
and forgotten to remove that one.


+T: git git://git.kernel.org/pub/scm/linux/kernel/git/jes/linux.git
rtl8723au-mac80211


I doubt that this will be in active enough development that a separate
git tree is needed. wireless-drivers trees should be enough and this
line can be removed.


I would like to keep this line, it points to my development tree, which
allows users to go back and look through my full development logs, as
well as see ongoing work before it's pushed upstream.


I'll do more detailed code review later, but my first impression was
that there was a lot of #if 0 code which is frowned upon.


Johannes already brought up the #if 0 pieces. I left those in because I
am not done with the driver, and this helps me map the flow of the
vendor driver codebase. Those #if 0 lines are not there to sit and rot,
but to help future development.


And I pushed this to wireless-drivers-next.git pending branch so that
kbuild will run it's tests with this driver.


I saw the kbuild warning, it was a false positive, which I do plan to
address down the line.


Kalle,

Jes has covered most of the points very well. I will add just a little 
explanation from my point of view.


There are two major software groups that write the original drivers for the 
Realtek chips. The PCI group, which is located in China, has worked very hard at 
their Linux drivers and have written the mac80211 versions found in rtlwifi. I 
have contributed to cleaning up their code and fixing kernel crashes, but the 
code is largely theirs. The USB group, which is located in Taiwan, also provide 
Linux drivers, but their drivers contain their own 

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Kalle Valo]

jes.soren...@redhat.com writes:

 From: Jes Sorensen jes.soren...@redhat.com

 This is an alternate driver for the Realtek 8723AU (rtl8723au) written
 from scratch utilizing the mac80211 stack.

 After spending months cleaning up the vendor provided rtl8723au
 driver, which comes with it's own 802.11 stack included, I decided to
 rewrite this driver from the bottom up.

Where is the vendor driver available, in staging or somewhere else? It
would be good to mention that in the commit log.

 Many thanks to Johannes Berg for 802.11 insights and help and Larry
 Finger for help with the vendor driver.

 The full git log for the development of this driver can be found here:
 git git://git.kernel.org/pub/scm/linux/kernel/git/jes/linux.git
 branch rtl8723au-mac80211

 This driver is still experimental, but has proven to be rather stable
 for me. It lacks some features found in the staging driver, such as
 power management, AMPDU, and 40MHz channel support. In addition there
 is no AP and monitor support at this point.

It's nice to document in the commit log what features are verified to be
working.

Also I see incosistencies with driver naming, in some places I see
RTL8723au and elsewhere rtl8xxxu. And commit log title could be
improved, for example something like rtl8xxxu: new wireless mac80211
driver for rtl8723au chipsets

And I would like to understand the relationship with rtlwifi, can you
describe that a bit more? Why a separate driver instead of extending
rtlwifi? When I look at the directory drivers/net/wireless/rtlwifi/rtl8723ae 
I'm confused what's the bigger plan here. Larry?

  MAINTAINERS  |8 +
  drivers/net/wireless/Kconfig |   19 +
  drivers/net/wireless/Makefile|2 +
  drivers/net/wireless/rtl8xxxu.c  | 4500 
 ++
  drivers/net/wireless/rtl8xxxu.h  |  497 
  drivers/net/wireless/rtl8xxxu_regs.h |  941 +++

I think someone else already mentioned, but it would be better that has
it's own directory. Or should this actually be under rtlwifi directory?

 --- a/MAINTAINERS
 +++ b/MAINTAINERS
 @@ -8297,6 +8297,14 @@ S: Maintained
  F:   drivers/net/wireless/rtlwifi/
  F:   drivers/net/wireless/rtlwifi/rtl8192ce/
  
 +RTL8XXXU WIRELESS DRIVER (rtl8xxxu)
 +M:   Jes Sorensen jes.soren...@redhat.com
 +L:   linux-wireless@vger.kernel.org
 +W:   http://intellinuxwireless.org

The link cannot be right.

 +T:   git git://git.kernel.org/pub/scm/linux/kernel/git/jes/linux.git 
 rtl8723au-mac80211

I doubt that this will be in active enough development that a separate
git tree is needed. wireless-drivers trees should be enough and this
line can be removed.

I'll do more detailed code review later, but my first impression was
that there was a lot of #if 0 code which is frowned upon.

And I pushed this to wireless-drivers-next.git pending branch so that
kbuild will run it's tests with this driver.

-- 
Kalle Valo
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Joe Perches]

On Mon, 2015-03-23 at 16:25 -0400, jes.soren...@redhat.com wrote:
 From: Jes Sorensen jes.soren...@redhat.com
 
 This is an alternate driver for the Realtek 8723AU (rtl8723au) written
 from scratch utilizing the mac80211 stack.

trivia:

 diff --git a/drivers/net/wireless/rtl8xxxu.c b/drivers/net/wireless/rtl8xxxu.c
[
 +#define USB_VENDER_ID_REALTEK0x0BDA

realtek can't spell.  VENDOR

 +/* Minimum IEEE80211_MAX_FRAME_LEN */
 +#define RTL_RX_BUFFER_SIZE   IEEE80211_MAX_FRAME_LEN
 +
 +static struct usb_device_id dev_table[] = {

const

 + {USB_DEVICE_AND_INTERFACE_INFO(USB_VENDER_ID_REALTEK, 0x8724,
 +0xff, 0xff, 0xff)},
 + {USB_DEVICE_AND_INTERFACE_INFO(USB_VENDER_ID_REALTEK, 0x1724,
 +0xff, 0xff, 0xff)},
 + {USB_DEVICE_AND_INTERFACE_INFO(USB_VENDER_ID_REALTEK, 0x0724,
 +0xff, 0xff, 0xff)},
 + { }
 +};

 +static int rtl8xxxu_read_efuse(struct rtl8xxxu_priv *priv)
 +{

[]

 + while (efuse_addr  EFUSE_REAL_CONTENT_LEN_8723A) {
 + ret = rtl8xxxu_read_efuse8(priv, efuse_addr++, header);
 + if (ret || header == 0xff)
 + goto exit;

break and no exit label would be more common

[]

 +exit:
 + rtl8723au_write8(priv, REG_EFUSE_ACCESS, EFUSE_ACCESS_DISABLE);
 +
 + if (priv-efuse_wifi.efuse.rtl_id != cpu_to_le16(0x8129))
 + ret = EINVAL;

-EINVAL

 +static bool rtl8xxxu_simularity_compare(struct rtl8xxxu_priv *priv,
 + int result[][8], int c1, int c2)

Looking through git history, simularity seems to be used
because realtek can't spell.


--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Jes Sorensen]

Joe Perches j...@perches.com writes:
 On Mon, 2015-03-23 at 16:25 -0400, jes.soren...@redhat.com wrote:
 From: Jes Sorensen jes.soren...@redhat.com
 
 This is an alternate driver for the Realtek 8723AU (rtl8723au) written
 from scratch utilizing the mac80211 stack.

 trivia:

 diff --git a/drivers/net/wireless/rtl8xxxu.c 
 b/drivers/net/wireless/rtl8xxxu.c
 [
 +#define USB_VENDER_ID_REALTEK   0x0BDA

 realtek can't spell.  VENDOR

Rather minor, but sure, I'll fix that up.

 +/* Minimum IEEE80211_MAX_FRAME_LEN */
 +#define RTL_RX_BUFFER_SIZE  IEEE80211_MAX_FRAME_LEN
 +
 +static struct usb_device_id dev_table[] = {

 const

 +{USB_DEVICE_AND_INTERFACE_INFO(USB_VENDER_ID_REALTEK, 0x8724,
 +   0xff, 0xff, 0xff)},
 +{USB_DEVICE_AND_INTERFACE_INFO(USB_VENDER_ID_REALTEK, 0x1724,
 +   0xff, 0xff, 0xff)},
 +{USB_DEVICE_AND_INTERFACE_INFO(USB_VENDER_ID_REALTEK, 0x0724,
 +   0xff, 0xff, 0xff)},
 +{ }
 +};

 +static int rtl8xxxu_read_efuse(struct rtl8xxxu_priv *priv)
 +{

 []

 +while (efuse_addr  EFUSE_REAL_CONTENT_LEN_8723A) {
 +ret = rtl8xxxu_read_efuse8(priv, efuse_addr++, header);
 +if (ret || header == 0xff)
 +goto exit;

 break and no exit label would be more common

 []

 +exit:
 +rtl8723au_write8(priv, REG_EFUSE_ACCESS, EFUSE_ACCESS_DISABLE);
 +
 +if (priv-efuse_wifi.efuse.rtl_id != cpu_to_le16(0x8129))
 +ret = EINVAL;

 -EINVAL

This needs to be fixed, however the return code isn't actually checked
at present, which is a bigger issue. It's a highly unlikely error
condition, so it can easily be fixed in a follow-on patch.

 +static bool rtl8xxxu_simularity_compare(struct rtl8xxxu_priv *priv,
 +int result[][8], int c1, int c2)

 Looking through git history, simularity seems to be used
 because realtek can't spell.

It might be the case, but until I see some actual evidence of this, I am
not going to spend cycles on it. It's hardly something justifying the
patch noise in the first place.

Jes
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Jes Sorensen]

Larry Finger larry.fin...@lwfinger.net writes:
 Quilt reports the following when this patch is refreshed:

 Warning: trailing whitespace in lines 2862,3273,3492,3521 of
 drivers/net/wireless/rtl8xxxu.c

Hi Larry,

I fixed up the trailing whitespaces in v2 (and added a
(setq show-trailing-whitespace t) to my config file).

 I have not analyzed all the temporary manipulations of rtl8xxxu_debug
 to see what you are doing; however, I suggest that you add a module
 parameter so that debugging can be enabled without rebuilding the
 module. That way a user who is using a distro binary can enable
 debugging without the hassle of a full kernel rebuild.

I added a module paramter, and worked around the %@#$%#$%#$ dynamic
debug issues to get the driver to actually print stuff when you set it.
For some reason I was convinced I already had this as a module
parameter.

 Because this driver is under drivers/net, checkpatch.pl adds
 additional tests that may not be used in other trees. For instance, it
 complains when it finds a block comment that starts with a bare
 /*. What is usually done is to use /** instead.

This part I will leave as is.

 I think I understand why some of the #if 0 blocks are present, but
 others are not clear. For example, I see no value of keeping code that
 is labelled only for PCIe. Is it your intention to add the RTL8723AE
 to this driver?

I prefer to keep them in place for now, as it makes it easier for me to
debug the tables when I compare them to the tables in the vendor
driver. I will eventually remove those, but it's to early to do that
just yet.

 Running checkpatch.pl on this patch results in total of 24 errors, 99
 warnings, and 105 checks. From your mail exchange with Joe Perches, I
 understand that you will not wish to fix all of these; however, the
 errors should be handled. The fewer of the warnings or checks that are
 left, the better, if only to prevent interference from the script
 kiddies.

I should have run checkpatch on Friday when I posted the first
version. Some of the formatting was due to converting from printk() to
other wrappers.

It's rather disturbing how checkpatch has been turned into a trolling
tool used by the script kiddies to harrass actual development, and to
bump their number of commits in the git log, but I guess there isn't
much to do about it. As always, I very much appreciate real bug reports!

 I like the clean look of the code. That is particularly impressive
 given the look of the original. I wish I had the hardware on which to
 test it. Perhaps I can wedge it into a kernel version that builds and
 runs on my Radxa Rock,

Thanks, I tried hard to keep it clean, even though there were parts of
badness that crept over from the original driver. In particular in the
radio configuration portions.

Cheers,
Jes
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Joe Perches]

On Mon, 2015-03-09 at 13:00 -0400, jes.soren...@redhat.com wrote:
 This is an alternate driver for the Realtek 8723AU (rtl8723au) written
 from scratch utilizing the mac80211 stack.

Mostly trivial comments:

 diff --git a/MAINTAINERS b/MAINTAINERS
[]
 +RTL8XXXU WIRELESS DRIVER (rtl8xxxu)
 +M:   Jes Sorensen jes.soren...@redhat.com
 +L:   linux-wireless@vger.kernel.org
 +W:   http://intellinuxwireless.org
 +T:   git git://git.Mkernel.org/pub/scm/linux/kernel/git/jes/linux.git
 + git branch rtl8723au-mac80211

please keep this on one line

T:  git git://git.kernel.org/pub/scm/linux/kernel/git/jes/linux.git 
rtl8723au-mac80211

 diff --git a/drivers/net/wireless/rtl8xxxu.c b/drivers/net/wireless/rtl8xxxu.c

[]

 +static struct usb_device_id dev_table[] = {

Please use const where appropriate.


 +static struct ieee80211_rate rtl8xxxu_rates[] = {
[]
 +static struct ieee80211_channel rtl8xxxu_channels_2g[] = {
[]
 +static struct ieee80211_supported_band rtl8xxxu_supported_band = {
[]
 +static struct rtl8xxxu_reg8val rtl8723a_mac_init_table[] = {
[]
 +static struct rtl8xxxu_reg32val rtl8723a_phy_1t_init_table[] = {
[]
 +static struct rtl8xxxu_reg32val rtl8723a_agc_1t_init_table[] = {
[]
 +static struct rtl8xxxu_rfregval rtl8723au_radioa_rf6052_1t_init_table[] = {

 +static u8 rtl8723au_read8(struct rtl8xxxu_priv *priv, u16 addr)

 + if (rtl8xxxu_debug  RTL8XXXU_DEBUG_REG_READ)
 + dev_info(udev-dev, %s(%04x)   = 0x%02x, len %i\n,
 +  __func__, addr, data, len);

Probably better to use a local rtl_dbg which use
 whatever flag you want (or no flag at all)

#define rtl_dbg(dev, flag, fmt, ...)
do {
if (rtl8xxxu_debug  RTL8XXX_DEBUG_##flag)
dev_dbg(dev, fmt, ##__VA_ARGS__);
} while (0)

so this becomes

rtl_dbg(udev-dev, REG_REG, %s(%04x)   = 0x%02x, len %i\n,
__func__, addr, data, len);

though you could remove the __func__ here as
dynamic_debug can add it for you.


 +static bool rtl8xxxu_simularity_compare(struct rtl8xxxu_priv *priv,
 + int result[][8], int c1, int c2)

similarity ?

 +{
[]
 + if (simubitmap == 0) {
 + for (i = 0; i  (bound / 4); i++) {
 + if (candidate[i] = 0) {
 + for (j = i * 4; j  (i + 1) * 4 - 2; j++)
 + result[3][j] = result[candidate[i]][j];
 + retval = false;
 + }
 + }
 + return retval;
 + } else if (!(simubitmap  0x0f)) {

Maybe remove the else

 + /* path A OK */
 + for (i = 0; i  4; i++)
 + result[3][i] = result[c1][i];
 + } else if (!(simubitmap  0xf0)  is_2t) {
 + /* path B OK */
 + for (i = 4; i  8; i++)
 + result[3][i] = result[c1][i];
 + }
 +
 + return false;
 +}

[]

 +static int rtl8xxxu_iqk_path_a(struct rtl8xxxu_priv *priv, bool configpathb)

It might be helpful to describe the return
flags here as it's not negative error

 + if (!(reg_eac  BIT(28)) 
 + ((reg_e94  0x03ff) != 0x0142) 
 + ((reg_e9c  0x03ff) != 0x0042))
 + result |= 0x01;
 + else/* If TX not OK, ignore RX */
 + goto out;
 +
 + /* If TX is OK, check whether RX is OK */
 + if (!(reg_eac  BIT(27)) 
 + ((reg_ea4  0x03ff) != 0x0132) 
 + ((reg_eac  0x03ff) != 0x0036))
 + result |= 0x02;
 + else
 + dev_warn(priv-udev-dev, %s: Path A RX IQK failed!\n,
 +  __func__);
 +out:
 + return result;
 +}
 +
 +static void rtl8xxxu_phy_iqcalibrate(struct rtl8xxxu_priv *priv,
 +  int result[][8], int t, bool is_2t)
 +{
 + struct device *dev = priv-udev-dev;
 + u32 i, val32;
 + int path_a_ok /*, path_b_ok */;
 + int retry = 2;
 +
 + u32 ADDA_REG[RTL8XXXU_ADDA_REGS] = {
 + REG_FPGA0_XCD_SWITCH_CTRL, REG_BLUETOOTH,
 + REG_RX_WAIT_CCA, REG_TX_CCK_RFON,
 + REG_TX_CCK_BBON, REG_TX_OFDM_RFON,
 + REG_TX_OFDM_BBON, REG_TX_TO_RX,
 + REG_TX_TO_TX, REG_RX_CCK,
 + REG_RX_OFDM, REG_RX_WAIT_RIFS,
 + REG_RX_TO_RX, REG_STANDBY,
 + REG_SLEEP, REG_PMPD_ANAEN
 + };

static const is generally better.

 +
 + u32 IQK_MAC_REG[RTL8XXXU_MAC_REGS] = {
 + REG_TXPAUSE, REG_BEACON_CTRL,
 + REG_BEACON_CTRL_1, REG_GPIO_MUXCFG
 + };
 +
 + u32 IQK_BB_REG_92C[RTL8XXXU_BB_REGS] = {
 + REG_OFDM0_TRX_PATH_ENABLE, REG_OFDM0_TR_MUX_PAR,
 + REG_FPGA0_XCD_RF_SW_CTRL, REG_CONFIG_ANT_A, REG_CONFIG_ANT_B,
 + REG_FPGA0_XAB_RF_SW_CTRL, REG_FPGA0_XA_RF_INT_OE,
 + REG_FPGA0_XB_RF_INT_OE, REG_FPGA0_RF_MODE
 + };

[]

 +static void rtl8723a_phy_iq_calibrate(struct rtl8xxxu_priv *priv, bool 
 

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Johannes Berg]

Hi Jes,

 Many thanks to Johannes Berg for 802.11 insights and help and Larry
 Finger for help with the vendor driver.

:-)

Just a few comments since this is really the first time I'm looking
closely at the code.

 + h2c.ramask.cmd = H2C_SET_RATE_MASK;
 + put_unaligned_le16(ramask  0x, h2c.ramask.mask_lo);
 + put_unaligned_le16(ramask  16, h2c.ramask.mask_hi);

you marked that __packed, so the put_unaligned_le16() isn't really
needed, you can use regular assignments (= cpu_to_le16(...))

 +static void
 +rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 +   struct ieee80211_bss_conf *bss_conf, u32 changed)
 +{
 + struct rtl8xxxu_priv *priv = hw-priv;
 + struct device *dev = priv-udev-dev;
 + struct ieee80211_sta *sta;
 + u32 val32;
 +#if 0
 + u16 val16;
 +#endif
 + u8 val8;
 +
 + if (changed  BSS_CHANGED_ASSOC) {
 + struct h2c_cmd h2c;
 +
 + memset(h2c, 0, sizeof(struct h2c_cmd));
 + rtl8xxxu_set_linktype(priv, vif-type);
 +
 + if (bss_conf-assoc) {
 + rcu_read_lock();
 + sta = ieee80211_find_sta(vif, bss_conf-bssid);
 + if (!sta) {
 + dev_info(dev, %s: ASSOC no sta found\n,
 +  __func__);
 + rcu_read_unlock();
 + goto error;
 + }
 +
 + if (sta-ht_cap.ht_supported)
 + dev_info(dev, %s: HT supported\n, __func__);
 + if (sta-vht_cap.vht_supported)
 + dev_info(dev, %s: VHT supported\n, __func__);
 + rtl8xxxu_update_rate_table(priv, sta);
 + rcu_read_unlock();

You could perhaps consider doing this in the sta_state() callback, when
the station becomes associated, which is just before (or just after?)
this gets called. Then you don't need to do the find_sta() here. Not
that it's really a problem, but might look a bit nicer/cleaner.

 + if (changed  BSS_CHANGED_HT) {
 + u8 ampdu_factor, ampdu_density;
 + u8 sifs;
 +
 + rcu_read_lock();
 + sta = ieee80211_find_sta(vif, bss_conf-bssid);
 + if (!sta) {
 + dev_info(dev, BSS_CHANGED_HT: No HT sta found!\n);
 + rcu_read_unlock();
 + goto error;
 + }
 + if (sta-ht_cap.ht_supported) {
 + ampdu_factor = sta-ht_cap.ampdu_factor;
 + ampdu_density = sta-ht_cap.ampdu_density;
 + sifs = 0x0e;
 + } else {
 + ampdu_factor = 0;
 + ampdu_density = 0;
 + sifs = 0x0a;
 + }
 + rcu_read_unlock();

This doesn't really make much sense - BSS_CHANGED_HT only says we
changed bss_conf.ht_operation_mode which you don't use here.

 + if (changed  BSS_CHANGED_BSSID) {
 + dev_info(dev, Changed BSSID!\n);
 + rtl8xxxu_set_bssid(priv, bss_conf-bssid);
 +
 + rcu_read_lock();
 + sta = ieee80211_find_sta(vif, bss_conf-bssid);
 + if (!sta) {
 + dev_info(dev, No bssid sta found!\n);
 + rcu_read_unlock();
 + goto error;
 + }
 + rcu_read_unlock();
 + }

You probably want to roll this up into CHANGED_ASSOC, since
CHANGED_BSSID always really comes together with that for station mode.
For IBSS things would be different but you don't seem to do that.

 + if (changed  BSS_CHANGED_BASIC_RATES) {
 + dev_info(dev, Changed BASIC_RATES!\n);
 + rcu_read_lock();
 + sta = ieee80211_find_sta(vif, bss_conf-bssid);
 + if (sta)
 + rtl8xxxu_set_basic_rates(priv, sta);
 + else
 + dev_info(dev,
 +  BSS_CHANGED_BASIC_RATES: No sta found!\n);
 +
 + rcu_read_unlock();
 + }

Ditto, this ever change during the lifetime of a BSS either, i.e. it
cannot change while you're associated, and you don't care while you're
not. Again, IBSS could be different, not quite sure off the top of my
head.

 +static u32 rtl8xxxu_queue_select(struct ieee80211_hw *hw, struct sk_buff 
 *skb)
 +{
 + struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb-data;
 + u32 queue;
 +
 + if (unlikely(ieee80211_is_beacon(hdr-frame_control)))
 + queue = TXDESC_QUEUE_BEACON;
 + if (ieee80211_is_mgmt(hdr-frame_control))

I think you mean else if?

Anyway beacons cannot happen here except perhaps for injection, in which
case you probably wouldn't want to use the beacon queue anyway. I'd
remove the beacon case here entirely.

 + queue = TXDESC_QUEUE_MGNT;
 + 

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Joe Perches]

On Mon, 2015-03-09 at 15:02 -0400, Jes Sorensen wrote:
 Joe Perches j...@perches.com writes:
  On Mon, 2015-03-09 at 14:43 -0400, Jes Sorensen wrote:
  Joe Perches j...@perches.com writes:
   On Mon, 2015-03-09 at 13:00 -0400, jes.soren...@redhat.com wrote:
   This is an alternate driver for the Realtek 8723AU (rtl8723au) written
   from scratch utilizing the mac80211 stack.
  
   Mostly trivial comments:
  
   diff --git a/MAINTAINERS b/MAINTAINERS
   []
   +RTL8XXXU WIRELESS DRIVER (rtl8xxxu)
   +M: Jes Sorensen jes.soren...@redhat.com
   +L: linux-wireless@vger.kernel.org
   +W: http://intellinuxwireless.org
   +T: git git://git.Mkernel.org/pub/scm/linux/kernel/git/jes/linux.git
   +   git branch rtl8723au-mac80211
  
   please keep this on one line
  
  Lines are 80 characters, and it won't fit.
 
  For code yes, for MAINTAINERS no.
  There are many lines  80 chars there.
  Please keep:
  TYPE: entry
  on single lines.
 
 Longer than 80 characters is ugly and you haven't provided any
 justification for putting it on one line.

TYPE: entries are single line.

   +struct rtl8xxxu_priv {
   []
   +   u32 has_wifi:1;
   +   u32 has_bluetooth:1;
   +   u32 enable_bluetooth:1;
   +   u32 has_gps:1;
   +   u32 vendor_umc:1;
   +   u32 has_polarity_ctrl:1;
   +   u32 has_eeprom:1;
   +   u32 boot_eeprom:1;
   +   u32 ep_tx_high_queue:1;
   +   u32 ep_tx_normal_queue:1;
   +   u32 ep_tx_low_queue:1;
   +   u32 path_a_hi_power:1;
  
   These might be better as bool instead of packed bitfields.
  
  bool wastes 4 bytes in the struct, so no that would be worse.
 
  It's a used-once struct.  4 bytes, wow.
 
 Once per device - but it all comes down to a matter of style and taste
 of the developer in the end. You seem to be obsessed with imposing your
 ideas on everybody, rather than actually looking for real bugs.

I'm not imposing anything.
You can accept it or not.

btw: every use of any bitfield requires more code
 for RMW than a byte access does.


--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Johannes Berg]

On Mon, 2015-03-09 at 14:35 -0400, Jes Sorensen wrote:

  +  if (sta-ht_cap.ht_supported) {
  +  ampdu_factor = sta-ht_cap.ampdu_factor;
  +  ampdu_density = sta-ht_cap.ampdu_density;
  +  sifs = 0x0e;
  +  } else {
  +  ampdu_factor = 0;
  +  ampdu_density = 0;
  +  sifs = 0x0a;
  +  }
  +  rcu_read_unlock();
 
  This doesn't really make much sense - BSS_CHANGED_HT only says we
  changed bss_conf.ht_operation_mode which you don't use here.
 
 In this case, is there another flag that would be more appropriate to
 handle this under?

I think this would be similarly appropriate to handle in the sta_state()
call (when it becomes associated) since these values also cannot change
on the fly. Or just also roll it up into ASSOC where you seem to need it
mostly.

  +  if (unlikely(ieee80211_is_beacon(hdr-frame_control)))
  +  queue = TXDESC_QUEUE_BEACON;
  +  if (ieee80211_is_mgmt(hdr-frame_control))
 
  I think you mean else if?
 
  Anyway beacons cannot happen here except perhaps for injection, in which
  case you probably wouldn't want to use the beacon queue anyway. I'd
  remove the beacon case here entirely.
 
 This is probably suffer from being derived from vendor driver. They have
 a special beacon queue in the hardware which doesn't seem to map
 directly to anything the Linux stack does.
 
 Until I or someone else eventually implements AP support in this driver,
 it probably doesn't matter.

Indeed. Nonetheless, I'd remove the is_beacon() as you could hit this
code path from userspace by injecting beacon frames, and it's unlikely
to work correctly.

When you actually add AP mode, you probably won't go through this
function to send beacons anyway, but through some other code path that
can just statically select the beacon queue, so even then it doesn't
seem like you'd need this.

  Something about the TX is a bit fishy, you're using some software rate
  control data but aren't reporting any data back for rate control which
  seems to imply the device does it. You're also setting
  IEEE80211_HW_HAS_RATE_CONTROL which means you can't be using rates etc.
  here? I'll probably have to look into this more, it's an area that I'm
  not intimately familiar with.
 
 The reason for this is that I can do either way and I have been playing
 around with both. The vendor driver explicitly uses sw rate control for
 management packets, whether this is necessary or legacy I cannot tell,
 but I was trying to map to what they were doing.

Ok. If you're actually getting good rates out of it then you probably do
have firmware rate control kicking in, so it's probably fine. Reporting
things back would be nice but isn't really essential.

  +  skb_size = sizeof(struct rtl8xxxu_rx_desc) +
  +  RTL_RX_BUFFER_SIZE;
  +  skb = dev_alloc_skb(skb_size);
 
  And then you allocate a new one into the same variable. From a
  maintenance POV that seems a bit ugly, perhaps you should avoid that.
 
 I have always done this, kinda like it, but I am not really objecting to
 it.

Sure, if you like doing it that way I certainly won't object to it. It
just caught my eye because initially I thought that you were using the
skb after giving up ownership by passing to mac80211.

  This is also very strange, you're sending essentially random data, from
  an skb pointer that's not even a proper skb len? I think you just want
  to use kmalloc() here instead of having an skb? Or at least use
  skb_put() to reserve that memory area - it doesn't make a big difference
  to you but using the skb API in this way is likely to confuse people in
  the future.
 
 I wanted to avoid having to allocate and then copy on arrival. It's been
 quite common for drivers to do this, but I guess I ought to set the skb
 len as well at this point to keep it clean.

Yeah I kinda understood that later :) I guess you set the len later, so
perhaps just adding some comments would be best, since otherwise you
could also arguably have to initialize the skb memory which you really
don't want to. Btw, there doesn't seem to be much point in initialising
the hdr either?

  In fact you could even allocate a page instead, and then add it to the
  skb as a frag like iwlwifi does for example, and potentially not have to
  copy the data around later when it goes somewhere. Not sure that's worth
  it for this device though :)
 
 I think the device can handle fragments, problem is I have zero
 documentation on how it does so.

Oh, no, I meant on RX. Submit URBs with pages, and then allocate a very
small skb only later, etc. But anyway since this works for you it's
probably better to keep as is unless you notice performance issues with
it. Using pages has some downsides as well.

 Well yes and no, the hardware happens to match to those values too,
 which is a bit of good luck. I think when it uses some of the strange
 encryptions 

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Jes Sorensen]

Johannes Berg johan...@sipsolutions.net writes:
 Hi Jes,

 Many thanks to Johannes Berg for 802.11 insights and help and Larry
 Finger for help with the vendor driver.

 :-)

 Just a few comments since this is really the first time I'm looking
 closely at the code.

Thanks!

 +h2c.ramask.cmd = H2C_SET_RATE_MASK;
 +put_unaligned_le16(ramask  0x, h2c.ramask.mask_lo);
 +put_unaligned_le16(ramask  16, h2c.ramask.mask_hi);

 you marked that __packed, so the put_unaligned_le16() isn't really
 needed, you can use regular assignments (= cpu_to_le16(...))

I see, I wasn't aware of this, so figured better safe than sorry.

 +static void
 +rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif 
 *vif,
 +  struct ieee80211_bss_conf *bss_conf, u32 changed)
 +{
 +struct rtl8xxxu_priv *priv = hw-priv;
 +struct device *dev = priv-udev-dev;
 +struct ieee80211_sta *sta;
 +u32 val32;
 +#if 0
 +u16 val16;
 +#endif
 +u8 val8;
 +
 +if (changed  BSS_CHANGED_ASSOC) {
 +struct h2c_cmd h2c;
 +
 +memset(h2c, 0, sizeof(struct h2c_cmd));
 +rtl8xxxu_set_linktype(priv, vif-type);
 +
 +if (bss_conf-assoc) {
 +rcu_read_lock();
 +sta = ieee80211_find_sta(vif, bss_conf-bssid);
 +if (!sta) {
 +dev_info(dev, %s: ASSOC no sta found\n,
 + __func__);
 +rcu_read_unlock();
 +goto error;
 +}
 +
 +if (sta-ht_cap.ht_supported)
 +dev_info(dev, %s: HT supported\n, __func__);
 +if (sta-vht_cap.vht_supported)
 +dev_info(dev, %s: VHT supported\n, __func__);
 +rtl8xxxu_update_rate_table(priv, sta);
 +rcu_read_unlock();

 You could perhaps consider doing this in the sta_state() callback, when
 the station becomes associated, which is just before (or just after?)
 this gets called. Then you don't need to do the find_sta() here. Not
 that it's really a problem, but might look a bit nicer/cleaner.

I'll have a look. I wasn't sure which cases were only valid as a
sub-case of others, as I didn't find any documentation on it (but
admittedly I didn't look too hard).

 +if (changed  BSS_CHANGED_HT) {
 +u8 ampdu_factor, ampdu_density;
 +u8 sifs;
 +
 +rcu_read_lock();
 +sta = ieee80211_find_sta(vif, bss_conf-bssid);
 +if (!sta) {
 +dev_info(dev, BSS_CHANGED_HT: No HT sta found!\n);
 +rcu_read_unlock();
 +goto error;
 +}
 +if (sta-ht_cap.ht_supported) {
 +ampdu_factor = sta-ht_cap.ampdu_factor;
 +ampdu_density = sta-ht_cap.ampdu_density;
 +sifs = 0x0e;
 +} else {
 +ampdu_factor = 0;
 +ampdu_density = 0;
 +sifs = 0x0a;
 +}
 +rcu_read_unlock();

 This doesn't really make much sense - BSS_CHANGED_HT only says we
 changed bss_conf.ht_operation_mode which you don't use here.

In this case, is there another flag that would be more appropriate to
handle this under?

 +if (changed  BSS_CHANGED_BSSID) {
 +dev_info(dev, Changed BSSID!\n);
 +rtl8xxxu_set_bssid(priv, bss_conf-bssid);
 +
 +rcu_read_lock();
 +sta = ieee80211_find_sta(vif, bss_conf-bssid);
 +if (!sta) {
 +dev_info(dev, No bssid sta found!\n);
 +rcu_read_unlock();
 +goto error;
 +}
 +rcu_read_unlock();
 +}

 You probably want to roll this up into CHANGED_ASSOC, since
 CHANGED_BSSID always really comes together with that for station mode.
 For IBSS things would be different but you don't seem to do that.

OK, sounds reasonable.

 +if (changed  BSS_CHANGED_BASIC_RATES) {
 +dev_info(dev, Changed BASIC_RATES!\n);
 +rcu_read_lock();
 +sta = ieee80211_find_sta(vif, bss_conf-bssid);
 +if (sta)
 +rtl8xxxu_set_basic_rates(priv, sta);
 +else
 +dev_info(dev,
 + BSS_CHANGED_BASIC_RATES: No sta found!\n);
 +
 +rcu_read_unlock();
 +}

 Ditto, this ever change during the lifetime of a BSS either, i.e. it
 cannot change while you're associated, and you don't care while you're
 not. Again, IBSS could be different, not quite sure off the top of my
 head.

Ditto

 +static u32 rtl8xxxu_queue_select(struct ieee80211_hw *hw, struct
 sk_buff *skb)
 +{
 +struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb-data;
 +u32 queue;
 +
 +if 

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Jes Sorensen]

Joe Perches j...@perches.com writes:
 On Mon, 2015-03-09 at 13:00 -0400, jes.soren...@redhat.com wrote:
 This is an alternate driver for the Realtek 8723AU (rtl8723au) written
 from scratch utilizing the mac80211 stack.

 Mostly trivial comments:

 diff --git a/MAINTAINERS b/MAINTAINERS
 []
 +RTL8XXXU WIRELESS DRIVER (rtl8xxxu)
 +M:  Jes Sorensen jes.soren...@redhat.com
 +L:  linux-wireless@vger.kernel.org
 +W:  http://intellinuxwireless.org
 +T:  git git://git.Mkernel.org/pub/scm/linux/kernel/git/jes/linux.git
 +git branch rtl8723au-mac80211

 please keep this on one line

Lines are 80 characters, and it won't fit.

 T: git git://git.kernel.org/pub/scm/linux/kernel/git/jes/linux.git
 rtl8723au-mac80211

 diff --git a/drivers/net/wireless/rtl8xxxu.c 
 b/drivers/net/wireless/rtl8xxxu.c

 []

 +static struct usb_device_id dev_table[] = {

 Please use const where appropriate.


 +static struct ieee80211_rate rtl8xxxu_rates[] = {
 []
 +static struct ieee80211_channel rtl8xxxu_channels_2g[] = {
 []
 +static struct ieee80211_supported_band rtl8xxxu_supported_band = {
 []
 +static struct rtl8xxxu_reg8val rtl8723a_mac_init_table[] = {
 []
 +static struct rtl8xxxu_reg32val rtl8723a_phy_1t_init_table[] = {
 []
 +static struct rtl8xxxu_reg32val rtl8723a_agc_1t_init_table[] = {
 []
 +static struct rtl8xxxu_rfregval rtl8723au_radioa_rf6052_1t_init_table[] = {

 +static u8 rtl8723au_read8(struct rtl8xxxu_priv *priv, u16 addr)

 +if (rtl8xxxu_debug  RTL8XXXU_DEBUG_REG_READ)
 +dev_info(udev-dev, %s(%04x)   = 0x%02x, len %i\n,
 + __func__, addr, data, len);

 Probably better to use a local rtl_dbg which use
  whatever flag you want (or no flag at all)

 #define rtl_dbg(dev, flag, fmt, ...)
 do {
   if (rtl8xxxu_debug  RTL8XXX_DEBUG_##flag)
   dev_dbg(dev, fmt, ##__VA_ARGS__);
 } while (0)

Ewww, absolutely not! This is way worse than how I wrote it!

 so this becomes

   rtl_dbg(udev-dev, REG_REG, %s(%04x)   = 0x%02x, len %i\n,
   __func__, addr, data, len);

 though you could remove the __func__ here as
 dynamic_debug can add it for you.


 +static bool rtl8xxxu_simularity_compare(struct rtl8xxxu_priv *priv,
 +int result[][8], int c1, int c2)

 similarity ?

Simularity came from the vendor driver, I assume it references comparing
the radio signal quality. Unless you have evidence to the contrary, I am
not changing it.

 +{
 []
 +if (simubitmap == 0) {
 +for (i = 0; i  (bound / 4); i++) {
 +if (candidate[i] = 0) {
 +for (j = i * 4; j  (i + 1) * 4 - 2; j++)
 +result[3][j] = result[candidate[i]][j];
 +retval = false;
 +}
 +}
 +return retval;
 +} else if (!(simubitmap  0x0f)) {

 Maybe remove the else

 +/* path A OK */
 +for (i = 0; i  4; i++)
 +result[3][i] = result[c1][i];
 +} else if (!(simubitmap  0xf0)  is_2t) {
 +/* path B OK */
 +for (i = 4; i  8; i++)
 +result[3][i] = result[c1][i];
 +}
 +
 +return false;
 +}

These are part of the same comparison chain, removing the else would be
inconsistent.

 +static int rtl8xxxu_iqk_path_a(struct rtl8xxxu_priv *priv, bool configpathb)

 It might be helpful to describe the return
 flags here as it's not negative error

 +if (!(reg_eac  BIT(28)) 
 +((reg_e94  0x03ff) != 0x0142) 
 +((reg_e9c  0x03ff) != 0x0042))
 +result |= 0x01;
 +else/* If TX not OK, ignore RX */
 +goto out;
 +
 +/* If TX is OK, check whether RX is OK */
 +if (!(reg_eac  BIT(27)) 
 +((reg_ea4  0x03ff) != 0x0132) 
 +((reg_eac  0x03ff) != 0x0036))
 +result |= 0x02;
 +else
 +dev_warn(priv-udev-dev, %s: Path A RX IQK failed!\n,
 + __func__);
 +out:
 +return result;
 +}
 +
 +static void rtl8xxxu_phy_iqcalibrate(struct rtl8xxxu_priv *priv,
 + int result[][8], int t, bool is_2t)
 +{
 +struct device *dev = priv-udev-dev;
 +u32 i, val32;
 +int path_a_ok /*, path_b_ok */;
 +int retry = 2;
 +
 +u32 ADDA_REG[RTL8XXXU_ADDA_REGS] = {
 +REG_FPGA0_XCD_SWITCH_CTRL, REG_BLUETOOTH,
 +REG_RX_WAIT_CCA, REG_TX_CCK_RFON,
 +REG_TX_CCK_BBON, REG_TX_OFDM_RFON,
 +REG_TX_OFDM_BBON, REG_TX_TO_RX,
 +REG_TX_TO_TX, REG_RX_CCK,
 +REG_RX_OFDM, REG_RX_WAIT_RIFS,
 +REG_RX_TO_RX, REG_STANDBY,
 +REG_SLEEP, REG_PMPD_ANAEN
 +};

 static const is generally better.

It's irrelevant

 +
 +u32 IQK_MAC_REG[RTL8XXXU_MAC_REGS] = {
 +REG_TXPAUSE, REG_BEACON_CTRL,
 +REG_BEACON_CTRL_1, REG_GPIO_MUXCFG
 +};
 +
 +u32 

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Jes Sorensen]

Joe Perches j...@perches.com writes:
 On Mon, 2015-03-09 at 14:43 -0400, Jes Sorensen wrote:
 Joe Perches j...@perches.com writes:
  On Mon, 2015-03-09 at 13:00 -0400, jes.soren...@redhat.com wrote:
  This is an alternate driver for the Realtek 8723AU (rtl8723au) written
  from scratch utilizing the mac80211 stack.
 
  Mostly trivial comments:
 
  diff --git a/MAINTAINERS b/MAINTAINERS
  []
  +RTL8XXXU WIRELESS DRIVER (rtl8xxxu)
  +M:   Jes Sorensen jes.soren...@redhat.com
  +L:   linux-wireless@vger.kernel.org
  +W:   http://intellinuxwireless.org
  +T:   git git://git.Mkernel.org/pub/scm/linux/kernel/git/jes/linux.git
  + git branch rtl8723au-mac80211
 
  please keep this on one line
 
 Lines are 80 characters, and it won't fit.

 For code yes, for MAINTAINERS no.
 There are many lines  80 chars there.
 Please keep:
 TYPE:   entry
 on single lines.

Longer than 80 characters is ugly and you haven't provided any
justification for putting it on one line.

  +static void rtl8xxxu_phy_iqcalibrate(struct rtl8xxxu_priv *priv,
  +  int result[][8], int t, bool is_2t)
  +{
  + struct device *dev = priv-udev-dev;
  + u32 i, val32;
  + int path_a_ok /*, path_b_ok */;
  + int retry = 2;
  +
  + u32 ADDA_REG[RTL8XXXU_ADDA_REGS] = {
  + REG_FPGA0_XCD_SWITCH_CTRL, REG_BLUETOOTH,
  + REG_RX_WAIT_CCA, REG_TX_CCK_RFON,
  + REG_TX_CCK_BBON, REG_TX_OFDM_RFON,
  + REG_TX_OFDM_BBON, REG_TX_TO_RX,
  + REG_TX_TO_TX, REG_RX_CCK,
  + REG_RX_OFDM, REG_RX_WAIT_RIFS,
  + REG_RX_TO_RX, REG_STANDBY,
  + REG_SLEEP, REG_PMPD_ANAEN
  + };
 
  static const is generally better.
 
 It's irrelevant

 Not really, it requires an initialization on entry
 and would make the object code smaller to use const.

As I said, it's irrelevant. The gain from this is microscopic, sure it
can be done, but it's not a priority.

  +struct rtl8xxxu_priv {
  []
  + u32 has_wifi:1;
  + u32 has_bluetooth:1;
  + u32 enable_bluetooth:1;
  + u32 has_gps:1;
  + u32 vendor_umc:1;
  + u32 has_polarity_ctrl:1;
  + u32 has_eeprom:1;
  + u32 boot_eeprom:1;
  + u32 ep_tx_high_queue:1;
  + u32 ep_tx_normal_queue:1;
  + u32 ep_tx_low_queue:1;
  + u32 path_a_hi_power:1;
 
  These might be better as bool instead of packed bitfields.
 
 bool wastes 4 bytes in the struct, so no that would be worse.

 It's a used-once struct.  4 bytes, wow.

Once per device - but it all comes down to a matter of style and taste
of the developer in the end. You seem to be obsessed with imposing your
ideas on everybody, rather than actually looking for real bugs.

 If you have any *bugs* to report, I welcome those comments very much.
 However if you are just looking to nag, please do that somewhere else.

 Imperious much?

So far you have reported one real bug (the endian issue) and a couple of
trailing whitespace and messed up indentation issues. If you find more
*real* issues to report, I would welcome those reports very much.

However you are clearly on a mission to have the last word on every
patch posted to the kernel - how about looking at real bugs or writing
code instead, that would be a lot more constructive for the kernel as a
whole!

Jes
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Joe Perches]

On Mon, 2015-03-09 at 14:43 -0400, Jes Sorensen wrote:
 Joe Perches j...@perches.com writes:
  On Mon, 2015-03-09 at 13:00 -0400, jes.soren...@redhat.com wrote:
  This is an alternate driver for the Realtek 8723AU (rtl8723au) written
  from scratch utilizing the mac80211 stack.
 
  Mostly trivial comments:
 
  diff --git a/MAINTAINERS b/MAINTAINERS
  []
  +RTL8XXXU WIRELESS DRIVER (rtl8xxxu)
  +M:Jes Sorensen jes.soren...@redhat.com
  +L:linux-wireless@vger.kernel.org
  +W:http://intellinuxwireless.org
  +T:git git://git.Mkernel.org/pub/scm/linux/kernel/git/jes/linux.git
  +  git branch rtl8723au-mac80211
 
  please keep this on one line
 
 Lines are 80 characters, and it won't fit.

For code yes, for MAINTAINERS no.
There are many lines  80 chars there.
Please keep:
TYPE: entry
on single lines.

  +static void rtl8xxxu_phy_iqcalibrate(struct rtl8xxxu_priv *priv,
  +   int result[][8], int t, bool is_2t)
  +{
  +  struct device *dev = priv-udev-dev;
  +  u32 i, val32;
  +  int path_a_ok /*, path_b_ok */;
  +  int retry = 2;
  +
  +  u32 ADDA_REG[RTL8XXXU_ADDA_REGS] = {
  +  REG_FPGA0_XCD_SWITCH_CTRL, REG_BLUETOOTH,
  +  REG_RX_WAIT_CCA, REG_TX_CCK_RFON,
  +  REG_TX_CCK_BBON, REG_TX_OFDM_RFON,
  +  REG_TX_OFDM_BBON, REG_TX_TO_RX,
  +  REG_TX_TO_TX, REG_RX_CCK,
  +  REG_RX_OFDM, REG_RX_WAIT_RIFS,
  +  REG_RX_TO_RX, REG_STANDBY,
  +  REG_SLEEP, REG_PMPD_ANAEN
  +  };
 
  static const is generally better.
 
 It's irrelevant

Not really, it requires an initialization on entry
and would make the object code smaller to use const.

  +struct rtl8xxxu_priv {
  []
  +  u32 has_wifi:1;
  +  u32 has_bluetooth:1;
  +  u32 enable_bluetooth:1;
  +  u32 has_gps:1;
  +  u32 vendor_umc:1;
  +  u32 has_polarity_ctrl:1;
  +  u32 has_eeprom:1;
  +  u32 boot_eeprom:1;
  +  u32 ep_tx_high_queue:1;
  +  u32 ep_tx_normal_queue:1;
  +  u32 ep_tx_low_queue:1;
  +  u32 path_a_hi_power:1;
 
  These might be better as bool instead of packed bitfields.
 
 bool wastes 4 bytes in the struct, so no that would be worse.

It's a used-once struct.  4 bytes, wow.

 If you have any *bugs* to report, I welcome those comments very much.
 However if you are just looking to nag, please do that somewhere else.

Imperious much?

--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Larry Finger]

On 03/06/2015 04:15 PM, jes.soren...@redhat.com wrote:

From: Jes Sorensen jes.soren...@redhat.com

This is an alternate driver for the Realtek 8723AU (rtl8723au) written
from scratch utilizing the mac80211 stack.

After spending months cleaning up the vendor provided rtl8723au
driver, which comes with it's own 802.11 stack included, I decided to
rewrite this driver from the bottom up.

Many thanks to Johannes Berg for 802.11 insights and help and Larry
Finger for help with the vendor driver.

The full git log for the development of this driver can be found here:
git git://git.kernel.org/pub/scm/linux/kernel/git/jes/linux.git
 branch rtl8723au-mac80211

This driver is still experimental, but has proven to be rather stable
for me. It lacks some features found in the staging driver, such as
power management, AMPDU, and 40MHz channel support. In addition there
is no AP and monitor support at this point.

Signed-off-by: Jes Sorensen jes.soren...@redhat.com


Quilt reports the following when this patch is refreshed:

Warning: trailing whitespace in lines 2862,3273,3492,3521 of 
drivers/net/wireless/rtl8xxxu.c


I have not analyzed all the temporary manipulations of rtl8xxxu_debug to see 
what you are doing; however, I suggest that you add a module parameter so that 
debugging can be enabled without rebuilding the module. That way a user who is 
using a distro binary can enable debugging without the hassle of a full kernel 
rebuild.


Because this driver is under drivers/net, checkpatch.pl adds additional tests 
that may not be used in other trees. For instance, it complains when it finds a 
block comment that starts with a bare /*. What is usually done is to use /** 
instead.


I think I understand why some of the #if 0 blocks are present, but others are 
not clear. For example, I see no value of keeping code that is labelled only 
for PCIe. Is it your intention to add the RTL8723AE to this driver?


Running checkpatch.pl on this patch results in total of 24 errors, 99 warnings, 
and 105 checks. From your mail exchange with Joe Perches, I understand that you 
will not wish to fix all of these; however, the errors should be handled. The 
fewer of the warnings or checks that are left, the better, if only to prevent 
interference from the script kiddies.


I like the clean look of the code. That is particularly impressive given the 
look of the original. I wish I had the hardware on which to test it. Perhaps I 
can wedge it into a kernel version that builds and runs on my Radxa Rock,


Larry

--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Joe Perches]

On Fri, 2015-03-06 at 17:15 -0500, jes.soren...@redhat.com wrote:
 From: Jes Sorensen jes.soren...@redhat.com
 
 This is an alternate driver for the Realtek 8723AU (rtl8723au) written
 from scratch utilizing the mac80211 stack.
 
 After spending months cleaning up the vendor provided rtl8723au
 driver, which comes with it's own 802.11 stack included, I decided to
 rewrite this driver from the bottom up.
 
 Many thanks to Johannes Berg for 802.11 insights and help and Larry
 Finger for help with the vendor driver.

cool.

I hope it can be fairly modular so that it can be
extensible to support multiple devices more sensibly
than the realtek code.

btw:  Here are some trivial checkpatch cleanups.

There are a few places where it seems there are
missing braces because indentation changes occur.

see:
@@ -1245,11 +1242,11 @@ static int rtl8xxxu_read_efuse(struct rtl8xxxu_priv 
*priv)
@@ -2540,10 +2534,10 @@ void rtl8xxxu_set_ampdu_factor(struct rtl8xxxu_priv 
*priv, u8 ampdu_factor)

and

+++ b/drivers/net/wireless/rtl8xxxu.h
[]
 struct rtl8723au_idx {
-#if defined (__LITTLE_ENDIAN)
+#if defined(__LITTLE_ENDIAN)
int a:4;
int b:4;
-#elif defined (__LITTLE_ENDIAN)
+#elif defined(__LITTLE_ENDIAN)
int b:4;
int a:4;

Presumably the second should be __BIG_ENDIAN.

---
 drivers/net/wireless/rtl8xxxu.c  | 124 ---
 drivers/net/wireless/rtl8xxxu.h  |   9 ++-
 drivers/net/wireless/rtl8xxxu_regs.h |  22 ++-
 3 files changed, 69 insertions(+), 86 deletions(-)

diff --git a/drivers/net/wireless/rtl8xxxu.c b/drivers/net/wireless/rtl8xxxu.c
index 6728a74..af1c44d 100644
--- a/drivers/net/wireless/rtl8xxxu.c
+++ b/drivers/net/wireless/rtl8xxxu.c
@@ -677,8 +677,8 @@ static int rtl8723a_h2c_cmd(struct rtl8xxxu_priv *priv, 
struct h2c_cmd *h2c)
pr_debug(H2C_EXT %04x\n, le16_to_cpu(h2c-raw.ext));
}
rtl8723au_write32(priv, mbox_reg, le32_to_cpu(h2c-raw.data));
-   if (rtl8xxxu_debug  RTL8XXXU_DEBUG_H2C)
-   pr_debug(H2C %08x\n, le16_to_cpu(h2c-raw.data));
+   if (rtl8xxxu_debug  RTL8XXXU_DEBUG_H2C)
+   pr_debug(H2C %08x\n, le16_to_cpu(h2c-raw.data));
 
priv-next_mbox = (mbox_nr + 1) % H2C_MAX_MBOX;
 
@@ -757,7 +757,6 @@ static void rtl8723a_disable_rf(struct rtl8xxxu_priv *priv)
rtl8723au_write8(priv, REG_SPS0_CTRL, sps0);
 }
 
-
 static void rtl8723a_stop_tx_beacon(struct rtl8xxxu_priv *priv)
 {
u8 val8;
@@ -774,7 +773,6 @@ static void rtl8723a_stop_tx_beacon(struct rtl8xxxu_priv 
*priv)
rtl8723au_write8(priv, REG_TBTT_PROHIBIT + 2, val8);
 }
 
-
 /*
  * The rtl8723a has 3 channel groups for it's efuse settings. It only
  * supports the 2.4GHz band, so channels 1 - 14:
@@ -834,9 +832,9 @@ static void rtl8723au_config_channel(struct ieee80211_hw 
*hw)
rtl8723au_write32(priv, REG_FPGA0_ANALOG2, val32);
break;
case NL80211_CHAN_WIDTH_40:
-if (hw-conf.chandef.center_freq1 
+   if (hw-conf.chandef.center_freq1 
hw-conf.chandef.chan-center_freq)
-sec_ch_above = 1;
+   sec_ch_above = 1;
else
sec_ch_above = 0;
 
@@ -935,8 +933,7 @@ rtl8723a_set_tx_power(struct rtl8xxxu_priv *priv, int 
channel, bool ht40)
}
 
if (rtl8xxxu_debug  RTL8XXXU_DEBUG_CHANNEL)
-   pr_debug(%s: Setting TX power CCK A: %02x, 
-CCK B: %02x, OFDM A: %02x, OFDM B: %02x\n,
+   pr_debug(%s: Setting TX power CCK A: %02x, CCK B: %02x, OFDM 
A: %02x, OFDM B: %02x\n,
 DRIVER_NAME, cck[0], cck[1], ofdm[0], ofdm[1]);
 
for (i = 0; i  RTL8723A_MAX_RF_PATHS; i++) {
@@ -977,9 +974,9 @@ rtl8723a_set_tx_power(struct rtl8xxxu_priv *priv, int 
channel, bool ht40)
mcsbase[1] += efuse-ht20_tx_power_index_diff[group].b;
 
ofdm_a = ofdmbase[0] | ofdmbase[0]  8 |
-   ofdmbase[0]  16 | ofdmbase[0] 24;
+   ofdmbase[0]  16 | ofdmbase[0]  24;
ofdm_b = ofdmbase[1] | ofdmbase[1]  8 |
-   ofdmbase[1]  16 | ofdmbase[1] 24;
+   ofdmbase[1]  16 | ofdmbase[1]  24;
rtl8723au_write32(priv, REG_TX_AGC_A_RATE18_06, ofdm_a);
rtl8723au_write32(priv, REG_TX_AGC_B_RATE18_06, ofdm_b);
 
@@ -987,9 +984,9 @@ rtl8723a_set_tx_power(struct rtl8xxxu_priv *priv, int 
channel, bool ht40)
rtl8723au_write32(priv, REG_TX_AGC_B_RATE54_24, ofdm_b);
 
mcs_a = mcsbase[0] | mcsbase[0]  8 |
-   mcsbase[0]  16 | mcsbase[0] 24;
+   mcsbase[0]  16 | mcsbase[0]  24;
mcs_b = mcsbase[1] | mcsbase[1]  8 |
-   mcsbase[1]  16 | mcsbase[1] 24;
+   mcsbase[1]  16 | mcsbase[1]  24;
 
rtl8723au_write32(priv, REG_TX_AGC_A_MCS03_MCS00, mcs_a);
rtl8723au_write32(priv, 

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Jes Sorensen]

Joe Perches j...@perches.com writes:
 On Sat, 2015-03-07 at 00:18 -0500, Jes Sorensen wrote:
 The long strings are an outright nack.

 Pity.  There's a specific exclusion in CodingStyle.

   Chapter 2: Breaking long lines and strings

 never break user-visible strings such as
 printk messages, because that breaks the ability to grep for them.

I know some people like this, and I disagree with this rule! I wrote
this driver, and I am the one maintaining it.

I love receiving fixes for bugs, but this is not a bug!

If you wrote a piece of code, I wouldn't question how you prefer to
maintain it.

Jes
--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Joe Perches]

On Sat, 2015-03-07 at 00:18 -0500, Jes Sorensen wrote:
 The long strings are an outright nack.

Pity.  There's a specific exclusion in CodingStyle.

Chapter 2: Breaking long lines and strings

never break user-visible strings such as
printk messages, because that breaks the ability to grep for them.

--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] New driver: rtl8723au (mac80211)

[Jes Sorensen]

Joe Perches j...@perches.com writes:
 On Fri, 2015-03-06 at 17:15 -0500, jes.soren...@redhat.com wrote:
 From: Jes Sorensen jes.soren...@redhat.com
 
 This is an alternate driver for the Realtek 8723AU (rtl8723au) written
 from scratch utilizing the mac80211 stack.
 
 After spending months cleaning up the vendor provided rtl8723au
 driver, which comes with it's own 802.11 stack included, I decided to
 rewrite this driver from the bottom up.
 
 Many thanks to Johannes Berg for 802.11 insights and help and Larry
 Finger for help with the vendor driver.

 cool.

 I hope it can be fairly modular so that it can be
 extensible to support multiple devices more sensibly
 than the realtek code.

 btw:  Here are some trivial checkpatch cleanups.

 There are a few places where it seems there are
 missing braces because indentation changes occur.

 see:
 @@ -1245,11 +1242,11 @@ static int rtl8xxxu_read_efuse(struct rtl8xxxu_priv 
 *priv)
 @@ -2540,10 +2534,10 @@ void rtl8xxxu_set_ampdu_factor(struct rtl8xxxu_priv 
 *priv, u8 ampdu_factor)

 and

 +++ b/drivers/net/wireless/rtl8xxxu.h
 []
  struct rtl8723au_idx {
 -#if defined (__LITTLE_ENDIAN)
 +#if defined(__LITTLE_ENDIAN)
   int a:4;
   int b:4;
 -#elif defined (__LITTLE_ENDIAN)
 +#elif defined(__LITTLE_ENDIAN)
   int b:4;
   int a:4;

 Presumably the second should be __BIG_ENDIAN.

Seems reasonable :)


 ---
  drivers/net/wireless/rtl8xxxu.c  | 124 
 ---
  drivers/net/wireless/rtl8xxxu.h  |   9 ++-
  drivers/net/wireless/rtl8xxxu_regs.h |  22 ++-
  3 files changed, 69 insertions(+), 86 deletions(-)

 diff --git a/drivers/net/wireless/rtl8xxxu.c b/drivers/net/wireless/rtl8xxxu.c
 index 6728a74..af1c44d 100644
 --- a/drivers/net/wireless/rtl8xxxu.c
 +++ b/drivers/net/wireless/rtl8xxxu.c
 @@ -677,8 +677,8 @@ static int rtl8723a_h2c_cmd(struct rtl8xxxu_priv *priv, 
 struct h2c_cmd *h2c)
   pr_debug(H2C_EXT %04x\n, le16_to_cpu(h2c-raw.ext));
   }
   rtl8723au_write32(priv, mbox_reg, le32_to_cpu(h2c-raw.data));
 - if (rtl8xxxu_debug  RTL8XXXU_DEBUG_H2C)
 - pr_debug(H2C %08x\n, le16_to_cpu(h2c-raw.data));
 + if (rtl8xxxu_debug  RTL8XXXU_DEBUG_H2C)
 + pr_debug(H2C %08x\n, le16_to_cpu(h2c-raw.data));
  
   priv-next_mbox = (mbox_nr + 1) % H2C_MAX_MBOX;
  
 @@ -757,7 +757,6 @@ static void rtl8723a_disable_rf(struct rtl8xxxu_priv 
 *priv)
   rtl8723au_write8(priv, REG_SPS0_CTRL, sps0);
  }
  
 -
  static void rtl8723a_stop_tx_beacon(struct rtl8xxxu_priv *priv)
  {
   u8 val8;
 @@ -774,7 +773,6 @@ static void rtl8723a_stop_tx_beacon(struct rtl8xxxu_priv 
 *priv)
   rtl8723au_write8(priv, REG_TBTT_PROHIBIT + 2, val8);
  }
  
 -
  /*
   * The rtl8723a has 3 channel groups for it's efuse settings. It only
   * supports the 2.4GHz band, so channels 1 - 14:

I like double blank lines in some places, so no to that one.

 @@ -834,9 +832,9 @@ static void rtl8723au_config_channel(struct ieee80211_hw 
 *hw)
   rtl8723au_write32(priv, REG_FPGA0_ANALOG2, val32);
   break;
   case NL80211_CHAN_WIDTH_40:
 -if (hw-conf.chandef.center_freq1 
 + if (hw-conf.chandef.center_freq1 
   hw-conf.chandef.chan-center_freq)
 -sec_ch_above = 1;
 + sec_ch_above = 1;
   else
   sec_ch_above = 0;
  
 @@ -935,8 +933,7 @@ rtl8723a_set_tx_power(struct rtl8xxxu_priv *priv, int 
 channel, bool ht40)
   }
  
   if (rtl8xxxu_debug  RTL8XXXU_DEBUG_CHANNEL)
 - pr_debug(%s: Setting TX power CCK A: %02x, 
 -  CCK B: %02x, OFDM A: %02x, OFDM B: %02x\n,
 + pr_debug(%s: Setting TX power CCK A: %02x, CCK B: %02x, OFDM 
 A: %02x, OFDM B: %02x\n,
DRIVER_NAME, cck[0], cck[1], ofdm[0], ofdm[1]);
  
   for (i = 0; i  RTL8723A_MAX_RF_PATHS; i++) {

NAK, I break my lines carefully  80 character lines suck.

 @@ -977,9 +974,9 @@ rtl8723a_set_tx_power(struct rtl8xxxu_priv *priv, int 
 channel, bool ht40)
   mcsbase[1] += efuse-ht20_tx_power_index_diff[group].b;
  
   ofdm_a = ofdmbase[0] | ofdmbase[0]  8 |
 - ofdmbase[0]  16 | ofdmbase[0] 24;
 + ofdmbase[0]  16 | ofdmbase[0]  24;
   ofdm_b = ofdmbase[1] | ofdmbase[1]  8 |
 - ofdmbase[1]  16 | ofdmbase[1] 24;
 + ofdmbase[1]  16 | ofdmbase[1]  24;
   rtl8723au_write32(priv, REG_TX_AGC_A_RATE18_06, ofdm_a);
   rtl8723au_write32(priv, REG_TX_AGC_B_RATE18_06, ofdm_b);
  
 @@ -987,9 +984,9 @@ rtl8723a_set_tx_power(struct rtl8xxxu_priv *priv, int 
 channel, bool ht40)
   rtl8723au_write32(priv, REG_TX_AGC_B_RATE54_24, ofdm_b);
  
   mcs_a = mcsbase[0] | mcsbase[0]  8 |
 - mcsbase[0]  16 | mcsbase[0] 24;
 + mcsbase[0]  16 | mcsbase[0]  24;
   mcs_b = mcsbase[1] | mcsbase[1]  8 |
 -