Re: [PATCH 1/3] hw/i2c: add support for PMBus

[Joel Stanley <joel@jms.id.au>]

On Tue, 4 May 2021 at 16:30, Titus Rwantare <titusr@google.com> wrote:
>
> QEMU has support for SMBus devices, and PMBus is a more specific
> implementation of SMBus. The additions made in this commit makes it easier to
> add new PMBus devices to QEMU.
>
> https://pmbus.org/specification-archives/

I'm not a pmbus expert, but I am happy that someone has created a
framework to model it. I've given them a read and some minor comments
below.

> Reviewed-by: Hao Wu <wuhaotsh@google.com>

Did this review happen on the mailing list? if not, I recommend doing
your review on the public lists, so we can see what comments Hao made.

> Signed-off-by: Titus Rwantare <titusr@google.com>

> +++ b/hw/i2c/pmbus_device.c
> @@ -0,0 +1,1611 @@
> +/*
> + * PMBus wrapper over SMBus
> + *
> + * Copyright 2021 Google LLC
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.

You can replace these two paragraphs with a SDPX line:

 SPDX-License-Identifier: GPL-2.0-or-later

In addition, add this to your git config to put the .h files on top :

git config diff.orderFile = /some/path/qemu.git/scripts/git.orderfile

It's easier for review.

I'd also suggest putting your tests in a separate patch, following the
addition of the model, again for easier review.

> + */
> +#include "qemu/osdep.h"
> +#include <math.h>
> +#include <string.h>
> +#include "hw/i2c/pmbus_device.h"
> +#include "qemu/module.h"
> +#include "qemu/log.h"
> +
> +uint16_t pmbus_data2direct_mode(PMBusCoefficients c, uint32_t value)
> +{
> +    /* R is usually negative to fit large readings into 16 bits */
> +    uint16_t y = (c.m * value + c.b) * pow(10, c.R);
> +    return y;
> +}
> +
> +uint32_t pmbus_direct_mode2data(PMBusCoefficients c, uint16_t value)
> +{
> +    /* X = (Y * 10^-R - b) / m */
> +    uint32_t x = (value / pow(10, c.R) - c.b) / c.m;
> +    return x;
> +}
> +
> +static void pmbus_send(PMBusDevice *pmdev, const uint8_t *data, uint16_t len)
> +{
> +    if (pmdev->out_buf_len + len > SMBUS_DATA_MAX_LEN) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "PMBus device tried to send too much data");
> +        len = 0;
> +    }
> +
> +    for (int i = len - 1; i >= 0; i--) {
> +        pmdev->out_buf[i + pmdev->out_buf_len] = data[len - i - 1];
> +    }
> +    pmdev->out_buf_len += len;
> +}
> +
> +/* Internal only, convert unsigned ints to the little endian bus */
> +static void pmbus_send_uint(PMBusDevice *pmdev, uint64_t data, uint8_t size)
> +{
> +    uint8_t bytes[8];

Do you need to assert that size is less than the array size? Probably
not as all the callers are local.

> +    for (int i = 0; i < size; i++) {
> +        bytes[i] = data & 0xFF;
> +        data = data >> 8;
> +    }
> +    pmbus_send(pmdev, bytes, size);
> +}
> +
> +void pmbus_send_block(PMBusDevice *pmdev, PMBusBlock block)
> +{
> +    pmbus_send(pmdev, block.buf, block.len);
> +}
> +
> +void pmbus_send8(PMBusDevice *pmdev, uint8_t data)
> +{
> +    pmbus_send_uint(pmdev, data, 1);
> +}
> +
> +void pmbus_send16(PMBusDevice *pmdev, uint16_t data)
> +{
> +    pmbus_send_uint(pmdev, data, 2);
> +}
> +
> +void pmbus_send32(PMBusDevice *pmdev, uint32_t data)
> +{
> +    pmbus_send_uint(pmdev, data, 4);
> +}
> +
> +void pmbus_send64(PMBusDevice *pmdev, uint64_t data)
> +{
> +    pmbus_send_uint(pmdev, data, 8);
> +}
> +
> +void pmbus_send_string(PMBusDevice *pmdev, const char *data)
> +{
> +    size_t len = strlen(data);

Do you need to assert that len is > 0?

> +    g_assert(len + pmdev->out_buf_len < SMBUS_DATA_MAX_LEN);
> +    pmdev->out_buf[len + pmdev->out_buf_len] = len;
> +
> +    for (int i = len - 1; i >= 0; i--) {
> +        pmdev->out_buf[i + pmdev->out_buf_len] = data[len - 1 - i];
> +    }
> +    pmdev->out_buf_len += len + 1;
> +}
> +
> +
> +static uint64_t pmbus_receive_uint(const uint8_t *buf, uint8_t len)
> +{
> +    uint64_t ret = 0;
> +
> +    /* Exclude command code from return value */
> +    buf++;
> +    len--;
> +
> +    for (int i = len - 1; i >= 0; i--) {
> +        ret = ret << 8 | buf[i];
> +    }
> +    return ret;
> +}
> +
> +uint8_t pmbus_receive8(PMBusDevice *pmdev)
> +{
> +    if (pmdev->in_buf_len - 1 != 1) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: length mismatch. Expected 1 byte, got %d bytes\n",
> +                      __func__, pmdev->in_buf_len - 1);
> +    }
> +    return (uint8_t) pmbus_receive_uint(pmdev->in_buf, pmdev->in_buf_len);

