2.6.35.6 fails to suspend (pxa2xx-mci.0)

2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Sven Neumann]

Hi,

we are running an embedded system here based on the PXA300 platform.
Suspend/resume used to work well so far. However after upgrading the
kernel from 2.6.34.7 to 2.6.35.6, we get the following error when trying
to suspend the system:

# echo "mem" > "/sys/power/state"
[ 5647.295953] PM: Syncing filesystems ... done.
[ 5647.318792] Freezing user space processes ... (elapsed 0.01 seconds) done.
[ 5647.337048] Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done.
[ 5647.356915] Suspending console(s) (use no_console_suspend to debug)
[ 5647.366651] pm_op(): platform_pm_suspend+0x0/0x5c returns -38
[ 5647.366671] PM: Device pxa2xx-mci.0 failed to suspend: error -38
[ 5647.367082] PM: Some devices failed to suspend


Regards,
Sven


-- 
Sven Neumann
Head of Software Development

RAUMFELD GmbH | Reichenberger Str. 124 | 10999 Berlin | Germany
Tel: +49.30.340.60.98.0 | Fax: +49.30.340.60.98.99 | s.neumann@raumfeld.com

2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Sven Neumann]

Hi,

we are running an embedded system here based on the PXA300 platform.
Suspend/resume used to work well so far. However after upgrading the
kernel from 2.6.34.7 to 2.6.35.6, we get the following error when trying
to suspend the system:

# echo "mem" > "/sys/power/state"
[ 5647.295953] PM: Syncing filesystems ... done.
[ 5647.318792] Freezing user space processes ... (elapsed 0.01 seconds) done.
[ 5647.337048] Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done.
[ 5647.356915] Suspending console(s) (use no_console_suspend to debug)
[ 5647.366651] pm_op(): platform_pm_suspend+0x0/0x5c returns -38
[ 5647.366671] PM: Device pxa2xx-mci.0 failed to suspend: error -38
[ 5647.367082] PM: Some devices failed to suspend


Regards,
Sven


-- 
Sven Neumann
Head of Software Development

RAUMFELD GmbH | Reichenberger Str. 124 | 10999 Berlin | Germany
Tel: +49.30.340.60.98.0 | Fax: +49.30.340.60.98.99 | s.neumann at raumfeld.com

Re: 2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Eric Miao]

On Mon, Oct 4, 2010 at 3:30 PM, Sven Neumann <s.neumann@raumfeld.com> wrote:
> Hi,
>
> we are running an embedded system here based on the PXA300 platform.
> Suspend/resume used to work well so far. However after upgrading the
> kernel from 2.6.34.7 to 2.6.35.6, we get the following error when trying
> to suspend the system:
>

I wonder if it's related to this issue:

https://bugs.launchpad.net/ubuntu/+source/linux-mvl-dove/+bug/530432

The issue above is not dove specific.
> # echo "mem" > "/sys/power/state"
> [ 5647.295953] PM: Syncing filesystems ... done.
> [ 5647.318792] Freezing user space processes ... (elapsed 0.01 seconds) done.
> [ 5647.337048] Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done.
> [ 5647.356915] Suspending console(s) (use no_console_suspend to debug)
> [ 5647.366651] pm_op(): platform_pm_suspend+0x0/0x5c returns -38
> [ 5647.366671] PM: Device pxa2xx-mci.0 failed to suspend: error -38
> [ 5647.367082] PM: Some devices failed to suspend
>
>
> Regards,
> Sven
>
>
> --
> Sven Neumann
> Head of Software Development
>
> RAUMFELD GmbH | Reichenberger Str. 124 | 10999 Berlin | Germany
> Tel: +49.30.340.60.98.0 | Fax: +49.30.340.60.98.99 | s.neumann@raumfeld.com
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Eric Miao]

On Mon, Oct 4, 2010 at 3:30 PM, Sven Neumann <s.neumann@raumfeld.com> wrote:
> Hi,
>
> we are running an embedded system here based on the PXA300 platform.
> Suspend/resume used to work well so far. However after upgrading the
> kernel from 2.6.34.7 to 2.6.35.6, we get the following error when trying
> to suspend the system:
>

I wonder if it's related to this issue:

https://bugs.launchpad.net/ubuntu/+source/linux-mvl-dove/+bug/530432

The issue above is not dove specific.
> # echo "mem" > "/sys/power/state"
> [ 5647.295953] PM: Syncing filesystems ... done.
> [ 5647.318792] Freezing user space processes ... (elapsed 0.01 seconds) done.
> [ 5647.337048] Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done.
> [ 5647.356915] Suspending console(s) (use no_console_suspend to debug)
> [ 5647.366651] pm_op(): platform_pm_suspend+0x0/0x5c returns -38
> [ 5647.366671] PM: Device pxa2xx-mci.0 failed to suspend: error -38
> [ 5647.367082] PM: Some devices failed to suspend
>
>
> Regards,
> Sven
>
>
> --
> Sven Neumann
> Head of Software Development
>
> RAUMFELD GmbH | Reichenberger Str. 124 | 10999 Berlin | Germany
> Tel: +49.30.340.60.98.0 | Fax: +49.30.340.60.98.99 | s.neumann at raumfeld.com
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

Re: 2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Maciej Rutecki]

On poniedziałek, 4 października 2010 o 09:30:35 Sven Neumann wrote:
> Hi,
> 
> we are running an embedded system here based on the PXA300 platform.
> Suspend/resume used to work well so far. However after upgrading the
> kernel from 2.6.34.7 to 2.6.35.6, we get the following error when trying
> to suspend the system:
>
I created a Bugzilla entry at 
https://bugzilla.kernel.org/show_bug.cgi?id=19792
for your bug report, please add your address to the CC list in there, thanks!

-- 
Maciej Rutecki
http://www.maciek.unixy.pl

2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Maciej Rutecki]

On poniedzia?ek, 4 pa?dziernika 2010 o 09:30:35 Sven Neumann wrote:
> Hi,
> 
> we are running an embedded system here based on the PXA300 platform.
> Suspend/resume used to work well so far. However after upgrading the
> kernel from 2.6.34.7 to 2.6.35.6, we get the following error when trying
> to suspend the system:
>
I created a Bugzilla entry at 
https://bugzilla.kernel.org/show_bug.cgi?id=19792
for your bug report, please add your address to the CC list in there, thanks!

-- 
Maciej Rutecki
http://www.maciek.unixy.pl

Re: 2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Daniel Mack]

On Mon, Oct 04, 2010 at 09:30:35AM +0200, Sven Neumann wrote:
> we are running an embedded system here based on the PXA300 platform.
> Suspend/resume used to work well so far. However after upgrading the
> kernel from 2.6.34.7 to 2.6.35.6, we get the following error when trying
> to suspend the system:
> 
> # echo "mem" > "/sys/power/state"
> [ 5647.295953] PM: Syncing filesystems ... done.
> [ 5647.318792] Freezing user space processes ... (elapsed 0.01 seconds) done.
> [ 5647.337048] Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done.
> [ 5647.356915] Suspending console(s) (use no_console_suspend to debug)
> [ 5647.366651] pm_op(): platform_pm_suspend+0x0/0x5c returns -38
> [ 5647.366671] PM: Device pxa2xx-mci.0 failed to suspend: error -38
> [ 5647.367082] PM: Some devices failed to suspend

We've bisected this effect down to commit 152e1d5920 ("PM: Prevent
waiting forever on asynchronous resume after failing suspend").
Suspending our PXA3xx based system breaks with this patch.

I tried to understand what's going wrong, but I didn't follow the
discussion about this logic, so I would rather like to pass it back to
the originating people.

I can only guess that the problem here is the somewhat tricky handling
around mmc_sdio_suspend(), which returns -ENODEV (-38) in case a
particular function of a card can not be suspended. The SDIO core would
have simply removed the card in this case normally, but the PM core
seems to interfere now, stopping the whole suspend procedure.

Can anyone shed some light on this?

Thanks,
Daniel

2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Daniel Mack]

On Mon, Oct 04, 2010 at 09:30:35AM +0200, Sven Neumann wrote:
> we are running an embedded system here based on the PXA300 platform.
> Suspend/resume used to work well so far. However after upgrading the
> kernel from 2.6.34.7 to 2.6.35.6, we get the following error when trying
> to suspend the system:
> 
> # echo "mem" > "/sys/power/state"
> [ 5647.295953] PM: Syncing filesystems ... done.
> [ 5647.318792] Freezing user space processes ... (elapsed 0.01 seconds) done.
> [ 5647.337048] Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done.
> [ 5647.356915] Suspending console(s) (use no_console_suspend to debug)
> [ 5647.366651] pm_op(): platform_pm_suspend+0x0/0x5c returns -38
> [ 5647.366671] PM: Device pxa2xx-mci.0 failed to suspend: error -38
> [ 5647.367082] PM: Some devices failed to suspend

We've bisected this effect down to commit 152e1d5920 ("PM: Prevent
waiting forever on asynchronous resume after failing suspend").
Suspending our PXA3xx based system breaks with this patch.

I tried to understand what's going wrong, but I didn't follow the
discussion about this logic, so I would rather like to pass it back to
the originating people.

I can only guess that the problem here is the somewhat tricky handling
around mmc_sdio_suspend(), which returns -ENODEV (-38) in case a
particular function of a card can not be suspended. The SDIO core would
have simply removed the card in this case normally, but the PM core
seems to interfere now, stopping the whole suspend procedure.

Can anyone shed some light on this?

Thanks,
Daniel

Re: 2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Rafael J. Wysocki]

On Thursday, October 07, 2010, Daniel Mack wrote:
> On Mon, Oct 04, 2010 at 09:30:35AM +0200, Sven Neumann wrote:
> > we are running an embedded system here based on the PXA300 platform.
> > Suspend/resume used to work well so far. However after upgrading the
> > kernel from 2.6.34.7 to 2.6.35.6, we get the following error when trying
> > to suspend the system:
> > 
> > # echo "mem" > "/sys/power/state"
> > [ 5647.295953] PM: Syncing filesystems ... done.
> > [ 5647.318792] Freezing user space processes ... (elapsed 0.01 seconds) done.
> > [ 5647.337048] Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done.
> > [ 5647.356915] Suspending console(s) (use no_console_suspend to debug)
> > [ 5647.366651] pm_op(): platform_pm_suspend+0x0/0x5c returns -38
> > [ 5647.366671] PM: Device pxa2xx-mci.0 failed to suspend: error -38
> > [ 5647.367082] PM: Some devices failed to suspend
> 
> We've bisected this effect down to commit 152e1d5920 ("PM: Prevent
> waiting forever on asynchronous resume after failing suspend").
> Suspending our PXA3xx based system breaks with this patch.
> 
> I tried to understand what's going wrong, but I didn't follow the
> discussion about this logic, so I would rather like to pass it back to
> the originating people.
> 
> I can only guess that the problem here is the somewhat tricky handling
> around mmc_sdio_suspend(), which returns -ENODEV (-38) in case a
> particular function of a card can not be suspended. The SDIO core would
> have simply removed the card in this case normally, but the PM core
> seems to interfere now, stopping the whole suspend procedure.
> 
> Can anyone shed some light on this?

I wonder what happens if you echo 0 to /sys/power/pm_async ?

Rafael

2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Rafael J. Wysocki]

On Thursday, October 07, 2010, Daniel Mack wrote:
> On Mon, Oct 04, 2010 at 09:30:35AM +0200, Sven Neumann wrote:
> > we are running an embedded system here based on the PXA300 platform.
> > Suspend/resume used to work well so far. However after upgrading the
> > kernel from 2.6.34.7 to 2.6.35.6, we get the following error when trying
> > to suspend the system:
> > 
> > # echo "mem" > "/sys/power/state"
> > [ 5647.295953] PM: Syncing filesystems ... done.
> > [ 5647.318792] Freezing user space processes ... (elapsed 0.01 seconds) done.
> > [ 5647.337048] Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done.
> > [ 5647.356915] Suspending console(s) (use no_console_suspend to debug)
> > [ 5647.366651] pm_op(): platform_pm_suspend+0x0/0x5c returns -38
> > [ 5647.366671] PM: Device pxa2xx-mci.0 failed to suspend: error -38
> > [ 5647.367082] PM: Some devices failed to suspend
> 
> We've bisected this effect down to commit 152e1d5920 ("PM: Prevent
> waiting forever on asynchronous resume after failing suspend").
> Suspending our PXA3xx based system breaks with this patch.
> 
> I tried to understand what's going wrong, but I didn't follow the
> discussion about this logic, so I would rather like to pass it back to
> the originating people.
> 
> I can only guess that the problem here is the somewhat tricky handling
> around mmc_sdio_suspend(), which returns -ENODEV (-38) in case a
> particular function of a card can not be suspended. The SDIO core would
> have simply removed the card in this case normally, but the PM core
> seems to interfere now, stopping the whole suspend procedure.
> 
> Can anyone shed some light on this?

I wonder what happens if you echo 0 to /sys/power/pm_async ?

Rafael

Re: 2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Sven Neumann]

Hi,

On Thu, 2010-10-07 at 02:18 +0200, Rafael J. Wysocki wrote:

> On Thursday, October 07, 2010, Daniel Mack wrote:
> > On Mon, Oct 04, 2010 at 09:30:35AM +0200, Sven Neumann wrote:
> > > we are running an embedded system here based on the PXA300 platform.
> > > Suspend/resume used to work well so far. However after upgrading the
> > > kernel from 2.6.34.7 to 2.6.35.6, we get the following error when trying
> > > to suspend the system:
> > > 
> > > # echo "mem" > "/sys/power/state"
> > > [ 5647.295953] PM: Syncing filesystems ... done.
> > > [ 5647.318792] Freezing user space processes ... (elapsed 0.01 seconds) done.
> > > [ 5647.337048] Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done.
> > > [ 5647.356915] Suspending console(s) (use no_console_suspend to debug)
> > > [ 5647.366651] pm_op(): platform_pm_suspend+0x0/0x5c returns -38
> > > [ 5647.366671] PM: Device pxa2xx-mci.0 failed to suspend: error -38
> > > [ 5647.367082] PM: Some devices failed to suspend
> > 
> > We've bisected this effect down to commit 152e1d5920 ("PM: Prevent
> > waiting forever on asynchronous resume after failing suspend").
> > Suspending our PXA3xx based system breaks with this patch.
> > 
> > I tried to understand what's going wrong, but I didn't follow the
> > discussion about this logic, so I would rather like to pass it back to
> > the originating people.
> > 
> > I can only guess that the problem here is the somewhat tricky handling
> > around mmc_sdio_suspend(), which returns -ENODEV (-38) in case a
> > particular function of a card can not be suspended. The SDIO core would
> > have simply removed the card in this case normally, but the PM core
> > seems to interfere now, stopping the whole suspend procedure.
> > 
> > Can anyone shed some light on this?
> 
> I wonder what happens if you echo 0 to /sys/power/pm_async ?

Nothing happens. The problem persists (tested with 2.6.36-rc7). What
would you expect to happen?


Sven

2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Sven Neumann]

Hi,

On Thu, 2010-10-07 at 02:18 +0200, Rafael J. Wysocki wrote:

> On Thursday, October 07, 2010, Daniel Mack wrote:
> > On Mon, Oct 04, 2010 at 09:30:35AM +0200, Sven Neumann wrote:
> > > we are running an embedded system here based on the PXA300 platform.
> > > Suspend/resume used to work well so far. However after upgrading the
> > > kernel from 2.6.34.7 to 2.6.35.6, we get the following error when trying
> > > to suspend the system:
> > > 
> > > # echo "mem" > "/sys/power/state"
> > > [ 5647.295953] PM: Syncing filesystems ... done.
> > > [ 5647.318792] Freezing user space processes ... (elapsed 0.01 seconds) done.
> > > [ 5647.337048] Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done.
> > > [ 5647.356915] Suspending console(s) (use no_console_suspend to debug)
> > > [ 5647.366651] pm_op(): platform_pm_suspend+0x0/0x5c returns -38
> > > [ 5647.366671] PM: Device pxa2xx-mci.0 failed to suspend: error -38
> > > [ 5647.367082] PM: Some devices failed to suspend
> > 
> > We've bisected this effect down to commit 152e1d5920 ("PM: Prevent
> > waiting forever on asynchronous resume after failing suspend").
> > Suspending our PXA3xx based system breaks with this patch.
> > 
> > I tried to understand what's going wrong, but I didn't follow the
> > discussion about this logic, so I would rather like to pass it back to
> > the originating people.
> > 
> > I can only guess that the problem here is the somewhat tricky handling
> > around mmc_sdio_suspend(), which returns -ENODEV (-38) in case a
> > particular function of a card can not be suspended. The SDIO core would
> > have simply removed the card in this case normally, but the PM core
> > seems to interfere now, stopping the whole suspend procedure.
> > 
> > Can anyone shed some light on this?
> 
> I wonder what happens if you echo 0 to /sys/power/pm_async ?

Nothing happens. The problem persists (tested with 2.6.36-rc7). What
would you expect to happen?


Sven

Re: 2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Rafael J. Wysocki]

On Thursday, October 07, 2010, Sven Neumann wrote:
> Hi,
> 
> On Thu, 2010-10-07 at 02:18 +0200, Rafael J. Wysocki wrote:
> 
> > On Thursday, October 07, 2010, Daniel Mack wrote:
> > > On Mon, Oct 04, 2010 at 09:30:35AM +0200, Sven Neumann wrote:
> > > > we are running an embedded system here based on the PXA300 platform.
> > > > Suspend/resume used to work well so far. However after upgrading the
> > > > kernel from 2.6.34.7 to 2.6.35.6, we get the following error when trying
> > > > to suspend the system:
> > > > 
> > > > # echo "mem" > "/sys/power/state"
> > > > [ 5647.295953] PM: Syncing filesystems ... done.
> > > > [ 5647.318792] Freezing user space processes ... (elapsed 0.01 seconds) done.
> > > > [ 5647.337048] Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done.
> > > > [ 5647.356915] Suspending console(s) (use no_console_suspend to debug)
> > > > [ 5647.366651] pm_op(): platform_pm_suspend+0x0/0x5c returns -38
> > > > [ 5647.366671] PM: Device pxa2xx-mci.0 failed to suspend: error -38
> > > > [ 5647.367082] PM: Some devices failed to suspend
> > > 
> > > We've bisected this effect down to commit 152e1d5920 ("PM: Prevent
> > > waiting forever on asynchronous resume after failing suspend").
> > > Suspending our PXA3xx based system breaks with this patch.
> > > 
> > > I tried to understand what's going wrong, but I didn't follow the
> > > discussion about this logic, so I would rather like to pass it back to
> > > the originating people.
> > > 
> > > I can only guess that the problem here is the somewhat tricky handling
> > > around mmc_sdio_suspend(), which returns -ENODEV (-38) in case a
> > > particular function of a card can not be suspended. The SDIO core would
> > > have simply removed the card in this case normally, but the PM core
> > > seems to interfere now, stopping the whole suspend procedure.
> > > 
> > > Can anyone shed some light on this?
> > 
> > I wonder what happens if you echo 0 to /sys/power/pm_async ?
> 
> Nothing happens. The problem persists (tested with 2.6.36-rc7). What
> would you expect to happen?

Exactly that. :-)

Commit 152e1d5920 should not affect the non-async case (I'd be surprised if
it did really) and things should work with /sys/power/pm_async = 0 anyway.

Please try check if you can reproduce with commt 152e1d5920 reverted and
/sys/power/pm_async = 0.  If you can, that's a driver bug.

Thanks,
Rafael

2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Rafael J. Wysocki]

On Thursday, October 07, 2010, Sven Neumann wrote:
> Hi,
> 
> On Thu, 2010-10-07 at 02:18 +0200, Rafael J. Wysocki wrote:
> 
> > On Thursday, October 07, 2010, Daniel Mack wrote:
> > > On Mon, Oct 04, 2010 at 09:30:35AM +0200, Sven Neumann wrote:
> > > > we are running an embedded system here based on the PXA300 platform.
> > > > Suspend/resume used to work well so far. However after upgrading the
> > > > kernel from 2.6.34.7 to 2.6.35.6, we get the following error when trying
> > > > to suspend the system:
> > > > 
> > > > # echo "mem" > "/sys/power/state"
> > > > [ 5647.295953] PM: Syncing filesystems ... done.
> > > > [ 5647.318792] Freezing user space processes ... (elapsed 0.01 seconds) done.
> > > > [ 5647.337048] Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done.
> > > > [ 5647.356915] Suspending console(s) (use no_console_suspend to debug)
> > > > [ 5647.366651] pm_op(): platform_pm_suspend+0x0/0x5c returns -38
> > > > [ 5647.366671] PM: Device pxa2xx-mci.0 failed to suspend: error -38
> > > > [ 5647.367082] PM: Some devices failed to suspend
> > > 
> > > We've bisected this effect down to commit 152e1d5920 ("PM: Prevent
> > > waiting forever on asynchronous resume after failing suspend").
> > > Suspending our PXA3xx based system breaks with this patch.
> > > 
> > > I tried to understand what's going wrong, but I didn't follow the
> > > discussion about this logic, so I would rather like to pass it back to
> > > the originating people.
> > > 
> > > I can only guess that the problem here is the somewhat tricky handling
> > > around mmc_sdio_suspend(), which returns -ENODEV (-38) in case a
> > > particular function of a card can not be suspended. The SDIO core would
> > > have simply removed the card in this case normally, but the PM core
> > > seems to interfere now, stopping the whole suspend procedure.
> > > 
> > > Can anyone shed some light on this?
> > 
> > I wonder what happens if you echo 0 to /sys/power/pm_async ?
> 
> Nothing happens. The problem persists (tested with 2.6.36-rc7). What
> would you expect to happen?

Exactly that. :-)

Commit 152e1d5920 should not affect the non-async case (I'd be surprised if
it did really) and things should work with /sys/power/pm_async = 0 anyway.

Please try check if you can reproduce with commt 152e1d5920 reverted and
/sys/power/pm_async = 0.  If you can, that's a driver bug.

Thanks,
Rafael

Re: 2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Sven Neumann]

On Thu, 2010-10-07 at 23:23 +0200, Rafael J. Wysocki wrote:

> > > I wonder what happens if you echo 0 to /sys/power/pm_async ?
> > 
> > Nothing happens. The problem persists (tested with 2.6.36-rc7). What
> > would you expect to happen?
> 
> Exactly that. :-)
> 
> Commit 152e1d5920 should not affect the non-async case (I'd be surprised if
> it did really) and things should work with /sys/power/pm_async = 0 anyway.
> 
> Please try check if you can reproduce with commt 152e1d5920 reverted and
> /sys/power/pm_async = 0.  If you can, that's a driver bug.

Ok, for the record, here's what I tried. I have rebooted between tests
to make sure there's no state pulled in from the previous test:

 2.6.36-rc7, no changes,      pm_async 1 :  suspend fails
 2.6.36-rc7, no changes,      pm_async 0 :  suspend fails

 2.6.36-rc7, 152e1d reverted, pm_async 1 :  suspend works
 2.6.36-rc7, 152e1d reverted, pm_async 0 :  suspend fails

 2.6.34.7, no changes,        pm_async 1 :  suspend works
 2.6.34.7, no changes,        pm_async 0 :  suspend works

I am not sure how this should be interpreted.


Sven

2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Sven Neumann]

On Thu, 2010-10-07 at 23:23 +0200, Rafael J. Wysocki wrote:

> > > I wonder what happens if you echo 0 to /sys/power/pm_async ?
> > 
> > Nothing happens. The problem persists (tested with 2.6.36-rc7). What
> > would you expect to happen?
> 
> Exactly that. :-)
> 
> Commit 152e1d5920 should not affect the non-async case (I'd be surprised if
> it did really) and things should work with /sys/power/pm_async = 0 anyway.
> 
> Please try check if you can reproduce with commt 152e1d5920 reverted and
> /sys/power/pm_async = 0.  If you can, that's a driver bug.

Ok, for the record, here's what I tried. I have rebooted between tests
to make sure there's no state pulled in from the previous test:

 2.6.36-rc7, no changes,      pm_async 1 :  suspend fails
 2.6.36-rc7, no changes,      pm_async 0 :  suspend fails

 2.6.36-rc7, 152e1d reverted, pm_async 1 :  suspend works
 2.6.36-rc7, 152e1d reverted, pm_async 0 :  suspend fails

 2.6.34.7, no changes,        pm_async 1 :  suspend works
 2.6.34.7, no changes,        pm_async 0 :  suspend works

I am not sure how this should be interpreted.


Sven

Re: 2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Rafael J. Wysocki]

On Friday, October 08, 2010, Sven Neumann wrote:
> On Thu, 2010-10-07 at 23:23 +0200, Rafael J. Wysocki wrote:
> 
> > > > I wonder what happens if you echo 0 to /sys/power/pm_async ?
> > > 
> > > Nothing happens. The problem persists (tested with 2.6.36-rc7). What
> > > would you expect to happen?
> > 
> > Exactly that. :-)
> > 
> > Commit 152e1d5920 should not affect the non-async case (I'd be surprised if
> > it did really) and things should work with /sys/power/pm_async = 0 anyway.
> > 
> > Please try check if you can reproduce with commt 152e1d5920 reverted and
> > /sys/power/pm_async = 0.  If you can, that's a driver bug.
> 
> Ok, for the record, here's what I tried. I have rebooted between tests
> to make sure there's no state pulled in from the previous test:
> 
>  2.6.36-rc7, no changes,      pm_async 1 :  suspend fails
>  2.6.36-rc7, no changes,      pm_async 0 :  suspend fails
> 
>  2.6.36-rc7, 152e1d reverted, pm_async 1 :  suspend works
>  2.6.36-rc7, 152e1d reverted, pm_async 0 :  suspend fails
> 
>  2.6.34.7, no changes,        pm_async 1 :  suspend works
>  2.6.34.7, no changes,        pm_async 0 :  suspend works
> 
> I am not sure how this should be interpreted.

There is a problem with the pxa2xx-mci.0 suspend that has been introduced
some time after 2.6.34, but it is not related to commit 152e1d5920.  The
problem was not visible with pm_async=1 because of the very bug fixed by
commit 152e1d5920.

Please see if the problem is there in 2.6.35 (please test with pm_async=0).

Thanks,
Rafael

2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Rafael J. Wysocki]

On Friday, October 08, 2010, Sven Neumann wrote:
> On Thu, 2010-10-07 at 23:23 +0200, Rafael J. Wysocki wrote:
> 
> > > > I wonder what happens if you echo 0 to /sys/power/pm_async ?
> > > 
> > > Nothing happens. The problem persists (tested with 2.6.36-rc7). What
> > > would you expect to happen?
> > 
> > Exactly that. :-)
> > 
> > Commit 152e1d5920 should not affect the non-async case (I'd be surprised if
> > it did really) and things should work with /sys/power/pm_async = 0 anyway.
> > 
> > Please try check if you can reproduce with commt 152e1d5920 reverted and
> > /sys/power/pm_async = 0.  If you can, that's a driver bug.
> 
> Ok, for the record, here's what I tried. I have rebooted between tests
> to make sure there's no state pulled in from the previous test:
> 
>  2.6.36-rc7, no changes,      pm_async 1 :  suspend fails
>  2.6.36-rc7, no changes,      pm_async 0 :  suspend fails
> 
>  2.6.36-rc7, 152e1d reverted, pm_async 1 :  suspend works
>  2.6.36-rc7, 152e1d reverted, pm_async 0 :  suspend fails
> 
>  2.6.34.7, no changes,        pm_async 1 :  suspend works
>  2.6.34.7, no changes,        pm_async 0 :  suspend works
> 
> I am not sure how this should be interpreted.

There is a problem with the pxa2xx-mci.0 suspend that has been introduced
some time after 2.6.34, but it is not related to commit 152e1d5920.  The
problem was not visible with pm_async=1 because of the very bug fixed by
commit 152e1d5920.

Please see if the problem is there in 2.6.35 (please test with pm_async=0).

Thanks,
Rafael

Re: 2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Ohad Ben-Cohen]

On Fri, Oct 8, 2010 at 10:08 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> There is a problem with the pxa2xx-mci.0 suspend that has been introduced
> some time after 2.6.34, but it is not related to commit 152e1d5920.

Sven, what type of card do you have in that slot ? is it SDIO ? what's
the function driver ?

I have a hunch about this, but it may be a wild guess as I don't know
what's your setup exactly.

Anyway, can you please try this and see if anything changes ?

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index c94565d..515ff39 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
        if (host->bus_ops && !host->bus_dead) {
                if (host->bus_ops->suspend)
                        err = host->bus_ops->suspend(host);
+               if (err == -ENOSYS || !host->bus_ops->resume) {
+                       /*
+                        * We simply "remove" the card in this case.
+                        * It will be redetected on resume.
+                        */
+                       if (host->bus_ops->remove)
+                               host->bus_ops->remove(host);
+                       mmc_claim_host(host);
+                       mmc_detach_bus(host);
+                       mmc_release_host(host);
+                       host->pm_flags = 0;
+                       err = 0;
+               }
        }
        mmc_bus_put(host);

