From: Jack Pham <jackp@codeaurora.org>
To: Heikki Krogerus <heikki.krogerus@linux.intel.com>,
Greg KH <gregkh@linuxfoundation.org>
Cc: Subbaraman Narayanamurthy <subbaram@codeaurora.org>,
linux-usb@vger.kernel.org, stable@vger.kernel.org,
Abhilash K V <abhilash.k.v@intel.com>
Subject: Re: [PATCH] usb: typec: ucsi: Retrieve all the PDOs instead of just the first 4
Date: Mon, 26 Apr 2021 12:26:05 -0700 [thread overview]
Message-ID: <20210426192605.GB20698@jackp-linux.qualcomm.com> (raw)
In-Reply-To: <20210426191825.30721-1-jackp@codeaurora.org>
Forgot to Cc: Abhilash who introduced the PDO code
On Mon, Apr 26, 2021 at 12:18:25PM -0700, Jack Pham wrote:
> commit 4dbc6a4ef06d ("usb: typec: ucsi: save power data objects
> in PD mode") introduced retrieval of the PDOs when connected to a
> PD-capable source. But only the first 4 PDOs are received since
> that is the maximum number that can be fetched at a time given the
> MESSAGE_IN length limitation (16 bytes). However, as per the PD spec
> a connected source may advertise up to a maximum of 7 PDOs.
>
> If such a source is connected it's possible the UCSI FW could have
> negotiated a power contract with one of the PDOs at index greater
> than 4, and would be reflected in the request data object's (RDO)
> object position field. This would result in an out-of-bounds access
> when the rdo_index() is used to index into the src_pdos array in
> ucsi_psy_get_voltage_now().
>
> We can resolve this by instead retrieving and storing up to the
> maximum of 7 PDOs in the con->src_pdos array. This would involve
> two calls to the GET_PDOS command.
>
> Fixes: 992a60ed0d5e ("usb: typec: ucsi: register with power_supply class")
> Fixes: 4dbc6a4ef06d ("usb: typec: ucsi: save power data objects in PD mode")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jack Pham <jackp@codeaurora.org>
> ---
> drivers/usb/typec/ucsi/ucsi.c | 41 +++++++++++++++++++++++++++--------
> drivers/usb/typec/ucsi/ucsi.h | 6 +++--
> 2 files changed, 36 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 244270755ae6..ac214b855986 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -495,7 +495,8 @@ static void ucsi_unregister_altmodes(struct ucsi_connector *con, u8 recipient)
> }
> }
>
> -static void ucsi_get_pdos(struct ucsi_connector *con, int is_partner)
> +static int ucsi_get_pdos(struct ucsi_connector *con, int is_partner,
> + u32 *pdos, int offset, int num_pdos)
> {
> struct ucsi *ucsi = con->ucsi;
> u64 command;
> @@ -503,17 +504,39 @@ static void ucsi_get_pdos(struct ucsi_connector *con, int is_partner)
>
> command = UCSI_COMMAND(UCSI_GET_PDOS) | UCSI_CONNECTOR_NUMBER(con->num);
> command |= UCSI_GET_PDOS_PARTNER_PDO(is_partner);
> - command |= UCSI_GET_PDOS_NUM_PDOS(UCSI_MAX_PDOS - 1);
> + command |= UCSI_GET_PDOS_PDO_OFFSET(offset);
> + command |= UCSI_GET_PDOS_NUM_PDOS(num_pdos - 1);
> command |= UCSI_GET_PDOS_SRC_PDOS;
> - ret = ucsi_send_command(ucsi, command, con->src_pdos,
> - sizeof(con->src_pdos));
> - if (ret < 0) {
> + ret = ucsi_send_command(ucsi, command, pdos + offset,
> + num_pdos * sizeof(u32));
> + if (ret < 0)
> dev_err(ucsi->dev, "UCSI_GET_PDOS failed (%d)\n", ret);
> + if (ret == 0 && offset == 0)
> + dev_warn(ucsi->dev, "UCSI_GET_PDOS returned 0 bytes\n");
> +
> + return ret;
> +}
> +
> +static void ucsi_get_src_pdos(struct ucsi_connector *con, int is_partner)
> +{
> + int ret;
> +
> + /* UCSI max payload means only getting at most 4 PDOs at a time */
> + ret = ucsi_get_pdos(con, 1, con->src_pdos, 0, UCSI_MAX_PDOS);
> + if (ret < 0)
> return;
> - }
> +
> con->num_pdos = ret / sizeof(u32); /* number of bytes to 32-bit PDOs */
> - if (ret == 0)
> - dev_warn(ucsi->dev, "UCSI_GET_PDOS returned 0 bytes\n");
> + if (con->num_pdos < UCSI_MAX_PDOS)
> + return;
> +
> + /* get the remaining PDOs, if any */
> + ret = ucsi_get_pdos(con, 1, con->src_pdos, UCSI_MAX_PDOS,
> + PDO_MAX_OBJECTS - UCSI_MAX_PDOS);
> + if (ret < 0)
> + return;
> +
> + con->num_pdos += ret / sizeof(u32);
> }
>
> static void ucsi_pwr_opmode_change(struct ucsi_connector *con)
> @@ -522,7 +545,7 @@ static void ucsi_pwr_opmode_change(struct ucsi_connector *con)
> case UCSI_CONSTAT_PWR_OPMODE_PD:
> con->rdo = con->status.request_data_obj;
> typec_set_pwr_opmode(con->port, TYPEC_PWR_MODE_PD);
> - ucsi_get_pdos(con, 1);
> + ucsi_get_src_pdos(con, 1);
> break;
> case UCSI_CONSTAT_PWR_OPMODE_TYPEC1_5:
> con->rdo = 0;
> diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> index 3920e20a9e9e..cee666790907 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -8,6 +8,7 @@
> #include <linux/power_supply.h>
> #include <linux/types.h>
> #include <linux/usb/typec.h>
> +#include <linux/usb/pd.h>
> #include <linux/usb/role.h>
>
> /* -------------------------------------------------------------------------- */
> @@ -134,7 +135,9 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num);
>
> /* GET_PDOS command bits */
> #define UCSI_GET_PDOS_PARTNER_PDO(_r_) ((u64)(_r_) << 23)
> +#define UCSI_GET_PDOS_PDO_OFFSET(_r_) ((u64)(_r_) << 24)
> #define UCSI_GET_PDOS_NUM_PDOS(_r_) ((u64)(_r_) << 32)
> +#define UCSI_MAX_PDOS (4)
> #define UCSI_GET_PDOS_SRC_PDOS ((u64)1 << 34)
>
> /* -------------------------------------------------------------------------- */
> @@ -302,7 +305,6 @@ struct ucsi {
>
> #define UCSI_MAX_SVID 5
> #define UCSI_MAX_ALTMODES (UCSI_MAX_SVID * 6)
> -#define UCSI_MAX_PDOS (4)
>
> #define UCSI_TYPEC_VSAFE5V 5000
> #define UCSI_TYPEC_1_5_CURRENT 1500
> @@ -330,7 +332,7 @@ struct ucsi_connector {
> struct power_supply *psy;
> struct power_supply_desc psy_desc;
> u32 rdo;
> - u32 src_pdos[UCSI_MAX_PDOS];
> + u32 src_pdos[PDO_MAX_OBJECTS];
> int num_pdos;
>
> struct usb_role_switch *usb_role_sw;
> --
> 2.24.0
>
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
next prev parent reply other threads:[~2021-04-26 19:26 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-26 19:18 [PATCH] usb: typec: ucsi: Retrieve all the PDOs instead of just the first 4 Jack Pham
2021-04-26 19:26 ` Jack Pham [this message]
2021-04-26 20:21 ` Subbaraman Narayanamurthy
2021-04-26 20:42 ` Jack Pham
2021-04-26 22:36 ` Subbaraman Narayanamurthy
2021-05-03 7:03 ` Heikki Krogerus
2021-05-03 7:29 ` Jack Pham
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210426192605.GB20698@jackp-linux.qualcomm.com \
--to=jackp@codeaurora.org \
--cc=abhilash.k.v@intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=heikki.krogerus@linux.intel.com \
--cc=linux-usb@vger.kernel.org \
--cc=stable@vger.kernel.org \
--cc=subbaram@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).