linux-kernel - Re: [PATCH 1/2] spi: axi-spi-engine: remove use of ida for sync id

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: <0158a82f-5445-43b9-9da5-8655c3a97c4d@wanadoo.fr>
Date: Tue, 6 Feb 2024 22:50:34 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: dlechner@...libre.com
Cc: broonie@...nel.org, linux-kernel@...r.kernel.org,
 linux-spi@...r.kernel.org, michael.hennerich@...log.com, nuno.sa@...log.com
Subject: Re: [PATCH 1/2] spi: axi-spi-engine: remove use of ida for sync id

Le 06/02/2024 à 21:31, David Lechner a écrit :
> Profiling has shown that ida_alloc_range() accounts for about 10% of the
> time spent in spi_sync() when using the AXI SPI Engine controller. This
> call is used to create a unique id for each SPI message to match to an
> IRQ when the message is complete.
> 
> Since the core SPI code serializes messages in a message queue, we can
> only have one message in flight at a time, namely host->cur_msg. This
> means that we can use a fixed value instead of a unique id for each
> message since there can never be more than one message pending at a
> time.
> 
> This patch removes the use of ida...

So, maybe #include <linux/idr.h> can be removed as well?
(untested)



Also, even if unrelated to your changes, spi_engine_prepare_message() 
could use struct_size() in:

	size = sizeof(*p->instructions) * (p_dry.length + 1);
	p = kzalloc(sizeof(*p) + size, GFP_KERNEL);

-->
	p = kzalloc(struct_size(p, instructions, p_dry.length + 1, GFP_KERNEL);

which can be a little safer and less verbose.

CJ

> ...for the sync id and replaces it with a
> constant value. This simplifies the driver and improves performance.
> 
> Signed-off-by: David Lechner <dlechner-rdvid1DuHRBWk0Htik3J/w@...lic.gmane.org>
> ---
>   drivers/spi/spi-axi-spi-engine.c | 27 ++++++---------------------
>   1 file changed, 6 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c
> index 6b0c72bf3395..9cc602075c17 100644
> --- a/drivers/spi/spi-axi-spi-engine.c
> +++ b/drivers/spi/spi-axi-spi-engine.c
> @@ -57,6 +57,9 @@
>   #define SPI_ENGINE_TRANSFER_WRITE		0x1
>   #define SPI_ENGINE_TRANSFER_READ		0x2
>   
> +/* Arbitrary sync ID for use by host->cur_msg */
> +#define AXI_SPI_ENGINE_CUR_MSG_SYNC_ID		0x1
> +
>   #define SPI_ENGINE_CMD(inst, arg1, arg2) \
>   	(((inst) << 12) | ((arg1) << 8) | (arg2))
>   
> @@ -98,8 +101,6 @@ struct spi_engine_message_state {
>   	unsigned int rx_length;
>   	/** @rx_buf: Bytes not yet written to the RX FIFO. */
>   	uint8_t *rx_buf;
> -	/** @sync_id: ID to correlate SYNC interrupts with this message. */
> -	u8 sync_id;
>   };
>   
>   struct spi_engine {
> @@ -109,7 +110,6 @@ struct spi_engine {
>   	spinlock_t lock;
>   
>   	void __iomem *base;
> -	struct ida sync_ida;
>   	struct timer_list watchdog_timer;
>   	struct spi_controller *controller;
>   
> @@ -483,9 +483,7 @@ static irqreturn_t spi_engine_irq(int irq, void *devid)
>   	}
>   
>   	if (pending & SPI_ENGINE_INT_SYNC && msg) {
> -		struct spi_engine_message_state *st = msg->state;
> -
> -		if (completed_id == st->sync_id) {
> +		if (completed_id == AXI_SPI_ENGINE_CUR_MSG_SYNC_ID) {
>   			if (timer_delete_sync(&spi_engine->watchdog_timer)) {
>   				msg->status = 0;
>   				msg->actual_length = msg->frame_length;
> @@ -510,10 +508,8 @@ static int spi_engine_prepare_message(struct spi_controller *host,
>   				      struct spi_message *msg)
>   {
>   	struct spi_engine_program p_dry, *p;
> -	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
>   	struct spi_engine_message_state *st;
>   	size_t size;
> -	int ret;
>   
>   	st = kzalloc(sizeof(*st), GFP_KERNEL);
>   	if (!st)
> @@ -531,18 +527,10 @@ static int spi_engine_prepare_message(struct spi_controller *host,
>   		return -ENOMEM;
>   	}
>   
> -	ret = ida_alloc_range(&spi_engine->sync_ida, 0, U8_MAX, GFP_KERNEL);
> -	if (ret < 0) {
> -		kfree(p);
> -		kfree(st);
> -		return ret;
> -	}
> -
> -	st->sync_id = ret;
> -
>   	spi_engine_compile_message(msg, false, p);
>   
> -	spi_engine_program_add_cmd(p, false, SPI_ENGINE_CMD_SYNC(st->sync_id));
> +	spi_engine_program_add_cmd(p, false, SPI_ENGINE_CMD_SYNC(
> +						AXI_SPI_ENGINE_CUR_MSG_SYNC_ID));
>   
>   	st->p = p;
>   	st->cmd_buf = p->instructions;
> @@ -555,10 +543,8 @@ static int spi_engine_prepare_message(struct spi_controller *host,
>   static int spi_engine_unprepare_message(struct spi_controller *host,
>   					struct spi_message *msg)
>   {
> -	struct spi_engine *spi_engine = spi_controller_get_devdata(host);
>   	struct spi_engine_message_state *st = msg->state;
>   
> -	ida_free(&spi_engine->sync_ida, st->sync_id);
>   	kfree(st->p);
>   	kfree(st);
>   
> @@ -640,7 +626,6 @@ static int spi_engine_probe(struct platform_device *pdev)
>   	spi_engine = spi_controller_get_devdata(host);
>   
>   	spin_lock_init(&spi_engine->lock);
> -	ida_init(&spi_engine->sync_ida);
>   	timer_setup(&spi_engine->watchdog_timer, spi_engine_timeout, TIMER_IRQSAFE);
>   	spi_engine->controller = host;
>   
>