Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1742123

Re: [PATCH 3/4] pci aer: fix deadlock in do_recovery

Show all headers | View raw

On 9/28/2017 7:46 PM, Govindarajulu Varadarajan wrote:
>> How about releasing the device_lock here on CPU0?>
> 
> pci_device_add() is called by driver's pci probe function. device_lock(dev)
> should be held before calling pci driver probe function.

I see. The goal of the lock held here is to protect probe() operation from
being disrupted. I also don't think we can change this. 

> 
>> or in other words keep device_lock as short as possible?
> 
> The problem is not the duration device_lock is held. It is the order two locks
> are aquired. We cannot control or implement a restriction that during
> device_lock() is held, driver probe should not call pci function which aquires
> pci_bus_sem. And in case of pci aer, aer handler needs to call driver err_handler()
> for which we need to hold device_lock() before calling err_handler(). In order
> to find all the devices on a pci bus, we should hold pci_bus_sem to do
> pci_walk_bus().

I was reacting to this to see if there is a better way to do this.

"Only fix I could think of is to lock &pci_bus_sem and try locking all
device->mutex under that pci_bus. If it fails, unlock all device->mutex
and &pci_bus_sem and try again."

How about gracefully returning from report_error_detected() when we cannot obtain
the device_lock() by replacing it with device_trylock()? 

aer_pci_walk_bus() can still poll like you did until it gets the lock. At least,
we don't get to introduce a new API, new lock semantics and code refactoring.
__pci_bus_trylock() looked very powerful and also dangerously flexible to
introduce new bugs to me.

For instance, you called it like this.

+		down_read(&pci_bus_sem);
+		locked = __pci_bus_trylock(bus, pci_device_trylock,
+					   pci_device_unlock);

pci_bus_trylock() would obtain device + cfg locks whereas pci_device_trylock() only
obtains the device lock. Can it race against cfg lock? It depends on the caller.
Very subtle difference.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 0/4] pci aer: fix deadlock in do_recovery Govindarajulu Varadarajan <gvaradar@cisco.com> - 2017-09-28 00:00 +0200
  [PATCH 1/4] pci: introduce __pci_walk_bus for caller with pci_bus_sem held Govindarajulu Varadarajan <gvaradar@cisco.com> - 2017-09-28 00:00 +0200
    Re: [PATCH 1/4] pci: introduce __pci_walk_bus for caller with  pci_bus_sem held Sinan Kaya <okaya@codeaurora.org> - 2017-09-28 18:20 +0200
      Re: [PATCH 1/4] pci: introduce __pci_walk_bus for caller with  pci_bus_sem held Govindarajulu Varadarajan <gvaradar@cisco.com> - 2017-09-29 02:00 +0200
  [PATCH 3/4] pci aer: fix deadlock in do_recovery Govindarajulu Varadarajan <gvaradar@cisco.com> - 2017-09-28 00:00 +0200
    Re: [PATCH 3/4] pci aer: fix deadlock in do_recovery Sinan Kaya <okaya@codeaurora.org> - 2017-09-28 18:50 +0200
      Re: [PATCH 3/4] pci aer: fix deadlock in do_recovery Govindarajulu Varadarajan <gvaradar@cisco.com> - 2017-09-29 01:50 +0200
        Re: [PATCH 3/4] pci aer: fix deadlock in do_recovery Sinan Kaya <okaya@codeaurora.org> - 2017-09-29 15:40 +0200
          Re: [PATCH 3/4] pci aer: fix deadlock in do_recovery Govindarajulu Varadarajan <gvaradar@cisco.com> - 2017-09-30 08:10 +0200
  [PATCH 4/4] lockdep: make MAX_LOCK_DEPTH configurable from Kconfig Govindarajulu Varadarajan <gvaradar@cisco.com> - 2017-09-28 00:00 +0200
    Re: [PATCH 4/4] lockdep: make MAX_LOCK_DEPTH configurable from  Kconfig Peter Zijlstra <peterz@infradead.org> - 2017-09-28 11:30 +0200
      Re: [PATCH 4/4] lockdep: make MAX_LOCK_DEPTH configurable from  Kconfig Govindarajulu Varadarajan <gvaradar@cisco.com> - 2017-09-29 02:00 +0200
        Re: [PATCH 4/4] lockdep: make MAX_LOCK_DEPTH configurable from  Kconfig Bjorn Helgaas <helgaas@kernel.org> - 2017-09-29 18:30 +0200
          Re: [PATCH 4/4] lockdep: make MAX_LOCK_DEPTH configurable from  Kconfig Govindarajulu Varadarajan <gvaradar@cisco.com> - 2017-09-30 08:10 +0200
  [PATCH 2/4] pci: code refactor pci_bus_lock/unlock/trylock Govindarajulu Varadarajan <gvaradar@cisco.com> - 2017-09-28 00:00 +0200

csiph-web