linux-kernel - Re: [PATCH V2] drivers/i2c/busses/i2c-at91.c: fix brokeness

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: <20111107094701.GF4265@legolas.emea.dhcp.ti.com>
Date:	Mon, 7 Nov 2011 11:47:02 +0200
From:	Felipe Balbi <balbi@...com>
To:	Nikolaus Voss <n.voss@...nmann.de>
Cc:	'linux-i2c@...r.kernel.org',
	'linux-arm-kernel@...ts.infradead.org', nicolas.ferre@...el.com,
	plagnioj@...osoft.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2] drivers/i2c/busses/i2c-at91.c: fix brokeness

Hi,

On Mon, Nov 07, 2011 at 10:27:52AM +0100, Nikolaus Voss wrote:
> From a05fa963f819dabf985ec0d76c769b2cf4894ccf Mon Sep 17 00:00:00 2001
> From: Nikolaus Voss <n.voss@...nmann.de>
> Date: Thu, 27 Oct 2011 11:12:55 +0200
> Subject: [PATCH 1/6] drivers/i2c/busses/i2c-at91.c: fix brokeness
> 
> This patch contains the following fixes:
> 1. Support for multiple interfaces (there are usually two of them).
> 2. Remove busy waiting in favour of interrupt driven io.
> 3. No repeated start (Sr) was possible. This implementation supports one
>    repeated start condition which is enough for most real-world applications
>    including all SMBus transfer types.
> 
> Tested on Atmel G45 with BQ20Z80 battery SMBus client.
> 
> Signed-off-by: Nikolaus Voss <n.voss@...nmann.de>

IMHO, you should split this patch into three or more smaller patches.
You're doing lots of different things in one commit and it'll be a pain
to bisect should this cause any issues to anyone.

> ---
> V2: No killed tabs, patch should apply now.
> 
>  drivers/i2c/busses/Kconfig    |   11 +-
>  drivers/i2c/busses/i2c-at91.c |  415 +++++++++++++++++++++++++++--------------
>  2 files changed, 278 insertions(+), 148 deletions(-)
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 646068e..c4b6bdc 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -286,18 +286,15 @@ comment "I2C system bus drivers (mostly embedded / system-on-chip)"
> 
>  config I2C_AT91
>  	tristate "Atmel AT91 I2C Two-Wire interface (TWI)"
> -	depends on ARCH_AT91 && EXPERIMENTAL && BROKEN
> +	depends on ARCH_AT91 && EXPERIMENTAL
>  	help
>  	  This supports the use of the I2C interface on Atmel AT91
>  	  processors.
> 
> -	  This driver is BROKEN because the controller which it uses
> -	  will easily trigger RX overrun and TX underrun errors.  Using
> -	  low I2C clock rates may partially work around those issues
> -	  on some systems.  Another serious problem is that there is no
> -	  documented way to issue repeated START conditions, as needed
> +	  A serious problem is that there is no documented way to issue
> +	  repeated START conditions for more than two messages, as needed
>  	  to support combined I2C messages.  Use the i2c-gpio driver
> -	  unless your system can cope with those limitations.
> +	  unless your system can cope with this limitation.
> 
>  config I2C_AU1550
>  	tristate "Au1550/Au1200 SMBus interface"
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index 305c075..a2c38ff 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -1,6 +1,10 @@
> -/*
> +/* -*- linux-c -*-

you shouldn't add this editor hooks to linux source files.

> @@ -279,33 +400,45 @@ static int __devexit at91_i2c_remove(struct platform_device *pdev)
> 
>  /* NOTE: could save a few mA by keeping clock off outside of at91_xfer... */
> 
> -static int at91_i2c_suspend(struct platform_device *pdev, pm_message_t mesg)
> +static int at91_i2c_suspend(struct device *dev)
>  {
> -	clk_disable(twi_clk);
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct at91_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
> +
> +	clk_disable(i2c_dev->clk);
> +
>  	return 0;
>  }
> 
> -static int at91_i2c_resume(struct platform_device *pdev)
> +static int at91_i2c_resume(struct device *dev)
>  {
> -	return clk_enable(twi_clk);
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct at91_i2c_dev *i2c_dev = platform_get_drvdata(pdev);

dev_get_drvdata(dev) will do the same of two above lines. You don't need
to fetch the platform_device...

> @@ -322,6 +455,6 @@ static void __exit at91_i2c_exit(void)
>  module_init(at91_i2c_init);
>  module_exit(at91_i2c_exit);
> 
> -MODULE_AUTHOR("Rick Bronson");
> +MODULE_AUTHOR("Nikolaus Voss");

wow... are you sure ? Rick will always be the original author, no ?

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)