Received-SPF: pass (google.com: domain of linux-kernel+bounces-203406-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <mpearson-lenovo@squebb.ca> <20240604194056.16625-1-mpearson-lenovo@squebb.ca>
 <2midmmssv2i3plvtc2hajar6alfvggpnbvgpmldspelxsnjvcl@qiblhwat6n3p> <a517bb13-9772-49f9-b75c-8fa4d98b2ae9@app.fastmail.com>
In-Reply-To: <a517bb13-9772-49f9-b75c-8fa4d98b2ae9@app.fastmail.com>
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date: Thu, 6 Jun 2024 02:26:46 +0300
Message-ID: <CAA8EJppzeBPHW1GPGvVzT-YSGm4PZUQ4qgq0FCgqn1e9Vt-MXg@mail.gmail.com>
Subject: Re: [PATCH] usb: typec: ucsi: treat get_pdos not supported condition
 as info instead of error
To: Mark Pearson <mpearson-lenovo@squebb.ca>
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>
Content-Type: text/plain; charset="UTF-8"

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.

-- 
With best wishes
Dmitry