[PATCH] usb: storage: fix runtime pm issue in usb_stor_probe2

[PATCH] usb: storage: fix runtime pm issue in usb_stor_probe2

[Heiner Kallweit]

Since commit 71723f95463d "PM / runtime: print error when activating a
child to unactive parent" I see the following error message:

scsi host2: usb-storage 1-3:1.0
scsi host2: runtime PM trying to activate child device host2 but parent
	    (1-3:1.0) is not active

Digging into it it seems to be related to the problem described in the
commit message for cd998ded5c12 "i2c: designware: Prevent runtime
suspend during adapter registration" as scsi_add_host also calls
device_add and after the call to device_add the parent device is
suspended.

Fix this by using the approach from the mentioned commit and getting
the runtime pm reference before calling scsi_add_host.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/usb/storage/usb.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index ef2d8cd..8c5f011 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -1070,17 +1070,17 @@ int usb_stor_probe2(struct us_data *us)
 	result = usb_stor_acquire_resources(us);
 	if (result)
 		goto BadDevice;
+	usb_autopm_get_interface_no_resume(us->pusb_intf);
 	snprintf(us->scsi_name, sizeof(us->scsi_name), "usb-storage %s",
 					dev_name(&us->pusb_intf->dev));
 	result = scsi_add_host(us_to_host(us), dev);
 	if (result) {
 		dev_warn(dev,
 				"Unable to add the scsi host\n");
-		goto BadDevice;
+		goto HostAddErr;
 	}
 
 	/* Submit the delayed_work for SCSI-device scanning */
-	usb_autopm_get_interface_no_resume(us->pusb_intf);
 	set_bit(US_FLIDX_SCAN_PENDING, &us->dflags);
 
 	if (delay_use > 0)
@@ -1090,6 +1090,8 @@ int usb_stor_probe2(struct us_data *us)
 	return 0;
 
 	/* We come here if there are any problems */
+HostAddErr:
+	usb_autopm_put_interface_no_suspend(us->pusb_intf);
 BadDevice:
 	usb_stor_dbg(us, "storage_probe() failed\n");
 	release_everything(us);
-- 
2.9.2

Re: [PATCH] usb: storage: fix runtime pm issue in usb_stor_probe2

[Alan Stern]

On Wed, 3 Aug 2016, Heiner Kallweit wrote:

> Since commit 71723f95463d "PM / runtime: print error when activating a
> child to unactive parent" I see the following error message:
> 
> scsi host2: usb-storage 1-3:1.0
> scsi host2: runtime PM trying to activate child device host2 but parent
> 	    (1-3:1.0) is not active
> 
> Digging into it it seems to be related to the problem described in the
> commit message for cd998ded5c12 "i2c: designware: Prevent runtime
> suspend during adapter registration" as scsi_add_host also calls
> device_add and after the call to device_add the parent device is
> suspended.
> 
> Fix this by using the approach from the mentioned commit and getting
> the runtime pm reference before calling scsi_add_host.

Acked-by: Alan Stern <stern@rowland.harvard.edu>

There's nothing wrong with this patch; it's a worthwhile thing to do 
because it can prevent an unnecessary runtime-suspend/resume cycle.

Still, it looks like the real problem here may lie in
drivers/scsi/hosts.c.  Commit bc4f24014de5 ("[SCSI] implement runtime
Power Management") added a call to pm_runtime_set_active() in
scsi_add_host_with_dma() _after_ device_add() instead of _before_.

If that were changed, this problem would not have occurred.

Alan Stern

Re: [PATCH] usb: storage: fix runtime pm issue in usb_stor_probe2

[Heiner Kallweit]

Am 03.08.2016 um 23:25 schrieb Alan Stern:
> On Wed, 3 Aug 2016, Heiner Kallweit wrote:
> 
>> Since commit 71723f95463d "PM / runtime: print error when activating a
>> child to unactive parent" I see the following error message:
>>
>> scsi host2: usb-storage 1-3:1.0
>> scsi host2: runtime PM trying to activate child device host2 but parent
>> 	    (1-3:1.0) is not active
>>
>> Digging into it it seems to be related to the problem described in the
>> commit message for cd998ded5c12 "i2c: designware: Prevent runtime
>> suspend during adapter registration" as scsi_add_host also calls
>> device_add and after the call to device_add the parent device is
>> suspended.
>>
>> Fix this by using the approach from the mentioned commit and getting
>> the runtime pm reference before calling scsi_add_host.
> 
> Acked-by: Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>
> 
> There's nothing wrong with this patch; it's a worthwhile thing to do 
> because it can prevent an unnecessary runtime-suspend/resume cycle.
> 
> Still, it looks like the real problem here may lie in
> drivers/scsi/hosts.c.  Commit bc4f24014de5 ("[SCSI] implement runtime
> Power Management") added a call to pm_runtime_set_active() in
> scsi_add_host_with_dma() _after_ device_add() instead of _before_.
> 
> If that were changed, this problem would not have occurred.
> 
In parallel to this patch I sent another one to address exactly the
ordering issue in scsi_add_host_with_dma you mention.
The other patch went to Martin and the SCSI mailing list only, sorry.
I'll forward it to you for review.

Heiner

> Alan Stern
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html