[PATCH] staging: rtlwifi: check for array overflow

[PATCH] staging: rtlwifi: check for array overflow

[Dan Carpenter]

Smatch is distrustful of the "capab" value and marks it as user
controlled.  I think it actually comes from the firmware?  Anyway, I
looked at other drivers and they added a bounds check and it seems like
a harmless thing to have so I have added it here as well.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/staging/rtlwifi/base.c b/drivers/staging/rtlwifi/base.c
index f7f207cbaee3..a30b928d5ee1 100644
--- a/drivers/staging/rtlwifi/base.c
+++ b/drivers/staging/rtlwifi/base.c
@@ -1414,6 +1414,10 @@ bool rtl_action_proc(struct ieee80211_hw *hw, struct sk_buff *skb, u8 is_tx)
 				  le16_to_cpu(mgmt->u.action.u.addba_req.capab);
 				tid = (capab &
 				       IEEE80211_ADDBA_PARAM_TID_MASK) >> 2;
+				if (tid >= MAX_TID_COUNT) {
+					rcu_read_unlock();
+					return true;
+				}
 				tid_data = &sta_entry->tids[tid];
 				if (tid_data->agg.rx_agg_state ==
 				    RTL_RX_AGG_START)

Two rtlwifi drivers?

[Kalle Valo]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> Smatch is distrustful of the "capab" value and marks it as user
> controlled.  I think it actually comes from the firmware?  Anyway, I
> looked at other drivers and they added a bounds check and it seems like
> a harmless thing to have so I have added it here as well.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/staging/rtlwifi/base.c b/drivers/staging/rtlwifi/base.c
> index f7f207cbaee3..a30b928d5ee1 100644
> --- a/drivers/staging/rtlwifi/base.c
> +++ b/drivers/staging/rtlwifi/base.c

I'm getting slightly annoyed that we now apparently have two duplicate
rtlwifi drivers (with the same name!) and I'm being spammed by staging
patches. Was this really a smart thing to do? And what will be the
future of these two drivers?

(Of course this is not directed to Dan, he is just fixing bugs found by
smatch, but more like a general question.)

-- 
Kalle Valo

Re: Two rtlwifi drivers?

[Larry Finger]

On 08/24/2017 07:14 AM, Kalle Valo wrote:
> Dan Carpenter <dan.carpenter@oracle.com> writes:
> 
>> Smatch is distrustful of the "capab" value and marks it as user
>> controlled.  I think it actually comes from the firmware?  Anyway, I
>> looked at other drivers and they added a bounds check and it seems like
>> a harmless thing to have so I have added it here as well.
>>
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>
>> diff --git a/drivers/staging/rtlwifi/base.c b/drivers/staging/rtlwifi/base.c
>> index f7f207cbaee3..a30b928d5ee1 100644
>> --- a/drivers/staging/rtlwifi/base.c
>> +++ b/drivers/staging/rtlwifi/base.c
> 
> I'm getting slightly annoyed that we now apparently have two duplicate
> rtlwifi drivers (with the same name!) and I'm being spammed by staging
> patches. Was this really a smart thing to do? And what will be the
> future of these two drivers?
> 
> (Of course this is not directed to Dan, he is just fixing bugs found by
> smatch, but more like a general question.)

That was the decision that you and Greg made. The version in wireless-drivers 
needs many patches to handle the new device. The progress in applying these to 
wireless-drivers was very slow for many reasons. Keeping a single version of 
that code would have required coordination between you and Greg, which was 
discouraged.

The future, as stated in the TODO in staging, is to merge all the changes in the 
support drivers into wireless-drivers, and then move the new RTL8822BE driver 
out of staging.

I'm sorry about the fallout affecting you, and I probably should have changed 
the directory names. In any case, ignore any patches that belong in staging. If 
I see any that do not include GregKH in the "To" list, I will NACK them and 
request proper routing.

Larry

Re: Two rtlwifi drivers?

[Kalle Valo]

(Sorry for taking so long with the reply, I wanted first to check what
the rtlwifi in staging contains.)

Larry Finger <Larry.Finger@lwfinger.net> writes:

> On 08/24/2017 07:14 AM, Kalle Valo wrote:
>> Dan Carpenter <dan.carpenter@oracle.com> writes:
>>
>>> Smatch is distrustful of the "capab" value and marks it as user
>>> controlled.  I think it actually comes from the firmware?  Anyway, I
>>> looked at other drivers and they added a bounds check and it seems like
>>> a harmless thing to have so I have added it here as well.
>>>
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>
>>> diff --git a/drivers/staging/rtlwifi/base.c b/drivers/staging/rtlwifi/base.c
>>> index f7f207cbaee3..a30b928d5ee1 100644
>>> --- a/drivers/staging/rtlwifi/base.c
>>> +++ b/drivers/staging/rtlwifi/base.c
>>
>> I'm getting slightly annoyed that we now apparently have two duplicate
>> rtlwifi drivers (with the same name!) and I'm being spammed by staging
>> patches. Was this really a smart thing to do? And what will be the
>> future of these two drivers?
>>
>> (Of course this is not directed to Dan, he is just fixing bugs found by
>> smatch, but more like a general question.)
>
> That was the decision that you and Greg made. The version in
> wireless-drivers needs many patches to handle the new device. The
> progress in applying these to wireless-drivers was very slow for many
> reasons. Keeping a single version of that code would have required
> coordination between you and Greg, which was discouraged.

I don't recall deciding anything, all I did was asking for more info
about the new code. I was against the idea since I first saw your mail
but I tried to be diplomatic and not shot it down immeadiately. Shows
that diplomacy is not really my thing...

We always take extra measures to avoid forking code, why is it now all
of sudden ok? Also this gives the wrong message to Realtek, and other
vendors, that they can just fork the driver and push all sort of crap to
staging.

So just to make clear to everyone: I think forking drivers like this is
a HORRIBLE idea and I do not want to have anything to do with that. If
schedule goes over quality then a much better approach is to move to the
whole driver to staging than to have duplicated drivers like we have
now.

> The future, as stated in the TODO in staging, is to merge all the
> changes in the support drivers into wireless-drivers, and then move
> the new RTL8822BE driver out of staging.

And how many years will that take? (If that will ever happen) Also I see
that multiple patches are applied to staging version of rtlwifi which is
not in net version of rtlwifi. That all should be synced somehow, the
bigger the delta becomes the more difficult everything is.

> I'm sorry about the fallout affecting you, and I probably should have
> changed the directory names. In any case, ignore any patches that
> belong in staging. If I see any that do not include GregKH in the "To"
> list, I will NACK them and request proper routing.

Thanks, but that doesn't really help as I apply patches from patchwork
and it doesn't show the To field. I'll try to be extra careful and not
apply patches the staging version of rtlwifi patches, but mistakes
always happen.

-- 
Kalle Valo

Re: Two rtlwifi drivers?

[Greg Kroah-Hartman]

On Wed, Oct 11, 2017 at 12:06:00PM +0300, Kalle Valo wrote:
> (Sorry for taking so long with the reply, I wanted first to check what
> the rtlwifi in staging contains.)
> 
> Larry Finger <Larry.Finger@lwfinger.net> writes:
> 
> > On 08/24/2017 07:14 AM, Kalle Valo wrote:
> >> Dan Carpenter <dan.carpenter@oracle.com> writes:
> >>
> >>> Smatch is distrustful of the "capab" value and marks it as user
> >>> controlled.  I think it actually comes from the firmware?  Anyway, I
> >>> looked at other drivers and they added a bounds check and it seems like
> >>> a harmless thing to have so I have added it here as well.
> >>>
> >>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >>>
> >>> diff --git a/drivers/staging/rtlwifi/base.c b/drivers/staging/rtlwifi/base.c
> >>> index f7f207cbaee3..a30b928d5ee1 100644
> >>> --- a/drivers/staging/rtlwifi/base.c
> >>> +++ b/drivers/staging/rtlwifi/base.c
> >>
> >> I'm getting slightly annoyed that we now apparently have two duplicate
> >> rtlwifi drivers (with the same name!) and I'm being spammed by staging
> >> patches. Was this really a smart thing to do? And what will be the
> >> future of these two drivers?
> >>
> >> (Of course this is not directed to Dan, he is just fixing bugs found by
> >> smatch, but more like a general question.)
> >
> > That was the decision that you and Greg made. The version in
> > wireless-drivers needs many patches to handle the new device. The
> > progress in applying these to wireless-drivers was very slow for many
> > reasons. Keeping a single version of that code would have required
> > coordination between you and Greg, which was discouraged.
> 
> I don't recall deciding anything, all I did was asking for more info
> about the new code. I was against the idea since I first saw your mail
> but I tried to be diplomatic and not shot it down immeadiately. Shows
> that diplomacy is not really my thing...
> 
> We always take extra measures to avoid forking code, why is it now all
> of sudden ok? Also this gives the wrong message to Realtek, and other
> vendors, that they can just fork the driver and push all sort of crap to
> staging.
> 
> So just to make clear to everyone: I think forking drivers like this is
> a HORRIBLE idea and I do not want to have anything to do with that. If
> schedule goes over quality then a much better approach is to move to the
> whole driver to staging than to have duplicated drivers like we have
> now.

