[PATCH] ricoh_mmc: Use suspend/resume_noirq (v2) (resend)

[PATCH] ricoh_mmc: Use suspend/resume_noirq (v2) (resend)

[philipl]

If ricoh_mmc suspends before sdhci_pci, it will pull the card
out from under the controller, which could leave the system in
a very confused state.

Using suspend/resume_noirq ensures that sdhci_pci suspends first
and resumes second.

Signed-off-by: Philip Langdale <philipl@overt.org>
Acked-by: Pierre Ossman <drzeus@drzeus.cx>
---

  ricoh_mmc.c |   20 ++++++++++++++++----
  1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/ricoh_mmc.c b/drivers/mmc/host/ricoh_mmc.c
index 1837719..b4958a7 100644
--- a/drivers/mmc/host/ricoh_mmc.c
+++ b/drivers/mmc/host/ricoh_mmc.c
@@ -194,10 +194,14 @@ static void __devexit ricoh_mmc_remove(struct pci_dev *pdev)
 	pci_set_drvdata(pdev, NULL);
 }

-static int ricoh_mmc_suspend(struct pci_dev *pdev, pm_message_t state) +static int
ricoh_mmc_suspend_noirq(struct device *dev)
 {
+	struct pci_dev *pdev = NULL;
 	struct pci_dev *fw_dev = NULL;

+	pdev = to_pci_dev(dev);
+	BUG_ON(pdev == NULL);
+
 	fw_dev = pci_get_drvdata(pdev);
 	BUG_ON(fw_dev == NULL);

@@ -208,10 +212,14 @@ static int ricoh_mmc_suspend(struct pci_dev *pdev, pm_message_t state)
 	return 0;
 }

-static int ricoh_mmc_resume(struct pci_dev *pdev)
+static int ricoh_mmc_resume_noirq(struct device *dev)
 {
+	struct pci_dev *pdev = NULL;
 	struct pci_dev *fw_dev = NULL;

+	pdev = to_pci_dev(dev);
+	BUG_ON(pdev == NULL);
+
 	fw_dev = pci_get_drvdata(pdev);
 	BUG_ON(fw_dev == NULL);

@@ -222,13 +230,17 @@ static int ricoh_mmc_resume(struct pci_dev *pdev)
 	return 0;
 }

