Re: [PATCH v13 15/15] soc: amd: Add support for AMD Pensando SoC Controller
From: Andy Shevchenko
Date: Tue Apr 11 2023 - 05:21:26 EST
On Mon, Apr 10, 2023 at 9:48 PM Brad Larson <blarson@xxxxxxx> wrote:
>
> The Pensando SoC controller is a SPI connected companion device
> that is present in all Pensando SoC board designs. The essential
> board management registers are accessed on chip select 0 with
> board mgmt IO support accessed using additional chip selects.
...
> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/reset-controller.h>
> +#include <linux/spi/spi.h>
+ Blank line?
> +#include <linux/amd-pensando-ctrl.h>
...
> +struct penctrl_device {
> + struct spi_device *spi;
> + struct reset_controller_dev rcdev;
Try to swap them and check if the code will be smaller (it depends on
how often one or another member is being used),
> +};
...
> +static long
> +penctrl_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> + void __user *in_arg = (void __user *)arg;
> + struct penctrl_device *penctrl;
> + u8 tx_buf[PENCTRL_MAX_MSG_LEN];
> + u8 rx_buf[PENCTRL_MAX_MSG_LEN];
> + struct spi_transfer t[2] = {};
> + struct penctrl_spi_xfer *msg;
> + struct spi_device *spi;
> + unsigned int num_msgs;
> + struct spi_message m;
> + u32 size;
> + int ret;
> +
> + /* Check for a valid command */
> + if (_IOC_TYPE(cmd) != PENCTRL_IOC_MAGIC)
> + return -ENOTTY;
> +
> + if (_IOC_NR(cmd) > PENCTRL_IOC_MAXNR)
> + return -ENOTTY;
> +
> + if (_IOC_DIR(cmd) & _IOC_READ)
> + ret = !access_ok(in_arg, _IOC_SIZE(cmd));
> + else if (_IOC_DIR(cmd) & _IOC_WRITE)
> + ret = !access_ok(in_arg, _IOC_SIZE(cmd));
> +
Unneeded blank line.
> + if (ret)
> + return -EFAULT;
But it seems you can actually rewrite above in less lines:
if ((_IOC_DIR(cmd) & _IOC_READ) && !access_ok(in_arg, _IOC_SIZE(cmd)))
return -EFAULT;
if ((_IOC_DIR(cmd) & _IOC_WRITE) && !access_ok(in_arg, _IOC_SIZE(cmd)))
return -EFAULT;
> + /* Get a reference to the SPI device */
> + penctrl = filp->private_data;
> + if (!penctrl)
> + return -ESHUTDOWN;
> +
> + spi = spi_dev_get(penctrl->spi);
> + if (!spi)
> + return -ESHUTDOWN;
> +
> + /* Verify and prepare SPI message */
> + size = _IOC_SIZE(cmd);
> + num_msgs = size / sizeof(struct penctrl_spi_xfer);
> + if (size == 0 || size % sizeof(struct penctrl_spi_xfer)) {
> + ret = -EINVAL;
> + goto done;
> + }
> + msg = memdup_user((struct penctrl_spi_xfer *)arg, size);
> + if (!msg) {
> + ret = PTR_ERR(msg);
This is strange.
> + goto done;
> + }
> + if (msg->len > PENCTRL_MAX_MSG_LEN) {
> + ret = -EINVAL;
> + goto done;
> + }
> +
> + t[0].tx_buf = tx_buf;
> + t[0].len = msg->len;
> + if (copy_from_user(tx_buf, (void __user *)msg->tx_buf, msg->len)) {
> + ret = -EFAULT;
> + goto done;
> + }
> + if (num_msgs > 1) {
> + msg++;
> + if (msg->len > PENCTRL_MAX_MSG_LEN) {
> + ret = -EINVAL;
> + goto done;
> + }
> + t[1].rx_buf = rx_buf;
> + t[1].len = msg->len;
> + }
> + spi_message_init_with_transfers(&m, t, num_msgs);
It seems there is no validation for the messages 3+.
> + /* Perform the transfer */
> + mutex_lock(&spi_lock);
> + ret = spi_sync(spi, &m);
> + mutex_unlock(&spi_lock);
> +
> + if (ret || (num_msgs == 1))
> + goto done;
> +
> + if (copy_to_user((void __user *)msg->rx_buf, rx_buf, msg->len))
> + ret = -EFAULT;
> +done:
out_unlock: ?
> + spi_dev_put(spi);
> + return ret;
> +}
> +
> +static int penctrl_open(struct inode *inode, struct file *filp)
> +{
> + struct spi_device *spi;
> + u8 current_cs;
> + if (!penctrl)
> + return -ENODEV;
Is it possible?
> + filp->private_data = penctrl;
> + current_cs = iminor(inode);
> + spi = penctrl->spi;
> + spi->chip_select = current_cs;
> + spi->cs_gpiod = spi->controller->cs_gpiods[current_cs];
Hmm... Why do you need this one? Isn't it a job of SPI core?
> + spi_setup(spi);
> + return stream_open(inode, filp);
> +}
> +static int penctrl_regs_read(struct penctrl_device *penctrl, u32 reg, u32 *val)
> +{
> + struct spi_device *spi = penctrl->spi;
> + struct spi_transfer t[2] = {};
> + struct spi_message m;
> + u8 txbuf[3];
> + u8 rxbuf[1];
> + int ret;
> +
> + txbuf[0] = PENCTRL_SPI_CMD_REGRD;
> + txbuf[1] = reg;
> + txbuf[2] = 0;
> + t[0].tx_buf = txbuf;
> + t[0].len = 3;
sizeof(txbuf) ?
> + rxbuf[0] = 0;
> + t[1].rx_buf = rxbuf;
> + t[1].len = 1;
sizeof(rxbuf) ?
> + spi_message_init_with_transfers(&m, t, ARRAY_SIZE(t));
> + ret = spi_sync(spi, &m);
> + if (ret == 0)
> + *val = rxbuf[0];
> +
> + return ret;
> +}
> +
> +static int penctrl_regs_write(struct penctrl_device *penctrl, u32 reg, u32 val)
> +{
> + struct spi_device *spi = penctrl->spi;
> + struct spi_transfer t;
> + struct spi_message m;
> + u8 txbuf[4];
> +
> + txbuf[0] = PENCTRL_SPI_CMD_REGWR;
> + txbuf[1] = reg;
> + txbuf[2] = val;
> + txbuf[3] = 0;
> +
> + t.tx_buf = txbuf;
> + t.len = 4;
sizeof(txbuf) ?
> + spi_message_init_with_transfers(&m, &t, 1);
> + return spi_sync(spi, &m);
> +}
> +
> +static int penctrl_reset_assert(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct penctrl_device *penctrl =
> + container_of(rcdev, struct penctrl_device, rcdev);
> + struct spi_device *spi = penctrl->spi;
> + unsigned int val;
> + int ret;
> +
> + mutex_lock(&spi_lock);
> + spi->chip_select = 0;
> + spi->cs_gpiod = spi->controller->cs_gpiods[0];
> + spi_setup(spi);
> + ret = penctrl_regs_read(penctrl, PENCTRL_REG_CTRL0, &val);
> + if (ret) {
> + dev_err(&spi->dev, "error reading ctrl0 reg\n");
> + goto done;
> + }
> +
> + val |= BIT(6);
> + ret = penctrl_regs_write(penctrl, PENCTRL_REG_CTRL0, val);
> + if (ret)
> + dev_err(&spi->dev, "error writing ctrl0 reg\n");
> +done:
out_unlock: ?
> + mutex_unlock(&spi_lock);
> + return ret;
> +}
> +
> +static int penctrl_reset_deassert(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct penctrl_device *penctrl =
> + container_of(rcdev, struct penctrl_device, rcdev);
> + struct spi_device *spi = penctrl->spi;
> + unsigned int val;
> + int ret;
> +
> + mutex_lock(&spi_lock);
> + spi->chip_select = 0;
> + spi->cs_gpiod = spi->controller->cs_gpiods[0];
> + spi_setup(spi);
> + ret = penctrl_regs_read(penctrl, PENCTRL_REG_CTRL0, &val);
> + if (ret) {
> + dev_err(&spi->dev, "error reading ctrl0 reg\n");
> + goto done;
> + }
> +
> + val &= ~BIT(6);
> + ret = penctrl_regs_write(penctrl, PENCTRL_REG_CTRL0, val);
> + if (ret)
> + dev_err(&spi->dev, "error writing ctrl0 reg\n");
> +done:
out_unlock: ?
> + mutex_unlock(&spi_lock);
> + return ret;
> +}
> +static int penctrl_spi_probe(struct spi_device *spi)
> +{
> + struct device *dev;
> + struct cdev *cdev;
> + u32 num_cs;
> + int ret;
> + u32 cs;
> +
> + ret = device_property_read_u32(spi->dev.parent, "num-cs", &num_cs);
> + if (ret)
> + return dev_err_probe(&spi->dev, ret,
> + "number of chip-selects not defined\n");
> +
> + ret = alloc_chrdev_region(&penctrl_devt, 0, num_cs, "penctrl");
> + if (ret)
> + return dev_err_probe(&spi->dev, ret,
> + "failed to alloc chrdev region\n");
> +
> + penctrl_class = class_create(THIS_MODULE, "penctrl");
> + if (IS_ERR(penctrl_class)) {
> + ret = dev_err_probe(&spi->dev, PTR_ERR(penctrl_class),
> + "failed to create class\n");
> + goto unregister_chrdev;
> + }
> +
> + cdev = cdev_alloc();
> + if (!cdev) {
> + ret = dev_err_probe(&spi->dev, -ENOMEM,
> + "allocation of cdev failed\n");
> + goto destroy_class;
> + }
> + cdev->owner = THIS_MODULE;
> + cdev_init(cdev, &penctrl_fops);
> +
> + ret = cdev_add(cdev, penctrl_devt, num_cs);
> + if (ret) {
> + ret = dev_err_probe(&spi->dev, ret,
> + "register of cdev failed\n");
> + goto free_cdev;
> + }
> +
> + /* Allocate driver data */
> + penctrl = kzalloc(sizeof(*penctrl), GFP_KERNEL);
> + if (!penctrl) {
> + ret = -ENOMEM;
> + goto free_cdev;
> + }
> + penctrl->spi = spi;
> + mutex_init(&spi_lock);
> +
> + /* Create a device for each chip select */
> + for (cs = 0; cs < num_cs; cs++) {
> + dev = device_create(penctrl_class,
> + &spi->dev,
> + MKDEV(MAJOR(penctrl_devt), cs),
> + penctrl,
> + "penctrl0.%d",
> + cs);
> + if (IS_ERR(dev)) {
> + ret = dev_err_probe(&spi->dev, PTR_ERR(dev),
> + "error creating device\n");
> + goto destroy_device;
> + }
> + dev_dbg(&spi->dev, "created device major %u, minor %d\n",
> + MAJOR(penctrl_devt), cs);
> + }
> +
> + /* Register emmc hardware reset */
> + penctrl->rcdev.nr_resets = 1;
> + penctrl->rcdev.owner = THIS_MODULE;
> + penctrl->rcdev.dev = &spi->dev;
> + penctrl->rcdev.ops = &penctrl_reset_ops;
> + penctrl->rcdev.of_node = spi->dev.of_node;
Either redundant or wrong. Shouldn't you first have the firmware node
to be set for spi->dev?
> + device_set_node(&spi->dev, dev_fwnode(dev));
> +
> + ret = reset_controller_register(&penctrl->rcdev);
> + if (ret)
> + return dev_err_probe(&spi->dev, ret,
> + "failed to register reset controller\n");
> + return 0;
> +
> +destroy_device:
> + for (cs = 0; cs < num_cs; cs++)
> + device_destroy(penctrl_class, MKDEV(MAJOR(penctrl_devt), cs));
> + kfree(penctrl);
> +free_cdev:
> + cdev_del(cdev);
> +destroy_class:
> + class_destroy(penctrl_class);
> +unregister_chrdev:
> + unregister_chrdev(MAJOR(penctrl_devt), "penctrl");
> +
> + return ret;
> +}
...
> +++ b/include/uapi/linux/amd-pensando-ctrl.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Userspace interface for /dev/penctrl
> + *
> + * This file can be used by applications that need to communicate
> + * with the AMD Pensando SoC controller device via the ioctl interface.
> + */
> +#ifndef _UAPI_LINUX_AMD_PENSANDO_CTRL_H
> +#define _UAPI_LINUX_AMD_PENSANDO_CTRL_H
> +#include <linux/ioctl.h>
Not used header.
> +#include <linux/types.h>
> +
> +#define PENCTRL_SPI_CMD_REGRD 0x0b
> +#define PENCTRL_SPI_CMD_REGWR 0x02
> +#define PENCTRL_IOC_MAGIC 'k'
> +#define PENCTRL_IOC_MAXNR 0
> +#define PENCTRL_MAX_MSG_LEN 16
> +#define PENCTRL_MAX_REG 0xff
> +#define PENCTRL_REG_CTRL0 0x10
> +
> +struct penctrl_spi_xfer {
> + __u64 tx_buf;
> + __u64 rx_buf;
> + __u32 len;
> + __u32 speed_hz;
> + __u64 compat;
> +};
> +
> +#endif /* _UAPI_LINUX_AMD_PENSANDO_CTRL_H */
--
With Best Regards,
Andy Shevchenko