| Message ID | 20231227072049.2281764-1-shinichiro.kawasaki@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] platform/x86: p2sb: Allow p2sb_bar() calls during PCI device probe | expand |
On Wed, Dec 27, 2023 at 04:20:49PM +0900, Shin'ichiro Kawasaki wrote: > p2sb_bar() unhides P2SB device to get resources from the device. It > guards the operation by locking pci_rescan_remove_lock so that parallel > rescans do not find the P2SB device. However, this lock causes deadlock > when PCI bus rescan is triggered by /sys/bus/pci/rescan. The rescan > locks pci_rescan_remove_lock and probes PCI devices. When PCI devices > call p2sb_bar() during probe, it locks pci_rescan_remove_lock again. > Hence the deadlock. > > To avoid the deadlock, do not lock pci_rescan_remove_lock in p2sb_bar(). > Instead, do the lock at fs_initcall. Introduce p2sb_cache_resources() > for fs_initcall which gets and caches the P2SB resources. At p2sb_bar(), > refer the cache and return to the caller. As per v3 discussion: Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
On Wed, 27 Dec 2023, Shin'ichiro Kawasaki wrote: > p2sb_bar() unhides P2SB device to get resources from the device. It > guards the operation by locking pci_rescan_remove_lock so that parallel > rescans do not find the P2SB device. However, this lock causes deadlock > when PCI bus rescan is triggered by /sys/bus/pci/rescan. The rescan > locks pci_rescan_remove_lock and probes PCI devices. When PCI devices > call p2sb_bar() during probe, it locks pci_rescan_remove_lock again. > Hence the deadlock. > > To avoid the deadlock, do not lock pci_rescan_remove_lock in p2sb_bar(). > Instead, do the lock at fs_initcall. Introduce p2sb_cache_resources() > for fs_initcall which gets and caches the P2SB resources. At p2sb_bar(), > refer the cache and return to the caller. > > Link: https://lore.kernel.org/linux-pci/6xb24fjmptxxn5js2fjrrddjae6twex5bjaftwqsuawuqqqydx@7cl3uik5ef6j/ > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > --- > This patch reflects discussions held at the Link tag. I confirmed this patch > fixes the problem using a system with i2c_i801 device, building i2c_i801 > module as both built-in and loadable. Reviews will be appreicated. Why no Fixes tag? > 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 > > drivers/platform/x86/p2sb.c | 171 +++++++++++++++++++++++++++--------- > 1 file changed, 129 insertions(+), 42 deletions(-) > > diff --git a/drivers/platform/x86/p2sb.c b/drivers/platform/x86/p2sb.c > index 1cf2471d54dd..ccebfd885718 100644 > --- a/drivers/platform/x86/p2sb.c > +++ b/drivers/platform/x86/p2sb.c > @@ -26,6 +26,19 @@ static const struct x86_cpu_id p2sb_cpu_ids[] = { > {} > }; > > +/* > + * Cache BAR0 of P2SB device functions 0 to 7. > + * TODO: Move this definition to pci.h together with same other definitions. "with same other definitions" sounds very odd and I don't understand what you're trying to say here. > + */ > +#define NR_P2SB_RES_CACHE 8 > + > +struct p2sb_res_cache { > + u32 bus_dev_id; > + struct resource res; > +}; > + > +static struct p2sb_res_cache p2sb_resources[NR_P2SB_RES_CACHE]; > + > static int p2sb_get_devfn(unsigned int *devfn) > { > unsigned int fn = P2SB_DEVFN_DEFAULT; > @@ -39,10 +52,18 @@ static int p2sb_get_devfn(unsigned int *devfn) > return 0; > } > > +static bool p2sb_valid_resource(struct resource *res) > +{ > + if (res->flags) > + return true; > + > + return false; > +} > + > /* Copy resource from the first BAR of the device in question */ > -static int p2sb_read_bar0(struct pci_dev *pdev, struct resource *mem) > +static void p2sb_read_bar0(struct pci_dev *pdev, struct resource *mem) > { > - struct resource *bar0 = &pdev->resource[0]; > + struct resource *bar0 = pci_resource_n(pdev, 0); This change is just cleanup and belongs IMO to own patch. (It's a good change still, just doesn't belong to this fix.) > /* Make sure we have no dangling pointers in the output */ > memset(mem, 0, sizeof(*mem)); > @@ -56,47 +77,64 @@ static int p2sb_read_bar0(struct pci_dev *pdev, struct resource *mem) > mem->end = bar0->end; > mem->flags = bar0->flags; > mem->desc = bar0->desc; > - > - return 0; > } > > -static int p2sb_scan_and_read(struct pci_bus *bus, unsigned int devfn, struct resource *mem) > +static void p2sb_scan_and_cache_devfn(struct pci_bus *bus, unsigned int devfn) > { > + struct p2sb_res_cache *cache = &p2sb_resources[PCI_FUNC(devfn)]; > struct pci_dev *pdev; > - int ret; > > pdev = pci_scan_single_device(bus, devfn); > if (!pdev) > - return -ENODEV; > + return; > > - ret = p2sb_read_bar0(pdev, mem); > + p2sb_read_bar0(pdev, &cache->res); > + cache->bus_dev_id = bus->dev.id; > > pci_stop_and_remove_bus_device(pdev); > - return ret; > + return; > } > > -/** > - * p2sb_bar - Get Primary to Sideband (P2SB) bridge device BAR > - * @bus: PCI bus to communicate with > - * @devfn: PCI slot and function to communicate with > - * @mem: memory resource to be filled in > - * > - * The BIOS prevents the P2SB device from being enumerated by the PCI > - * subsystem, so we need to unhide and hide it back to lookup the BAR. > - * > - * if @bus is NULL, the bus 0 in domain 0 will be used. > - * If @devfn is 0, it will be replaced by devfn of the P2SB device. > - * > - * Caller must provide a valid pointer to @mem. > - * > - * Locking is handled by pci_rescan_remove_lock mutex. > - * > - * Return: > - * 0 on success or appropriate errno value on error. > - */ > -int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem) > +static int p2sb_scan_and_cache(struct pci_bus *bus, unsigned int devfn) > +{ > + unsigned int slot, fn; > + > + if (PCI_FUNC(devfn) == 0) { > + /* > + * When function number of the P2SB device is zero, scan it and > + * other function numbers, and if devices are available, cache > + * their BAR0s. > + */ > + slot = PCI_SLOT(devfn); > + for (fn = 0; fn < NR_P2SB_RES_CACHE; fn++) > + p2sb_scan_and_cache_devfn(bus, PCI_DEVFN(slot, fn)); > + } else { > + /* Scan the P2SB device and cache its BAR0 */ > + p2sb_scan_and_cache_devfn(bus, devfn); > + } > + > + if (!p2sb_valid_resource(&p2sb_resources[PCI_FUNC(devfn)].res)) > + return -ENOENT; > + > + return 0; > +} > + > +static struct pci_bus *p2sb_get_bus(struct pci_bus *bus) > +{ > + static struct pci_bus *p2sb_bus; > + > + bus = bus ?: p2sb_bus; > + if (bus) > + return bus; > + > + /* Assume P2SB is on the bus 0 in domain 0 */ > + p2sb_bus = pci_find_bus(0, 0); > + return p2sb_bus; > +} > + > +static int p2sb_cache_resources(void) > { > - struct pci_dev *pdev_p2sb; > + struct pci_bus *bus; > unsigned int devfn_p2sb; > u32 value = P2SBC_HIDE; > int ret; > @@ -106,8 +144,9 @@ int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem) > if (ret) > return ret; > > - /* if @bus is NULL, use bus 0 in domain 0 */ > - bus = bus ?: pci_find_bus(0, 0); > + bus = p2sb_get_bus(NULL); > + if (!bus) > + return -ENODEV; > > /* > * Prevent concurrent PCI bus scan from seeing the P2SB device and > @@ -115,17 +154,16 @@ int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem) > */ > pci_lock_rescan_remove(); > > - /* Unhide the P2SB device, if needed */ > + /* > + * The BIOS prevents the P2SB device from being enumerated by the PCI > + * subsystem, so we need to unhide and hide it back to lookup the BAR. > + * Unhide the P2SB device here, if needed. > + */ > pci_bus_read_config_dword(bus, devfn_p2sb, P2SBC, &value); > if (value & P2SBC_HIDE) > pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, 0); > > - pdev_p2sb = pci_scan_single_device(bus, devfn_p2sb); > - if (devfn) > - ret = p2sb_scan_and_read(bus, devfn, mem); > - else > - ret = p2sb_read_bar0(pdev_p2sb, mem); > - pci_stop_and_remove_bus_device(pdev_p2sb); > + ret = p2sb_scan_and_cache(bus, devfn_p2sb); > > /* Hide the P2SB device, if it was hidden */ > if (value & P2SBC_HIDE) > @@ -133,12 +171,61 @@ int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem) > > pci_unlock_rescan_remove(); > > - if (ret) > - return ret; > + return ret; > +} > + > +/** > + * p2sb_bar - Get Primary to Sideband (P2SB) bridge device BAR > + * @bus: PCI bus to communicate with > + * @devfn: PCI slot and function to communicate with > + * @mem: memory resource to be filled in > + * > + * If @bus is NULL, the bus 0 in domain 0 will be used. > + * If @devfn is 0, it will be replaced by devfn of the P2SB device. > + * > + * Caller must provide a valid pointer to @mem. > + * > + * Return: > + * 0 on success or appropriate errno value on error. > + */ > +int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem) > +{ > + struct p2sb_res_cache *cache; > + int ret; > > - if (mem->flags == 0) > + bus = p2sb_get_bus(bus); > + if (!bus) > return -ENODEV; > > + if (!devfn) { > + ret = p2sb_get_devfn(&devfn); > + if (ret) > + return ret; > + } > + > + cache = &p2sb_resources[PCI_FUNC(devfn)]; > + if (cache->bus_dev_id != bus->dev.id) > + return -ENODEV; > + > + if (!p2sb_valid_resource(&cache->res)) > + return -ENOENT; > + > + memcpy(mem, &cache->res, sizeof(*mem)); > return 0; > } > EXPORT_SYMBOL_GPL(p2sb_bar); > + > +static int __init p2sb_fs_init(void) > +{ > + p2sb_cache_resources(); > + return 0; > +} > + > +/* > + * pci_rescan_remove_lock avoids access to unhidden P2SB devices, but it causes > + * deadlock with sysfs pci bus rescan. Does this actually tell how situation was before this change? If so, it should be either removed or reworded to something that says "x cannot be used to do y because of z". > To avoid the deadlock, access to P2SB > + * devices at an early step in kernel initialization and cache required > + * resources. This should happen after subsys_initcall which initializes PCI > + * subsystem and before device_initcall which requires P2SB resources. > + */ > +fs_initcall(p2sb_fs_init); >
On Wed, 27 Dec 2023, Ilpo Järvinen wrote: > On Wed, 27 Dec 2023, Shin'ichiro Kawasaki wrote: > > > p2sb_bar() unhides P2SB device to get resources from the device. It > > guards the operation by locking pci_rescan_remove_lock so that parallel > > rescans do not find the P2SB device. However, this lock causes deadlock > > when PCI bus rescan is triggered by /sys/bus/pci/rescan. The rescan > > locks pci_rescan_remove_lock and probes PCI devices. When PCI devices > > call p2sb_bar() during probe, it locks pci_rescan_remove_lock again. > > Hence the deadlock. > > > > To avoid the deadlock, do not lock pci_rescan_remove_lock in p2sb_bar(). > > Instead, do the lock at fs_initcall. Introduce p2sb_cache_resources() > > for fs_initcall which gets and caches the P2SB resources. At p2sb_bar(), > > refer the cache and return to the caller. > > > > Link: https://lore.kernel.org/linux-pci/6xb24fjmptxxn5js2fjrrddjae6twex5bjaftwqsuawuqqqydx@7cl3uik5ef6j/ > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > > --- > > This patch reflects discussions held at the Link tag. I confirmed this patch > > fixes the problem using a system with i2c_i801 device, building i2c_i801 > > module as both built-in and loadable. Reviews will be appreicated. > > Why no Fixes tag? > > > 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 > > > > drivers/platform/x86/p2sb.c | 171 +++++++++++++++++++++++++++--------- > > 1 file changed, 129 insertions(+), 42 deletions(-) > > > > diff --git a/drivers/platform/x86/p2sb.c b/drivers/platform/x86/p2sb.c > > index 1cf2471d54dd..ccebfd885718 100644 > > --- a/drivers/platform/x86/p2sb.c > > +++ b/drivers/platform/x86/p2sb.c > > @@ -26,6 +26,19 @@ static const struct x86_cpu_id p2sb_cpu_ids[] = { > > {} > > }; > > > > +/* > > + * Cache BAR0 of P2SB device functions 0 to 7. > > + * TODO: Move this definition to pci.h together with same other definitions. > > "with same other definitions" sounds very odd and I don't understand what > you're trying to say here. > > > + */ > > +#define NR_P2SB_RES_CACHE 8 > > + > > +struct p2sb_res_cache { > > + u32 bus_dev_id; > > + struct resource res; > > +}; > > + > > +static struct p2sb_res_cache p2sb_resources[NR_P2SB_RES_CACHE]; > > + > > static int p2sb_get_devfn(unsigned int *devfn) > > { > > unsigned int fn = P2SB_DEVFN_DEFAULT; > > @@ -39,10 +52,18 @@ static int p2sb_get_devfn(unsigned int *devfn) > > return 0; > > } > > > > +static bool p2sb_valid_resource(struct resource *res) > > +{ > > + if (res->flags) > > + return true; > > + > > + return false; > > +} > > + > > /* Copy resource from the first BAR of the device in question */ > > -static int p2sb_read_bar0(struct pci_dev *pdev, struct resource *mem) > > +static void p2sb_read_bar0(struct pci_dev *pdev, struct resource *mem) > > { > > - struct resource *bar0 = &pdev->resource[0]; > > + struct resource *bar0 = pci_resource_n(pdev, 0); > > This change is just cleanup and belongs IMO to own patch. (It's a good > change still, just doesn't belong to this fix.) > > > /* Make sure we have no dangling pointers in the output */ > > memset(mem, 0, sizeof(*mem)); > > @@ -56,47 +77,64 @@ static int p2sb_read_bar0(struct pci_dev *pdev, struct resource *mem) > > mem->end = bar0->end; > > mem->flags = bar0->flags; > > mem->desc = bar0->desc; > > - > > - return 0; > > } > > > > -static int p2sb_scan_and_read(struct pci_bus *bus, unsigned int devfn, struct resource *mem) > > +static void p2sb_scan_and_cache_devfn(struct pci_bus *bus, unsigned int devfn) > > { > > + struct p2sb_res_cache *cache = &p2sb_resources[PCI_FUNC(devfn)]; > > struct pci_dev *pdev; > > - int ret; > > > > pdev = pci_scan_single_device(bus, devfn); > > if (!pdev) > > - return -ENODEV; > > + return; > > > > - ret = p2sb_read_bar0(pdev, mem); > > + p2sb_read_bar0(pdev, &cache->res); > > + cache->bus_dev_id = bus->dev.id; > > > > pci_stop_and_remove_bus_device(pdev); > > - return ret; > > + return; > > } > > > > -/** > > - * p2sb_bar - Get Primary to Sideband (P2SB) bridge device BAR > > - * @bus: PCI bus to communicate with > > - * @devfn: PCI slot and function to communicate with > > - * @mem: memory resource to be filled in > > - * > > - * The BIOS prevents the P2SB device from being enumerated by the PCI > > - * subsystem, so we need to unhide and hide it back to lookup the BAR. > > - * > > - * if @bus is NULL, the bus 0 in domain 0 will be used. > > - * If @devfn is 0, it will be replaced by devfn of the P2SB device. > > - * > > - * Caller must provide a valid pointer to @mem. > > - * > > - * Locking is handled by pci_rescan_remove_lock mutex. > > - * > > - * Return: > > - * 0 on success or appropriate errno value on error. > > - */ > > -int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem) > > +static int p2sb_scan_and_cache(struct pci_bus *bus, unsigned int devfn) > > +{ > > + unsigned int slot, fn; > > + > > + if (PCI_FUNC(devfn) == 0) { > > + /* > > + * When function number of the P2SB device is zero, scan it and > > + * other function numbers, and if devices are available, cache > > + * their BAR0s. > > + */ > > + slot = PCI_SLOT(devfn); > > + for (fn = 0; fn < NR_P2SB_RES_CACHE; fn++) > > + p2sb_scan_and_cache_devfn(bus, PCI_DEVFN(slot, fn)); > > + } else { > > + /* Scan the P2SB device and cache its BAR0 */ > > + p2sb_scan_and_cache_devfn(bus, devfn); > > + } > > + > > + if (!p2sb_valid_resource(&p2sb_resources[PCI_FUNC(devfn)].res)) > > + return -ENOENT; > > + > > + return 0; > > +} > > + > > +static struct pci_bus *p2sb_get_bus(struct pci_bus *bus) > > +{ > > + static struct pci_bus *p2sb_bus; > > + > > + bus = bus ?: p2sb_bus; > > + if (bus) > > + return bus; > > + > > + /* Assume P2SB is on the bus 0 in domain 0 */ > > + p2sb_bus = pci_find_bus(0, 0); > > + return p2sb_bus; > > +} > > + > > +static int p2sb_cache_resources(void) > > { > > - struct pci_dev *pdev_p2sb; > > + struct pci_bus *bus; > > unsigned int devfn_p2sb; > > u32 value = P2SBC_HIDE; > > int ret; > > @@ -106,8 +144,9 @@ int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem) > > if (ret) > > return ret; > > > > - /* if @bus is NULL, use bus 0 in domain 0 */ > > - bus = bus ?: pci_find_bus(0, 0); > > + bus = p2sb_get_bus(NULL); > > + if (!bus) > > + return -ENODEV; > > > > /* > > * Prevent concurrent PCI bus scan from seeing the P2SB device and > > @@ -115,17 +154,16 @@ int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem) > > */ > > pci_lock_rescan_remove(); > > > > - /* Unhide the P2SB device, if needed */ > > + /* > > + * The BIOS prevents the P2SB device from being enumerated by the PCI > > + * subsystem, so we need to unhide and hide it back to lookup the BAR. > > + * Unhide the P2SB device here, if needed. > > + */ > > pci_bus_read_config_dword(bus, devfn_p2sb, P2SBC, &value); > > if (value & P2SBC_HIDE) > > pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, 0); > > > > - pdev_p2sb = pci_scan_single_device(bus, devfn_p2sb); > > - if (devfn) > > - ret = p2sb_scan_and_read(bus, devfn, mem); > > - else > > - ret = p2sb_read_bar0(pdev_p2sb, mem); > > - pci_stop_and_remove_bus_device(pdev_p2sb); > > + ret = p2sb_scan_and_cache(bus, devfn_p2sb); > > > > /* Hide the P2SB device, if it was hidden */ > > if (value & P2SBC_HIDE) > > @@ -133,12 +171,61 @@ int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem) > > > > pci_unlock_rescan_remove(); > > > > - if (ret) > > - return ret; > > + return ret; > > +} > > + > > +/** > > + * p2sb_bar - Get Primary to Sideband (P2SB) bridge device BAR > > + * @bus: PCI bus to communicate with > > + * @devfn: PCI slot and function to communicate with > > + * @mem: memory resource to be filled in > > + * > > + * If @bus is NULL, the bus 0 in domain 0 will be used. > > + * If @devfn is 0, it will be replaced by devfn of the P2SB device. > > + * > > + * Caller must provide a valid pointer to @mem. > > + * > > + * Return: > > + * 0 on success or appropriate errno value on error. > > + */ > > +int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem) > > +{ > > + struct p2sb_res_cache *cache; > > + int ret; > > > > - if (mem->flags == 0) > > + bus = p2sb_get_bus(bus); > > + if (!bus) > > return -ENODEV; > > > > + if (!devfn) { > > + ret = p2sb_get_devfn(&devfn); > > + if (ret) > > + return ret; > > + } > > + > > + cache = &p2sb_resources[PCI_FUNC(devfn)]; > > + if (cache->bus_dev_id != bus->dev.id) > > + return -ENODEV; > > + > > + if (!p2sb_valid_resource(&cache->res)) > > + return -ENOENT; > > + > > + memcpy(mem, &cache->res, sizeof(*mem)); > > return 0; > > } > > EXPORT_SYMBOL_GPL(p2sb_bar); > > + > > +static int __init p2sb_fs_init(void) > > +{ > > + p2sb_cache_resources(); > > + return 0; > > +} > > + > > +/* > > + * pci_rescan_remove_lock avoids access to unhidden P2SB devices, but it causes > > + * deadlock with sysfs pci bus rescan. > > Does this actually tell how situation was before this change? If so, it > should be either removed or reworded to something that says "x cannot be > used to do y because of z". > > > To avoid the deadlock, access to P2SB > > + * devices at an early step in kernel initialization and cache required > > + * resources. This should happen after subsys_initcall which initializes PCI > > + * subsystem and before device_initcall which requires P2SB resources. > > + */ > > +fs_initcall(p2sb_fs_init); > > > > I've applied this into review-ilpo branch with a Fixes tag but without any changes to the code. Please do consider my feedback though to see if they comments would need to be improved.
On Dec 28, 2023 / 17:47, Ilpo Järvinen wrote: > On Wed, 27 Dec 2023, Ilpo Järvinen wrote: > > > On Wed, 27 Dec 2023, Shin'ichiro Kawasaki wrote: > > > > > p2sb_bar() unhides P2SB device to get resources from the device. It > > > guards the operation by locking pci_rescan_remove_lock so that parallel > > > rescans do not find the P2SB device. However, this lock causes deadlock > > > when PCI bus rescan is triggered by /sys/bus/pci/rescan. The rescan > > > locks pci_rescan_remove_lock and probes PCI devices. When PCI devices > > > call p2sb_bar() during probe, it locks pci_rescan_remove_lock again. > > > Hence the deadlock. > > > > > > To avoid the deadlock, do not lock pci_rescan_remove_lock in p2sb_bar(). > > > Instead, do the lock at fs_initcall. Introduce p2sb_cache_resources() > > > for fs_initcall which gets and caches the P2SB resources. At p2sb_bar(), > > > refer the cache and return to the caller. > > > > > > Link: https://lore.kernel.org/linux-pci/6xb24fjmptxxn5js2fjrrddjae6twex5bjaftwqsuawuqqqydx@7cl3uik5ef6j/ > > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > > > --- > > > This patch reflects discussions held at the Link tag. I confirmed this patch > > > fixes the problem using a system with i2c_i801 device, building i2c_i801 > > > module as both built-in and loadable. Reviews will be appreicated. > > > > Why no Fixes tag? I should have added that. Will add it. I think the commit for the Fixes tag is: 9745fb07474f ("platform/x86/intel: Add Primary to Sideband (P2SB) bridge support") > > > > > 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 > > > > > > drivers/platform/x86/p2sb.c | 171 +++++++++++++++++++++++++++--------- > > > 1 file changed, 129 insertions(+), 42 deletions(-) > > > > > > diff --git a/drivers/platform/x86/p2sb.c b/drivers/platform/x86/p2sb.c > > > index 1cf2471d54dd..ccebfd885718 100644 > > > --- a/drivers/platform/x86/p2sb.c > > > +++ b/drivers/platform/x86/p2sb.c > > > @@ -26,6 +26,19 @@ static const struct x86_cpu_id p2sb_cpu_ids[] = { > > > {} > > > }; > > > > > > +/* > > > + * Cache BAR0 of P2SB device functions 0 to 7. > > > + * TODO: Move this definition to pci.h together with same other definitions. > > > > "with same other definitions" sounds very odd and I don't understand what > > you're trying to say here. I see, I will elaborate it as follows in the next post. * TODO: The constant 8 is the number of functions that PCI specification * defines. Same definitions exist tree-wide. Unify this definition and * the other definitions then move to include/uapi/linux/pci.h. > > > > > + */ > > > +#define NR_P2SB_RES_CACHE 8 > > > + > > > +struct p2sb_res_cache { > > > + u32 bus_dev_id; > > > + struct resource res; > > > +}; > > > + > > > +static struct p2sb_res_cache p2sb_resources[NR_P2SB_RES_CACHE]; > > > + > > > static int p2sb_get_devfn(unsigned int *devfn) > > > { > > > unsigned int fn = P2SB_DEVFN_DEFAULT; > > > @@ -39,10 +52,18 @@ static int p2sb_get_devfn(unsigned int *devfn) > > > return 0; > > > } > > > > > > +static bool p2sb_valid_resource(struct resource *res) > > > +{ > > > + if (res->flags) > > > + return true; > > > + > > > + return false; > > > +} > > > + > > > /* Copy resource from the first BAR of the device in question */ > > > -static int p2sb_read_bar0(struct pci_dev *pdev, struct resource *mem) > > > +static void p2sb_read_bar0(struct pci_dev *pdev, struct resource *mem) > > > { > > > - struct resource *bar0 = &pdev->resource[0]; > > > + struct resource *bar0 = pci_resource_n(pdev, 0); > > > > This change is just cleanup and belongs IMO to own patch. (It's a good > > change still, just doesn't belong to this fix.) I see. I will drop the hunk above from this patch, and will add a following patch dedicated for it. > > > > > /* Make sure we have no dangling pointers in the output */ > > > memset(mem, 0, sizeof(*mem)); > > > @@ -56,47 +77,64 @@ static int p2sb_read_bar0(struct pci_dev *pdev, struct resource *mem) > > > mem->end = bar0->end; > > > mem->flags = bar0->flags; > > > mem->desc = bar0->desc; > > > - > > > - return 0; > > > } > > > > > > -static int p2sb_scan_and_read(struct pci_bus *bus, unsigned int devfn, struct resource *mem) > > > +static void p2sb_scan_and_cache_devfn(struct pci_bus *bus, unsigned int devfn) > > > { > > > + struct p2sb_res_cache *cache = &p2sb_resources[PCI_FUNC(devfn)]; > > > struct pci_dev *pdev; > > > - int ret; > > > > > > pdev = pci_scan_single_device(bus, devfn); > > > if (!pdev) > > > - return -ENODEV; > > > + return; > > > > > > - ret = p2sb_read_bar0(pdev, mem); > > > + p2sb_read_bar0(pdev, &cache->res); > > > + cache->bus_dev_id = bus->dev.id; > > > > > > pci_stop_and_remove_bus_device(pdev); > > > - return ret; > > > + return; > > > } > > > > > > -/** > > > - * p2sb_bar - Get Primary to Sideband (P2SB) bridge device BAR > > > - * @bus: PCI bus to communicate with > > > - * @devfn: PCI slot and function to communicate with > > > - * @mem: memory resource to be filled in > > > - * > > > - * The BIOS prevents the P2SB device from being enumerated by the PCI > > > - * subsystem, so we need to unhide and hide it back to lookup the BAR. > > > - * > > > - * if @bus is NULL, the bus 0 in domain 0 will be used. > > > - * If @devfn is 0, it will be replaced by devfn of the P2SB device. > > > - * > > > - * Caller must provide a valid pointer to @mem. > > > - * > > > - * Locking is handled by pci_rescan_remove_lock mutex. > > > - * > > > - * Return: > > > - * 0 on success or appropriate errno value on error. > > > - */ > > > -int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem) > > > +static int p2sb_scan_and_cache(struct pci_bus *bus, unsigned int devfn) > > > +{ > > > + unsigned int slot, fn; > > > + > > > + if (PCI_FUNC(devfn) == 0) { > > > + /* > > > + * When function number of the P2SB device is zero, scan it and > > > + * other function numbers, and if devices are available, cache > > > + * their BAR0s. > > > + */ > > > + slot = PCI_SLOT(devfn); > > > + for (fn = 0; fn < NR_P2SB_RES_CACHE; fn++) > > > + p2sb_scan_and_cache_devfn(bus, PCI_DEVFN(slot, fn)); > > > + } else { > > > + /* Scan the P2SB device and cache its BAR0 */ > > > + p2sb_scan_and_cache_devfn(bus, devfn); > > > + } > > > + > > > + if (!p2sb_valid_resource(&p2sb_resources[PCI_FUNC(devfn)].res)) > > > + return -ENOENT; > > > + > > > + return 0; > > > +} > > > + > > > +static struct pci_bus *p2sb_get_bus(struct pci_bus *bus) > > > +{ > > > + static struct pci_bus *p2sb_bus; > > > + > > > + bus = bus ?: p2sb_bus; > > > + if (bus) > > > + return bus; > > > + > > > + /* Assume P2SB is on the bus 0 in domain 0 */ > > > + p2sb_bus = pci_find_bus(0, 0); > > > + return p2sb_bus; > > > +} > > > + > > > +static int p2sb_cache_resources(void) > > > { > > > - struct pci_dev *pdev_p2sb; > > > + struct pci_bus *bus; > > > unsigned int devfn_p2sb; > > > u32 value = P2SBC_HIDE; > > > int ret; > > > @@ -106,8 +144,9 @@ int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem) > > > if (ret) > > > return ret; > > > > > > - /* if @bus is NULL, use bus 0 in domain 0 */ > > > - bus = bus ?: pci_find_bus(0, 0); > > > + bus = p2sb_get_bus(NULL); > > > + if (!bus) > > > + return -ENODEV; > > > > > > /* > > > * Prevent concurrent PCI bus scan from seeing the P2SB device and > > > @@ -115,17 +154,16 @@ int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem) > > > */ > > > pci_lock_rescan_remove(); > > > > > > - /* Unhide the P2SB device, if needed */ > > > + /* > > > + * The BIOS prevents the P2SB device from being enumerated by the PCI > > > + * subsystem, so we need to unhide and hide it back to lookup the BAR. > > > + * Unhide the P2SB device here, if needed. > > > + */ > > > pci_bus_read_config_dword(bus, devfn_p2sb, P2SBC, &value); > > > if (value & P2SBC_HIDE) > > > pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, 0); > > > > > > - pdev_p2sb = pci_scan_single_device(bus, devfn_p2sb); > > > - if (devfn) > > > - ret = p2sb_scan_and_read(bus, devfn, mem); > > > - else > > > - ret = p2sb_read_bar0(pdev_p2sb, mem); > > > - pci_stop_and_remove_bus_device(pdev_p2sb); > > > + ret = p2sb_scan_and_cache(bus, devfn_p2sb); > > > > > > /* Hide the P2SB device, if it was hidden */ > > > if (value & P2SBC_HIDE) > > > @@ -133,12 +171,61 @@ int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem) > > > > > > pci_unlock_rescan_remove(); > > > > > > - if (ret) > > > - return ret; > > > + return ret; > > > +} > > > + > > > +/** > > > + * p2sb_bar - Get Primary to Sideband (P2SB) bridge device BAR > > > + * @bus: PCI bus to communicate with > > > + * @devfn: PCI slot and function to communicate with > > > + * @mem: memory resource to be filled in > > > + * > > > + * If @bus is NULL, the bus 0 in domain 0 will be used. > > > + * If @devfn is 0, it will be replaced by devfn of the P2SB device. > > > + * > > > + * Caller must provide a valid pointer to @mem. > > > + * > > > + * Return: > > > + * 0 on success or appropriate errno value on error. > > > + */ > > > +int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem) > > > +{ > > > + struct p2sb_res_cache *cache; > > > + int ret; > > > > > > - if (mem->flags == 0) > > > + bus = p2sb_get_bus(bus); > > > + if (!bus) > > > return -ENODEV; > > > > > > + if (!devfn) { > > > + ret = p2sb_get_devfn(&devfn); > > > + if (ret) > > > + return ret; > > > + } > > > + > > > + cache = &p2sb_resources[PCI_FUNC(devfn)]; > > > + if (cache->bus_dev_id != bus->dev.id) > > > + return -ENODEV; > > > + > > > + if (!p2sb_valid_resource(&cache->res)) > > > + return -ENOENT; > > > + > > > + memcpy(mem, &cache->res, sizeof(*mem)); > > > return 0; > > > } > > > EXPORT_SYMBOL_GPL(p2sb_bar); > > > + > > > +static int __init p2sb_fs_init(void) > > > +{ > > > + p2sb_cache_resources(); > > > + return 0; > > > +} > > > + > > > +/* > > > + * pci_rescan_remove_lock avoids access to unhidden P2SB devices, but it causes > > > + * deadlock with sysfs pci bus rescan. > > > > Does this actually tell how situation was before this change? If so, it > > should be either removed or reworded to something that says "x cannot be > > used to do y because of z". I will rephrase the sentence as follows. * pci_rescan_remove_lock to avoid access to unhidden P2SB devices can * not be locked in sysfs pci bus rescan path because of deadlock. > > > > > To avoid the deadlock, access to P2SB > > > + * devices at an early step in kernel initialization and cache required > > > + * resources. This should happen after subsys_initcall which initializes PCI > > > + * subsystem and before device_initcall which requires P2SB resources. > > > + */ > > > +fs_initcall(p2sb_fs_init); > > > > > > > > > I've applied this into review-ilpo branch with a Fixes tag but without > any changes to the code. Please do consider my feedback though to see if > they comments would need to be improved. Thanks for the comments. Will reflect them to v5.
diff --git a/drivers/platform/x86/p2sb.c b/drivers/platform/x86/p2sb.c index 1cf2471d54dd..ccebfd885718 100644 --- a/drivers/platform/x86/p2sb.c +++ b/drivers/platform/x86/p2sb.c @@ -26,6 +26,19 @@ static const struct x86_cpu_id p2sb_cpu_ids[] = { {} }; +/* + * Cache BAR0 of P2SB device functions 0 to 7. + * TODO: Move this definition to pci.h together with same other definitions. + */ +#define NR_P2SB_RES_CACHE 8 + +struct p2sb_res_cache { + u32 bus_dev_id; + struct resource res; +}; + +static struct p2sb_res_cache p2sb_resources[NR_P2SB_RES_CACHE]; + static int p2sb_get_devfn(unsigned int *devfn) { unsigned int fn = P2SB_DEVFN_DEFAULT; @@ -39,10 +52,18 @@ static int p2sb_get_devfn(unsigned int *devfn) return 0; } +static bool p2sb_valid_resource(struct resource *res) +{ + if (res->flags) + return true; + + return false; +} + /* Copy resource from the first BAR of the device in question */ -static int p2sb_read_bar0(struct pci_dev *pdev, struct resource *mem) +static void p2sb_read_bar0(struct pci_dev *pdev, struct resource *mem) { - struct resource *bar0 = &pdev->resource[0]; + struct resource *bar0 = pci_resource_n(pdev, 0); /* Make sure we have no dangling pointers in the output */ memset(mem, 0, sizeof(*mem)); @@ -56,47 +77,64 @@ static int p2sb_read_bar0(struct pci_dev *pdev, struct resource *mem) mem->end = bar0->end; mem->flags = bar0->flags; mem->desc = bar0->desc; - - return 0; } -static int p2sb_scan_and_read(struct pci_bus *bus, unsigned int devfn, struct resource *mem) +static void p2sb_scan_and_cache_devfn(struct pci_bus *bus, unsigned int devfn) { + struct p2sb_res_cache *cache = &p2sb_resources[PCI_FUNC(devfn)]; struct pci_dev *pdev; - int ret; pdev = pci_scan_single_device(bus, devfn); if (!pdev) - return -ENODEV; + return; - ret = p2sb_read_bar0(pdev, mem); + p2sb_read_bar0(pdev, &cache->res); + cache->bus_dev_id = bus->dev.id; pci_stop_and_remove_bus_device(pdev); - return ret; + return; } -/** - * p2sb_bar - Get Primary to Sideband (P2SB) bridge device BAR - * @bus: PCI bus to communicate with - * @devfn: PCI slot and function to communicate with - * @mem: memory resource to be filled in - * - * The BIOS prevents the P2SB device from being enumerated by the PCI - * subsystem, so we need to unhide and hide it back to lookup the BAR. - * - * if @bus is NULL, the bus 0 in domain 0 will be used. - * If @devfn is 0, it will be replaced by devfn of the P2SB device. - * - * Caller must provide a valid pointer to @mem. - * - * Locking is handled by pci_rescan_remove_lock mutex. - * - * Return: - * 0 on success or appropriate errno value on error. - */ -int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem) +static int p2sb_scan_and_cache(struct pci_bus *bus, unsigned int devfn) +{ + unsigned int slot, fn; + + if (PCI_FUNC(devfn) == 0) { + /* + * When function number of the P2SB device is zero, scan it and + * other function numbers, and if devices are available, cache + * their BAR0s. + */ + slot = PCI_SLOT(devfn); + for (fn = 0; fn < NR_P2SB_RES_CACHE; fn++) + p2sb_scan_and_cache_devfn(bus, PCI_DEVFN(slot, fn)); + } else { + /* Scan the P2SB device and cache its BAR0 */ + p2sb_scan_and_cache_devfn(bus, devfn); + } + + if (!p2sb_valid_resource(&p2sb_resources[PCI_FUNC(devfn)].res)) + return -ENOENT; + + return 0; +} + +static struct pci_bus *p2sb_get_bus(struct pci_bus *bus) +{ + static struct pci_bus *p2sb_bus; + + bus = bus ?: p2sb_bus; + if (bus) + return bus; + + /* Assume P2SB is on the bus 0 in domain 0 */ + p2sb_bus = pci_find_bus(0, 0); + return p2sb_bus; +} + +static int p2sb_cache_resources(void) { - struct pci_dev *pdev_p2sb; + struct pci_bus *bus; unsigned int devfn_p2sb; u32 value = P2SBC_HIDE; int ret; @@ -106,8 +144,9 @@ int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem) if (ret) return ret; - /* if @bus is NULL, use bus 0 in domain 0 */ - bus = bus ?: pci_find_bus(0, 0); + bus = p2sb_get_bus(NULL); + if (!bus) + return -ENODEV; /* * Prevent concurrent PCI bus scan from seeing the P2SB device and @@ -115,17 +154,16 @@ int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem) */ pci_lock_rescan_remove(); - /* Unhide the P2SB device, if needed */ + /* + * The BIOS prevents the P2SB device from being enumerated by the PCI + * subsystem, so we need to unhide and hide it back to lookup the BAR. + * Unhide the P2SB device here, if needed. + */ pci_bus_read_config_dword(bus, devfn_p2sb, P2SBC, &value); if (value & P2SBC_HIDE) pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, 0); - pdev_p2sb = pci_scan_single_device(bus, devfn_p2sb); - if (devfn) - ret = p2sb_scan_and_read(bus, devfn, mem); - else - ret = p2sb_read_bar0(pdev_p2sb, mem); - pci_stop_and_remove_bus_device(pdev_p2sb); + ret = p2sb_scan_and_cache(bus, devfn_p2sb); /* Hide the P2SB device, if it was hidden */ if (value & P2SBC_HIDE) @@ -133,12 +171,61 @@ int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem) pci_unlock_rescan_remove(); - if (ret) - return ret; + return ret; +} + +/** + * p2sb_bar - Get Primary to Sideband (P2SB) bridge device BAR + * @bus: PCI bus to communicate with + * @devfn: PCI slot and function to communicate with + * @mem: memory resource to be filled in + * + * If @bus is NULL, the bus 0 in domain 0 will be used. + * If @devfn is 0, it will be replaced by devfn of the P2SB device. + * + * Caller must provide a valid pointer to @mem. + * + * Return: + * 0 on success or appropriate errno value on error. + */ +int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem) +{ + struct p2sb_res_cache *cache; + int ret; - if (mem->flags == 0) + bus = p2sb_get_bus(bus); + if (!bus) return -ENODEV; + if (!devfn) { + ret = p2sb_get_devfn(&devfn); + if (ret) + return ret; + } + + cache = &p2sb_resources[PCI_FUNC(devfn)]; + if (cache->bus_dev_id != bus->dev.id) + return -ENODEV; + + if (!p2sb_valid_resource(&cache->res)) + return -ENOENT; + + memcpy(mem, &cache->res, sizeof(*mem)); return 0; } EXPORT_SYMBOL_GPL(p2sb_bar); + +static int __init p2sb_fs_init(void) +{ + p2sb_cache_resources(); + return 0; +} + +/* + * pci_rescan_remove_lock avoids access to unhidden P2SB devices, but it causes + * deadlock with sysfs pci bus rescan. To avoid the deadlock, access to P2SB + * devices at an early step in kernel initialization and cache required + * resources. This should happen after subsys_initcall which initializes PCI + * subsystem and before device_initcall which requires P2SB resources. + */ +fs_initcall(p2sb_fs_init);
p2sb_bar() unhides P2SB device to get resources from the device. It guards the operation by locking pci_rescan_remove_lock so that parallel rescans do not find the P2SB device. However, this lock causes deadlock when PCI bus rescan is triggered by /sys/bus/pci/rescan. The rescan locks pci_rescan_remove_lock and probes PCI devices. When PCI devices call p2sb_bar() during probe, it locks pci_rescan_remove_lock again. Hence the deadlock. To avoid the deadlock, do not lock pci_rescan_remove_lock in p2sb_bar(). Instead, do the lock at fs_initcall. Introduce p2sb_cache_resources() for fs_initcall which gets and caches the P2SB resources. At p2sb_bar(), refer the cache and return to the caller. Link: https://lore.kernel.org/linux-pci/6xb24fjmptxxn5js2fjrrddjae6twex5bjaftwqsuawuqqqydx@7cl3uik5ef6j/ Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> --- This patch reflects discussions held at the Link tag. I confirmed this patch fixes the problem using a system with i2c_i801 device, building i2c_i801 module as both built-in and loadable. Reviews will be appreicated. 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 drivers/platform/x86/p2sb.c | 171 +++++++++++++++++++++++++++--------- 1 file changed, 129 insertions(+), 42 deletions(-)