All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sinan Kaya <okaya@codeaurora.org>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: linux-pci@vger.kernel.org, timur@codeaurora.org, ryan@finnie.org,
	linux-arm-msm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, stable@vger.kernel.org,
	Bjorn Helgaas <bhelgaas@google.com>,
	"Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Kate Stewart <kstewart@linuxfoundation.org>,
	Frederick Lawler <fred@fredlawl.com>,
	Dongdong Liu <liudongdong3@huawei.com>,
	Mika Westerberg <mika.westerberg@linux.intel.com>,
	open list <linux-kernel@vger.kernel.org>,
	Don Brace <don.brace@microsemi.com>,
	esc.storagedev@microsemi.com, linux-scsi@vger.kernel.org
Subject: Re: [PATCH V2] PCI/portdrv: do not disable device on reboot/shutdown
Date: Thu, 24 May 2018 07:43:05 -0400	[thread overview]
Message-ID: <82656e20-e821-1944-3399-1667ceb27719@codeaurora.org> (raw)
In-Reply-To: <61f70fd6-52fd-da07-ce73-303f95132131@codeaurora.org>

On 5/23/2018 6:57 PM, Sinan Kaya wrote:
>> The crash seems to indicate that the hpsa device attempted a DMA after
>> we cleared the Root Port's PCI_COMMAND_MASTER, which means
>> hpsa_shutdown() didn't stop DMA from the device (it looks like *most*
>> shutdown methods don't disable device DMA, so it's in good company).
> All drivers are expected to shutdown DMA and interrupts in their shutdown()
> routines. They can skip removing threads, data structures etc. but DMA and
> interrupt disabling are required. This is the difference between shutdown()
> and remove() callbacks.

I found this note yesterday to see why we are not disabling the devices in the
PCI core itself. 

pci_device_remove()

	/*
	 * We would love to complain here if pci_dev->is_enabled is set, that
	 * the driver should have called pci_disable_device(), but the
	 * unfortunate fact is there are too many odd BIOS and bridge setups
	 * that don't like drivers doing that all of the time.
	 * Oh well, we can dream of sane hardware when we sleep, no matter how
	 * horrible the crap we have to deal with is when we are awake...
	 */ 

Ryan, can you discard the previous patch and test this one instead? remove() path
in hpsa driver seems to call pci_disable_device() via 

hpsa_remove_one()
	hpsa_free_pci_init()

but nothing on the shutdown path.

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 4ed3d26..3823f04 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -8651,6 +8651,7 @@ static void hpsa_shutdown(struct pci_dev *pdev)
        h->access.set_intr_mask(h, HPSA_INTR_OFF);
        hpsa_free_irqs(h);                      /* init_one 4 */
        hpsa_disable_interrupt_mode(h);         /* pci_init 2 */
+       pci_disable_device(h->pdev);
 }



-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

WARNING: multiple messages have this Message-ID (diff)
From: okaya@codeaurora.org (Sinan Kaya)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH V2] PCI/portdrv: do not disable device on reboot/shutdown
Date: Thu, 24 May 2018 07:43:05 -0400	[thread overview]
Message-ID: <82656e20-e821-1944-3399-1667ceb27719@codeaurora.org> (raw)
In-Reply-To: <61f70fd6-52fd-da07-ce73-303f95132131@codeaurora.org>

On 5/23/2018 6:57 PM, Sinan Kaya wrote:
>> The crash seems to indicate that the hpsa device attempted a DMA after
>> we cleared the Root Port's PCI_COMMAND_MASTER, which means
>> hpsa_shutdown() didn't stop DMA from the device (it looks like *most*
>> shutdown methods don't disable device DMA, so it's in good company).
> All drivers are expected to shutdown DMA and interrupts in their shutdown()
> routines. They can skip removing threads, data structures etc. but DMA and
> interrupt disabling are required. This is the difference between shutdown()
> and remove() callbacks.

I found this note yesterday to see why we are not disabling the devices in the
PCI core itself. 

pci_device_remove()

	/*
	 * We would love to complain here if pci_dev->is_enabled is set, that
	 * the driver should have called pci_disable_device(), but the
	 * unfortunate fact is there are too many odd BIOS and bridge setups
	 * that don't like drivers doing that all of the time.
	 * Oh well, we can dream of sane hardware when we sleep, no matter how
	 * horrible the crap we have to deal with is when we are awake...
	 */ 

Ryan, can you discard the previous patch and test this one instead? remove() path
in hpsa driver seems to call pci_disable_device() via 

hpsa_remove_one()
	hpsa_free_pci_init()

but nothing on the shutdown path.

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 4ed3d26..3823f04 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -8651,6 +8651,7 @@ static void hpsa_shutdown(struct pci_dev *pdev)
        h->access.set_intr_mask(h, HPSA_INTR_OFF);
        hpsa_free_irqs(h);                      /* init_one 4 */
        hpsa_disable_interrupt_mode(h);         /* pci_init 2 */
+       pci_disable_device(h->pdev);
 }



-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

  reply	other threads:[~2018-05-24 11:43 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-23  2:44 [PATCH V2] PCI/portdrv: do not disable device on reboot/shutdown Sinan Kaya
2018-05-23  2:44 ` Sinan Kaya
2018-05-23  2:44 ` Sinan Kaya
2018-05-23 21:32 ` Bjorn Helgaas
2018-05-23 21:32   ` Bjorn Helgaas
2018-05-23 21:32   ` Bjorn Helgaas
2018-05-23 22:57   ` Sinan Kaya
2018-05-23 22:57     ` Sinan Kaya
2018-05-24 11:43     ` Sinan Kaya [this message]
2018-05-24 11:43       ` Sinan Kaya
2018-05-24 13:07       ` Bjorn Helgaas
2018-05-24 13:07         ` Bjorn Helgaas
2018-05-24 13:07         ` Bjorn Helgaas
2018-05-24 13:35         ` okaya
2018-05-24 13:35           ` okaya at codeaurora.org
2018-05-24 13:37         ` Don Brace
2018-05-24 13:37           ` Don Brace
2018-05-24 13:37           ` Don Brace
2018-05-28 21:25           ` Sinan Kaya
2018-05-28 21:25             ` Sinan Kaya
2018-05-28 21:25             ` Sinan Kaya
2018-05-24 18:35     ` Bjorn Helgaas
2018-05-24 18:35       ` Bjorn Helgaas
2018-05-24 18:35       ` Bjorn Helgaas
2018-05-25 13:30       ` Sinan Kaya
2018-05-25 13:30         ` Sinan Kaya
2018-05-25 22:10         ` Bjorn Helgaas
2018-05-25 22:10           ` Bjorn Helgaas
2018-05-25 22:10           ` Bjorn Helgaas
2018-05-25 22:34           ` okaya
2018-05-25 22:34             ` okaya at codeaurora.org
2018-05-30  2:41           ` Sinan Kaya
2018-05-30  2:41             ` Sinan Kaya

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=82656e20-e821-1944-3399-1667ceb27719@codeaurora.org \
    --to=okaya@codeaurora.org \
    --cc=bhelgaas@google.com \
    --cc=don.brace@microsemi.com \
    --cc=esc.storagedev@microsemi.com \
    --cc=fred@fredlawl.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=helgaas@kernel.org \
    --cc=kstewart@linuxfoundation.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=liudongdong3@huawei.com \
    --cc=mika.westerberg@linux.intel.com \
    --cc=rafael.j.wysocki@intel.com \
    --cc=ryan@finnie.org \
    --cc=stable@vger.kernel.org \
    --cc=tglx@linutronix.de \
    --cc=timur@codeaurora.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.