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.
Reviewed-by: Dhruva Gole <[email protected]>
Signed-off-by: Théo Lebrun <[email protected]>
---
Changes in v2:
- Turn len and l2len into u64. Remove casts on all 4 nbytes fields.
Remove clamp of l2len to be positive.
- Replace "xferred" in comment by "transferred".
- Remove sysfs demo from commit message. Moved to below the tear line.
- Take Reviewed-by Dhruva Gole.
- Link to v1: https://lore.kernel.org/r/[email protected]
---
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
---
drivers/spi/spi-mem.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 48 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index 2dc8ceb85374..c9d6d42a88f5 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -297,6 +297,49 @@ 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;
+ u64 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 = op->cmd.nbytes + op->addr.nbytes;
+ len += op->dummy.nbytes + op->data.nbytes;
+ u64_stats_add(&stats->bytes, len);
+ l2len = min(fls(len), SPI_STATISTICS_HISTO_SIZE) - 1;
+ u64_stats_inc(&stats->transfer_bytes_histo[l2len]);
+
+ /* Only account for data bytes as transferred bytes. */
+ 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 +382,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);
+
return ret;
+ }
}
tmpbufsize = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
---
base-commit: 19b50f80b3a4865bd477aa5c026dd234d39a50d2
change-id: 20240209-spi-mem-stats-ff9bf91c0f7e
Best regards,
--
Théo Lebrun <[email protected]>
On 2/16/24 16:42, 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.
>
> Reviewed-by: Dhruva Gole <[email protected]>
> Signed-off-by: Théo Lebrun <[email protected]
> ---
> Changes in v2:
> - Turn len and l2len into u64. Remove casts on all 4 nbytes fields.
> Remove clamp of l2len to be positive.
> - Replace "xferred" in comment by "transferred".
> - Remove sysfs demo from commit message. Moved to below the tear line.
> - Take Reviewed-by Dhruva Gole.
> - Link to v1: https://lore.kernel.org/r/[email protected]
Reviewed-by: Tudor Ambarus <[email protected]>
> ---
>
> 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
> ---
> drivers/spi/spi-mem.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index 2dc8ceb85374..c9d6d42a88f5 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -297,6 +297,49 @@ 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;
> + u64 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 = op->cmd.nbytes + op->addr.nbytes;
> + len += op->dummy.nbytes + op->data.nbytes;
> + u64_stats_add(&stats->bytes, len);
> + l2len = min(fls(len), SPI_STATISTICS_HISTO_SIZE) - 1;
> + u64_stats_inc(&stats->transfer_bytes_histo[l2len]);
> +
> + /* Only account for data bytes as transferred bytes. */
> + 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 +382,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);
> +
> return ret;
> + }
> }
>
> tmpbufsize = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
>
> ---
> base-commit: 19b50f80b3a4865bd477aa5c026dd234d39a50d2
> change-id: 20240209-spi-mem-stats-ff9bf91c0f7e
>
> Best regards,
On Fri, 16 Feb 2024 17:42:19 +0100, 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.
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
Thanks!
[1/1] spi: spi-mem: add statistics support to ->exec_op() calls
commit: e63aef9c9121e5061cbf5112d12cadc9da399692
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark