| Message ID | 1564029037-22929-1-git-send-email-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | 88d02c9ba2e83fc22d37ccb1f11c62ea6fc9ae50 |
| Headers | show |
| Series | [RFC] usb: typec: tcpm: Ignore unsupported/unknown alternate mode requests | expand |
On 2019-07-25 12:30 a.m., Guenter Roeck wrote: > TCPM may receive PD messages associated with unknown or unsupported > alternate modes. If that happens, calls to typec_match_altmode() > will return NULL. The tcpm code does not currently take this into > account. This results in crashes. > > Unable to handle kernel NULL pointer dereference at virtual address 000001f0 > pgd = 41dad9a1 > [000001f0] *pgd=00000000 > Internal error: Oops: 5 [#1] THUMB2 > Modules linked in: tcpci tcpm > CPU: 0 PID: 2338 Comm: kworker/u2:0 Not tainted 5.1.18-sama5-armv7-r2 #6 > Hardware name: Atmel SAMA5 > Workqueue: 2-0050 tcpm_pd_rx_handler [tcpm] > PC is at typec_altmode_attention+0x0/0x14 > LR is at tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm] > ... > [<c03fbee8>] (typec_altmode_attention) from [<bf8030fb>] > (tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm]) > [<bf8030fb>] (tcpm_pd_rx_handler [tcpm]) from [<c012082b>] > (process_one_work+0x123/0x2a8) > [<c012082b>] (process_one_work) from [<c0120a6d>] > (worker_thread+0xbd/0x3b0) > [<c0120a6d>] (worker_thread) from [<c012431f>] (kthread+0xcf/0xf4) > [<c012431f>] (kthread) from [<c01010f9>] (ret_from_fork+0x11/0x38) > > Ignore PD messages if the asociated alternate mode is not supported. > > Reported-by: Douglas Gilbert <dgilbert@interlog.com> > Cc: Douglas Gilbert <dgilbert@interlog.com> > Fixes: e9576fe8e605c ("usb: typec: tcpm: Support for Alternate Modes") > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > Taking a stab at the problem. I don't really know if this is the correct > fix, or even if my understanding of the problem is correct, thus marking > the patch as RFC. Tested-by: Douglas Gilbert <dgilbert@interlog.com> Took a little while because I was changing my Linux end to being based on a Acme Systems Arietta (at91sam9g25). Reason: on the sama5d2xplained I can't have the shield plugged into the Arduino sockets because it drives the pin used by the OM13588 for its interrupt (nalert). So I need to "jump" the pins needed (SDA, SCL, nALERT, 3v3 and ground) to get this working (and 'cat /proc/interrupts' shows me its working with >0 interrupts on "2-0050"). Anyway the DUT is a OM13588+sama5d2xplained running lk 5.1.18 and the other end of the USB-C cable is a OM13588+KL27Z running NXP PD software. Both ends can "play" sink or source (power). The oops occurred when the DUT was source and the other end sent a "send attention" ("l") command. With this patch I do not see the oops. BTW I have tried to run lk 5.2.1 on the sama5d2xplained and the kernel wouldn't start :-( Zilch after u-boot said it was loading the kernel. > drivers/usb/typec/tcpm/tcpm.c | 38 ++++++++++++++++++++++++-------------- > 1 file changed, 24 insertions(+), 14 deletions(-) > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > index 77f71f602f73..1df2844469aa 100644 > --- a/drivers/usb/typec/tcpm/tcpm.c > +++ b/drivers/usb/typec/tcpm/tcpm.c > @@ -1096,7 +1096,8 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt, > break; > case CMD_ATTENTION: > /* Attention command does not have response */ > - typec_altmode_attention(adev, p[1]); > + if (adev) > + typec_altmode_attention(adev, p[1]); > return 0; > default: > break; > @@ -1148,20 +1149,26 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt, > } > break; > case CMD_ENTER_MODE: > - typec_altmode_update_active(pdev, true); > - > - if (typec_altmode_vdm(adev, p[0], &p[1], cnt)) { > - response[0] = VDO(adev->svid, 1, CMD_EXIT_MODE); > - response[0] |= VDO_OPOS(adev->mode); > - return 1; > + if (adev && pdev) { > + typec_altmode_update_active(pdev, true); > + > + if (typec_altmode_vdm(adev, p[0], &p[1], cnt)) { > + response[0] = VDO(adev->svid, 1, > + CMD_EXIT_MODE); > + response[0] |= VDO_OPOS(adev->mode); > + return 1; > + } > } > return 0; > case CMD_EXIT_MODE: > - typec_altmode_update_active(pdev, false); > + if (adev && pdev) { > + typec_altmode_update_active(pdev, false); > > - /* Back to USB Operation */ > - WARN_ON(typec_altmode_notify(adev, TYPEC_STATE_USB, > - NULL)); > + /* Back to USB Operation */ > + WARN_ON(typec_altmode_notify(adev, > + TYPEC_STATE_USB, > + NULL)); > + } > break; > default: > break; > @@ -1171,8 +1178,10 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt, > switch (cmd) { > case CMD_ENTER_MODE: > /* Back to USB Operation */ > - WARN_ON(typec_altmode_notify(adev, TYPEC_STATE_USB, > - NULL)); > + if (adev) > + WARN_ON(typec_altmode_notify(adev, > + TYPEC_STATE_USB, > + NULL)); > break; > default: > break; > @@ -1183,7 +1192,8 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt, > } > > /* Informing the alternate mode drivers about everything */ > - typec_altmode_vdm(adev, p[0], &p[1], cnt); > + if (adev) > + typec_altmode_vdm(adev, p[0], &p[1], cnt); > > return rlen; > } >
Hi, On Wed, Jul 24, 2019 at 09:30:37PM -0700, Guenter Roeck wrote: > TCPM may receive PD messages associated with unknown or unsupported > alternate modes. If that happens, calls to typec_match_altmode() > will return NULL. The tcpm code does not currently take this into > account. This results in crashes. > > Unable to handle kernel NULL pointer dereference at virtual address 000001f0 > pgd = 41dad9a1 > [000001f0] *pgd=00000000 > Internal error: Oops: 5 [#1] THUMB2 > Modules linked in: tcpci tcpm > CPU: 0 PID: 2338 Comm: kworker/u2:0 Not tainted 5.1.18-sama5-armv7-r2 #6 > Hardware name: Atmel SAMA5 > Workqueue: 2-0050 tcpm_pd_rx_handler [tcpm] > PC is at typec_altmode_attention+0x0/0x14 > LR is at tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm] > ... > [<c03fbee8>] (typec_altmode_attention) from [<bf8030fb>] > (tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm]) > [<bf8030fb>] (tcpm_pd_rx_handler [tcpm]) from [<c012082b>] > (process_one_work+0x123/0x2a8) > [<c012082b>] (process_one_work) from [<c0120a6d>] > (worker_thread+0xbd/0x3b0) > [<c0120a6d>] (worker_thread) from [<c012431f>] (kthread+0xcf/0xf4) > [<c012431f>] (kthread) from [<c01010f9>] (ret_from_fork+0x11/0x38) > > Ignore PD messages if the asociated alternate mode is not supported. > > Reported-by: Douglas Gilbert <dgilbert@interlog.com> > Cc: Douglas Gilbert <dgilbert@interlog.com> > Fixes: e9576fe8e605c ("usb: typec: tcpm: Support for Alternate Modes") > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > Taking a stab at the problem. I don't really know if this is the correct > fix, or even if my understanding of the problem is correct, thus marking > the patch as RFC. My guess is that typec_match_altmode() is the real culprit. We can't rely on the partner mode index number when identifying the port alt mode. Douglas, can you test the attached hack instead of this patch? thanks,
On Mon, Jul 29, 2019 at 05:04:57PM +0300, Heikki Krogerus wrote: > Hi, > > On Wed, Jul 24, 2019 at 09:30:37PM -0700, Guenter Roeck wrote: > > TCPM may receive PD messages associated with unknown or unsupported > > alternate modes. If that happens, calls to typec_match_altmode() > > will return NULL. The tcpm code does not currently take this into > > account. This results in crashes. > > > > Unable to handle kernel NULL pointer dereference at virtual address 000001f0 > > pgd = 41dad9a1 > > [000001f0] *pgd=00000000 > > Internal error: Oops: 5 [#1] THUMB2 > > Modules linked in: tcpci tcpm > > CPU: 0 PID: 2338 Comm: kworker/u2:0 Not tainted 5.1.18-sama5-armv7-r2 #6 > > Hardware name: Atmel SAMA5 > > Workqueue: 2-0050 tcpm_pd_rx_handler [tcpm] > > PC is at typec_altmode_attention+0x0/0x14 > > LR is at tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm] > > ... > > [<c03fbee8>] (typec_altmode_attention) from [<bf8030fb>] > > (tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm]) > > [<bf8030fb>] (tcpm_pd_rx_handler [tcpm]) from [<c012082b>] > > (process_one_work+0x123/0x2a8) > > [<c012082b>] (process_one_work) from [<c0120a6d>] > > (worker_thread+0xbd/0x3b0) > > [<c0120a6d>] (worker_thread) from [<c012431f>] (kthread+0xcf/0xf4) > > [<c012431f>] (kthread) from [<c01010f9>] (ret_from_fork+0x11/0x38) > > > > Ignore PD messages if the asociated alternate mode is not supported. > > > > Reported-by: Douglas Gilbert <dgilbert@interlog.com> > > Cc: Douglas Gilbert <dgilbert@interlog.com> > > Fixes: e9576fe8e605c ("usb: typec: tcpm: Support for Alternate Modes") > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > > --- > > Taking a stab at the problem. I don't really know if this is the correct > > fix, or even if my understanding of the problem is correct, thus marking > > the patch as RFC. > > My guess is that typec_match_altmode() is the real culprit. We can't > rely on the partner mode index number when identifying the port alt > mode. > > Douglas, can you test the attached hack instead of this patch? > > > thanks, > > -- > heikki > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > index ec525811a9eb..033dc097ba83 100644 > --- a/drivers/usb/typec/tcpm/tcpm.c > +++ b/drivers/usb/typec/tcpm/tcpm.c > @@ -1067,12 +1067,11 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt, > > modep = &port->mode_data; > > - adev = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX, > - PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0])); > - > pdev = typec_match_altmode(port->partner_altmode, ALTMODE_DISCOVERY_MAX, > PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0])); > > + adev = (void *)typec_altmode_get_partner(pdev); > + I understand that typec_altmode_get_partner() returns a const *; maybe adev should be declared as const struct typec_altmode * instead of using a typecast. Also, typec_altmode_get_partner() can return NULL as well if pdev is NULL. Is it guaranteed that typec_match_altmode() never returns NULL for pdev ? Thanks, Guenter > switch (cmd_type) { > case CMDT_INIT: > switch (cmd) {
On Mon, Jul 29, 2019 at 10:31:04AM -0700, Guenter Roeck wrote: > On Mon, Jul 29, 2019 at 05:04:57PM +0300, Heikki Krogerus wrote: > > Hi, > > > > On Wed, Jul 24, 2019 at 09:30:37PM -0700, Guenter Roeck wrote: > > > TCPM may receive PD messages associated with unknown or unsupported > > > alternate modes. If that happens, calls to typec_match_altmode() > > > will return NULL. The tcpm code does not currently take this into > > > account. This results in crashes. > > > > > > Unable to handle kernel NULL pointer dereference at virtual address 000001f0 > > > pgd = 41dad9a1 > > > [000001f0] *pgd=00000000 > > > Internal error: Oops: 5 [#1] THUMB2 > > > Modules linked in: tcpci tcpm > > > CPU: 0 PID: 2338 Comm: kworker/u2:0 Not tainted 5.1.18-sama5-armv7-r2 #6 > > > Hardware name: Atmel SAMA5 > > > Workqueue: 2-0050 tcpm_pd_rx_handler [tcpm] > > > PC is at typec_altmode_attention+0x0/0x14 > > > LR is at tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm] > > > ... > > > [<c03fbee8>] (typec_altmode_attention) from [<bf8030fb>] > > > (tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm]) > > > [<bf8030fb>] (tcpm_pd_rx_handler [tcpm]) from [<c012082b>] > > > (process_one_work+0x123/0x2a8) > > > [<c012082b>] (process_one_work) from [<c0120a6d>] > > > (worker_thread+0xbd/0x3b0) > > > [<c0120a6d>] (worker_thread) from [<c012431f>] (kthread+0xcf/0xf4) > > > [<c012431f>] (kthread) from [<c01010f9>] (ret_from_fork+0x11/0x38) > > > > > > Ignore PD messages if the asociated alternate mode is not supported. > > > > > > Reported-by: Douglas Gilbert <dgilbert@interlog.com> > > > Cc: Douglas Gilbert <dgilbert@interlog.com> > > > Fixes: e9576fe8e605c ("usb: typec: tcpm: Support for Alternate Modes") > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > > > --- > > > Taking a stab at the problem. I don't really know if this is the correct > > > fix, or even if my understanding of the problem is correct, thus marking > > > the patch as RFC. > > > > My guess is that typec_match_altmode() is the real culprit. We can't > > rely on the partner mode index number when identifying the port alt > > mode. > > > > Douglas, can you test the attached hack instead of this patch? > > > > > > thanks, > > > > -- > > heikki > > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > > index ec525811a9eb..033dc097ba83 100644 > > --- a/drivers/usb/typec/tcpm/tcpm.c > > +++ b/drivers/usb/typec/tcpm/tcpm.c > > @@ -1067,12 +1067,11 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt, > > > > modep = &port->mode_data; > > > > - adev = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX, > > - PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0])); > > - > > pdev = typec_match_altmode(port->partner_altmode, ALTMODE_DISCOVERY_MAX, > > PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0])); > > > > + adev = (void *)typec_altmode_get_partner(pdev); > > + > > I understand that typec_altmode_get_partner() returns a const *; > maybe adev should be declared as const struct typec_altmode * > instead of using a typecast. Yes... > Also, typec_altmode_get_partner() can return NULL as well if pdev is NULL. > Is it guaranteed that typec_match_altmode() never returns NULL for pdev ? ...and probable no. But I don't think we can receive Attention to a mode that hasn't been entered. I'm not proposing that as a patch. It's just a hunch. That's why I'm calling it a "hack". Before we prepare anything finalized, I would like to here from Douglas if he's able to test that or not? thanks,
On 2019-07-30 2:07 p.m., Heikki Krogerus wrote: > On Mon, Jul 29, 2019 at 10:31:04AM -0700, Guenter Roeck wrote: >> On Mon, Jul 29, 2019 at 05:04:57PM +0300, Heikki Krogerus wrote: >>> Hi, >>> >>> On Wed, Jul 24, 2019 at 09:30:37PM -0700, Guenter Roeck wrote: >>>> TCPM may receive PD messages associated with unknown or unsupported >>>> alternate modes. If that happens, calls to typec_match_altmode() >>>> will return NULL. The tcpm code does not currently take this into >>>> account. This results in crashes. >>>> >>>> Unable to handle kernel NULL pointer dereference at virtual address 000001f0 >>>> pgd = 41dad9a1 >>>> [000001f0] *pgd=00000000 >>>> Internal error: Oops: 5 [#1] THUMB2 >>>> Modules linked in: tcpci tcpm >>>> CPU: 0 PID: 2338 Comm: kworker/u2:0 Not tainted 5.1.18-sama5-armv7-r2 #6 >>>> Hardware name: Atmel SAMA5 >>>> Workqueue: 2-0050 tcpm_pd_rx_handler [tcpm] >>>> PC is at typec_altmode_attention+0x0/0x14 >>>> LR is at tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm] >>>> ... >>>> [<c03fbee8>] (typec_altmode_attention) from [<bf8030fb>] >>>> (tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm]) >>>> [<bf8030fb>] (tcpm_pd_rx_handler [tcpm]) from [<c012082b>] >>>> (process_one_work+0x123/0x2a8) >>>> [<c012082b>] (process_one_work) from [<c0120a6d>] >>>> (worker_thread+0xbd/0x3b0) >>>> [<c0120a6d>] (worker_thread) from [<c012431f>] (kthread+0xcf/0xf4) >>>> [<c012431f>] (kthread) from [<c01010f9>] (ret_from_fork+0x11/0x38) >>>> >>>> Ignore PD messages if the asociated alternate mode is not supported. >>>> >>>> Reported-by: Douglas Gilbert <dgilbert@interlog.com> >>>> Cc: Douglas Gilbert <dgilbert@interlog.com> >>>> Fixes: e9576fe8e605c ("usb: typec: tcpm: Support for Alternate Modes") >>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >>>> --- >>>> Taking a stab at the problem. I don't really know if this is the correct >>>> fix, or even if my understanding of the problem is correct, thus marking >>>> the patch as RFC. >>> >>> My guess is that typec_match_altmode() is the real culprit. We can't >>> rely on the partner mode index number when identifying the port alt >>> mode. >>> >>> Douglas, can you test the attached hack instead of this patch? >>> >>> >>> thanks, >>> >>> -- >>> heikki >> >>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c >>> index ec525811a9eb..033dc097ba83 100644 >>> --- a/drivers/usb/typec/tcpm/tcpm.c >>> +++ b/drivers/usb/typec/tcpm/tcpm.c >>> @@ -1067,12 +1067,11 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt, >>> >>> modep = &port->mode_data; >>> >>> - adev = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX, >>> - PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0])); >>> - >>> pdev = typec_match_altmode(port->partner_altmode, ALTMODE_DISCOVERY_MAX, >>> PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0])); >>> >>> + adev = (void *)typec_altmode_get_partner(pdev); >>> + >> >> I understand that typec_altmode_get_partner() returns a const *; >> maybe adev should be declared as const struct typec_altmode * >> instead of using a typecast. > > Yes... > >> Also, typec_altmode_get_partner() can return NULL as well if pdev is NULL. >> Is it guaranteed that typec_match_altmode() never returns NULL for pdev ? > > ...and probable no. But I don't think we can receive Attention to a > mode that hasn't been entered. > > I'm not proposing that as a patch. It's just a hunch. That's why I'm > calling it a "hack". Before we prepare anything finalized, I would > like to here from Douglas if he's able to test that or not? Hi, I'm an ocean away from my test rig at the moment, won't be back home till after August 10. Maybe I can set up something here as I have one OM13588. Will try later today. Also, switching from an sama5d2_xplained to an Acme Systems Arietta which is at91sam9g25 based to run Linux. So it won't be exactly the same hardware. Doug Gilbert
On 7/30/19 5:07 AM, Heikki Krogerus wrote: > On Mon, Jul 29, 2019 at 10:31:04AM -0700, Guenter Roeck wrote: >> On Mon, Jul 29, 2019 at 05:04:57PM +0300, Heikki Krogerus wrote: >>> Hi, >>> >>> On Wed, Jul 24, 2019 at 09:30:37PM -0700, Guenter Roeck wrote: >>>> TCPM may receive PD messages associated with unknown or unsupported >>>> alternate modes. If that happens, calls to typec_match_altmode() >>>> will return NULL. The tcpm code does not currently take this into >>>> account. This results in crashes. >>>> >>>> Unable to handle kernel NULL pointer dereference at virtual address 000001f0 >>>> pgd = 41dad9a1 >>>> [000001f0] *pgd=00000000 >>>> Internal error: Oops: 5 [#1] THUMB2 >>>> Modules linked in: tcpci tcpm >>>> CPU: 0 PID: 2338 Comm: kworker/u2:0 Not tainted 5.1.18-sama5-armv7-r2 #6 >>>> Hardware name: Atmel SAMA5 >>>> Workqueue: 2-0050 tcpm_pd_rx_handler [tcpm] >>>> PC is at typec_altmode_attention+0x0/0x14 >>>> LR is at tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm] >>>> ... >>>> [<c03fbee8>] (typec_altmode_attention) from [<bf8030fb>] >>>> (tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm]) >>>> [<bf8030fb>] (tcpm_pd_rx_handler [tcpm]) from [<c012082b>] >>>> (process_one_work+0x123/0x2a8) >>>> [<c012082b>] (process_one_work) from [<c0120a6d>] >>>> (worker_thread+0xbd/0x3b0) >>>> [<c0120a6d>] (worker_thread) from [<c012431f>] (kthread+0xcf/0xf4) >>>> [<c012431f>] (kthread) from [<c01010f9>] (ret_from_fork+0x11/0x38) >>>> >>>> Ignore PD messages if the asociated alternate mode is not supported. >>>> >>>> Reported-by: Douglas Gilbert <dgilbert@interlog.com> >>>> Cc: Douglas Gilbert <dgilbert@interlog.com> >>>> Fixes: e9576fe8e605c ("usb: typec: tcpm: Support for Alternate Modes") >>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >>>> --- >>>> Taking a stab at the problem. I don't really know if this is the correct >>>> fix, or even if my understanding of the problem is correct, thus marking >>>> the patch as RFC. >>> >>> My guess is that typec_match_altmode() is the real culprit. We can't >>> rely on the partner mode index number when identifying the port alt >>> mode. >>> >>> Douglas, can you test the attached hack instead of this patch? >>> >>> >>> thanks, >>> >>> -- >>> heikki >> >>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c >>> index ec525811a9eb..033dc097ba83 100644 >>> --- a/drivers/usb/typec/tcpm/tcpm.c >>> +++ b/drivers/usb/typec/tcpm/tcpm.c >>> @@ -1067,12 +1067,11 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt, >>> >>> modep = &port->mode_data; >>> >>> - adev = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX, >>> - PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0])); >>> - >>> pdev = typec_match_altmode(port->partner_altmode, ALTMODE_DISCOVERY_MAX, >>> PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0])); >>> >>> + adev = (void *)typec_altmode_get_partner(pdev); >>> + >> >> I understand that typec_altmode_get_partner() returns a const *; >> maybe adev should be declared as const struct typec_altmode * >> instead of using a typecast. > > Yes... > >> Also, typec_altmode_get_partner() can return NULL as well if pdev is NULL. >> Is it guaranteed that typec_match_altmode() never returns NULL for pdev ? > > ...and probable no. But I don't think we can receive Attention to a > mode that hasn't been entered. > If I understand correctly, the Attention was generated by a test system. What prevents badly implemented code in the connected system from sending such an Attention message ? Thanks, Guenter
On Tue, Jul 30, 2019 at 06:28:52AM -0700, Guenter Roeck wrote: > On 7/30/19 5:07 AM, Heikki Krogerus wrote: > > On Mon, Jul 29, 2019 at 10:31:04AM -0700, Guenter Roeck wrote: > > > On Mon, Jul 29, 2019 at 05:04:57PM +0300, Heikki Krogerus wrote: > > > > Hi, > > > > > > > > On Wed, Jul 24, 2019 at 09:30:37PM -0700, Guenter Roeck wrote: > > > > > TCPM may receive PD messages associated with unknown or unsupported > > > > > alternate modes. If that happens, calls to typec_match_altmode() > > > > > will return NULL. The tcpm code does not currently take this into > > > > > account. This results in crashes. > > > > > > > > > > Unable to handle kernel NULL pointer dereference at virtual address 000001f0 > > > > > pgd = 41dad9a1 > > > > > [000001f0] *pgd=00000000 > > > > > Internal error: Oops: 5 [#1] THUMB2 > > > > > Modules linked in: tcpci tcpm > > > > > CPU: 0 PID: 2338 Comm: kworker/u2:0 Not tainted 5.1.18-sama5-armv7-r2 #6 > > > > > Hardware name: Atmel SAMA5 > > > > > Workqueue: 2-0050 tcpm_pd_rx_handler [tcpm] > > > > > PC is at typec_altmode_attention+0x0/0x14 > > > > > LR is at tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm] > > > > > ... > > > > > [<c03fbee8>] (typec_altmode_attention) from [<bf8030fb>] > > > > > (tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm]) > > > > > [<bf8030fb>] (tcpm_pd_rx_handler [tcpm]) from [<c012082b>] > > > > > (process_one_work+0x123/0x2a8) > > > > > [<c012082b>] (process_one_work) from [<c0120a6d>] > > > > > (worker_thread+0xbd/0x3b0) > > > > > [<c0120a6d>] (worker_thread) from [<c012431f>] (kthread+0xcf/0xf4) > > > > > [<c012431f>] (kthread) from [<c01010f9>] (ret_from_fork+0x11/0x38) > > > > > > > > > > Ignore PD messages if the asociated alternate mode is not supported. > > > > > > > > > > Reported-by: Douglas Gilbert <dgilbert@interlog.com> > > > > > Cc: Douglas Gilbert <dgilbert@interlog.com> > > > > > Fixes: e9576fe8e605c ("usb: typec: tcpm: Support for Alternate Modes") > > > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > > > > > --- > > > > > Taking a stab at the problem. I don't really know if this is the correct > > > > > fix, or even if my understanding of the problem is correct, thus marking > > > > > the patch as RFC. > > > > > > > > My guess is that typec_match_altmode() is the real culprit. We can't > > > > rely on the partner mode index number when identifying the port alt > > > > mode. > > > > > > > > Douglas, can you test the attached hack instead of this patch? > > > > > > > > > > > > thanks, > > > > > > > > -- > > > > heikki > > > > > > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > > > > index ec525811a9eb..033dc097ba83 100644 > > > > --- a/drivers/usb/typec/tcpm/tcpm.c > > > > +++ b/drivers/usb/typec/tcpm/tcpm.c > > > > @@ -1067,12 +1067,11 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt, > > > > modep = &port->mode_data; > > > > - adev = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX, > > > > - PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0])); > > > > - > > > > pdev = typec_match_altmode(port->partner_altmode, ALTMODE_DISCOVERY_MAX, > > > > PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0])); > > > > + adev = (void *)typec_altmode_get_partner(pdev); > > > > + > > > > > > I understand that typec_altmode_get_partner() returns a const *; > > > maybe adev should be declared as const struct typec_altmode * > > > instead of using a typecast. > > > > Yes... > > > > > Also, typec_altmode_get_partner() can return NULL as well if pdev is NULL. > > > Is it guaranteed that typec_match_altmode() never returns NULL for pdev ? > > > > ...and probable no. But I don't think we can receive Attention to a > > mode that hasn't been entered. > > > > If I understand correctly, the Attention was generated by a test system. > What prevents badly implemented code in the connected system from sending > such an Attention message ? Oh, if that is the case, then I don't think my change has any effect. I misunderstood the scenario. Sorry for that. I think we should use your patch to fix this issue. thanks,
On Wed, Jul 31, 2019 at 12:55:55PM +0300, Heikki Krogerus wrote: > On Tue, Jul 30, 2019 at 06:28:52AM -0700, Guenter Roeck wrote: > > On 7/30/19 5:07 AM, Heikki Krogerus wrote: > > > On Mon, Jul 29, 2019 at 10:31:04AM -0700, Guenter Roeck wrote: > > > > On Mon, Jul 29, 2019 at 05:04:57PM +0300, Heikki Krogerus wrote: > > > > > Hi, > > > > > > > > > > On Wed, Jul 24, 2019 at 09:30:37PM -0700, Guenter Roeck wrote: > > > > > > TCPM may receive PD messages associated with unknown or unsupported > > > > > > alternate modes. If that happens, calls to typec_match_altmode() > > > > > > will return NULL. The tcpm code does not currently take this into > > > > > > account. This results in crashes. > > > > > > > > > > > > Unable to handle kernel NULL pointer dereference at virtual address 000001f0 > > > > > > pgd = 41dad9a1 > > > > > > [000001f0] *pgd=00000000 > > > > > > Internal error: Oops: 5 [#1] THUMB2 > > > > > > Modules linked in: tcpci tcpm > > > > > > CPU: 0 PID: 2338 Comm: kworker/u2:0 Not tainted 5.1.18-sama5-armv7-r2 #6 > > > > > > Hardware name: Atmel SAMA5 > > > > > > Workqueue: 2-0050 tcpm_pd_rx_handler [tcpm] > > > > > > PC is at typec_altmode_attention+0x0/0x14 > > > > > > LR is at tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm] > > > > > > ... > > > > > > [<c03fbee8>] (typec_altmode_attention) from [<bf8030fb>] > > > > > > (tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm]) > > > > > > [<bf8030fb>] (tcpm_pd_rx_handler [tcpm]) from [<c012082b>] > > > > > > (process_one_work+0x123/0x2a8) > > > > > > [<c012082b>] (process_one_work) from [<c0120a6d>] > > > > > > (worker_thread+0xbd/0x3b0) > > > > > > [<c0120a6d>] (worker_thread) from [<c012431f>] (kthread+0xcf/0xf4) > > > > > > [<c012431f>] (kthread) from [<c01010f9>] (ret_from_fork+0x11/0x38) > > > > > > > > > > > > Ignore PD messages if the asociated alternate mode is not supported. > > > > > > > > > > > > Reported-by: Douglas Gilbert <dgilbert@interlog.com> > > > > > > Cc: Douglas Gilbert <dgilbert@interlog.com> > > > > > > Fixes: e9576fe8e605c ("usb: typec: tcpm: Support for Alternate Modes") > > > > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > > > > > > --- > > > > > > Taking a stab at the problem. I don't really know if this is the correct > > > > > > fix, or even if my understanding of the problem is correct, thus marking > > > > > > the patch as RFC. > > > > > > > > > > My guess is that typec_match_altmode() is the real culprit. We can't > > > > > rely on the partner mode index number when identifying the port alt > > > > > mode. > > > > > > > > > > Douglas, can you test the attached hack instead of this patch? > > > > > > > > > > > > > > > thanks, > > > > > > > > > > -- > > > > > heikki > > > > > > > > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > > > > > index ec525811a9eb..033dc097ba83 100644 > > > > > --- a/drivers/usb/typec/tcpm/tcpm.c > > > > > +++ b/drivers/usb/typec/tcpm/tcpm.c > > > > > @@ -1067,12 +1067,11 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt, > > > > > modep = &port->mode_data; > > > > > - adev = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX, > > > > > - PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0])); > > > > > - > > > > > pdev = typec_match_altmode(port->partner_altmode, ALTMODE_DISCOVERY_MAX, > > > > > PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0])); > > > > > + adev = (void *)typec_altmode_get_partner(pdev); > > > > > + > > > > > > > > I understand that typec_altmode_get_partner() returns a const *; > > > > maybe adev should be declared as const struct typec_altmode * > > > > instead of using a typecast. > > > > > > Yes... > > > > > > > Also, typec_altmode_get_partner() can return NULL as well if pdev is NULL. > > > > Is it guaranteed that typec_match_altmode() never returns NULL for pdev ? > > > > > > ...and probable no. But I don't think we can receive Attention to a > > > mode that hasn't been entered. > > > > > > > If I understand correctly, the Attention was generated by a test system. > > What prevents badly implemented code in the connected system from sending > > such an Attention message ? > > Oh, if that is the case, then I don't think my change has any effect. > I misunderstood the scenario. Sorry for that. > > I think we should use your patch to fix this issue. So is this an "ack"? I'm confused as to if this patch should be applied or not... thanks, greg k-h
On Thu, Aug 01, 2019 at 06:07:09PM +0200, Greg Kroah-Hartman wrote: > On Wed, Jul 31, 2019 at 12:55:55PM +0300, Heikki Krogerus wrote: > > On Tue, Jul 30, 2019 at 06:28:52AM -0700, Guenter Roeck wrote: > > > On 7/30/19 5:07 AM, Heikki Krogerus wrote: > > > > On Mon, Jul 29, 2019 at 10:31:04AM -0700, Guenter Roeck wrote: > > > > > On Mon, Jul 29, 2019 at 05:04:57PM +0300, Heikki Krogerus wrote: > > > > > > Hi, > > > > > > > > > > > > On Wed, Jul 24, 2019 at 09:30:37PM -0700, Guenter Roeck wrote: > > > > > > > TCPM may receive PD messages associated with unknown or unsupported > > > > > > > alternate modes. If that happens, calls to typec_match_altmode() > > > > > > > will return NULL. The tcpm code does not currently take this into > > > > > > > account. This results in crashes. > > > > > > > > > > > > > > Unable to handle kernel NULL pointer dereference at virtual address 000001f0 > > > > > > > pgd = 41dad9a1 > > > > > > > [000001f0] *pgd=00000000 > > > > > > > Internal error: Oops: 5 [#1] THUMB2 > > > > > > > Modules linked in: tcpci tcpm > > > > > > > CPU: 0 PID: 2338 Comm: kworker/u2:0 Not tainted 5.1.18-sama5-armv7-r2 #6 > > > > > > > Hardware name: Atmel SAMA5 > > > > > > > Workqueue: 2-0050 tcpm_pd_rx_handler [tcpm] > > > > > > > PC is at typec_altmode_attention+0x0/0x14 > > > > > > > LR is at tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm] > > > > > > > ... > > > > > > > [<c03fbee8>] (typec_altmode_attention) from [<bf8030fb>] > > > > > > > (tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm]) > > > > > > > [<bf8030fb>] (tcpm_pd_rx_handler [tcpm]) from [<c012082b>] > > > > > > > (process_one_work+0x123/0x2a8) > > > > > > > [<c012082b>] (process_one_work) from [<c0120a6d>] > > > > > > > (worker_thread+0xbd/0x3b0) > > > > > > > [<c0120a6d>] (worker_thread) from [<c012431f>] (kthread+0xcf/0xf4) > > > > > > > [<c012431f>] (kthread) from [<c01010f9>] (ret_from_fork+0x11/0x38) > > > > > > > > > > > > > > Ignore PD messages if the asociated alternate mode is not supported. > > > > > > > > > > > > > > Reported-by: Douglas Gilbert <dgilbert@interlog.com> > > > > > > > Cc: Douglas Gilbert <dgilbert@interlog.com> > > > > > > > Fixes: e9576fe8e605c ("usb: typec: tcpm: Support for Alternate Modes") > > > > > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > > > > > > > --- > > > > > > > Taking a stab at the problem. I don't really know if this is the correct > > > > > > > fix, or even if my understanding of the problem is correct, thus marking > > > > > > > the patch as RFC. > > > > > > > > > > > > My guess is that typec_match_altmode() is the real culprit. We can't > > > > > > rely on the partner mode index number when identifying the port alt > > > > > > mode. > > > > > > > > > > > > Douglas, can you test the attached hack instead of this patch? > > > > > > > > > > > > > > > > > > thanks, > > > > > > > > > > > > -- > > > > > > heikki > > > > > > > > > > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > > > > > > index ec525811a9eb..033dc097ba83 100644 > > > > > > --- a/drivers/usb/typec/tcpm/tcpm.c > > > > > > +++ b/drivers/usb/typec/tcpm/tcpm.c > > > > > > @@ -1067,12 +1067,11 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt, > > > > > > modep = &port->mode_data; > > > > > > - adev = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX, > > > > > > - PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0])); > > > > > > - > > > > > > pdev = typec_match_altmode(port->partner_altmode, ALTMODE_DISCOVERY_MAX, > > > > > > PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0])); > > > > > > + adev = (void *)typec_altmode_get_partner(pdev); > > > > > > + > > > > > > > > > > I understand that typec_altmode_get_partner() returns a const *; > > > > > maybe adev should be declared as const struct typec_altmode * > > > > > instead of using a typecast. > > > > > > > > Yes... > > > > > > > > > Also, typec_altmode_get_partner() can return NULL as well if pdev is NULL. > > > > > Is it guaranteed that typec_match_altmode() never returns NULL for pdev ? > > > > > > > > ...and probable no. But I don't think we can receive Attention to a > > > > mode that hasn't been entered. > > > > > > > > > > If I understand correctly, the Attention was generated by a test system. > > > What prevents badly implemented code in the connected system from sending > > > such an Attention message ? > > > > Oh, if that is the case, then I don't think my change has any effect. > > I misunderstood the scenario. Sorry for that. > > > > I think we should use your patch to fix this issue. > > So is this an "ack"? I'm confused as to if this patch should be applied > or not... I meant to ack it, sorry. Here: Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
On Wed, Jul 24, 2019 at 09:30:37PM -0700, Guenter Roeck wrote: > TCPM may receive PD messages associated with unknown or unsupported > alternate modes. If that happens, calls to typec_match_altmode() > will return NULL. The tcpm code does not currently take this into > account. This results in crashes. > > Unable to handle kernel NULL pointer dereference at virtual address 000001f0 > pgd = 41dad9a1 > [000001f0] *pgd=00000000 > Internal error: Oops: 5 [#1] THUMB2 > Modules linked in: tcpci tcpm > CPU: 0 PID: 2338 Comm: kworker/u2:0 Not tainted 5.1.18-sama5-armv7-r2 #6 > Hardware name: Atmel SAMA5 > Workqueue: 2-0050 tcpm_pd_rx_handler [tcpm] > PC is at typec_altmode_attention+0x0/0x14 > LR is at tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm] > ... > [<c03fbee8>] (typec_altmode_attention) from [<bf8030fb>] > (tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm]) > [<bf8030fb>] (tcpm_pd_rx_handler [tcpm]) from [<c012082b>] > (process_one_work+0x123/0x2a8) > [<c012082b>] (process_one_work) from [<c0120a6d>] > (worker_thread+0xbd/0x3b0) > [<c0120a6d>] (worker_thread) from [<c012431f>] (kthread+0xcf/0xf4) > [<c012431f>] (kthread) from [<c01010f9>] (ret_from_fork+0x11/0x38) > > Ignore PD messages if the asociated alternate mode is not supported. > > Reported-by: Douglas Gilbert <dgilbert@interlog.com> > Cc: Douglas Gilbert <dgilbert@interlog.com> > Fixes: e9576fe8e605c ("usb: typec: tcpm: Support for Alternate Modes") > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > Taking a stab at the problem. I don't really know if this is the correct > fix, or even if my understanding of the problem is correct, thus marking > the patch as RFC. Now that I think everyone agrees this is a "real" fix, can you resend it in a format that I can apply it in, with the various acks added? thanks, greg k-h
On 2019-07-25 6:30 a.m., Guenter Roeck wrote: > TCPM may receive PD messages associated with unknown or unsupported > alternate modes. If that happens, calls to typec_match_altmode() > will return NULL. The tcpm code does not currently take this into > account. This results in crashes. > > Unable to handle kernel NULL pointer dereference at virtual address 000001f0 > pgd = 41dad9a1 > [000001f0] *pgd=00000000 > Internal error: Oops: 5 [#1] THUMB2 > Modules linked in: tcpci tcpm > CPU: 0 PID: 2338 Comm: kworker/u2:0 Not tainted 5.1.18-sama5-armv7-r2 #6 > Hardware name: Atmel SAMA5 > Workqueue: 2-0050 tcpm_pd_rx_handler [tcpm] > PC is at typec_altmode_attention+0x0/0x14 > LR is at tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm] > ... > [<c03fbee8>] (typec_altmode_attention) from [<bf8030fb>] > (tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm]) > [<bf8030fb>] (tcpm_pd_rx_handler [tcpm]) from [<c012082b>] > (process_one_work+0x123/0x2a8) > [<c012082b>] (process_one_work) from [<c0120a6d>] > (worker_thread+0xbd/0x3b0) > [<c0120a6d>] (worker_thread) from [<c012431f>] (kthread+0xcf/0xf4) > [<c012431f>] (kthread) from [<c01010f9>] (ret_from_fork+0x11/0x38) > > Ignore PD messages if the asociated alternate mode is not supported. > > Reported-by: Douglas Gilbert <dgilbert@interlog.com> > Cc: Douglas Gilbert <dgilbert@interlog.com> > Fixes: e9576fe8e605c ("usb: typec: tcpm: Support for Alternate Modes") > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > Taking a stab at the problem. I don't really know if this is the correct > fix, or even if my understanding of the problem is correct, thus marking > the patch as RFC. Tested-by: Douglas Gilbert <dgilbert@interlog.com> Tested on different hardware (Acme Arietta based on at91sam9g25) and lk 5.2.5 . Without this patch the oops was at PC is at typec_altmode_attention+0x0/0x24 [typec] The DUT was a Arietta with a OM13588 attached acting as a PD sink. At the other end of the type C cable was a KL27Z+OM13588 running NXP's usb_pd_bm app and it was a PD source. The other end did a "send attention" to cause the oops. With this patch nothing happened. Notable there was no oops. The other end (KL27Z+OM13588 reported "send attention result: success" but it also did that in the previous "oops" case. Is there something to replace "tail -f" that would make it easier to monitor what is being sent to /sys/kernel/debug/tcpm/1-0050 ? Doug Gilbert > drivers/usb/typec/tcpm/tcpm.c | 38 ++++++++++++++++++++++++-------------- > 1 file changed, 24 insertions(+), 14 deletions(-) > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > index 77f71f602f73..1df2844469aa 100644 > --- a/drivers/usb/typec/tcpm/tcpm.c > +++ b/drivers/usb/typec/tcpm/tcpm.c > @@ -1096,7 +1096,8 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt, > break; > case CMD_ATTENTION: > /* Attention command does not have response */ > - typec_altmode_attention(adev, p[1]); > + if (adev) > + typec_altmode_attention(adev, p[1]); > return 0; > default: > break; > @@ -1148,20 +1149,26 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt, > } > break; > case CMD_ENTER_MODE: > - typec_altmode_update_active(pdev, true); > - > - if (typec_altmode_vdm(adev, p[0], &p[1], cnt)) { > - response[0] = VDO(adev->svid, 1, CMD_EXIT_MODE); > - response[0] |= VDO_OPOS(adev->mode); > - return 1; > + if (adev && pdev) { > + typec_altmode_update_active(pdev, true); > + > + if (typec_altmode_vdm(adev, p[0], &p[1], cnt)) { > + response[0] = VDO(adev->svid, 1, > + CMD_EXIT_MODE); > + response[0] |= VDO_OPOS(adev->mode); > + return 1; > + } > } > return 0; > case CMD_EXIT_MODE: > - typec_altmode_update_active(pdev, false); > + if (adev && pdev) { > + typec_altmode_update_active(pdev, false); > > - /* Back to USB Operation */ > - WARN_ON(typec_altmode_notify(adev, TYPEC_STATE_USB, > - NULL)); > + /* Back to USB Operation */ > + WARN_ON(typec_altmode_notify(adev, > + TYPEC_STATE_USB, > + NULL)); > + } > break; > default: > break; > @@ -1171,8 +1178,10 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt, > switch (cmd) { > case CMD_ENTER_MODE: > /* Back to USB Operation */ > - WARN_ON(typec_altmode_notify(adev, TYPEC_STATE_USB, > - NULL)); > + if (adev) > + WARN_ON(typec_altmode_notify(adev, > + TYPEC_STATE_USB, > + NULL)); > break; > default: > break; > @@ -1183,7 +1192,8 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt, > } > > /* Informing the alternate mode drivers about everything */ > - typec_altmode_vdm(adev, p[0], &p[1], cnt); > + if (adev) > + typec_altmode_vdm(adev, p[0], &p[1], cnt); > > return rlen; > } >
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c index 77f71f602f73..1df2844469aa 100644 --- a/drivers/usb/typec/tcpm/tcpm.c +++ b/drivers/usb/typec/tcpm/tcpm.c @@ -1096,7 +1096,8 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt, break; case CMD_ATTENTION: /* Attention command does not have response */ - typec_altmode_attention(adev, p[1]); + if (adev) + typec_altmode_attention(adev, p[1]); return 0; default: break; @@ -1148,20 +1149,26 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt, } break; case CMD_ENTER_MODE: - typec_altmode_update_active(pdev, true); - - if (typec_altmode_vdm(adev, p[0], &p[1], cnt)) { - response[0] = VDO(adev->svid, 1, CMD_EXIT_MODE); - response[0] |= VDO_OPOS(adev->mode); - return 1; + if (adev && pdev) { + typec_altmode_update_active(pdev, true); + + if (typec_altmode_vdm(adev, p[0], &p[1], cnt)) { + response[0] = VDO(adev->svid, 1, + CMD_EXIT_MODE); + response[0] |= VDO_OPOS(adev->mode); + return 1; + } } return 0; case CMD_EXIT_MODE: - typec_altmode_update_active(pdev, false); + if (adev && pdev) { + typec_altmode_update_active(pdev, false); - /* Back to USB Operation */ - WARN_ON(typec_altmode_notify(adev, TYPEC_STATE_USB, - NULL)); + /* Back to USB Operation */ + WARN_ON(typec_altmode_notify(adev, + TYPEC_STATE_USB, + NULL)); + } break; default: break; @@ -1171,8 +1178,10 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt, switch (cmd) { case CMD_ENTER_MODE: /* Back to USB Operation */ - WARN_ON(typec_altmode_notify(adev, TYPEC_STATE_USB, - NULL)); + if (adev) + WARN_ON(typec_altmode_notify(adev, + TYPEC_STATE_USB, + NULL)); break; default: break; @@ -1183,7 +1192,8 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt, } /* Informing the alternate mode drivers about everything */ - typec_altmode_vdm(adev, p[0], &p[1], cnt); + if (adev) + typec_altmode_vdm(adev, p[0], &p[1], cnt); return rlen; }
TCPM may receive PD messages associated with unknown or unsupported alternate modes. If that happens, calls to typec_match_altmode() will return NULL. The tcpm code does not currently take this into account. This results in crashes. Unable to handle kernel NULL pointer dereference at virtual address 000001f0 pgd = 41dad9a1 [000001f0] *pgd=00000000 Internal error: Oops: 5 [#1] THUMB2 Modules linked in: tcpci tcpm CPU: 0 PID: 2338 Comm: kworker/u2:0 Not tainted 5.1.18-sama5-armv7-r2 #6 Hardware name: Atmel SAMA5 Workqueue: 2-0050 tcpm_pd_rx_handler [tcpm] PC is at typec_altmode_attention+0x0/0x14 LR is at tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm] ... [<c03fbee8>] (typec_altmode_attention) from [<bf8030fb>] (tcpm_pd_rx_handler+0xa3b/0xda0 [tcpm]) [<bf8030fb>] (tcpm_pd_rx_handler [tcpm]) from [<c012082b>] (process_one_work+0x123/0x2a8) [<c012082b>] (process_one_work) from [<c0120a6d>] (worker_thread+0xbd/0x3b0) [<c0120a6d>] (worker_thread) from [<c012431f>] (kthread+0xcf/0xf4) [<c012431f>] (kthread) from [<c01010f9>] (ret_from_fork+0x11/0x38) Ignore PD messages if the asociated alternate mode is not supported. Reported-by: Douglas Gilbert <dgilbert@interlog.com> Cc: Douglas Gilbert <dgilbert@interlog.com> Fixes: e9576fe8e605c ("usb: typec: tcpm: Support for Alternate Modes") Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- Taking a stab at the problem. I don't really know if this is the correct fix, or even if my understanding of the problem is correct, thus marking the patch as RFC. drivers/usb/typec/tcpm/tcpm.c | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-)