From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Xu Subject: Re: [PATCH kvm-unit-tests 06/17] pci: introduce struct pci_dev Date: Tue, 8 Nov 2016 10:48:32 -0500 Message-ID: <20161108154832.GF2793@pxdev.xzpeter.org> References: <1477468040-21034-1-git-send-email-peterx@redhat.com> <1477468040-21034-7-git-send-email-peterx@redhat.com> <20161104164101.ymyxzkjbbpsr4mbr@kamzik.brq.redhat.com> <20161108122730.GH10472@agordeev.lab.eng.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Cc: Andrew Jones , kvm@vger.kernel.org, rkrcmar@redhat.com, jan.kiszka@web.de, pbonzini@redhat.com To: Alexander Gordeev Return-path: Received: from mx1.redhat.com ([209.132.183.28]:55558 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751243AbcKHPsf (ORCPT ); Tue, 8 Nov 2016 10:48:35 -0500 Content-Disposition: inline In-Reply-To: <20161108122730.GH10472@agordeev.lab.eng.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Tue, Nov 08, 2016 at 01:27:31PM +0100, Alexander Gordeev wrote: > On Fri, Nov 04, 2016 at 05:41:01PM +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) > > > +int pci_find_dev(struct pci_dev *pci_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) { > > > > Why introduce this PCI_DEVFN_MAX define? > > > > > if (pci_config_readw(dev, PCI_VENDOR_ID) == vendor_id && > > > - pci_config_readw(dev, PCI_DEVICE_ID) == device_id) > > > - return dev; > > > + pci_config_readw(dev, PCI_DEVICE_ID) == device_id) { > > > + pci_dev_init(pci_dev, dev); > > > + return 0; > > > + } > > > } > > > > > > - return PCIDEVADDR_INVALID; > > > + return -1; > > > > Why not use bool for the ret type; true=good, false=bad? > > Both ways look strange to me. I would leave pci_find_dev() as is (low- > level) and move pci_dev_init() outside - so a usage would be i.e.: > > dev = pci_find_dev(PCI_VENDOR_ID_REDHAT, PCI_DEVICE_ID_REDHAT_TEST); > if (dev == PCIDEVADDR_INVALID) > ... > pci_dev_init(&pci_dev, dev); IMHO actually it'll be nicer with: struct pci_dev *pci_find_dev(uint16_t vendor_id, uint16_t device_id); But this needs dynamic allocation of memory, which might be an overkill for kvm-unit-test. So I chose to allocate the pci_dev on caller stack. Anyway, I take the interface issue as a matter of taste. How about I take Drew's suggestion here with true/false? Then if we have any better idea with solid reasons, we can post seperate patch for it. Thanks, -- peterx