Patchwork PCI: Avoid initialize MSI/MSIX if device power state != PCI_D0

login
register
mail settings
Submitter Yijing Wang
Date Oct. 10, 2013, 8:31 a.m.
Message ID <1381393916-12268-1-git-send-email-wangyijing@huawei.com>
Download mbox | patch
Permalink /patch/282171/
State Superseded
Headers show

Comments

Yijing Wang - Oct. 10, 2013, 8:31 a.m.
Currently, if device power state != PCI_D0, we still initialize
device MSI/MSIX, but we won't write the MSI message to device
MSI/MSIX registers. It's weird, we don't configure MSI/MSIX
registers properly, but pci_enable_msi() or pci_enable_msix()
return success, and even these registers will never be updated later.
So I think it should return error if device power state != PCI_D0.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/msi.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)
Ben Hutchings - Oct. 10, 2013, 12:15 p.m.
On Thu, 2013-10-10 at 16:31 +0800, Yijing Wang wrote:
> Currently, if device power state != PCI_D0, we still initialize
> device MSI/MSIX, but we won't write the MSI message to device
> MSI/MSIX registers. It's weird, we don't configure MSI/MSIX
> registers properly, but pci_enable_msi() or pci_enable_msix()
> return success, and even these registers will never be updated later.
> So I think it should return error if device power state != PCI_D0.
> 
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> ---
>  drivers/pci/msi.c |   10 ++++------
>  1 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index d5f90d6..eb502f6 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -308,9 +308,7 @@ void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg)
>  
>  void __write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
>  {
> -	if (entry->dev->current_state != PCI_D0) {
> -		/* Don't touch the hardware now */
> -	} else if (entry->msi_attrib.is_msix) {
> +	if (entry->msi_attrib.is_msix) {
[...]

As I said before, this function was being called to change IRQ
affinities during suspend/resume at a point when most PCI devices were
in D3.  If that is no longer the case then this change is probably OK.
Otherwise you should not touch this function.

Ben.
Yijing Wang - Oct. 10, 2013, 12:32 p.m.
On 2013/10/10 20:15, Ben Hutchings wrote:
> On Thu, 2013-10-10 at 16:31 +0800, Yijing Wang wrote:
>> Currently, if device power state != PCI_D0, we still initialize
>> device MSI/MSIX, but we won't write the MSI message to device
>> MSI/MSIX registers. It's weird, we don't configure MSI/MSIX
>> registers properly, but pci_enable_msi() or pci_enable_msix()
>> return success, and even these registers will never be updated later.
>> So I think it should return error if device power state != PCI_D0.
>>
>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>> ---
>>  drivers/pci/msi.c |   10 ++++------
>>  1 files changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
>> index d5f90d6..eb502f6 100644
>> --- a/drivers/pci/msi.c
>> +++ b/drivers/pci/msi.c
>> @@ -308,9 +308,7 @@ void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg)
>>  
>>  void __write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
>>  {
>> -	if (entry->dev->current_state != PCI_D0) {
>> -		/* Don't touch the hardware now */
>> -	} else if (entry->msi_attrib.is_msix) {
>> +	if (entry->msi_attrib.is_msix) {
> [...]
> 
> As I said before, this function was being called to change IRQ
> affinities during suspend/resume at a point when most PCI devices were
> in D3.  If that is no longer the case then this change is probably OK.
> Otherwise you should not touch this function.

Hi ben, sorry for the mistake, now I know what you worry about. I will update this patch,
keep __write_msi_msg() function not changed. Only add some guard for pci_enable_msi()/pci_enable_msix() ..

Thanks!
Yijing.

> 
> Ben.
>

Patch

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index d5f90d6..eb502f6 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -308,9 +308,7 @@  void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg)
 
 void __write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
 {
-	if (entry->dev->current_state != PCI_D0) {
-		/* Don't touch the hardware now */
-	} else if (entry->msi_attrib.is_msix) {
+	if (entry->msi_attrib.is_msix) {
 		void __iomem *base;
 		base = entry->mask_base +
 			entry->msi_attrib.entry_nr * PCI_MSIX_ENTRY_SIZE;
@@ -831,7 +829,7 @@  int pci_enable_msi_block(struct pci_dev *dev, unsigned int nvec)
 	int status, maxvec;
 	u16 msgctl;
 
-	if (!dev->msi_cap)
+	if (!dev->msi_cap || dev->current_state != PCI_D0)
 		return -EINVAL;
 
 	pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &msgctl);
@@ -862,7 +860,7 @@  int pci_enable_msi_block_auto(struct pci_dev *dev, unsigned int *maxvec)
 	int ret, nvec;
 	u16 msgctl;
 
-	if (!dev->msi_cap)
+	if (!dev->msi_cap || dev->current_state != PCI_D0)
 		return -EINVAL;
 
 	pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &msgctl);
@@ -955,7 +953,7 @@  int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec)
 	int status, nr_entries;
 	int i, j;
 
-	if (!entries || !dev->msix_cap)
+	if (!entries || !dev->msix_cap || dev->current_state != PCI_D0)
 		return -EINVAL;
 
 	status = pci_msi_check_device(dev, nvec, PCI_CAP_ID_MSIX);