mbox series

[v3,00/13] rtw88: mac80211 driver for Realtek 802.11ac wireless network chips

Message ID 1548654407-28469-1-git-send-email-yhchuang@realtek.com (mailing list archive)
Headers show
Series rtw88: mac80211 driver for Realtek 802.11ac wireless network chips | expand

Message

Tony Chuang Jan. 28, 2019, 5:46 a.m. UTC
From: Yan-Hsuan Chuang <yhchuang@realtek.com>

This is a new mac80211 driver for Realtek 802.11ac wireless network chips.
rtw88 supports 8822BE and 8822CE chips, and will be able to support
multi-vif combinations in run-time.

For now, only PCI bus is supported, but rtw88 was originally designed
to optionally support three buses includes USB & SDIO. USB & SDIO modules
will soon be supported by rtw88, with configurable core module to fit
with different bus modules in the same time.

For example, if we choose 8822BE and 8822CU, only PCI & USB modules will
be selected, built, loaded into kernel. This is one of the major
difference from rtlwifi, which can only support specific combinations.

Another difference from rtlwifi is that rtw88 is designed to support
the latest Realtek 802.11ac wireless network chips like 8822B and
8822C series. Compared to the earlier chips supported by rtlwifi like
the 802.11n 8192EE chipset or 802.11ac 8821AE/8812AE chips, newer ICs
have different MAC & PHY settings, such as new multi-port feature for the
MAC layer design and Jaguar2/Jaguar3 PHY layer IPs.

Multi-Port feature is also supported under rtw88's software architecture.
rtlwifi can only support one vif in the same time, most because of the
hardware limitations for early chips, hence the original design of it
also restricts the usage of multi-vif support, so latest chipset seems not
take advantages from its new MAC engine.

However, rtw88 can run multiple vifs concurrently by holding them on
hardware ports provided by MAC engine, hence can easily start different
roles on a single device.

Based on the reasons mentioned before, we implemented rtw88. It had many
authors, they are listed here alphabetically:

Ping-Ke Shih <pkshih@realtek.com>
Tzu-En Huang <tehuang@realtek.com>
Yan-Hsuan Chuang <yhchuang@realtek.com>


v2

 - add comment for watch dog


v3

 - change tree location to wireless-next


