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

[PATCH v2] 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")
Fixes: 9424693035a5 ("i2c: i801: Create iTCO device on newer Intel PCHs")
Cc: stable@vger.kernel.org
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(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 73ae06432..d4f59aecc 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1754,6 +1754,8 @@ 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);
+		pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
 		i801_acpi_remove(priv);
 		return err;
 	}
@@ -1779,14 +1781,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);
+	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
+	i801_acpi_remove(priv);
+	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
 
 	/* if acpi_reserved is set then usage_count is incremented already */
 	if (!priv->acpi_reserved)
@@ -1803,9 +1804,9 @@ 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);
 	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
+	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
 }
 
 static int i801_suspend(struct device *dev)
-- 
2.42.0

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

[Jean Delvare]

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

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

[Andi Shyti]

Hi Jean,

On Wed, Sep 06, 2023 at 01:47:45PM +0200, Jean Delvare wrote:
> 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).

we are having this same discussion in another thread: if a bug is
unlikely to happen, doesn't mean that there is no bug. A fix is a
fix and should be backported to stable kernels.

Sometimes bugs are reported some other times bugs are discovered
by reading the code (like in the other thread). In the latter
case bugs are just waiting for their time of glory.

I'm OK if this set of fixes have the Fixes tag or, like in the
other case, we find a way to get it backported anyway.

> 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.

In my opinion, Heiner, you should split this patch in the two
logical changes that Jean was suggesting, add the tags from Jean
and have them backported.

Thanks Jean for your review and inputs.

Andi

> > 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

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

[Jean Delvare]

Hi Andi,

On Wed, 6 Sep 2023 16:13:57 +0200, Andi Shyti wrote:
> On Wed, Sep 06, 2023 at 01:47:45PM +0200, Jean Delvare wrote:
> > On Sat, 02 Sep 2023 22:06:14 +0200, Heiner Kallweit wrote:  
> > > 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).  
> 
> we are having this same discussion in another thread: if a bug is
> unlikely to happen, doesn't mean that there is no bug. A fix is a
> fix and should be backported to stable kernels.

No. Please read:

  https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

There is clearly a list of conditions for a commit to be eligible for
stable kernel trees. It's not "every fix".

> Sometimes bugs are reported some other times bugs are discovered
> by reading the code (like in the other thread). In the latter
> case bugs are just waiting for their time of glory.

I'm not saying otherwise. But that's clearly one of the factor to
decide whether a fix should go to stable. A bug which has been reported
by a user who is affected by it is clearly a better candidate to
backport. The other factor is how bad things are if the bug happens. I
fully agree that a bug which is found by code review but would have
dramatic consequences should also have its fix backported to stable
kernel trees, even if it never happened before and is unlikely to
happen in the future.

My point is that the bugs being discussed here do not match any of
these criteria. They have not been reported, they most likely never
happened, they most likely never will, and if they would, consequences
would be pretty benign.

-- 
Jean Delvare
SUSE L3 Support

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

[Andi Shyti]

Hi Jean,

> > > 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).  
> > 
> > we are having this same discussion in another thread: if a bug is
> > unlikely to happen, doesn't mean that there is no bug. A fix is a
> > fix and should be backported to stable kernels.
> 
> No. Please read:
> 
>   https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> 
> There is clearly a list of conditions for a commit to be eligible for
> stable kernel trees. It's not "every fix".

I think you are putting these fixes into the ""This could be a
problem..." type of things".

But as I see these fixes don't belong to this category, as they
are clearing the exit path. This is a kind of fixes I want to see
going to stable.

Which means that if we exit through that path, do we exit
cleanly, e.g., without leaking? If the answer is "no", then this
is a fix and should go to stable.

It belongs to "This could be a problem..." type, things like
dev_err/dev_warn (first thing coming to my mind) or other non
functional fixes.

Maybe this is a matter of opinion and different background. For
the i2c side I'm in peace :-)

For the stable backport I'd love to hear another opinion.

Thanks, Jean!
Andi

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

[Heiner Kallweit]

On 06.09.2023 20:25, Andi Shyti wrote:
> Hi Jean,
> 
>>>> 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).  
>>>
>>> we are having this same discussion in another thread: if a bug is
>>> unlikely to happen, doesn't mean that there is no bug. A fix is a
>>> fix and should be backported to stable kernels.
>>
>> No. Please read:
>>
>>   https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
>>
>> There is clearly a list of conditions for a commit to be eligible for
>> stable kernel trees. It's not "every fix".
> 
> I think you are putting these fixes into the ""This could be a
> problem..." type of things".
> 
> But as I see these fixes don't belong to this category, as they
> are clearing the exit path. This is a kind of fixes I want to see
> going to stable.
> 
> Which means that if we exit through that path, do we exit
> cleanly, e.g., without leaking? If the answer is "no", then this
> is a fix and should go to stable.
> 
> It belongs to "This could be a problem..." type, things like
> dev_err/dev_warn (first thing coming to my mind) or other non
> functional fixes.
> 
> Maybe this is a matter of opinion and different background. For
> the i2c side I'm in peace :-)
> 
> For the stable backport I'd love to hear another opinion.
> 
> Thanks, Jean!
> Andi

Please let me know once you come to an agreement, then I'll
submit a (hopefully) final version.

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

[Heiner Kallweit]

On 07.09.2023 07:45, Heiner Kallweit wrote:
> On 06.09.2023 20:25, Andi Shyti wrote:
>> Hi Jean,
>>
>>>>> 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).  
>>>>
>>>> we are having this same discussion in another thread: if a bug is
>>>> unlikely to happen, doesn't mean that there is no bug. A fix is a
>>>> fix and should be backported to stable kernels.
>>>
>>> No. Please read:
>>>
>>>   https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
>>>
>>> There is clearly a list of conditions for a commit to be eligible for
>>> stable kernel trees. It's not "every fix".
>>
>> I think you are putting these fixes into the ""This could be a
>> problem..." type of things".
>>
>> But as I see these fixes don't belong to this category, as they
>> are clearing the exit path. This is a kind of fixes I want to see
>> going to stable.
>>
>> Which means that if we exit through that path, do we exit
>> cleanly, e.g., without leaking? If the answer is "no", then this
>> is a fix and should go to stable.
>>
>> It belongs to "This could be a problem..." type, things like
>> dev_err/dev_warn (first thing coming to my mind) or other non
>> functional fixes.
>>
>> Maybe this is a matter of opinion and different background. For
>> the i2c side I'm in peace :-)
>>
>> For the stable backport I'd love to hear another opinion.
>>
>> Thanks, Jean!
>> Andi
> 
> Please let me know once you come to an agreement, then I'll
> submit a (hopefully) final version.
> 
I think I'll split the patch, that should make dealing with it easier.