usb: dwc3: debugfs: Add and remove endpoint dirs dynamically

PATCH In review Jack Pham 2 participants 4 messages May 29, 2021 – Jun 2, 2021

Subsystems (2)

USB DesignWare USB3 DRD IP driver

lore

Share & tools

lore

Jack Pham

author

2021-05-29 19:30 UTC #0 lore raw

PATCH 1/1 usb: dwc3: debugfs: Add and remove endpoint dirs dynamically

The DWC3 DebugFS directory and files are currently created once during probe. This includes creation of subdirectories for each of the gadget's endpoints. This works fine for peripheral-only controllers, as dwc3_core_init_mode() calls dwc3_gadget_init() just prior to calling dwc3_debugfs_init().

However, for dual-role controllers, dwc3_core_init_mode() will instead call dwc3_drd_init() which is problematic in a few ways. First, the initial state must be determined, then dwc3_set_mode() will have to schedule drd_work and by then dwc3_debugfs_init() could have already been invoked. Even if the initial mode is peripheral, dwc3_gadget_init() happens after the DebugFS files are created, and worse so if the initial state is host and the controller switches to peripheral much later. And secondly, even if the gadget endpoints' debug entries were successfully created, if the controller exits peripheral mode, its dwc3_eps are freed so the debug files would now hold stale references.

So it is best if the DebugFS endpoint entries are created and removed dynamically at the same time the underlying dwc3_eps are. Do this by calling dwc3_debugfs_create_endpoint_dir() as each endpoint is created, and conversely remove the DebugFS entry when the endpoint is freed.

Fixes 41ce1456e1db ("usb: dwc3: core: make dwc3_set_mode() work properly")

Signed-off-by Jack Pham <[email protected]>

---
 drivers/usb/dwc3/debug.h   |  3 +++
 drivers/usb/dwc3/debugfs.c | 21 ++-------------------
 drivers/usb/dwc3/gadget.c  |  3 +++
 3 files changed, 8 insertions(+), 19 deletions(-)

drivers/usb/dwc3/debug.h +3 -0

index d0ac89c5b317..d223c54115f4 100644
@@ -413,9 +413,12 @@ static inline const char *dwc3_gadget_generic_cmd_status_string(int status)
 
 
 #ifdef CONFIG_DEBUG_FS
+extern void dwc3_debugfs_create_endpoint_dir(struct dwc3_ep *dep);
 extern void dwc3_debugfs_init(struct dwc3 *d);
 extern void dwc3_debugfs_exit(struct dwc3 *d);
 #else
+static inline void dwc3_debugfs_create_endpoint_dir(struct dwc3_ep *dep)
+{  }
 static inline void dwc3_debugfs_init(struct dwc3 *d)
 {  }
 static inline void dwc3_debugfs_exit(struct dwc3 *d)

drivers/usb/dwc3/debugfs.c +2 ~~-19~~

index 7146ee2ac057..5dbbe53269d3 100644
@@ -886,30 +886,14 @@ static void dwc3_debugfs_create_endpoint_files(struct dwc3_ep *dep,
 	}
 }
 
-static void dwc3_debugfs_create_endpoint_dir(struct dwc3_ep *dep,
-		struct dentry *parent)
+void dwc3_debugfs_create_endpoint_dir(struct dwc3_ep *dep)
 {
 	struct dentry		*dir;
 
-	dir = debugfs_create_dir(dep->name, parent);
+	dir = debugfs_create_dir(dep->name, dep->dwc->root);
 	dwc3_debugfs_create_endpoint_files(dep, dir);
 }
 
-static void dwc3_debugfs_create_endpoint_dirs(struct dwc3 *dwc,
-		struct dentry *parent)
-{
-	int			i;
-
-	for (i = 0; i < dwc->num_eps; i++) {
-		struct dwc3_ep	*dep = dwc->eps[i];
-
-		if (!dep)
-			continue;
-
-		dwc3_debugfs_create_endpoint_dir(dep, parent);
-	}
-}
-
 void dwc3_debugfs_init(struct dwc3 *dwc)
 {
 	struct dentry		*root;

@@ -940,7 +924,6 @@ void dwc3_debugfs_init(struct dwc3 *dwc)
 				&dwc3_testmode_fops);
 		debugfs_create_file("link_state", 0644, root, dwc,
 				    &dwc3_link_state_fops);
-		dwc3_debugfs_create_endpoint_dirs(dwc, root);
 	}
 }

