Hi Vladimir,
On Thu, Mar 10, 2016 at 11:02 AM, Vladimir Zapolskiy <vz@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
>>>>> ---
>>>>> 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.
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<----
Thanks,
--
Julian Calaby
Email: julian.calaby@xxxxxxxxx
Profile: http://www.google.com/profiles/julian.calaby/
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
