[PATCH v9] usb: dwc3: debugfs: Prevent any register access when devices is runtime suspended

[PATCH v9] usb: dwc3: debugfs: Prevent any register access when devices is runtime suspended

[Udipto Goswami]

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.

Fixes: 62ba09d6bb63 ("usb: dwc3: debugfs: Dump internal LSP and ep registers")
Cc: stable@vger.kernel.org
Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com>
---
v9: Fixed function dwc3_rx_fifo_size_show & return values in function
	dwc3_link_state_write along with minor changes for code symmetry.
v8: Replace pm_runtime_get_sync with pm_runtime_resume_and get.
v7: Replaced pm_runtime_put with pm_runtime_put_sync & returned proper values.
v6: Added changes to handle get_dync failure appropriately.
v5: Reworked the patch to resume dwc3 while accessing the registers.
v4: Introduced pm_runtime_get_if_in_use in order to make sure dwc3 isn't
	suspended while accessing the registers.
v3: Replace pr_err to dev_err. 
v2: Replaced return 0 with -EINVAL & seq_puts with pr_err.

 drivers/usb/dwc3/debugfs.c | 109 +++++++++++++++++++++++++++++++++++++
 1 file changed, 109 insertions(+)

diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
index e4a2560b9dc0..ebf03468fac4 100644
--- a/drivers/usb/dwc3/debugfs.c
+++ b/drivers/usb/dwc3/debugfs.c
@@ -332,6 +332,11 @@ 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_resume_and_get(dwc->dev);
+	if (ret < 0)
+		return ret;
 
 	spin_lock_irqsave(&dwc->lock, flags);
 	reg = dwc3_readl(dwc->regs, DWC3_GSTS);
@@ -350,6 +355,8 @@ static int dwc3_lsp_show(struct seq_file *s, void *unused)
 	}
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
+	pm_runtime_put_sync(dwc->dev);
+
 	return 0;
 }
 
@@ -395,6 +402,11 @@ static int dwc3_mode_show(struct seq_file *s, void *unused)
 	struct dwc3		*dwc = s->private;
 	unsigned long		flags;
 	u32			reg;
+	int			ret;
+
+	ret = pm_runtime_resume_and_get(dwc->dev);
+	if (ret < 0)
+		return ret;
 
 	spin_lock_irqsave(&dwc->lock, flags);
 	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
@@ -414,6 +426,8 @@ static int dwc3_mode_show(struct seq_file *s, void *unused)
 		seq_printf(s, "UNKNOWN %08x\n", DWC3_GCTL_PRTCAP(reg));
 	}
 
+	pm_runtime_put_sync(dwc->dev);
+
 	return 0;
 }
 
@@ -463,6 +477,11 @@ static int dwc3_testmode_show(struct seq_file *s, void *unused)
 	struct dwc3		*dwc = s->private;
 	unsigned long		flags;
 	u32			reg;
+	int			ret;
+
+	ret = pm_runtime_resume_and_get(dwc->dev);
+	if (ret < 0)
+		return ret;
 
 	spin_lock_irqsave(&dwc->lock, flags);
 	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
@@ -493,6 +512,8 @@ static int dwc3_testmode_show(struct seq_file *s, void *unused)
 		seq_printf(s, "UNKNOWN %d\n", reg);
 	}
 
+	pm_runtime_put_sync(dwc->dev);
+
 	return 0;
 }
 
@@ -509,6 +530,7 @@ static ssize_t dwc3_testmode_write(struct file *file,
 	unsigned long		flags;
 	u32			testmode = 0;
 	char			buf[32];
+	int			ret;
 
 	if (copy_from_user(&buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
 		return -EFAULT;
@@ -526,10 +548,16 @@ static ssize_t dwc3_testmode_write(struct file *file,
 	else
 		testmode = 0;
 
+	ret = pm_runtime_resume_and_get(dwc->dev);
+	if (ret < 0)
+		return ret;
+
 	spin_lock_irqsave(&dwc->lock, flags);
 	dwc3_gadget_set_test_mode(dwc, testmode);
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
+	pm_runtime_put_sync(dwc->dev);
+
 	return count;
 }
 
@@ -548,12 +576,18 @@ static int dwc3_link_state_show(struct seq_file *s, void *unused)
 	enum dwc3_link_state	state;
 	u32			reg;
 	u8			speed;
+	int			ret;
+
+	ret = pm_runtime_resume_and_get(dwc->dev);
+	if (ret < 0)
+		return ret;
 
 	spin_lock_irqsave(&dwc->lock, flags);
 	reg = dwc3_readl(dwc->regs, DWC3_GSTS);
 	if (DWC3_GSTS_CURMOD(reg) != DWC3_GSTS_CURMOD_DEVICE) {
 		seq_puts(s, "Not available\n");
 		spin_unlock_irqrestore(&dwc->lock, flags);
+		pm_runtime_put_sync(dwc->dev);
 		return 0;
 	}
 
@@ -566,6 +600,8 @@ static int dwc3_link_state_show(struct seq_file *s, void *unused)
 		   dwc3_gadget_hs_link_string(state));
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
+	pm_runtime_put_sync(dwc->dev);
+
 	return 0;
 }
 
@@ -584,6 +620,7 @@ static ssize_t dwc3_link_state_write(struct file *file,
 	char			buf[32];
 	u32			reg;
 	u8			speed;
+	int			ret;
 
 	if (copy_from_user(&buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
 		return -EFAULT;
@@ -603,10 +640,15 @@ static ssize_t dwc3_link_state_write(struct file *file,
 	else
 		return -EINVAL;
 
+	ret = pm_runtime_resume_and_get(dwc->dev);
+	if (ret < 0)
+		return ret;
+
 	spin_lock_irqsave(&dwc->lock, flags);
 	reg = dwc3_readl(dwc->regs, DWC3_GSTS);
 	if (DWC3_GSTS_CURMOD(reg) != DWC3_GSTS_CURMOD_DEVICE) {
 		spin_unlock_irqrestore(&dwc->lock, flags);
+		pm_runtime_put_sync(dwc->dev);
 		return -EINVAL;
 	}
 
@@ -616,12 +658,15 @@ static ssize_t dwc3_link_state_write(struct file *file,
 	if (speed < DWC3_DSTS_SUPERSPEED &&
 	    state != DWC3_LINK_STATE_RECOV) {
 		spin_unlock_irqrestore(&dwc->lock, flags);
+		pm_runtime_put_sync(dwc->dev);
 		return -EINVAL;
 	}
 
 	dwc3_gadget_set_link_state(dwc, state);
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
+	pm_runtime_put_sync(dwc->dev);
+
 	return count;
 }
 
@@ -645,6 +690,11 @@ static int dwc3_tx_fifo_size_show(struct seq_file *s, void *unused)
 	unsigned long		flags;
 	u32			mdwidth;
 	u32			val;
+	int			ret;
+
+	ret = pm_runtime_resume_and_get(dwc->dev);
+	if (ret < 0)
+		return ret;
 
 	spin_lock_irqsave(&dwc->lock, flags);
 	val = dwc3_core_fifo_space(dep, DWC3_TXFIFO);
@@ -657,6 +707,8 @@ static int dwc3_tx_fifo_size_show(struct seq_file *s, void *unused)
 	seq_printf(s, "%u\n", val);
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
+	pm_runtime_put_sync(dwc->dev);
+
 	return 0;
 }
 
@@ -667,6 +719,11 @@ static int dwc3_rx_fifo_size_show(struct seq_file *s, void *unused)
 	unsigned long		flags;
 	u32			mdwidth;
 	u32			val;
+	int			ret;
+
+	ret = pm_runtime_resume_and_get(dwc->dev);
+	if (ret < 0)
+		return ret;
 
 	spin_lock_irqsave(&dwc->lock, flags);
 	val = dwc3_core_fifo_space(dep, DWC3_RXFIFO);
@@ -679,6 +736,8 @@ static int dwc3_rx_fifo_size_show(struct seq_file *s, void *unused)
 	seq_printf(s, "%u\n", val);
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
+	pm_runtime_put_sync(dwc->dev);
+
 	return 0;
 }
 
@@ -688,12 +747,19 @@ static int dwc3_tx_request_queue_show(struct seq_file *s, void *unused)
 	struct dwc3		*dwc = dep->dwc;
 	unsigned long		flags;
 	u32			val;
+	int			ret;
+
+	ret = pm_runtime_resume_and_get(dwc->dev);
+	if (ret < 0)
+		return ret;
 
 	spin_lock_irqsave(&dwc->lock, flags);
 	val = dwc3_core_fifo_space(dep, DWC3_TXREQQ);
 	seq_printf(s, "%u\n", val);
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
+	pm_runtime_put_sync(dwc->dev);
+
 	return 0;
 }
 
@@ -703,12 +769,19 @@ static int dwc3_rx_request_queue_show(struct seq_file *s, void *unused)
 	struct dwc3		*dwc = dep->dwc;
 	unsigned long		flags;
 	u32			val;
+	int			ret;
+
+	ret = pm_runtime_resume_and_get(dwc->dev);
+	if (ret < 0)
+		return ret;
 
 	spin_lock_irqsave(&dwc->lock, flags);
 	val = dwc3_core_fifo_space(dep, DWC3_RXREQQ);
 	seq_printf(s, "%u\n", val);
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
+	pm_runtime_put_sync(dwc->dev);
+
 	return 0;
 }
 
@@ -718,12 +791,19 @@ static int dwc3_rx_info_queue_show(struct seq_file *s, void *unused)
 	struct dwc3		*dwc = dep->dwc;
 	unsigned long		flags;
 	u32			val;
+	int			ret;
+
+	ret = pm_runtime_resume_and_get(dwc->dev);
+	if (ret < 0)
+		return ret;
 
 	spin_lock_irqsave(&dwc->lock, flags);
 	val = dwc3_core_fifo_space(dep, DWC3_RXINFOQ);
 	seq_printf(s, "%u\n", val);
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
+	pm_runtime_put_sync(dwc->dev);
+
 	return 0;
 }
 
