All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/1] i2c: i801: Restore the presence state of P2SB PCI device after reading BAR
@ 2017-08-14 16:04 Qiuxu Zhuo
  2017-08-15  7:13 ` Mika Westerberg
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Qiuxu Zhuo @ 2017-08-14 16:04 UTC (permalink / raw)
  To: jdelvare, wsa; +Cc: tony.luck, mika.westerberg, bp, linux-i2c, Qiuxu Zhuo

Sun, Yunying reported the following failure on Denverton micro-server:

 EDAC DEBUG: pnd2_init:
 EDAC DEBUG: pnd2_probe:
 EDAC DEBUG: dnv_rd_reg: Read b_cr_tolud_pci=00000000_80000000
 EDAC DEBUG: dnv_rd_reg: Read b_cr_touud_lo_pci=00000000_80000000
 EDAC DEBUG: dnv_rd_reg: Read b_cr_touud_hi_pci=00000000_00000004
 EDAC DEBUG: dnv_rd_reg: Read b_cr_asym_mem_region0_mchbar=00000000_00000000
 EDAC DEBUG: dnv_rd_reg: Read b_cr_asym_mem_region1_mchbar=00000000_00000000
 EDAC DEBUG: dnv_rd_reg: Read b_cr_mot_out_base_mchbar=00000000_00000000
 EDAC DEBUG: dnv_rd_reg: Read b_cr_mot_out_mask_mchbar=00000000_00000000
 EDAC pnd2: Failed to register device with error -19.

On Denverton micro-server, the presence of the P2SB bridge PCI device is
enabled or disabled by the item 'RelaxSecConf' in BIOS setup menu. When
'RelaxSecConf' is enabled, the P2SB PCI device is present and the pnd2_edac
EDAC driver also uses it to get BAR. Hiding the P2SB PCI device caused the
pnd2_edac EDAC driver failed to get BAR then reported the above failure.

Therefor, store the presence state of P2SB PCI device before unhiding it
for reading BAR and restore the presence state after reading BAR.

Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
Reported-and-tested-by: Yunying Sun <yunying.sun@intel.com>
---
 drivers/i2c/busses/i2c-i801.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index c9536e1..e114e4e 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1332,6 +1332,7 @@ static void i801_add_tco(struct i801_priv *priv)
 	u32 tco_base, tco_ctl;
 	u32 base_addr, ctrl_val;
 	u64 base64_addr;
+	u8 hidden;
 
 	if (!(priv->features & FEATURE_TCO))
 		return;
@@ -1376,8 +1377,10 @@ static void i801_add_tco(struct i801_priv *priv)
 
 	devfn = PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 1);
 
-	/* Unhide the P2SB device */
-	pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, 0x0);
+	/* Unhide the P2SB device, if it is hidden */
+	pci_bus_read_config_byte(pci_dev->bus, devfn, 0xe1, &hidden);
+	if (hidden)
+		pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, 0x0);
 
 	pci_bus_read_config_dword(pci_dev->bus, devfn, SBREG_BAR, &base_addr);
 	base64_addr = base_addr & 0xfffffff0;
@@ -1385,8 +1388,9 @@ static void i801_add_tco(struct i801_priv *priv)
 	pci_bus_read_config_dword(pci_dev->bus, devfn, SBREG_BAR + 0x4, &base_addr);
 	base64_addr |= (u64)base_addr << 32;
 
-	/* Hide the P2SB device */
-	pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, 0x1);
+	/* Hide the P2SB device, if it was hidden before */
+	if (hidden)
+		pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, hidden);
 	spin_unlock(&p2sb_spinlock);
 
 	res = &tco_res[ICH_RES_MEM_OFF];
-- 
2.9.0.GIT

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] i2c: i801: Restore the presence state of P2SB PCI device after reading BAR
  2017-08-14 16:04 [PATCH 1/1] i2c: i801: Restore the presence state of P2SB PCI device after reading BAR Qiuxu Zhuo
@ 2017-08-15  7:13 ` Mika Westerberg
  2017-08-17 16:05 ` Wolfram Sang
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Mika Westerberg @ 2017-08-15  7:13 UTC (permalink / raw)
  To: Qiuxu Zhuo; +Cc: jdelvare, wsa, tony.luck, bp, linux-i2c

On Tue, Aug 15, 2017 at 12:04:50AM +0800, Qiuxu Zhuo wrote:
> Sun, Yunying reported the following failure on Denverton micro-server:
> 
>  EDAC DEBUG: pnd2_init:
>  EDAC DEBUG: pnd2_probe:
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_tolud_pci=00000000_80000000
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_touud_lo_pci=00000000_80000000
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_touud_hi_pci=00000000_00000004
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_asym_mem_region0_mchbar=00000000_00000000
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_asym_mem_region1_mchbar=00000000_00000000
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_mot_out_base_mchbar=00000000_00000000
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_mot_out_mask_mchbar=00000000_00000000
>  EDAC pnd2: Failed to register device with error -19.
> 
> On Denverton micro-server, the presence of the P2SB bridge PCI device is
> enabled or disabled by the item 'RelaxSecConf' in BIOS setup menu. When
> 'RelaxSecConf' is enabled, the P2SB PCI device is present and the pnd2_edac
> EDAC driver also uses it to get BAR. Hiding the P2SB PCI device caused the
> pnd2_edac EDAC driver failed to get BAR then reported the above failure.
> 
> Therefor, store the presence state of P2SB PCI device before unhiding it
> for reading BAR and restore the presence state after reading BAR.
> 
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Reported-and-tested-by: Yunying Sun <yunying.sun@intel.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] i2c: i801: Restore the presence state of P2SB PCI device after reading BAR
  2017-08-14 16:04 [PATCH 1/1] i2c: i801: Restore the presence state of P2SB PCI device after reading BAR Qiuxu Zhuo
  2017-08-15  7:13 ` Mika Westerberg
