* [PATCH 1/2] Input: serio_raw - cosmetic fixes @ 2012-01-06 11:03 Che-Liang Chiou 2012-01-06 11:03 ` [PATCH 2/2] Input: serio_raw - return error code instead of written on error Che-Liang Chiou 2012-01-09 8:24 ` [PATCH 1/2] Input: serio_raw - cosmetic fixes Dmitry Torokhov 0 siblings, 2 replies; 7+ messages in thread From: Che-Liang Chiou @ 2012-01-06 11:03 UTC (permalink / raw) To: dmitry.torokhov; +Cc: linux-kernel, Che-Liang Chiou Signed-off-by: Che-Liang Chiou <clchiou@chromium.org> --- drivers/input/serio/serio_raw.c | 4 +--- 1 files changed, 1 insertions(+), 3 deletions(-) diff --git a/drivers/input/serio/serio_raw.c b/drivers/input/serio/serio_raw.c index 4d4cd14..a935c38 100644 --- a/drivers/input/serio/serio_raw.c +++ b/drivers/input/serio/serio_raw.c @@ -220,7 +220,7 @@ static ssize_t serio_raw_write(struct file *file, const char __user *buffer, goto out; } written++; - }; + } out: mutex_unlock(&serio_raw_mutex); @@ -231,11 +231,9 @@ static unsigned int serio_raw_poll(struct file *file, poll_table *wait) { struct serio_raw_client *client = file->private_data; struct serio_raw *serio_raw = client->serio_raw; - unsigned int mask; poll_wait(file, &serio_raw->wait, wait); - mask = serio_raw->dead ? POLLHUP | POLLERR : POLLOUT | POLLWRNORM; if (serio_raw->head != serio_raw->tail) return POLLIN | POLLRDNORM; -- 1.7.3.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] Input: serio_raw - return error code instead of written on error 2012-01-06 11:03 [PATCH 1/2] Input: serio_raw - cosmetic fixes Che-Liang Chiou @ 2012-01-06 11:03 ` Che-Liang Chiou 2012-01-09 8:27 ` Dmitry Torokhov 2012-01-09 8:24 ` [PATCH 1/2] Input: serio_raw - cosmetic fixes Dmitry Torokhov 1 sibling, 1 reply; 7+ messages in thread From: Che-Liang Chiou @ 2012-01-06 11:03 UTC (permalink / raw) To: dmitry.torokhov; +Cc: linux-kernel, Che-Liang Chiou Even if an error occurs and error code is set, serio_raw_write() returns the amount of bytes written anyway. If this behavior is actually desirable, serio_raw_write() should not even bother to set the error code because it is not intended to be returned to the caller. Signed-off-by: Che-Liang Chiou <clchiou@chromium.org> --- drivers/input/serio/serio_raw.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/input/serio/serio_raw.c b/drivers/input/serio/serio_raw.c index a935c38..32416f7 100644 --- a/drivers/input/serio/serio_raw.c +++ b/drivers/input/serio/serio_raw.c @@ -224,7 +224,7 @@ static ssize_t serio_raw_write(struct file *file, const char __user *buffer, out: mutex_unlock(&serio_raw_mutex); - return written; + return retval ?: written; } static unsigned int serio_raw_poll(struct file *file, poll_table *wait) -- 1.7.3.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Input: serio_raw - return error code instead of written on error 2012-01-06 11:03 ` [PATCH 2/2] Input: serio_raw - return error code instead of written on error Che-Liang Chiou @ 2012-01-09 8:27 ` Dmitry Torokhov 2012-01-10 8:27 ` Che-liang Chiou 0 siblings, 1 reply; 7+ messages in thread From: Dmitry Torokhov @ 2012-01-09 8:27 UTC (permalink / raw) To: Che-Liang Chiou; +Cc: linux-kernel Hi Che-Liang, On Fri, Jan 06, 2012 at 07:03:15PM +0800, Che-Liang Chiou wrote: > Even if an error occurs and error code is set, serio_raw_write() returns > the amount of bytes written anyway. > > If this behavior is actually desirable, serio_raw_write() should not > even bother to set the error code because it is not intended to be > returned to the caller. Thank you for letting me know. The proper behavior is to return number of bytes successfully written in case of short (or full) transfer, and return appropriate error in case when we fail to transmit the very first byte. I will be applying the patch below. Thanks. -- Dmitry Input: serio-raw - return proper result when serio_raw_write fails From: Dmitry Torokhov <dmitry.torokhov@gmail.com> If serio_raw_write was always returning number of bytes successfully sent to serio port and never signalled error condition to the caller. Change it so that for completely failed transfers appropriate error code returned to the caller (partially successful writes still return number of bytes transferred). Signed-off-by: Dmitry Torokhov <dtor@mail.ru> --- drivers/input/serio/serio_raw.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/input/serio/serio_raw.c b/drivers/input/serio/serio_raw.c index c2c6ad8..5d9f9b1 100644 --- a/drivers/input/serio/serio_raw.c +++ b/drivers/input/serio/serio_raw.c @@ -224,7 +224,7 @@ static ssize_t serio_raw_write(struct file *file, const char __user *buffer, out: mutex_unlock(&serio_raw_mutex); - return written; + return written ?: retval; } static unsigned int serio_raw_poll(struct file *file, poll_table *wait) ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Input: serio_raw - return error code instead of written on error 2012-01-09 8:27 ` Dmitry Torokhov @ 2012-01-10 8:27 ` Che-liang Chiou 0 siblings, 0 replies; 7+ messages in thread From: Che-liang Chiou @ 2012-01-10 8:27 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: linux-kernel On Mon, Jan 9, 2012 at 4:27 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > Hi Che-Liang, > > On Fri, Jan 06, 2012 at 07:03:15PM +0800, Che-Liang Chiou wrote: >> Even if an error occurs and error code is set, serio_raw_write() returns >> the amount of bytes written anyway. >> >> If this behavior is actually desirable, serio_raw_write() should not >> even bother to set the error code because it is not intended to be >> returned to the caller. > > Thank you for letting me know. The proper behavior is to return number of > bytes successfully written in case of short (or full) transfer, and > return appropriate error in case when we fail to transmit the very first > byte. Look good to me. Thanks. > I will be applying the patch below. > > Thanks. > > -- > Dmitry > > Input: serio-raw - return proper result when serio_raw_write fails > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > If serio_raw_write was always returning number of bytes successfully > sent to serio port and never signalled error condition to the caller. > Change it so that for completely failed transfers appropriate error > code returned to the caller (partially successful writes still return > number of bytes transferred). > > Signed-off-by: Dmitry Torokhov <dtor@mail.ru> > --- > > drivers/input/serio/serio_raw.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > > diff --git a/drivers/input/serio/serio_raw.c b/drivers/input/serio/serio_raw.c > index c2c6ad8..5d9f9b1 100644 > --- a/drivers/input/serio/serio_raw.c > +++ b/drivers/input/serio/serio_raw.c > @@ -224,7 +224,7 @@ static ssize_t serio_raw_write(struct file *file, const char __user *buffer, > > out: > mutex_unlock(&serio_raw_mutex); > - return written; > + return written ?: retval; > } > > static unsigned int serio_raw_poll(struct file *file, poll_table *wait) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] Input: serio_raw - cosmetic fixes 2012-01-06 11:03 [PATCH 1/2] Input: serio_raw - cosmetic fixes Che-Liang Chiou 2012-01-06 11:03 ` [PATCH 2/2] Input: serio_raw - return error code instead of written on error Che-Liang Chiou @ 2012-01-09 8:24 ` Dmitry Torokhov 2012-01-10 8:24 ` Che-liang Chiou 1 sibling, 1 reply; 7+ messages in thread From: Dmitry Torokhov @ 2012-01-09 8:24 UTC (permalink / raw) To: Che-Liang Chiou; +Cc: linux-kernel Hi Che-Liang, On Fri, Jan 06, 2012 at 07:03:14PM +0800, Che-Liang Chiou wrote: > Signed-off-by: Che-Liang Chiou <clchiou@chromium.org> > --- > drivers/input/serio/serio_raw.c | 4 +--- > 1 files changed, 1 insertions(+), 3 deletions(-) > > diff --git a/drivers/input/serio/serio_raw.c b/drivers/input/serio/serio_raw.c > index 4d4cd14..a935c38 100644 > --- a/drivers/input/serio/serio_raw.c > +++ b/drivers/input/serio/serio_raw.c > @@ -220,7 +220,7 @@ static ssize_t serio_raw_write(struct file *file, const char __user *buffer, > goto out; > } > written++; > - }; > + } > I applied this chunk, thanks. > out: > mutex_unlock(&serio_raw_mutex); > @@ -231,11 +231,9 @@ static unsigned int serio_raw_poll(struct file *file, poll_table *wait) > { > struct serio_raw_client *client = file->private_data; > struct serio_raw *serio_raw = client->serio_raw; > - unsigned int mask; > > poll_wait(file, &serio_raw->wait, wait); > > - mask = serio_raw->dead ? POLLHUP | POLLERR : POLLOUT | POLLWRNORM; > if (serio_raw->head != serio_raw->tail) > return POLLIN | POLLRDNORM; > This however is not quite correct. I will be applying the patch below instead. Thanks. -- Dmitry Input: serio-raw - really signal HUP upon disconnect From: Dmitry Torokhov <dmitry.torokhov@gmail.com> Commit 8c1c10d5706bbb3b41cb4a5578339d67d3eeffc2 attempted to signal POLLHUP | POLLERR condition when polling disconnected device, unfortunately it did not do it quite correctly. Reported-by: Che-Liang Chiou <clchiou@chromium.org> Signed-off-by: Dmitry Torokhov <dtor@mail.ru> --- drivers/input/serio/serio_raw.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/input/serio/serio_raw.c b/drivers/input/serio/serio_raw.c index ca78a89..c2c6ad8 100644 --- a/drivers/input/serio/serio_raw.c +++ b/drivers/input/serio/serio_raw.c @@ -237,7 +237,7 @@ static unsigned int serio_raw_poll(struct file *file, poll_table *wait) mask = serio_raw->dead ? POLLHUP | POLLERR : POLLOUT | POLLWRNORM; if (serio_raw->head != serio_raw->tail) - return POLLIN | POLLRDNORM; + mask |= POLLIN | POLLRDNORM; return 0; } ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] Input: serio_raw - cosmetic fixes 2012-01-09 8:24 ` [PATCH 1/2] Input: serio_raw - cosmetic fixes Dmitry Torokhov @ 2012-01-10 8:24 ` Che-liang Chiou 2012-01-10 8:39 ` Dmitry Torokhov 0 siblings, 1 reply; 7+ messages in thread From: Che-liang Chiou @ 2012-01-10 8:24 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: linux-kernel On Mon, Jan 9, 2012 at 4:24 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > Hi Che-Liang, > > On Fri, Jan 06, 2012 at 07:03:14PM +0800, Che-Liang Chiou wrote: >> Signed-off-by: Che-Liang Chiou <clchiou@chromium.org> >> --- >> drivers/input/serio/serio_raw.c | 4 +--- >> 1 files changed, 1 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/input/serio/serio_raw.c b/drivers/input/serio/serio_raw.c >> index 4d4cd14..a935c38 100644 >> --- a/drivers/input/serio/serio_raw.c >> +++ b/drivers/input/serio/serio_raw.c >> @@ -220,7 +220,7 @@ static ssize_t serio_raw_write(struct file *file, const char __user *buffer, >> goto out; >> } >> written++; >> - }; >> + } >> > > I applied this chunk, thanks. > >> out: >> mutex_unlock(&serio_raw_mutex); >> @@ -231,11 +231,9 @@ static unsigned int serio_raw_poll(struct file *file, poll_table *wait) >> { >> struct serio_raw_client *client = file->private_data; >> struct serio_raw *serio_raw = client->serio_raw; >> - unsigned int mask; >> >> poll_wait(file, &serio_raw->wait, wait); >> >> - mask = serio_raw->dead ? POLLHUP | POLLERR : POLLOUT | POLLWRNORM; >> if (serio_raw->head != serio_raw->tail) >> return POLLIN | POLLRDNORM; >> > > This however is not quite correct. I will be applying the patch below > instead. Yeah I was wondering maybe you are going to make use of mask somewhere. But still the patch below is quite strange. To my understanding the mask is still not used; It is a local variable that not does not make any side effect. Do you intend to use that mask on any global state or return value? > Thanks. > > -- > Dmitry > > Input: serio-raw - really signal HUP upon disconnect > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > Commit 8c1c10d5706bbb3b41cb4a5578339d67d3eeffc2 attempted to signal > POLLHUP | POLLERR condition when polling disconnected device, > unfortunately it did not do it quite correctly. > > Reported-by: Che-Liang Chiou <clchiou@chromium.org> > Signed-off-by: Dmitry Torokhov <dtor@mail.ru> > --- > > drivers/input/serio/serio_raw.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > > diff --git a/drivers/input/serio/serio_raw.c b/drivers/input/serio/serio_raw.c > index ca78a89..c2c6ad8 100644 > --- a/drivers/input/serio/serio_raw.c > +++ b/drivers/input/serio/serio_raw.c > @@ -237,7 +237,7 @@ static unsigned int serio_raw_poll(struct file *file, poll_table *wait) > > mask = serio_raw->dead ? POLLHUP | POLLERR : POLLOUT | POLLWRNORM; > if (serio_raw->head != serio_raw->tail) > - return POLLIN | POLLRDNORM; > + mask |= POLLIN | POLLRDNORM; It seems that mask is a local variable that seems have no effect to any global state? > return 0; > } ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] Input: serio_raw - cosmetic fixes 2012-01-10 8:24 ` Che-liang Chiou @ 2012-01-10 8:39 ` Dmitry Torokhov 0 siblings, 0 replies; 7+ messages in thread From: Dmitry Torokhov @ 2012-01-10 8:39 UTC (permalink / raw) To: Che-liang Chiou; +Cc: linux-kernel On Tue, Jan 10, 2012 at 04:24:37PM +0800, Che-liang Chiou wrote: > On Mon, Jan 9, 2012 at 4:24 PM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > Hi Che-Liang, > > > > On Fri, Jan 06, 2012 at 07:03:14PM +0800, Che-Liang Chiou wrote: > >> Signed-off-by: Che-Liang Chiou <clchiou@chromium.org> > >> --- > >> drivers/input/serio/serio_raw.c | 4 +--- > >> 1 files changed, 1 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/input/serio/serio_raw.c b/drivers/input/serio/serio_raw.c > >> index 4d4cd14..a935c38 100644 > >> --- a/drivers/input/serio/serio_raw.c > >> +++ b/drivers/input/serio/serio_raw.c > >> @@ -220,7 +220,7 @@ static ssize_t serio_raw_write(struct file *file, const char __user *buffer, > >> goto out; > >> } > >> written++; > >> - }; > >> + } > >> > > > > I applied this chunk, thanks. > > > >> out: > >> mutex_unlock(&serio_raw_mutex); > >> @@ -231,11 +231,9 @@ static unsigned int serio_raw_poll(struct file *file, poll_table *wait) > >> { > >> struct serio_raw_client *client = file->private_data; > >> struct serio_raw *serio_raw = client->serio_raw; > >> - unsigned int mask; > >> > >> poll_wait(file, &serio_raw->wait, wait); > >> > >> - mask = serio_raw->dead ? POLLHUP | POLLERR : POLLOUT | POLLWRNORM; > >> if (serio_raw->head != serio_raw->tail) > >> return POLLIN | POLLRDNORM; > >> > > > > This however is not quite correct. I will be applying the patch below > > instead. > > Yeah I was wondering maybe you are going to make use of mask > somewhere. But still the patch below is quite strange. To my > understanding the mask is still not used; It is a local variable that > not does not make any side effect. Do you intend to use that mask on > any global state or return value? Right, Milton Miller noticed the same. We should be returning mask, not 0. I'll fix that up. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-01-10 8:39 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-01-06 11:03 [PATCH 1/2] Input: serio_raw - cosmetic fixes Che-Liang Chiou 2012-01-06 11:03 ` [PATCH 2/2] Input: serio_raw - return error code instead of written on error Che-Liang Chiou 2012-01-09 8:27 ` Dmitry Torokhov 2012-01-10 8:27 ` Che-liang Chiou 2012-01-09 8:24 ` [PATCH 1/2] Input: serio_raw - cosmetic fixes Dmitry Torokhov 2012-01-10 8:24 ` Che-liang Chiou 2012-01-10 8:39 ` Dmitry Torokhov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).