[PATCH] i2c: i801: fix cleanup code in remove() and error path of probe()

[PATCH] i2c: i801: fix cleanup code in remove() and error path of probe()

[Heiner Kallweit]

Jean pointed out that the referenced patch resulted in the remove()
path not having the reverse order of calls in probe(). I think there's
more to be done to ensure proper cleanup.
Especially cleanup in the probe() error path has to be extended.
Not every step there may be strictly needed, but it's in line with
remove() now.

Fixes: 9b5bf5878138 ("i2c: i801: Restore INTREN on unload")
Cc: stable@vger.kernel.org
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 73ae06432..7a0ccc584 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1754,6 +1754,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		"SMBus I801 adapter at %04lx", priv->smba);
 	err = i2c_add_adapter(&priv->adapter);
 	if (err) {
+		platform_device_unregister(priv->tco_pdev);
+		outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
+		pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
 		i801_acpi_remove(priv);
 		return err;
 	}
@@ -1779,14 +1782,13 @@ static void i801_remove(struct pci_dev *dev)
 {
 	struct i801_priv *priv = pci_get_drvdata(dev);
 
-	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
-	i801_disable_host_notify(priv);
 	i801_del_mux(priv);
+	i801_disable_host_notify(priv);
 	i2c_del_adapter(&priv->adapter);
-	i801_acpi_remove(priv);
-	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
-
 	platform_device_unregister(priv->tco_pdev);
+	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
+	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
+	i801_acpi_remove(priv);
 
 	/* if acpi_reserved is set then usage_count is incremented already */
 	if (!priv->acpi_reserved)
@@ -1803,8 +1805,8 @@ static void i801_shutdown(struct pci_dev *dev)
 	struct i801_priv *priv = pci_get_drvdata(dev);
 
 	/* Restore config registers to avoid hard hang on some systems */
-	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
 	i801_disable_host_notify(priv);
+	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
 	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
 }
 
-- 
2.42.0

Re: [PATCH] i2c: i801: fix cleanup code in remove() and error path of probe()

[Andi Shyti]

Hi Jean,

On Sun, Aug 27, 2023 at 10:26:05PM +0200, Heiner Kallweit wrote:
> Jean pointed out that the referenced patch resulted in the remove()
> path not having the reverse order of calls in probe(). I think there's
> more to be done to ensure proper cleanup.
> Especially cleanup in the probe() error path has to be extended.
> Not every step there may be strictly needed, but it's in line with
> remove() now.
> 
> Fixes: 9b5bf5878138 ("i2c: i801: Restore INTREN on unload")
> Cc: stable@vger.kernel.org
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

any chance to take a look at this?

Andi

Re: [PATCH] i2c: i801: fix cleanup code in remove() and error path of probe()

[Jean Delvare]

Hi Heiner,

On Sun, 27 Aug 2023 22:26:05 +0200, Heiner Kallweit wrote:
> Jean pointed out that the referenced patch resulted in the remove()
> path not having the reverse order of calls in probe(). I think there's
> more to be done to ensure proper cleanup.
> Especially cleanup in the probe() error path has to be extended.
> Not every step there may be strictly needed, but it's in line with
> remove() now.
> 
> Fixes: 9b5bf5878138 ("i2c: i801: Restore INTREN on unload")

I think it also fixes 9424693035a5 ("i2c: i801: Create iTCO device on
newer Intel PCHs").

> Cc: stable@vger.kernel.org
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 73ae06432..7a0ccc584 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1754,6 +1754,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  		"SMBus I801 adapter at %04lx", priv->smba);
>  	err = i2c_add_adapter(&priv->adapter);
>  	if (err) {
> +		platform_device_unregister(priv->tco_pdev);
> +		outb_p(priv->original_hstcnt, SMBHSTCNT(priv));

Doesn't seem to be needed, as I can't see SMBHSTCNT being written to
during probe?

> +		pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
>  		i801_acpi_remove(priv);
>  		return err;
>  	}
> @@ -1779,14 +1782,13 @@ static void i801_remove(struct pci_dev *dev)
>  {
>  	struct i801_priv *priv = pci_get_drvdata(dev);
>  
> -	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
> -	i801_disable_host_notify(priv);
>  	i801_del_mux(priv);
> +	i801_disable_host_notify(priv);
>  	i2c_del_adapter(&priv->adapter);
> -	i801_acpi_remove(priv);
> -	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
> -
>  	platform_device_unregister(priv->tco_pdev);
> +	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
> +	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
> +	i801_acpi_remove(priv);
>  
>  	/* if acpi_reserved is set then usage_count is incremented already */
>  	if (!priv->acpi_reserved)
> @@ -1803,8 +1805,8 @@ static void i801_shutdown(struct pci_dev *dev)
>  	struct i801_priv *priv = pci_get_drvdata(dev);
>  
>  	/* Restore config registers to avoid hard hang on some systems */
> -	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
>  	i801_disable_host_notify(priv);
> +	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
>  	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
>  }
>  

Rest looks OK, I'll give it some testing 

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: i801: fix cleanup code in remove() and error path of probe()

[Heiner Kallweit]

On 01.09.2023 14:19, Jean Delvare wrote:
> Hi Heiner,
> 
> On Sun, 27 Aug 2023 22:26:05 +0200, Heiner Kallweit wrote:
>> Jean pointed out that the referenced patch resulted in the remove()
>> path not having the reverse order of calls in probe(). I think there's
>> more to be done to ensure proper cleanup.
>> Especially cleanup in the probe() error path has to be extended.
>> Not every step there may be strictly needed, but it's in line with
>> remove() now.
>>
>> Fixes: 9b5bf5878138 ("i2c: i801: Restore INTREN on unload")
> 
> I think it also fixes 9424693035a5 ("i2c: i801: Create iTCO device on
> newer Intel PCHs").
> 

Right. We should add this, even though this fix won't apply cleanly
on some older kernel versions. I think we'll need separate patches for these
LTS kernel versions.

>> Cc: stable@vger.kernel.org
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-i801.c | 14 ++++++++------
>>  1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index 73ae06432..7a0ccc584 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -1754,6 +1754,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>  		"SMBus I801 adapter at %04lx", priv->smba);
>>  	err = i2c_add_adapter(&priv->adapter);
>>  	if (err) {
>> +		platform_device_unregister(priv->tco_pdev);
>> +		outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
> 
> Doesn't seem to be needed, as I can't see SMBHSTCNT being written to
> during probe?
> 
Right, this is what I was referring to in the commit message when saying
"not every step may be strictly needed". Restoring SMBHSTCNT isn't needed
here, but it makes cleanup in the probe error path the same as in remove
and therefore may improve readability and maintainability.
But I don't insist on it and don't have a strong opinion.

>> +		pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
>>  		i801_acpi_remove(priv);
>>  		return err;
>>  	}
>> @@ -1779,14 +1782,13 @@ static void i801_remove(struct pci_dev *dev)
>>  {
>>  	struct i801_priv *priv = pci_get_drvdata(dev);
>>  
>> -	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
>> -	i801_disable_host_notify(priv);
>>  	i801_del_mux(priv);
>> +	i801_disable_host_notify(priv);
>>  	i2c_del_adapter(&priv->adapter);
>> -	i801_acpi_remove(priv);
>> -	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
>> -
>>  	platform_device_unregister(priv->tco_pdev);
>> +	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
>> +	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
>> +	i801_acpi_remove(priv);
>>  
>>  	/* if acpi_reserved is set then usage_count is incremented already */
>>  	if (!priv->acpi_reserved)
>> @@ -1803,8 +1805,8 @@ static void i801_shutdown(struct pci_dev *dev)
>>  	struct i801_priv *priv = pci_get_drvdata(dev);
>>  
>>  	/* Restore config registers to avoid hard hang on some systems */
>> -	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
>>  	i801_disable_host_notify(priv);
>> +	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
>>  	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
>>  }
>>  
> 
> Rest looks OK, I'll give it some testing 
> 

Re: [PATCH] i2c: i801: fix cleanup code in remove() and error path of probe()

[Jean Delvare]

On Fri, 1 Sep 2023 14:41:38 +0200, Heiner Kallweit wrote:
> On 01.09.2023 14:19, Jean Delvare wrote:
> > On Sun, 27 Aug 2023 22:26:05 +0200, Heiner Kallweit wrote:  
> >> Jean pointed out that the referenced patch resulted in the remove()
> >> path not having the reverse order of calls in probe(). I think there's
> >> more to be done to ensure proper cleanup.
> >> Especially cleanup in the probe() error path has to be extended.
> >> Not every step there may be strictly needed, but it's in line with
> >> remove() now.
> >>
> >> Fixes: 9b5bf5878138 ("i2c: i801: Restore INTREN on unload")  
> > 
> > I think it also fixes 9424693035a5 ("i2c: i801: Create iTCO device on
> > newer Intel PCHs").
> 
> Right. We should add this, even though this fix won't apply cleanly
> on some older kernel versions. I think we'll need separate patches for these
> LTS kernel versions.

Our task is to annotate the commit message with the dependency
information. Whether or not people want to backport the fix to a given
kernel is their decision.

Personally I wouldn't bother backporting this to stable kernels, as it's
not "a real bug that bothers people", to quote
Documentation/process/stable-kernel-rules.rst.

> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >> ---
> >>  drivers/i2c/busses/i2c-i801.c | 14 ++++++++------
> >>  1 file changed, 8 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> >> index 73ae06432..7a0ccc584 100644
> >> --- a/drivers/i2c/busses/i2c-i801.c
> >> +++ b/drivers/i2c/busses/i2c-i801.c
> >> @@ -1754,6 +1754,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
> >>  		"SMBus I801 adapter at %04lx", priv->smba);
> >>  	err = i2c_add_adapter(&priv->adapter);
> >>  	if (err) {
> >> +		platform_device_unregister(priv->tco_pdev);
> >> +		outb_p(priv->original_hstcnt, SMBHSTCNT(priv));  
> > 
> > Doesn't seem to be needed, as I can't see SMBHSTCNT being written to
> > during probe?
> >   
> Right, this is what I was referring to in the commit message when saying
> "not every step may be strictly needed". Restoring SMBHSTCNT isn't needed

Oh right, missed that, sorry.

> here, but it makes cleanup in the probe error path the same as in remove
> and therefore may improve readability and maintainability.
> But I don't insist on it and don't have a strong opinion.

I think it would make sense to omit it from the probe error path (to
make it clear it's not needed there) and move it at the end of
i801_remove() and i801_shutdown(). That way the probe error path is
still a subset of the remove and shutdown paths and I think this solves
the maintainability issue. Would that be OK with you?

-- 
Jean Delvare
SUSE L3 Support