linux-kernel - Re: [PATCH v3 3/9] eeprom: Add a simple EEPROM framework for eeprom providers

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: <55143288.4040604@linaro.org>
Date:	Thu, 26 Mar 2015 16:23:36 +0000
From:	Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:	Mark Brown <broonie@...nel.org>
CC:	linux-arm-kernel@...ts.infradead.org,
	Maxime Ripard <maxime.ripard@...e-electrons.com>,
	Rob Herring <robh+dt@...nel.org>,
	Kumar Gala <galak@...eaurora.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-api@...r.kernel.org, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org, linux-arm-msm@...r.kernel.org,
	arnd@...db.de, sboyd@...eaurora.org
Subject: Re: [PATCH v3 3/9] eeprom: Add a simple EEPROM framework for eeprom
 providers



On 24/03/15 22:53, Mark Brown wrote:
> On Tue, Mar 24, 2015 at 10:30:08PM +0000, Srinivas Kandagatla wrote:
>
>> +static ssize_t bin_attr_eeprom_write(struct file *filp, struct kobject *kobj,
>> +				     struct bin_attribute *attr,
>> +				     char *buf, loff_t offset, size_t count)
>> +{
>> +	struct device *dev = container_of(kobj, struct device, kobj);
>> +	struct eeprom_device *eeprom = to_eeprom(dev);
>> +	int rc;
>> +
>> +	if (offset > eeprom->size)
>> +		return -EINVAL;
>> +
>> +	if (offset + count > eeprom->size)
>> +		count = eeprom->size - offset;
>> +
>> +	rc = regmap_bulk_write(eeprom->regmap, offset,
>> +			       buf, count/eeprom->stride);
>
> Are you sure that this and the read interface should be using the bulk
> interface and not the raw interface - do we want the byte swapping that
> the bulk interface provides?
>
You are correct, byte swapping is not really needed in this cases.
It should read/write data as it is.

I will fix it in next version.

> I'm also not entirely able to convince myself that the above error
> checks and code line up with what I'd expect the userspace ABI to be, we
> seem to be treating offset as both a byte offset into the data (which is
> what I'd expect the userspace ABI to do) and a word based index into the
> data (which is what the regmap API is doing).  For example with 16 bit
> words offset 2 will start at the 5th byte of data but if userspace seeks
> to offset 5 it will get the 11th byte and onwards.

Thanks for spotting this, Yes, the offset from userspace should be 
treated as byte oriented and the offset to regmap as word based index 
into the data.
Yes, logic here needs a fix to handle data correctly.

>
> The stride and the word size are separate, they will frequently line up
> for memory mapped devices but typically won't for other devices.  I
> think you need more data mangling to handle this robustly.
Yes, I agree I will address this too in next version.

thanks,
srini

>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/