@@ -733,12 +813,19 @@ static int dwc3_descriptor_fetch_queue_show(struct seq_file *s, void *unused)
 	struct dwc3		*dwc = dep->dwc;
 	unsigned long		flags;
 	u32			val;
+	int			ret;
+
+	ret = pm_runtime_resume_and_get(dwc->dev);
+	if (ret < 0)
+		return ret;
 
 	spin_lock_irqsave(&dwc->lock, flags);
 	val = dwc3_core_fifo_space(dep, DWC3_DESCFETCHQ);
 	seq_printf(s, "%u\n", val);
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
+	pm_runtime_put_sync(dwc->dev);
+
 	return 0;
 }
 
@@ -748,12 +835,19 @@ static int dwc3_event_queue_show(struct seq_file *s, void *unused)
 	struct dwc3		*dwc = dep->dwc;
 	unsigned long		flags;
 	u32			val;
+	int			ret;
+
+	ret = pm_runtime_resume_and_get(dwc->dev);
+	if (ret < 0)
+		return ret;
 
 	spin_lock_irqsave(&dwc->lock, flags);
 	val = dwc3_core_fifo_space(dep, DWC3_EVENTQ);
 	seq_printf(s, "%u\n", val);
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
+	pm_runtime_put_sync(dwc->dev);
+
 	return 0;
 }
 
@@ -798,6 +892,11 @@ static int dwc3_trb_ring_show(struct seq_file *s, void *unused)
 	struct dwc3		*dwc = dep->dwc;
 	unsigned long		flags;
 	int			i;
+	int			ret;
+
+	ret = pm_runtime_resume_and_get(dwc->dev);
+	if (ret < 0)
+		return ret;
 
 	spin_lock_irqsave(&dwc->lock, flags);
 	if (dep->number <= 1) {
@@ -827,6 +926,8 @@ static int dwc3_trb_ring_show(struct seq_file *s, void *unused)
 out:
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
+	pm_runtime_put_sync(dwc->dev);
+
 	return 0;
 }
 
