Re: [PATCH v3 2/4] coresight: ultrasoc-smb: Config SMB buffer before register sink

[James Clark]

On 14/11/2023 13:33, Junhao He wrote:
> The SMB dirver register the enable/disable sysfs interface in function
> smb_register_sink(), however the buffer depends on the following
> configuration to work well. So it'll be possible for user to access an
> unreset one.
> 
> Move the config buffer operation to before register_sink().
> Ignore the return value, if smb_config_inport() fails. That will
> cause the hardwares disable trace path to fail, should not affect
> SMB driver remove. So we make smb_remove() return success,
> 
> Fixes: 06f5c2926aaa ("drivers/coresight: Add UltraSoc System Memory Buffer driver")
> Signed-off-by: Junhao He <hejunhao3@xxxxxxxxxx>
> ---
>  drivers/hwtracing/coresight/ultrasoc-smb.c | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
> index 0a0fe9fcc57f..2f2aba90a514 100644
> --- a/drivers/hwtracing/coresight/ultrasoc-smb.c
> +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
> @@ -583,37 +583,32 @@ static int smb_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	ret = smb_config_inport(dev, true);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, drvdata);
>  	spin_lock_init(&drvdata->spinlock);
>  	drvdata->pid = -1;
>  
>  	ret = smb_register_sink(pdev, drvdata);
>  	if (ret) {
> +		smb_config_inport(&pdev->dev, false);
>  		dev_err(dev, "Failed to register SMB sink\n");
>  		return ret;
>  	}
>  
> -	ret = smb_config_inport(dev, true);
> -	if (ret) {
> -		smb_unregister_sink(drvdata);
> -		return ret;
> -	}
> -
> -	platform_set_drvdata(pdev, drvdata);
> -
>  	return 0;
>  }
>  
>  static int smb_remove(struct platform_device *pdev)
>  {
>  	struct smb_drv_data *drvdata = platform_get_drvdata(pdev);
> -	int ret;
> -
> -	ret = smb_config_inport(&pdev->dev, false);
> -	if (ret)
> -		return ret;
>  
>  	smb_unregister_sink(drvdata);
>  
> +	smb_config_inport(&pdev->dev, false);
> +
>  	return 0;
>  }
>  

I'm not sure if it makes sense to implement Uwe's change to use
.remove_new instead here? And then drop the change on the other
patchset. Otherwise Suzuki will have to resolve the conflict. Maybe it's
quite simple so it's not an issue.

Either way:

Reviewed-by: James Clark <james.clark@xxxxxxx>