linux-kernel - Re: [PATCH v2 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: <8fbb904c-8d24-4d00-bff8-65f1e4bad5bb@quicinc.com>
Date: Tue, 26 Mar 2024 17:26:09 +0800
From: Haixu Cui <quic_haixcui@...cinc.com>
To: Viresh Kumar <viresh.kumar@...aro.org>,
        Harald Mommer
	<harald.mommer@...nsynergy.com>
CC: Mark Brown <broonie@...nel.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 v2 3/3] virtio-spi: Add virtio SPI driver.

Hi all,

On 3/20/2024 11:12 AM, Viresh Kumar wrote:
> Hi Harald,
> 
> On 19-03-24, 16:05, Harald Mommer wrote:
>> On 18.03.24 10:39, Haixu Cui wrote:
>>> On 3/4/2024 11:43 PM, Harald Mommer wrote:
>>>> +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;
>>>> +    dev_set_drvdata(&vdev->dev, ctrl);
>>>
>>>      ctrl->dev.of_node is not set, so the child node cannot be parsed. I
>>> would say, it's necessary to support the virtio spi device node in the
>>> format:
>>
>>
>> What you most probably want to have here is
>>
>>    ctrl->dev.of_node = np;
> 
> Looking at how i2c-virtio does it, it should be tied to the device itself
> instead of its parent:

Yes, it is
         ctrl->dev.of_node = vdev->dev.of_node;

> 
> ctrl->dev.of_node = vdev->dev.of_node;
> 
>>>      virtio-spi@...13000 {
>>>          compatible = "virtio,mmio";
>>>          reg = <0x4b013000 0x1000>;
>>>          interrupts = <0 497 4>;
>>>
>>>          spi {
>>>              compatible = "virtio,device2d";
>>>              #address-cells = <1>;
>>>              #size-cells = <0>;
>>>
>>>              spidev {
>>>                  compatible = "xxx";
>>>                  reg = <0>;
>>>                  spi-max-frequency = <100000>;
>>>              };
>>>          };
>>>      };
> 
> Right, some work was done in the past to standardize these compatibles:
> 
> $ git log -p --stat --reverse 0d8c9e7d4b40..694a1116b405
> 
     Here I would like the inner layer "spidev", to match then probe the 
spidev driver, the "reg" is the chip select index. "spi-max-frequency" 
is not necessary, while It doesn't matter.

     You can also customize the inner layer to match your own driver.

     In my test, I set the cs max number as 1, and in device tree node 
inner layer, reg as 0. So certainly spidev driver probed and spidev0.0 
is added successfully.

     But then the driver proceed to the following code, chip select 
index 0 device is created again, the driver fail with log: "chipselect 0 
already in use".

         for (csi = 0; csi < ctrl->num_chipselect; csi++) {
             dev_dbg(&vdev->dev, "Setting up CS=%u\n", csi);
             board_info.chip_select = csi;

             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;

             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;
             }
         }


     I hope I made myself clear. Thank you, Harald and Kumar. Best regards.