linux-kernel - Re: [PATCH v9 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: <5dcabe90-c25b-4af5-b51f-5cda7113b5f4@quicinc.com>
Date: Wed, 3 Sep 2025 17:04:46 +0800
From: Haixu Cui <quic_haixcui@...cinc.com>
To: Andy Shevchenko <andriy.shevchenko@...el.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>, <virtualization@...ts.linux-foundation.org>
Subject: Re: [PATCH v9 3/3] SPI: Add virtio SPI driver

Hi Andy,

On 9/1/2025 8:07 PM, Andy Shevchenko wrote:
> On Thu, Aug 28, 2025 at 05:34:51PM +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>
> 
> A lot of headers are still missing. See below.
> 
> ...
> 

This driver compiles successfully, and I believe all required 
definitions are resolved through indirect inclusion. For example, since 
I included virtio.h, there is no need to explicitly include device.h, 
scatterlist.h or types.h.

I avoided redundant #includes to keep the code clean and minimal.

If there are any essential headers I’ve overlooked, please feel free to 
highlight them—I’ll gladly include them in the next revision.



> 
>> +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 __free(kfree);
> 
> This is incorrect template. It's one of the exceptions when we mix code and
> definitions...
>> +	spi_req = kzalloc(sizeof(*spi_req), GFP_KERNEL);
> 
> ...so this should be
> 
> 	struct virtio_spi_req *spi_req __free(kfree) =
> 		kzalloc(sizeof(*spi_req), GFP_KERNEL);
> 
> (or on one line if you are okay with a 100 limit).
> 

I plan to update the code as follows:

struct virtio_spi_req *spi_req __free(kfree) = NULL;
spi_req = kzalloc(sizeof(*spi_req), GFP_KERNEL);
if(!spi_req)
     return -ENOMEM;

This follows the pattern used in 
virtio_net(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/virtio_net.c?h=v6.17-rc4#n3746)

I'd like to check if this style is acceptable here, thanks.



> 
>> +	if (ret || bus_num > S16_MAX)
> 
> + limits.h
> 
>> +		ctrl->bus_num = -1;
>> +	else
>> +		ctrl->bus_num = bus_num;
> 
> But why do we need this property at all? And where is it documented in the
> device tree bindings?

I’ve reviewed other SPI drivers in the kernel, and it seems that bus_num 
is not a required property in most cases. I’ll remove the related code 
in the next revision.

Thanks again.

BR
Haixu Cui