| 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: <CAHp75VfRLv1=3M+a9pr=ZJgNwtBOrT9xi0UjDJMuY8uM9+ffSw@mail.gmail.com>
Date: Thu, 21 Sep 2023 18:19:57 +0300
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,
catalin.marinas@....com, conor+dt@...nel.org, davidgow@...gle.com,
gsomlo@...il.com, gerg@...ux-m68k.org, hal.feng@...rfivetech.com,
hasegawa-hitomi@...itsu.com, j.neuschaefer@....net, joel@....id.au,
kernel@...il.dk, krzk@...nel.org,
krzysztof.kozlowski+dt@...aro.org, lee@...nel.org,
lee.jones@...aro.org, broonie@...nel.org, p.zabel@...gutronix.de,
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, walker.chen@...rfivetech.com, will@...nel.org,
zhuyinbo@...ngson.cn, devicetree@...r.kernel.org
Subject: Re: [PATCH v16 6/6] soc: amd: Add support for AMD Pensando SoC Controller
On Thu, Sep 14, 2023 at 12:52 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.
...
> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include <linux/miscdevice.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/reset-controller.h>
> +#include <linux/spi/spi.h>
types.h ?
> +#include <linux/uaccess.h>
...
> + struct penctrl_device *penctrl;
> + u8 tx_buf[PENCTRL_MAX_MSG_LEN];
> + u8 rx_buf[PENCTRL_MAX_MSG_LEN];
These are not DMA-safe, is this a problem?
> + 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;
...
> + /* Verify and prepare SPI message */
> + size = _IOC_SIZE(cmd);
> + num_msgs = size / sizeof(struct penctrl_spi_xfer);
sizeof (*msg) ?
> + if (num_msgs > 2 || size == 0 || size % sizeof(struct penctrl_spi_xfer)) {
Dito.
> + ret = -EINVAL;
> + goto out_unlock;
> + }
...
> + msg = memdup_user((struct penctrl_spi_xfer *)arg, size);
> + if (IS_ERR(msg)) {
> + ret = PTR_ERR(msg);
> + goto out_unlock;
> + }
Wondering if you can start using cleanup.h.
...
> + /* Perform the transfer */
> + mutex_lock(&spi_lock);
> + ret = spi_sync(spi, &m);
> + mutex_unlock(&spi_lock);
> + if (ret || (num_msgs == 1))
> + goto out_unlock;
Second conditional will return 0. Is it by design?
Since it's not so obvious I would split these conditionals.
...
> + spi->chip_select = current_cs;
spi_set_chipselect()
...
> +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];
Not DMA-safe. Is it a problem?
> + int ret;
> + txbuf[0] = PENCTRL_SPI_CMD_REGRD;
> + txbuf[1] = reg;
> + txbuf[2] = 0;
Can be assigned in the definition block
u8 txbuf[] = { ... };
> + t[0].tx_buf = txbuf;
> + t[0].len = sizeof(txbuf);
> + rxbuf[0] = 0;
Ditto.
u8 rxbuf[] = { 0 };
> + t[1].rx_buf = rxbuf;
> + t[1].len = sizeof(rxbuf);
> +
> + spi_message_init_with_transfers(&m, t, ARRAY_SIZE(t));
> + ret = spi_sync(spi, &m);
> + if (ret)
> + return ret;
> +
> + *val = rxbuf[0];
> + return 0;
> +}
...
> +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;
Can be assigned in the definition block.
> + t.tx_buf = txbuf;
> + t.len = sizeof(txbuf);
> + spi_message_init_with_transfers(&m, &t, 1);
> + return spi_sync(spi, &m);
> +}
...
> + struct penctrl_device *penctrl =
> + container_of(rcdev, struct penctrl_device, rcdev);
One line?
...
> + spi->chip_select = 0;
spi_set_chipselect()
...
> + struct penctrl_device *penctrl =
> + container_of(rcdev, struct penctrl_device, rcdev);
One line?
...
> + spi->chip_select = 0;
spi_set_chipselect()
...
> +static int penctrl_spi_probe(struct spi_device *spi)
> +{
> + int i, ret;
> +
> + /* Allocate driver data */
> + penctrl = kzalloc(sizeof(*penctrl), GFP_KERNEL);
devm_kzalloc() ?
> + if (!penctrl)
> + return -ENOMEM;
> +
> + penctrl->spi = spi;
> + mutex_init(&spi_lock);
> +
> + for (i = 0; i < ARRAY_SIZE(penctrl_devices); i++) {
> + ret = misc_register(&penctrl_devices[i]);
> + if (ret) {
> + dev_err(&spi->dev, "Failed to register device %s\n",
> + penctrl_devices[i].name);
> + goto cleanup;
> + }
> + }
> +
> + /* Register reset controller */
> + penctrl->rcdev.dev = &spi->dev;
> + penctrl->rcdev.ops = &penctrl_reset_ops;
> + penctrl->rcdev.owner = THIS_MODULE;
> + penctrl->rcdev.of_node = spi->dev.of_node;
> + penctrl->rcdev.nr_resets = 1;
> + device_set_node(penctrl->rcdev.dev, dev_fwnode(&spi->dev));
> +
> + ret = reset_controller_register(&penctrl->rcdev);
> + if (ret)
> + return dev_err_probe(&spi->dev, ret,
> + "failed to register reset controller\n");
> + return 0;
> +cleanup:
err_cleanup: ?
> + for (i = 0; i < ARRAY_SIZE(penctrl_devices); i++) {
while (i--) {
> + if (penctrl_devices[i].this_device)
> + misc_deregister(&penctrl_devices[i]);
> + }
> + return ret;
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists