diff mbox series

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

Message ID 20240816135859.3499351-2-heikki.krogerus@linux.intel.com (mailing list archive)
State Accepted
Commit 9ecf52153b6f6681853b60c9912231e03857dff4
Headers show
Series usb: typec: ucsi: Minor improvements | expand

Commit Message

Heikki Krogerus Aug. 16, 2024, 1:58 p.m. UTC
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(-)

Comments

Abhishek Pandit-Subedi Aug. 19, 2024, 12:02 a.m. UTC | #1
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?
Heikki Krogerus Aug. 19, 2024, 11:11 a.m. UTC | #2
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,
Abhishek Pandit-Subedi Aug. 19, 2024, 11:23 p.m. UTC | #3
On Mon, Aug 19, 2024 at 4:11 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> 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@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 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
Heikki Krogerus Aug. 20, 2024, 1:12 p.m. UTC | #4
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@linux.intel.com> 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@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 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.

> 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.

thanks,
Abhishek Pandit-Subedi Aug. 20, 2024, 4:48 p.m. UTC | #5
On Tue, Aug 20, 2024 at 6:12 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> 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@linux.intel.com> 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@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 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
Heikki Krogerus Aug. 22, 2024, 11:24 a.m. UTC | #6
Hi Abhishek,

I'm sorry to keep you waiting.

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

I'll take a look at this next week. Right now I have to focus on
other tasks.

Br,
Heikki Krogerus Aug. 27, 2024, 3:23 p.m. UTC | #7
On Thu, Aug 22, 2024 at 02:24:17PM +0300, Heikki Krogerus wrote:
> Hi Abhishek,
> 
> I'm sorry to keep you waiting.
> 
> > 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);
> 
> I'll take a look at this next week. Right now I have to focus on
> other tasks.

Sorry again for the delay. Today I sketched the idea that I have. I
think this problem would be the easiest to handle with field specific
helpers (see attached).

But at the same time I would like to get rid of all command specific
data structures. I've been planning that for some time now. We can do
that with a macro like that UCSI_FIELD(). The problem with those
structures is that if the fields in the structure don't align nicely
(like in case of GET_CONNECTOR_STATUS), we have to come up with custom
members, and that is not ideal at all. There are probable other
problems too.

I did not convert anything yet, I'll prepare a more complete RFC
tomorrow, but you should be able to get the idea from this.

thanks,
diff mbox series

Patch

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;
 
 /*