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

[Jean Delvare <jdelvare@suse.de>]

Hi Heiner, Wolfram,

On Sat, 02 Sep 2023 22:06:14 +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.

This last sentence no longer applies to this version of the patch.

> Fixes: 9b5bf5878138 ("i2c: i801: Restore INTREN on unload")
> Fixes: 9424693035a5 ("i2c: i801: Create iTCO device on newer Intel PCHs")
> Cc: stable@vger.kernel.org

I wouldn't cc stable. For one thing, this patch doesn't fix a bug that
actually bothers people. Error paths are rarely taken, and driver
removal isn't that frequent either. Consequences are also rather
harmless (one-time resource leak, race condition which is quite
unlikely to trigger).

For another, this patch is a mix of 2 bug fixes (SMBHSTCNT being
restored too early in i801_remove, resource leak in error path of
i801_probe) which have been added in very different kernel versions
(v5.16 and v4.3, respectively), and tidying up (the reordering of some
of the statements in i801_remove is nice for consistency but is not
actually fixing any bug).

If you really want to push the fixes to stable, you'd have to split the
patch in 3 pieces, one for each fix (going to stable), and one for the
remainder (not going to stable). Otherwise it makes backporting to
older kernels error-prone and time-consuming. Considering how harmless
the bugs are in the first place, my position is that the extra work is
simply not worth it.

> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v2:
> - add Fixes tag for 9424693035a5
> - remove restoring SMBHSTCNT from probe error path
> - move restoring SMBHSTCNT to the end in remove/shutdown
> ---
>  drivers/i2c/busses/i2c-i801.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> (...)

That being said, the patch itself looks good to me, and I have tested
it too.

Reviewed-by: Jean Delvare <jdelvare@suse.de>
Tested-by: Jean Delvare <jdelvare@suse.de>

Thanks,
-- 
Jean Delvare
SUSE L3 Support