Re: [PATCH] misc: rtsx: usb: Ensure mmc child device is active when card is present

Re: [PATCH] misc: rtsx: usb: Ensure mmc child device is active when card is present

[Gwendal Grignou]

> ---
>  drivers/misc/cardreader/rtsx_usb.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/misc/cardreader/rtsx_usb.c b/drivers/misc/cardreader/rtsx_usb.c
> index 148107a4547c..d007a4455ce5 100644
> --- a/drivers/misc/cardreader/rtsx_usb.c
> +++ b/drivers/misc/cardreader/rtsx_usb.c
> @@ -698,6 +698,12 @@ static void rtsx_usb_disconnect(struct usb_interface *intf)
>  }
>
>  #ifdef CONFIG_PM
> +static int rtsx_usb_resume_child(struct device *dev, void *data)
> +{
> +       pm_request_resume(dev);
> +       return 0;
> +}
> +
>  static int rtsx_usb_suspend(struct usb_interface *intf, pm_message_t message)
>  {
>         struct rtsx_ucr *ucr =
> @@ -713,8 +719,10 @@ static int rtsx_usb_suspend(struct usb_interface *intf, pm_message_t message)
>                         mutex_unlock(&ucr->dev_mutex);
>
>                         /* Defer the autosuspend if card exists */
> -                       if (val & (SD_CD | MS_CD))
> +                       if (val & (SD_CD | MS_CD)) {
> +                               device_for_each_child(&intf->dev, NULL, rtsx_usb_resume_child);
Why not calling rtsx_usb_resume() here?
>                                 return -EAGAIN;
> +                       }
>                 } else {
>                         /* There is an ongoing operation*/
>                         return -EAGAIN;
> @@ -724,12 +732,6 @@ static int rtsx_usb_suspend(struct usb_interface *intf, pm_message_t message)
>         return 0;
>  }
>
> -static int rtsx_usb_resume_child(struct device *dev, void *data)
> -{
> -       pm_request_resume(dev);
> -       return 0;
> -}
> -
>  static int rtsx_usb_resume(struct usb_interface *intf)
>  {
>         device_for_each_child(&intf->dev, NULL, rtsx_usb_resume_child);
> --
> 2.25.1
>

RE: [PATCH] misc: rtsx: usb: Ensure mmc child device is active when card is present

[Ricky WU]

> > ---
> >  drivers/misc/cardreader/rtsx_usb.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/misc/cardreader/rtsx_usb.c
> b/drivers/misc/cardreader/rtsx_usb.c
> > index 148107a4547c..d007a4455ce5 100644
> > --- a/drivers/misc/cardreader/rtsx_usb.c
> > +++ b/drivers/misc/cardreader/rtsx_usb.c
> > @@ -698,6 +698,12 @@ static void rtsx_usb_disconnect(struct usb_interface
> *intf)
> >  }
> >
> >  #ifdef CONFIG_PM
> > +static int rtsx_usb_resume_child(struct device *dev, void *data)
> > +{
> > +       pm_request_resume(dev);
> > +       return 0;
> > +}
> > +
> >  static int rtsx_usb_suspend(struct usb_interface *intf, pm_message_t
> message)
> >  {
> >         struct rtsx_ucr *ucr =
> > @@ -713,8 +719,10 @@ static int rtsx_usb_suspend(struct usb_interface
> *intf, pm_message_t message)
> >                         mutex_unlock(&ucr->dev_mutex);
> >
> >                         /* Defer the autosuspend if card exists */
> > -                       if (val & (SD_CD | MS_CD))
> > +                       if (val & (SD_CD | MS_CD)) {
> > +                               device_for_each_child(&intf->dev,
> NULL, rtsx_usb_resume_child);
> Why not calling rtsx_usb_resume() here?

Because in this time rtsx_usb is not in runtime_suspend, only need to make sure child is not in suspend
Actually when the program came here this suspend will be rejected because return -EAGAIN

> >                                 return -EAGAIN;
> > +                       }
> >                 } else {
> >                         /* There is an ongoing operation*/
> >                         return -EAGAIN;
> > @@ -724,12 +732,6 @@ static int rtsx_usb_suspend(struct usb_interface
> *intf, pm_message_t message)
> >         return 0;
> >  }
> >
> > -static int rtsx_usb_resume_child(struct device *dev, void *data)
> > -{
> > -       pm_request_resume(dev);
> > -       return 0;
> > -}
> > -
> >  static int rtsx_usb_resume(struct usb_interface *intf)
> >  {
> >         device_for_each_child(&intf->dev, NULL, rtsx_usb_resume_child);
> > --
> > 2.25.1
> >

RE: [PATCH] misc: rtsx: usb: Ensure mmc child device is active when card is present

[Gwendal Grignou]

> > > -                       if (val & (SD_CD | MS_CD))
> > > +                       if (val & (SD_CD | MS_CD)) {
> > > +                               device_for_each_child(&intf->dev, NULL, rtsx_usb_resume_child);
> > Why not calling rtsx_usb_resume() here?

> Because in this time rtsx_usb is not in runtime_suspend, only need to make sure child is not in suspend
> Actually when the program came here this suspend will be rejected because return -EAGAIN

> > >                                 return -EAGAIN;
> > > +                       }

I meant:

if (val & (SD_CD | MS_CD)) {
  rtsx_usb_resume(intf)
  return -EAGAIN;
}

It looks cleaner, as it indicates the the supsend is rejected and
needs to be undone. The code is in the end indentical to the patch you
are proposing. This is just for look anyway, the patch as-is is
acceptable.

[PATCH] misc: rtsx: usb: Ensure mmc child device is active when card is present

[Ricky Wu]

When a card is present in the reader, the driver currently defers
autosuspend by returning -EAGAIN during the suspend callback to
trigger USB remote wakeup signaling. However, this does not guarantee
that the mmc child device has been resumed, which may cause issues if
it remains suspended while the card is accessible.
This patch ensures that all child devices, including the mmc host
controller, are explicitly resumed before returning -EAGAIN. This
fixes a corner case introduced by earlier remote wakeup handling,
improving reliability of runtime PM when a card is inserted.

Fixes: 883a87ddf2f1 ("misc: rtsx_usb: Use USB remote wakeup signaling for card insertion detection")
Cc: stable@vger.kernel.org
Signed-off-by: Ricky Wu <ricky_wu@realtek.com>
---
 drivers/misc/cardreader/rtsx_usb.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/misc/cardreader/rtsx_usb.c b/drivers/misc/cardreader/rtsx_usb.c
index 148107a4547c..d007a4455ce5 100644
--- a/drivers/misc/cardreader/rtsx_usb.c
+++ b/drivers/misc/cardreader/rtsx_usb.c
@@ -698,6 +698,12 @@ static void rtsx_usb_disconnect(struct usb_interface *intf)
 }
 
 #ifdef CONFIG_PM
