diff mbox series

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

Message ID 20230418121835.17550-1-quic_ugoswami@quicinc.com
State Superseded
Headers show
Series [v5] usb: dwc3: debugfs: Prevent any register access when devices | expand

Commit Message

Udipto Goswami April 18, 2023, 12:18 p.m. UTC 
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>
Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com>
---
v5: Reworked the patch to resume dwc3 while accessing the registers.

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

Comments

Udipto Goswami April 19, 2023, 3:43 a.m. UTC | #1 
On 4/18/23 6:56 PM, Greg Kroah-Hartman wrote:
> On Tue, Apr 18, 2023 at 03:01:39PM +0200, Johan Hovold wrote:
>> 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.
> 
> Ah, I thought so :(
> 
> Udipto, please go complete the developer legal training that I know your
> company provides for dealing with things like this as they take it very
> seriously, before you resubmit this, or any other kernel patches.
> 
> thanks,
> 
> greg k-h

Hi Greg, Johan, Oliver,

Apologies for this, will go through the guidelines and rework the patch
accordingly.
kernel test robot April 19, 2023, 6:14 a.m. UTC | #2 
Hi Udipto,

kernel test robot noticed the following build warnings:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on usb/usb-next usb/usb-linus driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus westeri-thunderbolt/next linus/master v6.3-rc7 next-20230418]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Udipto-Goswami/usb-dwc3-debugfs-Prevent-any-register-access-when-devices/20230418-202039
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20230418121835.17550-1-quic_ugoswami%40quicinc.com
patch subject: [PATCH v5] usb: dwc3: debugfs: Prevent any register access when devices
config: x86_64-randconfig-m001 (https://download.01.org/0day-ci/archive/20230419/202304191354.gk7ee6Gf-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Link: https://lore.kernel.org/r/202304191354.gk7ee6Gf-lkp@intel.com/

smatch warnings:
drivers/usb/dwc3/debugfs.c:338 dwc3_lsp_show() warn: pm_runtime_get_sync() also returns 1 on success
drivers/usb/dwc3/debugfs.c:409 dwc3_mode_show() warn: pm_runtime_get_sync() also returns 1 on success
drivers/usb/dwc3/debugfs.c:485 dwc3_testmode_show() warn: pm_runtime_get_sync() also returns 1 on success
drivers/usb/dwc3/debugfs.c:555 dwc3_testmode_write() warn: pm_runtime_get_sync() also returns 1 on success
drivers/usb/dwc3/debugfs.c:586 dwc3_link_state_show() warn: pm_runtime_get_sync() also returns 1 on success
drivers/usb/dwc3/debugfs.c:649 dwc3_link_state_write() warn: pm_runtime_get_sync() also returns 1 on success
drivers/usb/dwc3/debugfs.c:702 dwc3_tx_fifo_size_show() warn: pm_runtime_get_sync() also returns 1 on success
drivers/usb/dwc3/debugfs.c:732 dwc3_rx_fifo_size_show() warn: pm_runtime_get_sync() also returns 1 on success
drivers/usb/dwc3/debugfs.c:761 dwc3_tx_request_queue_show() warn: pm_runtime_get_sync() also returns 1 on success
drivers/usb/dwc3/debugfs.c:784 dwc3_rx_request_queue_show() warn: pm_runtime_get_sync() also returns 1 on success
drivers/usb/dwc3/debugfs.c:807 dwc3_rx_info_queue_show() warn: pm_runtime_get_sync() also returns 1 on success
drivers/usb/dwc3/debugfs.c:830 dwc3_descriptor_fetch_queue_show() warn: pm_runtime_get_sync() also returns 1 on success
drivers/usb/dwc3/debugfs.c:853 dwc3_event_queue_show() warn: pm_runtime_get_sync() also returns 1 on success
drivers/usb/dwc3/debugfs.c:911 dwc3_trb_ring_show() warn: pm_runtime_get_sync() also returns 1 on success
drivers/usb/dwc3/debugfs.c:960 dwc3_ep_info_register_show() warn: pm_runtime_get_sync() also returns 1 on success

vim +338 drivers/usb/dwc3/debugfs.c

62ba09d6bb6330 Thinh Nguyen              2018-11-07  329  static int dwc3_lsp_show(struct seq_file *s, void *unused)
62ba09d6bb6330 Thinh Nguyen              2018-11-07  330  {
62ba09d6bb6330 Thinh Nguyen              2018-11-07  331  	struct dwc3		*dwc = s->private;
62ba09d6bb6330 Thinh Nguyen              2018-11-07  332  	unsigned int		current_mode;
62ba09d6bb6330 Thinh Nguyen              2018-11-07  333  	unsigned long		flags;
62ba09d6bb6330 Thinh Nguyen              2018-11-07  334  	u32			reg;
86f75fd9ae2609 Udipto Goswami            2023-04-18  335  	int			ret;
86f75fd9ae2609 Udipto Goswami            2023-04-18  336  
86f75fd9ae2609 Udipto Goswami            2023-04-18  337  	ret = pm_runtime_get_sync(dwc->dev);
86f75fd9ae2609 Udipto Goswami            2023-04-18 @338  	if (!ret || ret < 0) {
86f75fd9ae2609 Udipto Goswami            2023-04-18  339  		pm_runtime_put(dwc->dev);
86f75fd9ae2609 Udipto Goswami            2023-04-18  340  		return 0;

I don't know what's going on here, but Smatch doesn't like it.  :P

86f75fd9ae2609 Udipto Goswami            2023-04-18  341  	}
62ba09d6bb6330 Thinh Nguyen              2018-11-07  342  
62ba09d6bb6330 Thinh Nguyen              2018-11-07  343  	spin_lock_irqsave(&dwc->lock, flags);
62ba09d6bb6330 Thinh Nguyen              2018-11-07  344  	reg = dwc3_readl(dwc->regs, DWC3_GSTS);
62ba09d6bb6330 Thinh Nguyen              2018-11-07  345  	current_mode = DWC3_GSTS_CURMOD(reg);
62ba09d6bb6330 Thinh Nguyen              2018-11-07  346  
62ba09d6bb6330 Thinh Nguyen              2018-11-07  347  	switch (current_mode) {
62ba09d6bb6330 Thinh Nguyen              2018-11-07  348  	case DWC3_GSTS_CURMOD_HOST:
62ba09d6bb6330 Thinh Nguyen              2018-11-07  349  		dwc3_host_lsp(s);
62ba09d6bb6330 Thinh Nguyen              2018-11-07  350  		break;
62ba09d6bb6330 Thinh Nguyen              2018-11-07  351  	case DWC3_GSTS_CURMOD_DEVICE:
62ba09d6bb6330 Thinh Nguyen              2018-11-07  352  		dwc3_gadget_lsp(s);
62ba09d6bb6330 Thinh Nguyen              2018-11-07  353  		break;
62ba09d6bb6330 Thinh Nguyen              2018-11-07  354  	default:
62ba09d6bb6330 Thinh Nguyen              2018-11-07  355  		seq_puts(s, "Mode is unknown, no LSP register printed\n");
62ba09d6bb6330 Thinh Nguyen              2018-11-07  356  		break;
62ba09d6bb6330 Thinh Nguyen              2018-11-07  357  	}
62ba09d6bb6330 Thinh Nguyen              2018-11-07  358  	spin_unlock_irqrestore(&dwc->lock, flags);
86f75fd9ae2609 Udipto Goswami            2023-04-18  359  	pm_runtime_put(dwc->dev);
62ba09d6bb6330 Thinh Nguyen              2018-11-07  360  
62ba09d6bb6330 Thinh Nguyen              2018-11-07  361  	return 0;
62ba09d6bb6330 Thinh Nguyen              2018-11-07  362  }
diff mbox series

Patch

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) {
+		pm_runtime_put(dwc->dev);
+		return 0;
+	}
 
 	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 || ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return 0;
+	}
 
 	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 || ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return 0;
+	}
 
 	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 || ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return 0;
+	}
+
 	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 || ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return 0;
+	}
 
 	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 || ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return 0;
+	}
+
 	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 || ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return 0;
+	}
 
 	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 || ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return 0;
+	}
 
 	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 || ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return 0;
+	}
 
 	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 || ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return 0;
+	}
 
 	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 || ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return 0;
+	}
 
 	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 || ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return 0;
+	}
 
 	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 || ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return 0;
+	}
 
 	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 || ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return 0;
+	}
 
 	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 || ret < 0) {
+		pm_runtime_put(dwc->dev);
+		return 0;
+	}
 
 	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;
 }