Patchwork ata: make DVD drive recognisable on systems with Intel Sandybridge CPT chipset

login
register
mail settings
Submitter Ming Lei
Date Sept. 22, 2011, 1:55 a.m.
Message ID <1316656512-4829-1-git-send-email-ming.lei@canonical.com>
Download mbox | patch
Permalink /patch/115877/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Ming Lei - Sept. 22, 2011, 1:55 a.m.
From: Ming Lei <ming.lei@canonical.com>

This quirk patch fixes one kind of bug inside Intel Sandybridge CPT
chipset, see reports from

	https://bugzilla.kernel.org/show_bug.cgi?id=40592.

Many guys also have reported the problem before:

	https://bugs.launchpad.net/bugs/737388
	https://bugs.launchpad.net/bugs/794642
	......

With help from Tejun, the problem is found to be caused by 32bit PIO
mode, so introduce the quirk patch to disable 32bit PIO on SATA piix
for Sandybridge CPT chipset.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/ata/ata_piix.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)
Tejun Heo - Sept. 24, 2011, 1:58 a.m.
Hello,

(cc'ing Jeff)

On Thu, Sep 22, 2011 at 09:55:12AM +0800, ming.lei@canonical.com wrote:
> From: Ming Lei <ming.lei@canonical.com>
> 
> This quirk patch fixes one kind of bug inside Intel Sandybridge CPT
> chipset, see reports from
> 
> 	https://bugzilla.kernel.org/show_bug.cgi?id=40592.
> 
> Many guys also have reported the problem before:
> 
> 	https://bugs.launchpad.net/bugs/737388
> 	https://bugs.launchpad.net/bugs/794642
> 	......
> 
> With help from Tejun, the problem is found to be caused by 32bit PIO
> mode, so introduce the quirk patch to disable 32bit PIO on SATA piix
> for Sandybridge CPT chipset.

Have we successfully localized the problem to SNB?  If so, great.

>  static struct ata_port_operations piix_vmw_ops = {
> @@ -1585,6 +1586,15 @@ static int __devinit piix_init_one(struct pci_dev *pdev,
>  				"on poweroff and hibernation\n");
>  	}
>  
> +	/*
> +	 * Sandybridge chipset H61/P67/H67 have broken 32 mode up to now
> +	 * see https://bugzilla.kernel.org/show_bug.cgi?id=40592
> +	 */
> +	if (pdev->vendor == PCI_VENDOR_ID_INTEL && pdev->device == 0x1c00)
> +		piix_sata_ops.inherits = &ata_bmdma_port_ops;
> +	else
> +		piix_sata_ops.inherits = &ata_bmdma32_port_ops;
> +

No, this wouldn't work.  Ops inheritance isn't dynamic.

Please define a separate ata_port_operations for controller which
require 16bit PIO - piix_pata16_ops, create a new controller id (say,
ich_snb_pata), add an accompanying port_info entry and device_id
entry.

Thank you.
Seth Heasley - Sept. 24, 2011, 4:28 a.m.
>Have we successfully localized the problem to SNB?  If so, great.

No, we haven't.  I've reproduced the issue on two newer Intel chipsets.  In IDE mode, ATAPI just isn't working on SATA3 ports.  With the provided patch, the issue is resolved.  At what cost, I can't say.  But if a patch will go in for the 6 Series, we need to apply it to the other platforms as well.  I can provide the DeviceIDs.

--Seth
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ming Lei - Sept. 24, 2011, 1:34 p.m.
Hi,

On Sat, Sep 24, 2011 at 9:58 AM, Tejun Heo <htejun@gmail.com> wrote:
> Hello,
> No, this wouldn't work.  Ops inheritance isn't dynamic.
>

I am sure that I have tested the patch and it does work.

> Please define a separate ata_port_operations for controller which
> require 16bit PIO - piix_pata16_ops, create a new controller id (say,
> ich_snb_pata), add an accompanying port_info entry and device_id
> entry.

In fact, I am not familiar with sata, but just want to fix the problem.
If you have a better patch, please ignore mine and apply yours.


On Sat, Sep 24, 2011 at 12:28 PM, Heasley, Seth <seth.heasley@intel.com> wrote:
>>Have we successfully localized the problem to SNB?  If so, great.
>
> No, we haven't.  I've reproduced the issue on two newer Intel chipsets.  In > IDE mode, ATAPI just isn't working on SATA3 ports.  With the provided
> patch, the issue is resolved.  At what cost, I can't say.  But if a patch will
> go in for the 6 Series, we need to apply it to the other platforms as well.  I > can provide the DeviceIDs.

I have seen someone reported the same problem on the device with
pci device id of 0x1c01[1]. I have asked them to test the patch but without
any response, so I had to not include the dev id in the patch.


[1], https://bugs.launchpad.net/bugs/782389

thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - Sept. 25, 2011, 12:03 a.m.
Hello,

On Sat, Sep 24, 2011 at 09:34:09PM +0800, Ming Lei wrote:
> On Sat, Sep 24, 2011 at 9:58 AM, Tejun Heo <htejun@gmail.com> wrote:
> > Hello,
> > No, this wouldn't work.  Ops inheritance isn't dynamic.
> 
> I am sure that I have tested the patch and it does work.

Yeah, but not by design.  That field is assumed to be static.
Inheritance currently is finalized during the first use of the
operation structure, where the first use also includes being inherited
by other ops structure, so doing it like that is asking for obscure
bugs.

> > Please define a separate ata_port_operations for controller which
> > require 16bit PIO - piix_pata16_ops, create a new controller id (say,
> > ich_snb_pata), add an accompanying port_info entry and device_id
> > entry.
> 
> In fact, I am not familiar with sata, but just want to fix the problem.
> If you have a better patch, please ignore mine and apply yours.

Sure I can do that but it would be better if you can revise your
patch.  Please take a look at how different ops are mapped to
different device IDs.  You just need to create another variant to be
mapped to the problematic device IDs.

> On Sat, Sep 24, 2011 at 12:28 PM, Heasley, Seth <seth.heasley@intel.com> wrote:
> >>Have we successfully localized the problem to SNB?  If so, great.
> >
> > No, we haven't.  I've reproduced the issue on two newer Intel chipsets.  In > IDE mode, ATAPI just isn't working on SATA3 ports.  With the provided
> > patch, the issue is resolved.  At what cost, I can't say.  But if a patch will
> > go in for the 6 Series, we need to apply it to the other platforms as well.  I > can provide the DeviceIDs.
> 
> I have seen someone reported the same problem on the device with
> pci device id of 0x1c01[1]. I have asked them to test the patch but without
> any response, so I had to not include the dev id in the patch.

Developing partial blacklist w/o knowing what's going on is messy.  If
we discover that something wasn't quite what we suspected it was and
had to revise, it'll be tricky to verify whicn ones need to remain.
Alan, can someone from intel verify the issue?  Is there an errata we
can look at?

Thanks.
Ming Lei - Sept. 26, 2011, 6:51 a.m.
Hi,

On Sun, Sep 25, 2011 at 8:03 AM, Tejun Heo <htejun@gmail.com> wrote:
> Sure I can do that but it would be better if you can revise your
> patch.  Please take a look at how different ops are mapped to
> different device IDs.  You just need to create another variant to be
> mapped to the problematic device IDs.

OK, I will do it.

>
>> On Sat, Sep 24, 2011 at 12:28 PM, Heasley, Seth <seth.heasley@intel.com> wrote:
>> >>Have we successfully localized the problem to SNB?  If so, great.
>> >
>> > No, we haven't.  I've reproduced the issue on two newer Intel chipsets.  In > IDE mode, ATAPI just isn't working on SATA3 ports.  With the provided
>> > patch, the issue is resolved.  At what cost, I can't say.  But if a patch will
>> > go in for the 6 Series, we need to apply it to the other platforms as well.  I > can provide the DeviceIDs.
>>

Seth, could you provide the DeviceIDs so that we can make a quirk for them
now?

>> I have seen someone reported the same problem on the device with
>> pci device id of 0x1c01[1]. I have asked them to test the patch but without
>> any response, so I had to not include the dev id in the patch.
>
> Developing partial blacklist w/o knowing what's going on is messy.  If
> we discover that something wasn't quite what we suspected it was and
> had to revise, it'll be tricky to verify whicn ones need to remain.
> Alan, can someone from intel verify the issue?  Is there an errata we
> can look at?

thanks,
---
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Sept. 26, 2011, 10:27 a.m.
> Developing partial blacklist w/o knowing what's going on is messy.  If
> we discover that something wasn't quite what we suspected it was and
> had to revise, it'll be tricky to verify whicn ones need to remain.
> Alan, can someone from intel verify the issue? 

Seth is "someone from Intel" 8)

