On Mon, Aug 22, 2022 at 03:31:05PM +0800, Szuying Chen wrote: > From: Szuying Chen <Chloe_Chen@xxxxxxxxxxxxxx> > > The patch add tb_switch_nvm_alloc() contain an array that has functions > pointers to vendor_ops that vendor to define. > And add tb_switch_nvm_upgradable() to enable nvm upgrade for vendors > that in switch_nvm_upgrade_vendors[]. > > Signed-off-by: Szuying Chen <Chloe_Chen@xxxxxxxxxxxxxx> > --- > v4 -> v5 : Take Mika's suggestion and use nvm->vops to point the vendor > specific operations. Moved vendor:intel part of the code to make all > the vendors (includes Intel) support it in nvm.c. And add nvm specific > operations for ASMedia. > > drivers/thunderbolt/nvm.c | 204 +++++++++++++++++++++++++++++++++++ > drivers/thunderbolt/switch.c | 100 ++++------------- > drivers/thunderbolt/tb.h | 19 +++- > 3 files changed, 242 insertions(+), 81 deletions(-) > > diff --git a/drivers/thunderbolt/nvm.c b/drivers/thunderbolt/nvm.c > index b3f310389378..6ee048565616 100644 > --- a/drivers/thunderbolt/nvm.c > +++ b/drivers/thunderbolt/nvm.c > @@ -12,8 +12,212 @@ > > #include "tb.h" > > +/* Switch NVM support */ > +#define NVM_CSS > + > static DEFINE_IDA(nvm_ida); > > +/* Vendor provides NVM firmware upgrade function */ > +static const u32 switch_nvm_upgrade_vendors[] = { > + 0x174c, > +}; > + > +void tb_switch_nvm_upgradable(struct tb_switch *sw) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(switch_nvm_upgrade_vendors); i++) { > + if (switch_nvm_upgrade_vendors[i] == sw->config.vendor_id) > + sw->no_nvm_upgrade = false; > + } > +} > + > +static inline int nvm_read(struct tb_switch *sw, unsigned int address, > + void *buf, size_t size) > +{ > + if (tb_switch_is_usb4(sw)) > + return usb4_switch_nvm_read(sw, address, buf, size); > + return dma_port_flash_read(sw->dma_port, address, buf, size); > +} > + > +static int asmedia_nvm_version(struct tb_switch *sw) Odd extra space :( > +{ > + struct tb_nvm *nvm = sw->nvm; > + u64 val; > + int ret; > + > + ret = nvm_read(sw, 0x1c, &val, sizeof(val)); > + if (ret) > + return ret; > + > + nvm->major = (((u8)val >> 0x18) << 0x10 | ((u8)(val >> 0x20)) << 0x8 | (u8)(val >> 0x28)); > + nvm->minor = (((u8)val) << 0x10 | ((u8)(val >> 0x8)) << 0x8 | (u8)(val >> 0x10)); That's tough to read, want to add a comment explaining what you are doing here? Like "the major number is in bits X-Y..." and such? > + nvm->nvm_size = SZ_512K; > + > + return 0; > +} > + > +static int intel_nvm_version(struct tb_switch *sw) extra space. > +{ > + struct tb_nvm *nvm = sw->nvm; > + u32 val; > + int ret; > + > + /* > + * 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 = nvm_read(sw, NVM_FLASH_SIZE, &val, sizeof(val)); > + if (ret) > + return ret; > + > + hdr_size = sw->generation < 3 ? SZ_8K : SZ_16K; > + nvm_size = (SZ_1M << (val & 7)) / 8; > + nvm_size = (nvm_size - hdr_size) / 2; > + > + ret = nvm_read(sw, NVM_VERSION, &val, sizeof(val)); > + if (ret) > + return ret; > + > + nvm->major = val >> 16; > + nvm->minor = val >> 8; > + nvm->nvm_size = nvm_size; > + extra blank line. Did you run this patch through checkpatch.pl first? > + } > + > + return 0; > +} > + > +static int intel_nvm_validate(struct tb_switch *sw) > +{ > + unsigned int image_size, hdr_size; > + const u8 *buf = sw->nvm->buf; > + u16 ds_size; > + int ret; > + > + image_size = sw->nvm->buf_data_size; > + if (image_size < NVM_MIN_SIZE || image_size > NVM_MAX_SIZE) > + return -EINVAL; > + > + /* > + * FARB pointer must point inside the image and must at least > + * contain parts of the digital section we will be reading here. > + */ > + hdr_size = (*(u32 *)buf) & 0xffffff; > + if (hdr_size + NVM_DEVID + 2 >= image_size) > + return -EINVAL; > + > + /* Digital section start should be aligned to 4k page */ > + if (!IS_ALIGNED(hdr_size, SZ_4K)) > + return -EINVAL; > + > + /* > + * Read digital section size and check that it also fits inside > + * the image. > + */ > + ds_size = *(u16 *)(buf + hdr_size); > + if (ds_size >= image_size) > + return -EINVAL; > + > + if (!sw->safe_mode) { > + u16 device_id; > + > + /* > + * Make sure the device ID in the image matches the one > + * we read from the switch config space. > + */ > + device_id = *(u16 *)(buf + hdr_size + NVM_DEVID); > + if (device_id != sw->config.device_id) > + return -EINVAL; > + > + if (sw->generation < 3) { > + /* Write CSS headers first */ > + ret = dma_port_flash_write(sw->dma_port, > + DMA_PORT_CSS_ADDRESS, buf + NVM_CSS, > + DMA_PORT_CSS_MAX_SIZE); > + if (ret) > + return ret; > + } > + > + /* Skip headers in the image */ > + buf += hdr_size; > + image_size -= hdr_size; > + } > + > + return image_size; > +} > + > +struct tb_nvm_vendor_ops asmedia_switch_nvm_ops = { > + .version = asmedia_nvm_version, > +}; > + > +struct tb_nvm_vendor_ops intel_switch_nvm_ops = { > + .version = intel_nvm_version, > + .validate = intel_nvm_validate, > +}; > + > +struct switch_nvm_vendor { > + u16 vendor; > + const struct tb_nvm_vendor_ops *vops; > +}; > + > +static const struct switch_nvm_vendor switch_nvm_vendors[] = { > + { 0x8086, &intel_switch_nvm_ops }, > + { 0x8087, &intel_switch_nvm_ops }, > + { 0x174c, &asmedia_switch_nvm_ops }, > + > +}; > + > +/** > + * tb_switch_nvm_alloc() - alloc nvm and set nvm->vops to point > + * the vendor specific operations. > + * @sw: thunderbolt switch > + * > + */ > +struct tb_nvm *tb_switch_nvm_alloc(struct tb_switch *sw) > +{ > + const struct tb_nvm_vendor_ops *vops = NULL; > + struct tb_nvm *nvm; > + int i; > + int ret; > + > + /** No need for kernel-doc format here. > + * If the vendor matches on the array then set nvm->vops to > + * point the vendor specific operations. > + */ > + for (i = 0; i < ARRAY_SIZE(switch_nvm_vendors); i++) { > + const struct switch_nvm_vendor *v = &switch_nvm_vendors[i]; > + > + if (v->vendor == sw->config.vendor_id) { > + vops = v->vops; > + break; > + } > + } > + > + if (!vops) > + return ERR_PTR(-EOPNOTSUPP); > + > + nvm = tb_nvm_alloc(&sw->dev); > + if (IS_ERR(nvm)) > + return nvm; > + > + nvm->vops = vops; > + sw->nvm = nvm; > + ret = vops->version(sw); > + if (ret) > + goto err_nvm; > + > + return nvm; > + > +err_nvm: > + tb_nvm_free(nvm); > + return ERR_PTR(ret); > +} > + > /** > * tb_nvm_alloc() - Allocate new NVM structure > * @dev: Device owning the NVM > diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c > index 244f8cd38b25..392586641828 100644 > --- a/drivers/thunderbolt/switch.c > +++ b/drivers/thunderbolt/switch.c > @@ -102,10 +102,10 @@ static void nvm_clear_auth_status(const struct tb_switch *sw) > > static int nvm_validate_and_write(struct tb_switch *sw) > { > - unsigned int image_size, hdr_size; > + unsigned int image_size; > const u8 *buf = sw->nvm->buf; > - u16 ds_size; > int ret; > + const struct tb_nvm_vendor_ops *vops = sw->nvm->vops; > > if (!buf) > return -EINVAL; > @@ -114,49 +114,13 @@ static int nvm_validate_and_write(struct tb_switch *sw) > if (image_size < NVM_MIN_SIZE || image_size > NVM_MAX_SIZE) > return -EINVAL; > > - /* > - * FARB pointer must point inside the image and must at least > - * contain parts of the digital section we will be reading here. > - */ > - hdr_size = (*(u32 *)buf) & 0xffffff; > - if (hdr_size + NVM_DEVID + 2 >= image_size) > - return -EINVAL; > - > - /* Digital section start should be aligned to 4k page */ > - if (!IS_ALIGNED(hdr_size, SZ_4K)) > - return -EINVAL; > - > - /* > - * Read digital section size and check that it also fits inside > - * the image. > - */ > - ds_size = *(u16 *)(buf + hdr_size); > - if (ds_size >= image_size) > - return -EINVAL; > - > - if (!sw->safe_mode) { > - u16 device_id; > - > - /* > - * Make sure the device ID in the image matches the one > - * we read from the switch config space. > - */ > - device_id = *(u16 *)(buf + hdr_size + NVM_DEVID); > - if (device_id != sw->config.device_id) > - return -EINVAL; > - > - if (sw->generation < 3) { > - /* Write CSS headers first */ > - ret = dma_port_flash_write(sw->dma_port, > - DMA_PORT_CSS_ADDRESS, buf + NVM_CSS, > - DMA_PORT_CSS_MAX_SIZE); > - if (ret) > - return ret; > - } > + /*Vendors to validate before write to router NVM*/ Extra ' ' character please. thanks, greg k-h