Re: [PATCH 05/13] staging:rtl8192u: Remove AdvCoding and GreenField - Style
On Fri, Sep 28, 2018 at 02:35:50PM +0200, Greg KH wrote:
> On Wed, Sep 26, 2018 at 08:16:56PM +0100, John Whitmore wrote:
> > The member variables AdvCoding and GreenField are unused in code so
> > have been removed from the structure and associated initialisation
> > function.
> >
> > This is a 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 | 2 --
> > 2 files changed, 4 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> > b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> > index 64d5359cf7e2..83fb8f34ccbd 100644
> > --- a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> > +++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> > @@ -39,10 +39,8 @@ enum ht_extension_chan_offset {
> >
> > struct ht_capability_ele {
> > //HT capability info
> > - u8 AdvCoding:1;
> > u8 ChlWidth:1;
> > u8 MimoPwrSave:2;
> > - u8 GreenField:1;
>
> Don't these fields come from the hardware itself? By removing them
> here, you just changed the memory layout of the structure. Does the
> driver still work properly after this? If you can't test it, I can't
> take this patch as it's too risky...
>
Sorry, yes the structure looks like it should come from the hardware
but as the structure is allocated from memory I expected to find a
memcopy either to or from the hardware. Yes risky, just because I
couldn't find it don't mean the connection to hardware ain't there.
I'll lay off the risky and who knows if I keep wondering through the
driver I'll find that illusive connection.
Re: [PATCH 05/13] staging:rtl8192u: Remove AdvCoding and GreenField - Style
On Fri, Sep 28, 2018 at 02:35:50PM +0200, Greg KH wrote:
> On Wed, Sep 26, 2018 at 08:16:56PM +0100, John Whitmore wrote:
> > The member variables AdvCoding and GreenField are unused in code so
> > have been removed from the structure and associated initialisation
> > function.
> >
> > This is a 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 | 2 --
> > 2 files changed, 4 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> > b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> > index 64d5359cf7e2..83fb8f34ccbd 100644
> > --- a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> > +++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> > @@ -39,10 +39,8 @@ enum ht_extension_chan_offset {
> >
> > struct ht_capability_ele {
> > //HT capability info
> > - u8 AdvCoding:1;
> > u8 ChlWidth:1;
> > u8 MimoPwrSave:2;
> > - u8 GreenField:1;
>
> Don't these fields come from the hardware itself? By removing them
> here, you just changed the memory layout of the structure. Does the
> driver still work properly after this? If you can't test it, I can't
> take this patch as it's too risky...
>
Sorry, yes the structure looks like it should come from the hardware
but as the structure is allocated from memory I expected to find a
memcopy either to or from the hardware. Yes risky, just because I
couldn't find it don't mean the connection to hardware ain't there.
I'll lay off the risky and who knows if I keep wondering through the
driver I'll find that illusive connection.
Re: [PATCH 05/13] staging:rtl8192u: Remove AdvCoding and GreenField - Style
On Fri, Sep 28, 2018 at 05:31:40PM +0300, Dan Carpenter wrote:
> On Wed, Sep 26, 2018 at 08:16:56PM +0100, John Whitmore wrote:
> > The member variables AdvCoding and GreenField are unused in code so
> > have been removed from the structure and associated initialisation
> > function.
> >
> > This is a 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 | 2 --
> > 2 files changed, 4 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> > b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> > index 64d5359cf7e2..83fb8f34ccbd 100644
> > --- a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> > +++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> > @@ -39,10 +39,8 @@ enum ht_extension_chan_offset {
> >
> > struct ht_capability_ele {
> > //HT capability info
> > - u8 AdvCoding:1;
> > u8 ChlWidth:1;
> > u8 MimoPwrSave:2;
> > - u8 GreenField:1;
> > u8 ShortGI20Mhz:1;
> > u8 ShortGI40Mhz:1;
> > u8 TxSTBC:1;
>
> I feel like we discussed this before. I'm pretty sure this comes from
> the firmware and so the format can't be changed. When I look at
> rtllib_parse_mife_generic() then I think that "info_element" probably
> comes from the firmware.
>
> I wouldn't want to accept this with out someone testing it.
>
> regards,
> dan carpenter
>
Thank you and sorry about not helping the signal to noise ratio on here.
I agree that a bit field like that and it looks like it comes from
firmware, but my question or possibly obsession was where. There are
structures inside structures, but they are all allocated from RAM.
Because of that I expected to find a memcopy from the device, or given
that the bitfield is initialised with values that it might be memcopy'd
to the device. I just couldn't find that memcopy, but that's down to
my untrained eye. I'll stumble across it in some obscure corner of the
driver.
Re: [PATCH 05/13] staging:rtl8192u: Remove AdvCoding and GreenField - Style
On Fri, Sep 28, 2018 at 05:31:40PM +0300, Dan Carpenter wrote:
> On Wed, Sep 26, 2018 at 08:16:56PM +0100, John Whitmore wrote:
> > The member variables AdvCoding and GreenField are unused in code so
> > have been removed from the structure and associated initialisation
> > function.
> >
> > This is a 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 | 2 --
> > 2 files changed, 4 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> > b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> > index 64d5359cf7e2..83fb8f34ccbd 100644
> > --- a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> > +++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> > @@ -39,10 +39,8 @@ enum ht_extension_chan_offset {
> >
> > struct ht_capability_ele {
> > //HT capability info
> > - u8 AdvCoding:1;
> > u8 ChlWidth:1;
> > u8 MimoPwrSave:2;
> > - u8 GreenField:1;
> > u8 ShortGI20Mhz:1;
> > u8 ShortGI40Mhz:1;
> > u8 TxSTBC:1;
>
> I feel like we discussed this before. I'm pretty sure this comes from
> the firmware and so the format can't be changed. When I look at
> rtllib_parse_mife_generic() then I think that "info_element" probably
> comes from the firmware.
>
> I wouldn't want to accept this with out someone testing it.
>
> regards,
> dan carpenter
>
Thank you and sorry about not helping the signal to noise ratio on here.
I agree that a bit field like that and it looks like it comes from
firmware, but my question or possibly obsession was where. There are
structures inside structures, but they are all allocated from RAM.
Because of that I expected to find a memcopy from the device, or given
that the bitfield is initialised with values that it might be memcopy'd
to the device. I just couldn't find that memcopy, but that's down to
my untrained eye. I'll stumble across it in some obscure corner of the
driver.
Re: [PATCH 05/13] staging:rtl8192u: Remove AdvCoding and GreenField - Style
On Wed, Sep 26, 2018 at 08:16:56PM +0100, John Whitmore wrote:
> The member variables AdvCoding and GreenField are unused in code so
> have been removed from the structure and associated initialisation
> function.
>
> This is a 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 | 2 --
> 2 files changed, 4 deletions(-)
>
> diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> index 64d5359cf7e2..83fb8f34ccbd 100644
> --- a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> +++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> @@ -39,10 +39,8 @@ enum ht_extension_chan_offset {
>
> struct ht_capability_ele {
> //HT capability info
> - u8 AdvCoding:1;
> u8 ChlWidth:1;
> u8 MimoPwrSave:2;
> - u8 GreenField:1;
Don't these fields come from the hardware itself? By removing them
here, you just changed the memory layout of the structure. Does the
driver still work properly after this? If you can't test it, I can't
take this patch as it's too risky...
sorry,
greg k-h
Re: [PATCH 05/13] staging:rtl8192u: Remove AdvCoding and GreenField - Style
On Wed, Sep 26, 2018 at 08:16:56PM +0100, John Whitmore wrote:
> The member variables AdvCoding and GreenField are unused in code so
> have been removed from the structure and associated initialisation
> function.
>
> This is a 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 | 2 --
> 2 files changed, 4 deletions(-)
>
> diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> index 64d5359cf7e2..83fb8f34ccbd 100644
> --- a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> +++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> @@ -39,10 +39,8 @@ enum ht_extension_chan_offset {
>
> struct ht_capability_ele {
> //HT capability info
> - u8 AdvCoding:1;
> u8 ChlWidth:1;
> u8 MimoPwrSave:2;
> - u8 GreenField:1;
Don't these fields come from the hardware itself? By removing them
here, you just changed the memory layout of the structure. Does the
driver still work properly after this? If you can't test it, I can't
take this patch as it's too risky...
sorry,
greg k-h
Re: [PATCH 05/13] staging:rtl8192u: Remove AdvCoding and GreenField - Style
On Wed, Sep 26, 2018 at 08:16:56PM +0100, John Whitmore wrote:
> The member variables AdvCoding and GreenField are unused in code so
> have been removed from the structure and associated initialisation
> function.
>
> This is a 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 | 2 --
> 2 files changed, 4 deletions(-)
>
> diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> index 64d5359cf7e2..83fb8f34ccbd 100644
> --- a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> +++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> @@ -39,10 +39,8 @@ enum ht_extension_chan_offset {
>
> struct ht_capability_ele {
> //HT capability info
> - u8 AdvCoding:1;
> u8 ChlWidth:1;
> u8 MimoPwrSave:2;
> - u8 GreenField:1;
> u8 ShortGI20Mhz:1;
> u8 ShortGI40Mhz:1;
> u8 TxSTBC:1;
I feel like we discussed this before. I'm pretty sure this comes from
the firmware and so the format can't be changed. When I look at
rtllib_parse_mife_generic() then I think that "info_element" probably
comes from the firmware.
I wouldn't want to accept this with out someone testing it.
regards,
dan carpenter
Re: [PATCH 05/13] staging:rtl8192u: Remove AdvCoding and GreenField - Style
On Wed, Sep 26, 2018 at 08:16:56PM +0100, John Whitmore wrote:
> The member variables AdvCoding and GreenField are unused in code so
> have been removed from the structure and associated initialisation
> function.
>
> This is a 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 | 2 --
> 2 files changed, 4 deletions(-)
>
> diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> index 64d5359cf7e2..83fb8f34ccbd 100644
> --- a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> +++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
> @@ -39,10 +39,8 @@ enum ht_extension_chan_offset {
>
> struct ht_capability_ele {
> //HT capability info
> - u8 AdvCoding:1;
> u8 ChlWidth:1;
> u8 MimoPwrSave:2;
> - u8 GreenField:1;
> u8 ShortGI20Mhz:1;
> u8 ShortGI40Mhz:1;
> u8 TxSTBC:1;
I feel like we discussed this before. I'm pretty sure this comes from
the firmware and so the format can't be changed. When I look at
rtllib_parse_mife_generic() then I think that "info_element" probably
comes from the firmware.
I wouldn't want to accept this with out someone testing it.
regards,
dan carpenter
[PATCH 05/13] staging:rtl8192u: Remove AdvCoding and GreenField - Style
The member variables AdvCoding and GreenField are unused in code so
have been removed from the structure and associated initialisation
function.
This is a 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 | 2 --
2 files changed, 4 deletions(-)
diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
index 64d5359cf7e2..83fb8f34ccbd 100644
--- a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
+++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
@@ -39,10 +39,8 @@ enum ht_extension_chan_offset {
struct ht_capability_ele {
//HT capability info
- u8 AdvCoding:1;
u8 ChlWidth:1;
u8 MimoPwrSave:2;
- u8 GreenField:1;
u8 ShortGI20Mhz:1;
u8 ShortGI40Mhz:1;
u8 TxSTBC:1;
diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c
b/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c
index c73a8058cf87..d1fbe65ed428 100644
--- a/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c
+++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c
@@ -493,7 +493,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
@@ -501,7 +500,6 @@ void HTConstructCapabilityElement(struct ieee80211_device
*ieee, u8 *posHTCap, u
// pCapELE->ChlWidth = (pHT->bRegBW40MHz?1:0);
pCapELE->MimoPwrSave= pHT->SelfMimoPs;
- pCapELE->GreenField = 0; // This feature is not supported
now!!
pCapELE->ShortGI20Mhz = 1; // We can receive Short GI!!
pCapELE->ShortGI40Mhz = 1; // We can receive Short GI!!
//DbgPrint("TX HT cap/info ele BW=%d SG20=%d SG40=%d\n\r",
--
2.18.0
[PATCH 05/13] staging:rtl8192u: Remove AdvCoding and GreenField - Style
The member variables AdvCoding and GreenField are unused in code so
have been removed from the structure and associated initialisation
function.
This is a 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 | 2 --
2 files changed, 4 deletions(-)
diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
index 64d5359cf7e2..83fb8f34ccbd 100644
--- a/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
+++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_HT.h
@@ -39,10 +39,8 @@ enum ht_extension_chan_offset {
struct ht_capability_ele {
//HT capability info
- u8 AdvCoding:1;
u8 ChlWidth:1;
u8 MimoPwrSave:2;
- u8 GreenField:1;
u8 ShortGI20Mhz:1;
u8 ShortGI40Mhz:1;
u8 TxSTBC:1;
diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c
b/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c
index c73a8058cf87..d1fbe65ed428 100644
--- a/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c
+++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c
@@ -493,7 +493,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
@@ -501,7 +500,6 @@ void HTConstructCapabilityElement(struct ieee80211_device
*ieee, u8 *posHTCap, u
// pCapELE->ChlWidth = (pHT->bRegBW40MHz?1:0);
pCapELE->MimoPwrSave= pHT->SelfMimoPs;
- pCapELE->GreenField = 0; // This feature is not supported
now!!
pCapELE->ShortGI20Mhz = 1; // We can receive Short GI!!
pCapELE->ShortGI40Mhz = 1; // We can receive Short GI!!
//DbgPrint("TX HT cap/info ele BW=%d SG20=%d SG40=%d\n\r",
--
2.18.0
