| 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: <aJn6Q3oPX3RyG22L@black.igk.intel.com>
Date: Mon, 11 Aug 2025 16:12:19 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Haixu Cui <quic_haixcui@...cinc.com>
Cc: broonie@...nel.org, virtio-dev@...ts.oasis-open.org,
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: [RFC PATCH v4 3/3] SPI: Add virtio SPI driver
On Tue, Apr 01, 2025 at 11:36:21AM +0800, Haixu Cui wrote:
...
> +VIRTIO SPI DRIVER
> +M: Haixu Cui <quic_haixcui@...cinc.com>
> +S: Maintained
> +F: drivers/spi/spi-virtio.c
> +F: include/uapi/linux/virtio_spi.h
This should have been started by the very first patch that brings a new file
into the kernel. Here I would expect only one line (file) being added. Have you
run checkpatch?
...
> +#include <linux/completion.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
Please, no. Try to follow IWYU principle.
> +#include <linux/module.h>
> +#include <linux/of.h>
Why? AFAIK we may avoid this in the new code. If even needed, there are device
property APIs and software nodes. But I believe the inclusions in this driver is
just a cargo cult, as I said follow Include What You Use principle.
> +#include <linux/spi/spi.h>
> +#include <linux/stddef.h>
> +#include <linux/virtio.h>
> +#include <linux/virtio_ring.h>
> +#include <linux/virtio_spi.h>
...
> +struct virtio_spi_req {
> + struct completion completion;
> + struct spi_transfer_head transfer_head ____cacheline_aligned;
> + const uint8_t *tx_buf ____cacheline_aligned;
Why ____cacheline_aligned for the *pointer*?
> + uint8_t *rx_buf ____cacheline_aligned;
Ditto.
> + struct spi_transfer_result result ____cacheline_aligned;
> +};
...
> + if (cs_word_delay_spi > cs_word_delay_xfer)
> + th->word_delay_ns = cpu_to_le32((u32)cs_word_delay_spi);
> + else
> + th->word_delay_ns = cpu_to_le32((u32)cs_word_delay_xfer);
Why explicit casting? What is the purpose? Same for other cases.
...
> + BUILD_BUG_ON(VIRTIO_SPI_CPHA != SPI_CPHA);
> + BUILD_BUG_ON(VIRTIO_SPI_CPOL != SPI_CPOL);
> + BUILD_BUG_ON(VIRTIO_SPI_CS_HIGH != SPI_CS_HIGH);
> + BUILD_BUG_ON(VIRTIO_SPI_MODE_LSB_FIRST != SPI_LSB_FIRST);
Make this to be static_assert():s as they give better error message.
...
> + th->mode = cpu_to_le32(spi->mode & (SPI_LSB_FIRST | SPI_CS_HIGH |
> + SPI_CPOL | SPI_CPHA));
We have _MODE_MASK, use it.
...
> + if ((spi->mode & SPI_LOOP) != 0)
' != 0' is redundant.
> + th->mode |= cpu_to_le32(VIRTIO_SPI_MODE_LOOP);
...
> + /* Read result from message and translate return code */
> + switch (priv->spi_req.result.result) {
> + case VIRTIO_SPI_TRANS_OK:
> + /* ret is 0 */
Why comment? Make it to be a code statement which also makes code robust to
subtle changes.
> + break;
> + case VIRTIO_SPI_PARAM_ERR:
> + ret = -EINVAL;
> + break;
> + case VIRTIO_SPI_TRANS_ERR:
> + ret = -EIO;
> + break;
> + default: /* Protocol violation */
> + ret = -EIO;
> + break;
> + }
...
> + if ((priv->mode_func_supported & VIRTIO_SPI_MF_SUPPORT_CPHA_1) != 0)
> + ctrl->mode_bits |= VIRTIO_SPI_CPHA;
> + if ((priv->mode_func_supported & VIRTIO_SPI_MF_SUPPORT_CPOL_1) != 0)
> + ctrl->mode_bits |= VIRTIO_SPI_CPOL;
> + if ((priv->mode_func_supported & VIRTIO_SPI_MF_SUPPORT_LSB_FIRST) != 0)
> + ctrl->mode_bits |= SPI_LSB_FIRST;
> + if ((priv->mode_func_supported & VIRTIO_SPI_MF_SUPPORT_LOOPBACK) != 0)
> + ctrl->mode_bits |= SPI_LOOP;
> + if ((tx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_DUAL) != 0)
> + ctrl->mode_bits |= SPI_TX_DUAL;
> + if ((tx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_QUAD) != 0)
> + ctrl->mode_bits |= SPI_TX_QUAD;
> + if ((tx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_OCTAL) != 0)
> + ctrl->mode_bits |= SPI_TX_OCTAL;
> + if ((rx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_DUAL) != 0)
> + ctrl->mode_bits |= SPI_RX_DUAL;
> + if ((rx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_QUAD) != 0)
> + ctrl->mode_bits |= SPI_RX_QUAD;
> + if ((rx_nbits_supported & VIRTIO_SPI_RX_TX_SUPPORT_OCTAL) != 0)
> + ctrl->mode_bits |= SPI_RX_OCTAL;
' != 0' is redundant in all of the above.
...
> +static int virtio_spi_find_vqs(struct virtio_spi_priv *priv)
> +{
> + struct virtqueue *vq;
> +
> + vq = virtio_find_single_vq(priv->vdev, virtio_spi_msg_done, "spi-rq");
> + if (IS_ERR(vq))
> + return (int)PTR_ERR(vq);
Why casting?
> + priv->vq = vq;
> + return 0;
> +}
...
> + ctrl->dev.of_node = vdev->dev.of_node;
> +
> + /*
> + * Setup ACPI node for controlled devices which will be probed through
> + * ACPI.
> + */
> + ACPI_COMPANION_SET(&vdev->dev, ACPI_COMPANION(vdev->dev.parent));
device_set_node();
(it might require to use dev_fwnode() from property.h)
...
> + err = device_property_read_u32(&ctrl->dev, "spi,bus-num", &bus_num);
Despite above, use &vdev->dev to read properties as this makes code clearer in
aspect of the device that provides the properties to begin with.
> + if (!err && bus_num <= S16_MAX)
> + ctrl->bus_num = (s16)bus_num;
Why casting?
> + else
> + ctrl->bus_num = -1;
...
> +static void virtio_spi_remove(struct virtio_device *vdev)
> +{
> + struct spi_controller *ctrl = dev_get_drvdata(&vdev->dev);
> +
> + /* Order: 1.) unregister controller, 2.) remove virtqueue */
> + spi_unregister_controller(ctrl);
> + virtio_spi_del_vq(vdev);
Wrap this to devm and drop the remove() completely.
> +}
...
> +static struct virtio_device_id virtio_spi_id_table[] = {
> + { VIRTIO_ID_SPI, VIRTIO_DEV_ANY_ID },
> + { 0 },
Remove trailing comma (it's a terminator) and also unneeded 0.
> +};
> +
> +static struct virtio_driver virtio_spi_driver = {
> + .driver.name = KBUILD_MODNAME,
Use standard pattern
.driver = {
.name = ...
},
> + .driver.owner = THIS_MODULE,
This field is set by module_virtio_driver(), isn't it?
> + .id_table = virtio_spi_id_table,
> + .probe = virtio_spi_probe,
> + .remove = virtio_spi_remove,
> + .freeze = pm_sleep_ptr(virtio_spi_freeze),
> + .restore = pm_sleep_ptr(virtio_spi_restore),
Why are these not a dev_pm_ops?
> +};
> +
Redundant blank line, remove.
> +module_virtio_driver(virtio_spi_driver);
> +MODULE_DEVICE_TABLE(virtio, virtio_spi_id_table);
Move this up to follow up the ID table initializer.
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists