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

Udipto Goswami <quic_ugoswami@xxxxxxxxxxx> · Thu, 27 Apr 2023 14:30:29 +0530

On 4/26/23 6:46 AM, Thinh Nguyen wrote:
On Thu, Apr 20, 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.

Signed-off-by: Udipto Goswami <quic_ugoswami@xxxxxxxxxxx>
---

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 | 124 +++++++++++++++++++++++++++++++++++++
  1 file changed, 124 insertions(+)

diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
index e4a2560b9dc0..b08604a73e69 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 < 0) {
+		pm_runtime_put(dwc->dev);

Should we use pm_runtime_put_sync

+		return -EINVAL;

Why not return "ret"?
Hi Thinh,
Sure will fix it in next version.

+	}
  
  	spin_lock_irqsave(&dwc->lock, flags);
  	reg = dwc3_readl(dwc->regs, DWC3_GSTS);
@@ -349,6 +356,7 @@ static int dwc3_lsp_show(struct seq_file *s, void *unused)
  		break;
  	}
  	spin_unlock_irqrestore(&dwc->lock, flags);
+	pm_runtime_put(dwc->dev);
  
  	return 0;
  }
@@ -395,6 +403,13 @@ 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_get_sync(dwc->dev);
+	if (ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return -EINVAL;
+	}
  
  	spin_lock_irqsave(&dwc->lock, flags);
  	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
@@ -414,6 +429,7 @@ static int dwc3_mode_show(struct seq_file *s, void *unused)
  		seq_printf(s, "UNKNOWN %08x\n", DWC3_GCTL_PRTCAP(reg));
  	}
  
+	pm_runtime_put(dwc->dev);
  	return 0;
  }
  
@@ -463,6 +479,13 @@ 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_get_sync(dwc->dev);
+	if (ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return -EINVAL;
+	}
  
  	spin_lock_irqsave(&dwc->lock, flags);
  	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
@@ -493,6 +516,7 @@ static int dwc3_testmode_show(struct seq_file *s, void *unused)
  		seq_printf(s, "UNKNOWN %d\n", reg);
  	}
  
+	pm_runtime_put(dwc->dev);
  	return 0;
  }
  
@@ -509,6 +533,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 +551,17 @@ static ssize_t dwc3_testmode_write(struct file *file,
  	else
  		testmode = 0;
  
+	ret = pm_runtime_get_sync(dwc->dev);
+	if (ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return -EINVAL;
+	}
+
  	spin_lock_irqsave(&dwc->lock, flags);
  	dwc3_gadget_set_test_mode(dwc, testmode);
  	spin_unlock_irqrestore(&dwc->lock, flags);
  
+	pm_runtime_put(dwc->dev);
  	return count;
  }
  
@@ -548,12 +580,20 @@ 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_get_sync(dwc->dev);
+	if (ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return -EINVAL;
+	}
  
  	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(dwc->dev);
  		return 0;
  	}
  
@@ -566,6 +606,7 @@ 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(dwc->dev);
  	return 0;
  }
  
@@ -584,6 +625,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 +645,17 @@ static ssize_t dwc3_link_state_write(struct file *file,
  	else
  		return -EINVAL;
  
+	ret = pm_runtime_get_sync(dwc->dev);
+	if (ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return -EINVAL;
+	}
+
  	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(dwc->dev);
  		return -EINVAL;
  	}
  
@@ -616,12 +665,14 @@ 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(dwc->dev);
  		return -EINVAL;
  	}
  
  	dwc3_gadget_set_link_state(dwc, state);
  	spin_unlock_irqrestore(&dwc->lock, flags);
  
+	pm_runtime_put(dwc->dev);
  	return count;
  }
  
@@ -645,6 +696,13 @@ 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_get_sync(dwc->dev);
+	if (ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return -EINVAL;
+	}
  
  	spin_lock_irqsave(&dwc->lock, flags);
  	val = dwc3_core_fifo_space(dep, DWC3_TXFIFO);
@@ -657,6 +715,7 @@ 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(dwc->dev);
  	return 0;
  }
  
@@ -667,6 +726,13 @@ 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_get_sync(dwc->dev);
+	if (ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return -EINVAL;
+	}
  
  	spin_lock_irqsave(&dwc->lock, flags);
  	val = dwc3_core_fifo_space(dep, DWC3_RXFIFO);
@@ -679,6 +745,7 @@ 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(dwc->dev);
  	return 0;
  }
  
@@ -688,12 +755,20 @@ 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_get_sync(dwc->dev);
+	if (ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return -EINVAL;
+	}
  
  	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(dwc->dev);
  	return 0;
  }
  
@@ -703,12 +778,20 @@ 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_get_sync(dwc->dev);
+	if (ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return -EINVAL;
+	}
  
  	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(dwc->dev);
  	return 0;
  }
  
@@ -718,12 +801,20 @@ 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_get_sync(dwc->dev);
+	if (ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return -EINVAL;
+	}
  
  	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(dwc->dev);
  	return 0;
  }
  
@@ -733,12 +824,20 @@ 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_get_sync(dwc->dev);
+	if (ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return -EINVAL;
+	}
  
  	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(dwc->dev);
  	return 0;
  }
  
@@ -748,12 +847,20 @@ 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_get_sync(dwc->dev);
+	if (ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return -EINVAL;
+	}
  
  	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(dwc->dev);
  	return 0;
  }
  
@@ -798,6 +905,13 @@ 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_get_sync(dwc->dev);
+	if (ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return -EINVAL;
+	}
  
  	spin_lock_irqsave(&dwc->lock, flags);
  	if (dep->number <= 1) {
@@ -827,6 +941,7 @@ static int dwc3_trb_ring_show(struct seq_file *s, void *unused)
  out:
  	spin_unlock_irqrestore(&dwc->lock, flags);
  
+	pm_runtime_put(dwc->dev);
  	return 0;
  }
  
@@ -839,6 +954,13 @@ 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_get_sync(dwc->dev);
+	if (ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return -EINVAL;
+	}
  
  	spin_lock_irqsave(&dwc->lock, flags);
  	reg = DWC3_GDBGLSPMUX_EPSELECT(dep->number);
@@ -851,6 +973,7 @@ 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(dwc->dev);
  	return 0;
  }
  
@@ -910,6 +1033,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


Thanks,
Thinh

Thanks,
-Udipto