drivers/usb/dwc3/gadget.c +3 -0

index 65d9b7227752..dbba31d415d7 100644
@@ -2754,6 +2754,8 @@ static int dwc3_gadget_init_endpoint(struct dwc3 *dwc, u8 epnum)
 	INIT_LIST_HEAD(&dep->started_list);
 	INIT_LIST_HEAD(&dep->cancelled_list);
 
+	dwc3_debugfs_create_endpoint_dir(dep);
+
 	return 0;
 }

@@ -2797,6 +2799,7 @@ static void dwc3_gadget_free_endpoints(struct dwc3 *dwc)
 			list_del(&dep->endpoint.ep_list);
 		}
 
+		debugfs_remove_recursive(debugfs_lookup(dep->name, dwc->root));
 		kfree(dep);
 	}
 }

Signature · 1 line

2.24.0

Peter Chen

2021-06-01 07:07 UTC #1 lore raw

Quoted 108 lines · Jack Pham

The DWC3 DebugFS directory and files are currently created once during probe. This includes creation of subdirectories for each of the gadget's endpoints. This works fine for peripheral-only controllers, as dwc3_core_init_mode() calls dwc3_gadget_init() just prior to calling dwc3_debugfs_init().
However, for dual-role controllers, dwc3_core_init_mode() will instead call dwc3_drd_init() which is problematic in a few ways. First, the initial state must be determined, then dwc3_set_mode() will have to schedule drd_work and by then dwc3_debugfs_init() could have already been invoked. Even if the initial mode is peripheral, dwc3_gadget_init() happens after the DebugFS files are created, and worse so if the initial state is host and the controller switches to peripheral much later. And secondly, even if the gadget endpoints' debug entries were successfully created, if the controller exits peripheral mode, its dwc3_eps are freed so the debug files would now hold stale references.
So it is best if the DebugFS endpoint entries are created and removed dynamically at the same time the underlying dwc3_eps are. Do this by calling dwc3_debugfs_create_endpoint_dir() as each endpoint is created, and conversely remove the DebugFS entry when the endpoint is freed.
Fixes 41ce1456e1db ("usb: dwc3: core: make dwc3_set_mode() work properly")
Signed-off-by Jack Pham <[email protected]>
---
 drivers/usb/dwc3/debug.h   |  3 +++
 drivers/usb/dwc3/debugfs.c | 21 ++-------------------
 drivers/usb/dwc3/gadget.c  |  3 +++
 3 files changed, 8 insertions(+), 19 deletions(-)
drivers/usb/dwc3/debug.h +3 -0
index d0ac89c5b317..d223c54115f4 100644
@@ -413,9 +413,12 @@ static inline const char *dwc3_gadget_generic_cmd_status_string(int status)
 
 
 #ifdef CONFIG_DEBUG_FS
+extern void dwc3_debugfs_create_endpoint_dir(struct dwc3_ep *dep);
 extern void dwc3_debugfs_init(struct dwc3 *d);
 extern void dwc3_debugfs_exit(struct dwc3 *d);
 #else
+static inline void dwc3_debugfs_create_endpoint_dir(struct dwc3_ep *dep)
+{  }
 static inline void dwc3_debugfs_init(struct dwc3 *d)
 {  }
 static inline void dwc3_debugfs_exit(struct dwc3 *d)
drivers/usb/dwc3/debugfs.c +2 ~~-19~~
index 7146ee2ac057..5dbbe53269d3 100644
@@ -886,30 +886,14 @@ static void dwc3_debugfs_create_endpoint_files(struct dwc3_ep *dep,
 	}
 }
 
-static void dwc3_debugfs_create_endpoint_dir(struct dwc3_ep *dep,
-		struct dentry *parent)
+void dwc3_debugfs_create_endpoint_dir(struct dwc3_ep *dep)
 {
 	struct dentry		*dir;
 
-	dir = debugfs_create_dir(dep->name, parent);
+	dir = debugfs_create_dir(dep->name, dep->dwc->root);
 	dwc3_debugfs_create_endpoint_files(dep, dir);
 }
 
