linux-kernel - Re: [PATCH v3 2/2] staging: iio: light: isl29018: use regmap for register access

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: <CANEJEGtvfwUmtXjsu_PTWP2zCRAbYMJh_jJp-c6Ex2ca=Sietg@mail.gmail.com>
Date:	Thu, 19 Apr 2012 10:52:23 -0700
From:	Grant Grundler <grundler@...omium.org>
To:	Laxman Dewangan <ldewangan@...dia.com>
Cc:	jic23@....ac.uk, gregkh@...uxfoundation.org, max@...o.at,
	jbrenner@...sinc.com, bfreed@...omium.org, lars@...afoo.de,
	linux-iio@...r.kernel.org, devel@...verdev.osuosl.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] staging: iio: light: isl29018: use regmap for
 register access

On Thu, Apr 19, 2012 at 4:15 AM, Laxman Dewangan <ldewangan@...dia.com> wrote:
> Using regmap for accessing register through i2c bus. This will
> remove the code for caching registers, read-modify-write logics.
> Also it will provide the debugfs feature to dump register
> through regmap debugfs.
>
> Signed-off-by: Laxman Dewangan <ldewangan@...dia.com>

Reviewed-by: Grant Grundler <grundler@...omium.org>

Laxman,
Thanks for reposting this patch. I was talking with Bryan Freed and it
looks like the caching of registers will change the usage of
ADD_COMMAND1. More details below.

...
> -static int isl29018_write_data(struct i2c_client *client, u8 reg,
> -                       u8 val, u8 mask, u8 shift)
> -{
> -       u8 regval = val;
> -       int ret;
> -       struct isl29018_chip *chip = iio_priv(i2c_get_clientdata(client));
> -
> -       /* don't cache or mask REG_TEST */
> -       if (reg < ISL29018_MAX_REGS) {
> -               regval = chip->reg_cache[reg];
> -               regval &= ~mask;
> -               regval |= val << shift;
> -       }

Note the only REG_TEST isn't cached. Everything else updates the cache
on write.

> -
> -       ret = i2c_smbus_write_byte_data(client, reg, regval);
> -       if (ret) {
> -               dev_err(&client->dev, "Write to device fails status %x\n", ret);
> -       } else {
> -               /* don't update cache on err */
> -               if (reg < ISL29018_MAX_REGS)
> -                       chip->reg_cache[reg] = regval;
> -       }
> -
> -       return ret;
> -}

...
> -static int isl29018_read_sensor_input(struct i2c_client *client, int mode)
> +static int isl29018_read_sensor_input(struct isl29018_chip *chip, int mode)
>  {
>        int status;
> -       int lsb;
> -       int msb;
> +       unsigned int lsb;
> +       unsigned int msb;
>
>        /* Set mode */
> -       status = isl29018_write_data(client, ISL29018_REG_ADD_COMMAND1,
> -                       mode, COMMMAND1_OPMODE_MASK, COMMMAND1_OPMODE_SHIFT);
> +       status = regmap_update_bits(chip->regmap, ISL29018_REG_ADD_COMMAND1,
> +                       COMMMAND1_OPMODE_MASK, mode << COMMMAND1_OPMODE_SHIFT);

The old code will do a single I2C write. The new code will do a
read+write for ADD_COMMAND1 register. I reviewed the behaviors in
drivers/base/regmap/regmap.c and regcache.c.

I can't judge if this is a "real" problem. Given the limited BW on
I2C, someone with more I2C experience should carefully consider and
share their opinion.

>        if (status) {
> -               dev_err(&client->dev, "Error in setting operating mode\n");
> +               dev_err(chip->dev,
> +                       "Error in setting operating mode err %d\n", status);
>                return status;
>        }
>        msleep(CONVERSION_TIME_MS);
> -       lsb = i2c_smbus_read_byte_data(client, ISL29018_REG_ADD_DATA_LSB);

Old code for DATA_LSB and DATA_MSB ignored the "write cache" for reads
- so marking these as volatile looks correct to me.

...
> +static bool is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +       switch (reg) {
> +       case ISL29018_REG_ADD_DATA_LSB:
> +       case ISL29018_REG_ADD_DATA_MSB:
> +       case ISL29018_REG_ADD_COMMAND1:
> +       case ISL29018_REG_TEST:

Of these four, I think only ADD_COMMAND1 wasn't treated as volatile in
the old code. Am I overlooking something?

My concern is only about the additional I2C read traffic this patch
might generate. It's possible *some* bits in that register are
volatile and we could previously ignore them.

cheers,
grant
--
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/