[PATCH] hwmod: (pmbus) disable PEC if not enabled

[PATCH] hwmod: (pmbus) disable PEC if not enabled

[Adam Wujek]

Explicitly disable PEC when the client does not support it.
Without the explicit disable, when the device with the PEC support is removed
later when a device without PEC support is inserted into the same address,
the driver uses the old value of client->flags which contains the I2C_CLIENT_PEC
flag. As a consequence the PEC is used when it should not.

Signed-off-by: Adam Wujek <dev_public@wujek.eu>
---
 drivers/hwmon/pmbus/pmbus_core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 82c3754e21e3..f8ca36759b0a 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2014,6 +2014,8 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 	ret = i2c_smbus_read_byte_data(client, PMBUS_CAPABILITY);
 	if (ret >= 0 && (ret & PB_CAPABILITY_ERROR_CHECK))
 		client->flags |= I2C_CLIENT_PEC;
+	else
+		client->flags &= ~I2C_CLIENT_PEC;

 	pmbus_clear_faults(client);

--
2.17.1

Re: [PATCH] hwmod: (pmbus) disable PEC if not enabled

[Guenter Roeck]

On 4/19/22 13:53, Adam Wujek wrote:
> Explicitly disable PEC when the client does not support it.
> Without the explicit disable, when the device with the PEC support is removed
> later when a device without PEC support is inserted into the same address,
> the driver uses the old value of client->flags which contains the I2C_CLIENT_PEC
> flag. As a consequence the PEC is used when it should not.
> 

How can that happen ? I would assume the I2C device gets deleted and re-created
in that case, which should clear the PEC flag.

Guenter

> Signed-off-by: Adam Wujek <dev_public@wujek.eu>
> ---
>   drivers/hwmon/pmbus/pmbus_core.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 82c3754e21e3..f8ca36759b0a 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2014,6 +2014,8 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
>   	ret = i2c_smbus_read_byte_data(client, PMBUS_CAPABILITY);
>   	if (ret >= 0 && (ret & PB_CAPABILITY_ERROR_CHECK))
>   		client->flags |= I2C_CLIENT_PEC;
> +	else
> +		client->flags &= ~I2C_CLIENT_PEC;
> 
>   	pmbus_clear_faults(client);
> 
> --
> 2.17.1
> 
> 

Re: [PATCH] hwmod: (pmbus) disable PEC if not enabled

[wujek dev]

------- Original Message -------
On Wednesday, April 20th, 2022 at 00:00, Guenter Roeck <linux@roeck-us.net> wrote:
>
>
> On 4/19/22 13:53, Adam Wujek wrote:
>
> > Explicitly disable PEC when the client does not support it.
> > Without the explicit disable, when the device with the PEC support is removed
> > later when a device without PEC support is inserted into the same address,
> > the driver uses the old value of client->flags which contains the I2C_CLIENT_PEC
> > flag. As a consequence the PEC is used when it should not.
>
>
> How can that happen ? I would assume the I2C device gets deleted and re-created
> in that case, which should clear the PEC flag.
>
> Guenter
In my case it was when I unloaded the driver for the I2C slave, changed the advertised PEC value in PMBUS_CAPABILITY register on slave. Then loaded the driver. When the switch was from disable->enable it worked as expected (this case was already covered), but when the PEC was set in the slave from enabled->disabled it was still using PEC to communicate.

Adam
>
> > Signed-off-by: Adam Wujek dev_public@wujek.eu
> > ---
> > drivers/hwmon/pmbus/pmbus_core.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> > index 82c3754e21e3..f8ca36759b0a 100644
> > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > @@ -2014,6 +2014,8 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
> > ret = i2c_smbus_read_byte_data(client, PMBUS_CAPABILITY);
> > if (ret >= 0 && (ret & PB_CAPABILITY_ERROR_CHECK))
> > client->flags |= I2C_CLIENT_PEC;
> > + else
> > + client->flags &= ~I2C_CLIENT_PEC;
> >
> > pmbus_clear_faults(client);
> >
> > --
> > 2.17.1

Re: [PATCH] hwmod: (pmbus) disable PEC if not enabled

[Guenter Roeck]

On 4/19/22 15:10, wujek dev wrote:
> ------- Original Message -------
> On Wednesday, April 20th, 2022 at 00:00, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>>
>> On 4/19/22 13:53, Adam Wujek wrote:
>>
>>> Explicitly disable PEC when the client does not support it.
>>> Without the explicit disable, when the device with the PEC support is removed
>>> later when a device without PEC support is inserted into the same address,
>>> the driver uses the old value of client->flags which contains the I2C_CLIENT_PEC
>>> flag. As a consequence the PEC is used when it should not.
>>
>>
>> How can that happen ? I would assume the I2C device gets deleted and re-created
>> in that case, which should clear the PEC flag.
>>
>> Guenter
> In my case it was when I unloaded the driver for the I2C slave, changed the advertised PEC value in PMBUS_CAPABILITY register on slave. Then loaded the driver. When the switch was from disable->enable it worked as expected (this case was already covered), but when the PEC was set in the slave from enabled->disabled it was still using PEC to communicate.