Yan-Hsuan Chuang (13):
  rtw88: main files
  rtw88: core files
  rtw88: hci files
  rtw88: trx files
  rtw88: mac files
  rtw88: fw and efuse files
  rtw88: phy files
  rtw88: debug files
  rtw88: chip files
  rtw88: 8822B init table
  rtw88: 8822C init table
  rtw88: Kconfig & Makefile
  rtw88: add support for Realtek 802.11ac wireless chips

 MAINTAINERS                                        |     8 +
 drivers/net/wireless/realtek/Kconfig               |     1 +
 drivers/net/wireless/realtek/Makefile              |     1 +
 drivers/net/wireless/realtek/rtw88/Kconfig         |    55 +
 drivers/net/wireless/realtek/rtw88/Makefile        |    19 +
 drivers/net/wireless/realtek/rtw88/debug.c         |   631 +
 drivers/net/wireless/realtek/rtw88/debug.h         |    35 +
 drivers/net/wireless/realtek/rtw88/efuse.c         |   150 +
 drivers/net/wireless/realtek/rtw88/efuse.h         |    53 +
 drivers/net/wireless/realtek/rtw88/fw.c            |   611 +
 drivers/net/wireless/realtek/rtw88/fw.h            |   213 +
 drivers/net/wireless/realtek/rtw88/hci.h           |   211 +
 drivers/net/wireless/realtek/rtw88/mac.c           |  1010 +
 drivers/net/wireless/realtek/rtw88/mac.h           |    35 +
 drivers/net/wireless/realtek/rtw88/mac80211.c      |   480 +
 drivers/net/wireless/realtek/rtw88/main.c          |  1190 ++
 drivers/net/wireless/realtek/rtw88/main.h          |  1119 +
 drivers/net/wireless/realtek/rtw88/pci.c           |  1208 ++
 drivers/net/wireless/realtek/rtw88/pci.h           |   229 +
 drivers/net/wireless/realtek/rtw88/phy.c           |  1670 ++
 drivers/net/wireless/realtek/rtw88/phy.h           |   125 +
 drivers/net/wireless/realtek/rtw88/ps.c            |   165 +
 drivers/net/wireless/realtek/rtw88/ps.h            |    20 +
 drivers/net/wireless/realtek/rtw88/reg.h           |   411 +
 drivers/net/wireless/realtek/rtw88/regd.c          |   391 +
 drivers/net/wireless/realtek/rtw88/regd.h          |    67 +
 drivers/net/wireless/realtek/rtw88/rtw8822b.c      |  1590 ++
 drivers/net/wireless/realtek/rtw88/rtw8822b.h      |   155 +
 .../net/wireless/realtek/rtw88/rtw8822b_table.c    | 20783 +++++++++++++++++++
 .../net/wireless/realtek/rtw88/rtw8822b_table.h    |    18 +
 drivers/net/wireless/realtek/rtw88/rtw8822c.c      |  1169 ++
 drivers/net/wireless/realtek/rtw88/rtw8822c.h      |   171 +
 .../net/wireless/realtek/rtw88/rtw8822c_table.c    |  4150 ++++
 .../net/wireless/realtek/rtw88/rtw8822c_table.h    |    16 +
 drivers/net/wireless/realtek/rtw88/rx.c            |   151 +
 drivers/net/wireless/realtek/rtw88/rx.h            |    41 +
 drivers/net/wireless/realtek/rtw88/sec.c           |   120 +
 drivers/net/wireless/realtek/rtw88/sec.h           |    39 +
 drivers/net/wireless/realtek/rtw88/tx.c            |   273 +
 drivers/net/wireless/realtek/rtw88/tx.h            |    81 +
 40 files changed, 38865 insertions(+)
 create mode 100644 drivers/net/wireless/realtek/rtw88/Kconfig
 create mode 100644 drivers/net/wireless/realtek/rtw88/Makefile
 create mode 100644 drivers/net/wireless/realtek/rtw88/debug.c
 create mode 100644 drivers/net/wireless/realtek/rtw88/debug.h
 create mode 100644 drivers/net/wireless/realtek/rtw88/efuse.c
 create mode 100644 drivers/net/wireless/realtek/rtw88/efuse.h
 create mode 100644 drivers/net/wireless/realtek/rtw88/fw.c
 create mode 100644 drivers/net/wireless/realtek/rtw88/fw.h
 create mode 100644 drivers/net/wireless/realtek/rtw88/hci.h
 create mode 100644 drivers/net/wireless/realtek/rtw88/mac.c
 create mode 100644 drivers/net/wireless/realtek/rtw88/mac.h
 create mode 100644 drivers/net/wireless/realtek/rtw88/mac80211.c
 create mode 100644 drivers/net/wireless/realtek/rtw88/main.c
 create mode 100644 drivers/net/wireless/realtek/rtw88/main.h
 create mode 100644 drivers/net/wireless/realtek/rtw88/pci.c
 create mode 100644 drivers/net/wireless/realtek/rtw88/pci.h
 create mode 100644 drivers/net/wireless/realtek/rtw88/phy.c
 create mode 100644 drivers/net/wireless/realtek/rtw88/phy.h
 create mode 100644 drivers/net/wireless/realtek/rtw88/ps.c
 create mode 100644 drivers/net/wireless/realtek/rtw88/ps.h
 create mode 100644 drivers/net/wireless/realtek/rtw88/reg.h
 create mode 100644 drivers/net/wireless/realtek/rtw88/regd.c
 create mode 100644 drivers/net/wireless/realtek/rtw88/regd.h
 create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8822b.c
 create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8822b.h
 create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8822b_table.c
 create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8822b_table.h
 create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8822c.c
 create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8822c.h
 create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8822c_table.c
 create mode 100644 drivers/net/wireless/realtek/rtw88/rtw8822c_table.h
 create mode 100644 drivers/net/wireless/realtek/rtw88/rx.c
 create mode 100644 drivers/net/wireless/realtek/rtw88/rx.h
 create mode 100644 drivers/net/wireless/realtek/rtw88/sec.c
 create mode 100644 drivers/net/wireless/realtek/rtw88/sec.h
 create mode 100644 drivers/net/wireless/realtek/rtw88/tx.c
 create mode 100644 drivers/net/wireless/realtek/rtw88/tx.h

Comments

Kalle Valo Jan. 28, 2019, 8:36 a.m. UTC | #1
<yhchuang@realtek.com> writes:

> From: Yan-Hsuan Chuang <yhchuang@realtek.com>
>
> This is a new mac80211 driver for Realtek 802.11ac wireless network chips.
> rtw88 supports 8822BE and 8822CE chips, and will be able to support
> multi-vif combinations in run-time.
>
> For now, only PCI bus is supported, but rtw88 was originally designed
> to optionally support three buses includes USB & SDIO. USB & SDIO modules
> will soon be supported by rtw88, with configurable core module to fit
> with different bus modules in the same time.
>
> For example, if we choose 8822BE and 8822CU, only PCI & USB modules will
> be selected, built, loaded into kernel. This is one of the major
> difference from rtlwifi, which can only support specific combinations.
>
> Another difference from rtlwifi is that rtw88 is designed to support
> the latest Realtek 802.11ac wireless network chips like 8822B and
> 8822C series. Compared to the earlier chips supported by rtlwifi like
> the 802.11n 8192EE chipset or 802.11ac 8821AE/8812AE chips, newer ICs
> have different MAC & PHY settings, such as new multi-port feature for the
> MAC layer design and Jaguar2/Jaguar3 PHY layer IPs.
>
> Multi-Port feature is also supported under rtw88's software architecture.
> rtlwifi can only support one vif in the same time, most because of the
> hardware limitations for early chips, hence the original design of it
> also restricts the usage of multi-vif support, so latest chipset seems not
> take advantages from its new MAC engine.
>
> However, rtw88 can run multiple vifs concurrently by holding them on
> hardware ports provided by MAC engine, hence can easily start different
> roles on a single device.
>
> Based on the reasons mentioned before, we implemented rtw88. It had many
> authors, they are listed here alphabetically:
>
> Ping-Ke Shih <pkshih@realtek.com>
> Tzu-En Huang <tehuang@realtek.com>
> Yan-Hsuan Chuang <yhchuang@realtek.com>
>
>
> v2
>
>  - add comment for watch dog
>
>
> v3
>
>  - change tree location to wireless-next

What does this mean? wireless-next has not been used for years, do you
mean wireless-drivers-next instead?
Tony Chuang Jan. 28, 2019, 9:25 a.m. UTC | #2
> -----Original Message-----
> From: Kalle Valo [mailto:kvalo@codeaurora.org]
> Sent: Monday, January 28, 2019 4:37 PM
> To: Tony Chuang
> Cc: johannes@sipsolutions.net; Larry.Finger@lwfinger.net; Pkshih; Andy Huang;
> briannorris@chromium.org; sgruszka@redhat.com;
> linux-wireless@vger.kernel.org
> Subject: Re: [PATCH v3 00/13] rtw88: mac80211 driver for Realtek 802.11ac
> wireless network chips
> 
> <yhchuang@realtek.com> writes:
> 
> > From: Yan-Hsuan Chuang <yhchuang@realtek.com>
> >
> > This is a new mac80211 driver for Realtek 802.11ac wireless network chips.
> > rtw88 supports 8822BE and 8822CE chips, and will be able to support
> > multi-vif combinations in run-time.
> >
> > For now, only PCI bus is supported, but rtw88 was originally designed
> > to optionally support three buses includes USB & SDIO. USB & SDIO modules
> > will soon be supported by rtw88, with configurable core module to fit
> > with different bus modules in the same time.
> >
> > For example, if we choose 8822BE and 8822CU, only PCI & USB modules will
> > be selected, built, loaded into kernel. This is one of the major
> > difference from rtlwifi, which can only support specific combinations.
> >
> > Another difference from rtlwifi is that rtw88 is designed to support
> > the latest Realtek 802.11ac wireless network chips like 8822B and
> > 8822C series. Compared to the earlier chips supported by rtlwifi like
> > the 802.11n 8192EE chipset or 802.11ac 8821AE/8812AE chips, newer ICs
> > have different MAC & PHY settings, such as new multi-port feature for the
> > MAC layer design and Jaguar2/Jaguar3 PHY layer IPs.
> >
> > Multi-Port feature is also supported under rtw88's software architecture.
> > rtlwifi can only support one vif in the same time, most because of the
> > hardware limitations for early chips, hence the original design of it
> > also restricts the usage of multi-vif support, so latest chipset seems not
> > take advantages from its new MAC engine.
> >
> > However, rtw88 can run multiple vifs concurrently by holding them on
> > hardware ports provided by MAC engine, hence can easily start different
> > roles on a single device.
> >
> > Based on the reasons mentioned before, we implemented rtw88. It had many
> > authors, they are listed here alphabetically:
> >
> > Ping-Ke Shih <pkshih@realtek.com>
> > Tzu-En Huang <tehuang@realtek.com>
> > Yan-Hsuan Chuang <yhchuang@realtek.com>
> >
> >
> > v2
> >
> >  - add comment for watch dog
> >
> >
> > v3
> >
> >  - change tree location to wireless-next
> 
> What does this mean? wireless-next has not been used for years, do you
> mean wireless-drivers-next instead?
> 
> --
> Kalle Valo

Sorry for the typo here. Yes, I mean " wireless-drivers-next ".
In MAINTAINERS file, I do change to "wireless-drivers-next" instead of "wireless-next".

Yan-Hsuan
Stanislaw Gruszka Jan. 28, 2019, 10:12 a.m. UTC | #3
On Mon, Jan 28, 2019 at 09:25:32AM +0000, Tony Chuang wrote:
> > >  - change tree location to wireless-next
> > 
> > What does this mean? wireless-next has not been used for years, do you
> > mean wireless-drivers-next instead?
> > 
> > --
> > Kalle Valo
> 
> Sorry for the typo here. Yes, I mean " wireless-drivers-next ".
> In MAINTAINERS file, I do change to "wireless-drivers-next" instead of "wireless-next".

The git entry in MAINTAINER file is not necessary and should point
the driver specific tree, which rtw88 do not have. Just remove this
entry. Also:

+W:     http://wireless.kernel.org/

is not right and should be removed or replaced by driver specific page.

Please also change topic of 13 patch. Is not "adding support for Realtek
802.11ac wireless chips",  but just adding MAINTAINER entry. 

Thanks
Stanislaw
Larry Finger Jan. 28, 2019, 8:56 p.m. UTC | #4
Tony,

I had not tested rtw88 for some time, so I built a kernel with the V3 patches 
and found that rtw88 crashed with a NULL pointer dereference. I did some 
debugging and found that the problem was in routine rtw_chip_efuse_enable() 
where fw->firmware was NULL.

Next I verified that rtw_load_firmware() had been called, which gave a clue that 
the firmware had not finished loading by the time rtw_chip_efuse_enable() was 
called. By adding a wait_for_completion() call before the 
rtw_download_firmware() call, the problem was fixed. The problem was a race 
between calling rtw_chip_efuse_enable() and the firmware load from disk. 
Obviously, the speed ratio between my CPU and the disk system is much different 
on my laptop than on your test machines, thus I have the problem.

The patch I used is attached.

Larry
diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
index a189c4500fd9..690c0a68a038 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -902,6 +902,7 @@ static int rtw_chip_efuse_enable(struct rtw_dev *rtwdev)
 		goto err;
 	}
 
+	wait_for_completion(&fw->completion);
 	rtw_write8(rtwdev, REG_C2HEVT, C2H_HW_FEATURE_DUMP);
 	ret = rtw_download_firmware(rtwdev, fw->firmware->data,
 				    fw->firmware->size);
Brian Norris Jan. 28, 2019, 10:38 p.m. UTC | #5
On Mon, Jan 28, 2019 at 2:16 AM Stanislaw Gruszka <sgruszka@redhat.com> wrote:
> On Mon, Jan 28, 2019 at 09:25:32AM +0000, Tony Chuang wrote:
> > Sorry for the typo here. Yes, I mean " wireless-drivers-next ".
> > In MAINTAINERS file, I do change to "wireless-drivers-next" instead of "wireless-next".
>
> The git entry in MAINTAINER file is not necessary and should point
> the driver specific tree, which rtw88 do not have. Just remove this
> entry.

