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.
>> 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 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" ;)
--
With best wishes,
Vladimir
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
