usb: typec: ucsi: Update UCSI alternate mode
v5 cover usb: typec: ucsi: Update UCSI alternate mode
Hi Heikki,
This series appliess some changes to the UCSI driver to help support AP driven alternate mode entry. This includes...
1. An update to the altmode sysfs group after registration to make "active" writable.
2. A change to the ucsi_partner_task delay when queuing ucsi_check_altmodes to prevent it from running before other discovery functions.
3. An update to always define a number of alternate modes for partners and plugs.
Not related to AP driven altmode entry, there is an additional fix for a null derefrence in this series.
I tested the series on a ChromeOS v6.8 kernel merged with usb-testing. That build had some additinal patches to enable a PPM in ChromeOS. Let me know if you have any questions.
Thanks,
Jameson
Changes in V5:
- Fixed signed off by line order.
Changes in V4:
- Updates to the commit messages.
Changes in V3:
- Returns typec_port_register_altmode call from ucsi_register_displayport when CONFIG_TYPEC_DP_ALTMODE is not enabled.
Changes in V2:
- Checks for error response from ucsi_register_displayport when registering DisplayPort alternate mode.
Abhishek Pandit-Subedi (2):
usb: typec: ucsi: Fix null pointer dereference in trace
usb: typec: Update sysfs when setting ops
Jameson Thies (2):
usb: typec: ucsi: Delay alternate mode discovery
usb: typec: ucsi: Always set number of alternate modes
drivers/usb/typec/altmodes/displayport.c | 2 +- drivers/usb/typec/class.c | 18 +++++++++++++++++- drivers/usb/typec/ucsi/displayport.c | 2 +- drivers/usb/typec/ucsi/ucsi.c | 18 +++++++++++++----- drivers/usb/typec/ucsi/ucsi.h | 2 +- include/linux/usb/typec.h | 3 +++ 6 files changed, 36 insertions(+), 9 deletions(-)
base-commit: e4306116b5e93748b3eaa7666aa55c390b48a8f4
Signature · 1 line
2.45.0.118.g7fe29c98d7-goog
PATCH v5 1/4 usb: typec: ucsi: Fix null pointer dereference in trace
From: Abhishek Pandit-Subedi <[email protected]>
ucsi_register_altmode checks IS_ERR for the alt pointer and treats NULL as valid. When CONFIG_TYPEC_DP_ALTMODE is not enabled, ucsi_register_displayport returns NULL which causes a NULL pointer dereference in trace. Rather than return NULL, call typec_port_register_altmode to register DisplayPort alternate mode as a non-controllable mode when CONFIG_TYPEC_DP_ALTMODE is not enabled.
---
Changes in V5:
- Fixed signed off by line order.
Changes in V4:
- Updated commit message.
Changes in V3:
- Returns typec_port_register_altmode call from ucsi_register_displayport when CONFIG_TYPEC_DP_ALTMODE is not enabled. Updated commit message.
Changes in V2:
- Checks for error response from ucsi_register_displayport when registering DisplayPort alternate mode.
drivers/usb/typec/ucsi/ucsi.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/typec/ucsi/ucsi.h +1 -1
@@ -496,7 +496,7 @@ ucsi_register_displayport(struct ucsi_connector *con, bool override, int offset, struct typec_altmode_desc *desc) { - return NULL; + return typec_port_register_altmode(con->port, desc); } static inline void
Signature · 1 line
2.45.0.118.g7fe29c98d7-goog
Re: [PATCH v5 1/4] usb: typec: ucsi: Fix null pointer dereference in trace
Quoted 9 lines · Jameson Thies
From: Abhishek Pandit-Subedi <[email protected]>
ucsi_register_altmodechecksIS_ERRfor the alt pointer and treats NULL as valid. WhenCONFIG_TYPEC_DP_ALTMODEis not enabled,ucsi_register_displayportreturns NULL which causes a NULL pointer dereference in trace. Rather than return NULL, calltypec_port_register_altmodeto register DisplayPort alternate mode as a non-controllable mode whenCONFIG_TYPEC_DP_ALTMODEis not enabled.
Quoted 4 lines
Reviewed-by Benson Leung <[email protected]>Reviewed-by Heikki Krogerus <[email protected]>Signed-off-by Abhishek Pandit-Subedi <[email protected]>Signed-off-by Jameson Thies <[email protected]>
Signature · 2 lines
With best wishes Dmitry
PATCH v5 2/4 usb: typec: Update sysfs when setting ops
In reply to Jameson ThiesFrom: Abhishek Pandit-Subedi <[email protected]>
When adding altmode ops, update the sysfs group so that visibility is also recalculated.
---
Changes in V5:
- Fixed signed off by line order.
Changes in V4:
- None.
Changes in V3:
- None.
Changes in V2:
- None.
drivers/usb/typec/altmodes/displayport.c | 2 +- drivers/usb/typec/class.c | 18 +++++++++++++++++- drivers/usb/typec/ucsi/displayport.c | 2 +- include/linux/usb/typec.h | 3 +++ 4 files changed, 22 insertions(+), 3 deletions(-)
drivers/usb/typec/altmodes/displayport.c +1 -1
@@ -746,7 +746,7 @@ int dp_altmode_probe(struct typec_altmode *alt) dp->alt = alt; alt->desc ="DisplayPort" ; - alt->ops = &dp_altmode_ops; +typec_altmode_set_ops (alt, &dp_altmode_ops);if (plug) { plug->desc ="Displayport" ;
drivers/usb/typec/class.c +17 -1
@@ -467,6 +467,22 @@ static const struct attribute_group *typec_altmode_groups[] = { NULL }; +/** +* typec_altmode_set_ops - Set ops for altmode +* @adev: Handle to the alternate mode +* @ops: Ops for the alternate mode +* +* After setting ops, attribute visiblity needs to be refreshed if the alternate +* mode can be activated. +*/ +void typec_altmode_set_ops (struct typec_altmode *adev, +const struct typec_altmode_ops *ops) +{ + adev->ops = ops; +sysfs_update_group (&adev->dev.kobj, &typec_altmode_group); +} +EXPORT_SYMBOL_GPL (typec_altmode_set_ops); +static int altmode_id_get (struct device *dev) {struct ida *ids;
@@ -2317,7 +2333,7 @@ void typec_port_register_altmodes(struct typec_port *port,continue ; } - alt->ops = ops; +typec_altmode_set_ops (alt, ops);typec_altmode_set_drvdata (alt, drvdata); altmodes[index] = alt; index++;
drivers/usb/typec/ucsi/displayport.c +1 -1
@@ -337,7 +337,7 @@ struct typec_altmode *ucsi_register_displayport(struct ucsi_connector *con, dp->con = con; dp->alt = alt; - alt->ops = &ucsi_displayport_ops; +typec_altmode_set_ops (alt, &ucsi_displayport_ops);typec_altmode_set_drvdata (alt, dp);return alt;
include/linux/usb/typec.h +3 -0
@@ -167,6 +167,9 @@ struct typec_port *typec_altmode2port(struct typec_altmode *alt);void typec_altmode_update_active (struct typec_altmode *alt,bool active); +void typec_altmode_set_ops (struct typec_altmode *alt, +const struct typec_altmode_ops *ops); +enum typec_plug_index {TYPEC_PLUG_SOP_P ,TYPEC_PLUG_SOP_PP ,
Signature · 1 line
2.45.0.118.g7fe29c98d7-goog
Re: [PATCH v5 2/4] usb: typec: Update sysfs when setting ops
Quoted 9 lines · Jameson Thies
From: Abhishek Pandit-Subedi <[email protected]>
When adding altmode ops, update the sysfs group so that visibility is also recalculated.
Reviewed-by Heikki Krogerus <[email protected]>Reviewed-by Benson Leung <[email protected]>Signed-off-by Abhishek Pandit-Subedi <[email protected]>Signed-off-by Jameson Thies <[email protected]>
Signature · 2 lines
With best wishes Dmitry
PATCH v5 3/4 usb: typec: ucsi: Delay alternate mode discovery
In reply to Jameson ThiesDelay the ucsi_check_altmodes task to be inline with surrounding partner tasks. This allows partner, cable and identity discovery to complete before alternate mode registration. With that order, alternate mode discovery can be used to indicate the ucsi driver has completed discovery.
---
Changes in V5:
- None.
Changes in V4:
- None.
Changes in V3:
- None.
Changes in V2:
- None.
drivers/usb/typec/ucsi/ucsi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/usb/typec/ucsi/ucsi.c +2 -2
@@ -963,7 +963,7 @@ static void ucsi_pwr_opmode_change(struct ucsi_connector *con) con->rdo = con->status.request_data_obj;typec_set_pwr_opmode (con->port,TYPEC_PWR_MODE_PD );ucsi_partner_task (con, ucsi_get_src_pdos,30 ,0 ); -ucsi_partner_task (con, ucsi_check_altmodes,30 ,0 ); +ucsi_partner_task (con, ucsi_check_altmodes,30 ,HZ );ucsi_partner_task (con, ucsi_register_partner_pdos,1 ,HZ );break ;case UCSI_CONSTAT_PWR_OPMODE_TYPEC1_5 :
@@ -1247,7 +1247,7 @@ static void ucsi_handle_connector_change(struct work_struct *work) }if (con->status.change &UCSI_CONSTAT_CAM_CHANGE ) -ucsi_partner_task (con, ucsi_check_altmodes,1 ,0 ); +ucsi_partner_task (con, ucsi_check_altmodes,1 ,HZ ); out_unlock:mutex_unlock (&con->lock);
Signature · 1 line
2.45.0.118.g7fe29c98d7-goog
Re: [PATCH v5 3/4] usb: typec: ucsi: Delay alternate mode discovery
Quoted 10 lines · Jameson Thies
Delay the
ucsi_check_altmodestask to be inline with surrounding partner tasks. This allows partner, cable and identity discovery to complete before alternate mode registration. With that order, alternate mode discovery can be used to indicate the ucsi driver has completed discovery.Reviewed-by Heikki Krogerus <[email protected]>Reviewed-by Benson Leung <[email protected]>Signed-off-by Jameson Thies <[email protected]>---
Signature · 2 lines
With best wishes Dmitry
PATCH v5 4/4 usb: typec: ucsi: Always set number of alternate modes
In reply to Jameson ThiesProviding the number of known alternate modes allows user space to determine when device registration has completed. Always register a number of known alternate modes for the partner and cable plug, even when the number of supported alternate modes is 0.
---
Changes in V5:
- None.
Changes in V4:
- None.
Changes in V3:
- None.
Changes in V2:
- None.
drivers/usb/typec/ucsi/ucsi.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
drivers/usb/typec/ucsi/ucsi.c +11 -3
@@ -812,10 +812,11 @@ static int ucsi_check_altmodes(struct ucsi_connector *con)/* Ignoring the errors in this case. */ if (con->partner_altmode[0 ]) { num_partner_am =ucsi_get_num_altmode (con->partner_altmode); -if (num_partner_am >0 ) -typec_partner_set_num_altmodes (con->partner, num_partner_am); +typec_partner_set_num_altmodes (con->partner, num_partner_am);ucsi_altmode_update_active (con);return 0 ; + }else { +typec_partner_set_num_altmodes (con->partner,0 ); }return ret;
@@ -1138,7 +1139,7 @@ static int ucsi_check_connection(struct ucsi_connector *con)static int ucsi_check_cable (struct ucsi_connector *con) {u64 command; -int ret; +int ret, num_plug_am;if (con->cable)return 0 ;
@@ -1172,6 +1173,13 @@ static int ucsi_check_cable(struct ucsi_connector *con)return ret; } +if (con->plug_altmode[0 ]) { + num_plug_am =ucsi_get_num_altmode (con->plug_altmode); +typec_plug_set_num_altmodes (con->plug, num_plug_am); + }else { +typec_plug_set_num_altmodes (con->plug,0 ); + } +return 0 ; }
Signature · 1 line
2.45.0.118.g7fe29c98d7-goog
Re: [PATCH v5 4/4] usb: typec: ucsi: Always set number of alternate modes
Quoted 9 lines · Jameson Thies
Providing the number of known alternate modes allows user space to determine when device registration has completed. Always register a number of known alternate modes for the partner and cable plug, even when the number of supported alternate modes is 0.
Reviewed-by Heikki Krogerus <[email protected]>Reviewed-by Benson Leung <[email protected]>Signed-off-by Jameson Thies <[email protected]>---
Signature · 2 lines
With best wishes Dmitry
Jon Hunter
maintainerRe: [PATCH v5 4/4] usb: typec: ucsi: Always set number of alternate modes
In reply to Jameson ThiesHi Jameson,
Quoted 55 lines · Jameson Thies
Providing the number of known alternate modes allows user space to determine when device registration has completed. Always register a number of known alternate modes for the partner and cable plug, even when the number of supported alternate modes is 0.
Reviewed-by Heikki Krogerus <[email protected]>Reviewed-by Benson Leung <[email protected]>Signed-off-by Jameson Thies <[email protected]>---
Changes in V5:
- None.Changes in V4:
- None.Changes in V3:
- None.Changes in V2:
- None.drivers/usb/typec/ucsi/ucsi.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)drivers/usb/typec/ucsi/ucsi.c +4
-3@@ -812,10 +812,11 @@ static int ucsi_check_altmodes(struct ucsi_connector *con)/* Ignoring the errors in this case. */ if (con->partner_altmode[0 ]) { num_partner_am =ucsi_get_num_altmode (con->partner_altmode); -if (num_partner_am >0 ) -typec_partner_set_num_altmodes (con->partner, num_partner_am); +typec_partner_set_num_altmodes (con->partner, num_partner_am);ucsi_altmode_update_active (con);return 0 ; + }else { +typec_partner_set_num_altmodes (con->partner,0 ); }return ret;@@ -1138,7 +1139,7 @@ static int ucsi_check_connection(struct ucsi_connector *con)static int ucsi_check_cable (struct ucsi_connector *con) {u64 command; -int ret; +int ret, num_plug_am;if (con->cable)return 0 ;@@ -1172,6 +1173,13 @@ static int ucsi_check_cable(struct ucsi_connector *con)return ret; }
drivers/usb/typec/ucsi/ucsi.c static int ucsi_check_cable(struct ucsi_connector *con) · line 1185
Quoted 10 lines
drivers/usb/typec/ucsi/ucsi.c +7
-0+if (con->plug_altmode[0 ]) { + num_plug_am =ucsi_get_num_altmode (con->plug_altmode); +typec_plug_set_num_altmodes (con->plug, num_plug_am); + }else { +typec_plug_set_num_altmodes (con->plug,0 ); + } +return 0 ; }
I am seeing the following panic on linux-next and bisect is pointing to this commit.
[ 16.411135] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000310 [ 16.411716] Mem abort info: [ 16.411806] ESR = 0x0000000096000044 [ 16.412147] EC = 0x25: DABT (current EL), IL = 32 bits [ 16.412465] SET = 0, FnV = 0 [ 16.412530] EA = 0, S1PTW = 0 [ 16.412778] FSC = 0x04: level 0 translation fault [ 16.413084] Data abort info: [ 16.413149] ISV = 0, ISS = 0x00000044, ISS2 = 0x00000000 [ 16.413534] CM = 0, WnR = 1, TnD = 0, TagAccess = 0 [ 16.414163] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [ 16.414649] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000101b2b000 [ 16.414784] [0000000000000310] pgd=0000000000000000, p4d=0000000000000000 [ 16.414940] Internal error: Oops: 0000000096000044 [#1] PREEMPT SMP [ 16.414946] Modules linked in: tegra210_adma(+) snd_soc_tegra210_ahub(+) drm backlight snd_soc_tegra_audio_graph_card snd_soc_audio_graph_card ucsi_ccg typec_ucsi crct10dif_ce snd_soc_simple_card_utils typec pwm_fan snd_soc_rt5659(+) snd_soc_rl6231 ina3221 pwm_tegra tegra_aconnect phy_tegra194_p2u snd_hda_codec_hdmi at24 snd_hda_tegra snd_hda_codec snd_hda_core lm90 tegra_xudc host1x pcie_tegra194 tegra_bpmp_thermal ip_tables x_tables ipv6 [ 16.415056] CPU: 0 PID: 290 Comm: kworker/0:4 Not tainted 6.10.0-rc4-next-20240617-g76db4c64526c #1 [ 16.415063] Hardware name: NVIDIA Jetson AGX Xavier Developer Kit (DT) [ 16.415067] Workqueue: events_long ucsi_init_work [typec_ucsi] [ 16.415082] pstate: 80400009 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 16.415091] pc : typec_plug_set_num_altmodes+0x18/0x6c [typec] [ 16.415112] lr : ucsi_check_cable.part.0+0x228/0x280 [typec_ucsi] [ 16.415120] sp : ffff8000848bbca0 [ 16.415123] x29: ffff8000848bbca0 x28: ffff000080e66000 x27: ffff80007adc96e8 [ 16.415140] x26: ffff80007adc96f8 x25: ffff0000834510c0 x24: ffff000080e66010 [ 16.415150] x23: ffff000080e6638c x22: ffff000080e664c0 x21: ffff000089c95800 [ 16.415160] x20: 0000000000000000 x19: 0000000000000000 x18: ffffffffffffffff [ 16.415170] x17: 1e00000001000000 x16: c203000000000000 x15: 00656c6261632d30 [ 16.415180] x14: ffff800082c922d8 x13: 0000000000000040 x12: 0000000000000228 [ 16.415191] x11: 0000000000000000 x10: 0000000000000000 x9 : 0000000000000000 [ 16.415201] x8 : 3d45505954564544 x7 : 0063657079743d4d x6 : 0000000000000000 [ 16.415212] x5 : ffff0000835ee780 x4 : ffff000080e66288 x3 : 0000000000000000 [ 16.415223] x2 : 0000000000000000 x1 : 0000000000000000 x0 : 0000000000000000 [ 16.415233] Call trace: [ 16.415236] typec_plug_set_num_altmodes+0x18/0x6c [typec] [ 16.415255] ucsi_check_cable.part.0+0x228/0x280 [typec_ucsi] [ 16.415264] ucsi_init_work+0x8b4/0x9b8 [typec_ucsi] [ 16.415271] process_one_work+0x150/0x294 [ 16.415282] worker_thread+0x2f4/0x3fc [ 16.415289] kthread+0x118/0x11c [ 16.415296] ret_from_fork+0x10/0x20 [ 16.415310] Code: a9be7bfd 910003fd f9000bf3 aa0003f3 (b9031001) [ 16.521018] ata1: SATA link down (SStatus 0 SControl 300) [ 16.524401] ---[ end trace 0000000000000000 ]---
I have not looked any further yet, but wanted to report this. If you have any thoughts let me know.
Thanks
Jon
Signature · 1 line
nvpublic
Jon Hunter
maintainerRe: [PATCH v5 4/4] usb: typec: ucsi: Always set number of alternate modes
Quoted 117 lines · Jon Hunter
Hi Jameson,
Quoted 55 lines · Jameson Thies
Providing the number of known alternate modes allows user space to determine when device registration has completed. Always register a number of known alternate modes for the partner and cable plug, even when the number of supported alternate modes is 0.
Reviewed-by Heikki Krogerus <[email protected]>Reviewed-by Benson Leung <[email protected]>Signed-off-by Jameson Thies <[email protected]>---
Changes in V5:
- None.Changes in V4:
- None.Changes in V3:
- None.Changes in V2:
- None.
drivers/usb/typec/ucsi/ucsi.c| 14 +++++++++++---1 file changed, 11 insertions(+), 3 deletions(-)drivers/usb/typec/ucsi/ucsi.c +3
-2@@ -812,10 +812,11 @@ static int ucsi_check_altmodes(struct ucsi_connector *con) /* Ignoring the errors in this case. */ if (con->partner_altmode[0]) { num_partner_am = ucsi_get_num_altmode(con->partner_altmode); - if (num_partner_am > 0) - typec_partner_set_num_altmodes(con->partner, num_partner_am); + typec_partner_set_num_altmodes(con->partner, num_partner_am); ucsi_altmode_update_active(con); return 0; + } else { + typec_partner_set_num_altmodes(con->partner, 0); }return ret;
drivers/usb/typec/ucsi/ucsi.c +1
-1@@ -1138,7 +1139,7 @@ static int ucsi_check_connection(struct ucsi_connector *con) static int ucsi_check_cable(struct ucsi_connector *con) { u64 command; - int ret; + int ret, num_plug_am; if (con->cable) return 0;@@ -1172,6 +1173,13 @@ static int ucsi_check_cable(struct ucsi_connector *con) return ret;drivers/usb/typec/ucsi/ucsi.c static int ucsi_check_cable(struct · line 1184
Quoted 10 lines
drivers/usb/typec/ucsi/ucsi.c +7
-0} + if (con->plug_altmode[0]) { + num_plug_am = ucsi_get_num_altmode(con->plug_altmode); + typec_plug_set_num_altmodes(con->plug, num_plug_am); + } else { + typec_plug_set_num_altmodes(con->plug, 0); + } + return 0; }I am seeing the following panic on linux-next and bisect is pointing to this commit.
[ 16.411135] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000310
[ 16.411716] Mem abort info:
[ 16.411806] ESR =0x0000000096000044
[ 16.412147] EC =0x25: DABT (current EL), IL = 32 bits
[ 16.412465] SET = 0, FnV = 0
[ 16.412530] EA = 0, S1PTW = 0
[ 16.412778] FSC =0x04: level 0 translation fault
[ 16.413084] Data abort info:
[ 16.413149] ISV = 0, ISS =0x00000044, ISS2 =0x00000000
[ 16.413534] CM = 0, WnR = 1, TnD = 0, TagAccess = 0
[ 16.414163] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[ 16.414649] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000101b2b000
[ 16.414784] [0000000000000310] pgd=0000000000000000, p4d=0000000000000000
[ 16.414940] Internal error: Oops: 0000000096000044 [#1] PREEMPT SMP
[ 16.414946] Modules linked in:tegra210_adma(+)snd_soc_tegra210_ahub(+)drm backlightsnd_soc_tegra_audio_graph_cardsnd_soc_audio_graph_carducsi_ccgtypec_ucsicrct10dif_cesnd_soc_simple_card_utilstypecpwm_fansnd_soc_rt5659(+)snd_soc_rl6231ina3221pwm_tegrategra_aconnectphy_tegra194_p2usnd_hda_codec_hdmiat24snd_hda_tegrasnd_hda_codecsnd_hda_corelm90tegra_xudchost1xpcie_tegra194tegra_bpmp_thermalip_tablesx_tablesipv6 [ 16.415056] CPU: 0 PID: 290 Comm: kworker/0:4 Not tainted 6.10.0-rc4-next-20240617-g76db4c64526c #1
[ 16.415063] Hardware name: NVIDIA Jetson AGX Xavier Developer Kit (DT)
[ 16.415067] Workqueue:events_longucsi_init_work[typec_ucsi]
[ 16.415082] pstate: 80400009 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 16.415091] pc :typec_plug_set_num_altmodes+0x18/0x6c[typec]
[ 16.415112] lr : ucsi_check_cable.part.0+0x228/0x280 [typec_ucsi]
[ 16.415120] sp :ffff8000848bbca0
[ 16.415123] x29:ffff8000848bbca0x28:ffff000080e66000x27:ffff80007adc96e8
[ 16.415140] x26:ffff80007adc96f8x25:ffff0000834510c0x24:ffff000080e66010
[ 16.415150] x23:ffff000080e6638cx22:ffff000080e664c0x21:ffff000089c95800
[ 16.415160] x20: 0000000000000000 x19: 0000000000000000 x18: ffffffffffffffff
[ 16.415170] x17:1e00000001000000x16:c203000000000000x15:00656c6261632d30
[ 16.415180] x14:ffff800082c922d8x13: 0000000000000040 x12: 0000000000000228
[ 16.415191] x11: 0000000000000000 x10: 0000000000000000 x9 : 0000000000000000
[ 16.415201] x8 :3d45505954564544x7 :0063657079743d4dx6 : 0000000000000000
[ 16.415212] x5 :ffff0000835ee780x4 :ffff000080e66288x3 : 0000000000000000
[ 16.415223] x2 : 0000000000000000 x1 : 0000000000000000 x0 : 0000000000000000
[ 16.415233] Call trace:
[ 16.415236]typec_plug_set_num_altmodes+0x18/0x6c[typec]
[ 16.415255] ucsi_check_cable.part.0+0x228/0x280 [typec_ucsi]
[ 16.415264]ucsi_init_work+0x8b4/0x9b8[typec_ucsi]
[ 16.415271]process_one_work+0x150/0x294
[ 16.415282]worker_thread+0x2f4/0x3fc
[ 16.415289]kthread+0x118/0x11c
[ 16.415296]ret_from_fork+0x10/0x20
[ 16.415310] Code:a9be7bfd910003fdf9000bf3aa0003f3(b9031001)
[ 16.521018] ata1: SATA link down (SStatus 0 SControl 300)
[ 16.524401] ---[ end trace 0000000000000000 ]---
Quoted 2 lines
I have not looked any further yet, but wanted to report this. If you have any thoughts let me know.
It is crashing because 'con->plug' is not initialised when typec_plug_set_num_altmodes() is called. Do we need to add a check to see if 'con->plug' is valid in ucsi_check_cable()?
Jon
Signature · 1 line
nvpublic
Re: [PATCH v5 4/4] usb: typec: ucsi: Always set number of alternate modes
Quoted 141 lines · Jon Hunter
Quoted 138 lines · Jon Hunter
Hi Jameson,
Quoted 56 lines · Jameson Thies
Providing the number of known alternate modes allows user space to determine when device registration has completed. Always register a number of known alternate modes for the partner and cable plug, even when the number of supported alternate modes is 0.
Reviewed-by Heikki Krogerus <[email protected]>Reviewed-by Benson Leung <[email protected]>Signed-off-by Jameson Thies <[email protected]>---
Changes in V5:
- None.Changes in V4:
- None.Changes in V3:
- None.Changes in V2:
- None.drivers/usb/typec/ucsi/ucsi.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)drivers/usb/typec/ucsi/ucsi.c +3
-2@@ -812,10 +812,11 @@ static int ucsi_check_altmodes(struct ucsi_connector *con)/* Ignoring the errors in this case. */ if (con->partner_altmode[0 ]) { num_partner_am =ucsi_get_num_altmode (con->partner_altmode); -if (num_partner_am >0 ) -typec_partner_set_num_altmodes (con->partner, num_partner_am); +typec_partner_set_num_altmodes (con->partner, num_partner_am);ucsi_altmode_update_active (con);return 0 ; + }else { +typec_partner_set_num_altmodes (con->partner,0 ); }return ret;drivers/usb/typec/ucsi/ucsi.c +1
-1@@ -1138,7 +1139,7 @@ static int ucsi_check_connection(struct ucsi_connector *con)static int ucsi_check_cable (struct ucsi_connector *con) {u64 command; -int ret; +int ret, num_plug_am;if (con->cable)return 0 ;@@ -1172,6 +1173,13 @@ static int ucsi_check_cable(struct ucsi_connector *con)return ret;drivers/usb/typec/ucsi/ucsi.c static int ucsi_check_cable(struct · line 1184
Quoted 10 lines
drivers/usb/typec/ucsi/ucsi.c +7
-0} +if (con->plug_altmode[0 ]) { + num_plug_am =ucsi_get_num_altmode (con->plug_altmode); +typec_plug_set_num_altmodes (con->plug, num_plug_am); + }else { +typec_plug_set_num_altmodes (con->plug,0 ); + } +return 0 ; }I am seeing the following panic on linux-next and bisect is pointing to this commit.
[ 16.411135] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000310 [ 16.411716] Mem abort info: [ 16.411806] ESR = 0x0000000096000044 [ 16.412147] EC = 0x25: DABT (current EL), IL = 32 bits [ 16.412465] SET = 0, FnV = 0 [ 16.412530] EA = 0, S1PTW = 0 [ 16.412778] FSC = 0x04: level 0 translation fault [ 16.413084] Data abort info: [ 16.413149] ISV = 0, ISS = 0x00000044, ISS2 = 0x00000000 [ 16.413534] CM = 0, WnR = 1, TnD = 0, TagAccess = 0 [ 16.414163] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [ 16.414649] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000101b2b000 [ 16.414784] [0000000000000310] pgd=0000000000000000, p4d=0000000000000000 [ 16.414940] Internal error: Oops: 0000000096000044 [#1] PREEMPT SMP [ 16.414946] Modules linked in: tegra210_adma(+)
snd_soc_tegra210_ahub(+)drm backlightsnd_soc_tegra_audio_graph_cardsnd_soc_audio_graph_carducsi_ccgtypec_ucsicrct10dif_cesnd_soc_simple_card_utilstypecpwm_fansnd_soc_rt5659(+)snd_soc_rl6231ina3221pwm_tegrategra_aconnectphy_tegra194_p2usnd_hda_codec_hdmiat24snd_hda_tegrasnd_hda_codecsnd_hda_corelm90tegra_xudchost1xpcie_tegra194tegra_bpmp_thermalip_tablesx_tablesipv6[ 16.415056] CPU: 0 PID: 290 Comm: kworker/0:4 Not tainted 6.10.0-rc4-next-20240617-g76db4c64526c #1 [ 16.415063] Hardware name: NVIDIA Jetson AGX Xavier Developer Kit (DT) [ 16.415067] Workqueue: events_long ucsi_init_work [typec_ucsi] [ 16.415082] pstate: 80400009 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 16.415091] pc : typec_plug_set_num_altmodes+0x18/0x6c [typec] [ 16.415112] lr : ucsi_check_cable.part.0+0x228/0x280 [typec_ucsi] [ 16.415120] sp : ffff8000848bbca0 [ 16.415123] x29: ffff8000848bbca0 x28: ffff000080e66000 x27: ffff80007adc96e8 [ 16.415140] x26: ffff80007adc96f8 x25: ffff0000834510c0 x24: ffff000080e66010 [ 16.415150] x23: ffff000080e6638c x22: ffff000080e664c0 x21: ffff000089c95800 [ 16.415160] x20: 0000000000000000 x19: 0000000000000000 x18: ffffffffffffffff [ 16.415170] x17: 1e00000001000000 x16: c203000000000000 x15: 00656c6261632d30 [ 16.415180] x14: ffff800082c922d8 x13: 0000000000000040 x12: 0000000000000228 [ 16.415191] x11: 0000000000000000 x10: 0000000000000000 x9 : 0000000000000000 [ 16.415201] x8 : 3d45505954564544 x7 : 0063657079743d4d x6 : 0000000000000000 [ 16.415212] x5 : ffff0000835ee780 x4 : ffff000080e66288 x3 : 0000000000000000 [ 16.415223] x2 : 0000000000000000 x1 : 0000000000000000 x0 : 0000000000000000 [ 16.415233] Call trace: [ 16.415236] typec_plug_set_num_altmodes+0x18/0x6c [typec] [ 16.415255] ucsi_check_cable.part.0+0x228/0x280 [typec_ucsi] [ 16.415264] ucsi_init_work+0x8b4/0x9b8 [typec_ucsi] [ 16.415271] process_one_work+0x150/0x294 [ 16.415282] worker_thread+0x2f4/0x3fc [ 16.415289] kthread+0x118/0x11c [ 16.415296] ret_from_fork+0x10/0x20 [ 16.415310] Code: a9be7bfd 910003fd f9000bf3 aa0003f3 (b9031001) [ 16.521018] ata1: SATA link down (SStatus 0 SControl 300) [ 16.524401] ---[ end trace 0000000000000000 ]---
Quoted 7 lines
Quoted 2 lines
I have not looked any further yet, but wanted to report this. If you have any thoughts let me know.
It is crashing because '
con->plug' is not initialised whentypec_plug_set_num_altmodes()is called. Do we need to add a check to see if 'con->plug' is valid inucsi_check_cable()?
Yes. Either of con->calbe and con->plug can be NULL.
Signature · 2 lines
With best wishes Dmitry
Jon Hunter
maintainerRe: [PATCH v5 4/4] usb: typec: ucsi: Always set number of alternate modes
Quoted 65 lines · Dmitry Baryshkov
Quoted 64 lines · Jon Hunter
Quoted 61 lines · Jon Hunter
Hi Jameson,
Quoted 58 lines · Jameson Thies
Providing the number of known alternate modes allows user space to determine when device registration has completed. Always register a number of known alternate modes for the partner and cable plug, even when the number of supported alternate modes is 0.
Reviewed-by Heikki Krogerus <[email protected]>Reviewed-by Benson Leung <[email protected]>Signed-off-by Jameson Thies <[email protected]>---
Changes in V5:
- None.Changes in V4:
- None.Changes in V3:
- None.Changes in V2:
- None.drivers/usb/typec/ucsi/ucsi.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)drivers/usb/typec/ucsi/ucsi.c +3
-2@@ -812,10 +812,11 @@ static int ucsi_check_altmodes(struct ucsi_connector *con)/* Ignoring the errors in this case. */ if (con->partner_altmode[0 ]) { num_partner_am =ucsi_get_num_altmode (con->partner_altmode); -if (num_partner_am >0 ) -typec_partner_set_num_altmodes (con->partner, num_partner_am); +typec_partner_set_num_altmodes (con->partner, num_partner_am);ucsi_altmode_update_active (con);return 0 ; + }else { +typec_partner_set_num_altmodes (con->partner,0 ); }return ret;drivers/usb/typec/ucsi/ucsi.c +2
-1@@ -1138,7 +1139,7 @@ static int ucsi_check_connection(struct ucsi_connector *con)static int ucsi_check_cable (struct ucsi_connector *con) {u64 command; -int ret; +int ret, num_plug_am;if (con->cable)return 0 ;@@ -1172,6 +1173,13 @@ static int ucsi_check_cable(struct ucsi_connector *con)return ret; } +if (con->plug_altmode[0 ]) {
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/usb/typec/ucsi/ucsi.c static int ucsi_check_cable(struct · line 1184
Quoted 8 lines
drivers/usb/typec/ucsi/ucsi.c +6
-0+ num_plug_am =ucsi_get_num_altmode (con->plug_altmode); +typec_plug_set_num_altmodes (con->plug, num_plug_am); + }else { +typec_plug_set_num_altmodes (con->plug,0 ); + } +return 0 ; }
Looking at this some more, the plug is only registered in ucsi_check_cable() if UCSI_CAP_ALT_MODE_DETAILS is specified for the Type C controller. The Cypress CCG explicitly clears this flag.
The following will only call typec_plug_set_num_altmodes() if the call to ucsi_register_plug() is successful ...
drivers/usb/typec/ucsi/ucsi.c +6 -6
@@ -1176,13 +1176,13 @@ static int ucsi_check_cable(struct ucsi_connector *con) ret =ucsi_register_altmodes (con,UCSI_RECIPIENT_SOP_P );if (ret <0 )return ret; - } -if (con->plug_altmode[0 ]) { - num_plug_am =ucsi_get_num_altmode (con->plug_altmode); -typec_plug_set_num_altmodes (con->plug, num_plug_am); - }else { -typec_plug_set_num_altmodes (con->plug,0 ); +if (con->plug_altmode[0 ]) { + num_plug_am =ucsi_get_num_altmode (con->plug_altmode); +typec_plug_set_num_altmodes (con->plug, num_plug_am); + }else { +typec_plug_set_num_altmodes (con->plug,0 ); + } }return 0 ;
Quoted 5 lines
Quoted 3 lines
It is crashing because '
con->plug' is not initialised whentypec_plug_set_num_altmodes()is called. Do we need to add a check to see if 'con->plug' is valid inucsi_check_cable()?Yes. Either of
con->calbeandcon->plugcan be NULL.
Thanks for confirming.
Jon
Signature · 1 line
nvpublic
Re: [PATCH v5 4/4] usb: typec: ucsi: Always set number of alternate modes
Hi Jon,
thank you for catching this. I'll post a fix to address the null pointer dereferencing.
Thanks,
Jameson
Hi everyone. Friendly ping on this series. It still applies cleanly to the top of usb-next. If there is anything you would like me to change, please let me know.
Thanks,
Jameson