[PATCH v3 0/4] p2sb: Fix unexpected P2SB device disappearance

[PATCH v3 0/4] p2sb: Fix unexpected P2SB device disappearance

[Shin'ichiro Kawasaki]

When the BIOS does not hide the P2SB device, it is expected to be visible from
userspace. However, the P2SB device disappears since the commit 5913320eb0b3
("platform/x86: p2sb: Allow p2sb_bar() calls during PCI device probe") [1]. This
series addresses the problem. The first three patches are preliminary
refactoring for the fix. The last patch resolves the issue by caching the P2SB
device resources only if the BIOS hides the P2SB device.

[1] https://lore.kernel.org/lkml/ZzTI+biIUTvFT6NC@goliath/

Changes from v2:
* Renamed the global flag from p2sb_hidden to p2sb_hidden_by_bios
* Moved P2SB hide and unhide code to p2sb_scan_and_cache()
* Introduced two helper functions which are called from p2sb_bar()
* Separated the preliminary refactoring work to 3 new patches
* Link to v2: https://lore.kernel.org/platform-driver-x86/20241125042326.304780-1-shinichiro.kawasaki@wdc.com/

Changes from v1:
* Put back P2SBC_HIDE flag reference code in the rescan_remove lock region
* Do not cache resources when the P2SB device is not hidden
* Added the Reported-by tag
* Link to v1: https://lore.kernel.org/platform-driver-x86/20241120064055.245969-1-shinichiro.kawasaki@wdc.com/

Shin'ichiro Kawasaki (4):
  p2sb: Factor out p2sb_read_from_cache()
  p2sb: Introduce the global flag p2sb_hidden_by_bios
  p2sb: Move P2SB hide and unhide code to p2sb_scan_and_cache()
  p2sb: Do not scan and remove the P2SB device when it is unhidden

 drivers/platform/x86/p2sb.c | 77 ++++++++++++++++++++++++++-----------
 1 file changed, 55 insertions(+), 22 deletions(-)

-- 
2.47.0

[PATCH v3 1/4] p2sb: Factor out p2sb_read_from_cache()

[Shin'ichiro Kawasaki]

To prepare for the following fix, factor out the code to read the P2SB
resource from the cache to the new function p2sb_read_from_cache().

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 drivers/platform/x86/p2sb.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/platform/x86/p2sb.c b/drivers/platform/x86/p2sb.c
index 31f38309b389..aa34b8a69bc1 100644
--- a/drivers/platform/x86/p2sb.c
+++ b/drivers/platform/x86/p2sb.c
@@ -171,6 +171,22 @@ static int p2sb_cache_resources(void)
 	return ret;
 }
 
+static int p2sb_read_from_cache(struct pci_bus *bus, unsigned int devfn,
+				struct resource *mem)
+{
+	struct p2sb_res_cache *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;
+}
+
 /**
  * p2sb_bar - Get Primary to Sideband (P2SB) bridge device BAR
  * @bus: PCI bus to communicate with
@@ -187,8 +203,6 @@ static int p2sb_cache_resources(void)
  */
 int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem)
 {
-	struct p2sb_res_cache *cache;
-
 	bus = p2sb_get_bus(bus);
 	if (!bus)
 		return -ENODEV;
@@ -196,15 +210,7 @@ int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem)
 	if (!devfn)
 		p2sb_get_devfn(&devfn);
 
-	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;
+	return p2sb_read_from_cache(bus, devfn, mem);
 }
 EXPORT_SYMBOL_GPL(p2sb_bar);
 
-- 
2.47.0

[PATCH v3 2/4] p2sb: Introduce the global flag p2sb_hidden_by_bios

