* [PATCH] misc/GenWQE: fix pci_enable_msi usage @ 2014-07-09 10:46 Sebastian Ott 2014-07-09 11:34 ` Alexander Gordeev 2014-07-16 21:10 ` Bjorn Helgaas 0 siblings, 2 replies; 8+ messages in thread From: Sebastian Ott @ 2014-07-09 10:46 UTC (permalink / raw) To: Frank Haverkamp, Bjorn Helgaas Cc: Greg Kroah-Hartman, Alexander Gordeev, linux-kernel 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> --- 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 related [flat|nested] 8+ messages in thread
* Re: [PATCH] misc/GenWQE: fix pci_enable_msi usage 2014-07-09 10:46 [PATCH] misc/GenWQE: fix pci_enable_msi usage Sebastian Ott @ 2014-07-09 11:34 ` Alexander Gordeev 2014-07-09 11:44 ` Sebastian Ott 2014-07-16 21:10 ` Bjorn Helgaas 1 sibling, 1 reply; 8+ messages in thread From: Alexander Gordeev @ 2014-07-09 11:34 UTC (permalink / raw) To: Sebastian Ott Cc: Frank Haverkamp, Bjorn Helgaas, Greg Kroah-Hartman, linux-kernel 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> > --- > 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 > Reviewed-by: Alexander Gordeev <agordeev@redhat.com> I am curious though why would you need convert error code to -ENODEV here (and throughout the code in general)? Thanks! -- Regards, Alexander Gordeev agordeev@redhat.com ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] misc/GenWQE: fix pci_enable_msi usage 2014-07-09 11:34 ` Alexander Gordeev @ 2014-07-09 11:44 ` Sebastian Ott 2014-07-16 13:38 ` Frank Haverkamp 0 siblings, 1 reply; 8+ messages in thread From: Sebastian Ott @ 2014-07-09 11:44 UTC (permalink / raw) To: Alexander Gordeev Cc: Frank Haverkamp, Bjorn Helgaas, Greg Kroah-Hartman, linux-kernel On Wed, 9 Jul 2014, Alexander Gordeev wrote: > 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> > > --- > > 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 > > > > Reviewed-by: Alexander Gordeev <agordeev@redhat.com> > > I am curious though why would you need convert error code to -ENODEV here > (and throughout the code in general)? Yes, I thought about that too but decided to just fix the bug and not change this (since it's more or less unrelated). Regards, Sebastian ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] misc/GenWQE: fix pci_enable_msi usage 2014-07-09 11:44 ` Sebastian Ott @ 2014-07-16 13:38 ` Frank Haverkamp 0 siblings, 0 replies; 8+ messages in thread From: Frank Haverkamp @ 2014-07-16 13:38 UTC (permalink / raw) To: Sebastian Ott Cc: Alexander Gordeev, Bjorn Helgaas, Greg Kroah-Hartman, linux-kernel Hi Sebastian, Am Mittwoch, den 09.07.2014, 13:44 +0200 schrieb Sebastian Ott: > On Wed, 9 Jul 2014, Alexander Gordeev wrote: > > > 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> > > > --- > > > 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 > > > > > > > Reviewed-by: Alexander Gordeev <agordeev@redhat.com> > > thanks for the fix and Alexander for reviewing it. > > I am curious though why would you need convert error code to -ENODEV here > > (and throughout the code in general)? > > Yes, I thought about that too but decided to just fix the bug and not > change this (since it's more or less unrelated). > > Regards, > Sebastian You are probably right, converting the return code into -ENODEV is not really needed. Reviewed-by: Frank Haverkamp <haver@linux.vnet.ibm.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] misc/GenWQE: fix pci_enable_msi usage 2014-07-09 10:46 [PATCH] misc/GenWQE: fix pci_enable_msi usage Sebastian Ott 2014-07-09 11:34 ` Alexander Gordeev @ 2014-07-16 21:10 ` Bjorn Helgaas 2014-07-17 15:48 ` Frank Haverkamp 1 sibling, 1 reply; 8+ 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] 8+ messages in thread
* Re: [PATCH] misc/GenWQE: fix pci_enable_msi usage 2014-07-16 21:10 ` Bjorn Helgaas @ 2014-07-17 15:48 ` Frank Haverkamp 2014-07-18 8:46 ` Sebastian Ott 0 siblings, 1 reply; 8+ 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] 8+ 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; 8+ 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] 8+ 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; 8+ 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] 8+ messages in thread
end of thread, other threads:[~2014-07-21 18:24 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-07-09 10:46 [PATCH] misc/GenWQE: fix pci_enable_msi usage Sebastian Ott 2014-07-09 11:34 ` Alexander Gordeev 2014-07-09 11:44 ` Sebastian Ott 2014-07-16 13:38 ` Frank Haverkamp 2014-07-16 21:10 ` 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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.