On 6/22/21 8:06 AM, Keith Busch wrote:

On Mon, Jun 21, 2021 at 05:27:09PM -0700, Casey Chen wrote:

quoted

+	/*
+	 * Free IRQ resources as soon as NVMEQ_ENABLED bit transitions
+	 * from set to unset. If there is a window to it is truely freed,
+	 * pci_free_irq_vectors() jumping into this window will crash.
+	 * And take lock to avoid racing with pci_free_irq_vectors() in
+	 * nvme_dev_disable() path.
+	 */
+	mutex_lock(&dev->shutdown_lock)

Sorry, I wasn't clear in previous review. All of the shutdown_locks
taken after initialization need to by mutex_trylock()'s. If you have to
wait to get the lock, the device is not going to be suitable for
continuing initialization after the lock is available.

And looking at this again, if trylock is successful, I think you need to
verify controller state is suitable for continuing the initialization.

I assume you mean all the mutex_lock() except the one in 
nvme_dev_disable(), right ? I agree with replacing all mutex_lock() with 
mutex_trylock() except the one in nvme_dev_disable(). But I doubt about 
checking controller state after lock being acquired. Two reasons:

1. What state can use to check ? Suppose there is a state set on 
surprise removal, what if the state check happen before it is set while 
surprise removal is on its way ?

2. Even we don't check controller state after acquiring lock, the 
initialization still fail fairly soon if surprise removal happens. We 
don't sacrifice any performance.

Casey


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme