| Message ID | 20230322175513.1550412-2-titusr@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PMBus fixes and new functions | expand |
It's generally frowned upon to have empty descriptions, some rationale would be helpful. For instance, you remove a length check from the send string, why did you do that? Any why is this being added? What's it supporting? -corey On Wed, Mar 22, 2023 at 05:55:09PM +0000, Titus Rwantare wrote: > Reviewed-by: Hao Wu <wuhaotsh@google.com> > Signed-off-by: Titus Rwantare <titusr@google.com> > --- > hw/i2c/pmbus_device.c | 30 +++++++++++++++++++++++++++++- > include/hw/i2c/pmbus_device.h | 7 +++++++ > 2 files changed, 36 insertions(+), 1 deletion(-) > > diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c > index c3d6046784..02647769cd 100644 > --- a/hw/i2c/pmbus_device.c > +++ b/hw/i2c/pmbus_device.c > @@ -95,7 +95,6 @@ void pmbus_send64(PMBusDevice *pmdev, uint64_t data) > void pmbus_send_string(PMBusDevice *pmdev, const char *data) > { > size_t len = strlen(data); > - g_assert(len > 0); > g_assert(len + pmdev->out_buf_len < SMBUS_DATA_MAX_LEN); > pmdev->out_buf[len + pmdev->out_buf_len] = len; > > @@ -105,6 +104,35 @@ void pmbus_send_string(PMBusDevice *pmdev, const char *data) > pmdev->out_buf_len += len + 1; > } > > +uint8_t pmbus_receive_block(PMBusDevice *pmdev, uint8_t *dest, size_t len) > +{ > + /* dest may contain data from previous writes */ > + memset(dest, 0, len); > + > + /* Exclude command code from return value */ > + pmdev->in_buf++; > + pmdev->in_buf_len--; > + > + /* The byte after the command code denotes the length */ > + uint8_t sent_len = pmdev->in_buf[0]; > + > + if (sent_len != pmdev->in_buf_len - 1) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: length mismatch. Expected %d bytes, got %d bytes\n", > + __func__, sent_len, pmdev->in_buf_len - 1); > + } > + > + /* exclude length byte */ > + pmdev->in_buf++; > + pmdev->in_buf_len--; > + > + if (pmdev->in_buf_len < len) { > + len = pmdev->in_buf_len; > + } > + memcpy(dest, pmdev->in_buf, len); > + return len; > +} > + > > static uint64_t pmbus_receive_uint(PMBusDevice *pmdev) > { > diff --git a/include/hw/i2c/pmbus_device.h b/include/hw/i2c/pmbus_device.h > index 93f5d57c9d..7dc00cc4d9 100644 > --- a/include/hw/i2c/pmbus_device.h > +++ b/include/hw/i2c/pmbus_device.h > @@ -501,6 +501,13 @@ void pmbus_send64(PMBusDevice *state, uint64_t data); > */ > void pmbus_send_string(PMBusDevice *state, const char *data); > > +/** > + * @brief Receive data sent with Block Write. > + * @param dest - memory with enough capacity to receive the write > + * @param len - the capacity of dest > + */ > +uint8_t pmbus_receive_block(PMBusDevice *pmdev, uint8_t *dest, size_t len); > + > /** > * @brief Receive data over PMBus > * These methods help track how much data is being received over PMBus > -- > 2.40.0.rc1.284.g88254d51c5-goog >
Apologies. I've updated the commit descriptions and added a sensor using this code in v2. While doing block receive I discovered that it is valid behaviour to erase a field and have it be an empty string. -Titus On Thu, 30 Mar 2023 at 09:18, Corey Minyard <minyard@acm.org> wrote: > It's generally frowned upon to have empty descriptions, some rationale > would be helpful. For instance, you remove a length check from the send > string, why did you do that? > > Any why is this being added? What's it supporting? > > -corey > > On Wed, Mar 22, 2023 at 05:55:09PM +0000, Titus Rwantare wrote: > > Reviewed-by: Hao Wu <wuhaotsh@google.com> > > Signed-off-by: Titus Rwantare <titusr@google.com> > > --- > > hw/i2c/pmbus_device.c | 30 +++++++++++++++++++++++++++++- > > include/hw/i2c/pmbus_device.h | 7 +++++++ > > 2 files changed, 36 insertions(+), 1 deletion(-) > > > > diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c > > index c3d6046784..02647769cd 100644 > > --- a/hw/i2c/pmbus_device.c > > +++ b/hw/i2c/pmbus_device.c > > @@ -95,7 +95,6 @@ void pmbus_send64(PMBusDevice *pmdev, uint64_t data) > > void pmbus_send_string(PMBusDevice *pmdev, const char *data) > > { > > size_t len = strlen(data); > > - g_assert(len > 0); > > g_assert(len + pmdev->out_buf_len < SMBUS_DATA_MAX_LEN); > > pmdev->out_buf[len + pmdev->out_buf_len] = len; > > > > @@ -105,6 +104,35 @@ void pmbus_send_string(PMBusDevice *pmdev, const > char *data) > > pmdev->out_buf_len += len + 1; > > } > > > > +uint8_t pmbus_receive_block(PMBusDevice *pmdev, uint8_t *dest, size_t > len) > > +{ > > + /* dest may contain data from previous writes */ > > + memset(dest, 0, len); > > + > > + /* Exclude command code from return value */ > > + pmdev->in_buf++; > > + pmdev->in_buf_len--; > > + > > + /* The byte after the command code denotes the length */ > > + uint8_t sent_len = pmdev->in_buf[0]; > > + > > + if (sent_len != pmdev->in_buf_len - 1) { > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "%s: length mismatch. Expected %d bytes, got %d > bytes\n", > > + __func__, sent_len, pmdev->in_buf_len - 1); > > + } > > + > > + /* exclude length byte */ > > + pmdev->in_buf++; > > + pmdev->in_buf_len--; > > + > > + if (pmdev->in_buf_len < len) { > > + len = pmdev->in_buf_len; > > + } > > + memcpy(dest, pmdev->in_buf, len); > > + return len; > > +} > > + > > > > static uint64_t pmbus_receive_uint(PMBusDevice *pmdev) > > { > > diff --git a/include/hw/i2c/pmbus_device.h > b/include/hw/i2c/pmbus_device.h > > index 93f5d57c9d..7dc00cc4d9 100644 > > --- a/include/hw/i2c/pmbus_device.h > > +++ b/include/hw/i2c/pmbus_device.h > > @@ -501,6 +501,13 @@ void pmbus_send64(PMBusDevice *state, uint64_t > data); > > */ > > void pmbus_send_string(PMBusDevice *state, const char *data); > > > > +/** > > + * @brief Receive data sent with Block Write. > > + * @param dest - memory with enough capacity to receive the write > > + * @param len - the capacity of dest > > + */ > > +uint8_t pmbus_receive_block(PMBusDevice *pmdev, uint8_t *dest, size_t > len); > > + > > /** > > * @brief Receive data over PMBus > > * These methods help track how much data is being received over PMBus > > -- > > 2.40.0.rc1.284.g88254d51c5-goog > > >
diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c index c3d6046784..02647769cd 100644 --- a/hw/i2c/pmbus_device.c +++ b/hw/i2c/pmbus_device.c @@ -95,7 +95,6 @@ void pmbus_send64(PMBusDevice *pmdev, uint64_t data) void pmbus_send_string(PMBusDevice *pmdev, const char *data) { size_t len = strlen(data); - g_assert(len > 0); g_assert(len + pmdev->out_buf_len < SMBUS_DATA_MAX_LEN); pmdev->out_buf[len + pmdev->out_buf_len] = len; @@ -105,6 +104,35 @@ void pmbus_send_string(PMBusDevice *pmdev, const char *data) pmdev->out_buf_len += len + 1; } +uint8_t pmbus_receive_block(PMBusDevice *pmdev, uint8_t *dest, size_t len) +{ + /* dest may contain data from previous writes */ + memset(dest, 0, len); + + /* Exclude command code from return value */ + pmdev->in_buf++; + pmdev->in_buf_len--; + + /* The byte after the command code denotes the length */ + uint8_t sent_len = pmdev->in_buf[0]; + + if (sent_len != pmdev->in_buf_len - 1) { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: length mismatch. Expected %d bytes, got %d bytes\n", + __func__, sent_len, pmdev->in_buf_len - 1); + } + + /* exclude length byte */ + pmdev->in_buf++; + pmdev->in_buf_len--; + + if (pmdev->in_buf_len < len) { + len = pmdev->in_buf_len; + } + memcpy(dest, pmdev->in_buf, len); + return len; +} + static uint64_t pmbus_receive_uint(PMBusDevice *pmdev) { diff --git a/include/hw/i2c/pmbus_device.h b/include/hw/i2c/pmbus_device.h index 93f5d57c9d..7dc00cc4d9 100644 --- a/include/hw/i2c/pmbus_device.h +++ b/include/hw/i2c/pmbus_device.h @@ -501,6 +501,13 @@ void pmbus_send64(PMBusDevice *state, uint64_t data); */ void pmbus_send_string(PMBusDevice *state, const char *data); +/** + * @brief Receive data sent with Block Write. + * @param dest - memory with enough capacity to receive the write + * @param len - the capacity of dest + */ +uint8_t pmbus_receive_block(PMBusDevice *pmdev, uint8_t *dest, size_t len); + /** * @brief Receive data over PMBus * These methods help track how much data is being received over PMBus