| Message ID | f5a224ee-b72f-7053-6030-b6c4d8a29be9@oderland.se |
|---|---|
| State | New |
| Headers | show |
| Series | PCI/MSI: msix_setup_msi_descs: Restore logic for msi_attrib.can_mask | expand |
[+cc Jason, since you reviewed the original commit] On Sat, Jan 22, 2022 at 02:10:01AM +0100, Josef Johansson wrote: > From: Josef Johansson <josef@oderland.se> > > PCI/MSI: msix_setup_msi_descs: Restore logic for msi_attrib.can_mask Please match the form and style of previous subject lines (in particular, omit "msix_setup_msi_descs:"). > 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. I agree, this looks like a typo in 71020a3c0dff4, but I might be missing something, so Thomas should take a look, and I added Jason since he reviewed it. Since it was merged by Thomas, I'll let him take care of this, too. If it *is* a typo, the fix looks like v5.17 material. Before: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/msi/msi.c?id=71020a3c0dff4%5E#n522 After: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/msi/msi.c?id=71020a3c0dff4#n520 > Fixes: 71020a3c0dff4 ("PCI/MSI: Use msi_add_msi_desc()") > Signed-off-by: Josef Johansson <josef@oderland.se> > > --- > 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, nor do I have the possibility > to see if anything works with my patch or without, since the kernel crashes > in other places. > > As such this is still untested, but as far as I can tell it should restore > functionality. > > Re-sending since it was rejected by linux-pci@vger.kernel.org due to HTML contents. > Sorry about that. > > 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 >
On 2/11/22 00:55, Bjorn Helgaas wrote: > [+cc Jason, since you reviewed the original commit] > > On Sat, Jan 22, 2022 at 02:10:01AM +0100, Josef Johansson wrote: >> From: Josef Johansson <josef@oderland.se> >> >> PCI/MSI: msix_setup_msi_descs: Restore logic for msi_attrib.can_mask > Please match the form and style of previous subject lines (in > particular, omit "msix_setup_msi_descs:"). Would this subject suffice? 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. > I agree, this looks like a typo in 71020a3c0dff4, but I might be > missing something, so Thomas should take a look, and I added Jason > since he reviewed it. > > Since it was merged by Thomas, I'll let him take care of this, too. > If it *is* a typo, the fix looks like v5.17 material. > > Before: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/msi/msi.c?id=71020a3c0dff4%5E#n522 > After: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/msi/msi.c?id=71020a3c0dff4#n520 It should be noted that I successfully ran the kernel with this patch. >> Fixes: 71020a3c0dff4 ("PCI/MSI: Use msi_add_msi_desc()") >> Signed-off-by: Josef Johansson <josef@oderland.se> >> >> --- >> 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, nor do I have the possibility >> to see if anything works with my patch or without, since the kernel crashes >> in other places. >> >> As such this is still untested, but as far as I can tell it should restore >> functionality. >> >> Re-sending since it was rejected by linux-pci@vger.kernel.org due to HTML contents. >> Sorry about that. >> >> 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 >>
On Thu, Feb 10, 2022 at 05:55:32PM -0600, Bjorn Helgaas wrote: > > 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. > > I agree, this looks like a typo in 71020a3c0dff4, but I might be > missing something, so Thomas should take a look, and I added Jason > since he reviewed it. I concur Jason
On Fri, Feb 11, 2022 at 01:10:22AM +0100, Josef Johansson wrote: > On 2/11/22 00:55, Bjorn Helgaas wrote: > > On Sat, Jan 22, 2022 at 02:10:01AM +0100, Josef Johansson wrote: > >> From: Josef Johansson <josef@oderland.se> > >> > >> PCI/MSI: msix_setup_msi_descs: Restore logic for msi_attrib.can_mask > > Please match the form and style of previous subject lines (in > > particular, omit "msix_setup_msi_descs:"). > Would this subject suffice? > PCI/MSI: Correct use of can_mask in msi_add_msi_desc() Looks good to me! Bjorn
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); }