| Message ID | 20240604194056.16625-1-mpearson-lenovo@squebb.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: typec: ucsi: treat get_pdos not supported condition as info instead of error | expand |
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? > + else > + dev_err(ucsi->dev, "UCSI_GET_PDOS failed (%d)\n", ret); > + } > > return ret; > } > -- > 2.45.1 >
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 <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
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.
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
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
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; }
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(-)