Received-SPF: pass (google.com: domain of linux-kernel+bounces-202959-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
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: <a517bb13-9772-49f9-b75c-8fa4d98b2ae9@app.fastmail.com>
In-Reply-To: 
 <2midmmssv2i3plvtc2hajar6alfvggpnbvgpmldspelxsnjvcl@qiblhwat6n3p>
References: <mpearson-lenovo@squebb.ca>
 <20240604194056.16625-1-mpearson-lenovo@squebb.ca>
 <2midmmssv2i3plvtc2hajar6alfvggpnbvgpmldspelxsnjvcl@qiblhwat6n3p>
Date: Wed, 05 Jun 2024 13:09:01 -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

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?

Thanks
Mark