| Message ID | 20240208220230.v4.1.Iacf5570a66b82b73ef03daa6557e2fc0db10266a@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: ucsi: Adding support for UCSI 3.0 | expand |
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?
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: > > 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. 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()? 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. > 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. > > -- > With Best Regards, > Andy Shevchenko > > Cheers, Abhishek
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.
On Fri, Feb 9, 2024 at 11:39 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > 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. > Ack, will send up a PATCH v5. And add to my backlog of foot-gun checkpatch and docs fixes I need to send up :) > ... > > > > 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. > > -- > With Best Regards, > Andy Shevchenko > >
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c index 14f5a7bfae2e..d9a345ec62c3 100644 --- a/drivers/usb/typec/ucsi/ucsi.c +++ b/drivers/usb/typec/ucsi/ucsi.c @@ -36,6 +36,19 @@ */ #define UCSI_SWAP_TIMEOUT_MS 5000 +static int ucsi_read_message_in(struct ucsi *ucsi, void *buf, + size_t buf_size) +{ + /* + * Below UCSI 2.0, MESSAGE_IN was limited to 16 bytes. Truncate the + * reads here. + */ + if (ucsi->version <= UCSI_VERSION_1_2) + buf_size = min_t(size_t, 16, buf_size); + + return ucsi->ops->read(ucsi, UCSI_MESSAGE_IN, buf, buf_size); +} + static int ucsi_acknowledge_command(struct ucsi *ucsi) { u64 ctrl; @@ -72,7 +85,7 @@ static int ucsi_read_error(struct ucsi *ucsi) if (ret < 0) return ret; - ret = ucsi->ops->read(ucsi, UCSI_MESSAGE_IN, &error, sizeof(error)); + ret = ucsi_read_message_in(ucsi, &error, sizeof(error)); if (ret) return ret; @@ -170,7 +183,7 @@ int ucsi_send_command(struct ucsi *ucsi, u64 command, length = ret; if (data) { - ret = ucsi->ops->read(ucsi, UCSI_MESSAGE_IN, data, size); + ret = ucsi_read_message_in(ucsi, data, size); if (ret) goto out; } @@ -1558,6 +1571,15 @@ int ucsi_register(struct ucsi *ucsi) if (!ucsi->version) return -ENODEV; + /* + * Version format is JJ.M.N (JJ = Major version, M = Minor version, + * N = sub-minor version). + */ + dev_dbg(ucsi->dev, "Registered UCSI interface with version %x.%x.%x", + UCSI_BCD_GET_MAJOR(ucsi->version), + UCSI_BCD_GET_MINOR(ucsi->version), + UCSI_BCD_GET_SUBMINOR(ucsi->version)); + queue_delayed_work(system_long_wq, &ucsi->work, 0); ucsi_debugfs_register(ucsi); diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h index 6478016d5cb8..bec920fa6b8a 100644 --- a/drivers/usb/typec/ucsi/ucsi.h +++ b/drivers/usb/typec/ucsi/ucsi.h @@ -23,6 +23,17 @@ struct dentry; #define UCSI_CONTROL 8 #define UCSI_MESSAGE_IN 16 #define UCSI_MESSAGE_OUT 32 +#define UCSIv2_MESSAGE_OUT 272 + +/* UCSI versions */ +#define UCSI_VERSION_1_2 0x0120 +#define UCSI_VERSION_2_0 0x0200 +#define UCSI_VERSION_2_1 0x0210 +#define UCSI_VERSION_3_0 0x0300 + +#define UCSI_BCD_GET_MAJOR(_v_) (((_v_) >> 8) & 0xFF) +#define UCSI_BCD_GET_MINOR(_v_) (((_v_) >> 4) & 0x0F) +#define UCSI_BCD_GET_SUBMINOR(_v_) ((_v_) & 0x0F) /* Command Status and Connector Change Indication (CCI) bits */ #define UCSI_CCI_CONNECTOR(_c_) (((_c_) & GENMASK(7, 1)) >> 1)