linux-kernel - Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation

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:	Thu, 11 Oct 2012 17:32:59 +0200
From:	Linus Walleij <linus.walleij@...aro.org>
To:	Christopher Heiny <Cheiny@...aptics.com>
Cc:	Anton Vorontsov <anton.vorontsov@...aro.org>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Jean Delvare <khali@...ux-fr.org>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	Linux Input <linux-input@...r.kernel.org>,
	Allie Xiong <axiong@...aptics.com>,
	Vivian Ly <vly@...aptics.com>,
	Daniel Rosenberg <daniel.rosenberg@...aptics.com>,
	Joerie de Gram <j.de.gram@...il.com>,
	Wolfram Sang <w.sang@...gutronix.de>,
	Mathieu Poirier <mathieu.poirier@...aro.org>,
	Linus Walleij <linus.walleij@...ricsson.com>,
	Naveen Kumar Gaddipati <naveen.gaddipati@...ricsson.com>,
	Alexandra Chin <alexandra.chin@...synaptics.com>
Subject: Re: [RFC PATCH 01/06] input/rmi4: Public header and documentation

On Thu, Oct 11, 2012 at 5:41 AM, Christopher Heiny <Cheiny@...aptics.com> wrote:
> Linus Walleij wrote:

>> But please use arithmetic operators (I think I said this on the last
>> review):
>>
>> dest[0] = src & 0xFF;
>> dest[1] = src >> 8;
>>
>> Doing it the above way makes artithmetic look like maths, and it isn't.
>> Besides it's done this way in most parts of the kernel and we're
>> familiar with it.
>
> Yes, you mentioned it previously.  I'm somewhat paranoid, though, and
> don't trust the shift/mask method to work correctly on big-endian
> machines.  If the shifts can be relied on to behave (I'm guessing the
> answer is "yes", since you say this idiom is used widely in the
> kernel), then I'll change it.

If the behaviour was not consistent across different endianness
it would not be part of the C language specification...

<< means shift left in the accumulator or whatever you have.
It will work the same no matter how bits are laid out in
memory.

>> > +static inline ssize_t rmi_store_error(struct device *dev,
>> > +                       struct device_attribute *attr,
>> > +                       const char *buf, size_t count)
>> > +{
>> > +       dev_warn(dev,
>> > +                "WARNING: Attempt to write %d characters to read-only
>> > attribute %s.", +               count, attr->attr.name);
>> > +       return -EPERM;
>> > +}
>>
>> Here it looks like you're hiding a lot of stuff that should be dev_warn()?
>> Consider my earlier point about dynamic debug.
>
> In previous patch submissions, we always used these warning functions.
> But in the feedback on those patches, we were asked to just make
> sysfs show/store NULL if the attribute is write/read only.  However,
> during their development process, our customers want to see the
> warnings if the attributes are accessed incorrectly.  So we made
> these warnings a debug option.

See Dmitry's comment ...

Basically my stance is that you should not lower yourself to the
level of others not getting the point of your technical solution
by making unelegant compromises, what
you should do is to bring them up to your level so they
understand that your solution is elegant.

Maybe a bit utopist I know...

Yours,
Linus Walleij
--
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/