| Message ID | 20231212114746.183639-1-shinichiro.kawasaki@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC,v2] platform/x86: p2sb: Allow p2sb_bar() calls during PCI device probe | expand |
On Tue, Dec 12, 2023 at 08:47:46PM +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. ... > +/* Cache BAR0 of P2SB device from function 0 ot 7 */ > +#define NR_P2SB_RES_CACHE 8 Here... (at least some better comment with TODO needs to be added, and next patches can address that). > struct resource *bar0 = &pdev->resource[0]; ...and here... (okay, not exactly here, but with the same idea, to use pci_resource_n() macro) > + if (!PCI_FUNC(devfn_p2sb)) { > + slot_p2sb = PCI_SLOT(devfn_p2sb); > + for (fn = 1; fn < 8; fn++) ...and here... > + p2sb_scan_and_cache(bus, PCI_DEVFN(slot_p2sb, fn)); > + } ...and so on I gave comments. Any reason why they left unaddressed?
On Tue, Dec 12, 2023 at 08:47:46PM +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. > ... > +/* Cache BAR0 of P2SB device from function 0 ot 7 */ s/ot/to/ or even s/from function 0 ot 7/functions 0 to 7/ > +static bool p2sb_invalid_resource(struct resource *res) > +{ > + return res->flags == 0; > +} IMO this would read better as "p2sb_valid_resource()" with corresponding negation in the callers. But it doesn't matter either way. > + * 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. s/if/If/
On Dec 13, 2023 / 16:05, Andy Shevchenko wrote: > On Tue, Dec 12, 2023 at 08:47:46PM +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. > > ... > > > +/* Cache BAR0 of P2SB device from function 0 ot 7 */ > > +#define NR_P2SB_RES_CACHE 8 > > Here... (at least some better comment with TODO needs to be added, > and next patches can address that). > > > struct resource *bar0 = &pdev->resource[0]; > > ...and here... (okay, not exactly here, but with the same idea, > to use pci_resource_n() macro) > > > + if (!PCI_FUNC(devfn_p2sb)) { > > + slot_p2sb = PCI_SLOT(devfn_p2sb); > > + for (fn = 1; fn < 8; fn++) > > ...and here... > > > + p2sb_scan_and_cache(bus, PCI_DEVFN(slot_p2sb, fn)); > > + } > > ...and so on I gave comments. Any reason why they left unaddressed? Andy, thanks for the response, but I'm not sure about the comments you gave. I guess you responded to the RFC v1 patch but it somehow did not reach to me, probably. According to the lore archive, only Hans responded to the RFC v1 [1]. If this guess is correct, could you resend your comments on the RFC v1? [1] https://lore.kernel.org/platform-driver-x86/20231201104759.949340-1-shinichiro.kawasaki@wdc.com/
On Thu, Dec 14, 2023 at 12:55:36AM +0000, Shinichiro Kawasaki wrote: > On Dec 13, 2023 / 16:05, Andy Shevchenko wrote: > > On Tue, Dec 12, 2023 at 08:47:46PM +0900, Shin'ichiro Kawasaki wrote: ... > > > +/* Cache BAR0 of P2SB device from function 0 ot 7 */ > > > +#define NR_P2SB_RES_CACHE 8 > > > > Here... (at least some better comment with TODO needs to be added, > > and next patches can address that). > > > > > struct resource *bar0 = &pdev->resource[0]; > > > > ...and here... (okay, not exactly here, but with the same idea, > > to use pci_resource_n() macro) > > > > > + if (!PCI_FUNC(devfn_p2sb)) { > > > + slot_p2sb = PCI_SLOT(devfn_p2sb); > > > + for (fn = 1; fn < 8; fn++) > > > > ...and here... > > > > > + p2sb_scan_and_cache(bus, PCI_DEVFN(slot_p2sb, fn)); > > > + } > > > > ...and so on I gave comments. Any reason why they left unaddressed? > > Andy, thanks for the response, but I'm not sure about the comments you gave. > I guess you responded to the RFC v1 patch but it somehow did not reach to me, > probably. According to the lore archive, only Hans responded to the RFC v1 [1]. > If this guess is correct, could you resend your comments on the RFC v1? Oh, my... It seems the message was never delivered and I haven't paid attention on the bounces. Give me time, I'll try to restore the review from my memory and will send a new email. > [1] https://lore.kernel.org/platform-driver-x86/20231201104759.949340-1-shinichiro.kawasaki@wdc.com/
On Tue, Dec 12, 2023 at 08:47:46PM +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. ... > +/* Cache BAR0 of P2SB device from function 0 ot 7 */ > +#define NR_P2SB_RES_CACHE 8 This is fifth or so definition for the same, isn't it a good time to create a treewide definition in pci.h? See also below. (In previous mail I even found all cases and listed, a bit lazy to repeat.) ... > +static bool p2sb_invalid_resource(struct resource *res) The naming is better to be p2sb_is_resource_valid(). ... > struct resource *bar0 = &pdev->resource[0]; This and in new code can use pci_resource_n() macro. ... > pdev = pci_scan_single_device(bus, devfn); > - if (!pdev) > + if (!pdev || p2sb_invalid_resource(&pdev->resource[0])) > return -ENODEV; I prefer to split and have different error code for the second one: -ENOENT / -EINVAL / etc. ... > + struct pci_bus *bus; > + unsigned int devfn_p2sb, slot_p2sb, fn; Please, preserve reversed xmas tree ordering. > u32 value = P2SBC_HIDE; > int ret; ... > - /* if @bus is NULL, use bus 0 in domain 0 */ > - bus = bus ?: pci_find_bus(0, 0); > + /* Assume P2SB is on the bus 0 in domain 0 */ > + bus = pci_find_bus(0, 0); The pci_find_bus() is called in two places now. Can we avoid doing this duplication? ... > + /* > + * When function number of the P2SB device is zero, scan other function > + * numbers. If devices are available, cache their BAR0. > + */ > + if (!PCI_FUNC(devfn_p2sb)) { I prefer to see '== 0' to make it clear that 0 has the same semantics as other numbers here. It's not special like NULL. > + slot_p2sb = PCI_SLOT(devfn_p2sb); > + for (fn = 1; fn < 8; fn++) As per above, use a definition for 8 > + p2sb_scan_and_cache(bus, PCI_DEVFN(slot_p2sb, fn)); > + } > + > +out: Can it be split the above to the previous call or a separate helper? ... > +static int __init p2sb_fs_init(void) > +{ > + p2sb_cache_resources(); > + return 0; > +} Please, add a comment justifying fs_initcall(). > +fs_initcall(p2sb_fs_init);
Andy, thank you for resending the comments. I will reflect all of your comments to the next version. Please find a couple of comments below in line. On Dec 14, 2023 / 18:38, Andy Shevchenko wrote: > On Tue, Dec 12, 2023 at 08:47:46PM +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. > > ... > > > +/* Cache BAR0 of P2SB device from function 0 ot 7 */ > > +#define NR_P2SB_RES_CACHE 8 > > This is fifth or so definition for the same, isn't it a good time to create > a treewide definition in pci.h? > > See also below. > > (In previous mail I even found all cases and listed, a bit lazy to repeat.) I'm not sure where are other definitions. I guess PCI_CONF1_FUNC_SHIFT in drivers/pci/pci.h is one of them. As you suggested in another mail, I'll add a TODO comment and note that the NR_P2SB_RES_CACHE should be refactored later. > > ... > > > +static bool p2sb_invalid_resource(struct resource *res) > > The naming is better to be p2sb_is_resource_valid(). > > ... > > > struct resource *bar0 = &pdev->resource[0]; > > This and in new code can use pci_resource_n() macro. > > ... > > > pdev = pci_scan_single_device(bus, devfn); > > - if (!pdev) > > + if (!pdev || p2sb_invalid_resource(&pdev->resource[0])) > > return -ENODEV; > > I prefer to split and have different error code for the second one: > -ENOENT / -EINVAL / etc. > > ... > > > + struct pci_bus *bus; > > + unsigned int devfn_p2sb, slot_p2sb, fn; > > Please, preserve reversed xmas tree ordering. > > > u32 value = P2SBC_HIDE; > > int ret; > > ... > > > - /* if @bus is NULL, use bus 0 in domain 0 */ > > - bus = bus ?: pci_find_bus(0, 0); > > + /* Assume P2SB is on the bus 0 in domain 0 */ > > + bus = pci_find_bus(0, 0); > > The pci_find_bus() is called in two places now. Can we avoid doing > this duplication? I will add a global variable "static struct pci_bus *p2sb_bus". It will keep the first call return value and allow to skip the second call. > > ... > > > + /* > > + * When function number of the P2SB device is zero, scan other function > > + * numbers. If devices are available, cache their BAR0. > > + */ > > + if (!PCI_FUNC(devfn_p2sb)) { > > I prefer to see '== 0' to make it clear that 0 has the same semantics as other > numbers here. It's not special like NULL. > > > + slot_p2sb = PCI_SLOT(devfn_p2sb); > > + for (fn = 1; fn < 8; fn++) > > As per above, use a definition for 8 > > > + p2sb_scan_and_cache(bus, PCI_DEVFN(slot_p2sb, fn)); > > + } > > + > > +out: > > Can it be split the above to the previous call or a separate helper? > > ... > > > +static int __init p2sb_fs_init(void) > > +{ > > + p2sb_cache_resources(); > > + return 0; > > +} > > Please, add a comment justifying fs_initcall(). > > > +fs_initcall(p2sb_fs_init); > > -- > With Best Regards, > Andy Shevchenko > >
On Dec 13, 2023 / 13:41, Bjorn Helgaas wrote: > On Tue, Dec 12, 2023 at 08:47:46PM +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. > > ... > > > +/* Cache BAR0 of P2SB device from function 0 ot 7 */ > > s/ot/to/ > > or even s/from function 0 ot 7/functions 0 to 7/ > > > +static bool p2sb_invalid_resource(struct resource *res) > > +{ > > + return res->flags == 0; > > +} > > IMO this would read better as "p2sb_valid_resource()" with > corresponding negation in the callers. But it doesn't matter either > way. > > > + * 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. > > s/if/If/ Thank you Bjorn. Will reflect them to the next version.
On Thu, Dec 14, 2023 at 06:38:41PM +0200, Andy Shevchenko wrote: > On Tue, Dec 12, 2023 at 08:47:46PM +0900, Shin'ichiro Kawasaki wrote: > > +/* Cache BAR0 of P2SB device from function 0 ot 7 */ > > +#define NR_P2SB_RES_CACHE 8 > > This is fifth or so definition for the same, isn't it a good time to create > a treewide definition in pci.h? This isn't something defined in the PCI spec but rather an x86-specific constant, so should preferrably live somewhere in arch/x86/include/asm/. If you have a "maximum number of PCI functions per device" constant in mind then include/uapi/linux/pci.h might be a good fit. Thanks, Lukas
On Fri, Dec 15, 2023 at 08:52:10AM +0100, Lukas Wunner wrote: > On Thu, Dec 14, 2023 at 06:38:41PM +0200, Andy Shevchenko wrote: > > On Tue, Dec 12, 2023 at 08:47:46PM +0900, Shin'ichiro Kawasaki wrote: > > > +/* Cache BAR0 of P2SB device from function 0 ot 7 */ > > > +#define NR_P2SB_RES_CACHE 8 > > > > This is fifth or so definition for the same, isn't it a good time to create > > a treewide definition in pci.h? > > This isn't something defined in the PCI spec but rather an x86-specific > constant, so should preferrably live somewhere in arch/x86/include/asm/. I'm not sure I am following both paragraphs. > If you have a "maximum number of PCI functions per device" constant in mind > then include/uapi/linux/pci.h might be a good fit. This is indeed what I have had in mind, but why is this x86 specific? I didn't get...
On Fri, Dec 15, 2023 at 07:33:37AM +0000, Shinichiro Kawasaki wrote: > On Dec 14, 2023 / 18:38, Andy Shevchenko wrote: > > On Tue, Dec 12, 2023 at 08:47:46PM +0900, Shin'ichiro Kawasaki wrote: ... > > > +/* Cache BAR0 of P2SB device from function 0 ot 7 */ > > > +#define NR_P2SB_RES_CACHE 8 > > > > This is fifth or so definition for the same, isn't it a good time to create > > a treewide definition in pci.h? > > > > See also below. > > > > (In previous mail I even found all cases and listed, a bit lazy to repeat.) > > I'm not sure where are other definitions. I guess PCI_CONF1_FUNC_SHIFT in > drivers/pci/pci.h is one of them. Maybe, but I'm talking about treewide (drivers/, arch/, etc) duplications IIRC it is 5+ of them. > As you suggested in another mail, I'll add a > TODO comment and note that the NR_P2SB_RES_CACHE should be refactored later. Yeah, there are two ways, make this one dependent on the definition or introduce a definition and update later on. In the latter case it means a technical debt +. In the lost email I suggested to list all existing cases somewhere in TODO, so we won't grep again, but okay we can do that (although I forgot the exact `git grep ...` command line I have used for that search). ... > > > - /* if @bus is NULL, use bus 0 in domain 0 */ > > > - bus = bus ?: pci_find_bus(0, 0); > > > + /* Assume P2SB is on the bus 0 in domain 0 */ > > > + bus = pci_find_bus(0, 0); > > > > The pci_find_bus() is called in two places now. Can we avoid doing > > this duplication? > > I will add a global variable "static struct pci_bus *p2sb_bus". It will keep the > first call return value and allow to skip the second call. Hmm... Global variables are prone to subtle race conditions, OTOH the P2SB is not supposed to be hotplugged or moved, so I think it will be fine.
On Fri, Dec 15, 2023 at 05:37:01PM +0200, Andy Shevchenko wrote: > On Fri, Dec 15, 2023 at 08:52:10AM +0100, Lukas Wunner wrote: > > On Thu, Dec 14, 2023 at 06:38:41PM +0200, Andy Shevchenko wrote: > > > On Tue, Dec 12, 2023 at 08:47:46PM +0900, Shin'ichiro Kawasaki wrote: > > > > +/* Cache BAR0 of P2SB device from function 0 ot 7 */ > > > > +#define NR_P2SB_RES_CACHE 8 > > > > > > This is fifth or so definition for the same, isn't it a good time to create > > > a treewide definition in pci.h? > > > > This isn't something defined in the PCI spec but rather an x86-specific > > constant, so should preferrably live somewhere in arch/x86/include/asm/. > > I'm not sure I am following both paragraphs. > > > If you have a "maximum number of PCI functions per device" constant in mind > > then include/uapi/linux/pci.h might be a good fit. > > This is indeed what I have had in mind, but why is this x86 specific? > I didn't get... If you look at it from the angle that you want to cache the BAR of function 0 of the P2SB and of up to 7 additional functions, it's an x86 thing. If you look at it from the angle "how many functions can a PCIe device have (absent ARI)", it's a PCIe thing. It depends on the way you look at it. ;) Lukas
On Fri, Dec 15, 2023 at 04:45:07PM +0100, Lukas Wunner wrote: > On Fri, Dec 15, 2023 at 05:37:01PM +0200, Andy Shevchenko wrote: > > On Fri, Dec 15, 2023 at 08:52:10AM +0100, Lukas Wunner wrote: > > > On Thu, Dec 14, 2023 at 06:38:41PM +0200, Andy Shevchenko wrote: > > > > On Tue, Dec 12, 2023 at 08:47:46PM +0900, Shin'ichiro Kawasaki wrote: > > > > > +/* Cache BAR0 of P2SB device from function 0 ot 7 */ > > > > > +#define NR_P2SB_RES_CACHE 8 > > > > > > > > This is fifth or so definition for the same, isn't it a good time to create > > > > a treewide definition in pci.h? > > > > > > This isn't something defined in the PCI spec but rather an x86-specific > > > constant, so should preferrably live somewhere in arch/x86/include/asm/. > > > > I'm not sure I am following both paragraphs. > > > > > If you have a "maximum number of PCI functions per device" constant in mind > > > then include/uapi/linux/pci.h might be a good fit. > > > > This is indeed what I have had in mind, but why is this x86 specific? > > I didn't get... > > If you look at it from the angle that you want to cache the > BAR of function 0 of the P2SB and of up to 7 additional functions, > it's an x86 thing. > > If you look at it from the angle "how many functions can a PCIe > device have (absent ARI)", it's a PCIe thing. > > It depends on the way you look at it. ;) I look here from the PCI specification / similar thing perspective. My angle here is "cache the BAR of function 0 of P2SB and of up to as many as PCI specification dictates the device may have".
diff --git a/drivers/platform/x86/p2sb.c b/drivers/platform/x86/p2sb.c index 1cf2471d54dd..5037d0387941 100644 --- a/drivers/platform/x86/p2sb.c +++ b/drivers/platform/x86/p2sb.c @@ -26,6 +26,16 @@ static const struct x86_cpu_id p2sb_cpu_ids[] = { {} }; +/* Cache BAR0 of P2SB device from function 0 ot 7 */ +#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,8 +49,13 @@ static int p2sb_get_devfn(unsigned int *devfn) return 0; } +static bool p2sb_invalid_resource(struct resource *res) +{ + return res->flags == 0; +} + /* 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]; @@ -56,48 +71,28 @@ 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 int p2sb_scan_and_cache(struct pci_bus *bus, unsigned int devfn) { struct pci_dev *pdev; - int ret; + struct p2sb_res_cache *cache = &p2sb_resources[PCI_FUNC(devfn)]; pdev = pci_scan_single_device(bus, devfn); - if (!pdev) + if (!pdev || p2sb_invalid_resource(&pdev->resource[0])) return -ENODEV; - 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 0; } -/** - * 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_cache_resources(void) { - struct pci_dev *pdev_p2sb; - unsigned int devfn_p2sb; + struct pci_bus *bus; + unsigned int devfn_p2sb, slot_p2sb, fn; u32 value = P2SBC_HIDE; int ret; @@ -106,8 +101,8 @@ 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); + /* Assume P2SB is on the bus 0 in domain 0 */ + bus = pci_find_bus(0, 0); /* * Prevent concurrent PCI bus scan from seeing the P2SB device and @@ -115,18 +110,31 @@ 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); + /* Scan the P2SB device and cache its BAR0 */ + ret = p2sb_scan_and_cache(bus, devfn_p2sb); + if (ret) + goto out; + /* + * When function number of the P2SB device is zero, scan other function + * numbers. If devices are available, cache their BAR0. + */ + if (!PCI_FUNC(devfn_p2sb)) { + slot_p2sb = PCI_SLOT(devfn_p2sb); + for (fn = 1; fn < 8; fn++) + p2sb_scan_and_cache(bus, PCI_DEVFN(slot_p2sb, fn)); + } + +out: /* Hide the P2SB device, if it was hidden */ if (value & P2SBC_HIDE) pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, P2SBC_HIDE); @@ -136,9 +144,49 @@ int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem) if (ret) return ret; - if (mem->flags == 0) + return 0; +} + +/** + * 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) +{ + int ret; + struct p2sb_res_cache *cache; + + bus = bus ? bus : pci_find_bus(0, 0); + + if (!devfn) { + ret = p2sb_get_devfn(&devfn); + if (ret) + return ret; + } + + cache = &p2sb_resources[PCI_FUNC(devfn)]; + if (p2sb_invalid_resource(&cache->res) || + cache->bus_dev_id != bus->dev.id) return -ENODEV; + 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; +} +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. Change from RFC v1: * Fixed a build warning poitned out in llvm list by kernel test robot drivers/platform/x86/p2sb.c | 128 +++++++++++++++++++++++++----------- 1 file changed, 88 insertions(+), 40 deletions(-)