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
| ||
|
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;
Powered by blists - more mailing lists