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

[Jean Delvare <jdelvare@suse.de>]

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