| 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: <b0844e5a-ee4b-4608-99a1-877660e01d57@linaro.org>
Date: Tue, 13 Feb 2024 12:39:02 +0000
From: Tudor Ambarus <tudor.ambarus@...aro.org>
To: Théo Lebrun <theo.lebrun@...tlin.com>,
Mark Brown <broonie@...nel.org>
Cc: linux-spi@...r.kernel.org, linux-kernel@...r.kernel.org,
Dhruva Gole <d-gole@...com>, Gregory CLEMENT <gregory.clement@...tlin.com>,
Vladimir Kondratiev <vladimir.kondratiev@...ileye.com>,
Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
Tawfik Bayouk <tawfik.bayouk@...ileye.com>
Subject: Re: [PATCH] spi: spi-mem: add statistics support to ->exec_op() calls
On 2/9/24 13:51, Théo Lebrun wrote:
> Current behavior is that spi-mem operations do not increment statistics,
> neither per-controller nor per-device, if ->exec_op() is used. For
> operations that do NOT use ->exec_op(), stats are increased as the
> usual spi_sync() is called.
>
> The newly implemented spi_mem_add_op_stats() function is strongly
> inspired by spi_statistics_add_transfer_stats(); locking logic and
> l2len computation comes from there.
>
> Statistics that are being filled: bytes{,_rx,_tx}, messages, transfers,
> errors, timedout, transfer_bytes_histo_*.
>
> Note about messages & transfers counters: in the fallback to spi_sync()
> case, there are from 1 to 4 transfers per message. We only register one
> big transfer in the ->exec_op() case as that is closer to reality.
>
> This patch is NOT touching:
> - spi_async, spi_sync, spi_sync_immediate: those counters describe
> precise function calls, incrementing them would be lying. I believe
> comparing the messages counter to spi_async+spi_sync is a good way
> to detect ->exec_op() calls, but I might be missing edge cases
> knowledge.
> - transfers_split_maxsize: splitting cannot happen if ->exec_op() is
> provided.
>
> Testing this patch:
>
> $ cd /sys/devices/platform/soc
> $ find . -type d -path "*spi*" -name statistics
> ./2100000.spi/spi_master/spi0/statistics
> ./2100000.spi/spi_master/spi0/spi0.0/statistics
> $ cd ./2100000.spi/spi_master/spi0/statistics
>
> $ for f in *; do printf "%s\t" $f; cat $f; done | \
> grep -v transfer_bytes_histo | column -t
> bytes 240745444
> bytes_rx 240170907
> bytes_tx 126320
> errors 0
> messages 97354
> spi_async 0
> spi_sync 0
> spi_sync_immediate 0
> timedout 0
> transfers 97354
> transfers_split_maxsize 0
>
> Signed-off-by: Théo Lebrun <theo.lebrun@...tlin.com>
> ---
> drivers/spi/spi-mem.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index 2dc8ceb85374..171fe6b1c247 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -297,6 +297,50 @@ static void spi_mem_access_end(struct spi_mem *mem)
> pm_runtime_put(ctlr->dev.parent);
> }
>
> +static void spi_mem_add_op_stats(struct spi_statistics __percpu *pcpu_stats,
> + const struct spi_mem_op *op, int exec_op_ret)
> +{
> + struct spi_statistics *stats;
> + int len, l2len;
> +
> + get_cpu();
> + stats = this_cpu_ptr(pcpu_stats);
> + u64_stats_update_begin(&stats->syncp);
> +
> + /*
> + * We do not have the concept of messages or transfers. Let's consider
> + * that one operation is equivalent to one message and one transfer.
> + */
> + u64_stats_inc(&stats->messages);
> + u64_stats_inc(&stats->transfers);
> +
> + /* Use the sum of all lengths as bytes count and histogram value. */
> + len = (int)op->cmd.nbytes + (int)op->addr.nbytes;
> + len += (int)op->dummy.nbytes + (int)op->data.nbytes;
spi_mem_check_op() makes sure that op->cmd.nbytes != 0, otherwise it
returns -EINVAL ...
> + u64_stats_add(&stats->bytes, len);
> + l2len = min(fls(len), SPI_STATISTICS_HISTO_SIZE) - 1;
.. thus l2len can never be negative. You can declare len and l2len as
u64. The casts from above shall disappear.
> + l2len = max(l2len, 0);
> + u64_stats_inc(&stats->transfer_bytes_histo[l2len]);
> +
> + /* Only account for data bytes as xferred bytes. */
s/xferred/transferred?
> + if (op->data.nbytes && op->data.dir == SPI_MEM_DATA_OUT)
> + u64_stats_add(&stats->bytes_tx, op->data.nbytes);
> + if (op->data.nbytes && op->data.dir == SPI_MEM_DATA_IN)
> + u64_stats_add(&stats->bytes_rx, op->data.nbytes);
> +
> + /*
> + * A timeout is not an error, following the same behavior as
> + * spi_transfer_one_message().
> + */
> + if (exec_op_ret == -ETIMEDOUT)
> + u64_stats_inc(&stats->timedout);
> + else if (exec_op_ret)
> + u64_stats_inc(&stats->errors);
> +
> + u64_stats_update_end(&stats->syncp);
> + put_cpu();
> +}
> +
> /**
> * spi_mem_exec_op() - Execute a memory operation
> * @mem: the SPI memory
> @@ -339,8 +383,12 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
> * read path) and expect the core to use the regular SPI
> * interface in other cases.
> */
> - if (!ret || ret != -ENOTSUPP || ret != -EOPNOTSUPP)
> + if (!ret || ret != -ENOTSUPP || ret != -EOPNOTSUPP) {
> + spi_mem_add_op_stats(ctlr->pcpu_statistics, op, ret);
> + spi_mem_add_op_stats(mem->spi->pcpu_statistics, op, ret);
> +
Would be good to be able to opt out the statistics if one wants it.
SPI NORs can write with a single write op maximum page_size bytes, which
is typically 256 bytes. And since there are SPI NORs that can run at 400
MHz, I guess some performance penalty shouldn't be excluded.
> return ret;
> + }
> }
>
> tmpbufsize = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
>
> ---
> base-commit: 19b50f80b3a4865bd477aa5c026dd234d39a50d2
> change-id: 20240209-spi-mem-stats-ff9bf91c0f7e
>
> Best regards,
Powered by blists - more mailing lists