Re: [PATCH v5] usb: dwc3: debugfs: Prevent any register access when devices

[Johan Hovold <johan@kernel.org>]

On Tue, Apr 18, 2023 at 05:48:35PM +0530, Udipto Goswami wrote:

First, the subject of this patch looks wrong.

> When the dwc3 device is runtime suspended, various required clocks would
> get disabled and it is not guaranteed that access to any registers would
> work. Depending on the SoC glue, a register read could be as benign as
> returning 0 or be fatal enough to hang the system.
> 
> In order to prevent such scenarios of fatal errors, make sure to resume
> dwc3 then allow the function to proceed.
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

I did not sign-off on this patch (and neither did Oliver I presume).

You must never add a SoB on behalf on someone else without their
permission. Please read Documentation/process/submitting-patches.rst and
make sure you understand what SoB means before sending any further
patches.

> Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com>
> ---
> v5: Reworked the patch to resume dwc3 while accessing the registers.

Please include the full changelog for v6 (and future patches).

>  drivers/usb/dwc3/debugfs.c | 123 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 123 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
> index e4a2560b9dc0..d622b0dfeb76 100644
> --- a/drivers/usb/dwc3/debugfs.c
> +++ b/drivers/usb/dwc3/debugfs.c
> @@ -332,6 +332,13 @@ static int dwc3_lsp_show(struct seq_file *s, void *unused)
>  	unsigned int		current_mode;
>  	unsigned long		flags;
>  	u32			reg;
> +	int			ret;
> +
> +	ret = pm_runtime_get_sync(dwc->dev);
> +	if (!ret || ret < 0) {

This is broken as you would not print anything on success (ret == 0).

Did you test this patch at all?

> +		pm_runtime_put(dwc->dev);
> +		return 0;

Why do you return 'success' on errors?

> +	}

Johan