diff mbox series

[v2,4/7] fpga: sec-mgr: expose sec-mgr update errors

Message ID 20201002223701.1317-5-russell.h.weight@intel.com (mailing list archive)
State Superseded, archived
Headers show
Series Intel FPGA Security Manager Class Driver | expand

Commit Message

Russ Weight Oct. 2, 2020, 10:36 p.m. UTC
Extend Intel Security Manager class driver to include
an update/error sysfs node that can be read for error
information when a secure update fails.

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
v2:
  - Bumped documentation date and version
  - Added warning to sec_progress() for invalid progress status
  - Added sec_error() function (similar to sec_progress())
---
 .../ABI/testing/sysfs-class-ifpga-sec-mgr     | 17 ++++
 drivers/fpga/ifpga-sec-mgr.c                  | 81 ++++++++++++++++---
 include/linux/fpga/ifpga-sec-mgr.h            |  1 +
 3 files changed, 89 insertions(+), 10 deletions(-)

Comments

Tom Rix Oct. 4, 2020, 9:06 p.m. UTC | #1
On 10/2/20 3:36 PM, Russ Weight wrote:
> Extend Intel Security Manager class driver to include
> an update/error sysfs node that can be read for error
> information when a secure update fails.
>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> ---
> v2:
>   - Bumped documentation date and version
>   - Added warning to sec_progress() for invalid progress status
>   - Added sec_error() function (similar to sec_progress())
> ---
>  .../ABI/testing/sysfs-class-ifpga-sec-mgr     | 17 ++++
>  drivers/fpga/ifpga-sec-mgr.c                  | 81 ++++++++++++++++---
>  include/linux/fpga/ifpga-sec-mgr.h            |  1 +
>  3 files changed, 89 insertions(+), 10 deletions(-)

This is 08/12 of the old patchset.

Also fine.

