From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Sent: Tuesday, October 5, 2021 7:11 PM
To: Linyu Yuan (QUIC) <redacted>
Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org
Subject: Re: [PATCH v5 1/3] usb: gadget: configfs: avoid list move operation
of usb_function

On Tue, Sep 07, 2021 at 09:09:35AM +0800, Linyu Yuan wrote:

quoted

add a new list which link all usb_function at configfs layers,
it means that after link a function a configuration,
from configfs layer, we can still found all functions,
it will allow trace all functions from configfs.

I am sorry, but I do not understand this paragraph.  Can you try
rewording it?

quoted

Reported-by: kernel test robot <redacted>

How did the kernel test robot report this?  You are adding a new
function here, it is not a bug you are fixing, right?

quoted

Signed-off-by: Linyu Yuan <redacted>
---
v2: fix unused cfg variable warning
v3: add struct inside configfs.c
v4: no change
v5: lost v2 fix, add it again

 drivers/usb/gadget/configfs.c | 55 ++++++++++++++++++++++++----------

---------

quoted

 1 file changed, 31 insertions(+), 24 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 477e72a..5b2e6f9 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c

@@ -58,6 +58,11 @@ static inline struct gadget_info

*to_gadget_info(struct config_item *item)

quoted

 	return container_of(to_config_group(item), struct gadget_info,

group);

quoted

 }

+struct config_usb_function {
+	struct list_head list;
+	struct usb_function *f;
+};

What lock protects this list?

quoted

+
 struct config_usb_cfg {
 	struct config_group group;
 	struct config_group strings_group;

@@ -420,7 +425,7 @@ static int config_usb_cfg_link(
 	struct usb_function_instance *fi = container_of(group,
 			struct usb_function_instance, group);
 	struct usb_function_instance *a_fi;
-	struct usb_function *f;
+	struct config_usb_function *cf;
 	int ret;

 	mutex_lock(&gi->lock);

@@ -438,21 +443,29 @@ static int config_usb_cfg_link(
 		goto out;
 	}

-	list_for_each_entry(f, &cfg->func_list, list) {
-		if (f->fi == fi) {
+	list_for_each_entry(cf, &cfg->func_list, list) {
+		if (cf->f->fi == fi) {
 			ret = -EEXIST;
 			goto out;
 		}
 	}

-	f = usb_get_function(fi);
-	if (IS_ERR(f)) {
-		ret = PTR_ERR(f);
+	cf = kzalloc(sizeof(*cf), GFP_KERNEL);

Why "kzalloc" and not "kmalloc"?

I don't understand why you are moving everything to a single list in the
system, what is wrong with the existing per-device one?

Thanks Greg,

Let me explain what I want to do in this  change,

For original code,  when add a function to configuration, it will add function 
struct usb_function {
...
struct list_head		list; [1]
};
to following list,
struct config_usb_cfg {
...
	struct list_head func_list; [2]
};
Then when bind happen, it will move [1] to following list,
struct usb_configuration {
...
struct list_head	functions; [3]
};

When unbind, it will move [1] back to [2].

We can see list [1] move between two list head, it is not easy to trace.

And when I add trace, I only want to get trace info from structure defined in configfs.c,

So I add a new list which ONLY add/remove to head [2] and it represent a function in configfs layer.
And original list [1] will ONLY add/remove to head [3].

thanks,

greg k-h