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@xxxxxxxxxxxxxxx
> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
> ---
> 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
