[PATCH v2 0/2] Improve error handling in fastrpc_rpmsg_probe

[PATCH v2 0/2] Improve error handling in fastrpc_rpmsg_probe

[Anandu Krishnan E]

Following changes are getting added as part of this patch series:
 - Add proper exit path label to handle deregister of device node
   properly in error scenarios.
 - Add meaningful labels for exit paths in fastrpc_rpmsg_probe
   function to make it intuitive.
Patch [v1] : https://lore.kernel.org/all/20241220061854.24428-1-quic_anane@quicinc.com/

Changes in v2:
 - Added Fixes: tag and cc:stable.
 - Fixed author name.

Anandu Krishnan E (2):
  misc: fastrpc: Deregister device nodes properly in error scenarios
  misc: fastrpc: Add meaningful labels for exit paths

 drivers/misc/fastrpc.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

-- 
2.17.1

[PATCH v2 1/2] misc: fastrpc: Deregister device nodes properly in error scenarios

[Anandu Krishnan E]

During fastrpc_rpmsg_probe, if secure device node registration
succeeds but non-secure device node registration fails, the secure
device node deregister is not called during error cleanup. Add proper
exit paths to ensure proper cleanup in case of error.

Fixes: 3abe3ab3cdab ("misc: fastrpc: add secure domain support")
Cc: stable <stable@kernel.org>
Signed-off-by: Anandu Krishnan E <quic_anane@quicinc.com>
---
 drivers/misc/fastrpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 48d08eeb2d20..ff144f0aa337 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -2344,7 +2344,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 
 		err = fastrpc_device_register(rdev, data, false, domains[domain_id]);
 		if (err)
-			goto fdev_error;
+			goto populate_error;
 		break;
 	default:
 		err = -EINVAL;
-- 
2.17.1

[PATCH v2 2/2] misc: fastrpc: Add meaningful labels for exit paths

[Anandu Krishnan E]

In fastrpc_rpmsg_probe function, exit path labels are not intuitive.
It does not metion what the goto does or why the goto exists. Rename
goto labels to make it more intuitive and to align with labels of
other functions.

Signed-off-by: Anandu Krishnan E <quic_anane@quicinc.com>
---
 drivers/misc/fastrpc.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index ff144f0aa337..fe0e40a06b8a 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -2311,7 +2311,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 		rmem = of_reserved_mem_lookup(rmem_node);
 		if (!rmem) {
 			err = -EINVAL;
-			goto fdev_error;
+			goto err_free_data;
 		}
 
 		src_perms = BIT(QCOM_SCM_VMID_HLOS);
@@ -2332,7 +2332,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 		data->unsigned_support = false;
 		err = fastrpc_device_register(rdev, data, secure_dsp, domains[domain_id]);
 		if (err)
-			goto fdev_error;
+			goto err_free_data;
 		break;
 	case CDSP_DOMAIN_ID:
 	case CDSP1_DOMAIN_ID:
@@ -2340,15 +2340,15 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 		/* Create both device nodes so that we can allow both Signed and Unsigned PD */
 		err = fastrpc_device_register(rdev, data, true, domains[domain_id]);
 		if (err)
-			goto fdev_error;
+			goto err_free_data;
 
 		err = fastrpc_device_register(rdev, data, false, domains[domain_id]);
 		if (err)
-			goto populate_error;
+			goto err_deregister_fdev;
 		break;
 	default:
 		err = -EINVAL;
-		goto fdev_error;
+		goto err_free_data;
 	}
 
 	kref_init(&data->refcount);
@@ -2365,17 +2365,17 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 
 	err = of_platform_populate(rdev->of_node, NULL, NULL, rdev);
 	if (err)
-		goto populate_error;
+		goto err_deregister_fdev;
 
 	return 0;
 
-populate_error:
+err_deregister_fdev:
 	if (data->fdevice)
 		misc_deregister(&data->fdevice->miscdev);
 	if (data->secure_fdevice)
 		misc_deregister(&data->secure_fdevice->miscdev);
 
-fdev_error:
+err_free_data:
 	kfree(data);
 	return err;
 }
-- 
2.17.1

Re: [PATCH v2 1/2] misc: fastrpc: Deregister device nodes properly in error scenarios

[Greg KH]

On Mon, Dec 23, 2024 at 03:31:00PM +0530, Anandu Krishnan E wrote:
> During fastrpc_rpmsg_probe, if secure device node registration
> succeeds but non-secure device node registration fails, the secure
> device node deregister is not called during error cleanup. Add proper
> exit paths to ensure proper cleanup in case of error.
> 
> Fixes: 3abe3ab3cdab ("misc: fastrpc: add secure domain support")
> Cc: stable <stable@kernel.org>
> Signed-off-by: Anandu Krishnan E <quic_anane@quicinc.com>
> ---
>  drivers/misc/fastrpc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Shouldn't this be a stand-alone patch, not part of a series, if you wish
to have it included in 6.14-final?

