mbox series

[v4,0/3] usb: typec: ucsi: Adding support for UCSI 3.0

Message ID 20240209060353.6613-1-abhishekpandit@chromium.org
Headers show
Series usb: typec: ucsi: Adding support for UCSI 3.0 | expand

Message

Abhishek Pandit-Subedi Feb. 9, 2024, 6:02 a.m. UTC 
Hi Heikki,

This series starts the work adding UCSI 3.0 support to the UCSI driver.

There's a couple of pieces to start here:
* Add version checks and limit read size on 1.2.
* Update Connector Status and Connector Capability structures.
* Expose Partner PD revision from Capability data.

These were tested against on a 6.6 kernel running a usermode PPM against
a Realtek Evaluation board.

One additional note: there are a lot more unaligned fields in UCSI now
and the struct definitions are getting a bit out of hand. We can discuss
alternate mechanisms for defining these structs in the patch that
changes these structures.

Thanks,
Abhishek

Changes in v4:
  - Added missing Tested-By tags from v1 and reviewed-by tags.
  - Fix BCD translation of PD Major Rev
  - Replace IS_MIN_VERSION macro and just compare version directly.

Changes in v3:
  - Change include to asm/unaligned.h and reorder include.

Changes in v2:
  - Changed log message to DEBUG
  - Formatting changes and update macro to use brackets.
  - Fix incorrect guard condition when checking connector capability.

Abhishek Pandit-Subedi (3):
  usb: typec: ucsi: Limit read size on v1.2
  usb: typec: ucsi: Update connector cap and status
  usb: typec: ucsi: Get PD revision for partner

 drivers/usb/typec/ucsi/ucsi.c | 49 +++++++++++++++++++++++--
 drivers/usb/typec/ucsi/ucsi.h | 67 ++++++++++++++++++++++++++++++++---
 2 files changed, 110 insertions(+), 6 deletions(-)

Comments

Andy Shevchenko Feb. 9, 2024, 2:28 p.m. UTC | #1 
On Thu, Feb 08, 2024 at 10:02:38PM -0800, Abhishek Pandit-Subedi wrote:
> Between UCSI 1.2 and UCSI 2.0, the size of the MESSAGE_IN region was
> increased from 16 to 256. In order to avoid overflowing reads for older
> systems, add a mechanism to use the read UCSI version to truncate read
> sizes on UCSI v1.2.

...

> +	if (ucsi->version <= UCSI_VERSION_1_2)
> +		buf_size = min_t(size_t, 16, buf_size);

Please, avoid using min_t(). Here the clamp() can be used.
Shouldn't magic number be defined?
Andy Shevchenko Feb. 9, 2024, 7:39 p.m. UTC | #2 
On Fri, Feb 09, 2024 at 10:01:07AM -0800, Abhishek Pandit-Subedi wrote:
> On Fri, Feb 9, 2024 at 6:28 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Feb 08, 2024 at 10:02:38PM -0800, Abhishek Pandit-Subedi wrote:

...

> > > +     if (ucsi->version <= UCSI_VERSION_1_2)
> > > +             buf_size = min_t(size_t, 16, buf_size);
> >
> > Please, avoid using min_t(). Here the clamp() can be used.
> I think this is likely the 4th time I've been tripped up by an
> undocumented practice in this patch series. <linux/minmax.h> says
> nothing about avoiding min_t -- why prefer clamp()?

While in this case it will work correctly, the size_t is unsigned type and 16
is signed, while buf_size is unknown in this context. It means if buf_size is
signed, the min_t gives wrong result. clamp() is better choice.

See also, e.g., https://lore.kernel.org/all/20231004064220.31452-1-biju.das.jz@bp.renesas.com/.

> Please add the
> recommendation here
> (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/minmax.h#n10)
> and I am more than happy to change it after.

It's not my recommendation :-)

https://lore.kernel.org/all/CAHk-=whwEAc22wm8h9FESPB5X+P4bLDgv0erBQMa1buTNQW7tA@mail.gmail.com/

Feel free to submit a patch.

...

> > Shouldn't magic number be defined?
> The comment right above this line documents the number.
> As this is the only use right now, I don't see a need to make it a
> macro/constant yet.

OK.