linux-kernel - Re: [PATCH 2/2] spi: spidev: fix a max speed setting

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: <CAGgjyvGQ3edNcBUUpcBmTaaNcYA3yvx19m+6MVyKMWgWPVabKQ@mail.gmail.com>
Date:   Fri, 21 Feb 2020 12:43:20 +0000
From:   Oleksandr Suvorov <oleksandr.suvorov@...adex.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>,
        Marcel Ziswiler <marcel.ziswiler@...adex.com>,
        Igor Opanyuk <igor.opanyuk@...adex.com>,
        Philippe Schenker <philippe.schenker@...adex.com>
Subject: Re: [PATCH 2/2] spi: spidev: fix a max speed setting

Hi Mark,

On Thu, Feb 20, 2020 at 8:42 PM Mark Brown <broonie@...nel.org> wrote:
>
> On Thu, Feb 20, 2020 at 02:11:51PM +0000, Oleksandr Suvorov wrote:
> > SPI_IOC_WR_MAX_SPEED_HZ command always sets spi->max_speed_hz
> > the initial value come from DT.
>
> This is intentional.  It's doing a call to spi_setup() then restoring
> the original value, the goal being just to run spi_setup() with the new
> value - it's not really a good idea to change the maximum speed in the
> first place.

Now I see it.
>
> > It leads to set a wrong max speed with IOCTL call.
> In what way does it lead to the wrong speed being set?

After all, I reviewed the code and found out that the problem is not
in spidev_ioctl,
the problem is in spidev_message()'s debug message :)
The real code is ok:

drivers/spi/spidev.c: spidev_message():
...
                 k_tmp->speed_hz = u_tmp->speed_hz;
                 if (!k_tmp->speed_hz)
                        k_tmp->speed_hz = spidev->speed_hz;
...

but the debug message takes wrong value:
drivers/spi/spidev.c: spidev_message():
...
              dev_info(&spidev->spi->dev,
                      "  xfer len %u %s%s%s%dbits %u usec %uHz
(speed_hz=%d max_speed_hz=%d)\n",
                        u_tmp->len,
                        u_tmp->rx_buf ? "rx " : "",
                        u_tmp->tx_buf ? "tx " : "",
                        u_tmp->cs_change ? "cs " : "",
                        u_tmp->bits_per_word ? : spidev->spi->bits_per_word,
                        u_tmp->delay_usecs,
>>>                        u_tmp->speed_hz ? : spidev->spi->max_speed_hz);
...
It leads to debug messages like:

[ 1227.512806] spidev spi0.0: setup mode 0, 32 bits/w, 1000000 Hz max --> 0
[ 1227.541749] spidev spi0.0:   xfer len 4096 tx 32bits 0 usec 10000000Hz
..
[ 1227.616165] spidev spi0.0: setup mode 0, 32 bits/w, 2000000 Hz max --> 0
[ 1227.645095] spidev spi0.0:   xfer len 4096 tx 32bits 0 usec 10000000Hz
...
[ 1227.702714] spidev spi0.0: setup mode 0, 32 bits/w, 20000000 Hz max --> 0
[ 1227.731801] spidev spi0.0:   xfer len 4096 tx 32bits 0 usec 10000000Hz
...
So if one passes the message (using ioctl instead of write to fd) with
empty speed_hz,
the debug message tells the wrong real speed. It forced me to think in
the wrong direction.

> > Fix the logic of a max speed assignment.
>
> If the expectation is that the default speed should be changed for the
> device this should be handled at the spidev level rather than in the
> core.

Agree. I fixed the wrong place :)
I'll replace this patch with better one.

Thanx!

-- 
Best regards
Oleksandr Suvorov

Toradex AG
Altsagenstrasse 5 | 6048 Horw/Luzern | Switzerland | T: +41 41 500
4800 (main line)