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  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:	Sun, 10 Nov 2013 13:48:35 +0100
From:	Tomasz Figa <tomasz.figa@...il.com>
To:	Naveen Krishna Chatradhi <ch.naveen@...sung.com>
Cc:	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-samsung-soc@...r.kernel.org, dianders@...omium.org,
	gregkh@...uxfoundation.org, naveenkrishna.ch@...il.com,
	lars@...afoo.de, grundler@...omium.org, cpgs@...sung.com
Subject: Re: [PATCH v6] iio: exynos_adc: use wait_for_completion_timeout instead of interruptible

Hi Naveen,

On Tuesday 05 of November 2013 15:15:30 Naveen Krishna Chatradhi wrote:
> 1. The irq routine is so simple (just one register read) shouldn't be
> long Hence, reduce the timeout to 100milli secs,

I believe that the timeout value depends mostly on maximum conversion time 
and interrupt handler complexity is not very significant here.

> 2. With 100ms of wait time, interruptible is very much unnecessary.
>    Hence, use wait_for_completion_timeout instead of
>    wait_for_completion_interruptible_timeout
> 3. Reset software if a timeout happens.
> 4. Add reinit_completion() before the wait_for_completion_timeout in
> raw_read()

All the four points listed above, even just by the form they are written 
in, suggest that they should be separate four patches.

In addition, patch description should explain why such change is needed, 
i.e. what it fixes, improves or prepares the code for.

> Note: submitted for review at
> https://patchwork.kernel.org/patch/2279591/
> 

This should not be located in patch description, but rather in comments 
section below, as is the change log.

> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@...sung.com>
> Cc: Doug Anderson <dianders@...omium.org>
> Cc: Lars-Peter Clausen <lars@...afoo.de>
> Reviewed-on: https://chromium-review.googlesource.com/172724
> Reviewed-by: Doug Anderson <dianders@...omium.org>
> ---
> Changes since v1:
> As per discussion at
> http://marc.info/?l=linux-kernel&m=136517637228869&w=3
> 
> Changes since v2:
> None.
> Rebased and reposting.
> 
> Changes since v3:
> 1. commit message change and
> 2. removed an unncessary assignment
> 
> Changes since v4:
> Moved INIT_COMPLETION call to the starting of the function
> 
> Changes since v5:
> INIT_COMPLETION was replaced by reinit_completion
> ("tree-wide: use reinit_completion instead of INIT_COMPLETION").
> Use it to avoid the following build error:
> undefined identifier 'INIT_COMPLETION'

Not really a comment to the patch, but rather a suggestion for future:

It is more convenient to read the change log if it goes from newest to 
oldest order.

Otherwise the changes alone look good.

Best regards,
Tomasz

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

Powered by blists - more mailing lists