Messages in this thread | | | From | Joel Stanley <> | Date | Mon, 14 Jan 2019 21:43:13 +1030 | Subject | Re: [PATCH v10 03/12] peci: Add support for PECI bus driver core |
| |
Hello Jae,
On Tue, 8 Jan 2019 at 08:11, Jae Hyun Yoo <jae.hyun.yoo@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; > +} > +
| |