Date: Thu, 17 Feb 2011 09:30:03 -0500
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
To: Ian Campbell <Ian.Campbell@citrix.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Konrad Rzeszutek Wilk <konrad@kernel.org>,
        Jeremy Fitzhardinge <jeremy@goop.org>,
        "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>,
        Stefano Stabellini <Stefano.Stabellini@eu.citrix.com>
Subject: Re: [Xen-devel] [PATCH 1/3] pci/xen: Use xen_allocate_pirq_msi
Message-ID: <20110217143003.GE5987@dumpdata.com>
References: <1297894638-28007-1-git-send-email-konrad.wilk@oracle.com>
 <1297894638-28007-2-git-send-email-konrad.wilk@oracle.com>
 <1297932091.16356.1292.camel@zakaz.uk.xensource.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1297932091.16356.1292.camel@zakaz.uk.xensource.com>
User-Agent: Mutt/1.5.20 (2009-06-14)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2677
Lines: 72

On Thu, Feb 17, 2011 at 08:41:31AM +0000, Ian Campbell wrote:
> On Wed, 2011-02-16 at 22:17 +0000, Konrad Rzeszutek Wilk wrote:
> > There is no need to use the old interface.
> 
> xen_allocate_pirq -> xen_map_pirq_gsi -> PHYSDEVOP_alloc_irq_vector IFF
> xen_initial_domain() in addition to the kernel side book-keeping side of
> things (set chip and handler, update irq_info etc) whereas
> xen_allocate_pirq_msi just does the kernel book keeping.
> 
> Also xen_allocate_pirq allocates an IRQ in the 1-1 GSI space whereas
> xen_allocate_pirq_msi allocates a dynamic one in the >GSI IRQ space.

Which is OK. These are MSIs.
> 
> So this change is actually a semantic change and not just a switch to a
> new interface. I think the change is OK (because the caller is domU

Right.

> only?) but a comment explaining this would be appreciated.

Correct: "domU side".

Will fix it up.
> 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  arch/x86/pci/xen.c |    6 +++---
> >  1 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
> > index 25cd4a0..6432f75 100644
> > --- a/arch/x86/pci/xen.c
> > +++ b/arch/x86/pci/xen.c
> > @@ -157,14 +157,14 @@ static int xen_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
> >  		goto error;
> >  	i = 0;
> >  	list_for_each_entry(msidesc, &dev->msi_list, list) {
> > -		irq = xen_allocate_pirq(v[i], 0, /* not sharable */
> > +		xen_allocate_pirq_msi(
> >  			(type == PCI_CAP_ID_MSIX) ?
> > -			"pcifront-msi-x" : "pcifront-msi");
> > +			"pcifront-msi-x" : "pcifront-msi",
> > +			&irq, &v[i], XEN_ALLOC_IRQ);
> 
> All callers have this (type == MSIX) ? "msi-x" : "msi" construct,
> differing only in their use of a "pcifront-" prefix. Something which
> could be hoisted into the function perhaps?
> 
> Another aside: I think there are other cleanups which could be made to
> the various pirq allocation/mapping interfaces (of which there seem to
> be too many, or at least their individual unique qualities are
> indistinguishable by their names), I made a start on this at one point
> since I wanted to get rid of the pirq_to_irq[nr_irqs] array, for obvious
> reasons, but didn't finish it off. I'll hopefully revisit this soon.
> 
> Ian.
> 
> >  		if (irq < 0) {
> >  			ret = -1;
> >  			goto free;
> >  		}
> > -
> >  		ret = set_irq_msi(irq, msidesc);
> >  		if (ret)
> >  			goto error_while;
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/