@@ -839,6 +940,11 @@ static int dwc3_ep_info_register_show(struct seq_file *s, void *unused)
 	u32			lower_32_bits;
 	u32			upper_32_bits;
 	u32			reg;
+	int			ret;
+
+	ret = pm_runtime_resume_and_get(dwc->dev);
+	if (ret < 0)
+		return ret;
 
 	spin_lock_irqsave(&dwc->lock, flags);
 	reg = DWC3_GDBGLSPMUX_EPSELECT(dep->number);
@@ -851,6 +957,8 @@ static int dwc3_ep_info_register_show(struct seq_file *s, void *unused)
 	seq_printf(s, "0x%016llx\n", ep_info);
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
+	pm_runtime_put_sync(dwc->dev);
+
 	return 0;
 }
 
@@ -910,6 +1018,7 @@ void dwc3_debugfs_init(struct dwc3 *dwc)
 	dwc->regset->regs = dwc3_regs;
 	dwc->regset->nregs = ARRAY_SIZE(dwc3_regs);
 	dwc->regset->base = dwc->regs - DWC3_GLOBALS_REGS_START;
+	dwc->regset->dev = dwc->dev;
 
 	root = debugfs_create_dir(dev_name(dwc->dev), usb_debug_root);
 	dwc->debug_root = root;
-- 
2.17.1

Re: [PATCH v9] usb: dwc3: debugfs: Prevent any register access when devices is runtime suspended

[Thinh Nguyen]

On Fri, May 05, 2023, Udipto Goswami wrote:
> 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.
> 
> Fixes: 62ba09d6bb63 ("usb: dwc3: debugfs: Dump internal LSP and ep registers")

This fix goes before the above change.

This also touches on many places and is more than 100 lines. While this
is a fix, I'm not sure if Cc stable is needed. Perhaps others can
comment.

Thanks,
Thinh

