Re: [PATCH RFC 09/15] pci: Add pci_bar_set()

[Alexander Gordeev <agordeev@redhat.com>]

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 <thuth@redhat.com>
> > Cc: Andrew Jones <drjones@redhat.com>
> > Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
> > ---
> >  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