drivers/firmware/microchip/mpfs-auto-update.c | 42 ++++--------------- 1 file changed, 7 insertions(+), 35 deletions(-)
From: Conor Dooley <conor.dooley@microchip.com>
fw_upload's poll_complete() is really intended for use with
asynchronous write() implementations - or at least those where the
write() loop may terminate without the kernel yet being aware of whether
or not the firmware upload has succeeded. For auto-update, write() is
only ever called once and will only return when uploading has completed,
be that by passing or failing. The core fw_upload code only calls
poll_complete() after the final call to write() has returned.
However, the poll_complete() implementation in the auto-update driver
was written to expect poll_complete() to be called from another context,
and it waits for a completion signalled from write(). Since
poll_complete() is actually called from the same context, after the
write() loop has terminated, wait_for_completion() never sees the
completion get signalled and always times out, causing programming to
always report a failing.
Since write() is full synchronous, and its return value will indicate
whether or not programming passed or failed, poll_complete() serves no
purpose and can be cut down to simply return FW_UPLOAD_ERR_NONE.
Fixes: ec5b0f1193ad4 ("firmware: microchip: add PolarFire SoC Auto Update support")
Reported-by: Jamie Gibbons <jamie.gibbons@microchip.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
CC: Conor Dooley <conor.dooley@microchip.com>
CC: Daire McNamara <daire.mcnamara@microchip.com>
CC: linux-riscv@lists.infradead.org
CC: linux-kernel@vger.kernel.org
---
drivers/firmware/microchip/mpfs-auto-update.c | 42 ++++---------------
1 file changed, 7 insertions(+), 35 deletions(-)
diff --git a/drivers/firmware/microchip/mpfs-auto-update.c b/drivers/firmware/microchip/mpfs-auto-update.c
index 9ca5ee58edbdf..0f7ec88482022 100644
--- a/drivers/firmware/microchip/mpfs-auto-update.c
+++ b/drivers/firmware/microchip/mpfs-auto-update.c
@@ -76,14 +76,11 @@
#define AUTO_UPDATE_INFO_SIZE SZ_1M
#define AUTO_UPDATE_BITSTREAM_BASE (AUTO_UPDATE_DIRECTORY_SIZE + AUTO_UPDATE_INFO_SIZE)
-#define AUTO_UPDATE_TIMEOUT_MS 60000
-
struct mpfs_auto_update_priv {
struct mpfs_sys_controller *sys_controller;
struct device *dev;
struct mtd_info *flash;
struct fw_upload *fw_uploader;
- struct completion programming_complete;
size_t size_per_bitstream;
bool cancel_request;
};
@@ -156,19 +153,6 @@ static void mpfs_auto_update_cancel(struct fw_upload *fw_uploader)
static enum fw_upload_err mpfs_auto_update_poll_complete(struct fw_upload *fw_uploader)
{
- struct mpfs_auto_update_priv *priv = fw_uploader->dd_handle;
- int ret;
-
- /*
- * There is no meaningful way to get the status of the programming while
- * it is in progress, so attempting anything other than waiting for it
- * to complete would be misplaced.
- */
- ret = wait_for_completion_timeout(&priv->programming_complete,
- msecs_to_jiffies(AUTO_UPDATE_TIMEOUT_MS));
- if (!ret)
- return FW_UPLOAD_ERR_TIMEOUT;
-
return FW_UPLOAD_ERR_NONE;
}
@@ -349,33 +333,23 @@ static enum fw_upload_err mpfs_auto_update_write(struct fw_upload *fw_uploader,
u32 offset, u32 size, u32 *written)
{
struct mpfs_auto_update_priv *priv = fw_uploader->dd_handle;
- enum fw_upload_err err = FW_UPLOAD_ERR_NONE;
int ret;
- reinit_completion(&priv->programming_complete);
-
ret = mpfs_auto_update_write_bitstream(fw_uploader, data, offset, size, written);
- if (ret) {
- err = FW_UPLOAD_ERR_RW_ERROR;
- goto out;
- }
+ if (ret)
+ return FW_UPLOAD_ERR_RW_ERROR;
- if (priv->cancel_request) {
- err = FW_UPLOAD_ERR_CANCELED;
- goto out;
- }
+ if (priv->cancel_request)
+ return FW_UPLOAD_ERR_CANCELED;
if (mpfs_auto_update_is_bitstream_info(data, size))
- goto out;
+ return FW_UPLOAD_ERR_NONE;
ret = mpfs_auto_update_verify_image(fw_uploader);
if (ret)
- err = FW_UPLOAD_ERR_FW_INVALID;
+ return FW_UPLOAD_ERR_FW_INVALID;
-out:
- complete(&priv->programming_complete);
-
- return err;
+ return FW_UPLOAD_ERR_NONE;
}
static const struct fw_upload_ops mpfs_auto_update_ops = {
@@ -461,8 +435,6 @@ static int mpfs_auto_update_probe(struct platform_device *pdev)
return dev_err_probe(dev, ret,
"The current bitstream does not support auto-update\n");
- init_completion(&priv->programming_complete);
-
fw_uploader = firmware_upload_register(THIS_MODULE, dev, "mpfs-auto-update",
&mpfs_auto_update_ops, priv);
if (IS_ERR(fw_uploader))
--
2.45.2From: Conor Dooley <conor.dooley@microchip.com>
On Wed, 16 Oct 2024 17:35:06 +0100, Conor Dooley wrote:
> fw_upload's poll_complete() is really intended for use with
> asynchronous write() implementations - or at least those where the
> write() loop may terminate without the kernel yet being aware of whether
> or not the firmware upload has succeeded. For auto-update, write() is
> only ever called once and will only return when uploading has completed,
> be that by passing or failing. The core fw_upload code only calls
> poll_complete() after the final call to write() has returned.
>
> [...]
Applied to riscv-soc-fixes, thanks!
[1/1] firmware: microchip: auto-update: fix poll_complete() to not report spurious timeout errors
https://git.kernel.org/conor/c/83beece5aff7
Thanks,
Conor.
On Wed, 2024-10-16 at 17:35 +0100, Conor Dooley wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> From: Conor Dooley <conor.dooley@microchip.com>
>
> fw_upload's poll_complete() is really intended for use with
> asynchronous write() implementations - or at least those where the
> write() loop may terminate without the kernel yet being aware of
> whether
> or not the firmware upload has succeeded. For auto-update, write() is
> only ever called once and will only return when uploading has
> completed,
> be that by passing or failing. The core fw_upload code only calls
> poll_complete() after the final call to write() has returned.
>
> However, the poll_complete() implementation in the auto-update driver
> was written to expect poll_complete() to be called from another
> context,
> and it waits for a completion signalled from write(). Since
> poll_complete() is actually called from the same context, after the
> write() loop has terminated, wait_for_completion() never sees the
> completion get signalled and always times out, causing programming to
> always report a failing.
>
> Since write() is full synchronous, and its return value will indicate
> whether or not programming passed or failed, poll_complete() serves
> no
> purpose and can be cut down to simply return FW_UPLOAD_ERR_NONE.
>
> Fixes: ec5b0f1193ad4 ("firmware: microchip: add PolarFire SoC Auto
> Update support")
> Reported-by: Jamie Gibbons <jamie.gibbons@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
Tested-by: Jamie Gibbons <jamie.gibbons@microchip.com
> ---
> CC: Conor Dooley <conor.dooley@microchip.com>
> CC: Daire McNamara <daire.mcnamara@microchip.com>
> CC: linux-riscv@lists.infradead.org
> CC: linux-kernel@vger.kernel.org
> ---
> drivers/firmware/microchip/mpfs-auto-update.c | 42 ++++-------------
> --
> 1 file changed, 7 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/firmware/microchip/mpfs-auto-update.c
> b/drivers/firmware/microchip/mpfs-auto-update.c
> index 9ca5ee58edbdf..0f7ec88482022 100644
> --- a/drivers/firmware/microchip/mpfs-auto-update.c
> +++ b/drivers/firmware/microchip/mpfs-auto-update.c
> @@ -76,14 +76,11 @@
> #define AUTO_UPDATE_INFO_SIZE SZ_1M
> #define AUTO_UPDATE_BITSTREAM_BASE (AUTO_UPDATE_DIRECTORY_SIZE +
> AUTO_UPDATE_INFO_SIZE)
>
> -#define AUTO_UPDATE_TIMEOUT_MS 60000
> -
> struct mpfs_auto_update_priv {
> struct mpfs_sys_controller *sys_controller;
> struct device *dev;
> struct mtd_info *flash;
> struct fw_upload *fw_uploader;
> - struct completion programming_complete;
> size_t size_per_bitstream;
> bool cancel_request;
> };
> @@ -156,19 +153,6 @@ static void mpfs_auto_update_cancel(struct
> fw_upload *fw_uploader)
>
> static enum fw_upload_err mpfs_auto_update_poll_complete(struct
> fw_upload *fw_uploader)
> {
> - struct mpfs_auto_update_priv *priv = fw_uploader->dd_handle;
> - int ret;
> -
> - /*
> - * There is no meaningful way to get the status of the
> programming while
> - * it is in progress, so attempting anything other than
> waiting for it
> - * to complete would be misplaced.
> - */
> - ret = wait_for_completion_timeout(&priv-
> >programming_complete,
> -
> msecs_to_jiffies(AUTO_UPDATE_TIMEOUT_MS));
> - if (!ret)
> - return FW_UPLOAD_ERR_TIMEOUT;
> -
> return FW_UPLOAD_ERR_NONE;
> }
>
> @@ -349,33 +333,23 @@ static enum fw_upload_err
> mpfs_auto_update_write(struct fw_upload *fw_uploader,
> u32 offset, u32
> size, u32 *written)
> {
> struct mpfs_auto_update_priv *priv = fw_uploader->dd_handle;
> - enum fw_upload_err err = FW_UPLOAD_ERR_NONE;
> int ret;
>
> - reinit_completion(&priv->programming_complete);
> -
> ret = mpfs_auto_update_write_bitstream(fw_uploader, data,
> offset, size, written);
> - if (ret) {
> - err = FW_UPLOAD_ERR_RW_ERROR;
> - goto out;
> - }
> + if (ret)
> + return FW_UPLOAD_ERR_RW_ERROR;
>
> - if (priv->cancel_request) {
> - err = FW_UPLOAD_ERR_CANCELED;
> - goto out;
> - }
> + if (priv->cancel_request)
> + return FW_UPLOAD_ERR_CANCELED;
>
> if (mpfs_auto_update_is_bitstream_info(data, size))
> - goto out;
> + return FW_UPLOAD_ERR_NONE;
>
> ret = mpfs_auto_update_verify_image(fw_uploader);
> if (ret)
> - err = FW_UPLOAD_ERR_FW_INVALID;
> + return FW_UPLOAD_ERR_FW_INVALID;
>
> -out:
> - complete(&priv->programming_complete);
> -
> - return err;
> + return FW_UPLOAD_ERR_NONE;
> }
>
> static const struct fw_upload_ops mpfs_auto_update_ops = {
> @@ -461,8 +435,6 @@ static int mpfs_auto_update_probe(struct
> platform_device *pdev)
> return dev_err_probe(dev, ret,
> "The current bitstream does not
> support auto-update\n");
>
> - init_completion(&priv->programming_complete);
> -
> fw_uploader = firmware_upload_register(THIS_MODULE, dev,
> "mpfs-auto-update",
> &mpfs_auto_update_ops,
> priv);
> if (IS_ERR(fw_uploader))
> --
> 2.45.2
>
On Wed, Oct 16, 2024 at 05:35:06PM +0100, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
>
> fw_upload's poll_complete() is really intended for use with
> asynchronous write() implementations - or at least those where the
> write() loop may terminate without the kernel yet being aware of whether
> or not the firmware upload has succeeded. For auto-update, write() is
> only ever called once and will only return when uploading has completed,
> be that by passing or failing. The core fw_upload code only calls
> poll_complete() after the final call to write() has returned.
>
> However, the poll_complete() implementation in the auto-update driver
> was written to expect poll_complete() to be called from another context,
> and it waits for a completion signalled from write(). Since
> poll_complete() is actually called from the same context, after the
> write() loop has terminated, wait_for_completion() never sees the
> completion get signalled and always times out, causing programming to
> always report a failing.
>
> Since write() is full synchronous, and its return value will indicate
> whether or not programming passed or failed, poll_complete() serves no
> purpose and can be cut down to simply return FW_UPLOAD_ERR_NONE.
>
> Fixes: ec5b0f1193ad4 ("firmware: microchip: add PolarFire SoC Auto Update support")
> Reported-by: Jamie Gibbons <jamie.gibbons@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
note to self
CC: stable@vger.kernel.org
© 2016 - 2024 Red Hat, Inc.