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

From: Andy Shevchenko
Date: Thu May 18 2017 - 15:35:29 EST


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