diff mbox series

[v2] PCI/MSI: Correct use of can_mask in msi_add_msi_desc()

Message ID d818f9c9-a432-213e-4152-eaff3b7da52e@oderland.se (mailing list archive)
State New, archived
Headers show
Series [v2] PCI/MSI: Correct use of can_mask in msi_add_msi_desc() | expand

Commit Message

Josef Johansson Feb. 14, 2022, 10:07 a.m. UTC
From: Josef Johansson <josef@oderland.se>

PCI/MSI: Correct use of can_mask in msi_add_msi_desc()
    
Commit 71020a3c0dff4 ("PCI/MSI: Use msi_add_msi_desc()") modifies
the logic of checking msi_attrib.can_mask, without any reason.
    
This commits restores that logic.

Fixes: 71020a3c0dff4 ("PCI/MSI: Use msi_add_msi_desc()")
Signed-off-by: Josef Johansson <josef@oderland.se>

---
v2: Changing subject line to fit earlier commits.

Trying to fix a NULL BUG in the NVMe MSIX implementation I stumbled upon this code,
which ironically was what my last MSI patch resulted into.

I don't see any reason why this logic was change, and it did not break anything
correcting the logic.

CC xen-devel since it very much relates to Xen kernel (via pci_msi_ignore_mask).
---


--
2.31.1

Comments

