On Tue, Aug 20, 2024 at 6:12 AM Heikki Krogerus
<heikki.krogerus@xxxxxxxxxxxxxxx> wrote:
>
> On Mon, Aug 19, 2024 at 04:23:56PM -0700, Abhishek Pandit-Subedi wrote:
> > 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?
>
> Nobody is proposing a driver split.
>
> > 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.
>
> Unfortunately 0 is a valid value also in this case.
>
> > 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).
>
> Again, nobody is proposing a driver split. I don't know where did you
> get this idea.
Well you did revert a backwards compatible structure :)
>
> > 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.
>
> Simply naming a macro is not enough. A macro is fine, but it must have
> the means to check the version and fail if it does not match.
>
> We have a golden opportunity here to do exactly that. In most cases
> only fields are extended, which is much more difficult situation, but
> in this case we actually have completely new fields that extend the
> data structure. That makes it possible to use the size like I'm doing
> in patch 3/5 which guarantees that driver fails if those extended
> fields are accessed when they are not available. That is exactly what
> we want.
>
> Otherwise accessing those fields when they are not available will
> create the issues silently, most likely in form of a horrible user
> experience: the cable works only if you plug it one way but not the
> other because something thinks the orientation field is valid, or the
> screen may simply be black. There are no error messages in dmesg, so
> from kernel PoW everything seems to be working as it should. This is
> not what we want. What we want is a spectacular failure if something
> like that happens.
>
> That failure will give us these two things:
>
> 1. It pin points the root cause of these issues.
> 2. If forces us to actually fix the issue (these are usually not
> considered as critical issues unfortunately).
>
> These "silent" issues are really common and they always take a lot of
> time to debug. I'm not going to waste this opportunity to make them a
> bit less "silent" in this case.
You have me convinced on the "failing loudly" part but I'm still
confused about the "how".
Making sure we always check versions to access the bits makes me think
we need wrappers on casting to the rightly versioned connector status.
Should we be versioning access for everything that's not in UCSI 1.2
then?
Example:
struct ucsi_connector_status_raw {
u8 bytes[19];
};
struct ucsi_connector_status_v1 {
...
};
struct ucsi_connector_status_v2 {
...
};
struct ucsi_connector_status_v1* get_connector_status_v1(struct
ucsi_connector *con) {
return (struct ucsi_connector_status_v1 *)con->raw_status;
}
struct ucsi_connector_status_v2* get_connector_status_v2(struct
ucsi_connector *con) {
return con->ucsi->version >= UCSI_VERSION_2_0 ? (struct
ucsi_connector_status_v2 *)&con->raw_status : NULL;
}
/* Read all bits supported by the current version. */
int ucsi_read_connector_status(struct ucsi_connector *con, struct
ucsi_connector_status_raw *raw_conn_status);
>
> thanks,
>
> --
> heikki
