[PATCH v2] staging: rtl8712: Fix CamelCase warnings

[PATCH v2] staging: rtl8712: Fix CamelCase warnings

[Sathish Kumar]

This patch fixes the checkpatch.pl warnings like:
CHECK: Avoid CamelCase: <blnEnableRxFF0Filter>
+   u8 blnEnableRxFF0Filter;

Signed-off-by: Sathish Kumar <skumark1902@gmail.com>
---
Changes in v2:
    - Remove the "bln" prefix
---
 drivers/staging/rtl8712/drv_types.h   | 2 +-
 drivers/staging/rtl8712/rtl871x_cmd.c | 2 +-
 drivers/staging/rtl8712/xmit_linux.c  | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/rtl8712/drv_types.h b/drivers/staging/rtl8712/drv_types.h
index a44d04effc8b..4de3aad08242 100644
--- a/drivers/staging/rtl8712/drv_types.h
+++ b/drivers/staging/rtl8712/drv_types.h
@@ -157,7 +157,7 @@ struct _adapter {
 	struct iw_statistics iwstats;
 	int pid; /*process id from UI*/
 	struct work_struct wk_filter_rx_ff0;
-	u8 blnEnableRxFF0Filter;
+	u8 enable_rx_ff0_filter;
 	spinlock_t lock_rx_ff0_filter;
 	const struct firmware *fw;
 	struct usb_interface *pusb_intf;
diff --git a/drivers/staging/rtl8712/rtl871x_cmd.c b/drivers/staging/rtl8712/rtl871x_cmd.c
index acda930722b2..69d3c55ee9e5 100644
--- a/drivers/staging/rtl8712/rtl871x_cmd.c
+++ b/drivers/staging/rtl8712/rtl871x_cmd.c
@@ -202,7 +202,7 @@ u8 r8712_sitesurvey_cmd(struct _adapter *padapter,
 	mod_timer(&pmlmepriv->scan_to_timer,
 		  jiffies + msecs_to_jiffies(SCANNING_TIMEOUT));
 	padapter->ledpriv.LedControlHandler(padapter, LED_CTL_SITE_SURVEY);
-	padapter->blnEnableRxFF0Filter = 0;
+	padapter->enable_rx_ff0_filter = 0;
 	return _SUCCESS;
 }
 
diff --git a/drivers/staging/rtl8712/xmit_linux.c b/drivers/staging/rtl8712/xmit_linux.c
index 90d34cf9d2ff..d58ae5b387d4 100644
--- a/drivers/staging/rtl8712/xmit_linux.c
+++ b/drivers/staging/rtl8712/xmit_linux.c
@@ -102,11 +102,11 @@ void r8712_SetFilter(struct work_struct *work)
 	r8712_write8(adapter, 0x117, newvalue);
 
 	spin_lock_irqsave(&adapter->lock_rx_ff0_filter, irqL);
-	adapter->blnEnableRxFF0Filter = 1;
+	adapter->enable_rx_ff0_filter = 1;
 	spin_unlock_irqrestore(&adapter->lock_rx_ff0_filter, irqL);
 	do {
 		msleep(100);
-	} while (adapter->blnEnableRxFF0Filter == 1);
+	} while (adapter->enable_rx_ff0_filter == 1);
 	r8712_write8(adapter, 0x117, oldvalue);
 }
 
-- 
2.17.1

Re: [PATCH v2] staging: rtl8712: Fix CamelCase warnings

[Greg KH]

