> -----Original Message-----
> From: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> Sent: Tuesday, June 23, 2020 9:33 AM
> To: Limonciello, Mario
> Cc: Andreas Noever; Michael Jamet; Yehezkel Bernat; linux-usb@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> 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 <mario.limonciello@xxxxxxxx>
> > ---
> > .../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: thunderbolt-software@xxxxxxxxxxxx
> > 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
