linux-kernel - Re: [RFC PATCH v4 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: <b5a6a9d6-50da-4ab9-878d-d4dbad270f22@quicinc.com>
Date: Fri, 25 Apr 2025 11:45:44 +0800
From: Haixu Cui <quic_haixcui@...cinc.com>
To: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>, <broonie@...nel.org>,
        <virtio-dev@...ts.oasis-open.org>, <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>
CC: <quic_ztu@...cinc.com>
Subject: Re: [RFC PATCH v4 3/3] SPI: Add virtio SPI driver

Hi Mukesh,

Thank you for your detailed review and valuable feedback.
I have carefully considered your comments and would like to
address them as follows:

> Could you please help add exact function description which can help 
> understand below details with some background/purpose ?
> 

Different delay parameters impact various phases of SPI transfers,
governing the assertion/deassertion of the CS and the timing intervals
between adjacent words in a single transfer or between consecutive
transfers.

Some delays are inherent device attributes, such as word_delay and
cs_setup in the spi_device structure, while others are specified
per-transfer via fields in the spi_transfer structure, such as
cs_change_delay and delay.

Although virtio SPI cannot directly configure transfer-stage delays by
manipulating hardware registers or precisely control CS signal behavior,
it can indirectly achieve fine-grained transfer control by passing
parameters like cs_delay_hold_ns and cs_setup_ns in the
spi_transfer_head structure to the host.

Function virtio_spi_set_delays is designed to implement this feature,
and will add description like:

/*
  * virtio_spi_set_delays - Set delay parameters for SPI transfer
  *
  * This function sets various delay parameters for SPI transfer,
  * including delay after CS asserted, timing intervals between
  * adjacent words within a transfer, delay before abdafter CS
  * deasserted. It converts these delay parameters to nanoseconds using
  * spi_delay_to_ns and stores the results in spi_transfer_head
  * structure.
  * If the conversion fails, the function logs a warning message and
  * returns an error code.
  */

Could you please review this comment and let me know if it's clear
and easy to understand? Your feedback will be greatly appreciated.

>> + * E => struct spi_device -> cs_inactive
>> + * F => struct spi_transfer -> cs_change_delay

> Alignment of single line arrow  -> .

Alignment is now consistent with "=>", could you please confirm if it 
also needs to be aligned with "->"?

>> + *
>> + * So the corresponding relationship:
>> + * A <===> cs_setup_ns (after CS asserted)
>> + * B <===> word_delay_ns (no matter with CS)
> what does it mean when you say "no matter with CS" ? Any other 
> simplified statement ?
> Delay from sending actual data /Clock generation ?

"no matter with CS" means this delay is not related to CS actually.
B is the timing interval between 2 words, during which the CS remains 
asserted.

I agree this description is unclear, just update as:

B <===> word_delay_ns (delay between adjacent words within a transfer)

>> + * C+D <===> cs_delay_hold_ns (before CS deasserted)
>> + * E+F <===> cs_change_delay_inactive_ns (after CS deasserted, these two
>> + * values are also recommended in the Linux driver to be added up)
> Alignment of all double dashed line arrows <===> to left side symbols .

OK, will update to be aligned with "<===>".

>> +    err = virtio_spi_find_vqs(priv);
>> +    if (err) {
>> +        dev_err(&vdev->dev, "Cannot setup virtqueues\n");
>> +        return err;
> Use dev_err_probe()
>> +    }

Yes, will update as:

dev_err_probe(&vdev->dev, err, "Cannot setup virtqueues\n");

Thanks & BR
Haixu Cui