usb: typec: tcpm: Add kernel config to wrap around tcpm logs
Subsystems (3)
Badhri Jagan Sridharan
maintainerPATCH 1/1 usb: typec: tcpm: Add kernel config to wrap around tcpm logs
This change adds CONFIG_TCPM_LOG_WRAPAROUND which when set allows the logs to be wrapped around. Additionally, when set, does not clear the TCPM logs when dumped.
--- drivers/usb/typec/tcpm/Kconfig | 6 ++++++ drivers/usb/typec/tcpm/tcpm.c | 9 +++++++-- 2 files changed, 13 insertions(+), 2 deletions(-)
drivers/usb/typec/tcpm/Kconfig +6 -0
@@ -18,6 +18,12 @@ config TYPEC_TCPCI help Type-C Port Controller driver for TCPCI-compliant controller. +config TCPM_LOG_WRAPAROUND + bool "Enable TCPM log wraparound" + help + When set, wraps around TCPM logs and does not clear the logs when dumped. TCPM logs by + default gets cleared when dumped and does not wraparound when full. + if TYPEC_TCPCI config TYPEC_RT1711H
drivers/usb/typec/tcpm/tcpm.c +7 -2
@@ -619,7 +619,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 (!IS_ENABLED (CONFIG_TCPM_LOG_WRAPAROUND ) &&tcpm_log_full (port)) { port->logbuffer_head =max (port->logbuffer_head -1 ,0 );strcpy (tmpbuffer,"overflow" ); }
@@ -644,6 +644,10 @@ static void _tcpm_log(struct tcpm_port *port, const char *fmt, va_list args) tmpbuffer); port->logbuffer_head = (port->logbuffer_head +1 ) %LOG_BUFFER_ENTRIES ; +if (IS_ENABLED (CONFIG_TCPM_LOG_WRAPAROUND ) && port->logbuffer_head == port->logbuffer_tail) + port->logbuffer_tail = + (port->logbuffer_tail +1 ) %LOG_BUFFER_ENTRIES ; + abort:mutex_unlock (&port->logbuffer_lock); }
@@ -746,7 +750,8 @@ static int tcpm_debug_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 (!IS_ENABLED (CONFIG_TCPM_LOG_WRAPAROUND ) && !seq_has_overflowed (s)) port->logbuffer_tail = tail;mutex_unlock (&port->logbuffer_lock);
Signature · 1 line
2.40.0.577.gac1e443424-goog
Greg KH
maintainerQuoted 15 lines · Badhri Jagan Sridharan
This change adds
CONFIG_TCPM_LOG_WRAPAROUNDwhich when set allows the logs to be wrapped around. Additionally, when set, does not clear the TCPM logs when dumped.Signed-off-by Badhri Jagan Sridharan <[email protected]>--- drivers/usb/typec/tcpm/Kconfig | 6 ++++++ drivers/usb/typec/tcpm/tcpm.c | 9 +++++++-- 2 files changed, 13 insertions(+), 2 deletions(-)drivers/usb/typec/tcpm/Kconfig +0
-0
drivers/usb/typec/tcpm/Kconfig config TYPEC_TCPCI · line 25
Quoted 8 lines
drivers/usb/typec/tcpm/Kconfig +5
-0@@ -18,6 +18,12 @@ config TYPEC_TCPCI help Type-C Port Controller driver for TCPCI-compliant controller. +config TCPM_LOG_WRAPAROUND + bool "Enable TCPM log wraparound" + help + When set, wraps around TCPM logs and does not clear the logs when dumped. TCPM logs by + default gets cleared when dumped and does not wraparound when full.
Kconfig help text needs to be wrapped at the properly width.
And you do not provide any hint here as to why this is not the default option, or why someone would want this.
So, why is this not just the default operation anyway? Why would you ever want the logs cleared?
thanks,
greg k-h
Badhri Jagan Sridharan
maintainerQuoted 20 lines · Greg KH
Quoted 18 lines · Badhri Jagan Sridharan
This change adds
CONFIG_TCPM_LOG_WRAPAROUNDwhich when set allows the logs to be wrapped around. Additionally, when set, does not clear the TCPM logs when dumped.Signed-off-by Badhri Jagan Sridharan <[email protected]>--- drivers/usb/typec/tcpm/Kconfig | 6 ++++++ drivers/usb/typec/tcpm/tcpm.c | 9 +++++++-- 2 files changed, 13 insertions(+), 2 deletions(-)drivers/usb/typec/tcpm/Kconfig +0
-0@@ -18,6 +18,12 @@ config TYPEC_TCPCI help Type-C Port Controller driver for TCPCI-compliant controller.
Quoted 7 lines
drivers/usb/typec/tcpm/Kconfig config TYPEC_TCPCI · line 25
Quoted 5 lines
drivers/usb/typec/tcpm/Kconfig +5
-0+config TCPM_LOG_WRAPAROUND + bool "Enable TCPM log wraparound" + help + When set, wraps around TCPM logs and does not clear the logs when dumped. TCPM logs by + default gets cleared when dumped and does not wraparound when full.
Kconfighelp text needs to be wrapped at the properly width.
I assumed that the width is 100 characters, but it looks like it is
80. Will fix it in the next version.
Quoted 3 lines
And you do not provide any hint here as to why this is not the default option, or why someone would want this.
"TCPM logs by default gets cleared when dumped and does not wraparound when full." was intended
to convey why someone would want to set this. Perhaps it's not effective.
Does the below look better:
"TCPM logs by default gets cleared when dumped and does not wraparound when full. This can be overridden by setting this config. When the config is set, TCPM wraps around logs and does not clear the logs when dumped."
Also, I could make this default if that's OK with Guenter.
Quoted 3 lines
So, why is this not just the default operation anyway? Why would you ever want the logs cleared?
I remember Guenter mentioning that he was finding it useful to not wrap around the logs to fix problems
during tcpm_register_port (init sequence). IMHO wrapping around the logs helps to triage interoperability
issues uncovered during testing. So both approaches have their own advantages.
Thanks for taking the time to review !
- Badhri
Quoted 4 lines
thanks,
greg k-h
Greg KH
maintainerQuoted 48 lines · Badhri Jagan Sridharan
Quoted 20 lines · Greg KH
Quoted 18 lines · Badhri Jagan Sridharan
This change adds
CONFIG_TCPM_LOG_WRAPAROUNDwhich when set allows the logs to be wrapped around. Additionally, when set, does not clear the TCPM logs when dumped.Signed-off-by Badhri Jagan Sridharan <[email protected]>--- drivers/usb/typec/tcpm/Kconfig | 6 ++++++ drivers/usb/typec/tcpm/tcpm.c | 9 +++++++-- 2 files changed, 13 insertions(+), 2 deletions(-)drivers/usb/typec/tcpm/Kconfig +0
-0@@ -18,6 +18,12 @@ config TYPEC_TCPCI help Type-C Port Controller driver for TCPCI-compliant controller.Quoted 7 lines
drivers/usb/typec/tcpm/Kconfig config TYPEC_TCPCI · line 25
Quoted 5 lines
drivers/usb/typec/tcpm/Kconfig +5
-0+config TCPM_LOG_WRAPAROUND + bool "Enable TCPM log wraparound" + help + When set, wraps around TCPM logs and does not clear the logs when dumped. TCPM logs by + default gets cleared when dumped and does not wraparound when full.
Kconfighelp text needs to be wrapped at the properly width.I assumed that the width is 100 characters, but it looks like it is
80. Will fix it in the next version.Quoted 3 lines
And you do not provide any hint here as to why this is not the default option, or why someone would want this.
"TCPM logs by default gets cleared when dumped and does not wraparound when full." was intended
to convey why someone would want to set this. Perhaps it's not effective.Does the below look better:
"TCPM logs by default gets cleared when dumped and does not wraparound when full. This can be overridden by setting this config. When the config is set, TCPM wraps around logs and does not clear the logs when dumped."Also, I could make this default if that's OK with Guenter.
Quoted 1 line
Quoted 8 lines
Quoted 2 lines
So, why is this not just the default operation anyway? Why would you ever want the logs cleared?
I remember Guenter mentioning that he was finding it useful to not wrap around the logs to fix problems
duringtcpm_register_port(init sequence). IMHO wrapping around the logs helps to triage interoperability
issues uncovered during testing. So both approaches have their own advantages.
But as this is a build-time option, what would cause someone to choose one over the other, and then when the system is running, they can't change them?
That does not seem good at all.
Why not just use tracing instead of this odd custom log buffer? That's a better solution overall, right?
thanks,
greg k-h
Badhri Jagan Sridharan
maintainerQuoted 59 lines · Greg KH
Quoted 48 lines · Badhri Jagan Sridharan
Quoted 20 lines · Greg KH
Quoted 18 lines · Badhri Jagan Sridharan
This change adds
CONFIG_TCPM_LOG_WRAPAROUNDwhich when set allows the logs to be wrapped around. Additionally, when set, does not clear the TCPM logs when dumped.Signed-off-by Badhri Jagan Sridharan <[email protected]>--- drivers/usb/typec/tcpm/Kconfig | 6 ++++++ drivers/usb/typec/tcpm/tcpm.c | 9 +++++++-- 2 files changed, 13 insertions(+), 2 deletions(-)drivers/usb/typec/tcpm/Kconfig +0
-0@@ -18,6 +18,12 @@ config TYPEC_TCPCI help Type-C Port Controller driver for TCPCI-compliant controller.Quoted 7 lines
drivers/usb/typec/tcpm/Kconfig config TYPEC_TCPCI · line 25
Quoted 5 lines
drivers/usb/typec/tcpm/Kconfig +5
-0+config TCPM_LOG_WRAPAROUND + bool "Enable TCPM log wraparound" + help + When set, wraps around TCPM logs and does not clear the logs when dumped. TCPM logs by + default gets cleared when dumped and does not wraparound when full.
Kconfighelp text needs to be wrapped at the properly width.I assumed that the width is 100 characters, but it looks like it is
80. Will fix it in the next version.Quoted 3 lines
And you do not provide any hint here as to why this is not the default option, or why someone would want this.
"TCPM logs by default gets cleared when dumped and does not wraparound when full." was intended
to convey why someone would want to set this. Perhaps it's not effective.Does the below look better:
"TCPM logs by default gets cleared when dumped and does not wraparound when full. This can be overridden by setting this config. When the config is set, TCPM wraps around logs and does not clear the logs when dumped."Also, I could make this default if that's OK with Guenter.
Quoted 1 line
Quoted 8 lines
Quoted 2 lines
So, why is this not just the default operation anyway? Why would you ever want the logs cleared?
I remember Guenter mentioning that he was finding it useful to not wrap around the logs to fix problems
duringtcpm_register_port(init sequence). IMHO wrapping around the logs helps to triage interoperability
issues uncovered during testing. So both approaches have their own advantages.
Quoted 3 lines
But as this is a build-time option, what would cause someone to choose one over the other, and then when the system is running, they can't change them?
During initial phases of bringup, it makes sense to not wrap around the logs, but, once bringup is done its most effective to wraparound so that interop issues reported by the end users can be triaged where TCPM logs are very effective.
Also, without wrapping around, the logbuffer logs are completely stale after the user goes through a few USB connect and disconnect cycles till the system is rebooted.
If we don't want to pursue the Kconfig option, we should atleast make TCPM default to wrapping the logs around when full so we could maximise the use of the logbuffer contents.
Quoted 5 lines
That does not seem good at all.
Why not just use tracing instead of this odd custom log buffer? That's a better solution overall, right?
Tracing is not enabled by default in most systems. End users don't want to retry and reproduce the failure to collect traces even if they could reproduce it consistently.
So tracing was not proving handy here.
Thanks,
Badhri
Quoted 4 lines
thanks,
greg k-h
Badhri Jagan Sridharan
maintainerQuoted 65 lines · Badhri Jagan Sridharan
Quoted 59 lines · Greg KH
Quoted 48 lines · Badhri Jagan Sridharan
Quoted 20 lines · Greg KH
Quoted 18 lines · Badhri Jagan Sridharan
This change adds
CONFIG_TCPM_LOG_WRAPAROUNDwhich when set allows the logs to be wrapped around. Additionally, when set, does not clear the TCPM logs when dumped.Signed-off-by Badhri Jagan Sridharan <[email protected]>--- drivers/usb/typec/tcpm/Kconfig | 6 ++++++ drivers/usb/typec/tcpm/tcpm.c | 9 +++++++-- 2 files changed, 13 insertions(+), 2 deletions(-)drivers/usb/typec/tcpm/Kconfig +0
-0@@ -18,6 +18,12 @@ config TYPEC_TCPCI help Type-C Port Controller driver for TCPCI-compliant controller.Quoted 7 lines
drivers/usb/typec/tcpm/Kconfig config TYPEC_TCPCI · line 25
Quoted 5 lines
drivers/usb/typec/tcpm/Kconfig +5
-0+config TCPM_LOG_WRAPAROUND + bool "Enable TCPM log wraparound" + help + When set, wraps around TCPM logs and does not clear the logs when dumped. TCPM logs by + default gets cleared when dumped and does not wraparound when full.
Kconfighelp text needs to be wrapped at the properly width.I assumed that the width is 100 characters, but it looks like it is
80. Will fix it in the next version.Quoted 3 lines
And you do not provide any hint here as to why this is not the default option, or why someone would want this.
"TCPM logs by default gets cleared when dumped and does not wraparound when full." was intended
to convey why someone would want to set this. Perhaps it's not effective.Does the below look better:
"TCPM logs by default gets cleared when dumped and does not wraparound when full. This can be overridden by setting this config. When the config is set, TCPM wraps around logs and does not clear the logs when dumped."Also, I could make this default if that's OK with Guenter.
Quoted 1 line
Quoted 8 lines
Quoted 2 lines
So, why is this not just the default operation anyway? Why would you ever want the logs cleared?
I remember Guenter mentioning that he was finding it useful to not wrap around the logs to fix problems
duringtcpm_register_port(init sequence). IMHO wrapping around the logs helps to triage interoperability
issues uncovered during testing. So both approaches have their own advantages.Quoted 3 lines
But as this is a build-time option, what would cause someone to choose one over the other, and then when the system is running, they can't change them?
Quoted 11 lines
During initial phases of bringup, it makes sense to not wrap around the logs, but, once bringup is done its most effective to wraparound so that interop issues reported by the end users can be triaged where TCPM logs are very effective.
Also, without wrapping around, the logbuffer logs are completely stale after the user goes through a few USB connect and disconnect cycles till the system is rebooted.
If we don't want to pursue theKconfigoption, we should atleast make TCPM default to wrapping the logs around when full so we could maximise the use of the logbuffer contents.
The other option that I could think of is to expose another debugfs node that can be used to control wrapping around the logs in runtime.
Thanks,
Badhri
Quoted 18 lines
Quoted 5 lines
That does not seem good at all.
Why not just use tracing instead of this odd custom log buffer? That's a better solution overall, right?
Tracing is not enabled by default in most systems. End users don't want to retry and reproduce the failure to collect traces even if they could reproduce it consistently.
So tracing was not proving handy here.Thanks,
BadhriQuoted 4 lines
thanks,
greg k-h
Quoted 60 lines · Badhri Jagan Sridharan
Quoted 59 lines · Greg KH
Quoted 56 lines · Badhri Jagan Sridharan
Quoted 20 lines · Greg KH
Quoted 18 lines · Badhri Jagan Sridharan
This change adds
CONFIG_TCPM_LOG_WRAPAROUNDwhich when set allows the logs to be wrapped around. Additionally, when set, does not clear the TCPM logs when dumped.Signed-off-by Badhri Jagan Sridharan <[email protected]>--- drivers/usb/typec/tcpm/Kconfig | 6 ++++++ drivers/usb/typec/tcpm/tcpm.c | 9 +++++++-- 2 files changed, 13 insertions(+), 2 deletions(-)drivers/usb/typec/tcpm/Kconfig +0
-0@@ -18,6 +18,12 @@ config TYPEC_TCPCI help Type-C Port Controller driver for TCPCI-compliant controller.Quoted 7 lines
drivers/usb/typec/tcpm/Kconfig config TYPEC_TCPCI · line 25
Quoted 5 lines
drivers/usb/typec/tcpm/Kconfig +5
-0+config TCPM_LOG_WRAPAROUND + bool "Enable TCPM log wraparound" + help + When set, wraps around TCPM logs and does not clear the logs when dumped. TCPM logs by + default gets cleared when dumped and does not wraparound when full.
Kconfighelp text needs to be wrapped at the properly width.I assumed that the width is 100 characters, but it looks like it is
80. Will fix it in the next version.Quoted 3 lines
And you do not provide any hint here as to why this is not the default option, or why someone would want this.
"TCPM logs by default gets cleared when dumped and does not wraparound when full." was intended
to convey why someone would want to set this. Perhaps it's not effective.Does the below look better:
"TCPM logs by default gets cleared when dumped and does not wraparound when full. This can be overridden by setting this config. When the config is set, TCPM wraps around logs and does not clear the logs when dumped."Also, I could make this default if that's OK with Guenter.
Quoted 3 lines
So, why is this not just the default operation anyway? Why would you ever want the logs cleared?
I remember Guenter mentioning that he was finding it useful to not wrap around the logs to fix problems
duringtcpm_register_port(init sequence). IMHO wrapping around the logs helps to triage interoperability
issues uncovered during testing. So both approaches have their own advantages.
Quoted 8 lines
Quoted 3 lines
But as this is a build-time option, what would cause someone to choose one over the other, and then when the system is running, they can't change them?
During initial phases of bringup, it makes sense to not wrap around the logs, but, once bringup is done its most effective to wraparound so that interop issues reported by the end users can be triaged where TCPM logs are very effective.
Not really, because the problem tends to be the remote device (or at least it used to be), not a driver under development.
Quoted 3 lines
Also, without wrapping around, the logbuffer logs are completely stale after the user goes through a few USB connect and disconnect cycles till the system is rebooted.
Unless they are cleared.
Again, the premise is wrong here. The idea was to ensure that the beginning of a problem is caught and available in the log, not its tail. This includes "beginning" as the behavior immediately after boot regarding an already connected device, and the behavior observed when inserting a device into the running system. Again, in both cases it was most important to catch the beginning of a problem, not its tail.
Quoted 4 lines
If we don't want to pursue the
Kconfigoption, we should atleast make TCPM default to wrapping the logs around when full so we could maximise the use of the logbuffer contents.
I don't really agree, but then I am not in a position to argue either. Maybe the premise and reasons have changed since I wrote the driver.
Guenter
Quoted 18 lines
Quoted 5 lines
That does not seem good at all.
Why not just use tracing instead of this odd custom log buffer? That's a better solution overall, right?
Tracing is not enabled by default in most systems. End users don't want to retry and reproduce the failure to collect traces even if they could reproduce it consistently.
So tracing was not proving handy here.Thanks,
BadhriQuoted 4 lines
thanks,
greg k-h
Badhri Jagan Sridharan
maintainerQuoted 66 lines · Guenter Roeck
Quoted 64 lines · Badhri Jagan Sridharan
Quoted 62 lines · Greg KH
Quoted 48 lines · Badhri Jagan Sridharan
Quoted 20 lines · Greg KH
Quoted 18 lines · Badhri Jagan Sridharan
This change adds
CONFIG_TCPM_LOG_WRAPAROUNDwhich when set allows the logs to be wrapped around. Additionally, when set, does not clear the TCPM logs when dumped.Signed-off-by Badhri Jagan Sridharan <[email protected]>--- drivers/usb/typec/tcpm/Kconfig | 6 ++++++ drivers/usb/typec/tcpm/tcpm.c | 9 +++++++-- 2 files changed, 13 insertions(+), 2 deletions(-)drivers/usb/typec/tcpm/Kconfig +0
-0@@ -18,6 +18,12 @@ config TYPEC_TCPCI help Type-C Port Controller driver for TCPCI-compliant controller.Quoted 7 lines
drivers/usb/typec/tcpm/Kconfig config TYPEC_TCPCI · line 25
Quoted 5 lines
drivers/usb/typec/tcpm/Kconfig +5
-0+config TCPM_LOG_WRAPAROUND + bool "Enable TCPM log wraparound" + help + When set, wraps around TCPM logs and does not clear the logs when dumped. TCPM logs by + default gets cleared when dumped and does not wraparound when full.
Kconfighelp text needs to be wrapped at the properly width.I assumed that the width is 100 characters, but it looks like it is
80. Will fix it in the next version.Quoted 3 lines
And you do not provide any hint here as to why this is not the default option, or why someone would want this.
"TCPM logs by default gets cleared when dumped and does not wraparound when full." was intended
to convey why someone would want to set this. Perhaps it's not effective.Does the below look better:
"TCPM logs by default gets cleared when dumped and does not wraparound when full. This can be overridden by setting this config. When the config is set, TCPM wraps around logs and does not clear the logs when dumped."Also, I could make this default if that's OK with Guenter.
Quoted 1 line
Quoted 8 lines
Quoted 2 lines
So, why is this not just the default operation anyway? Why would you ever want the logs cleared?
I remember Guenter mentioning that he was finding it useful to not wrap around the logs to fix problems
duringtcpm_register_port(init sequence). IMHO wrapping around the logs helps to triage interoperability
issues uncovered during testing. So both approaches have their own advantages.But as this is a build-time option, what would cause someone to choose one over the other, and then when the system is running, they can't change them?
Quoted 7 lines
Quoted 4 lines
During initial phases of bringup, it makes sense to not wrap around the logs, but, once bringup is done its most effective to wraparound so that interop issues reported by the end users can be triaged where TCPM logs are very effective.
Not really, because the problem tends to be the remote device (or at least it used to be), not a driver under development.
Thanks for clarifying Guenter !
Right now if we DONT wrap around, once an issue is observed with a remote device, the logs have to be cleared(if already full) and then the issue has to be reproduced to collect the TCPM logbuffer logsagain.
Having a log available _all_ the time, not just when explicitly enabled is still very useful to catch hard to reproduce intertop issues.
Given this would you be OK if I change the logic to wrap around always ?
IMHO based on what I have seen in the last couple of years, this would also cover the boot with accessory connected as if the link gets into a reset loop, the sequence after the reset reveals what had gone wrong.
Quoted 6 lines
Quoted 4 lines
Also, without wrapping around, the logbuffer logs are completely stale after the user goes through a few USB connect and disconnect cycles till the system is rebooted.
Unless they are cleared.
Ah yes. I forgot about that. Wrapping around would still make TCPM logbuffer logs more effective to debug issues with remote device.
Thanks,
Badhri
Quoted 36 lines
Again, the premise is wrong here. The idea was to ensure that the beginning of a problem is caught and available in the log, not its tail. This includes "beginning" as the behavior immediately after boot regarding an already connected device, and the behavior observed when inserting a device into the running system. Again, in both cases it was most important to catch the beginning of a problem, not its tail.
Quoted 4 lines
If we don't want to pursue the
Kconfigoption, we should atleast make TCPM default to wrapping the logs around when full so we could maximise the use of the logbuffer contents.I don't really agree, but then I am not in a position to argue either. Maybe the premise and reasons have changed since I wrote the driver.
Guenter
Quoted 18 lines
Quoted 5 lines
That does not seem good at all.
Why not just use tracing instead of this odd custom log buffer? That's a better solution overall, right?
Tracing is not enabled by default in most systems. End users don't want to retry and reproduce the failure to collect traces even if they could reproduce it consistently.
So tracing was not proving handy here.Thanks,
BadhriQuoted 4 lines
thanks,
greg k-h
Quoted 85 lines · Badhri Jagan Sridharan
Quoted 66 lines · Guenter Roeck
Quoted 64 lines · Badhri Jagan Sridharan
Quoted 62 lines · Greg KH
Quoted 48 lines · Badhri Jagan Sridharan
Quoted 20 lines · Greg KH
Quoted 18 lines · Badhri Jagan Sridharan
This change adds
CONFIG_TCPM_LOG_WRAPAROUNDwhich when set allows the logs to be wrapped around. Additionally, when set, does not clear the TCPM logs when dumped.Signed-off-by Badhri Jagan Sridharan <[email protected]>--- drivers/usb/typec/tcpm/Kconfig | 6 ++++++ drivers/usb/typec/tcpm/tcpm.c | 9 +++++++-- 2 files changed, 13 insertions(+), 2 deletions(-)drivers/usb/typec/tcpm/Kconfig +0
-0@@ -18,6 +18,12 @@ config TYPEC_TCPCI help Type-C Port Controller driver for TCPCI-compliant controller.Quoted 7 lines
drivers/usb/typec/tcpm/Kconfig config TYPEC_TCPCI · line 25
Quoted 5 lines
drivers/usb/typec/tcpm/Kconfig +5
-0+config TCPM_LOG_WRAPAROUND + bool "Enable TCPM log wraparound" + help + When set, wraps around TCPM logs and does not clear the logs when dumped. TCPM logs by + default gets cleared when dumped and does not wraparound when full.
Kconfighelp text needs to be wrapped at the properly width.I assumed that the width is 100 characters, but it looks like it is
80. Will fix it in the next version.Quoted 3 lines
And you do not provide any hint here as to why this is not the default option, or why someone would want this.
"TCPM logs by default gets cleared when dumped and does not wraparound when full." was intended
to convey why someone would want to set this. Perhaps it's not effective.Does the below look better:
"TCPM logs by default gets cleared when dumped and does not wraparound when full. This can be overridden by setting this config. When the config is set, TCPM wraps around logs and does not clear the logs when dumped."Also, I could make this default if that's OK with Guenter.
Quoted 1 line
Quoted 8 lines
Quoted 2 lines
So, why is this not just the default operation anyway? Why would you ever want the logs cleared?
I remember Guenter mentioning that he was finding it useful to not wrap around the logs to fix problems
duringtcpm_register_port(init sequence). IMHO wrapping around the logs helps to triage interoperability
issues uncovered during testing. So both approaches have their own advantages.But as this is a build-time option, what would cause someone to choose one over the other, and then when the system is running, they can't change them?
Quoted 7 lines
Quoted 4 lines
During initial phases of bringup, it makes sense to not wrap around the logs, but, once bringup is done its most effective to wraparound so that interop issues reported by the end users can be triaged where TCPM logs are very effective.
Not really, because the problem tends to be the remote device (or at least it used to be), not a driver under development.
Thanks for clarifying Guenter !
Right now if we DONT wrap around, once an issue is observed with a remote device, the logs have to be cleared(if already full) and then the issue has to be reproduced to collect the TCPM logbuffer logsagain.Having a log available _all_ the time, not just when explicitly enabled is still very useful to catch hard to reproduce intertop issues.
Quoted 7 lines
Given this would you be OK if I change the logic to wrap around always ?
IMHO based on what I have seen in the last couple of years, this would also cover the boot with accessory connected as if the link gets into a reset loop, the sequence after the reset reveals what had gone wrong.
I have no strong opinion. As I said, maybe conditions have changed. When I wrote the code, failure conditions happened all the time, and reset loops overwrote the log repeatedly. I didn't implement the current code because I was lazy or something, it was because wrap-around did not work for me. Again, maybe the situation has changed. And, to follow-up on Greg's question, maybe switching to tracing instead of re-implementing it would make sense nowadays. I simply don't know. I'll accept whatever solution you and Greg can agree on.
Thanks,
Guenter
Quoted 39 lines · Badhri Jagan Sridharan
Quoted 20 lines · Greg KH
Quoted 18 lines · Badhri Jagan Sridharan
This change adds
CONFIG_TCPM_LOG_WRAPAROUNDwhich when set allows the logs to be wrapped around. Additionally, when set, does not clear the TCPM logs when dumped.Signed-off-by Badhri Jagan Sridharan <[email protected]>--- drivers/usb/typec/tcpm/Kconfig | 6 ++++++ drivers/usb/typec/tcpm/tcpm.c | 9 +++++++-- 2 files changed, 13 insertions(+), 2 deletions(-)drivers/usb/typec/tcpm/Kconfig +0
-0@@ -18,6 +18,12 @@ config TYPEC_TCPCI help Type-C Port Controller driver for TCPCI-compliant controller.Quoted 7 lines
drivers/usb/typec/tcpm/Kconfig config TYPEC_TCPCI · line 25
Quoted 5 lines
drivers/usb/typec/tcpm/Kconfig +5
-0+config TCPM_LOG_WRAPAROUND + bool "Enable TCPM log wraparound" + help + When set, wraps around TCPM logs and does not clear the logs when dumped. TCPM logs by + default gets cleared when dumped and does not wraparound when full.
Kconfighelp text needs to be wrapped at the properly width.I assumed that the width is 100 characters, but it looks like it is
80. Will fix it in the next version.Quoted 3 lines
And you do not provide any hint here as to why this is not the default option, or why someone would want this.
"TCPM logs by default gets cleared when dumped and does not wraparound when full." was intended
to convey why someone would want to set this. Perhaps it's not effective.
Quoted 8 lines
Does the below look better:
"TCPM logs by default gets cleared when dumped and does not wraparound when full. This can be overridden by setting this config. When the config is set, TCPM wraps around logs and does not clear the logs when dumped."Also, I could make this default if that's OK with Guenter.
The reason to not wrap around the buffer was that it tends to fill up quickly if something goes wrong. Initially I had it wrap around and often ended up seeing endless reset sequences in the log but not the actual problem.
Quoted 10 lines
Quoted 3 lines
So, why is this not just the default operation anyway? Why would you ever want the logs cleared?
I remember Guenter mentioning that he was finding it useful to not wrap around the logs to fix problems
duringtcpm_register_port(init sequence). IMHO wrapping around the logs helps to triage interoperability
issues uncovered during testing. So both approaches have their own advantages.
Yes, except that "testing" at the time included connecting an arbitrary new device to the type c port. Pretty much each of them had its own flaws. This was also the reason for not using tracing, because I wanted to have a log available _all_ the time, not just when explicitly enabled.
Maybe the protocol is now more stable than it used to be, and switching to a wraparound buffer and/or to tracing makes more sense now. I can't really say.
Guenter
Quoted 7 lines
Thanks for taking the time to review !
- BadhriQuoted 4 lines
thanks,
greg k-h