I think it's horrid too.  But, if no one is able to do the real work
here, we hurt users who just need to use their hardware to get things
done.

And it seems like the company isn't willing to do the real work, so
dumping this in staging is the best we can do at the moment.

However, if this causes you problems, that's not good at all either.
Getting "real" support for this hardware is key, and hopefully can
happen somehow.  But fixing up the staging driver is almost never the
way to do it, as you know :)

So what to do?  Any ideas?  What makes your life easier?  You can just
ignore the staging tree, as it should not affect your portion of the
kernel at all, right?

thanks,

greg k-h

Re: Two rtlwifi drivers?

[Dan Carpenter]

On Wed, Oct 11, 2017 at 03:13:10PM +0200, Greg Kroah-Hartman wrote:
> And it seems like the company isn't willing to do the real work, so
> dumping this in staging is the best we can do at the moment.

I'm more optimistic.  There are a lot of @realtek.com addresses in the
CC list and that's a new thing.

regards,
dan carpenter

Re: Two rtlwifi drivers?

[Larry Finger]

On 10/11/2017 08:13 AM, Greg Kroah-Hartman wrote:
> On Wed, Oct 11, 2017 at 12:06:00PM +0300, Kalle Valo wrote:
>> (Sorry for taking so long with the reply, I wanted first to check what
>> the rtlwifi in staging contains.)
>>
>> Larry Finger <Larry.Finger@lwfinger.net> writes:
>>
>>> On 08/24/2017 07:14 AM, Kalle Valo wrote:
>>>> Dan Carpenter <dan.carpenter@oracle.com> writes:
>>>>
>>>>> Smatch is distrustful of the "capab" value and marks it as user
>>>>> controlled.  I think it actually comes from the firmware?  Anyway, I
>>>>> looked at other drivers and they added a bounds check and it seems like
>>>>> a harmless thing to have so I have added it here as well.
>>>>>
>>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>>>
>>>>> diff --git a/drivers/staging/rtlwifi/base.c b/drivers/staging/rtlwifi/base.c
>>>>> index f7f207cbaee3..a30b928d5ee1 100644
>>>>> --- a/drivers/staging/rtlwifi/base.c
>>>>> +++ b/drivers/staging/rtlwifi/base.c
>>>>
>>>> I'm getting slightly annoyed that we now apparently have two duplicate
>>>> rtlwifi drivers (with the same name!) and I'm being spammed by staging
>>>> patches. Was this really a smart thing to do? And what will be the
>>>> future of these two drivers?
>>>>
>>>> (Of course this is not directed to Dan, he is just fixing bugs found by
>>>> smatch, but more like a general question.)
>>>
>>> That was the decision that you and Greg made. The version in
>>> wireless-drivers needs many patches to handle the new device. The
>>> progress in applying these to wireless-drivers was very slow for many
>>> reasons. Keeping a single version of that code would have required
>>> coordination between you and Greg, which was discouraged.
>>
>> I don't recall deciding anything, all I did was asking for more info
>> about the new code. I was against the idea since I first saw your mail
>> but I tried to be diplomatic and not shot it down immeadiately. Shows
>> that diplomacy is not really my thing...
>>
>> We always take extra measures to avoid forking code, why is it now all
>> of sudden ok? Also this gives the wrong message to Realtek, and other
>> vendors, that they can just fork the driver and push all sort of crap to
>> staging.
>>
>> So just to make clear to everyone: I think forking drivers like this is
>> a HORRIBLE idea and I do not want to have anything to do with that. If
>> schedule goes over quality then a much better approach is to move to the
>> whole driver to staging than to have duplicated drivers like we have
>> now.
> 
> I think it's horrid too.  But, if no one is able to do the real work
> here, we hurt users who just need to use their hardware to get things
> done.
> 
> And it seems like the company isn't willing to do the real work, so
> dumping this in staging is the best we can do at the moment.
> 
> However, if this causes you problems, that's not good at all either.
> Getting "real" support for this hardware is key, and hopefully can
> happen somehow.  But fixing up the staging driver is almost never the
> way to do it, as you know :)
> 
> So what to do?  Any ideas?  What makes your life easier?  You can just
> ignore the staging tree, as it should not affect your portion of the
> kernel at all, right?

Greg and Kalle,

We can all agree that this situation is bad; however, several of the points made 
in your E-mails need to be addressed.

First of all, I am not an employee of Realtek, and I have no knowledge of the 
internals of any of their chips. I have never signed a non-disclosure agreement, 
and the only thing that I have received from them are sample chips for testing. 
My main interest is in helping the user experience. As there are a number of 
users with the new RTL8822BE device, that meant getting an in-kernel driver to 
them as soon as possible. As stated earlier, adding this particular device to 
the rtlwifi family involved roughly 120K lines of new code. Given our recent 
experience in getting code accepted into the wireless tree meant that this 
additional code would not have been accepted for many months. For that reason, 
we chose the staging route. It is important that we get this right as Realtek 
tells me that there will be two additional new drivers in the coming 6 months.

As to the convergence of the rtlwifi code between staging and wireless, I 
applied the 40 or 50 patches in my queue to the wireless version to create the 
version that went into staging. If any of the current patches to wireless do not 
seem to be in the staging version, sometimes temporary changes are necessary so 
that the intermediate drivers will build and work. Yes, it is code churn, but 
necessary to keep patches small. In keeping with Kalle's requests, only a 
fraction of those patches have been submitted to him.

My intent is to have the RTL8822BE driver moved from staging to mainline no 
later than 4.17. The affected drivers rtl_pci, rtlwifi and becoex will be moved 
in that order. Of course, the required changes will need to be in wireless 
before staging is changed, which will slow the process. As I see it, the switch 
can only occur with a new version.

My opinion is that the company has gone a long ways toward meeting the 
requirements of the Linux kernel. A lot of their code is written for Windows and 
Linux, with emphasis on Windows, which complicates matters as not all of the 
changes for Linux can be backported. Prior to the introduction of these drivers 
in 2.6.38, drivers were released periodically as tar files with no information 
regarding intermediate steps were recorded other than the SVN modification 
number. At least now, we get relatively small changes.

I have been pleased and surprised at the stability and performance of the driver 
in staging. Other than possible changes required by reviewers when it is moved, 
it is ready for the wireless tree.

Finally, I have notified my Realtek contact that I do not plan to continue as 
the maintainer of these drivers very much longer due to my age. I still have the 
interest, but not always the energy. The people at Realtek have proposed a plan 
that seems workable. Of course, there will be hiccups, but the current process 
does not seem to be that smooth.

Larry

Re: Two rtlwifi drivers?

[Kalle Valo]

Larry Finger <Larry.Finger@lwfinger.net> writes:

> On 10/11/2017 08:13 AM, Greg Kroah-Hartman wrote:
>
>> On Wed, Oct 11, 2017 at 12:06:00PM +0300, Kalle Valo wrote:
>> I think it's horrid too.  But, if no one is able to do the real work
>> here, we hurt users who just need to use their hardware to get things
>> done.
>>
>> And it seems like the company isn't willing to do the real work, so
>> dumping this in staging is the best we can do at the moment.
>>
>> However, if this causes you problems, that's not good at all either.
>> Getting "real" support for this hardware is key, and hopefully can
>> happen somehow.  But fixing up the staging driver is almost never the
>> way to do it, as you know :)
>>
>> So what to do?  Any ideas?  What makes your life easier?  You can just
>> ignore the staging tree, as it should not affect your portion of the
>> kernel at all, right?
>
> Greg and Kalle,
>
> We can all agree that this situation is bad; however, several of the
> points made in your E-mails need to be addressed.
>
> First of all, I am not an employee of Realtek, and I have no knowledge
> of the internals of any of their chips. I have never signed a
> non-disclosure agreement, and the only thing that I have received from
> them are sample chips for testing. My main interest is in helping the
> user experience.

And you are doing a great job at that! My only gripe here is forking the
driver.

> As there are a number of users with the new RTL8822BE device, that
> meant getting an in-kernel driver to them as soon as possible. As
> stated earlier, adding this particular device to the rtlwifi family
> involved roughly 120K lines of new code. Given our recent experience
> in getting code accepted into the wireless tree meant that this
> additional code would not have been accepted for many months. For that
> reason, we chose the staging route. It is important that we get this
> right as Realtek tells me that there will be two additional new
> drivers in the coming 6 months.

