| Message ID | 20220331230632.957634-1-jakobkoschel@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/3] iio: buffer: remove usage of list iterator variable for list_for_each_entry_continue_reverse() | expand |
Hi Jakob, > -----Original Message----- > From: Jakob Koschel <jakobkoschel@gmail.com> > Sent: Friday, April 1, 2022 1:07 AM > To: Jonathan Cameron <jic23@kernel.org> > Cc: Lars-Peter Clausen <lars@metafoo.de>; Dan Carpenter > <dan.carpenter@oracle.com>; Jakob Koschel > <jakobkoschel@gmail.com>; linux-iio@vger.kernel.org; linux- > kernel@vger.kernel.org; Mike Rapoport <rppt@kernel.org>; Brian > Johannesmeyer <bjohannesmeyer@gmail.com>; Cristiano Giuffrida > <c.giuffrida@vu.nl>; Bos, H.J. <h.j.bos@vu.nl> > Subject: [PATCH 1/3] iio: buffer: remove usage of list iterator variable > for list_for_each_entry_continue_reverse() > > [External] > > In preparation to limit the scope of the list iterator variable to the > list traversal loop, use a dedicated pointer to iterate through the > list [1]. > > Since that variable should not be used past the loop iteration, a > separate variable is used to 'remember the current location within the > loop'. > > To either continue iterating from that position or start a new > iteration (if the previous iteration was complete) list_prepare_entry() > is used. > > Link: https://urldefense.com/v3/__https://lore.kernel.org/all/CAHk- > =wgRr_D8CB-D9Kg- > c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/__;!!A3Ni8CS0y > 2Y!q8llw5UCaMIsAU7tPtPDhwVor0wy032I7FJHv0VxBZksNuRJF04HjWe > 0XYG7OQ$ [1] > Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com> > --- > drivers/iio/industrialio-buffer.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio- > buffer.c > index 208b5193c621..151a77c2affd 100644 > --- a/drivers/iio/industrialio-buffer.c > +++ b/drivers/iio/industrialio-buffer.c > @@ -1059,7 +1059,7 @@ static int iio_enable_buffers(struct iio_dev > *indio_dev, > struct iio_device_config *config) > { > struct iio_dev_opaque *iio_dev_opaque = > to_iio_dev_opaque(indio_dev); > - struct iio_buffer *buffer; > + struct iio_buffer *buffer, *tmp = NULL; > int ret; > > indio_dev->active_scan_mask = config->scan_mask; > @@ -1097,8 +1097,10 @@ static int iio_enable_buffers(struct iio_dev > *indio_dev, > > list_for_each_entry(buffer, &iio_dev_opaque->buffer_list, > buffer_list) { > ret = iio_buffer_enable(buffer, indio_dev); > - if (ret) > + if (ret) { > + tmp = buffer; > goto err_disable_buffers; > + } > } > > if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) { > @@ -1125,6 +1127,7 @@ static int iio_enable_buffers(struct iio_dev > *indio_dev, > indio_dev->pollfunc); > } > err_disable_buffers: > + buffer = list_prepare_entry(tmp, &iio_dev_opaque- > >buffer_list, buffer_list); Ok, it's Friday so I might be seeing ghosts... But looking at 'list_prepare_entry()'... If tmp != NULL, then all goes well but if tmp == NULL, then we get list_entry(&iio_dev_opaque->buffer_list, struct iio_buffer, buffer_list) which would require 'struct iio_dev_opaque'. It looks like like 'list_prepare_entry()' assumes that pos and head are of the same type... Am I missing something? - Nuno Sá
> On 1. Apr 2022, at 14:40, Sa, Nuno <Nuno.Sa@analog.com> wrote: > > Hi Jakob, > >> -----Original Message----- >> From: Jakob Koschel <jakobkoschel@gmail.com> >> Sent: Friday, April 1, 2022 1:07 AM >> To: Jonathan Cameron <jic23@kernel.org> >> Cc: Lars-Peter Clausen <lars@metafoo.de>; Dan Carpenter >> <dan.carpenter@oracle.com>; Jakob Koschel >> <jakobkoschel@gmail.com>; linux-iio@vger.kernel.org; linux- >> kernel@vger.kernel.org; Mike Rapoport <rppt@kernel.org>; Brian >> Johannesmeyer <bjohannesmeyer@gmail.com>; Cristiano Giuffrida >> <c.giuffrida@vu.nl>; Bos, H.J. <h.j.bos@vu.nl> >> Subject: [PATCH 1/3] iio: buffer: remove usage of list iterator variable >> for list_for_each_entry_continue_reverse() >> >> [External] >> >> In preparation to limit the scope of the list iterator variable to the >> list traversal loop, use a dedicated pointer to iterate through the >> list [1]. >> >> Since that variable should not be used past the loop iteration, a >> separate variable is used to 'remember the current location within the >> loop'. >> >> To either continue iterating from that position or start a new >> iteration (if the previous iteration was complete) list_prepare_entry() >> is used. >> >> Link: https://urldefense.com/v3/__https://lore.kernel.org/all/CAHk- >> =wgRr_D8CB-D9Kg- >> c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/__;!!A3Ni8CS0y >> 2Y!q8llw5UCaMIsAU7tPtPDhwVor0wy032I7FJHv0VxBZksNuRJF04HjWe >> 0XYG7OQ$ [1] >> Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com> >> --- >> drivers/iio/industrialio-buffer.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio- >> buffer.c >> index 208b5193c621..151a77c2affd 100644 >> --- a/drivers/iio/industrialio-buffer.c >> +++ b/drivers/iio/industrialio-buffer.c >> @@ -1059,7 +1059,7 @@ static int iio_enable_buffers(struct iio_dev >> *indio_dev, >> struct iio_device_config *config) >> { >> struct iio_dev_opaque *iio_dev_opaque = >> to_iio_dev_opaque(indio_dev); >> - struct iio_buffer *buffer; >> + struct iio_buffer *buffer, *tmp = NULL; >> int ret; >> >> indio_dev->active_scan_mask = config->scan_mask; >> @@ -1097,8 +1097,10 @@ static int iio_enable_buffers(struct iio_dev >> *indio_dev, >> >> list_for_each_entry(buffer, &iio_dev_opaque->buffer_list, >> buffer_list) { >> ret = iio_buffer_enable(buffer, indio_dev); >> - if (ret) >> + if (ret) { >> + tmp = buffer; >> goto err_disable_buffers; >> + } >> } >> >> if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) { >> @@ -1125,6 +1127,7 @@ static int iio_enable_buffers(struct iio_dev >> *indio_dev, >> indio_dev->pollfunc); >> } >> err_disable_buffers: >> + buffer = list_prepare_entry(tmp, &iio_dev_opaque- >>> buffer_list, buffer_list); > > Ok, it's Friday so I might be seeing ghosts... But looking at 'list_prepare_entry()'... > If tmp != NULL, then all goes well but if tmp == NULL, then we get > > list_entry(&iio_dev_opaque->buffer_list, struct iio_buffer, buffer_list) which > would require 'struct iio_dev_opaque'. It looks like like 'list_prepare_entry()' > assumes that pos and head are of the same type... > > Am I missing something? The list iterators are weird in this perspective... If you look at the original code, list_for_each_entry_continue_reverse() is called on 'buffer'. 'buffer' would be a valid struct element of &iio_dev_opaque->buffer_list if the break is hit, but if no break is hit in the earlier list_for_each_entry() buffer is not a valid entry. Before the terminating condition of list_for_each_entry() is met, it essentially does: buffer = list_entry(&iio_dev_opaque->buffer_list, typeof(*buffer), buffer_list); the buffer returned here is not a valid pointer to struct however. But since list_for_each_entry_continue_reverse() immediately calls list_prev_entry(buffer, buffer_list) on it you end up with the last entry of the list again and start iterating with that one. It's a very weird design choice but since list_for_each_entry_continue_reverse() expects a pointer to the element struct and not the list_head struct, you need to pass in this 'bogus' pointer if you want it to start on the head element. Keep in mind that the code here is just a more explicit version of this 'type confusion' whereas with the original code it was just hidden within the list_for_each_entry() macro and far less obvious. The functionality is exactly the same. PS: list_prepare_entry() was made for this use case, so basically it is expected to craft this bogus pointer from the head if pos == NULL. > > - Nuno Sá Thanks, Jakob
> From: Jakob Koschel <jakobkoschel@gmail.com> > Sent: Friday, April 1, 2022 3:55 PM > To: Sa, Nuno <Nuno.Sa@analog.com> > Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen > <lars@metafoo.de>; Dan Carpenter <dan.carpenter@oracle.com>; > linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org; Mike > Rapoport <rppt@kernel.org>; Brian Johannesmeyer > <bjohannesmeyer@gmail.com>; Cristiano Giuffrida > <c.giuffrida@vu.nl>; Bos, H.J. <h.j.bos@vu.nl> > Subject: Re: [PATCH 1/3] iio: buffer: remove usage of list iterator > variable for list_for_each_entry_continue_reverse() > > [External] > > > > > On 1. Apr 2022, at 14:40, Sa, Nuno <Nuno.Sa@analog.com> wrote: > > > > Hi Jakob, > > > >> -----Original Message----- > >> From: Jakob Koschel <jakobkoschel@gmail.com> > >> Sent: Friday, April 1, 2022 1:07 AM > >> To: Jonathan Cameron <jic23@kernel.org> > >> Cc: Lars-Peter Clausen <lars@metafoo.de>; Dan Carpenter > >> <dan.carpenter@oracle.com>; Jakob Koschel > >> <jakobkoschel@gmail.com>; linux-iio@vger.kernel.org; linux- > >> kernel@vger.kernel.org; Mike Rapoport <rppt@kernel.org>; Brian > >> Johannesmeyer <bjohannesmeyer@gmail.com>; Cristiano > Giuffrida > >> <c.giuffrida@vu.nl>; Bos, H.J. <h.j.bos@vu.nl> > >> Subject: [PATCH 1/3] iio: buffer: remove usage of list iterator > variable > >> for list_for_each_entry_continue_reverse() > >> > >> [External] > >> > >> In preparation to limit the scope of the list iterator variable to the > >> list traversal loop, use a dedicated pointer to iterate through the > >> list [1]. > >> > >> Since that variable should not be used past the loop iteration, a > >> separate variable is used to 'remember the current location within > the > >> loop'. > >> > >> To either continue iterating from that position or start a new > >> iteration (if the previous iteration was complete) > list_prepare_entry() > >> is used. > >> > >> Link: > https://urldefense.com/v3/__https://lore.kernel.org/all/CAHk- > >> =wgRr_D8CB-D9Kg- > >> > c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/__;!!A3Ni8CS0y > >> > 2Y!q8llw5UCaMIsAU7tPtPDhwVor0wy032I7FJHv0VxBZksNuRJF04HjWe > >> 0XYG7OQ$ [1] > >> Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com> > >> --- > >> drivers/iio/industrialio-buffer.c | 7 +++++-- > >> 1 file changed, 5 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio- > >> buffer.c > >> index 208b5193c621..151a77c2affd 100644 > >> --- a/drivers/iio/industrialio-buffer.c > >> +++ b/drivers/iio/industrialio-buffer.c > >> @@ -1059,7 +1059,7 @@ static int iio_enable_buffers(struct > iio_dev > >> *indio_dev, > >> struct iio_device_config *config) > >> { > >> struct iio_dev_opaque *iio_dev_opaque = > >> to_iio_dev_opaque(indio_dev); > >> - struct iio_buffer *buffer; > >> + struct iio_buffer *buffer, *tmp = NULL; > >> int ret; > >> > >> indio_dev->active_scan_mask = config->scan_mask; > >> @@ -1097,8 +1097,10 @@ static int iio_enable_buffers(struct > iio_dev > >> *indio_dev, > >> > >> list_for_each_entry(buffer, &iio_dev_opaque->buffer_list, > >> buffer_list) { > >> ret = iio_buffer_enable(buffer, indio_dev); > >> - if (ret) > >> + if (ret) { > >> + tmp = buffer; > >> goto err_disable_buffers; > >> + } > >> } > >> > >> if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) { > >> @@ -1125,6 +1127,7 @@ static int iio_enable_buffers(struct > iio_dev > >> *indio_dev, > >> indio_dev->pollfunc); > >> } > >> err_disable_buffers: > >> + buffer = list_prepare_entry(tmp, &iio_dev_opaque- > >>> buffer_list, buffer_list); > > > > Ok, it's Friday so I might be seeing ghosts... But looking at > 'list_prepare_entry()'... > > If tmp != NULL, then all goes well but if tmp == NULL, then we get > > > > list_entry(&iio_dev_opaque->buffer_list, struct iio_buffer, > buffer_list) which > > would require 'struct iio_dev_opaque'. It looks like like > 'list_prepare_entry()' > > assumes that pos and head are of the same type... > > > > Am I missing something? > > The list iterators are weird in this perspective... > > If you look at the original code, > list_for_each_entry_continue_reverse() is called on 'buffer'. > > 'buffer' would be a valid struct element of &iio_dev_opaque- > >buffer_list if the break is hit, > but if no break is hit in the earlier list_for_each_entry() buffer is not a > valid entry. > > Before the terminating condition of list_for_each_entry() is met, it > essentially does: > > buffer = list_entry(&iio_dev_opaque->buffer_list, > typeof(*buffer), buffer_list); > > the buffer returned here is not a valid pointer to struct however. > But since list_for_each_entry_continue_reverse() immediately calls > list_prev_entry(buffer, buffer_list) > on it you end up with the last entry of the list again and start iterating > with that one. Ahh I see it now... I really never had noticed this dance but in fact the same dance is also used to detect the end of list_for_each_entry()... Ok, then: Reviewed-by: Nuno Sá <nuno.sa@analog.com>
On Fri, 1 Apr 2022 01:06:30 +0200 Jakob Koschel <jakobkoschel@gmail.com> wrote: > In preparation to limit the scope of the list iterator variable to the > list traversal loop, use a dedicated pointer to iterate through the > list [1]. > > Since that variable should not be used past the loop iteration, a > separate variable is used to 'remember the current location within the > loop'. > > To either continue iterating from that position or start a new > iteration (if the previous iteration was complete) list_prepare_entry() > is used. > > Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1] > Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com> Hi Jakob, Series applied to the togreg branch of iio.git and pushed out as testing to let 0-day take a poke at it. Note I'll be rebasing that branch on rc1 once available before pushing it out for linux-next to pick up. Note I'm happy to add additional tags if anyone else takes a look at it. Thanks Jonathan > --- > drivers/iio/industrialio-buffer.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c > index 208b5193c621..151a77c2affd 100644 > --- a/drivers/iio/industrialio-buffer.c > +++ b/drivers/iio/industrialio-buffer.c > @@ -1059,7 +1059,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev, > struct iio_device_config *config) > { > struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); > - struct iio_buffer *buffer; > + struct iio_buffer *buffer, *tmp = NULL; > int ret; > > indio_dev->active_scan_mask = config->scan_mask; > @@ -1097,8 +1097,10 @@ static int iio_enable_buffers(struct iio_dev *indio_dev, > > list_for_each_entry(buffer, &iio_dev_opaque->buffer_list, buffer_list) { > ret = iio_buffer_enable(buffer, indio_dev); > - if (ret) > + if (ret) { > + tmp = buffer; > goto err_disable_buffers; > + } > } > > if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) { > @@ -1125,6 +1127,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev, > indio_dev->pollfunc); > } > err_disable_buffers: > + buffer = list_prepare_entry(tmp, &iio_dev_opaque->buffer_list, buffer_list); > list_for_each_entry_continue_reverse(buffer, &iio_dev_opaque->buffer_list, > buffer_list) > iio_buffer_disable(buffer, indio_dev); > > base-commit: f82da161ea75dc4db21b2499e4b1facd36dab275 > -- > 2.25.1 >
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c index 208b5193c621..151a77c2affd 100644 --- a/drivers/iio/industrialio-buffer.c +++ b/drivers/iio/industrialio-buffer.c @@ -1059,7 +1059,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev, struct iio_device_config *config) { struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); - struct iio_buffer *buffer; + struct iio_buffer *buffer, *tmp = NULL; int ret; indio_dev->active_scan_mask = config->scan_mask; @@ -1097,8 +1097,10 @@ static int iio_enable_buffers(struct iio_dev *indio_dev, list_for_each_entry(buffer, &iio_dev_opaque->buffer_list, buffer_list) { ret = iio_buffer_enable(buffer, indio_dev); - if (ret) + if (ret) { + tmp = buffer; goto err_disable_buffers; + } } if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) { @@ -1125,6 +1127,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev, indio_dev->pollfunc); } err_disable_buffers: + buffer = list_prepare_entry(tmp, &iio_dev_opaque->buffer_list, buffer_list); list_for_each_entry_continue_reverse(buffer, &iio_dev_opaque->buffer_list, buffer_list) iio_buffer_disable(buffer, indio_dev);
In preparation to limit the scope of the list iterator variable to the list traversal loop, use a dedicated pointer to iterate through the list [1]. Since that variable should not be used past the loop iteration, a separate variable is used to 'remember the current location within the loop'. To either continue iterating from that position or start a new iteration (if the previous iteration was complete) list_prepare_entry() is used. Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1] Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com> --- drivers/iio/industrialio-buffer.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) base-commit: f82da161ea75dc4db21b2499e4b1facd36dab275 -- 2.25.1