Skip to contentSkip to conversation

sp5100_tco: properly check for new register layouts

PATCH In review Lucas Stach 4 participants 4 messages May 3, 2016 – May 14, 2016 Watchdog device drivers

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 sp5100_tco: properly check for new register layouts

Commits 190aa4304de6 (Add AMD Mullins platform support) and cca118fa2a0a94 (Add AMD Carrizo platform support) enabled the driver on a lot more devices, but the following commit missed a single location in the code when checking if the SB800 register offsets should be used. This leads to the wrong register being written which in turn causes ACPI to go haywire.

Fix this by introducing a helper function to check for the new register layout and use this consistently.

https://bugzilla.kernel.org/show_bug.cgi?id=114201
https://bugzilla.redhat.com/show_bug.cgi?id=1329910

Fixes bdecfcdb5461 (sp5100_tco: fix the device check for SB800

and later chipsets)

---
 drivers/watchdog/sp5100_tco.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)
drivers/watchdog/sp5100_tco.c +10 -5
index 6467b91..028618c 100644
@@ -73,6 +73,13 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started."
 /*
  * Some TCO specific functions
  */
+
+static bool tco_has_sp5100_reg_layout(struct pci_dev *dev)
+{
+	return dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
+	       dev->revision < 0x40;
+}
+
 static void tco_timer_start(void)
 {
 	u32 val;
@@ -129,7 +136,7 @@ static void tco_timer_enable(void)
 {
 	int val;
 
-	if (sp5100_tco_pci->revision >= 0x40) {
+	if (!tco_has_sp5100_reg_layout(sp5100_tco_pci)) {
 		/* For SB800 or later */
 		/* Set the Watchdog timer resolution to 1 sec */
 		outb(SB800_PM_WATCHDOG_CONFIG, SB800_IO_PM_INDEX_REG);
@@ -342,8 +349,7 @@ static unsigned char sp5100_tco_setupdevice(void)
 	/*
 	 * Determine type of southbridge chipset.
 	 */
-	if (sp5100_tco_pci->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
-	    sp5100_tco_pci->revision < 0x40) {
+	if (tco_has_sp5100_reg_layout(sp5100_tco_pci)) {
 		dev_name = SP5100_DEVNAME;
 		index_reg = SP5100_IO_PM_INDEX_REG;
 		data_reg = SP5100_IO_PM_DATA_REG;
@@ -388,8 +394,7 @@ static unsigned char sp5100_tco_setupdevice(void)
 	 * Secondly, Find the watchdog timer MMIO address
 	 * from SBResource_MMIO register.
 	 */
-	if (sp5100_tco_pci->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
-	    sp5100_tco_pci->revision < 0x40) {
+	if (tco_has_sp5100_reg_layout(sp5100_tco_pci)) {
 		/* Read SBResource_MMIO from PCI config(PCI_Reg: 9Ch) */
 		pci_read_config_dword(sp5100_tco_pci,
 				      SP5100_SB_RESOURCE_MMIO_BASE, &val);
Signature · 1 line
2.5.5
maintainer Reviewed-by on v1 1/1
#1 lore raw
Quoted 10 lines · Lucas Stach

Commits 190aa4304de6 (Add AMD Mullins platform support) and cca118fa2a0a94 (Add AMD Carrizo platform support) enabled the driver on a lot more devices, but the following commit missed a single location in the code when checking if the SB800 register offsets should be used. This leads to the wrong register being written which in turn causes ACPI to go haywire.

Fix this by introducing a helper function to check for the new register layout and use this consistently.

Quoted 6 lines

https://bugzilla.kernel.org/show_bug.cgi?id=114201
https://bugzilla.redhat.com/show_bug.cgi?id=1329910

Fixes bdecfcdb5461 (sp5100_tco: fix the device check for SB800

and later chipsets)

Quoted 52 lines
---
  drivers/watchdog/sp5100_tco.c | 15 ++++++++++-----
  1 file changed, 10 insertions(+), 5 deletions(-)
drivers/watchdog/sp5100_tco.c +10 -5
index 6467b91..028618c 100644
@@ -73,6 +73,13 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started."
  /*
   * Some TCO specific functions
   */
+
+static bool tco_has_sp5100_reg_layout(struct pci_dev *dev)
+{
+	return dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
+	       dev->revision < 0x40;
+}
+
  static void tco_timer_start(void)
  {
  	u32 val;
@@ -129,7 +136,7 @@ static void tco_timer_enable(void)
  {
  	int val;
 
-	if (sp5100_tco_pci->revision >= 0x40) {
+	if (!tco_has_sp5100_reg_layout(sp5100_tco_pci)) {
  		/* For SB800 or later */
  		/* Set the Watchdog timer resolution to 1 sec */
  		outb(SB800_PM_WATCHDOG_CONFIG, SB800_IO_PM_INDEX_REG);
@@ -342,8 +349,7 @@ static unsigned char sp5100_tco_setupdevice(void)
  	/*
  	 * Determine type of southbridge chipset.
  	 */
-	if (sp5100_tco_pci->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
-	    sp5100_tco_pci->revision < 0x40) {
+	if (tco_has_sp5100_reg_layout(sp5100_tco_pci)) {
  		dev_name = SP5100_DEVNAME;
  		index_reg = SP5100_IO_PM_INDEX_REG;
  		data_reg = SP5100_IO_PM_DATA_REG;
@@ -388,8 +394,7 @@ static unsigned char sp5100_tco_setupdevice(void)
  	 * Secondly, Find the watchdog timer MMIO address
  	 * from SBResource_MMIO register.
  	 */
-	if (sp5100_tco_pci->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
-	    sp5100_tco_pci->revision < 0x40) {
+	if (tco_has_sp5100_reg_layout(sp5100_tco_pci)) {
  		/* Read SBResource_MMIO from PCI config(PCI_Reg: 9Ch) */
  		pci_read_config_dword(sp5100_tco_pci,
  				      SP5100_SB_RESOURCE_MMIO_BASE, &val);
#2 lore raw
Quoted 12 lines · Guenter Roeck
Quoted 10 lines · Lucas Stach

Commits 190aa4304de6 (Add AMD Mullins platform support) and cca118fa2a0a94 (Add AMD Carrizo platform support) enabled the driver on a lot more devices, but the following commit missed a single location in the code when checking if the SB800 register offsets should be used. This leads to the wrong register being written which in turn causes ACPI to go haywire.

Fix this by introducing a helper function to check for the new register layout and use this consistently.

Quoted 8 lines
Quoted 6 lines

https://bugzilla.kernel.org/show_bug.cgi?id=114201
https://bugzilla.redhat.com/show_bug.cgi?id=1329910

Fixes bdecfcdb5461 (sp5100_tco: fix the device check for SB800

and later chipsets)

What is the status of this patch? I still can't find it in linux-next.

We've got more and more bug reports about this, so would love to get this resolved soonish.

thanks,

Takashi

#3 lore raw
In reply to Lucas Stach

Hi Lucas,

Quoted 12 lines

Commits 190aa4304de6 (Add AMD Mullins platform support) and cca118fa2a0a94 (Add AMD Carrizo platform support) enabled the driver on a lot more devices, but the following commit missed a single location in the code when checking if the SB800 register offsets should be used. This leads to the wrong register being written which in turn causes ACPI to go haywire.

Fix this by introducing a helper function to check for the new register layout and use this consistently.

https://bugzilla.kernel.org/show_bug.cgi?id=114201
https://bugzilla.redhat.com/show_bug.cgi?id=1329910

Quoted 8 lines
Fixes bdecfcdb5461 (sp5100_tco: fix the device check for SB800

and later chipsets)

---
 drivers/watchdog/sp5100_tco.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

This patch has been added to linux-watchdog-next.

Kind regards,
Wim.