Yes, it seems Tony mis-interpreted my suggestion when I said he was
using the wrong tree. I agree this driver does not need a git entry at
all.

Regards,
Brian

P.S. I do plan to give this driver a spin this week and hopefully give
it a little closer review. No guarantees I actually get the time for
that, but I hope to...
Tony Chuang Jan. 29, 2019, 2:15 a.m. UTC | #6
> Tony,
> 
> I had not tested rtw88 for some time, so I built a kernel with the V3 patches and
> found that rtw88 crashed with a NULL pointer dereference. I did some
> debugging and found that the problem was in routine rtw_chip_efuse_enable()
> where fw->firmware was NULL.
> 

Hi Larry,

This NULL pointer was found months ago and has been fixed already.
Thanks for your test :).
I am holding the patch to fix it for the next patchsets.

BTW, since rtw88 has not been accepted, could I send next patch set based on
this patch set as long as I explicitly mark that the next patch is based on the previous one?
Thanks!

Yan-Hsuan
Tony Chuang Jan. 29, 2019, 2:27 a.m. UTC | #7
> -----Original Message-----
> From: Brian Norris [mailto:briannorris@chromium.org]
> Sent: Tuesday, January 29, 2019 6:39 AM
> To: Stanislaw Gruszka
> Cc: Tony Chuang; Kalle Valo; johannes@sipsolutions.net;
> Larry.Finger@lwfinger.net; Pkshih; Andy Huang; linux-wireless@vger.kernel.org
> Subject: Re: [PATCH v3 00/13] rtw88: mac80211 driver for Realtek 802.11ac
> wireless network chips
> 
> On Mon, Jan 28, 2019 at 2:16 AM Stanislaw Gruszka <sgruszka@redhat.com>
> wrote:
> > On Mon, Jan 28, 2019 at 09:25:32AM +0000, Tony Chuang wrote:
> > > Sorry for the typo here. Yes, I mean " wireless-drivers-next ".
> > > In MAINTAINERS file, I do change to "wireless-drivers-next" instead of
> "wireless-next".
> >
> > The git entry in MAINTAINER file is not necessary and should point
> > the driver specific tree, which rtw88 do not have. Just remove this
> > entry.
> 
> Yes, it seems Tony mis-interpreted my suggestion when I said he was
> using the wrong tree. I agree this driver does not need a git entry at
> all.

Sorry for that, now rtw88 indeed does not have an own tree other than wireless-drivers.
And Kalle can just remove it.

> 
> Regards,
> Brian
> 
> P.S. I do plan to give this driver a spin this week and hopefully give
> it a little closer review. No guarantees I actually get the time for
> that, but I hope to...


Thanks for your review.
Just point it out if I have done anything wrong or inappropriate.


Yan-Hsuan
Brian Norris Jan. 29, 2019, 2:41 a.m. UTC | #8
On Mon, Jan 28, 2019 at 6:15 PM Tony Chuang <yhchuang@realtek.com> wrote:
> This NULL pointer was found months ago and has been fixed already.
> Thanks for your test :).
> I am holding the patch to fix it for the next patchsets.
>
> BTW, since rtw88 has not been accepted, could I send next patch set based on
> this patch set as long as I explicitly mark that the next patch is based on the previous one?

I'd normally expect that if you find major bugs in your initial driver
submission that still isn't reviewed/merged, you might as well just
roll the fix into latest version and describe it in the
cover-letter/changelog. This particular change is so trivial it
doesn't really seem to deserve a separate patch.

(It would also help people like me, who may very well run into the
same bug when they get around to testing/reviewing the driver.)

I also don't know what the contents of the "next patch set" is -- if
it's a lot of new features, maybe they don't deserve to clutter the
initial submission, but if they're bugfixes like this, it seems like
you could just fix the original patch set.

But that's just my opinion.

