Re: [PATCH v2 1/6] usb: typec: ucsi: Remove unused fields from struct ucsi_connector_status

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

Hi Abhishek,

On Sun, Aug 18, 2024 at 05:02:28PM -0700, Abhishek Pandit-Subedi wrote:
> On Fri, Aug 16, 2024 at 6:59 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > The new fields are valid only with the new UCSI versions.
> > They are at offsets that go beyond the MAX_DATA_LENGTH (16
> > bytes) with the older UCSI versions. That has not caused any
> > problems before because nothing uses those new fields yet.
> > Because they are not used yet, dropping them for now.
> >
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> >  drivers/usb/typec/ucsi/ucsi.h | 27 ++-------------------------
> >  1 file changed, 2 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> > index 57129f3c0814..7bc132b59027 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.h
> > +++ b/drivers/usb/typec/ucsi/ucsi.h
> > @@ -344,35 +344,12 @@ struct ucsi_connector_status {
> >  #define   UCSI_CONSTAT_PARTNER_TYPE_AUDIO      6
> >         u32 request_data_obj;
> >
> > -       u8 pwr_status[3];
> > -#define UCSI_CONSTAT_BC_STATUS(_p_)            ((_p_[0]) & GENMASK(1, 0))
> > +       u8 pwr_status;
> > +#define UCSI_CONSTAT_BC_STATUS(_p_)            ((_p_) & GENMASK(1, 0))
> >  #define   UCSI_CONSTAT_BC_NOT_CHARGING         0
> >  #define   UCSI_CONSTAT_BC_NOMINAL_CHARGING     1
> >  #define   UCSI_CONSTAT_BC_SLOW_CHARGING                2
> >  #define   UCSI_CONSTAT_BC_TRICKLE_CHARGING     3
> > -#define UCSI_CONSTAT_PROVIDER_CAP_LIMIT(_p_)   (((_p_[0]) & GENMASK(5, 2)) >> 2)
> > -#define   UCSI_CONSTAT_CAP_PWR_LOWERED         0
> > -#define   UCSI_CONSTAT_CAP_PWR_BUDGET_LIMIT    1
> > -#define UCSI_CONSTAT_PROVIDER_PD_VERSION_OPER_MODE(_p_)        \
> > -       ((get_unaligned_le32(_p_) & GENMASK(21, 6)) >> 6)
> > -#define UCSI_CONSTAT_ORIENTATION(_p_)          (((_p_[2]) & GENMASK(6, 6)) >> 6)
> > -#define   UCSI_CONSTAT_ORIENTATION_DIRECT      0
> > -#define   UCSI_CONSTAT_ORIENTATION_FLIPPED     1
> > -#define UCSI_CONSTAT_SINK_PATH_STATUS(_p_)     (((_p_[2]) & GENMASK(7, 7)) >> 7)
> > -#define   UCSI_CONSTAT_SINK_PATH_DISABLED      0
> > -#define   UCSI_CONSTAT_SINK_PATH_ENABLED       1
> > -       u8 pwr_readings[9];
> > -#define UCSI_CONSTAT_REV_CURR_PROT_STATUS(_p_) ((_p_[0]) & 0x1)
> > -#define UCSI_CONSTAT_PWR_READING_VALID(_p_)    (((_p_[0]) & GENMASK(1, 1)) >> 1)
> > -#define UCSI_CONSTAT_CURRENT_SCALE(_p_)                (((_p_[0]) & GENMASK(4, 2)) >> 2)
> > -#define UCSI_CONSTAT_PEAK_CURRENT(_p_) \
> > -       ((get_unaligned_le32(_p_) & GENMASK(20, 5)) >> 5)
> > -#define UCSI_CONSTAT_AVG_CURRENT(_p_) \
> > -       ((get_unaligned_le32(&(_p_)[2]) & GENMASK(20, 5)) >> 5)
> > -#define UCSI_CONSTAT_VOLTAGE_SCALE(_p_) \
> > -       ((get_unaligned_le16(&(_p_)[4]) & GENMASK(8, 5)) >> 5)
> > -#define UCSI_CONSTAT_VOLTAGE_READING(_p_) \
> > -       ((get_unaligned_le32(&(_p_)[5]) & GENMASK(16, 1)) >> 1)
> >  } __packed;
> >
> >  /*
> > --
> > 2.43.0
> >
> >
> 
> I'm working on a patch series that depends on the sink path status so
> I would prefer we don't remove it:
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/5784952
> 
> Since GET_CONNECTOR_STATUS is the only command that exceeds 16 bytes
> for MESSAGE_IN, can we just add a wrapper that checks the UCSI version
> for that command only and limits the size sent to ucsi_run_command?

It's always "just this one command" :). It's actually not only the
GET_CONNECTOR_STATUS command in this case - at least GET_PD_MESSAGE
can also exceed 16 bytes - but even if it was, it would still not be
okay to simply guard the read. You would also have to check the
version with every access of those extended fields, and that's where
it's basically guaranteed to fail. Somebody will access those extended
fields unconditionally without anybody noticing sooner or later, and
that's why they can't be part of this data structure.

So there needs to be a completely separate data structure for the
extended version.

struct ucsi_connector_status_extended {
        u8 status[16];
        u8 extended[3];
} __packed;

Something like that. But that of course should not be introduced
before there is an actual user for it.

thanks,

-- 
heikki