"Prevent too long response times for suspend" breaks libertas_sdio

"Prevent too long response times for suspend" breaks libertas_sdio

[Daniel Drake]

Hi,

This commit breaks libertas_sdio suspend:

commit b6ad726e3fe69e1ff3c3b2ad272ba3e4c376cd6a
Author: Ulf Hansson <ulf.hansson@stericsson.com>
Date:   Thu Oct 13 16:03:58 2011 +0200

    mmc: core: Prevent too long response times for suspend

    While trying to suspend the mmc host there could still be
    ongoing requests that we need to wait for. At the same time
    a device driver must respond to a suspend request rather quickly.

This patch causes the device to be claimed while the driver's suspend
method is called. This seems questionable to me. It should be up to
the driver to deal with or cancel any pending requests in the
interests of suspend performance. Even if they take a while to
complete, it might be best to let them complete rather than discard
the user's data.

In this case in the suspend handler we have to communicate with the
card. In libertas_sdio we do the communication in a workqueue
(because, outside of the suspend routine, sometime we need to initiate
communication from atomic context), but that seems like an
implementation detail that shouldn't be trampled upon by the higher
layers.

This method of punishing "badly-behaved" drivers (for some definition
of the phrase) is also quite harsh. Maybe its just my incompetence but
it took me a couple of hours to track down why my driver was suddenly
hanging with no warning message during its suspend routine.

Can we revisit this?

Thanks,
Daniel

Re: "Prevent too long response times for suspend" breaks libertas_sdio

[Chris Ball]

Hi Ulf,

On Mon, Apr 16 2012, Daniel Drake wrote:
> This commit breaks libertas_sdio suspend:
>
> commit b6ad726e3fe69e1ff3c3b2ad272ba3e4c376cd6a
> Author: Ulf Hansson <ulf.hansson@stericsson.com>
> Date:   Thu Oct 13 16:03:58 2011 +0200
>
>     mmc: core: Prevent too long response times for suspend
>
>     While trying to suspend the mmc host there could still be
>     ongoing requests that we need to wait for. At the same time
>     a device driver must respond to a suspend request rather quickly.
>
> This patch causes the device to be claimed while the driver's suspend
> method is called. This seems questionable to me. It should be up to
> the driver to deal with or cancel any pending requests in the
> interests of suspend performance. Even if they take a while to
> complete, it might be best to let them complete rather than discard
> the user's data.
>
> In this case in the suspend handler we have to communicate with the
> card. In libertas_sdio we do the communication in a workqueue
> (because, outside of the suspend routine, sometime we need to initiate
> communication from atomic context), but that seems like an
> implementation detail that shouldn't be trampled upon by the higher
> layers.
>
> This method of punishing "badly-behaved" drivers (for some definition
> of the phrase) is also quite harsh. Maybe its just my incompetence but
> it took me a couple of hours to track down why my driver was suddenly
> hanging with no warning message during its suspend routine.
>
> Can we revisit this?

Please can you take a look at this?  I'll plan on sending a revert of
your patch if I don't hear from you soon; we can't break libertas_sdio
unannounced.  Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: "Prevent too long response times for suspend" breaks libertas_sdio

[Ulf Hansson]

Hi Daniel,

On 04/16/2012 10:25 PM, Daniel Drake wrote:
> Hi,
>
> This commit breaks libertas_sdio suspend:
>
> commit b6ad726e3fe69e1ff3c3b2ad272ba3e4c376cd6a
> Author: Ulf Hansson<ulf.hansson@stericsson.com>
> Date:   Thu Oct 13 16:03:58 2011 +0200
>
>      mmc: core: Prevent too long response times for suspend
>
>      While trying to suspend the mmc host there could still be
>      ongoing requests that we need to wait for. At the same time
>      a device driver must respond to a suspend request rather quickly.
>
> This patch causes the device to be claimed while the driver's suspend
> method is called. This seems questionable to me. It should be up to
> the driver to deal with or cancel any pending requests in the
> interests of suspend performance. Even if they take a while to
> complete, it might be best to let them complete rather than discard
> the user's data.
>
> In this case in the suspend handler we have to communicate with the
> card. In libertas_sdio we do the communication in a workqueue
> (because, outside of the suspend routine, sometime we need to initiate
> communication from atomic context), but that seems like an
> implementation detail that shouldn't be trampled upon by the higher
> layers.

Alright, I think I realize your problem.

The workqueue will be hanging waiting for the host to be claimed. At the 
same time libertas_sdio suspend handler, which is called from 
"mmc_sdio_suspend" function will wait for the work to be "synced", but 
since the host is pre-claimed from mmc_suspend_host, it will hang 
forever. Have I got it right?

Not good. :-)

In principle that means the host must not be claimed when calling the 
sdio drivers suspend/resume callbacks.

>
> This method of punishing "badly-behaved" drivers (for some definition
> of the phrase) is also quite harsh. Maybe its just my incompetence but
> it took me a couple of hours to track down why my driver was suddenly
> hanging with no warning message during its suspend routine.

The idea with my patch was to prevent scenarios that kind of should not 
occur, were for example and SDIO driver work queue suddenly wanted to do 
a sdio request at the same time when the host is being suspended. I will 
have to think of another way to handle that then.

Reverting the patch is likely not a good idea, since there has been 
other pieces of code adding the try_claim_host call in the 
mmc_suspend_host function. I think we shall create a "cleanup" patch 
instead, which removes the try_claim_host thing, at least as a first 
step. I am on it immediately.

Then we can you can think of a more long term solution, if still needed.

What do you think?

>
> Can we revisit this?
>
> Thanks,
> Daniel

Kind regards
Ulf Hansson