| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHp75VcbDBUf2cH_6rRqn5RCGSEOWqE85Yn3gDhYiJPhGf1S=Q@mail.gmail.com>
Date: Thu, 23 Mar 2023 13:06:04 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Brad Larson <blarson@....com>
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-mmc@...r.kernel.org, linux-spi@...r.kernel.org,
adrian.hunter@...el.com, alcooperx@...il.com, arnd@...db.de,
brendan.higgins@...ux.dev, briannorris@...omium.org,
brijeshkumar.singh@....com, catalin.marinas@....com,
davidgow@...gle.com, gsomlo@...il.com, gerg@...ux-m68k.org,
krzk@...nel.org, krzysztof.kozlowski+dt@...aro.org, lee@...nel.org,
lee.jones@...aro.org, broonie@...nel.org,
yamada.masahiro@...ionext.com, p.zabel@...gutronix.de,
piotrs@...ence.com, p.yadav@...com, rdunlap@...radead.org,
robh+dt@...nel.org, samuel@...lland.org, fancer.lancer@...il.com,
skhan@...uxfoundation.org, suravee.suthikulpanit@....com,
thomas.lendacky@....com, tonyhuang.sunplus@...il.com,
ulf.hansson@...aro.org, vaishnav.a@...com, will@...nel.org,
devicetree@...r.kernel.org
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 <blarson@....com> 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
Powered by blists - more mailing lists