Messages in this thread | | | From | Andy Shevchenko <> | Date | Thu, 18 May 2017 22:35:19 +0300 | Subject | Re: [PATCH 22/24] thunderbolt: Add support for host and device NVM firmware upgrade |
| |
On Thu, May 18, 2017 at 5:39 PM, Mika Westerberg <mika.westerberg@linux.intel.com> 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
| |