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

[Greg Kroah-Hartman]

On Thu, May 25, 2017 at 05:39:40PM +0300, Mika Westerberg wrote:
> On Thu, May 25, 2017 at 03:28:34PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, May 18, 2017 at 05:39:12PM +0300, Mika Westerberg wrote:
> > > +static int tb_switch_nvm_add(struct tb_switch *sw)
> > > +{
> > > + struct nvmem_device *nvm_dev;
> > > + struct tb_switch_nvm *nvm;
> > > + u32 val;
> > > + int ret;
> > > +
> > > + if (!sw->dma_port)
> > > + return 0;
> > > +
> > > + nvm = kzalloc(sizeof(*nvm), GFP_KERNEL);
> > > + if (!nvm)
> > > + return -ENOMEM;
> > > +
> > > + nvm->id = ida_simple_get(_ida, 0, 0, GFP_KERNEL);
> > > +
> > > + /*
> > > +  * If the switch is in safe-mode the only accessible portion of
> > > +  * the NVM is the non-active one where userspace is expected to
> > > +  * write new functional NVM.
> > > +  */
> > > + if (!sw->safe_mode) {
> > > + u32 nvm_size, hdr_size;
> > > +
> > > + ret = dma_port_flash_read(sw->dma_port, NVM_FLASH_SIZE, ,
> > > +   sizeof(val));
> > > + if (ret)
> > > + goto err_ida;
> > > +
> > > + hdr_size = sw->generation < 3 ? SZ_8K : SZ_16K;
> > > + nvm_size = (SZ_1M << (val & 7)) / 8;
> > > + nvm_size = (nvm_size - hdr_size) / 2;
> > > +
> > > + ret = dma_port_flash_read(sw->dma_port, NVM_VERSION, ,
> > > +   sizeof(val));
> > > + if (ret)
> > > + goto err_ida;
> > > +
> > > + nvm->major = val >> 16 & 0xff;
> > > + nvm->minor = val >> 8 & 0xff;
> > > +
> > > + nvm_dev = register_nvmem(sw, nvm->id, nvm_size, true);
> > > + if (IS_ERR(nvm_dev)) {
> > > + ret = PTR_ERR(nvm_dev);
> > > + goto err_ida;
> > > + }
> > > + nvm->active = nvm_dev;
> > > + }
> > > +
> > > + nvm_dev = register_nvmem(sw, nvm->id, NVM_MAX_SIZE, false);
> > > + if (IS_ERR(nvm_dev)) {
> > > + ret = PTR_ERR(nvm_dev);
> > > + goto err_nvm_active;
> > > + }
> > > + nvm->non_active = nvm_dev;
> > > +
> > > + sw->nvm = nvm;
> > > +
> > > + ret = sysfs_create_group(>dev.kobj, _group);
> > 
> > Why are you adding this to the sw device?  And doing so _after_ it was
> > announced to userspace?  Why can't you make it part of the device's
> > default groups so that the driver core can handle it properly?
> 
> I was thinking those attributes should show up only when we have
> successfully created the two NVMem devices. But maybe I can add those
> conditionally to the device default groups and make the attributes
> return error if the NVM device creation fails.

Or have the device files only show up if there is an nvm device (however
this is hooked up, I didn't spend the time to look at it deeply.)  The
is_visible() callback in the attribute is there just for that.

good luck!

greg k-h

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

[Greg Kroah-Hartman]

On Thu, May 25, 2017 at 05:39:40PM +0300, Mika Westerberg wrote:
> On Thu, May 25, 2017 at 03:28:34PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, May 18, 2017 at 05:39:12PM +0300, Mika Westerberg wrote:
> > > +static int tb_switch_nvm_add(struct tb_switch *sw)
> > > +{
> > > + struct nvmem_device *nvm_dev;
> > > + struct tb_switch_nvm *nvm;
> > > + u32 val;
> > > + int ret;
> > > +
> > > + if (!sw->dma_port)
> > > + return 0;
> > > +
> > > + nvm = kzalloc(sizeof(*nvm), GFP_KERNEL);
> > > + if (!nvm)
> > > + return -ENOMEM;
> > > +
> > > + nvm->id = ida_simple_get(_ida, 0, 0, GFP_KERNEL);
> > > +
> > > + /*
> > > +  * If the switch is in safe-mode the only accessible portion of
> > > +  * the NVM is the non-active one where userspace is expected to
> > > +  * write new functional NVM.
> > > +  */
> > > + if (!sw->safe_mode) {
> > > + u32 nvm_size, hdr_size;
> > > +
> > > + ret = dma_port_flash_read(sw->dma_port, NVM_FLASH_SIZE, ,
> > > +   sizeof(val));
> > > + if (ret)
> > > + goto err_ida;
> > > +
> > > + hdr_size = sw->generation < 3 ? SZ_8K : SZ_16K;
> > > + nvm_size = (SZ_1M << (val & 7)) / 8;
> > > + nvm_size = (nvm_size - hdr_size) / 2;
> > > +
> > > + ret = dma_port_flash_read(sw->dma_port, NVM_VERSION, ,
> > > +   sizeof(val));
> > > + if (ret)
> > > + goto err_ida;
> > > +
> > > + nvm->major = val >> 16 & 0xff;
> > > + nvm->minor = val >> 8 & 0xff;
> > > +
> > > + nvm_dev = register_nvmem(sw, nvm->id, nvm_size, true);
> > > + if (IS_ERR(nvm_dev)) {
> > > + ret = PTR_ERR(nvm_dev);
> > > + goto err_ida;
> > > + }
> > > + nvm->active = nvm_dev;
> > > + }
> > > +
> > > + nvm_dev = register_nvmem(sw, nvm->id, NVM_MAX_SIZE, false);
> > > + if (IS_ERR(nvm_dev)) {
> > > + ret = PTR_ERR(nvm_dev);
> > > + goto err_nvm_active;
> > > + }
> > > + nvm->non_active = nvm_dev;
> > > +
> > > + sw->nvm = nvm;
> > > +
> > > + ret = sysfs_create_group(>dev.kobj, _group);
> > 
> > Why are you adding this to the sw device?  And doing so _after_ it was
> > announced to userspace?  Why can't you make it part of the device's
> > default groups so that the driver core can handle it properly?
> 
> I was thinking those attributes should show up only when we have
> successfully created the two NVMem devices. But maybe I can add those
> conditionally to the device default groups and make the attributes
> return error if the NVM device creation fails.

Or have the device files only show up if there is an nvm device (however
this is hooked up, I didn't spend the time to look at it deeply.)  The
is_visible() callback in the attribute is there just for that.

good luck!

greg k-h

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

[Mika Westerberg]

On Thu, May 25, 2017 at 03:28:34PM +0200, Greg Kroah-Hartman wrote:
> On Thu, May 18, 2017 at 05:39:12PM +0300, Mika Westerberg wrote:
> > +static int tb_switch_nvm_add(struct tb_switch *sw)
> > +{
> > +   struct nvmem_device *nvm_dev;
> > +   struct tb_switch_nvm *nvm;
> > +   u32 val;
> > +   int ret;
> > +
> > +   if (!sw->dma_port)
> > +   return 0;
> > +
> > +   nvm = kzalloc(sizeof(*nvm), GFP_KERNEL);
> > +   if (!nvm)
> > +   return -ENOMEM;
> > +
> > +   nvm->id = ida_simple_get(_ida, 0, 0, GFP_KERNEL);
> > +
> > +   /*
> > +* If the switch is in safe-mode the only accessible portion of
> > +* the NVM is the non-active one where userspace is expected to
> > +* write new functional NVM.
> > +*/
> > +   if (!sw->safe_mode) {
> > +   u32 nvm_size, hdr_size;
> > +
> > +   ret = dma_port_flash_read(sw->dma_port, NVM_FLASH_SIZE, ,
> > + sizeof(val));
> > +   if (ret)
> > +   goto err_ida;
> > +
> > +   hdr_size = sw->generation < 3 ? SZ_8K : SZ_16K;
> > +   nvm_size = (SZ_1M << (val & 7)) / 8;
> > +   nvm_size = (nvm_size - hdr_size) / 2;
> > +
> > +   ret = dma_port_flash_read(sw->dma_port, NVM_VERSION, ,
> > + sizeof(val));
> > +   if (ret)
> > +   goto err_ida;
> > +
> > +   nvm->major = val >> 16 & 0xff;
> > +   nvm->minor = val >> 8 & 0xff;
> > +
> > +   nvm_dev = register_nvmem(sw, nvm->id, nvm_size, true);
> > +   if (IS_ERR(nvm_dev)) {
> > +   ret = PTR_ERR(nvm_dev);
> > +   goto err_ida;
> > +   }
> > +   nvm->active = nvm_dev;
> > +   }
> > +
> > +   nvm_dev = register_nvmem(sw, nvm->id, NVM_MAX_SIZE, false);
> > +   if (IS_ERR(nvm_dev)) {
> > +   ret = PTR_ERR(nvm_dev);
> > +   goto err_nvm_active;
> > +   }
> > +   nvm->non_active = nvm_dev;
> > +
> > +   sw->nvm = nvm;
> > +
> > +   ret = sysfs_create_group(>dev.kobj, _group);
> 
> Why are you adding this to the sw device?  And doing so _after_ it was
> announced to userspace?  Why can't you make it part of the device's
> default groups so that the driver core can handle it properly?

I was thinking those attributes should show up only when we have
successfully created the two NVMem devices. But maybe I can add those
conditionally to the device default groups and make the attributes
return error if the NVM device creation fails.

> Hint, if you ever have to call sysfs_* from within a driver, something
> might really be wrong :)

Understood, thanks.

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

[Mika Westerberg]

On Thu, May 25, 2017 at 03:28:34PM +0200, Greg Kroah-Hartman wrote:
> On Thu, May 18, 2017 at 05:39:12PM +0300, Mika Westerberg wrote:
> > +static int tb_switch_nvm_add(struct tb_switch *sw)
> > +{
> > +   struct nvmem_device *nvm_dev;
> > +   struct tb_switch_nvm *nvm;
> > +   u32 val;
> > +   int ret;
> > +
> > +   if (!sw->dma_port)
> > +   return 0;
> > +
> > +   nvm = kzalloc(sizeof(*nvm), GFP_KERNEL);
> > +   if (!nvm)
> > +   return -ENOMEM;
> > +
> > +   nvm->id = ida_simple_get(_ida, 0, 0, GFP_KERNEL);
> > +
> > +   /*
> > +* If the switch is in safe-mode the only accessible portion of
> > +* the NVM is the non-active one where userspace is expected to
> > +* write new functional NVM.
> > +*/
> > +   if (!sw->safe_mode) {
> > +   u32 nvm_size, hdr_size;
> > +
> > +   ret = dma_port_flash_read(sw->dma_port, NVM_FLASH_SIZE, ,
> > + sizeof(val));
> > +   if (ret)
> > +   goto err_ida;
> > +
> > +   hdr_size = sw->generation < 3 ? SZ_8K : SZ_16K;
> > +   nvm_size = (SZ_1M << (val & 7)) / 8;
> > +   nvm_size = (nvm_size - hdr_size) / 2;
> > +
> > +   ret = dma_port_flash_read(sw->dma_port, NVM_VERSION, ,
> > + sizeof(val));
> > +   if (ret)
> > +   goto err_ida;
> > +
> > +   nvm->major = val >> 16 & 0xff;
> > +   nvm->minor = val >> 8 & 0xff;
> > +
> > +   nvm_dev = register_nvmem(sw, nvm->id, nvm_size, true);
> > +   if (IS_ERR(nvm_dev)) {
> > +   ret = PTR_ERR(nvm_dev);
> > +   goto err_ida;
> > +   }
> > +   nvm->active = nvm_dev;
> > +   }
> > +
> > +   nvm_dev = register_nvmem(sw, nvm->id, NVM_MAX_SIZE, false);
> > +   if (IS_ERR(nvm_dev)) {
> > +   ret = PTR_ERR(nvm_dev);
> > +   goto err_nvm_active;
> > +   }
> > +   nvm->non_active = nvm_dev;
> > +
> > +   sw->nvm = nvm;
> > +
> > +   ret = sysfs_create_group(>dev.kobj, _group);
> 
> Why are you adding this to the sw device?  And doing so _after_ it was
> announced to userspace?  Why can't you make it part of the device's
> default groups so that the driver core can handle it properly?

I was thinking those attributes should show up only when we have
successfully created the two NVMem devices. But maybe I can add those
conditionally to the device default groups and make the attributes
return error if the NVM device creation fails.

> Hint, if you ever have to call sysfs_* from within a driver, something
> might really be wrong :)

Understood, thanks.

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

[Greg Kroah-Hartman]

On Thu, May 18, 2017 at 05:39:12PM +0300, Mika Westerberg wrote:
> +static int tb_switch_nvm_add(struct tb_switch *sw)
> +{
> + struct nvmem_device *nvm_dev;
> + struct tb_switch_nvm *nvm;
> + u32 val;
> + int ret;
> +
> + if (!sw->dma_port)
> + return 0;
> +
> + nvm = kzalloc(sizeof(*nvm), GFP_KERNEL);
> + if (!nvm)
> + return -ENOMEM;
> +
> + nvm->id = ida_simple_get(_ida, 0, 0, GFP_KERNEL);
> +
> + /*
> +  * If the switch is in safe-mode the only accessible portion of
> +  * the NVM is the non-active one where userspace is expected to
> +  * write new functional NVM.
> +  */
> + if (!sw->safe_mode) {
> + u32 nvm_size, hdr_size;
> +
> + ret = dma_port_flash_read(sw->dma_port, NVM_FLASH_SIZE, ,
> +   sizeof(val));
> + if (ret)
> + goto err_ida;
> +
> + hdr_size = sw->generation < 3 ? SZ_8K : SZ_16K;
> + nvm_size = (SZ_1M << (val & 7)) / 8;
> + nvm_size = (nvm_size - hdr_size) / 2;
> +
> + ret = dma_port_flash_read(sw->dma_port, NVM_VERSION, ,
> +   sizeof(val));
> + if (ret)
> + goto err_ida;
> +
> + nvm->major = val >> 16 & 0xff;
> + nvm->minor = val >> 8 & 0xff;
> +
> + nvm_dev = register_nvmem(sw, nvm->id, nvm_size, true);
> + if (IS_ERR(nvm_dev)) {
> + ret = PTR_ERR(nvm_dev);
> + goto err_ida;
> + }
> + nvm->active = nvm_dev;
> + }
> +
> + nvm_dev = register_nvmem(sw, nvm->id, NVM_MAX_SIZE, false);
> + if (IS_ERR(nvm_dev)) {
> + ret = PTR_ERR(nvm_dev);
> + goto err_nvm_active;
> + }
> + nvm->non_active = nvm_dev;
> +
> + sw->nvm = nvm;
> +
> + ret = sysfs_create_group(>dev.kobj, _group);

Why are you adding this to the sw device?  And doing so _after_ it was
announced to userspace?  Why can't you make it part of the device's
default groups so that the driver core can handle it properly?

Hint, if you ever have to call sysfs_* from within a driver, something
might really be wrong :)

thanks,

greg k-h

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

[Greg Kroah-Hartman]

On Thu, May 18, 2017 at 05:39:12PM +0300, Mika Westerberg wrote:
> +static int tb_switch_nvm_add(struct tb_switch *sw)
> +{
> + struct nvmem_device *nvm_dev;
> + struct tb_switch_nvm *nvm;
> + u32 val;
> + int ret;
> +
> + if (!sw->dma_port)
> + return 0;
> +
> + nvm = kzalloc(sizeof(*nvm), GFP_KERNEL);
> + if (!nvm)
> + return -ENOMEM;
> +
> + nvm->id = ida_simple_get(_ida, 0, 0, GFP_KERNEL);
> +
> + /*
> +  * If the switch is in safe-mode the only accessible portion of
> +  * the NVM is the non-active one where userspace is expected to
> +  * write new functional NVM.
> +  */
> + if (!sw->safe_mode) {
> + u32 nvm_size, hdr_size;
> +
> + ret = dma_port_flash_read(sw->dma_port, NVM_FLASH_SIZE, ,
> +   sizeof(val));
> + if (ret)
> + goto err_ida;
> +
> + hdr_size = sw->generation < 3 ? SZ_8K : SZ_16K;
> + nvm_size = (SZ_1M << (val & 7)) / 8;
> + nvm_size = (nvm_size - hdr_size) / 2;
> +
> + ret = dma_port_flash_read(sw->dma_port, NVM_VERSION, ,
> +   sizeof(val));
> + if (ret)
> + goto err_ida;
> +
> + nvm->major = val >> 16 & 0xff;
> + nvm->minor = val >> 8 & 0xff;
> +
> + nvm_dev = register_nvmem(sw, nvm->id, nvm_size, true);
> + if (IS_ERR(nvm_dev)) {
> + ret = PTR_ERR(nvm_dev);
> + goto err_ida;
> + }
> + nvm->active = nvm_dev;
> + }
> +
> + nvm_dev = register_nvmem(sw, nvm->id, NVM_MAX_SIZE, false);
> + if (IS_ERR(nvm_dev)) {
> + ret = PTR_ERR(nvm_dev);
> + goto err_nvm_active;
> + }
> + nvm->non_active = nvm_dev;
> +
> + sw->nvm = nvm;
> +
> + ret = sysfs_create_group(>dev.kobj, _group);

Why are you adding this to the sw device?  And doing so _after_ it was
announced to userspace?  Why can't you make it part of the device's
default groups so that the driver core can handle it properly?

Hint, if you ever have to call sysfs_* from within a driver, something
might really be wrong :)

thanks,

greg k-h

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

[Mika Westerberg]

On Thu, May 18, 2017 at 10:35:19PM +0300, Andy Shevchenko wrote:
> On Thu, May 18, 2017 at 5:39 PM, Mika Westerberg
>  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(_lock))
> > +   return -ERESTARTSYS;
> > +
> > +   ret = kstrtouint(buf, 0, );
> > +   if (ret)
> > +   goto unlock;
> 
> Looking below it would be
> ret = kstrtobool(..., );
> 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(_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.

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

[Mika Westerberg]

On Thu, May 18, 2017 at 10:35:19PM +0300, Andy Shevchenko wrote:
> On Thu, May 18, 2017 at 5:39 PM, Mika Westerberg
>  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(_lock))
> > +   return -ERESTARTSYS;
> > +
> > +   ret = kstrtouint(buf, 0, );
> > +   if (ret)
> > +   goto unlock;
> 
> Looking below it would be
> ret = kstrtobool(..., );
> 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(_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.

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
 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(_lock))
> +   return -ERESTARTSYS;
> +
> +   ret = kstrtouint(buf, 0, );
> +   if (ret)
> +   goto unlock;

Looking below it would be
ret = kstrtobool(..., );
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(_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

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
 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(_lock))
> +   return -ERESTARTSYS;
> +
> +   ret = kstrtouint(buf, 0, );
> +   if (ret)
> +   goto unlock;

Looking below it would be
ret = kstrtobool(..., );
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(_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

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

[Mika Westerberg]

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.

Signed-off-by: Michael Jamet 
Signed-off-by: Mika Westerberg 
Reviewed-by: Yehezkel Bernat 
---
 Documentation/ABI/testing/sysfs-bus-thunderbolt |  26 ++
 drivers/thunderbolt/Kconfig |   1 +
 drivers/thunderbolt/domain.c|  17 +
 drivers/thunderbolt/icm.c   |  33 +-
 drivers/thunderbolt/nhi.h   |   1 +
 drivers/thunderbolt/switch.c| 595 +++-
 drivers/thunderbolt/tb.h|  36 ++
 7 files changed, 684 insertions(+), 25 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-thunderbolt 
b/Documentation/ABI/testing/sysfs-bus-thunderbolt
index c100bdeacfaf..ed88f0eb593b 100644
--- a/Documentation/ABI/testing/sysfs-bus-thunderbolt
+++ b/Documentation/ABI/testing/sysfs-bus-thunderbolt
@@ -80,3 +80,29 @@ KernelVersion:   4.13
 Contact:   thunderbolt-softw...@lists.01.org
 Description:   This attribute contains unique id (UUID) of this device
extracted from the device DROM or hardware registers.
+
+What:  /sys/bus/thunderbolt/devices/.../nvm_version
+Date:  Sep 2017
+KernelVersion: 4.13
+Contact:   thunderbolt-softw...@lists.01.org
+Description:   If the device has upgradeable firmware the version
+   number is available here. Format: %x.%x, major.minor.
+   If the device is in safe mode reading the file returns
+   -ENODATA instead as the NVM version is not available.
+
+What:  /sys/bus/thunderbolt/devices/.../nvm_authenticate
+Date:  Sep 2017
+KernelVersion: 4.13
+Contact:   thunderbolt-softw...@lists.01.org
+Description:   When new NVM image is written to the non-active NVM
+   area (through non_activeX NVMem device), the
+   authentication procedure is started by writing 1 to
+   this file. If everything goes well, the device is
+   restarted with the new NVM firmware. If the image
+   verification fails an error code is returned instead.
+
+   When read holds status of the last authentication
+   operation if an error occurred during the process. This
+   is directly the status value from the DMA configuration
+   based mailbox before the device is power cycled. Writing
+   0 here clears the status.
diff --git a/drivers/thunderbolt/Kconfig b/drivers/thunderbolt/Kconfig
index a9cc724985ad..f4869c38c7e4 100644
--- a/drivers/thunderbolt/Kconfig
+++ b/drivers/thunderbolt/Kconfig
@@ -6,6 +6,7 @@ menuconfig THUNDERBOLT
select CRC32
select CRYPTO
select CRYPTO_HASH
+   select NVMEM
help
  Thunderbolt Controller driver. This driver is required if you
  want to hotplug Thunderbolt devices on Apple hardware or on PCs
diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
index b8754c222ab4..d5075266b202 100644
--- a/drivers/thunderbolt/domain.c
+++ b/drivers/thunderbolt/domain.c
@@ -427,6 +427,23 @@ int tb_domain_challenge_switch_key(struct tb *tb, struct 
tb_switch *sw)
return ret;
 }
 
+/**
+ * tb_domain_disconnect_pcie_paths() - Disconnect all PCIe paths
+ * @tb: Domain whose PCIe paths to disconnect
+ *
+ * This needs to be called in preparation for NVM upgrade of the host
+ * controller. Makes sure all PCIe paths are disconnected.
+ *
+ * Return %0 on success and negative errno in case of error.
+ */
+int tb_domain_disconnect_pcie_paths(struct tb *tb)
+{
+   if (!tb->cm_ops->disconnect_pcie_paths)
+

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

[Mika Westerberg]

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.

Signed-off-by: Michael Jamet 
Signed-off-by: Mika Westerberg 
Reviewed-by: Yehezkel Bernat 
---
 Documentation/ABI/testing/sysfs-bus-thunderbolt |  26 ++
 drivers/thunderbolt/Kconfig |   1 +
 drivers/thunderbolt/domain.c|  17 +
 drivers/thunderbolt/icm.c   |  33 +-
 drivers/thunderbolt/nhi.h   |   1 +
 drivers/thunderbolt/switch.c| 595 +++-
 drivers/thunderbolt/tb.h|  36 ++
 7 files changed, 684 insertions(+), 25 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-thunderbolt 
b/Documentation/ABI/testing/sysfs-bus-thunderbolt
index c100bdeacfaf..ed88f0eb593b 100644
--- a/Documentation/ABI/testing/sysfs-bus-thunderbolt
+++ b/Documentation/ABI/testing/sysfs-bus-thunderbolt
@@ -80,3 +80,29 @@ KernelVersion:   4.13
 Contact:   thunderbolt-softw...@lists.01.org
 Description:   This attribute contains unique id (UUID) of this device
extracted from the device DROM or hardware registers.
+
+What:  /sys/bus/thunderbolt/devices/.../nvm_version
+Date:  Sep 2017
+KernelVersion: 4.13
+Contact:   thunderbolt-softw...@lists.01.org
+Description:   If the device has upgradeable firmware the version
+   number is available here. Format: %x.%x, major.minor.
+   If the device is in safe mode reading the file returns
+   -ENODATA instead as the NVM version is not available.
+
+What:  /sys/bus/thunderbolt/devices/.../nvm_authenticate
+Date:  Sep 2017
+KernelVersion: 4.13
+Contact:   thunderbolt-softw...@lists.01.org
+Description:   When new NVM image is written to the non-active NVM
+   area (through non_activeX NVMem device), the
+   authentication procedure is started by writing 1 to
+   this file. If everything goes well, the device is
+   restarted with the new NVM firmware. If the image
+   verification fails an error code is returned instead.
+
+   When read holds status of the last authentication
+   operation if an error occurred during the process. This
+   is directly the status value from the DMA configuration
+   based mailbox before the device is power cycled. Writing
+   0 here clears the status.
diff --git a/drivers/thunderbolt/Kconfig b/drivers/thunderbolt/Kconfig
index a9cc724985ad..f4869c38c7e4 100644
--- a/drivers/thunderbolt/Kconfig
+++ b/drivers/thunderbolt/Kconfig
@@ -6,6 +6,7 @@ menuconfig THUNDERBOLT
select CRC32
select CRYPTO
select CRYPTO_HASH
+   select NVMEM
help
  Thunderbolt Controller driver. This driver is required if you
  want to hotplug Thunderbolt devices on Apple hardware or on PCs
diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
index b8754c222ab4..d5075266b202 100644
--- a/drivers/thunderbolt/domain.c
+++ b/drivers/thunderbolt/domain.c
@@ -427,6 +427,23 @@ int tb_domain_challenge_switch_key(struct tb *tb, struct 
tb_switch *sw)
return ret;
 }
 
+/**
+ * tb_domain_disconnect_pcie_paths() - Disconnect all PCIe paths
+ * @tb: Domain whose PCIe paths to disconnect
+ *
+ * This needs to be called in preparation for NVM upgrade of the host
+ * controller. Makes sure all PCIe paths are disconnected.
+ *
+ * Return %0 on success and negative errno in case of error.
+ */
+int tb_domain_disconnect_pcie_paths(struct tb *tb)
+{
+   if (!tb->cm_ops->disconnect_pcie_paths)
+   return -EPERM;
+
+   return tb->cm_ops->disconnect_pcie_paths(tb);
+}