[v1] opp: use list iterator only inside the loop

[PATCH] opp: use list iterator only inside the loop

Posted by Xiaomeng Tong 4 years, 2 months ago

    dev = new_dev->dev;

As discussed before, we should avoid to use a list iterator variable
outside the loop which is considered harmful[1].

In this case, use a new variable 'iter' as the list iterator, while
use the old variable 'new_dev' as a dedicated pointer to point to the
found entry.

[1]:  https://lkml.org/lkml/2022/2/17/1032

Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
---
 drivers/opp/debugfs.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/opp/debugfs.c b/drivers/opp/debugfs.c
index 596c185b5dda..a4476985e4ce 100644
--- a/drivers/opp/debugfs.c
+++ b/drivers/opp/debugfs.c
@@ -187,14 +187,19 @@ void opp_debug_register(struct opp_device *opp_dev, struct opp_table *opp_table)
 static void opp_migrate_dentry(struct opp_device *opp_dev,
 			       struct opp_table *opp_table)
 {
-	struct opp_device *new_dev;
+	struct opp_device *new_dev = NULL, *iter;
 	const struct device *dev;
 	struct dentry *dentry;
 
 	/* Look for next opp-dev */
-	list_for_each_entry(new_dev, &opp_table->dev_list, node)
-		if (new_dev != opp_dev)
+	list_for_each_entry(iter, &opp_table->dev_list, node)
+		if (iter != opp_dev) {
+			new_dev = iter;
 			break;
+		}
+
+	if (!new_dev)
+		return;
 
 	/* new_dev is guaranteed to be valid here */
 	dev = new_dev->dev;
-- 
2.17.1

Re: [PATCH] opp: use list iterator only inside the loop

Posted by Viresh Kumar 4 years, 2 months ago

Hi Xiaomeng,

On 31-03-22, 09:58, Xiaomeng Tong wrote:
>     dev = new_dev->dev;

Why is this added here ?

> 
> As discussed before,

Please remember that whatever you write here will go in the commit
logs for ever and no one will ever know what you discussed and with
whom.

This area should describe the problem at hand.

> we should avoid to use a list iterator variable
> outside the loop which is considered harmful[1].
> 
> In this case, use a new variable 'iter' as the list iterator, while
> use the old variable 'new_dev' as a dedicated pointer to point to the
> found entry.
> 
> [1]:  https://lkml.org/lkml/2022/2/17/1032
> 
> Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
> ---
>  drivers/opp/debugfs.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/opp/debugfs.c b/drivers/opp/debugfs.c
> index 596c185b5dda..a4476985e4ce 100644
> --- a/drivers/opp/debugfs.c
> +++ b/drivers/opp/debugfs.c
> @@ -187,14 +187,19 @@ void opp_debug_register(struct opp_device *opp_dev, struct opp_table *opp_table)
>  static void opp_migrate_dentry(struct opp_device *opp_dev,
>  			       struct opp_table *opp_table)
>  {
> -	struct opp_device *new_dev;
> +	struct opp_device *new_dev = NULL, *iter;
>  	const struct device *dev;
>  	struct dentry *dentry;
>  
>  	/* Look for next opp-dev */
> -	list_for_each_entry(new_dev, &opp_table->dev_list, node)
> -		if (new_dev != opp_dev)
> +	list_for_each_entry(iter, &opp_table->dev_list, node)
> +		if (iter != opp_dev) {
> +			new_dev = iter;
>  			break;
> +		}
> +
> +	if (!new_dev)

I will rather make this BUG_ON(!new_dev);

> +		return;
>  
>  	/* new_dev is guaranteed to be valid here */
>  	dev = new_dev->dev;


-- 
viresh

Re: [PATCH] opp: use list iterator only inside the loop

Posted by Xiaomeng Tong 4 years, 2 months ago

On  Thu, 31 Mar 2022 07:42:35 +0530, Viresh Kumar wrote:

> Hi Xiaomeng,
> 
> On 31-03-22, 09:58, Xiaomeng Tong wrote:
> >     dev = new_dev->dev;
> 
> Why is this added here ?

Sorry for that. I will delete it in next patch.

> 
> > 
> > As discussed before,
> 
> Please remember that whatever you write here will go in the commit
> logs for ever and no one will ever know what you discussed and with
> whom.
> 
> This area should describe the problem at hand.
> 

ok, thank you for the suggestion.

> > we should avoid to use a list iterator variable
> > outside the loop which is considered harmful[1].
> > 
> > In this case, use a new variable 'iter' as the list iterator, while
> > use the old variable 'new_dev' as a dedicated pointer to point to the
> > found entry.
> > 
> > [1]:  https://lkml.org/lkml/2022/2/17/1032
> > 
> > Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
> > ---
> >  drivers/opp/debugfs.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/opp/debugfs.c b/drivers/opp/debugfs.c
> > index 596c185b5dda..a4476985e4ce 100644
> > --- a/drivers/opp/debugfs.c
> > +++ b/drivers/opp/debugfs.c
> > @@ -187,14 +187,19 @@ void opp_debug_register(struct opp_device *opp_dev, struct opp_table *opp_table)
> >  static void opp_migrate_dentry(struct opp_device *opp_dev,
> >  			       struct opp_table *opp_table)
> >  {
> > -	struct opp_device *new_dev;
> > +	struct opp_device *new_dev = NULL, *iter;
> >  	const struct device *dev;
> >  	struct dentry *dentry;
> >  
> >  	/* Look for next opp-dev */
> > -	list_for_each_entry(new_dev, &opp_table->dev_list, node)
> > -		if (new_dev != opp_dev)
> > +	list_for_each_entry(iter, &opp_table->dev_list, node)
> > +		if (iter != opp_dev) {
> > +			new_dev = iter;
> >  			break;
> > +		}
> > +
> > +	if (!new_dev)
> 
> I will rather make this BUG_ON(!new_dev);
> 

Ok, i will take it.

> > +		return;
> >  
> >  	/* new_dev is guaranteed to be valid here */
> >  	dev = new_dev->dev;

--
Xiaomeng Tong