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

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

[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, bail out of debugfs
function is dwc3 is suspended.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com>
---
v4: Introduced pm_runtime_get_if_in_use in order to make sure dwc3 isn't
suspended while accessing the registers.

 drivers/usb/dwc3/debugfs.c | 191 ++++++++++++++++++++++++++++++++-----
 1 file changed, 169 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
index 850df0e6bcab..e57cafb7da4b 100644
--- a/drivers/usb/dwc3/debugfs.c
+++ b/drivers/usb/dwc3/debugfs.c
@@ -543,13 +543,25 @@ static int dwc3_link_state_show(struct seq_file *s, void *unused)
 	enum dwc3_link_state	state;
 	u32			reg;
 	u8			speed;
+	int			ret = 0;
+
+	ret = pm_runtime_get_if_in_use(dwc->dev);
+	/* check if pm runtime get fails, bail out early */
+	if (ret < 0)
+		goto err_nolock;
+
+	if (!ret) {
+		ret = -EINVAL;
+		dev_err(dwc->dev,
+				"Invalid operation, DWC3 suspended!");
+		goto err_nolock;
+	}
 
 	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);
-		return 0;
+		goto err;
 	}
 
 	reg = dwc3_readl(dwc->regs, DWC3_DSTS);
@@ -559,9 +571,11 @@ static int dwc3_link_state_show(struct seq_file *s, void *unused)
 	seq_printf(s, "%s\n", (speed >= DWC3_DSTS_SUPERSPEED) ?
 		   dwc3_gadget_link_string(state) :
 		   dwc3_gadget_hs_link_string(state));
+err:
 	spin_unlock_irqrestore(&dwc->lock, flags);
-
-	return 0;
+	pm_runtime_put(dwc->dev);
+err_nolock:
+	return ret;
 }
 
 static int dwc3_link_state_open(struct inode *inode, struct file *file)
@@ -579,6 +593,20 @@ static ssize_t dwc3_link_state_write(struct file *file,
 	char			buf[32];
 	u32			reg;
 	u8			speed;
+	int			ret = 0;
+
+	ret = pm_runtime_get_if_in_use(dwc->dev);
+	/* check if pm runtime get fails, bail out early */
+	if (ret < 0)
+		goto err_nolock;
+
+	if (!ret) {
+		ret = -EINVAL;
+		dev_err(dwc->dev,
+				"Invalid operation, DWC3 suspended!");
+		goto err_nolock;
+	}
+
 
 	if (copy_from_user(&buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
 		return -EFAULT;
@@ -601,8 +629,8 @@ static ssize_t dwc3_link_state_write(struct file *file,
 	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);
-		return -EINVAL;
+		count = -EINVAL;
+		goto err;
 	}
 
 	reg = dwc3_readl(dwc->regs, DWC3_DSTS);
@@ -610,13 +638,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);
-		return -EINVAL;
+		count = -EINVAL;
+		goto err;
 	}
 
 	dwc3_gadget_set_link_state(dwc, state);
+err:
 	spin_unlock_irqrestore(&dwc->lock, flags);
-
+	pm_runtime_put(dwc->dev);
+err_nolock:
 	return count;
 }
 
@@ -640,6 +670,19 @@ static int dwc3_tx_fifo_size_show(struct seq_file *s, void *unused)
 	unsigned long		flags;
 	u32			mdwidth;
 	u32			val;
+	int			ret = 0;
+
+	ret = pm_runtime_get_if_in_use(dwc->dev);
+	/* check if pm runtime get fails, bail out early */
+	if (ret < 0)
+		goto err_nolock;
+
+	if (!ret) {
+		ret = -EINVAL;
+		dev_err(dwc->dev,
+				"Invalid operation, DWC3 suspended!");
+		goto err_nolock;
+	}
 
 	spin_lock_irqsave(&dwc->lock, flags);
 	val = dwc3_core_fifo_space(dep, DWC3_TXFIFO);
@@ -650,9 +693,11 @@ static int dwc3_tx_fifo_size_show(struct seq_file *s, void *unused)
 	val *= mdwidth;
 	val >>= 3;
 	seq_printf(s, "%u\n", val);
-	spin_unlock_irqrestore(&dwc->lock, flags);
 
