[patch 1/2] thinkpad_acpi: signedness bugs getting current_mode

[patch 1/2] thinkpad_acpi: signedness bugs getting current_mode

[Dan Carpenter]

This needs to be signed for the error handling to work.  Valid modes are
small positive integers.

Fixes: b790ceeb0fd9 ('thinkpad_acpi: Add adaptive_kbd_mode sysfs attr')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 56eaddc..024861d 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -2938,7 +2938,7 @@ static ssize_t adaptive_kbd_mode_show(struct device *dev,
 			   struct device_attribute *attr,
 			   char *buf)
 {
-	u32 current_mode;
+	int current_mode;
 
 	current_mode = adaptive_keyboard_get_mode();
 	if (current_mode < 0)
@@ -3621,7 +3621,7 @@ static int adaptive_keyboard_get_next_mode(int mode)
 
 static bool adaptive_keyboard_hotkey_notify_hotkey(unsigned int scancode)
 {
-	u32 current_mode = 0;
+	int current_mode = 0;
 	int new_mode = 0;
 	int keycode;
 

Re: [patch 1/2] thinkpad_acpi: signedness bugs getting current_mode

[Bastien Nocera]

On Wed, 2015-03-11 at 12:34 +0300, Dan Carpenter wrote:
> This needs to be signed for the error handling to work.  Valid modes 
> are small positive integers.
> 
> Fixes: b790ceeb0fd9 ('thinkpad_acpi: Add adaptive_kbd_mode sysfs 
> attr') Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Acked-By: Bastien Nocera <hadess@hadess.net>

Re: [patch 1/2] thinkpad_acpi: signedness bugs getting current_mode

[Henrique de Moraes Holschuh]

On Wed, Mar 11, 2015, at 06:34, Dan Carpenter wrote:
> This needs to be signed for the error handling to work.  Valid modes are
> small positive integers.
> 
> Fixes: b790ceeb0fd9 ('thinkpad_acpi: Add adaptive_kbd_mode sysfs attr')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index 56eaddc..024861d 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -2938,7 +2938,7 @@ static ssize_t adaptive_kbd_mode_show(struct device
> *dev,
>  			   struct device_attribute *attr,
>  			   char *buf)
>  {
> -       u32 current_mode;
> +       int current_mode;
>  
>  	current_mode = adaptive_keyboard_get_mode();
>  	if (current_mode < 0)
> @@ -3621,7 +3621,7 @@ static int adaptive_keyboard_get_next_mode(int
> mode)
>  
>  static bool adaptive_keyboard_hotkey_notify_hotkey(unsigned int
>  scancode)
>  {
> -       u32 current_mode = 0;
> +       int current_mode = 0;
>  	int new_mode = 0;
>  	int keycode;
>  

Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [patch 1/2] thinkpad_acpi: signedness bugs getting current_mode

[Darren Hart]

On Wed, Mar 11, 2015 at 12:34:50PM +0300, Dan Carpenter wrote:
> This needs to be signed for the error handling to work.  Valid modes are
> small positive integers.
> 
> Fixes: b790ceeb0fd9 ('thinkpad_acpi: Add adaptive_kbd_mode sysfs attr')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 

Queued, thanks Dan!

-- 
Darren Hart
Intel Open Source Technology Center

Re: [patch 1/2] thinkpad_acpi: signedness bugs getting current_mode

[Darren Hart]

On Wed, Mar 11, 2015 at 12:34:50PM +0300, Dan Carpenter wrote:
> This needs to be signed for the error handling to work.  Valid modes are
> small positive integers.
> 
> Fixes: b790ceeb0fd9 ('thinkpad_acpi: Add adaptive_kbd_mode sysfs attr')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 

Question for HPA, Rafael, and Stephen,

I recall discussion at Kernel Summit 2014 about not rebasing or merging patches
when sending to Linus, that he'd prefer to see the history. I recall Stephen
mentioning something similar for linux-next.

That said, I've seen varying behavior among maintainers with respect to fixes
like this one from Dan. This patch fixes a patch that currently only exists in
my for-next and Stephen's linux-next trees.

What is the preference. Do I just queue it up to for-next as is (this is what
I've done for now), or do I roll it into the referred patch causing the error
and credit Dan with the fixup?

Left to my own devices I would prefer not to introduce bugs into the kernel
history if I can help it. That said, I don't want to make extra work for Stephen
or Linus.

What's the prefered best practice here?

> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 56eaddc..024861d 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -2938,7 +2938,7 @@ static ssize_t adaptive_kbd_mode_show(struct device *dev,
>  			   struct device_attribute *attr,
>  			   char *buf)
>  {
> -	u32 current_mode;
> +	int current_mode;
>  
>  	current_mode = adaptive_keyboard_get_mode();
>  	if (current_mode < 0)
> @@ -3621,7 +3621,7 @@ static int adaptive_keyboard_get_next_mode(int mode)
>  
>  static bool adaptive_keyboard_hotkey_notify_hotkey(unsigned int scancode)
>  {
> -	u32 current_mode = 0;
> +	int current_mode = 0;
>  	int new_mode = 0;
>  	int keycode;
>  
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [patch 1/2] thinkpad_acpi: signedness bugs getting current_mode

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 2290 bytes --]

Hi Darren,

On Sat, 14 Mar 2015 14:06:12 -0700 Darren Hart <dvhart@infradead.org> wrote:
>
> On Wed, Mar 11, 2015 at 12:34:50PM +0300, Dan Carpenter wrote:
> > This needs to be signed for the error handling to work.  Valid modes are
> > small positive integers.
> > 
> > Fixes: b790ceeb0fd9 ('thinkpad_acpi: Add adaptive_kbd_mode sysfs attr')
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> 
> Question for HPA, Rafael, and Stephen,
> 
> I recall discussion at Kernel Summit 2014 about not rebasing or merging patches
> when sending to Linus, that he'd prefer to see the history. I recall Stephen
> mentioning something similar for linux-next.
> 
> That said, I've seen varying behavior among maintainers with respect to fixes
> like this one from Dan. This patch fixes a patch that currently only exists in
> my for-next and Stephen's linux-next trees.
> 
> What is the preference. Do I just queue it up to for-next as is (this is what
> I've done for now), or do I roll it into the referred patch causing the error
> and credit Dan with the fixup?

The only answer, is "it depends" :-)  If it will cause problems for
people doing bisects, or if it is just the last one of two commits on
the top of your tree, it seems like it is OK to just squash the fix
into the original patch (with attribution as you say).  Otherwise, I
would prefer that trees just progress with no rebasing.

> Left to my own devices I would prefer not to introduce bugs into the
> kernel history if I can help it. That said, I don't want to make
> extra work for Stephen or Linus.

It is very unusual for a small rebase to cause me any more work (unless
the files involved and modified by other trees).  And for Linus, well
all he sees is the tree that you submit to him.

The people you will inconvenience more by rebasing are the developers
who write patches that are based on your tree.  If you rebase under
them, they may have to rebase and fix up the patches they have already
tested and had reviewed before they can then submit them to you
(hopefully before you rebase again).

Dave Miller seems to get on quite well in the networking tree with no
rebasing at all ...

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [patch 1/2] thinkpad_acpi: signedness bugs getting current_mode

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 676 bytes --]

Hi Darren,

On Sun, 15 Mar 2015 13:46:12 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> The people you will inconvenience more by rebasing are the developers
> who write patches that are based on your tree.  If you rebase under
> them, they may have to rebase and fix up the patches they have already
> tested and had reviewed before they can then submit them to you
> (hopefully before you rebase again).

This, of course, only applied to published trees (which includes
anything in linux-next) - what you do in your own (logically private)
development tree is your own business.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [patch 1/2] thinkpad_acpi: signedness bugs getting current_mode

[Darren Hart]

On Sun, Mar 15, 2015 at 01:48:13PM +1100, Stephen Rothwell wrote:
> Hi Darren,
> 
> On Sun, 15 Mar 2015 13:46:12 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> >
> > The people you will inconvenience more by rebasing are the developers
> > who write patches that are based on your tree.  If you rebase under
> > them, they may have to rebase and fix up the patches they have already
> > tested and had reviewed before they can then submit them to you
> > (hopefully before you rebase again).
> 
> This, of course, only applied to published trees (which includes
> anything in linux-next) - what you do in your own (logically private)
> development tree is your own business.

Thanks for the guidance Stephen. Unless a more compelling argument comes up,
I'll stick to this going forward.


-- 
Darren Hart
Intel Open Source Technology Center

Re: [ibm-acpi-devel] [patch 1/2] thinkpad_acpi: signedness bugs getting current_mode

[Henrique de Moraes Holschuh]

On Wed, Mar 11, 2015, at 06:34, Dan Carpenter wrote:
> This needs to be signed for the error handling to work.  Valid modes are
> small positive integers.
> 
> Fixes: b790ceeb0fd9 ('thinkpad_acpi: Add adaptive_kbd_mode sysfs attr')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh