[permalink] [raw]

Subject: 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 <[email protected]>
> > ---
> > 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

2018-08-30 21:37:15

by John Whitmore

[permalink] [raw]

Subject: 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 <[email protected]>
> > > ---
> > > 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

2018-08-30 22:36:08

by Joe Perches

[permalink] [raw]

Subject: 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/rtl8xxxxu 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.

2018-08-31 07:28:13

by Dan Carpenter

[permalink] [raw]

Subject: 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 <[email protected]>
> > > > ---
> > > > 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

2018-08-31 22:29:46

by John Whitmore

[permalink] [raw]

Subject: 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/rtl8xxxxu 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/rtl8xxxxu, 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.

2018-09-10 15:11:04

by Greg Kroah-Hartman

[permalink] [raw]

Subject: 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 <[email protected]>
> ---
> 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