| 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: <aKXePVShWzXGi8yP@smile.fi.intel.com>
Date: Wed, 20 Aug 2025 17:39:57 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Haixu Cui <quic_haixcui@...cinc.com>
Cc: harald.mommer@....qualcomm.com, quic_msavaliy@...cinc.com,
broonie@...nel.org, virtio-dev@...ts.linux.dev,
viresh.kumar@...aro.org, linux-spi@...r.kernel.org,
linux-kernel@...r.kernel.org, hdanton@...a.com,
qiang4.zhang@...ux.intel.com, alex.bennee@...aro.org,
quic_ztu@...cinc.com
Subject: Re: [PATCH v4 3/3] SPI: Add virtio SPI driver
On Wed, Aug 20, 2025 at 04:49:44PM +0800, Haixu Cui wrote:
> This is the virtio SPI Linux kernel driver.
...
> +#include <linux/completion.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/stddef.h>
> +#include <linux/virtio.h>
> +#include <linux/virtio_ring.h>
> +#include <linux/virtio_spi.h>
> +#define VIRTIO_SPI_MODE_MASK \
> + (SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_LSB_FIRST)
We have SPI_MODE_X_MASK.
...
> +struct virtio_spi_req {
> + struct completion completion;
> + struct spi_transfer_head transfer_head ____cacheline_aligned;
> + const u8 *tx_buf;
> + u8 *rx_buf;
> + struct spi_transfer_result result ____cacheline_aligned;
> +};
Dunno if `pahole` aware of ____cacheline_aligned attribute, but does it shows
any potential improvement?
I think the fields can be reshuffled to go last and only one needs that
attribute.
struct virtio_spi_req {
struct completion completion;
const u8 *tx_buf;
u8 *rx_buf;
struct spi_transfer_head transfer_head ____cacheline_aligned;
struct spi_transfer_result result;
};
...
> +static int virtio_spi_set_delays(struct spi_transfer_head *th,
> + struct spi_device *spi,
> + struct spi_transfer *xfer)
> +{
> + int cs_setup;
> + int cs_word_delay_xfer;
> + int cs_word_delay_spi;
> + int delay;
> + int cs_hold;
> + int cs_inactive;
> + int cs_change_delay;
> +
> + cs_setup = spi_delay_to_ns(&spi->cs_setup, xfer);
> + if (cs_setup < 0) {
Hmm... Not a problem in your code, but ns can be quite high for low speed
links, there is a potential overflow...
> + dev_warn(&spi->dev, "Cannot convert cs_setup\n");
> + return cs_setup;
> + }
> + th->cs_setup_ns = cpu_to_le32(cs_setup);
> +
> + cs_word_delay_xfer = spi_delay_to_ns(&xfer->word_delay, xfer);
> + if (cs_word_delay_xfer < 0) {
> + dev_warn(&spi->dev, "Cannot convert cs_word_delay_xfer\n");
> + return cs_word_delay_xfer;
> + }
> + cs_word_delay_spi = spi_delay_to_ns(&spi->word_delay, xfer);
> + if (cs_word_delay_spi < 0) {
> + dev_warn(&spi->dev, "Cannot convert cs_word_delay_spi\n");
> + return cs_word_delay_spi;
> + }
> + if (cs_word_delay_spi > cs_word_delay_xfer)
> + th->word_delay_ns = cpu_to_le32(cs_word_delay_spi);
> + else
> + th->word_delay_ns = cpu_to_le32(cs_word_delay_xfer);
Why not max() ?
> + delay = spi_delay_to_ns(&xfer->delay, xfer);
> + if (delay < 0) {
> + dev_warn(&spi->dev, "Cannot convert delay\n");
> + return delay;
> + }
> + cs_hold = spi_delay_to_ns(&spi->cs_hold, xfer);
> + if (cs_hold < 0) {
> + dev_warn(&spi->dev, "Cannot convert cs_hold\n");
> + return cs_hold;
> + }
> + th->cs_delay_hold_ns = cpu_to_le32(delay + cs_hold);
> +
> + cs_inactive = spi_delay_to_ns(&spi->cs_inactive, xfer);
> + if (cs_inactive < 0) {
> + dev_warn(&spi->dev, "Cannot convert cs_inactive\n");
> + return cs_inactive;
> + }
> + cs_change_delay = spi_delay_to_ns(&xfer->cs_change_delay, xfer);
> + if (cs_change_delay < 0) {
> + dev_warn(&spi->dev, "Cannot convert cs_change_delay\n");
> + return cs_change_delay;
> + }
> + th->cs_change_delay_inactive_ns =
> + cpu_to_le32(cs_inactive + cs_change_delay);
> +
> + return 0;
> +}
...
> +static int virtio_spi_transfer_one(struct spi_controller *ctrl,
> + struct spi_device *spi,
> + struct spi_transfer *xfer)
> +{
> + struct virtio_spi_priv *priv = spi_controller_get_devdata(ctrl);
> + struct virtio_spi_req *spi_req;
> + struct spi_transfer_head *th;
> + struct scatterlist sg_out_head, sg_out_payload;
> + struct scatterlist sg_in_result, sg_in_payload;
> + struct scatterlist *sgs[4];
> + unsigned int outcnt = 0u;
> + unsigned int incnt = 0u;
Are 'u':s important in this case/
> + int ret;
> +
> + spi_req = kzalloc(sizeof(*spi_req), GFP_KERNEL);
> + if (!spi_req)
> + return -ENOMEM;
> +
> + init_completion(&spi_req->completion);
> +
> + th = &spi_req->transfer_head;
> +
> + /* Fill struct spi_transfer_head */
> + th->chip_select_id = spi_get_chipselect(spi, 0);
> + th->bits_per_word = spi->bits_per_word;
> + th->cs_change = xfer->cs_change;
> + th->tx_nbits = xfer->tx_nbits;
> + th->rx_nbits = xfer->rx_nbits;
> + th->reserved[0] = 0;
> + th->reserved[1] = 0;
> + th->reserved[2] = 0;
> +
> + static_assert(VIRTIO_SPI_CPHA == SPI_CPHA,
> + "VIRTIO_SPI_CPHA must match SPI_CPHA");
> + static_assert(VIRTIO_SPI_CPOL == SPI_CPOL,
> + "VIRTIO_SPI_CPOL must match SPI_CPOL");
> + static_assert(VIRTIO_SPI_CS_HIGH == SPI_CS_HIGH,
> + "VIRTIO_SPI_CS_HIGH must match SPI_CS_HIGH");
> + static_assert(VIRTIO_SPI_MODE_LSB_FIRST == SPI_LSB_FIRST,
> + "VIRTIO_SPI_MODE_LSB_FIRST must match SPI_LSB_FIRST");
> +
> + th->mode = cpu_to_le32(spi->mode & VIRTIO_SPI_MODE_MASK);
> + if (spi->mode & SPI_LOOP)
> + th->mode |= cpu_to_le32(VIRTIO_SPI_MODE_LOOP);
> +
> + th->freq = cpu_to_le32(xfer->speed_hz);
> +
> + ret = virtio_spi_set_delays(th, spi, xfer);
> + if (ret)
> + goto msg_done;
> +
> + /* Set buffers */
> + spi_req->tx_buf = xfer->tx_buf;
> + spi_req->rx_buf = xfer->rx_buf;
> +
> + /* Prepare sending of virtio message */
> + init_completion(&spi_req->completion);
> +
> + sg_init_one(&sg_out_head, th, sizeof(*th));
> + sgs[outcnt] = &sg_out_head;
> + outcnt++;
> +
> + if (spi_req->tx_buf) {
> + sg_init_one(&sg_out_payload, spi_req->tx_buf, xfer->len);
> + sgs[outcnt] = &sg_out_payload;
> + outcnt++;
> + }
> +
> + if (spi_req->rx_buf) {
> + sg_init_one(&sg_in_payload, spi_req->rx_buf, xfer->len);
> + sgs[outcnt] = &sg_in_payload;
> + incnt++;
> + }
> +
> + sg_init_one(&sg_in_result, &spi_req->result,
> + sizeof(struct spi_transfer_result));
> + sgs[outcnt + incnt] = &sg_in_result;
> + incnt++;
> +
> + ret = virtqueue_add_sgs(priv->vq, sgs, outcnt, incnt, spi_req,
> + GFP_KERNEL);
> + if (ret)
> + goto msg_done;
> +
> + /* Simple implementation: There can be only one transfer in flight */
> + virtqueue_kick(priv->vq);
> +
> + wait_for_completion(&spi_req->completion);
> +
> + /* Read result from message and translate return code */
> + switch (spi_req->result.result) {
> + case VIRTIO_SPI_TRANS_OK:
> + break;
> + case VIRTIO_SPI_PARAM_ERR:
> + ret = -EINVAL;
> + break;
> + case VIRTIO_SPI_TRANS_ERR:
> + ret = -EIO;
> + break;
> + default:
> + ret = -EIO;
> + break;
> + }
> +
> +msg_done:
> + kfree(spi_req);
Can be called via __free() to simplify the error handling,
> + if (ret)
> + ctrl->cur_msg->status = ret;
> +
> + return ret;
> +}
...
> +static int virtio_spi_probe(struct virtio_device *vdev)
> +{
> + struct virtio_spi_priv *priv;
> + struct spi_controller *ctrl;
> + int err;
Out of a sudden it's named 'err'. Please, go through the code and make
style/naming/etc consistent.
> + u32 bus_num;
> +
> + ctrl = devm_spi_alloc_host(&vdev->dev, sizeof(*priv));
> + if (!ctrl)
> + return -ENOMEM;
> +
> + priv = spi_controller_get_devdata(ctrl);
> + priv->vdev = vdev;
> + vdev->priv = priv;
> + device_set_node(&ctrl->dev, dev_fwnode(&vdev->dev));
> + /* Setup ACPI node for controlled devices which will be probed through ACPI. */
> + ACPI_COMPANION_SET(&vdev->dev, ACPI_COMPANION(vdev->dev.parent));
This is strange. Either you need to put parent above in device_set_node() or
drop it here. Otherwise it's inconsistent. Needs a very good explanation what's
going on here...
> + dev_set_drvdata(&vdev->dev, ctrl);
> +
> + err = device_property_read_u32(&vdev->dev, "spi,bus-num", &bus_num);
> + if (!err && bus_num <= S16_MAX)
This is wrong. What is the bus_num value when err != 0?
And why do we even care about this?
> + ctrl->bus_num = bus_num;
> + else
> + ctrl->bus_num = -1;
> +
> + virtio_spi_read_config(vdev);
> +
> + ctrl->transfer_one = virtio_spi_transfer_one;
> +
> + err = virtio_spi_find_vqs(priv);
> + if (err) {
> + dev_err_probe(&vdev->dev, err, "Cannot setup virtqueues\n");
> + return err;
return dev_err_probe(...);
> + }
> + /* Register cleanup for virtqueues using devm */
> + err = devm_add_action_or_reset(&vdev->dev, (void (*)(void *))virtio_spi_del_vq, vdev);
> + if (err) {
> + dev_err_probe(&vdev->dev, err, "Cannot register virtqueue cleanup\n");
> + return err;
> + }
> +
> + /* Use devm version to register controller */
> + err = devm_spi_register_controller(&vdev->dev, ctrl);
> + if (err) {
> + dev_err_probe(&vdev->dev, err, "Cannot register controller\n");
> + return err;
As per above.
> + }
> +
> + return 0;
> +}
...
> +static int virtio_spi_freeze(struct device *dev)
> +{
> + struct spi_controller *ctrl = dev_get_drvdata(dev);
> + struct virtio_device *vdev = container_of(dev, struct virtio_device, dev);
Use dev_to_virtio()
> + int ret;
> +
> + ret = spi_controller_suspend(ctrl);
> + if (ret) {
> + dev_warn(dev, "cannot suspend controller (%d)\n", ret);
> + return ret;
> + }
> +
> + virtio_spi_del_vq(vdev);
> + return 0;
> +}
> +
> +static int virtio_spi_restore(struct device *dev)
> +{
> + struct spi_controller *ctrl = dev_get_drvdata(dev);
> + struct virtio_device *vdev = container_of(dev, struct virtio_device, dev);
As per above.
> + int ret;
> +
> + ret = virtio_spi_find_vqs(vdev->priv);
> + if (ret) {
> + dev_err(dev, "problem starting vqueue (%d)\n", ret);
> + return ret;
> + }
> +
> + ret = spi_controller_resume(ctrl);
> + if (ret)
> + dev_err(dev, "problem resuming controller (%d)\n", ret);
> +
> + return ret;
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists