[v6,0/2] platform/x86: p2sb: Fix deadlock at sysfs PCI bus rescan

Message

Shinichiro Kawasaki Jan. 8, 2024, 6:20 a.m. UTC

When PCI devices call p2sb_bar() during probe for sysfs PCI bus rescan, deadlock
happens due to double lock of pci_rescan_remove_lock [1]. The first patch in
this series addresses the deadlock. The second patch is a code improvement which
was pointed out during review for the first patch.

[1] https://lore.kernel.org/linux-pci/6xb24fjmptxxn5js2fjrrddjae6twex5bjaftwqsuawuqqqydx@7cl3uik5ef6j/

The first patch of the v5 series was upstreamed to the kernel v6.7-rc8. However,
it caused IDE controller detection failure on an old platform [2] then the patch
was reverted at v6.7. The failure happened because the IDE controller had same
DEVFN as P2SB device. To avoid this failure, I added device class check per
suggestion by Lukas. If the device at P2SB DEVFN does not have the device class
expected for P2SB, avoid touching the device.

[2] https://lore.kernel.org/platform-driver-x86/[e-mail]/

I confirmed the patches fix the problem [1] on the kernel v6.7, using a system
with i2c_i801 device, building i2c_i801 module as both built-in and loadable.
Reviews and comments will be appreciated.

Klara,

I hesitated to add your Tested-by tag to the v6 patch, since I modified the code
slightly from the code you tested (I used pci_bus_read_config_word() instead of
pci_bus_read_config_dword() to avoid a shift operator). I hope you have time to
afford to test this series again.


Changes from v5:
* Added device class check to avoid old IDE controller detection failure

Changes from v4:
* Separated a hunk for pci_resource_n() as the second patch
* Reflected other review comments by Ilpo

Changes from v3:
* Modified p2sb_valid_resource() to return boolean

Changes from v2:
* Improved p2sb_scan_and_cache() and p2sb_scan_and_cache_devfn()
* Reflected other review comments by Andy

Changes from v1:
* Reflected review comments by Andy
* Removed RFC prefix

Changes from RFC v2:
* Reflected review comments on the list

Changes from RFC v1:
* Fixed a build warning poitned out in llvm list by kernel test robot

Shin'ichiro Kawasaki (2):
  platform/x86: p2sb: Allow p2sb_bar() calls during PCI device probe
  platform/x86: p2sb: Use pci_resource_n() in p2sb_read_bar0()

 drivers/platform/x86/p2sb.c | 183 +++++++++++++++++++++++++++---------
 1 file changed, 141 insertions(+), 42 deletions(-)

Comments

Klara Modin Jan. 8, 2024, 10:37 a.m. UTC | #1

Den mån 8 jan. 2024 kl 07:21 skrev Shin'ichiro Kawasaki
<[e-mail]>:
> Klara,
>
> I hesitated to add your Tested-by tag to the v6 patch, since I modified the code
> slightly from the code you tested (I used pci_bus_read_config_word() instead of
> pci_bus_read_config_dword() to avoid a shift operator). I hope you have time to
> afford to test this series again.

I can confirm that the patchset is still working correctly on my
machine with this change.

Thanks,
Tested-by Klara Modin <[e-mail]>

Hans de Goede Jan. 8, 2024, 3:22 p.m. UTC | #2

Hi,

On 1/8/24 07:20, Shin'ichiro Kawasaki wrote:
> When PCI devices call p2sb_bar() during probe for sysfs PCI bus rescan, deadlock
> happens due to double lock of pci_rescan_remove_lock [1]. The first patch in
> this series addresses the deadlock. The second patch is a code improvement which
> was pointed out during review for the first patch.
> 
> [1] https://lore.kernel.org/linux-pci/6xb24fjmptxxn5js2fjrrddjae6twex5bjaftwqsuawuqqqydx@7cl3uik5ef6j/
> 
> The first patch of the v5 series was upstreamed to the kernel v6.7-rc8. However,
> it caused IDE controller detection failure on an old platform [2] then the patch
> was reverted at v6.7. The failure happened because the IDE controller had same
> DEVFN as P2SB device. To avoid this failure, I added device class check per
> suggestion by Lukas. If the device at P2SB DEVFN does not have the device class
> expected for P2SB, avoid touching the device.
> 
> [2] https://lore.kernel.org/platform-driver-x86/[e-mail]/
> 
> I confirmed the patches fix the problem [1] on the kernel v6.7, using a system
> with i2c_i801 device, building i2c_i801 module as both built-in and loadable.
> Reviews and comments will be appreciated.
> 
> Klara,
> 
> I hesitated to add your Tested-by tag to the v6 patch, since I modified the code
> slightly from the code you tested (I used pci_bus_read_config_word() instead of
> pci_bus_read_config_dword() to avoid a shift operator). I hope you have time to
> afford to test this series again.

Thank you for your patch-series, I've applied this series to
my review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

I will include this series in my next fixes pull-req to Linus
for the current kernel development cycle.

Note I have re-added Andy's and Ilpo's Reviewed-by for patch 1/2 since
there is only one small change there.

Regards,

Hans






> 
> 
> Changes from v5:
> * Added device class check to avoid old IDE controller detection failure
> 
> Changes from v4:
> * Separated a hunk for pci_resource_n() as the second patch
> * Reflected other review comments by Ilpo
> 
> Changes from v3:
> * Modified p2sb_valid_resource() to return boolean
> 
> Changes from v2:
> * Improved p2sb_scan_and_cache() and p2sb_scan_and_cache_devfn()
> * Reflected other review comments by Andy
> 
> Changes from v1:
> * Reflected review comments by Andy
> * Removed RFC prefix
> 
> Changes from RFC v2:
> * Reflected review comments on the list
> 
> Changes from RFC v1:
> * Fixed a build warning poitned out in llvm list by kernel test robot
> 
> Shin'ichiro Kawasaki (2):
>   platform/x86: p2sb: Allow p2sb_bar() calls during PCI device probe
>   platform/x86: p2sb: Use pci_resource_n() in p2sb_read_bar0()
> 
>  drivers/platform/x86/p2sb.c | 183 +++++++++++++++++++++++++++---------
>  1 file changed, 141 insertions(+), 42 deletions(-)
>