On 24.11.2023 11:49, Shinichiro Kawasaki wrote:
> On Nov 14, 2023 / 19:58, Andy Shevchenko wrote:
>> On Tue, Nov 14, 2023 at 06:11:40PM +0200, Andy Shevchenko wrote:
>>> On Tue, Nov 14, 2023 at 04:57:01PM +0100, Lukas Wunner wrote:
>>>> On Tue, Nov 14, 2023 at 02:04:34PM +0200, Andy Shevchenko wrote:
>>>>> On Tue, Nov 14, 2023 at 11:47:15AM +0100, Heiner Kallweit wrote:
>>>>>> On 14.11.2023 11:16, Wolfram Sang wrote:
>>>>>>> On Tue, Nov 14, 2023 at 06:54:29AM +0000, Shinichiro Kawasaki wrote:
>>
>> ...
>>
>>>>>>>> The lockdep splat indicates possible deadlock between
>>>>>>>> pci_rescan_remove_lock and work_completion lock have deadlock
>>>>>>>> possibility.
>>>>>>>> In the call stack, I found that the workqueue thread for
>>>>>>>> i801_probe() calls p2sb_bar(), which locks pci_rescan_remove_lock.
>>>>>>
>>>>>> i801 just uses p2sb_bar(), I don't see any issue in i801. Root cause
>>>>>> seems to be in the PCI subsystem. Calling p2sb_bar() from a PCI driver
>>>>>> probe callback seems to be problematic, nevertheless it's a valid API
>>>>>> usage.
>>>>>
>>>>> So, currently I'm lack of (good) ideas and would like to hear other (more
>>>>> experienced) PCI developers on how is to address this...
>>>>
>>>> Can you add a p2sb_bar_locked() library call which is used by the
>>>> i801 probe path?
>>>>
>>>> Basically rename p2sb_bar() to __p2sb_bar() and add a third parameter
>>>> of type boolean which signifies whether it's invoked in locked context
>>>> or not, then call that from p2sb_bar() and p2sb_bar_locked() wrappers.
>>>
>>> It may work, I assume. Let me cook the patch.
>>
>> Hmm... But this will open a window when probing phase happens, e.g. during
>> boot time, no? If somebody somehow calls for full rescan, we may end up in
>> the situation when P2SB is gone before accessing it in p2sb_bar().
>>
>> Now I'm wondering why simple pci_dev_get() can't be sufficient in the
>> p2sb_bar().
>
> All, thanks for the discussion. It looks rather difficult to avoid the WARN.
>
> To confirm that the deadlock is for real, I tried to remove i2c-i801 device and
> did /sys/bus/pci/rescan with two commands below:
>
> # echo 1 > /sys/bus/pci/devices/0000\:00\:1f.4/remove
> # echo 1 > /sys/bus/pci/rescan
>
> Then I observed the second command hangs.
>
> I came across another fix idea: assuming the guard by pci_rescan_remove_lock is
> required in p2sb_bar(), how about to do trylock? If the mutex can not be locked,
> make the p2sb_bar() call fail. This way, we can avoid the deadlock between
> pci_rescan_remove_lock and workqueue completion.
>
> I created a patch below and confirmed it avoided the lockdep WARN. The i2c-i801
> probe was ok at system boot. When I did the two commands above, I observed the
> i2c-i801 device probe failed due to trylock failure. But I think it's far better
> than hang.
>
I wouldn't call this a solution. A solution has to support pci drivers using
p2sb_bar() in probe(). You can't simply make them fail.
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index ed6b7f48736..3e784fb6cd9 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -3312,6 +3312,18 @@ void pci_lock_rescan_remove(void)
> }
> EXPORT_SYMBOL_GPL(pci_lock_rescan_remove);
>
> +/*
> + * Try to acquire pci_rescan_remove_lock. Returns 1 if the mutex
> + * has been acquired successfully, and 0 on contention. Use this
> + * to acquire the lock in workqueue context to avoid potential deadlock
> + * together with work_completion.
> + */
> +int pci_trylock_rescan_remove(void)
> +{
> + return mutex_trylock(&pci_rescan_remove_lock);
> +}
> +EXPORT_SYMBOL_GPL(pci_trylock_rescan_remove);
> +
> void pci_unlock_rescan_remove(void)
> {
> mutex_unlock(&pci_rescan_remove_lock);
> diff --git a/drivers/platform/x86/p2sb.c b/drivers/platform/x86/p2sb.c
> index 1cf2471d54d..7a6bee8abf9 100644
> --- a/drivers/platform/x86/p2sb.c
> +++ b/drivers/platform/x86/p2sb.c
> @@ -113,7 +113,10 @@ int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem)
> * Prevent concurrent PCI bus scan from seeing the P2SB device and
> * removing via sysfs while it is temporarily exposed.
> */
> - pci_lock_rescan_remove();
> + if (!pci_trylock_rescan_remove()) {
> + pr_err("P2SB device accessed during PCI rescan");
> + return -EBUSY;
> + }
>
> /* Unhide the P2SB device, if needed */
> pci_bus_read_config_dword(bus, devfn_p2sb, P2SBC, &value);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 60ca768bc86..e6db5096217 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1439,6 +1439,7 @@ void set_pcie_hotplug_bridge(struct pci_dev *pdev);
> unsigned int pci_rescan_bus_bridge_resize(struct pci_dev *bridge);
> unsigned int pci_rescan_bus(struct pci_bus *bus);
> void pci_lock_rescan_remove(void);
> +int pci_trylock_rescan_remove(void);
> void pci_unlock_rescan_remove(void);
>
> /* Vital Product Data routines */
