Message-ID: <CAMZVYU8-thd2Q2CuFVuhT35D7cmv46b600365j0Zo-bFaVvK2Q@mail.gmail.com>
Date:	Tue, 26 Nov 2013 04:11:40 +0000
From:	Chang Liu <cl91tp@...il.com>
To:	Bjorn Helgaas <bhelgaas@...gle.com>
Cc:	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	Lan Tianyu <tianyu.lan@...el.com>,
	Khalid Aziz <khalid.aziz@...cle.com>,
	Konstantin Khlebnikov <khlebnikov@...nvz.org>,
	Alan Cox <gnomes@...rguk.ukuu.org.uk>,
	Takao Indoh <indou.takao@...fujitsu.com>,
	Jility <jility09@...il.com>, Florian Otti <f.otti@....at>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] PCI: add a quirk for keeping Bus Master bit on shutdown

If this turns out to be a problem specific to Acer V573G, we can simply
wrap the if (pdev->...) line in ata/ahci.c with if
(dmi_check_system(acer_v573g))
so that only acer v573g laptops will be affected by this patch. The remaining
part of the patch won't need to be changed.

I can do an updated one as soon as it is confirmed that this is an acer specific
issue.

2013/11/26 Bjorn Helgaas <bhelgaas@...gle.com>:
> [+cc Lan, Khalid, Konstantin, Alan, Takao, Jility, Florian, linux-kernel]
>
> On Tue, Nov 12, 2013 at 07:40:03PM +0000, Chang Liu wrote:
>> This fixes https://bugzilla.kernel.org/show_bug.cgi?id=63861
>>
>> Commit b566a22c2 and 7897e60227 made pci_device_shutdown()
>> unconditionally clear Bus Master bit for every pci devices.
>> While this works for most hardware, certain devices are not
>> compatible with this. Intel Lynx Point-LP SATA Controller,
>> for example, will hang the system if its Bus Master bit is
>> cleared during device shutdown. This patch adds a pci quirk
>> so that device drivers can instruct pci_device_shutdown()
>> to keep Bus Master from being cleared, and then implements
>> this mechanism for the Intel Lynx Point-LP AHCI driver.
>>
>> Signed-off-by: Chang Liu <cl91tp@...il.com>
>
> I'm not 100% comfortable with disabling bus mastering in general;
> see [1] for more details.
>
> But given that we have been doing it for quite a while (b566a22c2 appeared
> in v3.5 on Jul 21, 2012), and Lynx Point should be in lots of machines, I'm
> surprised that we don't see more reports of this problem if it really
> affects all Lynx Point parts.
>
> Jility reported the same problem [2], and I did find one other similar
> report [3] from Florian.  All three reports (Chang, Jility, Florian) are on
> the same exact machine (Acer V5-573G), which seems sort of suspicious.
>
> Lan, do you have access to any Lynx Point boxes?  Can you test and see
> whether they hang on power-off also?  I suspect this might be something
> specific to the Acer box, not a generic Lynx Point issue.
>
> Bjorn
>
> [1] http://lkml.kernel.org/r/20120427190033.GA17588@ldl.usa.hp.com
> [2] https://bugzilla.kernel.org/show_bug.cgi?id=63861#c21
> [3] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1249329
>
>> ---
>> As per Takao's suggestion, add a new member into struct pci_dev
>> and add a quirk in the ahci driver. I tested this on my
>> machine (Acer V5-573G) and it works fine.
>>
>>  drivers/ata/ahci.c       |  8 ++++++++
>>  drivers/pci/pci-driver.c | 11 ++++++++---
>>  include/linux/pci.h      |  1 +
>>  3 files changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>> index 8e28f92..de6efcb 100644
>> --- a/drivers/ata/ahci.c
>> +++ b/drivers/ata/ahci.c
>> @@ -1385,6 +1385,14 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>>
>>   pci_set_master(pdev);
>>
>> + /* We normally clear Bus Master on pci device shutdown. However,
>> + * doing so for Intel Lynx Point-LP SATA Controller [AHCI mode]
>> + * hangs the system. Therefore keep it.
>> + * See bug report: https://bugzilla.kernel.org/show_bug.cgi?id=63861
>> + */
>> + if (pdev->vendor == PCI_VENDOR_ID_INTEL && pdev->device == 0x9c03)
>> + pdev->keep_busmaster_on_shutdown = 1;
>> +
>>   if (hpriv->flags & AHCI_HFLAG_MULTI_MSI)
>>   return ahci_host_activate(host, pdev->irq, n_msis);
>>
>> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>> index 38f3c01..ff15b0c 100644
>> --- a/drivers/pci/pci-driver.c
>> +++ b/drivers/pci/pci-driver.c
>> @@ -392,10 +392,15 @@ static void pci_device_shutdown(struct device *dev)
>>   pci_msix_shutdown(pci_dev);
>>
>>   /*
>> - * Turn off Bus Master bit on the device to tell it to not
>> - * continue to do DMA. Don't touch devices in D3cold or unknown states.
>> + * If the hardware is okay with it, turn off Bus Master bit
>> + * on the device to tell it not to continue doing DMA.
>> + * Don't touch devices in D3cold or unknown states.
>> + * On certain hardware clearing Bus Master bit on shutdown
>> + * may hang the entire system. In these cases the driver of
>> + * these devices should set keep_busmaster_on_shutdown to 1.
>>   */
>> - if (pci_dev->current_state <= PCI_D3hot)
>> + if (!pci_dev->keep_busmaster_on_shutdown
>> +    && pci_dev->current_state <= PCI_D3hot)
>>   pci_clear_master(pci_dev);
>>  }
>>
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index da172f9..63db735 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -322,6 +322,7 @@ struct pci_dev {
>>   /* keep track of device state */
>>   unsigned int is_added:1;
>>   unsigned int is_busmaster:1; /* device is busmaster */
>> + unsigned int keep_busmaster_on_shutdown:1; /* do not clear busmaster on shutdown */
>>   unsigned int no_msi:1; /* device may not use msi */
>>   unsigned int block_cfg_access:1; /* config space access is blocked */
>>   unsigned int broken_parity_status:1; /* Device generates false positive parity */
>> --
>> 1.8.4.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@...r.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists