lkml.org 
[lkml]   [2021]   [Sep]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3 3/5] drivers/acpi: Introduce Platform Firmware Runtime Update device driver
    On Wed, Sep 22, 2021 at 05:04:42PM +0800, Chen Yu wrote:
    > Hi Greg,
    > On Tue, Sep 21, 2021 at 05:59:05PM +0200, Greg Kroah-Hartman wrote:
    > > On Fri, Sep 17, 2021 at 12:02:18AM +0800, Chen Yu wrote:
    > > > Introduce the pfru_update driver which can be used for Platform Firmware
    > > > Runtime code injection and driver update. The user is expected to provide
    > > > the update firmware in the form of capsule file, and pass it to the driver
    > > > via ioctl. Then the driver would hand this capsule file to the Platform
    > > > Firmware Runtime Update via the ACPI device _DSM method. At last the low
    > > > level Management Mode would do the firmware update.
    > > >
    > > > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
    > >
    > > Where is the userspace code that uses this ioctl and has tested it out
    > > to verify it works properly? A link to that in the changelog would be
    > > great to have.
    > >
    > The patch [5/5] is a self testing tool to test the whole feature. I'll send a
    > new version and Cc you too.

    That tests it, but does not answer the question of who will actually use
    this. What userspace tool needs this new api?

    > > > +static void dump_update_result(struct pfru_updated_result *result)
    > > > +{
    > > > + pr_debug("Update result:\n");
    > > > + pr_debug("Status:%d\n", result->status);
    > > > + pr_debug("Extended Status:%d\n", result->ext_status);
    > > > + pr_debug("Authentication Time Low:%lld\n", result->low_auth_time);
    > > > + pr_debug("Authentication Time High:%lld\n", result->high_auth_time);
    > > > + pr_debug("Execution Time Low:%lld\n", result->low_exec_time);
    > > > + pr_debug("Execution Time High:%lld\n", result->high_exec_time);
    > >
    > > Why not dev_dbg()? Same for all pr_* calls in this "driver".
    > >
    > >
    > Ok, I'll switch to dev_dbg() in next version.
    > > > +static long pfru_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
    > > > +{
    > > > + void __user *p;
    > > > + int ret = 0, rev;
    > > > +
    > > > + p = (void __user *)arg;
    > > > +
    > > > + switch (cmd) {
    > > > + case PFRU_IOC_SET_REV:
    > > > + if (copy_from_user(&rev, p, sizeof(unsigned int)))
    > > > + return -EFAULT;
    > > > + if (!pfru_valid_revid(rev))
    > > > + return -EFAULT;
    > > > + pfru_dev->rev_id = rev;
    > > > + break;
    > > > + case PFRU_IOC_STAGE:
    > > > + ret = start_acpi_update(START_STAGE);
    > > > + break;
    > > > + case PFRU_IOC_ACTIVATE:
    > > > + ret = start_acpi_update(START_ACTIVATE);
    > > > + break;
    > > > + case PFRU_IOC_STAGE_ACTIVATE:
    > > > + ret = start_acpi_update(START_STAGE_ACTIVATE);
    > > > + break;
    > > > + default:
    > > > + ret = -ENOIOCTLCMD;
    > >
    > > Wrong value :(
    > Previously I thought that ENOIOCTLCMD stands for 'invalid ioctl command'.
    > After checking the lkml discussion, it seems that ENOIOCTLCMD should not
    > be returned to user space. ENOTTY might be more suitible if I understand
    > correctly.
    > http://lkml.iu.edu/hypermail/linux/kernel/0105.1/0734.html

    Yes, ENOTTY is correct.


    > > > + break;
    > > > + }
    > > > +
    > > > + return ret;
    > > > +}
    > > > +
    > > > +#ifdef CONFIG_COMPAT
    > > > +static long compat_pfru_ioctl(struct file *filep, unsigned int cmd,
    > > > + unsigned long arg)
    > > > +{
    > > > + return pfru_ioctl(filep, cmd, arg);
    > > > +}
    > > > +#endif
    > >
    > > Why is this compat ioctl needed at all?
    > >
    > We can not control if the user space tool would be compiled as 32bit.

    Then create your ioctl so that a compat ioctl is not needed at all.
    There is no need to ever use this for new ioctl commands these days.

    > But I realize that a compat_ptr() was missing. Will fix it in next version.
    > > > +static struct miscdevice pfru_misc_dev = {
    > > > + .minor = MISC_DYNAMIC_MINOR,
    > > > + .name = "pfru_update",
    > > > + .nodename = "pfru/update",
    > >
    > > Why is this in a subdirectory? What requires this? Why not just
    > > "pfru"?
    > >
    > The pfru directory might be reused for pfru_telemetry device, whose driver
    > is in 4/5 patch, I'll Cc you with the whole patch set in next version.

    "might be" is not a valid reason. Why does this simple driver deserve a
    whole /dev/ subdirectory?

    thanks,

    greg k-h

    \
     
     \ /
      Last update: 2021-09-22 11:11    [W:5.973 / U:0.092 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site