[RFC/PATCH]reconfigure MSI registers after resume

[RFC/PATCH]reconfigure MSI registers after resume

[Shaohua Li]

Hi,
It appears pci_enable_msi doesn't reconfigure msi registers if it
successfully look up a msi for a device. It assumes the data and address
registers unchanged after calling pci_disable_msi. But this isn't always
true, such as in a suspend/resume circle. In my test system, the
registers unsurprised become zero after a S3 resume. This patch fixes my
problem, please look at it. MSIX might have the same issue, but I
haven't taken a close look.

Thanks,
Shaohua
---

 linux-2.6.13-rc6-root/drivers/pci/msi.c |   72 +++++++++++++++++++-------------
 1 files changed, 43 insertions(+), 29 deletions(-)

diff -puN drivers/pci/msi.c~pci-msi drivers/pci/msi.c
--- linux-2.6.13-rc6/drivers/pci/msi.c~pci-msi	2005-08-18 12:58:45.032124008 +0800
+++ linux-2.6.13-rc6-root/drivers/pci/msi.c	2005-08-18 13:06:02.080682528 +0800
@@ -508,6 +508,45 @@ void pci_scan_msi_device(struct pci_dev 
 		nr_reserved_vectors++;
 }
 
+static void msi_register_init(struct pci_dev *dev, struct msi_desc *entry)
+{
+	struct msg_address address;
+	struct msg_data data;
+	int pos, vector = dev->irq;
+	u16 control;
+
+   	pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
+	pci_read_config_word(dev, msi_control_reg(pos), &control);
+	/* Configure MSI capability structure */
+	msi_address_init(&address);
+	msi_data_init(&data, vector);
+	entry->msi_attrib.current_cpu = ((address.lo_address.u.dest_id >>
+				MSI_TARGET_CPU_SHIFT) & MSI_TARGET_CPU_MASK);
+	pci_write_config_dword(dev, msi_lower_address_reg(pos),
+			address.lo_address.value);
+	if (is_64bit_address(control)) {
+		pci_write_config_dword(dev,
+			msi_upper_address_reg(pos), address.hi_address);
+		pci_write_config_word(dev,
+			msi_data_reg(pos, 1), *((u32*)&data));
+	} else
+		pci_write_config_word(dev,
+			msi_data_reg(pos, 0), *((u32*)&data));
+	if (entry->msi_attrib.maskbit) {
+		unsigned int maskbits, temp;
+		/* All MSIs are unmasked by default, Mask them all */
+		pci_read_config_dword(dev,
+			msi_mask_bits_reg(pos, is_64bit_address(control)),
+			&maskbits);
+		temp = (1 << multi_msi_capable(control));
+		temp = ((temp - 1) & ~temp);
+		maskbits |= temp;
+		pci_write_config_dword(dev,
+			msi_mask_bits_reg(pos, is_64bit_address(control)),
+			maskbits);
+	}
+}
+
 /**
  * msi_capability_init - configure device's MSI capability structure
  * @dev: pointer to the pci_dev data structure of MSI device function
@@ -520,8 +559,6 @@ void pci_scan_msi_device(struct pci_dev 
 static int msi_capability_init(struct pci_dev *dev)
 {
 	struct msi_desc *entry;
-	struct msg_address address;
-	struct msg_data data;
 	int pos, vector;
 	u16 control;
 
@@ -551,33 +588,8 @@ static int msi_capability_init(struct pc
 	/* Replace with MSI handler */
 	irq_handler_init(PCI_CAP_ID_MSI, vector, entry->msi_attrib.maskbit);
 	/* Configure MSI capability structure */
-	msi_address_init(&address);
-	msi_data_init(&data, vector);
-	entry->msi_attrib.current_cpu = ((address.lo_address.u.dest_id >>
-				MSI_TARGET_CPU_SHIFT) & MSI_TARGET_CPU_MASK);
-	pci_write_config_dword(dev, msi_lower_address_reg(pos),
-			address.lo_address.value);
-	if (is_64bit_address(control)) {
-		pci_write_config_dword(dev,
-			msi_upper_address_reg(pos), address.hi_address);
-		pci_write_config_word(dev,
-			msi_data_reg(pos, 1), *((u32*)&data));
-	} else
-		pci_write_config_word(dev,
-			msi_data_reg(pos, 0), *((u32*)&data));
-	if (entry->msi_attrib.maskbit) {
-		unsigned int maskbits, temp;
-		/* All MSIs are unmasked by default, Mask them all */
-		pci_read_config_dword(dev,
-			msi_mask_bits_reg(pos, is_64bit_address(control)),
-			&maskbits);
-		temp = (1 << multi_msi_capable(control));
-		temp = ((temp - 1) & ~temp);
-		maskbits |= temp;
-		pci_write_config_dword(dev,
-			msi_mask_bits_reg(pos, is_64bit_address(control)),
-			maskbits);
-	}
+	msi_register_init(dev, entry);
+
 	attach_msi_entry(entry, vector);
 	/* Set MSI enabled bits	 */
 	enable_msi_mode(dev, pos, PCI_CAP_ID_MSI);
