| Message ID | 20170824100832.lcmbwcjhzwlgozeh@mwanda (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
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.)
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
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) >
(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.
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
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
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
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.
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.
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
> -----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. 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. PK
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!
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
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
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
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.
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?
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.
> -----Original Message----- > From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org] > Sent: Monday, October 16, 2017 3:50 PM > To: Pkshih > Cc: Larry Finger; Kalle Valo; Dan Carpenter; 莊彥宣; Johannes Berg; Souptick Joarder; > devel@driverdev.osuosl.org; linux-wireless@vger.kernel.org; kernel-janitors@vger.kernel.org > Subject: Re: Two rtlwifi drivers? > > > > 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 for your help to resolve our questions. I'll pass this point to the guys. PK
> -----Original Message----- > From: Kalle Valo [mailto:kvalo@codeaurora.org] > Sent: Monday, October 16, 2017 9:23 PM > To: Pkshih > Cc: Larry Finger; Greg Kroah-Hartman; Dan Carpenter; 莊彥宣; Johannes Berg; Souptick Joarder; > devel@driverdev.osuosl.org; linux-wireless@vger.kernel.org; kernel-janitors@vger.kernel.org > Subject: Re: Two rtlwifi drivers? > > 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. > 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? Thanks PK
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.
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)
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>