[patch] mfd: 64 bit bug in bxtwc_val_store()

[patch] mfd: 64 bit bug in bxtwc_val_store()

[Dan Carpenter]

The call to kstrtoul() will corrupt memory on 64 bit systems because an
int is 4 bytes and a long is 8.

Also it's not a good idea to let users trigger a dev_err() because it
just ends up flooding /var/log/messages so I removed the printk.

Fixes: 2ddd2086ea9c ('mfd: add Intel Broxton Whiskey Cove PMIC driver')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/mfd/intel_soc_pmic_bxtwc.c b/drivers/mfd/intel_soc_pmic_bxtwc.c
index 40acaff..b942876 100644
--- a/drivers/mfd/intel_soc_pmic_bxtwc.c
+++ b/drivers/mfd/intel_soc_pmic_bxtwc.c
@@ -297,10 +297,9 @@ static ssize_t bxtwc_val_store(struct device *dev,
 	unsigned int val;
 	struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
 
-	if (kstrtoul(buf, 0, (unsigned long *)&val)) {
-		dev_err(dev, "Invalid register value\n");
-		return -EINVAL;
-	}
+	ret = kstrtouint(buf, 0, &val);
+	if (ret)
+		return ret;
 
 	ret = regmap_write(pmic->regmap, bxtwc_reg_addr, val);
 	if (ret) {

Re: [patch] mfd: 64 bit bug in bxtwc_val_store()

[Lee Jones]

I will fix the subject line for this patch, but in future please use
the format akin to the individual subsystem.

In the MFD case this is:

  mfd: <device>: <Summary starting with an uppercase character>

> The call to kstrtoul() will corrupt memory on 64 bit systems because an
> int is 4 bytes and a long is 8.
> 
> Also it's not a good idea to let users trigger a dev_err() because it
> just ends up flooding /var/log/messages so I removed the printk.
> 
> Fixes: 2ddd2086ea9c ('mfd: add Intel Broxton Whiskey Cove PMIC driver')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Applied, thanks.

> diff --git a/drivers/mfd/intel_soc_pmic_bxtwc.c b/drivers/mfd/intel_soc_pmic_bxtwc.c
> index 40acaff..b942876 100644
> --- a/drivers/mfd/intel_soc_pmic_bxtwc.c
> +++ b/drivers/mfd/intel_soc_pmic_bxtwc.c
> @@ -297,10 +297,9 @@ static ssize_t bxtwc_val_store(struct device *dev,
>  	unsigned int val;
>  	struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
>  
> -	if (kstrtoul(buf, 0, (unsigned long *)&val)) {
> -		dev_err(dev, "Invalid register value\n");
> -		return -EINVAL;
> -	}
> +	ret = kstrtouint(buf, 0, &val);
> +	if (ret)
> +		return ret;
>  
>  	ret = regmap_write(pmic->regmap, bxtwc_reg_addr, val);
>  	if (ret) {

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] mfd: 64 bit bug in bxtwc_val_store()

[Dan Carpenter]

On Thu, Oct 01, 2015 at 12:51:00PM +0100, Lee Jones wrote:
> I will fix the subject line for this patch, but in future please use
> the format akin to the individual subsystem.
> 
> In the MFD case this is:
> 
>   mfd: <device>: <Summary starting with an uppercase character>
> 

I looked at the subjects to every other patch to that file and I
followed the same pattern.

$ git log --oneline drivers/mfd/intel_soc_pmic_bxtwc.c
b412836 mfd: add Intel Broxton Whiskey Cove PMIC driver
$

regards,
dan carpenter

Re: [patch] mfd: 64 bit bug in bxtwc_val_store()

[Dan Carpenter]

On Thu, Oct 01, 2015 at 06:13:31PM +0300, Dan Carpenter wrote:
> I looked at the subjects to every other patch to that file and I
> followed the same pattern.

This is literally a part of my kpatch.sh script:

git log --oneline $fullname | head -n 10
echo "Copy and paste one of these subjects?"
read unused

regards,
dan carpenter

Re: [patch] mfd: 64 bit bug in bxtwc_val_store()

[Lee Jones]

On Thu, 01 Oct 2015, Dan Carpenter wrote:
> On Thu, Oct 01, 2015 at 06:13:31PM +0300, Dan Carpenter wrote:
> > I looked at the subjects to every other patch to that file and I
> > followed the same pattern.
> 
> This is literally a part of my kpatch.sh script:
> 
> git log --oneline $fullname | head -n 10
> echo "Copy and paste one of these subjects?"
> read unused

Okay, this is good.  Unfortunately I guess process over-ruled common
sense on this occasion (meant more of a joke than offensively). ;)

When new drivers are sent, the patch format is more likely to be:

  mfd: "Adding new driver for ..."

As there is no <device> to mention.  However, as yours is an add-on
patch which makes changes to an existing driver, a better subject
line would have been:

  mfd: <device>: "I'm doing this"

... but I completely understand the confusion.  Not much to change
then, keep doing what you're doing.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] mfd: 64 bit bug in bxtwc_val_store()

[Dan Carpenter]

I'm often the first person to touch a new patch after the author so this
is a conversation I have had before.  I "persuaded" the drivers/media
people and now they write patch descriptions like:

85756a0 [media] cobalt: add new driver

regards,
dan carpenter