The reason I'm asking this is because it seems like commit
4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs related
to mmc/sd card insert/removal during suspend/resume" has broken
suspending SDIO function drivers which don't have a suspend handler
(or do have one, but for some reason it returns -ENOSYS, like
libertas_sdio's does).

>  The
> problem was not visible with pm_async=1 because of the very bug fixed by
> commit 152e1d5920.
>
> Please see if the problem is there in 2.6.35 (please test with pm_async=0).
>
> Thanks,
> Rafael
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Ohad Ben-Cohen]

On Fri, Oct 8, 2010 at 10:08 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> There is a problem with the pxa2xx-mci.0 suspend that has been introduced
> some time after 2.6.34, but it is not related to commit 152e1d5920.

Sven, what type of card do you have in that slot ? is it SDIO ? what's
the function driver ?

I have a hunch about this, but it may be a wild guess as I don't know
what's your setup exactly.

Anyway, can you please try this and see if anything changes ?

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index c94565d..515ff39 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
        if (host->bus_ops && !host->bus_dead) {
                if (host->bus_ops->suspend)
                        err = host->bus_ops->suspend(host);
+               if (err == -ENOSYS || !host->bus_ops->resume) {
+                       /*
+                        * We simply "remove" the card in this case.
+                        * It will be redetected on resume.
+                        */
+                       if (host->bus_ops->remove)
+                               host->bus_ops->remove(host);
+                       mmc_claim_host(host);
+                       mmc_detach_bus(host);
+                       mmc_release_host(host);
+                       host->pm_flags = 0;
+                       err = 0;
+               }
        }
        mmc_bus_put(host);

The reason I'm asking this is because it seems like commit
4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs related
to mmc/sd card insert/removal during suspend/resume" has broken
suspending SDIO function drivers which don't have a suspend handler
(or do have one, but for some reason it returns -ENOSYS, like
libertas_sdio's does).

> ?The
> problem was not visible with pm_async=1 because of the very bug fixed by
> commit 152e1d5920.
>
> Please see if the problem is there in 2.6.35 (please test with pm_async=0).
>
> Thanks,
> Rafael
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>

Re: 2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Sven Neumann]

On Sat, 2010-10-09 at 03:07 +0200, Ohad Ben-Cohen wrote:
> On Fri, Oct 8, 2010 at 10:08 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > There is a problem with the pxa2xx-mci.0 suspend that has been introduced
> > some time after 2.6.34, but it is not related to commit 152e1d5920.
> 
> Sven, what type of card do you have in that slot ? is it SDIO ? what's
> the function driver ?
> 
> I have a hunch about this, but it may be a wild guess as I don't know
> what's your setup exactly.
> 
> Anyway, can you please try this and see if anything changes ?
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index c94565d..515ff39 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
>         if (host->bus_ops && !host->bus_dead) {
>                 if (host->bus_ops->suspend)
>                         err = host->bus_ops->suspend(host);
> +               if (err == -ENOSYS || !host->bus_ops->resume) {
> +                       /*
> +                        * We simply "remove" the card in this case.
> +                        * It will be redetected on resume.
> +                        */
> +                       if (host->bus_ops->remove)
> +                               host->bus_ops->remove(host);
> +                       mmc_claim_host(host);
> +                       mmc_detach_bus(host);
> +                       mmc_release_host(host);
> +                       host->pm_flags = 0;
> +                       err = 0;
> +               }
>         }
>         mmc_bus_put(host);
> 
> The reason I'm asking this is because it seems like commit
> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs related
> to mmc/sd card insert/removal during suspend/resume" has broken
> suspending SDIO function drivers which don't have a suspend handler
> (or do have one, but for some reason it returns -ENOSYS, like
> libertas_sdio's does).

That is exactly our setup (libertas over sdio). I will try your patch as
soon as I get to the office on Monday morning and report back.


Thanks,
Sven

2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Sven Neumann]

On Sat, 2010-10-09 at 03:07 +0200, Ohad Ben-Cohen wrote:
> On Fri, Oct 8, 2010 at 10:08 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > There is a problem with the pxa2xx-mci.0 suspend that has been introduced
> > some time after 2.6.34, but it is not related to commit 152e1d5920.
> 
> Sven, what type of card do you have in that slot ? is it SDIO ? what's
> the function driver ?
> 
> I have a hunch about this, but it may be a wild guess as I don't know
> what's your setup exactly.
> 
> Anyway, can you please try this and see if anything changes ?
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index c94565d..515ff39 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
>         if (host->bus_ops && !host->bus_dead) {
>                 if (host->bus_ops->suspend)
>                         err = host->bus_ops->suspend(host);
> +               if (err == -ENOSYS || !host->bus_ops->resume) {
> +                       /*
> +                        * We simply "remove" the card in this case.
> +                        * It will be redetected on resume.
> +                        */
> +                       if (host->bus_ops->remove)
> +                               host->bus_ops->remove(host);
> +                       mmc_claim_host(host);
> +                       mmc_detach_bus(host);
> +                       mmc_release_host(host);
> +                       host->pm_flags = 0;
> +                       err = 0;
> +               }
>         }
>         mmc_bus_put(host);
> 
> The reason I'm asking this is because it seems like commit
> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs related
> to mmc/sd card insert/removal during suspend/resume" has broken
> suspending SDIO function drivers which don't have a suspend handler
> (or do have one, but for some reason it returns -ENOSYS, like
> libertas_sdio's does).

That is exactly our setup (libertas over sdio). I will try your patch as
soon as I get to the office on Monday morning and report back.


Thanks,
Sven

Re: 2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Sven Neumann]

On Sat, 2010-10-09 at 03:07 +0200, Ohad Ben-Cohen wrote:
> On Fri, Oct 8, 2010 at 10:08 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > There is a problem with the pxa2xx-mci.0 suspend that has been introduced
> > some time after 2.6.34, but it is not related to commit 152e1d5920.
> 
> Sven, what type of card do you have in that slot ? is it SDIO ? what's
> the function driver ?
> 
> I have a hunch about this, but it may be a wild guess as I don't know
> what's your setup exactly.
> 
> Anyway, can you please try this and see if anything changes ?
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index c94565d..515ff39 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
>         if (host->bus_ops && !host->bus_dead) {
>                 if (host->bus_ops->suspend)
>                         err = host->bus_ops->suspend(host);
> +               if (err == -ENOSYS || !host->bus_ops->resume) {
> +                       /*
> +                        * We simply "remove" the card in this case.
> +                        * It will be redetected on resume.
> +                        */
> +                       if (host->bus_ops->remove)
> +                               host->bus_ops->remove(host);
> +                       mmc_claim_host(host);
> +                       mmc_detach_bus(host);
> +                       mmc_release_host(host);
> +                       host->pm_flags = 0;
> +                       err = 0;
> +               }
>         }
>         mmc_bus_put(host);
> 
> The reason I'm asking this is because it seems like commit
> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs related
> to mmc/sd card insert/removal during suspend/resume" has broken
> suspending SDIO function drivers which don't have a suspend handler
> (or do have one, but for some reason it returns -ENOSYS, like
> libertas_sdio's does).

Yes, this patch fixes the problem. Tested with v2.6.35-rc7. With above
patch applied, suspend (and resume!) work again with and without
pm_async=0.

Thanks a lot to all of you for your help with this problem.


Regards,
Sven

2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Sven Neumann]

On Sat, 2010-10-09 at 03:07 +0200, Ohad Ben-Cohen wrote:
> On Fri, Oct 8, 2010 at 10:08 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > There is a problem with the pxa2xx-mci.0 suspend that has been introduced
> > some time after 2.6.34, but it is not related to commit 152e1d5920.
> 
> Sven, what type of card do you have in that slot ? is it SDIO ? what's
> the function driver ?
> 
> I have a hunch about this, but it may be a wild guess as I don't know
> what's your setup exactly.
> 
> Anyway, can you please try this and see if anything changes ?
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index c94565d..515ff39 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
>         if (host->bus_ops && !host->bus_dead) {
>                 if (host->bus_ops->suspend)
>                         err = host->bus_ops->suspend(host);
> +               if (err == -ENOSYS || !host->bus_ops->resume) {
> +                       /*
> +                        * We simply "remove" the card in this case.
> +                        * It will be redetected on resume.
> +                        */
> +                       if (host->bus_ops->remove)
> +                               host->bus_ops->remove(host);
> +                       mmc_claim_host(host);
> +                       mmc_detach_bus(host);
> +                       mmc_release_host(host);
> +                       host->pm_flags = 0;
> +                       err = 0;
> +               }
>         }
>         mmc_bus_put(host);
> 
> The reason I'm asking this is because it seems like commit
> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs related
> to mmc/sd card insert/removal during suspend/resume" has broken
> suspending SDIO function drivers which don't have a suspend handler
> (or do have one, but for some reason it returns -ENOSYS, like
> libertas_sdio's does).

Yes, this patch fixes the problem. Tested with v2.6.35-rc7. With above
patch applied, suspend (and resume!) work again with and without
pm_async=0.

Thanks a lot to all of you for your help with this problem.


Regards,
Sven

Re: 2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Ohad Ben-Cohen]

On Mon, Oct 11, 2010 at 10:31 AM, Sven Neumann <s.neumann@raumfeld.com> wrote:
> On Sat, 2010-10-09 at 03:07 +0200, Ohad Ben-Cohen wrote:
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index c94565d..515ff39 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
>>         if (host->bus_ops && !host->bus_dead) {
>>                 if (host->bus_ops->suspend)
>>                         err = host->bus_ops->suspend(host);
>> +               if (err == -ENOSYS || !host->bus_ops->resume) {
>> +                       /*
>> +                        * We simply "remove" the card in this case.
>> +                        * It will be redetected on resume.
>> +                        */
>> +                       if (host->bus_ops->remove)
>> +                               host->bus_ops->remove(host);
>> +                       mmc_claim_host(host);
>> +                       mmc_detach_bus(host);
>> +                       mmc_release_host(host);
>> +                       host->pm_flags = 0;
>> +                       err = 0;
>> +               }
>>         }
>>         mmc_bus_put(host);
>>
>> The reason I'm asking this is because it seems like commit
>> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs related
>> to mmc/sd card insert/removal during suspend/resume" has broken
>> suspending SDIO function drivers which don't have a suspend handler
>> (or do have one, but for some reason it returns -ENOSYS, like
>> libertas_sdio's does).
>
> Yes, this patch fixes the problem. Tested with v2.6.35-rc7. With above
> patch applied, suspend (and resume!) work again with and without
> pm_async=0.
>
> Thanks a lot to all of you for your help with this problem.

Thanks a lot for testing this Sven !

It is completely fixing the regression, but I must say the end result
is not very beautiful. We may eventually still use it, at least as an
interim solution, but I'd at least like to explain what's going on
here.

Patch 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs
related to mmc/sd card insert/removal during suspend/resume" (which
was introduced in 2.6.36 and backported to stable kernels) moved the
card remove/insert mechanism out of the suspend/resume path and into
mmc_pm_notify (which uses pm notifiers; needed in order to let user
space sync the card upon removal).

It broke (part of) SDIO suspend because SDIO relied on
mmc_suspend_host() to remove the card, and squash the error, in case
-ENOSYS is returned from the bus suspend handler (mmc_sdio_suspend()
in this case).

mmc_sdio_suspend() is using this whenever one of the card's SDIO
function drivers does not have suspend/resume handlers - in that case
it is agreed to force removal of the entire card.

So the trivial thing to do here was to bring back that part of
mmc_suspend_host(), which was removed by
4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e.

The reason I said the end result is not too beautiful is because now
we have two places in the mmc suspend execution path that can remove
the card: mmc_pm_notify and mmc_suspend_host, where the second is
probably useful only for SDIO. The card re-insertion, btw, will only
take place in mmc_pm_notify, and be used by all scenarios (so it's
becoming asymmetric for SDIO).

To use mmc_pm_notify's card-removal code also for SDIO, we need to
check there if all the SDIO functions have suspend handlers. That
would probably make us add a new bus_ops handler (something like
host->bus_ops->remove_card_on_suspend ?).

It's starting to be a bit complicated though, and I'm not sure it
would make the code a lot more readable.

And, it would still not work for drivers like libertas sdio, which do
have a suspend hander, but sometimes let it return -ENOSYS, expecting
the MMC core to remove the card and squash the error. For those cases,
we still need the old card-removal logic in mmc_suspend_host().

Btw, it makes me wonder: does libertas_sdio really needs this
functionality ? When MMC_PM_KEEP_POWER is not needed, why can't
libertas_sdio just return 0 (and as a result the card will be powered
off, but not removed) ?

To sum up, I think we can begin with taking this patch, at least as an
interim solution, and take the discussion of making the whole thing
more elegant to linux-mmc.

If that's agreed, I'll send that patch again with a decent commit
message, so we can take it to 2.6.36 and stable kernels.

>
>
> Regards,
> Sven
>
>
>

2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Ohad Ben-Cohen]

On Mon, Oct 11, 2010 at 10:31 AM, Sven Neumann <s.neumann@raumfeld.com> wrote:
> On Sat, 2010-10-09 at 03:07 +0200, Ohad Ben-Cohen wrote:
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index c94565d..515ff39 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
>> ? ? ? ? if (host->bus_ops && !host->bus_dead) {
>> ? ? ? ? ? ? ? ? if (host->bus_ops->suspend)
>> ? ? ? ? ? ? ? ? ? ? ? ? err = host->bus_ops->suspend(host);
>> + ? ? ? ? ? ? ? if (err == -ENOSYS || !host->bus_ops->resume) {
>> + ? ? ? ? ? ? ? ? ? ? ? /*
>> + ? ? ? ? ? ? ? ? ? ? ? ?* We simply "remove" the card in this case.
>> + ? ? ? ? ? ? ? ? ? ? ? ?* It will be redetected on resume.
>> + ? ? ? ? ? ? ? ? ? ? ? ?*/
>> + ? ? ? ? ? ? ? ? ? ? ? if (host->bus_ops->remove)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? host->bus_ops->remove(host);
>> + ? ? ? ? ? ? ? ? ? ? ? mmc_claim_host(host);
>> + ? ? ? ? ? ? ? ? ? ? ? mmc_detach_bus(host);
>> + ? ? ? ? ? ? ? ? ? ? ? mmc_release_host(host);
>> + ? ? ? ? ? ? ? ? ? ? ? host->pm_flags = 0;
>> + ? ? ? ? ? ? ? ? ? ? ? err = 0;
>> + ? ? ? ? ? ? ? }
>> ? ? ? ? }
>> ? ? ? ? mmc_bus_put(host);
>>
>> The reason I'm asking this is because it seems like commit
>> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs related
>> to mmc/sd card insert/removal during suspend/resume" has broken
>> suspending SDIO function drivers which don't have a suspend handler
>> (or do have one, but for some reason it returns -ENOSYS, like
>> libertas_sdio's does).
>
> Yes, this patch fixes the problem. Tested with v2.6.35-rc7. With above
> patch applied, suspend (and resume!) work again with and without
> pm_async=0.
>
> Thanks a lot to all of you for your help with this problem.

Thanks a lot for testing this Sven !

It is completely fixing the regression, but I must say the end result
is not very beautiful. We may eventually still use it, at least as an
interim solution, but I'd at least like to explain what's going on
here.

Patch 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs
related to mmc/sd card insert/removal during suspend/resume" (which
was introduced in 2.6.36 and backported to stable kernels) moved the
card remove/insert mechanism out of the suspend/resume path and into
mmc_pm_notify (which uses pm notifiers; needed in order to let user
space sync the card upon removal).

