[v2,0/4] 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 V2:
- Checks for error response from ucsi_register_displayport when
registering DisplayPort alternate mode.

Abhishek Pandit-Subedi (2):
  usb: typec: ucsi: Fix null deref 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            | 21 ++++++++++++++++-----
 drivers/usb/typec/ucsi/ucsi.h            |  2 +-
 include/linux/usb/typec.h                |  3 +++
 6 files changed, 39 insertions(+), 9 deletions(-)


base-commit: 0d31ea587709216d88183fe4ca0c8aba5e0205b8

On Tue, Apr 23, 2024 at 7:06 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Wed, 24 Apr 2024 at 04:48, Jameson Thies <jthies@google.com> wrote:
> >
> > From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> >
> > ucsi_register_altmode checks IS_ERR on returned pointer and treats
> > NULL as valid. This results in a null deref when
> > trace_ucsi_register_altmode is called. Return an error from
> > ucsi_register_displayport when it is not supported and register the
> > altmode with typec_port_register_altmode.
> >
> > Reviewed-by: Jameson Thies <jthies@google.com>
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > ---
> > Changes in V2:
> > - Checks for error response from ucsi_register_displayport when
> > registering DisplayPort alternate mode.
> >
> >  drivers/usb/typec/ucsi/ucsi.c | 3 +++
> >  drivers/usb/typec/ucsi/ucsi.h | 2 +-
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > index cb52e7b0a2c5c..f3b413f94fd28 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > @@ -361,6 +361,9 @@ static int ucsi_register_altmode(struct ucsi_connector *con,
> >                 switch (desc->svid) {
> >                 case USB_TYPEC_DP_SID:
> >                         alt = ucsi_register_displayport(con, override, i, desc);
> > +                       if (IS_ERR(alt) && PTR_ERR(alt) == -EOPNOTSUPP)
>
> This makes it ignore EOPNOTSUPP if it is returned by the non-stub
> implementation. I think the current state is actually better than the
> implementation found in this patch. I'd suggest adding a comment to
> ucsi_register_displayport() stub instead.

So originally on my system, I didn't have the displayport driver
config enabled. My expectation was that the alt-mode would show up but
would not be controllable (like all other alt-modes without drivers).
What ends up happening is that no alt-mode shows up and trying to
enable the trace crashes.

When the displayport support isn't there, I think it should just be
enumerated as a normal, unsupported alt-mode.



>
> > +                               alt = typec_port_register_altmode(con->port, desc);
> > +
> >                         break;
> >                 case USB_TYPEC_NVIDIA_VLINK_SID:
> >                         if (desc->vdo == USB_TYPEC_NVIDIA_VLINK_DBG_VDO)
> > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> > index c4d103db9d0f8..c663dce0659ee 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.h
> > +++ b/drivers/usb/typec/ucsi/ucsi.h
> > @@ -496,7 +496,7 @@ ucsi_register_displayport(struct ucsi_connector *con,
> >                           bool override, int offset,
> >                           struct typec_altmode_desc *desc)
> >  {
> > -       return NULL;
> > +       return ERR_PTR(-EOPNOTSUPP);
> >  }
> >
> >  static inline void
> > --
> > 2.44.0.769.g3c40516874-goog
> >
>
>
> --
> With best wishes
> Dmitry

Hi,

On Wed, Apr 24, 2024 at 05:00:24PM -0700, Abhishek Pandit-Subedi wrote:
> On Tue, Apr 23, 2024 at 7:06 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Wed, 24 Apr 2024 at 04:48, Jameson Thies <jthies@google.com> wrote:
> > >
> > > From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > >
> > > ucsi_register_altmode checks IS_ERR on returned pointer and treats
> > > NULL as valid. This results in a null deref when
> > > trace_ucsi_register_altmode is called. Return an error from
> > > ucsi_register_displayport when it is not supported and register the
> > > altmode with typec_port_register_altmode.
> > >
> > > Reviewed-by: Jameson Thies <jthies@google.com>
> > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > ---
> > > Changes in V2:
> > > - Checks for error response from ucsi_register_displayport when
> > > registering DisplayPort alternate mode.
> > >
> > >  drivers/usb/typec/ucsi/ucsi.c | 3 +++
> > >  drivers/usb/typec/ucsi/ucsi.h | 2 +-
> > >  2 files changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > > index cb52e7b0a2c5c..f3b413f94fd28 100644
> > > --- a/drivers/usb/typec/ucsi/ucsi.c
> > > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > > @@ -361,6 +361,9 @@ static int ucsi_register_altmode(struct ucsi_connector *con,
> > >                 switch (desc->svid) {
> > >                 case USB_TYPEC_DP_SID:
> > >                         alt = ucsi_register_displayport(con, override, i, desc);
> > > +                       if (IS_ERR(alt) && PTR_ERR(alt) == -EOPNOTSUPP)
> >
> > This makes it ignore EOPNOTSUPP if it is returned by the non-stub
> > implementation. I think the current state is actually better than the
> > implementation found in this patch. I'd suggest adding a comment to
> > ucsi_register_displayport() stub instead.
> 
> So originally on my system, I didn't have the displayport driver
> config enabled. My expectation was that the alt-mode would show up but
> would not be controllable (like all other alt-modes without drivers).
> What ends up happening is that no alt-mode shows up and trying to
> enable the trace crashes.
> 
> When the displayport support isn't there, I think it should just be
> enumerated as a normal, unsupported alt-mode.

This sounds reasonable. Thus if displayport support is not compiled in
ucsi_register_displayport() should just call typec_port_register_altmode(),
I guess.

Best regards,
Christian

On Wed, Apr 24, 2024 at 01:48:18AM +0000, Jameson Thies wrote:
> From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> 
> ucsi_register_altmode checks IS_ERR on returned pointer and treats
> NULL as valid. This results in a null deref when
> trace_ucsi_register_altmode is called. Return an error from
> ucsi_register_displayport when it is not supported and register the
> altmode with typec_port_register_altmode.
> 
> Reviewed-by: Jameson Thies <jthies@google.com>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

This is not the correct way to fix this.  The normal thing to do would
be to add NULL checks.

https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/'

regards,
dan carpenter

Hi Dan,
thank you for the reference. In this case I think returning NULL from
ucsi_register_displayport stub and using a NULL check to prevent a
NULL pointer dereference won't give us the desired behavior. When
CONFIG_TYPEC_DP_ALTMODE is not enabled, the function would return NULL
and the UCSI driver will never end up registering DisplayPort
alternate mode. I'll update the commit message to note that this patch
adds a fallback registration for DisplayPort alternate mode in
addition to simply fixing the NULL pointer dereference caused by
returning it in the ucsi_register_displayport stub. Separately we
could add a NULL check, but I don't think it's necessary. Neither the
non-stub ucsi_register_displayport or typec_port_register_altmode look
like they will return NULL.

Thanks,
Jameson

It looks like in v3 you changed this to:

-       return NULL;
+       return typec_port_register_altmode(con->port, desc);

Which is fine.  Returning a special error pointer which means success is
the part that I objected to.

regards,
dan carpenter