Re: [PATCH] usb: typec: ucsi: Retrieve all the PDOs instead of just the first 4

[Jack Pham <jackp@codeaurora.org>]

On Mon, May 03, 2021 at 10:03:20AM +0300, Heikki Krogerus wrote:
> On Mon, Apr 26, 2021 at 12:26:05PM -0700, Jack Pham wrote:
> > 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.
> 
> This makes sense to me. A few nitpicks below. Besides those:
> 
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Thanks Heikki! I'll send a V2 along with the commit text improvements
Subbaraman had suggested.

> > > 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)
> 
> That should be aligned properly.

Will do.

> > >  {
> > >  	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));
> 
> That too. I think if you run scripts/checkpatch.pl it will complain
> about those.

Got it.

Actually checkpatch didn't warn about this. But I'll align to follow the
style in the rest of this file.

> > > +	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;

Thanks,
Jack
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project