+static struct dev_pm_ops ricoh_mmc_pm_opts = {
+	.suspend_noirq =	ricoh_mmc_suspend_noirq,
+	.resume_noirq =		ricoh_mmc_resume_noirq,
+};
+
 static struct pci_driver ricoh_mmc_driver = {
 	.name = 	DRIVER_NAME,
 	.id_table =	pci_ids,
 	.probe = 	ricoh_mmc_probe,
 	.remove =	__devexit_p(ricoh_mmc_remove),
-	.suspend =	ricoh_mmc_suspend,
-	.resume =	ricoh_mmc_resume,
+	.driver.pm =	&ricoh_mmc_pm_opts,
 };

 /*****************************************************************************\

Rafael,

Pierre asked that you take this patch into your tree because it depends on
the new pm ops design.

Thanks,

--phil

Re: [PATCH] ricoh_mmc: Use suspend/resume_noirq (v2) (resend)

[Rafael J. Wysocki]

On Wednesday 07 January 2009, philipl@overt.org wrote:
> If ricoh_mmc suspends before sdhci_pci, it will pull the card
> out from under the controller, which could leave the system in
> a very confused state.
> 
> Using suspend/resume_noirq ensures that sdhci_pci suspends first
> and resumes second.
> 
> Signed-off-by: Philip Langdale <philipl@overt.org>
> Acked-by: Pierre Ossman <drzeus@drzeus.cx>
> ---
> 
>   ricoh_mmc.c |   20 ++++++++++++++++----
>   1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/ricoh_mmc.c b/drivers/mmc/host/ricoh_mmc.c
> index 1837719..b4958a7 100644
> --- a/drivers/mmc/host/ricoh_mmc.c
> +++ b/drivers/mmc/host/ricoh_mmc.c
> @@ -194,10 +194,14 @@ static void __devexit ricoh_mmc_remove(struct pci_dev *pdev)
>  	pci_set_drvdata(pdev, NULL);
>  }
> 
> -static int ricoh_mmc_suspend(struct pci_dev *pdev, pm_message_t state) +static int
> ricoh_mmc_suspend_noirq(struct device *dev)
>  {
> +	struct pci_dev *pdev = NULL;
>  	struct pci_dev *fw_dev = NULL;
> 
> +	pdev = to_pci_dev(dev);
> +	BUG_ON(pdev == NULL);
> +
>  	fw_dev = pci_get_drvdata(pdev);
>  	BUG_ON(fw_dev == NULL);
> 
> @@ -208,10 +212,14 @@ static int ricoh_mmc_suspend(struct pci_dev *pdev, pm_message_t state)
>  	return 0;
>  }
> 
> -static int ricoh_mmc_resume(struct pci_dev *pdev)
> +static int ricoh_mmc_resume_noirq(struct device *dev)
>  {
> +	struct pci_dev *pdev = NULL;
>  	struct pci_dev *fw_dev = NULL;
> 
> +	pdev = to_pci_dev(dev);
> +	BUG_ON(pdev == NULL);
> +
>  	fw_dev = pci_get_drvdata(pdev);
>  	BUG_ON(fw_dev == NULL);
> 
> @@ -222,13 +230,17 @@ static int ricoh_mmc_resume(struct pci_dev *pdev)
>  	return 0;
>  }
> 
> +static struct dev_pm_ops ricoh_mmc_pm_opts = {
> +	.suspend_noirq =	ricoh_mmc_suspend_noirq,
> +	.resume_noirq =		ricoh_mmc_resume_noirq,
> +};
> +
>  static struct pci_driver ricoh_mmc_driver = {
>  	.name = 	DRIVER_NAME,
>  	.id_table =	pci_ids,
>  	.probe = 	ricoh_mmc_probe,
>  	.remove =	__devexit_p(ricoh_mmc_remove),
> -	.suspend =	ricoh_mmc_suspend,
> -	.resume =	ricoh_mmc_resume,
> +	.driver.pm =	&ricoh_mmc_pm_opts,
>  };
> 
>  /*****************************************************************************\
> 
> Rafael,
> 
> Pierre asked that you take this patch into your tree because it depends on
> the new pm ops design.

Yes, thanks.

I'll look into this and get back to you shortly, most likely tomorrow.

Thanks,
Rafael

Re: [PATCH] ricoh_mmc: Use suspend/resume_noirq (v2) (resend)

[Rafael J. Wysocki]

On Wednesday 07 January 2009, philipl@overt.org wrote:
> If ricoh_mmc suspends before sdhci_pci, it will pull the card
> out from under the controller, which could leave the system in
> a very confused state.

Does it really happen?  That depends on which of them is registered first.

> Using suspend/resume_noirq ensures that sdhci_pci suspends first
> and resumes second.

Well, I'm not sure if this is the best approach, but I don't know what the
dependencies between the devices are, so can you please explain that
to me?

That said, if you want to suspend-resume ricoh_mmc with interrupts disabled,
please use the legacy PCI .suspend_late() and .resume_early() callbacks for
that, since in the new framework the core will carry out some standard PM
operations in addition to your .suspend_noirq() and .resume_noirq().  It may
not be what you want in this case, though.

Thanks,
Rafael


> Signed-off-by: Philip Langdale <philipl@overt.org>
> Acked-by: Pierre Ossman <drzeus@drzeus.cx>
> ---
> 
>   ricoh_mmc.c |   20 ++++++++++++++++----
>   1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/ricoh_mmc.c b/drivers/mmc/host/ricoh_mmc.c
> index 1837719..b4958a7 100644
> --- a/drivers/mmc/host/ricoh_mmc.c
> +++ b/drivers/mmc/host/ricoh_mmc.c
> @@ -194,10 +194,14 @@ static void __devexit ricoh_mmc_remove(struct pci_dev *pdev)
>  	pci_set_drvdata(pdev, NULL);
>  }
> 
> -static int ricoh_mmc_suspend(struct pci_dev *pdev, pm_message_t state) +static int
> ricoh_mmc_suspend_noirq(struct device *dev)
>  {
> +	struct pci_dev *pdev = NULL;
>  	struct pci_dev *fw_dev = NULL;
> 
> +	pdev = to_pci_dev(dev);
> +	BUG_ON(pdev == NULL);
> +
>  	fw_dev = pci_get_drvdata(pdev);
>  	BUG_ON(fw_dev == NULL);
> 
> @@ -208,10 +212,14 @@ static int ricoh_mmc_suspend(struct pci_dev *pdev, pm_message_t state)
>  	return 0;
>  }
> 
> -static int ricoh_mmc_resume(struct pci_dev *pdev)
> +static int ricoh_mmc_resume_noirq(struct device *dev)
>  {
> +	struct pci_dev *pdev = NULL;
>  	struct pci_dev *fw_dev = NULL;
> 
> +	pdev = to_pci_dev(dev);
> +	BUG_ON(pdev == NULL);
> +
>  	fw_dev = pci_get_drvdata(pdev);
>  	BUG_ON(fw_dev == NULL);
> 
> @@ -222,13 +230,17 @@ static int ricoh_mmc_resume(struct pci_dev *pdev)
>  	return 0;
>  }
> 
> +static struct dev_pm_ops ricoh_mmc_pm_opts = {
> +	.suspend_noirq =	ricoh_mmc_suspend_noirq,
> +	.resume_noirq =		ricoh_mmc_resume_noirq,
> +};
> +
>  static struct pci_driver ricoh_mmc_driver = {
>  	.name = 	DRIVER_NAME,
>  	.id_table =	pci_ids,
>  	.probe = 	ricoh_mmc_probe,
>  	.remove =	__devexit_p(ricoh_mmc_remove),
> -	.suspend =	ricoh_mmc_suspend,
> -	.resume =	ricoh_mmc_resume,
> +	.driver.pm =	&ricoh_mmc_pm_opts,
>  };
> 
>  /*****************************************************************************\
> 
> Rafael,
> 
> Pierre asked that you take this patch into your tree because it depends on
> the new pm ops design.

Re: [PATCH] ricoh_mmc: Use suspend/resume_noirq (v2) (resend)

[Philip Langdale]

On Thu, 8 Jan 2009 23:14:52 +0100, "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> On Wednesday 07 January 2009, philipl@overt.org wrote:
>> If ricoh_mmc suspends before sdhci_pci, it will pull the card
>> out from under the controller, which could leave the system in
>> a very confused state.
> 
> Does it really happen?  That depends on which of them is registered
first.

Yes, it does. It seems that due to the typical ordering of the PCI devices,
ricoh_mmc loads first.

The larger context is how the hardware works and what ricoh_mmc does.

The hardware is a multiple function PCI device with a separate function for
each type of media card. SD and MMC are special because they use the same
physical format, and consequently the same physical contacts. Inside the
chip, a basic detection routine runs on insertion, and the card then
appears
to the appropriate controller function.

Now, the ricoh chip implements the standard SD host controller spec but
as no such thing exists for MMC, it implements its own custom one. Linux
only has an SDHCI driver, but due to how the hardware works, we can support
MMC cards through it - but only if the controller can see them!

For that to happen, we must disable the MMC function - when that happens,
the card is routed to the SD function and an insertion event is generated.
Similarly, if you reactive the MMC function, the card will be seen to
disapper on the SD side.

The ricoh_mmc driver automates this enable/disable process. It is bound to
the MMC function but actually acts on another function of the Ricoh chip
(typically the firewire controller) to do the actual disabling.

For consistency, the driver reactivates the mmc function at suspend time
and deactivates it again on resume.

Now, if the ricoh_mmc driver suspends before the SD driver, it will
reactive
the MMC function and trigger this removal event on the SD side. Then, when
the SD driver does its suspend/resume cycle, the card will be unexpectedly
absent, and confusion follows.

If ricoh_mmc uses suspend/resume_noirq, it both ensures that it runs at
the right time and that the insertion/removal events are not reported.

>> Using suspend/resume_noirq ensures that sdhci_pci suspends first
>> and resumes second.
> 
> Well, I'm not sure if this is the best approach, but I don't know what
the
> dependencies between the devices are, so can you please explain that
> to me?

I hope my write up answers this question.
 
> That said, if you want to suspend-resume ricoh_mmc with interrupts
> disabled,
> please use the legacy PCI .suspend_late() and .resume_early() callbacks
for
> that, since in the new framework the core will carry out some standard PM
> operations in addition to your .suspend_noirq() and .resume_noirq().  It
> may
> not be what you want in this case, though.

I'd need to know what those other operations are. I originally wrote this
patch with suspend_late and resume_early but the first pm_ops patch I saw
removed those. Are you now saying that both suspend_late/resume_early and
noirq callbacks are supported now.

It's all a bit confusing :-)

--phil

Re: [PATCH] ricoh_mmc: Use suspend/resume_noirq (v2) (resend)

[Rafael J. Wysocki]

On Saturday 10 January 2009, Philip Langdale wrote:
> 
> On Thu, 8 Jan 2009 23:14:52 +0100, "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > On Wednesday 07 January 2009, philipl@overt.org wrote:
[...]
> > dependencies between the devices are, so can you please explain that
> > to me?
> 
> I hope my write up answers this question.

I think they are, though I have to read them more thoroughly yet.

> > That said, if you want to suspend-resume ricoh_mmc with interrupts
> > disabled,
> > please use the legacy PCI .suspend_late() and .resume_early() callbacks
> for
> > that, since in the new framework the core will carry out some standard PM
> > operations in addition to your .suspend_noirq() and .resume_noirq().  It
> > may
> > not be what you want in this case, though.
> 
> I'd need to know what those other operations are. I originally wrote this
> patch with suspend_late and resume_early but the first pm_ops patch I saw
> removed those. Are you now saying that both suspend_late/resume_early and
> noirq callbacks are supported now.

Yes, they are.  You implement either the "old" ones (->suspen(), ->resume(),
->suspend_late(), ->resume_early() in struct pci_driver) and even if only one of
them is defined, the "legacy" handling will be used for the other operations
too, so the existing drivers' code is not affected, or the "new" ones (the pm
object).

> It's all a bit confusing :-)

Yes, it is, unfortunately.  There are a couple of reasons, one of them being
that the code has only been finished recently and another that the whole
concept changed slightly just before 2.6.28, when we discovered a mode of
failure that had not been known before and it had to be taken into account.

I'm now going to finally write some documentation. :-)

Thanks,
Rafael

Re: [PATCH] ricoh_mmc: Use suspend/resume_noirq (v2) (resend)

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 1468 bytes --]

On Fri, 09 Jan 2009 21:31:25 -0500
Philip Langdale <philipl@overt.org> wrote:

> 
> Now, the ricoh chip implements the standard SD host controller spec but
> as no such thing exists for MMC, it implements its own custom one. Linux
> only has an SDHCI driver, but due to how the hardware works, we can support
> MMC cards through it - but only if the controller can see them!
> 

Just to clarify, the problem is really that Microsoft are being
difficult. Their driver only implements support for SD (which I suppose
is only because they have a serious problem with lack of manpower, and
not because they're a member of the SD Association but not the MMC
one ;)), but since there is only a single device there is no
possibility for another driver to provide the MMC functionality.

The way most vendors have solved this Windows misfeature is by having
some voodoo where their own driver binds to a non-SDHCI interface, and
in the event of an SD card, hands it over to the other device (and
Microsoft's sdhci driver) or handles the SD in the vendor driver as
well.

Once again the limited design of that other OS forces the hardware
vendors to do sub-optimal devices and the rest of the world is poorer
for it.

Bitterly
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]