[PATCH v1 1/2] usb: typec: tcpm: Expose tcpm logs through a misc device

[PATCH v1 1/2] usb: typec: tcpm: Expose tcpm logs through a misc device

[Badhri Jagan Sridharan]

Although TCPM logs were primarily focussed to aid developers
during bringup, TCPM logs have proved to be critical even
post-bringup as it provides a good starting point to triage
interoperability issues with accessories. TCPM logs
are exposed through debugfs filesystem. For systems that
don't mount debugfs by default, this change introduces a
module parameter log_misc_dev which when set exports the
tcpm logs through a misc device. This change also leaves
the option of exporting tcpm logs through debugfs for
backwards compatibility.

Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 98 +++++++++++++++++++++++++----------
 1 file changed, 72 insertions(+), 26 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index c4fdc00a3bc8..b79194919b27 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -12,6 +12,7 @@
 #include <linux/jiffies.h>
 #include <linux/kernel.h>
 #include <linux/kthread.h>
+#include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/power_supply.h>
@@ -33,6 +34,10 @@
 
 #include <uapi/linux/sched/types.h>
 
+static bool modparam_log_misc_dev;
+module_param_named(log_misc_dev, modparam_log_misc_dev, bool, 0444);
+MODULE_PARM_DESC(log_misc_dev, "Expose tcpm logs through misc device");
+
 #define FOREACH_STATE(S)			\
 	S(INVALID_STATE),			\
 	S(TOGGLING),			\
