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

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

On 02/23/15 15:06, Vaishali Thakkar wrote:
> On Feb 24, 2015 1:22 AM, "Jes Sorensen" <jes.sorensen@gmail.com> wrote:
>>
>> 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().
> 
> Ok. Firstly, I am planning to eliminate use of _set_timer() completly in
> all network drivers and then I will go for setup_timer(). Is it right
> strategy?
> 
> I am just little confused about sending them in series. Can I send a patch
> series [ one patch for each driver] for eliminating _set_timer()?? And then
> one different patch series goes for setup_timer().

I would recommend one patch series per driver, as opposed to one series
for all drivers. So for each driver I would do:

1: Patch to eliminate _set_timer()
2: Patch(es) to convert code to setup_timer()

It is better to do one patch set per driver, as it allows individual
driver maintainers to apply them without touching code they do not
maintainer.

Cheers,
Jes


>> 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));
>>>>>               }
>>>>>       }
>>>>>  }
>>>>>
>>>>
>>>
>>
>