[Shin'ichiro Kawasaki]

To prepare for the following fix, introduce the global flag
p2sb_hidden_by_bios. Check if the BIOS hides the P2SB device and store
the result in the flag. This allows to refer to the check result across
functions.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 drivers/platform/x86/p2sb.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/p2sb.c b/drivers/platform/x86/p2sb.c
index aa34b8a69bc1..273ac90c8fbd 100644
--- a/drivers/platform/x86/p2sb.c
+++ b/drivers/platform/x86/p2sb.c
@@ -42,6 +42,7 @@ struct p2sb_res_cache {
 };
 
 static struct p2sb_res_cache p2sb_resources[NR_P2SB_RES_CACHE];
+static bool p2sb_hidden_by_bios;
 
 static void p2sb_get_devfn(unsigned int *devfn)
 {
@@ -157,13 +158,14 @@ static int p2sb_cache_resources(void)
 	 * Unhide the P2SB device here, if needed.
 	 */
 	pci_bus_read_config_dword(bus, devfn_p2sb, P2SBC, &value);
-	if (value & P2SBC_HIDE)
+	p2sb_hidden_by_bios = value & P2SBC_HIDE;
+	if (p2sb_hidden_by_bios)
 		pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, 0);
 
 	ret = p2sb_scan_and_cache(bus, devfn_p2sb);
 
 	/* Hide the P2SB device, if it was hidden */
-	if (value & P2SBC_HIDE)
+	if (p2sb_hidden_by_bios)
 		pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, P2SBC_HIDE);
 
 	pci_unlock_rescan_remove();
-- 
2.47.0

Re: [PATCH v3 2/4] p2sb: Introduce the global flag p2sb_hidden_by_bios

[Andy Shevchenko]

On Wed, Nov 27, 2024 at 03:00:53PM +0900, Shin'ichiro Kawasaki wrote:
> To prepare for the following fix, introduce the global flag
> p2sb_hidden_by_bios. Check if the BIOS hides the P2SB device and store
> the result in the flag. This allows to refer to the check result across
> functions.

Thinking about this I'm not sure we have no leftovers that are doing
the hide-unhide dance on their own, behind p2sb_bar()'s back. But
the idea is that all drivers that need this functionality in the kernel
have to work through p2sb_bar() entry point. If any problem is found
in the future with that, those will need to be fixed separately.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 3/4] p2sb: Move P2SB hide and unhide code to p2sb_scan_and_cache()