Brian
Tony Chuang Jan. 29, 2019, 2:53 a.m. UTC | #9
> -----Original Message-----
> From: Brian Norris [mailto:briannorris@chromium.org]
> Sent: Tuesday, January 29, 2019 10:41 AM
> To: Tony Chuang
> Cc: Larry Finger; Pkshih; Andy Huang; sgruszka@redhat.com;
> linux-wireless@vger.kernel.org
> Subject: Re: [PATCH v3 00/13] rtw88: mac80211 driver for Realtek 802.11ac
> wireless network chips
> 
> On Mon, Jan 28, 2019 at 6:15 PM Tony Chuang <yhchuang@realtek.com>
> wrote:
> > This NULL pointer was found months ago and has been fixed already.
> > Thanks for your test :).
> > I am holding the patch to fix it for the next patchsets.
> >
> > BTW, since rtw88 has not been accepted, could I send next patch set based on
> > this patch set as long as I explicitly mark that the next patch is based on the
> previous one?
> 
> I'd normally expect that if you find major bugs in your initial driver
> submission that still isn't reviewed/merged, you might as well just
> roll the fix into latest version and describe it in the
> cover-letter/changelog. This particular change is so trivial it
> doesn't really seem to deserve a separate patch.
> 
> (It would also help people like me, who may very well run into the
> same bug when they get around to testing/reviewing the driver.)
> 
> I also don't know what the contents of the "next patch set" is -- if
> it's a lot of new features, maybe they don't deserve to clutter the
> initial submission, but if they're bugfixes like this, it seems like
> you could just fix the original patch set.


It is because that the next patch set contains several major fixes.
And I don't know if it will bother the reviewers if I apply them into
the original patch set.

And I don't know if I can send the next patch set that is based on the
patch set have not been accepted. Just wonder which way is better.

So, if it is how things work I can do this and resend with those fixes.
It is actually better for me to fix them in the original patch set.
Or send the next patch set now and mark it is based on the original
patch set under reviewing.


> 
> But that's just my opinion.

Your opinion is helpful.
Thanks for your suggestion.

> 
> Brian
> 

Yan-Hsuan
Larry Finger Jan. 29, 2019, 3:23 a.m. UTC | #10
On 1/28/19 8:15 PM, Tony Chuang wrote:
>> Tony,
>>
>> I had not tested rtw88 for some time, so I built a kernel with the V3 patches and
>> found that rtw88 crashed with a NULL pointer dereference. I did some
>> debugging and found that the problem was in routine rtw_chip_efuse_enable()
>> where fw->firmware was NULL.
>>
> 
> Hi Larry,
> 
> This NULL pointer was found months ago and has been fixed already.
> Thanks for your test :).
> I am holding the patch to fix it for the next patchsets.
> 
> BTW, since rtw88 has not been accepted, could I send next patch set based on
> this patch set as long as I explicitly mark that the next patch is based on the previous one?
> Thanks!

You need to have that patch in whatever version is merged into the wireless 
tree. It would not look very good for the initial version to crash every users 
computer.

Yes, I would merge EVERY patch that you have pending into the source and submit V4.

Larry
Tony Chuang Jan. 29, 2019, 4:03 a.m. UTC | #11
> -----Original Message-----
> From: linux-wireless-owner@vger.kernel.org
> [mailto:linux-wireless-owner@vger.kernel.org] On Behalf Of Larry Finger
> Sent: Tuesday, January 29, 2019 11:24 AM
> To: Tony Chuang
> Cc: Pkshih; Andy Huang; briannorris@chromium.org; sgruszka@redhat.com;
> linux-wireless@vger.kernel.org
> Subject: Re: [PATCH v3 00/13] rtw88: mac80211 driver for Realtek 802.11ac
> wireless network chips
> 
> On 1/28/19 8:15 PM, Tony Chuang wrote:
> >> Tony,
> >>
> >> I had not tested rtw88 for some time, so I built a kernel with the V3 patches
> and
> >> found that rtw88 crashed with a NULL pointer dereference. I did some
> >> debugging and found that the problem was in routine
> rtw_chip_efuse_enable()
> >> where fw->firmware was NULL.
> >>
> >
> > Hi Larry,
> >
> > This NULL pointer was found months ago and has been fixed already.
> > Thanks for your test :).
> > I am holding the patch to fix it for the next patchsets.
> >
> > BTW, since rtw88 has not been accepted, could I send next patch set based on
> > this patch set as long as I explicitly mark that the next patch is based on the
> previous one?
> > Thanks!
> 
> You need to have that patch in whatever version is merged into the wireless
> tree. It would not look very good for the initial version to crash every users
> computer.
> 
> Yes, I would merge EVERY patch that you have pending into the source and
> submit V4.