-	return 0;
+	spin_unlock_irqrestore(&dwc->lock, flags);
+	pm_runtime_put(dwc->dev);
+err_nolock:
+	return ret;
 }
 
 static int dwc3_rx_fifo_size_show(struct seq_file *s, void *unused)
@@ -662,6 +707,19 @@ static int dwc3_rx_fifo_size_show(struct seq_file *s, void *unused)
 	unsigned long		flags;
 	u32			mdwidth;
 	u32			val;
+	int			ret = 0;
+
+	ret = pm_runtime_get_if_in_use(dwc->dev);
+	/* check if pm runtime get fails, bail out early */
+	if (ret < 0)
+		goto err_nolock;
+
+	if (!ret) {
+		ret = -EINVAL;
+		dev_err(dwc->dev,
+				"Invalid operation, DWC3 suspended!");
+		goto err_nolock;
+	}
 
 	spin_lock_irqsave(&dwc->lock, flags);
 	val = dwc3_core_fifo_space(dep, DWC3_RXFIFO);
@@ -673,8 +731,9 @@ static int dwc3_rx_fifo_size_show(struct seq_file *s, void *unused)
 	val >>= 3;
 	seq_printf(s, "%u\n", val);
 	spin_unlock_irqrestore(&dwc->lock, flags);
-
-	return 0;
+	pm_runtime_put(dwc->dev);
+err_nolock:
+	return ret;
 }
 
 static int dwc3_tx_request_queue_show(struct seq_file *s, void *unused)
@@ -683,13 +742,27 @@ 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 = 0;
+
+	ret = pm_runtime_get_if_in_use(dwc->dev);
+	/* check if pm runtime get fails, bail out early */
+	if (ret < 0)
+		goto err_nolock;
+
+	if (!ret) {
+		ret = -EINVAL;
+		dev_err(dwc->dev,
+				"Invalid operation, DWC3 suspended!");
+		goto err_nolock;
+	}
 
 	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);
-
-	return 0;
+	pm_runtime_put(dwc->dev);
+err_nolock:
+	return ret;
 }
 
 static int dwc3_rx_request_queue_show(struct seq_file *s, void *unused)
@@ -698,13 +771,27 @@ 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 = 0;
+
+	ret = pm_runtime_get_if_in_use(dwc->dev);
+	/* check if pm runtime get fails, bail out early */
+	if (ret < 0)
+		goto err_nolock;
+
+	if (!ret) {
+		ret = -EINVAL;
+		dev_err(dwc->dev,
+				"Invalid operation, DWC3 suspended!");
+		goto err_nolock;
+	}
 
 	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);
-
-	return 0;
+	pm_runtime_put(dwc->dev);
+err_nolock:
+	return ret;
 }
 
 static int dwc3_rx_info_queue_show(struct seq_file *s, void *unused)
@@ -713,13 +800,28 @@ 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 = 0;
+
+	ret = pm_runtime_get_if_in_use(dwc->dev);
+	/* check if pm runtime get fails, bail out early */
+	if (ret < 0)
+		goto err_nolock;
+
+	if (!ret) {
+		ret = -EINVAL;
+		dev_err(dwc->dev,
+				"Invalid operation, DWC3 suspended!");
+		goto err_nolock;
+	}
 
 	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);
+err_nolock:
+	return ret;
 
-	return 0;
 }
 
 static int dwc3_descriptor_fetch_queue_show(struct seq_file *s, void *unused)
@@ -728,13 +830,29 @@ 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 = 0;
+
+	ret = pm_runtime_get_if_in_use(dwc->dev);
+	/* check if pm runtime get fails, bail out early */
+	if (ret < 0)
+		goto err_nolock;
+
+	if (!ret) {
+		ret = -EINVAL;
+		dev_err(dwc->dev,
+				"Invalid operation, DWC3 suspended!");
+		goto err_nolock;
+	}
 
 	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);
+err_nolock:
+	return ret;
+
 
-	return 0;
 }
 
 static int dwc3_event_queue_show(struct seq_file *s, void *unused)
@@ -743,13 +861,28 @@ static int dwc3_event_queue_show(struct seq_file *s, void *unused)
 	struct dwc3		*dwc = dep->dwc;
 	unsigned long		flags;
 	u32			val;
