* Re: [PATCH 3.14 186/228] PCI: Wrong register used to check pending traffic [not found] ` <20140604232354.004988520@linuxfoundation.org> @ 2014-06-08 3:02 ` Ben Hutchings 2014-06-11 21:15 ` Bjorn Helgaas 0 siblings, 1 reply; 4+ messages in thread From: Ben Hutchings @ 2014-06-08 3:02 UTC (permalink / raw) To: Gavin Shan, Bjorn Helgaas Cc: linux-kernel, stable, Alex Williamson, Greg Kroah-Hartman, linux-pci [-- Attachment #1: Type: text/plain, Size: 1944 bytes --] On Wed, 2014-06-04 at 16:23 -0700, Greg Kroah-Hartman wrote: > 3.14-stable review patch. If anyone has any objections, please let me know. > > ------------------ > > From: Gavin Shan <gwshan@linux.vnet.ibm.com> > > commit d0b4cc4e32705ff00d90d32da7783c266c702c04 upstream. > > The incorrect register offset is passed to pci_wait_for_pending(), which is > caused by commit 157e876ffe ("PCI: Add pci_wait_for_pending() (refactor > pci_wait_for_pending_transaction())"). > > Fixes: 157e876ffe ("PCI: Add pci_wait_for_pending() (refactor pci_wait_for_pending_transaction()) > Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > Acked-by: Alex Williamson <alex.williamson@gmail.com> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > --- > drivers/pci/pci.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -3043,7 +3043,8 @@ int pci_wait_for_pending_transaction(str > if (!pci_is_pcie(dev)) > return 1; > > - return pci_wait_for_pending(dev, PCI_EXP_DEVSTA, PCI_EXP_DEVSTA_TRPND); > + return pci_wait_for_pending(dev, pci_pcie_cap(dev) + PCI_EXP_DEVSTA, > + PCI_EXP_DEVSTA_TRPND); > } > EXPORT_SYMBOL(pci_wait_for_pending_transaction); > > @@ -3085,7 +3086,7 @@ static int pci_af_flr(struct pci_dev *de > return 0; > > /* Wait for Transaction Pending bit clean */ > - if (pci_wait_for_pending(dev, PCI_AF_STATUS, PCI_AF_STATUS_TP)) > + if (pci_wait_for_pending(dev, pos + PCI_AF_STATUS, PCI_AF_STATUS_TP)) > goto clear; > > dev_err(&dev->dev, "transaction is not cleared; " This still seems to be broken because pci_wait_for_pending() does pci_read_config_word() but PCI_AF_STATUS is not word-aligned. Ben. -- Ben Hutchings Never attribute to conspiracy what can adequately be explained by stupidity. [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 828 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 3.14 186/228] PCI: Wrong register used to check pending traffic 2014-06-08 3:02 ` [PATCH 3.14 186/228] PCI: Wrong register used to check pending traffic Ben Hutchings @ 2014-06-11 21:15 ` Bjorn Helgaas 2014-06-11 22:07 ` Alex Williamson 0 siblings, 1 reply; 4+ messages in thread From: Bjorn Helgaas @ 2014-06-11 21:15 UTC (permalink / raw) To: Ben Hutchings Cc: Gavin Shan, linux-kernel@vger.kernel.org, stable@vger.kernel.org, Alex Williamson, Greg Kroah-Hartman, linux-pci On Sat, Jun 7, 2014 at 9:02 PM, Ben Hutchings <ben@decadent.org.uk> wrote: > On Wed, 2014-06-04 at 16:23 -0700, Greg Kroah-Hartman wrote: >> 3.14-stable review patch. If anyone has any objections, please let me know. >> >> ------------------ >> >> From: Gavin Shan <gwshan@linux.vnet.ibm.com> >> >> commit d0b4cc4e32705ff00d90d32da7783c266c702c04 upstream. >> >> The incorrect register offset is passed to pci_wait_for_pending(), which is >> caused by commit 157e876ffe ("PCI: Add pci_wait_for_pending() (refactor >> pci_wait_for_pending_transaction())"). >> >> Fixes: 157e876ffe ("PCI: Add pci_wait_for_pending() (refactor pci_wait_for_pending_transaction()) >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> >> Acked-by: Alex Williamson <alex.williamson@gmail.com> >> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> >> --- >> drivers/pci/pci.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -3043,7 +3043,8 @@ int pci_wait_for_pending_transaction(str >> if (!pci_is_pcie(dev)) >> return 1; >> >> - return pci_wait_for_pending(dev, PCI_EXP_DEVSTA, PCI_EXP_DEVSTA_TRPND); >> + return pci_wait_for_pending(dev, pci_pcie_cap(dev) + PCI_EXP_DEVSTA, >> + PCI_EXP_DEVSTA_TRPND); >> } >> EXPORT_SYMBOL(pci_wait_for_pending_transaction); >> >> @@ -3085,7 +3086,7 @@ static int pci_af_flr(struct pci_dev *de >> return 0; >> >> /* Wait for Transaction Pending bit clean */ >> - if (pci_wait_for_pending(dev, PCI_AF_STATUS, PCI_AF_STATUS_TP)) >> + if (pci_wait_for_pending(dev, pos + PCI_AF_STATUS, PCI_AF_STATUS_TP)) >> goto clear; >> >> dev_err(&dev->dev, "transaction is not cleared; " > > This still seems to be broken because pci_wait_for_pending() does > pci_read_config_word() but PCI_AF_STATUS is not word-aligned. I agree; this does seem broken. I think pci_read_config_word() will return PCIBIOS_BAD_REGISTER_NUMBER without updating the value returned, so I think we'll test garbage from the stack. What do you think, Alex? Bjorn ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 3.14 186/228] PCI: Wrong register used to check pending traffic 2014-06-11 21:15 ` Bjorn Helgaas @ 2014-06-11 22:07 ` Alex Williamson 2014-06-12 0:24 ` Gavin Shan 0 siblings, 1 reply; 4+ messages in thread From: Alex Williamson @ 2014-06-11 22:07 UTC (permalink / raw) To: Bjorn Helgaas Cc: Ben Hutchings, Gavin Shan, linux-kernel@vger.kernel.org, stable@vger.kernel.org, Alex Williamson, Greg Kroah-Hartman, linux-pci On Wed, 2014-06-11 at 15:15 -0600, Bjorn Helgaas wrote: > On Sat, Jun 7, 2014 at 9:02 PM, Ben Hutchings <ben@decadent.org.uk> wrote: > > On Wed, 2014-06-04 at 16:23 -0700, Greg Kroah-Hartman wrote: > >> 3.14-stable review patch. If anyone has any objections, please let me know. > >> > >> ------------------ > >> > >> From: Gavin Shan <gwshan@linux.vnet.ibm.com> > >> > >> commit d0b4cc4e32705ff00d90d32da7783c266c702c04 upstream. > >> > >> The incorrect register offset is passed to pci_wait_for_pending(), which is > >> caused by commit 157e876ffe ("PCI: Add pci_wait_for_pending() (refactor > >> pci_wait_for_pending_transaction())"). > >> > >> Fixes: 157e876ffe ("PCI: Add pci_wait_for_pending() (refactor pci_wait_for_pending_transaction()) > >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > >> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > >> Acked-by: Alex Williamson <alex.williamson@gmail.com> > >> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > >> > >> --- > >> drivers/pci/pci.c | 5 +++-- > >> 1 file changed, 3 insertions(+), 2 deletions(-) > >> > >> --- a/drivers/pci/pci.c > >> +++ b/drivers/pci/pci.c > >> @@ -3043,7 +3043,8 @@ int pci_wait_for_pending_transaction(str > >> if (!pci_is_pcie(dev)) > >> return 1; > >> > >> - return pci_wait_for_pending(dev, PCI_EXP_DEVSTA, PCI_EXP_DEVSTA_TRPND); > >> + return pci_wait_for_pending(dev, pci_pcie_cap(dev) + PCI_EXP_DEVSTA, > >> + PCI_EXP_DEVSTA_TRPND); > >> } > >> EXPORT_SYMBOL(pci_wait_for_pending_transaction); > >> > >> @@ -3085,7 +3086,7 @@ static int pci_af_flr(struct pci_dev *de > >> return 0; > >> > >> /* Wait for Transaction Pending bit clean */ > >> - if (pci_wait_for_pending(dev, PCI_AF_STATUS, PCI_AF_STATUS_TP)) > >> + if (pci_wait_for_pending(dev, pos + PCI_AF_STATUS, PCI_AF_STATUS_TP)) > >> goto clear; > >> > >> dev_err(&dev->dev, "transaction is not cleared; " > > > > This still seems to be broken because pci_wait_for_pending() does > > pci_read_config_word() but PCI_AF_STATUS is not word-aligned. > > I agree; this does seem broken. I think pci_read_config_word() will > return PCIBIOS_BAD_REGISTER_NUMBER without updating the value > returned, so I think we'll test garbage from the stack. > > What do you think, Alex? Yep, looks broken. The simple fix would be to pass the AF control offset instead of status, shift the mask, and maybe add a BUG_ON(pos & 1) to pci_read_config_word() to better enforce it. Alternatively we could make the function take a width and use an appropriate accessor for the size. I'm inclined to double-check the current users and take the first option unless anyone objects. Thanks, Alex ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 3.14 186/228] PCI: Wrong register used to check pending traffic 2014-06-11 22:07 ` Alex Williamson @ 2014-06-12 0:24 ` Gavin Shan 0 siblings, 0 replies; 4+ messages in thread From: Gavin Shan @ 2014-06-12 0:24 UTC (permalink / raw) To: Alex Williamson Cc: Bjorn Helgaas, Ben Hutchings, Gavin Shan, linux-kernel@vger.kernel.org, stable@vger.kernel.org, Alex Williamson, Greg Kroah-Hartman, linux-pci On Wed, Jun 11, 2014 at 04:07:44PM -0600, Alex Williamson wrote: >On Wed, 2014-06-11 at 15:15 -0600, Bjorn Helgaas wrote: >> On Sat, Jun 7, 2014 at 9:02 PM, Ben Hutchings <ben@decadent.org.uk> wrote: >> > On Wed, 2014-06-04 at 16:23 -0700, Greg Kroah-Hartman wrote: >> >> 3.14-stable review patch. If anyone has any objections, please let me know. >> >> >> >> ------------------ >> >> >> >> From: Gavin Shan <gwshan@linux.vnet.ibm.com> >> >> >> >> commit d0b4cc4e32705ff00d90d32da7783c266c702c04 upstream. >> >> >> >> The incorrect register offset is passed to pci_wait_for_pending(), which is >> >> caused by commit 157e876ffe ("PCI: Add pci_wait_for_pending() (refactor >> >> pci_wait_for_pending_transaction())"). >> >> >> >> Fixes: 157e876ffe ("PCI: Add pci_wait_for_pending() (refactor pci_wait_for_pending_transaction()) >> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >> >> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> >> >> Acked-by: Alex Williamson <alex.williamson@gmail.com> >> >> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> >> >> >> --- >> >> drivers/pci/pci.c | 5 +++-- >> >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> >> >> --- a/drivers/pci/pci.c >> >> +++ b/drivers/pci/pci.c >> >> @@ -3043,7 +3043,8 @@ int pci_wait_for_pending_transaction(str >> >> if (!pci_is_pcie(dev)) >> >> return 1; >> >> >> >> - return pci_wait_for_pending(dev, PCI_EXP_DEVSTA, PCI_EXP_DEVSTA_TRPND); >> >> + return pci_wait_for_pending(dev, pci_pcie_cap(dev) + PCI_EXP_DEVSTA, >> >> + PCI_EXP_DEVSTA_TRPND); >> >> } >> >> EXPORT_SYMBOL(pci_wait_for_pending_transaction); >> >> >> >> @@ -3085,7 +3086,7 @@ static int pci_af_flr(struct pci_dev *de >> >> return 0; >> >> >> >> /* Wait for Transaction Pending bit clean */ >> >> - if (pci_wait_for_pending(dev, PCI_AF_STATUS, PCI_AF_STATUS_TP)) >> >> + if (pci_wait_for_pending(dev, pos + PCI_AF_STATUS, PCI_AF_STATUS_TP)) >> >> goto clear; >> >> >> >> dev_err(&dev->dev, "transaction is not cleared; " >> > >> > This still seems to be broken because pci_wait_for_pending() does >> > pci_read_config_word() but PCI_AF_STATUS is not word-aligned. >> >> I agree; this does seem broken. I think pci_read_config_word() will >> return PCIBIOS_BAD_REGISTER_NUMBER without updating the value >> returned, so I think we'll test garbage from the stack. >> >> What do you think, Alex? > >Yep, looks broken. The simple fix would be to pass the AF control >offset instead of status, shift the mask, and maybe add a BUG_ON(pos & >1) to pci_read_config_word() to better enforce it. Alternatively we >could make the function take a width and use an appropriate accessor for >the size. I'm inclined to double-check the current users and take the >first option unless anyone objects. Thanks, > Hrm, I didn't notice that PCI_AF_STATUS isn't word-aligned. I agree with Alex to just use PCI_AF_CTRL with shifted PCI_AF_STATUS_TP. If we're going to add BUG_ON() for pci_read_config_word(), we have to add that for other accessors (dword, user_word, user_dword, bus_word, bus_dword). I guess we just need the first part here. Bjorn/Alex, please let me know if I need come up with a followup fix for this, or Alex can handle it. Thanks, Gavin ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-06-12 0:24 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20140604232347.966798903@linuxfoundation.org>
[not found] ` <20140604232354.004988520@linuxfoundation.org>
2014-06-08 3:02 ` [PATCH 3.14 186/228] PCI: Wrong register used to check pending traffic Ben Hutchings
2014-06-11 21:15 ` Bjorn Helgaas
2014-06-11 22:07 ` Alex Williamson
2014-06-12 0:24 ` Gavin Shan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).