On Fri, Mar 18, 2022 at 03:44:40PM +0530, Sathish Kumar wrote:
> This patch fixes the checkpatch.pl warnings like:
> CHECK: Avoid CamelCase: <blnEnableRxFF0Filter>
> +   u8 blnEnableRxFF0Filter;
> 
> Signed-off-by: Sathish Kumar <skumark1902@gmail.com>
> ---
> Changes in v2:
>     - Remove the "bln" prefix
> ---
>  drivers/staging/rtl8712/drv_types.h   | 2 +-
>  drivers/staging/rtl8712/rtl871x_cmd.c | 2 +-
>  drivers/staging/rtl8712/xmit_linux.c  | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/rtl8712/drv_types.h b/drivers/staging/rtl8712/drv_types.h
> index a44d04effc8b..4de3aad08242 100644
> --- a/drivers/staging/rtl8712/drv_types.h
> +++ b/drivers/staging/rtl8712/drv_types.h
> @@ -157,7 +157,7 @@ struct _adapter {
>  	struct iw_statistics iwstats;
>  	int pid; /*process id from UI*/
>  	struct work_struct wk_filter_rx_ff0;
> -	u8 blnEnableRxFF0Filter;
> +	u8 enable_rx_ff0_filter;

Shouldn't this be a boolean?

>  	spinlock_t lock_rx_ff0_filter;
>  	const struct firmware *fw;
>  	struct usb_interface *pusb_intf;
> diff --git a/drivers/staging/rtl8712/rtl871x_cmd.c b/drivers/staging/rtl8712/rtl871x_cmd.c
> index acda930722b2..69d3c55ee9e5 100644
> --- a/drivers/staging/rtl8712/rtl871x_cmd.c
> +++ b/drivers/staging/rtl8712/rtl871x_cmd.c
> @@ -202,7 +202,7 @@ u8 r8712_sitesurvey_cmd(struct _adapter *padapter,
>  	mod_timer(&pmlmepriv->scan_to_timer,
>  		  jiffies + msecs_to_jiffies(SCANNING_TIMEOUT));
>  	padapter->ledpriv.LedControlHandler(padapter, LED_CTL_SITE_SURVEY);
> -	padapter->blnEnableRxFF0Filter = 0;
> +	padapter->enable_rx_ff0_filter = 0;
>  	return _SUCCESS;
>  }
>  
> diff --git a/drivers/staging/rtl8712/xmit_linux.c b/drivers/staging/rtl8712/xmit_linux.c
> index 90d34cf9d2ff..d58ae5b387d4 100644
> --- a/drivers/staging/rtl8712/xmit_linux.c
> +++ b/drivers/staging/rtl8712/xmit_linux.c
> @@ -102,11 +102,11 @@ void r8712_SetFilter(struct work_struct *work)
>  	r8712_write8(adapter, 0x117, newvalue);
>  
>  	spin_lock_irqsave(&adapter->lock_rx_ff0_filter, irqL);
> -	adapter->blnEnableRxFF0Filter = 1;
> +	adapter->enable_rx_ff0_filter = 1;
>  	spin_unlock_irqrestore(&adapter->lock_rx_ff0_filter, irqL);
>  	do {
>  		msleep(100);
> -	} while (adapter->blnEnableRxFF0Filter == 1);
> +	} while (adapter->enable_rx_ff0_filter == 1);

Ah, that's funny.  It's amazing it works at all and that the compiler
doesn't optimize this away.  This isn't a good pattern to use in kernel
code.  I know it's not caused by your change here, but perhaps you might
want to fix this up to work properly?

thanks,

greg k-h

Re: [PATCH v2] staging: rtl8712: Fix CamelCase warnings

[Sathish Kumar]

On 18/03/22 4:58 pm, Greg KH wrote:

