| Message ID | 20220331015818.28045-1-xiam0nd.tong@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | opp: use list iterator only inside the loop | expand |
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;
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
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;
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(-)