Re: [PATCH 01/21] staging:rtl8192u: Rename AdvCoding - Style
On Wed, Aug 29, 2018 at 09:35:27PM +0100, John Whitmore wrote: > Rename the bit field element AdvCoding, as it causes a checkpatch issue > with CamelCase naming. As the element is not actually used in code it > has been renamed to 'not_used_adv_coding'. > > The single line of code which initialises the bit has been removed, > as the field is unused. > > This is a purely coding style change which should have no impact > on runtime code execution. > > Signed-off-by: John Whitmore > --- > drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h | 2 +- > drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c | 1 - > 2 files changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h > b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h > index 64d5359cf7e2..66a0274077d3 100644 > --- a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h > +++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h > @@ -39,7 +39,7 @@ enum ht_extension_chan_offset { > > struct ht_capability_ele { > //HT capability info > - u8 AdvCoding:1; > + u8 not_used_adv_coding:1; > u8 ChlWidth:1; > u8 MimoPwrSave:2; > u8 GreenField:1; > diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c > b/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c > index 9bf52cebe4cd..d7f73a5831da 100644 > --- a/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c > +++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c > @@ -550,7 +550,6 @@ void HTConstructCapabilityElement(struct ieee80211_device > *ieee, u8 *posHTCap, u > } > > //HT capability info > - pCapELE->AdvCoding = 0; // This feature is not supported > now!! > if (ieee->GetHalfNmodeSupportByAPsHandler(ieee->dev)) > pCapELE->ChlWidth = 0; > else If a structure like this, which is not read/written to hardware directly, has fields that are not really used at all, just delete them. That's much easier than having to apply this patch, and a later patch which deletes the usage. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 01/21] staging:rtl8192u: Rename AdvCoding - Style
On Thu, Aug 30, 2018 at 03:34:01PM -0700, Joe Perches wrote: > On Wed, 2018-08-29 at 22:55 +0100, John Whitmore wrote: > > On Wed, Aug 29, 2018 at 04:21:54PM -0500, Larry Finger wrote: > > > On 08/29/2018 04:14 PM, Joe Perches wrote: > [] > > > > Perhaps support for the chipset could be converted to use > > > > mac80211 and moved into the directory with the other realtek > > > > drivers in drivers/net/wireless/realtek/rtl8xxxu/... > [] > > > All the Realtek USB devices should be added to rtl8xxxu, not merely moved > > > into that directory. Jes Sorensen created a well-designed driver the is > > > structured to permit addition of different initialization routines, etc. > > > That said, the conversion will not be easy. In addition, it will require > > > having your hands on a real device - a requirement that I cannot meet for > > > the RTL8192U. > [] > > > Oops... it probably doesn't look like it, at the moment, but 'actual code > > changes to this driver' was where I'm hoping to get to. There's a lot of > > stuff in the header files, including member variables, which are not used > > in the code. I was hoping to strip these out incrementally to minimise > > the amount of source code that has to be understood. I'm not familiar > > with the overall network, or 80211, subsystem architecture, but it's hard > > to see why this driver has it's own private ieee80211.h file. > > That's true and the reason why this code should > eventually be deleted if possible and its functionality > integrated into the existing realtek/rtl8u code. > > > I must try and get my hands on the device in question. > > That'd be good, though likely this device is obsolete. > Good luck finding one. > Maybe if the whole driver was deleted nobody would notice ;) Thanks for your help. I'm afraid I carried on in the same vain cleaning stuff up but I do have to look at realtek/rtl8u, thanks for pointing out where the goalposts are, miles away ;) > > Thank ye for your comments, and sorry for the "somewhat useful" changes > > to date, but doing these changes is letting me get the hang of this code, > > which I refuse to comment on because there are probably guidelines on using > > naughty language on here. ;) > > Feel free. > > The one look I took at any existing multi-platform realtek > code with all the #ifdefs made me not want look at it again. > > https://github.com/hadess/rtl8723bs/ > > I said a naughty under my breath too. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 01/21] staging:rtl8192u: Rename AdvCoding - Style
On Thu, Aug 30, 2018 at 10:35:37PM +0100, John Whitmore wrote: > On Thu, Aug 30, 2018 at 11:26:28AM +0300, Dan Carpenter wrote: > > On Thu, Aug 30, 2018 at 11:23:05AM +0300, Dan Carpenter wrote: > > > On Wed, Aug 29, 2018 at 09:35:27PM +0100, John Whitmore wrote: > > > > Rename the bit field element AdvCoding, as it causes a checkpatch issue > > > > with CamelCase naming. As the element is not actually used in code it > > > > has been renamed to 'not_used_adv_coding'. > > > > > > > > The single line of code which initialises the bit has been removed, > > > > as the field is unused. > > > > > > > > This is a purely coding style change which should have no impact > > > > on runtime code execution. > > > > > > > > Signed-off-by: John Whitmore > > > > --- > > > > drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h | 2 +- > > > > drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c | 1 - > > > > 2 files changed, 1 insertion(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h > > > > b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h > > > > index 64d5359cf7e2..66a0274077d3 100644 > > > > --- a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h > > > > +++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h > > > > @@ -39,7 +39,7 @@ enum ht_extension_chan_offset { > > > > > > > > struct ht_capability_ele { > > > > //HT capability info > > > > - u8 AdvCoding:1; > > > > + u8 not_used_adv_coding:1; > > > > > > I can't review any more of this patchset when this is the first line... > > > > > > > That email wasn't very clear. If you think some of your patches are > > going to be more controversial than others, put them at the very end so > > we can at least apply part of the patchset. > > > > regards, > > dan carpenter > > > > Thanks for the clarification, I needed it ;) > > I'm not sure I consider any of the patches in the series as being > controversial. They are all just simple name changes of member > variables. As a number of the variables are not used in code the names > have been changed to reflect that fact. If I'd renamed them and left out > the 'not_used_' prefix would the changes not be controversial? I > don't think the fact that the members are unused is a biggy. > I didn't like the new name at all. Quite often when I review these I just use a script to verify that we're only renaming variables and not doing code changes outside of that. But if I review it by hand I would have seen that the variable was not used and investigated what was going on. > What might appear to be controversial is that I didn't simply remove > the members. I haven't done that because I'm not yet satisfied that > the structure's size is insignificant. As soon as I am happy that the > size of the structure is not important, to runtime code execution, > there'll be a patch which removes a number of 'not_used_...' members > from a structure. > > Alternatively if the size if important there might be a patch which > renames a number of unused bitfield members to reserved_1, reserved_2, > reserved_3 That's a good question. If a struct is part of the use space API or the network protocol or defined by the hardware then we can't change the layout. One thing I look for is if the struct has pointers which aren't tagged as __user pointers then it's internal to the kernel. Unfortunately that doesn't help here. Network protocol is all endian specific and this struct is not. But again that doesn't help much because it could just be a bug. If the struct is __packed then obviously the layout matters. So I don't know. I think you're right that actually we can't change the layout. The code is really a mess, so it's possible I have misread. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 01/21] staging:rtl8192u: Rename AdvCoding - Style
On Wed, 2018-08-29 at 22:55 +0100, John Whitmore wrote: > On Wed, Aug 29, 2018 at 04:21:54PM -0500, Larry Finger wrote: > > On 08/29/2018 04:14 PM, Joe Perches wrote: [] > > > Perhaps support for the chipset could be converted to use > > > mac80211 and moved into the directory with the other realtek > > > drivers in drivers/net/wireless/realtek/rtl8xxxu/... [] > > All the Realtek USB devices should be added to rtl8xxxu, not merely moved > > into that directory. Jes Sorensen created a well-designed driver the is > > structured to permit addition of different initialization routines, etc. > > That said, the conversion will not be easy. In addition, it will require > > having your hands on a real device - a requirement that I cannot meet for > > the RTL8192U. [] > > Oops... it probably doesn't look like it, at the moment, but 'actual code > changes to this driver' was where I'm hoping to get to. There's a lot of > stuff in the header files, including member variables, which are not used > in the code. I was hoping to strip these out incrementally to minimise > the amount of source code that has to be understood. I'm not familiar > with the overall network, or 80211, subsystem architecture, but it's hard > to see why this driver has it's own private ieee80211.h file. That's true and the reason why this code should eventually be deleted if possible and its functionality integrated into the existing realtek/rtl8u code. > I must try and get my hands on the device in question. That'd be good, though likely this device is obsolete. Good luck finding one. > Thank ye for your comments, and sorry for the "somewhat useful" changes > to date, but doing these changes is letting me get the hang of this code, > which I refuse to comment on because there are probably guidelines on using > naughty language on here. ;) Feel free. The one look I took at any existing multi-platform realtek code with all the #ifdefs made me not want look at it again. https://github.com/hadess/rtl8723bs/ I said a naughty under my breath too. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 01/21] staging:rtl8192u: Rename AdvCoding - Style
On Thu, Aug 30, 2018 at 11:26:28AM +0300, Dan Carpenter wrote: > On Thu, Aug 30, 2018 at 11:23:05AM +0300, Dan Carpenter wrote: > > On Wed, Aug 29, 2018 at 09:35:27PM +0100, John Whitmore wrote: > > > Rename the bit field element AdvCoding, as it causes a checkpatch issue > > > with CamelCase naming. As the element is not actually used in code it > > > has been renamed to 'not_used_adv_coding'. > > > > > > The single line of code which initialises the bit has been removed, > > > as the field is unused. > > > > > > This is a purely coding style change which should have no impact > > > on runtime code execution. > > > > > > Signed-off-by: John Whitmore > > > --- > > > drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h | 2 +- > > > drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c | 1 - > > > 2 files changed, 1 insertion(+), 2 deletions(-) > > > > > > diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h > > > b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h > > > index 64d5359cf7e2..66a0274077d3 100644 > > > --- a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h > > > +++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h > > > @@ -39,7 +39,7 @@ enum ht_extension_chan_offset { > > > > > > struct ht_capability_ele { > > > //HT capability info > > > - u8 AdvCoding:1; > > > + u8 not_used_adv_coding:1; > > > > I can't review any more of this patchset when this is the first line... > > > > That email wasn't very clear. If you think some of your patches are > going to be more controversial than others, put them at the very end so > we can at least apply part of the patchset. > > regards, > dan carpenter > Thanks for the clarification, I needed it ;) I'm not sure I consider any of the patches in the series as being controversial. They are all just simple name changes of member variables. As a number of the variables are not used in code the names have been changed to reflect that fact. If I'd renamed them and left out the 'not_used_' prefix would the changes not be controversial? I don't think the fact that the members are unused is a biggy. What might appear to be controversial is that I didn't simply remove the members. I haven't done that because I'm not yet satisfied that the structure's size is insignificant. As soon as I am happy that the size of the structure is not important, to runtime code execution, there'll be a patch which removes a number of 'not_used_...' members from a structure. Alternatively if the size if important there might be a patch which renames a number of unused bitfield members to reserved_1, reserved_2, reserved_3 Perhaps I should have held off this patch set until I had satisfied myself that the size either, does or does not, matter and combine all changes into one series of patches. On the other hand I've added an extra step which can be reviewed and clearly see the incremental changes. I'm happy enough for the whole patch set to be rejected for the moment, but I'd be even happier if on review somebody pointed out that a member which I have decided is unused in the code is actually used and a patch gets rejected on those grounds. If everybody accepts this series, that asserts that members are unused, the next step is easy either way. jwhitmore ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 01/21] staging:rtl8192u: Rename AdvCoding - Style
On Thu, Aug 30, 2018 at 11:23:05AM +0300, Dan Carpenter wrote: > On Wed, Aug 29, 2018 at 09:35:27PM +0100, John Whitmore wrote: > > Rename the bit field element AdvCoding, as it causes a checkpatch issue > > with CamelCase naming. As the element is not actually used in code it > > has been renamed to 'not_used_adv_coding'. > > > > The single line of code which initialises the bit has been removed, > > as the field is unused. > > > > This is a purely coding style change which should have no impact > > on runtime code execution. > > > > Signed-off-by: John Whitmore > > --- > > drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h | 2 +- > > drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c | 1 - > > 2 files changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h > > b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h > > index 64d5359cf7e2..66a0274077d3 100644 > > --- a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h > > +++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h > > @@ -39,7 +39,7 @@ enum ht_extension_chan_offset { > > > > struct ht_capability_ele { > > //HT capability info > > - u8 AdvCoding:1; > > + u8 not_used_adv_coding:1; > > I can't review any more of this patchset when this is the first line... > That email wasn't very clear. If you think some of your patches are going to be more controversial than others, put them at the very end so we can at least apply part of the patchset. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 01/21] staging:rtl8192u: Rename AdvCoding - Style
On Wed, Aug 29, 2018 at 09:35:27PM +0100, John Whitmore wrote: > Rename the bit field element AdvCoding, as it causes a checkpatch issue > with CamelCase naming. As the element is not actually used in code it > has been renamed to 'not_used_adv_coding'. > > The single line of code which initialises the bit has been removed, > as the field is unused. > > This is a purely coding style change which should have no impact > on runtime code execution. > > Signed-off-by: John Whitmore > --- > drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h | 2 +- > drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c | 1 - > 2 files changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h > b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h > index 64d5359cf7e2..66a0274077d3 100644 > --- a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h > +++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h > @@ -39,7 +39,7 @@ enum ht_extension_chan_offset { > > struct ht_capability_ele { > //HT capability info > - u8 AdvCoding:1; > + u8 not_used_adv_coding:1; I can't review any more of this patchset when this is the first line... :( regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 01/21] staging:rtl8192u: Rename AdvCoding - Style
On Wed, Aug 29, 2018 at 04:21:54PM -0500, Larry Finger wrote: > On 08/29/2018 04:14 PM, Joe Perches wrote: > > On Wed, 2018-08-29 at 21:35 +0100, John Whitmore wrote: > > > Rename the bit field element AdvCoding, as it causes a checkpatch issue > > > with CamelCase naming. As the element is not actually used in code it > > > has been renamed to 'not_used_adv_coding'. > > > > > > The single line of code which initialises the bit has been removed, > > > as the field is unused. > > > > > > This is a purely coding style change which should have no impact > > > on runtime code execution. > > > > Hi John. > > > > Other than the somewhat useful code style cleanups, is there > > a point at which you will feel comfortable doing actual code > > changes to this driver? > > > > Perhaps support for the chipset could be converted to use > > mac80211 and moved into the directory with the other realtek > > drivers in drivers/net/wireless/realtek/rtl8xxxu/... > > > > Larry? What do you think? > > First of all, if a variable is not used, then it should be removed, not > merely renamed to satisfy checkpatch. > > All the Realtek USB devices should be added to rtl8xxxu, not merely moved > into that directory. Jes Sorensen created a well-designed driver the is > structured to permit addition of different initialization routines, etc. > That said, the conversion will not be easy. In addition, it will require > having your hands on a real device - a requirement that I cannot meet for > the RTL8192U. > > Larry > Oops... it probably doesn't look like it, at the moment, but 'actual code changes to this driver' was where I'm hoping to get to. There's a lot of stuff in the header files, including member variables, which are not used in the code. I was hoping to strip these out incrementally to minimise the amount of source code that has to be understood. I'm not familiar with the overall network, or 80211, subsystem architecture, but it's hard to see why this driver has it's own private ieee80211.h file. I totally agree that the unused variables should be removed, and they will be, if we can accept that they are in fact unused && size doesn't matter. The structure in question 'struct ht_capability_ele' is used in, what I consider to be, a strange way in the file ieee80211_wx.c. struct ht_capability_ele *ht_cap = NULL; ... ht_cap = (struct ht_capability_ele *)&network->bssht.bdHTCapBuf[4]; ... ht_cap = (struct ht_capability_ele *)&network->bssht.bdHTCapBuf[0]; I have been trying to find a datasheet for this device but to date me query strings haven't given me much joy. I must try and get my hands on the device in question. Thank ye for your comments, and sorry for the "somewhat useful" changes to date, but doing these changes is letting me get the hang of this code, which I refuse to comment on because there are probably guidelines on using naughty language on here. ;) When I see lines like these two: ht_cap = (struct ht_capability_ele *)&network->bssht.bdHTCapBuf[4]; ... ht_cap = (struct ht_capability_ele *)&network->bssht.bdHTCapBuf[0]; I think that size might very well matter for the moment. The BSS_HT structure is farther down the header file so when I clean up that struct things might be clearer. I can only hope. jwhitmore ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 01/21] staging:rtl8192u: Rename AdvCoding - Style
On 08/29/2018 04:14 PM, Joe Perches wrote: On Wed, 2018-08-29 at 21:35 +0100, John Whitmore wrote: Rename the bit field element AdvCoding, as it causes a checkpatch issue with CamelCase naming. As the element is not actually used in code it has been renamed to 'not_used_adv_coding'. The single line of code which initialises the bit has been removed, as the field is unused. This is a purely coding style change which should have no impact on runtime code execution. Hi John. Other than the somewhat useful code style cleanups, is there a point at which you will feel comfortable doing actual code changes to this driver? Perhaps support for the chipset could be converted to use mac80211 and moved into the directory with the other realtek drivers in drivers/net/wireless/realtek/rtl8xxxu/... Larry? What do you think? First of all, if a variable is not used, then it should be removed, not merely renamed to satisfy checkpatch. All the Realtek USB devices should be added to rtl8xxxu, not merely moved into that directory. Jes Sorensen created a well-designed driver the is structured to permit addition of different initialization routines, etc. That said, the conversion will not be easy. In addition, it will require having your hands on a real device - a requirement that I cannot meet for the RTL8192U. Larry ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 01/21] staging:rtl8192u: Rename AdvCoding - Style
On Wed, 2018-08-29 at 21:35 +0100, John Whitmore wrote: > Rename the bit field element AdvCoding, as it causes a checkpatch issue > with CamelCase naming. As the element is not actually used in code it > has been renamed to 'not_used_adv_coding'. > > The single line of code which initialises the bit has been removed, > as the field is unused. > > This is a purely coding style change which should have no impact > on runtime code execution. Hi John. Other than the somewhat useful code style cleanups, is there a point at which you will feel comfortable doing actual code changes to this driver? Perhaps support for the chipset could be converted to use mac80211 and moved into the directory with the other realtek drivers in drivers/net/wireless/realtek/rtl8xxxu/... Larry? What do you think? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 01/21] staging:rtl8192u: Rename AdvCoding - Style
Rename the bit field element AdvCoding, as it causes a checkpatch issue with CamelCase naming. As the element is not actually used in code it has been renamed to 'not_used_adv_coding'. The single line of code which initialises the bit has been removed, as the field is unused. This is a purely coding style change which should have no impact on runtime code execution. Signed-off-by: John Whitmore --- drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h | 2 +- drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h index 64d5359cf7e2..66a0274077d3 100644 --- a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h +++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h @@ -39,7 +39,7 @@ enum ht_extension_chan_offset { struct ht_capability_ele { //HT capability info - u8 AdvCoding:1; + u8 not_used_adv_coding:1; u8 ChlWidth:1; u8 MimoPwrSave:2; u8 GreenField:1; diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c b/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c index 9bf52cebe4cd..d7f73a5831da 100644 --- a/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c +++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c @@ -550,7 +550,6 @@ void HTConstructCapabilityElement(struct ieee80211_device *ieee, u8 *posHTCap, u } //HT capability info - pCapELE->AdvCoding = 0; // This feature is not supported now!! if (ieee->GetHalfNmodeSupportByAPsHandler(ieee->dev)) pCapELE->ChlWidth = 0; else -- 2.18.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel