[PATCH 0/3] i2c: i801: Fix iTCO_wdt resource creation if PMC is not present

[PATCH 0/3] i2c: i801: Fix iTCO_wdt resource creation if PMC is not present

[Mika Westerberg]

Hi all,

This series aims to fix the issue reported by Martin Volf [1] that prevents
the nct6775 driver from loading.

I added Fixes tag to the last patch but not stable tag because the other
two patches it depends are not really stable material IMO. Please let me
know if there is a better way to organize these :)

[1] https://lore.kernel.org/linux-hwmon/CAM1AHpQ4196tyD=HhBu-2donSsuogabkfP03v1YF26Q7_BgvgA@mail.gmail.com/

Mika Westerberg (3):
  watchdog: iTCO_wdt: Export vendorsupport
  watchdog: iTCO_wdt: Make ICH_RES_IO_SMI optional
  i2c: i801: Do not add ICH_RES_IO_SMI if PMC device is not present

 drivers/i2c/busses/i2c-i801.c          | 45 +++++++++++++++-----------
 drivers/watchdog/iTCO_vendor.h         |  2 ++
 drivers/watchdog/iTCO_vendor_support.c | 16 +++++----
 drivers/watchdog/iTCO_wdt.c            | 22 +++++++------
 4 files changed, 51 insertions(+), 34 deletions(-)

-- 
2.25.0

[PATCH 1/3] watchdog: iTCO_wdt: Export vendorsupport

[Mika Westerberg]

In preparation for making ->smi_res optional the iTCO_wdt driver needs
to know whether vendorsupport is being set to non-zero. For this reason
export the variable.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/watchdog/iTCO_vendor.h         |  2 ++
 drivers/watchdog/iTCO_vendor_support.c | 16 +++++++++-------
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/watchdog/iTCO_vendor.h b/drivers/watchdog/iTCO_vendor.h
index 0f7373ba10d5..69e92e692ae0 100644
--- a/drivers/watchdog/iTCO_vendor.h
+++ b/drivers/watchdog/iTCO_vendor.h
@@ -1,10 +1,12 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /* iTCO Vendor Specific Support hooks */
 #ifdef CONFIG_ITCO_VENDOR_SUPPORT
+extern int iTCO_vendorsupport;
 extern void iTCO_vendor_pre_start(struct resource *, unsigned int);
 extern void iTCO_vendor_pre_stop(struct resource *);
 extern int iTCO_vendor_check_noreboot_on(void);
 #else
+#define iTCO_vendorsupport				0
 #define iTCO_vendor_pre_start(acpibase, heartbeat)	{}
 #define iTCO_vendor_pre_stop(acpibase)			{}
 #define iTCO_vendor_check_noreboot_on()			1
diff --git a/drivers/watchdog/iTCO_vendor_support.c b/drivers/watchdog/iTCO_vendor_support.c
index 4f1b96f59349..cf0eaa04b064 100644
--- a/drivers/watchdog/iTCO_vendor_support.c
+++ b/drivers/watchdog/iTCO_vendor_support.c
@@ -39,8 +39,10 @@
 /* Broken BIOS */
 #define BROKEN_BIOS		911
 
-static int vendorsupport;
-module_param(vendorsupport, int, 0);
+int iTCO_vendorsupport;
+EXPORT_SYMBOL(iTCO_vendorsupport);
+
+module_param_named(vendorsupport, iTCO_vendorsupport, int, 0);
 MODULE_PARM_DESC(vendorsupport, "iTCO vendor specific support mode, default="
 			"0 (none), 1=SuperMicro Pent3, 911=Broken SMI BIOS");
 
@@ -152,7 +154,7 @@ static void broken_bios_stop(struct resource *smires)
 void iTCO_vendor_pre_start(struct resource *smires,
 			   unsigned int heartbeat)
 {
-	switch (vendorsupport) {
+	switch (iTCO_vendorsupport) {
 	case SUPERMICRO_OLD_BOARD:
 		supermicro_old_pre_start(smires);
 		break;
@@ -165,7 +167,7 @@ EXPORT_SYMBOL(iTCO_vendor_pre_start);
 
 void iTCO_vendor_pre_stop(struct resource *smires)
 {
-	switch (vendorsupport) {
+	switch (iTCO_vendorsupport) {
 	case SUPERMICRO_OLD_BOARD:
 		supermicro_old_pre_stop(smires);
 		break;
@@ -178,7 +180,7 @@ EXPORT_SYMBOL(iTCO_vendor_pre_stop);
 
 int iTCO_vendor_check_noreboot_on(void)
 {
-	switch (vendorsupport) {
+	switch (iTCO_vendorsupport) {
 	case SUPERMICRO_OLD_BOARD:
 		return 0;
 	default:
@@ -189,13 +191,13 @@ EXPORT_SYMBOL(iTCO_vendor_check_noreboot_on);
 
 static int __init iTCO_vendor_init_module(void)
 {
-	if (vendorsupport == SUPERMICRO_NEW_BOARD) {
+	if (iTCO_vendorsupport == SUPERMICRO_NEW_BOARD) {
 		pr_warn("Option vendorsupport=%d is no longer supported, "
 			"please use the w83627hf_wdt driver instead\n",
 			SUPERMICRO_NEW_BOARD);
 		return -EINVAL;
 	}
-	pr_info("vendor-support=%d\n", vendorsupport);
+	pr_info("vendor-support=%d\n", iTCO_vendorsupport);
 	return 0;
 }
 
-- 
2.25.0

[PATCH 1/3] watchdog: iTCO_wdt: Export vendorsupport

[Mika Westerberg]

In preparation for making ->smi_res optional the iTCO_wdt driver needs
to know whether vendorsupport is being set to non-zero. For this reason
export the variable.

Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/watchdog/iTCO_vendor.h         |  2 ++
 drivers/watchdog/iTCO_vendor_support.c | 16 +++++++++-------
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/watchdog/iTCO_vendor.h b/drivers/watchdog/iTCO_vendor.h
index 0f7373ba10d5..69e92e692ae0 100644
--- a/drivers/watchdog/iTCO_vendor.h
+++ b/drivers/watchdog/iTCO_vendor.h
@@ -1,10 +1,12 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /* iTCO Vendor Specific Support hooks */
 #ifdef CONFIG_ITCO_VENDOR_SUPPORT
+extern int iTCO_vendorsupport;
 extern void iTCO_vendor_pre_start(struct resource *, unsigned int);
 extern void iTCO_vendor_pre_stop(struct resource *);
 extern int iTCO_vendor_check_noreboot_on(void);
 #else
+#define iTCO_vendorsupport				0
 #define iTCO_vendor_pre_start(acpibase, heartbeat)	{}
 #define iTCO_vendor_pre_stop(acpibase)			{}
 #define iTCO_vendor_check_noreboot_on()			1
diff --git a/drivers/watchdog/iTCO_vendor_support.c b/drivers/watchdog/iTCO_vendor_support.c
index 4f1b96f59349..cf0eaa04b064 100644
--- a/drivers/watchdog/iTCO_vendor_support.c
+++ b/drivers/watchdog/iTCO_vendor_support.c
@@ -39,8 +39,10 @@
 /* Broken BIOS */
 #define BROKEN_BIOS		911
 
-static int vendorsupport;
-module_param(vendorsupport, int, 0);
+int iTCO_vendorsupport;
+EXPORT_SYMBOL(iTCO_vendorsupport);
+
+module_param_named(vendorsupport, iTCO_vendorsupport, int, 0);
 MODULE_PARM_DESC(vendorsupport, "iTCO vendor specific support mode, default="
 			"0 (none), 1=SuperMicro Pent3, 911=Broken SMI BIOS");
 
@@ -152,7 +154,7 @@ static void broken_bios_stop(struct resource *smires)
 void iTCO_vendor_pre_start(struct resource *smires,
 			   unsigned int heartbeat)
 {
-	switch (vendorsupport) {
+	switch (iTCO_vendorsupport) {
 	case SUPERMICRO_OLD_BOARD:
 		supermicro_old_pre_start(smires);
 		break;
@@ -165,7 +167,7 @@ EXPORT_SYMBOL(iTCO_vendor_pre_start);
 
 void iTCO_vendor_pre_stop(struct resource *smires)
 {
-	switch (vendorsupport) {
+	switch (iTCO_vendorsupport) {
 	case SUPERMICRO_OLD_BOARD:
 		supermicro_old_pre_stop(smires);
 		break;
@@ -178,7 +180,7 @@ EXPORT_SYMBOL(iTCO_vendor_pre_stop);
 
 int iTCO_vendor_check_noreboot_on(void)
 {
-	switch (vendorsupport) {
+	switch (iTCO_vendorsupport) {
 	case SUPERMICRO_OLD_BOARD:
 		return 0;
 	default:
@@ -189,13 +191,13 @@ EXPORT_SYMBOL(iTCO_vendor_check_noreboot_on);
 
 static int __init iTCO_vendor_init_module(void)
 {
-	if (vendorsupport == SUPERMICRO_NEW_BOARD) {
+	if (iTCO_vendorsupport == SUPERMICRO_NEW_BOARD) {
 		pr_warn("Option vendorsupport=%d is no longer supported, "
 			"please use the w83627hf_wdt driver instead\n",
 			SUPERMICRO_NEW_BOARD);
 		return -EINVAL;
 	}
-	pr_info("vendor-support=%d\n", vendorsupport);
+	pr_info("vendor-support=%d\n", iTCO_vendorsupport);
 	return 0;
 }
 
-- 
2.25.0

[PATCH 2/3] watchdog: iTCO_wdt: Make ICH_RES_IO_SMI optional

[Mika Westerberg]

The iTCO_wdt driver only needs ICH_RES_IO_SMI I/O resource when either
turn_SMI_watchdog_clear_off module parameter is set to match ->iTCO_version
(or higher), and when legacy iTCO_vendorsupport is set. Modify the driver
so that ICH_RES_IO_SMI is optional if the two conditions are not met.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/watchdog/iTCO_wdt.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index 156360e37714..f1692452bc25 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -460,7 +460,7 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
 		return -ENODEV;
 
 	p->smi_res = platform_get_resource(pdev, IORESOURCE_IO, ICH_RES_IO_SMI);
-	if (!p->smi_res)
+	if (!p->smi_res && iTCO_vendorsupport)
 		return -ENODEV;
 
 	p->iTCO_version = pdata->version;
@@ -492,15 +492,19 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
 	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
 	p->update_no_reboot_bit(p->no_reboot_priv, true);
 
-	/* The TCO logic uses the TCO_EN bit in the SMI_EN register */
-	if (!devm_request_region(dev, p->smi_res->start,
-				 resource_size(p->smi_res),
-				 pdev->name)) {
-		pr_err("I/O address 0x%04llx already in use, device disabled\n",
-		       (u64)SMI_EN(p));
-		return -EBUSY;
-	}
 	if (turn_SMI_watchdog_clear_off >= p->iTCO_version) {
+		if (!p->smi_res) {
+			pr_err("SMI I/O resource is missing\n");
+			return -EINVAL;
+		}
+		/* The TCO logic uses the TCO_EN bit in the SMI_EN register */
+		if (!devm_request_region(dev, p->smi_res->start,
+					 resource_size(p->smi_res),
+					 pdev->name)) {
+			pr_err("I/O address 0x%04llx already in use, device disabled\n",
+			       (u64)SMI_EN(p));
+			return -EBUSY;
+		}
 		/*
 		 * Bit 13: TCO_EN -> 0
 		 * Disables TCO logic generating an SMI#
-- 
2.25.0

Re: [PATCH 2/3] watchdog: iTCO_wdt: Make ICH_RES_IO_SMI optional

[Guenter Roeck]

On Tue, Feb 25, 2020 at 03:38:01PM +0300, Mika Westerberg wrote:
> The iTCO_wdt driver only needs ICH_RES_IO_SMI I/O resource when either
> turn_SMI_watchdog_clear_off module parameter is set to match ->iTCO_version
> (or higher), and when legacy iTCO_vendorsupport is set. Modify the driver
> so that ICH_RES_IO_SMI is optional if the two conditions are not met.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/watchdog/iTCO_wdt.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index 156360e37714..f1692452bc25 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -460,7 +460,7 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  
>  	p->smi_res = platform_get_resource(pdev, IORESOURCE_IO, ICH_RES_IO_SMI);
> -	if (!p->smi_res)
> +	if (!p->smi_res && iTCO_vendorsupport)
>  		return -ENODEV;
>  
>  	p->iTCO_version = pdata->version;
> @@ -492,15 +492,19 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
>  	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
>  	p->update_no_reboot_bit(p->no_reboot_priv, true);
>  
> -	/* The TCO logic uses the TCO_EN bit in the SMI_EN register */
> -	if (!devm_request_region(dev, p->smi_res->start,
> -				 resource_size(p->smi_res),
> -				 pdev->name)) {
> -		pr_err("I/O address 0x%04llx already in use, device disabled\n",
> -		       (u64)SMI_EN(p));
> -		return -EBUSY;
> -	}
>  	if (turn_SMI_watchdog_clear_off >= p->iTCO_version) {
> +		if (!p->smi_res) {
> +			pr_err("SMI I/O resource is missing\n");
> +			return -EINVAL;
> +		}
> +		/* The TCO logic uses the TCO_EN bit in the SMI_EN register */
> +		if (!devm_request_region(dev, p->smi_res->start,
> +					 resource_size(p->smi_res),
> +					 pdev->name)) {
> +			pr_err("I/O address 0x%04llx already in use, device disabled\n",
> +			       (u64)SMI_EN(p));
> +			return -EBUSY;
> +		}

The request_region call is also needed if iTCO_vendorsupport is true.
Well, not strictly speaking, I guess, but then one could argue that
it isn't needed at all.

In this context - looking into the vendorsupport code, I wonder if
it is time to retire it. Separate patch, of course, but still.
Any thoughts ?

Guenter

Re: [PATCH 2/3] watchdog: iTCO_wdt: Make ICH_RES_IO_SMI optional

[Mika Westerberg]

On Tue, Feb 25, 2020 at 06:37:09AM -0800, Guenter Roeck wrote:
> On Tue, Feb 25, 2020 at 03:38:01PM +0300, Mika Westerberg wrote:
> > The iTCO_wdt driver only needs ICH_RES_IO_SMI I/O resource when either
> > turn_SMI_watchdog_clear_off module parameter is set to match ->iTCO_version
> > (or higher), and when legacy iTCO_vendorsupport is set. Modify the driver
> > so that ICH_RES_IO_SMI is optional if the two conditions are not met.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/watchdog/iTCO_wdt.c | 22 +++++++++++++---------
> >  1 file changed, 13 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> > index 156360e37714..f1692452bc25 100644
> > --- a/drivers/watchdog/iTCO_wdt.c
> > +++ b/drivers/watchdog/iTCO_wdt.c
> > @@ -460,7 +460,7 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
> >  		return -ENODEV;
> >  
> >  	p->smi_res = platform_get_resource(pdev, IORESOURCE_IO, ICH_RES_IO_SMI);
> > -	if (!p->smi_res)
> > +	if (!p->smi_res && iTCO_vendorsupport)
> >  		return -ENODEV;
> >  
> >  	p->iTCO_version = pdata->version;
> > @@ -492,15 +492,19 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
> >  	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
> >  	p->update_no_reboot_bit(p->no_reboot_priv, true);
> >  
> > -	/* The TCO logic uses the TCO_EN bit in the SMI_EN register */
> > -	if (!devm_request_region(dev, p->smi_res->start,
> > -				 resource_size(p->smi_res),
> > -				 pdev->name)) {
> > -		pr_err("I/O address 0x%04llx already in use, device disabled\n",
> > -		       (u64)SMI_EN(p));
> > -		return -EBUSY;
> > -	}
> >  	if (turn_SMI_watchdog_clear_off >= p->iTCO_version) {
> > +		if (!p->smi_res) {
> > +			pr_err("SMI I/O resource is missing\n");
> > +			return -EINVAL;
> > +		}
> > +		/* The TCO logic uses the TCO_EN bit in the SMI_EN register */
> > +		if (!devm_request_region(dev, p->smi_res->start,
> > +					 resource_size(p->smi_res),
> > +					 pdev->name)) {
> > +			pr_err("I/O address 0x%04llx already in use, device disabled\n",
> > +			       (u64)SMI_EN(p));
> > +			return -EBUSY;
> > +		}
> 
> The request_region call is also needed if iTCO_vendorsupport is true.
> Well, not strictly speaking, I guess, but then one could argue that
> it isn't needed at all.

Indeed, this is good point as well. Will be fixed in v2.

> In this context - looking into the vendorsupport code, I wonder if
> it is time to retire it. Separate patch, of course, but still.
> Any thoughts ?

No objections from me ;-)

Re: [PATCH 2/3] watchdog: iTCO_wdt: Make ICH_RES_IO_SMI optional

[Mika Westerberg]

On Tue, Feb 25, 2020 at 06:37:09AM -0800, Guenter Roeck wrote:
> On Tue, Feb 25, 2020 at 03:38:01PM +0300, Mika Westerberg wrote:
> > The iTCO_wdt driver only needs ICH_RES_IO_SMI I/O resource when either
> > turn_SMI_watchdog_clear_off module parameter is set to match ->iTCO_version
> > (or higher), and when legacy iTCO_vendorsupport is set. Modify the driver
> > so that ICH_RES_IO_SMI is optional if the two conditions are not met.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > ---
> >  drivers/watchdog/iTCO_wdt.c | 22 +++++++++++++---------
> >  1 file changed, 13 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> > index 156360e37714..f1692452bc25 100644
> > --- a/drivers/watchdog/iTCO_wdt.c
> > +++ b/drivers/watchdog/iTCO_wdt.c
> > @@ -460,7 +460,7 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
> >  		return -ENODEV;
> >  
> >  	p->smi_res = platform_get_resource(pdev, IORESOURCE_IO, ICH_RES_IO_SMI);
> > -	if (!p->smi_res)
> > +	if (!p->smi_res && iTCO_vendorsupport)
> >  		return -ENODEV;
> >  
> >  	p->iTCO_version = pdata->version;
> > @@ -492,15 +492,19 @@ static int iTCO_wdt_probe(struct platform_device *pdev)
> >  	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
> >  	p->update_no_reboot_bit(p->no_reboot_priv, true);
> >  
> > -	/* The TCO logic uses the TCO_EN bit in the SMI_EN register */
> > -	if (!devm_request_region(dev, p->smi_res->start,
> > -				 resource_size(p->smi_res),
> > -				 pdev->name)) {
> > -		pr_err("I/O address 0x%04llx already in use, device disabled\n",
> > -		       (u64)SMI_EN(p));
> > -		return -EBUSY;
> > -	}
> >  	if (turn_SMI_watchdog_clear_off >= p->iTCO_version) {
> > +		if (!p->smi_res) {
> > +			pr_err("SMI I/O resource is missing\n");
> > +			return -EINVAL;
> > +		}
> > +		/* The TCO logic uses the TCO_EN bit in the SMI_EN register */
> > +		if (!devm_request_region(dev, p->smi_res->start,
> > +					 resource_size(p->smi_res),
> > +					 pdev->name)) {
> > +			pr_err("I/O address 0x%04llx already in use, device disabled\n",
> > +			       (u64)SMI_EN(p));
> > +			return -EBUSY;
> > +		}
> 
> The request_region call is also needed if iTCO_vendorsupport is true.
> Well, not strictly speaking, I guess, but then one could argue that
> it isn't needed at all.

Indeed, this is good point as well. Will be fixed in v2.

> In this context - looking into the vendorsupport code, I wonder if
> it is time to retire it. Separate patch, of course, but still.
> Any thoughts ?

No objections from me ;-)

[PATCH 3/3] i2c: i801: Do not add ICH_RES_IO_SMI if PMC device is not present

[Mika Westerberg]

Martin noticed that nct6775 driver does not load properly on his system
in v5.4+ kernels. The issue was bisected to commit b84398d6d7f9 ("i2c:
i801: Use iTCO version 6 in Cannon Lake PCH and beyond") but it is
likely not the culprit because the faulty code has been in the driver
already since commit 9424693035a5 ("i2c: i801: Create iTCO device on
newer Intel PCHs"). So more likely some commit that added PCI IDs of
recent chipsets made the driver to create the iTCO_wdt device on Martins
system.

The issue was debugged to be PCI configuration access to the PMC device
that is not present. This returns all 1's when read and this caused the
iTCO_wdt driver to accidentally request resourses used by nct6775.

Fix this by checking that the PMC device is there and only then populate
the iTCO_wdt ICH_RES_IO_SMI resource. Since the resource is now optional
the iTCO_wdt driver should continue to work on recent systems without it.

Link: https://lore.kernel.org/linux-hwmon/CAM1AHpQ4196tyD=HhBu-2donSsuogabkfP03v1YF26Q7_BgvgA@mail.gmail.com/
Fixes: 9424693035a5 ("i2c: i801: Create iTCO device on newer Intel PCHs")
Reported-by: Martin Volf <martin.volf.42@gmail.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/i2c/busses/i2c-i801.c | 45 +++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index ca4f096fef74..7fa58375bd4b 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1519,7 +1519,7 @@ static DEFINE_SPINLOCK(p2sb_spinlock);
 
 static struct platform_device *
 i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
-		 struct resource *tco_res)
+		 struct resource *tco_res, size_t nres)
 {
 	struct resource *res;
 	unsigned int devfn;
@@ -1563,7 +1563,7 @@ i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
 	res->flags = IORESOURCE_MEM;
 
 	return platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
-					tco_res, 3, &spt_tco_platform_data,
+					tco_res, nres + 1, &spt_tco_platform_data,
 					sizeof(spt_tco_platform_data));
 }
 
@@ -1574,19 +1574,20 @@ static const struct itco_wdt_platform_data cnl_tco_platform_data = {
 
 static struct platform_device *
 i801_add_tco_cnl(struct i801_priv *priv, struct pci_dev *pci_dev,
-		 struct resource *tco_res)
+		 struct resource *tco_res, size_t nres)
 {
 	return platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
-					tco_res, 2, &cnl_tco_platform_data,
+					tco_res, nres, &cnl_tco_platform_data,
 					sizeof(cnl_tco_platform_data));
 }
 
 static void i801_add_tco(struct i801_priv *priv)
 {
 	u32 base_addr, tco_base, tco_ctl, ctrl_val;
-	struct pci_dev *pci_dev = priv->pci_dev;
+	struct pci_dev *pmc_dev, *pci_dev = priv->pci_dev;
 	struct resource tco_res[3], *res;
 	unsigned int devfn;
+	size_t nres = 0;
 
 	/* If we have ACPI based watchdog use that instead */
 	if (acpi_has_watchdog())
@@ -1606,29 +1607,37 @@ static void i801_add_tco(struct i801_priv *priv)
 	res->start = tco_base & ~1;
 	res->end = res->start + 32 - 1;
 	res->flags = IORESOURCE_IO;
+	nres++;
 
 	/*
 	 * Power Management registers.
 	 */
 	devfn = PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 2);
-	pci_bus_read_config_dword(pci_dev->bus, devfn, ACPIBASE, &base_addr);
+	pmc_dev = pci_get_slot(pci_dev->bus, devfn);
+	if (pmc_dev) {
+		pci_read_config_dword(pmc_dev, ACPIBASE, &base_addr);
 
-	res = &tco_res[ICH_RES_IO_SMI];
-	res->start = (base_addr & ~1) + ACPIBASE_SMI_OFF;
-	res->end = res->start + 3;
-	res->flags = IORESOURCE_IO;
+		res = &tco_res[ICH_RES_IO_SMI];
+		res->start = (base_addr & ~1) + ACPIBASE_SMI_OFF;
+		res->end = res->start + 3;
+		res->flags = IORESOURCE_IO;
+		nres++;
 
-	/*
-	 * Enable the ACPI I/O space.
-	 */
-	pci_bus_read_config_dword(pci_dev->bus, devfn, ACPICTRL, &ctrl_val);
-	ctrl_val |= ACPICTRL_EN;
-	pci_bus_write_config_dword(pci_dev->bus, devfn, ACPICTRL, ctrl_val);
+		/*
+		 * Enable the ACPI I/O space.
+		 */
+		pci_read_config_dword(pmc_dev, ACPICTRL, &ctrl_val);
+		ctrl_val |= ACPICTRL_EN;
+		pci_write_config_dword(pmc_dev, ACPICTRL, ctrl_val);
+
+		pci_dev_put(pmc_dev);
+	}
 
 	if (priv->features & FEATURE_TCO_CNL)
-		priv->tco_pdev = i801_add_tco_cnl(priv, pci_dev, tco_res);
+		priv->tco_pdev = i801_add_tco_cnl(priv, pci_dev, tco_res, nres);
 	else
-		priv->tco_pdev = i801_add_tco_spt(priv, pci_dev, tco_res);
+		priv->tco_pdev = i801_add_tco_spt(priv, pci_dev, tco_res, nres);
+
 
 	if (IS_ERR(priv->tco_pdev))
 		dev_warn(&pci_dev->dev, "failed to create iTCO device\n");
-- 
2.25.0

Re: [PATCH 3/3] i2c: i801: Do not add ICH_RES_IO_SMI if PMC device is not present

[Guenter Roeck]

On 2/25/20 4:38 AM, Mika Westerberg wrote:
> Martin noticed that nct6775 driver does not load properly on his system
> in v5.4+ kernels. The issue was bisected to commit b84398d6d7f9 ("i2c:
> i801: Use iTCO version 6 in Cannon Lake PCH and beyond") but it is
> likely not the culprit because the faulty code has been in the driver
> already since commit 9424693035a5 ("i2c: i801: Create iTCO device on
> newer Intel PCHs"). So more likely some commit that added PCI IDs of
> recent chipsets made the driver to create the iTCO_wdt device on Martins
> system.
> 
> The issue was debugged to be PCI configuration access to the PMC device
> that is not present. This returns all 1's when read and this caused the
> iTCO_wdt driver to accidentally request resourses used by nct6775.
> 
> Fix this by checking that the PMC device is there and only then populate
> the iTCO_wdt ICH_RES_IO_SMI resource. Since the resource is now optional
> the iTCO_wdt driver should continue to work on recent systems without it.
> 
> Link: https://lore.kernel.org/linux-hwmon/CAM1AHpQ4196tyD=HhBu-2donSsuogabkfP03v1YF26Q7_BgvgA@mail.gmail.com/
> Fixes: 9424693035a5 ("i2c: i801: Create iTCO device on newer Intel PCHs")
> Reported-by: Martin Volf <martin.volf.42@gmail.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>   drivers/i2c/busses/i2c-i801.c | 45 +++++++++++++++++++++--------------
>   1 file changed, 27 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index ca4f096fef74..7fa58375bd4b 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1519,7 +1519,7 @@ static DEFINE_SPINLOCK(p2sb_spinlock);
>   
>   static struct platform_device *
>   i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
> -		 struct resource *tco_res)
> +		 struct resource *tco_res, size_t nres)
>   {
>   	struct resource *res;
>   	unsigned int devfn;
> @@ -1563,7 +1563,7 @@ i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
>   	res->flags = IORESOURCE_MEM;
>   
>   	return platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
> -					tco_res, 3, &spt_tco_platform_data,
> +					tco_res, nres + 1, &spt_tco_platform_data,

Does this work as intended ? It still adds ICH_RES_MEM_OFF at index 2,
but if there is no SMI resource it will only pass two sets of resources
to the wdt driver, one of which (the SMI resource) would be empty,
ie have start == NULL and size == 0.

Guenter

Re: [PATCH 3/3] i2c: i801: Do not add ICH_RES_IO_SMI if PMC device is not present

[Guenter Roeck]

On 2/25/20 4:38 AM, Mika Westerberg wrote:
> Martin noticed that nct6775 driver does not load properly on his system
> in v5.4+ kernels. The issue was bisected to commit b84398d6d7f9 ("i2c:
> i801: Use iTCO version 6 in Cannon Lake PCH and beyond") but it is
> likely not the culprit because the faulty code has been in the driver
> already since commit 9424693035a5 ("i2c: i801: Create iTCO device on
> newer Intel PCHs"). So more likely some commit that added PCI IDs of
> recent chipsets made the driver to create the iTCO_wdt device on Martins
> system.
> 
> The issue was debugged to be PCI configuration access to the PMC device
> that is not present. This returns all 1's when read and this caused the
> iTCO_wdt driver to accidentally request resourses used by nct6775.
> 
> Fix this by checking that the PMC device is there and only then populate
> the iTCO_wdt ICH_RES_IO_SMI resource. Since the resource is now optional
> the iTCO_wdt driver should continue to work on recent systems without it.
> 
> Link: https://lore.kernel.org/linux-hwmon/CAM1AHpQ4196tyD=HhBu-2donSsuogabkfP03v1YF26Q7_BgvgA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org/
> Fixes: 9424693035a5 ("i2c: i801: Create iTCO device on newer Intel PCHs")
> Reported-by: Martin Volf <martin.volf.42-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>   drivers/i2c/busses/i2c-i801.c | 45 +++++++++++++++++++++--------------
>   1 file changed, 27 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index ca4f096fef74..7fa58375bd4b 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1519,7 +1519,7 @@ static DEFINE_SPINLOCK(p2sb_spinlock);
>   
>   static struct platform_device *
>   i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
> -		 struct resource *tco_res)
> +		 struct resource *tco_res, size_t nres)
>   {
>   	struct resource *res;
>   	unsigned int devfn;
> @@ -1563,7 +1563,7 @@ i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
>   	res->flags = IORESOURCE_MEM;
>   
>   	return platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
> -					tco_res, 3, &spt_tco_platform_data,
> +					tco_res, nres + 1, &spt_tco_platform_data,

Does this work as intended ? It still adds ICH_RES_MEM_OFF at index 2,
but if there is no SMI resource it will only pass two sets of resources
to the wdt driver, one of which (the SMI resource) would be empty,
ie have start == NULL and size == 0.

Guenter

Re: [PATCH 3/3] i2c: i801: Do not add ICH_RES_IO_SMI if PMC device is not present

[Mika Westerberg]

On Tue, Feb 25, 2020 at 06:21:16AM -0800, Guenter Roeck wrote:
> On 2/25/20 4:38 AM, Mika Westerberg wrote:
> > Martin noticed that nct6775 driver does not load properly on his system
> > in v5.4+ kernels. The issue was bisected to commit b84398d6d7f9 ("i2c:
> > i801: Use iTCO version 6 in Cannon Lake PCH and beyond") but it is
> > likely not the culprit because the faulty code has been in the driver
> > already since commit 9424693035a5 ("i2c: i801: Create iTCO device on
> > newer Intel PCHs"). So more likely some commit that added PCI IDs of
> > recent chipsets made the driver to create the iTCO_wdt device on Martins
> > system.
> > 
> > The issue was debugged to be PCI configuration access to the PMC device
> > that is not present. This returns all 1's when read and this caused the
> > iTCO_wdt driver to accidentally request resourses used by nct6775.
> > 
> > Fix this by checking that the PMC device is there and only then populate
> > the iTCO_wdt ICH_RES_IO_SMI resource. Since the resource is now optional
> > the iTCO_wdt driver should continue to work on recent systems without it.
> > 
> > Link: https://lore.kernel.org/linux-hwmon/CAM1AHpQ4196tyD=HhBu-2donSsuogabkfP03v1YF26Q7_BgvgA@mail.gmail.com/
> > Fixes: 9424693035a5 ("i2c: i801: Create iTCO device on newer Intel PCHs")
> > Reported-by: Martin Volf <martin.volf.42@gmail.com>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >   drivers/i2c/busses/i2c-i801.c | 45 +++++++++++++++++++++--------------
> >   1 file changed, 27 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> > index ca4f096fef74..7fa58375bd4b 100644
> > --- a/drivers/i2c/busses/i2c-i801.c
> > +++ b/drivers/i2c/busses/i2c-i801.c
> > @@ -1519,7 +1519,7 @@ static DEFINE_SPINLOCK(p2sb_spinlock);
> >   static struct platform_device *
> >   i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
> > -		 struct resource *tco_res)
> > +		 struct resource *tco_res, size_t nres)
> >   {
> >   	struct resource *res;
> >   	unsigned int devfn;
> > @@ -1563,7 +1563,7 @@ i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
> >   	res->flags = IORESOURCE_MEM;
> >   	return platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
> > -					tco_res, 3, &spt_tco_platform_data,
> > +					tco_res, nres + 1, &spt_tco_platform_data,
> 
> Does this work as intended ? It still adds ICH_RES_MEM_OFF at index 2,
> but if there is no SMI resource it will only pass two sets of resources
> to the wdt driver, one of which (the SMI resource) would be empty,
> ie have start == NULL and size == 0.

Good point that would not work as expected. I'll fix this one in the
next version.

Re: [PATCH 3/3] i2c: i801: Do not add ICH_RES_IO_SMI if PMC device is not present

[Mika Westerberg]

On Tue, Feb 25, 2020 at 06:21:16AM -0800, Guenter Roeck wrote:
> On 2/25/20 4:38 AM, Mika Westerberg wrote:
> > Martin noticed that nct6775 driver does not load properly on his system
> > in v5.4+ kernels. The issue was bisected to commit b84398d6d7f9 ("i2c:
> > i801: Use iTCO version 6 in Cannon Lake PCH and beyond") but it is
> > likely not the culprit because the faulty code has been in the driver
> > already since commit 9424693035a5 ("i2c: i801: Create iTCO device on
> > newer Intel PCHs"). So more likely some commit that added PCI IDs of
> > recent chipsets made the driver to create the iTCO_wdt device on Martins
> > system.
> > 
> > The issue was debugged to be PCI configuration access to the PMC device
> > that is not present. This returns all 1's when read and this caused the
> > iTCO_wdt driver to accidentally request resourses used by nct6775.
> > 
> > Fix this by checking that the PMC device is there and only then populate
> > the iTCO_wdt ICH_RES_IO_SMI resource. Since the resource is now optional
> > the iTCO_wdt driver should continue to work on recent systems without it.
> > 
> > Link: https://lore.kernel.org/linux-hwmon/CAM1AHpQ4196tyD=HhBu-2donSsuogabkfP03v1YF26Q7_BgvgA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org/
> > Fixes: 9424693035a5 ("i2c: i801: Create iTCO device on newer Intel PCHs")
> > Reported-by: Martin Volf <martin.volf.42-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > ---
> >   drivers/i2c/busses/i2c-i801.c | 45 +++++++++++++++++++++--------------
> >   1 file changed, 27 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> > index ca4f096fef74..7fa58375bd4b 100644
> > --- a/drivers/i2c/busses/i2c-i801.c
> > +++ b/drivers/i2c/busses/i2c-i801.c
> > @@ -1519,7 +1519,7 @@ static DEFINE_SPINLOCK(p2sb_spinlock);
> >   static struct platform_device *
> >   i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
> > -		 struct resource *tco_res)
> > +		 struct resource *tco_res, size_t nres)
> >   {
> >   	struct resource *res;
> >   	unsigned int devfn;
> > @@ -1563,7 +1563,7 @@ i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
> >   	res->flags = IORESOURCE_MEM;
> >   	return platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
> > -					tco_res, 3, &spt_tco_platform_data,
> > +					tco_res, nres + 1, &spt_tco_platform_data,
> 
> Does this work as intended ? It still adds ICH_RES_MEM_OFF at index 2,
> but if there is no SMI resource it will only pass two sets of resources
> to the wdt driver, one of which (the SMI resource) would be empty,
> ie have start == NULL and size == 0.

Good point that would not work as expected. I'll fix this one in the
next version.

Re: [PATCH 3/3] i2c: i801: Do not add ICH_RES_IO_SMI if PMC device is not present

[Andy Shevchenko]

On Tue, Feb 25, 2020 at 03:38:02PM +0300, Mika Westerberg wrote:
> Martin noticed that nct6775 driver does not load properly on his system
> in v5.4+ kernels. The issue was bisected to commit b84398d6d7f9 ("i2c:
> i801: Use iTCO version 6 in Cannon Lake PCH and beyond") but it is
> likely not the culprit because the faulty code has been in the driver
> already since commit 9424693035a5 ("i2c: i801: Create iTCO device on
> newer Intel PCHs"). So more likely some commit that added PCI IDs of
> recent chipsets made the driver to create the iTCO_wdt device on Martins
> system.
> 
> The issue was debugged to be PCI configuration access to the PMC device
> that is not present. This returns all 1's when read and this caused the
> iTCO_wdt driver to accidentally request resourses used by nct6775.
> 
> Fix this by checking that the PMC device is there and only then populate
> the iTCO_wdt ICH_RES_IO_SMI resource. Since the resource is now optional
> the iTCO_wdt driver should continue to work on recent systems without it.

...

>  	return platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
> -					tco_res, 3, &spt_tco_platform_data,
> +					tco_res, nres + 1, &spt_tco_platform_data,
>  					sizeof(spt_tco_platform_data));

Perhaps use the same pattern as below, i.e.

	nres++;
	return ...(..., nres, ...);

?

...

>  	return platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
> -					tco_res, 2, &cnl_tco_platform_data,
> +					tco_res, nres, &cnl_tco_platform_data,
>  					sizeof(cnl_tco_platform_data));

Ditto.

...

> +	nres++;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] i2c: i801: Do not add ICH_RES_IO_SMI if PMC device is not present

[Andy Shevchenko]

On Tue, Feb 25, 2020 at 03:38:02PM +0300, Mika Westerberg wrote:
> Martin noticed that nct6775 driver does not load properly on his system
> in v5.4+ kernels. The issue was bisected to commit b84398d6d7f9 ("i2c:
> i801: Use iTCO version 6 in Cannon Lake PCH and beyond") but it is
> likely not the culprit because the faulty code has been in the driver
> already since commit 9424693035a5 ("i2c: i801: Create iTCO device on
> newer Intel PCHs"). So more likely some commit that added PCI IDs of
> recent chipsets made the driver to create the iTCO_wdt device on Martins
> system.
> 
> The issue was debugged to be PCI configuration access to the PMC device
> that is not present. This returns all 1's when read and this caused the
> iTCO_wdt driver to accidentally request resourses used by nct6775.
> 
> Fix this by checking that the PMC device is there and only then populate
> the iTCO_wdt ICH_RES_IO_SMI resource. Since the resource is now optional
> the iTCO_wdt driver should continue to work on recent systems without it.

...

>  	return platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
> -					tco_res, 3, &spt_tco_platform_data,
> +					tco_res, nres + 1, &spt_tco_platform_data,
>  					sizeof(spt_tco_platform_data));

Perhaps use the same pattern as below, i.e.

	nres++;
	return ...(..., nres, ...);

?

...

>  	return platform_device_register_resndata(&pci_dev->dev, "iTCO_wdt", -1,
> -					tco_res, 2, &cnl_tco_platform_data,
> +					tco_res, nres, &cnl_tco_platform_data,
>  					sizeof(cnl_tco_platform_data));

Ditto.

...

> +	nres++;

-- 
With Best Regards,
Andy Shevchenko