If there are new drivers coming in 6 months they should submitting
patches already now, this is my main point. Don't work in a waterfall
model, release early and release often.

> As to the convergence of the rtlwifi code between staging and
> wireless, I applied the 40 or 50 patches in my queue to the wireless
> version to create the version that went into staging. If any of the
> current patches to wireless do not seem to be in the staging version,
> sometimes temporary changes are necessary so that the intermediate
> drivers will build and work. Yes, it is code churn, but necessary to
> keep patches small. In keeping with Kalle's requests, only a fraction
> of those patches have been submitted to him.
>
> My intent is to have the RTL8822BE driver moved from staging to
> mainline no later than 4.17. The affected drivers rtl_pci, rtlwifi and
> becoex will be moved in that order. Of course, the required changes
> will need to be in wireless before staging is changed, which will slow
> the process.

Great to hear that you will be working on that. But yeah, that's quite
an effort. IMHO a lot more work than if you would be working only with
drivers/net/wireless.

> As I see it, the switch can only occur with a new version.

Yeah, we need to be very careful with the switch so that we don't break
existing setups.

> My opinion is that the company has gone a long ways toward meeting the
> requirements of the Linux kernel. A lot of their code is written for
> Windows and Linux, with emphasis on Windows, which complicates matters
> as not all of the changes for Linux can be backported.

I think all vendors had these huge OS agnostic drivers before: TI,
Atheros/QCA etc. So it's not really a new thing and still most of the
companies have been able to adapt one way or another.

> Prior to the introduction of these drivers in 2.6.38, drivers were
> released periodically as tar files with no information regarding
> intermediate steps were recorded other than the SVN modification
> number. At least now, we get relatively small changes.

Yes, the changes might be small but to me it seems Realtek still dumps
them in one big release. That's the problem here.

> I have been pleased and surprised at the stability and performance of
> the driver in staging. Other than possible changes required by
> reviewers when it is moved, it is ready for the wireless tree.
>
> Finally, I have notified my Realtek contact that I do not plan to
> continue as the maintainer of these drivers very much longer due to my
> age. I still have the interest, but not always the energy. The people
> at Realtek have proposed a plan that seems workable. Of course, there
> will be hiccups, but the current process does not seem to be that
> smooth.

Sorry to hear that you are stepping down as the maintainer but it's
undertandable as you have had so much work to do. But I hope you still
stick around.

-- 
Kalle Valo

Re: Two rtlwifi drivers?

[Kalle Valo]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

>> >> I'm getting slightly annoyed that we now apparently have two duplicate
>> >> rtlwifi drivers (with the same name!) and I'm being spammed by staging
>> >> patches. Was this really a smart thing to do? And what will be the
>> >> future of these two drivers?
>> >>
>> >> (Of course this is not directed to Dan, he is just fixing bugs found by
>> >> smatch, but more like a general question.)
>> >
>> > That was the decision that you and Greg made. The version in
>> > wireless-drivers needs many patches to handle the new device. The
>> > progress in applying these to wireless-drivers was very slow for many
>> > reasons. Keeping a single version of that code would have required
>> > coordination between you and Greg, which was discouraged.
>> 
>> I don't recall deciding anything, all I did was asking for more info
>> about the new code. I was against the idea since I first saw your mail
>> but I tried to be diplomatic and not shot it down immeadiately. Shows
>> that diplomacy is not really my thing...
>> 
>> We always take extra measures to avoid forking code, why is it now all
>> of sudden ok? Also this gives the wrong message to Realtek, and other
>> vendors, that they can just fork the driver and push all sort of crap to
>> staging.
>> 
>> So just to make clear to everyone: I think forking drivers like this is
>> a HORRIBLE idea and I do not want to have anything to do with that. If
>> schedule goes over quality then a much better approach is to move to the
>> whole driver to staging than to have duplicated drivers like we have
>> now.
>
> I think it's horrid too.  But, if no one is able to do the real work
> here, we hurt users who just need to use their hardware to get things
> done.
>
> And it seems like the company isn't willing to do the real work, so
> dumping this in staging is the best we can do at the moment.

I understand that this decision was made because of the users. But I
just think that if Realtek is not interested to follow our way of
working then we should dump the whole rtlwifi driver to staging, not
duplicate the driver like this and get everyone confused (and introduce
more work everyone, especially for Larry).

We already have great vendor support. For example, Intel is awesome as
they support in iwlwifi even before the users get the hardware. Also
Marvell, Quantenna and RSI are improving all the time. I admit that
Realtek is trying but mostly it's because of Larry's tireless efforts.

But my point here is that if Realtek wants to have good support for
their hardware in rtlwifi they need to submit the patches in advance,
preferably months before their deadline. Not like "the deadline for
these patches were last week" type of philosophy they have now.

> However, if this causes you problems, that's not good at all either.
> Getting "real" support for this hardware is key, and hopefully can
> happen somehow.  But fixing up the staging driver is almost never the
> way to do it, as you know :)

I can manage for now, but if we don't react and fix this it can get
messy soon. And what I fear the most is that other vendors want to start
duplicating drivers like this to meet their schedules, we should make it
clear that this is not acceptable.

> So what to do?  Any ideas?  What makes your life easier?  You can just
> ignore the staging tree, as it should not affect your portion of the
> kernel at all, right?

Yes, I automatically ignore anything staging related. But the problem is
that we now have two drivers with the same name and people don't always
remember to prefix the patch with "staging: ". So on a bad day I might
accidentally apply a patch which was meant for your tree. Of course I
immediately revert it as soon as I, or someone else, catches that but
annoying still.

I think we have two options here:

1) We set a deadline (like 12 months or something) for the
   drivers/staging/rtlwifi and after that you refuse to take any patches
   for it. Hopefully this makes it clear for everyone that this fork is
   just temporary. I think Larry is trying to do this, which is great.

2) We move the whole rtlwifi driver to staging. A very bad option but
   still better than forking the drivers.

-- 
Kalle Valo

Re: Two rtlwifi drivers?

[Greg Kroah-Hartman]

On Thu, Oct 12, 2017 at 11:38:06AM +0300, Kalle Valo wrote:
> > So what to do?  Any ideas?  What makes your life easier?  You can just
> > ignore the staging tree, as it should not affect your portion of the
> > kernel at all, right?
> 
> Yes, I automatically ignore anything staging related. But the problem is
> that we now have two drivers with the same name and people don't always
> remember to prefix the patch with "staging: ". So on a bad day I might
> accidentally apply a patch which was meant for your tree. Of course I
> immediately revert it as soon as I, or someone else, catches that but
> annoying still.

It doesn't bother me if you apply staging patches, I can handle the
merge issues :)

> I think we have two options here:
> 
> 1) We set a deadline (like 12 months or something) for the
>    drivers/staging/rtlwifi and after that you refuse to take any patches
>    for it. Hopefully this makes it clear for everyone that this fork is
>    just temporary. I think Larry is trying to do this, which is great.

Fine with me, if Larry is ok with it.

> 2) We move the whole rtlwifi driver to staging. A very bad option but
>    still better than forking the drivers.

Ick, I don't want that to have to happen, that would not be good for the
users of other devices that the "real" rtlwifi driver supports.

thanks,

greg k-h

RE: Two rtlwifi drivers?

