thunderbolt: thunderbolt: add vendor's NVM formats

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(+)

--
2.34.1

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

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

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