> Cc: stable@vger.kernel.org
> Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com>
> ---
> v9: Fixed function dwc3_rx_fifo_size_show & return values in function
> 	dwc3_link_state_write along with minor changes for code symmetry.
> v8: Replace pm_runtime_get_sync with pm_runtime_resume_and get.
> v7: Replaced pm_runtime_put with pm_runtime_put_sync & returned proper values.
> v6: Added changes to handle get_dync failure appropriately.
> v5: Reworked the patch to resume dwc3 while accessing the registers.
> v4: Introduced pm_runtime_get_if_in_use in order to make sure dwc3 isn't
> 	suspended while accessing the registers.
> v3: Replace pr_err to dev_err. 
> v2: Replaced return 0 with -EINVAL & seq_puts with pr_err.
> 
>  drivers/usb/dwc3/debugfs.c | 109 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 109 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
> index e4a2560b9dc0..ebf03468fac4 100644
> --- a/drivers/usb/dwc3/debugfs.c
> +++ b/drivers/usb/dwc3/debugfs.c
> @@ -332,6 +332,11 @@ 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_resume_and_get(dwc->dev);
> +	if (ret < 0)
> +		return ret;
>  
>  	spin_lock_irqsave(&dwc->lock, flags);
>  	reg = dwc3_readl(dwc->regs, DWC3_GSTS);
> @@ -350,6 +355,8 @@ static int dwc3_lsp_show(struct seq_file *s, void *unused)
>  	}
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>  
> +	pm_runtime_put_sync(dwc->dev);
> +
>  	return 0;
>  }
>  
> @@ -395,6 +402,11 @@ static int dwc3_mode_show(struct seq_file *s, void *unused)
>  	struct dwc3		*dwc = s->private;
>  	unsigned long		flags;
>  	u32			reg;
> +	int			ret;
> +
> +	ret = pm_runtime_resume_and_get(dwc->dev);
> +	if (ret < 0)
> +		return ret;
>  
>  	spin_lock_irqsave(&dwc->lock, flags);
>  	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> @@ -414,6 +426,8 @@ static int dwc3_mode_show(struct seq_file *s, void *unused)
>  		seq_printf(s, "UNKNOWN %08x\n", DWC3_GCTL_PRTCAP(reg));
>  	}
>  
> +	pm_runtime_put_sync(dwc->dev);
> +
>  	return 0;
>  }
>  
> @@ -463,6 +477,11 @@ static int dwc3_testmode_show(struct seq_file *s, void *unused)
>  	struct dwc3		*dwc = s->private;
>  	unsigned long		flags;
>  	u32			reg;
> +	int			ret;
> +
> +	ret = pm_runtime_resume_and_get(dwc->dev);
> +	if (ret < 0)
> +		return ret;
>  
>  	spin_lock_irqsave(&dwc->lock, flags);
>  	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> @@ -493,6 +512,8 @@ static int dwc3_testmode_show(struct seq_file *s, void *unused)
>  		seq_printf(s, "UNKNOWN %d\n", reg);
>  	}
>  
> +	pm_runtime_put_sync(dwc->dev);
> +
>  	return 0;
>  }
>  
> @@ -509,6 +530,7 @@ static ssize_t dwc3_testmode_write(struct file *file,
>  	unsigned long		flags;
>  	u32			testmode = 0;
>  	char			buf[32];
> +	int			ret;
>  
>  	if (copy_from_user(&buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
>  		return -EFAULT;
> @@ -526,10 +548,16 @@ static ssize_t dwc3_testmode_write(struct file *file,
>  	else
>  		testmode = 0;
>  
> +	ret = pm_runtime_resume_and_get(dwc->dev);
> +	if (ret < 0)
> +		return ret;
> +
>  	spin_lock_irqsave(&dwc->lock, flags);
>  	dwc3_gadget_set_test_mode(dwc, testmode);
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>  
> +	pm_runtime_put_sync(dwc->dev);
> +
>  	return count;
>  }
>  
> @@ -548,12 +576,18 @@ static int dwc3_link_state_show(struct seq_file *s, void *unused)
>  	enum dwc3_link_state	state;
>  	u32			reg;
>  	u8			speed;
> +	int			ret;
> +
> +	ret = pm_runtime_resume_and_get(dwc->dev);
> +	if (ret < 0)
> +		return ret;
>  
>  	spin_lock_irqsave(&dwc->lock, flags);
>  	reg = dwc3_readl(dwc->regs, DWC3_GSTS);
>  	if (DWC3_GSTS_CURMOD(reg) != DWC3_GSTS_CURMOD_DEVICE) {
>  		seq_puts(s, "Not available\n");
>  		spin_unlock_irqrestore(&dwc->lock, flags);
> +		pm_runtime_put_sync(dwc->dev);
>  		return 0;
>  	}
>  
> @@ -566,6 +600,8 @@ static int dwc3_link_state_show(struct seq_file *s, void *unused)
>  		   dwc3_gadget_hs_link_string(state));
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>  
> +	pm_runtime_put_sync(dwc->dev);
> +
>  	return 0;
>  }
>  
> @@ -584,6 +620,7 @@ static ssize_t dwc3_link_state_write(struct file *file,
>  	char			buf[32];
>  	u32			reg;
>  	u8			speed;
> +	int			ret;
>  
>  	if (copy_from_user(&buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
>  		return -EFAULT;
> @@ -603,10 +640,15 @@ static ssize_t dwc3_link_state_write(struct file *file,
>  	else
>  		return -EINVAL;
>  
> +	ret = pm_runtime_resume_and_get(dwc->dev);
> +	if (ret < 0)
> +		return ret;
> +
>  	spin_lock_irqsave(&dwc->lock, flags);
>  	reg = dwc3_readl(dwc->regs, DWC3_GSTS);
>  	if (DWC3_GSTS_CURMOD(reg) != DWC3_GSTS_CURMOD_DEVICE) {
>  		spin_unlock_irqrestore(&dwc->lock, flags);
> +		pm_runtime_put_sync(dwc->dev);
>  		return -EINVAL;
>  	}
>  
> @@ -616,12 +658,15 @@ static ssize_t dwc3_link_state_write(struct file *file,
>  	if (speed < DWC3_DSTS_SUPERSPEED &&
>  	    state != DWC3_LINK_STATE_RECOV) {
>  		spin_unlock_irqrestore(&dwc->lock, flags);
> +		pm_runtime_put_sync(dwc->dev);
>  		return -EINVAL;
>  	}
>  
>  	dwc3_gadget_set_link_state(dwc, state);
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>  
> +	pm_runtime_put_sync(dwc->dev);
> +
>  	return count;
>  }
>  
> @@ -645,6 +690,11 @@ static int dwc3_tx_fifo_size_show(struct seq_file *s, void *unused)
>  	unsigned long		flags;
>  	u32			mdwidth;
>  	u32			val;
> +	int			ret;
> +
> +	ret = pm_runtime_resume_and_get(dwc->dev);
> +	if (ret < 0)
> +		return ret;
>  
>  	spin_lock_irqsave(&dwc->lock, flags);
>  	val = dwc3_core_fifo_space(dep, DWC3_TXFIFO);
> @@ -657,6 +707,8 @@ static int dwc3_tx_fifo_size_show(struct seq_file *s, void *unused)
>  	seq_printf(s, "%u\n", val);
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>  
> +	pm_runtime_put_sync(dwc->dev);
> +
>  	return 0;
>  }
>  
> @@ -667,6 +719,11 @@ static int dwc3_rx_fifo_size_show(struct seq_file *s, void *unused)
>  	unsigned long		flags;
>  	u32			mdwidth;
>  	u32			val;
> +	int			ret;
> +
> +	ret = pm_runtime_resume_and_get(dwc->dev);
> +	if (ret < 0)
> +		return ret;
>  
>  	spin_lock_irqsave(&dwc->lock, flags);
>  	val = dwc3_core_fifo_space(dep, DWC3_RXFIFO);
> @@ -679,6 +736,8 @@ static int dwc3_rx_fifo_size_show(struct seq_file *s, void *unused)
>  	seq_printf(s, "%u\n", val);
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>  
> +	pm_runtime_put_sync(dwc->dev);
> +
>  	return 0;
>  }
>  
> @@ -688,12 +747,19 @@ static int dwc3_tx_request_queue_show(struct seq_file *s, void *unused)
>  	struct dwc3		*dwc = dep->dwc;
>  	unsigned long		flags;
>  	u32			val;
> +	int			ret;
> +
> +	ret = pm_runtime_resume_and_get(dwc->dev);
> +	if (ret < 0)
> +		return ret;
>  
>  	spin_lock_irqsave(&dwc->lock, flags);
>  	val = dwc3_core_fifo_space(dep, DWC3_TXREQQ);
>  	seq_printf(s, "%u\n", val);
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>  
> +	pm_runtime_put_sync(dwc->dev);
> +
>  	return 0;
>  }
>  
> @@ -703,12 +769,19 @@ static int dwc3_rx_request_queue_show(struct seq_file *s, void *unused)
>  	struct dwc3		*dwc = dep->dwc;
>  	unsigned long		flags;
>  	u32			val;
> +	int			ret;
> +
> +	ret = pm_runtime_resume_and_get(dwc->dev);
> +	if (ret < 0)
> +		return ret;
>  
>  	spin_lock_irqsave(&dwc->lock, flags);
>  	val = dwc3_core_fifo_space(dep, DWC3_RXREQQ);
>  	seq_printf(s, "%u\n", val);
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>  
> +	pm_runtime_put_sync(dwc->dev);
> +
>  	return 0;
>  }
>  
> @@ -718,12 +791,19 @@ static int dwc3_rx_info_queue_show(struct seq_file *s, void *unused)
>  	struct dwc3		*dwc = dep->dwc;
>  	unsigned long		flags;
>  	u32			val;
> +	int			ret;
> +
> +	ret = pm_runtime_resume_and_get(dwc->dev);
> +	if (ret < 0)
> +		return ret;
>  
>  	spin_lock_irqsave(&dwc->lock, flags);
>  	val = dwc3_core_fifo_space(dep, DWC3_RXINFOQ);
>  	seq_printf(s, "%u\n", val);
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>  
> +	pm_runtime_put_sync(dwc->dev);
> +
>  	return 0;
>  }
>  
> @@ -733,12 +813,19 @@ static int dwc3_descriptor_fetch_queue_show(struct seq_file *s, void *unused)
>  	struct dwc3		*dwc = dep->dwc;
>  	unsigned long		flags;
>  	u32			val;
> +	int			ret;
> +
> +	ret = pm_runtime_resume_and_get(dwc->dev);
> +	if (ret < 0)
> +		return ret;
>  
>  	spin_lock_irqsave(&dwc->lock, flags);
>  	val = dwc3_core_fifo_space(dep, DWC3_DESCFETCHQ);
>  	seq_printf(s, "%u\n", val);
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>  
> +	pm_runtime_put_sync(dwc->dev);
> +
>  	return 0;
>  }
>  
> @@ -748,12 +835,19 @@ static int dwc3_event_queue_show(struct seq_file *s, void *unused)
>  	struct dwc3		*dwc = dep->dwc;
>  	unsigned long		flags;
>  	u32			val;
> +	int			ret;
> +
> +	ret = pm_runtime_resume_and_get(dwc->dev);
> +	if (ret < 0)
> +		return ret;
>  
>  	spin_lock_irqsave(&dwc->lock, flags);
>  	val = dwc3_core_fifo_space(dep, DWC3_EVENTQ);
>  	seq_printf(s, "%u\n", val);
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>  
> +	pm_runtime_put_sync(dwc->dev);
> +
>  	return 0;
>  }
>  
> @@ -798,6 +892,11 @@ static int dwc3_trb_ring_show(struct seq_file *s, void *unused)
>  	struct dwc3		*dwc = dep->dwc;
>  	unsigned long		flags;
>  	int			i;
> +	int			ret;
> +
> +	ret = pm_runtime_resume_and_get(dwc->dev);
> +	if (ret < 0)
> +		return ret;
>  
>  	spin_lock_irqsave(&dwc->lock, flags);
>  	if (dep->number <= 1) {
> @@ -827,6 +926,8 @@ static int dwc3_trb_ring_show(struct seq_file *s, void *unused)
>  out:
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>  
> +	pm_runtime_put_sync(dwc->dev);
> +
>  	return 0;
>  }
>  
> @@ -839,6 +940,11 @@ static int dwc3_ep_info_register_show(struct seq_file *s, void *unused)
>  	u32			lower_32_bits;
>  	u32			upper_32_bits;
>  	u32			reg;
> +	int			ret;
> +
> +	ret = pm_runtime_resume_and_get(dwc->dev);
> +	if (ret < 0)
> +		return ret;
>  
>  	spin_lock_irqsave(&dwc->lock, flags);
>  	reg = DWC3_GDBGLSPMUX_EPSELECT(dep->number);
> @@ -851,6 +957,8 @@ static int dwc3_ep_info_register_show(struct seq_file *s, void *unused)
>  	seq_printf(s, "0x%016llx\n", ep_info);
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>  
> +	pm_runtime_put_sync(dwc->dev);
> +
>  	return 0;
>  }
>  
> @@ -910,6 +1018,7 @@ void dwc3_debugfs_init(struct dwc3 *dwc)
>  	dwc->regset->regs = dwc3_regs;
>  	dwc->regset->nregs = ARRAY_SIZE(dwc3_regs);
>  	dwc->regset->base = dwc->regs - DWC3_GLOBALS_REGS_START;
> +	dwc->regset->dev = dwc->dev;
>  
>  	root = debugfs_create_dir(dev_name(dwc->dev), usb_debug_root);
>  	dwc->debug_root = root;
> -- 
> 2.17.1
> 