-static void dwc3_debugfs_create_endpoint_dirs(struct dwc3 *dwc,
-		struct dentry *parent)
-{
-	int			i;
-
-	for (i = 0; i < dwc->num_eps; i++) {
-		struct dwc3_ep	*dep = dwc->eps[i];
-
-		if (!dep)
-			continue;
-
-		dwc3_debugfs_create_endpoint_dir(dep, parent);
-	}
-}
-
 void dwc3_debugfs_init(struct dwc3 *dwc)
 {
 	struct dentry		*root;
@@ -940,7 +924,6 @@ void dwc3_debugfs_init(struct dwc3 *dwc)
 				&dwc3_testmode_fops);
 		debugfs_create_file("link_state", 0644, root, dwc,
 				    &dwc3_link_state_fops);
-		dwc3_debugfs_create_endpoint_dirs(dwc, root);
 	}
 }
 
drivers/usb/dwc3/gadget.c +2 -0
index 65d9b7227752..dbba31d415d7 100644
@@ -2754,6 +2754,8 @@ static int dwc3_gadget_init_endpoint(struct dwc3 *dwc, u8 epnum)
 	INIT_LIST_HEAD(&dep->started_list);
 	INIT_LIST_HEAD(&dep->cancelled_list);
 
+	dwc3_debugfs_create_endpoint_dir(dep);
+
 	return 0;
 }
 

drivers/usb/dwc3/gadget.c static void dwc3_gadget_free_endpoints(struct dwc3 *dwc) · line 2802

Quoted 4 lines

drivers/usb/dwc3/gadget.c +1 -0

@@ -2797,6 +2799,7 @@ static void dwc3_gadget_free_endpoints(struct dwc3 *dwc)
 			list_del(&dep->endpoint.ep_list);
 		}
 
+		debugfs_remove_recursive(debugfs_lookup(dep->name, dwc->root));

There is one more debugfs_remove_recursive at dwc3_debugfs_exit, need to delete?

Quoted 6 lines

drivers/usb/dwc3/gadget.c +0 -0
 		kfree(dep);
 	}
 }
Signature · 1 line
2.24.0

Signature · 2 lines

Thanks,
Peter Chen

Jack Pham

author

2021-06-01 16:09 UTC #2 lore raw

Hi Peter,

On Tue, Jun 01, 2021 at 03:07:44PM +0800, Peter Chen wrote:

<snip>

Quoted 13 lines

drivers/usb/dwc3/gadget.c +2 -0

index 65d9b7227752..dbba31d415d7 100644
@@ -2754,6 +2754,8 @@ static int dwc3_gadget_init_endpoint(struct dwc3 *dwc, u8 epnum)
 	INIT_LIST_HEAD(&dep->started_list);
 	INIT_LIST_HEAD(&dep->cancelled_list);
 
+	dwc3_debugfs_create_endpoint_dir(dep);
+
 	return 0;
 }

Quoted 7 lines

drivers/usb/dwc3/gadget.c static void dwc3_gadget_free_endpoints(struct dwc3 *dwc) · line 2802
Quoted 5 lines
drivers/usb/dwc3/gadget.c +1 -0
@@ -2797,6 +2799,7 @@ static void dwc3_gadget_free_endpoints(struct dwc3 *dwc)
 			list_del(&dep->endpoint.ep_list);
 		}
 
+		debugfs_remove_recursive(debugfs_lookup(dep->name, dwc->root));
There is one more debugfs_remove_recursive at dwc3_debugfs_exit, need to delete?

No I think it should be fine. dwc3_debugfs_exit() is only called by dwc3_remove(), and at that time it removes the debugfs directory for the entire instance from dwc->root, which includes the parent and all the endpoint subdirectories if present.

dwc3_core_exit_mode() -> dwc3_gadget_exit() is done after that, by which point the debugfs remove here at dwc3_gadget_free_endpoints() will be redundant and a no-op anyway.

Thanks,
Jack

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Peter Chen

Reviewed-by on v1 1/1

2021-06-02 00:56 UTC #3 lore raw

In reply to Jack Pham

Quoted 18 lines · Jack Pham

The DWC3 DebugFS directory and files are currently created once during probe. This includes creation of subdirectories for each of the gadget's endpoints. This works fine for peripheral-only controllers, as dwc3_core_init_mode() calls dwc3_gadget_init() just prior to calling dwc3_debugfs_init().
However, for dual-role controllers, dwc3_core_init_mode() will instead call dwc3_drd_init() which is problematic in a few ways. First, the initial state must be determined, then dwc3_set_mode() will have to schedule drd_work and by then dwc3_debugfs_init() could have already been invoked. Even if the initial mode is peripheral, dwc3_gadget_init() happens after the DebugFS files are created, and worse so if the initial state is host and the controller switches to peripheral much later. And secondly, even if the gadget endpoints' debug entries were successfully created, if the controller exits peripheral mode, its dwc3_eps are freed so the debug files would now hold stale references.

