[PATCH 5/5] s390/MSI: Fix msi mask issue

[PATCH 5/5] s390/MSI: Fix msi mask issue

[Yijing Wang]

Maskbit in msi_attrib indicates MSI whether
support mask-pending bit. It is read only,
should save mask state in masked.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 arch/s390/pci/pci.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index bdf0257..4651d6b 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -263,7 +263,7 @@ static int zpci_msi_set_mask_bits(struct msi_desc *msi, u32 mask, u32 flag)
 	} else
 		return 0;
 
-	msi->msi_attrib.maskbit = !!flag;
+	msi->masked = !!flag;
 	return 1;
 }
 
-- 
1.7.1

Re: [PATCH 5/5] s390/MSI: Fix msi mask issue

[Sebastian Ott]

On Thu, 19 Jun 2014, Yijing Wang wrote:

> Maskbit in msi_attrib indicates MSI whether
> support mask-pending bit. It is read only,
> should save mask state in masked.
> 
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> ---
>  arch/s390/pci/pci.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index bdf0257..4651d6b 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -263,7 +263,7 @@ static int zpci_msi_set_mask_bits(struct msi_desc *msi, u32 mask, u32 flag)
>  	} else
>  		return 0;
> 
> -	msi->msi_attrib.maskbit = !!flag;
> +	msi->masked = !!flag;
>  	return 1;
>  }
> 
> -- 
> 1.7.1
> 
> 
> 
Applied, thanks!
Sebastian

Re: [PATCH 5/5] s390/MSI: Fix msi mask issue

[Bjorn Helgaas]

On Thu, Jun 19, 2014 at 04:31:14PM +0800, Yijing Wang wrote:
> Maskbit in msi_attrib indicates MSI whether
> support mask-pending bit. It is read only,
> should save mask state in masked.
> 
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> ---
>  arch/s390/pci/pci.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index bdf0257..4651d6b 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -263,7 +263,7 @@ static int zpci_msi_set_mask_bits(struct msi_desc *msi, u32 mask, u32 flag)
>  	} else
>  		return 0;
>  
> -	msi->msi_attrib.maskbit = !!flag;
> +	msi->masked = !!flag;

This doesn't look right.  For MSI-X, you just overwrote the previous update
of msi->masked:

    if (msi->msi_attrib.is_msix) {
	...
	msi->masked = readl(msi->mask_base + offset);

Maybe the update values end up being the same, but why do it twice?

Also, the MSI mask bits are 32-bit field that can individually mask any
subset of the 32 possible MSI vectors, and you collapsed this down to a
single bit, so msi->masked no longer contains that information.

Maybe none of this matters because all the zpci_msi_set_mask_bits() uses
seem to be for a single bit, but the name says "mask_*bits*" and the
msi->masked field is generic and covers multiple MSI vectors in general.

>  	return 1;
>  }
>  
> -- 
> 1.7.1
> 
> 

Re: [PATCH 5/5] s390/MSI: Fix msi mask issue

[Yijing Wang]

>> @@ -263,7 +263,7 @@ static int zpci_msi_set_mask_bits(struct msi_desc *msi, u32 mask, u32 flag)
>>  	} else
>>  		return 0;
>>  
>> -	msi->msi_attrib.maskbit = !!flag;
>> +	msi->masked = !!flag;
> 
> This doesn't look right.  For MSI-X, you just overwrote the previous update
> of msi->masked:
> 
>     if (msi->msi_attrib.is_msix) {
> 	...
> 	msi->masked = readl(msi->mask_base + offset);
> 
> Maybe the update values end up being the same, but why do it twice?

Hi Bjorn, this code is some difficult to understand. I guess it's for save
MSI mask status, for MSI-X, do this twice, maybe this line should move up int to
MSI if ()...

Because I don't have the platform to test, so I do not want to change the code logic,
what I am sure is save the flag status in msi->msi_attrib.maskbit  is wrong.
So I change this like:
	msi->masked = !!flag;

Sebastian may know why do it like this.

Besides, I think we can use MSI code driver interface mask_msi_irq() and unmask_msi_irq()
to replace zpci_enable_irq() and zpci_disable_irq().

> 
> Also, the MSI mask bits are 32-bit field that can individually mask any
> subset of the 32 possible MSI vectors, and you collapsed this down to a
> single bit, so msi->masked no longer contains that information.
> 
> Maybe none of this matters because all the zpci_msi_set_mask_bits() uses
> seem to be for a single bit, but the name says "mask_*bits*" and the
> msi->masked field is generic and covers multiple MSI vectors in general.

Yes, it always uses single bit here, as you said, this code has some problems for
multiple MSI, although it may does not use it.

I will try to send a new patch fix the original problem.

Thanks!
Yijing.
> 
>>  	return 1;
>>  }
>>  
>> -- 
>> 1.7.1
>>
>>
> 
> .
> 


-- 
Thanks!
Yijing