[Shin'ichiro Kawasaki]

To prepare for the following fix, move the code to hide and unhide the
P2SB device from p2sb_cache_resources() to p2sb_scan_and_cache().

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 drivers/platform/x86/p2sb.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/platform/x86/p2sb.c b/drivers/platform/x86/p2sb.c
index 273ac90c8fbd..0bc6b21c4c20 100644
--- a/drivers/platform/x86/p2sb.c
+++ b/drivers/platform/x86/p2sb.c
@@ -97,6 +97,14 @@ static void p2sb_scan_and_cache_devfn(struct pci_bus *bus, unsigned int devfn)
 
 static int p2sb_scan_and_cache(struct pci_bus *bus, unsigned int devfn)
 {
+	/*
+	 * 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.
+	 */
+	if (p2sb_hidden_by_bios)
+		pci_bus_write_config_dword(bus, devfn, P2SBC, 0);
+
 	/* Scan the P2SB device and cache its BAR0 */
 	p2sb_scan_and_cache_devfn(bus, devfn);
 
@@ -104,6 +112,10 @@ static int p2sb_scan_and_cache(struct pci_bus *bus, unsigned int devfn)
 	if (devfn == P2SB_DEVFN_GOLDMONT)
 		p2sb_scan_and_cache_devfn(bus, SPI_DEVFN_GOLDMONT);
 
+	/* Hide the P2SB device, if it was hidden */
+	if (p2sb_hidden_by_bios)
+		pci_bus_write_config_dword(bus, devfn, P2SBC, P2SBC_HIDE);
+
 	if (!p2sb_valid_resource(&p2sb_resources[PCI_FUNC(devfn)].res))
 		return -ENOENT;
 
@@ -152,22 +164,11 @@ static int p2sb_cache_resources(void)
 	 */
 	pci_lock_rescan_remove();
 
-	/*
-	 * 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);
 	p2sb_hidden_by_bios = value & P2SBC_HIDE;
-	if (p2sb_hidden_by_bios)
-		pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, 0);
 
 	ret = p2sb_scan_and_cache(bus, devfn_p2sb);
 
-	/* Hide the P2SB device, if it was hidden */
-	if (p2sb_hidden_by_bios)
-		pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, P2SBC_HIDE);
-
 	pci_unlock_rescan_remove();
 
 	return ret;
-- 
2.47.0

[PATCH v3 4/4] p2sb: Do not scan and remove the P2SB device when it is unhidden

[Shin'ichiro Kawasaki]

When drivers access P2SB device resources, it calls p2sb_bar(). Before
the commit 5913320eb0b3 ("platform/x86: p2sb: Allow p2sb_bar() calls
during PCI device probe"), p2sb_bar() obtained the resources and then
called pci_stop_and_remove_bus_device() for clean up. Then the P2SB
device disappeared. The commit 5913320eb0b3 introduced the P2SB device
resource cache feature in the boot process. During the resource cache,
pci_stop_and_remove_bus_device() is called for the P2SB device, then the
P2SB device disappears regardless of whether p2sb_bar() is called or
not. Such P2SB device disappearance caused a confusion [1]. To avoid the
confusion, avoid the pci_stop_and_remove_bus_device() call when the BIOS
does not hide the P2SB device.

For that purpose, cache the P2SB device resources only if the BIOS hides
the P2SB device. Call p2sb_scan_and_cache() only if p2sb_hidden_by_bios
is true. This allows removing two branches from p2sb_scan_and_cache().
When p2sb_bar() is called, get the resources from the cache if the P2SB
device is hidden. Otherwise, read the resources from the unhidden P2SB
device.

Reported-by: "Daniel Walker (danielwa)" <danielwa@cisco.com>
Fixes: 5913320eb0b3 ("platform/x86: p2sb: Allow p2sb_bar() calls during PCI device probe")
Closes: https://lore.kernel.org/lkml/ZzTI+biIUTvFT6NC@goliath/ [1]
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 drivers/platform/x86/p2sb.c | 40 +++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/p2sb.c b/drivers/platform/x86/p2sb.c
index 0bc6b21c4c20..1b2d83c4b02a 100644
--- a/drivers/platform/x86/p2sb.c
+++ b/drivers/platform/x86/p2sb.c
@@ -100,10 +100,8 @@ static int p2sb_scan_and_cache(struct pci_bus *bus, unsigned int devfn)
 	/*
 	 * 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.
 	 */
-	if (p2sb_hidden_by_bios)
-		pci_bus_write_config_dword(bus, devfn, P2SBC, 0);
+	pci_bus_write_config_dword(bus, devfn, P2SBC, 0);
 
 	/* Scan the P2SB device and cache its BAR0 */
 	p2sb_scan_and_cache_devfn(bus, devfn);
@@ -112,9 +110,7 @@ static int p2sb_scan_and_cache(struct pci_bus *bus, unsigned int devfn)
 	if (devfn == P2SB_DEVFN_GOLDMONT)
 		p2sb_scan_and_cache_devfn(bus, SPI_DEVFN_GOLDMONT);
 
-	/* Hide the P2SB device, if it was hidden */
-	if (p2sb_hidden_by_bios)
-		pci_bus_write_config_dword(bus, devfn, P2SBC, P2SBC_HIDE);
+	pci_bus_write_config_dword(bus, devfn, P2SBC, P2SBC_HIDE);
 
 	if (!p2sb_valid_resource(&p2sb_resources[PCI_FUNC(devfn)].res))
 		return -ENOENT;
@@ -167,7 +163,12 @@ static int p2sb_cache_resources(void)
 	pci_bus_read_config_dword(bus, devfn_p2sb, P2SBC, &value);
 	p2sb_hidden_by_bios = value & P2SBC_HIDE;
 
-	ret = p2sb_scan_and_cache(bus, devfn_p2sb);
+	/*
+	 * If the BIOS does not hide the P2SB device then its resources
+	 * are accesilble. Cache them only if the P2SB device is hidden.
+	 */
+	if (p2sb_hidden_by_bios)
+		ret = p2sb_scan_and_cache(bus, devfn_p2sb);
 
 	pci_unlock_rescan_remove();
 
@@ -190,6 +191,26 @@ static int p2sb_read_from_cache(struct pci_bus *bus, unsigned int devfn,
 	return 0;
 }
 
+static int p2sb_read_from_dev(struct pci_bus *bus, unsigned int devfn,
+			      struct resource *mem)
+{
+	struct pci_dev *pdev;
+	int ret = 0;
+
+	pdev = pci_get_slot(bus, devfn);
+	if (!pdev)
+		return -ENODEV;
+
+	if (p2sb_valid_resource(pci_resource_n(pdev, 0)))
+		p2sb_read_bar0(pdev, mem);
+	else
+		ret = -ENOENT;
+
+	pci_dev_put(pdev);
+
+	return ret;
+}
+
 /**
  * p2sb_bar - Get Primary to Sideband (P2SB) bridge device BAR
  * @bus: PCI bus to communicate with
@@ -213,7 +234,10 @@ int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem)
 	if (!devfn)
 		p2sb_get_devfn(&devfn);
 
-	return p2sb_read_from_cache(bus, devfn, mem);
+	if (p2sb_hidden_by_bios)
+		return p2sb_read_from_cache(bus, devfn, mem);
+
+	return p2sb_read_from_dev(bus, devfn, mem);
 }
 EXPORT_SYMBOL_GPL(p2sb_bar);
 
-- 
2.47.0

Re: [PATCH v3 4/4] p2sb: Do not scan and remove the P2SB device when it is unhidden

[Hans de Goede]

Hi,

Seems I was a bit too quick with reviewing at a second
look I have found a small issue with this patch.

See my comment below.

On 27-Nov-24 7:00 AM, Shin'ichiro Kawasaki wrote:
> When drivers access P2SB device resources, it calls p2sb_bar(). Before
> the commit 5913320eb0b3 ("platform/x86: p2sb: Allow p2sb_bar() calls
> during PCI device probe"), p2sb_bar() obtained the resources and then
> called pci_stop_and_remove_bus_device() for clean up. Then the P2SB
> device disappeared. The commit 5913320eb0b3 introduced the P2SB device
> resource cache feature in the boot process. During the resource cache,
> pci_stop_and_remove_bus_device() is called for the P2SB device, then the
> P2SB device disappears regardless of whether p2sb_bar() is called or
> not. Such P2SB device disappearance caused a confusion [1]. To avoid the
> confusion, avoid the pci_stop_and_remove_bus_device() call when the BIOS
> does not hide the P2SB device.
> 
> For that purpose, cache the P2SB device resources only if the BIOS hides
> the P2SB device. Call p2sb_scan_and_cache() only if p2sb_hidden_by_bios
> is true. This allows removing two branches from p2sb_scan_and_cache().
> When p2sb_bar() is called, get the resources from the cache if the P2SB
> device is hidden. Otherwise, read the resources from the unhidden P2SB
> device.
> 
> Reported-by: "Daniel Walker (danielwa)" <danielwa@cisco.com>
> Fixes: 5913320eb0b3 ("platform/x86: p2sb: Allow p2sb_bar() calls during PCI device probe")
> Closes: https://lore.kernel.org/lkml/ZzTI+biIUTvFT6NC@goliath/ [1]
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>  drivers/platform/x86/p2sb.c | 40 +++++++++++++++++++++++++++++--------
>  1 file changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/platform/x86/p2sb.c b/drivers/platform/x86/p2sb.c
> index 0bc6b21c4c20..1b2d83c4b02a 100644
> --- a/drivers/platform/x86/p2sb.c
> +++ b/drivers/platform/x86/p2sb.c
> @@ -100,10 +100,8 @@ static int p2sb_scan_and_cache(struct pci_bus *bus, unsigned int devfn)
>  	/*
>  	 * 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.
>  	 */
> -	if (p2sb_hidden_by_bios)
> -		pci_bus_write_config_dword(bus, devfn, P2SBC, 0);
> +	pci_bus_write_config_dword(bus, devfn, P2SBC, 0);
>  
>  	/* Scan the P2SB device and cache its BAR0 */
>  	p2sb_scan_and_cache_devfn(bus, devfn);
> @@ -112,9 +110,7 @@ static int p2sb_scan_and_cache(struct pci_bus *bus, unsigned int devfn)
>  	if (devfn == P2SB_DEVFN_GOLDMONT)
>  		p2sb_scan_and_cache_devfn(bus, SPI_DEVFN_GOLDMONT);
>  
> -	/* Hide the P2SB device, if it was hidden */
> -	if (p2sb_hidden_by_bios)
> -		pci_bus_write_config_dword(bus, devfn, P2SBC, P2SBC_HIDE);
> +	pci_bus_write_config_dword(bus, devfn, P2SBC, P2SBC_HIDE);
>  
>  	if (!p2sb_valid_resource(&p2sb_resources[PCI_FUNC(devfn)].res))
>  		return -ENOENT;
> @@ -167,7 +163,12 @@ static int p2sb_cache_resources(void)
>  	pci_bus_read_config_dword(bus, devfn_p2sb, P2SBC, &value);
>  	p2sb_hidden_by_bios = value & P2SBC_HIDE;
>  
> -	ret = p2sb_scan_and_cache(bus, devfn_p2sb);
> +	/*
> +	 * If the BIOS does not hide the P2SB device then its resources
> +	 * are accesilble. Cache them only if the P2SB device is hidden.
> +	 */
> +	if (p2sb_hidden_by_bios)
> +		ret = p2sb_scan_and_cache(bus, devfn_p2sb);

ret will be returned uninitialized now when p2sb_hidden_by_bios is false,
so this patch also needs to initialize ret to 0 when declaring it.

With this fixed you can keep my Reviewed-by.

Regards,

Hans



>  
>  	pci_unlock_rescan_remove();
>  
> @@ -190,6 +191,26 @@ static int p2sb_read_from_cache(struct pci_bus *bus, unsigned int devfn,
>  	return 0;
>  }
>  
> +static int p2sb_read_from_dev(struct pci_bus *bus, unsigned int devfn,
> +			      struct resource *mem)
> +{
> +	struct pci_dev *pdev;
> +	int ret = 0;
> +
> +	pdev = pci_get_slot(bus, devfn);
> +	if (!pdev)
> +		return -ENODEV;
> +
> +	if (p2sb_valid_resource(pci_resource_n(pdev, 0)))
> +		p2sb_read_bar0(pdev, mem);
> +	else
> +		ret = -ENOENT;
> +
> +	pci_dev_put(pdev);
> +
> +	return ret;
> +}
> +
>  /**
>   * p2sb_bar - Get Primary to Sideband (P2SB) bridge device BAR
>   * @bus: PCI bus to communicate with
> @@ -213,7 +234,10 @@ int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem)
>  	if (!devfn)
>  		p2sb_get_devfn(&devfn);
>  
> -	return p2sb_read_from_cache(bus, devfn, mem);
> +	if (p2sb_hidden_by_bios)
> +		return p2sb_read_from_cache(bus, devfn, mem);
> +
> +	return p2sb_read_from_dev(bus, devfn, mem);
>  }
>  EXPORT_SYMBOL_GPL(p2sb_bar);
>  

Re: [PATCH v3 4/4] p2sb: Do not scan and remove the P2SB device when it is unhidden

[Shinichiro Kawasaki]

On Nov 27, 2024 / 10:55, Hans de Goede wrote:
> Hi,
> 
> Seems I was a bit too quick with reviewing at a second
> look I have found a small issue with this patch.
> 
> See my comment below.
> 
> On 27-Nov-24 7:00 AM, Shin'ichiro Kawasaki wrote:
[...]
> > @@ -167,7 +163,12 @@ static int p2sb_cache_resources(void)
> >  	pci_bus_read_config_dword(bus, devfn_p2sb, P2SBC, &value);
> >  	p2sb_hidden_by_bios = value & P2SBC_HIDE;
> >  
> > -	ret = p2sb_scan_and_cache(bus, devfn_p2sb);
> > +	/*
> > +	 * If the BIOS does not hide the P2SB device then its resources
> > +	 * are accesilble. Cache them only if the P2SB device is hidden.
> > +	 */
> > +	if (p2sb_hidden_by_bios)
> > +		ret = p2sb_scan_and_cache(bus, devfn_p2sb);
> 
> ret will be returned uninitialized now when p2sb_hidden_by_bios is false,
> so this patch also needs to initialize ret to 0 when declaring it.

Ah, right. Will fix it in v4. I compile tested with KCFLAGS=-Wall and expected
it would catch such mistakes, but it didn't. I found that -Wmaybe-uninitialized
does the check. Will use this check for my future patches.

> 
> With this fixed you can keep my Reviewed-by.

Thanks! Will send out v4 soon.

Re: [PATCH v3 4/4] p2sb: Do not scan and remove the P2SB device when it is unhidden

[Andy Shevchenko]

On Thu, Nov 28, 2024 at 12:26:05AM +0000, Shinichiro Kawasaki wrote:
> On Nov 27, 2024 / 10:55, Hans de Goede wrote:
> > On 27-Nov-24 7:00 AM, Shin'ichiro Kawasaki wrote:

[...]

> > > +	if (p2sb_hidden_by_bios)
> > > +		ret = p2sb_scan_and_cache(bus, devfn_p2sb);
> > 
> > ret will be returned uninitialized now when p2sb_hidden_by_bios is false,
> > so this patch also needs to initialize ret to 0 when declaring it.
> 
> Ah, right. Will fix it in v4. I compile tested with KCFLAGS=-Wall and expected
> it would catch such mistakes, but it didn't. I found that -Wmaybe-uninitialized
> does the check. Will use this check for my future patches.

Just use what kernel Kbuild provides already to you with carefully selected
warnings, i.e.

	`make W=1 ...`

without any need to hack KCFLAGS or anything else.

FYI, the above mentioned warning is included in level 1 of Linux kernel Kbuild
W facility. But if you want much more, there are level 2 and IIRC 3, but I'm
not sure.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/4] p2sb: Do not scan and remove the P2SB device when it is unhidden

[Shinichiro Kawasaki]

On Nov 28, 2024 / 14:59, Andy Shevchenko wrote:
> On Thu, Nov 28, 2024 at 12:26:05AM +0000, Shinichiro Kawasaki wrote:
> > On Nov 27, 2024 / 10:55, Hans de Goede wrote:
> > > On 27-Nov-24 7:00 AM, Shin'ichiro Kawasaki wrote:
> 
> [...]
> 
> > > > +	if (p2sb_hidden_by_bios)
> > > > +		ret = p2sb_scan_and_cache(bus, devfn_p2sb);
> > > 
> > > ret will be returned uninitialized now when p2sb_hidden_by_bios is false,
> > > so this patch also needs to initialize ret to 0 when declaring it.
> > 
> > Ah, right. Will fix it in v4. I compile tested with KCFLAGS=-Wall and expected
> > it would catch such mistakes, but it didn't. I found that -Wmaybe-uninitialized
> > does the check. Will use this check for my future patches.
> 
> Just use what kernel Kbuild provides already to you with carefully selected
> warnings, i.e.
> 
> 	`make W=1 ...`
> 
> without any need to hack KCFLAGS or anything else.
> 
> FYI, the above mentioned warning is included in level 1 of Linux kernel Kbuild
> W facility. But if you want much more, there are level 2 and IIRC 3, but I'm
> not sure.

Thanks! 'make W=n' is handier than KCFLAGS. I found 'make help' describes W=n,
and it says n can be 1, 2 or 3. Will use them in the future.

Re: [PATCH v3 0/4] p2sb: Fix unexpected P2SB device disappearance

[Hans de Goede]

Hi,

On 27-Nov-24 7:00 AM, Shin'ichiro Kawasaki wrote:
> When the BIOS does not hide the P2SB device, it is expected to be visible from
> userspace. However, the P2SB device disappears since the commit 5913320eb0b3
> ("platform/x86: p2sb: Allow p2sb_bar() calls during PCI device probe") [1]. This
> series addresses the problem. The first three patches are preliminary
> refactoring for the fix. The last patch resolves the issue by caching the P2SB
> device resources only if the BIOS hides the P2SB device.
> 
> [1] https://lore.kernel.org/lkml/ZzTI+biIUTvFT6NC@goliath/
> 
> Changes from v2:
> * Renamed the global flag from p2sb_hidden to p2sb_hidden_by_bios
> * Moved P2SB hide and unhide code to p2sb_scan_and_cache()
> * Introduced two helper functions which are called from p2sb_bar()
> * Separated the preliminary refactoring work to 3 new patches
> * Link to v2: https://lore.kernel.org/platform-driver-x86/20241125042326.304780-1-shinichiro.kawasaki@wdc.com/

Thank you for all your work on this.

This new split out series is much easier to review, much appreciated.

All patches look good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

for the series.

Regards,

Hans





> Changes from v1:
> * Put back P2SBC_HIDE flag reference code in the rescan_remove lock region
> * Do not cache resources when the P2SB device is not hidden
> * Added the Reported-by tag
> * Link to v1: https://lore.kernel.org/platform-driver-x86/20241120064055.245969-1-shinichiro.kawasaki@wdc.com/
> 
> Shin'ichiro Kawasaki (4):
>   p2sb: Factor out p2sb_read_from_cache()
>   p2sb: Introduce the global flag p2sb_hidden_by_bios
>   p2sb: Move P2SB hide and unhide code to p2sb_scan_and_cache()
>   p2sb: Do not scan and remove the P2SB device when it is unhidden
> 
>  drivers/platform/x86/p2sb.c | 77 ++++++++++++++++++++++++++-----------
>  1 file changed, 55 insertions(+), 22 deletions(-)
>