| 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
| ||
|
Message-ID: <CABXOdTdgxfsAWLzAdcTuwyj4kbcSGi-UKCr5x2GJyiLfoCR=tA@mail.gmail.com>
Date: Fri, 10 May 2019 17:31:33 -0700
From: Guenter Roeck <groeck@...gle.com>
To: Douglas Anderson <dianders@...omium.org>
Cc: Mark Brown <broonie@...nel.org>,
Benson Leung <bleung@...omium.org>,
Enric Balletbo i Serra <enric.balletbo@...labora.com>,
"open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
Nicolas Boichat <drinkcat@...omium.org>,
Guenter Roeck <groeck@...omium.org>,
Brian Norris <briannorris@...omium.org>,
Matthias Kaehlcke <mka@...omium.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
linux-spi@...r.kernel.org
Subject: Re: [PATCH 2/4] spi: Allow SPI devices to specify that they are
timing sensitive
From: Douglas Anderson <dianders@...omium.org>
Date: Fri, May 10, 2019 at 3:35 PM
To: Mark Brown, Benson Leung, Enric Balletbo i Serra
Cc: <linux-rockchip@...ts.infradead.org>, <drinkcat@...omium.org>,
Guenter Roeck, <briannorris@...omium.org>, <mka@...omium.org>, Douglas
Anderson, <linux-kernel@...r.kernel.org>, <linux-spi@...r.kernel.org>
> If a device on the SPI bus is very sensitive to timing then it may be
> necessary (for correctness) not to get interrupted during a transfer.
> One example is the EC (Embedded Controller) on Chromebooks. The
> Chrome OS EC will drop a transfer if more than ~8ms passes between the
> chip select being asserted and the transfer finishing.
>
> The SPI framework already has code to handle the case where transfers
> are timing senstive. It can set its message pumping thread to
> realtime to to minimize interruptions during the transfer. However,
> at the moment, this mode can only be requested by a SPI controller.
> Let's allow the drivers for SPI devices to also request this mode.
>
> NOTE: at the moment if a given device on a bus says that it's timing
> sensitive then we'll pump all messages on that bus at high priority.
> It is possible we might want to relax this in the future but it seems
> like it should be fine for now.
>
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
Nitpick: I would use 'rt' instead of 'timing_sensitive' as name for the
new variable.
Otherwise:
Reviewed-by: Guenter Roeck <groeck@...omium.org>
> ---
>
> drivers/spi/spi.c | 34 ++++++++++++++++++++++++++++------
> include/linux/spi/spi.h | 3 +++
> 2 files changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 0597f7086de3..d117ab3adafa 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1367,10 +1367,30 @@ static void spi_pump_messages(struct kthread_work *work)
> __spi_pump_messages(ctlr, true);
> }
>
> -static int spi_init_queue(struct spi_controller *ctlr)
> +/**
> + * spi_boost_thread_priority - set the controller to pump at realtime priority
> + * @ctlr: controller to boost priority of
> + *
> + * This can be called because the controller requested realtime priority
> + * (by setting the ->rt value before calling spi_register_controller()) or
> + * because a device on the bus said that its transfers were timing senstive.
> + *
> + * NOTE: at the moment if any device on a bus says it is timing sensitive then
> + * all the devices on this bus will do transfers at realtime priority. If
> + * this eventually becomes a problem we may see if we can find a way to boost
> + * the priority only temporarily during relevant transfers.
> + */
> +static void spi_boost_thread_priority(struct spi_controller *ctlr)
> {
> struct sched_param param = { .sched_priority = MAX_RT_PRIO - 1 };
>
> + dev_info(&ctlr->dev,
> + "will run message pump with realtime priority\n");
> + sched_setscheduler(ctlr->kworker_task, SCHED_FIFO, ¶m);
> +}
> +
> +static int spi_init_queue(struct spi_controller *ctlr)
> +{
> ctlr->running = false;
> ctlr->busy = false;
>
> @@ -1390,11 +1410,8 @@ static int spi_init_queue(struct spi_controller *ctlr)
> * request and the scheduling of the message pump thread. Without this
> * setting the message pump thread will remain at default priority.
> */
> - if (ctlr->rt) {
> - dev_info(&ctlr->dev,
> - "will run message pump with realtime priority\n");
> - sched_setscheduler(ctlr->kworker_task, SCHED_FIFO, ¶m);
> - }
> + if (ctlr->rt)
> + spi_boost_thread_priority(ctlr);
>
> return 0;
> }
> @@ -2985,6 +3002,11 @@ int spi_setup(struct spi_device *spi)
>
> spi_set_cs(spi, false);
>
> + if (spi->timing_sensitive && !spi->controller->rt) {
> + spi->controller->rt = true;
> + spi_boost_thread_priority(spi->controller);
> + }
> +
> dev_dbg(&spi->dev, "setup mode %d, %s%s%s%s%u bits/w, %u Hz max --> %d\n",
> (int) (spi->mode & (SPI_CPOL | SPI_CPHA)),
> (spi->mode & SPI_CS_HIGH) ? "cs_high, " : "",
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 053abd22ad31..ef6bdd4d25f2 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -109,6 +109,8 @@ void spi_statistics_add_transfer_stats(struct spi_statistics *stats,
> * This may be changed by the device's driver, or left at the
> * default (0) indicating protocol words are eight bit bytes.
> * The spi_transfer.bits_per_word can override this for each transfer.
> + * @timing_sensitive: Transfers for this device are senstive to timing
> + * so we should do our transfer at high priority.
> * @irq: Negative, or the number passed to request_irq() to receive
> * interrupts from this device.
> * @controller_state: Controller's runtime state
> @@ -143,6 +145,7 @@ struct spi_device {
> u32 max_speed_hz;
> u8 chip_select;
> u8 bits_per_word;
> + bool timing_sensitive;
> u32 mode;
> #define SPI_CPHA 0x01 /* clock phase */
> #define SPI_CPOL 0x02 /* clock polarity */
> --
> 2.21.0.1020.gf2820cf01a-goog
>
Powered by blists - more mailing lists