@@ -465,13 +470,15 @@ struct tcpm_port {
 	 * SNK_READY for non-pd link.
 	 */
 	bool slow_charger_loop;
-#ifdef CONFIG_DEBUG_FS
+
 	struct dentry *dentry;
 	struct mutex logbuffer_lock;	/* log buffer access lock */
 	int logbuffer_head;
 	int logbuffer_tail;
 	u8 *logbuffer[LOG_BUFFER_ENTRIES];
-#endif
+
+	/* TCPM logs are exposed through misc device when modparam_log_misc_dev is set. */
+	struct miscdevice misc;
 };
 
 struct pd_rx_event {
@@ -565,8 +572,6 @@ static bool tcpm_port_is_disconnected(struct tcpm_port *port)
  * Logging
  */
 
-#ifdef CONFIG_DEBUG_FS
-
 static bool tcpm_log_full(struct tcpm_port *port)
 {
 	return port->logbuffer_tail ==
@@ -626,6 +631,9 @@ static void tcpm_log(struct tcpm_port *port, const char *fmt, ...)
 {
 	va_list args;
 
+	if (!modparam_log_misc_dev && !IS_ENABLED(CONFIG_DEBUG_FS))
+		return;
+
 	/* Do not log while disconnected and unattached */
 	if (tcpm_port_is_disconnected(port) &&
 	    (port->state == SRC_UNATTACHED || port->state == SNK_UNATTACHED ||
@@ -642,6 +650,9 @@ static void tcpm_log_force(struct tcpm_port *port, const char *fmt, ...)
 {
 	va_list args;
 
+	if (!modparam_log_misc_dev && !IS_ENABLED(CONFIG_DEBUG_FS))
+		return;
+
 	va_start(args, fmt);
 	_tcpm_log(port, fmt, args);
 	va_end(args);
@@ -651,6 +662,9 @@ static void tcpm_log_source_caps(struct tcpm_port *port)
 {
 	int i;
 
+	if (!modparam_log_misc_dev && !IS_ENABLED(CONFIG_DEBUG_FS))
+		return;
+
 	for (i = 0; i < port->nr_source_caps; i++) {
 		u32 pdo = port->source_caps[i];
 		enum pd_pdo_type type = pdo_type(pdo);
@@ -708,7 +722,7 @@ static void tcpm_log_source_caps(struct tcpm_port *port)
 	}
 }
 
-static int tcpm_debug_show(struct seq_file *s, void *v)
+static int tcpm_log_show(struct seq_file *s, void *v)
 {
 	struct tcpm_port *port = (struct tcpm_port *)s->private;
 	int tail;
@@ -725,23 +739,59 @@ static int tcpm_debug_show(struct seq_file *s, void *v)
 
 	return 0;
 }
-DEFINE_SHOW_ATTRIBUTE(tcpm_debug);
+DEFINE_SHOW_ATTRIBUTE(tcpm_log);
 
-static void tcpm_debugfs_init(struct tcpm_port *port)
+static int tcpm_log_dev_open(struct inode *inode, struct file *file)
+{
+	struct tcpm_port *port = container_of(file->private_data, struct tcpm_port, misc);
+
+	inode->i_private = port;
+	file->private_data = NULL;
+	return single_open(file, tcpm_log_show, inode->i_private);
+}
+
+static const struct file_operations tcpm_log_dev_operations = {
+	.owner = THIS_MODULE,
+	.open = tcpm_log_dev_open,
+	.read = seq_read,
+	.release = single_release,
+};
+
+static int tcpm_log_init(struct tcpm_port *port)
 {
 	char name[NAME_MAX];
+	int ret;
+
+	if (!modparam_log_misc_dev && !IS_ENABLED(CONFIG_DEBUG_FS))
+		return 0;
 
 	mutex_init(&port->logbuffer_lock);
 	snprintf(name, NAME_MAX, "tcpm-%s", dev_name(port->dev));
+	if (modparam_log_misc_dev) {
+		port->misc.minor = MISC_DYNAMIC_MINOR;
+		port->misc.name = name;
+		port->misc.fops = &tcpm_log_dev_operations;
+
+		ret = misc_register(&port->misc);
+		if (ret < 0)
+			dev_err(port->dev, "error while doing misc_register ret=%d\n", ret);
+		return ret;
+	}
+
 	port->dentry = debugfs_create_dir(name, usb_debug_root);
 	debugfs_create_file("log", S_IFREG | 0444, port->dentry, port,
-			    &tcpm_debug_fops);
+			    &tcpm_log_fops);
+
+	return 0;
 }
 
-static void tcpm_debugfs_exit(struct tcpm_port *port)
+static void tcpm_log_exit(struct tcpm_port *port)
 {
 	int i;
 
+	if (!modparam_log_misc_dev && !IS_ENABLED(CONFIG_DEBUG_FS))
+		return;
+
 	mutex_lock(&port->logbuffer_lock);
 	for (i = 0; i < LOG_BUFFER_ENTRIES; i++) {
 		kfree(port->logbuffer[i]);
@@ -749,21 +799,14 @@ static void tcpm_debugfs_exit(struct tcpm_port *port)
 	}
 	mutex_unlock(&port->logbuffer_lock);
 
+	if (modparam_log_misc_dev) {
+		misc_deregister(&port->misc);
+		return;
+	}
+
 	debugfs_remove(port->dentry);
 }
 
-#else
-
-__printf(2, 3)
-static void tcpm_log(const struct tcpm_port *port, const char *fmt, ...) { }
-__printf(2, 3)
-static void tcpm_log_force(struct tcpm_port *port, const char *fmt, ...) { }
-static void tcpm_log_source_caps(struct tcpm_port *port) { }
-static void tcpm_debugfs_init(const struct tcpm_port *port) { }
-static void tcpm_debugfs_exit(const struct tcpm_port *port) { }
-
-#endif
-
 static void tcpm_set_cc(struct tcpm_port *port, enum typec_cc_status cc)
 {
 	tcpm_log(port, "cc:=%d", cc);
@@ -6135,11 +6178,13 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 	init_completion(&port->tx_complete);
 	init_completion(&port->swap_complete);
 	init_completion(&port->pps_complete);
-	tcpm_debugfs_init(port);
+	err = tcpm_log_init(port);
+	if (err < 0)
+		goto out_destroy_wq;
 
 	err = tcpm_fw_get_caps(port, tcpc->fwnode);
 	if (err < 0)
-		goto out_destroy_wq;
+		goto out_unreg_log;
 
 	port->try_role = port->typec_caps.prefer_role;
 
@@ -6157,7 +6202,7 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 	port->role_sw = usb_role_switch_get(port->dev);
 	if (IS_ERR(port->role_sw)) {
 		err = PTR_ERR(port->role_sw);
-		goto out_destroy_wq;
+		goto out_unreg_log;
 	}
 
 	err = devm_tcpm_psy_register(port);
@@ -6184,8 +6229,9 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 
 out_role_sw_put:
 	usb_role_switch_put(port->role_sw);
+out_unreg_log:
+	tcpm_log_exit(port);
 out_destroy_wq:
-	tcpm_debugfs_exit(port);
 	kthread_destroy_worker(port->wq);
 	return ERR_PTR(err);
 }
@@ -6200,7 +6246,7 @@ void tcpm_unregister_port(struct tcpm_port *port)
 		typec_unregister_altmode(port->port_altmode[i]);
 	typec_unregister_port(port->typec_port);
 	usb_role_switch_put(port->role_sw);
-	tcpm_debugfs_exit(port);
+	tcpm_log_exit(port);
 	kthread_destroy_worker(port->wq);
 }
 EXPORT_SYMBOL_GPL(tcpm_unregister_port);
-- 
2.31.1.751.gd2f1c929bd-goog

[PATCH v1 2/2] usb: typec: tcpm: Add module parameter to wrap around logs

[Badhri Jagan Sridharan]

When the buffer is full, TCPM stops logging into the buffer.
This change adds log_wraparound module parameter which when set
flushes out the oldest log entries (FIFO) to make way for the
newer ones.

Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index b79194919b27..a369decade60 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -38,6 +38,10 @@ static bool modparam_log_misc_dev;
 module_param_named(log_misc_dev, modparam_log_misc_dev, bool, 0444);
 MODULE_PARM_DESC(log_misc_dev, "Expose tcpm logs through misc device");
 
+static bool modparam_log_wraparound;
+module_param_named(log_wraparound, modparam_log_wraparound, bool, 0444);
+MODULE_PARM_DESC(log_wraparound, "Wrap around logs");
+
 #define FOREACH_STATE(S)			\
 	S(INVALID_STATE),			\
 	S(TOGGLING),			\
@@ -597,7 +601,7 @@ static void _tcpm_log(struct tcpm_port *port, const char *fmt, va_list args)
 
 	vsnprintf(tmpbuffer, sizeof(tmpbuffer), fmt, args);
 
-	if (tcpm_log_full(port)) {
+	if (!modparam_log_wraparound && tcpm_log_full(port)) {
 		port->logbuffer_head = max(port->logbuffer_head - 1, 0);
 		strcpy(tmpbuffer, "overflow");
 	}
@@ -621,6 +625,9 @@ static void _tcpm_log(struct tcpm_port *port, const char *fmt, va_list args)
 		  (unsigned long)ts_nsec, rem_nsec / 1000,
 		  tmpbuffer);
 	port->logbuffer_head = (port->logbuffer_head + 1) % LOG_BUFFER_ENTRIES;
+	if (modparam_log_wraparound && port->logbuffer_head == port->logbuffer_tail)
+		port->logbuffer_tail =
+			(port->logbuffer_tail + 1) % LOG_BUFFER_ENTRIES;
 
 abort:
 	mutex_unlock(&port->logbuffer_lock);
@@ -733,7 +740,7 @@ static int tcpm_log_show(struct seq_file *s, void *v)
 		seq_printf(s, "%s\n", port->logbuffer[tail]);
 		tail = (tail + 1) % LOG_BUFFER_ENTRIES;
 	}
-	if (!seq_has_overflowed(s))
+	if (!modparam_log_wraparound && !seq_has_overflowed(s))
 		port->logbuffer_tail = tail;
 	mutex_unlock(&port->logbuffer_lock);
 
-- 
2.31.1.751.gd2f1c929bd-goog

Re: [PATCH v1 2/2] usb: typec: tcpm: Add module parameter to wrap around logs

[Greg Kroah-Hartman]

On Mon, May 17, 2021 at 04:36:09PM -0700, Badhri Jagan Sridharan wrote:
> When the buffer is full, TCPM stops logging into the buffer.
> This change adds log_wraparound module parameter which when set
> flushes out the oldest log entries (FIFO) to make way for the
> newer ones.
> 
> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index b79194919b27..a369decade60 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -38,6 +38,10 @@ static bool modparam_log_misc_dev;
>  module_param_named(log_misc_dev, modparam_log_misc_dev, bool, 0444);
>  MODULE_PARM_DESC(log_misc_dev, "Expose tcpm logs through misc device");
>  
> +static bool modparam_log_wraparound;
> +module_param_named(log_wraparound, modparam_log_wraparound, bool, 0444);
> +MODULE_PARM_DESC(log_wraparound, "Wrap around logs");

Again, no, this we know better than to add new module parameters, this
is not ok.

greg k-h

Re: [PATCH v1 1/2] usb: typec: tcpm: Expose tcpm logs through a misc device

[Heikki Krogerus]

On Mon, May 17, 2021 at 04:36:08PM -0700, Badhri Jagan Sridharan wrote:
> Although TCPM logs were primarily focussed to aid developers
> during bringup, TCPM logs have proved to be critical even
> post-bringup as it provides a good starting point to triage
> interoperability issues with accessories. TCPM logs
> are exposed through debugfs filesystem. For systems that
> don't mount debugfs by default, this change introduces a
> module parameter log_misc_dev which when set exports the
> tcpm logs through a misc device. This change also leaves
> the option of exporting tcpm logs through debugfs for
> backwards compatibility.

This does not look correct to me. At the very least you need to now
document your misc device interface. Why are you removing the choice
whether to enable this or not? The module parameter does also not
sound like a good idea at all.

I'm really not sure about this. This is just a poor man's debugfs that
removes any choice of enabling it. Since clearly debugging related
information is what you are after here, debugfs really should be
enough for you.

> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 98 +++++++++++++++++++++++++----------
>  1 file changed, 72 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index c4fdc00a3bc8..b79194919b27 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -12,6 +12,7 @@
>  #include <linux/jiffies.h>
>  #include <linux/kernel.h>
>  #include <linux/kthread.h>
> +#include <linux/miscdevice.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/power_supply.h>
> @@ -33,6 +34,10 @@
>  
>  #include <uapi/linux/sched/types.h>
>  
> +static bool modparam_log_misc_dev;
> +module_param_named(log_misc_dev, modparam_log_misc_dev, bool, 0444);
> +MODULE_PARM_DESC(log_misc_dev, "Expose tcpm logs through misc device");
> +
>  #define FOREACH_STATE(S)			\
>  	S(INVALID_STATE),			\
>  	S(TOGGLING),			\
> @@ -465,13 +470,15 @@ struct tcpm_port {
>  	 * SNK_READY for non-pd link.
>  	 */
>  	bool slow_charger_loop;
> -#ifdef CONFIG_DEBUG_FS
> +
>  	struct dentry *dentry;
>  	struct mutex logbuffer_lock;	/* log buffer access lock */
>  	int logbuffer_head;
>  	int logbuffer_tail;
>  	u8 *logbuffer[LOG_BUFFER_ENTRIES];
> -#endif
> +
> +	/* TCPM logs are exposed through misc device when modparam_log_misc_dev is set. */
> +	struct miscdevice misc;
>  };
>  
>  struct pd_rx_event {
> @@ -565,8 +572,6 @@ static bool tcpm_port_is_disconnected(struct tcpm_port *port)
>   * Logging
>   */
>  
> -#ifdef CONFIG_DEBUG_FS
> -
>  static bool tcpm_log_full(struct tcpm_port *port)
>  {
>  	return port->logbuffer_tail ==
> @@ -626,6 +631,9 @@ static void tcpm_log(struct tcpm_port *port, const char *fmt, ...)
>  {
>  	va_list args;
>  
> +	if (!modparam_log_misc_dev && !IS_ENABLED(CONFIG_DEBUG_FS))
> +		return;
> +
>  	/* Do not log while disconnected and unattached */
>  	if (tcpm_port_is_disconnected(port) &&
>  	    (port->state == SRC_UNATTACHED || port->state == SNK_UNATTACHED ||
> @@ -642,6 +650,9 @@ static void tcpm_log_force(struct tcpm_port *port, const char *fmt, ...)
>  {
>  	va_list args;
>  
> +	if (!modparam_log_misc_dev && !IS_ENABLED(CONFIG_DEBUG_FS))
> +		return;
> +
>  	va_start(args, fmt);
>  	_tcpm_log(port, fmt, args);
>  	va_end(args);
> @@ -651,6 +662,9 @@ static void tcpm_log_source_caps(struct tcpm_port *port)
>  {
>  	int i;
>  
> +	if (!modparam_log_misc_dev && !IS_ENABLED(CONFIG_DEBUG_FS))
> +		return;
> +
>  	for (i = 0; i < port->nr_source_caps; i++) {
>  		u32 pdo = port->source_caps[i];
>  		enum pd_pdo_type type = pdo_type(pdo);
> @@ -708,7 +722,7 @@ static void tcpm_log_source_caps(struct tcpm_port *port)
>  	}
>  }
>  
> -static int tcpm_debug_show(struct seq_file *s, void *v)
> +static int tcpm_log_show(struct seq_file *s, void *v)
>  {
>  	struct tcpm_port *port = (struct tcpm_port *)s->private;
>  	int tail;
> @@ -725,23 +739,59 @@ static int tcpm_debug_show(struct seq_file *s, void *v)
>  
>  	return 0;
>  }
> -DEFINE_SHOW_ATTRIBUTE(tcpm_debug);
> +DEFINE_SHOW_ATTRIBUTE(tcpm_log);
>  
> -static void tcpm_debugfs_init(struct tcpm_port *port)
> +static int tcpm_log_dev_open(struct inode *inode, struct file *file)
> +{
> +	struct tcpm_port *port = container_of(file->private_data, struct tcpm_port, misc);
> +
> +	inode->i_private = port;
> +	file->private_data = NULL;
> +	return single_open(file, tcpm_log_show, inode->i_private);
> +}
> +
> +static const struct file_operations tcpm_log_dev_operations = {
> +	.owner = THIS_MODULE,
> +	.open = tcpm_log_dev_open,
> +	.read = seq_read,
> +	.release = single_release,
> +};
> +
> +static int tcpm_log_init(struct tcpm_port *port)
>  {
>  	char name[NAME_MAX];
> +	int ret;
> +
> +	if (!modparam_log_misc_dev && !IS_ENABLED(CONFIG_DEBUG_FS))
> +		return 0;
>  
>  	mutex_init(&port->logbuffer_lock);
>  	snprintf(name, NAME_MAX, "tcpm-%s", dev_name(port->dev));
> +	if (modparam_log_misc_dev) {
> +		port->misc.minor = MISC_DYNAMIC_MINOR;
> +		port->misc.name = name;
> +		port->misc.fops = &tcpm_log_dev_operations;
> +
> +		ret = misc_register(&port->misc);
> +		if (ret < 0)
> +			dev_err(port->dev, "error while doing misc_register ret=%d\n", ret);
> +		return ret;
> +	}
> +
>  	port->dentry = debugfs_create_dir(name, usb_debug_root);
>  	debugfs_create_file("log", S_IFREG | 0444, port->dentry, port,
> -			    &tcpm_debug_fops);
> +			    &tcpm_log_fops);
> +
> +	return 0;
>  }
>  
> -static void tcpm_debugfs_exit(struct tcpm_port *port)
> +static void tcpm_log_exit(struct tcpm_port *port)
>  {
>  	int i;
>  
> +	if (!modparam_log_misc_dev && !IS_ENABLED(CONFIG_DEBUG_FS))
> +		return;
> +
>  	mutex_lock(&port->logbuffer_lock);
>  	for (i = 0; i < LOG_BUFFER_ENTRIES; i++) {
>  		kfree(port->logbuffer[i]);
> @@ -749,21 +799,14 @@ static void tcpm_debugfs_exit(struct tcpm_port *port)
>  	}
>  	mutex_unlock(&port->logbuffer_lock);
>  
> +	if (modparam_log_misc_dev) {
> +		misc_deregister(&port->misc);
> +		return;
> +	}
> +
>  	debugfs_remove(port->dentry);
>  }
>  
> -#else
> -
> -__printf(2, 3)
> -static void tcpm_log(const struct tcpm_port *port, const char *fmt, ...) { }
> -__printf(2, 3)
> -static void tcpm_log_force(struct tcpm_port *port, const char *fmt, ...) { }
> -static void tcpm_log_source_caps(struct tcpm_port *port) { }
> -static void tcpm_debugfs_init(const struct tcpm_port *port) { }
> -static void tcpm_debugfs_exit(const struct tcpm_port *port) { }
> -
> -#endif
> -
>  static void tcpm_set_cc(struct tcpm_port *port, enum typec_cc_status cc)
>  {
>  	tcpm_log(port, "cc:=%d", cc);
> @@ -6135,11 +6178,13 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>  	init_completion(&port->tx_complete);
>  	init_completion(&port->swap_complete);
>  	init_completion(&port->pps_complete);
> -	tcpm_debugfs_init(port);
> +	err = tcpm_log_init(port);
> +	if (err < 0)
> +		goto out_destroy_wq;
>  
>  	err = tcpm_fw_get_caps(port, tcpc->fwnode);
>  	if (err < 0)
> -		goto out_destroy_wq;
> +		goto out_unreg_log;
>  
>  	port->try_role = port->typec_caps.prefer_role;
>  
> @@ -6157,7 +6202,7 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>  	port->role_sw = usb_role_switch_get(port->dev);
>  	if (IS_ERR(port->role_sw)) {
>  		err = PTR_ERR(port->role_sw);
> -		goto out_destroy_wq;
> +		goto out_unreg_log;
>  	}
>  
>  	err = devm_tcpm_psy_register(port);
> @@ -6184,8 +6229,9 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>  
>  out_role_sw_put:
>  	usb_role_switch_put(port->role_sw);
> +out_unreg_log:
> +	tcpm_log_exit(port);
>  out_destroy_wq:
> -	tcpm_debugfs_exit(port);
>  	kthread_destroy_worker(port->wq);
>  	return ERR_PTR(err);
>  }
> @@ -6200,7 +6246,7 @@ void tcpm_unregister_port(struct tcpm_port *port)
>  		typec_unregister_altmode(port->port_altmode[i]);
>  	typec_unregister_port(port->typec_port);
>  	usb_role_switch_put(port->role_sw);
> -	tcpm_debugfs_exit(port);
> +	tcpm_log_exit(port);
>  	kthread_destroy_worker(port->wq);
>  }
>  EXPORT_SYMBOL_GPL(tcpm_unregister_port);
> -- 
> 2.31.1.751.gd2f1c929bd-goog

-- 
heikki

Re: [PATCH v1 1/2] usb: typec: tcpm: Expose tcpm logs through a misc device

[Greg Kroah-Hartman]

On Tue, May 18, 2021 at 04:00:48PM +0300, Heikki Krogerus wrote:
> On Mon, May 17, 2021 at 04:36:08PM -0700, Badhri Jagan Sridharan wrote:
> > Although TCPM logs were primarily focussed to aid developers
> > during bringup, TCPM logs have proved to be critical even
> > post-bringup as it provides a good starting point to triage
> > interoperability issues with accessories. TCPM logs
> > are exposed through debugfs filesystem. For systems that
> > don't mount debugfs by default, this change introduces a
> > module parameter log_misc_dev which when set exports the
> > tcpm logs through a misc device. This change also leaves
> > the option of exporting tcpm logs through debugfs for
> > backwards compatibility.
> 
> This does not look correct to me. At the very least you need to now
> document your misc device interface. Why are you removing the choice
> whether to enable this or not? The module parameter does also not
> sound like a good idea at all.
> 
> I'm really not sure about this. This is just a poor man's debugfs that
> removes any choice of enabling it. Since clearly debugging related
> information is what you are after here, debugfs really should be
> enough for you.

I agree, this is an abuse of a misc device, not ok at all.

Just use debugfs if you really want this type of thing, that is what it
is there for.

> 
> > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> > ---
> >  drivers/usb/typec/tcpm/tcpm.c | 98 +++++++++++++++++++++++++----------
> >  1 file changed, 72 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > index c4fdc00a3bc8..b79194919b27 100644
> > --- a/drivers/usb/typec/tcpm/tcpm.c
> > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/jiffies.h>
> >  #include <linux/kernel.h>
> >  #include <linux/kthread.h>
> > +#include <linux/miscdevice.h>
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> >  #include <linux/power_supply.h>
> > @@ -33,6 +34,10 @@
> >  
> >  #include <uapi/linux/sched/types.h>
> >  
> > +static bool modparam_log_misc_dev;
> > +module_param_named(log_misc_dev, modparam_log_misc_dev, bool, 0444);
> > +MODULE_PARM_DESC(log_misc_dev, "Expose tcpm logs through misc device");

This is not the 1990's, no new module parameters please.

thanks,

greg k-h

Re: [PATCH v1 1/2] usb: typec: tcpm: Expose tcpm logs through a misc device

[Greg Kroah-Hartman]

On Mon, May 17, 2021 at 04:36:08PM -0700, Badhri Jagan Sridharan wrote:
> Although TCPM logs were primarily focussed to aid developers
> during bringup, TCPM logs have proved to be critical even
> post-bringup as it provides a good starting point to triage
> interoperability issues with accessories. TCPM logs
> are exposed through debugfs filesystem. For systems that
> don't mount debugfs by default, this change introduces a
> module parameter log_misc_dev which when set exports the
> tcpm logs through a misc device. This change also leaves
> the option of exporting tcpm logs through debugfs for
> backwards compatibility.
> 
> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 98 +++++++++++++++++++++++++----------
>  1 file changed, 72 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index c4fdc00a3bc8..b79194919b27 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -12,6 +12,7 @@
>  #include <linux/jiffies.h>
>  #include <linux/kernel.h>
>  #include <linux/kthread.h>
> +#include <linux/miscdevice.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/power_supply.h>
> @@ -33,6 +34,10 @@
>  
>  #include <uapi/linux/sched/types.h>
>  
> +static bool modparam_log_misc_dev;
> +module_param_named(log_misc_dev, modparam_log_misc_dev, bool, 0444);
> +MODULE_PARM_DESC(log_misc_dev, "Expose tcpm logs through misc device");
> +
>  #define FOREACH_STATE(S)			\
>  	S(INVALID_STATE),			\
>  	S(TOGGLING),			\
> @@ -465,13 +470,15 @@ struct tcpm_port {
>  	 * SNK_READY for non-pd link.
>  	 */
>  	bool slow_charger_loop;
> -#ifdef CONFIG_DEBUG_FS
> +
>  	struct dentry *dentry;
>  	struct mutex logbuffer_lock;	/* log buffer access lock */
>  	int logbuffer_head;
>  	int logbuffer_tail;
>  	u8 *logbuffer[LOG_BUFFER_ENTRIES];
> -#endif
> +
> +	/* TCPM logs are exposed through misc device when modparam_log_misc_dev is set. */
> +	struct miscdevice misc;

From a technical point of view, this is not ok as now you have 2
different things controlling the lifecycle/lifespan of this structure,
making it impossible to audit and actually figure out what is happening
when.  Please do not do that.

greg k-h