@ 2017-08-17 16:05 ` Wolfram Sang
  2017-08-22  8:48   ` Zhuo, Qiuxu
  2017-08-28 15:33 ` Wolfram Sang
  2017-10-13 13:01 ` Jean Delvare
  3 siblings, 1 reply; 8+ messages in thread
From: Wolfram Sang @ 2017-08-17 16:05 UTC (permalink / raw)
  To: Qiuxu Zhuo; +Cc: jdelvare, tony.luck, mika.westerberg, bp, linux-i2c

[-- Attachment #1: Type: text/plain, Size: 1488 bytes --]

On Tue, Aug 15, 2017 at 12:04:50AM +0800, Qiuxu Zhuo wrote:
> Sun, Yunying reported the following failure on Denverton micro-server:
> 
>  EDAC DEBUG: pnd2_init:
>  EDAC DEBUG: pnd2_probe:
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_tolud_pci=00000000_80000000
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_touud_lo_pci=00000000_80000000
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_touud_hi_pci=00000000_00000004
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_asym_mem_region0_mchbar=00000000_00000000
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_asym_mem_region1_mchbar=00000000_00000000
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_mot_out_base_mchbar=00000000_00000000
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_mot_out_mask_mchbar=00000000_00000000
>  EDAC pnd2: Failed to register device with error -19.
> 
> On Denverton micro-server, the presence of the P2SB bridge PCI device is
> enabled or disabled by the item 'RelaxSecConf' in BIOS setup menu. When
> 'RelaxSecConf' is enabled, the P2SB PCI device is present and the pnd2_edac
> EDAC driver also uses it to get BAR. Hiding the P2SB PCI device caused the
> pnd2_edac EDAC driver failed to get BAR then reported the above failure.
> 
> Therefor, store the presence state of P2SB PCI device before unhiding it
> for reading BAR and restore the presence state after reading BAR.
> 
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Reported-and-tested-by: Yunying Sun <yunying.sun@intel.com>

Looks good to me, but I'll give Jean a chance to comment...


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH 1/1] i2c: i801: Restore the presence state of P2SB PCI device after reading BAR
  2017-08-17 16:05 ` Wolfram Sang
@ 2017-08-22  8:48   ` Zhuo, Qiuxu
  0 siblings, 0 replies; 8+ messages in thread
From: Zhuo, Qiuxu @ 2017-08-22  8:48 UTC (permalink / raw)
  To: Wolfram Sang, jdelvare; +Cc: Luck, Tony, Westerberg, Mika, bp, linux-i2c

> From: Wolfram Sang [mailto:wsa@the-dreams.de]
... 
> Looks good to me, but I'll give Jean a chance to comment...

Hi Wolfram Sang,
   Thanks for your review!

Hi Jean,

    Would you please review/comment this patch?
    Thanks!

- Qiuxu

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] i2c: i801: Restore the presence state of P2SB PCI device after reading BAR
  2017-08-14 16:04 [PATCH 1/1] i2c: i801: Restore the presence state of P2SB PCI device after reading BAR Qiuxu Zhuo
  2017-08-15  7:13 ` Mika Westerberg
  2017-08-17 16:05 ` Wolfram Sang
