* Re: [PATCH] misc/GenWQE: fix pci_enable_msi usage [not found] <alpine.LFD.2.11.1407091237030.1590@denkbrett> @ 2014-07-16 21:10 ` Bjorn Helgaas 2014-07-17 15:48 ` Frank Haverkamp 0 siblings, 1 reply; 4+ messages in thread From: Bjorn Helgaas @ 2014-07-16 21:10 UTC (permalink / raw) To: Sebastian Ott Cc: Frank Haverkamp, Greg Kroah-Hartman, Alexander Gordeev, linux-kernel, linux-pci [+cc linux-pci] On Wed, Jul 09, 2014 at 12:46:39PM +0200, Sebastian Ott wrote: > GenWQE used to call pci_enable_msi_block to allocate a desired number > of MSI's. If that was not possible pci_enable_msi_block returned with a > smaller number which might be possible to allocate. GenWQE then called > pci_enable_msi_block with that number. > > Since commit a30d0108b > "GenWQE: Use pci_enable_msi_exact() instead of pci_enable_msi_block()" > pci_enable_msi_exact is used which fails if the desired number of MSI's > was not possible to allocate. Change GenWQE to use pci_enable_msi_range > to restore the old behavior. > > Signed-off-by: Sebastian Ott <sebott@linux.vnet.ibm.com> Since this fixes a30d0108b09a, which we merged via my tree in v3.16-rc1, I applied this with Alexander and Frank's acks to for-linus for v3.16, thanks! > --- > drivers/misc/genwqe/card_ddcb.c | 4 +--- > drivers/misc/genwqe/card_utils.c | 10 ++++++---- > 2 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/misc/genwqe/card_ddcb.c b/drivers/misc/genwqe/card_ddcb.c > index c8046db..f66d43d 100644 > --- a/drivers/misc/genwqe/card_ddcb.c > +++ b/drivers/misc/genwqe/card_ddcb.c > @@ -1237,9 +1237,7 @@ int genwqe_setup_service_layer(struct genwqe_dev *cd) > } > > rc = genwqe_set_interrupt_capability(cd, GENWQE_MSI_IRQS); > - if (rc > 0) > - rc = genwqe_set_interrupt_capability(cd, rc); > - if (rc != 0) { > + if (rc) { > rc = -ENODEV; > goto stop_kthread; > } > diff --git a/drivers/misc/genwqe/card_utils.c b/drivers/misc/genwqe/card_utils.c > index 62cc6bb..6abc437 100644 > --- a/drivers/misc/genwqe/card_utils.c > +++ b/drivers/misc/genwqe/card_utils.c > @@ -718,10 +718,12 @@ int genwqe_set_interrupt_capability(struct genwqe_dev *cd, int count) > int rc; > struct pci_dev *pci_dev = cd->pci_dev; > > - rc = pci_enable_msi_exact(pci_dev, count); > - if (rc == 0) > - cd->flags |= GENWQE_FLAG_MSI_ENABLED; > - return rc; > + rc = pci_enable_msi_range(pci_dev, 1, count); > + if (rc < 0) > + return rc; > + > + cd->flags |= GENWQE_FLAG_MSI_ENABLED; > + return 0; > } > > /** > -- > 1.9.3 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] misc/GenWQE: fix pci_enable_msi usage 2014-07-16 21:10 ` [PATCH] misc/GenWQE: fix pci_enable_msi usage Bjorn Helgaas @ 2014-07-17 15:48 ` Frank Haverkamp 2014-07-18 8:46 ` Sebastian Ott 0 siblings, 1 reply; 4+ messages in thread From: Frank Haverkamp @ 2014-07-17 15:48 UTC (permalink / raw) To: Bjorn Helgaas Cc: Sebastian Ott, Greg Kroah-Hartman, Alexander Gordeev, linux-kernel, linux-pci Hi Bjorn and Sebastian, Am Mittwoch, den 16.07.2014, 15:10 -0600 schrieb Bjorn Helgaas: > [+cc linux-pci] > > On Wed, Jul 09, 2014 at 12:46:39PM +0200, Sebastian Ott wrote: > > GenWQE used to call pci_enable_msi_block to allocate a desired number > > of MSI's. If that was not possible pci_enable_msi_block returned with a > > smaller number which might be possible to allocate. GenWQE then called > > pci_enable_msi_block with that number. > > > > Since commit a30d0108b > > "GenWQE: Use pci_enable_msi_exact() instead of pci_enable_msi_block()" > > pci_enable_msi_exact is used which fails if the desired number of MSI's > > was not possible to allocate. Change GenWQE to use pci_enable_msi_range > > to restore the old behavior. > > > > Signed-off-by: Sebastian Ott <sebott@linux.vnet.ibm.com> > > Since this fixes a30d0108b09a, which we merged via my tree in v3.16-rc1, I > applied this with Alexander and Frank's acks to for-linus for v3.16, > thanks! > > > --- > > drivers/misc/genwqe/card_ddcb.c | 4 +--- > > drivers/misc/genwqe/card_utils.c | 10 ++++++---- > > 2 files changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/misc/genwqe/card_ddcb.c b/drivers/misc/genwqe/card_ddcb.c > > index c8046db..f66d43d 100644 > > --- a/drivers/misc/genwqe/card_ddcb.c > > +++ b/drivers/misc/genwqe/card_ddcb.c > > @@ -1237,9 +1237,7 @@ int genwqe_setup_service_layer(struct genwqe_dev *cd) > > } > > > > rc = genwqe_set_interrupt_capability(cd, GENWQE_MSI_IRQS); > > - if (rc > 0) > > - rc = genwqe_set_interrupt_capability(cd, rc); > > - if (rc != 0) { > > + if (rc) { > > rc = -ENODEV; > > goto stop_kthread; > > } > > diff --git a/drivers/misc/genwqe/card_utils.c b/drivers/misc/genwqe/card_utils.c > > index 62cc6bb..6abc437 100644 > > --- a/drivers/misc/genwqe/card_utils.c > > +++ b/drivers/misc/genwqe/card_utils.c > > @@ -718,10 +718,12 @@ int genwqe_set_interrupt_capability(struct genwqe_dev *cd, int count) > > int rc; > > struct pci_dev *pci_dev = cd->pci_dev; > > > > - rc = pci_enable_msi_exact(pci_dev, count); > > - if (rc == 0) > > - cd->flags |= GENWQE_FLAG_MSI_ENABLED; > > - return rc; > > + rc = pci_enable_msi_range(pci_dev, 1, count); > > + if (rc < 0) > > + return rc; > > + > > + cd->flags |= GENWQE_FLAG_MSI_ENABLED; > > + return 0; > > } The original code tried to register for 4 interrupts. On my system the following scenario was executed: We tried to register for 4 MSI interrupts, which was the original plan of interrupts for the card. Linux returned that it could not do that and that just 1 would work. We tried to register for 1 MSI interrupt and it worked. GENWQE_MSI_IRQS is still 4. That is how my original version looked like: rc = genwqe_set_interrupt_capability(cd, GENWQE_MSI_IRQS); if (rc > 0) rc = genwqe_set_interrupt_capability(cd, rc); if (rc != 0) { rc = -ENODEV; goto stop_kthread; } So genwqe_set_interrupt_capability() was indeed called two times. Calling it with 4 returned with rc=1 and no interrupts enabled. So calling it a 2nd time with rc=1 was leading to the proper enablement. I think the simplest way is to set GENWQE_MSI_IRQS to 1. The rest of the code at least assumes just one interrupt, because 4 never worked. Regards Frank ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] misc/GenWQE: fix pci_enable_msi usage 2014-07-17 15:48 ` Frank Haverkamp @ 2014-07-18 8:46 ` Sebastian Ott 2014-07-21 18:24 ` Bjorn Helgaas 0 siblings, 1 reply; 4+ messages in thread From: Sebastian Ott @ 2014-07-18 8:46 UTC (permalink / raw) To: Frank Haverkamp Cc: Bjorn Helgaas, Greg Kroah-Hartman, Alexander Gordeev, linux-kernel, linux-pci On Thu, 17 Jul 2014, Frank Haverkamp wrote: > Hi Bjorn and Sebastian, > > Am Mittwoch, den 16.07.2014, 15:10 -0600 schrieb Bjorn Helgaas: > > [+cc linux-pci] > > > > On Wed, Jul 09, 2014 at 12:46:39PM +0200, Sebastian Ott wrote: > > > GenWQE used to call pci_enable_msi_block to allocate a desired number > > > of MSI's. If that was not possible pci_enable_msi_block returned with a > > > smaller number which might be possible to allocate. GenWQE then called > > > pci_enable_msi_block with that number. > > > > > > Since commit a30d0108b > > > "GenWQE: Use pci_enable_msi_exact() instead of pci_enable_msi_block()" > > > pci_enable_msi_exact is used which fails if the desired number of MSI's > > > was not possible to allocate. Change GenWQE to use pci_enable_msi_range > > > to restore the old behavior. > > > > > > Signed-off-by: Sebastian Ott <sebott@linux.vnet.ibm.com> > > > > Since this fixes a30d0108b09a, which we merged via my tree in v3.16-rc1, I > > applied this with Alexander and Frank's acks to for-linus for v3.16, > > thanks! > > > > > --- > > > drivers/misc/genwqe/card_ddcb.c | 4 +--- > > > drivers/misc/genwqe/card_utils.c | 10 ++++++---- > > > 2 files changed, 7 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/misc/genwqe/card_ddcb.c b/drivers/misc/genwqe/card_ddcb.c > > > index c8046db..f66d43d 100644 > > > --- a/drivers/misc/genwqe/card_ddcb.c > > > +++ b/drivers/misc/genwqe/card_ddcb.c > > > @@ -1237,9 +1237,7 @@ int genwqe_setup_service_layer(struct genwqe_dev *cd) > > > } > > > > > > rc = genwqe_set_interrupt_capability(cd, GENWQE_MSI_IRQS); > > > - if (rc > 0) > > > - rc = genwqe_set_interrupt_capability(cd, rc); > > > - if (rc != 0) { > > > + if (rc) { > > > rc = -ENODEV; > > > goto stop_kthread; > > > } > > > diff --git a/drivers/misc/genwqe/card_utils.c b/drivers/misc/genwqe/card_utils.c > > > index 62cc6bb..6abc437 100644 > > > --- a/drivers/misc/genwqe/card_utils.c > > > +++ b/drivers/misc/genwqe/card_utils.c > > > @@ -718,10 +718,12 @@ int genwqe_set_interrupt_capability(struct genwqe_dev *cd, int count) > > > int rc; > > > struct pci_dev *pci_dev = cd->pci_dev; > > > > > > - rc = pci_enable_msi_exact(pci_dev, count); > > > - if (rc == 0) > > > - cd->flags |= GENWQE_FLAG_MSI_ENABLED; > > > - return rc; > > > + rc = pci_enable_msi_range(pci_dev, 1, count); > > > + if (rc < 0) > > > + return rc; > > > + > > > + cd->flags |= GENWQE_FLAG_MSI_ENABLED; > > > + return 0; > > > } > > The original code tried to register for 4 interrupts. On my system the > following scenario was executed: > > We tried to register for 4 MSI interrupts, which was the original plan > of interrupts for the card. Linux returned that it could not do that and > that just 1 would work. We tried to register for 1 MSI interrupt and it > worked. GENWQE_MSI_IRQS is still 4. That is how my original version > looked like: I changed the code that way, because I was under the impression GENWQE_MSI_IRQS was set to 4 for a reason (I have no knowledge of the internal workings of this driver). A lot of archs don't support more than one MSI anyway - but are you saying that the genwqe card/driver itself doesn't support more than one MSI? Regards, Sebastian > > rc = genwqe_set_interrupt_capability(cd, GENWQE_MSI_IRQS); > if (rc > 0) > rc = genwqe_set_interrupt_capability(cd, rc); > if (rc != 0) { > rc = -ENODEV; > goto stop_kthread; > } > > So genwqe_set_interrupt_capability() was indeed called two times. > Calling it with 4 returned with rc=1 and no interrupts enabled. So > calling it a 2nd time with rc=1 was leading to the proper enablement. > > I think the simplest way is to set GENWQE_MSI_IRQS to 1. The rest of the > code at least assumes just one interrupt, because 4 never worked. > > Regards > > Frank > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] misc/GenWQE: fix pci_enable_msi usage 2014-07-18 8:46 ` Sebastian Ott @ 2014-07-21 18:24 ` Bjorn Helgaas 0 siblings, 0 replies; 4+ messages in thread From: Bjorn Helgaas @ 2014-07-21 18:24 UTC (permalink / raw) To: Sebastian Ott Cc: Frank Haverkamp, Greg Kroah-Hartman, Alexander Gordeev, linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org On Fri, Jul 18, 2014 at 2:46 AM, Sebastian Ott <sebott@linux.vnet.ibm.com> wrote: > On Thu, 17 Jul 2014, Frank Haverkamp wrote: >> Hi Bjorn and Sebastian, >> >> Am Mittwoch, den 16.07.2014, 15:10 -0600 schrieb Bjorn Helgaas: >> > [+cc linux-pci] >> > >> > On Wed, Jul 09, 2014 at 12:46:39PM +0200, Sebastian Ott wrote: >> > > GenWQE used to call pci_enable_msi_block to allocate a desired number >> > > of MSI's. If that was not possible pci_enable_msi_block returned with a >> > > smaller number which might be possible to allocate. GenWQE then called >> > > pci_enable_msi_block with that number. >> > > >> > > Since commit a30d0108b >> > > "GenWQE: Use pci_enable_msi_exact() instead of pci_enable_msi_block()" >> > > pci_enable_msi_exact is used which fails if the desired number of MSI's >> > > was not possible to allocate. Change GenWQE to use pci_enable_msi_range >> > > to restore the old behavior. >> > > >> > > Signed-off-by: Sebastian Ott <sebott@linux.vnet.ibm.com> >> > >> > Since this fixes a30d0108b09a, which we merged via my tree in v3.16-rc1, I >> > applied this with Alexander and Frank's acks to for-linus for v3.16, >> > thanks! >> > >> > > --- >> > > drivers/misc/genwqe/card_ddcb.c | 4 +--- >> > > drivers/misc/genwqe/card_utils.c | 10 ++++++---- >> > > 2 files changed, 7 insertions(+), 7 deletions(-) >> > > >> > > diff --git a/drivers/misc/genwqe/card_ddcb.c b/drivers/misc/genwqe/card_ddcb.c >> > > index c8046db..f66d43d 100644 >> > > --- a/drivers/misc/genwqe/card_ddcb.c >> > > +++ b/drivers/misc/genwqe/card_ddcb.c >> > > @@ -1237,9 +1237,7 @@ int genwqe_setup_service_layer(struct genwqe_dev *cd) >> > > } >> > > >> > > rc = genwqe_set_interrupt_capability(cd, GENWQE_MSI_IRQS); >> > > - if (rc > 0) >> > > - rc = genwqe_set_interrupt_capability(cd, rc); >> > > - if (rc != 0) { >> > > + if (rc) { >> > > rc = -ENODEV; >> > > goto stop_kthread; >> > > } >> > > diff --git a/drivers/misc/genwqe/card_utils.c b/drivers/misc/genwqe/card_utils.c >> > > index 62cc6bb..6abc437 100644 >> > > --- a/drivers/misc/genwqe/card_utils.c >> > > +++ b/drivers/misc/genwqe/card_utils.c >> > > @@ -718,10 +718,12 @@ int genwqe_set_interrupt_capability(struct genwqe_dev *cd, int count) >> > > int rc; >> > > struct pci_dev *pci_dev = cd->pci_dev; >> > > >> > > - rc = pci_enable_msi_exact(pci_dev, count); >> > > - if (rc == 0) >> > > - cd->flags |= GENWQE_FLAG_MSI_ENABLED; >> > > - return rc; >> > > + rc = pci_enable_msi_range(pci_dev, 1, count); >> > > + if (rc < 0) >> > > + return rc; >> > > + >> > > + cd->flags |= GENWQE_FLAG_MSI_ENABLED; >> > > + return 0; >> > > } >> >> The original code tried to register for 4 interrupts. On my system the >> following scenario was executed: >> >> We tried to register for 4 MSI interrupts, which was the original plan >> of interrupts for the card. Linux returned that it could not do that and >> that just 1 would work. We tried to register for 1 MSI interrupt and it >> worked. GENWQE_MSI_IRQS is still 4. That is how my original version >> looked like: > > I changed the code that way, because I was under the impression > GENWQE_MSI_IRQS was set to 4 for a reason (I have no knowledge of the > internal workings of this driver). A lot of archs don't support more than > one MSI anyway - but are you saying that the genwqe card/driver itself > doesn't support more than one MSI? > > Regards, > Sebastian >> >> rc = genwqe_set_interrupt_capability(cd, GENWQE_MSI_IRQS); >> if (rc > 0) >> rc = genwqe_set_interrupt_capability(cd, rc); >> if (rc != 0) { >> rc = -ENODEV; >> goto stop_kthread; >> } >> >> So genwqe_set_interrupt_capability() was indeed called two times. >> Calling it with 4 returned with rc=1 and no interrupts enabled. So >> calling it a 2nd time with rc=1 was leading to the proper enablement. >> >> I think the simplest way is to set GENWQE_MSI_IRQS to 1. The rest of the >> code at least assumes just one interrupt, because 4 never worked. >> >> Regards >> >> Frank OK, I dropped this patch from for-linus until this gets sorted out. If you need any changes before v3.16, they'll have to be merged via some other route, because I'll be on vacation until v3.16 releases. Bjorn ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-07-21 18:24 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <alpine.LFD.2.11.1407091237030.1590@denkbrett>
2014-07-16 21:10 ` [PATCH] misc/GenWQE: fix pci_enable_msi usage Bjorn Helgaas
2014-07-17 15:48 ` Frank Haverkamp
2014-07-18 8:46 ` Sebastian Ott
2014-07-21 18:24 ` Bjorn Helgaas
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).