thanks,

greg k-h

Re: [PATCH v2 1/2] misc: fastrpc: Deregister device nodes properly in error scenarios

[Dmitry Baryshkov]

On Mon, Dec 23, 2024 at 03:31:00PM +0530, Anandu Krishnan E wrote:
> During fastrpc_rpmsg_probe, if secure device node registration
> succeeds but non-secure device node registration fails, the secure
> device node deregister is not called during error cleanup. Add proper
> exit paths to ensure proper cleanup in case of error.
> 
> Fixes: 3abe3ab3cdab ("misc: fastrpc: add secure domain support")
> Cc: stable <stable@kernel.org>
> Signed-off-by: Anandu Krishnan E <quic_anane@quicinc.com>
> ---
>  drivers/misc/fastrpc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

This triggers checkpatch warning. If it is due to c&p from some internal
documentation, please fix it too.

> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 48d08eeb2d20..ff144f0aa337 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -2344,7 +2344,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  
>  		err = fastrpc_device_register(rdev, data, false, domains[domain_id]);
>  		if (err)
> -			goto fdev_error;
> +			goto populate_error;
>  		break;
>  	default:
>  		err = -EINVAL;
> -- 
> 2.17.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/2] misc: fastrpc: Add meaningful labels for exit paths

[Dmitry Baryshkov]

On Mon, Dec 23, 2024 at 03:31:01PM +0530, Anandu Krishnan E wrote:
> In fastrpc_rpmsg_probe function, exit path labels are not intuitive.
> It does not metion what the goto does or why the goto exists. Rename
> goto labels to make it more intuitive and to align with labels of
> other functions.
> 
> Signed-off-by: Anandu Krishnan E <quic_anane@quicinc.com>
> ---
>  drivers/misc/fastrpc.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH v2 1/2] misc: fastrpc: Deregister device nodes properly in error scenarios

[Anandu Krishnan E]

On 12/23/2024 3:40 PM, Greg KH wrote:
> On Mon, Dec 23, 2024 at 03:31:00PM +0530, Anandu Krishnan E wrote:
>> During fastrpc_rpmsg_probe, if secure device node registration
>> succeeds but non-secure device node registration fails, the secure
>> device node deregister is not called during error cleanup. Add proper
>> exit paths to ensure proper cleanup in case of error.
>>
>> Fixes: 3abe3ab3cdab ("misc: fastrpc: add secure domain support")
>> Cc: stable <stable@kernel.org>
>> Signed-off-by: Anandu Krishnan E <quic_anane@quicinc.com>
>> ---
>>  drivers/misc/fastrpc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Shouldn't this be a stand-alone patch, not part of a series, if you wish
> to have it included in 6.14-final?
> 
> thanks,
> 
> greg k-h

Sure, I will send this change as a stand-alone patch in the next spin,
so that it can be included in 6.14-final.

Thanks 
Anandu

Re: [PATCH v2 1/2] misc: fastrpc: Deregister device nodes properly in error scenarios

[Anandu Krishnan E]

On 12/23/2024 3:49 PM, Dmitry Baryshkov wrote:
> On Mon, Dec 23, 2024 at 03:31:00PM +0530, Anandu Krishnan E wrote:
>> During fastrpc_rpmsg_probe, if secure device node registration
>> succeeds but non-secure device node registration fails, the secure
>> device node deregister is not called during error cleanup. Add proper
>> exit paths to ensure proper cleanup in case of error.
>>
>> Fixes: 3abe3ab3cdab ("misc: fastrpc: add secure domain support")
>> Cc: stable <stable@kernel.org>
>> Signed-off-by: Anandu Krishnan E <quic_anane@quicinc.com>
>> ---
>>  drivers/misc/fastrpc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> This triggers checkpatch warning. If it is due to c&p from some internal
> documentation, please fix it too.
Understood. I will fix the checkpatch warning in the next spin.

-Anandu
> 
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index 48d08eeb2d20..ff144f0aa337 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -2344,7 +2344,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>  
>>  		err = fastrpc_device_register(rdev, data, false, domains[domain_id]);
>>  		if (err)
>> -			goto fdev_error;
>> +			goto populate_error;
>>  		break;
>>  	default:
>>  		err = -EINVAL;
>> -- 
>> 2.17.1
>>
>