On Tue, Nov 08, 2016 at 06:54:16PM +0100, Alexander Gordeev wrote:
[...]
> > > IMHO actually it'll be nicer with:
> > >
> > > struct pci_dev *pci_find_dev(uint16_t vendor_id, uint16_t device_id);
> >
> > Yes, I was going to suggest this too, but then...
> >
> > >
> > > 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.
> >
> > ...saw the value of avoiding the alloc. That said, we do have precedent
> > for allocating in lib/* code already (virtio_bind), and I am currently
> > reworking x86's memory management code in order to enable malloc/calloc
> > use within lib code for x86. So we could go the alloc way as well...
> >
> > >
> > > Anyway, I take the interface issue as a matter of taste.
> >
> > Not quite. I see Alex's reasoning. Perhaps users want to simply query
> > if a device is there (pci_find_dev), but not actually init a pci_dev
> > struct each time they call it. Alex's proposal keeps pci_find_dev more
> > generally useful.
Sounds reasonable.
>
> Yep. So in the end of the day I would kept pci_find_dev() and added
> pci_dev_alloc()/pci_dev_free() helpers (not sure about naming though).
> The former would have to initialize pci_dev data to be consistent
> and safely used by any other PCI API that takes pci_dev pointer.
Then I'll keep the old pci_find_dev() interface. And then when init a
PCI device, it will look like:
struct pci_dev;
pcidevaddr_t dev_addr;
dev_addr = pci_find_dev(VENDOR_ID, DEVICE_ID);
if (dev_addr == PCIDEVADDR_INVALID)
goto fail;
pci_dev_init(&pci_dev, dev_addr);
pci_enable_defaults(&pci_dev);
Introducing pci_dev_alloc()/free() is a good idea as well. Since that
can be a work upon current codes, I'll avoid doing it in this series
to make the series shorter (it's already longer than I thought it
should be :-).
Thanks,
-- peterx
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