Re: [PATCH v9] usb: dwc3: debugfs: Prevent any register access when devices is runtime suspended

[Johan Hovold]

On Sat, May 06, 2023 at 01:30:52AM +0000, Thinh Nguyen wrote:

Udipto, looks like you just ignored my comment about fixing up the patch
Subject.

> On Fri, May 05, 2023, Udipto Goswami wrote:
> > 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.
> > 
> > Fixes: 62ba09d6bb63 ("usb: dwc3: debugfs: Dump internal LSP and ep registers")
> 
> This fix goes before the above change.

Yes, this clearly is not the commit that first introduced this issue.

Either add a Fixes tag for the oldest one or add one for each commit
that introduced debugfs attributes with this issues.

> This also touches on many places and is more than 100 lines. While this
> is a fix, I'm not sure if Cc stable is needed. Perhaps others can
> comment.

I believe this should be backported as it fixes a crash/hang.

The stable rules are flexible, but it may also be possible to break the
patch up in pieces and add a corresponding Fixes tag.

Johan

Re: [PATCH v9] usb: dwc3: debugfs: Prevent any register access when devices is runtime suspended

[Udipto Goswami]

On 5/8/23 5:04 PM, Johan Hovold wrote:
> On Sat, May 06, 2023 at 01:30:52AM +0000, Thinh Nguyen wrote:
> 
> Udipto, looks like you just ignored my comment about fixing up the patch
> Subject.

