* [PATCH] video: treat signal like timeout as failure
@ 2015-01-20 5:23 ` Nicholas Mc Guire
0 siblings, 0 replies; 33+ messages in thread
From: Nicholas Mc Guire @ 2015-01-20 5:23 UTC (permalink / raw)
To: linux-arm-kernel
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@hofr.at>
---
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");
mutex_unlock(&dsim->lock);
return -EAGAIN;
}
@@ -345,6 +348,7 @@ int exynos_mipi_dsi_rd_data(struct mipi_dsim_device *dsim, unsigned int data_id,
unsigned int rx_data, rcv_pkt, i;
u8 response = 0;
u16 rxsize;
+ long timeout;
if (dsim->state == DSIM_STATE_ULPS) {
dev_err(dsim->dev, "state is ULPS.\n");
@@ -380,9 +384,10 @@ int exynos_mipi_dsi_rd_data(struct mipi_dsim_device *dsim, unsigned int data_id,
return -EINVAL;
}
- if (!wait_for_completion_interruptible_timeout(&dsim_rd_comp,
- MIPI_FIFO_TIMEOUT)) {
- pr_err("RX done interrupt timeout\n");
+ timeout = wait_for_completion_interruptible_timeout(&dsim_rd_comp,
+ MIPI_FIFO_TIMEOUT);
+ if (timeout <= 0) {
+ pr_err("RX done interrupt timeout/interrupted\n");
mutex_unlock(&dsim->lock);
return 0;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH] video: treat signal like timeout as failure
@ 2015-01-20 5:23 ` Nicholas Mc Guire
0 siblings, 0 replies; 33+ messages in thread
From: Nicholas Mc Guire @ 2015-01-20 5:23 UTC (permalink / raw)
To: Inki Dae
Cc: Donghwa Lee, Kyungmin Park, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, Kukjin Kim, linux-fbdev, linux-arm-kernel,
linux-samsung-soc, Nicholas Mc Guire
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@hofr.at>
---
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");
mutex_unlock(&dsim->lock);
return -EAGAIN;
}
@@ -345,6 +348,7 @@ int exynos_mipi_dsi_rd_data(struct mipi_dsim_device *dsim, unsigned int data_id,
unsigned int rx_data, rcv_pkt, i;
u8 response = 0;
u16 rxsize;
+ long timeout;
if (dsim->state == DSIM_STATE_ULPS) {
dev_err(dsim->dev, "state is ULPS.\n");
@@ -380,9 +384,10 @@ int exynos_mipi_dsi_rd_data(struct mipi_dsim_device *dsim, unsigned int data_id,
return -EINVAL;
}
- if (!wait_for_completion_interruptible_timeout(&dsim_rd_comp,
- MIPI_FIFO_TIMEOUT)) {
- pr_err("RX done interrupt timeout\n");
+ timeout = wait_for_completion_interruptible_timeout(&dsim_rd_comp,
+ MIPI_FIFO_TIMEOUT);
+ if (timeout <= 0) {
+ pr_err("RX done interrupt timeout/interrupted\n");
mutex_unlock(&dsim->lock);
return 0;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH] video: treat signal like timeout as failure
2015-01-20 5:23 ` Nicholas Mc Guire
(?)
@ 2015-01-26 12:50 ` Tomi Valkeinen
-1 siblings, 0 replies; 33+ messages in thread
From: Tomi Valkeinen @ 2015-01-26 12:50 UTC (permalink / raw)
To: linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 1128 bytes --]
Hi,
On 20/01/15 07:23, 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@hofr.at>
> ---
>
> 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.
The code changes look ok to me, but again you have detailed descriptions
above which are not in the patch description. All the above looks like
something that should be in the patch description.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] video: treat signal like timeout as failure
@ 2015-01-26 12:50 ` Tomi Valkeinen
0 siblings, 0 replies; 33+ messages in thread
From: Tomi Valkeinen @ 2015-01-26 12:50 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On 20/01/15 07:23, 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@hofr.at>
> ---
>
> 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.
The code changes look ok to me, but again you have detailed descriptions
above which are not in the patch description. All the above looks like
something that should be in the patch description.
Tomi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150126/f242a1a6/attachment.sig>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] video: treat signal like timeout as failure
@ 2015-01-26 12:50 ` Tomi Valkeinen
0 siblings, 0 replies; 33+ messages in thread
From: Tomi Valkeinen @ 2015-01-26 12:50 UTC (permalink / raw)
To: Nicholas Mc Guire, Inki Dae
Cc: Donghwa Lee, Kyungmin Park, Jean-Christophe Plagniol-Villard,
Kukjin Kim, linux-fbdev, linux-arm-kernel, linux-samsung-soc
[-- Attachment #1: Type: text/plain, Size: 1128 bytes --]
Hi,
On 20/01/15 07:23, 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@hofr.at>
> ---
>
> 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.
The code changes look ok to me, but again you have detailed descriptions
above which are not in the patch description. All the above looks like
something that should be in the patch description.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] video: treat signal like timeout as failure
2015-01-20 5:23 ` Nicholas Mc Guire
(?)
@ 2015-01-26 12:59 ` Russell King - ARM Linux
-1 siblings, 0 replies; 33+ messages in thread
From: Russell King - ARM Linux @ 2015-01-26 12:59 UTC (permalink / raw)
To: linux-arm-kernel
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@hofr.at>
> ---
>
> 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.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 33+ messages in thread* [PATCH] video: treat signal like timeout as failure
@ 2015-01-26 12:59 ` Russell King - ARM Linux
0 siblings, 0 replies; 33+ messages in thread
From: Russell King - ARM Linux @ 2015-01-26 12:59 UTC (permalink / raw)
To: linux-arm-kernel
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@hofr.at>
> ---
>
> 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.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH] video: treat signal like timeout as failure
@ 2015-01-26 12:59 ` Russell King - ARM Linux
0 siblings, 0 replies; 33+ messages in thread
From: Russell King - ARM Linux @ 2015-01-26 12:59 UTC (permalink / raw)
To: Nicholas Mc Guire
Cc: Inki Dae, linux-fbdev, linux-samsung-soc, Donghwa Lee,
Kyungmin Park, Tomi Valkeinen, Kukjin Kim,
Jean-Christophe Plagniol-Villard, linux-arm-kernel
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@hofr.at>
> ---
>
> 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.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH] video: treat signal like timeout as failure
2015-01-26 12:59 ` Russell King - ARM Linux
(?)
@ 2015-01-29 9:43 ` Nicholas Mc Guire
-1 siblings, 0 replies; 33+ messages in thread
From: Nicholas Mc Guire @ 2015-01-29 9:43 UTC (permalink / raw)
To: linux-arm-kernel
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@hofr.at>
> > ---
> >
> > 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
^ permalink raw reply [flat|nested] 33+ messages in thread* [PATCH] video: treat signal like timeout as failure
@ 2015-01-29 9:43 ` Nicholas Mc Guire
0 siblings, 0 replies; 33+ messages in thread
From: Nicholas Mc Guire @ 2015-01-29 9:43 UTC (permalink / raw)
To: linux-arm-kernel
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@hofr.at>
> > ---
> >
> > 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
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH] video: treat signal like timeout as failure
@ 2015-01-29 9:43 ` Nicholas Mc Guire
0 siblings, 0 replies; 33+ messages in thread
From: Nicholas Mc Guire @ 2015-01-29 9:43 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Inki Dae, linux-fbdev, linux-samsung-soc, Donghwa Lee,
Kyungmin Park, Tomi Valkeinen, Kukjin Kim,
Jean-Christophe Plagniol-Villard, linux-arm-kernel
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@hofr.at>
> > ---
> >
> > 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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] video: treat signal like timeout as failure
2015-01-20 5:23 ` Nicholas Mc Guire
(?)
@ 2015-03-10 12:43 ` Tomi Valkeinen
-1 siblings, 0 replies; 33+ messages in thread
From: Tomi Valkeinen @ 2015-03-10 12:43 UTC (permalink / raw)
To: linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]
On 20/01/15 07:23, 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@hofr.at>
> ---
>
> 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.
While I agree that this patch is a bit better than the current state,
the code still looks wrong as Russell said.
I can merge this, but I'd rather have someone from Samsung look at the
code and change it to use wait_for_completion_killable_timeout() if
that's what this code is really supposed to use.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] video: treat signal like timeout as failure
@ 2015-03-10 12:43 ` Tomi Valkeinen
0 siblings, 0 replies; 33+ messages in thread
From: Tomi Valkeinen @ 2015-03-10 12:43 UTC (permalink / raw)
To: linux-arm-kernel
On 20/01/15 07:23, 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@hofr.at>
> ---
>
> 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.
While I agree that this patch is a bit better than the current state,
the code still looks wrong as Russell said.
I can merge this, but I'd rather have someone from Samsung look at the
code and change it to use wait_for_completion_killable_timeout() if
that's what this code is really supposed to use.
Tomi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150310/43bc402a/attachment.sig>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] video: treat signal like timeout as failure
@ 2015-03-10 12:43 ` Tomi Valkeinen
0 siblings, 0 replies; 33+ messages in thread
From: Tomi Valkeinen @ 2015-03-10 12:43 UTC (permalink / raw)
To: Nicholas Mc Guire, Inki Dae
Cc: Donghwa Lee, Kyungmin Park, Jean-Christophe Plagniol-Villard,
Kukjin Kim, linux-fbdev, linux-arm-kernel, linux-samsung-soc
[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]
On 20/01/15 07:23, 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@hofr.at>
> ---
>
> 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.
While I agree that this patch is a bit better than the current state,
the code still looks wrong as Russell said.
I can merge this, but I'd rather have someone from Samsung look at the
code and change it to use wait_for_completion_killable_timeout() if
that's what this code is really supposed to use.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] video: treat signal like timeout as failure
2015-03-10 12:43 ` Tomi Valkeinen
(?)
@ 2015-03-10 12:51 ` Nicholas Mc Guire
-1 siblings, 0 replies; 33+ messages in thread
From: Nicholas Mc Guire @ 2015-03-10 12:51 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 10 Mar 2015, Tomi Valkeinen wrote:
> On 20/01/15 07:23, 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@hofr.at>
> > ---
> >
> > 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.
>
> While I agree that this patch is a bit better than the current state,
> the code still looks wrong as Russell said.
>
> I can merge this, but I'd rather have someone from Samsung look at the
> code and change it to use wait_for_completion_killable_timeout() if
> that's what this code is really supposed to use.
>
If someone that knows the details takes care of it
that is of course the best solution. If someone Samsung is
going to look into it then it is probably best to completly
drop this speculative patch so that this does not lead
to more confusion than it does good.
thx!
hofrat
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] video: treat signal like timeout as failure
@ 2015-03-10 12:51 ` Nicholas Mc Guire
0 siblings, 0 replies; 33+ messages in thread
From: Nicholas Mc Guire @ 2015-03-10 12:51 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 10 Mar 2015, Tomi Valkeinen wrote:
> On 20/01/15 07:23, 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@hofr.at>
> > ---
> >
> > 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.
>
> While I agree that this patch is a bit better than the current state,
> the code still looks wrong as Russell said.
>
> I can merge this, but I'd rather have someone from Samsung look at the
> code and change it to use wait_for_completion_killable_timeout() if
> that's what this code is really supposed to use.
>
If someone that knows the details takes care of it
that is of course the best solution. If someone Samsung is
going to look into it then it is probably best to completly
drop this speculative patch so that this does not lead
to more confusion than it does good.
thx!
hofrat
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] video: treat signal like timeout as failure
@ 2015-03-10 12:51 ` Nicholas Mc Guire
0 siblings, 0 replies; 33+ messages in thread
From: Nicholas Mc Guire @ 2015-03-10 12:51 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Inki Dae, Donghwa Lee, Kyungmin Park,
Jean-Christophe Plagniol-Villard, Kukjin Kim, linux-fbdev,
linux-arm-kernel, linux-samsung-soc
On Tue, 10 Mar 2015, Tomi Valkeinen wrote:
> On 20/01/15 07:23, 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@hofr.at>
> > ---
> >
> > 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.
>
> While I agree that this patch is a bit better than the current state,
> the code still looks wrong as Russell said.
>
> I can merge this, but I'd rather have someone from Samsung look at the
> code and change it to use wait_for_completion_killable_timeout() if
> that's what this code is really supposed to use.
>
If someone that knows the details takes care of it
that is of course the best solution. If someone Samsung is
going to look into it then it is probably best to completly
drop this speculative patch so that this does not lead
to more confusion than it does good.
thx!
hofrat
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] video: treat signal like timeout as failure
2015-03-10 12:51 ` Nicholas Mc Guire
(?)
@ 2015-03-10 14:15 ` Russell King - ARM Linux
-1 siblings, 0 replies; 33+ messages in thread
From: Russell King - ARM Linux @ 2015-03-10 14:15 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Mar 10, 2015 at 01:51:16PM +0100, Nicholas Mc Guire wrote:
> On Tue, 10 Mar 2015, Tomi Valkeinen wrote:
>
> > On 20/01/15 07:23, 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@hofr.at>
> > > ---
> > >
> > > 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.
> >
> > While I agree that this patch is a bit better than the current state,
> > the code still looks wrong as Russell said.
> >
> > I can merge this, but I'd rather have someone from Samsung look at the
> > code and change it to use wait_for_completion_killable_timeout() if
> > that's what this code is really supposed to use.
> >
> If someone that knows the details takes care of it
> that is of course the best solution. If someone Samsung is
> going to look into it then it is probably best to completly
> drop this speculative patch so that this does not lead
> to more confusion than it does good.
IMHO, just change it to wait_for_completion_killable_timeout() - that's
a much better change than the change you're proposing.
If we think about it... The current code uses this:
if (!wait_for_completion_interruptible_timeout(&dsim_wr_comp,
MIPI_FIFO_TIMEOUT)) {
dev_warn(dsim->dev, "command write timeout.\n");
mutex_unlock(&dsim->lock);
return -EAGAIN;
}
which has the effect of treating a signal as "success", and doesn't return
an error. So, if the calling application receives (eg) a SIGPIPE or a
SIGALRM, we proceed as if we received the FIFO empty interrupt and doesn't
cause an error.
Your change results in:
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");
mutex_unlock(&dsim->lock);
return -EAGAIN;
}
which now means that this call returns -EAGAIN when a signal is raised.
Now, further auditing of this exynos crap (and I really do mean crap)
shows that this function is assigned to a method called "cmd_write".
Grepping for that shows that *no caller ever checks the return value*!
So, really, there's a bug here in that we should _never_ complete on a
signal, and we most *definitely can not* error out on a signal either.
The *only* sane change to this code without author/maintainer input is
to change this to wait_for_completion_killable_timeout() - so that
signals do not cause either premature completion nor premature failure
of the wait.
The proper fix is absolutely huge: all call paths need to be augmented
with code to detect this function failing, and back out whatever changes
they've made, and restoring the previous state (if they can) and
propagate the error all the way back to userland, so that syscall
restarting can work correctly. _Only then_ is it safe to use a call
which causes an interruptible sleep.
Personally, I'd be happier seeing this moved into drivers/staging and
eventually deleted from the kernel unless someone is willing to review
the driver and fix some of these glaring problems. I wouldn't be
surprised if there was _loads_ of this kind of crap there.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 33+ messages in thread* [PATCH] video: treat signal like timeout as failure
@ 2015-03-10 14:15 ` Russell King - ARM Linux
0 siblings, 0 replies; 33+ messages in thread
From: Russell King - ARM Linux @ 2015-03-10 14:15 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Mar 10, 2015 at 01:51:16PM +0100, Nicholas Mc Guire wrote:
> On Tue, 10 Mar 2015, Tomi Valkeinen wrote:
>
> > On 20/01/15 07:23, 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@hofr.at>
> > > ---
> > >
> > > 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.
> >
> > While I agree that this patch is a bit better than the current state,
> > the code still looks wrong as Russell said.
> >
> > I can merge this, but I'd rather have someone from Samsung look at the
> > code and change it to use wait_for_completion_killable_timeout() if
> > that's what this code is really supposed to use.
> >
> If someone that knows the details takes care of it
> that is of course the best solution. If someone Samsung is
> going to look into it then it is probably best to completly
> drop this speculative patch so that this does not lead
> to more confusion than it does good.
IMHO, just change it to wait_for_completion_killable_timeout() - that's
a much better change than the change you're proposing.
If we think about it... The current code uses this:
if (!wait_for_completion_interruptible_timeout(&dsim_wr_comp,
MIPI_FIFO_TIMEOUT)) {
dev_warn(dsim->dev, "command write timeout.\n");
mutex_unlock(&dsim->lock);
return -EAGAIN;
}
which has the effect of treating a signal as "success", and doesn't return
an error. So, if the calling application receives (eg) a SIGPIPE or a
SIGALRM, we proceed as if we received the FIFO empty interrupt and doesn't
cause an error.
Your change results in:
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");
mutex_unlock(&dsim->lock);
return -EAGAIN;
}
which now means that this call returns -EAGAIN when a signal is raised.
Now, further auditing of this exynos crap (and I really do mean crap)
shows that this function is assigned to a method called "cmd_write".
Grepping for that shows that *no caller ever checks the return value*!
So, really, there's a bug here in that we should _never_ complete on a
signal, and we most *definitely can not* error out on a signal either.
The *only* sane change to this code without author/maintainer input is
to change this to wait_for_completion_killable_timeout() - so that
signals do not cause either premature completion nor premature failure
of the wait.
The proper fix is absolutely huge: all call paths need to be augmented
with code to detect this function failing, and back out whatever changes
they've made, and restoring the previous state (if they can) and
propagate the error all the way back to userland, so that syscall
restarting can work correctly. _Only then_ is it safe to use a call
which causes an interruptible sleep.
Personally, I'd be happier seeing this moved into drivers/staging and
eventually deleted from the kernel unless someone is willing to review
the driver and fix some of these glaring problems. I wouldn't be
surprised if there was _loads_ of this kind of crap there.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH] video: treat signal like timeout as failure
@ 2015-03-10 14:15 ` Russell King - ARM Linux
0 siblings, 0 replies; 33+ messages in thread
From: Russell King - ARM Linux @ 2015-03-10 14:15 UTC (permalink / raw)
To: Nicholas Mc Guire
Cc: Tomi Valkeinen, linux-fbdev, linux-samsung-soc, Donghwa Lee,
Inki Dae, Kyungmin Park, Kukjin Kim,
Jean-Christophe Plagniol-Villard, linux-arm-kernel
On Tue, Mar 10, 2015 at 01:51:16PM +0100, Nicholas Mc Guire wrote:
> On Tue, 10 Mar 2015, Tomi Valkeinen wrote:
>
> > On 20/01/15 07:23, 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@hofr.at>
> > > ---
> > >
> > > 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.
> >
> > While I agree that this patch is a bit better than the current state,
> > the code still looks wrong as Russell said.
> >
> > I can merge this, but I'd rather have someone from Samsung look at the
> > code and change it to use wait_for_completion_killable_timeout() if
> > that's what this code is really supposed to use.
> >
> If someone that knows the details takes care of it
> that is of course the best solution. If someone Samsung is
> going to look into it then it is probably best to completly
> drop this speculative patch so that this does not lead
> to more confusion than it does good.
IMHO, just change it to wait_for_completion_killable_timeout() - that's
a much better change than the change you're proposing.
If we think about it... The current code uses this:
if (!wait_for_completion_interruptible_timeout(&dsim_wr_comp,
MIPI_FIFO_TIMEOUT)) {
dev_warn(dsim->dev, "command write timeout.\n");
mutex_unlock(&dsim->lock);
return -EAGAIN;
}
which has the effect of treating a signal as "success", and doesn't return
an error. So, if the calling application receives (eg) a SIGPIPE or a
SIGALRM, we proceed as if we received the FIFO empty interrupt and doesn't
cause an error.
Your change results in:
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");
mutex_unlock(&dsim->lock);
return -EAGAIN;
}
which now means that this call returns -EAGAIN when a signal is raised.
Now, further auditing of this exynos crap (and I really do mean crap)
shows that this function is assigned to a method called "cmd_write".
Grepping for that shows that *no caller ever checks the return value*!
So, really, there's a bug here in that we should _never_ complete on a
signal, and we most *definitely can not* error out on a signal either.
The *only* sane change to this code without author/maintainer input is
to change this to wait_for_completion_killable_timeout() - so that
signals do not cause either premature completion nor premature failure
of the wait.
The proper fix is absolutely huge: all call paths need to be augmented
with code to detect this function failing, and back out whatever changes
they've made, and restoring the previous state (if they can) and
propagate the error all the way back to userland, so that syscall
restarting can work correctly. _Only then_ is it safe to use a call
which causes an interruptible sleep.
Personally, I'd be happier seeing this moved into drivers/staging and
eventually deleted from the kernel unless someone is willing to review
the driver and fix some of these glaring problems. I wouldn't be
surprised if there was _loads_ of this kind of crap there.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH] video: treat signal like timeout as failure
2015-03-10 14:15 ` Russell King - ARM Linux
(?)
@ 2015-03-10 14:39 ` Nicholas Mc Guire
-1 siblings, 0 replies; 33+ messages in thread
From: Nicholas Mc Guire @ 2015-03-10 14:39 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 10 Mar 2015, Russell King - ARM Linux wrote:
> On Tue, Mar 10, 2015 at 01:51:16PM +0100, Nicholas Mc Guire wrote:
> > On Tue, 10 Mar 2015, Tomi Valkeinen wrote:
> >
> > > On 20/01/15 07:23, 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@hofr.at>
> > > > ---
> > > >
> > > > 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.
> > >
> > > While I agree that this patch is a bit better than the current state,
> > > the code still looks wrong as Russell said.
> > >
> > > I can merge this, but I'd rather have someone from Samsung look at the
> > > code and change it to use wait_for_completion_killable_timeout() if
> > > that's what this code is really supposed to use.
> > >
> > If someone that knows the details takes care of it
> > that is of course the best solution. If someone Samsung is
> > going to look into it then it is probably best to completly
> > drop this speculative patch so that this does not lead
> > to more confusion than it does good.
>
> IMHO, just change it to wait_for_completion_killable_timeout() - that's
> a much better change than the change you're proposing.
>
> If we think about it... The current code uses this:
>
> if (!wait_for_completion_interruptible_timeout(&dsim_wr_comp,
> MIPI_FIFO_TIMEOUT)) {
> dev_warn(dsim->dev, "command write timeout.\n");
> mutex_unlock(&dsim->lock);
> return -EAGAIN;
> }
>
> which has the effect of treating a signal as "success", and doesn't return
> an error. So, if the calling application receives (eg) a SIGPIPE or a
> SIGALRM, we proceed as if we received the FIFO empty interrupt and doesn't
> cause an error.
>
> Your change results in:
>
> 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");
> mutex_unlock(&dsim->lock);
> return -EAGAIN;
> }
>
> which now means that this call returns -EAGAIN when a signal is raised.
but in case of wait_for_completion_killable_timeout it also would return
-ERESTARTSYS (unless I'm missreading do_wait_for_common -> signal_pending_state(state, current)) so I still think it would be better to have the
dev_warn() in the path and then when the task is killed it atleast leaves
some trace of the of what was going on ?
>
> Now, further auditing of this exynos crap (and I really do mean crap)
> shows that this function is assigned to a method called "cmd_write".
> Grepping for that shows that *no caller ever checks the return value*!
>
yup - as was noted in the patch - and this is also why it was
not really possible to figure out what should really be done
as it runs into a dead end in all cases - the only point of the patch was
to atleast generate a debug message and return some signal
indicating error ... which is then unhandled...
> So, really, there's a bug here in that we should _never_ complete on a
> signal, and we most *definitely can not* error out on a signal either.
> The *only* sane change to this code without author/maintainer input is
> to change this to wait_for_completion_killable_timeout() - so that
> signals do not cause either premature completion nor premature failure
> of the wait.
>
> The proper fix is absolutely huge: all call paths need to be augmented
> with code to detect this function failing, and back out whatever changes
> they've made, and restoring the previous state (if they can) and
> propagate the error all the way back to userland, so that syscall
> restarting can work correctly. _Only then_ is it safe to use a call
> which causes an interruptible sleep.
>
> Personally, I'd be happier seeing this moved into drivers/staging and
> eventually deleted from the kernel unless someone is willing to review
> the driver and fix some of these glaring problems. I wouldn't be
> surprised if there was _loads_ of this kind of crap there.
>
there is plenty of this - actually all of the wait_for_completion* related
findings I've been posting in the past 2 month are based on the attempt to
write up a more or less complete API spec in form of coccinelle scripts that
then can be used to scan and sometimes fix-up this kind of problems - but of
course just "local-fixes" - this can't fix fundamentally broken code.
thx!
hofrat
^ permalink raw reply [flat|nested] 33+ messages in thread* [PATCH] video: treat signal like timeout as failure
@ 2015-03-10 14:39 ` Nicholas Mc Guire
0 siblings, 0 replies; 33+ messages in thread
From: Nicholas Mc Guire @ 2015-03-10 14:39 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 10 Mar 2015, Russell King - ARM Linux wrote:
> On Tue, Mar 10, 2015 at 01:51:16PM +0100, Nicholas Mc Guire wrote:
> > On Tue, 10 Mar 2015, Tomi Valkeinen wrote:
> >
> > > On 20/01/15 07:23, 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@hofr.at>
> > > > ---
> > > >
> > > > 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.
> > >
> > > While I agree that this patch is a bit better than the current state,
> > > the code still looks wrong as Russell said.
> > >
> > > I can merge this, but I'd rather have someone from Samsung look at the
> > > code and change it to use wait_for_completion_killable_timeout() if
> > > that's what this code is really supposed to use.
> > >
> > If someone that knows the details takes care of it
> > that is of course the best solution. If someone Samsung is
> > going to look into it then it is probably best to completly
> > drop this speculative patch so that this does not lead
> > to more confusion than it does good.
>
> IMHO, just change it to wait_for_completion_killable_timeout() - that's
> a much better change than the change you're proposing.
>
> If we think about it... The current code uses this:
>
> if (!wait_for_completion_interruptible_timeout(&dsim_wr_comp,
> MIPI_FIFO_TIMEOUT)) {
> dev_warn(dsim->dev, "command write timeout.\n");
> mutex_unlock(&dsim->lock);
> return -EAGAIN;
> }
>
> which has the effect of treating a signal as "success", and doesn't return
> an error. So, if the calling application receives (eg) a SIGPIPE or a
> SIGALRM, we proceed as if we received the FIFO empty interrupt and doesn't
> cause an error.
>
> Your change results in:
>
> 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");
> mutex_unlock(&dsim->lock);
> return -EAGAIN;
> }
>
> which now means that this call returns -EAGAIN when a signal is raised.
but in case of wait_for_completion_killable_timeout it also would return
-ERESTARTSYS (unless I'm missreading do_wait_for_common -> signal_pending_state(state, current)) so I still think it would be better to have the
dev_warn() in the path and then when the task is killed it atleast leaves
some trace of the of what was going on ?
>
> Now, further auditing of this exynos crap (and I really do mean crap)
> shows that this function is assigned to a method called "cmd_write".
> Grepping for that shows that *no caller ever checks the return value*!
>
yup - as was noted in the patch - and this is also why it was
not really possible to figure out what should really be done
as it runs into a dead end in all cases - the only point of the patch was
to atleast generate a debug message and return some signal
indicating error ... which is then unhandled...
> So, really, there's a bug here in that we should _never_ complete on a
> signal, and we most *definitely can not* error out on a signal either.
> The *only* sane change to this code without author/maintainer input is
> to change this to wait_for_completion_killable_timeout() - so that
> signals do not cause either premature completion nor premature failure
> of the wait.
>
> The proper fix is absolutely huge: all call paths need to be augmented
> with code to detect this function failing, and back out whatever changes
> they've made, and restoring the previous state (if they can) and
> propagate the error all the way back to userland, so that syscall
> restarting can work correctly. _Only then_ is it safe to use a call
> which causes an interruptible sleep.
>
> Personally, I'd be happier seeing this moved into drivers/staging and
> eventually deleted from the kernel unless someone is willing to review
> the driver and fix some of these glaring problems. I wouldn't be
> surprised if there was _loads_ of this kind of crap there.
>
there is plenty of this - actually all of the wait_for_completion* related
findings I've been posting in the past 2 month are based on the attempt to
write up a more or less complete API spec in form of coccinelle scripts that
then can be used to scan and sometimes fix-up this kind of problems - but of
course just "local-fixes" - this can't fix fundamentally broken code.
thx!
hofrat
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH] video: treat signal like timeout as failure
@ 2015-03-10 14:39 ` Nicholas Mc Guire
0 siblings, 0 replies; 33+ messages in thread
From: Nicholas Mc Guire @ 2015-03-10 14:39 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Tomi Valkeinen, linux-fbdev, linux-samsung-soc, Donghwa Lee,
Inki Dae, Kyungmin Park, Kukjin Kim,
Jean-Christophe Plagniol-Villard, linux-arm-kernel
On Tue, 10 Mar 2015, Russell King - ARM Linux wrote:
> On Tue, Mar 10, 2015 at 01:51:16PM +0100, Nicholas Mc Guire wrote:
> > On Tue, 10 Mar 2015, Tomi Valkeinen wrote:
> >
> > > On 20/01/15 07:23, 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@hofr.at>
> > > > ---
> > > >
> > > > 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.
> > >
> > > While I agree that this patch is a bit better than the current state,
> > > the code still looks wrong as Russell said.
> > >
> > > I can merge this, but I'd rather have someone from Samsung look at the
> > > code and change it to use wait_for_completion_killable_timeout() if
> > > that's what this code is really supposed to use.
> > >
> > If someone that knows the details takes care of it
> > that is of course the best solution. If someone Samsung is
> > going to look into it then it is probably best to completly
> > drop this speculative patch so that this does not lead
> > to more confusion than it does good.
>
> IMHO, just change it to wait_for_completion_killable_timeout() - that's
> a much better change than the change you're proposing.
>
> If we think about it... The current code uses this:
>
> if (!wait_for_completion_interruptible_timeout(&dsim_wr_comp,
> MIPI_FIFO_TIMEOUT)) {
> dev_warn(dsim->dev, "command write timeout.\n");
> mutex_unlock(&dsim->lock);
> return -EAGAIN;
> }
>
> which has the effect of treating a signal as "success", and doesn't return
> an error. So, if the calling application receives (eg) a SIGPIPE or a
> SIGALRM, we proceed as if we received the FIFO empty interrupt and doesn't
> cause an error.
>
> Your change results in:
>
> 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");
> mutex_unlock(&dsim->lock);
> return -EAGAIN;
> }
>
> which now means that this call returns -EAGAIN when a signal is raised.
but in case of wait_for_completion_killable_timeout it also would return
-ERESTARTSYS (unless I'm missreading do_wait_for_common -> signal_pending_state(state, current)) so I still think it would be better to have the
dev_warn() in the path and then when the task is killed it atleast leaves
some trace of the of what was going on ?
>
> Now, further auditing of this exynos crap (and I really do mean crap)
> shows that this function is assigned to a method called "cmd_write".
> Grepping for that shows that *no caller ever checks the return value*!
>
yup - as was noted in the patch - and this is also why it was
not really possible to figure out what should really be done
as it runs into a dead end in all cases - the only point of the patch was
to atleast generate a debug message and return some signal
indicating error ... which is then unhandled...
> So, really, there's a bug here in that we should _never_ complete on a
> signal, and we most *definitely can not* error out on a signal either.
> The *only* sane change to this code without author/maintainer input is
> to change this to wait_for_completion_killable_timeout() - so that
> signals do not cause either premature completion nor premature failure
> of the wait.
>
> The proper fix is absolutely huge: all call paths need to be augmented
> with code to detect this function failing, and back out whatever changes
> they've made, and restoring the previous state (if they can) and
> propagate the error all the way back to userland, so that syscall
> restarting can work correctly. _Only then_ is it safe to use a call
> which causes an interruptible sleep.
>
> Personally, I'd be happier seeing this moved into drivers/staging and
> eventually deleted from the kernel unless someone is willing to review
> the driver and fix some of these glaring problems. I wouldn't be
> surprised if there was _loads_ of this kind of crap there.
>
there is plenty of this - actually all of the wait_for_completion* related
findings I've been posting in the past 2 month are based on the attempt to
write up a more or less complete API spec in form of coccinelle scripts that
then can be used to scan and sometimes fix-up this kind of problems - but of
course just "local-fixes" - this can't fix fundamentally broken code.
thx!
hofrat
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH] video: treat signal like timeout as failure
2015-03-10 14:39 ` Nicholas Mc Guire
(?)
@ 2015-03-10 14:46 ` Russell King - ARM Linux
-1 siblings, 0 replies; 33+ messages in thread
From: Russell King - ARM Linux @ 2015-03-10 14:46 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Mar 10, 2015 at 03:39:28PM +0100, Nicholas Mc Guire wrote:
> On Tue, 10 Mar 2015, Russell King - ARM Linux wrote:
> > On Tue, Mar 10, 2015 at 01:51:16PM +0100, Nicholas Mc Guire wrote:
> > > On Tue, 10 Mar 2015, Tomi Valkeinen wrote:
> > >
> > > > On 20/01/15 07:23, 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@hofr.at>
> > > > > ---
> > > > >
> > > > > 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.
> > > >
> > > > While I agree that this patch is a bit better than the current state,
> > > > the code still looks wrong as Russell said.
> > > >
> > > > I can merge this, but I'd rather have someone from Samsung look at the
> > > > code and change it to use wait_for_completion_killable_timeout() if
> > > > that's what this code is really supposed to use.
> > > >
> > > If someone that knows the details takes care of it
> > > that is of course the best solution. If someone Samsung is
> > > going to look into it then it is probably best to completly
> > > drop this speculative patch so that this does not lead
> > > to more confusion than it does good.
> >
> > IMHO, just change it to wait_for_completion_killable_timeout() - that's
> > a much better change than the change you're proposing.
> >
> > If we think about it... The current code uses this:
> >
> > if (!wait_for_completion_interruptible_timeout(&dsim_wr_comp,
> > MIPI_FIFO_TIMEOUT)) {
> > dev_warn(dsim->dev, "command write timeout.\n");
> > mutex_unlock(&dsim->lock);
> > return -EAGAIN;
> > }
> >
> > which has the effect of treating a signal as "success", and doesn't return
> > an error. So, if the calling application receives (eg) a SIGPIPE or a
> > SIGALRM, we proceed as if we received the FIFO empty interrupt and doesn't
> > cause an error.
> >
> > Your change results in:
> >
> > 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");
> > mutex_unlock(&dsim->lock);
> > return -EAGAIN;
> > }
> >
> > which now means that this call returns -EAGAIN when a signal is raised.
>
> but in case of wait_for_completion_killable_timeout it also would return
> -ERESTARTSYS (unless I'm missreading do_wait_for_common -> signal_pending_state(state, current)) so I still think it would be better to have the
> dev_warn() in the path and then when the task is killed it atleast leaves
> some trace of the of what was going on ?
>
> >
> > Now, further auditing of this exynos crap (and I really do mean crap)
> > shows that this function is assigned to a method called "cmd_write".
> > Grepping for that shows that *no caller ever checks the return value*!
> >
>
> yup - as was noted in the patch - and this is also why it was
> not really possible to figure out what should really be done
> as it runs into a dead end in all cases - the only point of the patch was
> to atleast generate a debug message and return some signal
> indicating error ... which is then unhandled...
>
> > So, really, there's a bug here in that we should _never_ complete on a
> > signal, and we most *definitely can not* error out on a signal either.
> > The *only* sane change to this code without author/maintainer input is
> > to change this to wait_for_completion_killable_timeout() - so that
> > signals do not cause either premature completion nor premature failure
> > of the wait.
> >
> > The proper fix is absolutely huge: all call paths need to be augmented
> > with code to detect this function failing, and back out whatever changes
> > they've made, and restoring the previous state (if they can) and
> > propagate the error all the way back to userland, so that syscall
> > restarting can work correctly. _Only then_ is it safe to use a call
> > which causes an interruptible sleep.
> >
> > Personally, I'd be happier seeing this moved into drivers/staging and
> > eventually deleted from the kernel unless someone is willing to review
> > the driver and fix some of these glaring problems. I wouldn't be
> > surprised if there was _loads_ of this kind of crap there.
> >
> there is plenty of this - actually all of the wait_for_completion* related
> findings I've been posting in the past 2 month are based on the attempt to
> write up a more or less complete API spec in form of coccinelle scripts that
> then can be used to scan and sometimes fix-up this kind of problems - but of
> course just "local-fixes" - this can't fix fundamentally broken code.
In which case, let me propose that the exynos fbdev driver needs to be
moved to drivers/staging, and stay there until this stuff gets fixed.
drivers/staging is supposed to be for stuff which isn't up to the mark,
and which is potentially unstable. And that's what this driver exactly
is.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 33+ messages in thread* [PATCH] video: treat signal like timeout as failure
@ 2015-03-10 14:46 ` Russell King - ARM Linux
0 siblings, 0 replies; 33+ messages in thread
From: Russell King - ARM Linux @ 2015-03-10 14:46 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Mar 10, 2015 at 03:39:28PM +0100, Nicholas Mc Guire wrote:
> On Tue, 10 Mar 2015, Russell King - ARM Linux wrote:
> > On Tue, Mar 10, 2015 at 01:51:16PM +0100, Nicholas Mc Guire wrote:
> > > On Tue, 10 Mar 2015, Tomi Valkeinen wrote:
> > >
> > > > On 20/01/15 07:23, 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@hofr.at>
> > > > > ---
> > > > >
> > > > > 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.
> > > >
> > > > While I agree that this patch is a bit better than the current state,
> > > > the code still looks wrong as Russell said.
> > > >
> > > > I can merge this, but I'd rather have someone from Samsung look at the
> > > > code and change it to use wait_for_completion_killable_timeout() if
> > > > that's what this code is really supposed to use.
> > > >
> > > If someone that knows the details takes care of it
> > > that is of course the best solution. If someone Samsung is
> > > going to look into it then it is probably best to completly
> > > drop this speculative patch so that this does not lead
> > > to more confusion than it does good.
> >
> > IMHO, just change it to wait_for_completion_killable_timeout() - that's
> > a much better change than the change you're proposing.
> >
> > If we think about it... The current code uses this:
> >
> > if (!wait_for_completion_interruptible_timeout(&dsim_wr_comp,
> > MIPI_FIFO_TIMEOUT)) {
> > dev_warn(dsim->dev, "command write timeout.\n");
> > mutex_unlock(&dsim->lock);
> > return -EAGAIN;
> > }
> >
> > which has the effect of treating a signal as "success", and doesn't return
> > an error. So, if the calling application receives (eg) a SIGPIPE or a
> > SIGALRM, we proceed as if we received the FIFO empty interrupt and doesn't
> > cause an error.
> >
> > Your change results in:
> >
> > 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");
> > mutex_unlock(&dsim->lock);
> > return -EAGAIN;
> > }
> >
> > which now means that this call returns -EAGAIN when a signal is raised.
>
> but in case of wait_for_completion_killable_timeout it also would return
> -ERESTARTSYS (unless I'm missreading do_wait_for_common -> signal_pending_state(state, current)) so I still think it would be better to have the
> dev_warn() in the path and then when the task is killed it atleast leaves
> some trace of the of what was going on ?
>
> >
> > Now, further auditing of this exynos crap (and I really do mean crap)
> > shows that this function is assigned to a method called "cmd_write".
> > Grepping for that shows that *no caller ever checks the return value*!
> >
>
> yup - as was noted in the patch - and this is also why it was
> not really possible to figure out what should really be done
> as it runs into a dead end in all cases - the only point of the patch was
> to atleast generate a debug message and return some signal
> indicating error ... which is then unhandled...
>
> > So, really, there's a bug here in that we should _never_ complete on a
> > signal, and we most *definitely can not* error out on a signal either.
> > The *only* sane change to this code without author/maintainer input is
> > to change this to wait_for_completion_killable_timeout() - so that
> > signals do not cause either premature completion nor premature failure
> > of the wait.
> >
> > The proper fix is absolutely huge: all call paths need to be augmented
> > with code to detect this function failing, and back out whatever changes
> > they've made, and restoring the previous state (if they can) and
> > propagate the error all the way back to userland, so that syscall
> > restarting can work correctly. _Only then_ is it safe to use a call
> > which causes an interruptible sleep.
> >
> > Personally, I'd be happier seeing this moved into drivers/staging and
> > eventually deleted from the kernel unless someone is willing to review
> > the driver and fix some of these glaring problems. I wouldn't be
> > surprised if there was _loads_ of this kind of crap there.
> >
> there is plenty of this - actually all of the wait_for_completion* related
> findings I've been posting in the past 2 month are based on the attempt to
> write up a more or less complete API spec in form of coccinelle scripts that
> then can be used to scan and sometimes fix-up this kind of problems - but of
> course just "local-fixes" - this can't fix fundamentally broken code.
In which case, let me propose that the exynos fbdev driver needs to be
moved to drivers/staging, and stay there until this stuff gets fixed.
drivers/staging is supposed to be for stuff which isn't up to the mark,
and which is potentially unstable. And that's what this driver exactly
is.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH] video: treat signal like timeout as failure
@ 2015-03-10 14:46 ` Russell King - ARM Linux
0 siblings, 0 replies; 33+ messages in thread
From: Russell King - ARM Linux @ 2015-03-10 14:46 UTC (permalink / raw)
To: Nicholas Mc Guire
Cc: Tomi Valkeinen, linux-fbdev, linux-samsung-soc, Donghwa Lee,
Inki Dae, Kyungmin Park, Kukjin Kim,
Jean-Christophe Plagniol-Villard, linux-arm-kernel
On Tue, Mar 10, 2015 at 03:39:28PM +0100, Nicholas Mc Guire wrote:
> On Tue, 10 Mar 2015, Russell King - ARM Linux wrote:
> > On Tue, Mar 10, 2015 at 01:51:16PM +0100, Nicholas Mc Guire wrote:
> > > On Tue, 10 Mar 2015, Tomi Valkeinen wrote:
> > >
> > > > On 20/01/15 07:23, 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@hofr.at>
> > > > > ---
> > > > >
> > > > > 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.
> > > >
> > > > While I agree that this patch is a bit better than the current state,
> > > > the code still looks wrong as Russell said.
> > > >
> > > > I can merge this, but I'd rather have someone from Samsung look at the
> > > > code and change it to use wait_for_completion_killable_timeout() if
> > > > that's what this code is really supposed to use.
> > > >
> > > If someone that knows the details takes care of it
> > > that is of course the best solution. If someone Samsung is
> > > going to look into it then it is probably best to completly
> > > drop this speculative patch so that this does not lead
> > > to more confusion than it does good.
> >
> > IMHO, just change it to wait_for_completion_killable_timeout() - that's
> > a much better change than the change you're proposing.
> >
> > If we think about it... The current code uses this:
> >
> > if (!wait_for_completion_interruptible_timeout(&dsim_wr_comp,
> > MIPI_FIFO_TIMEOUT)) {
> > dev_warn(dsim->dev, "command write timeout.\n");
> > mutex_unlock(&dsim->lock);
> > return -EAGAIN;
> > }
> >
> > which has the effect of treating a signal as "success", and doesn't return
> > an error. So, if the calling application receives (eg) a SIGPIPE or a
> > SIGALRM, we proceed as if we received the FIFO empty interrupt and doesn't
> > cause an error.
> >
> > Your change results in:
> >
> > 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");
> > mutex_unlock(&dsim->lock);
> > return -EAGAIN;
> > }
> >
> > which now means that this call returns -EAGAIN when a signal is raised.
>
> but in case of wait_for_completion_killable_timeout it also would return
> -ERESTARTSYS (unless I'm missreading do_wait_for_common -> signal_pending_state(state, current)) so I still think it would be better to have the
> dev_warn() in the path and then when the task is killed it atleast leaves
> some trace of the of what was going on ?
>
> >
> > Now, further auditing of this exynos crap (and I really do mean crap)
> > shows that this function is assigned to a method called "cmd_write".
> > Grepping for that shows that *no caller ever checks the return value*!
> >
>
> yup - as was noted in the patch - and this is also why it was
> not really possible to figure out what should really be done
> as it runs into a dead end in all cases - the only point of the patch was
> to atleast generate a debug message and return some signal
> indicating error ... which is then unhandled...
>
> > So, really, there's a bug here in that we should _never_ complete on a
> > signal, and we most *definitely can not* error out on a signal either.
> > The *only* sane change to this code without author/maintainer input is
> > to change this to wait_for_completion_killable_timeout() - so that
> > signals do not cause either premature completion nor premature failure
> > of the wait.
> >
> > The proper fix is absolutely huge: all call paths need to be augmented
> > with code to detect this function failing, and back out whatever changes
> > they've made, and restoring the previous state (if they can) and
> > propagate the error all the way back to userland, so that syscall
> > restarting can work correctly. _Only then_ is it safe to use a call
> > which causes an interruptible sleep.
> >
> > Personally, I'd be happier seeing this moved into drivers/staging and
> > eventually deleted from the kernel unless someone is willing to review
> > the driver and fix some of these glaring problems. I wouldn't be
> > surprised if there was _loads_ of this kind of crap there.
> >
> there is plenty of this - actually all of the wait_for_completion* related
> findings I've been posting in the past 2 month are based on the attempt to
> write up a more or less complete API spec in form of coccinelle scripts that
> then can be used to scan and sometimes fix-up this kind of problems - but of
> course just "local-fixes" - this can't fix fundamentally broken code.
In which case, let me propose that the exynos fbdev driver needs to be
moved to drivers/staging, and stay there until this stuff gets fixed.
drivers/staging is supposed to be for stuff which isn't up to the mark,
and which is potentially unstable. And that's what this driver exactly
is.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH] video: treat signal like timeout as failure
2015-03-10 14:46 ` Russell King - ARM Linux
(?)
@ 2015-03-10 14:55 ` Tomi Valkeinen
-1 siblings, 0 replies; 33+ messages in thread
From: Tomi Valkeinen @ 2015-03-10 14:55 UTC (permalink / raw)
To: linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 1077 bytes --]
On 10/03/15 16:46, Russell King - ARM Linux wrote:
> In which case, let me propose that the exynos fbdev driver needs to be
> moved to drivers/staging, and stay there until this stuff gets fixed.
> drivers/staging is supposed to be for stuff which isn't up to the mark,
> and which is potentially unstable. And that's what this driver exactly
> is.
There is drivers/gpu/drm/exynos/ which is getting a lot of updates. So...
I'd propose removing the exynos fbdev driver if the exynos drm driver
offers the same functionality. I don't know if that's the case. Does the
drm driver support all the devices the fbdev supports?
Also, I'm not sure if and how we can remove drivers. If exynos fbdev
driver is dropped, that would perhaps break boards that have exynos
fbdev in their .dts file. And if the drm driver doesn't offer the exact
same /dev/fbX interface, it would break the userspace.
So I don't know if that's possible. But that's what I'd like to do,
eventually, for all the fbdev drivers. Implement drm driver, remove the
fbdev one.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] video: treat signal like timeout as failure
@ 2015-03-10 14:55 ` Tomi Valkeinen
0 siblings, 0 replies; 33+ messages in thread
From: Tomi Valkeinen @ 2015-03-10 14:55 UTC (permalink / raw)
To: linux-arm-kernel
On 10/03/15 16:46, Russell King - ARM Linux wrote:
> In which case, let me propose that the exynos fbdev driver needs to be
> moved to drivers/staging, and stay there until this stuff gets fixed.
> drivers/staging is supposed to be for stuff which isn't up to the mark,
> and which is potentially unstable. And that's what this driver exactly
> is.
There is drivers/gpu/drm/exynos/ which is getting a lot of updates. So...
I'd propose removing the exynos fbdev driver if the exynos drm driver
offers the same functionality. I don't know if that's the case. Does the
drm driver support all the devices the fbdev supports?
Also, I'm not sure if and how we can remove drivers. If exynos fbdev
driver is dropped, that would perhaps break boards that have exynos
fbdev in their .dts file. And if the drm driver doesn't offer the exact
same /dev/fbX interface, it would break the userspace.
So I don't know if that's possible. But that's what I'd like to do,
eventually, for all the fbdev drivers. Implement drm driver, remove the
fbdev one.
Tomi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150310/d88fe89e/attachment.sig>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] video: treat signal like timeout as failure
@ 2015-03-10 14:55 ` Tomi Valkeinen
0 siblings, 0 replies; 33+ messages in thread
From: Tomi Valkeinen @ 2015-03-10 14:55 UTC (permalink / raw)
To: Russell King - ARM Linux, Nicholas Mc Guire
Cc: linux-fbdev, linux-samsung-soc, Donghwa Lee, Inki Dae,
Kyungmin Park, Kukjin Kim, Jean-Christophe Plagniol-Villard,
linux-arm-kernel, Ajay Kumar
[-- Attachment #1: Type: text/plain, Size: 1077 bytes --]
On 10/03/15 16:46, Russell King - ARM Linux wrote:
> In which case, let me propose that the exynos fbdev driver needs to be
> moved to drivers/staging, and stay there until this stuff gets fixed.
> drivers/staging is supposed to be for stuff which isn't up to the mark,
> and which is potentially unstable. And that's what this driver exactly
> is.
There is drivers/gpu/drm/exynos/ which is getting a lot of updates. So...
I'd propose removing the exynos fbdev driver if the exynos drm driver
offers the same functionality. I don't know if that's the case. Does the
drm driver support all the devices the fbdev supports?
Also, I'm not sure if and how we can remove drivers. If exynos fbdev
driver is dropped, that would perhaps break boards that have exynos
fbdev in their .dts file. And if the drm driver doesn't offer the exact
same /dev/fbX interface, it would break the userspace.
So I don't know if that's possible. But that's what I'd like to do,
eventually, for all the fbdev drivers. Implement drm driver, remove the
fbdev one.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] video: treat signal like timeout as failure
2015-03-10 14:55 ` Tomi Valkeinen
(?)
@ 2015-03-10 15:26 ` Russell King - ARM Linux
-1 siblings, 0 replies; 33+ messages in thread
From: Russell King - ARM Linux @ 2015-03-10 15:26 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Mar 10, 2015 at 04:55:56PM +0200, Tomi Valkeinen wrote:
> On 10/03/15 16:46, Russell King - ARM Linux wrote:
>
> > In which case, let me propose that the exynos fbdev driver needs to be
> > moved to drivers/staging, and stay there until this stuff gets fixed.
> > drivers/staging is supposed to be for stuff which isn't up to the mark,
> > and which is potentially unstable. And that's what this driver exactly
> > is.
>
> There is drivers/gpu/drm/exynos/ which is getting a lot of updates. So...
>
> I'd propose removing the exynos fbdev driver if the exynos drm driver
> offers the same functionality. I don't know if that's the case. Does the
> drm driver support all the devices the fbdev supports?
>
> Also, I'm not sure if and how we can remove drivers. If exynos fbdev
> driver is dropped, that would perhaps break boards that have exynos
> fbdev in their .dts file. And if the drm driver doesn't offer the exact
> same /dev/fbX interface, it would break the userspace.
>
> So I don't know if that's possible. But that's what I'd like to do,
> eventually, for all the fbdev drivers. Implement drm driver, remove the
> fbdev one.
That's why I suggested moving it to drivers/staging - it's a hint that
the driver needs a serious amount of work, and when built as a module,
it also provides users with the hint that the module they're loading is
of questionable quality (which is definitely the case here.)
Others have done that kind of thing before - we've had drivers which
have fallen by the way side, and at some point the decision has been
made to move them to drivers/staging, and if nothing happens to fix
them up (showing that no one cares about them), they've eventually
been dropped.
Of course, us talking about this might be enough to spur some effort
to get the thing properly fixed. :)
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] video: treat signal like timeout as failure
@ 2015-03-10 15:26 ` Russell King - ARM Linux
0 siblings, 0 replies; 33+ messages in thread
From: Russell King - ARM Linux @ 2015-03-10 15:26 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Mar 10, 2015 at 04:55:56PM +0200, Tomi Valkeinen wrote:
> On 10/03/15 16:46, Russell King - ARM Linux wrote:
>
> > In which case, let me propose that the exynos fbdev driver needs to be
> > moved to drivers/staging, and stay there until this stuff gets fixed.
> > drivers/staging is supposed to be for stuff which isn't up to the mark,
> > and which is potentially unstable. And that's what this driver exactly
> > is.
>
> There is drivers/gpu/drm/exynos/ which is getting a lot of updates. So...
>
> I'd propose removing the exynos fbdev driver if the exynos drm driver
> offers the same functionality. I don't know if that's the case. Does the
> drm driver support all the devices the fbdev supports?
>
> Also, I'm not sure if and how we can remove drivers. If exynos fbdev
> driver is dropped, that would perhaps break boards that have exynos
> fbdev in their .dts file. And if the drm driver doesn't offer the exact
> same /dev/fbX interface, it would break the userspace.
>
> So I don't know if that's possible. But that's what I'd like to do,
> eventually, for all the fbdev drivers. Implement drm driver, remove the
> fbdev one.
That's why I suggested moving it to drivers/staging - it's a hint that
the driver needs a serious amount of work, and when built as a module,
it also provides users with the hint that the module they're loading is
of questionable quality (which is definitely the case here.)
Others have done that kind of thing before - we've had drivers which
have fallen by the way side, and at some point the decision has been
made to move them to drivers/staging, and if nothing happens to fix
them up (showing that no one cares about them), they've eventually
been dropped.
Of course, us talking about this might be enough to spur some effort
to get the thing properly fixed. :)
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] video: treat signal like timeout as failure
@ 2015-03-10 15:26 ` Russell King - ARM Linux
0 siblings, 0 replies; 33+ messages in thread
From: Russell King - ARM Linux @ 2015-03-10 15:26 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Nicholas Mc Guire, linux-fbdev, linux-samsung-soc, Donghwa Lee,
Inki Dae, Kyungmin Park, Kukjin Kim,
Jean-Christophe Plagniol-Villard, linux-arm-kernel, Ajay Kumar
On Tue, Mar 10, 2015 at 04:55:56PM +0200, Tomi Valkeinen wrote:
> On 10/03/15 16:46, Russell King - ARM Linux wrote:
>
> > In which case, let me propose that the exynos fbdev driver needs to be
> > moved to drivers/staging, and stay there until this stuff gets fixed.
> > drivers/staging is supposed to be for stuff which isn't up to the mark,
> > and which is potentially unstable. And that's what this driver exactly
> > is.
>
> There is drivers/gpu/drm/exynos/ which is getting a lot of updates. So...
>
> I'd propose removing the exynos fbdev driver if the exynos drm driver
> offers the same functionality. I don't know if that's the case. Does the
> drm driver support all the devices the fbdev supports?
>
> Also, I'm not sure if and how we can remove drivers. If exynos fbdev
> driver is dropped, that would perhaps break boards that have exynos
> fbdev in their .dts file. And if the drm driver doesn't offer the exact
> same /dev/fbX interface, it would break the userspace.
>
> So I don't know if that's possible. But that's what I'd like to do,
> eventually, for all the fbdev drivers. Implement drm driver, remove the
> fbdev one.
That's why I suggested moving it to drivers/staging - it's a hint that
the driver needs a serious amount of work, and when built as a module,
it also provides users with the hint that the module they're loading is
of questionable quality (which is definitely the case here.)
Others have done that kind of thing before - we've had drivers which
have fallen by the way side, and at some point the decision has been
made to move them to drivers/staging, and if nothing happens to fix
them up (showing that no one cares about them), they've eventually
been dropped.
Of course, us talking about this might be enough to spur some effort
to get the thing properly fixed. :)
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 33+ messages in thread