Hello Jae,
On Tue, 8 Jan 2019 at 08:11, Jae Hyun Yoo <jae.hyun....@linux.intel.com> wrote:
>
> This commit adds driver implementation for PECI bus core into linux
> driver framework.
I would like to help you get this merged next release cycle, as we are
now carrying it in OpenBMC. I suggest we ask Greg to queue it up if
there are no objections after you've addressed my questions.
> +static u8 peci_aw_fcs(u8 *data, int len)
I was wondering what aw_fcs meant. I notice that later on you describe
it as an Assure Write Frame Check Sequence byte. You could add a
comment next to this function :)
Instead of casing to u8 every time you call this, you could have this
take a struct peci_xfer_msg * and cast when calling crc8.
> +{
> + return crc8(peci_crc8_table, data, (size_t)len, 0);
> +}
> +
> +static int __peci_xfer(struct peci_adapter *adapter, struct peci_xfer_msg
> *msg,
> + bool do_retry, bool has_aw_fcs)
> +{
> + ktime_t start, end;
> + s64 elapsed_ms;
> + int rc = 0;
> +
> + /**
These are for kerneldoc, and the comments aren't kerneldoc. Replace
them with /* instead.
> + * For some commands, the PECI originator may need to retry a command
> if
> + * the processor PECI client responds with a 0x8x completion code. In
> + * each instance, the processor PECI client may have started the
> + * operation but not completed it yet. When the 'retry' bit is set,
> the
> + * PECI client will ignore a new request if it exactly matches a
> + * previous valid request.
> + */
> +
> + if (do_retry)
> + start = ktime_get();
> +
> + do {
> + rc = adapter->xfer(adapter, msg);
> +
> + if (!do_retry || rc)
> + break;
> +
> + if (msg->rx_buf[0] == DEV_PECI_CC_SUCCESS)
> + break;
> +
> + /* Retry is needed when completion code is 0x8x */
> + if ((msg->rx_buf[0] & DEV_PECI_CC_RETRY_CHECK_MASK) !=
> + DEV_PECI_CC_NEED_RETRY) {
> + rc = -EIO;
> + break;
> + }
> +
> + /* Set the retry bit to indicate a retry attempt */
> + msg->tx_buf[1] |= DEV_PECI_RETRY_BIT;
> +
> + /* Recalculate the AW FCS if it has one */
> + if (has_aw_fcs)
> + msg->tx_buf[msg->tx_len - 1] = 0x80 ^
Can we guarantee that msg->tx_len will be set to non-zero whenever has_aw_fcs?
I suggest checking before doing the assignment in case a new caller is
added and they make a mistake.
> +static int peci_ioctl_get_dib(struct peci_adapter *adapter, void *vmsg)
> +{
> + struct peci_get_dib_msg *umsg = vmsg;
> + struct peci_xfer_msg msg;
> + int rc;
> +
> + msg.addr = umsg->addr;
> + msg.tx_len = GET_DIB_WR_LEN;
> + msg.rx_len = GET_DIB_RD_LEN;
> + msg.tx_buf[0] = GET_DIB_PECI_CMD;
> +
> + rc = peci_xfer(adapter, &msg);
Most of tx_buf is going to be uninitialised. I assume a well behaving
adapter->xfer will check this and only send the correct number of
bytes, but it might pay to zero out struct peci_xfer_msg in all of
these functions?
> + if (rc)
> + return rc;
> +
> + umsg->dib = le64_to_cpup((__le64 *)msg.rx_buf);
> +
> + return 0;
> +}
> +
> +#if IS_ENABLED(CONFIG_OF)
> +static struct peci_client *peci_of_register_device(struct peci_adapter
> *adapter,
> + struct device_node *node)
> +{
> + struct peci_board_info info = {};
> + struct peci_client *result;
> + const __be32 *addr_be;
> + int len;
> +
> + dev_dbg(&adapter->dev, "register %pOF\n", node);
> +
> + if (of_modalias_node(node, info.type, sizeof(info.type)) < 0) {
I don't understand why you're doing this. Won't this always be peci,
as your binding requires?
> + dev_err(&adapter->dev, "modalias failure on %pOF\n", node);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + addr_be = of_get_property(node, "reg", &len);
> + if (!addr_be || len < sizeof(*addr_be)) {
The second check looks suspicious.
You could fix it to check the expected length (4), or use of_property_read_u32.
> + dev_err(&adapter->dev, "invalid reg on %pOF\n", node);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + info.addr = be32_to_cpup(addr_be);
> + info.of_node = of_node_get(node);
> +
> + result = peci_new_device(adapter, &info);
> + if (!result)
Should you do an of_node_put here?
> + result = ERR_PTR(-EINVAL);
> +
> + of_node_put(node);
Why do you release the reference here?
> + return result;
> +}
> +