+static int rtsx_usb_resume_child(struct device *dev, void *data)
+{
+	pm_request_resume(dev);
+	return 0;
+}
+
 static int rtsx_usb_suspend(struct usb_interface *intf, pm_message_t message)
 {
 	struct rtsx_ucr *ucr =
@@ -713,8 +719,10 @@ static int rtsx_usb_suspend(struct usb_interface *intf, pm_message_t message)
 			mutex_unlock(&ucr->dev_mutex);
 
 			/* Defer the autosuspend if card exists */
-			if (val & (SD_CD | MS_CD))
+			if (val & (SD_CD | MS_CD)) {
+				device_for_each_child(&intf->dev, NULL, rtsx_usb_resume_child);
 				return -EAGAIN;
+			}
 		} else {
 			/* There is an ongoing operation*/
 			return -EAGAIN;
@@ -724,12 +732,6 @@ static int rtsx_usb_suspend(struct usb_interface *intf, pm_message_t message)
 	return 0;
 }
 
-static int rtsx_usb_resume_child(struct device *dev, void *data)
-{
-	pm_request_resume(dev);
-	return 0;
-}
-
 static int rtsx_usb_resume(struct usb_interface *intf)
 {
 	device_for_each_child(&intf->dev, NULL, rtsx_usb_resume_child);
-- 
2.25.1

Re: [PATCH] misc: rtsx: usb: Ensure mmc child device is active when card is present

[Ulf Hansson]

+ Gavin

On Fri, 11 Jul 2025 at 16:02, Ricky Wu <ricky_wu@realtek.com> wrote:
>
> When a card is present in the reader, the driver currently defers
> autosuspend by returning -EAGAIN during the suspend callback to
> trigger USB remote wakeup signaling. However, this does not guarantee
> that the mmc child device has been resumed, which may cause issues if
> it remains suspended while the card is accessible.
> This patch ensures that all child devices, including the mmc host
> controller, are explicitly resumed before returning -EAGAIN. This
> fixes a corner case introduced by earlier remote wakeup handling,
> improving reliability of runtime PM when a card is inserted.
>
> Fixes: 883a87ddf2f1 ("misc: rtsx_usb: Use USB remote wakeup signaling for card insertion detection")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ricky Wu <ricky_wu@realtek.com>

This seems reasonable to me, but perhaps some of the USB maintainers
should have a closer look to see if this makes sense. Nevertheless,
feel free to add:

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Moreover, we had a related bug-report/fix posted for the
rtsx_usb_sdmmc driver [1] not that long ago. Do you know if $subject
patch solves this problem too? I have looped in Gavin, if he has some
additional comments around this.

Kind regards
Uffe

[1]
https://lore.kernel.org/all/20250510031945.1004129-1-git@thegavinli.com/

> ---
>  drivers/misc/cardreader/rtsx_usb.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/misc/cardreader/rtsx_usb.c b/drivers/misc/cardreader/rtsx_usb.c
> index 148107a4547c..d007a4455ce5 100644
> --- a/drivers/misc/cardreader/rtsx_usb.c
> +++ b/drivers/misc/cardreader/rtsx_usb.c
> @@ -698,6 +698,12 @@ static void rtsx_usb_disconnect(struct usb_interface *intf)
>  }
>
>  #ifdef CONFIG_PM
> +static int rtsx_usb_resume_child(struct device *dev, void *data)
> +{
> +       pm_request_resume(dev);
> +       return 0;
> +}
> +
>  static int rtsx_usb_suspend(struct usb_interface *intf, pm_message_t message)
>  {
>         struct rtsx_ucr *ucr =
> @@ -713,8 +719,10 @@ static int rtsx_usb_suspend(struct usb_interface *intf, pm_message_t message)
>                         mutex_unlock(&ucr->dev_mutex);
>
>                         /* Defer the autosuspend if card exists */
> -                       if (val & (SD_CD | MS_CD))
> +                       if (val & (SD_CD | MS_CD)) {
> +                               device_for_each_child(&intf->dev, NULL, rtsx_usb_resume_child);
>                                 return -EAGAIN;
> +                       }
>                 } else {
>                         /* There is an ongoing operation*/
>                         return -EAGAIN;
> @@ -724,12 +732,6 @@ static int rtsx_usb_suspend(struct usb_interface *intf, pm_message_t message)
>         return 0;
>  }
>
> -static int rtsx_usb_resume_child(struct device *dev, void *data)
> -{
> -       pm_request_resume(dev);
> -       return 0;
> -}
> -
>  static int rtsx_usb_resume(struct usb_interface *intf)
>  {
>         device_for_each_child(&intf->dev, NULL, rtsx_usb_resume_child);
> --
> 2.25.1
>

Re: [PATCH] misc: rtsx: usb: Ensure mmc child device is active when card is present

[Gavin Li]

I just tested out this patch, and it unfortunately does not address
the issue I was running into (where the driver fails to detect
inserting a SD card after the initial driver probe).

On Wed, Jul 16, 2025 at 6:39 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> + Gavin
>
> On Fri, 11 Jul 2025 at 16:02, Ricky Wu <ricky_wu@realtek.com> wrote:
> >
> > When a card is present in the reader, the driver currently defers
> > autosuspend by returning -EAGAIN during the suspend callback to
> > trigger USB remote wakeup signaling. However, this does not guarantee
> > that the mmc child device has been resumed, which may cause issues if
> > it remains suspended while the card is accessible.
> > This patch ensures that all child devices, including the mmc host
> > controller, are explicitly resumed before returning -EAGAIN. This
> > fixes a corner case introduced by earlier remote wakeup handling,
> > improving reliability of runtime PM when a card is inserted.
> >
> > Fixes: 883a87ddf2f1 ("misc: rtsx_usb: Use USB remote wakeup signaling for card insertion detection")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Ricky Wu <ricky_wu@realtek.com>
>
> This seems reasonable to me, but perhaps some of the USB maintainers
> should have a closer look to see if this makes sense. Nevertheless,
> feel free to add:
>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> Moreover, we had a related bug-report/fix posted for the
> rtsx_usb_sdmmc driver [1] not that long ago. Do you know if $subject
> patch solves this problem too? I have looped in Gavin, if he has some
> additional comments around this.
>
> Kind regards
> Uffe
>
> [1]
> https://lore.kernel.org/all/20250510031945.1004129-1-git@thegavinli.com/
>
> > ---
> >  drivers/misc/cardreader/rtsx_usb.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/misc/cardreader/rtsx_usb.c b/drivers/misc/cardreader/rtsx_usb.c
> > index 148107a4547c..d007a4455ce5 100644
> > --- a/drivers/misc/cardreader/rtsx_usb.c
> > +++ b/drivers/misc/cardreader/rtsx_usb.c
> > @@ -698,6 +698,12 @@ static void rtsx_usb_disconnect(struct usb_interface *intf)
> >  }
> >
> >  #ifdef CONFIG_PM
> > +static int rtsx_usb_resume_child(struct device *dev, void *data)
> > +{
> > +       pm_request_resume(dev);
> > +       return 0;
> > +}
> > +
> >  static int rtsx_usb_suspend(struct usb_interface *intf, pm_message_t message)
> >  {
> >         struct rtsx_ucr *ucr =
> > @@ -713,8 +719,10 @@ static int rtsx_usb_suspend(struct usb_interface *intf, pm_message_t message)
> >                         mutex_unlock(&ucr->dev_mutex);
> >
> >                         /* Defer the autosuspend if card exists */
> > -                       if (val & (SD_CD | MS_CD))
> > +                       if (val & (SD_CD | MS_CD)) {
> > +                               device_for_each_child(&intf->dev, NULL, rtsx_usb_resume_child);
> >                                 return -EAGAIN;
> > +                       }
> >                 } else {
> >                         /* There is an ongoing operation*/
> >                         return -EAGAIN;
> > @@ -724,12 +732,6 @@ static int rtsx_usb_suspend(struct usb_interface *intf, pm_message_t message)
> >         return 0;
> >  }
> >
> > -static int rtsx_usb_resume_child(struct device *dev, void *data)
> > -{
> > -       pm_request_resume(dev);
> > -       return 0;
> > -}
> > -
> >  static int rtsx_usb_resume(struct usb_interface *intf)
> >  {
> >         device_for_each_child(&intf->dev, NULL, rtsx_usb_resume_child);
> > --
> > 2.25.1
> >