linux-kernel - Re: [RFC PATCH v3 3/3] SPI: Add virtio SPI driver.

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3b8e947c-51fb-461e-bb9e-7bc1bb547471@sirena.org.uk>
Date: Tue, 13 Feb 2024 17:49:06 +0000
From: Mark Brown <broonie@...nel.org>
To: Harald Mommer <Harald.Mommer@...nsynergy.com>
Cc: virtio-dev@...ts.oasis-open.org, Haixu Cui <quic_haixcui@...cinc.com>,
	Viresh Kumar <viresh.kumar@...aro.org>, linux-spi@...r.kernel.org,
	linux-kernel@...r.kernel.org, quic_ztu@...cinc.com,
	Matti Moell <Matti.Moell@...nsynergy.com>,
	Mikhail Golubev <Mikhail.Golubev@...nsynergy.com>
Subject: Re: [RFC PATCH v3 3/3] SPI: Add virtio SPI driver.

On Tue, Feb 13, 2024 at 02:53:50PM +0100, Harald Mommer wrote:

> +/*
> + * See also
> + * https://lore.kernel.org/all/6171c1c3-55ba-4f74-ae60-764820cf1caf@quicinc.com
> + */
> +static int virtio_spi_set_delays(struct spi_transfer_head *th,
> +				 struct spi_device *spi,
> +				 struct spi_transfer *xfer)

Please write actual comments that can be read standalone, the reader has
absolutely no idea why they'd want to follow the link and there's
nothing being referenced by that "also".

> +static int virtio_spi_one_transfer(struct virtio_spi_req *spi_req,
> +				   struct spi_controller *ctrl,
> +				   struct spi_message *msg,
> +				   struct spi_transfer *xfer)

> +	/*
> +	 * Got comment: "The virtio spec for cs_change is *not* what the Linux
> +	 * cs_change field does, this will not do the right thing."
> +	 * TODO: Understand/discuss this, still unclear what may be wrong here
> +	 */
> +	th->cs_change = xfer->cs_change;

> +static int virtio_spi_transfer_one_message(struct spi_controller *ctrl,
> +					   struct spi_message *msg)
> +{
> +	struct virtio_spi_priv *priv = spi_controller_get_devdata(ctrl);
> +	struct virtio_spi_req *spi_req;
> +	struct spi_transfer *xfer;
> +	int ret = 0;
> +
> +	spi_req = kzalloc(sizeof(*spi_req), GFP_KERNEL);
> +	if (!spi_req) {
> +		ret = -ENOMEM;
> +		goto no_mem;
> +	}

Why not just allocate this once, it's not like it's possible to send
more than one message simultaneously?

> +	/*
> +	 * Simple implementation: Process message by message and wait for each
> +	 * message to be completed by the device side.
> +	 */
> +	list_for_each_entry(xfer, &msg->transfers, transfer_list) {

This is processing transfers within a message rather than messages.

> +		ret = virtio_spi_one_transfer(spi_req, ctrl, msg, xfer);
> +		if (ret)
> +			goto msg_done;
> +
> +		virtqueue_kick(priv->vq);
> +
> +		wait_for_completion(&spi_req->completion);
> +
> +		/* Read result from message */
> +		ret = (int)spi_req->result.result;
> +		if (ret)
> +			goto msg_done;

It's not clear why this isn't within _spi_transfer_one() and then we
don't just use a transfer_one() callback and factor everything out to
the core?

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)