On Thu, May 18, 2017 at 10:35:19PM +0300, Andy Shevchenko wrote: > On Thu, May 18, 2017 at 5:39 PM, Mika Westerberg > <mika.westerb...@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: > ...
OK. > > + > > + 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. It is indeed u8 so I can drop the & 0xff. Thanks.