Date:   Mon, 15 Aug 2022 14:20:10 -0500
From:   "Limonciello, Mario" <mario.limonciello@....com>
To:     Szuying Chen <chensiying21@...il.com>, gregkh@...uxfoundation.org,
        mika.westerberg@...ux.intel.com, andreas.noever@...il.com,
        michael.jamet@...el.com, YehezkelShB@...il.com,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     Yd_Tseng@...edia.com.tw, Chloe_Chen@...edia.com.tw,
        Richard_Hsu@...edia.com.tw, Richard Hughes <rhughes@...hat.com>
Subject: Re: [PATCH] thunderbolt: thunderbolt: add vendor's NVM formats

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@...edia.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@...edia.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@...e.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
>>
>>
> 

Powered by blists - more mailing lists