Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1650504 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 22/24] thunderbolt: Add support for host and device NVM  firmware upgrade Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-05-25 15:30 +0200
    Re: [PATCH 22/24] thunderbolt: Add support for host and device NVM  firmware upgrade Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-05-25 16:50 +0200
      Re: [PATCH 22/24] thunderbolt: Add support for host and device NVM  firmware upgrade Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-05-25 17:00 +0200

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

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(&nvm_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, &val,
> +					  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, &val,
> +					  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(&sw->dev.kobj, &nvm_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

[toc] | [next] | [standalone]

#1650568

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(&nvm_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, &val,
> > +					  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, &val,
> > +					  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(&sw->dev.kobj, &nvm_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.

[toc] | [prev] | [next] | [standalone]

#1650570

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(&nvm_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, &val,
> > > +					  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, &val,
> > > +					  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(&sw->dev.kobj, &nvm_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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web