From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Gordeev Subject: Re: [PATCH RFC 09/15] pci: Add pci_bar_set() Date: Wed, 13 Apr 2016 20:39:35 +0200 Message-ID: <20160413183934.GD26689@agordeev.lab.eng.brq.redhat.com> References: <6f9eacb97be8b97e89c4d630ccc6795e8a6bc47d.1460190352.git.agordeev@redhat.com> <20160413163843.4ninacw5axlghab3@hawk.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, Thomas Huth To: Andrew Jones Return-path: Received: from mx1.redhat.com ([209.132.183.28]:55533 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750882AbcDMSkW (ORCPT ); Wed, 13 Apr 2016 14:40:22 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9CBFF12B2D for ; Wed, 13 Apr 2016 18:40:21 +0000 (UTC) Content-Disposition: inline In-Reply-To: <20160413163843.4ninacw5axlghab3@hawk.localdomain> Sender: kvm-owner@vger.kernel.org List-ID: On Wed, Apr 13, 2016 at 06:38:43PM +0200, Andrew Jones wrote: > On Mon, Apr 11, 2016 at 01:04:21PM +0200, Alexander Gordeev wrote: > > Cc: Thomas Huth > > Cc: Andrew Jones > > Signed-off-by: Alexander Gordeev > > --- > > lib/pci.c | 8 ++++++++ > > lib/pci.h | 1 + > > 2 files changed, 9 insertions(+) > > > > diff --git a/lib/pci.c b/lib/pci.c > > index 43e9c0c38434..46aee60e0f90 100644 > > --- a/lib/pci.c > > +++ b/lib/pci.c > > @@ -84,6 +84,14 @@ phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num) > > } > > } > > > > +void pci_bar_set(pcidevaddr_t dev, int bar_num, uint64_t addr) > > +{ > > + int off = PCI_BASE_ADDRESS_0 + bar_num * 4; > > Blank line here please. Please run the kernel's checkpatch script on all > your patches. We strive to follow the kernel coding style. (Well, don't > look at the legacy code :-) > > > + pci_config_writel(dev, off, (uint32_t)addr); > > + if (pci_bar_is64(dev, bar_num)) > > + pci_config_writel(dev, (uint32_t)(addr >> 32), off + 4); > > This pci_config_writel() call is using offset as the 3rd parameter, but > it should be the 2nd. Bummer. > Also, where do you introduce pci_config_writel()? I haven't closely > reviewed all the previous patches, but so far I only see uses of it, no > introduction of it. Does this series compile one patch at a time? We > should ensure it does to maintain bisectability. 06/15 "pci/x86: Add remaining PCI configuration space accessors" pci_config_read*/write* accessors are not expected implemented on archs that do not include lib/pci.c. Once lib/pci.c added to an arch accessors must be introduced to lib/asm/pci.h as well. Also, no generic implementation introduced as there is no such thing as generic PCI access. If it makes sense? > Thanks, > drew > > > +} > > + > > bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num) > > { > > uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4); > > diff --git a/lib/pci.h b/lib/pci.h > > index 09b500ea19f0..69d2a62f1b32 100644 > > --- a/lib/pci.h > > +++ b/lib/pci.h > > @@ -15,6 +15,7 @@ enum { > > PCIDEVADDR_INVALID = 0xffff, > > }; > > pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id); > > +void pci_bar_set(pcidevaddr_t dev, int bar_num, uint64_t addr); > > phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num); > > phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num); > > bool pci_bar_is64(pcidevaddr_t dev, int bar_num); > > -- > > 1.8.3.1 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe kvm" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html