+	int			ret = 0;
+
+	ret = pm_runtime_get_if_in_use(dwc->dev);
+	/* check if pm runtime get fails, bail out early */
+	if (ret < 0)
+		goto err_nolock;
+
+	if (!ret) {
+		ret = -EINVAL;
+		dev_err(dwc->dev,
+				"Invalid operation, DWC3 suspended!");
+		goto err_nolock;
+	}
 
 	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);
+err_nolock:
+	return ret;
 
-	return 0;
 }
 
 static int dwc3_transfer_type_show(struct seq_file *s, void *unused)
@@ -834,6 +967,19 @@ 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 = 0;
+
+	ret = pm_runtime_get_if_in_use(dwc->dev);
+	/* check if pm runtime get fails, bail out early */
+	if (ret < 0)
+		goto err_nolock;
+
+	if (!ret) {
+		ret = -EINVAL;
+		dev_err(dwc->dev,
+				"Invalid operation, DWC3 suspended!");
+		goto err_nolock;
+	}
 
 	spin_lock_irqsave(&dwc->lock, flags);
 	reg = DWC3_GDBGLSPMUX_EPSELECT(dep->number);
@@ -845,8 +991,9 @@ static int dwc3_ep_info_register_show(struct seq_file *s, void *unused)
 	ep_info = ((u64)upper_32_bits << 32) | lower_32_bits;
 	seq_printf(s, "0x%016llx\n", ep_info);
 	spin_unlock_irqrestore(&dwc->lock, flags);
-
-	return 0;
+	pm_runtime_put(dwc->dev);
+err_nolock:
+	return ret;
 }
 
 DEFINE_SHOW_ATTRIBUTE(dwc3_tx_fifo_size);
-- 
2.17.1

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

[Oliver Neukum]

On 04.04.23 12:00, 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, bail out of debugfs
> function is dwc3 is suspended.
> 

Hi,

those look nice, except that they are doing a lot of seq_printf() under
a spinlock that is not needed.

	Regards
		Oliver

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

[Johan Hovold]

On Tue, Apr 04, 2023 at 03:30:55PM +0530, 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, bail out of debugfs
> function is dwc3 is suspended.
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com>
> ---
> v4: Introduced pm_runtime_get_if_in_use in order to make sure dwc3 isn't
> suspended while accessing the registers.
> 
>  drivers/usb/dwc3/debugfs.c | 191 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 169 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
> index 850df0e6bcab..e57cafb7da4b 100644
> --- a/drivers/usb/dwc3/debugfs.c
> +++ b/drivers/usb/dwc3/debugfs.c
> @@ -543,13 +543,25 @@ static int dwc3_link_state_show(struct seq_file *s, void *unused)
>  	enum dwc3_link_state	state;
>  	u32			reg;
>  	u8			speed;
> +	int			ret = 0;
> +
> +	ret = pm_runtime_get_if_in_use(dwc->dev);
> +	/* check if pm runtime get fails, bail out early */
> +	if (ret < 0)
> +		goto err_nolock;
> +
> +	if (!ret) {
> +		ret = -EINVAL;
> +		dev_err(dwc->dev,
> +				"Invalid operation, DWC3 suspended!");
> +		goto err_nolock;
> +	}

This is backwards. If you need the device to be active to access these
registers then you should resume it unconditionally instead of failing.

Johan

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

[Oliver Neukum]

On 04.04.23 13:05, Johan Hovold wrote:

> This is backwards. If you need the device to be active to access these
> registers then you should resume it unconditionally instead of failing.

Hi,

usually you would be right. But this is debugfs. It is intended to observe
the system in the state it is actually in. If by the act of observation you
wake up the device, you change the experiment.

	Regards
		Oliver

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

[Johan Hovold]

On Tue, Apr 04, 2023 at 01:09:19PM +0200, Oliver Neukum wrote:
> On 04.04.23 13:05, Johan Hovold wrote:
> 
> > This is backwards. If you need the device to be active to access these
> > registers then you should resume it unconditionally instead of failing.

> usually you would be right. But this is debugfs. It is intended to observe
> the system in the state it is actually in. If by the act of observation you
> wake up the device, you change the experiment.

I admittedly didn't look to closely at what this particular debugfs
interface is used for, but I generally do not agree with that reasoning.

The device is being used; by the driver and ultimately by a user telling
the driver to do something on their behalf. The fact that the user is
initiating an action through an interface which intended for debugging
should not matter (and the user always has the option to check the
runtime pm state before initiating the action if that matters at all).

Johan

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

[Oliver Neukum]

On 04.04.23 13:43, Johan Hovold wrote:

> The device is being used; by the driver and ultimately by a user telling

I am afraid that is just an assumption we cannot make. The user may just as
well be reading a device state before a device is being used as that may matter.

> the driver to do something on their behalf. The fact that the user is
> initiating an action through an interface which intended for debugging
> should not matter (and the user always has the option to check the
> runtime pm state before initiating the action if that matters at all).

1. That is a race condition.
2. Quite a lot of bugs we are looking at involve power transitions.
You just cannot assume that a device will react the same way if it was
waken up between events.

	Regards
		Oliver

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

[Udipto Goswami]

On 4/4/23 5:13 PM, Johan Hovold wrote:
> On Tue, Apr 04, 2023 at 01:09:19PM +0200, Oliver Neukum wrote:
>> On 04.04.23 13:05, Johan Hovold wrote:
>>
>>> This is backwards. If you need the device to be active to access these
>>> registers then you should resume it unconditionally instead of failing.
> 
>> usually you would be right. But this is debugfs. It is intended to observe
>> the system in the state it is actually in. If by the act of observation you
>> wake up the device, you change the experiment.
> 
> I admittedly didn't look to closely at what this particular debugfs
> interface is used for, but I generally do not agree with that reasoning.
> 
> The device is being used; by the driver and ultimately by a user telling
> the driver to do something on their behalf. The fact that the user is
> initiating an action through an interface which intended for debugging
> should not matter (and the user always has the option to check the
> runtime pm state before initiating the action if that matters at all) >
> Johan

Hi Johan,

for instance, lets say user is trying to dump the value of link_state 
register through dwc3_link_state_show, wouldn't a pm_runtime_get would 
change the link_state? or I'm assuming it wrong?

Thanks,
-Udipto

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

[Johan Hovold]

On Tue, Apr 04, 2023 at 02:07:22PM +0200, Oliver Neukum wrote:
> 
> 
> On 04.04.23 13:43, Johan Hovold wrote:
> 
> > The device is being used; by the driver and ultimately by a user telling
> 
> I am afraid that is just an assumption we cannot make. The user may just as
> well be reading a device state before a device is being used as that may matter.

It's a perfectly valid assumption to make, and it is was all drivers do
for debugfs (as well as sysfs). You are the one arguing for making an
exception, which I don't think is warranted.

> > the driver to do something on their behalf. The fact that the user is
> > initiating an action through an interface which intended for debugging
> > should not matter (and the user always has the option to check the
> > runtime pm state before initiating the action if that matters at all).
> 
> 1. That is a race condition.

Sure, but you can't have it both ways. Your proposed inverted logic is
racy as you may or may not get any data.

> 2. Quite a lot of bugs we are looking at involve power transitions.
> You just cannot assume that a device will react the same way if it was
> waken up between events.

Then just don't use the interface if you for whatever reason don't want
to wake the device up.

Johan

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

[Alan Stern]

On Tue, Apr 04, 2023 at 02:30:25PM +0200, Johan Hovold wrote:
> On Tue, Apr 04, 2023 at 02:07:22PM +0200, Oliver Neukum wrote:
> > 
> > 
> > On 04.04.23 13:43, Johan Hovold wrote:
> > 
> > > The device is being used; by the driver and ultimately by a user telling
> > 
> > I am afraid that is just an assumption we cannot make. The user may just as
> > well be reading a device state before a device is being used as that may matter.
> 
> It's a perfectly valid assumption to make, and it is was all drivers do
> for debugfs (as well as sysfs). You are the one arguing for making an
> exception, which I don't think is warranted.
> 
> > > the driver to do something on their behalf. The fact that the user is
> > > initiating an action through an interface which intended for debugging
> > > should not matter (and the user always has the option to check the
> > > runtime pm state before initiating the action if that matters at all).
> > 
> > 1. That is a race condition.
> 
> Sure, but you can't have it both ways. Your proposed inverted logic is
> racy as you may or may not get any data.
> 
> > 2. Quite a lot of bugs we are looking at involve power transitions.
> > You just cannot assume that a device will react the same way if it was
> > waken up between events.
> 
> Then just don't use the interface if you for whatever reason don't want
> to wake the device up.