@ 2017-08-28 15:33 ` Wolfram Sang
  2017-08-29  1:13   ` Zhuo, Qiuxu
  2017-10-13 13:01 ` Jean Delvare
  3 siblings, 1 reply; 8+ messages in thread
From: Wolfram Sang @ 2017-08-28 15:33 UTC (permalink / raw)
  To: Qiuxu Zhuo; +Cc: jdelvare, tony.luck, mika.westerberg, bp, linux-i2c

[-- Attachment #1: Type: text/plain, Size: 1631 bytes --]

On Tue, Aug 15, 2017 at 12:04:50AM +0800, Qiuxu Zhuo wrote:
> Sun, Yunying reported the following failure on Denverton micro-server:
> 
>  EDAC DEBUG: pnd2_init:
>  EDAC DEBUG: pnd2_probe:
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_tolud_pci=00000000_80000000
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_touud_lo_pci=00000000_80000000
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_touud_hi_pci=00000000_00000004
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_asym_mem_region0_mchbar=00000000_00000000
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_asym_mem_region1_mchbar=00000000_00000000
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_mot_out_base_mchbar=00000000_00000000
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_mot_out_mask_mchbar=00000000_00000000
>  EDAC pnd2: Failed to register device with error -19.
> 
> On Denverton micro-server, the presence of the P2SB bridge PCI device is
> enabled or disabled by the item 'RelaxSecConf' in BIOS setup menu. When
> 'RelaxSecConf' is enabled, the P2SB PCI device is present and the pnd2_edac
> EDAC driver also uses it to get BAR. Hiding the P2SB PCI device caused the
> pnd2_edac EDAC driver failed to get BAR then reported the above failure.
> 
> Therefor, store the presence state of P2SB PCI device before unhiding it
> for reading BAR and restore the presence state after reading BAR.
> 
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Reported-and-tested-by: Yunying Sun <yunying.sun@intel.com>

Applied to for-next, thanks! I don't want to apply it to for-current
or stable without Jean's ack. So, let's get more test coverage first. It
can still be backported later. Hope this is fine with you.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH 1/1] i2c: i801: Restore the presence state of P2SB PCI device after reading BAR
  2017-08-28 15:33 ` Wolfram Sang
@ 2017-08-29  1:13   ` Zhuo, Qiuxu
  0 siblings, 0 replies; 8+ messages in thread
From: Zhuo, Qiuxu @ 2017-08-29  1:13 UTC (permalink / raw)
  To: Wolfram Sang; +Cc: jdelvare, Luck, Tony, Westerberg, Mika, bp, linux-i2c

> From: Wolfram Sang [mailto:wsa@the-dreams.de]
> Sent: Monday, August 28, 2017 11:34 PM
... 
> Applied to for-next, thanks! I don't want to apply it to for-current
> or stable without Jean's ack. So, let's get more test coverage first. It
> can still be backported later. Hope this is fine with you.

It's fine for me. Thanks Wolfram Sang!

- Qiuxu 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] i2c: i801: Restore the presence state of P2SB PCI device after reading BAR
  2017-08-14 16:04 [PATCH 1/1] i2c: i801: Restore the presence state of P2SB PCI device after reading BAR Qiuxu Zhuo
                   ` (2 preceding siblings ...)
  2017-08-28 15:33 ` Wolfram Sang
@ 2017-10-13 13:01 ` Jean Delvare
  2017-10-16  5:31   ` Zhuo, Qiuxu
  3 siblings, 1 reply; 8+ messages in thread
From: Jean Delvare @ 2017-10-13 13:01 UTC (permalink / raw)
  To: Qiuxu Zhuo; +Cc: wsa, tony.luck, mika.westerberg, bp, linux-i2c

On Tue, 15 Aug 2017 00:04:50 +0800, Qiuxu Zhuo wrote:
> Sun, Yunying reported the following failure on Denverton micro-server:
> 
>  EDAC DEBUG: pnd2_init:
>  EDAC DEBUG: pnd2_probe:
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_tolud_pci=00000000_80000000
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_touud_lo_pci=00000000_80000000
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_touud_hi_pci=00000000_00000004
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_asym_mem_region0_mchbar=00000000_00000000
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_asym_mem_region1_mchbar=00000000_00000000
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_mot_out_base_mchbar=00000000_00000000
>  EDAC DEBUG: dnv_rd_reg: Read b_cr_mot_out_mask_mchbar=00000000_00000000
>  EDAC pnd2: Failed to register device with error -19.
> 
> On Denverton micro-server, the presence of the P2SB bridge PCI device is
> enabled or disabled by the item 'RelaxSecConf' in BIOS setup menu. When
> 'RelaxSecConf' is enabled, the P2SB PCI device is present and the pnd2_edac
> EDAC driver also uses it to get BAR. Hiding the P2SB PCI device caused the
> pnd2_edac EDAC driver failed to get BAR then reported the above failure.
> 
> Therefor, store the presence state of P2SB PCI device before unhiding it
> for reading BAR and restore the presence state after reading BAR.
> 
> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> Reported-and-tested-by: Yunying Sun <yunying.sun@intel.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index c9536e1..e114e4e 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1332,6 +1332,7 @@ static void i801_add_tco(struct i801_priv *priv)
>  	u32 tco_base, tco_ctl;
>  	u32 base_addr, ctrl_val;
>  	u64 base64_addr;
> +	u8 hidden;
>  
>  	if (!(priv->features & FEATURE_TCO))
>  		return;
> @@ -1376,8 +1377,10 @@ static void i801_add_tco(struct i801_priv *priv)
>  
>  	devfn = PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 1);
>  
> -	/* Unhide the P2SB device */
> -	pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, 0x0);
> +	/* Unhide the P2SB device, if it is hidden */
> +	pci_bus_read_config_byte(pci_dev->bus, devfn, 0xe1, &hidden);
> +	if (hidden)
> +		pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, 0x0);
>  
>  	pci_bus_read_config_dword(pci_dev->bus, devfn, SBREG_BAR, &base_addr);
>  	base64_addr = base_addr & 0xfffffff0;
> @@ -1385,8 +1388,9 @@ static void i801_add_tco(struct i801_priv *priv)
>  	pci_bus_read_config_dword(pci_dev->bus, devfn, SBREG_BAR + 0x4, &base_addr);
>  	base64_addr |= (u64)base_addr << 32;
>  
> -	/* Hide the P2SB device */
> -	pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, 0x1);
> +	/* Hide the P2SB device, if it was hidden before */
> +	if (hidden)
> +		pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, hidden);
>  	spin_unlock(&p2sb_spinlock);
>  
>  	res = &tco_res[ICH_RES_MEM_OFF];

Sorry, this happened while I was on vacation and I missed it.

The change looks sane, so:

Reviewed-by: Jean Delvare <jdelvare@suse.de>

And I approve backporting to older kernels if needed.

That being said... Looking at the code, I have to say I'm surprised
that the whole value of configuration byte 0xe1 is used as true/false.
I'd expect a single bit (bit 0) in this configuration file to decide if
the P2SB device is hidden or visible. So it seems to me that we are
trashing 7 bits in the process. These bits may be unused at the moment,
but could be used in future devices, and then unexpected behavior will
appear.

I'm still approving this patch because the same problem was present
before, but I think this should be checked, and fixed if I'm right.
I'll be happy to do it myself if someone at Intel points me to the
Denverton datasheet - if it is public.

-- 
Jean Delvare
SUSE L3 Support

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH 1/1] i2c: i801: Restore the presence state of P2SB PCI device after reading BAR
  2017-10-13 13:01 ` Jean Delvare
