linux-kernel - Re: [PATCH v2 2/2] iio: si7020: Lock root adapter to wait for reset

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c7f6baa3-8181-a612-72cc-23f570f31ab3@linux.ibm.com>
Date:   Wed, 7 Sep 2022 08:53:20 -0500
From:   Eddie James <eajames@...ux.ibm.com>
To:     Peter Rosin <peda@...ntia.se>, linux-i2c@...r.kernel.org
Cc:     linux-iio@...r.kernel.org, wsa@...nel.org, jic23@...nel.org,
        lars@...afoo.de, miltonm@...ibm.com, joel@....id.au,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] iio: si7020: Lock root adapter to wait for reset


On 9/7/22 02:10, Peter Rosin wrote:
> Hi!
>
> First off, I'm very sorry for being too busy and too unresponsive.
>
> 2022-09-06 at 22:28, Eddie James wrote:
>> Use the new mux root operations to lock the root adapter while waiting for
>> the reset to complete. I2C commands issued after the SI7020 is starting up
>> or after reset can potentially upset the startup sequence. Therefore, the
>> host needs to wait for the startup sequence to finish before issuing
>> further I2C commands.
>>
>> Signed-off-by: Eddie James <eajames@...ux.ibm.com>
>> ---
>>   drivers/iio/humidity/si7020.c | 16 ++++++++++++++--
>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iio/humidity/si7020.c b/drivers/iio/humidity/si7020.c
>> index ab6537f136ba..76ca7863f35b 100644
>> --- a/drivers/iio/humidity/si7020.c
>> +++ b/drivers/iio/humidity/si7020.c
>> @@ -106,6 +106,7 @@ static const struct iio_info si7020_info = {
>>   static int si7020_probe(struct i2c_client *client,
>>   			const struct i2c_device_id *id)
>>   {
>> +	struct i2c_adapter *root;
>>   	struct iio_dev *indio_dev;
>>   	struct i2c_client **data;
>>   	int ret;
>> @@ -115,13 +116,24 @@ static int si7020_probe(struct i2c_client *client,
>>   				     I2C_FUNC_SMBUS_READ_WORD_DATA))
>>   		return -EOPNOTSUPP;
>>   
>> +	root = i2c_lock_select_bus(client->adapter);
>> +	if (IS_ERR(root))
>> +		return PTR_ERR(root);
>> +
>>   	/* Reset device, loads default settings. */
>> -	ret = i2c_smbus_write_byte(client, SI7020CMD_RESET);
>> -	if (ret < 0)
>> +	ret = __i2c_smbus_xfer(root, client->addr, client->flags,
>> +			       I2C_SMBUS_WRITE, SI7020CMD_RESET,
>> +			       I2C_SMBUS_BYTE, NULL);
> I'd say that this is too ugly. We should not add stuff that basically
> hides the actual xfer from the mux like this. That is too much of a
> break in the abstraction.


Hm, I guess I'm not sure I follow - I see several drivers that use the 
raw __i2c_smbus_xfer or __i2c_transfer, some without a lock in sight. If 
it's not acceptable to use the unlocked versions in some cases, why are 
they exported in the header file?


>
> Looking back, expanding on the previous series [1] so that it installs
> the hook on the root adapter, handles smbus xfers and clears out the
> callback afterwards is much more sensible. No?


Maybe so, though adding the callback is a more intrusive change, in my 
opinion, since every transfer has to check if the pointer is null.


Thanks for your feedback!

Eddie



>
> Maybe the callback in that series should also include a reference to
> the xfer that has just been done, so that the hook can potentially
> discriminate and only do the delay for the key xfer. But maybe that's
> overkill?
>
> Cheers,
> Peter
>
> [1] https://lore.kernel.org/lkml/20220518204119.38943-1-eajames@linux.ibm.com/
>
>> +	if (ret < 0) {
>> +		i2c_unlock_deselect_bus(client->adapter);
>>   		return ret;
>> +	}
>> +
>>   	/* Wait the maximum power-up time after software reset. */
>>   	msleep(15);
>>   
>> +	i2c_unlock_deselect_bus(client->adapter);
>> +
>>   	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>>   	if (!indio_dev)
>>   		return -ENOMEM;