For what it's worth, the ehci-hcd driver tests (under its private 
spinlock) whether the hardware is accessible -- i.e., not suspended -- 
before trying to carry out any debugfs operations that would use the 
device registers.  If not, all you get is something like:

	bus <buspath>, device <devname>
	<description>
	SUSPENDED (no register access)

Alan Stern

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

[Johan Hovold]

On Tue, Apr 04, 2023 at 05:46:13PM +0530, Udipto Goswami wrote:
> On 4/4/23 5:13 PM, Johan Hovold wrote:
> > On Tue, Apr 04, 2023 at 01:09:19PM +0200, Oliver Neukum wrote:
> >> On 04.04.23 13:05, Johan Hovold wrote:

> >>> This is backwards. If you need the device to be active to access these
> >>> registers then you should resume it unconditionally instead of failing.
> > 
> >> usually you would be right. But this is debugfs. It is intended to observe
> >> the system in the state it is actually in. If by the act of observation you
> >> wake up the device, you change the experiment.
> > 
> > I admittedly didn't look to closely at what this particular debugfs
> > interface is used for, but I generally do not agree with that reasoning.
> > 
> > The device is being used; by the driver and ultimately by a user telling
> > the driver to do something on their behalf. The fact that the user is
> > initiating an action through an interface which intended for debugging
> > should not matter (and the user always has the option to check the
> > runtime pm state before initiating the action if that matters at all)

> for instance, lets say user is trying to dump the value of link_state 
> register through dwc3_link_state_show, wouldn't a pm_runtime_get would 
> change the link_state? or I'm assuming it wrong?

There may be some specific debugfs interfaces (e.g. related to PM) where
taking the runtime pm state into account makes sense, but then I don't
think you should return an error if the device happens to be suspended.

Looking at the dwc3 'link_state', it currently just returns "Not
available\n" when not in peripheral mode, for example. Perhaps you can
do something similar if you can neither infer or retrieve the actual
link state.

But after skimming the backstory for this patch, you yourself said that
the motivation for this patch is simply to avoid crashing when accessing
these interfaces if the device happens to be runtime suspended.

For that you should just resume the device unconditionally before the
register accesses as all other drivers do (and such a patch should
be backported to stable).

There's no need to take hypothetical PM debugging issues into account to
address this.

Johan

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

[Johan Hovold]

On Tue, Apr 04, 2023 at 10:29:31AM -0400, Alan Stern wrote:
> On Tue, Apr 04, 2023 at 02:30:25PM +0200, Johan Hovold wrote:
> > On Tue, Apr 04, 2023 at 02:07:22PM +0200, Oliver Neukum wrote:
> > > On 04.04.23 13:43, Johan Hovold wrote:
> > > 
> > > > The device is being used; by the driver and ultimately by a user telling
> > > 
> > > I am afraid that is just an assumption we cannot make. The user may just as
> > > well be reading a device state before a device is being used as that may matter.
> > 
> > It's a perfectly valid assumption to make, and it is was all drivers do
> > for debugfs (as well as sysfs). You are the one arguing for making an
> > exception, which I don't think is warranted.
> > 
> > > > the driver to do something on their behalf. The fact that the user is
> > > > initiating an action through an interface which intended for debugging
> > > > should not matter (and the user always has the option to check the
> > > > runtime pm state before initiating the action if that matters at all).
> > > 
> > > 1. That is a race condition.
> > 
> > Sure, but you can't have it both ways. Your proposed inverted logic is
> > racy as you may or may not get any data.
> > 
> > > 2. Quite a lot of bugs we are looking at involve power transitions.
> > > You just cannot assume that a device will react the same way if it was
> > > waken up between events.
> > 
> > Then just don't use the interface if you for whatever reason don't want
> > to wake the device up.
> 
> For what it's worth, the ehci-hcd driver tests (under its private 
> spinlock) whether the hardware is accessible -- i.e., not suspended -- 
> before trying to carry out any debugfs operations that would use the 
> device registers.  If not, all you get is something like:
> 
> 	bus <buspath>, device <devname>
> 	<description>
> 	SUSPENDED (no register access)

Thanks, I only grepped the tree for drivers using runtime pm directly in
their debugfs callbacks so I missed this. Apparently, ohci and uhci do
the same. And when resources are not managed using runtime PM, I guess
there is no other good alternative.

