UCSI:CCG: AMD Platform

UCSI:CCG: AMD Platform

[Shah, Nehal-bakulchandra]

Hi

Currently i am working on enabling UCSI support
for CCGx based controller on AMD GPU Cards.

Now i am observing the issue reported here when
i unplug the cable.

https://bugzilla.redhat.com/show_bug.cgi?id=1762031



Also would like to know is there any way we can
get user level notifications for UCSI?


Thanks
Nehal Shah

Re: UCSI:CCG: AMD Platform

[Heikki Krogerus]

Hi,

On Mon, Feb 03, 2020 at 10:52:52AM +0530, Shah, Nehal-bakulchandra wrote:
> Currently i am working on enabling UCSI support
> for CCGx based controller on AMD GPU Cards.
> 
> Now i am observing the issue reported here when
> i unplug the cable.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1762031
> 
> Also would like to know is there any way we can
> get user level notifications for UCSI?

If you want to see the actual UCSI notification in user space, then
that is not possible, but the driver does produce trace output, and I
would actually like to see what we got there. You need debugfs to be
mounted. Then try the following:

        # Unload all UCSI modules
        modprobe -r ucsi_acpi

        # At this point you should plug-in the problematic device

        # Reload the UCSI core module
        modprobe typec_ucsi

        # Enable UCSI tracing
        echo 1 > /sys/kernel/debug/tracing/events/ucsi/enable

        # Now reload the ACPI glue driver
        modprobe ucsi_acpi

        # Unplug the problematic device so that you see the error

        # Finally dump the trace output
        cat /sys/kernel/debug/tracing/trace

So if that works, please send the trace output to me.

Thanks,

-- 
heikki

Re: UCSI:CCG: AMD Platform

[Heikki Krogerus]

On Mon, Feb 03, 2020 at 03:28:11PM +0200, Heikki Krogerus wrote:
> Hi,
> 
> On Mon, Feb 03, 2020 at 10:52:52AM +0530, Shah, Nehal-bakulchandra wrote:
> > Currently i am working on enabling UCSI support
> > for CCGx based controller on AMD GPU Cards.
> > 
> > Now i am observing the issue reported here when
> > i unplug the cable.
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1762031
> > 
> > Also would like to know is there any way we can
> > get user level notifications for UCSI?
> 
> If you want to see the actual UCSI notification in user space, then
> that is not possible, but the driver does produce trace output, and I
> would actually like to see what we got there. You need debugfs to be
> mounted. Then try the following:
> 
>         # Unload all UCSI modules
>         modprobe -r ucsi_acpi
> 
>         # At this point you should plug-in the problematic device
> 
>         # Reload the UCSI core module
>         modprobe typec_ucsi
> 
>         # Enable UCSI tracing
>         echo 1 > /sys/kernel/debug/tracing/events/ucsi/enable
> 
>         # Now reload the ACPI glue driver
>         modprobe ucsi_acpi
> 
>         # Unplug the problematic device so that you see the error
> 
>         # Finally dump the trace output
>         cat /sys/kernel/debug/tracing/trace
> 
> So if that works, please send the trace output to me.

Actually, first things first. Please share your dmesg output. Are you
using ucsi_acpi or ucsi_ccg glue driver?

thanks,

-- 
heikki

Re: UCSI:CCG: AMD Platform

[Shah, Nehal-bakulchandra]

[-- Attachment #1: Type: text/plain, Size: 2093 bytes --]

Hi

Sorry for the delayed response. I was on vacation.
On 2/3/2020 7:02 PM, Heikki Krogerus wrote:
> On Mon, Feb 03, 2020 at 03:28:11PM +0200, Heikki Krogerus wrote:
>> Hi,
>>
>> On Mon, Feb 03, 2020 at 10:52:52AM +0530, Shah, Nehal-bakulchandra wrote:
>>> Currently i am working on enabling UCSI support
>>> for CCGx based controller on AMD GPU Cards.
>>>
>>> Now i am observing the issue reported here when
>>> i unplug the cable.
>>>
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.redhat.com%2Fshow_bug.cgi%3Fid%3D1762031&data=02%7C01%7CNehal-bakulchandra.Shah%40amd.com%7Ceb1ac5e877db4fa9d75f08d7a8ad87a3%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637163335569266081&sdata=KemJKkVhpqDo%2FSbHhVaMz7jrcploEALJYg%2BRWvhJ7bM%3D&reserved=0
>>>
>>> Also would like to know is there any way we can
>>> get user level notifications for UCSI?
>>
>> If you want to see the actual UCSI notification in user space, then
>> that is not possible, but the driver does produce trace output, and I
>> would actually like to see what we got there. You need debugfs to be
>> mounted. Then try the following:
>>
>>         # Unload all UCSI modules
>>         modprobe -r ucsi_acpi
>>
>>         # At this point you should plug-in the problematic device
>>
>>         # Reload the UCSI core module
>>         modprobe typec_ucsi
>>
>>         # Enable UCSI tracing
>>         echo 1 > /sys/kernel/debug/tracing/events/ucsi/enable
>>
>>         # Now reload the ACPI glue driver
>>         modprobe ucsi_acpi
>>
>>         # Unplug the problematic device so that you see the error
>>
>>         # Finally dump the trace output
>>         cat /sys/kernel/debug/tracing/trace
>>
>> So if that works, please send the trace output to me.
> 
> Actually, first things first. Please share your dmesg output. Are you
> using ucsi_acpi or ucsi_ccg glue driver?
> 
> thanks,
> 

I am using CCG based UCSI driver without any
modification.For I2C part i have written custom
driver.

I have attached the trace out and dmesg crash log.

Please have a look


Thanks
Nehal Shah



[-- Attachment #2: dmesg.txt --]
[-- Type: text/plain, Size: 4002 bytes --]


                
[ 1024.251288] BUG: kernel NULL pointer dereference, address: 0000000000000080
[ 1024.251290] #PF: supervisor read access in kernel mode
[ 1024.251291] #PF: error_code(0x0000) - not-present page
[ 1024.251292] PGD 0 P4D 0 
[ 1024.251296] Oops: 0000 [#1] SMP NOPTI
[ 1024.251299] CPU: 6 PID: 282 Comm: kworker/6:2 Tainted: G           O      5.5.0-rc7+ #15
[ 1024.251300] Hardware name: AMD Myrtle/Myrtle-RV, BIOS WMX0108N 01/06/2020
[ 1024.251306] Workqueue: events ucsi_handle_connector_change [typec_ucsi]
[ 1024.251311] RIP: 0010:ucsi_displayport_remove_partner+0xb/0x30 [typec_ucsi]
[ 1024.251314] Code: 00 c6 43 38 00 c7 43 28 00 00 00 00 48 83 c7 10 e8 ea a6 0d f3 5b 5d c3 0f 1f 80 00 00 00 00 0f 1f 44 00 00 48 85 ff 74 1e 55 <48> 8b 47 78 48 89 e5 48 85 c0 0f 84 50 03 00 00 48 c7 00 00 00 00
[ 1024.251316] RSP: 0018:ffffbec040617dc0 EFLAGS: 00010202
[ 1024.251318] RAX: 0000000000000008 RBX: ffffa0b5dd16a970 RCX: 000000000000767f
[ 1024.251320] RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000000000008
[ 1024.251321] RBP: ffffbec040617df0 R08: 0000000000000000 R09: ffffbec040617c78
[ 1024.251322] R10: ffffa0b59777e818 R11: ffffa0b5f872cc78 R12: 0000000000000000
[ 1024.251323] R13: ffffa0b5dd16a970 R14: 0000000000000001 R15: ffffa0b5dd16a800
[ 1024.251325] FS:  0000000000000000(0000) GS:ffffa0b5f8780000(0000) knlGS:0000000000000000
[ 1024.251326] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1024.251327] CR2: 0000000000000080 CR3: 000000019e10e000 CR4: 00000000003406e0
[ 1024.251329] Call Trace:
[ 1024.251334]  ? ucsi_unregister_altmodes+0x7f/0xa0 [typec_ucsi]
[ 1024.251337]  ucsi_unregister_partner.part.0+0x17/0x30 [typec_ucsi]
[ 1024.251341]  ucsi_handle_connector_change+0x220/0x320 [typec_ucsi]
[ 1024.251345]  ? __schedule+0x2e0/0x760
[ 1024.251349]  process_one_work+0x1ec/0x3a0
[ 1024.251352]  worker_thread+0x4d/0x400
[ 1024.251354]  kthread+0x104/0x140
[ 1024.251357]  ? process_one_work+0x3a0/0x3a0
[ 1024.251360]  ? kthread_park+0x90/0x90
[ 1024.251362]  ret_from_fork+0x22/0x40
[ 1024.251364] Modules linked in: typec_displayport ucsi_ccg ucsi_pci_amd(O) typec_ucsi typec nls_iso8859_1 amdgpu edac_mce_amd kvm_amd ccp kvm snd_hda_codec_realtek irqbypass snd_seq_midi snd_hda_codec_generic snd_seq_midi_event ledtrig_audio snd_rawmidi snd_hda_codec_hdmi crct10dif_pclmul crc32_pclmul snd_hda_intel amd_iommu_v2 snd_intel_dspcfg gpu_sched snd_hda_codec snd_seq ghash_clmulni_intel ttm snd_hda_core aesni_intel crypto_simd drm_kms_helper snd_hwdep cryptd glue_helper snd_pcm snd_seq_device drm k10temp wmi_bmof snd_pci_acp3x snd_timer input_leds i2c_algo_bit snd fb_sys_fops syscopyarea sysfillrect soundcore sysimgblt mac_hid sch_fq_codel parport_pc ppdev lp parport ip_tables x_tables autofs4 hid_generic usbhid hid nvme i2c_piix4 nvme_core sdhci_pci cqhci tg3 sdhci ahci libahci wmi video gpio_amdpt gpio_generic
[ 1024.251398] CR2: 0000000000000080
[ 1024.251401] ---[ end trace b474f602fe29e6b8 ]---
[ 1024.251404] RIP: 0010:ucsi_displayport_remove_partner+0xb/0x30 [typec_ucsi]
[ 1024.251406] Code: 00 c6 43 38 00 c7 43 28 00 00 00 00 48 83 c7 10 e8 ea a6 0d f3 5b 5d c3 0f 1f 80 00 00 00 00 0f 1f 44 00 00 48 85 ff 74 1e 55 <48> 8b 47 78 48 89 e5 48 85 c0 0f 84 50 03 00 00 48 c7 00 00 00 00
[ 1024.251407] RSP: 0018:ffffbec040617dc0 EFLAGS: 00010202
[ 1024.251409] RAX: 0000000000000008 RBX: ffffa0b5dd16a970 RCX: 000000000000767f
[ 1024.251410] RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000000000008
[ 1024.251411] RBP: ffffbec040617df0 R08: 0000000000000000 R09: ffffbec040617c78
[ 1024.251412] R10: ffffa0b59777e818 R11: ffffa0b5f872cc78 R12: 0000000000000000
[ 1024.251413] R13: ffffa0b5dd16a970 R14: 0000000000000001 R15: ffffa0b5dd16a800
[ 1024.251414] FS:  0000000000000000(0000) GS:ffffa0b5f8780000(0000) knlGS:0000000000000000
[ 1024.251416] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1024.251418] CR2: 0000000000000080 CR3: 000000019e10e000 CR4: 00000000003406e0
[ 1024.252359] Nehal xfer  success 

[-- Attachment #3: trace.txt --]
[-- Type: text/plain, Size: 1772 bytes --]

# tracer: nop
#
# entries-in-buffer/entries-written: 7/7   #P:8
#
#                              _-----=> irqs-off
#                             / _----=> need-resched
#                            | / _---=> hardirq/softirq
#                            || / _--=> preempt-depth
#                            ||| /     delay
#           TASK-PID   CPU#  ||||    TIMESTAMP  FUNCTION
#              | |       |   ||||       |         |
     kworker/0:4-189   [000] ....   221.043174: ucsi_register_port: port0 status: change=0000, opmode=0, connected=0, sourcing=0, partner_flags=0, partner_type=0, request_data_obj=00000000, BC status=0
     kworker/0:4-189   [000] ....   221.094474: ucsi_register_port: port1 status: change=0000, opmode=0, connected=0, sourcing=0, partner_flags=0, partner_type=0, request_data_obj=00000000, BC status=0
     kworker/6:2-282   [006] ....   808.864397: ucsi_connector_change: port0 status: change=4800, opmode=5, connected=1, sourcing=1, partner_flags=1, partner_type=2, request_data_obj=00000000, BC status=0
     kworker/6:2-282   [006] ....   808.934334: ucsi_connector_change: port0 status: change=0040, opmode=3, connected=1, sourcing=1, partner_flags=1, partner_type=2, request_data_obj=1201685a, BC status=0
     kworker/6:2-282   [006] ....   809.937685: ucsi_connector_change: port0 status: change=0100, opmode=3, connected=1, sourcing=1, partner_flags=1, partner_type=2, request_data_obj=1201685a, BC status=0
     kworker/6:2-282   [006] ....   810.005748: ucsi_register_altmode: partner alt mode: svid ff01, mode 1 vdo 1405
     kworker/6:2-282   [006] ....   810.042554: ucsi_connector_change: port0 status: change=0800, opmode=3, connected=1, sourcing=1, partner_flags=2, partner_type=2, request_data_obj=1201685a, BC status=0

Re: UCSI:CCG: AMD Platform

[Heikki Krogerus]

[-- Attachment #1: Type: text/plain, Size: 2336 bytes --]

On Mon, Feb 10, 2020 at 03:39:38PM +0530, Shah, Nehal-bakulchandra wrote:
> Hi
> 
> Sorry for the delayed response. I was on vacation.
> On 2/3/2020 7:02 PM, Heikki Krogerus wrote:
> > On Mon, Feb 03, 2020 at 03:28:11PM +0200, Heikki Krogerus wrote:
> >> Hi,
> >>
> >> On Mon, Feb 03, 2020 at 10:52:52AM +0530, Shah, Nehal-bakulchandra wrote:
> >>> Currently i am working on enabling UCSI support
> >>> for CCGx based controller on AMD GPU Cards.
> >>>
> >>> Now i am observing the issue reported here when
> >>> i unplug the cable.
> >>>
> >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.redhat.com%2Fshow_bug.cgi%3Fid%3D1762031&data=02%7C01%7CNehal-bakulchandra.Shah%40amd.com%7Ceb1ac5e877db4fa9d75f08d7a8ad87a3%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637163335569266081&sdata=KemJKkVhpqDo%2FSbHhVaMz7jrcploEALJYg%2BRWvhJ7bM%3D&reserved=0
> >>>
> >>> Also would like to know is there any way we can
> >>> get user level notifications for UCSI?
> >>
> >> If you want to see the actual UCSI notification in user space, then
> >> that is not possible, but the driver does produce trace output, and I
> >> would actually like to see what we got there. You need debugfs to be
> >> mounted. Then try the following:
> >>
> >>         # Unload all UCSI modules
> >>         modprobe -r ucsi_acpi
> >>
> >>         # At this point you should plug-in the problematic device
> >>
> >>         # Reload the UCSI core module
> >>         modprobe typec_ucsi
> >>
> >>         # Enable UCSI tracing
> >>         echo 1 > /sys/kernel/debug/tracing/events/ucsi/enable
> >>
> >>         # Now reload the ACPI glue driver
> >>         modprobe ucsi_acpi
> >>
> >>         # Unplug the problematic device so that you see the error
> >>
> >>         # Finally dump the trace output
> >>         cat /sys/kernel/debug/tracing/trace
> >>
> >> So if that works, please send the trace output to me.
> > 
> > Actually, first things first. Please share your dmesg output. Are you
> > using ucsi_acpi or ucsi_ccg glue driver?
> > 
> > thanks,
> > 
> 
> I am using CCG based UCSI driver without any
> modification.For I2C part i have written custom
> driver.
> 
> I have attached the trace out and dmesg crash log.
> 
> Please have a look

Thanks for the logs. Can you test the attached diff?

thanks,

-- 
heikki

[-- Attachment #2: port_lock.diff --]
[-- Type: text/plain, Size: 1485 bytes --]

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index ddf2ad3752de..37837bf5385b 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -870,12 +870,16 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 	con->num = index + 1;
 	con->ucsi = ucsi;
 
+	mutex_lock(&con->lock);
+
 	/* Get connector capability */
 	command = UCSI_GET_CONNECTOR_CAPABILITY;
 	command |= UCSI_CONNECTOR_NUMBER(con->num);
 	ret = ucsi_run_command(ucsi, command, &con->cap, sizeof(con->cap));
-	if (ret < 0)
+	if (ret < 0) {
+		mutex_unlock(&con->lock);
 		return ret;
+	}
 
 	if (con->cap.op_mode & UCSI_CONCAP_OPMODE_DRP)
 		cap->data = TYPEC_PORT_DRD;
@@ -907,8 +911,10 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 
 	/* Register the connector */
 	con->port = typec_register_port(ucsi->dev, cap);
-	if (IS_ERR(con->port))
+	if (IS_ERR(con->port)) {
+		mutex_unlock(&con->lock);
 		return PTR_ERR(con->port);
+	}
 
 	/* Alternate modes */
 	ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_CON);
@@ -922,6 +928,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 			       sizeof(con->status));
 	if (ret < 0) {
 		dev_err(ucsi->dev, "con%d: failed to get status\n", con->num);
+		mutex_unlock(&con->lock);
 		return 0;
 	}
 
@@ -956,6 +963,7 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
 
 	trace_ucsi_register_port(con->num, &con->status);
 
+	mutex_unlock(&con->lock);
 	return 0;
 }
 

Re: UCSI:CCG: AMD Platform

[Heikki Krogerus]

[-- Attachment #1: Type: text/plain, Size: 389 bytes --]

On Thu, Feb 13, 2020 at 02:00:14PM +0200, Heikki Krogerus wrote:
> > I am using CCG based UCSI driver without any
> > modification.For I2C part i have written custom
> > driver.
> > 
> > I have attached the trace out and dmesg crash log.
> > 
> > Please have a look
> 
> Thanks for the logs. Can you test the attached diff?

Actually, don't try that one. Try this one instead.

-- 
heikki

[-- Attachment #2: displayport_lock.diff --]
[-- Type: text/plain, Size: 1179 bytes --]

diff --git a/drivers/usb/typec/ucsi/displayport.c b/drivers/usb/typec/ucsi/displayport.c
index 0f1273ae086c..0f8f5d07e270 100644
--- a/drivers/usb/typec/ucsi/displayport.c
+++ b/drivers/usb/typec/ucsi/displayport.c
@@ -285,6 +285,8 @@ struct typec_altmode *ucsi_register_displayport(struct ucsi_connector *con,
 	struct typec_altmode *alt;
 	struct ucsi_dp *dp;
 
+	mutex_lock(&con->lock);
+
 	/* We can't rely on the firmware with the capabilities. */
 	desc->vdo |= DP_CAP_DP_SIGNALING | DP_CAP_RECEPTACLE;
 
@@ -293,12 +295,15 @@ struct typec_altmode *ucsi_register_displayport(struct ucsi_connector *con,
 	desc->vdo |= all_assignments << 16;
 
 	alt = typec_port_register_altmode(con->port, desc);
-	if (IS_ERR(alt))
+	if (IS_ERR(alt)) {
+		mutex_unlock(&con->lock);
 		return alt;
+	}
 
 	dp = devm_kzalloc(&alt->dev, sizeof(*dp), GFP_KERNEL);
 	if (!dp) {
 		typec_unregister_altmode(alt);
+		mutex_unlock(&con->lock);
 		return ERR_PTR(-ENOMEM);
 	}
 
@@ -311,5 +316,7 @@ struct typec_altmode *ucsi_register_displayport(struct ucsi_connector *con,
 	alt->ops = &ucsi_displayport_ops;
 	typec_altmode_set_drvdata(alt, dp);
 
+	mutex_unlock(&con->lock);
+
 	return alt;
 }

Re: UCSI:CCG: AMD Platform

[Shah, Nehal-bakulchandra]

Hi

On 2/13/2020 5:35 PM, Heikki Krogerus wrote:
> On Thu, Feb 13, 2020 at 02:00:14PM +0200, Heikki Krogerus wrote:
>>> I am using CCG based UCSI driver without any
>>> modification.For I2C part i have written custom
>>> driver.
>>>
>>> I have attached the trace out and dmesg crash log.
>>>
>>> Please have a look
>> Thanks for the logs. Can you test the attached diff?
> Actually, don't try that one. Try this one instead.

Sure i will update on this on Monday.


thanks

Nehal

Re: UCSI:CCG: AMD Platform

[Shah, Nehal-bakulchandra]

[-- Attachment #1: Type: text/plain, Size: 656 bytes --]

Hi

On 2/14/2020 7:58 PM, Shah, Nehal-bakulchandra wrote:
> Hi
>
> On 2/13/2020 5:35 PM, Heikki Krogerus wrote:
>> On Thu, Feb 13, 2020 at 02:00:14PM +0200, Heikki Krogerus wrote:
>>>> I am using CCG based UCSI driver without any
>>>> modification.For I2C part i have written custom
>>>> driver.
>>>>
>>>> I have attached the trace out and dmesg crash log.
>>>>
>>>> Please have a look
>>> Thanks for the logs. Can you test the attached diff?
>> Actually, don't try that one. Try this one instead.
> Sure i will update on this on Monday.
>
>
> thanks
>
> Nehal

Patch is not solving the issue. I have attached both trace and dmesg output.

Thanks

Nehal



[-- Attachment #2: trace.txt --]
[-- Type: text/plain, Size: 1772 bytes --]

# tracer: nop
#
# entries-in-buffer/entries-written: 7/7   #P:8
#
#                              _-----=> irqs-off
#                             / _----=> need-resched
#                            | / _---=> hardirq/softirq
#                            || / _--=> preempt-depth
#                            ||| /     delay
#           TASK-PID   CPU#  ||||    TIMESTAMP  FUNCTION
#              | |       |   ||||       |         |
     kworker/3:2-268   [003] ....   334.360355: ucsi_register_port: port0 status: change=0000, opmode=0, connected=0, sourcing=0, partner_flags=0, partner_type=0, request_data_obj=00000000, BC status=0
     kworker/3:2-268   [003] ....   334.410534: ucsi_register_port: port1 status: change=0000, opmode=0, connected=0, sourcing=0, partner_flags=0, partner_type=0, request_data_obj=00000000, BC status=0
     kworker/0:4-2368  [000] ....   344.162496: ucsi_connector_change: port0 status: change=4800, opmode=5, connected=1, sourcing=1, partner_flags=1, partner_type=2, request_data_obj=00000000, BC status=0
     kworker/0:4-2368  [000] ....   344.185817: ucsi_connector_change: port0 status: change=0040, opmode=3, connected=1, sourcing=1, partner_flags=1, partner_type=2, request_data_obj=1201685a, BC status=0
     kworker/0:4-2368  [000] ....   345.180458: ucsi_connector_change: port0 status: change=0100, opmode=3, connected=1, sourcing=1, partner_flags=1, partner_type=2, request_data_obj=1201685a, BC status=0
     kworker/0:4-2368  [000] ....   345.263533: ucsi_register_altmode: partner alt mode: svid ff01, mode 1 vdo 1405
     kworker/0:4-2368  [000] ....   345.281688: ucsi_connector_change: port0 status: change=0800, opmode=3, connected=1, sourcing=1, partner_flags=2, partner_type=2, request_data_obj=1201685a, BC status=0

[-- Attachment #3: demsg --]
[-- Type: text/plain, Size: 2606 bytes --]

20614] FS:  0000000000000000(0000) GS:ffff8c6c38640000(0000) knlGS:0000000000000000
[  362.320616] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  362.320617] CR2: 0000000000000080 CR3: 00000001979d2000 CR4: 00000000003406e0
[  362.320618] Call Trace:
[  362.320622]  ucsi_unregister_altmodes+0x6c/0x89 [typec_ucsi]
[  362.320625]  ucsi_unregister_partner.part.0+0x17/0x2b [typec_ucsi]
[  362.320629]  ucsi_handle_connector_change.cold+0x62/0x6e [typec_ucsi]
[  362.320631]  process_one_work+0x1e9/0x3a0
[  362.320633]  worker_thread+0x4d/0x400
[  362.320636]  kthread+0x104/0x140
[  362.320638]  ? process_one_work+0x3a0/0x3a0
[  362.320640]  ? kthread_park+0x90/0x90
[  362.320642]  ret_from_fork+0x22/0x40
[  362.320644] Modules linked in: typec_displayport ucsi_ccg ucsi_pci_amd(OE) typec_ucsi typec nls_iso8859_1 amdgpu snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio input_leds snd_hda_codec_hdmi snd_hda_intel snd_intel_dspcfg snd_hda_codec edac_mce_amd snd_hda_core kvm_amd snd_hwdep kvm gpu_sched snd_pcm ttm snd_seq_midi snd_seq_midi_event snd_rawmidi drm_kms_helper snd_seq drm snd_seq_device irqbypass snd_timer crct10dif_pclmul mac_hid snd crc32_pclmul i2c_algo_bit ghash_clmulni_intel fb_sys_fops aesni_intel syscopyarea crypto_simd soundcore sysfillrect sysimgblt wmi_bmof k10temp cryptd ccp glue_helper sch_fq_codel parport_pc ppdev lp parport ip_tables x_tables autofs4 hid_generic usbhid hid tg3 sdhci_pci i2c_piix4 ptp cqhci ahci sdhci pps_core libahci wmi video gpio_amdpt gpio_generic
[  362.320679] CR2: 0000000000000080
[  362.320681] ---[ end trace ad3ed17854ca5182 ]---
[  362.320685] RIP: 0010:ucsi_displayport_remove_partner+0xe/0x20 [typec_ucsi]
[  362.320688] Code: 38 00 c7 43 28 00 00 00 00 48 83 c7 10 e8 0a fe b3 c7 5b 5d c3 0f 1f 80 00 00 00 00 0f 1f 44 00 00 55 48 89 e5 48 85 ff 74 0f <48> 8b 47 78 48 c7 00 00 00 00 00 c6 40 3d 00 5d c3 90 0f 1f 44 00
[  362.320689] RSP: 0018:ffffac3c42aabdb8 EFLAGS: 00010202
[  362.320691] RAX: 0000000000000008 RBX: ffff8c6bf1526970 RCX: ffffac3c4006d000
[  362.320692] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000008
[  362.320693] RBP: ffffac3c42aabdb8 R08: 0000000000000000 R09: 0000000000000038
[  362.320694] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
[  362.320696] R13: 0000000000000001 R14: ffff8c6bf1526970 R15: ffff8c6bf1526800
[  362.320697] FS:  0000000000000000(0000) GS:ffff8c6c38640000(0000) knlGS:0000000000000000
[  362.320699] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  362.320700] CR2: 0000000000000080 CR3: 00000001979d2000 CR4: 00000000003406e0

Re: UCSI:CCG: AMD Platform

[Heikki Krogerus]

[-- Attachment #1: Type: text/plain, Size: 864 bytes --]

On Mon, Feb 24, 2020 at 02:38:12PM +0530, Shah, Nehal-bakulchandra wrote:
> Hi
> 
> On 2/14/2020 7:58 PM, Shah, Nehal-bakulchandra wrote:
> > Hi
> >
> > On 2/13/2020 5:35 PM, Heikki Krogerus wrote:
> >> On Thu, Feb 13, 2020 at 02:00:14PM +0200, Heikki Krogerus wrote:
> >>>> I am using CCG based UCSI driver without any
> >>>> modification.For I2C part i have written custom
> >>>> driver.
> >>>>
> >>>> I have attached the trace out and dmesg crash log.
> >>>>
> >>>> Please have a look
> >>> Thanks for the logs. Can you test the attached diff?
> >> Actually, don't try that one. Try this one instead.
> > Sure i will update on this on Monday.
> >
> >
> > thanks
> >
> > Nehal
> 
> Patch is not solving the issue. I have attached both trace and dmesg output.

How about if you try this (the attached patch) together with that
previous diff?

thanks,

-- 
heikki

[-- Attachment #2: 0001-usb-typec-ucsi-displayport-Fix-potential-NULL-pointe.patch --]
[-- Type: text/plain, Size: 1084 bytes --]

From fa1aff5e8e7464851470f29eeae45bde1f089ce1 Mon Sep 17 00:00:00 2001
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Date: Mon, 17 Feb 2020 18:07:17 +0300
Subject: [PATCH] usb: typec: ucsi: displayport: Fix potential NULL pointer
 dereference

In ucsi_displayport_remove_partner(), if the DisplayPort alt
mode was never registered, then there is also no driver data
for it. Adding a check to make sure there really is driver
data for the device before modifying it.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/displayport.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/typec/ucsi/displayport.c b/drivers/usb/typec/ucsi/displayport.c
index 0f1273ae086c..261131c9e37c 100644
--- a/drivers/usb/typec/ucsi/displayport.c
+++ b/drivers/usb/typec/ucsi/displayport.c
@@ -271,6 +271,9 @@ void ucsi_displayport_remove_partner(struct typec_altmode *alt)
 		return;
 
 	dp = typec_altmode_get_drvdata(alt);
+	if (!dp)
+		return;
+
 	dp->data.conf = 0;
 	dp->data.status = 0;
 	dp->initialized = false;
-- 
2.25.0

Re: UCSI:CCG: AMD Platform

[Shah, Nehal-bakulchandra]

Hi
On 2/27/2020 5:53 PM, Heikki Krogerus wrote:
> On Mon, Feb 24, 2020 at 02:38:12PM +0530, Shah, Nehal-bakulchandra wrote:
>> Hi
>>
>> On 2/14/2020 7:58 PM, Shah, Nehal-bakulchandra wrote:
>>> Hi
>>>
>>> On 2/13/2020 5:35 PM, Heikki Krogerus wrote:
>>>> On Thu, Feb 13, 2020 at 02:00:14PM +0200, Heikki Krogerus wrote:
>>>>>> I am using CCG based UCSI driver without any
>>>>>> modification.For I2C part i have written custom
>>>>>> driver.
>>>>>>
>>>>>> I have attached the trace out and dmesg crash log.
>>>>>>
>>>>>> Please have a look
>>>>> Thanks for the logs. Can you test the attached diff?
>>>> Actually, don't try that one. Try this one instead.
>>> Sure i will update on this on Monday.
>>>
>>>
>>> thanks
>>>
>>> Nehal
>> Patch is not solving the issue. I have attached both trace and dmesg output.
> How about if you try this (the attached patch) together with that
> previous diff?
>
> thanks,

Sure, infact i suspected that in first place and tried same logic but it was failed but now i will check with both patch combine and shall update.

Thanks

Nehal Shah

Re: UCSI:CCG: AMD Platform

[Shah, Nehal-bakulchandra]

Hi
On 2/27/2020 10:29 PM, Shah, Nehal-bakulchandra wrote:
> Hi
> On 2/27/2020 5:53 PM, Heikki Krogerus wrote:
>> On Mon, Feb 24, 2020 at 02:38:12PM +0530, Shah, Nehal-bakulchandra wrote:
>>> Hi
>>>
>>> On 2/14/2020 7:58 PM, Shah, Nehal-bakulchandra wrote:
>>>> Hi
>>>>
>>>> On 2/13/2020 5:35 PM, Heikki Krogerus wrote:
>>>>> On Thu, Feb 13, 2020 at 02:00:14PM +0200, Heikki Krogerus wrote:
>>>>>>> I am using CCG based UCSI driver without any
>>>>>>> modification.For I2C part i have written custom
>>>>>>> driver.
>>>>>>>
>>>>>>> I have attached the trace out and dmesg crash log.
>>>>>>>
>>>>>>> Please have a look
>>>>>> Thanks for the logs. Can you test the attached diff?
>>>>> Actually, don't try that one. Try this one instead.
>>>> Sure i will update on this on Monday.
>>>>
>>>>
>>>> thanks
>>>>
>>>> Nehal
>>> Patch is not solving the issue. I have attached both trace and dmesg output.
>> How about if you try this (the attached patch) together with that
>> previous diff?
>>
>> thanks,
> Sure, infact i suspected that in first place and tried same logic but it was failed but now i will check with both patch combine and shall update.
>
> Thanks
>
> Nehal Shah

This is still crashing .

Thanks

Nehal Shah

Re: UCSI:CCG: AMD Platform

[Heikki Krogerus]

[-- Attachment #1: Type: text/plain, Size: 1793 bytes --]

Hi,

On Sat, Feb 29, 2020 at 08:55:50AM +0530, Shah, Nehal-bakulchandra wrote:
> Hi
> On 2/27/2020 10:29 PM, Shah, Nehal-bakulchandra wrote:
> > Hi
> > On 2/27/2020 5:53 PM, Heikki Krogerus wrote:
> >> On Mon, Feb 24, 2020 at 02:38:12PM +0530, Shah, Nehal-bakulchandra wrote:
> >>> Hi
> >>>
> >>> On 2/14/2020 7:58 PM, Shah, Nehal-bakulchandra wrote:
> >>>> Hi
> >>>>
> >>>> On 2/13/2020 5:35 PM, Heikki Krogerus wrote:
> >>>>> On Thu, Feb 13, 2020 at 02:00:14PM +0200, Heikki Krogerus wrote:
> >>>>>>> I am using CCG based UCSI driver without any
> >>>>>>> modification.For I2C part i have written custom
> >>>>>>> driver.
> >>>>>>>
> >>>>>>> I have attached the trace out and dmesg crash log.
> >>>>>>>
> >>>>>>> Please have a look
> >>>>>> Thanks for the logs. Can you test the attached diff?
> >>>>> Actually, don't try that one. Try this one instead.
> >>>> Sure i will update on this on Monday.
> >>>>
> >>>>
> >>>> thanks
> >>>>
> >>>> Nehal
> >>> Patch is not solving the issue. I have attached both trace and dmesg output.
> >> How about if you try this (the attached patch) together with that
> >> previous diff?
> >>
> >> thanks,
> > Sure, infact i suspected that in first place and tried same logic but it was failed but now i will check with both patch combine and shall update.
> >
> > Thanks
> >
> > Nehal Shah
> 
> This is still crashing .

Sorry about the slow process with this, and the late reply.

Right now I'm out of ideas. I'll need to get my hands on the products
that allow me to reproduce the issue. Staring at the code does not
help anymore.

I'm going to cleanup the code a little bit in any case. I'm attaching
a diff with my changes. I don't think it will fix this issue, but I
would appreciate if you tested it in any case, just to be sure.

thanks,

-- 
heikki

[-- Attachment #2: ucsi.diff --]
[-- Type: text/plain, Size: 3261 bytes --]

diff --git a/drivers/usb/typec/ucsi/displayport.c b/drivers/usb/typec/ucsi/displayport.c
index 048381c058a5..1a5c76907827 100644
--- a/drivers/usb/typec/ucsi/displayport.c
+++ b/drivers/usb/typec/ucsi/displayport.c
@@ -263,10 +263,12 @@ static void ucsi_displayport_work(struct work_struct *work)
 	mutex_unlock(&dp->con->lock);
 }
 
-void ucsi_displayport_remove_partner(struct typec_altmode *alt)
+void ucsi_displayport_remove_partner(struct typec_altmode *pdev)
 {
+	const struct typec_altmode *alt;
 	struct ucsi_dp *dp;
 
+	alt = typec_altmode_get_partner(pdev);
 	if (!alt)
 		return;
 
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index ddf2ad3752de..921499407403 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -432,8 +432,12 @@ static int ucsi_register_altmodes(struct ucsi_connector *con, u8 recipient)
 		command |= UCSI_GET_ALTMODE_CONNECTOR_NUMBER(con->num);
 		command |= UCSI_GET_ALTMODE_OFFSET(i);
 		len = ucsi_run_command(con->ucsi, command, alt, sizeof(alt));
-		if (len <= 0)
+		if (len <= 0) {
+			/* Assuming that all alt modes are now registered */
+			if (recipient == UCSI_RECIPIENT_SOP && i)
+				break;
 			return len;
+		}
 
 		/*
 		 * This code is requesting one alt mode at a time, but some PPMs
@@ -464,9 +468,8 @@ static int ucsi_register_altmodes(struct ucsi_connector *con, u8 recipient)
 
 static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
 {
-	const struct typec_altmode *pdev;
 	struct typec_altmode **adev;
-	int i = 0;
+	int i;
 
 	switch (recipient) {
 	case UCSI_RECIPIENT_CON:
@@ -479,16 +482,15 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
 		return;
 	}
 
-	while (adev[i]) {
+	for (i = 0; adev[i]; i++) {
 		if (recipient == UCSI_RECIPIENT_SOP &&
 		    (adev[i]->svid == USB_TYPEC_DP_SID ||
-			(adev[i]->svid == USB_TYPEC_NVIDIA_VLINK_SID &&
-			adev[i]->vdo != USB_TYPEC_NVIDIA_VLINK_DBG_VDO))) {
-			pdev = typec_altmode_get_partner(adev[i]);
-			ucsi_displayport_remove_partner((void *)pdev);
-		}
+		     (adev[i]->svid == USB_TYPEC_NVIDIA_VLINK_SID &&
+		      adev[i]->vdo != USB_TYPEC_NVIDIA_VLINK_DBG_VDO)))
+			ucsi_displayport_remove_partner(adev[i]);
+
 		typec_unregister_altmode(adev[i]);
-		adev[i++] = NULL;
+		adev[i] = NULL;
 	}
 }
 
@@ -583,8 +585,8 @@ static void ucsi_partner_change(struct ucsi_connector *con)
 	ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_SOP);
 	if (ret)
 		dev_err(con->ucsi->dev,
-			"con%d: failed to register partner alternate modes\n",
-			con->num);
+			"con%d: failed to register partner alternate modes (%d)\n",
+			con->num, ret);
 	else
 		ucsi_altmode_update_active(con);
 }
diff --git a/include/linux/usb/typec_altmode.h b/include/linux/usb/typec_altmode.h
index d834e236c6df..85267df13bf1 100644
--- a/include/linux/usb/typec_altmode.h
+++ b/include/linux/usb/typec_altmode.h
@@ -40,7 +40,8 @@ static inline void typec_altmode_set_drvdata(struct typec_altmode *altmode,
 	dev_set_drvdata(&altmode->dev, data);
 }
 
-static inline void *typec_altmode_get_drvdata(struct typec_altmode *altmode)
+static inline void *
+typec_altmode_get_drvdata(const struct typec_altmode *altmode)
 {
 	return dev_get_drvdata(&altmode->dev);
 }

Re: UCSI:CCG: AMD Platform

[Shah, Nehal-bakulchandra]

HiHeikki

On 3/26/2020 2:05 PM, Heikki Krogerus wrote:

> Hi,
>
> On Sat, Feb 29, 2020 at 08:55:50AM +0530, Shah, Nehal-bakulchandra wrote:
>> Hi
>> On 2/27/2020 10:29 PM, Shah, Nehal-bakulchandra wrote:
>>> Hi
>>> On 2/27/2020 5:53 PM, Heikki Krogerus wrote:
>>>> On Mon, Feb 24, 2020 at 02:38:12PM +0530, Shah, Nehal-bakulchandra wrote:
>>>>> Hi
>>>>>
>>>>> On 2/14/2020 7:58 PM, Shah, Nehal-bakulchandra wrote:
>>>>>> Hi
>>>>>>
>>>>>> On 2/13/2020 5:35 PM, Heikki Krogerus wrote:
>>>>>>> On Thu, Feb 13, 2020 at 02:00:14PM +0200, Heikki Krogerus wrote:
>>>>>>>>> I am using CCG based UCSI driver without any
>>>>>>>>> modification.For I2C part i have written custom
>>>>>>>>> driver.
>>>>>>>>>
>>>>>>>>> I have attached the trace out and dmesg crash log.
>>>>>>>>>
>>>>>>>>> Please have a look
>>>>>>>> Thanks for the logs. Can you test the attached diff?
>>>>>>> Actually, don't try that one. Try this one instead.
>>>>>> Sure i will update on this on Monday.
>>>>>>
>>>>>>
>>>>>> thanks
>>>>>>
>>>>>> Nehal
>>>>> Patch is not solving the issue. I have attached both trace and dmesg output.
>>>> How about if you try this (the attached patch) together with that
>>>> previous diff?
>>>>
>>>> thanks,
>>> Sure, infact i suspected that in first place and tried same logic but it was failed but now i will check with both patch combine and shall update.
>>>
>>> Thanks
>>>
>>> Nehal Shah
>> This is still crashing .
> Sorry about the slow process with this, and the late reply.
>
> Right now I'm out of ideas. I'll need to get my hands on the products
> that allow me to reproduce the issue. Staring at the code does not
> help anymore.
>
> I'm going to cleanup the code a little bit in any case. I'm attaching
> a diff with my changes. I don't think it will fix this issue, but I
> would appreciate if you tested it in any case, just to be sure.
>
> thanks,

Thanks for the patch. But i will able to validate it after few days due to countrywide lock down.

regards

Nehal Shah