> From: Jakob Koschel <jakobkoschel@xxxxxxxxx> > Sent: Friday, April 1, 2022 3:55 PM > To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx> > Cc: Jonathan Cameron <jic23@xxxxxxxxxx>; Lars-Peter Clausen > <lars@xxxxxxxxxx>; Dan Carpenter <dan.carpenter@xxxxxxxxxx>; > linux-iio@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Mike > Rapoport <rppt@xxxxxxxxxx>; Brian Johannesmeyer > <bjohannesmeyer@xxxxxxxxx>; Cristiano Giuffrida > <c.giuffrida@xxxxx>; Bos, H.J. <h.j.bos@xxxxx> > 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@xxxxxxxxxx> wrote: > > > > Hi Jakob, > > > >> -----Original Message----- > >> From: Jakob Koschel <jakobkoschel@xxxxxxxxx> > >> Sent: Friday, April 1, 2022 1:07 AM > >> To: Jonathan Cameron <jic23@xxxxxxxxxx> > >> Cc: Lars-Peter Clausen <lars@xxxxxxxxxx>; Dan Carpenter > >> <dan.carpenter@xxxxxxxxxx>; Jakob Koschel > >> <jakobkoschel@xxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx; linux- > >> kernel@xxxxxxxxxxxxxxx; Mike Rapoport <rppt@xxxxxxxxxx>; Brian > >> Johannesmeyer <bjohannesmeyer@xxxxxxxxx>; Cristiano > Giuffrida > >> <c.giuffrida@xxxxx>; Bos, H.J. <h.j.bos@xxxxx> > >> 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@xxxxxxxxxxxxxx/__;!!A3Ni8CS0y > >> > 2Y!q8llw5UCaMIsAU7tPtPDhwVor0wy032I7FJHv0VxBZksNuRJF04HjWe > >> 0XYG7OQ$ [1] > >> Signed-off-by: Jakob Koschel <jakobkoschel@xxxxxxxxx> > >> --- > >> 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@xxxxxxxxxx>
