> 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. 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
