Re: [PATCH v3] spmi: core: Add generic DebugFS interface for bus inspection

[Abel Vesa]

On 26-10-20 01:51:07, vamshi gajjela wrote:
> Introduce a generic, controller-agnostic DebugFS interface in the SPMI
> core under /sys/kernel/debug/spmi/spmi-<nr>/:
> - name: Displays the controller parent device name and OF node.
> - detect: Probes the bus for active slave IDs (0x0 to 0xF).
> - xfer: Transactional interface supporting register reads, writes,
>   and directed command opcodes with automatic opcode selection.
> 
> Write and command operations restrict access to root (mode 0600) and
> mark the kernel tainted with TAINT_USER. Runtime PM is managed
> automatically across transactions.
> 
> Detailed interface syntax and parameters are documented in
> Documentation/ABI/testing/debugfs-spmi.
> 
> Co-developed-by: Jim Wylder <jwylder@xxxxxxxxxx>
> Signed-off-by: Jim Wylder <jwylder@xxxxxxxxxx>
> Signed-off-by: vamshi gajjela <vamshigajjela@xxxxxxxxxx>
> ---
> Changes in v3:
> - Move internal SPMI core command helpers (spmi_cmd(), spmi_read_cmd(),
>   spmi_write_cmd()) and DebugFS prototypes out of the public
>   include/linux/spmi.h header into a local drivers/spmi/spmi-internal.h
>   header.

This doesn't apply on top of the current spmi tree or -next anymore.
Please rebase.

Also, please split the spmi.c -> spmi-core.c rename into a separate
preparatory patch.

> 
> Changes in v2:
> - Rename spmi.c to spmi-core.c and link spmi-debugfs.o into the composite
>   spmi.o module so CONFIG_SPMI=m builds cleanly with CONFIG_DEBUG_FS=y.
> - Route DebugFS bus operations through spmi_read_cmd(), spmi_write_cmd(),
>   and spmi_cmd() instead of calling controller callbacks directly,
>   preserving SPMI tracepoints and core validation.
> - Allocate a per-open-file struct spmi_xfer_ctx in open() instead of
>   sharing a controller-wide buffer, avoiding races across concurrent file
>   descriptors, and store struct dentry *debugfs directly on
>   struct spmi_controller.
> - Reset *ppos = 0 on register read commands so repeated transactions on
>   the same open file descriptor work without manual seeking.
> - Restrict SPMI_CMD_ZERO_WRITE at address 0x00 to 7-bit payloads
>   (data[0] <= 0x7f) per the MIPI SPMI specification, falling back to
>   SPMI_CMD_WRITE for 8-bit values.
> - Parse register addresses with kstrtou16() so values above 0xffff are
>   rejected instead of silently truncated.
> - Update Documentation/ABI/testing/debugfs-spmi to document per-fd xfer
>   buffers and 7-bit Zero Write opcode selection.
> 
>  Documentation/ABI/testing/debugfs-spmi |  77 +++++
>  MAINTAINERS                            |   1 +
>  drivers/spmi/Makefile                  |   2 +
>  drivers/spmi/{spmi.c => spmi-core.c}   |  20 +-
>  drivers/spmi/spmi-debugfs.c            | 382 +++++++++++++++++++++++++
>  drivers/spmi/spmi-internal.h           |  28 ++
>  include/linux/spmi.h                   |   6 +
>  7 files changed, 510 insertions(+), 6 deletions(-)
>  create mode 100644 Documentation/ABI/testing/debugfs-spmi
>  rename drivers/spmi/{spmi.c => spmi-core.c} (97%)
>  create mode 100644 drivers/spmi/spmi-debugfs.c
>  create mode 100644 drivers/spmi/spmi-internal.h
> 
> diff --git a/Documentation/ABI/testing/debugfs-spmi b/Documentation/ABI/testing/debugfs-spmi
> new file mode 100644
> index 000000000000..ad0246a5306a
> --- /dev/null
> +++ b/Documentation/ABI/testing/debugfs-spmi
> @@ -0,0 +1,77 @@
> +What:		/sys/kernel/debug/spmi/spmi-<nr>/name
> +Date:		September 2026
> +KernelVersion:	7.0

