| Message ID | 20220815041145.35629-1-chensiying21@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | thunderbolt: thunderbolt: add vendor's NVM formats | expand |
On Mon, Aug 15, 2022 at 12:11:45PM +0800, Szuying Chen wrote: > From: Szuying Chen <Chloe_Chen@asmedia.com.tw> > > The patch add tb_nvm_validate() contain an array that has functions > pointers to asmedia_nvm_validate(). > And asmedia_nvm_validate() that recognize supported vendor works in one > of the following cases: > Case nvm_upgrade: enable nvm's attribute by setting no_nvm_upgrade > flag to create nvm_authenticate file node. > Case nvm_add:add active/non-active NVM devices. > Case nvm_write:update firmware to non-ative NVM device. > > Our patches were through checkpatch.pl. But the file(switch.c.) > have existed 13 warning before we patch it. > > Signed-off-by: Szuying Chen <Chloe_Chen@asmedia.com.tw> > --- > drivers/thunderbolt/nvm.c | 147 +++++++++++++++++++++++++++++++++++ > drivers/thunderbolt/switch.c | 17 ++++ > drivers/thunderbolt/tb.h | 18 +++++ > 3 files changed, 182 insertions(+) Hi, This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree. You are receiving this message because of the following common error(s) as indicated below: - This looks like a new version of a previously submitted patch, but you did not list below the --- line any changes from the previous version. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/SubmittingPatches for what needs to be done here to properly describe this. If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers. thanks, greg k-h's patch email bot
On Mon, Aug 15, 2022 at 12:11:45PM +0800, Szuying Chen wrote: > +/* ASMedia specific validation mode */ > +#define nvm_upgrade 0 > +#define nvm_add 1 > +#define nvm_write 2 Why is this not an enum? And why is this specific to a single vendor, yet that vendor name is not in the #define? > +struct nvm_asmedia { > + u32 date; __le32? > + u32 customerID:16; > + u32 version:8; > + u32 reserved:8; Are you sure these bitfields are correct on all platforms? thanks, greg k-h
Hi Szuying,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.0-rc1 next-20220815]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Szuying-Chen/thunderbolt-thunderbolt-add-vendor-s-NVM-formats/20220815-121330
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220815/202208152107.P6jLM6te-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/de39c1897bc8c08ddfa4d3019c95314e697b7256
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Szuying-Chen/thunderbolt-thunderbolt-add-vendor-s-NVM-formats/20220815-121330
git checkout de39c1897bc8c08ddfa4d3019c95314e697b7256
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash drivers/thunderbolt/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/thunderbolt/nvm.c:150: warning: Function parameter or member 'mode' not described in 'tb_nvm_validate'
>> drivers/thunderbolt/nvm.c:150: warning: expecting prototype for tb_nvm_vendor_handle(). Prototype was for tb_nvm_validate() instead
vim +150 drivers/thunderbolt/nvm.c
143
144 /**
145 * tb_nvm_vendor_handle() - support vendor's NVM format
146 * @sw: Thunderbolt switch
147 * @handle: 0:NvmUpgradeSuppport, 1:NvmAdd, 2:NvmWrite
148 */
149 int tb_nvm_validate(struct tb_switch *sw, unsigned int mode)
> 150 {
151 int res, i;
152
153 for (i = 0; i < ARRAY_SIZE(tb_nvm_vendors); i++) {
154 const struct tb_nvm_id *id = &tb_nvm_vendors[i];
155
156 if (id->hw_vendor_id && id->hw_vendor_id != sw->config.vendor_id)
157 continue;
158
159 res = id->validate(sw, mode);
160 }
161 return res;
162 }
163
+hughsie for additional comments Various inline comments below. On 8/14/2022 23:11, Szuying Chen wrote: > From: Szuying Chen <Chloe_Chen@asmedia.com.tw> > > The patch add tb_nvm_validate() contain an array that has functions > pointers to asmedia_nvm_validate(). > And asmedia_nvm_validate() that recognize supported vendor works in one > of the following cases: > Case nvm_upgrade: enable nvm's attribute by setting no_nvm_upgrade > flag to create nvm_authenticate file node. > Case nvm_add:add active/non-active NVM devices. > Case nvm_write:update firmware to non-ative NVM device. > > Our patches were through checkpatch.pl. But the file(switch.c.) > have existed 13 warning before we patch it. Please feel free to add other patches to the series to clean up warnings. Just because you didn't introduce them doesn't mean you can't fix them =) > > Signed-off-by: Szuying Chen <Chloe_Chen@asmedia.com.tw> > --- > drivers/thunderbolt/nvm.c | 147 +++++++++++++++++++++++++++++++++++ > drivers/thunderbolt/switch.c | 17 ++++ > drivers/thunderbolt/tb.h | 18 +++++ > 3 files changed, 182 insertions(+) > > diff --git a/drivers/thunderbolt/nvm.c b/drivers/thunderbolt/nvm.c > index b3f310389378..6db2034ec8e5 100644 > --- a/drivers/thunderbolt/nvm.c > +++ b/drivers/thunderbolt/nvm.c > @@ -9,11 +9,158 @@ > #include <linux/idr.h> > #include <linux/slab.h> > #include <linux/vmalloc.h> > +#include <linux/pm_runtime.h> > > #include "tb.h" > > static DEFINE_IDA(nvm_ida); > > +static int tb_switch_nvm_read(void *priv, unsigned int offset, void *val, > + size_t bytes) > +{ > + struct tb_nvm *nvm = priv; > + struct tb_switch *sw = tb_to_switch(nvm->dev); > + int ret; > + > + pm_runtime_get_sync(&sw->dev); > + if (!mutex_trylock(&sw->tb->lock)) { > + ret = restart_syscall(); > + goto out; > + } > + ret = usb4_switch_nvm_read(sw, offset, val, bytes); > + mutex_unlock(&sw->tb->lock); > + > +out: > + pm_runtime_mark_last_busy(&sw->dev); > + pm_runtime_put_autosuspend(&sw->dev); > + > + return ret; > +} > + > +static int tb_switch_nvm_write(void *priv, unsigned int offset, void *val, > + size_t bytes) > +{ > + struct tb_nvm *nvm = priv; > + struct tb_switch *sw = tb_to_switch(nvm->dev); > + int ret; > + > + if (!mutex_trylock(&sw->tb->lock)) > + return restart_syscall(); > + > + /* > + * Since writing the NVM image might require some special steps, > + * for example when CSS headers are written, we cache the image > + * locally here and handle the special cases when the user asks > + * us to authenticate the image. > + */ > + ret = tb_nvm_write_buf(nvm, offset, val, bytes); > + mutex_unlock(&sw->tb->lock); > + > + return ret; > +} > + > +static int asmedia_nvm_validate(struct tb_switch *sw, unsigned int mode) > +{ > + struct tb_nvm *nvm; > + u32 val; > + u32 nvm_size; > + int ret = 0; > + unsigned int image_size; > + const u8 *buf = sw->nvm->buf; > + > + switch (mode) { > + case nvm_upgrade: > + if (sw->no_nvm_upgrade) > + sw->no_nvm_upgrade = false; > + > + break; > + > + case nvm_add: > + nvm = tb_nvm_alloc(&sw->dev); > + if (IS_ERR(nvm)) { > + ret = PTR_ERR(nvm); > + break; > + } > + > + ret = usb4_switch_nvm_read(sw, NVM_Date, &val, sizeof(val)); > + if (ret) > + break; > + > + nvm->nvm_asm.date = (((u8)val) << 0x10 | ((u8)(val >> 0x8)) << 0x8 | (u8)(val >> 0x10)); > + ret = usb4_switch_nvm_read(sw, NVM_CUSTOMER_ID, &val, sizeof(val)); > + if (ret) > + break; > + > + nvm->nvm_asm.customerID = (((u8)val) << 0x8 | ((u8)(val >> 0x8))); > + nvm->nvm_asm.version = (u8)(val >> 0x10); > + nvm_size = SZ_512K; > + ret = tb_nvm_add_active(nvm, nvm_size, tb_switch_nvm_read); > + if (ret) > + break; > + > + ret = tb_nvm_add_non_active(nvm, NVM_MAX_SIZE, tb_switch_nvm_write); > + if (ret) > + break; > + > + sw->nvm = nvm; > + break; > + > + case nvm_write: > + if (!buf) { > + ret = -EINVAL; > + break; > + } > + image_size = sw->nvm->buf_data_size; > + if (image_size < NVM_MIN_SIZE || image_size > NVM_MAX_SIZE) { > + ret = -EINVAL; > + break; > + } > + ret = usb4_switch_nvm_write(sw, 0, buf, image_size); > + if (!ret) > + sw->nvm->flushed = true; > + > + break; > + > + default: > + break; > + } > + > + if ((mode == nvm_add) && (ret != 0)) > + tb_nvm_free(sw->nvm); > + > + return ret; > +} > + > +struct tb_nvm_id { > + u16 hw_vendor_id; > + int (*validate)(struct tb_switch *sw, unsigned int handle); > +}; > + > +static const struct tb_nvm_id tb_nvm_vendors[] = { > + /* ASMedia software CM firmware upgrade */ > + { 0x174c, asmedia_nvm_validate }, > +}; > + > +/** > + * tb_nvm_vendor_handle() - support vendor's NVM format > + * @sw: Thunderbolt switch > + * @handle: 0:NvmUpgradeSuppport, 1:NvmAdd, 2:NvmWrite > + */ > +int tb_nvm_validate(struct tb_switch *sw, unsigned int mode) > +{ > + int res, i; > + > + for (i = 0; i < ARRAY_SIZE(tb_nvm_vendors); i++) { > + const struct tb_nvm_id *id = &tb_nvm_vendors[i]; > + > + if (id->hw_vendor_id && id->hw_vendor_id != sw->config.vendor_id) > + continue; > + > + res = id->validate(sw, mode); > + } > + return res; > +} > + > /** > * 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..352e64f3dc92 100644 > --- a/drivers/thunderbolt/switch.c > +++ b/drivers/thunderbolt/switch.c > @@ -114,6 +114,14 @@ static int nvm_validate_and_write(struct tb_switch *sw) > if (image_size < NVM_MIN_SIZE || image_size > NVM_MAX_SIZE) > return -EINVAL; > > + /* > + * Vendor's nvm write. If the image has been flushed to the > + * storage are, nvm write is complete. > + */ > + ret = tb_nvm_validate(sw, nvm_write); > + if (sw->nvm->flushed) > + return ret; > + > /* > * FARB pointer must point inside the image and must at least > * contain parts of the digital section we will be reading here. > @@ -390,6 +398,11 @@ static int tb_switch_nvm_add(struct tb_switch *sw) > if (!nvm_readable(sw)) > return 0; > > + /* Vendor's NVM formats add */ > + ret = tb_nvm_validate(sw, nvm_add); > + if (ret) > + return ret; > + > /* > * The NVM format of non-Intel hardware is not known so > * currently restrict NVM upgrade for Intel hardware. We may This comment should be adjusted as after your patch lands both Intel and ASMedia formats are known and included. > @@ -1953,6 +1966,9 @@ static ssize_t nvm_version_show(struct device *dev, > ret = -ENODATA; > else if (!sw->nvm) > ret = -EAGAIN; > + /*ASMedia NVM version show format xxxxxx_xxxx_xx */ > + else if (sw->config.vendor_id == 0x174C) > + ret = sprintf(buf, "%06x_%04x_%02x\n", sw->nvm->nvm_asm.date, sw->nvm->nvm_asm.customerID, sw->nvm->nvm_asm.version); Are you hard-pressed to use this specific format for the string? I feel like it's overloading the definition of a version string quite a bit. I also worry that userspace has come to expect "major.minor" for this and making your string use 2 decimals may mean more deviations for userspace too. Perhaps should you just export it instead as: "%02x.%06x", sw->nvm->nvm_asm.version, sw->nvm->nvm_asm.date And move the customer ID into another sysfs file? I would think this fits pretty well with the existing "vendor" or "device" sysfs file depending upon it's meaning. If you do end up having a strong reason for deviating the version string format, then I think you should document both what the Intel format is (major.minor) and your format explicitly in Documentation/admin-guide/thunderbolt.rst. > else > ret = sprintf(buf, "%x.%x\n", sw->nvm->major, sw->nvm->minor); > > @@ -2860,6 +2876,7 @@ int tb_switch_add(struct tb_switch *sw) > tb_sw_dbg(sw, "uid: %#llx\n", sw->uid); > > tb_check_quirks(sw); > + tb_nvm_validate(sw, nvm_upgrade); > > ret = tb_switch_set_uuid(sw); > if (ret) { > diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h > index 5db76de40cc1..7f20f10352d9 100644 > --- a/drivers/thunderbolt/tb.h > +++ b/drivers/thunderbolt/tb.h > @@ -28,6 +28,22 @@ > #define NVM_VERSION 0x08 > #define NVM_FLASH_SIZE 0x45 > > +/* ASMedia specific NVM offsets */ > +#define NVM_Date 0x1c > +#define NVM_CUSTOMER_ID 0x28 > + > +/* ASMedia specific validation mode */ > +#define nvm_upgrade 0 > +#define nvm_add 1 > +#define nvm_write 2 As all of these values are ASMedia specific, I think the #defines should have ASMEDIA in the name. I know Greg mentioned to roll into an enum, and I think you still can but make it something like: #define ASMEDIA_NVM_DATE 0x1c #define ASMEDIA_NVM_CUSTOMER_ID 0x28 enum asmeda_nvm_ops { ASMEDIA_NVM_UPGRADE = 0, ASMEDIA_NVM_ADD = 1, ASMEDIA_NVM_WRITE = 2, }; > + > +struct nvm_asmedia { > + u32 date; > + u32 customerID:16; > + u32 version:8; > + u32 reserved:8; > +}; > + > /** > * struct tb_nvm - Structure holding NVM information > * @dev: Owner of the NVM > @@ -57,6 +73,7 @@ struct tb_nvm { > size_t buf_data_size; > bool authenticating; > bool flushed; > + struct nvm_asmedia nvm_asm; Furthermore if you follow my suggestion on how to encode the version you can re-use the 'major' and 'minor' members from this struct and don't need to deviate in any way from it for your data. * Major would map to your "version". * Minor would map to "date". You could instead then store the customer ID into the switches vendor ID or device ID member (whichever makes more sense). > }; > > enum tb_nvm_write_ops { > @@ -736,6 +753,7 @@ static inline void tb_domain_put(struct tb *tb) > put_device(&tb->dev); > } > > +int tb_nvm_validate(struct tb_switch *sw, unsigned int mode); > struct tb_nvm *tb_nvm_alloc(struct device *dev); > int tb_nvm_add_active(struct tb_nvm *nvm, size_t size, nvmem_reg_read_t reg_read); > int tb_nvm_write_buf(struct tb_nvm *nvm, unsigned int offset, void *val, > -- > 2.34.1 > >
On 8/15/2022 12:28, Limonciello, Mario wrote: > +hughsie for additional comments > > Various inline comments below. > > On 8/14/2022 23:11, Szuying Chen wrote: >> From: Szuying Chen <Chloe_Chen@asmedia.com.tw> >> >> The patch add tb_nvm_validate() contain an array that has functions >> pointers to asmedia_nvm_validate(). >> And asmedia_nvm_validate() that recognize supported vendor works in one >> of the following cases: >> Case nvm_upgrade: enable nvm's attribute by setting no_nvm_upgrade >> flag to create nvm_authenticate file node. >> Case nvm_add:add active/non-active NVM devices. >> Case nvm_write:update firmware to non-ative NVM device. >> >> Our patches were through checkpatch.pl. But the file(switch.c.) >> have existed 13 warning before we patch it. > > Please feel free to add other patches to the series to clean up > warnings. Just because you didn't introduce them doesn't mean you can't > fix them =) > >> >> Signed-off-by: Szuying Chen <Chloe_Chen@asmedia.com.tw> >> --- >> drivers/thunderbolt/nvm.c | 147 +++++++++++++++++++++++++++++++++++ >> drivers/thunderbolt/switch.c | 17 ++++ >> drivers/thunderbolt/tb.h | 18 +++++ >> 3 files changed, 182 insertions(+) When you submit patches to the mailing list, you'll want to do two things I haven't noticed you doing: 1) When you create the patch with git format-patch use the "--subject-prefix" option to set your subject prefix. I see that your patch has been submitted at least 3 times but Lore groups all 3 submissions together. The first should have been [PATCH], next should have been [PATCH v2], next [PATCH v3]. So I think technically your next submission SHOULD be [PATCH v4] 2) You should add below the --- comments about what changed from last submission. This helps people have reviewed it in the past be able to better focus on what they should look most closely at. It should be something like this: ``` . . . Signed-off-by: User Name <user@name.com> --- changes from v3->v4: - Foo the bar Note: The three previous submissions accidentally used the same subject prefix. This changelog is relative to the most recent submission at $URL. drivers/thunderbolt/nvm.c | 147 . . . ``` >> >> diff --git a/drivers/thunderbolt/nvm.c b/drivers/thunderbolt/nvm.c >> index b3f310389378..6db2034ec8e5 100644 >> --- a/drivers/thunderbolt/nvm.c >> +++ b/drivers/thunderbolt/nvm.c >> @@ -9,11 +9,158 @@ >> #include <linux/idr.h> >> #include <linux/slab.h> >> #include <linux/vmalloc.h> >> +#include <linux/pm_runtime.h> >> >> #include "tb.h" >> >> static DEFINE_IDA(nvm_ida); >> >> +static int tb_switch_nvm_read(void *priv, unsigned int offset, void >> *val, >> + size_t bytes) >> +{ >> + struct tb_nvm *nvm = priv; >> + struct tb_switch *sw = tb_to_switch(nvm->dev); >> + int ret; >> + >> + pm_runtime_get_sync(&sw->dev); >> + if (!mutex_trylock(&sw->tb->lock)) { >> + ret = restart_syscall(); >> + goto out; >> + } >> + ret = usb4_switch_nvm_read(sw, offset, val, bytes); >> + mutex_unlock(&sw->tb->lock); >> + >> +out: >> + pm_runtime_mark_last_busy(&sw->dev); >> + pm_runtime_put_autosuspend(&sw->dev); >> + >> + return ret; >> +} >> + >> +static int tb_switch_nvm_write(void *priv, unsigned int offset, void >> *val, >> + size_t bytes) >> +{ >> + struct tb_nvm *nvm = priv; >> + struct tb_switch *sw = tb_to_switch(nvm->dev); >> + int ret; >> + >> + if (!mutex_trylock(&sw->tb->lock)) >> + return restart_syscall(); >> + >> + /* >> + * Since writing the NVM image might require some special steps, >> + * for example when CSS headers are written, we cache the image >> + * locally here and handle the special cases when the user asks >> + * us to authenticate the image. >> + */ >> + ret = tb_nvm_write_buf(nvm, offset, val, bytes); >> + mutex_unlock(&sw->tb->lock); >> + >> + return ret; >> +} >> + >> +static int asmedia_nvm_validate(struct tb_switch *sw, unsigned int mode) >> +{ >> + struct tb_nvm *nvm; >> + u32 val; >> + u32 nvm_size; >> + int ret = 0; >> + unsigned int image_size; >> + const u8 *buf = sw->nvm->buf; >> + >> + switch (mode) { >> + case nvm_upgrade: >> + if (sw->no_nvm_upgrade) >> + sw->no_nvm_upgrade = false; >> + >> + break; >> + >> + case nvm_add: >> + nvm = tb_nvm_alloc(&sw->dev); >> + if (IS_ERR(nvm)) { >> + ret = PTR_ERR(nvm); >> + break; >> + } >> + >> + ret = usb4_switch_nvm_read(sw, NVM_Date, &val, sizeof(val)); >> + if (ret) >> + break; >> + >> + nvm->nvm_asm.date = (((u8)val) << 0x10 | ((u8)(val >> 0x8)) >> << 0x8 | (u8)(val >> 0x10)); >> + ret = usb4_switch_nvm_read(sw, NVM_CUSTOMER_ID, &val, >> sizeof(val)); >> + if (ret) >> + break; >> + >> + nvm->nvm_asm.customerID = (((u8)val) << 0x8 | ((u8)(val >> >> 0x8))); >> + nvm->nvm_asm.version = (u8)(val >> 0x10); >> + nvm_size = SZ_512K; >> + ret = tb_nvm_add_active(nvm, nvm_size, tb_switch_nvm_read); >> + if (ret) >> + break; >> + >> + ret = tb_nvm_add_non_active(nvm, NVM_MAX_SIZE, >> tb_switch_nvm_write); >> + if (ret) >> + break; >> + >> + sw->nvm = nvm; >> + break; >> + >> + case nvm_write: >> + if (!buf) { >> + ret = -EINVAL; >> + break; >> + } >> + image_size = sw->nvm->buf_data_size; >> + if (image_size < NVM_MIN_SIZE || image_size > NVM_MAX_SIZE) { >> + ret = -EINVAL; >> + break; >> + } >> + ret = usb4_switch_nvm_write(sw, 0, buf, image_size); >> + if (!ret) >> + sw->nvm->flushed = true; >> + >> + break; >> + >> + default: >> + break; >> + } >> + >> + if ((mode == nvm_add) && (ret != 0)) >> + tb_nvm_free(sw->nvm); >> + >> + return ret; >> +} >> + >> +struct tb_nvm_id { >> + u16 hw_vendor_id; >> + int (*validate)(struct tb_switch *sw, unsigned int handle); >> +}; >> + >> +static const struct tb_nvm_id tb_nvm_vendors[] = { >> + /* ASMedia software CM firmware upgrade */ >> + { 0x174c, asmedia_nvm_validate }, >> +}; >> + >> +/** >> + * tb_nvm_vendor_handle() - support vendor's NVM format >> + * @sw: Thunderbolt switch >> + * @handle: 0:NvmUpgradeSuppport, 1:NvmAdd, 2:NvmWrite >> + */ >> +int tb_nvm_validate(struct tb_switch *sw, unsigned int mode) >> +{ >> + int res, i; >> + >> + for (i = 0; i < ARRAY_SIZE(tb_nvm_vendors); i++) { >> + const struct tb_nvm_id *id = &tb_nvm_vendors[i]; >> + >> + if (id->hw_vendor_id && id->hw_vendor_id != >> sw->config.vendor_id) >> + continue; >> + >> + res = id->validate(sw, mode); >> + } >> + return res; >> +} >> + >> /** >> * 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..352e64f3dc92 100644 >> --- a/drivers/thunderbolt/switch.c >> +++ b/drivers/thunderbolt/switch.c >> @@ -114,6 +114,14 @@ static int nvm_validate_and_write(struct >> tb_switch *sw) >> if (image_size < NVM_MIN_SIZE || image_size > NVM_MAX_SIZE) >> return -EINVAL; >> >> + /* >> + * Vendor's nvm write. If the image has been flushed to the >> + * storage are, nvm write is complete. >> + */ >> + ret = tb_nvm_validate(sw, nvm_write); >> + if (sw->nvm->flushed) >> + return ret; >> + >> /* >> * FARB pointer must point inside the image and must at least >> * contain parts of the digital section we will be reading here. >> @@ -390,6 +398,11 @@ static int tb_switch_nvm_add(struct tb_switch *sw) >> if (!nvm_readable(sw)) >> return 0; >> >> + /* Vendor's NVM formats add */ >> + ret = tb_nvm_validate(sw, nvm_add); >> + if (ret) >> + return ret; >> + >> /* >> * The NVM format of non-Intel hardware is not known so >> * currently restrict NVM upgrade for Intel hardware. We may > > This comment should be adjusted as after your patch lands both Intel and > ASMedia formats are known and included. > >> @@ -1953,6 +1966,9 @@ static ssize_t nvm_version_show(struct device *dev, >> ret = -ENODATA; >> else if (!sw->nvm) >> ret = -EAGAIN; >> + /*ASMedia NVM version show format xxxxxx_xxxx_xx */ >> + else if (sw->config.vendor_id == 0x174C) >> + ret = sprintf(buf, "%06x_%04x_%02x\n", sw->nvm->nvm_asm.date, >> sw->nvm->nvm_asm.customerID, sw->nvm->nvm_asm.version); > > Are you hard-pressed to use this specific format for the string? I feel > like it's overloading the definition of a version string quite a bit. > > I also worry that userspace has come to expect "major.minor" for this > and making your string use 2 decimals may mean more deviations for > userspace too. > > Perhaps should you just export it instead as: > > "%02x.%06x", sw->nvm->nvm_asm.version, sw->nvm->nvm_asm.date > > And move the customer ID into another sysfs file? I would think this > fits pretty well with the existing "vendor" or "device" sysfs file > depending upon it's meaning. > > If you do end up having a strong reason for deviating the version string > format, then I think you should document both what the Intel format is > (major.minor) and your format explicitly in > Documentation/admin-guide/thunderbolt.rst. > >> else >> ret = sprintf(buf, "%x.%x\n", sw->nvm->major, sw->nvm->minor); >> >> @@ -2860,6 +2876,7 @@ int tb_switch_add(struct tb_switch *sw) >> tb_sw_dbg(sw, "uid: %#llx\n", sw->uid); >> >> tb_check_quirks(sw); >> + tb_nvm_validate(sw, nvm_upgrade); >> >> ret = tb_switch_set_uuid(sw); >> if (ret) { >> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h >> index 5db76de40cc1..7f20f10352d9 100644 >> --- a/drivers/thunderbolt/tb.h >> +++ b/drivers/thunderbolt/tb.h >> @@ -28,6 +28,22 @@ >> #define NVM_VERSION 0x08 >> #define NVM_FLASH_SIZE 0x45 >> >> +/* ASMedia specific NVM offsets */ >> +#define NVM_Date 0x1c >> +#define NVM_CUSTOMER_ID 0x28 >> + >> +/* ASMedia specific validation mode */ >> +#define nvm_upgrade 0 >> +#define nvm_add 1 >> +#define nvm_write 2 > > As all of these values are ASMedia specific, I think the #defines should > have ASMEDIA in the name. I know Greg mentioned to roll into an enum, > and I think you still can but make it something like: > > #define ASMEDIA_NVM_DATE 0x1c > #define ASMEDIA_NVM_CUSTOMER_ID 0x28 > enum asmeda_nvm_ops { > ASMEDIA_NVM_UPGRADE = 0, > ASMEDIA_NVM_ADD = 1, > ASMEDIA_NVM_WRITE = 2, > }; > >> + >> +struct nvm_asmedia { >> + u32 date; >> + u32 customerID:16; >> + u32 version:8; >> + u32 reserved:8; >> +}; >> + >> /** >> * struct tb_nvm - Structure holding NVM information >> * @dev: Owner of the NVM >> @@ -57,6 +73,7 @@ struct tb_nvm { >> size_t buf_data_size; >> bool authenticating; >> bool flushed; >> + struct nvm_asmedia nvm_asm; > > Furthermore if you follow my suggestion on how to encode the version you > can re-use the 'major' and 'minor' members from this struct and don't > need to deviate in any way from it for your data. > > * Major would map to your "version". > * Minor would map to "date". > > You could instead then store the customer ID into the switches vendor ID > or device ID member (whichever makes more sense). > >> }; >> >> enum tb_nvm_write_ops { >> @@ -736,6 +753,7 @@ static inline void tb_domain_put(struct tb *tb) >> put_device(&tb->dev); >> } >> >> +int tb_nvm_validate(struct tb_switch *sw, unsigned int mode); >> struct tb_nvm *tb_nvm_alloc(struct device *dev); >> int tb_nvm_add_active(struct tb_nvm *nvm, size_t size, >> nvmem_reg_read_t reg_read); >> int tb_nvm_write_buf(struct tb_nvm *nvm, unsigned int offset, void >> *val, >> -- >> 2.34.1 >> >> >
Hi Szuying,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.0-rc1 next-20220815]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Szuying-Chen/thunderbolt-thunderbolt-add-vendor-s-NVM-formats/20220815-121330
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
config: i386-randconfig-m021
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
smatch warnings:
drivers/thunderbolt/nvm.c:159 tb_nvm_validate() warn: inconsistent indenting
drivers/thunderbolt/nvm.c:161 tb_nvm_validate() error: uninitialized symbol 'res'.
vim +159 drivers/thunderbolt/nvm.c
143
144 /**
145 * tb_nvm_vendor_handle() - support vendor's NVM format
146 * @sw: Thunderbolt switch
147 * @handle: 0:NvmUpgradeSuppport, 1:NvmAdd, 2:NvmWrite
148 */
149 int tb_nvm_validate(struct tb_switch *sw, unsigned int mode)
150 {
151 int res, i;
152
153 for (i = 0; i < ARRAY_SIZE(tb_nvm_vendors); i++) {
154 const struct tb_nvm_id *id = &tb_nvm_vendors[i];
155
156 if (id->hw_vendor_id && id->hw_vendor_id != sw->config.vendor_id)
157 continue;
158
> 159 res = id->validate(sw, mode);
160 }
> 161 return res;
162 }
163
diff --git a/drivers/thunderbolt/nvm.c b/drivers/thunderbolt/nvm.c index b3f310389378..6db2034ec8e5 100644 --- a/drivers/thunderbolt/nvm.c +++ b/drivers/thunderbolt/nvm.c @@ -9,11 +9,158 @@ #include <linux/idr.h> #include <linux/slab.h> #include <linux/vmalloc.h> +#include <linux/pm_runtime.h> #include "tb.h" static DEFINE_IDA(nvm_ida); +static int tb_switch_nvm_read(void *priv, unsigned int offset, void *val, + size_t bytes) +{ + struct tb_nvm *nvm = priv; + struct tb_switch *sw = tb_to_switch(nvm->dev); + int ret; + + pm_runtime_get_sync(&sw->dev); + if (!mutex_trylock(&sw->tb->lock)) { + ret = restart_syscall(); + goto out; + } + ret = usb4_switch_nvm_read(sw, offset, val, bytes); + mutex_unlock(&sw->tb->lock); + +out: + pm_runtime_mark_last_busy(&sw->dev); + pm_runtime_put_autosuspend(&sw->dev); + + return ret; +} + +static int tb_switch_nvm_write(void *priv, unsigned int offset, void *val, + size_t bytes) +{ + struct tb_nvm *nvm = priv; + struct tb_switch *sw = tb_to_switch(nvm->dev); + int ret; + + if (!mutex_trylock(&sw->tb->lock)) + return restart_syscall(); + + /* + * Since writing the NVM image might require some special steps, + * for example when CSS headers are written, we cache the image + * locally here and handle the special cases when the user asks + * us to authenticate the image. + */ + ret = tb_nvm_write_buf(nvm, offset, val, bytes); + mutex_unlock(&sw->tb->lock); + + return ret; +} + +static int asmedia_nvm_validate(struct tb_switch *sw, unsigned int mode) +{ + struct tb_nvm *nvm; + u32 val; + u32 nvm_size; + int ret = 0; + unsigned int image_size; + const u8 *buf = sw->nvm->buf; + + switch (mode) { + case nvm_upgrade: + if (sw->no_nvm_upgrade) + sw->no_nvm_upgrade = false; + + break; + + case nvm_add: + nvm = tb_nvm_alloc(&sw->dev); + if (IS_ERR(nvm)) { + ret = PTR_ERR(nvm); + break; + } + + ret = usb4_switch_nvm_read(sw, NVM_Date, &val, sizeof(val)); + if (ret) + break; + + nvm->nvm_asm.date = (((u8)val) << 0x10 | ((u8)(val >> 0x8)) << 0x8 | (u8)(val >> 0x10)); + ret = usb4_switch_nvm_read(sw, NVM_CUSTOMER_ID, &val, sizeof(val)); + if (ret) + break; + + nvm->nvm_asm.customerID = (((u8)val) << 0x8 | ((u8)(val >> 0x8))); + nvm->nvm_asm.version = (u8)(val >> 0x10); + nvm_size = SZ_512K; + ret = tb_nvm_add_active(nvm, nvm_size, tb_switch_nvm_read); + if (ret) + break; + + ret = tb_nvm_add_non_active(nvm, NVM_MAX_SIZE, tb_switch_nvm_write); + if (ret) + break; + + sw->nvm = nvm; + break; + + case nvm_write: + if (!buf) { + ret = -EINVAL; + break; + } + image_size = sw->nvm->buf_data_size; + if (image_size < NVM_MIN_SIZE || image_size > NVM_MAX_SIZE) { + ret = -EINVAL; + break; + } + ret = usb4_switch_nvm_write(sw, 0, buf, image_size); + if (!ret) + sw->nvm->flushed = true; + + break; + + default: + break; + } + + if ((mode == nvm_add) && (ret != 0)) + tb_nvm_free(sw->nvm); + + return ret; +} + +struct tb_nvm_id { + u16 hw_vendor_id; + int (*validate)(struct tb_switch *sw, unsigned int handle); +}; + +static const struct tb_nvm_id tb_nvm_vendors[] = { + /* ASMedia software CM firmware upgrade */ + { 0x174c, asmedia_nvm_validate }, +}; + +/** + * tb_nvm_vendor_handle() - support vendor's NVM format + * @sw: Thunderbolt switch + * @handle: 0:NvmUpgradeSuppport, 1:NvmAdd, 2:NvmWrite + */ +int tb_nvm_validate(struct tb_switch *sw, unsigned int mode) +{ + int res, i; + + for (i = 0; i < ARRAY_SIZE(tb_nvm_vendors); i++) { + const struct tb_nvm_id *id = &tb_nvm_vendors[i]; + + if (id->hw_vendor_id && id->hw_vendor_id != sw->config.vendor_id) + continue; + + res = id->validate(sw, mode); + } + return res; +} + /** * 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..352e64f3dc92 100644 --- a/drivers/thunderbolt/switch.c +++ b/drivers/thunderbolt/switch.c @@ -114,6 +114,14 @@ static int nvm_validate_and_write(struct tb_switch *sw) if (image_size < NVM_MIN_SIZE || image_size > NVM_MAX_SIZE) return -EINVAL; + /* + * Vendor's nvm write. If the image has been flushed to the + * storage are, nvm write is complete. + */ + ret = tb_nvm_validate(sw, nvm_write); + if (sw->nvm->flushed) + return ret; + /* * FARB pointer must point inside the image and must at least * contain parts of the digital section we will be reading here. @@ -390,6 +398,11 @@ static int tb_switch_nvm_add(struct tb_switch *sw) if (!nvm_readable(sw)) return 0; + /* Vendor's NVM formats add */ + ret = tb_nvm_validate(sw, nvm_add); + if (ret) + return ret; + /* * The NVM format of non-Intel hardware is not known so * currently restrict NVM upgrade for Intel hardware. We may @@ -1953,6 +1966,9 @@ static ssize_t nvm_version_show(struct device *dev, ret = -ENODATA; else if (!sw->nvm) ret = -EAGAIN; + /*ASMedia NVM version show format xxxxxx_xxxx_xx */ + else if (sw->config.vendor_id == 0x174C) + ret = sprintf(buf, "%06x_%04x_%02x\n", sw->nvm->nvm_asm.date, sw->nvm->nvm_asm.customerID, sw->nvm->nvm_asm.version); else ret = sprintf(buf, "%x.%x\n", sw->nvm->major, sw->nvm->minor); @@ -2860,6 +2876,7 @@ int tb_switch_add(struct tb_switch *sw) tb_sw_dbg(sw, "uid: %#llx\n", sw->uid); tb_check_quirks(sw); + tb_nvm_validate(sw, nvm_upgrade); ret = tb_switch_set_uuid(sw); if (ret) { diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h index 5db76de40cc1..7f20f10352d9 100644 --- a/drivers/thunderbolt/tb.h +++ b/drivers/thunderbolt/tb.h @@ -28,6 +28,22 @@ #define NVM_VERSION 0x08 #define NVM_FLASH_SIZE 0x45 +/* ASMedia specific NVM offsets */ +#define NVM_Date 0x1c +#define NVM_CUSTOMER_ID 0x28 + +/* ASMedia specific validation mode */ +#define nvm_upgrade 0 +#define nvm_add 1 +#define nvm_write 2 + +struct nvm_asmedia { + u32 date; + u32 customerID:16; + u32 version:8; + u32 reserved:8; +}; + /** * struct tb_nvm - Structure holding NVM information * @dev: Owner of the NVM @@ -57,6 +73,7 @@ struct tb_nvm { size_t buf_data_size; bool authenticating; bool flushed; + struct nvm_asmedia nvm_asm; }; enum tb_nvm_write_ops { @@ -736,6 +753,7 @@ static inline void tb_domain_put(struct tb *tb) put_device(&tb->dev); } +int tb_nvm_validate(struct tb_switch *sw, unsigned int mode); struct tb_nvm *tb_nvm_alloc(struct device *dev); int tb_nvm_add_active(struct tb_nvm *nvm, size_t size, nvmem_reg_read_t reg_read); int tb_nvm_write_buf(struct tb_nvm *nvm, unsigned int offset, void *val,