Alan

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Seth Heasley - Sept. 26, 2011, 4:52 p.m.
>Seth, could you provide the DeviceIDs so that we can make a quirk for

>them

>now?


The DeviceIDs I've reproduced the issue with are the following:

1c00, 1c01 (6 Series/Cougar Point)
1d00 (Patsburg)
1e00, 1e01 (Panther Point)

>> Developing partial blacklist w/o knowing what's going on is messy.  If

>> we discover that something wasn't quite what we suspected it was and

>> had to revise, it'll be tricky to verify whicn ones need to remain.

>> Alan, can someone from intel verify the issue?  Is there an errata we

>> can look at?


Adding Kristin Accardi to the thread.  I've repro'd the issue on several systems and can verify any patch that's proposed, but I don't have the SATA expertise to provide much else.

-Seth

Patch

diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index 43107e9..eb7ea56 100644
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -341,11 +341,12 @@  static struct ata_port_operations piix_sata_ops = {
 };
 
 static struct ata_port_operations piix_pata_ops = {
-	.inherits		= &piix_sata_ops,
+	.inherits		= &ata_bmdma32_port_ops,
 	.cable_detect		= ata_cable_40wire,
 	.set_piomode		= piix_set_piomode,
 	.set_dmamode		= piix_set_dmamode,
 	.prereset		= piix_pata_prereset,
+	.sff_irq_check		= piix_irq_check,
 };
 
 static struct ata_port_operations piix_vmw_ops = {
@@ -1585,6 +1586,15 @@  static int __devinit piix_init_one(struct pci_dev *pdev,
 				"on poweroff and hibernation\n");
 	}
 
+	/*
+	 * Sandybridge chipset H61/P67/H67 have broken 32 mode up to now
+	 * see https://bugzilla.kernel.org/show_bug.cgi?id=40592
+	 */
+	if (pdev->vendor == PCI_VENDOR_ID_INTEL && pdev->device == 0x1c00)
+		piix_sata_ops.inherits = &ata_bmdma_port_ops;
+	else
+		piix_sata_ops.inherits = &ata_bmdma32_port_ops;
+
 	port_info[0] = piix_port_info[ent->driver_data];
 	port_info[1] = piix_port_info[ent->driver_data];