So it is really the same device, only you unload the driver, change the
device configuration (presumably with i2cset commands), and load it
again. Please explain that in more detail in the commit description.

Thanks,
Guenter

Re: [PATCH] hwmod: (pmbus) disable PEC if not enabled

[Guenter Roeck]

On 4/19/22 13:53, Adam Wujek wrote:
> Explicitly disable PEC when the client does not support it.
> Without the explicit disable, when the device with the PEC support is removed
> later when a device without PEC support is inserted into the same address,
> the driver uses the old value of client->flags which contains the I2C_CLIENT_PEC
> flag. As a consequence the PEC is used when it should not.
> 
> Signed-off-by: Adam Wujek <dev_public@wujek.eu>
> ---
>   drivers/hwmon/pmbus/pmbus_core.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 82c3754e21e3..f8ca36759b0a 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2014,6 +2014,8 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
>   	ret = i2c_smbus_read_byte_data(client, PMBUS_CAPABILITY);
>   	if (ret >= 0 && (ret & PB_CAPABILITY_ERROR_CHECK))
>   		client->flags |= I2C_CLIENT_PEC;
> +	else
> +		client->flags &= ~I2C_CLIENT_PEC;
> 

I just realized that this patch is not based on the latest
kernel version. Please rebase.

Guenter

[PATCH] hwmod: (pmbus) disable PEC if not enabled

[Adam Wujek]

Explicitly disable PEC when the client does not support it.
The problematic scenario is the following. A device with enabled PEC
support is up and running, a kernel driver loaded.
Then the driver is unloaded (or device unbound), the HW device
is reconfigured externally (e.g. by i2cset) to advertise itself as not
supporting PEC. Without a new code, at the second load of the driver
(or bind) the "flags" variable is not updated to avoid PEC usage. As a
consequence the further communication with the device is done with
the PEC enabled, which is wrong.

Signed-off-by: Adam Wujek <dev_public@wujek.eu>
---
 drivers/hwmon/pmbus/pmbus_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index b2618b1d529e..0af7a3d74f47 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2334,7 +2334,8 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 				client->flags |= I2C_CLIENT_PEC;
 			}
 		}
-	}
+	} else
+		client->flags &= ~I2C_CLIENT_PEC;

 	/*
 	 * Check if the chip is write protected. If it is, we can not clear
--
2.25.1

Re: [PATCH] hwmod: (pmbus) disable PEC if not enabled

[Guenter Roeck]

On 4/20/22 05:54, Adam Wujek wrote:
> Explicitly disable PEC when the client does not support it.
> The problematic scenario is the following. A device with enabled PEC
> support is up and running, a kernel driver loaded.
> Then the driver is unloaded (or device unbound), the HW device
> is reconfigured externally (e.g. by i2cset) to advertise itself as not
> supporting PEC. Without a new code, at the second load of the driver
> (or bind) the "flags" variable is not updated to avoid PEC usage. As a
> consequence the further communication with the device is done with
> the PEC enabled, which is wrong.
> 
> Signed-off-by: Adam Wujek <dev_public@wujek.eu>

Subject should start with hwmon:. Please version your patches,
and provide change logs.

> ---
>   drivers/hwmon/pmbus/pmbus_core.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index b2618b1d529e..0af7a3d74f47 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2334,7 +2334,8 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
>   				client->flags |= I2C_CLIENT_PEC;
>   			}
>   		}
> -	}
> +	} else
> +		client->flags &= ~I2C_CLIENT_PEC;

Since if() is in {}, else should be in {} as well.

Guenter

> 
>   	/*
>   	 * Check if the chip is write protected. If it is, we can not clear
> --
> 2.25.1
> 
> 

[v3 PATCH] hwmon: (pmbus) disable PEC if not enabled

[Adam Wujek]

Explicitly disable PEC when the client does not support it.
The problematic scenario is the following. A device with enabled PEC
support is up and running and a kernel driver is loaded.
Then the driver is unloaded (or device unbound), the HW device
is reconfigured externally (e.g. by i2cset) to advertise itself as not
supporting PEC. Without a new code, at the second load of the driver
(or bind) the "flags" variable is not updated to avoid PEC usage. As a
consequence the further communication with the device is done with
the PEC enabled, which is wrong and may fail.

The implementation first disable the I2C_CLIENT_PEC flag, then the old
code enable it if needed.

Signed-off-by: Adam Wujek <dev_public@wujek.eu>
---
Notes:
    Changes in v2:
    - Rebase to the latest kernel
    - Update commit message

    Changes in v3:
    - Rework the patch, disable the flag first, then enable if needed.
      Adding three else statements to disable the flag will make the code
      less readable.
    - Update commit message

 drivers/hwmon/pmbus/pmbus_core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index b2618b1d529e..d93574d6a1fb 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2326,6 +2326,9 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 		data->has_status_word = true;
 	}

+	/* Make sure PEC is disabled, will be enabled later if needed */
+	client->flags &= ~I2C_CLIENT_PEC;
+
 	/* Enable PEC if the controller and bus supports it */
 	if (!(data->flags & PMBUS_NO_CAPABILITY)) {
 		ret = i2c_smbus_read_byte_data(client, PMBUS_CAPABILITY);
--
2.25.1