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

Message-ID: <2b0e8a6c-f89e-4d71-a816-9da46ea695eb@mev.co.uk>
Date: Thu, 11 Jul 2024 15:05:01 +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.

I'm not convinced.  What value of `spi->mode & SPI_CS_HIGH` do you think 
the device should end up using?  (I think it should end up using 
`SPI_CS_HIGH`, which was the case before commit 3b52093dc917, because 
the RTC chip requires active high CS.)  What do you think the value of 
`spi->mode & SPI_CS_HIGH` will be *before* the `^=` operation? (For 
devicetree, that depends on the `spi-cs-high` property.)

I think the devicetree node for the RTC device ought to be setting 
`spi-cs-high` but cannot do so at the moment because the driver clobbers it.

> Are you sure you are not missing an active high/low flag on a gpio
> definition?

The CS might be internal to the SPI controller, not using a GPIO line 
(`cs-gpios` property).  SPI peripheral device drivers shouldn't care if 
CS is using GPIO or not.

> 
>>   	spi->bits_per_word = 8;
>>   	res = spi_setup(spi);
>>   	if (res)
>> -- 
>> 2.43.0
>>
> 

-- 
-=( 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 )=-