linux-kernel - Re: [RESEND PATCH] rtc: ds1343: Force SPI chip select to be active high

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] [day] [month] [year] [list]

Message-ID: <128fc360-39c6-49d7-ba48-66914b1bfe06@mev.co.uk>
Date: Wed, 24 Jul 2024 17:46:39 +0100
From: Ian Abbott <abbotti@....co.uk>
To: Alexandre Belloni <alexandre.belloni@...tlin.com>
Cc: linux-rtc@...r.kernel.org, linux-kernel@...r.kernel.org,
 stable@...r.kernel.org, Linus Walleij <linus.walleij@...aro.org>,
 Mark Brown <broonie@...nel.org>
Subject: Re: [RESEND PATCH] rtc: ds1343: Force SPI chip select to be active
 high

Greetings,

On 10/07/2024 19:40, Alexandre Belloni wrote:
> Hello,
> 
> On 10/07/2024 18:52:07+0100, Ian Abbott wrote:
>> Commit 3b52093dc917 ("rtc: ds1343: Do not hardcode SPI mode flags")
>> bit-flips (^=) the existing SPI_CS_HIGH setting in the SPI mode during
>> device probe.  This will set it to the wrong value if the spi-cs-high
>> property has been set in the devicetree node.  Just force it to be set
>> active high and get rid of some commentary that attempted to explain why
>> flipping the bit was the correct choice.
>>
>> Fixes: 3b52093dc917 ("rtc: ds1343: Do not hardcode SPI mode flags")
>> Cc: <stable@...r.kernel.org> # 5.6+
>> Cc: Linus Walleij <linus.walleij@...aro.org>
>> Cc: Mark Brown <broonie@...nel.org>
>> Signed-off-by: Ian Abbott <abbotti@....co.uk>
>> ---
>>   drivers/rtc/rtc-ds1343.c | 9 +++------
>>   1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-ds1343.c b/drivers/rtc/rtc-ds1343.c
>> index ed5a6ba89a3e..484b5756b55c 100644
>> --- a/drivers/rtc/rtc-ds1343.c
>> +++ b/drivers/rtc/rtc-ds1343.c
>> @@ -361,13 +361,10 @@ static int ds1343_probe(struct spi_device *spi)
>>   	if (!priv)
>>   		return -ENOMEM;
>>   
>> -	/* RTC DS1347 works in spi mode 3 and
>> -	 * its chip select is active high. Active high should be defined as
>> -	 * "inverse polarity" as GPIO-based chip selects can be logically
>> -	 * active high but inverted by the GPIO library.
>> +	/*
>> +	 * RTC DS1347 works in spi mode 3 and its chip select is active high.
>>   	 */
>> -	spi->mode |= SPI_MODE_3;
>> -	spi->mode ^= SPI_CS_HIGH;
>> +	spi->mode |= SPI_MODE_3 | SPI_CS_HIGH;
> 
> Linus being the gpio maintainer and Mark being the SPI maintainer, I'm
> pretty sure this was correct at the time.
> 
> Are you sure you are not missing an active high/low flag on a gpio
> definition?
> 
>>   	spi->bits_per_word = 8;
>>   	res = spi_setup(spi);
>>   	if (res)
>> -- 
>> 2.43.0
>>
> 

I now have an actual SPI controller using cs-gpios with a DS1343 
connected.  I have tested all 8 combinations of: 
GPIO_ACTIVE_LOW/GPIO_ACTIVE_HIGH, with/without spi-cs-high, without/with 
my patch.  Here are my results and observations:

                                  Final        Final
GPIO_ACTIVE  spi-cs-high  Patcb  GPIO active  CS high  Works?
===========  ===========  =====  ===========  =======  =======
    LOW           No        No      LOW         Yes      No
    LOW           No        Yes     LOW         Yes      No
    LOW           Yes       No      HIGH(1)     No       Yes(3)
    LOW           Yes       Yes     HIGH(1)     Yes      Yes
    HIGH          No        No      LOW(2)      Yes      No
    HIGH          No        Yes     LOW(2)      Yes      No
    HIGH          Yes       No      HIGH        No       Yes(3)
    HIGH          Yes       Yes     HIGH        Yes      Yes

The "Final GPIO active" column refers to the GPIO active state after any 
quirks have been applied.  The "Final CS High" column refers to whether 
SPI_CS_HIGH is set in spi->mode when spi_setup() is called.

Notes:

(1) GPIO was forced active high by of_gpio_flags_quirks() before RTC 
device probed.

(2) GPIO was forced active low by of_gpio_flags_quirks() before RTC 
device probed.

(3) Works if cs-gpios being used, but probably will not work for SPI 
controllers that do not use cs-gpios because SPI_CS_HIGH is not set.

In summary:

1. Without the patch, the RTC device node requires the spi-cs-high 
property to be present if the SPI controller uses cs-gpios, and requires 
the spi-cs-high property to be omitted if the SPI controller does not 
use cs-gpios.  (I think that is a confusing situation.)

2. With the patch, the RTC device node requires the spi-cs-high property 
to be present if the SPI controller uses cs-gpios, and it does not care 
about the spi-cs-high property if the SPI controller does not use 
cs-gpios.  (I therefore think that the patch should be applied so that 
the device node can just set spi-cs-high without caring whether ht SPI 
controller uses cs-gpios or not.)

-- 
-=( Ian Abbott <abbotti@....co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-