On Mon, Aug 19, 2024 at 4:11 AM Heikki Krogerus
<heikki.krogerus@xxxxxxxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > > ---
> > > 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 this kind of points out a fundamental question to UCSI 1.2 vs
UCSI2+ in this driver: should we be doing a single driver that checks
the UCSI version before doing things or have two separate drivers?
I'm in favor of a single driver approach as I think there are a lot of
commonalities between the different UCSI versions. I think zero-ing
out the extended data on reads should be sufficient to support both
versions from the same code-base.
The alternative of creating a separate driver comes with more problems
-- do we fork for ucsi2 AND ucsi3? ucsi3 adds additional commands
(i.e. set_usb) and, in the case of get_pd_message, adds additional
behavior to existing commands (Get_Revision).
If your main worry is that people will unconditionally use the new
bits, why don't we simply change the macros from UCSI_CONSTAT to
UCSI_CONSTAT_v2 to indicate they need a revision bump? We can make
similar changes to other macros + enums to indicate the minimum UCSI
version that added those values.
>
> 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
