[4/5] PCI: don't touch enable_cnt in pci_device_shutdown()

Commit Message

Konstantin Khlebnikov Jan. 18, 2013, 11:42 a.m. UTC

comment in commit b566a22c23327f18ce941ffad0ca907e50a53d41
("PCI: disable Bus Master on PCI device shutdown") says:

| Disable Bus Master bit on the device in pci_device_shutdown() to ensure PCI
| devices do not continue to DMA data after shutdown.  This can cause memory
| corruption in case of a kexec where the current kernel shuts down and
| transfers control to a new kernel while a PCI device continues to DMA to
| memory that does not belong to it any more in the new kernel.

Seems like pci_clear_master() must be used here instead of pci_disable_device(),
because it disables Bus Muster unconditionally and doesn't changes enable_cnt.

Signed-off-by: Konstantin Khlebnikov <[e-mail]>
Cc: [e-mail]
Cc: Bjorn Helgaas <[e-mail]>
Cc: Khalid Aziz <[e-mail]>
---
 drivers/pci/pci-driver.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Donald Dutile Jan. 18, 2013, 4:16 p.m. UTC | #1

On 01/18/2013 06:42 AM, Konstantin Khlebnikov wrote:
> comment in commit b566a22c23327f18ce941ffad0ca907e50a53d41
> ("PCI: disable Bus Master on PCI device shutdown") says:
>
> | Disable Bus Master bit on the device in pci_device_shutdown() to ensure PCI
> | devices do not continue to DMA data after shutdown.  This can cause memory
> | corruption in case of a kexec where the current kernel shuts down and
> | transfers control to a new kernel while a PCI device continues to DMA to
> | memory that does not belong to it any more in the new kernel.
>
> Seems like pci_clear_master() must be used here instead of pci_disable_device(),
> because it disables Bus Muster unconditionally and doesn't changes enable_cnt.
>
> Signed-off-by: Konstantin Khlebnikov<[e-mail]>
> Cc: [e-mail]
> Cc: Bjorn Helgaas<[e-mail]>
> Cc: Khalid Aziz<[e-mail]>

Hmmm.... wondering if this was the problem why kexec folks
said that device_shutdown() didn't work on all systems (when
trying to stop DMA, esp. on IOMMU-enabled systems...).

Bjorn: do you have a list &/or contact in kexec space to try this
patch vs the "reset every PCI bus" strategy that is currently
being pushed for kexec's method to halt DMA from a PCI device ?

> ---
>   drivers/pci/pci-driver.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 030dbf0..853d605 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -392,7 +392,7 @@ static void pci_device_shutdown(struct device *dev)
>   	 * Turn off Bus Master bit on the device to tell it to not
>   	 * continue to do DMA
>   	 */
> -	pci_disable_device(pci_dev);
> +	pci_clear_master(pci_dev);
>   }
>
>   #ifdef CONFIG_PM
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bjorn Helgaas Jan. 28, 2013, 11:40 p.m. UTC | #2

On Fri, Jan 18, 2013 at 4:42 AM, Konstantin Khlebnikov
<[e-mail]> wrote:
> comment in commit b566a22c23327f18ce941ffad0ca907e50a53d41
> ("PCI: disable Bus Master on PCI device shutdown") says:
>
> | Disable Bus Master bit on the device in pci_device_shutdown() to ensure PCI
> | devices do not continue to DMA data after shutdown.  This can cause memory
> | corruption in case of a kexec where the current kernel shuts down and
> | transfers control to a new kernel while a PCI device continues to DMA to
> | memory that does not belong to it any more in the new kernel.
>
> Seems like pci_clear_master() must be used here instead of pci_disable_device(),
> because it disables Bus Muster unconditionally and doesn't changes enable_cnt.
>
> Signed-off-by: Konstantin Khlebnikov <[e-mail]>
> Cc: [e-mail]
> Cc: Bjorn Helgaas <[e-mail]>
> Cc: Khalid Aziz <[e-mail]>
> ---
>  drivers/pci/pci-driver.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 030dbf0..853d605 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -392,7 +392,7 @@ static void pci_device_shutdown(struct device *dev)
>          * Turn off Bus Master bit on the device to tell it to not
>          * continue to do DMA
>          */
> -       pci_disable_device(pci_dev);
> +       pci_clear_master(pci_dev);

We currently only call pci_enable_device() and pci_disable_device()
from drivers, and I think that's a nice division that's worth keeping.
 It keeps the core's mitts off device operation and helps preserve the
enable_cnt integrity.  So I like this change from that perspective.

Any objections to this, Khalid?

>  }
>
>  #ifdef CONFIG_PM
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bjorn Helgaas Jan. 28, 2013, 11:44 p.m. UTC | #3

[+cc Khalid new email addr]

