* [PATCH] pci: Enable bus master till the FLR completes
@ 2013-07-01 11:22 Vipul Pandya
2013-07-26 22:27 ` Bjorn Helgaas
0 siblings, 1 reply; 4+ messages in thread
From: Vipul Pandya @ 2013-07-01 11:22 UTC (permalink / raw)
To: linux-pci; +Cc: bhelgaas, leedom, tomreu, vipul, divy, dm, nirranjan
From: Casey Leedom <leedom@chelsio.com>
T4 can wedge if there are DMAs in flight within the chip and Bus master has
been disabled. We need to have it on till the Function Level Reset completes.
T4 can also suffer a Head Of Line blocking problem if MSI-X interrupts are
disabled before the FLR has completed.
Signed-off-by: Casey Leedom <leedom@chelsio.com>
---
drivers/pci/quirks.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 91 insertions(+)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index e85d230..6357ba1 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3208,6 +3208,95 @@ reset_complete:
return 0;
}
+/*
+ * Device-specific reset method for Chelsio T4-based adapters.
+ */
+static int reset_chelsio_generic_dev(struct pci_dev *dev, int probe)
+{
+ u16 old_command;
+ u16 status, msix_flags;
+ int i, rc, msix_pos;
+
+ /*
+ * If this isn't a Chelsio T4-based device, return -ENOTTY indicating
+ * that we have no device-specific reset method.
+ */
+ if ((dev->device & 0xf000) != 0x4000)
+ return -ENOTTY;
+
+ /*
+ * If this is the "probe" phase, return 0 indicating that we can
+ * reset this device.
+ */
+ if (probe)
+ return 0;
+
+ /*
+ * T4 can wedge if their are DMAs in flight within the chip and Bus
+ * master has been disabled. We need to have it on till the Function
+ * Level Reset completes.
+ */
+ pci_read_config_word(dev, PCI_COMMAND, &old_command);
+ pci_write_config_word(dev, PCI_COMMAND,
+ old_command | PCI_COMMAND_MASTER);
+
+ /*
+ * Perform the actual device function reset, saving and restoring
+ * configuration information around the reset.
+ */
+ pci_save_state(dev);
+
+ /*
+ * T4 also suffers a Head-Of-Line blocking problem if MSI-X interrupts
+ * are disabled when an MSI-X interrupt message needs to be delivered.
+ * So we briefly re-enable MSI-X interrupts for the duration of the
+ * FLR. The pci_restore_state() below will restore the original
+ * MSI-X state.
+ */
+ msix_pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
+ pci_read_config_word(dev, msix_pos+PCI_MSIX_FLAGS, &msix_flags);
+ if ((msix_flags & PCI_MSIX_FLAGS_ENABLE) == 0)
+ pci_write_config_word(dev, msix_pos+PCI_MSIX_FLAGS,
+ msix_flags |
+ PCI_MSIX_FLAGS_ENABLE |
+ PCI_MSIX_FLAGS_MASKALL);
+
+ /*
+ * This reset code is a copy of the guts of pcie_flr() because that's
+ * not an exported function.
+ */
+
+ /* Wait for Transaction Pending bit clean */
+ for (i = 0; i < 4; i++) {
+ if (i)
+ msleep((1 << (i - 1)) * 100);
+
+ pcie_capability_read_word(dev, PCI_EXP_DEVSTA, &status);
+ if (!(status & PCI_EXP_DEVSTA_TRPND))
+ goto clear;
+ }
+
+ dev_err(&dev->dev, "transaction is not cleared; proceeding with reset anyway\n");
+
+clear:
+ pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
+ msleep(100);
+
+ /*
+ * End of pcie_flr() code sequence.
+ */
+
+ rc = 0;
+ pci_restore_state(dev);
+
+ /*
+ * Restore the original PCI Configuration Space Command word (which
+ * probably had Bus Master disabled).
+ */
+ pci_write_config_word(dev, PCI_COMMAND, old_command);
+ return rc;
+}
+
#define PCI_DEVICE_ID_INTEL_82599_SFP_VF 0x10ed
#define PCI_DEVICE_ID_INTEL_IVB_M_VGA 0x0156
#define PCI_DEVICE_ID_INTEL_IVB_M2_VGA 0x0166
@@ -3221,6 +3310,8 @@ static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {
reset_ivb_igd },
{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
reset_intel_generic_dev },
+ { PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
+ reset_chelsio_generic_dev },
{ 0 }
};
--
1.8.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] pci: Enable bus master till the FLR completes
2013-07-01 11:22 [PATCH] pci: Enable bus master till the FLR completes Vipul Pandya
@ 2013-07-26 22:27 ` Bjorn Helgaas
2013-07-26 22:53 ` Casey Leedom
0 siblings, 1 reply; 4+ messages in thread
From: Bjorn Helgaas @ 2013-07-26 22:27 UTC (permalink / raw)
To: Vipul Pandya
Cc: linux-pci@vger.kernel.org, leedom, tomreu, divy, dm, nirranjan
On Mon, Jul 1, 2013 at 5:22 AM, Vipul Pandya <vipul@chelsio.com> wrote:
> From: Casey Leedom <leedom@chelsio.com>
>
> T4 can wedge if there are DMAs in flight within the chip and Bus master has
> been disabled. We need to have it on till the Function Level Reset completes.
> T4 can also suffer a Head Of Line blocking problem if MSI-X interrupts are
> disabled before the FLR has completed.
>
> Signed-off-by: Casey Leedom <leedom@chelsio.com>
> ---
> drivers/pci/quirks.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 91 insertions(+)
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index e85d230..6357ba1 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3208,6 +3208,95 @@ reset_complete:
> return 0;
> }
>
> +/*
> + * Device-specific reset method for Chelsio T4-based adapters.
> + */
> +static int reset_chelsio_generic_dev(struct pci_dev *dev, int probe)
> +{
> + u16 old_command;
> + u16 status, msix_flags;
> + int i, rc, msix_pos;
> +
> + /*
> + * If this isn't a Chelsio T4-based device, return -ENOTTY indicating
> + * that we have no device-specific reset method.
> + */
> + if ((dev->device & 0xf000) != 0x4000)
> + return -ENOTTY;
> +
> + /*
> + * If this is the "probe" phase, return 0 indicating that we can
> + * reset this device.
> + */
> + if (probe)
> + return 0;
> +
> + /*
> + * T4 can wedge if their are DMAs in flight within the chip and Bus
> + * master has been disabled. We need to have it on till the Function
> + * Level Reset completes.
> + */
> + pci_read_config_word(dev, PCI_COMMAND, &old_command);
> + pci_write_config_word(dev, PCI_COMMAND,
> + old_command | PCI_COMMAND_MASTER);
> +
> + /*
> + * Perform the actual device function reset, saving and restoring
> + * configuration information around the reset.
> + */
> + pci_save_state(dev);
> +
> + /*
> + * T4 also suffers a Head-Of-Line blocking problem if MSI-X interrupts
> + * are disabled when an MSI-X interrupt message needs to be delivered.
> + * So we briefly re-enable MSI-X interrupts for the duration of the
> + * FLR. The pci_restore_state() below will restore the original
> + * MSI-X state.
> + */
> + msix_pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
dev->msix_cap contains the capability offset already.
> + pci_read_config_word(dev, msix_pos+PCI_MSIX_FLAGS, &msix_flags);
> + if ((msix_flags & PCI_MSIX_FLAGS_ENABLE) == 0)
> + pci_write_config_word(dev, msix_pos+PCI_MSIX_FLAGS,
> + msix_flags |
> + PCI_MSIX_FLAGS_ENABLE |
> + PCI_MSIX_FLAGS_MASKALL);
> +
> + /*
> + * This reset code is a copy of the guts of pcie_flr() because that's
> + * not an exported function.
> + */
> +
> + /* Wait for Transaction Pending bit clean */
> + for (i = 0; i < 4; i++) {
> + if (i)
> + msleep((1 << (i - 1)) * 100);
> +
> + pcie_capability_read_word(dev, PCI_EXP_DEVSTA, &status);
> + if (!(status & PCI_EXP_DEVSTA_TRPND))
> + goto clear;
> + }
This is at least the fifth copy of this loop:
pcie_flr()
pci_af_flr()
bnx2x_do_flr()
reset_intel_82599_sfp_virtfn()
Can we factor this out and just implement it once? Maybe even make it
smart enough to look at the PCIe DEVSTA if it exists, and fall back to
using PCI_AF_STATUS register if *it* exists?
> +
> + dev_err(&dev->dev, "transaction is not cleared; proceeding with reset anyway\n");
> +
> +clear:
> + pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
> + msleep(100);
> +
> + /*
> + * End of pcie_flr() code sequence.
> + */
> +
> + rc = 0;
> + pci_restore_state(dev);
> +
> + /*
> + * Restore the original PCI Configuration Space Command word (which
> + * probably had Bus Master disabled).
Why was Bus Master probably originally disabled? I don't see anything
in the pci_reset_function() path that would have disabled it before
getting to this function.
But I don't think you need the comment anyway. "Probably had Bus
Master disabled" isn't anything one can rely on.
> + */
> + pci_write_config_word(dev, PCI_COMMAND, old_command);
> + return rc;
"rc" appears useless.
> +}
> +
> #define PCI_DEVICE_ID_INTEL_82599_SFP_VF 0x10ed
> #define PCI_DEVICE_ID_INTEL_IVB_M_VGA 0x0156
> #define PCI_DEVICE_ID_INTEL_IVB_M2_VGA 0x0166
> @@ -3221,6 +3310,8 @@ static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {
> reset_ivb_igd },
> { PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
> reset_intel_generic_dev },
> + { PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
> + reset_chelsio_generic_dev },
> { 0 }
> };
>
> --
> 1.8.0
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] pci: Enable bus master till the FLR completes
2013-07-26 22:27 ` Bjorn Helgaas
@ 2013-07-26 22:53 ` Casey Leedom
2013-07-26 23:08 ` Bjorn Helgaas
0 siblings, 1 reply; 4+ messages in thread
From: Casey Leedom @ 2013-07-26 22:53 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Vipul Pandya, linux-pci@vger.kernel.org, tomreu, divy, dm,
nirranjan
Thanks for the review comments. I'll work with Vipul to get these
properly incorporated. Answers to your comments are inline below:
On 07/26/13 15:27, Bjorn Helgaas wrote:
> On Mon, Jul 1, 2013 at 5:22 AM, Vipul Pandya <vipul@chelsio.com> wrote:
>> From: Casey Leedom <leedom@chelsio.com>
>>
>> T4 can wedge if there are DMAs in flight within the chip and Bus master has
>> been disabled. We need to have it on till the Function Level Reset completes.
>> T4 can also suffer a Head Of Line blocking problem if MSI-X interrupts are
>> disabled before the FLR has completed.
>>
>> Signed-off-by: Casey Leedom <leedom@chelsio.com>
>> ---
>> drivers/pci/quirks.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 91 insertions(+)
>>
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index e85d230..6357ba1 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -3208,6 +3208,95 @@ reset_complete:
>> return 0;
>> }
>>
>> +/*
>> + * Device-specific reset method for Chelsio T4-based adapters.
>> + */
>> +static int reset_chelsio_generic_dev(struct pci_dev *dev, int probe)
>> +{
>> + u16 old_command;
>> + u16 status, msix_flags;
>> + int i, rc, msix_pos;
>> +
>> + /*
>> + * If this isn't a Chelsio T4-based device, return -ENOTTY indicating
>> + * that we have no device-specific reset method.
>> + */
>> + if ((dev->device & 0xf000) != 0x4000)
>> + return -ENOTTY;
>> +
>> + /*
>> + * If this is the "probe" phase, return 0 indicating that we can
>> + * reset this device.
>> + */
>> + if (probe)
>> + return 0;
>> +
>> + /*
>> + * T4 can wedge if their are DMAs in flight within the chip and Bus
>> + * master has been disabled. We need to have it on till the Function
>> + * Level Reset completes.
>> + */
>> + pci_read_config_word(dev, PCI_COMMAND, &old_command);
>> + pci_write_config_word(dev, PCI_COMMAND,
>> + old_command | PCI_COMMAND_MASTER);
>> +
>> + /*
>> + * Perform the actual device function reset, saving and restoring
>> + * configuration information around the reset.
>> + */
>> + pci_save_state(dev);
>> +
>> + /*
>> + * T4 also suffers a Head-Of-Line blocking problem if MSI-X interrupts
>> + * are disabled when an MSI-X interrupt message needs to be delivered.
>> + * So we briefly re-enable MSI-X interrupts for the duration of the
>> + * FLR. The pci_restore_state() below will restore the original
>> + * MSI-X state.
>> + */
>> + msix_pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
> dev->msix_cap contains the capability offset already.
Thanks. I didn't know about that.
>> + pci_read_config_word(dev, msix_pos+PCI_MSIX_FLAGS, &msix_flags);
>> + if ((msix_flags & PCI_MSIX_FLAGS_ENABLE) == 0)
>> + pci_write_config_word(dev, msix_pos+PCI_MSIX_FLAGS,
>> + msix_flags |
>> + PCI_MSIX_FLAGS_ENABLE |
>> + PCI_MSIX_FLAGS_MASKALL);
>> +
>> + /*
>> + * This reset code is a copy of the guts of pcie_flr() because that's
>> + * not an exported function.
>> + */
>> +
>> + /* Wait for Transaction Pending bit clean */
>> + for (i = 0; i < 4; i++) {
>> + if (i)
>> + msleep((1 << (i - 1)) * 100);
>> +
>> + pcie_capability_read_word(dev, PCI_EXP_DEVSTA, &status);
>> + if (!(status & PCI_EXP_DEVSTA_TRPND))
>> + goto clear;
>> + }
> This is at least the fifth copy of this loop:
>
> pcie_flr()
> pci_af_flr()
> bnx2x_do_flr()
> reset_intel_82599_sfp_virtfn()
>
> Can we factor this out and just implement it once? Maybe even make it
> smart enough to look at the PCIe DEVSTA if it exists, and fall back to
> using PCI_AF_STATUS register if *it* exists?
Should such a task be taken up in the patch we're trying to submit or
should it be a follow on patch? I had thought that, in general, patches
were supposed to essentially do "one thing." Thanks for your insight on
this.
>> +
>> + dev_err(&dev->dev, "transaction is not cleared; proceeding with reset anyway\n");
>> +
>> +clear:
>> + pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
>> + msleep(100);
>> +
>> + /*
>> + * End of pcie_flr() code sequence.
>> + */
>> +
>> + rc = 0;
>> + pci_restore_state(dev);
>> +
>> + /*
>> + * Restore the original PCI Configuration Space Command word (which
>> + * probably had Bus Master disabled).
> Why was Bus Master probably originally disabled? I don't see anything
> in the pci_reset_function() path that would have disabled it before
> getting to this function.
>
> But I don't think you need the comment anyway. "Probably had Bus
> Master disabled" isn't anything one can rely on.
Sorry, I probably should have included our example. In this case it's
coming from KVM's kvm_free_assigned_device() routine which calls
pci_reset_function() where we do:
/*
* both INTx and MSI are disabled after the Interrupt Disable bit
* is set and the Bus Master bit is cleared.
*/
pci_write_config_word(dev, PCI_COMMAND, PCI_COMMAND_INTX_DISABLE);
rc = pci_dev_reset(dev, 0);
The pci_dev_reset(dev, 0) eventually dives into pcie_flr() where the
Function Level Reset is performed — with Bus Master cleared by
pci_reset_function() as above.
I'm happy to enhance the comment with this information since I'm a
comment maven.
>> + */
>> + pci_write_config_word(dev, PCI_COMMAND, old_command);
>> + return rc;
> "rc" appears useless.
I agree. I was probably trying to follow coding styles as slavishly as
possible with some intermediate coding efforts and didn't see that the
final result had made "rc" pointless.
Casey
>> +}
>> +
>> #define PCI_DEVICE_ID_INTEL_82599_SFP_VF 0x10ed
>> #define PCI_DEVICE_ID_INTEL_IVB_M_VGA 0x0156
>> #define PCI_DEVICE_ID_INTEL_IVB_M2_VGA 0x0166
>> @@ -3221,6 +3310,8 @@ static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {
>> reset_ivb_igd },
>> { PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
>> reset_intel_generic_dev },
>> + { PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
>> + reset_chelsio_generic_dev },
>> { 0 }
>> };
>>
>> --
>> 1.8.0
>>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] pci: Enable bus master till the FLR completes
2013-07-26 22:53 ` Casey Leedom
@ 2013-07-26 23:08 ` Bjorn Helgaas
0 siblings, 0 replies; 4+ messages in thread
From: Bjorn Helgaas @ 2013-07-26 23:08 UTC (permalink / raw)
To: Casey Leedom
Cc: Vipul Pandya, linux-pci@vger.kernel.org, tomreu, divy, dm,
nirranjan
On Fri, Jul 26, 2013 at 4:53 PM, Casey Leedom <leedom@chelsio.com> wrote:
> Thanks for the review comments. I'll work with Vipul to get these properly
> incorporated. Answers to your comments are inline below:
>
>
>
> On 07/26/13 15:27, Bjorn Helgaas wrote:
>>
>> On Mon, Jul 1, 2013 at 5:22 AM, Vipul Pandya <vipul@chelsio.com> wrote:
>>>
>>> From: Casey Leedom <leedom@chelsio.com>
>>>
>>> T4 can wedge if there are DMAs in flight within the chip and Bus master
>>> has
>>> been disabled. We need to have it on till the Function Level Reset
>>> completes.
>>> T4 can also suffer a Head Of Line blocking problem if MSI-X interrupts
>>> are
>>> disabled before the FLR has completed.
>>>
>>> Signed-off-by: Casey Leedom <leedom@chelsio.com>
>>> ---
>>> drivers/pci/quirks.c | 91
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 91 insertions(+)
>>>
>>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>>> index e85d230..6357ba1 100644
>>> --- a/drivers/pci/quirks.c
>>> +++ b/drivers/pci/quirks.c
>>> @@ -3208,6 +3208,95 @@ reset_complete:
>>> return 0;
>>> }
>>>
>>> +/*
>>> + * Device-specific reset method for Chelsio T4-based adapters.
>>> + */
>>> +static int reset_chelsio_generic_dev(struct pci_dev *dev, int probe)
>>> +{
>>> + u16 old_command;
>>> + u16 status, msix_flags;
>>> + int i, rc, msix_pos;
>>> +
>>> + /*
>>> + * If this isn't a Chelsio T4-based device, return -ENOTTY
>>> indicating
>>> + * that we have no device-specific reset method.
>>> + */
>>> + if ((dev->device & 0xf000) != 0x4000)
>>> + return -ENOTTY;
>>> +
>>> + /*
>>> + * If this is the "probe" phase, return 0 indicating that we can
>>> + * reset this device.
>>> + */
>>> + if (probe)
>>> + return 0;
>>> +
>>> + /*
>>> + * T4 can wedge if their are DMAs in flight within the chip and
>>> Bus
>>> + * master has been disabled. We need to have it on till the
>>> Function
>>> + * Level Reset completes.
>>> + */
>>> + pci_read_config_word(dev, PCI_COMMAND, &old_command);
>>> + pci_write_config_word(dev, PCI_COMMAND,
>>> + old_command | PCI_COMMAND_MASTER);
>>> +
>>> + /*
>>> + * Perform the actual device function reset, saving and restoring
>>> + * configuration information around the reset.
>>> + */
>>> + pci_save_state(dev);
>>> +
>>> + /*
>>> + * T4 also suffers a Head-Of-Line blocking problem if MSI-X
>>> interrupts
>>> + * are disabled when an MSI-X interrupt message needs to be
>>> delivered.
>>> + * So we briefly re-enable MSI-X interrupts for the duration of
>>> the
>>> + * FLR. The pci_restore_state() below will restore the original
>>> + * MSI-X state.
>>> + */
>>> + msix_pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
>>
>> dev->msix_cap contains the capability offset already.
>
>
> Thanks. I didn't know about that.
>
>
>>> + pci_read_config_word(dev, msix_pos+PCI_MSIX_FLAGS, &msix_flags);
>>> + if ((msix_flags & PCI_MSIX_FLAGS_ENABLE) == 0)
>>> + pci_write_config_word(dev, msix_pos+PCI_MSIX_FLAGS,
>>> + msix_flags |
>>> + PCI_MSIX_FLAGS_ENABLE |
>>> + PCI_MSIX_FLAGS_MASKALL);
>>> +
>>> + /*
>>> + * This reset code is a copy of the guts of pcie_flr() because
>>> that's
>>> + * not an exported function.
>>> + */
>>> +
>>> + /* Wait for Transaction Pending bit clean */
>>> + for (i = 0; i < 4; i++) {
>>> + if (i)
>>> + msleep((1 << (i - 1)) * 100);
>>> +
>>> + pcie_capability_read_word(dev, PCI_EXP_DEVSTA, &status);
>>> + if (!(status & PCI_EXP_DEVSTA_TRPND))
>>> + goto clear;
>>> + }
>>
>> This is at least the fifth copy of this loop:
>>
>> pcie_flr()
>> pci_af_flr()
>> bnx2x_do_flr()
>> reset_intel_82599_sfp_virtfn()
>>
>> Can we factor this out and just implement it once? Maybe even make it
>> smart enough to look at the PCIe DEVSTA if it exists, and fall back to
>> using PCI_AF_STATUS register if *it* exists?
>
>
> Should such a task be taken up in the patch we're trying to submit or should
> it be a follow on patch? I had thought that, in general, patches were
> supposed to essentially do "one thing." Thanks for your insight on this.
You're right. The *ideal* thing would be a series like this:
1 add pci_wait_for_pending_transactions() and use it in drivers/pci
2 convert bnx2x to use it
3 chelsio patch (this one) using it
But I wouldn't hold your feet to the fire about the bnx2x one because
that's outside of your control. It's just good citizenship if you can
propose the patch.
>>> +
>>> + dev_err(&dev->dev, "transaction is not cleared; proceeding with
>>> reset anyway\n");
>>> +
>>> +clear:
>>> + pcie_capability_set_word(dev, PCI_EXP_DEVCTL,
>>> PCI_EXP_DEVCTL_BCR_FLR);
>>> + msleep(100);
>>> +
>>> + /*
>>> + * End of pcie_flr() code sequence.
>>> + */
>>> +
>>> + rc = 0;
>>> + pci_restore_state(dev);
>>> +
>>> + /*
>>> + * Restore the original PCI Configuration Space Command word
>>> (which
>>> + * probably had Bus Master disabled).
>>
>> Why was Bus Master probably originally disabled? I don't see anything
>> in the pci_reset_function() path that would have disabled it before
>> getting to this function.
>>
>> But I don't think you need the comment anyway. "Probably had Bus
>> Master disabled" isn't anything one can rely on.
>
>
> Sorry, I probably should have included our example. In this case it's coming
> from KVM's kvm_free_assigned_device() routine which calls
> pci_reset_function() where we do:
>
> /*
> * both INTx and MSI are disabled after the Interrupt Disable bit
> * is set and the Bus Master bit is cleared.
> */
> pci_write_config_word(dev, PCI_COMMAND, PCI_COMMAND_INTX_DISABLE);
Wow, I must be blind to have missed that :) Personally I would just
drop the whole comment and not worry about it.
Bjorn
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-07-26 23:08 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-07-01 11:22 [PATCH] pci: Enable bus master till the FLR completes Vipul Pandya
2013-07-26 22:27 ` Bjorn Helgaas
2013-07-26 22:53 ` Casey Leedom
2013-07-26 23:08 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox