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 <[email protected]>
---
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");
+ else
+ dev_err(ucsi->dev, "UCSI_GET_PDOS failed (%d)\n", ret);
+ }
return ret;
}
--
2.45.1
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 <[email protected]>
> ---
> 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?
> + else
> + dev_err(ucsi->dev, "UCSI_GET_PDOS failed (%d)\n", ret);
> + }
>
> return ret;
> }
> --
> 2.45.1
>
--
With best wishes
Dmitry
Hi Mark,
Do these devices report GET_PDOS support as a response to GET_CAPABILITY?
If they don't I think the best way of addressing this would be to guard
against executing this command for devices without "PDO details supported"
(in ucsi_get_pdos() for example).
Best regards,
Diogo
Just realized Dmitry said literally the same thing as me. Sorry for the
extra noise, please ignore my comment.
Best regards,
Diogo
Hi Diogo,
On Wed, Jun 5, 2024, at 11:29 AM, Diogo Ivo wrote:
> Just realized Dmitry said literally the same thing as me. Sorry for the
> extra noise, please ignore my comment.
>
All good - it's a good suggestion and I appreciate the reviews from both you and Dmitry.
I'm looking into it, to see if it works out. Will update to the thread in a bit.
Thanks
Mark
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 <[email protected]>
>> ---
>> 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
On Wed, 5 Jun 2024 at 20:09, Mark Pearson <[email protected]> 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 <[email protected]>
> >> ---
> >> 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
On Wed, Jun 5, 2024, at 7:26 PM, Dmitry Baryshkov wrote:
> On Wed, 5 Jun 2024 at 20:09, Mark Pearson <[email protected]> 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 <[email protected]>
>> >> ---
>> >> 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
Hi Mark,
On 6/5/24 6:09 PM, Mark Pearson wrote:
>
> @@ -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);
>
Seems good to me too.
Best regards,
Diogo