by Andy Shevchenko

[permalink] [raw]

Subject: 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 <[email protected]> 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

2023-03-31 22:29:14

by Brad Larson

[permalink] [raw]

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

Hi Andy,

Thanks for the review.

On Thu, Mar 23, 2023 at 13:06 Andy Shevchenko <[email protected]> wrote:
> On Thu, Mar 23, 2023 at 2:11 AM Brad Larson <[email protected]> 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

Changed to 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 ?

Fixed the typo

> ...
>
>> +#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?

Added mod_devicetable.h.
Removed delay.h, fs.h and of_device.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.

Its a 96 byte struct with pointer followed by the reset controller.
The spi_device variable is accessed frequently and rcdev during
boot and ideally never again so if rcdev is mostly missing from
cache that is fine. Likely the address of spi_dev is also in
cache given it is periodically accessed.

> ...
>
>> + struct spi_transfer t[2] = { 0 };
>
> 0 is not needed.

Dropped the 0.

> ...
>
>> + 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;

Yes, created temp variable.

>
>> + if (ret)
>> + return -EFAULT;
>
> ...
>
>> + /* Verify and prepare spi message */
>
> SPI

Changed to SPI

>> + size = _IOC_SIZE(cmd);
>> + if ((size % sizeof(struct penctrl_spi_xfer)) != 0) {
>
> ' != 0' is redundant.

Fixed

>
>> + 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 % ...)

Yes, made this change.

>> + 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?

Yes, overkill, changed to:

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

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

Changed to:

if (copy_to_user((void __user *)msg->rx_buf, rx_buf, msg->len))

> ...
>
>> + struct spi_transfer t[2] = { 0 };
>
> 0 is redundant.

Dropped the 0.

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

Dropped the 0.

> Why is this an array?

Fixed, it's a single message and shouldn't be an array.

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

Fixed.

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

It can be and removed two lines with below result. Also changed the
variable spi_dev to spi which is the more common usage.

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");

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

Fixed.

>> + 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() ?

Fixed.

>> + 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.

Fixed this way

penctrl = kzalloc(sizeof(*penctrl), GFP_KERNEL);
if (!penctrl) {
ret = -ENOMEM;
goto free_cdev;
}

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

Fixed.

> ...
>
> + spi_set_drvdata(spi_dev, penctrl);
>
> Is it in use?

Not used and now dropped.

> ...
>
>> + penctrl->rcdev.of_node = spi_dev->dev.of_node;
>
> device_set_node();

Added: device_set_node(&spi->dev, dev_fwnode(dev));

> ...
>
>> + 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;

Yes, changed.

> ...
>
>> + device_destroy(penctrl_class, penctrl_devt);
>
> Are you sure this is the correct API?

Yes, however a probe error could call up to 5 APIs to clean up which resulted
in this update:

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;

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

Swapped these

Regards,
Brad