[Pkshih]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogR3JlZyBLcm9haC1IYXJ0
bWFuIFttYWlsdG86Z3JlZ2toQGxpbnV4Zm91bmRhdGlvbi5vcmddDQo+IFNlbnQ6IFRodXJzZGF5
LCBPY3RvYmVyIDEyLCAyMDE3IDY6MzUgUE0NCj4gVG86IEthbGxlIFZhbG8NCj4gQ2M6IExhcnJ5
IEZpbmdlcjsgRGFuIENhcnBlbnRlcjsgUGtzaGloOyCy+Kvbq8U7IEpvaGFubmVzIEJlcmc7IFNv
dXB0aWNrIEpvYXJkZXI7DQo+IGRldmVsQGRyaXZlcmRldi5vc3Vvc2wub3JnOyBsaW51eC13aXJl
bGVzc0B2Z2VyLmtlcm5lbC5vcmc7IGtlcm5lbC1qYW5pdG9yc0B2Z2VyLmtlcm5lbC5vcmcNCj4g
U3ViamVjdDogUmU6IFR3byBydGx3aWZpIGRyaXZlcnM/DQo+IA0KPiBPbiBUaHUsIE9jdCAxMiwg
MjAxNyBhdCAxMTozODowNkFNICswMzAwLCBLYWxsZSBWYWxvIHdyb3RlOg0KPiA+ID4gU28gd2hh
dCB0byBkbz8gIEFueSBpZGVhcz8gIFdoYXQgbWFrZXMgeW91ciBsaWZlIGVhc2llcj8gIFlvdSBj
YW4ganVzdA0KPiA+ID4gaWdub3JlIHRoZSBzdGFnaW5nIHRyZWUsIGFzIGl0IHNob3VsZCBub3Qg
YWZmZWN0IHlvdXIgcG9ydGlvbiBvZiB0aGUNCj4gPiA+IGtlcm5lbCBhdCBhbGwsIHJpZ2h0Pw0K
PiA+DQo+ID4gWWVzLCBJIGF1dG9tYXRpY2FsbHkgaWdub3JlIGFueXRoaW5nIHN0YWdpbmcgcmVs
YXRlZC4gQnV0IHRoZSBwcm9ibGVtIGlzDQo+ID4gdGhhdCB3ZSBub3cgaGF2ZSB0d28gZHJpdmVy
cyB3aXRoIHRoZSBzYW1lIG5hbWUgYW5kIHBlb3BsZSBkb24ndCBhbHdheXMNCj4gPiByZW1lbWJl
ciB0byBwcmVmaXggdGhlIHBhdGNoIHdpdGggInN0YWdpbmc6ICIuIFNvIG9uIGEgYmFkIGRheSBJ
IG1pZ2h0DQo+ID4gYWNjaWRlbnRhbGx5IGFwcGx5IGEgcGF0Y2ggd2hpY2ggd2FzIG1lYW50IGZv
ciB5b3VyIHRyZWUuIE9mIGNvdXJzZSBJDQo+ID4gaW1tZWRpYXRlbHkgcmV2ZXJ0IGl0IGFzIHNv
b24gYXMgSSwgb3Igc29tZW9uZSBlbHNlLCBjYXRjaGVzIHRoYXQgYnV0DQo+ID4gYW5ub3lpbmcg
c3RpbGwuDQo+IA0KPiBJdCBkb2Vzbid0IGJvdGhlciBtZSBpZiB5b3UgYXBwbHkgc3RhZ2luZyBw
YXRjaGVzLCBJIGNhbiBoYW5kbGUgdGhlDQo+IG1lcmdlIGlzc3VlcyA6KQ0KPiANCj4gPiBJIHRo
aW5rIHdlIGhhdmUgdHdvIG9wdGlvbnMgaGVyZToNCj4gPg0KPiA+IDEpIFdlIHNldCBhIGRlYWRs
aW5lIChsaWtlIDEyIG1vbnRocyBvciBzb21ldGhpbmcpIGZvciB0aGUNCj4gPiAgICBkcml2ZXJz
L3N0YWdpbmcvcnRsd2lmaSBhbmQgYWZ0ZXIgdGhhdCB5b3UgcmVmdXNlIHRvIHRha2UgYW55IHBh
dGNoZXMNCj4gPiAgICBmb3IgaXQuIEhvcGVmdWxseSB0aGlzIG1ha2VzIGl0IGNsZWFyIGZvciBl
dmVyeW9uZSB0aGF0IHRoaXMgZm9yayBpcw0KPiA+ICAgIGp1c3QgdGVtcG9yYXJ5LiBJIHRoaW5r
IExhcnJ5IGlzIHRyeWluZyB0byBkbyB0aGlzLCB3aGljaCBpcyBncmVhdC4NCj4gDQo+IEZpbmUg
d2l0aCBtZSwgaWYgTGFycnkgaXMgb2sgd2l0aCBpdC4NCj4gDQo+ID4gMikgV2UgbW92ZSB0aGUg
d2hvbGUgcnRsd2lmaSBkcml2ZXIgdG8gc3RhZ2luZy4gQSB2ZXJ5IGJhZCBvcHRpb24gYnV0DQo+
ID4gICAgc3RpbGwgYmV0dGVyIHRoYW4gZm9ya2luZyB0aGUgZHJpdmVycy4NCj4gDQo+IEljaywg
SSBkb24ndCB3YW50IHRoYXQgdG8gaGF2ZSB0byBoYXBwZW4sIHRoYXQgd291bGQgbm90IGJlIGdv
b2QgZm9yIHRoZQ0KPiB1c2VycyBvZiBvdGhlciBkZXZpY2VzIHRoYXQgdGhlICJyZWFsIiBydGx3
aWZpIGRyaXZlciBzdXBwb3J0cy4NCj4gDQoNCkhpIExhcnJ5LCBLYWxsZSBhbmQgR2VyZywNCg0K
VGhpcyBpcyBSZWFsdGVrIGVuZ2luZWVyLCBQSy4gSSBhcHByZWNpYXRlIHlvdXIgc3VwcG9ydCB0
byBzdWJtaXQsIHJldmlldyANCmFuZCBtZXJnZSBwYXRjaC4gU2luY2UgSSdtIGEgTGludXggbmV3
YmllLCBJJ2xsIGRlc2NyaWJlIHRoZSBzaXR1YXRpb24gb2YgDQpydGx3aWZpIGFuZCBuZWVkIHlv
dXIgc3VnZ2VzdGlvbnMuDQoNCg0KMSkgTmV3IG1vZHVsZXMgaW4gcnRsd2lmaQ0KICAgV2UgYWRk
IHR3byBtb2R1bGVzIG5hbWVkIHBoeWRtIGFuZCBoYWxtYWMsIHdoZW4gYWRkaW5nIHJ0bDg4MjJi
ZSBpbiANCiAgIHN0YWdpbmcuIFRoZSBwaHlkbSBpcyBCQi9SRiByZWxhdGVkIG1vZHVsZSBjb250
YWluaW5nIHRoZSBwYXJhbWV0ZXJzDQogICBhbmQgQVBJcyBvZiBCQi9SRiwgYW5kIGEgZHluYW1p
YyBtZWNoYW5pc20gdG8gYWRhcHQgdG8gZGlmZmVyZW50DQogICBmaWVsZCBlbnZpcm9ubWVudC4g
VGhlIGhhbG1hYyBjb25zaXN0cyBvZiBNQUMgQVBJcy4NCiAgIFRoZSB0d28gbW9kdWxlcyBhcmUg
dXNlZCBieSBtYW55IE9Tcywgc28gJyNpZmRlZicsIENhbWVsQ2FzZSBhbmQNCiAgIHNvIG9uIGFy
ZSBleGlzdGluZyBpbiBvcmlnaW5hbCBmaWxlcy4gSGVuY2UsIHdlIGNvbnZlcnQgdGhlbSB0byBM
aW51eCANCiAgIGZvcm0gYnkgc2NyaXB0LCBidXQgaXQncyBub3QgcGVyZmVjdC4gRG8geW91IGhh
dmUgc3VnZ2VzdGlvbiB0byBkZWFsDQogICB3aXRoIHRoaXMgcHJvYmxlbT8NCg0KDQoyKSBUaGUg
cnRsd2lmaSBpbiBzdGFnaW5nDQogICBJbiBzdGFnaW5nLCB0aGUgbW9kdWxlIHBoeWRtIHYxMyBj
b250YWlucyBidWdzLCBzbyBJIHdhbnQgdG8gdXBncmFkZQ0KICAgdG8gdjIxIChSZWFsdGVrIGlu
dGVybmFsIHZlcnNpb24gbnVtYmVyKS4gVGhpcyB1cGdyYWRlIGNvbnRhaW5zIGENCiAgIGJpZyBw
YXRjaCB0aGF0IHRoZSBkaWZmZXJlbmNlIGJldHdlZW4gdjEzIGFuZCB2MjEsIGFuZCB0aGVyZSBh
cmUgDQogICA0MCsgcGF0Y2hlcyB0byBzdXBwb3J0IHRoaXMgdXBncmFkZS4gSSBoYXZlIHRocmVl
IG9wdGlvbnMsIGFuZA0KICAgSSB3YW50IHRvIGtub3cgd2hpY2ggb25lIGlzIHByZWZlci4NCg0K
Mi4xKSBhcHBseSA0MCsgcGF0Y2hlcyB0byBib3RoIHN0YWdpbmcgYW5kIHdpcmVsZXNzIHRyZWUs
IGFuZCBhcHBseQ0KICAgICB0aGUgYmlnIHBhdGNoIHRvIHN0YWdpbmcgb25seS4gQWZ0ZXIgcmV2
aWV3aW5nLCB3ZSBtb3ZlIHRoZSBtb2R1bGUNCiAgICAgdG8gd2lyZWxlc3MgdHJlZS4NCg0KMi4y
KSBhcHBseSA0MCsgcGF0Y2hlcyB0byB3aXJlbGVzcyB0cmVlLCBhbmQgYXBwbHkgYSBzaW5nbGUg
YmlnZ2VyIA0KICAgICBwYXRjaCBjb250YWluaW5nIDQwKyBwYXRjaGVzIGFuZCB0aGUgYmlnIHBh
dGNoIHRvIHN0YWdpbmcuIEkgdGhpbmsNCiAgICAgdGhpcyBjYW4gYmUgc2VlbiBhcyBhIG5ldyBk
cml2ZXIgaW4gc3RhZ2luZy4gQWZ0ZXIgcmV2aWV3aW5nLCANCiAgICAgd2UgbW92ZSB0aGUgbW9k
dWxlIHRvIHdpcmVsZXNzIHRyZWUuDQoNCjIuMykgZG9uJ3QgYXBwbHkgYW55dGhpbmcgdG8gc3Rh
Z2luZy4gSnVzdCBhcHBseSA0MCsgcGF0Y2hlcyBhbmQgYWRkDQogICAgIHBoeWRtIHYyMSB0byB3
aXJlbGVzcy4NCg0KDQozKSBDb21pbmcgZHJpdmVycyAtLSBydGw4NzIzZGUgYW5kIHJ0bDg4MjFj
ZQ0KICAgV2UncmUgZGV2ZWxvcGluZyB0aGUgdHdvIGRyaXZlcnMsIGFuZCBydGw4NzIzZGUgYW5k
IHJ0bDg4MjFjZSB3aWxsDQogICBiZSByZWFkeSBvbiAyMDE3UTQgYW5kIDIwMThRMSByZXNwZWN0
aXZlbHkuIFRoZSBkcml2ZXJzIGFyZSBiYXNlZCBvbg0KICAgcnRsODgyMmJlIHRoYXQgaW4gc3Rh
Z2luZyBub3csIHNvIHRoZSBsaW5lIG9mIGNvZGUgd2lsbCBiZSBmZXdlci4NCiAgIFRoZSBuZXcg
ZmlsZXMgd2lsbCBiZSBhIG5ldyBJQyBmb2xkZXIgYW5kIElDIHN1cHBvcnRlZCBmaWxlcyBvZiAN
CiAgIHRocmVlIG1vZHVsZXMgdGhhdCBidGNvZXhpc3QsIHBoeWRtIGFuZCBoYWxtYWMuIENvdWxk
IEkgc3VibWl0DQogICB0aGVtIHRvIHdpcmxlc3MgdHJlZSB3aGVuIHRoZXkncmUgcmVhZHk/DQoN
Cg0KNCkgQXMgS2FsbGUgbWVudGlvbmVkLCBydGx3aWZpIGNvbnRhaW5zIG1hbnkgbWFnaWMgbnVt
YmVycywgYW5kIEkgDQogICBwbGFuIHRvIGZpeCB0aGVtIGFmdGVyIHJ0bDg3MjNkZSBhbmQgcnRs
ODgyMWNlLiBCZWNhdXNlIHRoZSBkcml2ZXJzDQogICBhcmUgZGV2ZWxvcGluZywgdGhlIGNoYW5n
ZXMgd2lsbCBtYWtlIHVzIGhhcmQgdG8gaW50ZWdyYXRlLiBIb3dldmVyLA0KICAgSSBkb24ndCBo
YXZlIHBsYW4gdG8gcHJvY2VzcyB0aGUgbWFnaWMgbnVtYmVycyBpbiB0aGUgbW9kdWxlIHBoeWRt
LA0KICAgYmVjYXVzZSB0aGUgbW9zdCBvZiBCQi9SRiByZWdpc3RlcnMgY29udGFpbiBtYW55IGZ1
bmN0aW9ucy4gQW5kDQogICBpdCBkb2Vzbid0IGhhdmUgYSByZWdpc3RlciBuYW1lIGJ1dCBhIGJp
dCBmaWVsZCBuYW1lIGluc3RlYWQuDQogICBPdXIgQkIgdGVhbSBndXlzIHNheSB0aGUgdXNlIG9m
IGVudW1lcmF0aW9uIG9yIGRlZmluZWQgbmFtZSB3aWxsDQogICBiZSB1bnJlYWRhYmxlLCBhbmQg
dGhlIG5hbWUgaXMgbWVhbmluZ2xlc3MgZm9yIG1vc3QgcGVvcGxlLg0KDQoNCk1hbnkgTGludXgg
dXNlcnMgYXNrIExhcnJ5IGFib3V0IHRoZSBuZXcgZHJpdmVycywgYW5kIFJlYWx0ZWsgd2lsbA0K
cHJvdmlkZSBkcml2ZXJzIGFuZCB0cnkgdG8gc3VibWl0IHRoZW0gYnkgbXlzZWxmLiBJIGhvcGUg
dGhlIExpbnV4DQp1c2VycyBjYW4geWllbGQgdGhlIGRyaXZlcnMgYXMgc29vbiBhcyBJIGNhbi4g
T24gdGhlIHdheSwgSSdsbCANCmF0dGVuZCBuZXRkZXYgd29ya3Nob3AgaW4gS29yZWEsIHNvIHdl
IGNhbiBtZWV0IHRoZXJlIGlmIHlvdSBhdHRlbmQgdG9vLg0KDQoNClBLDQoNCg==

