On Mon, 26 Jan 2015, Russell King - ARM Linux wrote:
> On Tue, Jan 20, 2015 at 06:23:50AM +0100, Nicholas Mc Guire wrote:
> > if(!wait_for_completion_interruptible_timeout(...))
> > only handles the timeout case - this patch adds handling the
> > signal case the same as timeout and cleans up.
> >
> > Signed-off-by: Nicholas Mc Guire <der.herr@xxxxxxx>
> > ---
> >
> > Only the timeout case was being handled, return of 0 in
> > wait_for_completion_interruptible_timeout, the signal case (-ERESTARTSYS)
> > was treated just like the case of successful completion, which is most
> > likely not reasonable.
> >
> > Note that exynos_mipi_dsi_wr_data/exynos_mipi_dsi_rd_data return values
> > are not checked at the call sites in s6e8ax0.c (cmd_read/cmd_write)!
> >
> > This patch simply treats the signal case the same way as the timeout case,
> > by releasing locks and returning 0 - which might not be the right thing to
> > do - this needs a review by someone knowing the details of this driver.
> >
> > Patch is against 3.19.0-rc5 -next-20150119
> >
> > Patch was only compile-tested with exynos_defconfig
> >
> > drivers/video/fbdev/exynos/exynos_mipi_dsi_common.c | 17 +++++++++++------
> > 1 file changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/video/fbdev/exynos/exynos_mipi_dsi_common.c b/drivers/video/fbdev/exynos/exynos_mipi_dsi_common.c
> > index 2358a2f..55a7a45 100644
> > --- a/drivers/video/fbdev/exynos/exynos_mipi_dsi_common.c
> > +++ b/drivers/video/fbdev/exynos/exynos_mipi_dsi_common.c
> > @@ -157,6 +157,7 @@ int exynos_mipi_dsi_wr_data(struct mipi_dsim_device *dsim, unsigned int data_id,
> > const unsigned char *data0, unsigned int data_size)
> > {
> > unsigned int check_rx_ack = 0;
> > + long timeout;
> >
> > if (dsim->state == DSIM_STATE_ULPS) {
> > dev_err(dsim->dev, "state is ULPS.\n");
> > @@ -244,9 +245,11 @@ int exynos_mipi_dsi_wr_data(struct mipi_dsim_device *dsim, unsigned int data_id,
> > exynos_mipi_dsi_wr_tx_header(dsim, data_id, data_size & 0xff,
> > (data_size & 0xff00) >> 8);
> >
> > - if (!wait_for_completion_interruptible_timeout(&dsim_wr_comp,
> > - MIPI_FIFO_TIMEOUT)) {
> > - dev_warn(dsim->dev, "command write timeout.\n");
> > + timeout = wait_for_completion_interruptible_timeout(
> > + &dsim_wr_comp, MIPI_FIFO_TIMEOUT);
> > + if (timeout <= 0) {
> > + dev_warn(dsim->dev,
> > + "command write timed-out/interrupted.\n");
>
> This is really silly. Let's say that the program which results in
> this function called is using signals (eg, alarm() with SIGALRM, or
> asynchronous IO with SIGIO, etc).
>
> Why should having a SIGALRM raised print a kernel message? If this
> happens a lot, it will result in the kernel log being flooded with
> these messages.
>
> Signals should not be seen as exceptional conditions. For some programs,
> they are merely asynchronous events which are a normal part of the
> programs operation (eg, SIGIO, SIGALRM, etc.)
>
> Please, if you are going to handle signals, then handle them properly.
> If you're not going to handle them properly, don't use a wait that
> caters for them - use wait_for_completion_killable_timeout() which
> doesn't finish waiting on a signal unless the signal is going to result
> in the death of the program.
>
the current code would treat the signal case identical with the
completion success case - and that hardly can be the intention
so while it might not be necessary to call printk in the signal
case it should in some way be handled - if there is not need to
handle signals then it might be more resonable to use
wait_for_completion_timeout which is not interruptible.
So the key issue here is not that a signal should necessarily print
a message but that it should not be treated as the success case. The
current code will only treat timeout as an error condition and a received
signal (implying that the condition being waited for is most likely not
satisfied) as a successful completion.
thx!
hofrat
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
