Re: [PATCH] drivers/base: fix iterator cleanup in attribute_container

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Feb 26, 2025 at 11:57:52PM +0530, Ayaan Mirza Baig wrote:
> This patch addresses a long-standing FIXME in
> dirvers/base/attribute_container.c, where the function
> failed to call klist_iter_exit() before breaking
> out of the loop when a matching class device was found.
> 
> The code is now refactored to use a goto-based cleanup approach
> that ensure klist_iter_exit() is invoked exactly once regardless
> of the exit path.
> 
> In addition, checkpatch.pl warnings were fixed along with this change.
> 
> Tested on a minimal Debian System in QEMU with GDB attached.
> No regressions or errors were observed.
> 
> Signed-off-by: Ayaan Mirza Baig <ayaanmirzabaig85@gmail.com>
> ---
>  drivers/base/attribute_container.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/base/attribute_container.c b/drivers/base/attribute_container.c
> index b6f941a6ab69..1e7af328dd49 100644
> --- a/drivers/base/attribute_container.c
> +++ b/drivers/base/attribute_container.c
> @@ -5,7 +5,7 @@
>   * Copyright (c) 2005 - James Bottomley <James.Bottomley@steeleye.com>
>   *
>   * The basic idea here is to enable a device to be attached to an
> - * aritrary numer of classes without having to allocate storage for them.
> + * aritrary number of classes without having to allocate storage for them.

Nit, why change this in this commit?

>   * Instead, the contained classes select the devices they need to attach
>   * to via a matching function.
>   */
> @@ -21,7 +21,8 @@
>  #include "base.h"
>  
>  /* This is a private structure used to tie the classdev and the
> - * container .. it should never be visible outside this file */
> + * container .. it should never be visible outside this file
> + */

Same for this one?
>  struct internal_container {
>  	struct klist_node node;
>  	struct attribute_container *cont;
> @@ -42,7 +43,6 @@ static void internal_container_klist_put(struct klist_node *n)
>  	put_device(&ic->classdev);
>  }
>  
> -
>  /**

And this?

I stopped reading here, please, if you are going to make logic changes,
just do that, don't mix things up in one larger commit, that's not how
we handle kernel patches at all.

Also, did you test a system that uses container devices?

thanks,

greg k-h