Update this, please.

> +Contact:	vamshi gajjela <vamshigajjela@xxxxxxxxxx>
> +Description:
> +		Displays the SPMI controller parent device name and
> +		associated Device Tree node.
> +Users:		Debugging, hardware diagnostics
> +
> +What:		/sys/kernel/debug/spmi/spmi-<nr>/detect
> +Date:		September 2026
> +KernelVersion:	7.0
> +Contact:	vamshi gajjela <vamshigajjela@xxxxxxxxxx>
> +Description:
> +		Scans the SPMI bus for all 16 possible MIPI SPMI slave IDs
> +		(0x0 to 0xF) using standard register reads at offset 0x0.
> +		Active slaves are printed in two-digit hexadecimal notation
> +		in an i2cdetect-style grid, while unresponsive slaves are
> +		represented with "--".
> +Users:		Debugging, hardware diagnostics
> +
> +What:		/sys/kernel/debug/spmi/spmi-<nr>/xfer
> +Date:		September 2026
> +KernelVersion:	7.0
> +Contact:	vamshi gajjela <vamshigajjela@xxxxxxxxxx>
> +Description:
> +		Transactional interface for raw SPMI bus operations:

[...]

> +static int spmi_debugfs_detect_show(struct seq_file *s, void *data)
> +{
> +	struct spmi_controller *ctrl = s->private;
> +	struct device *parent = ctrl->dev.parent;
> +	int ret, sid;
> +	u8 dummy;
> +	bool rpm;
> +
> +	if (!ctrl->read_cmd)
> +		return -EOPNOTSUPP;
> +
> +	rpm = parent ? pm_runtime_enabled(parent) : false;
> +	if (rpm) {
> +		ret = pm_runtime_resume_and_get(parent);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	/*
> +	 * MIPI SPMI defines a 4-bit Slave Identifier (SID), allowing a maximum
> +	 * of 16 slave devices (0x0 to 0xF) per bus. Format as a single-row grid.
> +	 */
> +	seq_puts(s, "     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f\n00: ");
> +	for (sid = 0; sid < SPMI_MAX_SLAVE_ID; sid++) {
> +		ret = spmi_read_cmd(ctrl, SPMI_CMD_READ, sid, 0x0, &dummy, 1);

On the Qualcomm PMIC arbiters, address 0x0 belongs to the peripheral 0,
which doesn't exists, so AFAICT this returns -ENODEV for every SID and
detect reports an empty bus even with PMICs present.

> +		if (ret == 0)
> +			seq_printf(s, "%02x ", sid);
> +		else
> +			seq_puts(s, "-- ");
> +	}
> +	seq_puts(s, "\n");
> +
> +	if (rpm) {
> +		pm_runtime_mark_last_busy(parent);
> +		pm_runtime_put_autosuspend(parent);
> +	}
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(spmi_debugfs_detect);
> +

[...]

> +static ssize_t spmi_debugfs_xfer_write(struct file *file, const char __user *user_buf,
> +				       size_t count, loff_t *ppos)
> +{
> +	struct spmi_xfer_ctx *ctx = file->private_data;
> +	struct spmi_controller *ctrl = ctx->ctrl;
> +	struct device *parent = ctrl->dev.parent;
> +	unsigned int cmd_opc, val;
> +	unsigned int sid, len;
> +	char *str, *tok;
> +	char kbuf[256];
> +	int ret = 0, i;
> +	u8 data[16];
> +	size_t off;
> +	u8 opcode;
> +	u16 addr;
> +	char type;
> +	bool rpm;
> +
> +	if (count == 0 || count >= sizeof(kbuf))
> +		return -EINVAL;
> +
> +	if (copy_from_user(kbuf, user_buf, count))
> +		return -EFAULT;
> +	kbuf[count] = '\0';
> +
> +	str = strim(kbuf);
> +	type = tolower(*str);
> +	if (type != 'r' && type != 'w' && type != 'c')
> +		return -EINVAL;
> +	str = skip_spaces(str + 1);
> +
> +	tok = spmi_debugfs_next_tok(&str);
> +	if (!tok || kstrtouint(tok, 0, &sid) || sid >= SPMI_MAX_SLAVE_ID)
> +		return -EINVAL;
> +
> +	rpm = parent ? pm_runtime_enabled(parent) : false;
> +	if (rpm) {
> +		ret = pm_runtime_resume_and_get(parent);
> +		if (ret < 0)
> +			return ret;
> +	}

None of the SPMI controllers use runtime PM, and the regular client
path don't do this either. If a controller ever needs it, it should be
handled in the controller (or the core helpers), not only in debugfs.

So please drop it.

Also, pm_runtime_put_autosuspend() already marks last busy.

> +
> +	mutex_lock(&ctx->lock);
> +	ctx->xfer_len = 0;
> +
> +	if (type == 'r') {
> +		tok = spmi_debugfs_next_tok(&str);
> +		if (!tok || kstrtou16(tok, 16, &addr)) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		len = 1;
> +		tok = spmi_debugfs_next_tok(&str);
> +		if (tok) {
> +			if (kstrtouint(tok, 0, &len) || len == 0 || len > sizeof(data)) {
> +				ret = -EINVAL;
> +				goto out;
> +			}
> +			if (spmi_debugfs_next_tok(&str)) {
> +				ret = -EINVAL;
> +				goto out;
> +			}
> +		}
> +
> +		if (!ctrl->read_cmd) {
> +			ret = -EOPNOTSUPP;
> +			goto out;
> +		}
> +
> +		ret = spmi_calc_read_opcode(addr, len, &opcode);
> +		if (ret)
> +			goto out;
> +
> +		ret = spmi_read_cmd(ctrl, opcode, sid, addr, data, len);
> +		if (!ret) {
> +			off = scnprintf(ctx->xfer_buf, sizeof(ctx->xfer_buf),
> +					"0x%04x:", addr);
> +
> +			for (i = 0; i < len; i++)
> +				off += scnprintf(ctx->xfer_buf + off, sizeof(ctx->xfer_buf) - off,
> +						 " %02x", data[i]);
> +			off += scnprintf(ctx->xfer_buf + off, sizeof(ctx->xfer_buf) - off, "\n");
> +			ctx->xfer_len = off;
> +			*ppos = 0;
> +		}
> +	} else if (type == 'w') {
> +		tok = spmi_debugfs_next_tok(&str);
> +		if (!tok || kstrtou16(tok, 16, &addr)) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		len = 0;
> +		while ((tok = spmi_debugfs_next_tok(&str)) != NULL) {
> +			if (len >= sizeof(data) || kstrtouint(tok, 16, &val) || val > 0xff) {
> +				ret = -EINVAL;
> +				goto out;
> +			}
> +			data[len++] = val;
> +		}
> +
> +		if (len == 0) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		if (!ctrl->write_cmd) {
> +			ret = -EOPNOTSUPP;
> +			goto out;
> +		}
> +
> +		ret = spmi_calc_write_opcode(addr, data, len, &opcode);
> +		if (ret)
> +			goto out;
> +
> +		add_taint(TAINT_USER, LOCKDEP_STILL_OK);
> +
> +		ret = spmi_write_cmd(ctrl, opcode, sid, addr, data, len);

Raw writes (and SHUTDOWN/RESET via 'c') to PMICs can easily damage the
hardware, and they bypass the drivers and thier regmap caches. regmap
already exposes these registers read-only via debugfs and keeps writes
behind a compile-time switch.