Create wrapper for Giant taken for newbus

[imp]

Summary

Create an API to take and drop the topo lock needed to manage
the newbus tree (bus_topo_lock() and bus_topo_unlock()). Add
bus_topo_mtx() to return this mutext (current Giant but soon to
be an SX lock I think).

Sponsored by: Netflix

Test Plan

This needs a man page.
Asserts will be a separate commit
This may have been posted as a review before, but I can't find it. I know hps hated the name newbus_lock()/unlock() so I've changed it.

[imp]

Use bus_topo_lock()/unlock() instead of giant in devctl2

[• hselasky]

Patch looks good to me. The only comment I have is the function name?

Why not just call it:

bus_top_lock();

The four letters "topo" doesn't sound that good to me. Not that it matters.

--HPS

[imp]

In D31831#718027, @hselasky wrote:

Patch looks good to me. The only comment I have is the function name?

Why not just call it:

bus_top_lock();

The four letters "topo" doesn't sound that good to me. Not that it matters.

"topo" is the typical abbreviation for topology. Geom uses g_topology_lock() etc, but I thought that too long. I'd rather do topology than top since top means something a little different.

[mav]

I am mostly unfamiliar with miibus interface, but I see ~80 calls to mii_attach() in a tree, many of which are going from driver attach methods and are already under the lock. Are you expecting to recurse on the lock?

[imp]

In D31831#718073, @mav wrote:

I am mostly unfamiliar with miibus interface, but I see ~80 calls to mii_attach() in a tree, many of which are going from driver attach methods and are already under the lock. Are you expecting to recurse on the lock?

Yea, that's one of the reasons why I need to separate out that change.... I convinced myself it was OK when I originally did it last year, but I need to study the code again I think to articulate the reason. With giant it doesn't matter, but it may in the future...

[imp]

remove miibus pushdown change that arrived I think via a rebase mistake when
updating earlier in the summer.

[imp]

Add some notes for the reviewers that maybe don't belong in comments

sys/dev/mfi/mfi.c

1930

This could move up (we don't need to hold giant past the end of add_child(), but I'm just retaining the same code structure as before.

2018

This could move up (we don't need to hold giant past the end of add_child(), but I'm just retaining the same code structure as before.

sys/dev/mlx5/mlx5_core/mlx5_health.c

378

I'm unsure what newbus interaction is here that this protects... @jhb ?

sys/dev/pci/pci_user.c

1063

I'm not sure what this is actually protecting here... Or was protecting in the past... We don't change topologies here, so shouldn't need it for everything, though I may have overlooked something in an individual ioctl call...

sys/dev/usb/net/if_ure.c

1026

I narrowed things here because only mii_attach needs this topo lock. We don't need it to add media types to if_media.
I'm also sure this path will be executed during the rollout of these changes, so I won't leave a hidden bomb (though there's some risk I'll have to fix something after we commit)

[• hselasky]

In D31831#718033, @imp wrote:

In D31831#718027, @hselasky wrote:

The four letters "topo" doesn't sound that good to me. Not that it matters.

"topo" is the typical abbreviation for topology. Geom uses g_topology_lock() etc, but I thought that too long. I'd rather do topology than top since top means something a little different.

I suggest you spell it out then, bus_topology_lock() is better than bus_topo_lock(), because "topo" means "top" in Portuguese, and in Spanish it means something completely different, and you already pointed out that "top" is not what you want to describe?

--HPS

[• hselasky]

Hi Warner,

We don't want to have another "minion" discussion, look at this:

In Italian "topo" means "mouse".
In Spanish "topo" means "mole".
In Portugese "topo" means "top".
In English "topo" means "a topographic map".

--HPS

[mav]

Hans, I think you are too picky here. We already have xpt_topo_lock in CAM, ng_topo_lock in NetGraph, and 4 more line that in random drivers, so I think this abbreviation is quite typical.

[imp]

In D31831#718182, @hselasky wrote:

Hi Warner,

We don't want to have another "minion" discussion, look at this:

In Italian "topo" means "mouse".
In Spanish "topo" means "mole".
In Portugese "topo" means "top".
In English "topo" means "a topographic map".

I think this may be different. We have the following:

smp already has topo_nodes to map out the system topography, with lots of machdep code that uses the _topo_ names in various places.
CAM already has a xpt_topo_lock to control changes to the device lists
We have clock, regulator, syscon, phy and other embedded code that uses a topology lock is named *_topo with similar _lock, _slock, _unlock, etc
The mpr and mps driver's code to map out the nodes on the bus uses 'topo_change' and other variations on 'topo'
The fiberchanel code (osc_fc) has 'read_topo' used with the SLI4_MBOX_COMMAND_READ_TOPOLOGY command

So unlike minion, which didn't have an established use, this just follows the patter that exists across a range of kernel subsystems.
On the other side, geom, netgraph and bits of VMM and VM spell out topology, the former two for their topo locks, the latter two
only for info routines.

So there's already a preference for topo_ in this circumstance, imho, but I'm keeping an open mind for the moment.

[imp]

Make a lock/unlock of Giant in cardbus bus_topo_lock()/unlock()

[• hselasky]

In D31831#718192, @mav wrote:

Hans, I think you are too picky here. We already have xpt_topo_lock in CAM, ng_topo_lock in NetGraph, and 4 more line that in random drivers, so I think this abbreviation is quite typical.

OK, maybe I need to extend my vocabulary :-)

[mav]

You could also add the explicit topology locking into sysctl_hdac_pindump() and hdaa_sysctl_reconfig(), and remove CTLFLAG_NEEDGIANT from all 3 places in hda(4) driver. Or I can do it separately after, if you prefer.

[imp]

In D31831#718392, @mav wrote:

You could also add the explicit topology locking into sysctl_hdac_pindump() and hdaa_sysctl_reconfig(), and remove CTLFLAG_NEEDGIANT from all 3 places in hda(4) driver. Or I can do it separately after, if you prefer.

I'll do that as a followup commit.

[jhb]

Not sure if it is "new-bus" or "newbus", should just be consistent in the comments for DEVICE_SUSPEND/RESUME and the new comment in bus.h.

An interesting question is if you want to go ahead and annotate shared locks vs write locks? For example, the pci_user case is likely a read lock of the topology. Possibly not really worth worrying about though.

sys/dev/acpica/acpi.c

3099

sys/dev/mlx5/mlx5_core/mlx5_health.c

378

I have no idea and mlx5 is more of a @hselasky / @kib question. I suspect this is the other side of a BUS_RESET_CHILD and perhaps it is added there because of that (or maybe the code used to invoke a new-bus method here but no longer does?)

sys/dev/pci/pci_user.c

1063

I suspect this is to "hold" a PCI device_t. That is, I suspect this locks the list searched by pci_find_dbsf and then serves as a hold on the returned PCI device until the ioctl handler is finished with the dinfo

sys/dev/xen/control/control.c

235

sys/sys/bus.h

739–741

776

Unrelated change?

[imp]

sys/dev/mlx5/mlx5_core/mlx5_health.c

378

I see no topology changing calls here to warrant needing to take it out.
I suspect it was an abundance of caution that's not completely warranted and could just be removed.

sys/dev/pci/pci_user.c

1063

I actually think it's an attempt to keep pci_devq stable since it's a parallel structure to newbus linked together with dinfo. I'll remove the comment, but the usage and/or structure is ripe for revisiting early in these efforts.

sys/sys/bus.h

776

Likely... Unsure if it makes sense to split that out since it's so trivial.

[kib]

sys/dev/mlx5/mlx5_core/mlx5_health.c

378

It is something that might end up in mlx5_load_one(), and perhaps we do not want an another instance of detach/attach being initiated in parallel with us working on attach (effectively).

This is not too effective right now, because Giant cannot protect sleeping places, and should be revisited when locking rules are defined. But most likely, bus_topo_lock() here is to stay.

[jhb]

sys/dev/pci/pci_user.c

1063

We are in agreement (pci_devq would be the list pci_find_dbsf traverses). It might be that this ends up becoming some private lock to the PCI bus layer, but it may be simplest to end up reusing the bus_topo_lock for that anyway. I think the bigger fish to fry in the PCI bus layer is how we protect all the state in dinfo objects for things like MSI/MSI-X, etc.