Hi Johan,

Apologies for this, i missed the comment on the Subject will rectify it.

> 
>> On Fri, May 05, 2023, Udipto Goswami wrote:
>>> 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.
>>>
>>> Fixes: 62ba09d6bb63 ("usb: dwc3: debugfs: Dump internal LSP and ep registers")
>>
>> This fix goes before the above change.
> 
> Yes, this clearly is not the commit that first introduced this issue.
> 
> Either add a Fixes tag for the oldest one or add one for each commit
> that introduced debugfs attributes with this issues.
> 
>> This also touches on many places and is more than 100 lines. While this
>> is a fix, I'm not sure if Cc stable is needed. Perhaps others can
>> comment.
> 
> I believe this should be backported as it fixes a crash/hang.
> 
> The stable rules are flexible, but it may also be possible to break the
> patch up in pieces and add a corresponding Fixes tag.

Agree, I will add a fixes tag for the oldest change that introduced the 
debugfs attributes instead of breaking it to multiple patches and adding 
fixes for each one. (I think the present code changes can stay in one 
patch as we are fixing the same issue in all the functions).

Let me know if you think otherwise?

Thanks,
-Udipto

Re: [PATCH v9] usb: dwc3: debugfs: Prevent any register access when devices is runtime suspended

[Thinh Nguyen]

