[PATCH 07/14] peci: Add peci-aspeed controller driver

On Tue, 2021-07-13 at 00:04 +0200, Iwona Winiarska wrote:

Perhaps a note about how one might make this determination, or maybe a
general recommendation that if they are building for deployment on an
OpenBMC system say Y else say N?

Uh oh... this looks like a driver private data structure, and I know
there's a 'struct device' allocated in @controller. /me goes to check
->probe()...

+???????struct device *dev;
+???????void __iomem *base;
+???????struct clk *clk;
+???????struct reset_control *rst;
+???????int irq;
+???????spinlock_t lock; /* to sync completion status handling */
+???????struct completion xfer_complete;
+???????u32 status;
+???????u32 cmd_timeout_ms;
+???????u32 msg_timing;
+???????u32 addr_timing;
+???????u32 rd_sampling_point;
+???????u32 clk_div;
+};
+
+static inline struct aspeed_peci *to_aspeed_peci(struct peci_controller *a)
+{
+???????return container_of(a, struct aspeed_peci, controller);
+}
+
+static void aspeed_peci_init_regs(struct aspeed_peci *priv)
+{
+???????u32 val;
+
+???????val = FIELD_PREP(ASPEED_PECI_CTRL_CLK_DIV_MASK, ASPEED_PECI_CLK_DIV_DEFAULT);
+???????val |= ASPEED_PECI_CTRL_PECI_CLK_EN;
+???????writel(val, priv->base + ASPEED_PECI_CTRL);
+???????/*
+??????? * Timing negotiation period setting.
+??????? * The unit of the programmed value is 4 times of PECI clock period.
+??????? */
+???????val = FIELD_PREP(ASPEED_PECI_TIMING_MESSAGE_MASK, priv->msg_timing);
+???????val |= FIELD_PREP(ASPEED_PECI_TIMING_ADDRESS_MASK, priv->addr_timing);
+???????writel(val, priv->base + ASPEED_PECI_TIMING_NEGOTIATION);
+
+???????/* Clear interrupts */
+???????val = readl(priv->base + ASPEED_PECI_INT_STS) | ASPEED_PECI_INT_MASK;
+???????writel(val, priv->base + ASPEED_PECI_INT_STS);
+
+???????/* Set timing negotiation mode and enable interrupts */
+???????val = FIELD_PREP(ASPEED_PECI_TIMING_NEGO_SEL_MASK, ASPEED_PECI_1ST_BIT_OF_ADDR_NEGO);
+???????val |= ASPEED_PECI_INT_MASK;
+???????writel(val, priv->base + ASPEED_PECI_INT_CTRL);
+
+???????val = FIELD_PREP(ASPEED_PECI_CTRL_SAMPLING_MASK, priv->rd_sampling_point);
+???????val |= FIELD_PREP(ASPEED_PECI_CTRL_CLK_DIV_MASK, priv->clk_div);
+???????val |= ASPEED_PECI_CTRL_PECI_EN;
+???????val |= ASPEED_PECI_CTRL_PECI_CLK_EN;
+???????writel(val, priv->base + ASPEED_PECI_CTRL);

Do these MMIO access follow a standard? I.e. is there any possibility
to have a common / generic MMIO xfer function, but just pass in a
different base address discovered by the PECI device rather than a
fully custom xfer function per controller?

+}
+
+static inline int aspeed_peci_check_idle(struct aspeed_peci *priv)
+{
+???????u32 cmd_sts = readl(priv->base + ASPEED_PECI_CMD);
+
+???????if (FIELD_GET(ASPEED_PECI_CMD_STS_MASK, cmd_sts) == ASPEED_PECI_CMD_STS_ADDR_T_NEGO)
+???????????????aspeed_peci_init_regs(priv);
+
+???????return readl_poll_timeout(priv->base + ASPEED_PECI_CMD,
+???????????????????????????????? cmd_sts,
+???????????????????????????????? !(cmd_sts & ASPEED_PECI_CMD_IDLE_MASK),
+???????????????????????????????? ASPEED_PECI_IDLE_CHECK_INTERVAL_US,
+???????????????????????????????? ASPEED_PECI_IDLE_CHECK_TIMEOUT_US);
+}
+
+static int aspeed_peci_xfer(struct peci_controller *controller,
+?????????????????????????? u8 addr, struct peci_request *req)
+{
+???????struct aspeed_peci *priv = to_aspeed_peci(controller);
+???????unsigned long flags, timeout = msecs_to_jiffies(priv->cmd_timeout_ms);
+???????u32 peci_head;
+???????int ret;
+
+???????if (req->tx.len > ASPEED_PECI_DATA_BUF_SIZE_MAX ||
+?????????? req->rx.len > ASPEED_PECI_DATA_BUF_SIZE_MAX)
+???????????????return -EINVAL;
+
+???????/* Check command sts and bus idle state */
+???????ret = aspeed_peci_check_idle(priv);
+???????if (ret)
+???????????????return ret; /* -ETIMEDOUT */
+
+???????spin_lock_irqsave(&priv->lock, flags);
+???????reinit_completion(&priv->xfer_complete);
+
+???????peci_head = FIELD_PREP(ASPEED_PECI_TAGET_ADDR_MASK, addr) |
+?????????????????? FIELD_PREP(ASPEED_PECI_WRITE_LEN_MASK, req->tx.len) |
+?????????????????? FIELD_PREP(ASPEED_PECI_READ_LEN_MASK, req->rx.len);
+
+???????writel(peci_head, priv->base + ASPEED_PECI_RW_LENGTH);
+
+???????memcpy_toio(priv->base + ASPEED_PECI_W_DATA0, req->tx.buf,
+?????????????????? req->tx.len > 16 ? 16 : req->tx.len);
+???????if (req->tx.len > 16)
+???????????????memcpy_toio(priv->base + ASPEED_PECI_W_DATA4, req->tx.buf + 16,
+?????????????????????????? req->tx.len - 16);
+
+???????dev_dbg(priv->dev, "HEAD : 0x%08x\n", peci_head);
+???????print_hex_dump_bytes("TX : ", DUMP_PREFIX_NONE, req->tx.buf, req->tx.len);
+
+???????priv->status = 0;
+???????writel(ASPEED_PECI_CMD_FIRE, priv->base + ASPEED_PECI_CMD);
+???????spin_unlock_irqrestore(&priv->lock, flags);
+
+???????ret = wait_for_completion_interruptible_timeout(&priv->xfer_complete, timeout);
+???????if (ret < 0)
+???????????????return ret;
+
+???????if (ret == 0) {
+???????????????dev_dbg(priv->dev, "Timeout waiting for a response!\n");
+???????????????return -ETIMEDOUT;
+???????}
+
+???????spin_lock_irqsave(&priv->lock, flags);
+
+???????writel(0, priv->base + ASPEED_PECI_CMD);
+
+???????if (priv->status != ASPEED_PECI_INT_CMD_DONE) {
+???????????????spin_unlock_irqrestore(&priv->lock, flags);
+???????????????dev_dbg(priv->dev, "No valid response!\n");
+???????????????return -EIO;
+???????}
+
+???????spin_unlock_irqrestore(&priv->lock, flags);
+
+???????memcpy_fromio(req->rx.buf, priv->base + ASPEED_PECI_R_DATA0,
+???????????????????? req->rx.len > 16 ? 16 : req->rx.len);
+???????if (req->rx.len > 16)
+???????????????memcpy_fromio(req->rx.buf + 16, priv->base + ASPEED_PECI_R_DATA4,
+???????????????????????????? req->rx.len - 16);
+
+???????print_hex_dump_bytes("RX : ", DUMP_PREFIX_NONE, req->rx.buf, req->rx.len);

If dynamic debug is not enabled this will be an unconditional
printk(KERN_DEBUG.

I'm ok with dev_dbg() in slow paths, but in fast paths you should look
to tracing, or putting potentially heavyweight debug behind a
CONFIG_X_DEBUG option. I have seen Greg is even less of a fan of
dev_dbg().

+
+???????return 0;
+}
+
+static irqreturn_t aspeed_peci_irq_handler(int irq, void *arg)
+{
+???????struct aspeed_peci *priv = arg;
+???????u32 status;
+
+???????spin_lock(&priv->lock);
+???????status = readl(priv->base + ASPEED_PECI_INT_STS);
+???????writel(status, priv->base + ASPEED_PECI_INT_STS);
+???????priv->status |= (status & ASPEED_PECI_INT_MASK);
+
+???????/*
+??????? * In most cases, interrupt bits will be set one by one but also note
+??????? * that multiple interrupt bits could be set at the same time.
+??????? */
+???????if (status & ASPEED_PECI_INT_BUS_TIMEOUT)
+???????????????dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_BUS_TIMEOUT\n");
+
+???????if (status & ASPEED_PECI_INT_BUS_CONNECT)
+???????????????dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_BUS_CONNECT\n");
+
+???????if (status & ASPEED_PECI_INT_W_FCS_BAD)
+???????????????dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_W_FCS_BAD\n");
+
+???????if (status & ASPEED_PECI_INT_W_FCS_ABORT)
+???????????????dev_dbg_ratelimited(priv->dev, "ASPEED_PECI_INT_W_FCS_ABORT\n");

What's the utility of these debug statements? If they are for
development than maybe they are ok, if they are for debug in the field
I would make them counters and export them via debugfs, or sysfs if
you expect to always be able to debug these events in case a kernel in
the field has dev_dbg and debugfs disabled.

+
+???????/*
+??????? * All commands should be ended up with a ASPEED_PECI_INT_CMD_DONE bit
+??????? * set even in an error case.
+??????? */
+???????if (status & ASPEED_PECI_INT_CMD_DONE)
+???????????????complete(&priv->xfer_complete);
+
+???????spin_unlock(&priv->lock);
+
+???????return IRQ_HANDLED;
+}
+
+static void __sanitize_clock_divider(struct aspeed_peci *priv)
+{
+???????u32 clk_div;
+???????int ret;
+
+???????ret = device_property_read_u32(priv->dev, "clock-divider", &clk_div);
+???????if (ret) {
+???????????????clk_div = ASPEED_PECI_CLK_DIV_DEFAULT;
+???????} else if (clk_div > ASPEED_PECI_CLK_DIV_MAX) {
+???????????????dev_warn(priv->dev, "Invalid clock-divider: %u, Using default: %u\n",
+??????????????????????? clk_div, ASPEED_PECI_CLK_DIV_DEFAULT);
+
+???????????????clk_div = ASPEED_PECI_CLK_DIV_DEFAULT;
+???????}
+
+???????priv->clk_div = clk_div;
+}
+
+static void __sanitize_msg_timing(struct aspeed_peci *priv)
+{
+???????u32 msg_timing;
+???????int ret;
+
+???????ret = device_property_read_u32(priv->dev, "msg-timing", &msg_timing);
+???????if (ret) {
+???????????????msg_timing = ASPEED_PECI_MSG_TIMING_DEFAULT;
+???????} else if (msg_timing > ASPEED_PECI_MSG_TIMING_MAX) {
+???????????????dev_warn(priv->dev, "Invalid msg-timing : %u, Use default : %u\n",
+??????????????????????? msg_timing, ASPEED_PECI_MSG_TIMING_DEFAULT);
+
+???????????????msg_timing = ASPEED_PECI_MSG_TIMING_DEFAULT;
+???????}
+
+???????priv->msg_timing = msg_timing;
+}
+
+static void __sanitize_addr_timing(struct aspeed_peci *priv)
+{
+???????u32 addr_timing;
+???????int ret;
+
+???????ret = device_property_read_u32(priv->dev, "addr-timing", &addr_timing);
+???????if (ret) {
+???????????????addr_timing = ASPEED_PECI_ADDR_TIMING_DEFAULT;
+???????} else if (addr_timing > ASPEED_PECI_ADDR_TIMING_MAX) {
+???????????????dev_warn(priv->dev, "Invalid addr-timing : %u, Use default : %u\n",
+??????????????????????? addr_timing, ASPEED_PECI_ADDR_TIMING_DEFAULT);
+
+???????????????addr_timing = ASPEED_PECI_ADDR_TIMING_DEFAULT;
+???????}
+
+???????priv->addr_timing = addr_timing;
+}
+
+static void __sanitize_rd_sampling_point(struct aspeed_peci *priv)
+{
+???????u32 rd_sampling_point;
+???????int ret;
+
+???????ret = device_property_read_u32(priv->dev, "rd-sampling-point", &rd_sampling_point);
+???????if (ret) {
+???????????????rd_sampling_point = ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT;
+???????} else if (rd_sampling_point > ASPEED_PECI_RD_SAMPLING_POINT_MAX) {
+???????????????dev_warn(priv->dev, "Invalid rd-sampling-point: %u, Use default : %u\n",
+??????????????????????? rd_sampling_point, ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT);
+
+???????????????rd_sampling_point = ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT;
+???????}
+
+???????priv->rd_sampling_point = rd_sampling_point;
+}
+
+static void __sanitize_cmd_timeout(struct aspeed_peci *priv)
+{
+???????u32 timeout;
+???????int ret;
+
+???????ret = device_property_read_u32(priv->dev, "cmd-timeout-ms", &timeout);
+???????if (ret) {
+???????????????timeout = ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT;
+???????} else if (timeout > ASPEED_PECI_CMD_TIMEOUT_MS_MAX || timeout == 0) {
+???????????????dev_warn(priv->dev, "Invalid cmd-timeout-ms: %u, Use default: %u\n",
+??????????????????????? timeout, ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT);
+

For all of the same pattern like this above I would say "falling back
to: %u" otherwise "Use default" sounds like an action the platform
owner is expected to take.

Also, if the driver is correcting the issue does the log need to be
spammed with a warning? Is this 'info' or 'debug'?

+???????????????timeout = ASPEED_PECI_CMD_TIMEOUT_MS_DEFAULT;
+???????}
+
+???????priv->cmd_timeout_ms = timeout;
+}
+
+static void aspeed_peci_device_property_sanitize(struct aspeed_peci *priv)
+{
+???????__sanitize_clock_divider(priv);
+???????__sanitize_msg_timing(priv);
+???????__sanitize_addr_timing(priv);
+???????__sanitize_rd_sampling_point(priv);
+???????__sanitize_cmd_timeout(priv);
+}
+
+static void aspeed_peci_disable_clk(void *data)
+{
+???????clk_disable_unprepare(data);
+}
+
+static int aspeed_peci_init_ctrl(struct aspeed_peci *priv)
+{
+???????int ret;
+
+???????priv->clk = devm_clk_get(priv->dev, NULL);
+???????if (IS_ERR(priv->clk))
+???????????????return dev_err_probe(priv->dev, PTR_ERR(priv->clk), "Failed to get clk source\n");
+
+???????ret = clk_prepare_enable(priv->clk);
+???????if (ret) {
+???????????????dev_err(priv->dev, "Failed to enable clock\n");
+???????????????return ret;
+???????}
+
+???????ret = devm_add_action_or_reset(priv->dev, aspeed_peci_disable_clk, priv->clk);
+???????if (ret)
+???????????????return ret;
+
+???????aspeed_peci_device_property_sanitize(priv);
+
+???????aspeed_peci_init_regs(priv);
+
+???????return 0;
+}
+
+static int aspeed_peci_probe(struct platform_device *pdev)
+{
+???????struct aspeed_peci *priv;
+???????int ret;
+
+???????priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);

..."uh oh" from above confirmed. devm allocation lifetime and 'struct
device' lifetime are not compatible.

You can trigger use after free bugs by turning on
CONFIG_DEBUG_KOBJECT_RELEASE.

devm can be used to automatically unregister the peci_controller
device. The flow would be something like:

priv = devm_kzalloc(..., sizeof(*priv), ...);
controller = peci_controller_alloc(...);
if (IS_ERR(controller))
    return PTR_ERR(controller);
rc = devm_peci_controller_add(...)
if (rc)
    return rc;

This arranges for the peci_controller_alloc() to be undone by
put_device() in all cases. Internal to peci_controller_alloc() is
typical goto unwind allocation error handling.

+???????if (!priv)
+???????????????return -ENOMEM;
+
+???????priv->dev = &pdev->dev;
+???????dev_set_drvdata(priv->dev, priv);
+
+???????priv->base = devm_platform_ioremap_resource(pdev, 0);
+???????if (IS_ERR(priv->base))
+???????????????return PTR_ERR(priv->base);
+
+???????priv->irq = platform_get_irq(pdev, 0);
+???????if (!priv->irq)
+???????????????return priv->irq;
+
+???????ret = devm_request_irq(&pdev->dev, priv->irq, aspeed_peci_irq_handler,
+????????????????????????????? 0, "peci-aspeed-irq", priv);
+???????if (ret)
+???????????????return ret;
+
+???????init_completion(&priv->xfer_complete);
+???????spin_lock_init(&priv->lock);
+
+???????priv->controller.xfer = aspeed_peci_xfer;
+
+???????priv->rst = devm_reset_control_get(&pdev->dev, NULL);
+???????if (IS_ERR(priv->rst)) {
+???????????????dev_err(&pdev->dev, "Missing or invalid reset controller entry\n");
+???????????????return PTR_ERR(priv->rst);
+???????}
+???????reset_control_deassert(priv->rst);
+
+???????ret = aspeed_peci_init_ctrl(priv);
+???????if (ret)
+???????????????return ret;
+
+???????return peci_controller_add(&priv->controller, priv->dev);
+}
+
+static int aspeed_peci_remove(struct platform_device *pdev)
+{
+???????struct aspeed_peci *priv = dev_get_drvdata(&pdev->dev);
+
+???????peci_controller_remove(&priv->controller);
+???????reset_control_assert(priv->rst);
+

It's odd to have devm in the probe path and still publish a remove
handler, i.e. why not handle controller removal and reset via devm?
The example above with devm_peci_controller_add() already assumes
peci_controller_remove is triggered by devm, reset assert can be
managed the same way.

+???????return 0;
+}
+
+static const struct of_device_id aspeed_peci_of_table[] = {
+???????{ .compatible = "aspeed,ast2400-peci", },
+???????{ .compatible = "aspeed,ast2500-peci", },
+???????{ .compatible = "aspeed,ast2600-peci", },
+???????{ }
+};
+MODULE_DEVICE_TABLE(of, aspeed_peci_of_table);
+
+static struct platform_driver aspeed_peci_driver = {
+???????.probe? = aspeed_peci_probe,
+???????.remove = aspeed_peci_remove,
+???????.driver = {
+???????????????.name?????????? = "peci-aspeed",
+???????????????.of_match_table = aspeed_peci_of_table,
+???????},
+};
+module_platform_driver(aspeed_peci_driver);
+
+MODULE_AUTHOR("Ryan Chen [off-list ref]");
+MODULE_AUTHOR("Jae Hyun Yoo [off-list ref]");

Same comments about MODULE_AUTHOR from patch 6, i.e. make sure this is
not duplicating what MAINTAINERS and git log handle.

I'll pause here until you've had a chance to consider fixes to the
devm vs 'struct device' lifetime issue.

+MODULE_DESCRIPTION("ASPEED PECI driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(PECI);