| 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
| ||
|
Message-Id: <20110722145451.78390257.akpm@linux-foundation.org>
Date: Fri, 22 Jul 2011 14:54:51 -0700
From: Andrew Morton <akpm@...ux-foundation.org>
To: Austin Boyle <Austin.Boyle@...atnet.com>
Cc: Alessandro Zummo <a.zummo@...ertech.it>,
<rtc-linux@...glegroups.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2.6.39-rc7] RTC: rtc-ds1307 EEPROM support for ds1388
chip
On Tue, 14 Jun 2011 16:14:57 +1200
Austin Boyle <Austin.Boyle@...atnet.com> wrote:
> On Mon, 2011-05-23 at 17:18 +1200, Austin Boyle wrote:
> > This is my patch which extends the NVRAM access functions in the DS1307
> > driver to support the 512 bytes of EEPROM on the DS1388 chip. Could you
> > please review the patch? If there are any problems with the way I have
> > implemented this or suggested improvements I will gladly modify. Thanks!
>
> This is an updated version of the patch which prints the correct NVRAM
> size at init. Could someone please give me some feedback?
>
> [PATCH 3.0-rc3] RTC: rtc-ds1307: EEPROM support for ds1388 chip
>
> This patch adds support for different sized NVRAM, such as
> the 512 bytes of EEPROM on the DS1388 chip.
The patch is wordwrapped - please fix your email client.
The patch makes rather a mess of a decent-looking driver. Please use
scripts/checkpatch.pl and review its report.
>
> ...
>
> @@ -540,18 +545,39 @@ ds1307_nvram_read(struct file *filp, str
> struct i2c_client *client;
> struct ds1307 *ds1307;
> int result;
> + unsigned int addr = 0;
>
> client = kobj_to_i2c_client(kobj);
> ds1307 = i2c_get_clientdata(client);
>
> - if (unlikely(off >= NVRAM_SIZE))
> + if (unlikely(off >= ds1307->nvram_size))
> return 0;
> - if ((off + count) > NVRAM_SIZE)
> - count = NVRAM_SIZE - off;
> + if ((off + count) > ds1307->nvram_size)
> + count = ds1307->nvram_size - off;
> if (unlikely(!count))
> return count;
>
> - result = ds1307->read_block_data(client, 8 + off, count, buf);
> + if (ds1307->nvram_offset < 0xff) { /* ds1307, ds1338 */
> + result = ds1307->read_block_data(client, ds1307->nvram_offset + off,
> count, buf);
> + } else { /* ds1388 */
> + if ((off <= 0x00ff) && (off + count > 0x00ff)) { /* read spans two
> blocks */
> + client = ds1307->client[1]; /* first block */
> + addr = (unsigned int)off;
> + result = ds1307->read_block_data(client, addr, (0x100-addr), buf);
Ignores a possible error return value?
> + client = ds1307->client[2]; /* second block */
> + result += ds1307->read_block_data(client, 0x00, (count+addr-0x100),
> (buf+0x100-addr));
> +
> + } else { /* read spans one blocks */
> + if (off <= 0x00ff) {
> + client = ds1307->client[1];
> + addr = (unsigned int)off;
> + } else if (off <= 0x0200) {
> + client = ds1307->client[2];
> + addr = (unsigned int)off - 0x100;
> + }
> + result = ds1307->read_block_data(client, addr, count, buf);
> + }
> + }
> if (result < 0)
> dev_err(&client->dev, "%s error %d\n", "nvram read", result);
> return result;
> @@ -565,18 +591,40 @@ ds1307_nvram_write(struct file *filp, st
> struct i2c_client *client;
> struct ds1307 *ds1307;
> int result;
> + unsigned int addr = 0;
>
> client = kobj_to_i2c_client(kobj);
> ds1307 = i2c_get_clientdata(client);
>
> - if (unlikely(off >= NVRAM_SIZE))
> + if (unlikely(off >= ds1307->nvram_size))
> return -EFBIG;
> - if ((off + count) > NVRAM_SIZE)
> - count = NVRAM_SIZE - off;
> + if ((off + count) > ds1307->nvram_size)
> + count = ds1307->nvram_size - off;
> if (unlikely(!count))
> return count;
>
> - result = ds1307->write_block_data(client, 8 + off, count, buf);
> + if (ds1307->nvram_offset < 0xff) { /* ds1307, ds1338 */
> + result = ds1307->write_block_data(client, ds1307->nvram_offset + off,
> count, buf);
> + } else { /* ds1388 */
> + if ((off <= 0x00ff) && ((off + count) > 0x00ff)) { /* read spans two
> blocks */
> + client = ds1307->client[1]; /* first block */
> + addr = (unsigned int)off;
> + result = ds1307->write_block_data(client, addr, (0x100-addr), buf);
Error checking?
> + client = ds1307->client[2]; /* second block */
> + result = ds1307->write_block_data(client, 0x00, (addr+count-0x100),
> (buf+0x100-addr));
> +
> + } else { /* Device address/offset translation */
> + dev_dbg(&client->dev, "Write spans one block\n");
> + if (off <= 0x00ff) {
A single space before the {
> + client = ds1307->client[1];
> + addr = (unsigned int)off;
> + } else if (off <= 0x0200) {
ditto
> + client = ds1307->client[2];
> + addr = (unsigned int)off - 0x100;
> + }
> + result = ds1307->write_block_data(client, addr, count, buf);
> + }
> + }
> if (result < 0) {
> dev_err(&client->dev, "%s error %d\n", "nvram write", result);
> return result;
>
> ...
>
> @@ -866,16 +923,35 @@ read_rtc:
> dev_dbg(&client->dev, "got IRQ %d\n", client->irq);
> }
>
> - if (chip->nvram56) {
> + if (chip->nvram) {
> + nvram.size = ds1307->nvram_size;
> err = sysfs_create_bin_file(&client->dev.kobj, &nvram);
> if (err == 0) {
> set_bit(HAS_NVRAM, &ds1307->flags);
> - dev_info(&client->dev, "56 bytes nvram\n");
> + dev_info(&client->dev, "%d bytes nvram\n", nvram.size);
> + }
> + }
> +
> + /* use dummy devices for multiple address chips */
> + for (i = 1; i < ds1307->num_addrs; i++) {
> + ds1307->client[i] = i2c_new_dummy(client->adapter, client->addr + i);
> + if (!ds1307->client[i]) {
> + dev_err(&client->dev, "address 0x%02x unavailable\n",
> + client->addr + i);
> + err = -EADDRINUSE;
EADDRINUSE is a networking error code. Returning that from an
rtc-related operation might mislead the operator.
> + goto err_clients;
> }
> + i2c_set_clientdata(ds1307->client[i], ds1307);
> }
>
> return 0;
>
> +err_clients:
> + for (i = 1; i < ds1307->num_addrs; i++) {
> + if (ds1307->client[i]) {
> + i2c_unregister_device(ds1307->client[i]);
> + }
We normally omit braces around a single statement.
The `if (ds1307->client[i])' test should be unneeded anyway - just
unregister client[1] though client[i - 1].
Why isn't client[0] used here btw?
> + }
> exit_irq:
> rtc_device_unregister(ds1307->rtc);
> exit_free:
--
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/
Powered by blists - more mailing lists