Re: [PATCH v1 1/2] usb: typec: ucsi: Command mailbox interface for the userspace

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, Feb 06, 2025 at 04:19:31PM +0200, Heikki Krogerus wrote:
> Some of the UCSI commands can be used to configure the
> entire Platform Policy Manager (PPM) instead of just
> individual connectors. To allow the user space communicate
> those commands with the PPM, adding a mailbox interface. The
> interface is a single attribute file that represents the
> main "OPM to PPM" UCSI data structure.
> 
> The mailbox allows any UCSI command to be sent to the PPM so
> it should be also useful for validation, testing and
> debugging purposes.

As it's for this type of thing, why not put it in debugfs instead?

> +static ssize_t ucsi_write(struct file *filp, struct kobject *kobj,
> +			  const struct bin_attribute *attr,
> +			  char *buf, loff_t off, size_t count)
> +{
> +	struct ucsi_sysfs *sysfs = attr->private;
> +	struct ucsi *ucsi = sysfs->ucsi;
> +	int ret;
> +
> +	u64 *control = (u64 *)&sysfs->mailbox[UCSI_CONTROL];
> +	u32 *cci = (u32 *)&sysfs->mailbox[UCSI_CCI];
> +	void *data = &sysfs->mailbox[UCSI_MESSAGE_IN];
> +
> +	/* TODO: MESSAGE_OUT. */
> +	if (off != UCSI_CONTROL || count != sizeof(*control))
> +		return -EFAULT;
> +
> +	mutex_lock(&sysfs->lock);
> +
> +	memset(data, 0, UCSI_MAX_DATA_LENGTH(ucsi));
> +
> +	/* PPM_RESET has to be handled separately. */
> +	*control = get_unaligned_le64(buf);
> +	if (UCSI_COMMAND(*control) == UCSI_PPM_RESET) {
> +		ret = ucsi_reset_ppm(ucsi, cci);
> +		goto out_unlock_sysfs;
> +	}
> +
> +	mutex_lock(&ucsi->ppm_lock);
> +
> +	ret = ucsi->ops->sync_control(ucsi, *control, cci, NULL, 0);
> +	if (ret)
> +		goto out_unlock_ppm;
> +
> +	if (UCSI_CCI_LENGTH(*cci) && ucsi->ops->read_message_in(ucsi, data, UCSI_CCI_LENGTH(*cci)))
> +		dev_err(ucsi->dev, "failed to read MESSAGE_IN\n");
> +
> +	ret = ucsi->ops->sync_control(ucsi, UCSI_ACK_CC_CI | UCSI_ACK_COMMAND_COMPLETE,
> +				      NULL, NULL, 0);
> +out_unlock_ppm:
> +	mutex_unlock(&ucsi->ppm_lock);
> +out_unlock_sysfs:
> +	mutex_unlock(&sysfs->lock);
> +
> +	return ret ?: count;
> +}

This worries me, any userspace tool can now do this?  What other "bad"
things can it to the connection?

> +
> +int ucsi_sysfs_register(struct ucsi *ucsi)
> +{
> +	struct ucsi_sysfs *sysfs;
> +	int ret;
> +
> +	sysfs = kzalloc(struct_size(sysfs, mailbox, UCSI_MAILBOX_SIZE(ucsi)), GFP_KERNEL);
> +	if (!sysfs)
> +		return -ENOMEM;
> +
> +	sysfs->ucsi = ucsi;
> +	mutex_init(&sysfs->lock);
> +	memcpy(sysfs->mailbox, &ucsi->version, sizeof(ucsi->version));
> +
> +	sysfs_bin_attr_init(&sysfs->bin_attr);
> +
> +	sysfs->bin_attr.attr.name = "ucsi";
> +	sysfs->bin_attr.attr.mode = 0644;
> +
> +	sysfs->bin_attr.size = UCSI_MAILBOX_SIZE(ucsi);
> +	sysfs->bin_attr.private = sysfs;
> +	sysfs->bin_attr.read_new = ucsi_read;
> +	sysfs->bin_attr.write_new = ucsi_write;
> +
> +	ret = sysfs_create_bin_file(&ucsi->dev->kobj, &sysfs->bin_attr);

You raced with userspace and lost, right?  Why are you dynamically
creating this attribute, can't you just use a static one?

But again, why not debugfs?  I'd feel a lot more comfortable with that
instead of sysfs.

thanks,

greg k-h