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: <20240710184053c34201f0@mail.local>
Date: Wed, 10 Jul 2024 20:40:53 +0200
From: Alexandre Belloni <alexandre.belloni@...tlin.com>
To: Ian Abbott <abbotti@....co.uk>
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

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
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com