Reviewed-by: Tom Rix <trix@redhat.com>
Wu, Hao Oct. 5, 2020, 8:55 a.m. UTC | #2
> Subject: [PATCH v2 4/7] fpga: sec-mgr: expose sec-mgr update errors
> 
> Extend Intel Security Manager class driver to include
> an update/error sysfs node that can be read for error
> information when a secure update fails.
> 
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> ---
> v2:
>   - Bumped documentation date and version
>   - Added warning to sec_progress() for invalid progress status
>   - Added sec_error() function (similar to sec_progress())
> ---
>  .../ABI/testing/sysfs-class-ifpga-sec-mgr     | 17 ++++
>  drivers/fpga/ifpga-sec-mgr.c                  | 81 ++++++++++++++++---
>  include/linux/fpga/ifpga-sec-mgr.h            |  1 +
>  3 files changed, 89 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> index 73a5246fea1b..1f9f2c215e0c 100644
> --- a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> +++ b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> @@ -89,3 +89,20 @@ Description:	Read-only. Returns a string describing
> the current
>  		programming. Userspace code can poll on this file,
>  		as it will be signaled by sysfs_notify() on each
>  		state change.
> +
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/update/error
> +Date:		Oct 2020
> +KernelVersion:  5.11
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read-only. Returns a string describing the failure
> +		of a secure update. This string will be in the form
> +		of <STATUS>:<ERROR>, where <STATUS> will be one of
> +		the status strings described for the status sysfs
> +		file and <ERROR> will be one of the following:
> +		hw-error, timeout, user-abort, device-busy,
> +		invalid-file-size, read-write-error, flash-wearout,
> +		file-read-error.  The error sysfs file is only
> +		meaningful when the secure update engine is in the
> +		idle state. If this file is read while a secure
> +		update is in progress, then the read will fail with
> +		EBUSY.
> diff --git a/drivers/fpga/ifpga-sec-mgr.c b/drivers/fpga/ifpga-sec-mgr.c
> index ad918fb42dc2..456ea0b71e3d 100644
> --- a/drivers/fpga/ifpga-sec-mgr.c
> +++ b/drivers/fpga/ifpga-sec-mgr.c
> @@ -146,10 +146,16 @@ static void update_progress(struct ifpga_sec_mgr
> *imgr,
>  	sysfs_notify(&imgr->dev.kobj, "update", "status");
>  }
> 
> +static void set_error(struct ifpga_sec_mgr *imgr, enum ifpga_sec_err
> err_code)
> +{
> +	imgr->err_state = imgr->progress;
> +	imgr->err_code = err_code;
> +}
> +
>  static void ifpga_sec_dev_error(struct ifpga_sec_mgr *imgr,
>  				enum ifpga_sec_err err_code)
>  {
> -	imgr->err_code = err_code;
> +	set_error(imgr, err_code);
>  	imgr->iops->cancel(imgr);
>  }
> 
> @@ -172,7 +178,7 @@ static void ifpga_sec_mgr_update(struct work_struct
> *work)
> 
>  	get_device(&imgr->dev);
>  	if (request_firmware(&fw, imgr->filename, &imgr->dev)) {
> -		imgr->err_code = IFPGA_SEC_ERR_FILE_READ;
> +		set_error(imgr, IFPGA_SEC_ERR_FILE_READ);
>  		goto idle_exit;
>  	}
> 
> @@ -180,7 +186,7 @@ static void ifpga_sec_mgr_update(struct work_struct
> *work)
>  	imgr->remaining_size = fw->size;
> 
>  	if (!try_module_get(imgr->dev.parent->driver->owner)) {
> -		imgr->err_code = IFPGA_SEC_ERR_BUSY;
> +		set_error(imgr, IFPGA_SEC_ERR_BUSY);
>  		goto release_fw_exit;
>  	}
> 
> @@ -274,22 +280,76 @@ static const char * const sec_mgr_prog_str[] = {
>  	"programming"		/* IFPGA_SEC_PROG_PROGRAMMING */
>  };
> 
> -static ssize_t
> -status_show(struct device *dev, struct device_attribute *attr, char *buf)
> +static const char * const sec_mgr_err_str[] = {
> +	"none",			/* IFPGA_SEC_ERR_NONE */
> +	"hw-error",		/* IFPGA_SEC_ERR_HW_ERROR */
> +	"timeout",		/* IFPGA_SEC_ERR_TIMEOUT */
> +	"user-abort",		/* IFPGA_SEC_ERR_CANCELED */
> +	"device-busy",		/* IFPGA_SEC_ERR_BUSY */
> +	"invalid-file-size",	/* IFPGA_SEC_ERR_INVALID_SIZE */
> +	"read-write-error",	/* IFPGA_SEC_ERR_RW_ERROR */
> +	"flash-wearout",	/* IFPGA_SEC_ERR_WEAROUT */
> +	"file-read-error"	/* IFPGA_SEC_ERR_FILE_READ */
> +};
> +
> +static const char *sec_progress(struct device *dev, enum ifpga_sec_prog
> prog)
>  {
> -	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
>  	const char *status = "unknown-status";
> 
> -	if (imgr->progress < IFPGA_SEC_PROG_MAX)
> -		status = sec_mgr_prog_str[imgr->progress];
> +	if (prog < IFPGA_SEC_PROG_MAX)
> +		status = sec_mgr_prog_str[prog];
>  	else
>  		dev_warn(dev, "Invalid status during secure update: %d\n",
> -			 imgr->progress);
> +			 prog);
> +
> +	return status;
> +}
> +
> +static const char *sec_error(struct device *dev, enum ifpga_sec_err
> err_code)
> +{
> +	const char *error = "unknown-error";
> +
> +	if (err_code < IFPGA_SEC_ERR_MAX)
> +		error = sec_mgr_err_str[err_code];
> +	else
> +		dev_warn(dev, "Invalid error code during secure
> update: %d\n",
> +			 err_code);
> +
> +	return error;
> +}
> +
> +static ssize_t
> +status_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
> 
> -	return sprintf(buf, "%s\n", status);
> +	return sprintf(buf, "%s\n", sec_progress(dev, imgr->progress));
>  }
>  static DEVICE_ATTR_RO(status);
> 
> +static ssize_t
> +error_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
> +	int ret;
> +
> +	mutex_lock(&imgr->lock);
> +
> +	if (imgr->progress != IFPGA_SEC_PROG_IDLE)
> +		ret = -EBUSY;
> +	else if (!imgr->err_code)
> +		ret = 0;
> +	else
> +		ret = sprintf(buf, "%s:%s\n",
> +			      sec_progress(dev, imgr->err_state),
> +			      sec_error(dev, imgr->err_code));

In scheduled work for update, there is no lock protection, would that be a 
real problem here? e.g. progress is changed, or err_state and err_code
are not matched.

