linux-kernel - Re: [PATCH v3 1/8] i2c: core: Add support for best effort block read emulation

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 for Android: free password hash cracker in your pocket

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

Date:	Sat, 1 Aug 2015 21:53:20 +0200
From:	Wolfram Sang <wsa@...-dreams.de>
To:	"Tirdea, Irina" <irina.tirdea@...el.com>
Cc:	Jonathan Cameron <jic23@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Pandruvada, Srinivas" <srinivas.pandruvada@...el.com>,
	Peter Meerwald <pmeerw@...erw.net>,
	"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
	"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>
Subject: Re: [PATCH v3 1/8] i2c: core: Add support for best effort block read
 emulation


> > I wonder what devices do if you do a word read beyond their end address?
> > Perhaps in odd cases we should always fall back to byte reads?
> 
> In my tests I can read beyond the end address, but I cannot be sure if this is OK for all
> devices. This was actually a suggestion from Wolfram for v1, but maybe I'm missing
> something.
> 
> Wolfram, is it safe to read one byte beyond the end address or should I better use
> only byte reads for odd lengths?

Well, from a device perspective, it is probably better to be safe than
sorry and fall back to a single byte read. That means, however, that we
need READ_WORD_DATA and READ_BYTE_DATA to be supported by the adapter.
Should be OK, I don't think there are adapters which can only read words.

> > > + */
> > > +s32 i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client,
> > > +					      u8 command, u8 length, u8 *values)
> > > +{
> > > +	u8 i;
> > > +	int status;
> > > +
> > > +	if (length > I2C_SMBUS_BLOCK_MAX)
> > > +		length = I2C_SMBUS_BLOCK_MAX;
> > > +
> > > +	if (i2c_check_functionality(client->adapter,
> > > +				    I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
> > > +		return i2c_smbus_read_i2c_block_data(client, command,
> > > +						     length, values);

I am not very strict with the 80 char limit. I'd think the code is more
readable if the two statements above would be oneliners. And for some
other lines later as well.

> > > +	} else if (i2c_check_functionality(client->adapter,

No need for else since we return in the above if anyhow.

> > > +					   I2C_FUNC_SMBUS_READ_WORD_DATA)) {
> > > +		for (i = 0; i < length; i += 2) {
> > > +			status = i2c_smbus_read_word_data(client, command + i);
> > > +			if (status < 0)
> > > +				return status;
> > > +			values[i] = status & 0xff;
> > > +			if ((i + 1) < length)
> > > +				values[i + 1] = status >> 8;
> > > +		}
> > > +		if (i > length)
> > > +			return length;
> > > +		return i;
> > > +	} else if (i2c_check_functionality(client->adapter,
> > > +					   I2C_FUNC_SMBUS_READ_BYTE_DATA)) {
> > > +		for (i = 0; i < length; i++) {
> > > +			status = i2c_smbus_read_byte_data(client, command + i);
> > > +			if (status < 0)
> > > +				return status;
> > > +			values[i] = status;
> > > +		}
> > > +		return i;
> > > +	}
> > > +
> > > +	dev_err(&client->adapter->dev, "Unsupported transactions: %d,%d,%d\n",
> > > +		I2C_SMBUS_I2C_BLOCK_DATA, I2C_SMBUS_WORD_DATA,
> > > +		I2C_SMBUS_BYTE_DATA);

I don't think the %d printouts are useful. Just say that the adapter
lacks support for needed transactions. And I think the device which
reports the error should be the client device, no?

Thanks,

   Wolfram


Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)