Re: [PATCH 22/24] thunderbolt: Add support for host and device NVM firmware upgrade

[Andy Shevchenko]

On Thu, May 18, 2017 at 5:39 PM, Mika Westerberg
<mika.westerberg@xxxxxxxxxxxxxxx> wrote:
> Starting from Intel Falcon Ridge the NVM firmware can be upgraded by
> using DMA configuration based mailbox commands. If we detect that the
> host or device (device support starts from Intel Alpine Ridge) has the
> DMA configuration based mailbox we expose NVM information to the
> userspace as two separate Linux NVMem devices: nvm_active and
> nvm_non_active. The former is read-only portion of the active NVM which
> firmware upgrade tools can be use to find out suitable NVM image if the
> device identification strings are not enough.
>
> The latter is write-only portion where the new NVM image is to be
> written by the userspace. It is up to the userspace to find out right
> NVM image (the kernel does very minimal validation). The ICM firmware
> itself authenticates the new NVM firmware and fails the operation if it
> is not what is expected.
>
> We also expose two new sysfs files per each switch: nvm_version and
> nvm_authenticate which can be used to read the active NVM version and
> start the upgrade process.
>
> We also introduce safe mode which is the mode a switch goes when it does
> not have properly authenticated firmware. In this mode the switch only
> accepts a couple of commands including flashing a new NVM firmware image
> and triggering power cycle.
>
> This code is based on the work done by Amir Levy and Michael Jamet.

Couple of nitpicks below.

> +static ssize_t nvm_authenticate_store(struct device *dev,
> +       struct device_attribute *attr, const char *buf, size_t count)
> +{
> +       struct tb_switch *sw = tb_to_switch(dev);
> +       unsigned int val;
> +       int ret;
> +
> +       if (mutex_lock_interruptible(&switch_lock))
> +               return -ERESTARTSYS;
> +
> +       ret = kstrtouint(buf, 0, &val);
> +       if (ret)
> +               goto unlock;

Looking below it would be
ret = kstrtobool(..., &x);
if (ret)
 goto ...;

if (x) {
} else {
}

exit_unlock:
...

> +
> +       switch (val) {
> +       case 0:
> +               /* Just clear the authentication status */
> +               nvm_clear_auth_status(sw);
> +               break;
> +
> +       case 1:
> +               ret = nvm_validate_and_write(sw);
> +               if (ret)
> +                       goto unlock;
> +
> +               sw->nvm->authenticating = true;
> +
> +               if (!tb_route(sw))
> +                       ret = nvm_authenticate_host(sw);
> +               else
> +                       ret = nvm_authenticate_device(sw);
> +               break;
> +
> +       default:
> +               ret = -EINVAL;
> +               break;
> +       }
> +
> +unlock:
> +       mutex_unlock(&switch_lock);
> +
> +       if (ret)
> +               return ret;
> +       return count;
> +}

> +               nvm->major = val >> 16 & 0xff;
> +               nvm->minor = val >> 8 & 0xff;

If lvalue is u8 or alike the conjunction becomes redundant.

-- 
With Best Regards,
Andy Shevchenko