linux-kernel - Re: [PATCH 0/11] introduce macros for i2c

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:	Tue, 9 Oct 2012 17:43:36 +0200 (CEST)
From:	Julia Lawall <julia.lawall@...6.fr>
To:	Jean Delvare <khali@...ux-fr.org>
cc:	Julia Lawall <Julia.Lawall@...6.fr>,
	Mauro Carvalho Chehab <mchehab@...radead.org>,
	ben-linux@...ff.org, w.sang@...gutronix.de,
	linux-i2c@...r.kernel.org, kernel-janitors@...r.kernel.org,
	rmallon@...il.com, shubhrajyoti@...com,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/11] introduce macros for i2c_msg initialization

On Tue, 9 Oct 2012, Jean Delvare wrote:

> Hi Julia,
>
> On Sun,  7 Oct 2012 17:38:30 +0200, Julia Lawall wrote:
> > This patch set introduces some macros for describing how an i2c_msg is
> > being initialized.  There are three macros: I2C_MSG_READ, for a read
> > message, I2C_MSG_WRITE, for a write message, and I2C_MSG_OP, for some other
> > kind of message, which is expected to be very rarely used.
>
> "Some other kind of message" is actually messages which need extra
> flags. They are still read or write messages.

I agree.  We could also have a read with extra options macro and a write
with extra options macro.  That would give four macros, which is not too
much more than three.

> OK, I've read the whole series now and grepped the kernel tree so I
> have a better overview. There are a lot more occurrences than what you
> converted. I presume the conversions were just an example and you leave
> the rest up to the relevant maintainers (e.g. me) if they are
> interested?

I would be happy to do the rest, or at least to do more.  I just didn't
want to do 600+ cases before knowing how others felt about the various
changes.  Actually, now that we seem to have decided to make fewer changes
at once, I could probably work more quickly.  So far, I have been
comparing the results after running cpp, as well as checking that the
sizeof transformation is correct, which is a bit slow.

> Given the huge number of affected drivers (a quick grep suggests 230
> drivers and more than 300 occurrences), we'd better think twice before
> going on as it will be intrusive and hard to change afterward.
>
> So my first question will be: what is your goal with this change? Are
> you only trying to save a few lines of source code? Or do you expect to
> actually fix/prevent bugs by introducing these macros? Or something
> else?

The main goal just seems to be to provide something that is more readable.

> I admit I am not completely convinced by the benefit at the moment. A
> number of these drivers should be using i2c_smbus_*() functions instead
> of i2c_transfer() for improved compatibility, or i2c_master_send/recv()
> for single message transfers (383 occurrences!), so making
> i2c_transfer() easier to use isn't at the top of my priority list. And
> I see the extra work for the pre-processor, so we need a good reason
> for doing that.

OK, if it doesn't seem like a good idea, it is no problem to drop the idea
completely.  It does seem a bit nicer to have writing indicated as WRITE
rather than as 0, but that might not be a big enough benefit to justify
making changes.

julia
--
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/