Thanks
Hao

> +
> +	mutex_unlock(&imgr->lock);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_RO(error);
> +
>  static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
>  			      const char *buf, size_t count)
>  {
> @@ -325,6 +385,7 @@ static DEVICE_ATTR_WO(filename);
>  static struct attribute *sec_mgr_update_attrs[] = {
>  	&dev_attr_filename.attr,
>  	&dev_attr_status.attr,
> +	&dev_attr_error.attr,
>  	NULL,
>  };
> 
> diff --git a/include/linux/fpga/ifpga-sec-mgr.h b/include/linux/fpga/ifpga-
> sec-mgr.h
> index 27008abd8e75..246e3d452c59 100644
> --- a/include/linux/fpga/ifpga-sec-mgr.h
> +++ b/include/linux/fpga/ifpga-sec-mgr.h
> @@ -110,6 +110,7 @@ struct ifpga_sec_mgr {
>  	const u8 *data;			/* pointer to update data */
>  	u32 remaining_size;		/* size remaining to transfer */
>  	enum ifpga_sec_prog progress;
> +	enum ifpga_sec_prog err_state;	/* progress state at time of
> failure */
>  	enum ifpga_sec_err err_code;	/* security manager error code */
>  	bool driver_unload;
>  	void *priv;
> --
> 2.17.1
Russ Weight Oct. 6, 2020, 8 p.m. UTC | #3
On 10/5/20 1:55 AM, Wu, Hao wrote:
>> Subject: [PATCH v2 4/7] fpga: sec-mgr: expose sec-mgr update errors
>>
>> Extend Intel Security Manager class driver to include
>> an update/error sysfs node that can be read for error
>> information when a secure update fails.
>>
>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>> ---
>> v2:
>>   - Bumped documentation date and version
>>   - Added warning to sec_progress() for invalid progress status
>>   - Added sec_error() function (similar to sec_progress())
>> ---
>>  .../ABI/testing/sysfs-class-ifpga-sec-mgr     | 17 ++++
>>  drivers/fpga/ifpga-sec-mgr.c                  | 81 ++++++++++++++++---
>>  include/linux/fpga/ifpga-sec-mgr.h            |  1 +
>>  3 files changed, 89 insertions(+), 10 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>> b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>> index 73a5246fea1b..1f9f2c215e0c 100644
>> --- a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>> +++ b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>> @@ -89,3 +89,20 @@ Description:Read-only. Returns a string describing
>> the current
>>  programming. Userspace code can poll on this file,
>>  as it will be signaled by sysfs_notify() on each
>>  state change.
>> +
>> +What: /sys/class/ifpga_sec_mgr/ifpga_secX/update/error
>> +Date:Oct 2020
>> +KernelVersion:  5.11
>> +Contact:Russ Weight <russell.h.weight@intel.com>
>> +Description:Read-only. Returns a string describing the failure
>> +of a secure update. This string will be in the form
>> +of <STATUS>:<ERROR>, where <STATUS> will be one of
>> +the status strings described for the status sysfs
>> +file and <ERROR> will be one of the following:
>> +hw-error, timeout, user-abort, device-busy,
>> +invalid-file-size, read-write-error, flash-wearout,
>> +file-read-error.  The error sysfs file is only
>> +meaningful when the secure update engine is in the
>> +idle state. If this file is read while a secure
>> +update is in progress, then the read will fail with
>> +EBUSY.
>> diff --git a/drivers/fpga/ifpga-sec-mgr.c b/drivers/fpga/ifpga-sec-mgr.c
>> index ad918fb42dc2..456ea0b71e3d 100644
>> --- a/drivers/fpga/ifpga-sec-mgr.c
>> +++ b/drivers/fpga/ifpga-sec-mgr.c
>> @@ -146,10 +146,16 @@ static void update_progress(struct ifpga_sec_mgr
>> *imgr,
>>  sysfs_notify(&imgr->dev.kobj, "update", "status");
>>  }
>>
>> +static void set_error(struct ifpga_sec_mgr *imgr, enum ifpga_sec_err
>> err_code)
>> +{
>> +imgr->err_state = imgr->progress;
>> +imgr->err_code = err_code;
>> +}
>> +
>>  static void ifpga_sec_dev_error(struct ifpga_sec_mgr *imgr,
>>  enum ifpga_sec_err err_code)
>>  {
>> -imgr->err_code = err_code;
>> +set_error(imgr, err_code);
>>  imgr->iops->cancel(imgr);
>>  }
>>
>> @@ -172,7 +178,7 @@ static void ifpga_sec_mgr_update(struct work_struct
>> *work)
>>
>>  get_device(&imgr->dev);
>>  if (request_firmware(&fw, imgr->filename, &imgr->dev)) {
>> -imgr->err_code = IFPGA_SEC_ERR_FILE_READ;
>> +set_error(imgr, IFPGA_SEC_ERR_FILE_READ);
>>  goto idle_exit;
>>  }
>>
>> @@ -180,7 +186,7 @@ static void ifpga_sec_mgr_update(struct work_struct
>> *work)
>>  imgr->remaining_size = fw->size;
>>
>>  if (!try_module_get(imgr->dev.parent->driver->owner)) {
>> -imgr->err_code = IFPGA_SEC_ERR_BUSY;
>> +set_error(imgr, IFPGA_SEC_ERR_BUSY);
>>  goto release_fw_exit;
>>  }
>>
>> @@ -274,22 +280,76 @@ static const char * const sec_mgr_prog_str[] = {
>>  "programming"/* IFPGA_SEC_PROG_PROGRAMMING */
>>  };
>>
>> -static ssize_t
>> -status_show(struct device *dev, struct device_attribute *attr, char *buf)
>> +static const char * const sec_mgr_err_str[] = {
>> +"none",/* IFPGA_SEC_ERR_NONE */
>> +"hw-error",/* IFPGA_SEC_ERR_HW_ERROR */
>> +"timeout",/* IFPGA_SEC_ERR_TIMEOUT */
>> +"user-abort",/* IFPGA_SEC_ERR_CANCELED */
>> +"device-busy",/* IFPGA_SEC_ERR_BUSY */
>> +"invalid-file-size",/* IFPGA_SEC_ERR_INVALID_SIZE */
>> +"read-write-error",/* IFPGA_SEC_ERR_RW_ERROR */
>> +"flash-wearout",/* IFPGA_SEC_ERR_WEAROUT */
>> +"file-read-error"/* IFPGA_SEC_ERR_FILE_READ */
>> +};
>> +
>> +static const char *sec_progress(struct device *dev, enum ifpga_sec_prog
>> prog)
>>  {
>> -struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
>>  const char *status = "unknown-status";
>>
>> -if (imgr->progress < IFPGA_SEC_PROG_MAX)
>> -status = sec_mgr_prog_str[imgr->progress];
>> +if (prog < IFPGA_SEC_PROG_MAX)
>> +status = sec_mgr_prog_str[prog];
>>  else
>>  dev_warn(dev, "Invalid status during secure update: %d\n",
>> - imgr->progress);
>> + prog);
>> +
>> +return status;
>> +}
>> +
>> +static const char *sec_error(struct device *dev, enum ifpga_sec_err
>> err_code)
>> +{
>> +const char *error = "unknown-error";
>> +
>> +if (err_code < IFPGA_SEC_ERR_MAX)
>> +error = sec_mgr_err_str[err_code];
>> +else
>> +dev_warn(dev, "Invalid error code during secure
>> update: %d\n",
>> + err_code);
>> +
>> +return error;
>> +}
>> +
>> +static ssize_t
>> +status_show(struct device *dev, struct device_attribute *attr, char *buf)
>> +{
>> +struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
>>
>> -return sprintf(buf, "%s\n", status);
>> +return sprintf(buf, "%s\n", sec_progress(dev, imgr->progress));
>>  }
>>  static DEVICE_ATTR_RO(status);
>>
>> +static ssize_t
>> +error_show(struct device *dev, struct device_attribute *attr, char *buf)
>> +{
>> +struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
>> +int ret;
>> +
>> +mutex_lock(&imgr->lock);
>> +
>> +if (imgr->progress != IFPGA_SEC_PROG_IDLE)
>> +ret = -EBUSY;
>> +else if (!imgr->err_code)
>> +ret = 0;
>> +else
>> +ret = sprintf(buf, "%s:%s\n",
>> +      sec_progress(dev, imgr->err_state),
>> +      sec_error(dev, imgr->err_code));
> In scheduled work for update, there is no lock protection, would that be a
> real problem here? e.g. progress is changed, or err_state and err_code
> are not matched.
-EBUSY is returned unless we are in the IDLE state. Also, the lock is held
during this function, so new updates cannot begin. This means there is no
worker thread in progress and there is no concurrent execution context
that can change the err_state and err_code values. They would retain the
error state from the previous update.

Thanks for the comments!
- Russ
>
> Thanks
> Hao
>
>> +
>> +mutex_unlock(&imgr->lock);
>> +
>> +return ret;
>> +}
>> +static DEVICE_ATTR_RO(error);
>> +
>>  static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
>>        const char *buf, size_t count)
>>  {
>> @@ -325,6 +385,7 @@ static DEVICE_ATTR_WO(filename);
>>  static struct attribute *sec_mgr_update_attrs[] = {
>>  &dev_attr_filename.attr,
>>  &dev_attr_status.attr,
>> +&dev_attr_error.attr,
>>  NULL,
>>  };
>>
>> diff --git a/include/linux/fpga/ifpga-sec-mgr.h b/include/linux/fpga/ifpga-
>> sec-mgr.h
>> index 27008abd8e75..246e3d452c59 100644
>> --- a/include/linux/fpga/ifpga-sec-mgr.h
>> +++ b/include/linux/fpga/ifpga-sec-mgr.h
>> @@ -110,6 +110,7 @@ struct ifpga_sec_mgr {
>>  const u8 *data;/* pointer to update data */
>>  u32 remaining_size;/* size remaining to transfer */
>>  enum ifpga_sec_prog progress;
>> +enum ifpga_sec_prog err_state;/* progress state at time of
>> failure */
>>  enum ifpga_sec_err err_code;/* security manager error code */
>>  bool driver_unload;
>>  void *priv;
>> --
>> 2.17.1
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
index 73a5246fea1b..1f9f2c215e0c 100644
--- a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
+++ b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
@@ -89,3 +89,20 @@  Description:	Read-only. Returns a string describing the current
 		programming. Userspace code can poll on this file,
 		as it will be signaled by sysfs_notify() on each
 		state change.
+
+What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/update/error
+Date:		Oct 2020
+KernelVersion:  5.11
+Contact:	Russ Weight <russell.h.weight@intel.com>
+Description:	Read-only. Returns a string describing the failure
+		of a secure update. This string will be in the form
+		of <STATUS>:<ERROR>, where <STATUS> will be one of
+		the status strings described for the status sysfs
+		file and <ERROR> will be one of the following:
+		hw-error, timeout, user-abort, device-busy,
+		invalid-file-size, read-write-error, flash-wearout,
+		file-read-error.  The error sysfs file is only
+		meaningful when the secure update engine is in the
+		idle state. If this file is read while a secure
+		update is in progress, then the read will fail with
+		EBUSY.
diff --git a/drivers/fpga/ifpga-sec-mgr.c b/drivers/fpga/ifpga-sec-mgr.c
index ad918fb42dc2..456ea0b71e3d 100644
--- a/drivers/fpga/ifpga-sec-mgr.c
+++ b/drivers/fpga/ifpga-sec-mgr.c
@@ -146,10 +146,16 @@  static void update_progress(struct ifpga_sec_mgr *imgr,
 	sysfs_notify(&imgr->dev.kobj, "update", "status");
 }
 
+static void set_error(struct ifpga_sec_mgr *imgr, enum ifpga_sec_err err_code)
+{
+	imgr->err_state = imgr->progress;
+	imgr->err_code = err_code;
+}
+
 static void ifpga_sec_dev_error(struct ifpga_sec_mgr *imgr,
 				enum ifpga_sec_err err_code)
 {
-	imgr->err_code = err_code;
+	set_error(imgr, err_code);
 	imgr->iops->cancel(imgr);
 }
 
@@ -172,7 +178,7 @@  static void ifpga_sec_mgr_update(struct work_struct *work)
 
 	get_device(&imgr->dev);
 	if (request_firmware(&fw, imgr->filename, &imgr->dev)) {
-		imgr->err_code = IFPGA_SEC_ERR_FILE_READ;
+		set_error(imgr, IFPGA_SEC_ERR_FILE_READ);
 		goto idle_exit;
 	}
 
@@ -180,7 +186,7 @@  static void ifpga_sec_mgr_update(struct work_struct *work)
 	imgr->remaining_size = fw->size;
 
 	if (!try_module_get(imgr->dev.parent->driver->owner)) {
-		imgr->err_code = IFPGA_SEC_ERR_BUSY;
+		set_error(imgr, IFPGA_SEC_ERR_BUSY);
 		goto release_fw_exit;
 	}
 
@@ -274,22 +280,76 @@  static const char * const sec_mgr_prog_str[] = {
 	"programming"		/* IFPGA_SEC_PROG_PROGRAMMING */
 };
 
-static ssize_t
-status_show(struct device *dev, struct device_attribute *attr, char *buf)
+static const char * const sec_mgr_err_str[] = {
+	"none",			/* IFPGA_SEC_ERR_NONE */
+	"hw-error",		/* IFPGA_SEC_ERR_HW_ERROR */
+	"timeout",		/* IFPGA_SEC_ERR_TIMEOUT */
+	"user-abort",		/* IFPGA_SEC_ERR_CANCELED */
+	"device-busy",		/* IFPGA_SEC_ERR_BUSY */
+	"invalid-file-size",	/* IFPGA_SEC_ERR_INVALID_SIZE */
+	"read-write-error",	/* IFPGA_SEC_ERR_RW_ERROR */
+	"flash-wearout",	/* IFPGA_SEC_ERR_WEAROUT */
+	"file-read-error"	/* IFPGA_SEC_ERR_FILE_READ */
+};
+
+static const char *sec_progress(struct device *dev, enum ifpga_sec_prog prog)
 {
-	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
 	const char *status = "unknown-status";
 
-	if (imgr->progress < IFPGA_SEC_PROG_MAX)
-		status = sec_mgr_prog_str[imgr->progress];
+	if (prog < IFPGA_SEC_PROG_MAX)
+		status = sec_mgr_prog_str[prog];
 	else
 		dev_warn(dev, "Invalid status during secure update: %d\n",
-			 imgr->progress);
+			 prog);
+
+	return status;
+}
+
+static const char *sec_error(struct device *dev, enum ifpga_sec_err err_code)
+{
+	const char *error = "unknown-error";
+
+	if (err_code < IFPGA_SEC_ERR_MAX)
+		error = sec_mgr_err_str[err_code];
+	else
+		dev_warn(dev, "Invalid error code during secure update: %d\n",
+			 err_code);
+
+	return error;
+}
+
+static ssize_t
+status_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
 
-	return sprintf(buf, "%s\n", status);
+	return sprintf(buf, "%s\n", sec_progress(dev, imgr->progress));
 }
 static DEVICE_ATTR_RO(status);
 
+static ssize_t
+error_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
+	int ret;
+
+	mutex_lock(&imgr->lock);
+
+	if (imgr->progress != IFPGA_SEC_PROG_IDLE)
+		ret = -EBUSY;
+	else if (!imgr->err_code)
+		ret = 0;
+	else
+		ret = sprintf(buf, "%s:%s\n",
+			      sec_progress(dev, imgr->err_state),
+			      sec_error(dev, imgr->err_code));
+
+	mutex_unlock(&imgr->lock);
+
+	return ret;
+}
+static DEVICE_ATTR_RO(error);
+
 static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
 			      const char *buf, size_t count)
 {
@@ -325,6 +385,7 @@  static DEVICE_ATTR_WO(filename);
 static struct attribute *sec_mgr_update_attrs[] = {
 	&dev_attr_filename.attr,
 	&dev_attr_status.attr,
+	&dev_attr_error.attr,
 	NULL,
 };
 
diff --git a/include/linux/fpga/ifpga-sec-mgr.h b/include/linux/fpga/ifpga-sec-mgr.h
index 27008abd8e75..246e3d452c59 100644
--- a/include/linux/fpga/ifpga-sec-mgr.h
+++ b/include/linux/fpga/ifpga-sec-mgr.h
@@ -110,6 +110,7 @@  struct ifpga_sec_mgr {
 	const u8 *data;			/* pointer to update data */
 	u32 remaining_size;		/* size remaining to transfer */
 	enum ifpga_sec_prog progress;
+	enum ifpga_sec_prog err_state;	/* progress state at time of failure */
 	enum ifpga_sec_err err_code;	/* security manager error code */
 	bool driver_unload;
 	void *priv;