Hi Larry,

But here I am holding almost 40 patches, some of them are common major fixes.
The rest of them are to enhance and stabilize 8822C.

From the initial submit to now, 8822C has many fixes. Because we tested
a lot for developing 8822C and we have many parameter changes for it.
I am not sure should I merge every patch into the original patch set. We will
have huge difference to the original patch set, means extra effort for review.

But I can filter out the less important patches (will be like around 20, still many).
How do you think?
Thanks.

Yan-Hsuan
Kalle Valo Jan. 29, 2019, 7:53 a.m. UTC | #12
Brian Norris <briannorris@chromium.org> writes:

> On Mon, Jan 28, 2019 at 6:15 PM Tony Chuang <yhchuang@realtek.com> wrote:
>> This NULL pointer was found months ago and has been fixed already.
>> Thanks for your test :).
>> I am holding the patch to fix it for the next patchsets.
>>
>> BTW, since rtw88 has not been accepted, could I send next patch set based on
>> this patch set as long as I explicitly mark that the next patch is based on the previous one?
>
> I'd normally expect that if you find major bugs in your initial driver
> submission that still isn't reviewed/merged, you might as well just
> roll the fix into latest version and describe it in the
> cover-letter/changelog. This particular change is so trivial it
> doesn't really seem to deserve a separate patch.
>
> (It would also help people like me, who may very well run into the
> same bug when they get around to testing/reviewing the driver.)
>
> I also don't know what the contents of the "next patch set" is -- if
> it's a lot of new features, maybe they don't deserve to clutter the
> initial submission, but if they're bugfixes like this, it seems like
> you could just fix the original patch set.

This is what I suggest as well. Don't add new features or any other
large changes while the driver is under review, instead keep them in a
separate branch etc. But bugfixes are ok.
Kalle Valo Jan. 29, 2019, 7:59 a.m. UTC | #13
Tony Chuang <yhchuang@realtek.com> writes:

>> -----Original Message-----
>> From: linux-wireless-owner@vger.kernel.org
>> [mailto:linux-wireless-owner@vger.kernel.org] On Behalf Of Larry Finger
>> Sent: Tuesday, January 29, 2019 11:24 AM
>> To: Tony Chuang
>> Cc: Pkshih; Andy Huang; briannorris@chromium.org; sgruszka@redhat.com;
>> linux-wireless@vger.kernel.org
>> Subject: Re: [PATCH v3 00/13] rtw88: mac80211 driver for Realtek 802.11ac
>> wireless network chips
>> 
>> On 1/28/19 8:15 PM, Tony Chuang wrote:
>> >> Tony,
>> >>
>> >> I had not tested rtw88 for some time, so I built a kernel with the V3 patches
>> and
>> >> found that rtw88 crashed with a NULL pointer dereference. I did some
>> >> debugging and found that the problem was in routine
>> rtw_chip_efuse_enable()
>> >> where fw->firmware was NULL.
>> >>
>> >
>> > Hi Larry,
>> >
>> > This NULL pointer was found months ago and has been fixed already.
>> > Thanks for your test :).
>> > I am holding the patch to fix it for the next patchsets.
>> >
>> > BTW, since rtw88 has not been accepted, could I send next patch set based on
>> > this patch set as long as I explicitly mark that the next patch is based on the
>> previous one?
>> > Thanks!
>> 
>> You need to have that patch in whatever version is merged into the wireless
>> tree. It would not look very good for the initial version to crash every users
>> computer.
>> 
>> Yes, I would merge EVERY patch that you have pending into the source and
>> submit V4.
>
> Hi Larry,
>
> But here I am holding almost 40 patches, some of them are common major fixes.
> The rest of them are to enhance and stabilize 8822C.
>
> From the initial submit to now, 8822C has many fixes. Because we tested
> a lot for developing 8822C and we have many parameter changes for it.
> I am not sure should I merge every patch into the original patch set. We will
> have huge difference to the original patch set, means extra effort for
> review.

Yeah, it's very good that you try to keep the changes to minimum while
the driver is under review.

> But I can filter out the less important patches (will be like around 20, still many).
> How do you think?