@ 2017-10-16  5:31   ` Zhuo, Qiuxu
  0 siblings, 0 replies; 8+ messages in thread
From: Zhuo, Qiuxu @ 2017-10-16  5:31 UTC (permalink / raw)
  To: Jean Delvare; +Cc: wsa, Luck, Tony, Westerberg, Mika, bp, linux-i2c

> From: Jean Delvare [mailto:jdelvare@suse.de]
... 
> Sorry, this happened while I was on vacation and I missed it.
> 
> The change looks sane, so:
> 
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
> 
> And I approve backporting to older kernels if needed.
> 
> That being said... Looking at the code, I have to say I'm surprised that the whole
> value of configuration byte 0xe1 is used as true/false.
> I'd expect a single bit (bit 0) in this configuration file to decide if the P2SB device
> is hidden or visible. So it seems to me that we are trashing 7 bits in the process.
> These bits may be unused at the moment, but could be used in future devices,
> and then unexpected behavior will appear.
> 
> I'm still approving this patch because the same problem was present before, but
> I think this should be checked, and fixed if I'm right.
> I'll be happy to do it myself if someone at Intel points me to the Denverton
> datasheet - if it is public.

Hi Jean,
    Yes, bit[0] is to decide the visibility of P2SB and bits[1-7] are reserved. 
    The internal datasheet (I didn't find the public spec) says that hardware will not allocate the reserved bits[1-7] for any purpose in the future. And it can always write to bit[0] without to remember any other field values. 
    But I agree you that using a single bit[0] would be better :-) 

Thanks!
- Qiuxu

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2017-10-16  5:31 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-14 16:04 [PATCH 1/1] i2c: i801: Restore the presence state of P2SB PCI device after reading BAR Qiuxu Zhuo
2017-08-15  7:13 ` Mika Westerberg
2017-08-17 16:05 ` Wolfram Sang
2017-08-22  8:48   ` Zhuo, Qiuxu
2017-08-28 15:33 ` Wolfram Sang
2017-08-29  1:13   ` Zhuo, Qiuxu
2017-10-13 13:01 ` Jean Delvare
2017-10-16  5:31   ` Zhuo, Qiuxu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.