> On Fri, Mar 18, 2022 at 03:44:40PM +0530, Sathish Kumar wrote:
>> This patch fixes the checkpatch.pl warnings like:
>> CHECK: Avoid CamelCase: <blnEnableRxFF0Filter>
>> +   u8 blnEnableRxFF0Filter;
>>
>> Signed-off-by: Sathish Kumar <skumark1902@gmail.com>
>> ---
>> Changes in v2:
>>      - Remove the "bln" prefix
>> ---
>>   drivers/staging/rtl8712/drv_types.h   | 2 +-
>>   drivers/staging/rtl8712/rtl871x_cmd.c | 2 +-
>>   drivers/staging/rtl8712/xmit_linux.c  | 4 ++--
>>   3 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/staging/rtl8712/drv_types.h b/drivers/staging/rtl8712/drv_types.h
>> index a44d04effc8b..4de3aad08242 100644
>> --- a/drivers/staging/rtl8712/drv_types.h
>> +++ b/drivers/staging/rtl8712/drv_types.h
>> @@ -157,7 +157,7 @@ struct _adapter {
>>   	struct iw_statistics iwstats;
>>   	int pid; /*process id from UI*/
>>   	struct work_struct wk_filter_rx_ff0;
>> -	u8 blnEnableRxFF0Filter;
>> +	u8 enable_rx_ff0_filter;
> Shouldn't this be a boolean?
Yes. It should be boolean(dealing only with either 0 or 1). Will fix this.
>
>>   	spinlock_t lock_rx_ff0_filter;
>>   	const struct firmware *fw;
>>   	struct usb_interface *pusb_intf;
>> diff --git a/drivers/staging/rtl8712/rtl871x_cmd.c b/drivers/staging/rtl8712/rtl871x_cmd.c
>> index acda930722b2..69d3c55ee9e5 100644
>> --- a/drivers/staging/rtl8712/rtl871x_cmd.c
>> +++ b/drivers/staging/rtl8712/rtl871x_cmd.c
>> @@ -202,7 +202,7 @@ u8 r8712_sitesurvey_cmd(struct _adapter *padapter,
>>   	mod_timer(&pmlmepriv->scan_to_timer,
>>   		  jiffies + msecs_to_jiffies(SCANNING_TIMEOUT));
>>   	padapter->ledpriv.LedControlHandler(padapter, LED_CTL_SITE_SURVEY);
>> -	padapter->blnEnableRxFF0Filter = 0;
>> +	padapter->enable_rx_ff0_filter = 0;
>>   	return _SUCCESS;
>>   }
>>   
>> diff --git a/drivers/staging/rtl8712/xmit_linux.c b/drivers/staging/rtl8712/xmit_linux.c
>> index 90d34cf9d2ff..d58ae5b387d4 100644
>> --- a/drivers/staging/rtl8712/xmit_linux.c
>> +++ b/drivers/staging/rtl8712/xmit_linux.c
>> @@ -102,11 +102,11 @@ void r8712_SetFilter(struct work_struct *work)
>>   	r8712_write8(adapter, 0x117, newvalue);
>>   
>>   	spin_lock_irqsave(&adapter->lock_rx_ff0_filter, irqL);
>> -	adapter->blnEnableRxFF0Filter = 1;
>> +	adapter->enable_rx_ff0_filter = 1;
>>   	spin_unlock_irqrestore(&adapter->lock_rx_ff0_filter, irqL);
>>   	do {
>>   		msleep(100);
>> -	} while (adapter->blnEnableRxFF0Filter == 1);
>> +	} while (adapter->enable_rx_ff0_filter == 1);
> Ah, that's funny.  It's amazing it works at all and that the compiler
> doesn't optimize this away.  This isn't a good pattern to use in kernel
Do you mean "do { msleep(); } while()" here?
> code.  I know it's not caused by your change here, but perhaps you might
> want to fix this up to work properly?
>
> thanks,
>
> greg k-h

Sure. Will fix this up to work properly.

Thanks,

Sathish

Re: [PATCH v2] staging: rtl8712: Fix CamelCase warnings

[Sathish Kumar]

