linux-kernel - Re: [PATCH] misc/GenWQE: fix pci_enable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAErSpo57gyyiEAdsqxTwu5EWggxohxXv0PpEtYJfUFJAfp2GHg@mail.gmail.com>
Date:	Mon, 21 Jul 2014 12:24:19 -0600
From:	Bjorn Helgaas <bhelgaas@...gle.com>
To:	Sebastian Ott <sebott@...ux.vnet.ibm.com>
Cc:	Frank Haverkamp <haver@...ux.vnet.ibm.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Alexander Gordeev <agordeev@...hat.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>
Subject: Re: [PATCH] misc/GenWQE: fix pci_enable_msi usage

On Fri, Jul 18, 2014 at 2:46 AM, Sebastian Ott
<sebott@...ux.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@...ux.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/