linux-kernel - Re: 回复: [PATCH] spi: disable chipselect after complete transfer

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]

Date:   Wed, 16 Feb 2022 18:41:21 +0800
From:   Yun Zhou <yun.zhou@...driver.com>
To:     Mark Brown <broonie@...nel.org>
Cc:     "linux-spi@...r.kernel.org" <linux-spi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Xue, Ying" <Ying.Xue@...driver.com>
Subject: Re: 回复: [PATCH] spi: disable chipselect after complete transfer



On 2/14/22 10:36 PM, Mark Brown wrote:
> On Mon, Feb 14, 2022 at 10:20:21PM +0800, Yun Zhou wrote:
> 
>> I can't see from anywhere that when cs_change is true, we must keep CS
>> active. If an individual controller needs to keep CS active after the whole
>> message transmission complete, I think we should set cs_change to false
>> rather than true, because cs_change means to change CS, not keep CS,
>> otherwise let us rename cs_change to cs_keep.
> 
> *sigh*  Please also look back at how this has historically been handled,
> this is not new behaviour.
 From the first commit(8ae12a0d85987dc13) for spi subsystem, we can see 
that:
   - An spi_message is a sequence of of protocol operations, executed
     as one atomic sequence.  SPI driver controls include:

       + when bidirectional reads and writes start ... by how its
         sequence of spi_transfer requests is arranged;

       + optionally defining short delays after transfers ... using
         the spi_transfer.delay_usecs setting;

       + whether the chipselect becomes inactive after a transfer and
         any delay ... by using the spi_transfer.cs_change flag; 
 


       + hinting whether the next message is likely to go to this same
         device ... using the spi_transfer.cs_change flag on the last
         transfer in that atomic group, and potentially saving costs
         for chip deselect and select operations
When we want chipselect to become inactive after a transfer completes,
should cs_change be true or false? Although it is not stated here, I 
think it is obviously true, otherwise we should call it cs_keep not 
cs_change.

> I'm not saying that this is the greatest API
> ever or that it'd be done this way if it were new but that doesn't mean
> we can just randomly change the interface and potentially disrupt users.
> Whatever else is going on the current behaviour is intentional.
> 
Although the logic dealing with cs_change in spi_transfer_one_message() 
has existed a long time and nobody reports issue on it, that doesn't 
mean it is correct. I think the main reason is, cs_change is only used 
to change chipselect inactive in the middle of message, and nobody set 
it at the end of message. Even if the cs_change of the end of message is 
set to true, probably there is no impact before 
d347b4aaa1a042ea528e385d9070b74c77a14321, since no matter the chipselect 
is active or inactive, we will set it to active before a new message. 
Even if meet an issue, most of users think it is the incorrect usage 
himself, and then the issue can be solved easily by clearing cs_change 
of the end of message.
So there are several reasons why we must correct it:
1. We cannot accept an API with the name opposite to its actual performance.
2. The wrong cs_change and the lax last_cs_enable have caused serious bug.
3. My patch only affects the last transfer only when cs_change is true. 
I can't think of anyone who will set a flag to complete operations with 
opposite semantics.