It broke (part of) SDIO suspend because SDIO relied on
mmc_suspend_host() to remove the card, and squash the error, in case
-ENOSYS is returned from the bus suspend handler (mmc_sdio_suspend()
in this case).

mmc_sdio_suspend() is using this whenever one of the card's SDIO
function drivers does not have suspend/resume handlers - in that case
it is agreed to force removal of the entire card.

So the trivial thing to do here was to bring back that part of
mmc_suspend_host(), which was removed by
4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e.

The reason I said the end result is not too beautiful is because now
we have two places in the mmc suspend execution path that can remove
the card: mmc_pm_notify and mmc_suspend_host, where the second is
probably useful only for SDIO. The card re-insertion, btw, will only
take place in mmc_pm_notify, and be used by all scenarios (so it's
becoming asymmetric for SDIO).

To use mmc_pm_notify's card-removal code also for SDIO, we need to
check there if all the SDIO functions have suspend handlers. That
would probably make us add a new bus_ops handler (something like
host->bus_ops->remove_card_on_suspend ?).

It's starting to be a bit complicated though, and I'm not sure it
would make the code a lot more readable.

And, it would still not work for drivers like libertas sdio, which do
have a suspend hander, but sometimes let it return -ENOSYS, expecting
the MMC core to remove the card and squash the error. For those cases,
we still need the old card-removal logic in mmc_suspend_host().

Btw, it makes me wonder: does libertas_sdio really needs this
functionality ? When MMC_PM_KEEP_POWER is not needed, why can't
libertas_sdio just return 0 (and as a result the card will be powered
off, but not removed) ?

To sum up, I think we can begin with taking this patch, at least as an
interim solution, and take the discussion of making the whole thing
more elegant to linux-mmc.

If that's agreed, I'll send that patch again with a decent commit
message, so we can take it to 2.6.36 and stable kernels.

>
>
> Regards,
> Sven
>
>
>

Re: 2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Sven Neumann]

On Mon, 2010-10-11 at 10:45 +0200, Ohad Ben-Cohen wrote:
> On Mon, Oct 11, 2010 at 10:31 AM, Sven Neumann <s.neumann@raumfeld.com> wrote:
> > On Sat, 2010-10-09 at 03:07 +0200, Ohad Ben-Cohen wrote:
> >> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> >> index c94565d..515ff39 100644
> >> --- a/drivers/mmc/core/core.c
> >> +++ b/drivers/mmc/core/core.c
> >> @@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
> >>         if (host->bus_ops && !host->bus_dead) {
> >>                 if (host->bus_ops->suspend)
> >>                         err = host->bus_ops->suspend(host);
> >> +               if (err == -ENOSYS || !host->bus_ops->resume) {
> >> +                       /*
> >> +                        * We simply "remove" the card in this case.
> >> +                        * It will be redetected on resume.
> >> +                        */
> >> +                       if (host->bus_ops->remove)
> >> +                               host->bus_ops->remove(host);
> >> +                       mmc_claim_host(host);
> >> +                       mmc_detach_bus(host);
> >> +                       mmc_release_host(host);
> >> +                       host->pm_flags = 0;
> >> +                       err = 0;
> >> +               }
> >>         }
> >>         mmc_bus_put(host);
> >>
> >> The reason I'm asking this is because it seems like commit
> >> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs related
> >> to mmc/sd card insert/removal during suspend/resume" has broken
> >> suspending SDIO function drivers which don't have a suspend handler
> >> (or do have one, but for some reason it returns -ENOSYS, like
> >> libertas_sdio's does).
> >
> > Yes, this patch fixes the problem. Tested with v2.6.35-rc7. With above
> > patch applied, suspend (and resume!) work again with and without
> > pm_async=0.
> >
> > Thanks a lot to all of you for your help with this problem.
> 
> Thanks a lot for testing this Sven !
> 
> It is completely fixing the regression, but I must say the end result
> is not very beautiful. We may eventually still use it, at least as an
> interim solution, but I'd at least like to explain what's going on
> here.
> 
> Patch 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs
> related to mmc/sd card insert/removal during suspend/resume" (which
> was introduced in 2.6.36 and backported to stable kernels) moved the
> card remove/insert mechanism out of the suspend/resume path and into
> mmc_pm_notify (which uses pm notifiers; needed in order to let user
> space sync the card upon removal).
> 
> It broke (part of) SDIO suspend because SDIO relied on
> mmc_suspend_host() to remove the card, and squash the error, in case
> -ENOSYS is returned from the bus suspend handler (mmc_sdio_suspend()
> in this case).
> 
> mmc_sdio_suspend() is using this whenever one of the card's SDIO
> function drivers does not have suspend/resume handlers - in that case
> it is agreed to force removal of the entire card.
> 
> So the trivial thing to do here was to bring back that part of
> mmc_suspend_host(), which was removed by
> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e.
> 
> The reason I said the end result is not too beautiful is because now
> we have two places in the mmc suspend execution path that can remove
> the card: mmc_pm_notify and mmc_suspend_host, where the second is
> probably useful only for SDIO. The card re-insertion, btw, will only
> take place in mmc_pm_notify, and be used by all scenarios (so it's
> becoming asymmetric for SDIO).
> 
> To use mmc_pm_notify's card-removal code also for SDIO, we need to
> check there if all the SDIO functions have suspend handlers. That
> would probably make us add a new bus_ops handler (something like
> host->bus_ops->remove_card_on_suspend ?).
> 
> It's starting to be a bit complicated though, and I'm not sure it
> would make the code a lot more readable.
> 
> And, it would still not work for drivers like libertas sdio, which do
> have a suspend hander, but sometimes let it return -ENOSYS, expecting
> the MMC core to remove the card and squash the error. For those cases,
> we still need the old card-removal logic in mmc_suspend_host().
> 
> Btw, it makes me wonder: does libertas_sdio really needs this
> functionality ? When MMC_PM_KEEP_POWER is not needed, why can't
> libertas_sdio just return 0 (and as a result the card will be powered
> off, but not removed) ?

Perhaps we should bring up this question on the libertas-dev list. I am
Cc'ing them in the hope that someone can answer this question.


Sven

2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Sven Neumann]

On Mon, 2010-10-11 at 10:45 +0200, Ohad Ben-Cohen wrote:
> On Mon, Oct 11, 2010 at 10:31 AM, Sven Neumann <s.neumann@raumfeld.com> wrote:
> > On Sat, 2010-10-09 at 03:07 +0200, Ohad Ben-Cohen wrote:
> >> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> >> index c94565d..515ff39 100644
> >> --- a/drivers/mmc/core/core.c
> >> +++ b/drivers/mmc/core/core.c
> >> @@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
> >>         if (host->bus_ops && !host->bus_dead) {
> >>                 if (host->bus_ops->suspend)
> >>                         err = host->bus_ops->suspend(host);
> >> +               if (err == -ENOSYS || !host->bus_ops->resume) {
> >> +                       /*
> >> +                        * We simply "remove" the card in this case.
> >> +                        * It will be redetected on resume.
> >> +                        */
> >> +                       if (host->bus_ops->remove)
> >> +                               host->bus_ops->remove(host);
> >> +                       mmc_claim_host(host);
> >> +                       mmc_detach_bus(host);
> >> +                       mmc_release_host(host);
> >> +                       host->pm_flags = 0;
> >> +                       err = 0;
> >> +               }
> >>         }
> >>         mmc_bus_put(host);
> >>
> >> The reason I'm asking this is because it seems like commit
> >> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs related
> >> to mmc/sd card insert/removal during suspend/resume" has broken
> >> suspending SDIO function drivers which don't have a suspend handler
> >> (or do have one, but for some reason it returns -ENOSYS, like
> >> libertas_sdio's does).
> >
> > Yes, this patch fixes the problem. Tested with v2.6.35-rc7. With above
> > patch applied, suspend (and resume!) work again with and without
> > pm_async=0.
> >
> > Thanks a lot to all of you for your help with this problem.
> 
> Thanks a lot for testing this Sven !
> 
> It is completely fixing the regression, but I must say the end result
> is not very beautiful. We may eventually still use it, at least as an
> interim solution, but I'd at least like to explain what's going on
> here.
> 
> Patch 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs
> related to mmc/sd card insert/removal during suspend/resume" (which
> was introduced in 2.6.36 and backported to stable kernels) moved the
> card remove/insert mechanism out of the suspend/resume path and into
> mmc_pm_notify (which uses pm notifiers; needed in order to let user
> space sync the card upon removal).
> 
> It broke (part of) SDIO suspend because SDIO relied on
> mmc_suspend_host() to remove the card, and squash the error, in case
> -ENOSYS is returned from the bus suspend handler (mmc_sdio_suspend()
> in this case).
> 
> mmc_sdio_suspend() is using this whenever one of the card's SDIO
> function drivers does not have suspend/resume handlers - in that case
> it is agreed to force removal of the entire card.
> 
> So the trivial thing to do here was to bring back that part of
> mmc_suspend_host(), which was removed by
> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e.
> 
> The reason I said the end result is not too beautiful is because now
> we have two places in the mmc suspend execution path that can remove
> the card: mmc_pm_notify and mmc_suspend_host, where the second is
> probably useful only for SDIO. The card re-insertion, btw, will only
> take place in mmc_pm_notify, and be used by all scenarios (so it's
> becoming asymmetric for SDIO).
> 
> To use mmc_pm_notify's card-removal code also for SDIO, we need to
> check there if all the SDIO functions have suspend handlers. That
> would probably make us add a new bus_ops handler (something like
> host->bus_ops->remove_card_on_suspend ?).
> 
> It's starting to be a bit complicated though, and I'm not sure it
> would make the code a lot more readable.
> 
> And, it would still not work for drivers like libertas sdio, which do
> have a suspend hander, but sometimes let it return -ENOSYS, expecting
> the MMC core to remove the card and squash the error. For those cases,
> we still need the old card-removal logic in mmc_suspend_host().
> 
> Btw, it makes me wonder: does libertas_sdio really needs this
> functionality ? When MMC_PM_KEEP_POWER is not needed, why can't
> libertas_sdio just return 0 (and as a result the card will be powered
> off, but not removed) ?

Perhaps we should bring up this question on the libertas-dev list. I am
Cc'ing them in the hope that someone can answer this question.


Sven

[PATCH] sdio: fix suspend/resume regression

[Ohad Ben-Cohen]

Fix SDIO suspend/resume regression introduced by
4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs related to
mmc/sd card insert/removal during suspend/resume":

[ 5647.295953] PM: Syncing filesystems ... done.
[ 5647.318792] Freezing user space processes ... (elapsed 0.01 seconds) done.
[ 5647.337048] Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done.
[ 5647.356915] Suspending console(s) (use no_console_suspend to debug)
[ 5647.366651] pm_op(): platform_pm_suspend+0x0/0x5c returns -38
[ 5647.366671] PM: Device pxa2xx-mci.0 failed to suspend: error -38
[ 5647.367082] PM: Some devices failed to suspend

4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e moved the card removal/insertion
mechanism out of MMC's suspend/resume path and into pm notifiers
(mmc_pm_notify), and that broke SDIO's expectation that mmc_suspend_host()
will remove the card, and squash the error, in case -ENOSYS is returned
from the bus suspend handler (mmc_sdio_suspend() in this case).

mmc_sdio_suspend() is using this whenever at least one of the card's SDIO
function drivers does not have suspend/resume handlers - in that case
it is agreed to force removal of the entire card.

This patch fixes this regression by trivially bringing back that part of
mmc_suspend_host(), which was removed by 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e.

Reported-and-tested-by: Sven Neumann <s.neumann@raumfeld.com>
Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
Cc: Maxim Levitsky <maximlevitsky@gmail.com>
Cc: <stable@kernel.org>
--

It may still be desired to further clean this area up by using the card
removal mechanism in mmc_pm_notify() for SDIO as well.

To use mmc_pm_notify's card-removal code also for SDIO, we need it
to check if all the SDIO functions have suspend handlers. That
would probably make us add a new bus_ops handler (something like
host->bus_ops->remove_card_on_suspend ?).

It's starting to get a bit complicated though, and I'm not sure it
would make the code a lot more readable.

In addition, this would still not work for drivers like libertas sdio,
which do have a suspend handler, but sometimes let it return -ENOSYS,
expecting mmc_suspend_host() to remove the card and squash the error.
For those cases, we still need the old card-removal logic in mmc_suspend_host().

This brings up a question whether libertas_sdio really needs this
functionality; When MMC_PM_KEEP_POWER is not needed, can't it just return 0
(and as a result the card will be powered down, but not removed) ?

Until we have an agreement on this, I suggest we at least fix the
regression with this patch.

Thanks Sven Neumann for reporting and testing the issue and this patch.

 drivers/mmc/core/core.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index c94565d..515ff39 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
 	if (host->bus_ops && !host->bus_dead) {
 		if (host->bus_ops->suspend)
 			err = host->bus_ops->suspend(host);
+		if (err == -ENOSYS || !host->bus_ops->resume) {
+			/*
+			 * We simply "remove" the card in this case.
+			 * It will be redetected on resume.
+			 */
+			if (host->bus_ops->remove)
+				host->bus_ops->remove(host);
+			mmc_claim_host(host);
+			mmc_detach_bus(host);
+			mmc_release_host(host);
+			host->pm_flags = 0;
+			err = 0;
+		}
 	}
 	mmc_bus_put(host);
 
-- 
1.7.0.4

[PATCH] sdio: fix suspend/resume regression

[Ohad Ben-Cohen]

Fix SDIO suspend/resume regression introduced by
4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs related to
mmc/sd card insert/removal during suspend/resume":