Re: Two rtlwifi drivers?

[Oleksij Rempel]

[-- Attachment #1.1: Type: text/plain, Size: 5689 bytes --]

Hi
Just my two cents, :)

Am 16.10.2017 um 04:41 schrieb Pkshih:
> 
> 
>> -----Original Message-----
>> From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org]
>> Sent: Thursday, October 12, 2017 6:35 PM
>> To: Kalle Valo
>> Cc: Larry Finger; Dan Carpenter; Pkshih; 莊彥宣; Johannes Berg; Souptick Joarder;
>> devel@driverdev.osuosl.org; linux-wireless@vger.kernel.org; kernel-janitors@vger.kernel.org
>> Subject: Re: Two rtlwifi drivers?
>>
>> On Thu, Oct 12, 2017 at 11:38:06AM +0300, Kalle Valo wrote:
>>>> So what to do?  Any ideas?  What makes your life easier?  You can just
>>>> ignore the staging tree, as it should not affect your portion of the
>>>> kernel at all, right?
>>>
>>> Yes, I automatically ignore anything staging related. But the problem is
>>> that we now have two drivers with the same name and people don't always
>>> remember to prefix the patch with "staging: ". So on a bad day I might
>>> accidentally apply a patch which was meant for your tree. Of course I
>>> immediately revert it as soon as I, or someone else, catches that but
>>> annoying still.
>>
>> It doesn't bother me if you apply staging patches, I can handle the
>> merge issues :)
>>
>>> I think we have two options here:
>>>
>>> 1) We set a deadline (like 12 months or something) for the
>>>    drivers/staging/rtlwifi and after that you refuse to take any patches
>>>    for it. Hopefully this makes it clear for everyone that this fork is
>>>    just temporary. I think Larry is trying to do this, which is great.
>>
>> Fine with me, if Larry is ok with it.
>>
>>> 2) We move the whole rtlwifi driver to staging. A very bad option but
>>>    still better than forking the drivers.
>>
>> Ick, I don't want that to have to happen, that would not be good for the
>> users of other devices that the "real" rtlwifi driver supports.
>>
> 
> Hi Larry, Kalle and Gerg,
> 
> This is Realtek engineer, PK. I appreciate your support to submit, review 
> and merge patch. Since I'm a Linux newbie, I'll describe the situation of 
> rtlwifi and need your suggestions.
> 
> 
> 1) New modules in rtlwifi
>    We add two modules named phydm and halmac, when adding rtl8822be in 
>    staging. The phydm is BB/RF related module containing the parameters
>    and APIs of BB/RF, and a dynamic mechanism to adapt to different
>    field environment. The halmac consists of MAC APIs.
>    The two modules are used by many OSs, so '#ifdef', CamelCase and
>    so on are existing in original files. Hence, we convert them to Linux 
>    form by script, but it's not perfect. Do you have suggestion to deal
>    with this problem?
> 
> 
> 2) The rtlwifi in staging
>    In staging, the module phydm v13 contains bugs, so I want to upgrade
>    to v21 (Realtek internal version number). This upgrade contains a
>    big patch that the difference between v13 and v21, and there are 
>    40+ patches to support this upgrade. I have three options, and
>    I want to know which one is prefer.
> 
> 2.1) apply 40+ patches to both staging and wireless tree, and apply
>      the big patch to staging only. After reviewing, we move the module
>      to wireless tree.
> 
> 2.2) apply 40+ patches to wireless tree, and apply a single bigger 
>      patch containing 40+ patches and the big patch to staging. I think
>      this can be seen as a new driver in staging. After reviewing, 
>      we move the module to wireless tree.
> 
> 2.3) don't apply anything to staging. Just apply 40+ patches and add
>      phydm v21 to wireless.
> 
> 
> 3) Coming drivers -- rtl8723de and rtl8821ce
>    We're developing the two drivers, and rtl8723de and rtl8821ce will
>    be ready on 2017Q4 and 2018Q1 respectively. The drivers are based on
>    rtl8822be that in staging now, so the line of code will be fewer.
>    The new files will be a new IC folder and IC supported files of 
>    three modules that btcoexist, phydm and halmac. Could I submit
>    them to wirless tree when they're ready?
> 
> 
> 4) As Kalle mentioned, rtlwifi contains many magic numbers, and I 
>    plan to fix them after rtl8723de and rtl8821ce. Because the drivers
>    are developing, the changes will make us hard to integrate. However,
>    I don't have plan to process the magic numbers in the module phydm,
>    because the most of BB/RF registers contain many functions. And
>    it doesn't have a register name but a bit field name instead.
>    Our BB team guys say the use of enumeration or defined name will
>    be unreadable, and the name is meaningless for most people.