On 18/03/22 4:58 pm, Greg KH wrote:
> On Fri, Mar 18, 2022 at 03:44:40PM +0530, Sathish Kumar wrote:
>> This patch fixes the checkpatch.pl warnings like:
>> CHECK: Avoid CamelCase: <blnEnableRxFF0Filter>
>> +   u8 blnEnableRxFF0Filter;
>>
>> Signed-off-by: Sathish Kumar <skumark1902@gmail.com>
>> ---
>> Changes in v2:
>>      - Remove the "bln" prefix
>> ---
>>   drivers/staging/rtl8712/drv_types.h   | 2 +-
>>   drivers/staging/rtl8712/rtl871x_cmd.c | 2 +-
>>   drivers/staging/rtl8712/xmit_linux.c  | 4 ++--
>>   3 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/staging/rtl8712/drv_types.h b/drivers/staging/rtl8712/drv_types.h
>> index a44d04effc8b..4de3aad08242 100644
>> --- a/drivers/staging/rtl8712/drv_types.h
>> +++ b/drivers/staging/rtl8712/drv_types.h
>> @@ -157,7 +157,7 @@ struct _adapter {
>>   	struct iw_statistics iwstats;
>>   	int pid; /*process id from UI*/
>>   	struct work_struct wk_filter_rx_ff0;
>> -	u8 blnEnableRxFF0Filter;
>> +	u8 enable_rx_ff0_filter;
> Shouldn't this be a boolean?
>
>>   	spinlock_t lock_rx_ff0_filter;
>>   	const struct firmware *fw;
>>   	struct usb_interface *pusb_intf;
>> diff --git a/drivers/staging/rtl8712/rtl871x_cmd.c b/drivers/staging/rtl8712/rtl871x_cmd.c
>> index acda930722b2..69d3c55ee9e5 100644
>> --- a/drivers/staging/rtl8712/rtl871x_cmd.c
>> +++ b/drivers/staging/rtl8712/rtl871x_cmd.c
>> @@ -202,7 +202,7 @@ u8 r8712_sitesurvey_cmd(struct _adapter *padapter,
>>   	mod_timer(&pmlmepriv->scan_to_timer,
>>   		  jiffies + msecs_to_jiffies(SCANNING_TIMEOUT));
>>   	padapter->ledpriv.LedControlHandler(padapter, LED_CTL_SITE_SURVEY);
>> -	padapter->blnEnableRxFF0Filter = 0;
>> +	padapter->enable_rx_ff0_filter = 0;
>>   	return _SUCCESS;
>>   }
>>   
>> diff --git a/drivers/staging/rtl8712/xmit_linux.c b/drivers/staging/rtl8712/xmit_linux.c
>> index 90d34cf9d2ff..d58ae5b387d4 100644
>> --- a/drivers/staging/rtl8712/xmit_linux.c
>> +++ b/drivers/staging/rtl8712/xmit_linux.c
>> @@ -102,11 +102,11 @@ void r8712_SetFilter(struct work_struct *work)
>>   	r8712_write8(adapter, 0x117, newvalue);
>>   
>>   	spin_lock_irqsave(&adapter->lock_rx_ff0_filter, irqL);
>> -	adapter->blnEnableRxFF0Filter = 1;
>> +	adapter->enable_rx_ff0_filter = 1;
>>   	spin_unlock_irqrestore(&adapter->lock_rx_ff0_filter, irqL);
>>   	do {
>>   		msleep(100);
>> -	} while (adapter->blnEnableRxFF0Filter == 1);
>> +	} while (adapter->enable_rx_ff0_filter == 1);
> Ah, that's funny.  It's amazing it works at all and that the compiler
> doesn't optimize this away.  This isn't a good pattern to use in kernel
Do you mean the following code is not a good pattern in kernel?
do {
msleep();
} while(condition);
> code.  I know it's not caused by your change here, but perhaps you might
> want to fix this up to work properly?
>
> thanks,
>
> greg k-h

Do i need to replace the above code with some other mechanism?

If yes, please let me know which mechanism i should use? Or what should 
I do here?

Note : I am new to Linux kernel development and looking forward to learn 
and contribute.

Thanks,
Sathish

Re: [PATCH v2] staging: rtl8712: Fix CamelCase warnings

[Fabio M. De Francesco]