The casts are implicit in C, so you do not need to explicitly cast the
return types in this and the functions below.

> +}
> +
> +uint16_t pmbus_receive16(PMBusDevice *pmdev)
> +{
> +    if (pmdev->in_buf_len - 1 != 2) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: length mismatch. Expected 2 bytes, got %d bytes\n",
> +                      __func__, pmdev->in_buf_len - 1);
> +    }
> +    return (uint16_t) pmbus_receive_uint(pmdev->in_buf, pmdev->in_buf_len);
> +}
> +
> +uint32_t pmbus_receive32(PMBusDevice *pmdev)
> +{
> +    if (pmdev->in_buf_len - 1 != 4) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: length mismatch. Expected 4 bytes, got %d bytes\n",
> +                      __func__, pmdev->in_buf_len - 1);
> +    }
> +    return (uint32_t) pmbus_receive_uint(pmdev->in_buf, pmdev->in_buf_len);
> +}
> +
> +uint64_t pmbus_receive64(PMBusDevice *pmdev)
> +{
> +    if (pmdev->in_buf_len - 1 != 8) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: length mismatch. Expected 8 bytes, got %d bytes\n",
> +                      __func__, pmdev->in_buf_len - 1);
> +    }
> +    return (uint64_t) pmbus_receive_uint(pmdev->in_buf, pmdev->in_buf_len);
> +}
> +
> +PMBusBlock pmbus_receive_block(PMBusDevice *pmdev)
> +{
> +    PMBusBlock data = { pmdev->in_buf, pmdev->in_buf_len };
> +    return data;

Returning the local variable seems like a dangerous function to have.
You don't have any users of this function in the models you have
submitted.


> +static int pmbus_write_data(SMBusDevice *smd, uint8_t *buf, uint8_t len)
> +{
> +    PMBusDevice *pmdev = PMBUS_DEVICE(smd);
> +    PMBusDeviceClass *pmdc = PMBUS_DEVICE_GET_CLASS(pmdev);
> +    int ret = 0;
> +    uint8_t index;
> +
> +    if (len == 0) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: writing empty data\n", __func__);
> +        return -1;
> +    }

I was going to suggest you find a meaningful error code to return.
Poking around at this callback, it seems like there's not much error
checking going on at the call sites, so it's hard to make a
suggestion.

Cheers,

Joel