Experience with ath9k driver showed, that development was kind of
balanced between two groups, QCA and Community (Other companies,
researches, education and so on.). Saying: "you will not understand it
any way" is nor really helpful :)
Please don't repeat bad experience of Broadcom.

> Many Linux users ask Larry about the new drivers, and Realtek will
> provide drivers and try to submit them by myself. I hope the Linux
> users can yield the drivers as soon as I can. On the way, I'll 
> attend netdev workshop in Korea, so we can meet there if you attend too.

I hope to see Realtek providing patches and supporting Larry. Currently
RTL WiFi is taken by many user only if there is no other choice. So far
I would say:
1. provide testfarm and upstream patches - this will make reliable
driver and make most user happy.
2. provide documentation - this will make industrial and education
customers happy, so we will pay back with more patches.
3. make it interesting for WiFi hacker and your product will win!

-- 
Regards,
Oleksij


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: Two rtlwifi drivers?

[Kalle Valo]

Oleksij Rempel <linux@rempel-privat.de> writes:

>> 4) As Kalle mentioned, rtlwifi contains many magic numbers, and I 
>>    plan to fix them after rtl8723de and rtl8821ce. Because the drivers
>>    are developing, the changes will make us hard to integrate. However,
>>    I don't have plan to process the magic numbers in the module phydm,
>>    because the most of BB/RF registers contain many functions. And
>>    it doesn't have a register name but a bit field name instead.
>>    Our BB team guys say the use of enumeration or defined name will
>>    be unreadable, and the name is meaningless for most people.
>
> Experience with ath9k driver showed, that development was kind of
> balanced between two groups, QCA and Community (Other companies,
> researches, education and so on.). Saying: "you will not understand it
> any way" is nor really helpful :)
> Please don't repeat bad experience of Broadcom.

I agree with Oleksij here, but I want to still point out that there are
cases when using magic numbers are ok, for example look at
ar5008_initvals.h from ath9k. So it depends on case by case.

-- 
Kalle Valo

Re: Two rtlwifi drivers?

[Oleksij Rempel]

[-- Attachment #1.1: Type: text/plain, Size: 1433 bytes --]

Am 16.10.2017 um 15:07 schrieb Kalle Valo:
> Oleksij Rempel <linux@rempel-privat.de> writes:
> 
>>> 4) As Kalle mentioned, rtlwifi contains many magic numbers, and I 
>>>    plan to fix them after rtl8723de and rtl8821ce. Because the drivers
>>>    are developing, the changes will make us hard to integrate. However,
>>>    I don't have plan to process the magic numbers in the module phydm,
>>>    because the most of BB/RF registers contain many functions. And
>>>    it doesn't have a register name but a bit field name instead.
>>>    Our BB team guys say the use of enumeration or defined name will
>>>    be unreadable, and the name is meaningless for most people.
>>
>> Experience with ath9k driver showed, that development was kind of
>> balanced between two groups, QCA and Community (Other companies,
>> researches, education and so on.). Saying: "you will not understand it
>> any way" is nor really helpful :)
>> Please don't repeat bad experience of Broadcom.
> 
> I agree with Oleksij here, but I want to still point out that there are
> cases when using magic numbers are ok, for example look at
> ar5008_initvals.h from ath9k. So it depends on case by case.

Beside. It is probably offtopic. I assume this initvals related to BB.
Is it possible to force a 802.11n controller to work in 802.11b mode? I
can image, it should be possible by reconfiguring BB. Correct?
-- 
Regards,
Oleksij


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: Two rtlwifi drivers?

[Dan Carpenter]

On Mon, Oct 16, 2017 at 02:41:38AM +0000, Pkshih wrote:
> 2) The rtlwifi in staging
>    In staging, the module phydm v13 contains bugs, so I want to upgrade
>    to v21 (Realtek internal version number). This upgrade contains a
>    big patch that the difference between v13 and v21, and there are 
>    40+ patches to support this upgrade. I have three options, and
>    I want to know which one is prefer.

We can't discuss patches we haven't seend.  Please just send them right
away without any delay to both lists, and we can decide from there.

40 patches is a medium size set, patch that we are used to dealing with
every day.

regards,
dan carpenter

Re: Two rtlwifi drivers?

[Kalle Valo]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> On Mon, Oct 16, 2017 at 02:41:38AM +0000, Pkshih wrote:
>> 2) The rtlwifi in staging
>>    In staging, the module phydm v13 contains bugs, so I want to upgrade
>>    to v21 (Realtek internal version number). This upgrade contains a
>>    big patch that the difference between v13 and v21, and there are 
>>    40+ patches to support this upgrade. I have three options, and
>>    I want to know which one is prefer.
>
> We can't discuss patches we haven't seend.  Please just send them right
> away without any delay to both lists, and we can decide from there.
>
> 40 patches is a medium size set, patch that we are used to dealing with
> every day.

I review every patch I commit and 40 patches is just pure pain to dig
through. So when submitting patches to me please keep the size more
reasonable, like 10-12 patches per set. I think Dave also has a similar
rule for net patches.

I wrote a FAQ entry about this:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#too_many_patches

-- 
Kalle Valo

Re: Two rtlwifi drivers?

[Greg Kroah-Hartman]

On Mon, Oct 16, 2017 at 02:41:38AM +0000, Pkshih wrote:
> 
> 
> > -----Original Message-----
> > From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org]
> > Sent: Thursday, October 12, 2017 6:35 PM
> > To: Kalle Valo
> > Cc: Larry Finger; Dan Carpenter; Pkshih; 莊彥宣; Johannes Berg; Souptick Joarder;
> > devel@driverdev.osuosl.org; linux-wireless@vger.kernel.org; kernel-janitors@vger.kernel.org
> > Subject: Re: Two rtlwifi drivers?
> > 
> > On Thu, Oct 12, 2017 at 11:38:06AM +0300, Kalle Valo wrote:
> > > > So what to do?  Any ideas?  What makes your life easier?  You can just
> > > > ignore the staging tree, as it should not affect your portion of the
> > > > kernel at all, right?
> > >
> > > Yes, I automatically ignore anything staging related. But the problem is
> > > that we now have two drivers with the same name and people don't always
> > > remember to prefix the patch with "staging: ". So on a bad day I might
> > > accidentally apply a patch which was meant for your tree. Of course I
> > > immediately revert it as soon as I, or someone else, catches that but
> > > annoying still.
> > 
> > It doesn't bother me if you apply staging patches, I can handle the
> > merge issues :)
> > 
> > > I think we have two options here:
> > >
> > > 1) We set a deadline (like 12 months or something) for the
> > >    drivers/staging/rtlwifi and after that you refuse to take any patches
> > >    for it. Hopefully this makes it clear for everyone that this fork is
> > >    just temporary. I think Larry is trying to do this, which is great.
> > 
> > Fine with me, if Larry is ok with it.
> > 
> > > 2) We move the whole rtlwifi driver to staging. A very bad option but
> > >    still better than forking the drivers.
> > 
> > Ick, I don't want that to have to happen, that would not be good for the
> > users of other devices that the "real" rtlwifi driver supports.
> > 
> 
> Hi Larry, Kalle and Gerg,
> 
> This is Realtek engineer, PK. I appreciate your support to submit, review 
> and merge patch. Since I'm a Linux newbie, I'll describe the situation of 
> rtlwifi and need your suggestions.
> 
> 
> 1) New modules in rtlwifi
>    We add two modules named phydm and halmac, when adding rtl8822be in 
>    staging. The phydm is BB/RF related module containing the parameters
>    and APIs of BB/RF, and a dynamic mechanism to adapt to different
>    field environment. The halmac consists of MAC APIs.
>    The two modules are used by many OSs, so '#ifdef', CamelCase and
>    so on are existing in original files. Hence, we convert them to Linux 
>    form by script, but it's not perfect. Do you have suggestion to deal
>    with this problem?

