Re: [PATCH] usb: typec: ucsi: treat get_pdos not supported condition as info instead of error

[Dmitry Baryshkov]

On Wed, 5 Jun 2024 at 20:09, Mark Pearson <mpearson-lenovo@xxxxxxxxx> wrote:
>
> Thanks Dmitry (& Diogo from the other thread)
>
> On Tue, Jun 4, 2024, at 7:45 PM, Dmitry Baryshkov wrote:
> > On Tue, Jun 04, 2024 at 03:40:44PM -0400, Mark Pearson wrote:
> >> On systems where the UCSI PDOs are not supported, the UCSI driver is
> >> giving an error message. This can cause users to believe there is a HW
> >> issue with their system when in fact it is working as designed.
> >>
> >> Downgrade message to dev_info for EOPNOTSUPP condition.
> >>
> >> Tested on Lenovo L14 G5 AMD and confirmed with Lenovo FW team that PDOs
> >> are not supported on this platform.
> >>
> >> Signed-off-by: Mark Pearson <mpearson-lenovo@xxxxxxxxx>
> >> ---
> >>  drivers/usb/typec/ucsi/ucsi.c | 8 ++++++--
> >>  1 file changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> >> index cb52e7b0a2c5..090be87d5485 100644
> >> --- a/drivers/usb/typec/ucsi/ucsi.c
> >> +++ b/drivers/usb/typec/ucsi/ucsi.c
> >> @@ -632,8 +632,12 @@ static int ucsi_read_pdos(struct ucsi_connector *con,
> >>      command |= is_source(role) ? UCSI_GET_PDOS_SRC_PDOS : 0;
> >>      ret = ucsi_send_command(ucsi, command, pdos + offset,
> >>                              num_pdos * sizeof(u32));
> >> -    if (ret < 0 && ret != -ETIMEDOUT)
> >> -            dev_err(ucsi->dev, "UCSI_GET_PDOS failed (%d)\n", ret);
> >> +    if (ret < 0 && ret != -ETIMEDOUT) {
> >> +            if (ret == -EOPNOTSUPP)
> >> +                    dev_info(ucsi->dev, "UCSI_GET_PDOS not supported on this hardware\n");
> >
> > Maybe it would be enough to guard GET_PDOS commands with the
> > UCSI_CAP_PDO_DETAILS check? Is it cleared on affected platforms?
> >
>
> I checked on the system I have and the features are 0x84, so the CAP_PDO_DETAILS aren't set.
> I can do a formal patch if the approach is better, I ended up doing:
>
> @@ -645,9 +645,13 @@ static int ucsi_read_pdos(struct ucsi_connector *con,
>  static int ucsi_get_pdos(struct ucsi_connector *con, enum typec_role role,
>                          int is_partner, u32 *pdos)
>  {
> +       struct ucsi *ucsi = con->ucsi;
>         u8 num_pdos;
>         int ret;
>
> +       if (!(ucsi->cap.features & UCSI_CAP_PDO_DETAILS))
> +               return 0;
> +
>         /* UCSI max payload means only getting at most 4 PDOs at a time */
>         ret = ucsi_read_pdos(con, role, is_partner, pdos, 0, UCSI_MAX_PDOS);
>
> And this did indeed squelch the 'error' message.
>
> Couple of notes:
>  - I don't know this area very well, so don't know if there are risks of any regressions in other circumstances. I think it's pretty safe, but if any experts have an opinion that would be appreciated.
>  - It means that there isn't a log message saying that PDO capabilities are not available. Are there going to be power related tooling that won't work and it would be useful to have that message available?

>From my POV this patch looks good. Also if there are no PDOs, then the
UCSI driver will register an empty usb_power_delivery object with
neither source nor sink capabilities being present. So userspace can
identify the case of PDOs retrieval being unsupported. If you really
want to have a possible trace in the logs, it might be a good idea to
add dev_dbg under this if statement.

-- 
With best wishes
Dmitry