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 for Android: free password hash cracker in your pocket

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

Date: Tue, 27 Feb 2024 15:12:55 +0100
From: Harald Mommer <harald.mommer@...nsynergy.com>
To: Mark Brown <broonie@...nel.org>
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 13.02.24 18:49, Mark Brown wrote:
> 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".
Replace link by Haixu Cui's diagram + explanations.The explanations were 
helpful so I wanted to keep this but the comment may now be too long to 
be accepted. We will see what happens.
>> +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;

I got the comment originally from you, Mark Brown. After some digging 
still unclear what should be wrong and in the meantime I think nothing 
is wrong at all. To point you with the nose on the pending issue I added 
this comment here.

I'll remove the comment because I think there is no problem. Please 
protest if I'm wrong.

>> +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?
Will be done, struct virtio_spi_req spi_req will become a member of 
struct virtio_spi_priv.
>> +	/*
>> +	 * 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.
Obviously I did not get the terminology of messages and transfers not 
correct which made the comment wrong. Comment to be corrected (and 
shortened).
>> +		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?

Lack of experience. I saw one way of doing the job which missing the 
more simple way. Therefore we have reviews. Using now the alternative 
callback which shortens and simplifies the code.

Applied code changes, have to run some more tests.