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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1595 bytes --]

On Thu, Feb 10, 2022 at 03:41:41PM +0000, Zhou, Yun wrote:

Please don't top post, reply in line with needed context.  This allows
readers to readily follow the flow of conversation and understand what
you are talking about and also helps ensure that everything in the
discussion is being addressed.

> I am sorry that I can not agree with you.

> If there are multiple messages, and each message only has one xfer,
> and the cs_change of each xfer is 1, during the transmission of the
> messages, the CS will keep active even until at the end. This must be
> unreasonable.

This is not something that most drivers are expected to use, cs_change
should only be being used at all for very unusual hardware and it should
be used even less frequently for the last transfer in a message.  It is
fragile and anyone using it really needs to know what they're doing but
the feature is there.

> I can't understand why it have to keep CS active after the
> transmission is completed. Could you please explain this in detail?

The feature predates me working on the SPI stack, the obvious examples
would be a device that doesn't actually use chip select where you want
to avoid all chip select changes or if you need to do some other actions
in the middle of a SPI transaction for some reason (which would need a
bunch of system level considerations to actually be safe/sensible like
making sure you're not sharing the SPI bus).

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Yun Zhou]

Hi Brown,

>> If there are multiple messages, and each message only has one xfer,
>> and the cs_change of each xfer is 1, during the transmission of the
>> messages, the CS will keep active even until at the end. This must be
>> unreasonable.
> This is not something that most drivers are expected to use, cs_change
> should only be being used at all for very unusual hardware and it should
> be used even less frequently for the last transfer in a message.  It is
> fragile and anyone using it really needs to know what they're doing but
> the feature is there.
Maybe it's not normal to set "cs_change" in the last xfer. However, in
most cases, SPI messages come from user space, and these messages may
come from multiple different applications. We can't make the whole
controller fail to work normally due to an inappropriate message of one
application.
>
>> I can't understand why it have to keep CS active after the
>> transmission is completed. Could you please explain this in detail?
> The feature predates me working on the SPI stack, the obvious examples
> would be a device that doesn't actually use chip select where you want
> to avoid all chip select changes or if you need to do some other actions
> in the middle of a SPI transaction for some reason (which would need a
> bunch of system level considerations to actually be safe/sensible like
> making sure you're not sharing the SPI bus).
At present, if "cs_change" is not set, CS will be changed back to inactive
after the transmission is completed. If "cs_change" is set, CS will not
be changed. This obviously violates the definition of "cs_change".

Therefore, I think my patch does not break the support of "cs_change",
but consolidates the support of "cs_change". It also ensures that the whole
SPI controller will not be abnormal due to a message from userspace.

Regards,

Yun

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2080 bytes --]

On Fri, Feb 11, 2022 at 01:01:20AM +0800, Yun Zhou wrote:

> > > If there are multiple messages, and each message only has one xfer,
> > > and the cs_change of each xfer is 1, during the transmission of the
> > > messages, the CS will keep active even until at the end. This must be
> > > unreasonable.

> > This is not something that most drivers are expected to use, cs_change
> > should only be being used at all for very unusual hardware and it should
> > be used even less frequently for the last transfer in a message.  It is
> > fragile and anyone using it really needs to know what they're doing but
> > the feature is there.

> Maybe it's not normal to set "cs_change" in the last xfer. However, in
> most cases, SPI messages come from user space, and these messages may

I would question your use of "most" here...

> come from multiple different applications. We can't make the whole
> controller fail to work normally due to an inappropriate message of one
> application.

This is one of the many hazards of using spidev, it is not an especially
safe or robust interface.  To the extent that there's an issue here it's
something that should be addressed at the spidev level, though I expect
that there will be some users who want this facility and would want a
way to disable any access controls.  I recommend writing device drivers
in kernel.

> > The feature predates me working on the SPI stack, the obvious examples
> > would be a device that doesn't actually use chip select where you want
> > to avoid all chip select changes or if you need to do some other actions
> > in the middle of a SPI transaction for some reason (which would need a
> > bunch of system level considerations to actually be safe/sensible like
> > making sure you're not sharing the SPI bus).

> At present, if "cs_change" is not set, CS will be changed back to inactive
> after the transmission is completed. If "cs_change" is set, CS will not
> be changed. This obviously violates the definition of "cs_change".

No, it is exactly the specified behaviour of cs_change.  Please see
spi.h.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Yun Zhou]

Hi Brown,

The focus of our differences is what the role of cs_change is.
I think the direct effect of cs_change is to change CS to inactive.
Here are some comments from spi.h:
1. Line 573, comments of 'transfer' operation in struct spi_controller:
  572      * + Chipselect stays active during the entire message
  573      *   (unless modified by spi_transfer.cs_change != 0).
It means that if cs_change != 0, chipselect will be changed to inactive.

2. Line 820, comments for struct spi_transfer:
  820  * @cs_change: affects chipselect after this transfer completes
  821  * @cs_change_delay: delay between cs deassert and assert when
  822  *      @cs_change is set and @spi_transfer is not the last in 
@spi_message
It says that the chipselect will be changed after this transfer completes.

3. Line 885, also the comments for struct spi_transfer:
  883  * All SPI transfers start with the relevant chipselect active.  
Normally
  884  * it stays selected until after the last transfer in a message.  
Drivers
  885  * can affect the chipselect signal using cs_change.
It means that we can use cs_change to make chipselect not active.