[ 5647.295953] PM: Syncing filesystems ... done.
[ 5647.318792] Freezing user space processes ... (elapsed 0.01 seconds) done.
[ 5647.337048] Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done.
[ 5647.356915] Suspending console(s) (use no_console_suspend to debug)
[ 5647.366651] pm_op(): platform_pm_suspend+0x0/0x5c returns -38
[ 5647.366671] PM: Device pxa2xx-mci.0 failed to suspend: error -38
[ 5647.367082] PM: Some devices failed to suspend

4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e moved the card removal/insertion
mechanism out of MMC's suspend/resume path and into pm notifiers
(mmc_pm_notify), and that broke SDIO's expectation that mmc_suspend_host()
will remove the card, and squash the error, in case -ENOSYS is returned
from the bus suspend handler (mmc_sdio_suspend() in this case).

mmc_sdio_suspend() is using this whenever at least one of the card's SDIO
function drivers does not have suspend/resume handlers - in that case
it is agreed to force removal of the entire card.

This patch fixes this regression by trivially bringing back that part of
mmc_suspend_host(), which was removed by 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e.

Reported-and-tested-by: Sven Neumann <s.neumann@raumfeld.com>
Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
Cc: Maxim Levitsky <maximlevitsky@gmail.com>
Cc: <stable@kernel.org>
--

It may still be desired to further clean this area up by using the card
removal mechanism in mmc_pm_notify() for SDIO as well.

To use mmc_pm_notify's card-removal code also for SDIO, we need it
to check if all the SDIO functions have suspend handlers. That
would probably make us add a new bus_ops handler (something like
host->bus_ops->remove_card_on_suspend ?).

It's starting to get a bit complicated though, and I'm not sure it
would make the code a lot more readable.

In addition, this would still not work for drivers like libertas sdio,
which do have a suspend handler, but sometimes let it return -ENOSYS,
expecting mmc_suspend_host() to remove the card and squash the error.
For those cases, we still need the old card-removal logic in mmc_suspend_host().

This brings up a question whether libertas_sdio really needs this
functionality; When MMC_PM_KEEP_POWER is not needed, can't it just return 0
(and as a result the card will be powered down, but not removed) ?

Until we have an agreement on this, I suggest we at least fix the
regression with this patch.

Thanks Sven Neumann for reporting and testing the issue and this patch.

 drivers/mmc/core/core.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index c94565d..515ff39 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
 	if (host->bus_ops && !host->bus_dead) {
 		if (host->bus_ops->suspend)
 			err = host->bus_ops->suspend(host);
+		if (err == -ENOSYS || !host->bus_ops->resume) {
+			/*
+			 * We simply "remove" the card in this case.
+			 * It will be redetected on resume.
+			 */
+			if (host->bus_ops->remove)
+				host->bus_ops->remove(host);
+			mmc_claim_host(host);
+			mmc_detach_bus(host);
+			mmc_release_host(host);
+			host->pm_flags = 0;
+			err = 0;
+		}
 	}
 	mmc_bus_put(host);
 
-- 
1.7.0.4

[PATCH] sdio: fix suspend/resume regression

[Ohad Ben-Cohen]

Fix SDIO suspend/resume regression introduced by
4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs related to
mmc/sd card insert/removal during suspend/resume":

[ 5647.295953] PM: Syncing filesystems ... done.
[ 5647.318792] Freezing user space processes ... (elapsed 0.01 seconds) done.
[ 5647.337048] Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done.
[ 5647.356915] Suspending console(s) (use no_console_suspend to debug)
[ 5647.366651] pm_op(): platform_pm_suspend+0x0/0x5c returns -38
[ 5647.366671] PM: Device pxa2xx-mci.0 failed to suspend: error -38
[ 5647.367082] PM: Some devices failed to suspend

4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e moved the card removal/insertion
mechanism out of MMC's suspend/resume path and into pm notifiers
(mmc_pm_notify), and that broke SDIO's expectation that mmc_suspend_host()
will remove the card, and squash the error, in case -ENOSYS is returned
from the bus suspend handler (mmc_sdio_suspend() in this case).

mmc_sdio_suspend() is using this whenever at least one of the card's SDIO
function drivers does not have suspend/resume handlers - in that case
it is agreed to force removal of the entire card.

This patch fixes this regression by trivially bringing back that part of
mmc_suspend_host(), which was removed by 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e.

Reported-and-tested-by: Sven Neumann <s.neumann@raumfeld.com>
Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
Cc: Maxim Levitsky <maximlevitsky@gmail.com>
Cc: <stable@kernel.org>
--

It may still be desired to further clean this area up by using the card
removal mechanism in mmc_pm_notify() for SDIO as well.

To use mmc_pm_notify's card-removal code also for SDIO, we need it
to check if all the SDIO functions have suspend handlers. That
would probably make us add a new bus_ops handler (something like
host->bus_ops->remove_card_on_suspend ?).

It's starting to get a bit complicated though, and I'm not sure it
would make the code a lot more readable.

In addition, this would still not work for drivers like libertas sdio,
which do have a suspend handler, but sometimes let it return -ENOSYS,
expecting mmc_suspend_host() to remove the card and squash the error.
For those cases, we still need the old card-removal logic in mmc_suspend_host().

This brings up a question whether libertas_sdio really needs this
functionality; When MMC_PM_KEEP_POWER is not needed, can't it just return 0
(and as a result the card will be powered down, but not removed) ?

Until we have an agreement on this, I suggest we at least fix the
regression with this patch.

Thanks Sven Neumann for reporting and testing the issue and this patch.

 drivers/mmc/core/core.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index c94565d..515ff39 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
 	if (host->bus_ops && !host->bus_dead) {
 		if (host->bus_ops->suspend)
 			err = host->bus_ops->suspend(host);
+		if (err == -ENOSYS || !host->bus_ops->resume) {
+			/*
+			 * We simply "remove" the card in this case.
+			 * It will be redetected on resume.
+			 */
+			if (host->bus_ops->remove)
+				host->bus_ops->remove(host);
+			mmc_claim_host(host);
+			mmc_detach_bus(host);
+			mmc_release_host(host);
+			host->pm_flags = 0;
+			err = 0;
+		}
 	}
 	mmc_bus_put(host);
 
-- 
1.7.0.4

Re: [PATCH] sdio: fix suspend/resume regression

[Vitaly Wool]

Hi Ohad,

On Wed, Oct 13, 2010 at 9:31 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:

> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index c94565d..515ff39 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
>        if (host->bus_ops && !host->bus_dead) {
>                if (host->bus_ops->suspend)
>                        err = host->bus_ops->suspend(host);
> +               if (err == -ENOSYS || !host->bus_ops->resume) {
> +                       /*
> +                        * We simply "remove" the card in this case.
> +                        * It will be redetected on resume.
> +                        */
> +                       if (host->bus_ops->remove)
> +                               host->bus_ops->remove(host);
> +                       mmc_claim_host(host);
> +                       mmc_detach_bus(host);
> +                       mmc_release_host(host);
> +                       host->pm_flags = 0;
> +                       err = 0;
> +               }
>        }
>        mmc_bus_put(host);
>

what about moving this logic to sdio bus_ops as this seems to be
completely specific to sdio right now?

Thanks,
   Vitaly

[PATCH] sdio: fix suspend/resume regression

[Vitaly Wool]

Hi Ohad,

On Wed, Oct 13, 2010 at 9:31 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:

> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index c94565d..515ff39 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
> ? ? ? ?if (host->bus_ops && !host->bus_dead) {
> ? ? ? ? ? ? ? ?if (host->bus_ops->suspend)
> ? ? ? ? ? ? ? ? ? ? ? ?err = host->bus_ops->suspend(host);
> + ? ? ? ? ? ? ? if (err == -ENOSYS || !host->bus_ops->resume) {
> + ? ? ? ? ? ? ? ? ? ? ? /*
> + ? ? ? ? ? ? ? ? ? ? ? ?* We simply "remove" the card in this case.
> + ? ? ? ? ? ? ? ? ? ? ? ?* It will be redetected on resume.
> + ? ? ? ? ? ? ? ? ? ? ? ?*/
> + ? ? ? ? ? ? ? ? ? ? ? if (host->bus_ops->remove)
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? host->bus_ops->remove(host);
> + ? ? ? ? ? ? ? ? ? ? ? mmc_claim_host(host);
> + ? ? ? ? ? ? ? ? ? ? ? mmc_detach_bus(host);
> + ? ? ? ? ? ? ? ? ? ? ? mmc_release_host(host);
> + ? ? ? ? ? ? ? ? ? ? ? host->pm_flags = 0;
> + ? ? ? ? ? ? ? ? ? ? ? err = 0;
> + ? ? ? ? ? ? ? }
> ? ? ? ?}
> ? ? ? ?mmc_bus_put(host);
>

what about moving this logic to sdio bus_ops as this seems to be
completely specific to sdio right now?

Thanks,
   Vitaly

Re: [PATCH] sdio: fix suspend/resume regression

[Ohad Ben-Cohen]

Hi Vitaly,

On Wed, Oct 13, 2010 at 9:54 AM, Vitaly Wool <vitalywool@gmail.com> wrote:
> what about moving this logic to sdio bus_ops as this seems to be
> completely specific to sdio right now?

I actually prefer not to have any SDIO-specific logic of that kind at
all, and instead, to use the logic we already have in mmc_pm_notify
for all types of MMC buses.

I resent this patch in the hope of stimulating this discussion, since
it seemed to have stalled since the last message:

http://lkml.org/lkml/2010/10/11/70

It seems to work ;)

Thanks,
Ohad.

[PATCH] sdio: fix suspend/resume regression

[Ohad Ben-Cohen]

Hi Vitaly,

On Wed, Oct 13, 2010 at 9:54 AM, Vitaly Wool <vitalywool@gmail.com> wrote:
> what about moving this logic to sdio bus_ops as this seems to be
> completely specific to sdio right now?

I actually prefer not to have any SDIO-specific logic of that kind at
all, and instead, to use the logic we already have in mmc_pm_notify
for all types of MMC buses.

I resent this patch in the hope of stimulating this discussion, since
it seemed to have stalled since the last message:

http://lkml.org/lkml/2010/10/11/70

It seems to work ;)

Thanks,
Ohad.

Re: [PATCH] sdio: fix suspend/resume regression

[Vitaly Wool]

On Wed, Oct 13, 2010 at 10:55 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Hi Vitaly,
>
> On Wed, Oct 13, 2010 at 9:54 AM, Vitaly Wool <vitalywool@gmail.com> wrote:
>> what about moving this logic to sdio bus_ops as this seems to be
>> completely specific to sdio right now?
>
> I actually prefer not to have any SDIO-specific logic of that kind at
> all, and instead, to use the logic we already have in mmc_pm_notify
> for all types of MMC buses.

I don't really see the point. Instead of having SDIO-specific handling
in the SDIO specific code, we're trying to address SDIO specific
corner cases elsewhere. What's the rationale?

Thanks,
   Vitaly

[PATCH] sdio: fix suspend/resume regression

[Vitaly Wool]

On Wed, Oct 13, 2010 at 10:55 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Hi Vitaly,
>
> On Wed, Oct 13, 2010 at 9:54 AM, Vitaly Wool <vitalywool@gmail.com> wrote:
>> what about moving this logic to sdio bus_ops as this seems to be
>> completely specific to sdio right now?
>
> I actually prefer not to have any SDIO-specific logic of that kind at
> all, and instead, to use the logic we already have in mmc_pm_notify
> for all types of MMC buses.

I don't really see the point. Instead of having SDIO-specific handling
in the SDIO specific code, we're trying to address SDIO specific
corner cases elsewhere. What's the rationale?

Thanks,
   Vitaly

Re: [PATCH] sdio: fix suspend/resume regression

[Ohad Ben-Cohen]

On Wed, Oct 13, 2010 at 11:06 AM, Vitaly Wool <vitalywool@gmail.com> wrote:
> I don't really see the point. Instead of having SDIO-specific handling
> in the SDIO specific code, we're trying to address SDIO specific
> corner cases elsewhere. What's the rationale?

MMC core already has a card-removal mechanism which is bus independent
(in mmc_pm_notify).

Why would you prefer to duplicate this code for SDIO ?

Thanks,
Ohad.

[PATCH] sdio: fix suspend/resume regression

[Ohad Ben-Cohen]

On Wed, Oct 13, 2010 at 11:06 AM, Vitaly Wool <vitalywool@gmail.com> wrote:
> I don't really see the point. Instead of having SDIO-specific handling
> in the SDIO specific code, we're trying to address SDIO specific
> corner cases elsewhere. What's the rationale?

MMC core already has a card-removal mechanism which is bus independent
(in mmc_pm_notify).

Why would you prefer to duplicate this code for SDIO ?

Thanks,
Ohad.

Re: [PATCH] sdio: fix suspend/resume regression

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1370 bytes --]

On Wed, 13 Oct 2010, Vitaly Wool wrote:

> Hi Ohad,
> 
> On Wed, Oct 13, 2010 at 9:31 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> 
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index c94565d..515ff39 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
> >        if (host->bus_ops && !host->bus_dead) {
> >                if (host->bus_ops->suspend)
> >                        err = host->bus_ops->suspend(host);
> > +               if (err == -ENOSYS || !host->bus_ops->resume) {
> > +                       /*
> > +                        * We simply "remove" the card in this case.
> > +                        * It will be redetected on resume.
> > +                        */
> > +                       if (host->bus_ops->remove)
> > +                               host->bus_ops->remove(host);
> > +                       mmc_claim_host(host);
> > +                       mmc_detach_bus(host);
> > +                       mmc_release_host(host);
> > +                       host->pm_flags = 0;
> > +                       err = 0;
> > +               }
> >        }
> >        mmc_bus_put(host);
> >
> 
> what about moving this logic to sdio bus_ops as this seems to be
> completely specific to sdio right now?

It doesn't have to.


Nicolas

[PATCH] sdio: fix suspend/resume regression

[Nicolas Pitre]

On Wed, 13 Oct 2010, Vitaly Wool wrote:

> Hi Ohad,
> 
> On Wed, Oct 13, 2010 at 9:31 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> 
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index c94565d..515ff39 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
> > ? ? ? ?if (host->bus_ops && !host->bus_dead) {
> > ? ? ? ? ? ? ? ?if (host->bus_ops->suspend)
> > ? ? ? ? ? ? ? ? ? ? ? ?err = host->bus_ops->suspend(host);
> > + ? ? ? ? ? ? ? if (err == -ENOSYS || !host->bus_ops->resume) {
> > + ? ? ? ? ? ? ? ? ? ? ? /*
> > + ? ? ? ? ? ? ? ? ? ? ? ?* We simply "remove" the card in this case.
> > + ? ? ? ? ? ? ? ? ? ? ? ?* It will be redetected on resume.
> > + ? ? ? ? ? ? ? ? ? ? ? ?*/
> > + ? ? ? ? ? ? ? ? ? ? ? if (host->bus_ops->remove)
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? host->bus_ops->remove(host);
> > + ? ? ? ? ? ? ? ? ? ? ? mmc_claim_host(host);
> > + ? ? ? ? ? ? ? ? ? ? ? mmc_detach_bus(host);
> > + ? ? ? ? ? ? ? ? ? ? ? mmc_release_host(host);
> > + ? ? ? ? ? ? ? ? ? ? ? host->pm_flags = 0;
> > + ? ? ? ? ? ? ? ? ? ? ? err = 0;
> > + ? ? ? ? ? ? ? }
> > ? ? ? ?}
> > ? ? ? ?mmc_bus_put(host);
> >
> 
> what about moving this logic to sdio bus_ops as this seems to be
> completely specific to sdio right now?

It doesn't have to.


Nicolas

Re: [PATCH] sdio: fix suspend/resume regression

[Nicolas Pitre]

On Wed, 13 Oct 2010, Ohad Ben-Cohen wrote:

> Fix SDIO suspend/resume regression introduced by
> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs related to
> mmc/sd card insert/removal during suspend/resume":
> 
> [ 5647.295953] PM: Syncing filesystems ... done.
> [ 5647.318792] Freezing user space processes ... (elapsed 0.01 seconds) done.
> [ 5647.337048] Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done.
> [ 5647.356915] Suspending console(s) (use no_console_suspend to debug)
> [ 5647.366651] pm_op(): platform_pm_suspend+0x0/0x5c returns -38
> [ 5647.366671] PM: Device pxa2xx-mci.0 failed to suspend: error -38
> [ 5647.367082] PM: Some devices failed to suspend
> 
> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e moved the card removal/insertion
> mechanism out of MMC's suspend/resume path and into pm notifiers
> (mmc_pm_notify), and that broke SDIO's expectation that mmc_suspend_host()
> will remove the card, and squash the error, in case -ENOSYS is returned
> from the bus suspend handler (mmc_sdio_suspend() in this case).
> 
> mmc_sdio_suspend() is using this whenever at least one of the card's SDIO
> function drivers does not have suspend/resume handlers - in that case
> it is agreed to force removal of the entire card.
> 
> This patch fixes this regression by trivially bringing back that part of
> mmc_suspend_host(), which was removed by 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e.
> 
> Reported-and-tested-by: Sven Neumann <s.neumann@raumfeld.com>
> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
> Cc: Maxim Levitsky <maximlevitsky@gmail.com>
> Cc: <stable@kernel.org>

Acked-by: Nicolas Pitre <nico@fluxnic.net>

> --
> 
> It may still be desired to further clean this area up by using the card
> removal mechanism in mmc_pm_notify() for SDIO as well.
> 
> To use mmc_pm_notify's card-removal code also for SDIO, we need it
> to check if all the SDIO functions have suspend handlers. That
> would probably make us add a new bus_ops handler (something like
> host->bus_ops->remove_card_on_suspend ?).
> 
> It's starting to get a bit complicated though, and I'm not sure it
> would make the code a lot more readable.
> 
> In addition, this would still not work for drivers like libertas sdio,
> which do have a suspend handler, but sometimes let it return -ENOSYS,
> expecting mmc_suspend_host() to remove the card and squash the error.
> For those cases, we still need the old card-removal logic in mmc_suspend_host().
> 
> This brings up a question whether libertas_sdio really needs this
> functionality; When MMC_PM_KEEP_POWER is not needed, can't it just return 0
> (and as a result the card will be powered down, but not removed) ?

It could, but then there has to be extra code in the resume handler to 
cope with the fact that the card was powered off.  When the card is 
simply removed, then redetecting and reinserting the card on resume is 
trivial.


Nicolas

[PATCH] sdio: fix suspend/resume regression

[Nicolas Pitre]

On Wed, 13 Oct 2010, Ohad Ben-Cohen wrote:

> Fix SDIO suspend/resume regression introduced by
> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs related to
> mmc/sd card insert/removal during suspend/resume":
> 
> [ 5647.295953] PM: Syncing filesystems ... done.
> [ 5647.318792] Freezing user space processes ... (elapsed 0.01 seconds) done.
> [ 5647.337048] Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done.
> [ 5647.356915] Suspending console(s) (use no_console_suspend to debug)
> [ 5647.366651] pm_op(): platform_pm_suspend+0x0/0x5c returns -38
> [ 5647.366671] PM: Device pxa2xx-mci.0 failed to suspend: error -38
> [ 5647.367082] PM: Some devices failed to suspend
> 
> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e moved the card removal/insertion
> mechanism out of MMC's suspend/resume path and into pm notifiers
> (mmc_pm_notify), and that broke SDIO's expectation that mmc_suspend_host()
> will remove the card, and squash the error, in case -ENOSYS is returned
> from the bus suspend handler (mmc_sdio_suspend() in this case).
> 
> mmc_sdio_suspend() is using this whenever at least one of the card's SDIO
> function drivers does not have suspend/resume handlers - in that case
> it is agreed to force removal of the entire card.
> 
> This patch fixes this regression by trivially bringing back that part of
> mmc_suspend_host(), which was removed by 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e.
> 
> Reported-and-tested-by: Sven Neumann <s.neumann@raumfeld.com>
> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
> Cc: Maxim Levitsky <maximlevitsky@gmail.com>
> Cc: <stable@kernel.org>

Acked-by: Nicolas Pitre <nico@fluxnic.net>

> --
> 
> It may still be desired to further clean this area up by using the card
> removal mechanism in mmc_pm_notify() for SDIO as well.
> 
> To use mmc_pm_notify's card-removal code also for SDIO, we need it
> to check if all the SDIO functions have suspend handlers. That
> would probably make us add a new bus_ops handler (something like
> host->bus_ops->remove_card_on_suspend ?).
> 
> It's starting to get a bit complicated though, and I'm not sure it
> would make the code a lot more readable.
> 
> In addition, this would still not work for drivers like libertas sdio,
> which do have a suspend handler, but sometimes let it return -ENOSYS,
> expecting mmc_suspend_host() to remove the card and squash the error.
> For those cases, we still need the old card-removal logic in mmc_suspend_host().
> 
> This brings up a question whether libertas_sdio really needs this
> functionality; When MMC_PM_KEEP_POWER is not needed, can't it just return 0
> (and as a result the card will be powered down, but not removed) ?

It could, but then there has to be extra code in the resume handler to 
cope with the fact that the card was powered off.  When the card is 
simply removed, then redetecting and reinserting the card on resume is 
trivial.


Nicolas

Re: [PATCH] sdio: fix suspend/resume regression

[Ohad Ben-Cohen]

On Wed, Oct 13, 2010 at 10:08 PM, Nicolas Pitre <nico@fluxnic.net> wrote:
> It could, but then there has to be extra code in the resume handler to
> cope with the fact that the card was powered off.

Good point.

It's much better the way it is now.

Thanks,
Ohad.

[PATCH] sdio: fix suspend/resume regression

[Ohad Ben-Cohen]

On Wed, Oct 13, 2010 at 10:08 PM, Nicolas Pitre <nico@fluxnic.net> wrote:
> It could, but then there has to be extra code in the resume handler to
> cope with the fact that the card was powered off.

Good point.

It's much better the way it is now.

Thanks,
Ohad.

Re: [PATCH] sdio: fix suspend/resume regression

[Chris Ball]

Hi Ohad,

On Wed, Oct 13, 2010 at 09:31:56AM +0200, Ohad Ben-Cohen wrote:
> Fix SDIO suspend/resume regression introduced by
> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs related to
> mmc/sd card insert/removal during suspend/resume":
> 
> [ 5647.295953] PM: Syncing filesystems ... done.
> [ 5647.318792] Freezing user space processes ... (elapsed 0.01 seconds) done.
> [ 5647.337048] Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done.
> [ 5647.356915] Suspending console(s) (use no_console_suspend to debug)
> [ 5647.366651] pm_op(): platform_pm_suspend+0x0/0x5c returns -38
> [ 5647.366671] PM: Device pxa2xx-mci.0 failed to suspend: error -38
> [ 5647.367082] PM: Some devices failed to suspend
> 
> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e moved the card removal/insertion
> mechanism out of MMC's suspend/resume path and into pm notifiers
> (mmc_pm_notify), and that broke SDIO's expectation that mmc_suspend_host()
> will remove the card, and squash the error, in case -ENOSYS is returned
> from the bus suspend handler (mmc_sdio_suspend() in this case).
> 
> mmc_sdio_suspend() is using this whenever at least one of the card's SDIO
> function drivers does not have suspend/resume handlers - in that case
> it is agreed to force removal of the entire card.
> 
> This patch fixes this regression by trivially bringing back that part of
> mmc_suspend_host(), which was removed by 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e.
> 
> Reported-and-tested-by: Sven Neumann <s.neumann@raumfeld.com>
> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
> Cc: Maxim Levitsky <maximlevitsky@gmail.com>
> Cc: <stable@kernel.org>
> --
> 
> It may still be desired to further clean this area up by using the card
> removal mechanism in mmc_pm_notify() for SDIO as well.
> 
> To use mmc_pm_notify's card-removal code also for SDIO, we need it
> to check if all the SDIO functions have suspend handlers. That
> would probably make us add a new bus_ops handler (something like
> host->bus_ops->remove_card_on_suspend ?).
> 
> It's starting to get a bit complicated though, and I'm not sure it
> would make the code a lot more readable.
> 
> In addition, this would still not work for drivers like libertas sdio,
> which do have a suspend handler, but sometimes let it return -ENOSYS,
> expecting mmc_suspend_host() to remove the card and squash the error.
> For those cases, we still need the old card-removal logic in mmc_suspend_host().
> 
> This brings up a question whether libertas_sdio really needs this
> functionality; When MMC_PM_KEEP_POWER is not needed, can't it just return 0
> (and as a result the card will be powered down, but not removed) ?
> 
> Until we have an agreement on this, I suggest we at least fix the
> regression with this patch.
> 
> Thanks Sven Neumann for reporting and testing the issue and this patch.
> 
>  drivers/mmc/core/core.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index c94565d..515ff39 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
>  	if (host->bus_ops && !host->bus_dead) {
>  		if (host->bus_ops->suspend)
>  			err = host->bus_ops->suspend(host);
> +		if (err == -ENOSYS || !host->bus_ops->resume) {
> +			/*
> +			 * We simply "remove" the card in this case.
> +			 * It will be redetected on resume.
> +			 */
> +			if (host->bus_ops->remove)
> +				host->bus_ops->remove(host);
> +			mmc_claim_host(host);
> +			mmc_detach_bus(host);
> +			mmc_release_host(host);
> +			host->pm_flags = 0;
> +			err = 0;
> +		}
>  	}
>  	mmc_bus_put(host);
>  
> -- 

Thanks very much, I've applied this to mmc-next with Nicolas' ACK.

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

[PATCH] sdio: fix suspend/resume regression

[Chris Ball]

Hi Ohad,

On Wed, Oct 13, 2010 at 09:31:56AM +0200, Ohad Ben-Cohen wrote:
> Fix SDIO suspend/resume regression introduced by
> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs related to
> mmc/sd card insert/removal during suspend/resume":
> 
> [ 5647.295953] PM: Syncing filesystems ... done.
> [ 5647.318792] Freezing user space processes ... (elapsed 0.01 seconds) done.
> [ 5647.337048] Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done.
> [ 5647.356915] Suspending console(s) (use no_console_suspend to debug)
> [ 5647.366651] pm_op(): platform_pm_suspend+0x0/0x5c returns -38
> [ 5647.366671] PM: Device pxa2xx-mci.0 failed to suspend: error -38
> [ 5647.367082] PM: Some devices failed to suspend
> 
> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e moved the card removal/insertion
> mechanism out of MMC's suspend/resume path and into pm notifiers
> (mmc_pm_notify), and that broke SDIO's expectation that mmc_suspend_host()
> will remove the card, and squash the error, in case -ENOSYS is returned
> from the bus suspend handler (mmc_sdio_suspend() in this case).
> 
> mmc_sdio_suspend() is using this whenever at least one of the card's SDIO
> function drivers does not have suspend/resume handlers - in that case
> it is agreed to force removal of the entire card.
> 
> This patch fixes this regression by trivially bringing back that part of
> mmc_suspend_host(), which was removed by 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e.
> 
> Reported-and-tested-by: Sven Neumann <s.neumann@raumfeld.com>
> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
> Cc: Maxim Levitsky <maximlevitsky@gmail.com>
> Cc: <stable@kernel.org>
> --
> 
> It may still be desired to further clean this area up by using the card
> removal mechanism in mmc_pm_notify() for SDIO as well.
> 
> To use mmc_pm_notify's card-removal code also for SDIO, we need it
> to check if all the SDIO functions have suspend handlers. That
> would probably make us add a new bus_ops handler (something like
> host->bus_ops->remove_card_on_suspend ?).
> 
> It's starting to get a bit complicated though, and I'm not sure it
> would make the code a lot more readable.
> 
> In addition, this would still not work for drivers like libertas sdio,
> which do have a suspend handler, but sometimes let it return -ENOSYS,
> expecting mmc_suspend_host() to remove the card and squash the error.
> For those cases, we still need the old card-removal logic in mmc_suspend_host().
> 
> This brings up a question whether libertas_sdio really needs this
> functionality; When MMC_PM_KEEP_POWER is not needed, can't it just return 0
> (and as a result the card will be powered down, but not removed) ?
> 
> Until we have an agreement on this, I suggest we at least fix the
> regression with this patch.
> 
> Thanks Sven Neumann for reporting and testing the issue and this patch.
> 
>  drivers/mmc/core/core.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index c94565d..515ff39 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
>  	if (host->bus_ops && !host->bus_dead) {
>  		if (host->bus_ops->suspend)
>  			err = host->bus_ops->suspend(host);
> +		if (err == -ENOSYS || !host->bus_ops->resume) {
> +			/*
> +			 * We simply "remove" the card in this case.
> +			 * It will be redetected on resume.
> +			 */
> +			if (host->bus_ops->remove)
> +				host->bus_ops->remove(host);
> +			mmc_claim_host(host);
> +			mmc_detach_bus(host);
> +			mmc_release_host(host);
> +			host->pm_flags = 0;
> +			err = 0;
> +		}
>  	}
>  	mmc_bus_put(host);
>  
> -- 

