Re: [RFC] PCI device list locking - take 2

[Greg KH]

On Wed, Jun 18, 2003 at 03:33:24PM -0700, Chris Wright wrote:
> * Greg KH ([e-mail]) wrote:
> >  static void *pci_seq_start(struct seq_file *m, loff_t *pos)
> >  {
> > -	struct list_head *p = &pci_devices;
> > +	struct pci_dev *dev = NULL;
> >  	loff_t n = *pos;
> >  
> > -	/* XXX: surely we need some locking for traversing the list? */
> > +	dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, dev);
> >  	while (n--) {
> > -		p = p->next;
> > -		if (p == &pci_devices)
> > -			return NULL;
> > +		dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, dev);
> > +		if (dev == NULL)
> > +			goto exit;
> 
> I think this still has the same problem.  pci_get_device grabs lock,
> walks list, gets ref, and drops lock.  But the ref doesn't hold it on the
> list, right?.  So some pci_remove_* could do list_del(&dev->global_list),
> poison the prev/next pointers.  Subsequent pci_get_device would do ->next
> and oops.  It seems the lock needs to be held for entire start/next/stop
> sequence, or the ref needs to keep it on list.

Hm, I think we should probably just check in pci_get_device() to verify
that ->next is really valid.  If it isn't just return NULL, as we have
no idea what the next device would possibly be.  The worse thing that
would happen is the proc file would be a bit shorter than expected.  If
read again, it would be correct, with the previously referenced device
now gone.

I don't want to try to hold a lock over start/next/stop as that would
just be asking for trouble :)

Sound acceptable?

> > +struct pci_dev * 
> > +pci_get_subsys(unsigned int vendor, unsigned int device,
> <snip>
> > +exit:
> > +	if (from)
> > +		pci_put_dev(from);
> > +	if (dev)
> > +		pci_get_dev(dev);
> 
> Heh, the hch in me notes that pci_{put,get}_dev already check NULL device ;-)

Heh, good point, I'll go fix that :)

thanks for looking at this,

greg k-h