The xhci driver on the other hand, do appear to call
pm_runtime_get_sync() before accessing registers through debugfs (e.g.
see debugfs_regset32_show).

Johan

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

[Thinh Nguyen]

On Tue, Apr 04, 2023, Johan Hovold wrote:
> On Tue, Apr 04, 2023 at 05:46:13PM +0530, Udipto Goswami wrote:
> > On 4/4/23 5:13 PM, Johan Hovold wrote:
> > > On Tue, Apr 04, 2023 at 01:09:19PM +0200, Oliver Neukum wrote:
> > >> On 04.04.23 13:05, Johan Hovold wrote:
> 
> > >>> This is backwards. If you need the device to be active to access these
> > >>> registers then you should resume it unconditionally instead of failing.
> > > 
> > >> usually you would be right. But this is debugfs. It is intended to observe
> > >> the system in the state it is actually in. If by the act of observation you
> > >> wake up the device, you change the experiment.
> > > 
> > > I admittedly didn't look to closely at what this particular debugfs
> > > interface is used for, but I generally do not agree with that reasoning.
> > > 
> > > The device is being used; by the driver and ultimately by a user telling
> > > the driver to do something on their behalf. The fact that the user is
> > > initiating an action through an interface which intended for debugging
> > > should not matter (and the user always has the option to check the
> > > runtime pm state before initiating the action if that matters at all)
> 
> > for instance, lets say user is trying to dump the value of link_state 
> > register through dwc3_link_state_show, wouldn't a pm_runtime_get would 
> > change the link_state? or I'm assuming it wrong?
> 
> There may be some specific debugfs interfaces (e.g. related to PM) where
> taking the runtime pm state into account makes sense, but then I don't
> think you should return an error if the device happens to be suspended.

Agree here.

> 
> Looking at the dwc3 'link_state', it currently just returns "Not
> available\n" when not in peripheral mode, for example. Perhaps you can
> do something similar if you can neither infer or retrieve the actual
> link state.
> 
> But after skimming the backstory for this patch, you yourself said that
> the motivation for this patch is simply to avoid crashing when accessing
> these interfaces if the device happens to be runtime suspended.
> 
> For that you should just resume the device unconditionally before the
> register accesses as all other drivers do (and such a patch should
> be backported to stable).
> 
> There's no need to take hypothetical PM debugging issues into account to
> address this.
> 

I disagree here. We should not unconditionally resume the device.
Checking these states should not interfere with the device current
operation. These debugfs attributes are meant to provide the user with
debug info. Whether the controller is currently in suspend or not,
that's a data point.

Thanks,
Thinh

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

[Thinh Nguyen]

On Tue, Apr 04, 2023, Johan Hovold wrote:
> On Tue, Apr 04, 2023 at 02:07:22PM +0200, Oliver Neukum wrote:
> > 
> > 
> > On 04.04.23 13:43, Johan Hovold wrote:
> > 
> > > The device is being used; by the driver and ultimately by a user telling
> > 
> > I am afraid that is just an assumption we cannot make. The user may just as
> > well be reading a device state before a device is being used as that may matter.
> 
> It's a perfectly valid assumption to make, and it is was all drivers do
> for debugfs (as well as sysfs). You are the one arguing for making an
> exception, which I don't think is warranted.
> 
> > > the driver to do something on their behalf. The fact that the user is
> > > initiating an action through an interface which intended for debugging
> > > should not matter (and the user always has the option to check the
> > > runtime pm state before initiating the action if that matters at all).
> > 
> > 1. That is a race condition.
> 
> Sure, but you can't have it both ways. Your proposed inverted logic is
> racy as you may or may not get any data.
> 
> > 2. Quite a lot of bugs we are looking at involve power transitions.
> > You just cannot assume that a device will react the same way if it was
> > waken up between events.
> 
> Then just don't use the interface if you for whatever reason don't want
> to wake the device up.
> 

How can we know when to use and when not to use it? We can't just rely
on runtime_status for that. The device can go into suspend any time.

Thanks,
Thinh

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

[Johan Hovold]

On Tue, Apr 04, 2023 at 09:36:28PM +0000, Thinh Nguyen wrote:
> On Tue, Apr 04, 2023, Johan Hovold wrote:
> > On Tue, Apr 04, 2023 at 05:46:13PM +0530, Udipto Goswami wrote:

> > > for instance, lets say user is trying to dump the value of link_state 
> > > register through dwc3_link_state_show, wouldn't a pm_runtime_get would 
> > > change the link_state? or I'm assuming it wrong?
> > 
> > There may be some specific debugfs interfaces (e.g. related to PM) where
> > taking the runtime pm state into account makes sense, but then I don't
> > think you should return an error if the device happens to be suspended.
> 
> Agree here.
> 
> > 
> > Looking at the dwc3 'link_state', it currently just returns "Not
> > available\n" when not in peripheral mode, for example. Perhaps you can
> > do something similar if you can neither infer or retrieve the actual
> > link state.
> > 
> > But after skimming the backstory for this patch, you yourself said that
> > the motivation for this patch is simply to avoid crashing when accessing
> > these interfaces if the device happens to be runtime suspended.
> > 
> > For that you should just resume the device unconditionally before the
> > register accesses as all other drivers do (and such a patch should
> > be backported to stable).
> > 
> > There's no need to take hypothetical PM debugging issues into account to
> > address this.
> > 
> 
> I disagree here. We should not unconditionally resume the device.
> Checking these states should not interfere with the device current
> operation. These debugfs attributes are meant to provide the user with
> debug info. Whether the controller is currently in suspend or not,
> that's a data point.

But this is not what other drivers do, which means that you will end up
with different behaviour with regards to runtime PM (possibly even for
the very same piece of hardware, see below) depending, on which file in
debugfs you access.