Thanks very much, I've applied this to mmc-next with Nicolas' ACK.

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

Re: [PATCH] sdio: fix suspend/resume regression

[Ohad Ben-Cohen]

Hi Chris,

On Thu, Oct 14, 2010 at 4:24 AM, Chris Ball <cjb@laptop.org> wrote:
> On Wed, Oct 13, 2010 at 09:31:56AM +0200, Ohad Ben-Cohen wrote:
>> Fix SDIO suspend/resume regression introduced by
>> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs related to
>> mmc/sd card insert/removal during suspend/resume":

> Thanks very much, I've applied this to mmc-next with Nicolas' ACK.

Thanks a lot, Chris.

Btw, you might want to consider this as a 2.6.36 material, since it
fixes a regression introduced in this cycle.

Thanks,
Ohad.

[PATCH] sdio: fix suspend/resume regression

[Ohad Ben-Cohen]

Hi Chris,

On Thu, Oct 14, 2010 at 4:24 AM, Chris Ball <cjb@laptop.org> wrote:
> On Wed, Oct 13, 2010 at 09:31:56AM +0200, Ohad Ben-Cohen wrote:
>> Fix SDIO suspend/resume regression introduced by
>> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs related to
>> mmc/sd card insert/removal during suspend/resume":

> Thanks very much, I've applied this to mmc-next with Nicolas' ACK.

Thanks a lot, Chris.

Btw, you might want to consider this as a 2.6.36 material, since it
fixes a regression introduced in this cycle.

Thanks,
Ohad.

Re: [PATCH] sdio: fix suspend/resume regression

[Maxim Levitsky]

On Wed, 2010-10-13 at 09:31 +0200, Ohad Ben-Cohen wrote:
> Fix SDIO suspend/resume regression introduced by
> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs related to
> mmc/sd card insert/removal during suspend/resume":
> 
> [ 5647.295953] PM: Syncing filesystems ... done.
> [ 5647.318792] Freezing user space processes ... (elapsed 0.01 seconds) done.
> [ 5647.337048] Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done.
> [ 5647.356915] Suspending console(s) (use no_console_suspend to debug)
> [ 5647.366651] pm_op(): platform_pm_suspend+0x0/0x5c returns -38
> [ 5647.366671] PM: Device pxa2xx-mci.0 failed to suspend: error -38
> [ 5647.367082] PM: Some devices failed to suspend
> 
> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e moved the card removal/insertion
> mechanism out of MMC's suspend/resume path and into pm notifiers
> (mmc_pm_notify), and that broke SDIO's expectation that mmc_suspend_host()
> will remove the card, and squash the error, in case -ENOSYS is returned
> from the bus suspend handler (mmc_sdio_suspend() in this case).
> 
> mmc_sdio_suspend() is using this whenever at least one of the card's SDIO
> function drivers does not have suspend/resume handlers - in that case
> it is agreed to force removal of the entire card.
> 
> This patch fixes this regression by trivially bringing back that part of
> mmc_suspend_host(), which was removed by 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e.
> 
> Reported-and-tested-by: Sven Neumann <s.neumann@raumfeld.com>
> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
> Cc: Maxim Levitsky <maximlevitsky@gmail.com>
> Cc: <stable@kernel.org>
> --
> 
> It may still be desired to further clean this area up by using the card
> removal mechanism in mmc_pm_notify() for SDIO as well.
> 
> To use mmc_pm_notify's card-removal code also for SDIO, we need it
> to check if all the SDIO functions have suspend handlers. That
> would probably make us add a new bus_ops handler (something like
> host->bus_ops->remove_card_on_suspend ?).
> 
> It's starting to get a bit complicated though, and I'm not sure it
> would make the code a lot more readable.
> 
> In addition, this would still not work for drivers like libertas sdio,
> which do have a suspend handler, but sometimes let it return -ENOSYS,
> expecting mmc_suspend_host() to remove the card and squash the error.
> For those cases, we still need the old card-removal logic in mmc_suspend_host().
> 
> This brings up a question whether libertas_sdio really needs this
> functionality; When MMC_PM_KEEP_POWER is not needed, can't it just return 0
> (and as a result the card will be powered down, but not removed) ?
> 
> Until we have an agreement on this, I suggest we at least fix the
> regression with this patch.
> 
> Thanks Sven Neumann for reporting and testing the issue and this patch.
> 
>  drivers/mmc/core/core.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index c94565d..515ff39 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
>  	if (host->bus_ops && !host->bus_dead) {
>  		if (host->bus_ops->suspend)
>  			err = host->bus_ops->suspend(host);
> +		if (err == -ENOSYS || !host->bus_ops->resume) {
This reintroduces the bug I fixed.

if the CONFIG_MMC_UNSAFE_RESUME isn't set (and that is default
unfortunately), the host->bus_ops->resume will be NULL (see core/mmc.c
mmc_ops), and therefore card will be removed, that will trigger a block
device removal, sync, and deadlock).

I actually thought about the sdio case. Sorry for breaking yet.

My idea was to move the effective suspend/resume to a pm notifier,
and the mmc_pm_notify is supposed to do the job.

Could you test why it fails?

The relevant code in mmc_pm_notify:

		if (!host->bus_ops || host->bus_ops->suspend)
			break;

		mmc_claim_host(host);

		if (host->bus_ops->remove)
			host->bus_ops->remove(host);

		mmc_detach_bus(host);
		mmc_release_host(host);
		host->pm_flags = 0;
		break;


So NULL host->bus_ops->suspend should trigger a card remove by that
function and it did here on my system without CONFIG_MMC_UNSAFE_RESUME.

I suspect that in your case, the .suspend isn't NULL, but .resume is.
Then, we just need an one liner change to mmc_pm_notify to account that
case.

Note that I don't call the  host->bus_ops->suspend(host); in
mmc_pm_notify on purpose as it is too early.

So what happens if you set .suspend to NULL? instead of -ENOSYS return?



> +			/*
> +			 * We simply "remove" the card in this case.
> +			 * It will be redetected on resume.
> +			 */
> +			if (host->bus_ops->remove)
> +				host->bus_ops->remove(host);
> +			mmc_claim_host(host);
> +			mmc_detach_bus(host);
> +			mmc_release_host(host);
> +			host->pm_flags = 0;
> +			err = 0;
> +		}
>  	}
>  	mmc_bus_put(host);
>  

Best regards,
	Maxim Levitsky

[PATCH] sdio: fix suspend/resume regression

[Maxim Levitsky]

On Wed, 2010-10-13 at 09:31 +0200, Ohad Ben-Cohen wrote:
> Fix SDIO suspend/resume regression introduced by
> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs related to
> mmc/sd card insert/removal during suspend/resume":
> 
> [ 5647.295953] PM: Syncing filesystems ... done.
> [ 5647.318792] Freezing user space processes ... (elapsed 0.01 seconds) done.
> [ 5647.337048] Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done.
> [ 5647.356915] Suspending console(s) (use no_console_suspend to debug)
> [ 5647.366651] pm_op(): platform_pm_suspend+0x0/0x5c returns -38
> [ 5647.366671] PM: Device pxa2xx-mci.0 failed to suspend: error -38
> [ 5647.367082] PM: Some devices failed to suspend
> 
> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e moved the card removal/insertion
> mechanism out of MMC's suspend/resume path and into pm notifiers
> (mmc_pm_notify), and that broke SDIO's expectation that mmc_suspend_host()
> will remove the card, and squash the error, in case -ENOSYS is returned
> from the bus suspend handler (mmc_sdio_suspend() in this case).
> 
> mmc_sdio_suspend() is using this whenever at least one of the card's SDIO
> function drivers does not have suspend/resume handlers - in that case
> it is agreed to force removal of the entire card.
> 
> This patch fixes this regression by trivially bringing back that part of
> mmc_suspend_host(), which was removed by 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e.
> 
> Reported-and-tested-by: Sven Neumann <s.neumann@raumfeld.com>
> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
> Cc: Maxim Levitsky <maximlevitsky@gmail.com>
> Cc: <stable@kernel.org>
> --
> 
> It may still be desired to further clean this area up by using the card
> removal mechanism in mmc_pm_notify() for SDIO as well.
> 
> To use mmc_pm_notify's card-removal code also for SDIO, we need it
> to check if all the SDIO functions have suspend handlers. That
> would probably make us add a new bus_ops handler (something like
> host->bus_ops->remove_card_on_suspend ?).
> 
> It's starting to get a bit complicated though, and I'm not sure it
> would make the code a lot more readable.
> 
> In addition, this would still not work for drivers like libertas sdio,
> which do have a suspend handler, but sometimes let it return -ENOSYS,
> expecting mmc_suspend_host() to remove the card and squash the error.
> For those cases, we still need the old card-removal logic in mmc_suspend_host().
> 
> This brings up a question whether libertas_sdio really needs this
> functionality; When MMC_PM_KEEP_POWER is not needed, can't it just return 0
> (and as a result the card will be powered down, but not removed) ?
> 
> Until we have an agreement on this, I suggest we at least fix the
> regression with this patch.
> 
> Thanks Sven Neumann for reporting and testing the issue and this patch.
> 
>  drivers/mmc/core/core.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index c94565d..515ff39 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
>  	if (host->bus_ops && !host->bus_dead) {
>  		if (host->bus_ops->suspend)
>  			err = host->bus_ops->suspend(host);
> +		if (err == -ENOSYS || !host->bus_ops->resume) {
This reintroduces the bug I fixed.

if the CONFIG_MMC_UNSAFE_RESUME isn't set (and that is default
unfortunately), the host->bus_ops->resume will be NULL (see core/mmc.c
mmc_ops), and therefore card will be removed, that will trigger a block
device removal, sync, and deadlock).

I actually thought about the sdio case. Sorry for breaking yet.

My idea was to move the effective suspend/resume to a pm notifier,
and the mmc_pm_notify is supposed to do the job.

Could you test why it fails?

The relevant code in mmc_pm_notify:

		if (!host->bus_ops || host->bus_ops->suspend)
			break;

		mmc_claim_host(host);

		if (host->bus_ops->remove)
			host->bus_ops->remove(host);

		mmc_detach_bus(host);
		mmc_release_host(host);
		host->pm_flags = 0;
		break;


So NULL host->bus_ops->suspend should trigger a card remove by that
function and it did here on my system without CONFIG_MMC_UNSAFE_RESUME.

I suspect that in your case, the .suspend isn't NULL, but .resume is.
Then, we just need an one liner change to mmc_pm_notify to account that
case.

Note that I don't call the  host->bus_ops->suspend(host); in
mmc_pm_notify on purpose as it is too early.

So what happens if you set .suspend to NULL? instead of -ENOSYS return?



> +			/*
> +			 * We simply "remove" the card in this case.
> +			 * It will be redetected on resume.
> +			 */
> +			if (host->bus_ops->remove)
> +				host->bus_ops->remove(host);
> +			mmc_claim_host(host);
> +			mmc_detach_bus(host);
> +			mmc_release_host(host);
> +			host->pm_flags = 0;
> +			err = 0;
> +		}
>  	}
>  	mmc_bus_put(host);
>  

Best regards,
	Maxim Levitsky

Re: [PATCH] sdio: fix suspend/resume regression

[Nicolas Pitre]

On Fri, 22 Oct 2010, Maxim Levitsky wrote:

> So what happens if you set .suspend to NULL? instead of -ENOSYS return?

At run time, the driver may decide to handle the suspend request and 
return 0, or decide to simply return -ENOSYS if it doesn't want to 
bother with the suspend meaning that the card should be removed, or 
return another error code meaning that the suspend request must be 
aborted.



Nicolas

[PATCH] sdio: fix suspend/resume regression

[Nicolas Pitre]

On Fri, 22 Oct 2010, Maxim Levitsky wrote:

> So what happens if you set .suspend to NULL? instead of -ENOSYS return?

At run time, the driver may decide to handle the suspend request and 
return 0, or decide to simply return -ENOSYS if it doesn't want to 
bother with the suspend meaning that the card should be removed, or 
return another error code meaning that the suspend request must be 
aborted.



Nicolas

Re: [PATCH] sdio: fix suspend/resume regression

[Maxim Levitsky]

On Thu, 2010-10-21 at 19:55 -0400, Nicolas Pitre wrote:
> On Fri, 22 Oct 2010, Maxim Levitsky wrote:
> 
> > So what happens if you set .suspend to NULL? instead of -ENOSYS return?
> 
> At run time, the driver may decide to handle the suspend request and 
> return 0, or decide to simply return -ENOSYS if it doesn't want to 
> bother with the suspend meaning that the card should be removed, or 
> return another error code meaning that the suspend request must be 
> aborted.

I am afraid that this is very hard to support.
If .suspend would be called from pm notifier, it would create problems
last time I checked.
Or maybe it won't?
I will test that again, as it might be very nice solution.
I could even just call the mmc_suspend_host from the pm notifier.

Best regards,
	Maxim Levitsky

[PATCH] sdio: fix suspend/resume regression

[Maxim Levitsky]

On Thu, 2010-10-21 at 19:55 -0400, Nicolas Pitre wrote:
> On Fri, 22 Oct 2010, Maxim Levitsky wrote:
> 
> > So what happens if you set .suspend to NULL? instead of -ENOSYS return?
> 
> At run time, the driver may decide to handle the suspend request and 
> return 0, or decide to simply return -ENOSYS if it doesn't want to 
> bother with the suspend meaning that the card should be removed, or 
> return another error code meaning that the suspend request must be 
> aborted.

I am afraid that this is very hard to support.
If .suspend would be called from pm notifier, it would create problems
last time I checked.
Or maybe it won't?
I will test that again, as it might be very nice solution.
I could even just call the mmc_suspend_host from the pm notifier.

Best regards,
	Maxim Levitsky

Re: [PATCH] sdio: fix suspend/resume regression

[Nicolas Pitre]

On Fri, 22 Oct 2010, Maxim Levitsky wrote:

> This reintroduces the bug I fixed.
> 
> if the CONFIG_MMC_UNSAFE_RESUME isn't set (and that is default
> unfortunately), the host->bus_ops->resume will be NULL (see core/mmc.c
> mmc_ops), and therefore card will be removed, that will trigger a block
> device removal, sync, and deadlock).

Maybe mmc_ops should have a non NULL resume method then?


Nicolas

[PATCH] sdio: fix suspend/resume regression

[Nicolas Pitre]

On Fri, 22 Oct 2010, Maxim Levitsky wrote:

