Re: [PATCH] hwmon: (applesmc) Fix checkpatch errors

Re: [PATCH] hwmon: (applesmc) Fix checkpatch errors

[Henrik Rydberg]

On 11/10/2010 07:53 PM, Guenter Roeck wrote:

> Fix all checkpatch errors and most of the warnings.
> 
> Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> ---
>  drivers/hwmon/applesmc.c |   40 +++++++++++++++++++++-------------------
>  1 files changed, 21 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 1e49b30..365761b 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -174,9 +174,8 @@ static int __wait_status(u8 val)
>  
>  	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
>  		udelay(us);
> -		if ((inb(APPLESMC_CMD_PORT) & APPLESMC_STATUS_MASK) == val) {
> +		if ((inb(APPLESMC_CMD_PORT) & APPLESMC_STATUS_MASK) == val)
>  			return 0;
> -		}
>  	}
>  
>  	return -EIO;
> @@ -431,7 +430,7 @@ static int applesmc_has_key(const char *key, bool *value)
>  /*
>   * applesmc_read_motion_sensor - Read motion sensor (X, Y or Z).
>   */
> -static int applesmc_read_motion_sensor(int index, s16* value)
> +static int applesmc_read_motion_sensor(int index, s16 *value)
>  {
>  	u8 buffer[2];
>  	int ret;
> @@ -779,14 +778,12 @@ static ssize_t applesmc_store_fan_speed(struct device *dev,
>  					const char *sysfsbuf, size_t count)
>  {
>  	int ret;
> -	u32 speed;
> +	unsigned long speed;
>  	char newkey[5];
>  	u8 buffer[2];
>  
> -	speed = simple_strtoul(sysfsbuf, NULL, 10);
> -
> -	if (speed > 0x4000) /* Bigger than a 14-bit value */
> -		return -EINVAL;
> +	if (strict_strtoul(sysfsbuf, 10, &speed) < 0 || speed >= 0x4000)
> +		return -EINVAL;		/* Bigger than a 14-bit value */


Thanks for the bug fix, I forgot to include it.

>
>  	sprintf(newkey, fan_speed_fmt[to_option(attr)], to_index(attr));
>  
> @@ -822,10 +819,11 @@ static ssize_t applesmc_store_fan_manual(struct device *dev,
>  {
>  	int ret;
>  	u8 buffer[2];
> -	u32 input;
> +	unsigned long input;
>  	u16 val;
>  
> -	input = simple_strtoul(sysfsbuf, NULL, 10);
> +	if (strict_strtoul(sysfsbuf, 10, &input) < 0)
> +		return -EINVAL;
>  
>  	ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
>  	val = (buffer[0] << 8 | buffer[1]);
> @@ -977,7 +975,11 @@ static ssize_t applesmc_key_at_index_show(struct device *dev,
>  static ssize_t applesmc_key_at_index_store(struct device *dev,
>  	struct device_attribute *attr, const char *sysfsbuf, size_t count)
>  {
> -	key_at_index = simple_strtoul(sysfsbuf, NULL, 10);
> +	unsigned long newkey;
> +
> +	if (strict_strtoul(sysfsbuf, 10, &newkey) < 0)
> +		return -EINVAL;
> +	key_at_index = newkey;


Crash alert - key_at_index is not range checked, and the remake uses this value
as an array index...

>  	return count;
>  }
> @@ -1193,24 +1195,24 @@ static __initdata struct dmi_system_id applesmc_whitelist[] = {
>  	  DMI_MATCH(DMI_PRODUCT_NAME, "MacBookAir") },
>  	},
>  	{ applesmc_dmi_match, "Apple MacBook Pro", {
> -	  DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
> -	  DMI_MATCH(DMI_PRODUCT_NAME,"MacBookPro") },
> +	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
> +	  DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro") },
>  	},
>  	{ applesmc_dmi_match, "Apple MacBook", {
> -	  DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
> -	  DMI_MATCH(DMI_PRODUCT_NAME,"MacBook") },
> +	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
> +	  DMI_MATCH(DMI_PRODUCT_NAME, "MacBook") },
>  	},
>  	{ applesmc_dmi_match, "Apple Macmini", {
> -	  DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
> -	  DMI_MATCH(DMI_PRODUCT_NAME,"Macmini") },
> +	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
> +	  DMI_MATCH(DMI_PRODUCT_NAME, "Macmini") },
>  	},
>  	{ applesmc_dmi_match, "Apple MacPro", {
>  	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
>  	  DMI_MATCH(DMI_PRODUCT_NAME, "MacPro") },
>  	},
>  	{ applesmc_dmi_match, "Apple iMac", {
> -	  DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
> -	  DMI_MATCH(DMI_PRODUCT_NAME,"iMac") },
> +	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
> +	  DMI_MATCH(DMI_PRODUCT_NAME, "iMac") },
>  	},
>  	{ .ident = NULL }
>  };