@@ -721,6 +733,8 @@ int pci_enable_msi(struct pci_dev* dev)
 			vector_irq[dev->irq] = -1;
 			nr_released_vectors--;
 			spin_unlock_irqrestore(&msi_lock, flags);
+
+			msi_register_init(dev, msi_desc[dev->irq]);
 			enable_msi_mode(dev, pos, PCI_CAP_ID_MSI);
 			return 0;
 		}
_

Re: [RFC/PATCH]reconfigure MSI registers after resume

[Greg KH]

On Thu, Aug 18, 2005 at 01:35:46PM +0800, Shaohua Li wrote:
> Hi,
> It appears pci_enable_msi doesn't reconfigure msi registers if it
> successfully look up a msi for a device. It assumes the data and address
> registers unchanged after calling pci_disable_msi. But this isn't always
> true, such as in a suspend/resume circle. In my test system, the
> registers unsurprised become zero after a S3 resume. This patch fixes my
> problem, please look at it. MSIX might have the same issue, but I
> haven't taken a close look.

Tom, any comments on this?

thanks,

greg k-h

RE: [RFC/PATCH]reconfigure MSI registers after resume

[Nguyen, Tom L]

On Wednesday, August 31, 2005 2:44 PM Greg KH wrote:
>>On Thu, Aug 18, 2005 at 01:35:46PM +0800, Shaohua Li wrote:
>> Hi,
>> It appears pci_enable_msi doesn't reconfigure msi registers if it
>> successfully look up a msi for a device. It assumes the data and
address
>> registers unchanged after calling pci_disable_msi. But this isn't
always
>> true, such as in a suspend/resume circle. In my test system, the
>> registers unsurprised become zero after a S3 resume. This patch fixes
my
>> problem, please look at it. MSIX might have the same issue, but I
>> haven't taken a close look.

> Tom, any comments on this?

In the cases of suspend/resume, a device driver needs to restore its PCI
configuration space registers, which include the MSI/MSI-X capability
structures if a device uses MSI/MSI-X. I think reconfiguring MSI
data/address each time a driver calls pci_enable_msi may not be
necessary.

Thanks,
Tom

Re: [RFC/PATCH]reconfigure MSI registers after resume

[Andrew Morton]

"Nguyen, Tom L" <tom.l.nguyen@intel.com> wrote:
>
> On Wednesday, August 31, 2005 2:44 PM Greg KH wrote:
> >>On Thu, Aug 18, 2005 at 01:35:46PM +0800, Shaohua Li wrote:
> >> Hi,
> >> It appears pci_enable_msi doesn't reconfigure msi registers if it
> >> successfully look up a msi for a device. It assumes the data and
> address
> >> registers unchanged after calling pci_disable_msi. But this isn't
> always
> >> true, such as in a suspend/resume circle. In my test system, the
> >> registers unsurprised become zero after a S3 resume. This patch fixes
> my
> >> problem, please look at it. MSIX might have the same issue, but I
> >> haven't taken a close look.
> 
> > Tom, any comments on this?
> 
> In the cases of suspend/resume, a device driver needs to restore its PCI
> configuration space registers, which include the MSI/MSI-X capability
> structures if a device uses MSI/MSI-X. I think reconfiguring MSI
> data/address each time a driver calls pci_enable_msi may not be
> necessary.
> 

So what is the alternative to Shaohua's fix?  Restore all the msi registers
on resume?

RE: [RFC/PATCH]reconfigure MSI registers after resume

[Nguyen, Tom L]

On Thursday, September 01, 2005 12:32 PM Andrew Morton wrote:
> So what is the alternative to Shaohua's fix?  Restore all the msi 
> registers on resume?

Yes, the PCIe port bus driver, for example, did that.

Thanks,
Tom

Re: [RFC/PATCH]reconfigure MSI registers after resume

[Andrew Morton]

"Nguyen, Tom L" <tom.l.nguyen@intel.com> wrote:
>
> On Thursday, September 01, 2005 12:32 PM Andrew Morton wrote:
> > So what is the alternative to Shaohua's fix?  Restore all the msi 
> > registers on resume?
> 
> Yes, the PCIe port bus driver, for example, did that.
> 

So you're saying that each individual driver which uses MSI is responsible
for the restore?  Is it not possible to do this in some single centralised
place?

RE: [RFC/PATCH]reconfigure MSI registers after resume

[Nguyen, Tom L]

On Thursday, September 01, 2005 1:10 PM Andrew Morton wrote:
>>
>> On Thursday, September 01, 2005 12:32 PM Andrew Morton wrote:
>> > So what is the alternative to Shaohua's fix?  Restore all the msi 
>> > registers on resume?
>> 
>> Yes, the PCIe port bus driver, for example, did that.
>> 

> So you're saying that each individual driver which uses MSI is
responsible
> for the restore?  
Yes.

