[PATCH v3] fpga: stratix10-soc: Fix SVC mailbox handling during reconfiguration

[PATCH v3] fpga: stratix10-soc: Fix SVC mailbox handling during reconfiguration

[tze.yee.ng]

From: Tien Sung Ang <tien.sung.ang@altera.com>

Fix incorrect stratix10_svc_done() usage during FPGA reconfiguration.

Do not call stratix10_svc_done() at the end of write_init() on success, so
the SVC session remains active through write() and write_complete(). Call
stratix10_svc_done() on failure in write_init() and write() so the shared
SVC mailbox is released when reconfiguration aborts, allowing coexistence
with other SVC clients such as soc64-hwmon.

Fixes: e7eef1d7633a ("fpga: add intel stratix10 soc fpga manager driver")
Cc: stable@vger.kernel.org # 5.1+
Signed-off-by: Tien Sung Ang <tien.sung.ang@altera.com>
Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
---
Changes in v3:
- Remove blank line before Signed-off-by.
- No code changes from v2.

Changes in v2:
- Add Fixes: e7eef1d7633a per maintainer review; the incorrect
  stratix10_svc_done() call in write_init() dates to the initial driver.
- Add Cc: stable@vger.kernel.org # 5.1+ for stable backport consideration.
- No code changes from v1.
---
 drivers/fpga/stratix10-soc.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
index 0a295ccf1644..0d07d303bad1 100644
--- a/drivers/fpga/stratix10-soc.c
+++ b/drivers/fpga/stratix10-soc.c
@@ -195,20 +195,20 @@ static int s10_ops_write_init(struct fpga_manager *mgr,
 	ret = s10_svc_send_msg(priv, COMMAND_RECONFIG,
 			       &ctype, sizeof(ctype));
 	if (ret < 0)
-		goto init_done;
+		goto init_error;
 
 	ret = wait_for_completion_timeout(
 		&priv->status_return_completion, S10_RECONFIG_TIMEOUT);
 	if (!ret) {
 		dev_err(dev, "timeout waiting for RECONFIG_REQUEST\n");
 		ret = -ETIMEDOUT;
-		goto init_done;
+		goto init_error;
 	}
 
 	ret = 0;
 	if (!test_and_clear_bit(SVC_STATUS_OK, &priv->status)) {
 		ret = -ETIMEDOUT;
-		goto init_done;
+		goto init_error;
 	}
 
 	/* Allocate buffers from the service layer's pool. */
@@ -216,16 +216,19 @@ static int s10_ops_write_init(struct fpga_manager *mgr,
 		kbuf = stratix10_svc_allocate_memory(priv->chan, SVC_BUF_SIZE);
 		if (IS_ERR(kbuf)) {
 			s10_free_buffers(mgr);
-			ret = PTR_ERR(kbuf);
-			goto init_done;
+			ret = -ENOMEM;
+			goto init_error;
 		}
 
 		priv->svc_bufs[i].buf = kbuf;
 		priv->svc_bufs[i].lock = 0;
 	}
 
-init_done:
+	goto init_done;
+
+init_error:
 	stratix10_svc_done(priv->chan);
+init_done:
 	return ret;
 }
 
@@ -342,6 +345,9 @@ static int s10_ops_write(struct fpga_manager *mgr, const char *buf,
 	if (!s10_free_buffers(mgr))
 		dev_err(dev, "%s not all buffers were freed\n", __func__);
 
+	if (ret < 0)
+		stratix10_svc_done(priv->chan);
+
 	return ret;
 }
 
-- 
2.43.7

Re: [PATCH v3] fpga: stratix10-soc: Fix SVC mailbox handling during reconfiguration

[Xu Yilun]

> @@ -195,20 +195,20 @@ static int s10_ops_write_init(struct fpga_manager *mgr,
>  	ret = s10_svc_send_msg(priv, COMMAND_RECONFIG,
>  			       &ctype, sizeof(ctype));
>  	if (ret < 0)
> -		goto init_done;
> +		goto init_error;
>  
>  	ret = wait_for_completion_timeout(
>  		&priv->status_return_completion, S10_RECONFIG_TIMEOUT);
>  	if (!ret) {
>  		dev_err(dev, "timeout waiting for RECONFIG_REQUEST\n");
>  		ret = -ETIMEDOUT;
> -		goto init_done;
> +		goto init_error;
>  	}
>  
>  	ret = 0;

Why re-initialize ret here?

>  	if (!test_and_clear_bit(SVC_STATUS_OK, &priv->status)) {
>  		ret = -ETIMEDOUT;
> -		goto init_done;
> +		goto init_error;
>  	}
>  
>  	/* Allocate buffers from the service layer's pool. */
> @@ -216,16 +216,19 @@ static int s10_ops_write_init(struct fpga_manager *mgr,
>  		kbuf = stratix10_svc_allocate_memory(priv->chan, SVC_BUF_SIZE);
>  		if (IS_ERR(kbuf)) {
>  			s10_free_buffers(mgr);
> -			ret = PTR_ERR(kbuf);
> -			goto init_done;
> +			ret = -ENOMEM;

Why change the ret?

> +			goto init_error;
>  		}
>  
>  		priv->svc_bufs[i].buf = kbuf;
>  		priv->svc_bufs[i].lock = 0;
>  	}
>  
> -init_done:
> +	goto init_done;

Happy path? why not just return 0; And you don't need init_done at all.

> +
> +init_error:
>  	stratix10_svc_done(priv->chan);
> +init_done:
>  	return ret;
>  }

Re: [PATCH v3] fpga: stratix10-soc: Fix SVC mailbox handling during reconfiguration

[NG, TZE YEE]

On 27/6/2026 1:11 am, Xu Yilun wrote:
> [You don't often get email from yilun.xu@linux.intel.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
>> @@ -195,20 +195,20 @@ static int s10_ops_write_init(struct fpga_manager *mgr,
>>        ret = s10_svc_send_msg(priv, COMMAND_RECONFIG,
>>                               &ctype, sizeof(ctype));
>>        if (ret < 0)
>> -             goto init_done;
>> +             goto init_error;
>>
>>        ret = wait_for_completion_timeout(
>>                &priv->status_return_completion, S10_RECONFIG_TIMEOUT);
>>        if (!ret) {
>>                dev_err(dev, "timeout waiting for RECONFIG_REQUEST\n");
>>                ret = -ETIMEDOUT;
>> -             goto init_done;
>> +             goto init_error;
>>        }
>>
>>        ret = 0;
> 
> Why re-initialize ret here?
> 

That was clearing the remaining jiffies left in ret by 
wait_for_completion_timeout(). I'll drop it by not assigning the wait 
result to ret.

>>        if (!test_and_clear_bit(SVC_STATUS_OK, &priv->status)) {
>>                ret = -ETIMEDOUT;
>> -             goto init_done;
>> +             goto init_error;
>>        }
>>
>>        /* Allocate buffers from the service layer's pool. */
>> @@ -216,16 +216,19 @@ static int s10_ops_write_init(struct fpga_manager *mgr,
>>                kbuf = stratix10_svc_allocate_memory(priv->chan, SVC_BUF_SIZE);
>>                if (IS_ERR(kbuf)) {
>>                        s10_free_buffers(mgr);
>> -                     ret = PTR_ERR(kbuf);
>> -                     goto init_done;
>> +                     ret = -ENOMEM;
> 
> Why change the ret?
> 

That was unrelated churn.  I'll revert to PTR_ERR(kbuf).

>> +                     goto init_error;
>>                }
>>
>>                priv->svc_bufs[i].buf = kbuf;
>>                priv->svc_bufs[i].lock = 0;
>>        }
>>
>> -init_done:
>> +     goto init_done;
> 
> Happy path? why not just return 0; And you don't need init_done at all.
> 

Agreed. I'll use return 0 on success and keep only init_error for 
stratix10_svc_done() + return ret.

>> +
>> +init_error:
>>        stratix10_svc_done(priv->chan);
>> +init_done:
>>        return ret;
>>   }