Received-SPF: pass (google.com: domain of linux-kernel+bounces-203525-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Feedback-ID: ibe194615:Fastmail
User-Agent: Cyrus-JMAP/3.11.0-alpha0-497-g97f96844c-fm-20240526.001-g97f96844
Precedence: bulk
MIME-Version: 1.0
Message-Id: <69b41002-69d7-4327-a8cd-b8bdc371ad2c@app.fastmail.com>
In-Reply-To: 
 <CAA8EJppzeBPHW1GPGvVzT-YSGm4PZUQ4qgq0FCgqn1e9Vt-MXg@mail.gmail.com>
References: <mpearson-lenovo@squebb.ca>
 <20240604194056.16625-1-mpearson-lenovo@squebb.ca>
 <2midmmssv2i3plvtc2hajar6alfvggpnbvgpmldspelxsnjvcl@qiblhwat6n3p>
 <a517bb13-9772-49f9-b75c-8fa4d98b2ae9@app.fastmail.com>
 <CAA8EJppzeBPHW1GPGvVzT-YSGm4PZUQ4qgq0FCgqn1e9Vt-MXg@mail.gmail.com>
Date: Wed, 05 Jun 2024 22:12:45 -0400
From: "Mark Pearson" <mpearson-lenovo@squebb.ca>
To: "Dmitry Baryshkov" <dmitry.baryshkov@linaro.org>
Cc: "Heikki Krogerus" <heikki.krogerus@linux.intel.com>,
 "Greg KH" <gregkh@linuxfoundation.org>, linux-usb@vger.kernel.org,
 linux-kernel@vger.kernel.org, "Diogo Ivo" <diogo.ivo@siemens.com>
Subject: Re: [PATCH] usb: typec: ucsi: treat get_pdos not supported condition as info
 instead of error
Content-Type: text/plain


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