rtsx_usb: runtime PM fixes before tray-reader detect rework
Subsystems (3)
cover rtsx_usb: runtime PM fixes before tray-reader detect rework
This is an RFC for the USB/cardreader side of the rtsx tray-reader issue.
Assuming the separate resume-reset fix has landed, this is the next step:
clean up runtime PM handling in the USB/cardreader layer before attempting any further MMC-side detect changes.
Patch 1 holds a runtime PM reference across transfers and marks the device busy afterwards.
Patch 2 avoids issuing USB register I/O from the runtime autosuspend path, which can deadlock runtime PM and leave the device stuck in a suspending state.
The remaining tray-reader false-detect problem likely needs event qualification on the USB/cardreader side rather than MMC command probing from ->get_cd(). Guidance on the right place for that logic would be useful.
Sean Rhodes (2):
rtsx_usb: hold runtime PM during transfers
rtsx_usb: avoid USB I/O in runtime autosuspend
drivers/misc/cardreader/rtsx_usb.c | 49 +++++++++++++++++++++++------- include/linux/rtsx_usb.h | 3 ++ 2 files changed, 41 insertions(+), 11 deletions(-)
RFC PATCH 1/2 rtsx_usb: hold runtime PM during transfers
Hold a runtime-PM reference across bulk transfers, and mark the device busy afterwards.
When runtime PM is already in progress (e.g. from rtsx_usb_suspend()), avoid forcing a runtime resume from within the PM path by using usb_autopm_get_interface_no_resume() unless the interface is already runtime-suspended.
--- drivers/misc/cardreader/rtsx_usb.c | 38 ++++++++++++++++++++++++------ 1 file changed, 31 insertions(+), 7 deletions(-)
drivers/misc/cardreader/rtsx_usb.c +31 -7
@@ -12,6 +12,7 @@#include <linux/usb.h> #include <linux/platform_device.h> #include <linux/mfd/core.h> +#include <linux/pm_runtime.h> #include <linux/rtsx_usb.h> static int polling_pipe =1 ;
@@ -65,19 +66,42 @@ static int rtsx_usb_bulk_transfer_sglist(struct rtsx_ucr *ucr, }int rtsx_usb_transfer_data (struct rtsx_ucr *ucr,unsigned int pipe, -void *buf,unsigned int len,int num_sg, -unsigned int *act_len,int timeout) +void *buf,unsigned int len,int num_sg, +unsigned int *act_len,int timeout) { +int ret; +struct device *dev = &ucr->pusb_intf->dev; +if (timeout <600 ) timeout =600 ; +/* +* During runtime suspend/resume callbacks, avoid forcing a runtime resume +* from within the PM path. The device is still active when +* rtsx_usb_suspend() runs, but usb_autopm_get_interface() can block when +* runtime PM is already in progress. +*/ +if (pm_runtime_status_suspended (dev)) { + ret =usb_autopm_get_interface (ucr->pusb_intf); + }else { +usb_autopm_get_interface_no_resume (ucr->pusb_intf); + ret =0 ; + } +if (ret) +return ret; +if (num_sg) -return rtsx_usb_bulk_transfer_sglist (ucr, pipe, - (struct scatterlist *)buf, num_sg, len, act_len, - timeout); + ret =rtsx_usb_bulk_transfer_sglist (ucr, pipe, + (struct scatterlist *)buf, + num_sg, len, act_len, + timeout);else -return usb_bulk_msg (ucr->pusb_dev, pipe, buf, len, act_len, - timeout); + ret =usb_bulk_msg (ucr->pusb_dev, pipe, buf, len, act_len, + timeout); + +usb_mark_last_busy (ucr->pusb_dev); +usb_autopm_put_interface (ucr->pusb_intf); +return ret; }EXPORT_SYMBOL_GPL (rtsx_usb_transfer_data);
Re: [RFC PATCH 1/2] rtsx_usb: hold runtime PM during transfers
Quoted 3 lines · Sean Rhodes
Hold a runtime-PM reference across bulk transfers, and mark the device busy afterwards.
Why isn't it sufficient to let the mmc host device (child device) and its corresponding driver to manage runtime PM reference counting?
As soon as the mmc host driver is requested to power-on the card via its ->ios() callback (set to sdmmc_set_ios() in drivers/mmc/host/rtsx_usb_sdmmc.c, it bumps the runtime PM usage count. Therefore, the usb device should be prevented from being runtime suspended as long as a card is inserted. Isn't that sufficient?
Quoted 5 lines
When runtime PM is already in progress (e.g. from
rtsx_usb_suspend()), avoid forcing a runtime resume from within the PM path by usingusb_autopm_get_interface_no_resume()unless the interface is already runtime-suspended.
Why is this needed? What problem does it solve?
Quoted 2 lines
Signed-off-by Sean Rhodes <[email protected]>
Kind regards
Uffe
Quoted 66 lines
--- drivers/misc/cardreader/rtsx_usb.c | 38 ++++++++++++++++++++++++------ 1 file changed, 31 insertions(+), 7 deletions(-)drivers/misc/cardreader/rtsx_usb.c +31
-7@@ -12,6 +12,7 @@#include <linux/usb.h> #include <linux/platform_device.h> #include <linux/mfd/core.h> +#include <linux/pm_runtime.h> #include <linux/rtsx_usb.h> static int polling_pipe =1 ;@@ -65,19 +66,42 @@ static int rtsx_usb_bulk_transfer_sglist(struct rtsx_ucr *ucr, }int rtsx_usb_transfer_data (struct rtsx_ucr *ucr,unsigned int pipe, -void *buf,unsigned int len,int num_sg, -unsigned int *act_len,int timeout) +void *buf,unsigned int len,int num_sg, +unsigned int *act_len,int timeout) { +int ret; +struct device *dev = &ucr->pusb_intf->dev; +if (timeout <600 ) timeout =600 ; +/* +* During runtime suspend/resume callbacks, avoid forcing a runtime resume +* from within the PM path. The device is still active when +* rtsx_usb_suspend() runs, but usb_autopm_get_interface() can block when +* runtime PM is already in progress. +*/ +if (pm_runtime_status_suspended (dev)) { + ret =usb_autopm_get_interface (ucr->pusb_intf); + }else { +usb_autopm_get_interface_no_resume (ucr->pusb_intf); + ret =0 ; + } +if (ret) +return ret; +if (num_sg) -return rtsx_usb_bulk_transfer_sglist (ucr, pipe, - (struct scatterlist *)buf, num_sg, len, act_len, - timeout); + ret =rtsx_usb_bulk_transfer_sglist (ucr, pipe, + (struct scatterlist *)buf, + num_sg, len, act_len, + timeout);else -return usb_bulk_msg (ucr->pusb_dev, pipe, buf, len, act_len, - timeout); + ret =usb_bulk_msg (ucr->pusb_dev, pipe, buf, len, act_len, + timeout); + +usb_mark_last_busy (ucr->pusb_dev); +usb_autopm_put_interface (ucr->pusb_intf); +return ret; }EXPORT_SYMBOL_GPL (rtsx_usb_transfer_data);
Re: [RFC PATCH 1/2] rtsx_usb: hold runtime PM during transfers
In reply to Sean RhodesQuoted 5 lines · Sean Rhodes
Hold a runtime-PM reference across bulk transfers, and mark the device busy afterwards.
When runtime PM is already in progress (e.g. from
rtsx_usb_suspend()),
What do you mean by "runtime PM is already in progress"? Is the device in the process of being runtime resumed?
Quoted 29 lines
avoid forcing a runtime resume from within the PM path by using
usb_autopm_get_interface_no_resume()unless the interface is already runtime-suspended.Signed-off-by Sean Rhodes <[email protected]>--- drivers/misc/cardreader/rtsx_usb.c | 38 ++++++++++++++++++++++++------ 1 file changed, 31 insertions(+), 7 deletions(-)drivers/misc/cardreader/rtsx_usb.c +3
-2@@ -12,6 +12,7 @@#include <linux/usb.h> #include <linux/platform_device.h> #include <linux/mfd/core.h> +#include <linux/pm_runtime.h> #include <linux/rtsx_usb.h> static int polling_pipe =1 ;@@ -65,19 +66,42 @@ static int rtsx_usb_bulk_transfer_sglist(struct rtsx_ucr *ucr, }int rtsx_usb_transfer_data (struct rtsx_ucr *ucr,unsigned int pipe, -void *buf,unsigned int len,int num_sg, -unsigned int *act_len,int timeout) +void *buf,unsigned int len,int num_sg, +unsigned int *act_len,int timeout)
drivers/misc/cardreader/rtsx_usb.c static int rtsx_usb_bulk_transfer_sglist(struct rtsx_ucr *ucr, · line 80
Quoted 10 lines
drivers/misc/cardreader/rtsx_usb.c +6
-0{ +int ret; +struct device *dev = &ucr->pusb_intf->dev; +if (timeout <600 ) timeout =600 ; +/* +* During runtime suspend/resume callbacks, avoid forcing a runtime resume +* from within the PM path. The device is still active when
Sorry, but I don't understand the above. Can you please elaborate?
drivers/misc/cardreader/rtsx_usb.c static int rtsx_usb_bulk_transfer_sglist(struct rtsx_ucr *ucr, · line 84
Quoted 4 lines
drivers/misc/cardreader/rtsx_usb.c +4
-0+* rtsx_usb_suspend() runs, but usb_autopm_get_interface() can block when +* runtime PM is already in progress. +*/ +if (pm_runtime_status_suspended (dev)) {
This looks odd to me. Should it not be the responsibility of the caller to make sure the device's runtime is resumed before this function gets called?
Quoted 17 lines
drivers/misc/cardreader/rtsx_usb.c +12
-3+ ret =usb_autopm_get_interface (ucr->pusb_intf); + }else { +usb_autopm_get_interface_no_resume (ucr->pusb_intf); + ret =0 ; + } +if (ret) +return ret; +if (num_sg) -return rtsx_usb_bulk_transfer_sglist (ucr, pipe, - (struct scatterlist *)buf, num_sg, len, act_len, - timeout); + ret =rtsx_usb_bulk_transfer_sglist (ucr, pipe, + (struct scatterlist *)buf, + num_sg, len, act_len, + timeout);else
drivers/misc/cardreader/rtsx_usb.c static int rtsx_usb_bulk_transfer_sglist(struct rtsx_ucr *ucr, · line 106
Quoted 10 lines
drivers/misc/cardreader/rtsx_usb.c +6
-2-return usb_bulk_msg (ucr->pusb_dev, pipe, buf, len, act_len, - timeout); + ret =usb_bulk_msg (ucr->pusb_dev, pipe, buf, len, act_len, + timeout); + +usb_mark_last_busy (ucr->pusb_dev); +usb_autopm_put_interface (ucr->pusb_intf); +return ret; }EXPORT_SYMBOL_GPL (rtsx_usb_transfer_data);
Kind regards
Uffe
RFC PATCH 2/2 rtsx_usb: avoid USB I/O in runtime autosuspend
In reply to Sean RhodesThe runtime autosuspend callback currently queries card status and clears OCP via USB register accesses.
On some systems this can deadlock runtime PM and leave the USB device stuck in the "suspending" state.
Avoid issuing USB commands from the runtime autosuspend callback. Instead, use the last cached card status (updated on successful status reads) and conservatively assume a card may be present until status is known.
--- drivers/misc/cardreader/rtsx_usb.c | 11 +++++++---- include/linux/rtsx_usb.h | 3 +++ 2 files changed, 10 insertions(+), 4 deletions(-)
drivers/misc/cardreader/rtsx_usb.c +7 -4
@@ -336,6 +336,9 @@ int rtsx_usb_get_card_status(struct rtsx_ucr *ucr, u16 *status)if (ret <0 )return ret; + ucr->card_status_cache = *status; + ucr->card_status_valid = true; +return 0 ; }EXPORT_SYMBOL_GPL (rtsx_usb_get_card_status);
@@ -753,16 +756,16 @@ static int rtsx_usb_suspend(struct usb_interface *intf, pm_message_t message)if (PMSG_IS_AUTO (message)) {if (mutex_trylock (&ucr->dev_mutex)) { -rtsx_usb_get_card_status (ucr, &val); +if (ucr->card_status_valid) + val = ucr->card_status_cache; +else + val =SD_CD |MS_CD ;mutex_unlock (&ucr->dev_mutex);/* Defer the autosuspend if card exists */ if (val & (SD_CD |MS_CD )) {device_for_each_child (&intf->dev,NULL , rtsx_usb_resume_child);return -EAGAIN ; - }else { -/* if the card does not exists, clear OCP status */ -rtsx_usb_write_register (ucr,OCPCTL ,MS_OCP_CLEAR ,MS_OCP_CLEAR ); } }else {/* There is an ongoing operation*/
include/linux/rtsx_usb.h +3 -0
@@ -62,6 +62,9 @@ struct rtsx_ucr {struct usb_interface *pusb_intf;struct usb_sg_request current_sg; +u16 card_status_cache; +bool card_status_valid; +struct timer_list sg_timer;struct mutex dev_mutex; };