See for example commit 30332eeefec8 ("debugfs: regset32: Add Runtime PM
support") which made this behaviour part of the generic debugfs helpers,
and grepping for drivers that bail out from their debugsfs callbacks
does not seem to show any other instances.

Alan did point out though, that the ehci driver returns a string like
"suspended" when trying to access registers for a suspended device.

That behaviour dates back to before the git era though and long before
we had runtime PM. In fact, ehci still does not seem to implement
runtime PM so this check would essentially only kick when the HCD is
dead IIUC.

Notably both the chipidea and musb drivers runtime resume the controller
when accessing registers through debugfs.

I just tried the xhci debugfs interface and it apparently crashes just
like the dwc3 debugfs do if the device is suspended. Turns out no one
has yet wired up the device pointer which would be used by the generic
debugfs helpers to resume the device before register accesses.

I'll send a patch to fix that up.

Johan

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

[Johan Hovold]

On Tue, Apr 04, 2023 at 10:03:09PM +0000, Thinh Nguyen wrote:
> On Tue, Apr 04, 2023, Johan Hovold wrote:
> > On Tue, Apr 04, 2023 at 02:07:22PM +0200, Oliver Neukum wrote:
> > > 
> > > 
> > > On 04.04.23 13:43, Johan Hovold wrote:
> > > 
> > > > The device is being used; by the driver and ultimately by a user telling
> > > 
> > > I am afraid that is just an assumption we cannot make. The user may just as
> > > well be reading a device state before a device is being used as that may matter.
> > 
> > It's a perfectly valid assumption to make, and it is was all drivers do
> > for debugfs (as well as sysfs). You are the one arguing for making an
> > exception, which I don't think is warranted.
> > 
> > > > the driver to do something on their behalf. The fact that the user is
> > > > initiating an action through an interface which intended for debugging
> > > > should not matter (and the user always has the option to check the
> > > > runtime pm state before initiating the action if that matters at all).
> > > 
> > > 1. That is a race condition.
> > 
> > Sure, but you can't have it both ways. Your proposed inverted logic is
> > racy as you may or may not get any data.
> > 
> > > 2. Quite a lot of bugs we are looking at involve power transitions.
> > > You just cannot assume that a device will react the same way if it was
> > > waken up between events.
> > 
> > Then just don't use the interface if you for whatever reason don't want
> > to wake the device up.
> > 
> 
> How can we know when to use and when not to use it? We can't just rely
> on runtime_status for that. The device can go into suspend any time.

You can disable runtime PM through sysfs if you want to avoid prevent
the device from suspending underneath you.

The point is that by having a consistent behaviour for debugfs with
respect to runtime PM you can *always* use it. It doesn't crash and it
does not return random errors based on external events.

Sure, if you are trying to debug some runtime PM related issue, this may
or may not interfere with that, but then you probably need a more
specialised instrumentation to debug that anyway (e.g. dumping state
when you receive wakeup events, etc).

A manually controlled sysfs interface for dumping registers really isn't
enough to debug power-state *transitions*.

That leaves inspection of the suspended state, but the whole reason that
we're having this discussion is that that state is not available to the
driver while suspended in the first place.

Johan

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

[Alan Stern]

On Wed, Apr 05, 2023 at 10:20:45AM +0200, Johan Hovold wrote:
> Alan did point out though, that the ehci driver returns a string like
> "suspended" when trying to access registers for a suspended device.
> 
> That behaviour dates back to before the git era though and long before
> we had runtime PM. In fact, ehci still does not seem to implement
> runtime PM so this check would essentially only kick when the HCD is
> dead IIUC.

In fact there is runtime PM support for ehci-hcd.  You probably missed
it because the routines are defined in core/hcd-pci.c.

Alan Stern

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

[Thinh Nguyen]

On Wed, Apr 05, 2023, Johan Hovold wrote:
> On Tue, Apr 04, 2023 at 10:03:09PM +0000, Thinh Nguyen wrote:
> > On Tue, Apr 04, 2023, Johan Hovold wrote:
> > > On Tue, Apr 04, 2023 at 02:07:22PM +0200, Oliver Neukum wrote:
> > > > 
> > > > 
> > > > On 04.04.23 13:43, Johan Hovold wrote:
> > > > 
> > > > > The device is being used; by the driver and ultimately by a user telling
> > > > 
> > > > I am afraid that is just an assumption we cannot make. The user may just as
> > > > well be reading a device state before a device is being used as that may matter.
> > > 
> > > It's a perfectly valid assumption to make, and it is was all drivers do
> > > for debugfs (as well as sysfs). You are the one arguing for making an
> > > exception, which I don't think is warranted.
> > > 
> > > > > the driver to do something on their behalf. The fact that the user is
> > > > > initiating an action through an interface which intended for debugging
> > > > > should not matter (and the user always has the option to check the
> > > > > runtime pm state before initiating the action if that matters at all).
> > > > 
> > > > 1. That is a race condition.
> > > 
> > > Sure, but you can't have it both ways. Your proposed inverted logic is
> > > racy as you may or may not get any data.
> > > 
> > > > 2. Quite a lot of bugs we are looking at involve power transitions.
> > > > You just cannot assume that a device will react the same way if it was
> > > > waken up between events.
> > > 
> > > Then just don't use the interface if you for whatever reason don't want
> > > to wake the device up.
> > > 
> > 
> > How can we know when to use and when not to use it? We can't just rely
> > on runtime_status for that. The device can go into suspend any time.
> 
> You can disable runtime PM through sysfs if you want to avoid prevent
> the device from suspending underneath you.
> 
> The point is that by having a consistent behaviour for debugfs with
> respect to runtime PM you can *always* use it. It doesn't crash and it
> does not return random errors based on external events.
> 
> Sure, if you are trying to debug some runtime PM related issue, this may
> or may not interfere with that, but then you probably need a more
> specialised instrumentation to debug that anyway (e.g. dumping state
> when you receive wakeup events, etc).

Fair points. I was entertaining the option where this can be used to
poll during runtime PM enabled, but the data may not be useful enough in
actual debug.

> 
> A manually controlled sysfs interface for dumping registers really isn't
> enough to debug power-state *transitions*.
> 
> That leaves inspection of the suspended state, but the whole reason that
> we're having this discussion is that that state is not available to the
> driver while suspended in the first place.
> 

I'm convinced now.

Thanks,
Thinh

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

[Johan Hovold]

On Wed, Apr 05, 2023 at 10:35:45AM -0400, Alan Stern wrote:
> On Wed, Apr 05, 2023 at 10:20:45AM +0200, Johan Hovold wrote:
> > Alan did point out though, that the ehci driver returns a string like
> > "suspended" when trying to access registers for a suspended device.
> > 
> > That behaviour dates back to before the git era though and long before
> > we had runtime PM. In fact, ehci still does not seem to implement
> > runtime PM so this check would essentially only kick when the HCD is
> > dead IIUC.
> 
> In fact there is runtime PM support for ehci-hcd.  You probably missed
> it because the routines are defined in core/hcd-pci.c.

Ah, thanks. I grepped for callers of ehci_suspend, but missed the
indirection via the pci_suspend pointer.

Johan