In the whole SPI subsystem, cs_change is written in several places, but 
only read
in one place: in function spi_transfer_one_message(), which is the only 
place
where cs_change comes into play.
1426     list_for_each_entry(xfer, &msg->transfers, transfer_list) {
...
1441             ret = ctlr->transfer_one(ctlr, msg->spi, xfer);
...
1484         if (xfer->cs_change) {
1485             if (list_is_last(&xfer->transfer_list,
1486                      &msg->transfers)) {
1487                 keep_cs = true;
1488             } else {
1489                 spi_set_cs(msg->spi, false, false);
1490                 _spi_transfer_cs_change_delay(msg, xfer);
1491                 spi_set_cs(msg->spi, true, false);
1492             }
1493         }
1495         msg->actual_length += xfer->len;
1496     }
spi_transfer_one_message() will process one or more transfers in the message
one by one. During this period, if cs_change is true, chipselect will be 
changed
to inactive and then be changed back to active(for next transfer), 
except the
last transfer as we no need to change chipselect back to active after 
the last
transfer. In other words, if cs_change is true, we will make an independent
transmission for each transfer.

I also investigated the role of cs_change in several drivers, the result is
similar, e.g. spi-mpc52xx-psc.c, spi-fsl-spi.c, spi-bcm63xx.c, 
spi-mpc52xx.c, etc.

To sum up, there is no indication and no requirement that when cs_change is
true, we need to keep chipselect active.

I hope you can seriously consider my analysis. If what I said is wrong, 
please
correct it.

Regards,

Yun

On 2/11/22 1:14 AM, Mark Brown wrote:
> On Fri, Feb 11, 2022 at 01:01:20AM +0800, Yun Zhou wrote:
>
>>>> If there are multiple messages, and each message only has one xfer,
>>>> and the cs_change of each xfer is 1, during the transmission of the
>>>> messages, the CS will keep active even until at the end. This must be
>>>> unreasonable.
>>> This is not something that most drivers are expected to use, cs_change
>>> should only be being used at all for very unusual hardware and it should
>>> be used even less frequently for the last transfer in a message.  It is
>>> fragile and anyone using it really needs to know what they're doing but
>>> the feature is there.
>> Maybe it's not normal to set "cs_change" in the last xfer. However, in
>> most cases, SPI messages come from user space, and these messages may
> I would question your use of "most" here...
>
>> come from multiple different applications. We can't make the whole
>> controller fail to work normally due to an inappropriate message of one
>> application.
> This is one of the many hazards of using spidev, it is not an especially
> safe or robust interface.  To the extent that there's an issue here it's
> something that should be addressed at the spidev level, though I expect
> that there will be some users who want this facility and would want a
> way to disable any access controls.  I recommend writing device drivers
> in kernel.
>
>>> The feature predates me working on the SPI stack, the obvious examples
>>> would be a device that doesn't actually use chip select where you want
>>> to avoid all chip select changes or if you need to do some other actions
>>> in the middle of a SPI transaction for some reason (which would need a
>>> bunch of system level considerations to actually be safe/sensible like
>>> making sure you're not sharing the SPI bus).
>> At present, if "cs_change" is not set, CS will be changed back to inactive
>> after the transmission is completed. If "cs_change" is set, CS will not
>> be changed. This obviously violates the definition of "cs_change".
> No, it is exactly the specified behaviour of cs_change.  Please see
> spi.h.

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2529 bytes --]

On Mon, Feb 14, 2022 at 08:35:33PM +0800, Yun Zhou wrote:

Please don't top post, reply in line with needed context.  This allows
readers to readily follow the flow of conversation and understand what
you are talking about and also helps ensure that everything in the
discussion is being addressed.

> The focus of our differences is what the role of cs_change is.
> I think the direct effect of cs_change is to change CS to inactive.

During a message.  At the end of the message it does the opposite.

> I also investigated the role of cs_change in several drivers, the result is
> similar, e.g. spi-mpc52xx-psc.c, spi-fsl-spi.c, spi-bcm63xx.c,
> spi-mpc52xx.c, etc.

Individual drivers may not respect chip select at all, never mind in
this specific way - in general any driver actively managing chip select
is going to be unable to implement the full semnatics of cs_change,
possibly even most of the semantics of cs_change, due to hardware
limiations.  If a driver has full control over chip select it'll
normally be able to implement a set_cs() operation and just use the core
handling.

> To sum up, there is no indication and no requirement that when cs_change is
> true, we need to keep chipselect active.
> 
> I hope you can seriously consider my analysis. If what I said is wrong,
> please
> correct it.

 * All SPI transfers start with the relevant chipselect active.  Normally
 * it stays selected until after the last transfer in a message.  Drivers
 * can affect the chipselect signal using cs_change.
 *
 * (i) If the transfer isn't the last one in the message, this flag is
 * used to make the chipselect briefly go inactive in the middle of the
 * message.  Toggling chipselect in this way may be needed to terminate
 * a chip command, letting a single spi_message perform all of group of
 * chip transactions together.
 *
 * (ii) When the transfer is the last one in the message, the chip may
 * stay selected until the next transfer.  On multi-device SPI busses
 * with nothing blocking messages going to other devices, this is just
 * a performance hint; starting a message to another device deselects
 * this one.  But in other cases, this can be used to ensure correctness.
 * Some devices need protocol transactions to be built from a series of
 * spi_message submissions, where the content of one message is determined
 * by the results of previous messages and where the whole transaction
 * ends when the chipselect goes intactive.

especially (ii).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 788 bytes --]

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.  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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Yun Zhou]

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.

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1032 bytes --]

On Wed, Feb 16, 2022 at 06:41:21PM +0800, Yun Zhou wrote:
> On 2/14/22 10:36 PM, Mark Brown wrote:

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

Please read and engage with what what I said above about not disrupting
existing users by just randomly changing this, silently changing how the
parameter operates will break any users that rely on the functionality
which is not going to help anyone and to the extent there is an issue
here it is only those users who would be affected in the first place.

This is not a productive discussion, please stop unless you have
concrete proposals that are considerate of existing users.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]