On martedì 22 marzo 2022 05:30:29 CET Sathish Kumar wrote:
> On 18/03/22 4:58 pm, Greg KH wrote:
> > On Fri, Mar 18, 2022 at 03:44:40PM +0530, Sathish Kumar wrote:
> >> This patch fixes the checkpatch.pl warnings like:
> >> CHECK: Avoid CamelCase: <blnEnableRxFF0Filter>
> >> +   u8 blnEnableRxFF0Filter;
> >>
> >> Signed-off-by: Sathish Kumar <skumark1902@gmail.com>
> >> ---
> >> Changes in v2:
> >>      - Remove the "bln" prefix
> >> ---
> >>   drivers/staging/rtl8712/drv_types.h   | 2 +-
> >>   drivers/staging/rtl8712/rtl871x_cmd.c | 2 +-
> >>   drivers/staging/rtl8712/xmit_linux.c  | 4 ++--
> >>   3 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >> [...]
> >>
> >>   	do {
> >>   		msleep(100);
> >> -	} while (adapter->blnEnableRxFF0Filter == 1);
> >> +	} while (adapter->enable_rx_ff0_filter == 1);
> > Ah, that's funny.  It's amazing it works at all and that the compiler
> > doesn't optimize this away.  This isn't a good pattern to use in kernel
> Do you mean the following code is not a good pattern in kernel?
> do {
> msleep();
> } while(condition);

Exactly, this is not a pattern that works as you expect :)

I was waiting for Greg to detail something more about this subject but, 
since it looks like he has no time yet to respond, I'll try to interpret 
his words.

(@Greg, please forgive me if I saying something different from what you
intended to convey :)).

The reason why this pattern does not work as expected is too long to be
explained here. However, I think that Greg is suggesting to you to research
and use what are called "Condition variables".

Take some time to research and understand what the Linuc kernel uses for
waiting for completion or state changes: struct completion, 
wait_for_completion(), complete(), and others.

Another related mechanism are the Linux kernel "Wait Queues". Do some 
research and understand how to use wait_event{,_interruptible,timeout} 
and wake_up{,all,interruptible,interruptible_all}.

If I recall correctly you may find one or more of my own patches to
r8188eu where I use those API (git-log is your friend).

I hope that all the above will be sufficient to start with.

Again, Greg please correct my words if I'm misinterpreting what you
requested to Sathish.

Thanks,

Fabio M. De Francesco

> > I know it's not caused by your change here, but perhaps you might
> > want to fix this up to work properly?
> >
> > thanks,
> >
> > greg k-h
> 
> Do i need to replace the above code with some other mechanism?
> 
> If yes, please let me know which mechanism i should use? Or what should 
> I do here?
> 
> Note : I am new to Linux kernel development and looking forward to learn 
> and contribute.
> 
> Thanks,
> Sathish
> 
> 

Re: [PATCH v2] staging: rtl8712: Fix CamelCase warnings

[Fabio M. De Francesco]

On martedì 22 marzo 2022 11:42:21 CET Fabio M. De Francesco wrote:

> The reason why this pattern does not work as expected is too long to be
> explained here. However, I think that Greg is suggesting to you to research
> and use what are called "Condition variables".

Sorry, "Condition" -> "Completion".

Fabio

Re: [PATCH v2] staging: rtl8712: Fix CamelCase warnings

[Greg KH]

On Tue, Mar 22, 2022 at 11:42:21AM +0100, Fabio M. De Francesco wrote:
> On martedì 22 marzo 2022 05:30:29 CET Sathish Kumar wrote:
> > On 18/03/22 4:58 pm, Greg KH wrote:
> > > On Fri, Mar 18, 2022 at 03:44:40PM +0530, Sathish Kumar wrote:
> > >> This patch fixes the checkpatch.pl warnings like:
> > >> CHECK: Avoid CamelCase: <blnEnableRxFF0Filter>
> > >> +   u8 blnEnableRxFF0Filter;
> > >>
> > >> Signed-off-by: Sathish Kumar <skumark1902@gmail.com>
> > >> ---
> > >> Changes in v2:
> > >>      - Remove the "bln" prefix
> > >> ---
> > >>   drivers/staging/rtl8712/drv_types.h   | 2 +-
> > >>   drivers/staging/rtl8712/rtl871x_cmd.c | 2 +-
> > >>   drivers/staging/rtl8712/xmit_linux.c  | 4 ++--
> > >>   3 files changed, 4 insertions(+), 4 deletions(-)
> > >>
> > >> [...]
> > >>
> > >>   	do {
> > >>   		msleep(100);
> > >> -	} while (adapter->blnEnableRxFF0Filter == 1);
> > >> +	} while (adapter->enable_rx_ff0_filter == 1);
> > > Ah, that's funny.  It's amazing it works at all and that the compiler
> > > doesn't optimize this away.  This isn't a good pattern to use in kernel
> > Do you mean the following code is not a good pattern in kernel?
> > do {
> > msleep();
> > } while(condition);
> 
> Exactly, this is not a pattern that works as you expect :)
> 
> I was waiting for Greg to detail something more about this subject but, 
> since it looks like he has no time yet to respond, I'll try to interpret 
> his words.
> 
> (@Greg, please forgive me if I saying something different from what you
> intended to convey :)).
> 
> The reason why this pattern does not work as expected is too long to be
> explained here. However, I think that Greg is suggesting to you to research
> and use what are called "Condition variables".

Kind of.  The problem is that "condition" here is just looking at a
random variable.  There is no sort of assurance that the variable will
actually change or that that compiler even has to read from memory for
it.  It could cache the value the first time it is read and then never
update it for the whole loop logic.

Please read Documentation/memory-barriers.txt for how to fix this all up
and do it properly.

Again, it's amazing that the current code even works at all.  So maybe
it doesn't!  :)

thansks,

greg k-h

Re: [PATCH v2] staging: rtl8712: Fix CamelCase warnings

[Sathish Kumar]

On 22/03/22 5:01 pm, Greg KH wrote:
> On Tue, Mar 22, 2022 at 11:42:21AM +0100, Fabio M. De Francesco wrote:
>> On martedì 22 marzo 2022 05:30:29 CET Sathish Kumar wrote:
>>> On 18/03/22 4:58 pm, Greg KH wrote:
>>>> On Fri, Mar 18, 2022 at 03:44:40PM +0530, Sathish Kumar wrote:
>>>>> This patch fixes the checkpatch.pl warnings like:
>>>>> CHECK: Avoid CamelCase: <blnEnableRxFF0Filter>
>>>>> +   u8 blnEnableRxFF0Filter;
>>>>>
>>>>> Signed-off-by: Sathish Kumar <skumark1902@gmail.com>
>>>>> ---
>>>>> Changes in v2:
>>>>>       - Remove the "bln" prefix
>>>>> ---
>>>>>    drivers/staging/rtl8712/drv_types.h   | 2 +-
>>>>>    drivers/staging/rtl8712/rtl871x_cmd.c | 2 +-
>>>>>    drivers/staging/rtl8712/xmit_linux.c  | 4 ++--
>>>>>    3 files changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> [...]
>>>>>
>>>>>    	do {
>>>>>    		msleep(100);
>>>>> -	} while (adapter->blnEnableRxFF0Filter == 1);
>>>>> +	} while (adapter->enable_rx_ff0_filter == 1);
>>>> Ah, that's funny.  It's amazing it works at all and that the compiler
>>>> doesn't optimize this away.  This isn't a good pattern to use in kernel
>>> Do you mean the following code is not a good pattern in kernel?
>>> do {
>>> msleep();
>>> } while(condition);
>> Exactly, this is not a pattern that works as you expect :)
>>
>> I was waiting for Greg to detail something more about this subject but,
>> since it looks like he has no time yet to respond, I'll try to interpret
>> his words.
>>
>> (@Greg, please forgive me if I saying something different from what you
>> intended to convey :)).
>>
>> The reason why this pattern does not work as expected is too long to be
>> explained here. However, I think that Greg is suggesting to you to research
>> and use what are called "Condition variables".
> Kind of.  The problem is that "condition" here is just looking at a
> random variable.  There is no sort of assurance that the variable will
> actually change or that that compiler even has to read from memory for
> it.  It could cache the value the first time it is read and then never
> update it for the whole loop logic.
>
> Please read Documentation/memory-barriers.txt for how to fix this all up
> and do it properly.
>
> Again, it's amazing that the current code even works at all.  So maybe
> it doesn't!  :)
>
> thansks,
>
> greg k-h

Thank you Greg and Fabio for your inputs. Will check and fix this up.

Regards,

Sathish