Currently updates to Thunderbolt and USB4 controllers are fully atomic
actions. When writing into the non-active NVM nothing gets flushed to
the hardware until authenticate is sent.
There has been some desire to improve the perceived performance of these
updates, particularly for userland that may perform the update upon
a performance sensitive time like logging out.
So allow userland to flush the image to hardware at runtime, and then
allow authenticating the image at another time.
For the Dell WD19TB some specific hardware capability exists that allows
extending this to automatically complete the update when unplugged.
Export that functionality to userspace as well.
Changes from v1 to v2:
- Improve documentation
- Drop tb-quirks.h
- Adjust function and parameter names to Mika's preferences
- Rebase onto thunderbolt.git/bleeding-edge to move on top of retimer work
Mario Limonciello (2):
thunderbolt: Add support for separating the flush to SPI and
authenticate
thunderbolt: Add support for authenticate on disconnect
.../ABI/testing/sysfs-bus-thunderbolt | 24 +++++-
drivers/thunderbolt/Makefile | 1 +
drivers/thunderbolt/eeprom.c | 2 +
drivers/thunderbolt/lc.c | 14 ++++
drivers/thunderbolt/quirks.c | 38 +++++++++
drivers/thunderbolt/switch.c | 81 +++++++++++++++----
drivers/thunderbolt/tb-quirks.h | 16 ++++
drivers/thunderbolt/tb.h | 4 +
drivers/thunderbolt/tb_regs.h | 1 +
9 files changed, 162 insertions(+), 19 deletions(-)
create mode 100644 drivers/thunderbolt/quirks.c
create mode 100644 drivers/thunderbolt/tb-quirks.h
--
2.25.1
This allows userspace to have a shorter period of time that the device
is unusable and to call it at a more convenient time.
For example flushing the image may happen while the user is using the
machine and authenticating/rebooting may happen while logging out.
Signed-off-by: Mario Limonciello <[email protected]>
---
.../ABI/testing/sysfs-bus-thunderbolt | 11 ++++-
drivers/thunderbolt/nvm.c | 1 +
drivers/thunderbolt/switch.c | 42 ++++++++++++-------
drivers/thunderbolt/tb.h | 2 +
4 files changed, 39 insertions(+), 17 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-bus-thunderbolt b/Documentation/ABI/testing/sysfs-bus-thunderbolt
index bd504ed323e8..7d0500b4d58a 100644
--- a/Documentation/ABI/testing/sysfs-bus-thunderbolt
+++ b/Documentation/ABI/testing/sysfs-bus-thunderbolt
@@ -178,11 +178,18 @@ KernelVersion: 4.13
Contact: [email protected]
Description: When new NVM image is written to the non-active NVM
area (through non_activeX NVMem device), the
- authentication procedure is started by writing 1 to
- this file. If everything goes well, the device is
+ authentication procedure is started by writing to
+ this file.
+ If everything goes well, the device is
restarted with the new NVM firmware. If the image
verification fails an error code is returned instead.
+ This file will accept writing values "1" or "2"
+ - Writing "1" will flush the image to the storage
+ area and authenticate the image in one action.
+ - Writing "2" will run some basic validation on the image
+ and flush it to the storage area.
+
When read holds status of the last authentication
operation if an error occurred during the process. This
is directly the status value from the DMA configuration
diff --git a/drivers/thunderbolt/nvm.c b/drivers/thunderbolt/nvm.c
index 4c6aa06ab3d5..29de6d95c6e7 100644
--- a/drivers/thunderbolt/nvm.c
+++ b/drivers/thunderbolt/nvm.c
@@ -100,6 +100,7 @@ int tb_nvm_write_buf(struct tb_nvm *nvm, unsigned int offset, void *val,
return -ENOMEM;
}
+ nvm->flushed = false;
nvm->buf_data_size = offset + bytes;
memcpy(nvm->buf + offset, val, bytes);
return 0;
diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 817c66c7adcf..bbfbfebeee7f 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -26,6 +26,11 @@ struct nvm_auth_status {
u32 status;
};
+enum nvm_write_ops {
+ WRITE_AND_AUTHENTICATE = 1,
+ WRITE_ONLY = 2,
+};
+
/*
* Hold NVM authentication failure status per switch This information
* needs to stay around even when the switch gets power cycled so we
@@ -155,8 +160,12 @@ static int nvm_validate_and_write(struct tb_switch *sw)
}
if (tb_switch_is_usb4(sw))
- return usb4_switch_nvm_write(sw, 0, buf, image_size);
- return dma_port_flash_write(sw->dma_port, 0, buf, image_size);
+ ret = usb4_switch_nvm_write(sw, 0, buf, image_size);
+ else
+ ret = dma_port_flash_write(sw->dma_port, 0, buf, image_size);
+ if (!ret)
+ sw->nvm->flushed = true;
+ return ret;
}
static int nvm_authenticate_host_dma_port(struct tb_switch *sw)
@@ -1488,7 +1497,7 @@ static ssize_t nvm_authenticate_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t count)
{
struct tb_switch *sw = tb_to_switch(dev);
- bool val;
+ int val;
int ret;
pm_runtime_get_sync(&sw->dev);
@@ -1504,25 +1513,28 @@ static ssize_t nvm_authenticate_store(struct device *dev,
goto exit_unlock;
}
- ret = kstrtobool(buf, &val);
+ ret = kstrtoint(buf, 10, &val);
if (ret)
goto exit_unlock;
/* Always clear the authentication status */
nvm_clear_auth_status(sw);
- if (val) {
- if (!sw->nvm->buf) {
- ret = -EINVAL;
- goto exit_unlock;
- }
-
- ret = nvm_validate_and_write(sw);
- if (ret)
- goto exit_unlock;
+ if (val > 0) {
+ if (!sw->nvm->flushed) {
+ if (!sw->nvm->buf) {
+ ret = -EINVAL;
+ goto exit_unlock;
+ }
- sw->nvm->authenticating = true;
- ret = nvm_authenticate(sw);
+ ret = nvm_validate_and_write(sw);
+ if (ret || val == WRITE_ONLY)
+ goto exit_unlock;
+ }
+ if (val == WRITE_AND_AUTHENTICATE) {
+ sw->nvm->authenticating = true;
+ ret = nvm_authenticate(sw);
+ }
}
exit_unlock:
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index 736d1589c31e..43a8ca2eb3d8 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -42,6 +42,7 @@
*
* The user of this structure needs to handle serialization of possible
* concurrent access.
+ * @flushed: The image has been flushed to the storage area
*/
struct tb_nvm {
struct device *dev;
@@ -53,6 +54,7 @@ struct tb_nvm {
void *buf;
size_t buf_data_size;
bool authenticating;
+ bool flushed;
};
#define TB_SWITCH_KEY_SIZE 32
--
2.25.1
On Mon, Jun 22, 2020 at 01:57:57PM -0500, Mario Limonciello wrote:
> This allows userspace to have a shorter period of time that the device
> is unusable and to call it at a more convenient time.
>
> For example flushing the image may happen while the user is using the
> machine and authenticating/rebooting may happen while logging out.
>
> Signed-off-by: Mario Limonciello <[email protected]>
> ---
> .../ABI/testing/sysfs-bus-thunderbolt | 11 ++++-
> drivers/thunderbolt/nvm.c | 1 +
> drivers/thunderbolt/switch.c | 42 ++++++++++++-------
> drivers/thunderbolt/tb.h | 2 +
> 4 files changed, 39 insertions(+), 17 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-thunderbolt b/Documentation/ABI/testing/sysfs-bus-thunderbolt
> index bd504ed323e8..7d0500b4d58a 100644
> --- a/Documentation/ABI/testing/sysfs-bus-thunderbolt
> +++ b/Documentation/ABI/testing/sysfs-bus-thunderbolt
> @@ -178,11 +178,18 @@ KernelVersion: 4.13
> Contact: [email protected]
> Description: When new NVM image is written to the non-active NVM
> area (through non_activeX NVMem device), the
> - authentication procedure is started by writing 1 to
> - this file. If everything goes well, the device is
> + authentication procedure is started by writing to
> + this file.
> + If everything goes well, the device is
> restarted with the new NVM firmware. If the image
> verification fails an error code is returned instead.
>
> + This file will accept writing values "1" or "2"
> + - Writing "1" will flush the image to the storage
> + area and authenticate the image in one action.
> + - Writing "2" will run some basic validation on the image
> + and flush it to the storage area.
> +
> When read holds status of the last authentication
> operation if an error occurred during the process. This
> is directly the status value from the DMA configuration
> diff --git a/drivers/thunderbolt/nvm.c b/drivers/thunderbolt/nvm.c
> index 4c6aa06ab3d5..29de6d95c6e7 100644
> --- a/drivers/thunderbolt/nvm.c
> +++ b/drivers/thunderbolt/nvm.c
> @@ -100,6 +100,7 @@ int tb_nvm_write_buf(struct tb_nvm *nvm, unsigned int offset, void *val,
> return -ENOMEM;
> }
>
> + nvm->flushed = false;
This means every write invalidates the "flushed" state, right?
> nvm->buf_data_size = offset + bytes;
> memcpy(nvm->buf + offset, val, bytes);
> return 0;
> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
> index 817c66c7adcf..bbfbfebeee7f 100644
> --- a/drivers/thunderbolt/switch.c
> +++ b/drivers/thunderbolt/switch.c
> @@ -26,6 +26,11 @@ struct nvm_auth_status {
> u32 status;
> };
>
> +enum nvm_write_ops {
> + WRITE_AND_AUTHENTICATE = 1,
> + WRITE_ONLY = 2,
> +};
> +
> /*
> * Hold NVM authentication failure status per switch This information
> * needs to stay around even when the switch gets power cycled so we
> @@ -155,8 +160,12 @@ static int nvm_validate_and_write(struct tb_switch *sw)
> }
>
> if (tb_switch_is_usb4(sw))
> - return usb4_switch_nvm_write(sw, 0, buf, image_size);
> - return dma_port_flash_write(sw->dma_port, 0, buf, image_size);
> + ret = usb4_switch_nvm_write(sw, 0, buf, image_size);
> + else
> + ret = dma_port_flash_write(sw->dma_port, 0, buf, image_size);
> + if (!ret)
> + sw->nvm->flushed = true;
> + return ret;
> }
>
> static int nvm_authenticate_host_dma_port(struct tb_switch *sw)
> @@ -1488,7 +1497,7 @@ static ssize_t nvm_authenticate_store(struct device *dev,
> struct device_attribute *attr, const char *buf, size_t count)
> {
> struct tb_switch *sw = tb_to_switch(dev);
> - bool val;
> + int val;
> int ret;
>
> pm_runtime_get_sync(&sw->dev);
> @@ -1504,25 +1513,28 @@ static ssize_t nvm_authenticate_store(struct device *dev,
> goto exit_unlock;
> }
>
> - ret = kstrtobool(buf, &val);
> + ret = kstrtoint(buf, 10, &val);
> if (ret)
> goto exit_unlock;
>
> /* Always clear the authentication status */
> nvm_clear_auth_status(sw);
>
> - if (val) {
> - if (!sw->nvm->buf) {
> - ret = -EINVAL;
> - goto exit_unlock;
> - }
> -
> - ret = nvm_validate_and_write(sw);
> - if (ret)
> - goto exit_unlock;
> + if (val > 0) {
> + if (!sw->nvm->flushed) {
> + if (!sw->nvm->buf) {
> + ret = -EINVAL;
> + goto exit_unlock;
> + }
>
> - sw->nvm->authenticating = true;
> - ret = nvm_authenticate(sw);
> + ret = nvm_validate_and_write(sw);
> + if (ret || val == WRITE_ONLY)
> + goto exit_unlock;
> + }
> + if (val == WRITE_AND_AUTHENTICATE) {
> + sw->nvm->authenticating = true;
> + ret = nvm_authenticate(sw);
> + }
> }
>
> exit_unlock:
> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> index 736d1589c31e..43a8ca2eb3d8 100644
> --- a/drivers/thunderbolt/tb.h
> +++ b/drivers/thunderbolt/tb.h
> @@ -42,6 +42,7 @@
> *
> * The user of this structure needs to handle serialization of possible
> * concurrent access.
> + * @flushed: The image has been flushed to the storage area
This should go below the @authenticating description.
> */
> struct tb_nvm {
> struct device *dev;
> @@ -53,6 +54,7 @@ struct tb_nvm {
> void *buf;
> size_t buf_data_size;
> bool authenticating;
> + bool flushed;
> };
>
> #define TB_SWITCH_KEY_SIZE 32
> --
> 2.25.1
> -----Original Message-----
> From: Mika Westerberg <[email protected]>
> Sent: Tuesday, June 23, 2020 9:33 AM
> To: Limonciello, Mario
> Cc: Andreas Noever; Michael Jamet; Yehezkel Bernat; [email protected];
> [email protected]
> Subject: Re: [PATCH v2 1/2] thunderbolt: Add support for separating the flush
> to SPI and authenticate
>
>
> [EXTERNAL EMAIL]
>
> On Mon, Jun 22, 2020 at 01:57:57PM -0500, Mario Limonciello wrote:
> > This allows userspace to have a shorter period of time that the device
> > is unusable and to call it at a more convenient time.
> >
> > For example flushing the image may happen while the user is using the
> > machine and authenticating/rebooting may happen while logging out.
> >
> > Signed-off-by: Mario Limonciello <[email protected]>
> > ---
> > .../ABI/testing/sysfs-bus-thunderbolt | 11 ++++-
> > drivers/thunderbolt/nvm.c | 1 +
> > drivers/thunderbolt/switch.c | 42 ++++++++++++-------
> > drivers/thunderbolt/tb.h | 2 +
> > 4 files changed, 39 insertions(+), 17 deletions(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-thunderbolt
> b/Documentation/ABI/testing/sysfs-bus-thunderbolt
> > index bd504ed323e8..7d0500b4d58a 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-thunderbolt
> > +++ b/Documentation/ABI/testing/sysfs-bus-thunderbolt
> > @@ -178,11 +178,18 @@ KernelVersion: 4.13
> > Contact: [email protected]
> > Description: When new NVM image is written to the non-active NVM
> > area (through non_activeX NVMem device), the
> > - authentication procedure is started by writing 1 to
> > - this file. If everything goes well, the device is
> > + authentication procedure is started by writing to
> > + this file.
> > + If everything goes well, the device is
> > restarted with the new NVM firmware. If the image
> > verification fails an error code is returned instead.
> >
> > + This file will accept writing values "1" or "2"
> > + - Writing "1" will flush the image to the storage
> > + area and authenticate the image in one action.
> > + - Writing "2" will run some basic validation on the image
> > + and flush it to the storage area.
> > +
> > When read holds status of the last authentication
> > operation if an error occurred during the process. This
> > is directly the status value from the DMA configuration
> > diff --git a/drivers/thunderbolt/nvm.c b/drivers/thunderbolt/nvm.c
> > index 4c6aa06ab3d5..29de6d95c6e7 100644
> > --- a/drivers/thunderbolt/nvm.c
> > +++ b/drivers/thunderbolt/nvm.c
> > @@ -100,6 +100,7 @@ int tb_nvm_write_buf(struct tb_nvm *nvm, unsigned int
> offset, void *val,
> > return -ENOMEM;
> > }
> >
> > + nvm->flushed = false;
>
> This means every write invalidates the "flushed" state, right?
Correct.
>
> > nvm->buf_data_size = offset + bytes;
> > memcpy(nvm->buf + offset, val, bytes);
> > return 0;
> > diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
> > index 817c66c7adcf..bbfbfebeee7f 100644
> > --- a/drivers/thunderbolt/switch.c
> > +++ b/drivers/thunderbolt/switch.c
> > @@ -26,6 +26,11 @@ struct nvm_auth_status {
> > u32 status;
> > };
> >
> > +enum nvm_write_ops {
> > + WRITE_AND_AUTHENTICATE = 1,
> > + WRITE_ONLY = 2,
> > +};
> > +
> > /*
> > * Hold NVM authentication failure status per switch This information
> > * needs to stay around even when the switch gets power cycled so we
> > @@ -155,8 +160,12 @@ static int nvm_validate_and_write(struct tb_switch *sw)
> > }
> >
> > if (tb_switch_is_usb4(sw))
> > - return usb4_switch_nvm_write(sw, 0, buf, image_size);
> > - return dma_port_flash_write(sw->dma_port, 0, buf, image_size);
> > + ret = usb4_switch_nvm_write(sw, 0, buf, image_size);
> > + else
> > + ret = dma_port_flash_write(sw->dma_port, 0, buf, image_size);
> > + if (!ret)
> > + sw->nvm->flushed = true;
> > + return ret;
> > }
> >
> > static int nvm_authenticate_host_dma_port(struct tb_switch *sw)
> > @@ -1488,7 +1497,7 @@ static ssize_t nvm_authenticate_store(struct device
> *dev,
> > struct device_attribute *attr, const char *buf, size_t count)
> > {
> > struct tb_switch *sw = tb_to_switch(dev);
> > - bool val;
> > + int val;
> > int ret;
> >
> > pm_runtime_get_sync(&sw->dev);
> > @@ -1504,25 +1513,28 @@ static ssize_t nvm_authenticate_store(struct device
> *dev,
> > goto exit_unlock;
> > }
> >
> > - ret = kstrtobool(buf, &val);
> > + ret = kstrtoint(buf, 10, &val);
> > if (ret)
> > goto exit_unlock;
> >
> > /* Always clear the authentication status */
> > nvm_clear_auth_status(sw);
> >
> > - if (val) {
> > - if (!sw->nvm->buf) {
> > - ret = -EINVAL;
> > - goto exit_unlock;
> > - }
> > -
> > - ret = nvm_validate_and_write(sw);
> > - if (ret)
> > - goto exit_unlock;
> > + if (val > 0) {
> > + if (!sw->nvm->flushed) {
> > + if (!sw->nvm->buf) {
> > + ret = -EINVAL;
> > + goto exit_unlock;
> > + }
> >
> > - sw->nvm->authenticating = true;
> > - ret = nvm_authenticate(sw);
> > + ret = nvm_validate_and_write(sw);
> > + if (ret || val == WRITE_ONLY)
> > + goto exit_unlock;
> > + }
> > + if (val == WRITE_AND_AUTHENTICATE) {
> > + sw->nvm->authenticating = true;
> > + ret = nvm_authenticate(sw);
> > + }
> > }
> >
> > exit_unlock:
> > diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> > index 736d1589c31e..43a8ca2eb3d8 100644
> > --- a/drivers/thunderbolt/tb.h
> > +++ b/drivers/thunderbolt/tb.h
> > @@ -42,6 +42,7 @@
> > *
> > * The user of this structure needs to handle serialization of possible
> > * concurrent access.
> > + * @flushed: The image has been flushed to the storage area
>
> This should go below the @authenticating description.
Oh thanks, I obviously didn't read the actual comments in the merge conflict
when I rebased. Will fix in a v3.
>
> > */
> > struct tb_nvm {
> > struct device *dev;
> > @@ -53,6 +54,7 @@ struct tb_nvm {
> > void *buf;
> > size_t buf_data_size;
> > bool authenticating;
> > + bool flushed;
> > };
> >
> > #define TB_SWITCH_KEY_SIZE 32
> > --
> > 2.25.1