RE: [PATCH] PCI: MSI: Remove unsafe and unnecessary hardware access

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



>-----Original Message-----
>From: Jesse Barnes [mailto:jbarnes@xxxxxxxxxxxxxxxx]
>Sent: Friday, October 15, 2010 1:06 PM
>To: Emil S Tantilov
>Cc: Ben Hutchings; Michael Chan; Matthew Wilcox; linux-pci@xxxxxxxxxxxxxxx;
>NetDev; Tantilov, Emil S; Brandeburg, Jesse; Kirsher, Jeffrey T
>Subject: Re: [PATCH] PCI: MSI: Remove unsafe and unnecessary hardware
>access
>
>On Fri, 15 Oct 2010 11:26:08 -0700
>Emil S Tantilov <emils.tantilov@xxxxxxxxx> wrote:
>
>> On Thu, Jun 17, 2010 at 12:16 PM, Ben Hutchings
>> <bhutchings@xxxxxxxxxxxxxx> wrote:
>> > During suspend on an SMP system, {read,write}_msi_msg_desc() may be
>> > called to mask and unmask interrupts on a device that is already in a
>> > reduced power state. ÂAt this point memory-mapped registers including
>> > MSI-X tables are not accessible, and config space may not be fully
>> > functional either.
>> >
>> > While a device is in a reduced power state its interrupts are
>> > effectively masked and its MSI(-X) state will be restored when it is
>> > brought back to D0. ÂTherefore these functions can simply read and
>> > write msi_desc::msg for devices not in D0.
>> >
>> > Further, read_msi_msg_desc() should only ever be used to update a
>> > previously written message, so it can always read msi_desc::msg
>> > and never needs to touch the hardware.
>> >
>> > Signed-off-by: Ben Hutchings <bhutchings@xxxxxxxxxxxxxx>
>> > ---
>> > On Mon, 2010-06-14 at 18:13 -0700, Michael Chan wrote:
>> >> I'm debugging the bnx2 driver which doesn't work after suspend/resume
>if
>> >> it is running in MSI-X mode. ÂThe problem is that during suspend, the
>> >> MSI-X vectors are disabled by the following sequence on x86:
>> >>
>> >> take_cpu_down() -> cpu_disable_common() -> fixup_irqs()
>> >>
>> >> The MSI-X address/data used to disable the vectors are remembered in
>the
>> >> above sequence. During resume, these address/data are then programmed
>> >> back to the device during pci_restore_state(), causing all the vectors
>> >> to remain disabled.
>> >
>> > That's not quite what I see. ÂWhat I see is that the message is read
>> > back from the table *after* the driver's suspend method has been
>called.
>> > At this point the device is already in D3 and memory-mapped registers
>> > are not accessible, so we get random bits as the message. ÂAt least,
>> > that's what I see happening with the sfc driver.
>> >
>> >> Some drivers call free_irq() during suspend and request_irq() during
>> >> resume, and that should avoid the problem. Âbnx2 and some other
>drivers
>> >> do not do that. ÂThese drivers rely on pci_restore_state() to restore
>> >> the MSI-X vectors to the same working state before suspend.
>> >>
>> >> What's the right way to fix this? ÂThanks.
>> >
>> > This is my attempt, which works for sfc. ÂSee if it works for bnx2.
>> >
>> > Ben.
>> >
>> > Âdrivers/pci/msi.c | Â 34 +++++++++++-----------------------
>> > Â1 files changed, 11 insertions(+), 23 deletions(-)
>> >
>> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
>> > index 77b68ea..03f04dc 100644
>> > --- a/drivers/pci/msi.c
>> > +++ b/drivers/pci/msi.c
>> > @@ -196,30 +196,15 @@ void unmask_msi_irq(unsigned int irq)
>> > Âvoid read_msi_msg_desc(struct irq_desc *desc, struct msi_msg *msg)
>> > Â{
>> > Â Â Â Âstruct msi_desc *entry = get_irq_desc_msi(desc);
>> > - Â Â Â if (entry->msi_attrib.is_msix) {
>> > - Â Â Â Â Â Â Â void __iomem *base = entry->mask_base +
>> > - Â Â Â Â Â Â Â Â Â Â Â entry->msi_attrib.entry_nr *
>PCI_MSIX_ENTRY_SIZE;
>> >
>> > - Â Â Â Â Â Â Â msg->address_lo = readl(base +
>PCI_MSIX_ENTRY_LOWER_ADDR);
>> > - Â Â Â Â Â Â Â msg->address_hi = readl(base +
>PCI_MSIX_ENTRY_UPPER_ADDR);
>> > - Â Â Â Â Â Â Â msg->data = readl(base + PCI_MSIX_ENTRY_DATA);
>> > - Â Â Â } else {
>> > - Â Â Â Â Â Â Â struct pci_dev *dev = entry->dev;
>> > - Â Â Â Â Â Â Â int pos = entry->msi_attrib.pos;
>> > - Â Â Â Â Â Â Â u16 data;
>> > + Â Â Â /* We do not touch the hardware (which may not even be
>> > + Â Â Â Â* accessible at the moment) but return the last message
>> > + Â Â Â Â* written. ÂAssert that this is valid, assuming that
>> > + Â Â Â Â* valid messages are not all-zeroes. */
>> > + Â Â Â BUG_ON(!(entry->msg.address_hi | entry->msg.address_lo |
>> > + Â Â Â Â Â Â Â Âentry->msg.data));
>> >
>> > - Â Â Â Â Â Â Â pci_read_config_dword(dev, msi_lower_address_reg(pos),
>> > - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â &msg->address_lo);
>> > - Â Â Â Â Â Â Â if (entry->msi_attrib.is_64) {
>> > - Â Â Â Â Â Â Â Â Â Â Â pci_read_config_dword(dev,
>msi_upper_address_reg(pos),
>> > - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â &msg->address_hi);
>> > - Â Â Â Â Â Â Â Â Â Â Â pci_read_config_word(dev, msi_data_reg(pos, 1),
>&data);
>> > - Â Â Â Â Â Â Â } else {
>> > - Â Â Â Â Â Â Â Â Â Â Â msg->address_hi = 0;
>> > - Â Â Â Â Â Â Â Â Â Â Â pci_read_config_word(dev, msi_data_reg(pos, 0),
>&data);
>> > - Â Â Â Â Â Â Â }
>> > - Â Â Â Â Â Â Â msg->data = data;
>> > - Â Â Â }
>> > + Â Â Â *msg = entry->msg;
>> > Â}
>> >
>> > Âvoid read_msi_msg(unsigned int irq, struct msi_msg *msg)
>> > @@ -232,7 +217,10 @@ void read_msi_msg(unsigned int irq, struct msi_msg
>*msg)
>> > Âvoid write_msi_msg_desc(struct irq_desc *desc, struct msi_msg *msg)
>> > Â{
>> > Â Â Â Âstruct msi_desc *entry = get_irq_desc_msi(desc);
>> > - Â Â Â if (entry->msi_attrib.is_msix) {
>> > +
>> > + Â Â Â if (entry->dev->current_state != PCI_D0) {
>>
>> This check exposed a problem in ixgb (patch is on the way) where
>> pci_disable_device() was not being called in ixgb_remove(). As a
>> result the current_state was set to PCI_UNKNOWN and the interface
>> failed to work on subsequent load of the driver.
>>
>> Even though the problem was in ixgb, it made me wonder about this
>> check as the presumption here (low power state) may not always be
>> true. Like in the case of unloading a driver, which sets
>> dev->current_state to PCI_UNKNOWN which is not a representation of the
>> _real_ state of the device (actual state could be D0).
>>
>> BTW - quick search shows other drivers that could potentially suffer
>> the faith of ixgb due to lack of pci_disable_device() call on removal.
>
>Yeah we just ran into this in the DRM layer as well; which does a
>pci_enable_device but never calls _disable, so we're stuck with
>potentially stale state.
>
>I came up with the below to address that, but really I don't like the
>idea of nested pci_enable_device() calls at all.  But I haven't looked
>at the latest Wireless USB stuff to see if those drivers still rely on
>it.
>
>--
>Jesse Barnes, Intel Open Source Technology Center
>
>diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>index 7fa3cbd..37facc1 100644
>--- a/drivers/pci/pci.c
>+++ b/drivers/pci/pci.c
>@@ -994,6 +994,18 @@ static int __pci_enable_device_flags(struct pci_dev
>*dev,
> 	int err;
> 	int i, bars = 0;
>
>+	/*
>+	 * Power state could be unknown at this point, either due to a fresh
>+	 * boot or a device removal call.  So get the current power state
>+	 * so that things like MSI message writing will behave as expected
>+	 * (e.g. if the device really is in D0 at enable time).
>+	 */
>+	if (dev->pm_cap) {
>+		u16 pmcsr;
>+		pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
>+		dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);
>+	}
>+
> 	if (atomic_add_return(1, &dev->enable_cnt) > 1)
> 		return 0;		/* already enabled */
>

With this patch applied I could reload the driver and confirmed that current_state is set to the actual power state.

Thanks,
Emil

ÿô.nlj·Ÿ®‰­†+%ŠË±é¥Šwÿº{.nlj·¥Š{±þ—"þ)íèjg¬±¨¶‰šŽŠÝjÿ¾«þG«é¸¢·¦j:+v‰¨Šwèm¶Ÿÿþø®w¥þŠà£¢·hšâÿ†Ù



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux