[PATCH] MMC: Add 400ms to CAFE SD controller resume path

[PATCH] MMC: Add 400ms to CAFE SD controller resume path

[Deepak Saxena]

The CAFE SD controller takes a long time to completely resume
and w/o this patch, we do not redetect an existing card but
instead detect it as a new one. Even with MMC_UNSAFE_RESUME
enabled, this leads to the partition table on the device being
wiped out.
    
See http://dev.laptop.org/ticket/6532 for gory details.

Signed-off-by: Deepak Saxena <dsaxena@laptop.org>

---

diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index 65be279..9941bb0 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -105,10 +105,26 @@ static const struct sdhci_pci_fixes sdhci_ene_714 = {
 			  SDHCI_QUIRK_BROKEN_DMA,
 };
 
+
+/*
+ * This 400ms timeout is needed to ensure that the chip has proper time to
+ * wake up an redetect an already inserted card. Without this, we end up
+ * corrupting the partition table on the device.
+ *
+ * See http://dev.laptop.org/ticket/6532 for lots of ugly details.
+ */
+static int cafe_resume(struct sdhci_pci_chip *chip)
+{
+	msleep(400);
+
+	return 0;
+}
+
 static const struct sdhci_pci_fixes sdhci_cafe = {
 	.quirks		= SDHCI_QUIRK_NO_SIMULT_VDD_AND_POWER |
 			  SDHCI_QUIRK_NO_BUSY_IRQ |
 			  SDHCI_QUIRK_BROKEN_TIMEOUT_VAL,
+	.resume		= cafe_resume,
 };
 
 static int jmicron_pmos(struct sdhci_pci_chip *chip, int on)


-- 
In the end, they will not say, "those were dark times,"  they will ask
"why were their poets silent?" - Bertold Brecht

Re: [PATCH] MMC: Add 400ms to CAFE SD controller resume path

[Pierre Ossman]

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

On Wed, 13 May 2009 20:05:56 +0000
Deepak Saxena <dsaxena@plexity.net> wrote:

> 
> The CAFE SD controller takes a long time to completely resume
> and w/o this patch, we do not redetect an existing card but
> instead detect it as a new one. Even with MMC_UNSAFE_RESUME
> enabled, this leads to the partition table on the device being
> wiped out.
>     
> See http://dev.laptop.org/ticket/6532 for gory details.
> 
> Signed-off-by: Deepak Saxena <dsaxena@laptop.org>
> 

Reading through that report, I don't believe you properly worked around
the bug. You only avoid bug 1339, but that's only mildly related.

What this workaround does is to make sure that MMC_UNSAFE_RESUME
actually works. But if you change cards during suspend, the VFS bug
should reappear and you'll corrupt the partition table.

And as for 1339, has there been any more work done on why this problem
doesn't appear in OFW? I'd be a lot happier if we could make things
work without artificial delays.

RGds
-- 
     -- 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: 198 bytes --]

Re: [PATCH] MMC: Add 400ms to CAFE SD controller resume path

[Andrew Morton]

On Fri, 22 May 2009 13:51:50 +0200 Pierre Ossman <pierre@ossman.eu> wrote:

> On Wed, 13 May 2009 20:05:56 +0000
> Deepak Saxena <dsaxena@plexity.net> wrote:
> 
> > 
> > The CAFE SD controller takes a long time to completely resume
> > and w/o this patch, we do not redetect an existing card but
> > instead detect it as a new one. Even with MMC_UNSAFE_RESUME
> > enabled, this leads to the partition table on the device being
> > wiped out.
> >     
> > See http://dev.laptop.org/ticket/6532 for gory details.

hm, this sounds like a fairly serious problem, but the patch is nearly
a year old.

> > Signed-off-by: Deepak Saxena <dsaxena@laptop.org>
> > 
> 
> Reading through that report, I don't believe you properly worked around
> the bug. You only avoid bug 1339, but that's only mildly related.
> 
> What this workaround does is to make sure that MMC_UNSAFE_RESUME
> actually works. But if you change cards during suspend, the VFS bug
> should reappear and you'll corrupt the partition table.

What do you think the VFS did wrong here?

> And as for 1339, has there been any more work done on why this problem
> doesn't appear in OFW? I'd be a lot happier if we could make things
> work without artificial delays.

Re: [PATCH] MMC: Add 400ms to CAFE SD controller resume path

[Pierre Ossman]

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

On Thu, 28 May 2009 01:36:29 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Fri, 22 May 2009 13:51:50 +0200 Pierre Ossman <pierre@ossman.eu> wrote:
> 
> > 
> > Reading through that report, I don't believe you properly worked around
> > the bug. You only avoid bug 1339, but that's only mildly related.
> > 
> > What this workaround does is to make sure that MMC_UNSAFE_RESUME
> > actually works. But if you change cards during suspend, the VFS bug
> > should reappear and you'll corrupt the partition table.
> 
> What do you think the VFS did wrong here?
> 

Might be the block layer as well. Somehow requests associated with an
old block device end up on the queue of a new block device. I don't
see how this can happen given Linux' device model, but somehow it does.

Rgds
-- 
     -- 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: 198 bytes --]

Re: [PATCH] MMC: Add 400ms to CAFE SD controller resume path

[Deepak Saxena]

On May 28 2009, at 10:48, Pierre Ossman was caught saying:
> On Thu, 28 May 2009 01:36:29 -0700
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Fri, 22 May 2009 13:51:50 +0200 Pierre Ossman <pierre@ossman.eu> wrote:
> > 
> > > 
> > > Reading through that report, I don't believe you properly worked around
> > > the bug. You only avoid bug 1339, but that's only mildly related.
> > > 
> > > What this workaround does is to make sure that MMC_UNSAFE_RESUME
> > > actually works. But if you change cards during suspend, the VFS bug
> > > should reappear and you'll corrupt the partition table.
> > 
> > What do you think the VFS did wrong here?
> > 
> 
> Might be the block layer as well. Somehow requests associated with an
> old block device end up on the queue of a new block device. I don't
> see how this can happen given Linux' device model, but somehow it does.

The request is not ending up in the queue of the new device.  If I 
recall correctyl What is happening is that userspace does an unmount on the 
old device when it receives the notification that it is gone (since it is not 
redected and a new entry is created). By the time that unmount is called, the 
various data structures for the device has been zeroed out but the various 
operation pointers are !NULL, so when we go back to write the superblock for 
the mounted partition, we overwrite the device's partition table as the
partition offset is now 0. The main issue here is that the kernel is allowing 
an I/O to a device that is technically still there but not from the kernel's
POV.
 
~Deepak

-- 
In the end, they will not say, "those were dark times,"  they will ask
"why were their poets silent?" - Bertold Brecht

Re: [PATCH] MMC: Add 400ms to CAFE SD controller resume path

[Pierre Ossman]

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

On Thu, 4 Jun 2009 23:33:49 +0000
Deepak Saxena <dsaxena@plexity.net> wrote:

> 
> The request is not ending up in the queue of the new device.  If I 
> recall correctyl What is happening is that userspace does an unmount on the 
> old device when it receives the notification that it is gone (since it is not 
> redected and a new entry is created). By the time that unmount is called, the 
> various data structures for the device has been zeroed out but the various 
> operation pointers are !NULL, so when we go back to write the superblock for 
> the mounted partition, we overwrite the device's partition table as the
> partition offset is now 0. The main issue here is that the kernel is allowing 
> an I/O to a device that is technically still there but not from the kernel's
> POV.

But if the device is not there from the kernel's POV then it should be
impossible to write to it. It seems this is a kernel bug though. See
this thread:

http://marc.info/?t=124413860500006&r=1&w=2

Rgds
-- 
     -- 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: 198 bytes --]