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

[Jean Delvare <jdelvare@suse.de>]

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