[Lucid] SRU: [PATCH] UBUNTU: Make ahci ignore the controller for MBP7, 1 and fall back to ata_generic. Patch based on Tejun Heo's submitted upstream.

As of today, this change is upstream but as two separate patches. I took the
liberty to do provide them as backported in your name.
Both have the necessary tags to make them go upstream stable and should come in
one of the next stable releases. But due to the fact that an installation is not
possible without that changes on the affected hw, I think it is sensible to take
them pre-stable. Changes are limited to specific device, so regression potential
is low.

-Stefan

For the split up version of patches.
Acked-by: Stefan Bader <stefan.bader@canonical.com>

On 07/02/2010 01:37 AM, Dave Walker wrote:
> The Linux kernel doesn't currently support the latest generation of the
> Macbook Pro (7,1), and also seems to have issues with the MacBook Mini.
>  The issue presents itself as SATA support being unavailable once the
> initrd tries to reset the SATA device.
> 
> There is a high priority open bug[0] regarding this, and at time of
> writing 106 Launchpad users have shared that the bug affects them (aka
> "me too")
> 
> The patch attached is heavily based on the work of Tejun Heo who has
> submitted it upstream[1] (and submitted to stable).  The changes to this
> patch include some slight refactoring to which make it compatiable with
> the 2.6.32 code, and a manual refresh to apply cleanly to the lucid git
> tree.
> 
> Based on looking what the patch achieves, i might suggest that the
> regression potential is low as it essentially blacklists if the
> following conditions are met:
> - The vendor of the SATA device is nvidia
> - The device is MCP89
> - The hardware is Apple
> - Particular subsystem id
> 
> If the blacklist is achieved, then ahci ignores the controller and
> allows fall back to ata_generic.
> 
> Whilst it hasn't yet been applied to Linus' git tree (so can't send a
> stable pull request), I would personally consider it paramount that this
> patch is applied soon to the Lucid kernel with the hope of landing on
> the 10.04.1 cd image.
> 
> As it stands, Ubuntu users with this hardware are unable to install from
> the 10.04 cd image, and are currently dependent on using an community
> built unofficial iso to be able to install Ubuntu.
> 
> Based on the high "me too" on Launchpad, I feel that many users are
> highly dependent on this landing in an official cd spin, and am
> therefore hoping that this patch is included and an SRU is created in
> time for the 10.04.1 release.
> 
> The original author of this patch described it as:
> "For yet unknown reason, MCP89 on MBP 7,1 doesn't work w/ ahci under
> linux but the controller doesn't require explicit mode setting and
> works fine with ata_generic.  Make ahci ignore the controller on MBP
> 7,1 and let ata_generic take it for now." -- Tejun Heo
> 
> Thanks
> 
> [0] http://bugs.launchpad.net/bugs/576601
> [1] http://article.gmane.org/gmane.linux.ide/46514

On Fri, Jul 02, 2010 at 09:43:29AM +0200, Stefan Bader wrote:
> As of today, this change is upstream but as two separate patches. I took the
> liberty to do provide them as backported in your name.
> Both have the necessary tags to make them go upstream stable and should come in
> one of the next stable releases. But due to the fact that an installation is not
> possible without that changes on the affected hw, I think it is sensible to take
> them pre-stable. Changes are limited to specific device, so regression potential
> is low.
> 
> -Stefan
> 
> For the split up version of patches.
> Acked-by: Stefan Bader <stefan.bader@canonical.com>
> 
> On 07/02/2010 01:37 AM, Dave Walker wrote:
> > The Linux kernel doesn't currently support the latest generation of the
> > Macbook Pro (7,1), and also seems to have issues with the MacBook Mini.
> >  The issue presents itself as SATA support being unavailable once the
> > initrd tries to reset the SATA device.
> > 
> > There is a high priority open bug[0] regarding this, and at time of
> > writing 106 Launchpad users have shared that the bug affects them (aka
> > "me too")
> > 
> > The patch attached is heavily based on the work of Tejun Heo who has
> > submitted it upstream[1] (and submitted to stable).  The changes to this
> > patch include some slight refactoring to which make it compatiable with
> > the 2.6.32 code, and a manual refresh to apply cleanly to the lucid git
> > tree.
> > 
> > Based on looking what the patch achieves, i might suggest that the
> > regression potential is low as it essentially blacklists if the
> > following conditions are met:
> > - The vendor of the SATA device is nvidia
> > - The device is MCP89
> > - The hardware is Apple
> > - Particular subsystem id
> > 
> > If the blacklist is achieved, then ahci ignores the controller and
> > allows fall back to ata_generic.
> > 
> > Whilst it hasn't yet been applied to Linus' git tree (so can't send a
> > stable pull request), I would personally consider it paramount that this
> > patch is applied soon to the Lucid kernel with the hope of landing on
> > the 10.04.1 cd image.
> > 
> > As it stands, Ubuntu users with this hardware are unable to install from
> > the 10.04 cd image, and are currently dependent on using an community
> > built unofficial iso to be able to install Ubuntu.
> > 
> > Based on the high "me too" on Launchpad, I feel that many users are
> > highly dependent on this landing in an official cd spin, and am
> > therefore hoping that this patch is included and an SRU is created in
> > time for the 10.04.1 release.
> > 
> > The original author of this patch described it as:
> > "For yet unknown reason, MCP89 on MBP 7,1 doesn't work w/ ahci under
> > linux but the controller doesn't require explicit mode setting and
> > works fine with ata_generic.  Make ahci ignore the controller on MBP
> > 7,1 and let ata_generic take it for now." -- Tejun Heo
> > 
> > Thanks
> > 
> > [0] http://bugs.launchpad.net/bugs/576601
> > [1] http://article.gmane.org/gmane.linux.ide/46514

> From b09867f655ad766cf2aa8032d75423d681530cea Mon Sep 17 00:00:00 2001
> From: Tejun Heo <tj@kernel.org>
> Date: Thu, 17 Jun 2010 11:42:22 +0200
> Subject: [PATCH 1/2] (pre-stable) ahci,ata_generic: let ata_generic handle new MBP w/ MCP89
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> BugLink: http://bugs.launchpad.net/bugs/576601
> 
> For yet unknown reason, MCP89 on MBP 7,1 doesn't work w/ ahci under
> linux but the controller doesn't require explicit mode setting and
> works fine with ata_generic.  Make ahci ignore the controller on MBP
> 7,1 and let ata_generic take it for now.
> 
> Reported in bko#15923.
> 
>   https://bugzilla.kernel.org/show_bug.cgi?id=15923
> 
> NVIDIA is investigating why ahci mode doesn't work.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Peer Chen <pchen@nvidia.com>
> Cc: stable@kernel.org
> Reported-by: Anders ??sthus <grapz666@gmail.com>
> Reported-by: Andreas Graf <andreas_graf@csgraf.de>
> Reported-by: Benoit Gschwind <gschwind@gnu-log.net>
> Reported-by: Damien Cassou <damien.cassou@gmail.com>
> Reported-by: tixetsal@juno.com
> Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
> (backported from commit b09867f655ad766cf2aa8032d75423d681530cea upstream)
> Signed-off-by: Dave Walker (Daviey) <DaveWalker@ubuntu.com>
> ---
>  drivers/ata/ahci.c        |   10 ++++++++++
>  drivers/ata/ata_generic.c |    6 ++++++
>  include/linux/pci_ids.h   |    1 +
>  3 files changed, 17 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index cb05205..e3d9816 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -3037,6 +3037,16 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	if (pdev->vendor == PCI_VENDOR_ID_MARVELL && !marvell_enable)
>  		return -ENODEV;
>  
> +	/*
> +	 * For some reason, MCP89 on MacBook 7,1 doesn't work with
> +	 * ahci, use ata_generic instead.
> +	 */
> +	if (pdev->vendor == PCI_VENDOR_ID_NVIDIA &&
> +	    pdev->device == PCI_DEVICE_ID_NVIDIA_NFORCE_MCP89_SATA &&
> +	    pdev->subsystem_vendor == PCI_VENDOR_ID_APPLE &&
> +	    pdev->subsystem_device == 0xcb89)
> +		return -ENODEV;
> +
>  	/* acquire resources */
>  	rc = pcim_enable_device(pdev);
>  	if (rc)
> diff --git a/drivers/ata/ata_generic.c b/drivers/ata/ata_generic.c
> index ecfd22b..b5aa93c 100644
> --- a/drivers/ata/ata_generic.c
> +++ b/drivers/ata/ata_generic.c
> @@ -168,6 +168,12 @@ static struct pci_device_id ata_generic[] = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_VIA,    PCI_DEVICE_ID_VIA_82C561), },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_OPTI,   PCI_DEVICE_ID_OPTI_82C558), },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_CENATEK,PCI_DEVICE_ID_CENATEK_IDE), },
> +	/*
> +	 * For some reason, MCP89 on MacBook 7,1 doesn't work with
> +	 * ahci, use ata_generic instead.
> +	 */
> +	{ PCI_VENDOR_ID_NVIDIA, PCI_DEVICE_ID_NVIDIA_NFORCE_MCP89_SATA,
> +	  PCI_VENDOR_ID_APPLE, 0xcb89, },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO), },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO_1), },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO_2),  },
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 63adefe..84f60fc 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -1262,6 +1262,7 @@
>  #define PCI_DEVICE_ID_NVIDIA_NFORCE_MCP77_IDE       0x0759
>  #define PCI_DEVICE_ID_NVIDIA_NFORCE_MCP73_SMBUS     0x07D8
>  #define PCI_DEVICE_ID_NVIDIA_NFORCE_MCP79_SMBUS     0x0AA2
> +#define PCI_DEVICE_ID_NVIDIA_NFORCE_MCP89_SATA	    0x0D85
>  
>  #define PCI_VENDOR_ID_IMS		0x10e0
>  #define PCI_DEVICE_ID_IMS_TT128		0x9128
> -- 
> 1.7.0.4
> 

> From 281f43a7aa754ad6a4eda9077548a532c147a246 Mon Sep 17 00:00:00 2001
> From: Tejun Heo <tj@kernel.org>
> Date: Tue, 22 Jun 2010 12:27:26 +0200
> Subject: [PATCH 2/2] (pre-stable) ata_generic: implement ATA_GEN_* flags and force enable DMA on MBP 7,1
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> BugLink: http://bugs.launchpad.net/bugs/576601
> 
> IDE mode of MCP89 on MBP 7,1 doesn't set DMA enable bits in the BMDMA
> status register.  Make the following changes to work around the problem.
> 
> * Instead of using hard coded 1 in id->driver_data as class code
>   match, use ATA_GEN_CLASS_MATCH and carry the matched id in
>   host->private_data.
> 
> * Instead of matching PCI_VENDOR_ID_CENATEK, use ATA_GEN_FORCE_DMA
>   flag in id instead.
> 
> * Add ATA_GEN_FORCE_DMA to the id entry of MBP 7,1.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Peer Chen <pchen@nvidia.com>
> Cc: stable@kernel.org
> Reported-by: Anders ??sthus <grapz666@gmail.com>
> Reported-by: Andreas Graf <andreas_graf@csgraf.de>
> Reported-by: Benoit Gschwind <gschwind@gnu-log.net>
> Reported-by: Damien Cassou <damien.cassou@gmail.com>
> Reported-by: tixetsal@juno.com
> Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
> (backported from commit 281f43a7aa754ad6a4eda9077548a532c147a246 upstream)
> Signed-off-by: Dave Walker (Daviey) <DaveWalker@ubuntu.com>
> ---
>  drivers/ata/ata_generic.c |   26 +++++++++++++++++++-------
>  1 files changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/ata/ata_generic.c b/drivers/ata/ata_generic.c
> index b5aa93c..99e7196 100644
> --- a/drivers/ata/ata_generic.c
> +++ b/drivers/ata/ata_generic.c
> @@ -32,6 +32,11 @@
>   *	A generic parallel ATA driver using libata
>   */
>  
> +enum {
> +	ATA_GEN_CLASS_MATCH		= (1 << 0),
> +	ATA_GEN_FORCE_DMA		= (1 << 1),
> +};
> +
>  /**
>   *	generic_set_mode	-	mode setting
>   *	@link: link to set up
> @@ -46,13 +51,17 @@
>  static int generic_set_mode(struct ata_link *link, struct ata_device **unused)
>  {
>  	struct ata_port *ap = link->ap;
> +	const struct pci_device_id *id = ap->host->private_data;
>  	int dma_enabled = 0;
>  	struct ata_device *dev;
>  	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
>  
> -	/* Bits 5 and 6 indicate if DMA is active on master/slave */
> -	if (ap->ioaddr.bmdma_addr)
> +	if (id->driver_data & ATA_GEN_FORCE_DMA) {
> +		dma_enabled = 0xff;
> +	} else if (ap->ioaddr.bmdma_addr) {
> +		/* Bits 5 and 6 indicate if DMA is active on master/slave */
>  		dma_enabled = ioread8(ap->ioaddr.bmdma_addr + ATA_DMA_STATUS);
> +	}
>  
>  	if (pdev->vendor == PCI_VENDOR_ID_CENATEK)
>  		dma_enabled = 0xFF;

