linux-kernel - Re: [PATCH 2.6.28] mfd: Correct WM8350 I2C return code usage

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: <20081112234331.GB8272@sirena.org.uk>
Date:	Wed, 12 Nov 2008 23:43:31 +0000
From:	Mark Brown <broonie@...ena.org.uk>
To:	Samuel Ortiz <sameo@...nedhand.com>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2.6.28] mfd: Correct WM8350 I2C return code usage

On Thu, Nov 13, 2008 at 12:06:36AM +0100, Samuel Ortiz wrote:

> I understand that. I'm just saying that I would prefer wm8350->read_dev() to
> return the actual bytes read, be it for SPI or I2C. Same for write_dev(), of
> course.

Hrm.  That's never been the case, even with the buggy code since the
register address was included in the physical access for at least the
writes.

> With this patch you're breaking that expectation because the read|write_dev()
> I'd prefer to fix the callers code, so that we keep the expected semantics

Just to clarify, when you say "expectation" and "fix" are you talking
about your preference that the hardware access functions return the
number of bytes read or written or is there something else going on
here? 

> for your read|write_dev() routines. For example with wm8350_clear_bits():

> -	if (err) {
> +	ret = wm8350_read(wm8350, reg, 1, &data);
> +	if (ret != 1) {
>  		dev_err(wm8350->dev, "read from reg R%d failed\n", reg);
> +		err = -EIO;

Note that wm8350_read() works a level up, only calling the physical read
function if the register cache can't satisfy the access.  Changing that
would be another layer of alteration.

To be honest I'm really not sure it's worth changing this - I'm having a
hard time thinking of a user that would be able to do anything useful
with a short access.  It also seems like it'd be a rather invasive
change for an -rc.
--
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/