On Mon, Jan 28, 2013 at 4:40 PM, Bjorn Helgaas <[e-mail]> wrote:
> On Fri, Jan 18, 2013 at 4:42 AM, Konstantin Khlebnikov
> <[e-mail]> wrote:
>> comment in commit b566a22c23327f18ce941ffad0ca907e50a53d41
>> ("PCI: disable Bus Master on PCI device shutdown") says:
>>
>> | Disable Bus Master bit on the device in pci_device_shutdown() to ensure PCI
>> | devices do not continue to DMA data after shutdown.  This can cause memory
>> | corruption in case of a kexec where the current kernel shuts down and
>> | transfers control to a new kernel while a PCI device continues to DMA to
>> | memory that does not belong to it any more in the new kernel.
>>
>> Seems like pci_clear_master() must be used here instead of pci_disable_device(),
>> because it disables Bus Muster unconditionally and doesn't changes enable_cnt.
>>
>> Signed-off-by: Konstantin Khlebnikov <[e-mail]>
>> Cc: [e-mail]
>> Cc: Bjorn Helgaas <[e-mail]>
>> Cc: Khalid Aziz <[e-mail]>
>> ---
>>  drivers/pci/pci-driver.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>> index 030dbf0..853d605 100644
>> --- a/drivers/pci/pci-driver.c
>> +++ b/drivers/pci/pci-driver.c
>> @@ -392,7 +392,7 @@ static void pci_device_shutdown(struct device *dev)
>>          * Turn off Bus Master bit on the device to tell it to not
>>          * continue to do DMA
>>          */
>> -       pci_disable_device(pci_dev);
>> +       pci_clear_master(pci_dev);
>
> We currently only call pci_enable_device() and pci_disable_device()
> from drivers, and I think that's a nice division that's worth keeping.
>  It keeps the core's mitts off device operation and helps preserve the
> enable_cnt integrity.  So I like this change from that perspective.
>
> Any objections to this, Khalid?
>
>>  }
>>
>>  #ifdef CONFIG_PM
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Khalid Aziz Jan. 29, 2013, 2:47 a.m. UTC | #4

[Sorry about the resend. gmail sent my original reply in html by
default which didn't go through to all mailing lists]

On Mon, Jan 28, 2013 at 4:44 PM, Bjorn Helgaas <[e-mail]> wrote:
>
> [+cc Khalid new email addr]
>
> On Mon, Jan 28, 2013 at 4:40 PM, Bjorn Helgaas <[e-mail]> wrote:
> > On Fri, Jan 18, 2013 at 4:42 AM, Konstantin Khlebnikov
> > <[e-mail]> wrote:
> >> comment in commit b566a22c23327f18ce941ffad0ca907e50a53d41
> >> ("PCI: disable Bus Master on PCI device shutdown") says:
> >>
> >> | Disable Bus Master bit on the device in pci_device_shutdown() to ensure PCI
> >> | devices do not continue to DMA data after shutdown.  This can cause memory
> >> | corruption in case of a kexec where the current kernel shuts down and
> >> | transfers control to a new kernel while a PCI device continues to DMA to
> >> | memory that does not belong to it any more in the new kernel.
> >>
> >> Seems like pci_clear_master() must be used here instead of pci_disable_device(),
> >> because it disables Bus Muster unconditionally and doesn't changes enable_cnt.
> >>
> >> Signed-off-by: Konstantin Khlebnikov <[e-mail]>
> >> Cc: [e-mail]
> >> Cc: Bjorn Helgaas <[e-mail]>
> >> Cc: Khalid Aziz <[e-mail]>
> >> ---
> >>  drivers/pci/pci-driver.c |    2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> >> index 030dbf0..853d605 100644
> >> --- a/drivers/pci/pci-driver.c
> >> +++ b/drivers/pci/pci-driver.c
> >> @@ -392,7 +392,7 @@ static void pci_device_shutdown(struct device *dev)
> >>          * Turn off Bus Master bit on the device to tell it to not
> >>          * continue to do DMA
> >>          */
> >> -       pci_disable_device(pci_dev);
> >> +       pci_clear_master(pci_dev);
> >
> > We currently only call pci_enable_device() and pci_disable_device()
> > from drivers, and I think that's a nice division that's worth keeping.
> >  It keeps the core's mitts off device operation and helps preserve the
> > enable_cnt integrity.  So I like this change from that perspective.
> >
> > Any objections to this, Khalid?

Just turning the Bus Master bit off on a device does worry me. For
well behaved PCI devices this should not be an issue at all, but it
has been brought up before that there are misbehaved PCI devices that
do not respond well when Bus Master bit is cleared on them while they
are active. Turning interrupts off as well on them seems to help with
being able to re-initialize them in the new kernel.  As a result, I
wanted to make sure we at least call  pcibios_disable_irq() to turn
off their interrupt and hence I chose to call pci_disable_device()
instead of pci_clear_master(). I understand the concern around messing
up enable_cnt. Would it be better to replace pci_disable_device() with
these two instead:

pci_clear_master(pci_dev);
pcibios_disable_device(pci_dev);

--
Khalid
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 030dbf0..853d605 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -392,7 +392,7 @@  static void pci_device_shutdown(struct device *dev)
 	 * Turn off Bus Master bit on the device to tell it to not
 	 * continue to do DMA
 	 */
-	pci_disable_device(pci_dev);
+	pci_clear_master(pci_dev);
 }
 
 #ifdef CONFIG_PM