I think this patch should have removed the conditional above as it is
now encoded into the driver_data field for the Cenatek devices.  Though
it remaining is benign.  Tejun you probabally want to kill that for the
upstream version.

> @@ -126,7 +135,7 @@ static int ata_generic_init_one(struct pci_dev *dev, const struct pci_device_id
>  	const struct ata_port_info *ppi[] = { &info, NULL };
>  
>  	/* Don't use the generic entry unless instructed to do so */
> -	if (id->driver_data == 1 && all_generic_ide == 0)
> +	if ((id->driver_data & ATA_GEN_CLASS_MATCH) && all_generic_ide == 0)
>  		return -ENODEV;
>  
>  	/* Devices that need care */
> @@ -155,7 +164,7 @@ static int ata_generic_init_one(struct pci_dev *dev, const struct pci_device_id
>  			return rc;
>  		pcim_pin_device(dev);
>  	}
> -	return ata_pci_sff_init_one(dev, ppi, &generic_sht, NULL);
> +	return ata_pci_sff_init_one(dev, ppi, &generic_sht, (void *)id);
>  }
>  
>  static struct pci_device_id ata_generic[] = {
> @@ -167,18 +176,21 @@ static struct pci_device_id ata_generic[] = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_HINT,   PCI_DEVICE_ID_HINT_VXPROII_IDE), },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_VIA,    PCI_DEVICE_ID_VIA_82C561), },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_OPTI,   PCI_DEVICE_ID_OPTI_82C558), },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_CENATEK,PCI_DEVICE_ID_CENATEK_IDE), },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_CENATEK,PCI_DEVICE_ID_CENATEK_IDE),
> +	  .driver_data = ATA_GEN_FORCE_DMA },
>  	/*
>  	 * For some reason, MCP89 on MacBook 7,1 doesn't work with
>  	 * ahci, use ata_generic instead.
>  	 */
>  	{ PCI_VENDOR_ID_NVIDIA, PCI_DEVICE_ID_NVIDIA_NFORCE_MCP89_SATA,
> -	  PCI_VENDOR_ID_APPLE, 0xcb89, },
> +	  PCI_VENDOR_ID_APPLE, 0xcb89,
> +	  .driver_data = ATA_GEN_FORCE_DMA },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO), },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO_1), },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_TOSHIBA,PCI_DEVICE_ID_TOSHIBA_PICCOLO_2),  },
>  	/* Must come last. If you add entries adjust this table appropriately */
> -	{ PCI_ANY_ID,		PCI_ANY_ID,			   PCI_ANY_ID, PCI_ANY_ID, PCI_CLASS_STORAGE_IDE << 8, 0xFFFFFF00UL, 1},
> +	{ PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_IDE << 8, 0xFFFFFF00UL),
> +	  .driver_data = ATA_GEN_CLASS_MATCH },
>  	{ 0, },
>  };

Looks good for Ubuntu.

Acked-by: Andy Whitcroft <apw@canonical.com>

-apw

Hello,

On 07/02/2010 10:17 AM, Andy Whitcroft wrote:
>>  	if (pdev->vendor == PCI_VENDOR_ID_CENATEK)
>>  		dma_enabled = 0xFF;
> 
> I think this patch should have removed the conditional above as it is
> now encoded into the driver_data field for the Cenatek devices.  Though
> it remaining is benign.  Tejun you probabally want to kill that for the
> upstream version.

Oh, right, forgot about that.  Will kill it in mainline.

Thanks.