RE: [PATCH] staging: wilc1000: fix check of kthread_run() return value

[Kim, Leo]

Hi Vladimir & Julian

The driver is still moving to the Linux driver and also its being changed.
Your patch is everything's ok.
I also agree to Julian review.

 Thanks, BR
 Leo

-Original Message-
From: Vladimir Zapolskiy [mailto:v...@mleia.com] 
Sent: Thursday, March 10, 2016 9:36 AM
To: Julian Calaby 
Cc: Lee, Glen ; Greg Kroah-Hartman 
; Kim, Johnny ; Shin, Austin 
; Park, Chris ; Cho, Tony 
; Kim, Leo ; linux-wireless 
; de...@driverdev.osuosl.org
Subject: Re: [PATCH] staging: wilc1000: fix check of kthread_run() return value

Hi Julian,

On 10.03.2016 02:24, Julian Calaby wrote:
> Hi Vladimir,
> 
> On Thu, Mar 10, 2016 at 11:02 AM, Vladimir Zapolskiy  wrote:
>> Hi Julian,
>>
>> On 10.03.2016 01:42, Julian Calaby wrote:
>>> Hi Vladimir,
>>>
>>> On Thu, Mar 10, 2016 at 10:30 AM, Vladimir Zapolskiy  wrote:
>>>> Hi Julian,
>>>>
>>>> On 10.03.2016 01:27, Julian Calaby wrote:
>>>>> Hi Vladimir,
>>>>>
>>>>> On Thu, Mar 10, 2016 at 10:13 AM, Vladimir Zapolskiy  
>>>>> wrote:
>>>>>> The kthread_run() function returns either a valid task_struct or
>>>>>> ERR_PTR() value, check for NULL is invalid. The change fixes 
>>>>>> potential oops, e.g. in OOM situation.
>>>>>>
>>>>>> Signed-off-by: Vladimir Zapolskiy 
>>>>>> ---
>>>>>>  drivers/staging/wilc1000/linux_wlan.c | 4 ++--
>>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/staging/wilc1000/linux_wlan.c 
>>>>>> b/drivers/staging/wilc1000/linux_wlan.c
>>>>>> index 54fe9d7..5077c30 100644
>>>>>> --- a/drivers/staging/wilc1000/linux_wlan.c
>>>>>> +++ b/drivers/staging/wilc1000/linux_wlan.c
>>>>>> @@ -849,10 +849,10 @@ static int wlan_initialize_threads(struct 
>>>>>> net_device *dev)
>>>>>> PRINT_D(INIT_DBG, "Creating kthread for transmission\n");
>>>>>> wilc->txq_thread = kthread_run(linux_wlan_txq_task, (void *)dev,
>>>>>>  "K_TXQ_TASK");
>>>>>> -   if (!wilc->txq_thread) {
>>>>>> +   if (IS_ERR(wilc->txq_thread)) {
>>>>>> PRINT_ER("couldn't create TXQ thread\n");
>>>>>> wilc->close = 0;
>>>>>> -   return -ENOBUFS;
>>>>>> +   return PTR_ERR(wilc->txq_thread);
>>>>>
>>>>> Are you sure changing the error returned is correct? Do all the 
>>>>> callers of wlan_initialize_threads() handle the full range of 
>>>>> errors from kthread_run()?
>>>>
>>>> Have you checked the driver?
>>>
>>> I'm making sure you have. It's possible that there's a good reason 
>>> why this returns -ENOBUFS I want to know that you've at least 
>>> considered that possibility.
>>
>> You have my confirmation, I've checked the call stack before 
>> publishing this fix.
> 
> Awesome.
> 
>>>> This function is called once on initialization, the check on the 
>>>> upper layer has "if (ret < 0) goto exit_badly;" form.
>>>
>>> And practically everything in the chain up to net_device_ops uses 
>>> the same error handling scheme so it's probably fine.
>>
>> dev_open()
>>   __dev_open()
>> wilc_mac_open()
>>   wilc1000_wlan_init()
>> wlan_initialize_threads()
>>
>> Oh, why kernel threads within a driver are init'ed/destroyed on each 
>> device up/down state transition?
> 
> You'll have to ask the driver developers. I believe this was a cross 
> platform driver that is currently being Linux-ised, so I'm guessing 
> this is some artefact of that.
> 
>>> You should also document this change in the commit message.
>>
>> The change is documented in the commit message, take a look. But I 
>> didn't add "I swear it does not break anything" ;)
> 
> You
> 1. corrected the test in the if statement 2. changed the return value 
> from -ENOBUFS in your patch, however you only documented the first 
> part.

Agree, it makes sense.

> I would have expected a commit message along the lines of:
> 
> >8
> 
> The kthread_run() function returns either a valid task_struct or
> ERR_PTR() value, so the check for NULL is invalid.
> 
> Also return the error from kthread_run() instead of -ENOBUFS.
> 
> 8<
> 

Before publishing v2 let see if driver maintainers have something else to add, 
or may be it is better to preserve -ENOBUFS, or may be they are so kind to 
update the commit message on patch application.

Julian, thanks for review.

--
With best wishes,
Vladimir

RE: [PATCH RESEND 04/10] staging: wilc1000: removes unnecessary wilc_debug print log

[Kim, Leo]

Dear Dan,

Thank you your great comments.
I will be not forget your advice. 

Please understand that if you have a misunderstanding.

Thank you & Best Regards.
Leo

-Original Message-
From: Dan Carpenter [mailto:dan.carpen...@oracle.com] 
Sent: Monday, February 22, 2016 11:44 PM
To: Kim, Leo 
Cc: gre...@linuxfoundation.org; de...@driverdev.osuosl.org; Park, Chris 
; Shin, Austin ; 
linux-wireless@vger.kernel.org; Ferre, Nicolas ; Cho, 
Tony ; Abozaeid, Adham 
Subject: Re: [PATCH RESEND 04/10] staging: wilc1000: removes unnecessary 
wilc_debug print log

On Mon, Feb 22, 2016 at 11:42:42AM +0000, Kim, Leo wrote:
> Dear Dan,
> 
> This patch is subject "removes unnecessary wilc_debug print log".

I'm fine with you fixing it up in a later patch, but you should not be 
defending this patch as valid way to do things.

The rule is "do one thing at a time", not "do half a thing at a time."
In the original code the if statement was required because it was determining 
when to print, but now it is a confusing unneeded line of code.  I'm not asking 
for an additional unrelated cleanup for something that was already there in the 
original code.  It was this patch which introduced the problem (the stray 
unneeded line of code).

Also I had already asked you to redo this on Feb 19.

Part of the reason that we like people to "do one thing per patch" is that 
people promise they will clean things up in the future but they get distracted 
and forget.

regards,
dan carpenter


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH RESEND 04/10] staging: wilc1000: removes unnecessary wilc_debug print log

[Kim, Leo]

Dear Dan,

This patch is subject "removes unnecessary wilc_debug print log".
Your advice are correct but in this patch is not applied.
I'll be applied to next patch about the your good advice.
Would be okay?
Always appreciate your positive advice & great information.

 Thanks, BR
 Leo

-Original Message-
From: Dan Carpenter [mailto:dan.carpen...@oracle.com] 
Sent: Monday, February 22, 2016 7:45 PM
To: Kim, Leo 
Cc: gre...@linuxfoundation.org; de...@driverdev.osuosl.org; Park, Chris 
; Shin, Austin ; 
linux-wireless@vger.kernel.org; Ferre, Nicolas ; Cho, 
Tony ; Abozaeid, Adham 
Subject: Re: [PATCH RESEND 04/10] staging: wilc1000: removes unnecessary 
wilc_debug print log

On Mon, Feb 22, 2016 at 01:41:13PM +0900, Leo Kim wrote:
> This patch removes unnecessary wilc_debug print log.
> The print log was written when if condition fail.
> The condition is chip-id check function.
> Also, replaces this condition with normal function.
> 
> Signed-off-by: Leo Kim 
> ---
>  drivers/staging/wilc1000/wilc_wlan.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/wilc_wlan.c 
> b/drivers/staging/wilc1000/wilc_wlan.c
> index f0ac47f..34018a1 100644
> --- a/drivers/staging/wilc1000/wilc_wlan.c
> +++ b/drivers/staging/wilc1000/wilc_wlan.c
> @@ -502,9 +502,7 @@ void chip_wakeup(struct wilc *wilc)
>  
>   do {
>   usleep_range(2 * 1000, 2 * 1000);
> - if ((wilc_get_chipid(wilc, true) == 0))
> - wilc_debug(N_ERR, "Couldn't read chip 
> id. Wake up failed\n");
> -
> + wilc_get_chipid(wilc, true);

Remove this as well.  Don't leave random no-op function calls lying around.

>   } while ((wilc_get_chipid(wilc, true) == 0) && 
> ((++trials % 3) == 
> 0));
 


This doesn't work as intended either.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH V2 18/24] staging: wilc1000: linux_wlan.c: removes unnecessary log messages

[Kim, Leo]

Hi Julian,

Yes, ongoing remove this function.
I will be removing this function for next patch.
Thank you in your advice.

 Thanks, BR
 Leo

-Original Message-
From: Julian Calaby [mailto:julian.cal...@gmail.com] 
Sent: Friday, February 19, 2016 9:41 AM
To: Kim, Leo 
Cc: Greg KH ; de...@driverdev.osuosl.org; 
linux-wireless ; Cho, Tony 
; Lee, Glen ; Shin, Austin 
; Park, Chris ; Abozaeid, Adham 
; Ferre, Nicolas 
Subject: Re: [PATCH V2 18/24] staging: wilc1000: linux_wlan.c: removes 
unnecessary log messages

Hi Leo,

On Thu, Feb 18, 2016 at 10:30 PM, Leo Kim  wrote:
> From: Chris Park 
>
> This patch removes unnecessary log messages.
>
> Signed-off-by: Chris Park 
> Signed-off-by: Leo Kim 
> ---
>  drivers/staging/wilc1000/linux_wlan.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/staging/wilc1000/linux_wlan.c 
> b/drivers/staging/wilc1000/linux_wlan.c
> index a731b46..660bf63 100644
> --- a/drivers/staging/wilc1000/linux_wlan.c
> +++ b/drivers/staging/wilc1000/linux_wlan.c
> @@ -226,7 +226,6 @@ static void deinit_irq(struct net_device *dev)
>
>  void wilc_dbg(u8 *buff)
>  {
> -   PRINT_D(INIT_DBG, "%d\n", *buff);
>  }

Are you sure this is right? If so, why not just remove this function entirely?

Thanks,

-- 
Julian Calaby

Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
N�r��yb�X��ǧv�^�)޺{.n�+{��*ޕ�,�{ay�ʇڙ�,j��f���h���z��w���
���j:+v���w�j�mzZ+�ݢj"��!�i

RE: drivers/staging/wilc1000/wilc_spi.c won't compile ?

[Kim, Leo]

Hi David,

You're had old wilc1000 source.
Remove the wilc1000_ops structure.
 (Branch) staging-testing
 (SHA) ce7b516f3f9e11fe4ee06fad0d7e853bb6e8f160
Please receive the latest source.

 Thanks, BR
 Leo

-Original Message-
From: David Binderman [mailto:dcb...@hotmail.com] 
Sent: Monday, February 08, 2016 9:18 PM
To: Kim, Johnny ; Shin, Austin ; 
Park, Chris ; Cho, Tony ; Lee, Glen 
; Kim, Leo ; 
linux-wireless@vger.kernel.org
Subject: drivers/staging/wilc1000/wilc_spi.c won't compile ?

Hello there,

drivers/staging/wilc1000/wilc_spi.c:123:34: error: storage size of 
'wilc1000_spi_ops' isn't known
 static const struct wilc1000_ops wilc1000_spi_ops;
  ^~~~
drivers/staging/wilc1000/wilc_spi.c:123:34: warning: 'wilc1000_spi_ops' defined 
but not used [-Wunused-const-variable]

Suggest remove.

Regards

David Binderman

  
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 08/12] staging: wilc1000: rename variable s32Error

[Kim, Leo]

Dear Julian

Thank you in your advises.
This patch subject is 'rename variables'.
This time, I hope the patch is applied.

I will be your positive advises change to next patch.

 Thanks, BR
 Leo

-Original Message-
From: Julian Calaby [mailto:julian.cal...@gmail.com] 
Sent: Friday, January 29, 2016 9:55 AM
To: Kim, Leo 
Cc: Greg KH ; de...@driverdev.osuosl.org; 
linux-wireless ; Cho, Tony 
; Lee, Glen ; Shin, Austin 
; Park, Chris ; Abozaeid, Adham 
; Ferre, Nicolas 
Subject: Re: [PATCH 08/12] staging: wilc1000: rename variable s32Error

Hi Leo,

On Thu, Jan 28, 2016 at 6:13 PM, Leo Kim  wrote:
> This patch renames variable s32Error to result to avoid CamelCase 
> naming convention.
> Also, remove the unused variable s32Error and replace with direct return.
>
> Signed-off-by: Leo Kim 
> ---
>  drivers/staging/wilc1000/coreconfigurator.c | 19 +--
>  1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/coreconfigurator.c 
> b/drivers/staging/wilc1000/coreconfigurator.c
> index 2a4e324..88e5661 100644
> --- a/drivers/staging/wilc1000/coreconfigurator.c
> +++ b/drivers/staging/wilc1000/coreconfigurator.c
> @@ -356,30 +356,29 @@ s32 wilc_parse_network_info(u8 *msg_buffer, 
> tstrNetworkInfo **ret_network_info)
>
>  s32 wilc_dealloc_network_info(tstrNetworkInfo *pstrNetworkInfo)  {
> -   s32 s32Error = 0;
> +   s32 result = 0;
>
> if (pstrNetworkInfo) {
> if (pstrNetworkInfo->pu8IEs) {
> kfree(pstrNetworkInfo->pu8IEs);
> pstrNetworkInfo->pu8IEs = NULL;

I'm not sure it's necessary to set this to NULL here.

> } else {
> -   s32Error = -EFAULT;
> +   result = -EFAULT;
> }
>
> kfree(pstrNetworkInfo);
> pstrNetworkInfo = NULL;

Or this to NULL here.

Also, is the return value from this function actually checked? kfree()
can have NULL passed to it.

If the return value isn't checked, this entire function could be simplified to:

- - - - -

if (pstrNetworkInfo) {
kfree(pstrNetworkInfo->pu8IEs);
kfree(pstrNetworkInfo);
}

return 0;

- - - - -

Also, when tstrNetworkInfo structs are allocated, wouldn't the
allocating function fail if ->pu8IEs is NULL?

> } else {
> -   s32Error = -EFAULT;
> +   result = -EFAULT;
> }
>
> -   return s32Error;
> +   return result;
>  }
>
>  s32 wilc_parse_assoc_resp_info(u8 *pu8Buffer, u32 u32BufferLen,
>tstrConnectRespInfo **ppstrConnectRespInfo)
>  {
> -   s32 s32Error = 0;
> tstrConnectRespInfo *pstrConnectRespInfo = NULL;
> u16 u16AssocRespLen = 0;
> u8 *pu8IEs = NULL;
> @@ -408,27 +407,27 @@ s32 wilc_parse_assoc_resp_info(u8 *pu8Buffer, u32 
> u32BufferLen,
>
> *ppstrConnectRespInfo = pstrConnectRespInfo;
>
> -   return s32Error;
> +   return 0;
>  }
>
>  s32 wilc_dealloc_assoc_resp_info(tstrConnectRespInfo *pstrConnectRespInfo)
>  {
> -   s32 s32Error = 0;
> +   s32 result = 0;

All the comments above apply to this function as well.

>
> if (pstrConnectRespInfo) {
> if (pstrConnectRespInfo->pu8RespIEs) {
> kfree(pstrConnectRespInfo->pu8RespIEs);
> pstrConnectRespInfo->pu8RespIEs = NULL;
> } else {
> -   s32Error = -EFAULT;
> +   result = -EFAULT;
> }
>
> kfree(pstrConnectRespInfo);
> pstrConnectRespInfo = NULL;
>
> } else {
> -   s32Error = -EFAULT;
> +   result = -EFAULT;
> }
>
> -   return s32Error;
> +   return result;
>  }

Thanks,

-- 
Julian Calaby

Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
N�r��yb�X��ǧv�^�)޺{.n�+{��*ޕ�,�{ay�ʇڙ�,j��f���h���z��w���
���j:+v���w�j�mzZ+�ݢj"��!�i

RE: [PATCH RESEND 01/38] staging: wilc1000: rename pu8IPAddr of fuction Handle_set_IPAddress

[Kim, Leo]

Dear Dan,

 Because Greg say,
 - 'This series doesn't apply probably because I didn't take your previous 
series.
Please resend after you refresh and resend that one."
 So resend :)

 Thanks, BR
 Leo

-Original Message-
From: Dan Carpenter [mailto:dan.carpen...@oracle.com] 
Sent: Thursday, November 05, 2015 4:17 PM
To: Lee, Glen
Cc: gre...@linuxfoundation.org; de...@driverdev.osuosl.org; Shin, Austin; 
linux-wireless@vger.kernel.org; Ferre, Nicolas; Noureldin, Adel; Cho, Tony; 
Kim, Leo; Abozaeid, Adham
Subject: Re: [PATCH RESEND 01/38] staging: wilc1000: rename pu8IPAddr of 
fuction Handle_set_IPAddress

Why are you resending these?

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 32/32] staging: wilc1000: fix kernel fail after ifconfig wlan0 up

[Kim, Leo]

Dear Dan,

Thank you! 
Always helpful your advice listen carefully.

I will send in the morning.

 Thanks, BR
 Leo

-Original Message-
From: Dan Carpenter [mailto:dan.carpen...@oracle.com] 
Sent: Wednesday, September 30, 2015 7:05 PM
To: Cho, Tony
Cc: gre...@linuxfoundation.org; de...@driverdev.osuosl.org; Kim, Rachel; Park, 
Chris; Shin, Austin; linux-wireless@vger.kernel.org; Kim, Johnny; Ferre, 
Nicolas; Noureldin, Adel; Kim, Leo; Abozaeid, Adham
Subject: Re: [PATCH 32/32] staging: wilc1000: fix kernel fail after ifconfig 
wlan0 up

Ugh...  No.  Just do this:

diff --git a/drivers/staging/wilc1000/wilc_msgqueue.c 
b/drivers/staging/wilc1000/wilc_msgqueue.c
index 869736a..52d8f95 100644
--- a/drivers/staging/wilc1000/wilc_msgqueue.c
+++ b/drivers/staging/wilc1000/wilc_msgqueue.c
@@ -102,6 +102,8 @@ int wilc_mq_send(WILC_MsgQueueHandle *pHandle,
 
up(&pHandle->hSem);
 
+   return 0;
+
 ERRORHANDLER:
/* error occured, free any allocations */
if (pstrMessage) {
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html