On Fri, Feb 07, 2025 at 03:04:34PM +0200, Heikki Krogerus wrote:
> On Thu, Feb 06, 2025 at 03:51:48PM +0100, Greg Kroah-Hartman wrote:
> > 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?
>
> Although, there is actually only a limited number of things that you
> can do to the connection using UCSI, that is definitely a concern.
>
> The PPM (which is the EC firmware in most cases) is expected to prevent
> any harmful or "unauthorized" UCSI commands from being executed, but
> I'm not sure there is any guarantees for that at the moment.
>
> > > +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?
>
> The size of the attribute depends on the UCSI version.
>
> > But again, why not debugfs? I'd feel a lot more comfortable with that
> > instead of sysfs.
>
> I would actually prefer debugfs for this, but this is in any case
> not primarily for debugging and validation.
>
> The initial goal was to supply the user space some way to control the
> EC's power related policies using UCSI commands such as
> SET_POWER_LEVEL and GET_POWER_LEVEL (guys, please correct me if I got
> that wrong).
It generally feels that exporting the whole unmoderated channel to the
firmware just to set power level is wrong. It should be interfaced
through the PSY driver.
>
> But I'm now again wondering could those power policy tasks be handled
> using the UCSI power supplies after all? Venkat, did you look into
> that?
>
> thanks,
>
> --
> heikki
--
With best wishes
Dmitry