Thanks,
Henrik

[PATCH] hwmon: (applesmc) Fix checkpatch errors

[Guenter Roeck]

Fix all checkpatch errors and most of the warnings.

Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
---
 drivers/hwmon/applesmc.c |   40 +++++++++++++++++++++-------------------
 1 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 1e49b30..365761b 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -174,9 +174,8 @@ static int __wait_status(u8 val)
 
 	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
 		udelay(us);
-		if ((inb(APPLESMC_CMD_PORT) & APPLESMC_STATUS_MASK) == val) {
+		if ((inb(APPLESMC_CMD_PORT) & APPLESMC_STATUS_MASK) == val)
 			return 0;
-		}
 	}
 
 	return -EIO;
@@ -431,7 +430,7 @@ static int applesmc_has_key(const char *key, bool *value)
 /*
  * applesmc_read_motion_sensor - Read motion sensor (X, Y or Z).
  */
-static int applesmc_read_motion_sensor(int index, s16* value)
+static int applesmc_read_motion_sensor(int index, s16 *value)
 {
 	u8 buffer[2];
 	int ret;
@@ -779,14 +778,12 @@ static ssize_t applesmc_store_fan_speed(struct device *dev,
 					const char *sysfsbuf, size_t count)
 {
 	int ret;
-	u32 speed;
+	unsigned long speed;
 	char newkey[5];
 	u8 buffer[2];
 
-	speed = simple_strtoul(sysfsbuf, NULL, 10);
-
-	if (speed > 0x4000) /* Bigger than a 14-bit value */
-		return -EINVAL;
+	if (strict_strtoul(sysfsbuf, 10, &speed) < 0 || speed >= 0x4000)
+		return -EINVAL;		/* Bigger than a 14-bit value */
 
 	sprintf(newkey, fan_speed_fmt[to_option(attr)], to_index(attr));
 
@@ -822,10 +819,11 @@ static ssize_t applesmc_store_fan_manual(struct device *dev,
 {
 	int ret;
 	u8 buffer[2];
-	u32 input;
+	unsigned long input;
 	u16 val;
 
-	input = simple_strtoul(sysfsbuf, NULL, 10);
+	if (strict_strtoul(sysfsbuf, 10, &input) < 0)
+		return -EINVAL;
 
 	ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
 	val = (buffer[0] << 8 | buffer[1]);
@@ -977,7 +975,11 @@ static ssize_t applesmc_key_at_index_show(struct device *dev,
 static ssize_t applesmc_key_at_index_store(struct device *dev,
 	struct device_attribute *attr, const char *sysfsbuf, size_t count)
 {
-	key_at_index = simple_strtoul(sysfsbuf, NULL, 10);
+	unsigned long newkey;
+
+	if (strict_strtoul(sysfsbuf, 10, &newkey) < 0)
+		return -EINVAL;
+	key_at_index = newkey;
 
 	return count;
 }
@@ -1193,24 +1195,24 @@ static __initdata struct dmi_system_id applesmc_whitelist[] = {
 	  DMI_MATCH(DMI_PRODUCT_NAME, "MacBookAir") },
 	},
 	{ applesmc_dmi_match, "Apple MacBook Pro", {
-	  DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME,"MacBookPro") },
+	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
+	  DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro") },
 	},
 	{ applesmc_dmi_match, "Apple MacBook", {
-	  DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME,"MacBook") },
+	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
+	  DMI_MATCH(DMI_PRODUCT_NAME, "MacBook") },
 	},
 	{ applesmc_dmi_match, "Apple Macmini", {
-	  DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME,"Macmini") },
+	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
+	  DMI_MATCH(DMI_PRODUCT_NAME, "Macmini") },
 	},
 	{ applesmc_dmi_match, "Apple MacPro", {
 	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
 	  DMI_MATCH(DMI_PRODUCT_NAME, "MacPro") },
 	},
 	{ applesmc_dmi_match, "Apple iMac", {
-	  DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME,"iMac") },
+	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
+	  DMI_MATCH(DMI_PRODUCT_NAME, "iMac") },
 	},
 	{ .ident = NULL }
 };
