From mboxrd@z Thu Jan  1 00:00:00 1970
From: Peter Xu <peterx@redhat.com>
Subject: Re: [PATCH kvm-unit-tests v2 07/17] pci: introduce struct pci_dev
Date: Mon, 14 Nov 2016 15:22:35 -0500
Message-ID: <20161114202235.GC3125@pxdev.xzpeter.org>
References: <1478704224-20472-1-git-send-email-peterx@redhat.com>
 <1478704224-20472-8-git-send-email-peterx@redhat.com>
 <20161110192154.m5xt5hvxmeiz4z5t@hawk.localdomain>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Cc: kvm@vger.kernel.org, rkrcmar@redhat.com, agordeev@redhat.com,
        jan.kiszka@web.de, pbonzini@redhat.com
To: Andrew Jones <drjones@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:35518 "EHLO mx1.redhat.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S932218AbcKNUWg (ORCPT <rfc822;kvm@vger.kernel.org>);
        Mon, 14 Nov 2016 15:22:36 -0500
Content-Disposition: inline
In-Reply-To: <20161110192154.m5xt5hvxmeiz4z5t@hawk.localdomain>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Thu, Nov 10, 2016 at 08:21:54PM +0100, Andrew Jones wrote:

[...]

> >  /* Scan bus look for a specific device. Only bus 0 scanned for now. */
> >  pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)
> >  {
> >  	pcidevaddr_t dev;
> >  
> > -	for (dev = 0; dev < 256; ++dev) {
> > +	for (dev = 0; dev < PCI_DEVFN_MAX; ++dev)
> >  		if (pci_config_readw(dev, PCI_VENDOR_ID) == vendor_id &&
> >  		    pci_config_readw(dev, PCI_DEVICE_ID) == device_id)
> >  			return dev;
> > -	}
> 
> I liked the {} here because the "one" line spans three.

Sure. I'll keep them.

[...]

> > -void pci_bar_print(pcidevaddr_t dev, int bar_num)
> > +void pci_bar_print(struct pci_dev *dev, int bar_num)
> >  {
> >  	phys_addr_t size, start, end;
> >  	uint32_t bar;
> > @@ -187,6 +195,9 @@ static void pci_dev_print(pcidevaddr_t dev)
> >  	uint8_t subclass = pci_config_readb(dev, PCI_CLASS_DEVICE);
> >  	uint8_t class = pci_config_readb(dev, PCI_CLASS_DEVICE + 1);
> >  	int i;
> > +	struct pci_dev pci_dev;
> 
> Putting that above the 'int i' would appease my aesthetics OCD...

Sure.

[...]

> Besides my nits
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com> 

Thanks,

-- peterx