linux-kernel - Re: [PATCH v3 1/3] i2c: use void pointers for supplying data for reads and writes

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]

Date:   Mon, 18 Nov 2019 09:38:10 +0100
From:   Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     Wolfram Sang <wsa@...-dreams.de>, linux-iio@...r.kernel.org,
        Luca Ceresoli <luca@...aceresoli.net>,
        linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Hartmut Knaack <knaack.h@....de>,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>
Subject: Re: [PATCH v3 1/3] i2c: use void pointers for supplying data for
 reads and writes

[Fixing the address of the linux-iio list]

Hello Dmitry,

On Mon, Nov 18, 2019 at 12:04:46AM -0800, Dmitry Torokhov wrote:
> On Mon, Nov 18, 2019 at 08:43:49AM +0100, Uwe Kleine-König wrote:
> > On Tue, Nov 12, 2019 at 12:31:30PM -0800, Dmitry Torokhov wrote:
> > > There is no need to force users of i2c_master_send()/i2c_master_recv()
> > > and other i2c read/write bulk data API to cast everything into u8 pointers.
> > > While everything can be considered byte stream, the drivers are usually
> > > work with more structured data.
> > > 
> > > Let's switch the APIs to accept [const] void pointers to cut amount of
> > > casting needed.
> > > 
> > > Acked-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@...il.com>
> > 
> > Can you give an example where you save some casts? Given that i2c is a
> > byte oriented protocol (as opposed to for example spi) I think it's a
> > good idea to expose this in the API.
> 
> I see this at least:
> 
> dtor@...r-ws:~/kernel/work $ git grep "i2c_master.*(u8 \*)"
> drivers/crypto/atmel-i2c.c:     ret = i2c_master_send(client, (u8 *)cmd, cmd->count + WORD_ADDR_SIZE);
> drivers/iio/common/ms_sensors/ms_sensors_i2c.c:         ret = i2c_master_recv(client, (u8 *)&buf, 3);
> drivers/iio/humidity/hdc100x.c: ret = i2c_master_recv(client, (u8 *)buf, 4);

I think this one has an endianness bug.

> drivers/iio/pressure/abp060mg.c:        ret = i2c_master_send(client, (u8 *)&buf, state->mreq_len);

>From a quick look: mreq_len might be 1 in some cases and buf is declared
as __be16[2]. Hmm.

> drivers/iio/pressure/abp060mg.c:        ret = i2c_master_recv(client, (u8 *)&buf, sizeof(buf));
> drivers/input/misc/ad714x-i2c.c:        error = i2c_master_send(client, (u8 *)chip->xfer_buf,
> drivers/input/misc/ad714x-i2c.c:        error = i2c_master_send(client, (u8 *)chip->xfer_buf,
> drivers/input/misc/ad714x-i2c.c:                error = i2c_master_recv(client, (u8 *)chip->xfer_buf,
> drivers/input/touchscreen/sx8654.c:     len = i2c_master_recv(ts->client, (u8 *)data, readlen);
> drivers/nfc/nfcmrvl/i2c.c:      ret = i2c_master_recv(drv_data->i2c, (u8 *)&nci_hdr, NCI_CTRL_HDR_SIZE);
> drivers/nfc/nxp-nci/i2c.c:      r = i2c_master_recv(client, (u8 *) &header, NXP_NCI_FW_HDR_LEN);
> drivers/nfc/nxp-nci/i2c.c:      r = i2c_master_recv(client, (u8 *) &header, NCI_CTRL_HDR_SIZE);
> drivers/video/fbdev/ssd1307fb.c:        ret = i2c_master_send(client, (u8 *)array, len);
> 
> I am pretty sure there are more that my quick grep did not catch.
> 
> And I agree that I2C itself is a byte-oriented protocol, but the data from the
> point of the driver (once transfer is done) is often more structured.

I think it is fine to require from a caller that they are aware that a
byte (or byte array) must be passed to i2c functions. Given the (maybe)
two problems I pointed out above making it a bit harder to pass non-byte
data to these functions doesn't sound like a bad idea to me.

Obviously your mileage varies, but I personally like having an explicit
type in the API. I guess we have to agree to not agree and let Wolfram
decide if he likes your change or not.

> > > diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
> > > index 5c2cc61b666e7..48ed76a0e83d4 100644
> > > --- a/drivers/iio/adc/max1363.c
> > > +++ b/drivers/iio/adc/max1363.c
> > 
> > This change isn't motivated in the commit log. Is this here by mistake?
> 
> No, it is needed as signature of i2c_master_send/recv has changed.

Ah, I see, there is

	st->send = i2c_master_send;

in the code. I think this is worth mentioning in the commit log that it
changes to this file don't look like a mistake as I wondered.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |