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

["Mark Pearson" <mpearson-lenovo@squebb.ca>]

On Wed, Jun 5, 2024, at 7:26 PM, Dmitry Baryshkov wrote:
> On Wed, 5 Jun 2024 at 20:09, Mark Pearson <mpearson-lenovo@squebb.ca> 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@squebb.ca>
>> >> ---
>> >>  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.
>
Thanks Dmitry.

I don't have any concerns about not having a log message myself.
I'll wait a couple more days in case there is other feedback and, if all good, get a new patch submitted with this change instead.

Mark