> Is it not possible to do this in some single centralized place?
Existing pci_save_state(dev)/pci_restore_state(dev) covers only 64 bytes
of PCI header. One solution is to extend these APIs to cover up to 256
bytes. What do you think?

Thanks,
Tom

RE: [RFC/PATCH]reconfigure MSI registers after resume

[Shaohua Li]

On Thu, 2005-09-01 at 23:20 +0800, Nguyen, Tom L wrote:
> On Wednesday, August 31, 2005 2:44 PM Greg KH wrote:
> >>On Thu, Aug 18, 2005 at 01:35:46PM +0800, Shaohua Li wrote:
> >> Hi,
> >> It appears pci_enable_msi doesn't reconfigure msi registers if it
> >> successfully look up a msi for a device. It assumes the data and
> address
> >> registers unchanged after calling pci_disable_msi. But this isn't
> always
> >> true, such as in a suspend/resume circle. In my test system, the
> >> registers unsurprised become zero after a S3 resume. This patch fixes
> my
> >> problem, please look at it. MSIX might have the same issue, but I
> >> haven't taken a close look.
> 
> > Tom, any comments on this?
> 
> In the cases of suspend/resume, a device driver needs to restore its PCI
> configuration space registers, which include the MSI/MSI-X capability
> structures if a device uses MSI/MSI-X. I think reconfiguring MSI
> data/address each time a driver calls pci_enable_msi may not be
> necessary.
Just when you called pci_disable_msi, reconfiguring MSI registers should
be done. Is there any pain of reconfiguring MSI registers?
I don't understand why should we have the assumption. If you disabled
the ability, you must reconfigure it to me. This is the easy logic.

Thanks,
Shaohua

Re: [RFC/PATCH]reconfigure MSI registers after resume

[Greg KH]

On Thu, Sep 01, 2005 at 01:59:32PM -0700, Nguyen, Tom L wrote:
> On Thursday, September 01, 2005 1:10 PM Andrew Morton wrote:
> >>
> >> On Thursday, September 01, 2005 12:32 PM Andrew Morton wrote:
> >> > So what is the alternative to Shaohua's fix?  Restore all the msi 
> >> > registers on resume?
> >> 
> >> Yes, the PCIe port bus driver, for example, did that.
> >> 
> 
> > So you're saying that each individual driver which uses MSI is
> responsible
> > for the restore?  
> Yes.
> 
> > Is it not possible to do this in some single centralized place?
> Existing pci_save_state(dev)/pci_restore_state(dev) covers only 64 bytes
> of PCI header. One solution is to extend these APIs to cover up to 256
> bytes. What do you think?

Will that solve this issue?  I need to dig up my PCI spec to see if that
will still work properly on older pci devices...

thanks,

greg k-h

RE: [RFC/PATCH]reconfigure MSI registers after resume

[Nguyen, Tom L]

On Thursday, September 01, 2005 10:38 PM Greg KH wrote:
>> Existing pci_save_state(dev)/pci_restore_state(dev) covers only 64
bytes
>> of PCI header. One solution is to extend these APIs to cover up to
256
>> bytes. What do you think?
> Will that solve this issue? 
Yes.

Thanks,
Tom

Re: [RFC/PATCH]reconfigure MSI registers after resume

[Rajesh Shah]

On Thu, Sep 01, 2005 at 01:59:32PM -0700, Nguyen, Tom L wrote:
> On Thursday, September 01, 2005 1:10 PM Andrew Morton wrote:
> > Is it not possible to do this in some single centralized place?
> Existing pci_save_state(dev)/pci_restore_state(dev) covers only 64 bytes
> of PCI header. One solution is to extend these APIs to cover up to 256
> bytes. What do you think?
> 
No, we can't have these generic functions blindly save/restore
device specific parts of the config space (offset 64+). I know
of several chipset devices which have read-clear or write-clear
bits where reading/writing would have bad side effects. If at
all the pci core does this, it needs to explicitly walk the
capability list and save/restore the well known capability
registers only.

Rajesh

Re: [RFC/PATCH]reconfigure MSI registers after resume

[Andrew Morton]

Rajesh Shah <rajesh.shah@intel.com> wrote:
>
> On Thu, Sep 01, 2005 at 01:59:32PM -0700, Nguyen, Tom L wrote:
> > On Thursday, September 01, 2005 1:10 PM Andrew Morton wrote:
> > > Is it not possible to do this in some single centralized place?
> > Existing pci_save_state(dev)/pci_restore_state(dev) covers only 64 bytes
> > of PCI header. One solution is to extend these APIs to cover up to 256
> > bytes. What do you think?
> > 
> No, we can't have these generic functions blindly save/restore
> device specific parts of the config space (offset 64+). I know
> of several chipset devices which have read-clear or write-clear
> bits where reading/writing would have bad side effects. If at
> all the pci core does this, it needs to explicitly walk the
> capability list and save/restore the well known capability
> registers only.
> 

OK, thanks.  I'll drop Shaohua's
reconfigure-msi-registers-after-resume.patch while this gets sorted out.