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

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

Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com>
---
v3: Replace pr_err to dev_err.

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

diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
index 850df0e6bcab..4a9d0994f3b4 100644
--- a/drivers/usb/dwc3/debugfs.c
+++ b/drivers/usb/dwc3/debugfs.c
@@ -544,6 +544,12 @@ static int dwc3_link_state_show(struct seq_file *s, void *unused)
 	u32			reg;
 	u8			speed;
 
+	if (pm_runtime_suspended(dwc->dev)) {
+		dev_err(dwc->dev,
+			"Invalid operation, DWC3 suspended!");
+		return -EINVAL;
+	}
+
 	spin_lock_irqsave(&dwc->lock, flags);
 	reg = dwc3_readl(dwc->regs, DWC3_GSTS);
 	if (DWC3_GSTS_CURMOD(reg) != DWC3_GSTS_CURMOD_DEVICE) {
@@ -580,6 +586,12 @@ static ssize_t dwc3_link_state_write(struct file *file,
 	u32			reg;
 	u8			speed;
 
+	if (pm_runtime_suspended(dwc->dev)) {
+		dev_err(dwc->dev,
+			"Invalid operation, DWC3 suspended!");
+		return -EINVAL;
+	}
+
 	if (copy_from_user(&buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
 		return -EFAULT;
 
@@ -641,6 +653,12 @@ static int dwc3_tx_fifo_size_show(struct seq_file *s, void *unused)
 	u32			mdwidth;
 	u32			val;
 
+	if (pm_runtime_suspended(dwc->dev)) {
+		dev_err(dwc->dev,
+			"Invalid operation, DWC3 suspended!");
+		return -EINVAL;
+	}
+
 	spin_lock_irqsave(&dwc->lock, flags);
 	val = dwc3_core_fifo_space(dep, DWC3_TXFIFO);
 
@@ -663,6 +681,12 @@ static int dwc3_rx_fifo_size_show(struct seq_file *s, void *unused)
 	u32			mdwidth;
 	u32			val;
 
+	if (pm_runtime_suspended(dwc->dev)) {
+		dev_err(dwc->dev,
+			"Invalid operation, DWC3 suspended!");
+		return -EINVAL;
+	}
+
 	spin_lock_irqsave(&dwc->lock, flags);
 	val = dwc3_core_fifo_space(dep, DWC3_RXFIFO);
 
@@ -684,6 +708,12 @@ static int dwc3_tx_request_queue_show(struct seq_file *s, void *unused)
 	unsigned long		flags;
 	u32			val;
 
+	if (pm_runtime_suspended(dwc->dev)) {
+		dev_err(dwc->dev,
+			"Invalid operation, DWC3 suspended!");
+		return -EINVAL;
+	}
+
 	spin_lock_irqsave(&dwc->lock, flags);
 	val = dwc3_core_fifo_space(dep, DWC3_TXREQQ);
 	seq_printf(s, "%u\n", val);
@@ -699,6 +729,12 @@ static int dwc3_rx_request_queue_show(struct seq_file *s, void *unused)
 	unsigned long		flags;
 	u32			val;
 
+	if (pm_runtime_suspended(dwc->dev)) {
+		dev_err(dwc->dev,
+			"Invalid operation, DWC3 suspended!");
+		return -EINVAL;
+	}
+
 	spin_lock_irqsave(&dwc->lock, flags);
 	val = dwc3_core_fifo_space(dep, DWC3_RXREQQ);
 	seq_printf(s, "%u\n", val);
@@ -714,6 +750,12 @@ static int dwc3_rx_info_queue_show(struct seq_file *s, void *unused)
 	unsigned long		flags;
 	u32			val;
 
+	if (pm_runtime_suspended(dwc->dev)) {
+		dev_err(dwc->dev,
+			"Invalid operation, DWC3 suspended!");
+		return -EINVAL;
+	}
+
 	spin_lock_irqsave(&dwc->lock, flags);
 	val = dwc3_core_fifo_space(dep, DWC3_RXINFOQ);
 	seq_printf(s, "%u\n", val);
@@ -729,6 +771,12 @@ static int dwc3_descriptor_fetch_queue_show(struct seq_file *s, void *unused)
 	unsigned long		flags;
 	u32			val;
 
+	if (pm_runtime_suspended(dwc->dev)) {
+		dev_err(dwc->dev,
+			"Invalid operation, DWC3 suspended!");
+		return -EINVAL;
+	}
+
 	spin_lock_irqsave(&dwc->lock, flags);
 	val = dwc3_core_fifo_space(dep, DWC3_DESCFETCHQ);
 	seq_printf(s, "%u\n", val);
@@ -744,6 +792,12 @@ static int dwc3_event_queue_show(struct seq_file *s, void *unused)
 	unsigned long		flags;
 	u32			val;
 
+	if (pm_runtime_suspended(dwc->dev)) {
+		dev_err(dwc->dev,
+			"Invalid operation, DWC3 suspended!");
+		return -EINVAL;
+	}
+
 	spin_lock_irqsave(&dwc->lock, flags);
 	val = dwc3_core_fifo_space(dep, DWC3_EVENTQ);
 	seq_printf(s, "%u\n", val);
@@ -835,6 +889,12 @@ static int dwc3_ep_info_register_show(struct seq_file *s, void *unused)
 	u32			upper_32_bits;
 	u32			reg;
 
+	if (pm_runtime_suspended(dwc->dev)) {
+		dev_err(dwc->dev,
+			"Invalid operation, DWC3 suspended!");
+		return -EINVAL;
+	}
+
 	spin_lock_irqsave(&dwc->lock, flags);
 	reg = DWC3_GDBGLSPMUX_EPSELECT(dep->number);
 	dwc3_writel(dwc->regs, DWC3_GDBGLSPMUX, reg);
-- 
2.17.1

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

[Greg Kroah-Hartman]

On Thu, Mar 16, 2023 at 12:28:58PM +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: Udipto Goswami <quic_ugoswami@quicinc.com>
> ---
> v3: Replace pr_err to dev_err.
> 
>  drivers/usb/dwc3/debugfs.c | 60 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
> index 850df0e6bcab..4a9d0994f3b4 100644
> --- a/drivers/usb/dwc3/debugfs.c
> +++ b/drivers/usb/dwc3/debugfs.c
> @@ -544,6 +544,12 @@ static int dwc3_link_state_show(struct seq_file *s, void *unused)
>  	u32			reg;
>  	u8			speed;
>  
> +	if (pm_runtime_suspended(dwc->dev)) {
> +		dev_err(dwc->dev,
> +			"Invalid operation, DWC3 suspended!");
> +		return -EINVAL;
> +	}

What prevents the device from being suspened right after you check this?

thanks,

greg k-h

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

[Oliver Neukum]

[-- Attachment #1: Type: text/plain, Size: 1455 bytes --]



On 16.03.23 12:17, Greg Kroah-Hartman wrote:
> On Thu, Mar 16, 2023 at 12:28:58PM +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: Udipto Goswami <quic_ugoswami@quicinc.com>
>> ---
>> v3: Replace pr_err to dev_err.
>>
>>   drivers/usb/dwc3/debugfs.c | 60 ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 60 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
>> index 850df0e6bcab..4a9d0994f3b4 100644
>> --- a/drivers/usb/dwc3/debugfs.c
>> +++ b/drivers/usb/dwc3/debugfs.c
>> @@ -544,6 +544,12 @@ static int dwc3_link_state_show(struct seq_file *s, void *unused)
>>   	u32			reg;
>>   	u8			speed;
>>   
>> +	if (pm_runtime_suspended(dwc->dev)) {
>> +		dev_err(dwc->dev,
>> +			"Invalid operation, DWC3 suspended!");
>> +		return -EINVAL;
>> +	}
> 
> What prevents the device from being suspened right after you check this?

Indeed. If you really need a semantics of not waking up the device if
somebody reads through debugfs, the attached patch should do the job.
But do you really need it?

	Regards
		Oliver

[-- Attachment #2: 0001-dwc3-debugfs-no-register-access-if-suspended.patch --]
[-- Type: text/x-patch, Size: 1681 bytes --]

From 86454823b682a9b2414c0bbb45c3ed2f9d0a1f5f Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.com>
Date: Thu, 16 Mar 2023 12:32:55 +0100
Subject: [PATCH] dwc3: debugfs: no register access if suspended

Preventing register access while suspended and
prevent suspension during access via debugfs

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/usb/dwc3/debugfs.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
index 850df0e6bcab..6922076572cd 100644
--- a/drivers/usb/dwc3/debugfs.c
+++ b/drivers/usb/dwc3/debugfs.c
@@ -543,13 +543,19 @@ static int dwc3_link_state_show(struct seq_file *s, void *unused)
 	enum dwc3_link_state	state;
 	u32			reg;
 	u8			speed;
+	int			ret = 0;
 
+	if (!pm_runtime_get_if_in_use(dwc->dev)) {
+		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 +565,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)
-- 
2.39.2

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

[Udipto Goswami]

Hi Greg, Oliver,

On 3/16/23 5:06 PM, Oliver Neukum wrote:
> 
> 
> On 16.03.23 12:17, Greg Kroah-Hartman wrote:
>> On Thu, Mar 16, 2023 at 12:28:58PM +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: Udipto Goswami <quic_ugoswami@quicinc.com>
>>> ---
>>> v3: Replace pr_err to dev_err.
>>>
>>>   drivers/usb/dwc3/debugfs.c | 60 ++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 60 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
>>> index 850df0e6bcab..4a9d0994f3b4 100644
>>> --- a/drivers/usb/dwc3/debugfs.c
>>> +++ b/drivers/usb/dwc3/debugfs.c
>>> @@ -544,6 +544,12 @@ static int dwc3_link_state_show(struct seq_file 
>>> *s, void *unused)
>>>       u32            reg;
>>>       u8            speed;
>>> +    if (pm_runtime_suspended(dwc->dev)) {
>>> +        dev_err(dwc->dev,
>>> +            "Invalid operation, DWC3 suspended!");
>>> +        return -EINVAL;
>>> +    }
>>
>> What prevents the device from being suspened right after you check this?

I agree if the debugfs is access while suspend is in progress and lets 
say the pm suspended status got reflected after the check. In this case 
we will still run into the same fatal error scenario. But this will be 
very tight race if in my understanding.

Our scenario was very simple, plug out the cable and try accessing the 
debugfs. In this scenario at least the kernel should not crash.

> 
> Indeed. If you really need a semantics of not waking up the device if
> somebody reads through debugfs, the attached patch should do the job.
> But do you really need it?

Well, the intention was just to avoid the kernel crash. I believe in any 
case the kernel should handle it gracefully and bail out. I understand 
this patch won't help in the race condition which Greg pointed out though.

Could you please suggest any other way which we can try out ?
> 
>      Regards
>          Oliver

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

[Udipto Goswami]

Hi Oliver,

On 3/17/23 5:26 PM, Udipto Goswami wrote:
> Hi Greg, Oliver,
> 
> On 3/16/23 5:06 PM, Oliver Neukum wrote:
>>
>>
>> On 16.03.23 12:17, Greg Kroah-Hartman wrote:
>>> On Thu, Mar 16, 2023 at 12:28:58PM +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: Udipto Goswami <quic_ugoswami@quicinc.com>
>>>> ---
>>>> v3: Replace pr_err to dev_err.
>>>>
>>>>   drivers/usb/dwc3/debugfs.c | 60 
>>>> ++++++++++++++++++++++++++++++++++++++
>>>>   1 file changed, 60 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
>>>> index 850df0e6bcab..4a9d0994f3b4 100644
>>>> --- a/drivers/usb/dwc3/debugfs.c
>>>> +++ b/drivers/usb/dwc3/debugfs.c
>>>> @@ -544,6 +544,12 @@ static int dwc3_link_state_show(struct seq_file 
>>>> *s, void *unused)
>>>>       u32            reg;
>>>>       u8            speed;
>>>> +    if (pm_runtime_suspended(dwc->dev)) {
>>>> +        dev_err(dwc->dev,
>>>> +            "Invalid operation, DWC3 suspended!");
>>>> +        return -EINVAL;
>>>> +    }
>>>
>>> What prevents the device from being suspened right after you check this?
> 
> I agree if the debugfs is access while suspend is in progress and lets 
> say the pm suspended status got reflected after the check. In this case 
> we will still run into the same fatal error scenario. But this will be 
> very tight race if in my understanding.
> 
> Our scenario was very simple, plug out the cable and try accessing the 
> debugfs. In this scenario at least the kernel should not crash.
> 
>>
>> Indeed. If you really need a semantics of not waking up the device if
>> somebody reads through debugfs, the attached patch should do the job.
>> But do you really need it?
> 
> Well, the intention was just to avoid the kernel crash. I believe in any 
> case the kernel should handle it gracefully and bail out. I understand 
> this patch won't help in the race condition which Greg pointed out though.
> 
> Could you please suggest any other way which we can try out ?
>>
>>      Regards
>>          Oliver

Apologies for the delay,
I checked with the patch you had attached,

  drivers/usb/dwc3/debugfs.c | 16 ++++++++++++----
  1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
index 850df0e6bcab..6922076572cd 100644
--- a/drivers/usb/dwc3/debugfs.c
+++ b/drivers/usb/dwc3/debugfs.c
@@ -543,13 +543,19 @@ static int dwc3_link_state_show(struct seq_file 
*s, void *unused)
  	enum dwc3_link_state	state;
  	u32			reg;
  	u8			speed;
+	int			ret = 0;

+	if (!pm_runtime_get_if_in_use(dwc->dev)) {
+		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 +565,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;
  }


This works for my usecase. Tested it and I don't see any fatal errors.
Can we consider mainlining it ? We will need to cover the other 
functions as well in similar way. Can I proceed and make the change and 
include your Suggested-by/Signed-off-by tag ?
Or do you want to push this on a separate one ?

Thanks,
-Udipto

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

[Udipto Goswami]

On 3/30/23 11:59 AM, Udipto Goswami wrote:
> Hi Oliver,
> 
> On 3/17/23 5:26 PM, Udipto Goswami wrote:
>> Hi Greg, Oliver,
>>
>> On 3/16/23 5:06 PM, Oliver Neukum wrote:
>>>
>>>
>>> On 16.03.23 12:17, Greg Kroah-Hartman wrote:
>>>> On Thu, Mar 16, 2023 at 12:28:58PM +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: Udipto Goswami <quic_ugoswami@quicinc.com>
>>>>> ---
>>>>> v3: Replace pr_err to dev_err.
>>>>>
>>>>>   drivers/usb/dwc3/debugfs.c | 60 
>>>>> ++++++++++++++++++++++++++++++++++++++
>>>>>   1 file changed, 60 insertions(+)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
>>>>> index 850df0e6bcab..4a9d0994f3b4 100644
>>>>> --- a/drivers/usb/dwc3/debugfs.c
>>>>> +++ b/drivers/usb/dwc3/debugfs.c
>>>>> @@ -544,6 +544,12 @@ static int dwc3_link_state_show(struct 
>>>>> seq_file *s, void *unused)
>>>>>       u32            reg;
>>>>>       u8            speed;
>>>>> +    if (pm_runtime_suspended(dwc->dev)) {
>>>>> +        dev_err(dwc->dev,
>>>>> +            "Invalid operation, DWC3 suspended!");
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>
>>>> What prevents the device from being suspened right after you check 
>>>> this?
>>
>> I agree if the debugfs is access while suspend is in progress and lets 
>> say the pm suspended status got reflected after the check. In this 
>> case we will still run into the same fatal error scenario. But this 
>> will be very tight race if in my understanding.
>>
>> Our scenario was very simple, plug out the cable and try accessing the 
>> debugfs. In this scenario at least the kernel should not crash.
>>
>>>
>>> Indeed. If you really need a semantics of not waking up the device if
>>> somebody reads through debugfs, the attached patch should do the job.
>>> But do you really need it?
>>
>> Well, the intention was just to avoid the kernel crash. I believe in 
>> any case the kernel should handle it gracefully and bail out. I 
>> understand this patch won't help in the race condition which Greg 
>> pointed out though.
>>
>> Could you please suggest any other way which we can try out ?
>>>
>>>      Regards
>>>          Oliver
> 
> Apologies for the delay,
> I checked with the patch you had attached,
> 
>   drivers/usb/dwc3/debugfs.c | 16 ++++++++++++----
>   1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
> index 850df0e6bcab..6922076572cd 100644
> --- a/drivers/usb/dwc3/debugfs.c
> +++ b/drivers/usb/dwc3/debugfs.c
> @@ -543,13 +543,19 @@ static int dwc3_link_state_show(struct seq_file 
> *s, void *unused)
>       enum dwc3_link_state    state;
>       u32            reg;
>       u8            speed;
> +    int            ret = 0;
> 
> +    if (!pm_runtime_get_if_in_use(dwc->dev)) {

Hi Oliver,

I think we need a little tweak here because as per this function's 
description [1]:

int pm_runtime_get_if_in_use(struct device *dev);

- return -EINVAL if 'power.disable_depth' is nonzero; otherwise, if the 
runtime PM status is RPM_ACTIVE and the runtime PM usage counter is 
nonzero, increment the counter and return 1; otherwise return 0 without 
changing the counter

if it returns -EINVAL i think we still will go ahead and try accessing 
the rest of the code.

How about something like this ?



-       if (!pm_runtime_get_if_in_use(dwc->dev)) {
+       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!");

> +        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 +565,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;
>   }
> 
> 
> This works for my usecase. Tested it and I don't see any fatal errors.
> Can we consider mainlining it ? We will need to cover the other 
> functions as well in similar way. Can I proceed and make the change and 
> include your Suggested-by/Signed-off-by tag ?
> Or do you want to push this on a separate one ?
> 
> Thanks,
> -Udipto

[1]: https://www.kernel.org/doc/Documentation/power/runtime_pm.txt

Thanks,
-Udipto