Re: [Outreachy kernel] [PATCH v2] Staging: rtl8712: Use mod_timer

[Jes Sorensen <jes.sorensen@gmail.com>]

On 02/23/15 14:49, Vaishali Thakkar wrote:
> On Feb 24, 2015 1:09 AM, "Jes Sorensen" <jes.sorensen@gmail.com> wrote:
>>
>> On 02/23/15 05:56, Vaishali Thakkar wrote:
>>> This patch introduces the use of API function mod_timer
>>> instead of driver specific function as it is a more
>>> efficient and standard way to update the expire field of
>>> an active timer.
>>>
>>> Signed-off-by: Vaishali Thakkar <vthakkar1994@gmail.com>
>>> ---
>>> Changes since v1:
>>>       -Edit commit message
>>>       -Remove unnecessory parenthesis
>>>       -Align second parameter of function
>>>        mod_timer properly
>>>
>>>  drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> I like this patch!
>>
>> However if you start getting rid of _set_timer() from a driver like
>> this, you should try and remove all instances, and then get rid of all
>> uses of _set_timer().
> 
> Yes. Actually I sent this patch for mentors' reviews. I thought if this
> will be accepted then I will go for sending more patches as there are many
> instances of _set_timer() in this driver and in other network drivers as
> well. And then I can remove those definitions once all cases are handled.
> 
> Also, network drivers are using driver specific functions where we can use
> setup_ timer() function. So, I will go for it as well.

Sounds great - I recommend you focus on one change, so start out with
one patch that eliminates _set_timer() and then do follow-on patches
where you convert other portions of code to use setup_timer().

Cheers,
Jes

>> In addition you are still leaving some unnecessary parenthesis in place
>> - see below.
> 
> Oops! yeah. I will remove them in next version.
> 
>> Cheers,
>> Jes
>>
>>
>>> diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
>>> index 9bb364f..0ce8e9d 100644
>>> --- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
>>> +++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
>>> @@ -129,7 +129,8 @@ static inline void handle_pairwise_key(struct
> sta_info *psta,
>>>               memcpy(psta->tkiprxmickey. skey, &(param->u.crypt.
>>>                       key[24]), 8);
>>>               padapter->securitypriv. busetkipkey = false;
>>> -             _set_timer(&padapter->securitypriv.tkip_timer, 50);
>>> +             mod_timer(&padapter->securitypriv.tkip_timer,
>>> +                       jiffies + msecs_to_jiffies(50));
>>>       }
>>>       r8712_setstakey_cmd(padapter, (unsigned char *)psta, true);
>>>  }
>>> @@ -153,8 +154,8 @@ static inline void handle_group_key(struct
> ieee_param *param,
>>>               if (padapter->registrypriv.power_mgnt > PS_MODE_ACTIVE) {
>>>                       if (padapter->registrypriv.power_mgnt !=
> padapter->
>>>                           pwrctrlpriv.pwr_mode)
>>> -
>  _set_timer(&(padapter->mlmepriv.dhcp_timer),
>>> -                                        60000);
>>> +
>  mod_timer(&(padapter->mlmepriv.dhcp_timer),
>>                                       here ^
>>> +                                       jiffies +
> msecs_to_jiffies(60000));
>>>               }
>>>       }
>>>  }
>>>
>>
>