Josef Johansson Aug. 5, 2022, 10:03 a.m. UTC | #1
On 2/14/22 11:07, Josef Johansson wrote:
> From: Josef Johansson <josef@oderland.se>
>
> PCI/MSI: Correct use of can_mask in msi_add_msi_desc()
>      
> Commit 71020a3c0dff4 ("PCI/MSI: Use msi_add_msi_desc()") modifies
> the logic of checking msi_attrib.can_mask, without any reason.
>      
> This commits restores that logic.
>
> Fixes: 71020a3c0dff4 ("PCI/MSI: Use msi_add_msi_desc()")
> Signed-off-by: Josef Johansson <josef@oderland.se>
>
> ---
> v2: Changing subject line to fit earlier commits.
>
> Trying to fix a NULL BUG in the NVMe MSIX implementation I stumbled upon this code,
> which ironically was what my last MSI patch resulted into.
>
> I don't see any reason why this logic was change, and it did not break anything
> correcting the logic.
>
> CC xen-devel since it very much relates to Xen kernel (via pci_msi_ignore_mask).
> ---
>
> diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
> index c19c7ca58186..146e7b9a01cc 100644
> --- a/drivers/pci/msi/msi.c
> +++ b/drivers/pci/msi/msi.c
> @@ -526,7 +526,7 @@ static int msix_setup_msi_descs(struct pci_dev *dev, void __iomem *base,
>   		desc.pci.msi_attrib.can_mask = !pci_msi_ignore_mask &&
>   					       !desc.pci.msi_attrib.is_virtual;
>   
> -		if (!desc.pci.msi_attrib.can_mask) {
> +		if (desc.pci.msi_attrib.can_mask) {
>   			addr = pci_msix_desc_addr(&desc);
>   			desc.pci.msix_ctrl = readl(addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
>   		}
>
> --
> 2.31.1
>

Hi,

I'm sending a ping such that this patch will not get forgotten in the 
oblivion.

Regards
Josef
Jason Gunthorpe Aug. 5, 2022, 12:10 p.m. UTC | #2
On Fri, Aug 05, 2022 at 12:03:15PM +0200, Josef Johansson wrote:
> On 2/14/22 11:07, Josef Johansson wrote:
> > From: Josef Johansson <josef@oderland.se>
> > 
> > PCI/MSI: Correct use of can_mask in msi_add_msi_desc()
> > Commit 71020a3c0dff4 ("PCI/MSI: Use msi_add_msi_desc()") modifies
> > the logic of checking msi_attrib.can_mask, without any reason.
> > This commits restores that logic.
> >
> > Fixes: 71020a3c0dff4 ("PCI/MSI: Use msi_add_msi_desc()")
> > Signed-off-by: Josef Johansson <josef@oderland.se>
> > 
> > ---
> > v2: Changing subject line to fit earlier commits.
> > 
> > Trying to fix a NULL BUG in the NVMe MSIX implementation I stumbled upon this code,
> > which ironically was what my last MSI patch resulted into.
> > 
> > I don't see any reason why this logic was change, and it did not break anything
> > correcting the logic.
> > 
> > CC xen-devel since it very much relates to Xen kernel (via pci_msi_ignore_mask).
> > ---
> > 
> > diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
> > index c19c7ca58186..146e7b9a01cc 100644
> > --- a/drivers/pci/msi/msi.c
> > +++ b/drivers/pci/msi/msi.c
> > @@ -526,7 +526,7 @@ static int msix_setup_msi_descs(struct pci_dev *dev, void __iomem *base,
> >   		desc.pci.msi_attrib.can_mask = !pci_msi_ignore_mask &&
> >   					       !desc.pci.msi_attrib.is_virtual;
> > -		if (!desc.pci.msi_attrib.can_mask) {
> > +		if (desc.pci.msi_attrib.can_mask) {
> >   			addr = pci_msix_desc_addr(&desc);
> >   			desc.pci.msix_ctrl = readl(addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
> >   		}
> > 

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Bjorn, please take it?

Jason
Bjorn Helgaas Aug. 5, 2022, 3:53 p.m. UTC | #3
On Fri, Aug 05, 2022 at 09:10:41AM -0300, Jason Gunthorpe wrote:
> On Fri, Aug 05, 2022 at 12:03:15PM +0200, Josef Johansson wrote:
> > On 2/14/22 11:07, Josef Johansson wrote:
> > > From: Josef Johansson <josef@oderland.se>
> > > 
> > > PCI/MSI: Correct use of can_mask in msi_add_msi_desc()
> > > Commit 71020a3c0dff4 ("PCI/MSI: Use msi_add_msi_desc()") modifies
> > > the logic of checking msi_attrib.can_mask, without any reason.
> > > This commits restores that logic.
> > >
> > > Fixes: 71020a3c0dff4 ("PCI/MSI: Use msi_add_msi_desc()")
> > > Signed-off-by: Josef Johansson <josef@oderland.se>
> > > 
> > > ---
> > > v2: Changing subject line to fit earlier commits.
> > > 
> > > Trying to fix a NULL BUG in the NVMe MSIX implementation I stumbled upon this code,
> > > which ironically was what my last MSI patch resulted into.
> > > 
> > > I don't see any reason why this logic was change, and it did not break anything
> > > correcting the logic.
> > > 
> > > CC xen-devel since it very much relates to Xen kernel (via pci_msi_ignore_mask).
> > > ---
> > > 
> > > diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
> > > index c19c7ca58186..146e7b9a01cc 100644
> > > --- a/drivers/pci/msi/msi.c
> > > +++ b/drivers/pci/msi/msi.c
> > > @@ -526,7 +526,7 @@ static int msix_setup_msi_descs(struct pci_dev *dev, void __iomem *base,
> > >   		desc.pci.msi_attrib.can_mask = !pci_msi_ignore_mask &&
> > >   					       !desc.pci.msi_attrib.is_virtual;
> > > -		if (!desc.pci.msi_attrib.can_mask) {
> > > +		if (desc.pci.msi_attrib.can_mask) {
> > >   			addr = pci_msix_desc_addr(&desc);
> > >   			desc.pci.msix_ctrl = readl(addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
> > >   		}
> > > 
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> 
> Bjorn, please take it?

Thanks for the ping.  Since 71020a3c0dff4 is by Thomas, and he merged
that along with a whole series of MSI work, I think I probably
expected him to take care of this.

This looks like a simple typo, so I think the commit log should be
reworded along that line, e.g., something like:

  71020a3c0dff4 ("PCI/MSI: Use msi_add_msi_desc()") inadvertently
  reversed the sense of "msi_attrib.can_mask" in one use:

    - if (entry->pci.msi_attrib.can_mask) {
    -         addr = pci_msix_desc_addr(entry);
    -         entry->pci.msix_ctrl = readl(addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
    + if (!desc.pci.msi_attrib.can_mask) {
    +         addr = pci_msix_desc_addr(&desc);
    +         desc.pci.msix_ctrl = readl(addr + PCI_MSIX_ENTRY_VECTOR_CTRL);

  Restore the original test.

Thomas, do you want to take this?  I'm happy to merge it, but would
like your reviewed-by or ack first.

Bjorn
Jason Gunthorpe Aug. 26, 2022, 1:08 p.m. UTC | #4
On Fri, Aug 05, 2022 at 10:53:36AM -0500, Bjorn Helgaas wrote:
> On Fri, Aug 05, 2022 at 09:10:41AM -0300, Jason Gunthorpe wrote:
> > On Fri, Aug 05, 2022 at 12:03:15PM +0200, Josef Johansson wrote:
> > > On 2/14/22 11:07, Josef Johansson wrote:
> > > > From: Josef Johansson <josef@oderland.se>
> > > > 
> > > > PCI/MSI: Correct use of can_mask in msi_add_msi_desc()
> > > > Commit 71020a3c0dff4 ("PCI/MSI: Use msi_add_msi_desc()") modifies
> > > > the logic of checking msi_attrib.can_mask, without any reason.
> > > > This commits restores that logic.
> > > >
> > > > Fixes: 71020a3c0dff4 ("PCI/MSI: Use msi_add_msi_desc()")
> > > > Signed-off-by: Josef Johansson <josef@oderland.se>
> > > > 
> > > > ---
> > > > v2: Changing subject line to fit earlier commits.
> > > > 
> > > > Trying to fix a NULL BUG in the NVMe MSIX implementation I stumbled upon this code,
> > > > which ironically was what my last MSI patch resulted into.
> > > > 
> > > > I don't see any reason why this logic was change, and it did not break anything
> > > > correcting the logic.
> > > > 
> > > > CC xen-devel since it very much relates to Xen kernel (via pci_msi_ignore_mask).
> > > > ---
> > > > 
> > > > diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
> > > > index c19c7ca58186..146e7b9a01cc 100644
> > > > --- a/drivers/pci/msi/msi.c
> > > > +++ b/drivers/pci/msi/msi.c
> > > > @@ -526,7 +526,7 @@ static int msix_setup_msi_descs(struct pci_dev *dev, void __iomem *base,
> > > >   		desc.pci.msi_attrib.can_mask = !pci_msi_ignore_mask &&
> > > >   					       !desc.pci.msi_attrib.is_virtual;
> > > > -		if (!desc.pci.msi_attrib.can_mask) {
> > > > +		if (desc.pci.msi_attrib.can_mask) {
> > > >   			addr = pci_msix_desc_addr(&desc);
> > > >   			desc.pci.msix_ctrl = readl(addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
> > > >   		}
> > > > 
> > 
> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> > 
> > Bjorn, please take it?
> 
> Thanks for the ping.  Since 71020a3c0dff4 is by Thomas, and he merged
> that along with a whole series of MSI work, I think I probably
> expected him to take care of this.
> 
> This looks like a simple typo, so I think the commit log should be
> reworded along that line, e.g., something like:
> 
>   71020a3c0dff4 ("PCI/MSI: Use msi_add_msi_desc()") inadvertently
>   reversed the sense of "msi_attrib.can_mask" in one use:
> 
>     - if (entry->pci.msi_attrib.can_mask) {
>     -         addr = pci_msix_desc_addr(entry);
>     -         entry->pci.msix_ctrl = readl(addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
>     + if (!desc.pci.msi_attrib.can_mask) {
>     +         addr = pci_msix_desc_addr(&desc);
>     +         desc.pci.msix_ctrl = readl(addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
> 
>   Restore the original test.
> 
> Thomas, do you want to take this?  I'm happy to merge it, but would
> like your reviewed-by or ack first.

At this point I think you should take it Bjorn..

Jason
Bjorn Helgaas Aug. 26, 2022, 3:51 p.m. UTC | #5
On Mon, Feb 14, 2022 at 11:07:47AM +0100, Josef Johansson wrote:
> From: Josef Johansson <josef@oderland.se>
> 
> PCI/MSI: Correct use of can_mask in msi_add_msi_desc()
>     
> Commit 71020a3c0dff4 ("PCI/MSI: Use msi_add_msi_desc()") modifies
> the logic of checking msi_attrib.can_mask, without any reason.
>     
> This commits restores that logic.
> 
> Fixes: 71020a3c0dff4 ("PCI/MSI: Use msi_add_msi_desc()")
> Signed-off-by: Josef Johansson <josef@oderland.se>

Applied to pci/misc for v6.1 with commit log below, thanks!

  PCI/MSI: Correct 'can_mask' test in msi_add_msi_desc()

  71020a3c0dff4 ("PCI/MSI: Use msi_add_msi_desc()") inadvertently reversed
  the sense of "msi_attrib.can_mask" in one use:

    - if (entry->pci.msi_attrib.can_mask) {
    -         addr = pci_msix_desc_addr(entry);
    -         entry->pci.msix_ctrl = readl(addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
    + if (!desc.pci.msi_attrib.can_mask) {
    +         addr = pci_msix_desc_addr(&desc);
    +         desc.pci.msix_ctrl = readl(addr + PCI_MSIX_ENTRY_VECTOR_CTRL);

  Restore the original test.

> ---
> v2: Changing subject line to fit earlier commits.
> 
> Trying to fix a NULL BUG in the NVMe MSIX implementation I stumbled upon this code,
> which ironically was what my last MSI patch resulted into.
> 
> I don't see any reason why this logic was change, and it did not break anything
> correcting the logic.
> 
> CC xen-devel since it very much relates to Xen kernel (via pci_msi_ignore_mask).
> ---
> 
> diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
> index c19c7ca58186..146e7b9a01cc 100644
> --- a/drivers/pci/msi/msi.c
> +++ b/drivers/pci/msi/msi.c
> @@ -526,7 +526,7 @@ static int msix_setup_msi_descs(struct pci_dev *dev, void __iomem *base,
>  		desc.pci.msi_attrib.can_mask = !pci_msi_ignore_mask &&
>  					       !desc.pci.msi_attrib.is_virtual;
>  
> -		if (!desc.pci.msi_attrib.can_mask) {
> +		if (desc.pci.msi_attrib.can_mask) {
>  			addr = pci_msix_desc_addr(&desc);
>  			desc.pci.msix_ctrl = readl(addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
>  		}
> 
> --
> 2.31.1
> 
>
diff mbox series

Patch

diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
index c19c7ca58186..146e7b9a01cc 100644
--- a/drivers/pci/msi/msi.c
+++ b/drivers/pci/msi/msi.c
@@ -526,7 +526,7 @@  static int msix_setup_msi_descs(struct pci_dev *dev, void __iomem *base,
 		desc.pci.msi_attrib.can_mask = !pci_msi_ignore_mask &&
 					       !desc.pci.msi_attrib.is_virtual;
 
-		if (!desc.pci.msi_attrib.can_mask) {
+		if (desc.pci.msi_attrib.can_mask) {
 			addr = pci_msix_desc_addr(&desc);
 			desc.pci.msix_ctrl = readl(addr + PCI_MSIX_ENTRY_VECTOR_CTRL);
 		}