Quoted 8 lines

So it is best if the DebugFS endpoint entries are created and removed dynamically at the same time the underlying dwc3_eps are. Do this by calling dwc3_debugfs_create_endpoint_dir() as each endpoint is created, and conversely remove the DebugFS entry when the endpoint is freed.
Fixes 41ce1456e1db ("usb: dwc3: core: make dwc3_set_mode() work properly")
Signed-off-by Jack Pham <[email protected]>

Reviewed-by Peter Chen <[email protected]>

Peter

Quoted 92 lines

---
 drivers/usb/dwc3/debug.h   |  3 +++
 drivers/usb/dwc3/debugfs.c | 21 ++-------------------
 drivers/usb/dwc3/gadget.c  |  3 +++
 3 files changed, 8 insertions(+), 19 deletions(-)

drivers/usb/dwc3/debug.h +3 -0

index d0ac89c5b317..d223c54115f4 100644
@@ -413,9 +413,12 @@ static inline const char *dwc3_gadget_generic_cmd_status_string(int status)
 
 
 #ifdef CONFIG_DEBUG_FS
+extern void dwc3_debugfs_create_endpoint_dir(struct dwc3_ep *dep);
 extern void dwc3_debugfs_init(struct dwc3 *d);
 extern void dwc3_debugfs_exit(struct dwc3 *d);
 #else
+static inline void dwc3_debugfs_create_endpoint_dir(struct dwc3_ep *dep)
+{  }
 static inline void dwc3_debugfs_init(struct dwc3 *d)
 {  }
 static inline void dwc3_debugfs_exit(struct dwc3 *d)

drivers/usb/dwc3/debugfs.c +2 ~~-19~~

index 7146ee2ac057..5dbbe53269d3 100644
@@ -886,30 +886,14 @@ static void dwc3_debugfs_create_endpoint_files(struct dwc3_ep *dep,
 	}
 }
 
-static void dwc3_debugfs_create_endpoint_dir(struct dwc3_ep *dep,
-		struct dentry *parent)
+void dwc3_debugfs_create_endpoint_dir(struct dwc3_ep *dep)
 {
 	struct dentry		*dir;
 
-	dir = debugfs_create_dir(dep->name, parent);
+	dir = debugfs_create_dir(dep->name, dep->dwc->root);
 	dwc3_debugfs_create_endpoint_files(dep, dir);
 }
 
-static void dwc3_debugfs_create_endpoint_dirs(struct dwc3 *dwc,
-		struct dentry *parent)
-{
-	int			i;
-
-	for (i = 0; i < dwc->num_eps; i++) {
-		struct dwc3_ep	*dep = dwc->eps[i];
-
-		if (!dep)
-			continue;
-
-		dwc3_debugfs_create_endpoint_dir(dep, parent);
-	}
-}
-
 void dwc3_debugfs_init(struct dwc3 *dwc)
 {
 	struct dentry		*root;

@@ -940,7 +924,6 @@ void dwc3_debugfs_init(struct dwc3 *dwc)
 				&dwc3_testmode_fops);
 		debugfs_create_file("link_state", 0644, root, dwc,
 				    &dwc3_link_state_fops);
-		dwc3_debugfs_create_endpoint_dirs(dwc, root);
 	}
 }

drivers/usb/dwc3/gadget.c +3 -0

index 65d9b7227752..dbba31d415d7 100644
@@ -2754,6 +2754,8 @@ static int dwc3_gadget_init_endpoint(struct dwc3 *dwc, u8 epnum)
 	INIT_LIST_HEAD(&dep->started_list);
 	INIT_LIST_HEAD(&dep->cancelled_list);
 
+	dwc3_debugfs_create_endpoint_dir(dep);
+
 	return 0;
 }

@@ -2797,6 +2799,7 @@ static void dwc3_gadget_free_endpoints(struct dwc3 *dwc)
 			list_del(&dep->endpoint.ep_list);
 		}
 
+		debugfs_remove_recursive(debugfs_lookup(dep->name, dwc->root));
 		kfree(dep);
 	}
 }

Signature · 1 line

2.24.0

Signature · 2 lines

Thanks,
Peter Chen