| Message ID | 20220302015053.1984165-3-titusr@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | This patch series contains updates to PMBus in QEMU along with some PMBus device models for Renesas regulators. I have also added myself to MAINTAINERS as this code is in use daily, where I am responsible for it. | expand |
On 2/3/22 02:50, Titus Rwantare wrote: > Signed-off-by: Titus Rwantare <titusr@google.com> > --- > hw/i2c/pmbus_device.c | 41 ++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 40 insertions(+), 1 deletion(-) > static uint8_t pmbus_receive_byte(SMBusDevice *smd) > { > PMBusDevice *pmdev = PMBUS_DEVICE(smd); > PMBusDeviceClass *pmdc = PMBUS_DEVICE_GET_CLASS(pmdev); > uint8_t ret = 0xFF; > - uint8_t index = pmdev->page; > + uint8_t index; > > if (pmdev->out_buf_len != 0) { > ret = pmbus_out_buf_pop(pmdev); > return ret; > } > > + /* > + * Reading from all pages will return the value from page 0, > + * this is unspecified behaviour in general. > + */ > + if (pmdev->page == PB_ALL_PAGES) { > + index = 0; > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: tried to read from all pages\n", > + __func__); > + pmbus_cml_error(pmdev); > + } else if (pmdev->page > pmdev->num_pages - 1) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: page %d is out of range\n", > + __func__, pmdev->page); > + pmbus_cml_error(pmdev); > + return -1; This file returns a mix of 0xFF/-1 for error. It would be nice to pick one. Adding a definition (PMBUS_ERR_BYTE?) could help. Preferably with error unified: Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Ack. All errors for PMBus should ideally be reflected in status and status_cml registers instead of carrying meaning in return values. I'll have to separately go through the existing code to make it consistent. On Fri, 4 Mar 2022 at 16:08, Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> wrote: > > On 2/3/22 02:50, Titus Rwantare wrote: > > Signed-off-by: Titus Rwantare <titusr@google.com> > > --- > > hw/i2c/pmbus_device.c | 41 ++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 40 insertions(+), 1 deletion(-) > > > static uint8_t pmbus_receive_byte(SMBusDevice *smd) > > { > > PMBusDevice *pmdev = PMBUS_DEVICE(smd); > > PMBusDeviceClass *pmdc = PMBUS_DEVICE_GET_CLASS(pmdev); > > uint8_t ret = 0xFF; > > - uint8_t index = pmdev->page; > > + uint8_t index; > > > > if (pmdev->out_buf_len != 0) { > > ret = pmbus_out_buf_pop(pmdev); > > return ret; > > } > > > > + /* > > + * Reading from all pages will return the value from page 0, > > + * this is unspecified behaviour in general. > > + */ > > + if (pmdev->page == PB_ALL_PAGES) { > > + index = 0; > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "%s: tried to read from all pages\n", > > + __func__); > > + pmbus_cml_error(pmdev); > > + } else if (pmdev->page > pmdev->num_pages - 1) { > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "%s: page %d is out of range\n", > > + __func__, pmdev->page); > > + pmbus_cml_error(pmdev); > > + return -1; > > This file returns a mix of 0xFF/-1 for error. It would be nice > to pick one. Adding a definition (PMBUS_ERR_BYTE?) could help. > > Preferably with error unified: > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c index 07a45c99f9..93c746bab3 100644 --- a/hw/i2c/pmbus_device.c +++ b/hw/i2c/pmbus_device.c @@ -243,18 +243,47 @@ void pmbus_check_limits(PMBusDevice *pmdev) } } +/* assert the status_cml error upon receipt of malformed command */ +static void pmbus_cml_error(PMBusDevice *pmdev) +{ + for (int i = 0; i < pmdev->num_pages; i++) { + pmdev->pages[i].status_word |= PMBUS_STATUS_CML; + pmdev->pages[i].status_cml |= PB_CML_FAULT_INVALID_CMD; + } +} + static uint8_t pmbus_receive_byte(SMBusDevice *smd) { PMBusDevice *pmdev = PMBUS_DEVICE(smd); PMBusDeviceClass *pmdc = PMBUS_DEVICE_GET_CLASS(pmdev); uint8_t ret = 0xFF; - uint8_t index = pmdev->page; + uint8_t index; if (pmdev->out_buf_len != 0) { ret = pmbus_out_buf_pop(pmdev); return ret; } + /* + * Reading from all pages will return the value from page 0, + * this is unspecified behaviour in general. + */ + if (pmdev->page == PB_ALL_PAGES) { + index = 0; + qemu_log_mask(LOG_GUEST_ERROR, + "%s: tried to read from all pages\n", + __func__); + pmbus_cml_error(pmdev); + } else if (pmdev->page > pmdev->num_pages - 1) { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: page %d is out of range\n", + __func__, pmdev->page); + pmbus_cml_error(pmdev); + return -1; + } else { + index = pmdev->page; + } + switch (pmdev->code) { case PMBUS_PAGE: pmbus_send8(pmdev, pmdev->page); @@ -1038,6 +1067,7 @@ static int pmbus_write_data(SMBusDevice *smd, uint8_t *buf, uint8_t len) pmdev->page = pmbus_receive8(pmdev); return 0; } + /* loop through all the pages when 0xFF is received */ if (pmdev->page == PB_ALL_PAGES) { for (int i = 0; i < pmdev->num_pages; i++) { @@ -1048,6 +1078,15 @@ static int pmbus_write_data(SMBusDevice *smd, uint8_t *buf, uint8_t len) return 0; } + if (pmdev->page > pmdev->num_pages - 1) { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: page %u is out of range\n", + __func__, pmdev->page); + pmdev->page = 0; /* undefined behaviour - reset to page 0 */ + pmbus_cml_error(pmdev); + return -1; + } + index = pmdev->page; switch (pmdev->code) {
Signed-off-by: Titus Rwantare <titusr@google.com> --- hw/i2c/pmbus_device.c | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-)