[PATCH v2 05/21] powerpc/papr_scm: Convert timeouts to secs_to_jiffies()

Easwar Hariharan posted 21 patches 1 week, 2 days ago
[PATCH v2 05/21] powerpc/papr_scm: Convert timeouts to secs_to_jiffies()
Posted by Easwar Hariharan 1 week, 2 days ago
Changes made with the following Coccinelle rules:

@@ constant C; @@

- msecs_to_jiffies(C * 1000)
+ secs_to_jiffies(C)

@@ constant C; @@

- msecs_to_jiffies(C * MSEC_PER_SEC)
+ secs_to_jiffies(C)

Signed-off-by: Easwar Hariharan <eahariha@linux.microsoft.com>
---
 arch/powerpc/platforms/pseries/papr_scm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index 9e297f88adc5d97d4dc7b267b0bfebd58e5cf193..9e8086ec66e0f0e555ac27933854c06cfcf91a04 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -543,7 +543,7 @@ static int drc_pmem_query_health(struct papr_scm_priv *p)
 
 	/* Jiffies offset for which the health data is assumed to be same */
 	cache_timeout = p->lasthealth_jiffies +
-		msecs_to_jiffies(MIN_HEALTH_QUERY_INTERVAL * 1000);
+		secs_to_jiffies(MIN_HEALTH_QUERY_INTERVAL);
 
 	/* Fetch new health info is its older than MIN_HEALTH_QUERY_INTERVAL */
 	if (time_after(jiffies, cache_timeout))

-- 
2.34.1
Re: [PATCH v2 05/21] powerpc/papr_scm: Convert timeouts to secs_to_jiffies()
Posted by Christophe Leroy 1 week, 1 day ago

Le 15/11/2024 à 22:26, Easwar Hariharan a écrit :
> [Vous ne recevez pas souvent de courriers de eahariha@linux.microsoft.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
> 
> Changes made with the following Coccinelle rules:
> 
> @@ constant C; @@
> 
> - msecs_to_jiffies(C * 1000)
> + secs_to_jiffies(C)
> 
> @@ constant C; @@
> 
> - msecs_to_jiffies(C * MSEC_PER_SEC)
> + secs_to_jiffies(C)

Is it a special script or is it done with the script in patch 2.

That's nice to say how it is done, but you also have to say _what_ and 
_why_ you do it. This is even more important as you plan to get it 
merged independently in each tree instead of merging it as a single series.

It could be something like:

Commit b35108a51cf7 ("jiffies: Define secs_to_jiffies()") introduced 
secs_to_jiffies(). As the value here is a multiple of 1000, use 
secs_to_jiffies() instead of msecs_to_jiffies to avoid the multiplication.

This is converted using scripts/coccinelle/misc/secs_to_jiffies.cocci

> 
> Signed-off-by: Easwar Hariharan <eahariha@linux.microsoft.com>
> ---
>   arch/powerpc/platforms/pseries/papr_scm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index 9e297f88adc5d97d4dc7b267b0bfebd58e5cf193..9e8086ec66e0f0e555ac27933854c06cfcf91a04 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -543,7 +543,7 @@ static int drc_pmem_query_health(struct papr_scm_priv *p)
> 
>          /* Jiffies offset for which the health data is assumed to be same */
>          cache_timeout = p->lasthealth_jiffies +
> -               msecs_to_jiffies(MIN_HEALTH_QUERY_INTERVAL * 1000);
> +               secs_to_jiffies(MIN_HEALTH_QUERY_INTERVAL);

Wouldn't it now fit on a single line ?

	cache_timeout = p->lasthealth_jiffies + 
secs_to_jiffies(MIN_HEALTH_QUERY_INTERVAL);


Also I'm not sure it is worth the MIN_HEALTH_QUERY_INTERVAL macro as it 
is defined localy and used only once, but that's another story.

> 
>          /* Fetch new health info is its older than MIN_HEALTH_QUERY_INTERVAL */
>          if (time_after(jiffies, cache_timeout))
> 
> --
> 2.34.1
> 
Re: [PATCH v2 05/21] powerpc/papr_scm: Convert timeouts to secs_to_jiffies()
Posted by Dan Carpenter 1 week, 1 day ago
On Sat, Nov 16, 2024 at 11:06:55AM +0100, Christophe Leroy wrote:
> > diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> > index 9e297f88adc5d97d4dc7b267b0bfebd58e5cf193..9e8086ec66e0f0e555ac27933854c06cfcf91a04 100644
> > --- a/arch/powerpc/platforms/pseries/papr_scm.c
> > +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> > @@ -543,7 +543,7 @@ static int drc_pmem_query_health(struct papr_scm_priv *p)
> > 
> >          /* Jiffies offset for which the health data is assumed to be same */
> >          cache_timeout = p->lasthealth_jiffies +
> > -               msecs_to_jiffies(MIN_HEALTH_QUERY_INTERVAL * 1000);
> > +               secs_to_jiffies(MIN_HEALTH_QUERY_INTERVAL);
> 
> Wouldn't it now fit on a single line ?
> 

Some maintainers still prefer to put a line break at 80 characters.  It's kind
of a nightmare for an automated script like this to figure out everyone's
preferences.  In this particular file, there are some lines which go over 80
characters so sure.  Earlier in the patchset one of these introduced a line
break that wasn't there before so I think maybe Coccinelle is applying the 80
character line break rule?

There are sometimes where the 80 character rule really hurts readability, but
here it doesn't make any difference.

regards,
dan carpenter
Re: [PATCH v2 05/21] powerpc/papr_scm: Convert timeouts to secs_to_jiffies()
Posted by Julia Lawall 1 week, 1 day ago
Sent from my iPhone

> On 16 Nov 2024, at 05:40, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> 
> On Sat, Nov 16, 2024 at 11:06:55AM +0100, Christophe Leroy wrote:
>>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>>> index 9e297f88adc5d97d4dc7b267b0bfebd58e5cf193..9e8086ec66e0f0e555ac27933854c06cfcf91a04 100644
>>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>>> @@ -543,7 +543,7 @@ static int drc_pmem_query_health(struct papr_scm_priv *p)
>>> 
>>>         /* Jiffies offset for which the health data is assumed to be same */
>>>         cache_timeout = p->lasthealth_jiffies +
>>> -               msecs_to_jiffies(MIN_HEALTH_QUERY_INTERVAL * 1000);
>>> +               secs_to_jiffies(MIN_HEALTH_QUERY_INTERVAL);
>> 
>> Wouldn't it now fit on a single line ?
>> 
> 
> Some maintainers still prefer to put a line break at 80 characters.  

Coccinelle tries for 80 chars. It may have a command line option to specify something else.

Julia

> It's kind
> of a nightmare for an automated script like this to figure out everyone's
> preferences.  In this particular
> file, there are some lines which go over 80
> characters so sure.  Earlier in the patchset one of these introduced a line
> break that wasn't there before so I think maybe Coccinelle is applying the 80
> character line break rule?
> 
> There are sometimes where the 80 character rule really hurts readability, but
> here it doesn't make any difference.
> 
> regards,
> dan carpenter
>