Re: [PATCH 9 of 20] ipath - char devices for diagnostics and lightweight subnet management

["Bryan O'Sullivan" <bos@pathscale.com>]

On Fri, 2006-03-10 at 09:08 -0800, Roland Dreier wrote:
>     Bryan> OK.  What's a safe way to iterate over the devices in the
>     Bryan> presence of hotplug, then?  I assume it's
>     Bryan> list_for_each_mumble; I just don't know what mumble is :-)
> 
> You need something that takes a reference to each device while you're
> looking at it, like for_each_pci_dev().

OK, thanks.

It seems like the thing to do to be fully safe might be to have
ipath_get() (just rename ipath_lookup) and ipath_put() functions.  Embed
a kref inside ipath_devdata, and have ipath_dev_get increment the ref
count on both the ipath_devdata and the pci_dev.

Is that sane, or am I way off base?

>   But in general iterating
> through devices is usually the wrong thing to do, because devices can
> come and go in the middle of your loop.

I understand that.  In practice, though, I think this is not a good
approach in many of the cases we're dealing with.  Every use of
ipath_max iterates over all devices for a reason.

For example, the diag open routine wants to find at least one device
that's up.  We could maintain a separate list of devices that are in the
state that it needs, so that it can just get the first entry off that
list or fail if the list is empty, but that's more complex than simply
iterating over every device and checking each one.

> (BTW, what does making ipath_max an atomic_t get
> you?  The updates are protected by a lock anyway).

Probably not much.  The motivation was to ensure that if it got
incremented during an iteration, whoever was iterating would see the
update in a timely fashion.

>   But I was talking
> about the code in ipath_verbs_init(), which is the only place you call
> ipath_verbs_register() that I could find.  You make one pass through
> the devices that are present when ipath_verbs_init() is called at
> module load time, and any devices that get added later are missed.

Yes, that code ought to be restructured.

Thanks for the helpful feedback.

	<b