Yes, use a script like you are doing, and then just work from the Linux
versions from then on.  Trying to do an OS independant layer almost
never works well over time, the only people that I have seen doing it
successfully is the ACPI core code, and for that, those developers do
have lots of scripts to turn their internal version into Linux-friendly
patches.

It takes a lot more work and time to try to do this, just working
directly on Linux is easier and cheaper over time :)

> 2) The rtlwifi in staging
>    In staging, the module phydm v13 contains bugs, so I want to upgrade
>    to v21 (Realtek internal version number). This upgrade contains a
>    big patch that the difference between v13 and v21, and there are 
>    40+ patches to support this upgrade. I have three options, and
>    I want to know which one is prefer.
> 
> 2.1) apply 40+ patches to both staging and wireless tree, and apply
>      the big patch to staging only. After reviewing, we move the module
>      to wireless tree.

I don't want a "big patch" for staging.

> 2.2) apply 40+ patches to wireless tree, and apply a single bigger 
>      patch containing 40+ patches and the big patch to staging. I think
>      this can be seen as a new driver in staging. After reviewing, 
>      we move the module to wireless tree.
> 
> 2.3) don't apply anything to staging. Just apply 40+ patches and add
>      phydm v21 to wireless.

This last option seems good to me, then move the driver that is in
staging to use the wireless core version instead?

> 3) Coming drivers -- rtl8723de and rtl8821ce
>    We're developing the two drivers, and rtl8723de and rtl8821ce will
>    be ready on 2017Q4 and 2018Q1 respectively. The drivers are based on
>    rtl8822be that in staging now, so the line of code will be fewer.
>    The new files will be a new IC folder and IC supported files of 
>    three modules that btcoexist, phydm and halmac. Could I submit
>    them to wirless tree when they're ready?

Sure, please submit them when they are ready, as long as they follow the
correct Linux style rules.

> 4) As Kalle mentioned, rtlwifi contains many magic numbers, and I 
>    plan to fix them after rtl8723de and rtl8821ce. Because the drivers
>    are developing, the changes will make us hard to integrate. However,
>    I don't have plan to process the magic numbers in the module phydm,
>    because the most of BB/RF registers contain many functions. And
>    it doesn't have a register name but a bit field name instead.
>    Our BB team guys say the use of enumeration or defined name will
>    be unreadable, and the name is meaningless for most people.

Don't be so sure that names are "meaningless", there are people here
that have been doing network drivers and development for longer than
anyone else in the world.  It's best to at least name values, even if
they do not seem to make sense, as that way the value can be tracked
correctly, and possibly be understood later by someone else, or even
you!

thanks,

greg k-h

RE: Two rtlwifi drivers?

[Pkshih]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogR3JlZyBLcm9haC1IYXJ0
bWFuIFttYWlsdG86Z3JlZ2toQGxpbnV4Zm91bmRhdGlvbi5vcmddDQo+IFNlbnQ6IE1vbmRheSwg
T2N0b2JlciAxNiwgMjAxNyAzOjUwIFBNDQo+IFRvOiBQa3NoaWgNCj4gQ2M6IExhcnJ5IEZpbmdl
cjsgS2FsbGUgVmFsbzsgRGFuIENhcnBlbnRlcjsg6I6K5b2l5a6jOyBKb2hhbm5lcyBCZXJnOyBT
b3VwdGljayBKb2FyZGVyOw0KPiBkZXZlbEBkcml2ZXJkZXYub3N1b3NsLm9yZzsgbGludXgtd2ly
ZWxlc3NAdmdlci5rZXJuZWwub3JnOyBrZXJuZWwtamFuaXRvcnNAdmdlci5rZXJuZWwub3JnDQo+
IFN1YmplY3Q6IFJlOiBUd28gcnRsd2lmaSBkcml2ZXJzPw0KPiANCj4gDQo+ID4gNCkgQXMgS2Fs
bGUgbWVudGlvbmVkLCBydGx3aWZpIGNvbnRhaW5zIG1hbnkgbWFnaWMgbnVtYmVycywgYW5kIEkN
Cj4gPiAgICBwbGFuIHRvIGZpeCB0aGVtIGFmdGVyIHJ0bDg3MjNkZSBhbmQgcnRsODgyMWNlLiBC
ZWNhdXNlIHRoZSBkcml2ZXJzDQo+ID4gICAgYXJlIGRldmVsb3BpbmcsIHRoZSBjaGFuZ2VzIHdp
bGwgbWFrZSB1cyBoYXJkIHRvIGludGVncmF0ZS4gSG93ZXZlciwNCj4gPiAgICBJIGRvbid0IGhh
dmUgcGxhbiB0byBwcm9jZXNzIHRoZSBtYWdpYyBudW1iZXJzIGluIHRoZSBtb2R1bGUgcGh5ZG0s
DQo+ID4gICAgYmVjYXVzZSB0aGUgbW9zdCBvZiBCQi9SRiByZWdpc3RlcnMgY29udGFpbiBtYW55
IGZ1bmN0aW9ucy4gQW5kDQo+ID4gICAgaXQgZG9lc24ndCBoYXZlIGEgcmVnaXN0ZXIgbmFtZSBi
dXQgYSBiaXQgZmllbGQgbmFtZSBpbnN0ZWFkLg0KPiA+ICAgIE91ciBCQiB0ZWFtIGd1eXMgc2F5
IHRoZSB1c2Ugb2YgZW51bWVyYXRpb24gb3IgZGVmaW5lZCBuYW1lIHdpbGwNCj4gPiAgICBiZSB1
bnJlYWRhYmxlLCBhbmQgdGhlIG5hbWUgaXMgbWVhbmluZ2xlc3MgZm9yIG1vc3QgcGVvcGxlLg0K
PiANCj4gRG9uJ3QgYmUgc28gc3VyZSB0aGF0IG5hbWVzIGFyZSAibWVhbmluZ2xlc3MiLCB0aGVy
ZSBhcmUgcGVvcGxlIGhlcmUNCj4gdGhhdCBoYXZlIGJlZW4gZG9pbmcgbmV0d29yayBkcml2ZXJz
IGFuZCBkZXZlbG9wbWVudCBmb3IgbG9uZ2VyIHRoYW4NCj4gYW55b25lIGVsc2UgaW4gdGhlIHdv
cmxkLiAgSXQncyBiZXN0IHRvIGF0IGxlYXN0IG5hbWUgdmFsdWVzLCBldmVuIGlmDQo+IHRoZXkg
ZG8gbm90IHNlZW0gdG8gbWFrZSBzZW5zZSwgYXMgdGhhdCB3YXkgdGhlIHZhbHVlIGNhbiBiZSB0
cmFja2VkDQo+IGNvcnJlY3RseSwgYW5kIHBvc3NpYmx5IGJlIHVuZGVyc3Rvb2QgbGF0ZXIgYnkg
c29tZW9uZSBlbHNlLCBvciBldmVuDQo+IHlvdSENCj4gDQoNClRoYW5rcyBmb3IgeW91ciBoZWxw
IHRvIHJlc29sdmUgb3VyIHF1ZXN0aW9ucy4NCkknbGwgcGFzcyB0aGlzIHBvaW50IHRvIHRoZSBn
dXlzLg0KDQpQSw0KDQo=

Re: Two rtlwifi drivers?

[Kalle Valo]

Hi PK,

you got good answers already so only short reply from me:

Pkshih <pkshih@realtek.com> writes:

> 3) Coming drivers -- rtl8723de and rtl8821ce
>    We're developing the two drivers, and rtl8723de and rtl8821ce will
>    be ready on 2017Q4 and 2018Q1 respectively. The drivers are based on
>    rtl8822be that in staging now, so the line of code will be fewer.
>    The new files will be a new IC folder and IC supported files of 
>    three modules that btcoexist, phydm and halmac. Could I submit
>    them to wirless tree when they're ready?

My recommendation is to avoid accumulating patches at all cost and start
submitting them as soon as you can. This way you get patches committed
much more smoother. So do not wait until _all_ patches are ready,
instead start submitting patches as soon as you have _some_ patches
ready. In other words, keep the delta between mainline and your
not-yet-submitted patches as small as possible.

And the patches don't need to be bug free as you can always fix bugs
later. Just mention in the commit logs that this is preparation for some
new feature and not fully tested yet. We do that all the time, for
example Intel's iwlwifi has support for hardware which have not reached
customers yet.

> On the way, I'll attend netdev workshop in Korea, so we can meet there
> if you attend too.

I'm also attending Netdev 2.2, looking forward to meeting you there.

-- 
Kalle Valo

RE: Two rtlwifi drivers?

