linux-kernel - Re: [PATCH v3 3/3] virtio-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: <9442d3c7-ba72-44ee-8370-6a7bd53c2ac7@quicinc.com>
Date: Mon, 23 Dec 2024 15:34:45 +0800
From: Haixu Cui <quic_haixcui@...cinc.com>
To: Qiang Zhang <qiang4.zhang@...ux.intel.com>,
        Harald Mommer
	<Harald.Mommer@...nsynergy.com>
CC: <virtio-dev@...ts.oasis-open.org>, Mark Brown <broonie@...nel.org>,
        "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: [PATCH v3 3/3] virtio-spi: Add virtio SPI driver.

Hi @Qiang Zhang,

     Thank you for your comments, and please refer to my response below. 
I would co-work with Harald on this driver.

On 9/9/2024 8:59 PM, Qiang Zhang wrote:
> Hi, Harald
> 
> On Tue, Mar 26, 2024 at 12:28:12PM +0100, Harald Mommer wrote:
>> From: Harald Mommer <harald.mommer@...nsynergy.com>
>>
>> This is the virtio SPI Linux kernel driver.
>>
>> Signed-off-by: Harald Mommer <Harald.Mommer@...nsynergy.com>
> [...]
>> +static int virtio_spi_probe(struct virtio_device *vdev)
>> +{
>> +	struct device_node *np = vdev->dev.parent->of_node;
>> +	struct virtio_spi_priv *priv;
>> +	struct spi_controller *ctrl;
>> +	int err;
>> +	u32 bus_num;
>> +	u16 csi;
>> +
>> +	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;
>> +	ctrl->dev.of_node = vdev->dev.of_node;
> 
> To support ACPI, ACPI node of the controller needs to be set:
> 
> 	/*
> 	 * Setup ACPI node for controlled devices which will be probed through
> 	 * ACPI.
> 	 */
> 	ACPI_COMPANION_SET(&vdev->dev, ACPI_COMPANION(vdev->dev.parent));
> 

Yes, will add in next patch.

>> +	dev_set_drvdata(&vdev->dev, ctrl);
>> +
>> +	init_completion(&priv->spi_req.completion);
>> +
>> +	err = of_property_read_u32(np, "spi,bus-num", &bus_num);
>> +	if (!err && bus_num <= S16_MAX)
>> +		ctrl->bus_num = (s16)bus_num;
> 
> This is not the right way to fix bus number. You can use OF alias.
> Also, to work with ACPI, we should use common API like
> device_property_read_u32.
> 

I agree with you, "spi,bus-num" should not be mandatory in the device tree.

How about updating as follows:

+	err = device_property_read_u32(ctrl->dev, "spi,bus-num", &bus_num);
+	if (!err && bus_num <= S16_MAX)
+		ctrl->bus_num = (s16)bus_num;
+	else
+		ctrl->bus_num = -1;

So if "spi,bus-num" not set, bus_num is initialized as -1, then in 
function spi_register_controller, bus_num will be reassigned by OF 
alias(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/spi/spi.c?h=v6.13-rc4#n3287), 
or dynamic 
allocated(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/spi/spi.c?h=v6.13-rc4#n3295). 


>> +
>> +	virtio_spi_read_config(vdev);
>> +
>> +	ctrl->transfer_one = virtio_spi_transfer_one;
>> +
>> +	err = virtio_spi_find_vqs(priv);
>> +	if (err) {
>> +		dev_err(&vdev->dev, "Cannot setup virtqueues\n");
>> +		return err;
>> +	}
>> +
>> +	board_info.max_speed_hz = priv->max_freq_hz;
>> +	board_info.bus_num = (u16)ctrl->bus_num;
>> +
>> +	if (!(priv->mode_func_supported & VIRTIO_SPI_CS_HIGH))
>> +		board_info.mode = SPI_MODE_0;
>> +	else
>> +		board_info.mode = SPI_MODE_0 | SPI_CS_HIGH;
>> +
>> +	err = spi_register_controller(ctrl);
>> +	if (err) {
>> +		dev_err(&vdev->dev, "Cannot register controller\n");
>> +		goto err_return;
>> +	}
>> +
>> +	if (vdev->dev.of_node) {
>> +		dev_dbg(&vdev->dev, "Final setup triggered by DT child node\n");
>> +		return 0;
>> +	}
> How about ACPI path?
>> +
>> +	/* Add chip selects to controller */
>> +	for (csi = 0; csi < ctrl->num_chipselect; csi++) {
>> +		dev_dbg(&vdev->dev, "Setting up CS=%u\n", csi);
>> +		board_info.chip_select = csi;
>> +
>> +		if (!spi_new_device(ctrl, &board_info)) {
>> +			dev_err(&vdev->dev, "Cannot setup device %u\n", csi);
>> +			spi_unregister_controller(ctrl);
>> +			err = -ENODEV;
>> +			goto err_return;
>> +		}
>> +	}
> Just enumerate SPI devices via DT/ACPI. And a fixed SPI modalias "spi-virtio"
> is no better than match method from DT/ACPI.

How about deleting the following code.

+	if (vdev->dev.of_node) {
+		dev_dbg(&vdev->dev, "Final setup triggered by DT child node\n");
+		return 0;
+	}

+	/* Add chip selects to controller */
+	for (csi = 0; csi < ctrl->num_chipselect; csi++) {
+		dev_dbg(&vdev->dev, "Setting up CS=%u\n", csi);
+		board_info.chip_select = csi;
+
+		if (!spi_new_device(ctrl, &board_info)) {
+			dev_err(&vdev->dev, "Cannot setup device %u\n", csi);
+			spi_unregister_controller(ctrl);
+			err = -ENODEV;
+			goto err_return;
+		}
+	}

Then we can use the ACPI implementation in 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/spi/spi.c?h=v6.13-rc4#n3378.

Also creating SPI devices statically based on the board info seems 
somewhat unreasonable. I agree that DT/ACPI are the only reasonable way 
to specify the SPI nodes.


Thanks again
Haixu Cui

> 
> Thanks,
> Qiang