On Mon, May 08, 2023, Johan Hovold wrote:
> 
> The stable rules are flexible, but it may also be possible to break the
> patch up in pieces and add a corresponding Fixes tag.
> 
> Johan

In this case, I'd prefer to keep it all in a single patch to easily
track the change. Anyone who needs this for the older kernel can rework
the upstream version for backport.

Thanks,
Thinh

Re: [PATCH v9] usb: dwc3: debugfs: Prevent any register access when devices is runtime suspended

[Johan Hovold]

On Mon, May 08, 2023 at 09:18:17PM +0530, Udipto Goswami wrote:

> > I believe this should be backported as it fixes a crash/hang.
> > 
> > The stable rules are flexible, but it may also be possible to break the
> > patch up in pieces and add a corresponding Fixes tag.
> 
> Agree, I will add a fixes tag for the oldest change that introduced the 
> debugfs attributes instead of breaking it to multiple patches and adding 
> fixes for each one. (I think the present code changes can stay in one 
> patch as we are fixing the same issue in all the functions).
> 
> Let me know if you think otherwise?

Sounds good to me. Note that the fix depends on 

	30332eeefec8 ("debugfs: regset32: Add Runtime PM support")

which went into 5.7.

This can be documented as

	Cc: stable@vger.kernel.org	# 3.2: 30332eeefec8: debugfs: regset32: Add Runtime PM support

(see Documentation/process/stable-kernel-rules.rst).

Note that this issue appears to have been there since the driver was
first merged in 3.2.

Johan