[Pkshih]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogS2FsbGUgVmFsbyBbbWFp
bHRvOmt2YWxvQGNvZGVhdXJvcmEub3JnXQ0KPiBTZW50OiBNb25kYXksIE9jdG9iZXIgMTYsIDIw
MTcgOToyMyBQTQ0KPiBUbzogUGtzaGloDQo+IENjOiBMYXJyeSBGaW5nZXI7IEdyZWcgS3JvYWgt
SGFydG1hbjsgRGFuIENhcnBlbnRlcjsgsvir26vFOyBKb2hhbm5lcyBCZXJnOyBTb3VwdGljayBK
b2FyZGVyOw0KPiBkZXZlbEBkcml2ZXJkZXYub3N1b3NsLm9yZzsgbGludXgtd2lyZWxlc3NAdmdl
ci5rZXJuZWwub3JnOyBrZXJuZWwtamFuaXRvcnNAdmdlci5rZXJuZWwub3JnDQo+IFN1YmplY3Q6
IFJlOiBUd28gcnRsd2lmaSBkcml2ZXJzPw0KPiANCj4gSGkgUEssDQo+IA0KPiB5b3UgZ290IGdv
b2QgYW5zd2VycyBhbHJlYWR5IHNvIG9ubHkgc2hvcnQgcmVwbHkgZnJvbSBtZToNCj4gDQo+IFBr
c2hpaCA8cGtzaGloQHJlYWx0ZWsuY29tPiB3cml0ZXM6DQo+IA0KPiA+IDMpIENvbWluZyBkcml2
ZXJzIC0tIHJ0bDg3MjNkZSBhbmQgcnRsODgyMWNlDQo+ID4gICAgV2UncmUgZGV2ZWxvcGluZyB0
aGUgdHdvIGRyaXZlcnMsIGFuZCBydGw4NzIzZGUgYW5kIHJ0bDg4MjFjZSB3aWxsDQo+ID4gICAg
YmUgcmVhZHkgb24gMjAxN1E0IGFuZCAyMDE4UTEgcmVzcGVjdGl2ZWx5LiBUaGUgZHJpdmVycyBh
cmUgYmFzZWQgb24NCj4gPiAgICBydGw4ODIyYmUgdGhhdCBpbiBzdGFnaW5nIG5vdywgc28gdGhl
IGxpbmUgb2YgY29kZSB3aWxsIGJlIGZld2VyLg0KPiA+ICAgIFRoZSBuZXcgZmlsZXMgd2lsbCBi
ZSBhIG5ldyBJQyBmb2xkZXIgYW5kIElDIHN1cHBvcnRlZCBmaWxlcyBvZg0KPiA+ICAgIHRocmVl
IG1vZHVsZXMgdGhhdCBidGNvZXhpc3QsIHBoeWRtIGFuZCBoYWxtYWMuIENvdWxkIEkgc3VibWl0
DQo+ID4gICAgdGhlbSB0byB3aXJsZXNzIHRyZWUgd2hlbiB0aGV5J3JlIHJlYWR5Pw0KPiANCj4g
TXkgcmVjb21tZW5kYXRpb24gaXMgdG8gYXZvaWQgYWNjdW11bGF0aW5nIHBhdGNoZXMgYXQgYWxs
IGNvc3QgYW5kIHN0YXJ0DQo+IHN1Ym1pdHRpbmcgdGhlbSBhcyBzb29uIGFzIHlvdSBjYW4uIFRo
aXMgd2F5IHlvdSBnZXQgcGF0Y2hlcyBjb21taXR0ZWQNCj4gbXVjaCBtb3JlIHNtb290aGVyLiBT
byBkbyBub3Qgd2FpdCB1bnRpbCBfYWxsXyBwYXRjaGVzIGFyZSByZWFkeSwNCj4gaW5zdGVhZCBz
dGFydCBzdWJtaXR0aW5nIHBhdGNoZXMgYXMgc29vbiBhcyB5b3UgaGF2ZSBfc29tZV8gcGF0Y2hl
cw0KPiByZWFkeS4gSW4gb3RoZXIgd29yZHMsIGtlZXAgdGhlIGRlbHRhIGJldHdlZW4gbWFpbmxp
bmUgYW5kIHlvdXINCj4gbm90LXlldC1zdWJtaXR0ZWQgcGF0Y2hlcyBhcyBzbWFsbCBhcyBwb3Nz
aWJsZS4NCj4gDQo+IEFuZCB0aGUgcGF0Y2hlcyBkb24ndCBuZWVkIHRvIGJlIGJ1ZyBmcmVlIGFz
IHlvdSBjYW4gYWx3YXlzIGZpeCBidWdzDQo+IGxhdGVyLiBKdXN0IG1lbnRpb24gaW4gdGhlIGNv
bW1pdCBsb2dzIHRoYXQgdGhpcyBpcyBwcmVwYXJhdGlvbiBmb3Igc29tZQ0KPiBuZXcgZmVhdHVy
ZSBhbmQgbm90IGZ1bGx5IHRlc3RlZCB5ZXQuIFdlIGRvIHRoYXQgYWxsIHRoZSB0aW1lLCBmb3IN
Cj4gZXhhbXBsZSBJbnRlbCdzIGl3bHdpZmkgaGFzIHN1cHBvcnQgZm9yIGhhcmR3YXJlIHdoaWNo
IGhhdmUgbm90IHJlYWNoZWQNCj4gY3VzdG9tZXJzIHlldC4NCj4gDQoNClRoYW5rcyBmb3IgeW91
ciBhbnN3ZXIuIEknbGwgc3VibWl0IHBhdGNoZXMgd2hlbiB0aGUgZHJpdmVycyBhcmUgcmVhZHkg
YW5kDQpzdGFibGUuIEkgaGF2ZSBhbm90aGVyIHF1ZXN0aW9uIGFib3V0IHRoZSBydWxlcyBvZiBu
ZXcgZmlsZXMuIElmIEkgd2FudCB0bw0KYWRkIHNvbWUgbmV3IGZpbGVzLCBjb3VsZCBJIHNlbmQg
YSBiaWcgcGF0Y2ggd2l0aCBhbGwgbmV3IGZpbGVzPyBJcyB0aGVyZQ0KYW55IGxpbWl0PyANCg0K
VGhhbmtzDQpQSw0KDQoNCg==

Re: Two rtlwifi drivers?

[Kalle Valo]

Pkshih <pkshih@realtek.com> writes:

>> My recommendation is to avoid accumulating patches at all cost and start
>> submitting them as soon as you can. This way you get patches committed
>> much more smoother. So do not wait until _all_ patches are ready,
>> instead start submitting patches as soon as you have _some_ patches
>> ready. In other words, keep the delta between mainline and your
>> not-yet-submitted patches as small as possible.
>> 
>> And the patches don't need to be bug free as you can always fix bugs
>> later. Just mention in the commit logs that this is preparation for some
>> new feature and not fully tested yet. We do that all the time, for
>> example Intel's iwlwifi has support for hardware which have not reached
>> customers yet.
>> 
>
> Thanks for your answer. I'll submit patches when the drivers are ready and
> stable. I have another question about the rules of new files. If I want to
> add some new files, could I send a big patch with all new files? Is there
> any limit? 

The only limit I know is the byte size limit in the mailing list. When
submitting a new driver some people split the driver to one file per
patch for easier review but the final commit needs to be one big patch
with all files included. If you are adding big files to rtlwifi I think
you could try to follow the same model.

-- 
Kalle Valo

Re: [PATCH] staging: rtlwifi: check for array overflow

[Larry Finger]

On 08/24/2017 05:08 AM, Dan Carpenter wrote:
> Smatch is distrustful of the "capab" value and marks it as user
> controlled.  I think it actually comes from the firmware?  Anyway, I
> looked at other drivers and they added a bounds check and it seems like
> a harmless thing to have so I have added it here as well.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Acked-by: Larry Finger <Larry.Finger@lwfinger.net>

Thanks,

Larry

> 
> diff --git a/drivers/staging/rtlwifi/base.c b/drivers/staging/rtlwifi/base.c
> index f7f207cbaee3..a30b928d5ee1 100644
> --- a/drivers/staging/rtlwifi/base.c
> +++ b/drivers/staging/rtlwifi/base.c
> @@ -1414,6 +1414,10 @@ bool rtl_action_proc(struct ieee80211_hw *hw, struct sk_buff *skb, u8 is_tx)
>   				  le16_to_cpu(mgmt->u.action.u.addba_req.capab);
>   				tid = (capab &
>   				       IEEE80211_ADDBA_PARAM_TID_MASK) >> 2;
> +				if (tid >= MAX_TID_COUNT) {
> +					rcu_read_unlock();
> +					return true;
> +				}
>   				tid_data = &sta_entry->tids[tid];
>   				if (tid_data->agg.rx_agg_state ==
>   				    RTL_RX_AGG_START)
>