Commit c1e62062ff54 ("iio: light: cm32181: Handle CM3218 ACPI devices
with 2 I2C resources") creates a second client for the actual I2C
address, but the "struct device" passed to PM ops is the first client
that can't talk to the sensor.
That means the I2C transfers in both suspend and resume routines can
fail and blocking the whole suspend process.
Instead of using the first client for I2C transfer, store the cm32181
private struct on both cases so the PM ops can get the correct I2C
client to perfrom suspend and resume.
Fixes: 68c1b3dd5c48 ("iio: light: cm32181: Add PM support")
Tested-by: Wahaj <[email protected]>
Signed-off-by: Kai-Heng Feng <[email protected]>
---
drivers/iio/light/cm32181.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
index 001055d097509..0f319c891353c 100644
--- a/drivers/iio/light/cm32181.c
+++ b/drivers/iio/light/cm32181.c
@@ -440,6 +440,8 @@ static int cm32181_probe(struct i2c_client *client)
if (!indio_dev)
return -ENOMEM;
+ i2c_set_clientdata(client, indio_dev);
+
/*
* Some ACPI systems list 2 I2C resources for the CM3218 sensor, the
* SMBus Alert Response Address (ARA, 0x0c) and the actual I2C address.
@@ -458,9 +460,9 @@ static int cm32181_probe(struct i2c_client *client)
client = i2c_acpi_new_device(dev, 1, &board_info);
if (IS_ERR(client))
return PTR_ERR(client);
- }
- i2c_set_clientdata(client, indio_dev);
+ i2c_set_clientdata(client, indio_dev);
+ }
cm32181 = iio_priv(indio_dev);
cm32181->client = client;
@@ -490,7 +492,8 @@ static int cm32181_probe(struct i2c_client *client)
static int cm32181_suspend(struct device *dev)
{
- struct i2c_client *client = to_i2c_client(dev);
+ struct cm32181_chip *cm32181 = iio_priv(dev_get_drvdata(dev));
+ struct i2c_client *client = cm32181->client;
return i2c_smbus_write_word_data(client, CM32181_REG_ADDR_CMD,
CM32181_CMD_ALS_DISABLE);
@@ -498,8 +501,8 @@ static int cm32181_suspend(struct device *dev)
static int cm32181_resume(struct device *dev)
{
- struct i2c_client *client = to_i2c_client(dev);
struct cm32181_chip *cm32181 = iio_priv(dev_get_drvdata(dev));
+ struct i2c_client *client = cm32181->client;
return i2c_smbus_write_word_data(client, CM32181_REG_ADDR_CMD,
cm32181->conf_regs[CM32181_REG_ADDR_CMD]);
--
2.34.1
Hi,
On 1/17/23 17:09, Kai-Heng Feng wrote:
> Commit c1e62062ff54 ("iio: light: cm32181: Handle CM3218 ACPI devices
> with 2 I2C resources") creates a second client for the actual I2C
> address, but the "struct device" passed to PM ops is the first client
> that can't talk to the sensor.
>
> That means the I2C transfers in both suspend and resume routines can
> fail and blocking the whole suspend process.
>
> Instead of using the first client for I2C transfer, store the cm32181
> private struct on both cases so the PM ops can get the correct I2C
> client to perfrom suspend and resume.
>
> Fixes: 68c1b3dd5c48 ("iio: light: cm32181: Add PM support")
> Tested-by: Wahaj <[email protected]>
> Signed-off-by: Kai-Heng Feng <[email protected]>
Thank you for this fix. I had looking into this on my todo list,
since I have been seeing some bug reports about this too.
One remark inline:
> ---
> drivers/iio/light/cm32181.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
> index 001055d097509..0f319c891353c 100644
> --- a/drivers/iio/light/cm32181.c
> +++ b/drivers/iio/light/cm32181.c
> @@ -440,6 +440,8 @@ static int cm32181_probe(struct i2c_client *client)
> if (!indio_dev)
> return -ENOMEM;
>
> + i2c_set_clientdata(client, indio_dev);
> +
Why move this up, the suspend/resume callbacks cannot run until
probe() completes, so no need for this change.
> /*
> * Some ACPI systems list 2 I2C resources for the CM3218 sensor, the
> * SMBus Alert Response Address (ARA, 0x0c) and the actual I2C address.
> @@ -458,9 +460,9 @@ static int cm32181_probe(struct i2c_client *client)
> client = i2c_acpi_new_device(dev, 1, &board_info);
> if (IS_ERR(client))
> return PTR_ERR(client);
> - }
>
> - i2c_set_clientdata(client, indio_dev);
> + i2c_set_clientdata(client, indio_dev);
> + }
And moving it inside the if block here (instead of just dropping it)
is also weird. I guess you meant to just delete it since you moved it up.
>
> cm32181 = iio_priv(indio_dev);
> cm32181->client = client;
Also note that the ->client used in suspend/resume now is not set until
here, so moving the i2c_set_clientdata() up really does not do anything.
I beleive it would be best to just these 2 hunks from the patch and
only keep the changes to the suspend/resume callbacks.
Regards,
Hans
> @@ -490,7 +492,8 @@ static int cm32181_probe(struct i2c_client *client)
>
> static int cm32181_suspend(struct device *dev)
> {
> - struct i2c_client *client = to_i2c_client(dev);
> + struct cm32181_chip *cm32181 = iio_priv(dev_get_drvdata(dev));
> + struct i2c_client *client = cm32181->client;
>
> return i2c_smbus_write_word_data(client, CM32181_REG_ADDR_CMD,
> CM32181_CMD_ALS_DISABLE);
> @@ -498,8 +501,8 @@ static int cm32181_suspend(struct device *dev)
>
> static int cm32181_resume(struct device *dev)
> {
> - struct i2c_client *client = to_i2c_client(dev);
> struct cm32181_chip *cm32181 = iio_priv(dev_get_drvdata(dev));
> + struct i2c_client *client = cm32181->client;
>
> return i2c_smbus_write_word_data(client, CM32181_REG_ADDR_CMD,
> cm32181->conf_regs[CM32181_REG_ADDR_CMD]);
Hi,
On 1/17/23 18:19, Hans de Goede wrote:
> Hi,
>
> On 1/17/23 17:09, Kai-Heng Feng wrote:
>> Commit c1e62062ff54 ("iio: light: cm32181: Handle CM3218 ACPI devices
>> with 2 I2C resources") creates a second client for the actual I2C
>> address, but the "struct device" passed to PM ops is the first client
>> that can't talk to the sensor.
>>
>> That means the I2C transfers in both suspend and resume routines can
>> fail and blocking the whole suspend process.
>>
>> Instead of using the first client for I2C transfer, store the cm32181
>> private struct on both cases so the PM ops can get the correct I2C
>> client to perfrom suspend and resume.
>>
>> Fixes: 68c1b3dd5c48 ("iio: light: cm32181: Add PM support")
>> Tested-by: Wahaj <[email protected]>
>> Signed-off-by: Kai-Heng Feng <[email protected]>
>
> Thank you for this fix. I had looking into this on my todo list,
> since I have been seeing some bug reports about this too.
>
> One remark inline:
>
>> ---
>> drivers/iio/light/cm32181.c | 11 +++++++----
>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
>> index 001055d097509..0f319c891353c 100644
>> --- a/drivers/iio/light/cm32181.c
>> +++ b/drivers/iio/light/cm32181.c
>> @@ -440,6 +440,8 @@ static int cm32181_probe(struct i2c_client *client)
>> if (!indio_dev)
>> return -ENOMEM;
>>
>> + i2c_set_clientdata(client, indio_dev);
>> +
>
> Why move this up, the suspend/resume callbacks cannot run until
> probe() completes, so no need for this change.
>
>> /*
>> * Some ACPI systems list 2 I2C resources for the CM3218 sensor, the
>> * SMBus Alert Response Address (ARA, 0x0c) and the actual I2C address.
>> @@ -458,9 +460,9 @@ static int cm32181_probe(struct i2c_client *client)
>> client = i2c_acpi_new_device(dev, 1, &board_info);
>> if (IS_ERR(client))
>> return PTR_ERR(client);
>> - }
>>
>> - i2c_set_clientdata(client, indio_dev);
>> + i2c_set_clientdata(client, indio_dev);
>> + }
>
> And moving it inside the if block here (instead of just dropping it)
> is also weird. I guess you meant to just delete it since you moved it up.
>
>>
>> cm32181 = iio_priv(indio_dev);
>> cm32181->client = client;
>
> Also note that the ->client used in suspend/resume now is not set until
> here, so moving the i2c_set_clientdata() up really does not do anything.
>
> I beleive it would be best to just these 2 hunks from the patch and
> only keep the changes to the suspend/resume callbacks.
p.s.
I believe that this will likely also fix:
https://bugzilla.redhat.com/show_bug.cgi?id=2152281
Can you please add a:
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2152281
to the next version.
Regards,
Hans
>
>
>> @@ -490,7 +492,8 @@ static int cm32181_probe(struct i2c_client *client)
>>
>> static int cm32181_suspend(struct device *dev)
>> {
>> - struct i2c_client *client = to_i2c_client(dev);
>> + struct cm32181_chip *cm32181 = iio_priv(dev_get_drvdata(dev));
>> + struct i2c_client *client = cm32181->client;
>>
>> return i2c_smbus_write_word_data(client, CM32181_REG_ADDR_CMD,
>> CM32181_CMD_ALS_DISABLE);
>> @@ -498,8 +501,8 @@ static int cm32181_suspend(struct device *dev)
>>
>> static int cm32181_resume(struct device *dev)
>> {
>> - struct i2c_client *client = to_i2c_client(dev);
>> struct cm32181_chip *cm32181 = iio_priv(dev_get_drvdata(dev));
>> + struct i2c_client *client = cm32181->client;
>>
>> return i2c_smbus_write_word_data(client, CM32181_REG_ADDR_CMD,
>> cm32181->conf_regs[CM32181_REG_ADDR_CMD]);
>
Hi Hans,
On Wed, Jan 18, 2023 at 1:21 AM Hans de Goede <[email protected]> wrote:
>
> Hi,
>
> On 1/17/23 17:09, Kai-Heng Feng wrote:
> > Commit c1e62062ff54 ("iio: light: cm32181: Handle CM3218 ACPI devices
> > with 2 I2C resources") creates a second client for the actual I2C
> > address, but the "struct device" passed to PM ops is the first client
> > that can't talk to the sensor.
> >
> > That means the I2C transfers in both suspend and resume routines can
> > fail and blocking the whole suspend process.
> >
> > Instead of using the first client for I2C transfer, store the cm32181
> > private struct on both cases so the PM ops can get the correct I2C
> > client to perfrom suspend and resume.
> >
> > Fixes: 68c1b3dd5c48 ("iio: light: cm32181: Add PM support")
> > Tested-by: Wahaj <[email protected]>
> > Signed-off-by: Kai-Heng Feng <[email protected]>
>
> Thank you for this fix. I had looking into this on my todo list,
> since I have been seeing some bug reports about this too.
>
> One remark inline:
>
> > ---
> > drivers/iio/light/cm32181.c | 11 +++++++----
> > 1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
> > index 001055d097509..0f319c891353c 100644
> > --- a/drivers/iio/light/cm32181.c
> > +++ b/drivers/iio/light/cm32181.c
> > @@ -440,6 +440,8 @@ static int cm32181_probe(struct i2c_client *client)
> > if (!indio_dev)
> > return -ENOMEM;
> >
> > + i2c_set_clientdata(client, indio_dev);
> > +
>
> Why move this up, the suspend/resume callbacks cannot run until
> probe() completes, so no need for this change.
The intention is to save indio_dev as drvdata in the first (i.e.
original) i2c_client's dev.
>
> > /*
> > * Some ACPI systems list 2 I2C resources for the CM3218 sensor, the
> > * SMBus Alert Response Address (ARA, 0x0c) and the actual I2C address.
> > @@ -458,9 +460,9 @@ static int cm32181_probe(struct i2c_client *client)
> > client = i2c_acpi_new_device(dev, 1, &board_info);
> > if (IS_ERR(client))
> > return PTR_ERR(client);
> > - }
> >
> > - i2c_set_clientdata(client, indio_dev);
> > + i2c_set_clientdata(client, indio_dev);
> > + }
>
> And moving it inside the if block here (instead of just dropping it)
> is also weird. I guess you meant to just delete it since you moved it up.
Doesn't i2c_acpi_new_device() creates a new i2c_client (and its dev embedded)?
So the intention is to save indio_dev for the second (ARA case) i2c_client too.
>
> >
> > cm32181 = iio_priv(indio_dev);
> > cm32181->client = client;
>
> Also note that the ->client used in suspend/resume now is not set until
> here, so moving the i2c_set_clientdata() up really does not do anything.
>
> I beleive it would be best to just these 2 hunks from the patch and
> only keep the changes to the suspend/resume callbacks.
Yes, it seems like those 2 hunks are not necessary. Let me send a new patch.
But I do wonder what happens for the removing case? Will the second
i2c_client leak?
Kai-Heng
>
> Regards,
>
> Hans
>
>
> > @@ -490,7 +492,8 @@ static int cm32181_probe(struct i2c_client *client)
> >
> > static int cm32181_suspend(struct device *dev)
> > {
> > - struct i2c_client *client = to_i2c_client(dev);
> > + struct cm32181_chip *cm32181 = iio_priv(dev_get_drvdata(dev));
> > + struct i2c_client *client = cm32181->client;
> >
> > return i2c_smbus_write_word_data(client, CM32181_REG_ADDR_CMD,
> > CM32181_CMD_ALS_DISABLE);
> > @@ -498,8 +501,8 @@ static int cm32181_suspend(struct device *dev)
> >
> > static int cm32181_resume(struct device *dev)
> > {
> > - struct i2c_client *client = to_i2c_client(dev);
> > struct cm32181_chip *cm32181 = iio_priv(dev_get_drvdata(dev));
> > + struct i2c_client *client = cm32181->client;
> >
> > return i2c_smbus_write_word_data(client, CM32181_REG_ADDR_CMD,
> > cm32181->conf_regs[CM32181_REG_ADDR_CMD]);
>
On Wed, Jan 18, 2023 at 11:29 AM Kai-Heng Feng
<[email protected]> wrote:
>
> Hi Hans,
>
> On Wed, Jan 18, 2023 at 1:21 AM Hans de Goede <[email protected]> wrote:
> >
> > Hi,
> >
> > On 1/17/23 17:09, Kai-Heng Feng wrote:
> > > Commit c1e62062ff54 ("iio: light: cm32181: Handle CM3218 ACPI devices
> > > with 2 I2C resources") creates a second client for the actual I2C
> > > address, but the "struct device" passed to PM ops is the first client
> > > that can't talk to the sensor.
> > >
> > > That means the I2C transfers in both suspend and resume routines can
> > > fail and blocking the whole suspend process.
> > >
> > > Instead of using the first client for I2C transfer, store the cm32181
> > > private struct on both cases so the PM ops can get the correct I2C
> > > client to perfrom suspend and resume.
> > >
> > > Fixes: 68c1b3dd5c48 ("iio: light: cm32181: Add PM support")
> > > Tested-by: Wahaj <[email protected]>
> > > Signed-off-by: Kai-Heng Feng <[email protected]>
> >
> > Thank you for this fix. I had looking into this on my todo list,
> > since I have been seeing some bug reports about this too.
> >
> > One remark inline:
> >
> > > ---
> > > drivers/iio/light/cm32181.c | 11 +++++++----
> > > 1 file changed, 7 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
> > > index 001055d097509..0f319c891353c 100644
> > > --- a/drivers/iio/light/cm32181.c
> > > +++ b/drivers/iio/light/cm32181.c
> > > @@ -440,6 +440,8 @@ static int cm32181_probe(struct i2c_client *client)
> > > if (!indio_dev)
> > > return -ENOMEM;
> > >
> > > + i2c_set_clientdata(client, indio_dev);
> > > +
> >
> > Why move this up, the suspend/resume callbacks cannot run until
> > probe() completes, so no need for this change.
>
> The intention is to save indio_dev as drvdata in the first (i.e.
> original) i2c_client's dev.
>
> >
> > > /*
> > > * Some ACPI systems list 2 I2C resources for the CM3218 sensor, the
> > > * SMBus Alert Response Address (ARA, 0x0c) and the actual I2C address.
> > > @@ -458,9 +460,9 @@ static int cm32181_probe(struct i2c_client *client)
> > > client = i2c_acpi_new_device(dev, 1, &board_info);
> > > if (IS_ERR(client))
> > > return PTR_ERR(client);
> > > - }
> > >
> > > - i2c_set_clientdata(client, indio_dev);
> > > + i2c_set_clientdata(client, indio_dev);
> > > + }
> >
> > And moving it inside the if block here (instead of just dropping it)
> > is also weird. I guess you meant to just delete it since you moved it up.
>
> Doesn't i2c_acpi_new_device() creates a new i2c_client (and its dev embedded)?
>
> So the intention is to save indio_dev for the second (ARA case) i2c_client too.
>
> >
> > >
> > > cm32181 = iio_priv(indio_dev);
> > > cm32181->client = client;
> >
> > Also note that the ->client used in suspend/resume now is not set until
> > here, so moving the i2c_set_clientdata() up really does not do anything.
> >
> > I beleive it would be best to just these 2 hunks from the patch and
> > only keep the changes to the suspend/resume callbacks.
>
> Yes, it seems like those 2 hunks are not necessary. Let me send a new patch.
if (ACPI_HANDLE(dev) && client->addr == SMBUS_ALERT_RESPONSE_ADDRESS) {
...
client = i2c_acpi_new_device(dev, 1, &board_info);
...
}
i2c_set_clientdata(client, indio_dev);
It means the indio_dev is only assigned to the new i2c_client->dev's
drvdata, the original dev's drvdata remains NULL.
So we need to assign it before the original client gets replaced by
the new one, otherwise we can't get cm32181 in PM ops.
Kai-Heng
>
> But I do wonder what happens for the removing case? Will the second
> i2c_client leak?
>
> Kai-Heng
>
> >
> > Regards,
> >
> > Hans
> >
> >
> > > @@ -490,7 +492,8 @@ static int cm32181_probe(struct i2c_client *client)
> > >
> > > static int cm32181_suspend(struct device *dev)
> > > {
> > > - struct i2c_client *client = to_i2c_client(dev);
> > > + struct cm32181_chip *cm32181 = iio_priv(dev_get_drvdata(dev));
> > > + struct i2c_client *client = cm32181->client;
> > >
> > > return i2c_smbus_write_word_data(client, CM32181_REG_ADDR_CMD,
> > > CM32181_CMD_ALS_DISABLE);
> > > @@ -498,8 +501,8 @@ static int cm32181_suspend(struct device *dev)
> > >
> > > static int cm32181_resume(struct device *dev)
> > > {
> > > - struct i2c_client *client = to_i2c_client(dev);
> > > struct cm32181_chip *cm32181 = iio_priv(dev_get_drvdata(dev));
> > > + struct i2c_client *client = cm32181->client;
> > >
> > > return i2c_smbus_write_word_data(client, CM32181_REG_ADDR_CMD,
> > > cm32181->conf_regs[CM32181_REG_ADDR_CMD]);
> >
Hi,
On 1/18/23 06:15, Kai-Heng Feng wrote:
> On Wed, Jan 18, 2023 at 11:29 AM Kai-Heng Feng
> <[email protected]> wrote:
>>
>> Hi Hans,
>>
>> On Wed, Jan 18, 2023 at 1:21 AM Hans de Goede <[email protected]> wrote:
>>>
>>> Hi,
>>>
>>> On 1/17/23 17:09, Kai-Heng Feng wrote:
>>>> Commit c1e62062ff54 ("iio: light: cm32181: Handle CM3218 ACPI devices
>>>> with 2 I2C resources") creates a second client for the actual I2C
>>>> address, but the "struct device" passed to PM ops is the first client
>>>> that can't talk to the sensor.
>>>>
>>>> That means the I2C transfers in both suspend and resume routines can
>>>> fail and blocking the whole suspend process.
>>>>
>>>> Instead of using the first client for I2C transfer, store the cm32181
>>>> private struct on both cases so the PM ops can get the correct I2C
>>>> client to perfrom suspend and resume.
>>>>
>>>> Fixes: 68c1b3dd5c48 ("iio: light: cm32181: Add PM support")
>>>> Tested-by: Wahaj <[email protected]>
>>>> Signed-off-by: Kai-Heng Feng <[email protected]>
>>>
>>> Thank you for this fix. I had looking into this on my todo list,
>>> since I have been seeing some bug reports about this too.
>>>
>>> One remark inline:
>>>
>>>> ---
>>>> drivers/iio/light/cm32181.c | 11 +++++++----
>>>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
>>>> index 001055d097509..0f319c891353c 100644
>>>> --- a/drivers/iio/light/cm32181.c
>>>> +++ b/drivers/iio/light/cm32181.c
>>>> @@ -440,6 +440,8 @@ static int cm32181_probe(struct i2c_client *client)
>>>> if (!indio_dev)
>>>> return -ENOMEM;
>>>>
>>>> + i2c_set_clientdata(client, indio_dev);
>>>> +
>>>
>>> Why move this up, the suspend/resume callbacks cannot run until
>>> probe() completes, so no need for this change.
>>
>> The intention is to save indio_dev as drvdata in the first (i.e.
>> original) i2c_client's dev.
>>
>>>
>>>> /*
>>>> * Some ACPI systems list 2 I2C resources for the CM3218 sensor, the
>>>> * SMBus Alert Response Address (ARA, 0x0c) and the actual I2C address.
>>>> @@ -458,9 +460,9 @@ static int cm32181_probe(struct i2c_client *client)
>>>> client = i2c_acpi_new_device(dev, 1, &board_info);
>>>> if (IS_ERR(client))
>>>> return PTR_ERR(client);
>>>> - }
>>>>
>>>> - i2c_set_clientdata(client, indio_dev);
>>>> + i2c_set_clientdata(client, indio_dev);
>>>> + }
>>>
>>> And moving it inside the if block here (instead of just dropping it)
>>> is also weird. I guess you meant to just delete it since you moved it up.
>>
>> Doesn't i2c_acpi_new_device() creates a new i2c_client (and its dev embedded)?
>>
>> So the intention is to save indio_dev for the second (ARA case) i2c_client too.
>>
>>>
>>>>
>>>> cm32181 = iio_priv(indio_dev);
>>>> cm32181->client = client;
>>>
>>> Also note that the ->client used in suspend/resume now is not set until
>>> here, so moving the i2c_set_clientdata() up really does not do anything.
>>>
>>> I beleive it would be best to just these 2 hunks from the patch and
>>> only keep the changes to the suspend/resume callbacks.
>>
>> Yes, it seems like those 2 hunks are not necessary. Let me send a new patch.
>
> if (ACPI_HANDLE(dev) && client->addr == SMBUS_ALERT_RESPONSE_ADDRESS) {
> ...
> client = i2c_acpi_new_device(dev, 1, &board_info);
> ...
> }
> i2c_set_clientdata(client, indio_dev);
>
> It means the indio_dev is only assigned to the new i2c_client->dev's
> drvdata, the original dev's drvdata remains NULL.
> So we need to assign it before the original client gets replaced by
> the new one, otherwise we can't get cm32181 in PM ops.
You are right, my bad. The original code has a bug where it indeed was
making the i2c_set_clientdata() call on the wrong client device.
So the i2c_set_clientdata() call needs to be moved up.
There is no need to also call i2c_set_clientdata() on the dummy
i2c-client though. That one does not have a driver attached.
The suspend/resume callbacks are made on the original client-dev,
not on the one of the dummy-client (which is the one which we
actually use to communicate).
>> But I do wonder what happens for the removing case? Will the second
>> i2c_client leak?
Yes it does, good point. That should probably also be fixed, but
that needs to be a different / second patch.
Regards,
Hans
>>>> @@ -490,7 +492,8 @@ static int cm32181_probe(struct i2c_client *client)
>>>>
>>>> static int cm32181_suspend(struct device *dev)
>>>> {
>>>> - struct i2c_client *client = to_i2c_client(dev);
>>>> + struct cm32181_chip *cm32181 = iio_priv(dev_get_drvdata(dev));
>>>> + struct i2c_client *client = cm32181->client;
>>>>
>>>> return i2c_smbus_write_word_data(client, CM32181_REG_ADDR_CMD,
>>>> CM32181_CMD_ALS_DISABLE);
>>>> @@ -498,8 +501,8 @@ static int cm32181_suspend(struct device *dev)
>>>>
>>>> static int cm32181_resume(struct device *dev)
>>>> {
>>>> - struct i2c_client *client = to_i2c_client(dev);
>>>> struct cm32181_chip *cm32181 = iio_priv(dev_get_drvdata(dev));
>>>> + struct i2c_client *client = cm32181->client;
>>>>
>>>> return i2c_smbus_write_word_data(client, CM32181_REG_ADDR_CMD,
>>>> cm32181->conf_regs[CM32181_REG_ADDR_CMD]);
>>>
>
Hi Hans,
On Wed, Jan 18, 2023 at 6:52 PM Hans de Goede <[email protected]> wrote:
>
> Hi,
>
> On 1/18/23 06:15, Kai-Heng Feng wrote:
> > On Wed, Jan 18, 2023 at 11:29 AM Kai-Heng Feng
> > <[email protected]> wrote:
> >>
> >> Hi Hans,
> >>
> >> On Wed, Jan 18, 2023 at 1:21 AM Hans de Goede <[email protected]> wrote:
> >>>
> >>> Hi,
> >>>
> >>> On 1/17/23 17:09, Kai-Heng Feng wrote:
> >>>> Commit c1e62062ff54 ("iio: light: cm32181: Handle CM3218 ACPI devices
> >>>> with 2 I2C resources") creates a second client for the actual I2C
> >>>> address, but the "struct device" passed to PM ops is the first client
> >>>> that can't talk to the sensor.
> >>>>
> >>>> That means the I2C transfers in both suspend and resume routines can
> >>>> fail and blocking the whole suspend process.
> >>>>
> >>>> Instead of using the first client for I2C transfer, store the cm32181
> >>>> private struct on both cases so the PM ops can get the correct I2C
> >>>> client to perfrom suspend and resume.
> >>>>
> >>>> Fixes: 68c1b3dd5c48 ("iio: light: cm32181: Add PM support")
> >>>> Tested-by: Wahaj <[email protected]>
> >>>> Signed-off-by: Kai-Heng Feng <[email protected]>
> >>>
> >>> Thank you for this fix. I had looking into this on my todo list,
> >>> since I have been seeing some bug reports about this too.
> >>>
> >>> One remark inline:
> >>>
> >>>> ---
> >>>> drivers/iio/light/cm32181.c | 11 +++++++----
> >>>> 1 file changed, 7 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
> >>>> index 001055d097509..0f319c891353c 100644
> >>>> --- a/drivers/iio/light/cm32181.c
> >>>> +++ b/drivers/iio/light/cm32181.c
> >>>> @@ -440,6 +440,8 @@ static int cm32181_probe(struct i2c_client *client)
> >>>> if (!indio_dev)
> >>>> return -ENOMEM;
> >>>>
> >>>> + i2c_set_clientdata(client, indio_dev);
> >>>> +
> >>>
> >>> Why move this up, the suspend/resume callbacks cannot run until
> >>> probe() completes, so no need for this change.
> >>
> >> The intention is to save indio_dev as drvdata in the first (i.e.
> >> original) i2c_client's dev.
> >>
> >>>
> >>>> /*
> >>>> * Some ACPI systems list 2 I2C resources for the CM3218 sensor, the
> >>>> * SMBus Alert Response Address (ARA, 0x0c) and the actual I2C address.
> >>>> @@ -458,9 +460,9 @@ static int cm32181_probe(struct i2c_client *client)
> >>>> client = i2c_acpi_new_device(dev, 1, &board_info);
> >>>> if (IS_ERR(client))
> >>>> return PTR_ERR(client);
> >>>> - }
> >>>>
> >>>> - i2c_set_clientdata(client, indio_dev);
> >>>> + i2c_set_clientdata(client, indio_dev);
> >>>> + }
> >>>
> >>> And moving it inside the if block here (instead of just dropping it)
> >>> is also weird. I guess you meant to just delete it since you moved it up.
> >>
> >> Doesn't i2c_acpi_new_device() creates a new i2c_client (and its dev embedded)?
> >>
> >> So the intention is to save indio_dev for the second (ARA case) i2c_client too.
> >>
> >>>
> >>>>
> >>>> cm32181 = iio_priv(indio_dev);
> >>>> cm32181->client = client;
> >>>
> >>> Also note that the ->client used in suspend/resume now is not set until
> >>> here, so moving the i2c_set_clientdata() up really does not do anything.
> >>>
> >>> I beleive it would be best to just these 2 hunks from the patch and
> >>> only keep the changes to the suspend/resume callbacks.
> >>
> >> Yes, it seems like those 2 hunks are not necessary. Let me send a new patch.
> >
> > if (ACPI_HANDLE(dev) && client->addr == SMBUS_ALERT_RESPONSE_ADDRESS) {
> > ...
> > client = i2c_acpi_new_device(dev, 1, &board_info);
> > ...
> > }
> > i2c_set_clientdata(client, indio_dev);
> >
> > It means the indio_dev is only assigned to the new i2c_client->dev's
> > drvdata, the original dev's drvdata remains NULL.
> > So we need to assign it before the original client gets replaced by
> > the new one, otherwise we can't get cm32181 in PM ops.
>
> You are right, my bad. The original code has a bug where it indeed was
> making the i2c_set_clientdata() call on the wrong client device.
>
> So the i2c_set_clientdata() call needs to be moved up.
>
> There is no need to also call i2c_set_clientdata() on the dummy
> i2c-client though. That one does not have a driver attached.
Sure, will update this in v2.
>
> The suspend/resume callbacks are made on the original client-dev,
> not on the one of the dummy-client (which is the one which we
> actually use to communicate).
>
> >> But I do wonder what happens for the removing case? Will the second
> >> i2c_client leak?
>
> Yes it does, good point. That should probably also be fixed, but
> that needs to be a different / second patch.
Agree, thanks for the input.
Kai-Heng
>
> Regards,
>
> Hans
>
>
>
> >>>> @@ -490,7 +492,8 @@ static int cm32181_probe(struct i2c_client *client)
> >>>>
> >>>> static int cm32181_suspend(struct device *dev)
> >>>> {
> >>>> - struct i2c_client *client = to_i2c_client(dev);
> >>>> + struct cm32181_chip *cm32181 = iio_priv(dev_get_drvdata(dev));
> >>>> + struct i2c_client *client = cm32181->client;
> >>>>
> >>>> return i2c_smbus_write_word_data(client, CM32181_REG_ADDR_CMD,
> >>>> CM32181_CMD_ALS_DISABLE);
> >>>> @@ -498,8 +501,8 @@ static int cm32181_suspend(struct device *dev)
> >>>>
> >>>> static int cm32181_resume(struct device *dev)
> >>>> {
> >>>> - struct i2c_client *client = to_i2c_client(dev);
> >>>> struct cm32181_chip *cm32181 = iio_priv(dev_get_drvdata(dev));
> >>>> + struct i2c_client *client = cm32181->client;
> >>>>
> >>>> return i2c_smbus_write_word_data(client, CM32181_REG_ADDR_CMD,
> >>>> cm32181->conf_regs[CM32181_REG_ADDR_CMD]);
> >>>
> >
>