-- 
1.7.3.1

Re: [PATCH] hwmon: (applesmc) Fix checkpatch errors

[Guenter Roeck]

On Wed, Nov 10, 2010 at 01:52:07PM -0500, Henrik Rydberg wrote:
> On 11/10/2010 07:53 PM, Guenter Roeck wrote:
> 
> > Fix all checkpatch errors and most of the warnings.
> > 
> > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > ---
> >  drivers/hwmon/applesmc.c |   40 +++++++++++++++++++++-------------------
> >  1 files changed, 21 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> > index 1e49b30..365761b 100644
> > --- a/drivers/hwmon/applesmc.c
> > +++ b/drivers/hwmon/applesmc.c
> > @@ -174,9 +174,8 @@ static int __wait_status(u8 val)
> >  
> >  	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
> >  		udelay(us);
> > -		if ((inb(APPLESMC_CMD_PORT) & APPLESMC_STATUS_MASK) == val) {
> > +		if ((inb(APPLESMC_CMD_PORT) & APPLESMC_STATUS_MASK) == val)
> >  			return 0;
> > -		}
> >  	}
> >  
> >  	return -EIO;
> > @@ -431,7 +430,7 @@ static int applesmc_has_key(const char *key, bool *value)
> >  /*
> >   * applesmc_read_motion_sensor - Read motion sensor (X, Y or Z).
> >   */
> > -static int applesmc_read_motion_sensor(int index, s16* value)
> > +static int applesmc_read_motion_sensor(int index, s16 *value)
> >  {
> >  	u8 buffer[2];
> >  	int ret;
> > @@ -779,14 +778,12 @@ static ssize_t applesmc_store_fan_speed(struct device *dev,
> >  					const char *sysfsbuf, size_t count)
> >  {
> >  	int ret;
> > -	u32 speed;
> > +	unsigned long speed;
> >  	char newkey[5];
> >  	u8 buffer[2];
> >  
> > -	speed = simple_strtoul(sysfsbuf, NULL, 10);
> > -
> > -	if (speed > 0x4000) /* Bigger than a 14-bit value */
> > -		return -EINVAL;
> > +	if (strict_strtoul(sysfsbuf, 10, &speed) < 0 || speed >= 0x4000)
> > +		return -EINVAL;		/* Bigger than a 14-bit value */
> 
> 
> Thanks for the bug fix, I forgot to include it.
> 
> >
> >  	sprintf(newkey, fan_speed_fmt[to_option(attr)], to_index(attr));
> >  
> > @@ -822,10 +819,11 @@ static ssize_t applesmc_store_fan_manual(struct device *dev,
> >  {
> >  	int ret;
> >  	u8 buffer[2];
> > -	u32 input;
> > +	unsigned long input;
> >  	u16 val;
> >  
> > -	input = simple_strtoul(sysfsbuf, NULL, 10);
> > +	if (strict_strtoul(sysfsbuf, 10, &input) < 0)
> > +		return -EINVAL;
> >  
> >  	ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
> >  	val = (buffer[0] << 8 | buffer[1]);
> > @@ -977,7 +975,11 @@ static ssize_t applesmc_key_at_index_show(struct device *dev,
> >  static ssize_t applesmc_key_at_index_store(struct device *dev,
> >  	struct device_attribute *attr, const char *sysfsbuf, size_t count)
> >  {
> > -	key_at_index = simple_strtoul(sysfsbuf, NULL, 10);
> > +	unsigned long newkey;
> > +
> > +	if (strict_strtoul(sysfsbuf, 10, &newkey) < 0)
> > +		return -EINVAL;
> > +	key_at_index = newkey;
> 
> 
> Crash alert - key_at_index is not range checked, and the remake uses this value
> as an array index...
> 
Good that I made this change ;). I'll add the check and re-send.

This points to another problem, though. You allocate key_count entries,
ie cache[0]..cache[key_count-1]. Yet, the key searches are from 0..key_count,
ie span key_count+1 entries. Is that another problem ?

Seems to me you would either have to allocate key_count+1 entries, or terminate
the search at key_count - 1. Not sure which one would be correct. Let me know,
and I'll update the affected patch(es).

Thanks,
Guenter

Re: [PATCH] hwmon: (applesmc) Fix checkpatch errors

[Henrik Rydberg]

>>> @@ -977,7 +975,11 @@ static ssize_t applesmc_key_at_index_show

>>>  static ssize_t applesmc_key_at_index_store(struct device *dev,
>>>  	struct device_attribute *attr, const char *sysfsbuf, size_t count)
>>>  {
>>> -	key_at_index = simple_strtoul(sysfsbuf, NULL, 10);
>>> +	unsigned long newkey;
>>> +
>>> +	if (strict_strtoul(sysfsbuf, 10, &newkey) < 0)
>>> +		return -EINVAL;
>>> +	key_at_index = newkey;
>>
>>
>> Crash alert - key_at_index is not range checked, and the remake uses this value
>> as an array index...
>>
> Good that I made this change ;). I'll add the check and re-send.


Indeed! The downside of remakes... sorry about that. :-)

I guess the change should go into patch 4 already? There is also the option to
put the bounds check in applesmc_get_entry_by_index, but I like the simplicity
of "|| newkey >= smcreg.key_count".

> This points to another problem, though. You allocate key_count entries,
> ie cache[0]..cache[key_count-1]. Yet, the key searches are from 0..key_count,
> ie span key_count+1 entries. Is that another problem ?

>

> Seems to me you would either have to allocate key_count+1 entries, or terminate
> the search at key_count - 1. Not sure which one would be correct. Let me know,
> and I'll update the affected patch(es).


If you are referring to the lower and upper bound functions, those use the
one-past-the-last-element convention, so it is actually still 0..key_count - 1.
I stayed very close to the stl reference implementation, which relies on the
fact that when begin != end, (begin + (end - begin) / 2) < end.

Cheers,
Henrik

Re: [PATCH] hwmon: (applesmc) Fix checkpatch errors

[Guenter Roeck]

On Wed, Nov 10, 2010 at 02:56:20PM -0500, Henrik Rydberg wrote:
> >>> @@ -977,7 +975,11 @@ static ssize_t applesmc_key_at_index_show
> 
> >>>  static ssize_t applesmc_key_at_index_store(struct device *dev,
> >>>  	struct device_attribute *attr, const char *sysfsbuf, size_t count)
> >>>  {
> >>> -	key_at_index = simple_strtoul(sysfsbuf, NULL, 10);
> >>> +	unsigned long newkey;
> >>> +
> >>> +	if (strict_strtoul(sysfsbuf, 10, &newkey) < 0)
> >>> +		return -EINVAL;
> >>> +	key_at_index = newkey;
> >>
> >>
> >> Crash alert - key_at_index is not range checked, and the remake uses this value
> >> as an array index...
> >>
> > Good that I made this change ;). I'll add the check and re-send.
> 
> 
> Indeed! The downside of remakes... sorry about that. :-)
> 
Happens.

> I guess the change should go into patch 4 already? There is also the option to
> put the bounds check in applesmc_get_entry_by_index, but I like the simplicity
> of "|| newkey >= smcreg.key_count".
> 
I don't like the idea of putting the check into applesmc_get_entry_by_index().
Let me see if I can merge it into patch #4. If not, I'll keep it in my patch
for simplicity.

> > This points to another problem, though. You allocate key_count entries,
> > ie cache[0]..cache[key_count-1]. Yet, the key searches are from 0..key_count,
> > ie span key_count+1 entries. Is that another problem ?
> 
> >
> 
> > Seems to me you would either have to allocate key_count+1 entries, or terminate
> > the search at key_count - 1. Not sure which one would be correct. Let me know,
> > and I'll update the affected patch(es).
> 
> 
> If you are referring to the lower and upper bound functions, those use the
> one-past-the-last-element convention, so it is actually still 0..key_count - 1.
> I stayed very close to the stl reference implementation, which relies on the
> fact that when begin != end, (begin + (end - begin) / 2) < end.
> 
Ok, you are right - I was concerned about applesmc_get_upper_bound() returning
smcreg.key_count, but that is ok, since that value is never used to actually
retrieve a key.

Thanks,
Guenter