Re: [PATCH v12 15/15] soc: amd: Add support for AMD Pensando SoC Controller

On Thu, Mar 23, 2023 at 2:11 AM Brad Larson [off-list ref] 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.

...

+config AMD_PENSANDO_CTRL
+       tristate "AMD Pensando SoC Controller"
+       depends on SPI_MASTER=y
+       depends on (ARCH_PENSANDO && OF) || COMPILE_TEST
+       default y if ARCH_PENSANDO

       default ARCH_PENSANDO

?

+       select REGMAP_SPI
+       select MFD_SYSCON

...

+/*
+ * AMD Pensando SoC Controller
+ *
+ * Userspace interface and reset driver support for SPI connected Pensando SoC
+ * controller device.  This device is present in all Pensando SoC designs and
+ * contains board control/status regsiters and management IO support.

registers ?

+ *
+ * Copyright 2023 Advanced Micro Devices, Inc.
+ */

...

+#include <linux/cdev.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/fs.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_device.h>

Seems semi-random. Are you sure you use this and not missing mod_devicetable.h?

+#include <linux/reset-controller.h>
+#include <linux/spi/spi.h>

...

+struct penctrl_device {
+       struct spi_device *spi_dev;
+       struct reset_controller_dev rcdev;

Perhaps swapping these two might provide a better code generation.

+};

...

+       struct spi_transfer t[2] = { 0 };

0 is not needed.

...

+       if (_IOC_DIR(cmd) & _IOC_READ)
+               ret = !access_ok((void __user *)arg, _IOC_SIZE(cmd));
+       else if (_IOC_DIR(cmd) & _IOC_WRITE)
+               ret = !access_ok((void __user *)arg, _IOC_SIZE(cmd));

Maybe you should create a temporary variable as

    void __user *in = ... arg;

?

+       if (ret)
+               return -EFAULT;

...

+       /* Verify and prepare spi message */

SPI

+       size = _IOC_SIZE(cmd);
+       if ((size % sizeof(struct penctrl_spi_xfer)) != 0) {

' != 0' is redundant.

+               ret = -EINVAL;
+               goto done;
+       }
+       num_msgs = size / sizeof(struct penctrl_spi_xfer);

+       if (num_msgs == 0) {
+               ret = -EINVAL;
+               goto done;
+       }

Can be unified with a previous check as

if (size == 0 || size % ...)

+       msg = memdup_user((struct penctrl_spi_xfer __user *)arg, size);
+       if (!msg) {
+               ret = PTR_ERR(msg);
+               goto done;
+       }

...

+       if (copy_from_user((void *)(uintptr_t)tx_buf,
+                          (void __user *)msg->tx_buf, msg->len)) {

Why are all these castings here?

+               ret = -EFAULT;
+               goto done;
+       }

...

+       if (copy_to_user((void __user *)msg->rx_buf,
+                        (void *)(uintptr_t)rx_buf, msg->len))
+               ret = -EFAULT;

Ditto.

...

+       struct spi_transfer t[2] = { 0 };

0 is redundant.

...

+       struct spi_transfer t[1] = { 0 };

Ditto.

Why is this an array?

...

+       ret = spi_sync(spi_dev, &m);
+       return ret;

return spi_sync(...);

...

+       np = spi_dev->dev.parent->of_node;
+       ret = of_property_read_u32(np, "num-cs", &num_cs);

Why not simply device_property_read_u32()?

+       if (ret)
+               return dev_err_probe(&spi_dev->dev, ret,
+                                    "number of chip-selects not defined");

...

+       cdev = cdev_alloc();
+       if (!cdev) {
+               dev_err(&spi_dev->dev, "allocation of cdev failed");
+               ret = -ENOMEM;

ret = dev_err_probe(...);

+               goto cdev_failed;
+       }

...

+       ret = cdev_add(cdev, penctrl_devt, num_cs);
+       if (ret) {

+               dev_err(&spi_dev->dev, "register of cdev failed");

dev_err_probe() ?

+               goto cdev_delete;
+       }

...

+       penctrl = kzalloc(sizeof(*penctrl), GFP_KERNEL);
+       if (!penctrl) {

+               ret = -ENOMEM;
+               dev_err(&spi_dev->dev, "allocate driver data failed");

ret = dev_err_probe();
But we do not print memory allocation failure messages.

+               goto cdev_delete;
+       }

...

+               if (IS_ERR(dev)) {
+                       ret = IS_ERR(dev);
+                       dev_err(&spi_dev->dev, "error creating device\n");

ret = dev_err_probe();

+                       goto cdev_delete;
+               }
+               dev_dbg(&spi_dev->dev, "created device major %u, minor %d\n",
+                       MAJOR(penctrl_devt), cs);
+       }

...

+       spi_set_drvdata(spi_dev, penctrl);

Is it in use?

...

+       penctrl->rcdev.of_node = spi_dev->dev.of_node;

device_set_node();

...

+       ret = reset_controller_register(&penctrl->rcdev);
+       if (ret)
+               return dev_err_probe(&spi_dev->dev, ret,
+                                    "failed to register reset controller\n");
+       return ret;

return 0;

...

+       device_destroy(penctrl_class, penctrl_devt);

Are you sure this is the correct API?

+       return ret;

...

+#include <linux/types.h>
+#include <linux/ioctl.h>

Sorted?

-- 
With Best Regards,
Andy Shevchenko