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
