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

Kim, Leo Leo.Kim at atmel.com
Thu Mar 10 06:49:39 UTC 2016


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:vz at mleia.com] 
Sent: Thursday, March 10, 2016 9:36 AM
To: Julian Calaby <julian.calaby at gmail.com>
Cc: Lee, Glen <Glen.Lee at atmel.com>; Greg Kroah-Hartman <gregkh at linuxfoundation.org>; Kim, Johnny <johnny.kim at atmel.com>; Shin, Austin <Austin.Shin at atmel.com>; Park, Chris <Chris.Park at atmel.com>; Cho, Tony <Tony.Cho at atmel.com>; Kim, Leo <Leo.Kim at atmel.com>; linux-wireless <linux-wireless at vger.kernel.org>; devel at 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 <vz at mleia.com> 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 <vz at mleia.com> 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 <vz at mleia.com> 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 <vz at mleia.com>
>>>>>> ---
>>>>>>  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


More information about the devel mailing list