I suggest to look at criticality of the bug and size of the patch. For
example, if the bug is a minor and the patch is large you should
definitely drop that. And smaller fixes to severe bugs you should
definitely again include them.

Just remember to list in the changelog every change you made since
previous version.
Tony Chuang Jan. 29, 2019, 1:01 p.m. UTC | #14
> I suggest to look at criticality of the bug and size of the patch. For
> example, if the bug is a minor and the patch is large you should
> definitely drop that. And smaller fixes to severe bugs you should
> definitely again include them.
> 
> Just remember to list in the changelog every change you made since
> previous version.
> 

Hi Kalle,

One more question, should I merge the trivial patches into the original
patch set and resend them like [PATCH v4 00/13].

Or should I add them after the 13 patches for people to review easily.
Such as [PATCH v4 00/21]. Which do you prefer?
Thanks.

Yan-Hsuan
Larry Finger Jan. 29, 2019, 2:54 p.m. UTC | #15
On 1/28/19 10:03 PM, Tony Chuang wrote:
> 
> 
>> -----Original Message-----
>> From: linux-wireless-owner@vger.kernel.org
>> [mailto:linux-wireless-owner@vger.kernel.org] On Behalf Of Larry Finger
>> Sent: Tuesday, January 29, 2019 11:24 AM
>> To: Tony Chuang
>> Cc: Pkshih; Andy Huang; briannorris@chromium.org; sgruszka@redhat.com;
>> linux-wireless@vger.kernel.org
>> Subject: Re: [PATCH v3 00/13] rtw88: mac80211 driver for Realtek 802.11ac
>> wireless network chips
>>
>> On 1/28/19 8:15 PM, Tony Chuang wrote:
>>>> Tony,
>>>>
>>>> I had not tested rtw88 for some time, so I built a kernel with the V3 patches
>> and
>>>> found that rtw88 crashed with a NULL pointer dereference. I did some
>>>> debugging and found that the problem was in routine
>> rtw_chip_efuse_enable()
>>>> where fw->firmware was NULL.
>>>>
>>>
>>> Hi Larry,
>>>
>>> This NULL pointer was found months ago and has been fixed already.
>>> Thanks for your test :).
>>> I am holding the patch to fix it for the next patchsets.
>>>
>>> BTW, since rtw88 has not been accepted, could I send next patch set based on
>>> this patch set as long as I explicitly mark that the next patch is based on the
>> previous one?
>>> Thanks!
>>
>> You need to have that patch in whatever version is merged into the wireless
>> tree. It would not look very good for the initial version to crash every users
>> computer.
>>
>> Yes, I would merge EVERY patch that you have pending into the source and
>> submit V4.
> 
> Hi Larry,
> 
> But here I am holding almost 40 patches, some of them are common major fixes.
> The rest of them are to enhance and stabilize 8822C.
> 
>  From the initial submit to now, 8822C has many fixes. Because we tested
> a lot for developing 8822C and we have many parameter changes for it.
> I am not sure should I merge every patch into the original patch set. We will
> have huge difference to the original patch set, means extra effort for review.
> 
> But I can filter out the less important patches (will be like around 20, still many).
> How do you think?
> Thanks.

As a minimum, you need to add all patches that fix crashes.

Larry
Kalle Valo Jan. 29, 2019, 5:23 p.m. UTC | #16
Tony Chuang <yhchuang@realtek.com> writes:

>> I suggest to look at criticality of the bug and size of the patch. For
>> example, if the bug is a minor and the patch is large you should
>> definitely drop that. And smaller fixes to severe bugs you should
>> definitely again include them.
>> 
>> Just remember to list in the changelog every change you made since
>> previous version.
>> 
>
> Hi Kalle,
>
> One more question, should I merge the trivial patches into the original
> patch set and resend them like [PATCH v4 00/13].
>
> Or should I add them after the 13 patches for people to review easily.
> Such as [PATCH v4 00/21]. Which do you prefer?

I don't have any strong opinion but I prefer the former (13 patches).
Others might think differently.

BTW, I don't if you are aware but I will add the driver to
wireless-drivers-next in commit, so in the end we need to squash these
patches into patch. Only in review it's easier to have them split like
this. But we don't need to worry about that right now, let's focus on
people reviewing the driver first.