linux-kernel - Re: [PATCH v7 4/4] i2c: tegra: proper handling of error cases

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] [day] [month] [year] [list]

Date:	Wed, 11 May 2016 16:47:52 +0200
From:	Wolfram Sang <wsa@...-dreams.de>
To:	Shardar Shariff Md <smohammed@...dia.com>
Cc:	ldewangan@...dia.com, swarren@...dotorg.org,
	thierry.reding@...il.com, gnurou@...il.com,
	linux-i2c@...r.kernel.org, linux-tegra@...r.kernel.org,
	linux-kernel@...r.kernel.org, jonathanh@...dia.com
Subject: Re: [PATCH v7 4/4] i2c: tegra: proper handling of error cases

On Mon, Apr 25, 2016 at 07:08:39PM +0530, Shardar Shariff Md wrote:
> To summarize the issue observed in error cases:
> 
> SW Flow: For i2c message transfer, packet header and data payload is
> posted and then required error/packet completion interrupts are enabled
> later.
> 
> HW flow: HW process the packet just after packet header is posted, if
> ARB lost/NACK error occurs (SW will not handle immediately when error
> happens as error interrupts are not enabled at this point). HW assumes
> error is acknowledged and clears current data in FIFO, But SW here posts
> the remaining data payload which still stays in FIFO as stale data
> (data without packet header).
> 
> Now once the interrupts are enabled, SW handles ARB lost/NACK error by
> clearing the ARB lost/NACK interrupt. Now HW assumes that SW attended
> the error and will parse/process stale data (data without packet header)
> present in FIFO which causes invalid NACK errors.
> 
> Fix: Enable the error interrupts before posting the packet into FIFO
> which make sure HW to not clear the fifo. Also disable the packet mode
> before acknowledging errors (ARB lost/NACK error) to not process any
> stale data. As error interrupts are enabled before posting the packet
> header use spinlock to avoid preempting.
> 
> Signed-off-by: Shardar Shariff Md <smohammed@...dia.com>
> 
> ---
> Changes in v2:
> - Align the commit message to 72 characters per line.
> - Removing unnecessary paranthesis.
> - Handle error in isr
> 
> Changes in v3:
> - Printing error if tegra_i2c_disable_packet_mode() fails
>   is already present and handling error is not taken cared
>   in ISR which was done in v2 but keeping return error in
>   *wait_for_config_load() as its used in tegra_i2c_init()
> ---
>  drivers/i2c/busses/i2c-tegra.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 8d49995..1181bbf 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -194,6 +194,7 @@ struct tegra_i2c_dev {
>  	u16 clk_divisor_non_hs_mode;
>  	bool is_suspended;
>  	bool is_multimaster_mode;
> +	spinlock_t xfer_lock;

This lock needs a description. The sentence from above is a good
template: "As error interrupts are enabled before posting the packet
header use spinlock to avoid preempting.".

Patches 3+4 look good otherwise but need to be rebased on modified
patch 2.

Thanks,

   Wolfram


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