> This reintroduces the bug I fixed.
> 
> if the CONFIG_MMC_UNSAFE_RESUME isn't set (and that is default
> unfortunately), the host->bus_ops->resume will be NULL (see core/mmc.c
> mmc_ops), and therefore card will be removed, that will trigger a block
> device removal, sync, and deadlock).

Maybe mmc_ops should have a non NULL resume method then?


Nicolas

Re: [PATCH] sdio: fix suspend/resume regression

[Ohad Ben-Cohen]

Hi Maxim,

On Fri, Oct 22, 2010 at 1:47 AM, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> On Wed, 2010-10-13 at 09:31 +0200, Ohad Ben-Cohen wrote:
>> Fix SDIO suspend/resume regression introduced by
>> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs related to
>> mmc/sd card insert/removal during suspend/resume":
...
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index c94565d..515ff39 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
>>       if (host->bus_ops && !host->bus_dead) {
>>               if (host->bus_ops->suspend)
>>                       err = host->bus_ops->suspend(host);
>> +             if (err == -ENOSYS || !host->bus_ops->resume) {
> This reintroduces the bug I fixed.
>
> if the CONFIG_MMC_UNSAFE_RESUME isn't set (and that is default
> unfortunately), the host->bus_ops->resume will be NULL (see core/mmc.c
> mmc_ops), and therefore card will be removed, that will trigger a block
> device removal, sync, and deadlock).

Have you reproduced this or is it just a general concern ?

I'm asking this because if CONFIG_MMC_UNSAFE_RESUME isn't set, then
mmc_pm_notify() will remove the card and detach the bus.

As a result, host->bus_ops will be unset, and host->bus_dead will be set.

In this case, mmc_suspend_host() should skip the code you are concerned about.

Regards,
Ohad.

[PATCH] sdio: fix suspend/resume regression

[Ohad Ben-Cohen]

Hi Maxim,

On Fri, Oct 22, 2010 at 1:47 AM, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> On Wed, 2010-10-13 at 09:31 +0200, Ohad Ben-Cohen wrote:
>> Fix SDIO suspend/resume regression introduced by
>> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs related to
>> mmc/sd card insert/removal during suspend/resume":
...
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index c94565d..515ff39 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
>> ? ? ? if (host->bus_ops && !host->bus_dead) {
>> ? ? ? ? ? ? ? if (host->bus_ops->suspend)
>> ? ? ? ? ? ? ? ? ? ? ? err = host->bus_ops->suspend(host);
>> + ? ? ? ? ? ? if (err == -ENOSYS || !host->bus_ops->resume) {
> This reintroduces the bug I fixed.
>
> if the CONFIG_MMC_UNSAFE_RESUME isn't set (and that is default
> unfortunately), the host->bus_ops->resume will be NULL (see core/mmc.c
> mmc_ops), and therefore card will be removed, that will trigger a block
> device removal, sync, and deadlock).

Have you reproduced this or is it just a general concern ?

I'm asking this because if CONFIG_MMC_UNSAFE_RESUME isn't set, then
mmc_pm_notify() will remove the card and detach the bus.

As a result, host->bus_ops will be unset, and host->bus_dead will be set.

In this case, mmc_suspend_host() should skip the code you are concerned about.

Regards,
Ohad.

Re: [PATCH] sdio: fix suspend/resume regression

[Maxim Levitsky]

On Sat, 2010-10-23 at 12:09 +0200, Ohad Ben-Cohen wrote:
> Hi Maxim,
> 
> On Fri, Oct 22, 2010 at 1:47 AM, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> > On Wed, 2010-10-13 at 09:31 +0200, Ohad Ben-Cohen wrote:
> >> Fix SDIO suspend/resume regression introduced by
> >> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs related to
> >> mmc/sd card insert/removal during suspend/resume":
> ...
> >> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> >> index c94565d..515ff39 100644
> >> --- a/drivers/mmc/core/core.c
> >> +++ b/drivers/mmc/core/core.c
> >> @@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
> >>       if (host->bus_ops && !host->bus_dead) {
> >>               if (host->bus_ops->suspend)
> >>                       err = host->bus_ops->suspend(host);
> >> +             if (err == -ENOSYS || !host->bus_ops->resume) {
> > This reintroduces the bug I fixed.
> >
> > if the CONFIG_MMC_UNSAFE_RESUME isn't set (and that is default
> > unfortunately), the host->bus_ops->resume will be NULL (see core/mmc.c
> > mmc_ops), and therefore card will be removed, that will trigger a block
> > device removal, sync, and deadlock).
> 
> Have you reproduced this or is it just a general concern ?
Only a general concern.
> 
> I'm asking this because if CONFIG_MMC_UNSAFE_RESUME isn't set, then
> mmc_pm_notify() will remove the card and detach the bus.
> 
> As a result, host->bus_ops will be unset, and host->bus_dead will be set.
> 
> In this case, mmc_suspend_host() should skip the code you are concerned about.
You are right here.
There is a very unlikely case that suspend of sd/mmc card fails (with
CONFIG_MMC_UNSAFE_RESUME set, and that will trigger the hang, but that
currently isn't possible because both mmc_sd_suspend and mmc_suspend
just return 0 unconditionally).

Btw, two above functions are identical, so some refactoring won't hurt.

So sorry for noise (and bug).

Best regards,
	Maxim Levitsky

[PATCH] sdio: fix suspend/resume regression

[Maxim Levitsky]

On Sat, 2010-10-23 at 12:09 +0200, Ohad Ben-Cohen wrote:
> Hi Maxim,
> 
> On Fri, Oct 22, 2010 at 1:47 AM, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> > On Wed, 2010-10-13 at 09:31 +0200, Ohad Ben-Cohen wrote:
> >> Fix SDIO suspend/resume regression introduced by
> >> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs related to
> >> mmc/sd card insert/removal during suspend/resume":
> ...
> >> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> >> index c94565d..515ff39 100644
> >> --- a/drivers/mmc/core/core.c
> >> +++ b/drivers/mmc/core/core.c
> >> @@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
> >>       if (host->bus_ops && !host->bus_dead) {
> >>               if (host->bus_ops->suspend)
> >>                       err = host->bus_ops->suspend(host);
> >> +             if (err == -ENOSYS || !host->bus_ops->resume) {
> > This reintroduces the bug I fixed.
> >
> > if the CONFIG_MMC_UNSAFE_RESUME isn't set (and that is default
> > unfortunately), the host->bus_ops->resume will be NULL (see core/mmc.c
> > mmc_ops), and therefore card will be removed, that will trigger a block
> > device removal, sync, and deadlock).
> 
> Have you reproduced this or is it just a general concern ?
Only a general concern.
> 
> I'm asking this because if CONFIG_MMC_UNSAFE_RESUME isn't set, then
> mmc_pm_notify() will remove the card and detach the bus.
> 
> As a result, host->bus_ops will be unset, and host->bus_dead will be set.
> 
> In this case, mmc_suspend_host() should skip the code you are concerned about.
You are right here.
There is a very unlikely case that suspend of sd/mmc card fails (with
CONFIG_MMC_UNSAFE_RESUME set, and that will trigger the hang, but that
currently isn't possible because both mmc_sd_suspend and mmc_suspend
just return 0 unconditionally).

Btw, two above functions are identical, so some refactoring won't hurt.

So sorry for noise (and bug).

Best regards,
	Maxim Levitsky

Re: [PATCH] sdio: fix suspend/resume regression

[Ohad Ben-Cohen]

On Sat, Oct 23, 2010 at 4:18 PM, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> On Sat, 2010-10-23 at 12:09 +0200, Ohad Ben-Cohen wrote:
...
>> >> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> >> index c94565d..515ff39 100644
>> >> --- a/drivers/mmc/core/core.c
>> >> +++ b/drivers/mmc/core/core.c
>> >> @@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
>> >>       if (host->bus_ops && !host->bus_dead) {
>> >>               if (host->bus_ops->suspend)
>> >>                       err = host->bus_ops->suspend(host);
>> >> +             if (err == -ENOSYS || !host->bus_ops->resume) {

>> > This reintroduces the bug I fixed.
...
>> In this case, mmc_suspend_host() should skip the code you are concerned about.

> You are right here.
> There is a very unlikely case that suspend of sd/mmc card fails (with
> CONFIG_MMC_UNSAFE_RESUME set, and that will trigger the hang, but that
> currently isn't possible because both mmc_sd_suspend and mmc_suspend
> just return 0 unconditionally).

Please note that the card is removed only on -ENOSYS, which is not a
random error that can happen. In this context, it's a deliberate
request (from the underlying bus) to remove the card.

So even if mmc_{sd_}suspend will be changed to propagate an error code
one day, it is unlikely that they will trigger this bug even if they
fail.

> Btw, two above functions are identical, so some refactoring won't hurt.

Feel free to send a patch ;)

Regards,
Ohad.

[PATCH] sdio: fix suspend/resume regression

[Ohad Ben-Cohen]

On Sat, Oct 23, 2010 at 4:18 PM, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> On Sat, 2010-10-23 at 12:09 +0200, Ohad Ben-Cohen wrote:
...
>> >> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> >> index c94565d..515ff39 100644
>> >> --- a/drivers/mmc/core/core.c
>> >> +++ b/drivers/mmc/core/core.c
>> >> @@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
>> >> ? ? ? if (host->bus_ops && !host->bus_dead) {
>> >> ? ? ? ? ? ? ? if (host->bus_ops->suspend)
>> >> ? ? ? ? ? ? ? ? ? ? ? err = host->bus_ops->suspend(host);
>> >> + ? ? ? ? ? ? if (err == -ENOSYS || !host->bus_ops->resume) {

>> > This reintroduces the bug I fixed.
...
>> In this case, mmc_suspend_host() should skip the code you are concerned about.

> You are right here.
> There is a very unlikely case that suspend of sd/mmc card fails (with
> CONFIG_MMC_UNSAFE_RESUME set, and that will trigger the hang, but that
> currently isn't possible because both mmc_sd_suspend and mmc_suspend
> just return 0 unconditionally).

Please note that the card is removed only on -ENOSYS, which is not a
random error that can happen. In this context, it's a deliberate
request (from the underlying bus) to remove the card.

So even if mmc_{sd_}suspend will be changed to propagate an error code
one day, it is unlikely that they will trigger this bug even if they
fail.

> Btw, two above functions are identical, so some refactoring won't hurt.

Feel free to send a patch ;)

Regards,
Ohad.

Re: 2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Sven Neumann]

On Fri, 2010-10-08 at 22:08 +0200, Rafael J. Wysocki wrote:
> On Friday, October 08, 2010, Sven Neumann wrote:
> > On Thu, 2010-10-07 at 23:23 +0200, Rafael J. Wysocki wrote:
> > 
> > > > > I wonder what happens if you echo 0 to /sys/power/pm_async ?
> > > > 
> > > > Nothing happens. The problem persists (tested with 2.6.36-rc7). What
> > > > would you expect to happen?
> > > 
> > > Exactly that. :-)
> > > 
> > > Commit 152e1d5920 should not affect the non-async case (I'd be surprised if
> > > it did really) and things should work with /sys/power/pm_async = 0 anyway.
> > > 
> > > Please try check if you can reproduce with commt 152e1d5920 reverted and
> > > /sys/power/pm_async = 0.  If you can, that's a driver bug.
> > 
> > Ok, for the record, here's what I tried. I have rebooted between tests
> > to make sure there's no state pulled in from the previous test:
> > 
> >  2.6.36-rc7, no changes,      pm_async 1 :  suspend fails
> >  2.6.36-rc7, no changes,      pm_async 0 :  suspend fails
> > 
> >  2.6.36-rc7, 152e1d reverted, pm_async 1 :  suspend works
> >  2.6.36-rc7, 152e1d reverted, pm_async 0 :  suspend fails
> > 
> >  2.6.34.7, no changes,        pm_async 1 :  suspend works
> >  2.6.34.7, no changes,        pm_async 0 :  suspend works
> > 
> > I am not sure how this should be interpreted.
> 
> There is a problem with the pxa2xx-mci.0 suspend that has been introduced
> some time after 2.6.34, but it is not related to commit 152e1d5920.  The
> problem was not visible with pm_async=1 because of the very bug fixed by
> commit 152e1d5920.
> 
> Please see if the problem is there in 2.6.35 (please test with pm_async=0).

Works fine with v2.6.35:

 2.6.35, no changes,        pm_async 1 :  suspend works
 2.6.35, no changes,        pm_async 0 :  suspend works

So it looks like the problem has been introduced after 2.6.35 and was
backported...


Greetings,
Sven

2.6.35.6 fails to suspend (pxa2xx-mci.0)

[Sven Neumann]

On Fri, 2010-10-08 at 22:08 +0200, Rafael J. Wysocki wrote:
> On Friday, October 08, 2010, Sven Neumann wrote:
> > On Thu, 2010-10-07 at 23:23 +0200, Rafael J. Wysocki wrote:
> > 
> > > > > I wonder what happens if you echo 0 to /sys/power/pm_async ?
> > > > 
> > > > Nothing happens. The problem persists (tested with 2.6.36-rc7). What
> > > > would you expect to happen?
> > > 
> > > Exactly that. :-)
> > > 
> > > Commit 152e1d5920 should not affect the non-async case (I'd be surprised if
> > > it did really) and things should work with /sys/power/pm_async = 0 anyway.
> > > 
> > > Please try check if you can reproduce with commt 152e1d5920 reverted and
> > > /sys/power/pm_async = 0.  If you can, that's a driver bug.
> > 
> > Ok, for the record, here's what I tried. I have rebooted between tests
> > to make sure there's no state pulled in from the previous test:
> > 
> >  2.6.36-rc7, no changes,      pm_async 1 :  suspend fails
> >  2.6.36-rc7, no changes,      pm_async 0 :  suspend fails
> > 
> >  2.6.36-rc7, 152e1d reverted, pm_async 1 :  suspend works
> >  2.6.36-rc7, 152e1d reverted, pm_async 0 :  suspend fails
> > 
> >  2.6.34.7, no changes,        pm_async 1 :  suspend works
> >  2.6.34.7, no changes,        pm_async 0 :  suspend works
> > 
> > I am not sure how this should be interpreted.
> 
> There is a problem with the pxa2xx-mci.0 suspend that has been introduced
> some time after 2.6.34, but it is not related to commit 152e1d5920.  The
> problem was not visible with pm_async=1 because of the very bug fixed by
> commit 152e1d5920.
> 
> Please see if the problem is there in 2.6.35 (please test with pm_async=0).

Works fine with v2.6.35:

 2.6.35, no changes,        pm_async 1 :  suspend works
 2.6.35, no changes,        pm_async 0 :  suspend works

So it looks like the problem has been introduced after 2.6.35 and was
backported...


Greetings,
Sven