This patch replaces the code for getting an unsigned long from a
userspace buffer by a simple call to kstroul_from_user.
This makes it easier to read and less error prone.
Kernel Version: v3.0-rc2
Signed-off-by: Peter Huewe <[email protected]>
---
drivers/mfd/ab3550-core.c | 41 +++++++++++------------------------------
1 files changed, 11 insertions(+), 30 deletions(-)
diff --git a/drivers/mfd/ab3550-core.c b/drivers/mfd/ab3550-core.c
index 3d7dce6..56ba194 100644
--- a/drivers/mfd/ab3550-core.c
+++ b/drivers/mfd/ab3550-core.c
@@ -879,20 +879,13 @@ static ssize_t ab3550_bank_write(struct file *file,
size_t count, loff_t *ppos)
{
struct ab3550 *ab = ((struct seq_file *)(file->private_data))->private;
- char buf[32];
- int buf_size;
unsigned long user_bank;
int err;
/* Get userspace string and assure termination */
- buf_size = min(count, (sizeof(buf) - 1));
- if (copy_from_user(buf, user_buf, buf_size))
- return -EFAULT;
- buf[buf_size] = 0;
-
- err = strict_strtoul(buf, 0, &user_bank);
+ err = kstrtoul_from_user(user_buf, count, 0, &user_bank);
if (err)
- return -EINVAL;
+ return err;
if (user_bank >= AB3550_NUM_BANKS) {
dev_err(&ab->i2c_client[0]->dev,
@@ -902,7 +895,7 @@ static ssize_t ab3550_bank_write(struct file *file,
ab->debug_bank = user_bank;
- return buf_size;
+ return count;
}
static int ab3550_address_print(struct seq_file *s, void *p)
@@ -923,27 +916,21 @@ static ssize_t ab3550_address_write(struct file *file,
size_t count, loff_t *ppos)
{
struct ab3550 *ab = ((struct seq_file *)(file->private_data))->private;
- char buf[32];
- int buf_size;
unsigned long user_address;
int err;
/* Get userspace string and assure termination */
- buf_size = min(count, (sizeof(buf) - 1));
- if (copy_from_user(buf, user_buf, buf_size))
- return -EFAULT;
- buf[buf_size] = 0;
-
- err = strict_strtoul(buf, 0, &user_address);
+ err = kstrtoul_from_user(user_buf, count, 0, &user_address);
if (err)
- return -EINVAL;
+ return err;
+
if (user_address > 0xff) {
dev_err(&ab->i2c_client[0]->dev,
"debugfs error input > 0xff\n");
return -EINVAL;
}
ab->debug_address = user_address;
- return buf_size;
+ return count;
}
static int ab3550_val_print(struct seq_file *s, void *p)
@@ -971,21 +958,15 @@ static ssize_t ab3550_val_write(struct file *file,
size_t count, loff_t *ppos)
{
struct ab3550 *ab = ((struct seq_file *)(file->private_data))->private;
- char buf[32];
- int buf_size;
unsigned long user_val;
int err;
u8 regvalue;
/* Get userspace string and assure termination */
- buf_size = min(count, (sizeof(buf)-1));
- if (copy_from_user(buf, user_buf, buf_size))
- return -EFAULT;
- buf[buf_size] = 0;
-
- err = strict_strtoul(buf, 0, &user_val);
+ err = kstrtoul_from_user(user_buf, count, 0, &user_val);
if (err)
- return -EINVAL;
+ return err;
+
if (user_val > 0xff) {
dev_err(&ab->i2c_client[0]->dev,
"debugfs error input > 0xff\n");
@@ -1002,7 +983,7 @@ static ssize_t ab3550_val_write(struct file *file,
if (err)
return -EINVAL;
- return buf_size;
+ return count;
}
static const struct file_operations ab3550_bank_fops = {
--
1.7.3.4
This patch replaces the code for getting an unsigned long from a
userspace buffer by a simple call to kstroul_from_user.
This makes it easier to read and less error prone.
Kernel Version: v3.0-rc2
Signed-off-by: Peter Huewe <[email protected]>
---
drivers/mfd/ab8500-debugfs.c | 41 +++++++++++------------------------------
1 files changed, 11 insertions(+), 30 deletions(-)
diff --git a/drivers/mfd/ab8500-debugfs.c b/drivers/mfd/ab8500-debugfs.c
index 64748e4..64bdeeb 100644
--- a/drivers/mfd/ab8500-debugfs.c
+++ b/drivers/mfd/ab8500-debugfs.c
@@ -419,20 +419,13 @@ static ssize_t ab8500_bank_write(struct file *file,
size_t count, loff_t *ppos)
{
struct device *dev = ((struct seq_file *)(file->private_data))->private;
- char buf[32];
- int buf_size;
unsigned long user_bank;
int err;
/* Get userspace string and assure termination */
- buf_size = min(count, (sizeof(buf) - 1));
- if (copy_from_user(buf, user_buf, buf_size))
- return -EFAULT;
- buf[buf_size] = 0;
-
- err = strict_strtoul(buf, 0, &user_bank);
+ err = kstrtoul_from_user(user_buf, count, 0, &user_bank);
if (err)
- return -EINVAL;
+ return err;
if (user_bank >= AB8500_NUM_BANKS) {
dev_err(dev, "debugfs error input > number of banks\n");
@@ -441,7 +434,7 @@ static ssize_t ab8500_bank_write(struct file *file,
debug_bank = user_bank;
- return buf_size;
+ return count;
}
static int ab8500_address_print(struct seq_file *s, void *p)
@@ -459,26 +452,20 @@ static ssize_t ab8500_address_write(struct file *file,
size_t count, loff_t *ppos)
{
struct device *dev = ((struct seq_file *)(file->private_data))->private;
- char buf[32];
- int buf_size;
unsigned long user_address;
int err;
/* Get userspace string and assure termination */
- buf_size = min(count, (sizeof(buf) - 1));
- if (copy_from_user(buf, user_buf, buf_size))
- return -EFAULT;
- buf[buf_size] = 0;
-
- err = strict_strtoul(buf, 0, &user_address);
+ err = kstrtoul_from_user(user_buf, count, 0, &user_address);
if (err)
- return -EINVAL;
+ return err;
+
if (user_address > 0xff) {
dev_err(dev, "debugfs error input > 0xff\n");
return -EINVAL;
}
debug_address = user_address;
- return buf_size;
+ return count;
}
static int ab8500_val_print(struct seq_file *s, void *p)
@@ -509,20 +496,14 @@ static ssize_t ab8500_val_write(struct file *file,
size_t count, loff_t *ppos)
{
struct device *dev = ((struct seq_file *)(file->private_data))->private;
- char buf[32];
- int buf_size;
unsigned long user_val;
int err;
/* Get userspace string and assure termination */
- buf_size = min(count, (sizeof(buf)-1));
- if (copy_from_user(buf, user_buf, buf_size))
- return -EFAULT;
- buf[buf_size] = 0;
-
- err = strict_strtoul(buf, 0, &user_val);
+ err = kstrtoul_from_user(user_buf, count, 0, &user_val);
if (err)
- return -EINVAL;
+ return err;
+
if (user_val > 0xff) {
dev_err(dev, "debugfs error input > 0xff\n");
return -EINVAL;
@@ -534,7 +515,7 @@ static ssize_t ab8500_val_write(struct file *file,
return -EINVAL;
}
- return buf_size;
+ return count;
}
static const struct file_operations ab8500_bank_fops = {
--
1.7.3.4
Hi Peter,
On Mon, Jun 06, 2011 at 10:43:31PM +0200, Peter Huewe wrote:
> This patch replaces the code for getting an unsigned long from a
> userspace buffer by a simple call to kstroul_from_user.
> This makes it easier to read and less error prone.
Thanks, both patches applied.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
NAK
You should use kstrtou8_from_user() and drop 0xff check as well.
Do NOT blindly replace strict_strtoul with kstrtoul.
On Mon, Jun 6, 2011 at 11:43 PM, Peter Huewe <[email protected]> wrote:
> - ? ? ? err = strict_strtoul(buf, 0, &user_val);
> + ? ? ? err = kstrtoul_from_user(user_buf, count, 0, &user_val);
> ? ? ? ?if (err)
> - ? ? ? ? ? ? ? return -EINVAL;
> + ? ? ? ? ? ? ? return err;
> +
> ? ? ? ?if (user_val > 0xff) {
> ? ? ? ? ? ? ? ?dev_err(dev, "debugfs error input > 0xff\n");
> ? ? ? ? ? ? ? ?return -EINVAL;
Am Montag 20 Juni 2011, 15:45:46 schrieb Alexey Dobriyan:
> NAK
> You should use kstrtou8_from_user() and drop 0xff check as well.
>
> Do NOT blindly replace strict_strtoul with kstrtoul.
Thanks for spotting this - I'll review my other patches as well and redo them.
Sorry for any inconvenience.
Thanks,
Peter
This patch replaces the code for getting an number from a
userspace buffer by a simple call to kstrou8_from_user.
This makes it easier to read and less error prone.
Since the old buffers held only values up to 255, we don't need
kstrtoul, but rather kstrtou8.
Kernel Version: v3.0-rc3
Changes in v2:
- Use kstrtou8 instead of kstrtoul due to small numbers
- Dropped then unnecessary checks
(Both remarks from Alexey Dobriyan, Thanks!)
Signed-off-by: Peter Huewe <[email protected]>
---
drivers/mfd/ab3550-core.c | 63 ++++++++++++--------------------------------
1 files changed, 17 insertions(+), 46 deletions(-)
diff --git a/drivers/mfd/ab3550-core.c b/drivers/mfd/ab3550-core.c
index 3d7dce6..a7370ba 100644
--- a/drivers/mfd/ab3550-core.c
+++ b/drivers/mfd/ab3550-core.c
@@ -879,20 +879,13 @@ static ssize_t ab3550_bank_write(struct file *file,
size_t count, loff_t *ppos)
{
struct ab3550 *ab = ((struct seq_file *)(file->private_data))->private;
- char buf[32];
- int buf_size;
- unsigned long user_bank;
+ u8 user_bank;
int err;
- /* Get userspace string and assure termination */
- buf_size = min(count, (sizeof(buf) - 1));
- if (copy_from_user(buf, user_buf, buf_size))
- return -EFAULT;
- buf[buf_size] = 0;
-
- err = strict_strtoul(buf, 0, &user_bank);
+ /* Get userspace string and convert to number */
+ err = kstrtou8_from_user(user_buf, count, 0, &user_bank);
if (err)
- return -EINVAL;
+ return err;
if (user_bank >= AB3550_NUM_BANKS) {
dev_err(&ab->i2c_client[0]->dev,
@@ -902,7 +895,7 @@ static ssize_t ab3550_bank_write(struct file *file,
ab->debug_bank = user_bank;
- return buf_size;
+ return count;
}
static int ab3550_address_print(struct seq_file *s, void *p)
@@ -923,27 +916,16 @@ static ssize_t ab3550_address_write(struct file *file,
size_t count, loff_t *ppos)
{
struct ab3550 *ab = ((struct seq_file *)(file->private_data))->private;
- char buf[32];
- int buf_size;
- unsigned long user_address;
+ u8 user_address;
int err;
- /* Get userspace string and assure termination */
- buf_size = min(count, (sizeof(buf) - 1));
- if (copy_from_user(buf, user_buf, buf_size))
- return -EFAULT;
- buf[buf_size] = 0;
-
- err = strict_strtoul(buf, 0, &user_address);
+ /* Get userspace string and convert to number */
+ err = kstrtou8_from_user(user_buf, count, 0, &user_address);
if (err)
- return -EINVAL;
- if (user_address > 0xff) {
- dev_err(&ab->i2c_client[0]->dev,
- "debugfs error input > 0xff\n");
- return -EINVAL;
- }
+ return err;
+
ab->debug_address = user_address;
- return buf_size;
+ return count;
}
static int ab3550_val_print(struct seq_file *s, void *p)
@@ -971,26 +953,15 @@ static ssize_t ab3550_val_write(struct file *file,
size_t count, loff_t *ppos)
{
struct ab3550 *ab = ((struct seq_file *)(file->private_data))->private;
- char buf[32];
- int buf_size;
- unsigned long user_val;
+ u8 user_val;
int err;
u8 regvalue;
- /* Get userspace string and assure termination */
- buf_size = min(count, (sizeof(buf)-1));
- if (copy_from_user(buf, user_buf, buf_size))
- return -EFAULT;
- buf[buf_size] = 0;
-
- err = strict_strtoul(buf, 0, &user_val);
+ /* Get userspace string and convert to number */
+ err = kstrtou8_from_user(user_buf, count, 0, &user_val);
if (err)
- return -EINVAL;
- if (user_val > 0xff) {
- dev_err(&ab->i2c_client[0]->dev,
- "debugfs error input > 0xff\n");
- return -EINVAL;
- }
+ return err;
+
err = mask_and_set_register_interruptible(
ab, (u8)ab->debug_bank,
(u8)ab->debug_address, 0xFF, (u8)user_val);
@@ -1002,7 +973,7 @@ static ssize_t ab3550_val_write(struct file *file,
if (err)
return -EINVAL;
- return buf_size;
+ return count;
}
static const struct file_operations ab3550_bank_fops = {
--
1.7.3.4
This patch replaces the code for getting an number from a
userspace buffer by a simple call to kstrou8_from_user.
This makes it easier to read and less error prone.
This patch replaces the code for getting an number from a
userspace buffer by a simple call to kstrou8_from_user.
This makes it easier to read and less error prone.
Since the old buffers held only values up to 255, we don't need
kstrtoul, but rather kstrtou8.
Kernel Version: v3.0-rc3
Changes in v2:
- Use kstrtou8 instead of kstrtoul due to small numbers
- Dropped then unnecessary checks
(Both remarks from Alexey Dobriyan, Thanks!)
Signed-off-by: Peter Huewe <[email protected]>
---
drivers/mfd/ab8500-debugfs.c | 61 +++++++++++------------------------------
1 files changed, 17 insertions(+), 44 deletions(-)
diff --git a/drivers/mfd/ab8500-debugfs.c b/drivers/mfd/ab8500-debugfs.c
index 64748e4..7304919 100644
--- a/drivers/mfd/ab8500-debugfs.c
+++ b/drivers/mfd/ab8500-debugfs.c
@@ -419,20 +419,13 @@ static ssize_t ab8500_bank_write(struct file *file,
size_t count, loff_t *ppos)
{
struct device *dev = ((struct seq_file *)(file->private_data))->private;
- char buf[32];
- int buf_size;
- unsigned long user_bank;
+ u8 user_bank;
int err;
- /* Get userspace string and assure termination */
- buf_size = min(count, (sizeof(buf) - 1));
- if (copy_from_user(buf, user_buf, buf_size))
- return -EFAULT;
- buf[buf_size] = 0;
-
- err = strict_strtoul(buf, 0, &user_bank);
+ /* Get userspace string and convert to number */
+ err = kstrtou8_from_user(user_buf, count, 0, &user_bank);
if (err)
- return -EINVAL;
+ return err;
if (user_bank >= AB8500_NUM_BANKS) {
dev_err(dev, "debugfs error input > number of banks\n");
@@ -441,7 +434,7 @@ static ssize_t ab8500_bank_write(struct file *file,
debug_bank = user_bank;
- return buf_size;
+ return count;
}
static int ab8500_address_print(struct seq_file *s, void *p)
@@ -459,26 +452,16 @@ static ssize_t ab8500_address_write(struct file *file,
size_t count, loff_t *ppos)
{
struct device *dev = ((struct seq_file *)(file->private_data))->private;
- char buf[32];
- int buf_size;
- unsigned long user_address;
+ u8 user_address;
int err;
- /* Get userspace string and assure termination */
- buf_size = min(count, (sizeof(buf) - 1));
- if (copy_from_user(buf, user_buf, buf_size))
- return -EFAULT;
- buf[buf_size] = 0;
-
- err = strict_strtoul(buf, 0, &user_address);
+ /* Get userspace string and convert to number*/
+ err = kstrtou8_from_user(user_buf, count, 0, &user_address);
if (err)
- return -EINVAL;
- if (user_address > 0xff) {
- dev_err(dev, "debugfs error input > 0xff\n");
- return -EINVAL;
- }
+ return err;
+
debug_address = user_address;
- return buf_size;
+ return count;
}
static int ab8500_val_print(struct seq_file *s, void *p)
@@ -509,24 +492,14 @@ static ssize_t ab8500_val_write(struct file *file,
size_t count, loff_t *ppos)
{
struct device *dev = ((struct seq_file *)(file->private_data))->private;
- char buf[32];
- int buf_size;
- unsigned long user_val;
+ u8 user_val;
int err;
- /* Get userspace string and assure termination */
- buf_size = min(count, (sizeof(buf)-1));
- if (copy_from_user(buf, user_buf, buf_size))
- return -EFAULT;
- buf[buf_size] = 0;
-
- err = strict_strtoul(buf, 0, &user_val);
+ /* Get userspace string and convert to number */
+ err = kstrtou8_from_user(user_buf, count, 0, &user_val);
if (err)
- return -EINVAL;
- if (user_val > 0xff) {
- dev_err(dev, "debugfs error input > 0xff\n");
- return -EINVAL;
- }
+ return err;
+
err = abx500_set_register_interruptible(dev,
(u8)debug_bank, debug_address, (u8)user_val);
if (err < 0) {
@@ -534,7 +507,7 @@ static ssize_t ab8500_val_write(struct file *file,
return -EINVAL;
}
- return buf_size;
+ return count;
}
static const struct file_operations ab8500_bank_fops = {
--
1.7.3.4
On Mon, Jun 20, 2011 at 09:46:28PM +0200, Peter Huewe wrote:
> @@ -923,27 +916,16 @@ static ssize_t ab3550_address_write(struct file *file,
> size_t count, loff_t *ppos)
> {
> struct ab3550 *ab = ((struct seq_file *)(file->private_data))->private;
> - char buf[32];
> - int buf_size;
> - unsigned long user_address;
> + u8 user_address;
> int err;
>
> - /* Get userspace string and assure termination */
> - buf_size = min(count, (sizeof(buf) - 1));
> - if (copy_from_user(buf, user_buf, buf_size))
> - return -EFAULT;
> - buf[buf_size] = 0;
> -
> - err = strict_strtoul(buf, 0, &user_address);
> + /* Get userspace string and convert to number */
> + err = kstrtou8_from_user(user_buf, count, 0, &user_address);
> if (err)
> - return -EINVAL;
> - if (user_address > 0xff) {
> - dev_err(&ab->i2c_client[0]->dev,
> - "debugfs error input > 0xff\n");
> - return -EINVAL;
> - }
> + return err;
> +
> ab->debug_address = user_address;
> - return buf_size;
> + return count;
You don't need temporary variable and should write straight to final location,
because kstrto* functions will never write to result unless it was converted
successfully.
This patch replaces the code for getting an number from a
userspace buffer by a simple call to kstrou8_from_user.
This makes it easier to read and less error prone.
This patch replaces the code for getting an number from a
userspace buffer by a simple call to kstrou8_from_user.
This makes it easier to read and less error prone.
Since the old buffers held only values up to 255, we don't need
kstrtoul, but rather kstrtou8.
Kernel Version: v3.0-rc3
Changes in v2:
- Use kstrtou8 instead of kstrtoul due to small numbers
- Dropped then unnecessary checks
(Both remarks from Alexey Dobriyan, Thanks!)
Changes in v3:
The local struct dev variable isn't needed anymore
Signed-off-by: Peter Huewe <[email protected]>
---
drivers/mfd/ab8500-debugfs.c | 62 +++++++++++------------------------------
1 files changed, 17 insertions(+), 45 deletions(-)
diff --git a/drivers/mfd/ab8500-debugfs.c b/drivers/mfd/ab8500-debugfs.c
index 64748e4..bb78f4a 100644
--- a/drivers/mfd/ab8500-debugfs.c
+++ b/drivers/mfd/ab8500-debugfs.c
@@ -419,20 +419,13 @@ static ssize_t ab8500_bank_write(struct file *file,
size_t count, loff_t *ppos)
{
struct device *dev = ((struct seq_file *)(file->private_data))->private;
- char buf[32];
- int buf_size;
- unsigned long user_bank;
+ u8 user_bank;
int err;
- /* Get userspace string and assure termination */
- buf_size = min(count, (sizeof(buf) - 1));
- if (copy_from_user(buf, user_buf, buf_size))
- return -EFAULT;
- buf[buf_size] = 0;
-
- err = strict_strtoul(buf, 0, &user_bank);
+ /* Get userspace string and convert to number */
+ err = kstrtou8_from_user(user_buf, count, 0, &user_bank);
if (err)
- return -EINVAL;
+ return err;
if (user_bank >= AB8500_NUM_BANKS) {
dev_err(dev, "debugfs error input > number of banks\n");
@@ -441,7 +434,7 @@ static ssize_t ab8500_bank_write(struct file *file,
debug_bank = user_bank;
- return buf_size;
+ return count;
}
static int ab8500_address_print(struct seq_file *s, void *p)
@@ -458,27 +451,16 @@ static ssize_t ab8500_address_write(struct file *file,
const char __user *user_buf,
size_t count, loff_t *ppos)
{
- struct device *dev = ((struct seq_file *)(file->private_data))->private;
- char buf[32];
- int buf_size;
- unsigned long user_address;
+ u8 user_address;
int err;
- /* Get userspace string and assure termination */
- buf_size = min(count, (sizeof(buf) - 1));
- if (copy_from_user(buf, user_buf, buf_size))
- return -EFAULT;
- buf[buf_size] = 0;
-
- err = strict_strtoul(buf, 0, &user_address);
+ /* Get userspace string and convert to number*/
+ err = kstrtou8_from_user(user_buf, count, 0, &user_address);
if (err)
- return -EINVAL;
- if (user_address > 0xff) {
- dev_err(dev, "debugfs error input > 0xff\n");
- return -EINVAL;
- }
+ return err;
+
debug_address = user_address;
- return buf_size;
+ return count;
}
static int ab8500_val_print(struct seq_file *s, void *p)
@@ -509,24 +491,14 @@ static ssize_t ab8500_val_write(struct file *file,
size_t count, loff_t *ppos)
{
struct device *dev = ((struct seq_file *)(file->private_data))->private;
- char buf[32];
- int buf_size;
- unsigned long user_val;
+ u8 user_val;
int err;
- /* Get userspace string and assure termination */
- buf_size = min(count, (sizeof(buf)-1));
- if (copy_from_user(buf, user_buf, buf_size))
- return -EFAULT;
- buf[buf_size] = 0;
-
- err = strict_strtoul(buf, 0, &user_val);
+ /* Get userspace string and convert to number */
+ err = kstrtou8_from_user(user_buf, count, 0, &user_val);
if (err)
- return -EINVAL;
- if (user_val > 0xff) {
- dev_err(dev, "debugfs error input > 0xff\n");
- return -EINVAL;
- }
+ return err;
+
err = abx500_set_register_interruptible(dev,
(u8)debug_bank, debug_address, (u8)user_val);
if (err < 0) {
@@ -534,7 +506,7 @@ static ssize_t ab8500_val_write(struct file *file,
return -EINVAL;
}
- return buf_size;
+ return count;
}
static const struct file_operations ab8500_bank_fops = {
--
1.7.3.4
Am Montag 20 Juni 2011, 21:50:38 schrieb Alexey Dobriyan:
> On Mon, Jun 20, 2011 at 09:46:28PM +0200, Peter Huewe wrote:
> > - char buf[32];
> > - int buf_size;
> > - unsigned long user_address;
> > + u8 user_address;
> > + /* Get userspace string and convert to number */
> > + err = kstrtou8_from_user(user_buf, count, 0, &user_address);
...
> >
> > ab->debug_address = user_address;
>
> You don't need temporary variable and should write straight to final
> location, because kstrto* functions will never write to result unless it
> was converted successfully.
Alexey thank you very much for your review, hints and most of all patience ;)
The code really gets cleaner and cleaner.
While changing the code (once again ;) and looking at your remarks I also saw
that ab3550->debug_address and ->debug_bank are always casted to u8.
Do you think I could also change the two fields of the struct ab3550 (only
used in this file) to u8 in this patch, too?
This way I could get rid of the u8* cast which is now needed in this case, if
I take you last remark into account.
> err = kstrtou8_from_user(user_buf, count, 0, (u8 *)&ab->debug_address);
And also clean the code from all the other unnecessary u8 casts.
What do you think?
Or split it up into two seperate patches?
Thanks,
Peter
This patch replaces the code for getting an number from a
userspace buffer by a simple call to kstrou8_from_user.
This makes it easier to read and less error prone.
Since the old buffers held only values up to 255, we don't need
kstrtoul, but rather kstrtou8.
Kernel Version: v3.0-rc3
Changes in v2:
- Use kstrtou8 instead of kstrtoul due to small numbers
- Dropped then unnecessary checks
Changes in v3:
- Drop now unnecessary local variables
- Change the debug_address and debug_bank members to u8 since they hold
only values up to 255, and also remove the now superfluous u8 casts
(Remarks from Alexey Dobriyan, Thanks!)
Signed-off-by: Peter Huewe <[email protected]>
---
drivers/mfd/ab3550-core.c | 79 +++++++++++++-------------------------------
1 files changed, 24 insertions(+), 55 deletions(-)
diff --git a/drivers/mfd/ab3550-core.c b/drivers/mfd/ab3550-core.c
index 3d7dce6..bd0df90 100644
--- a/drivers/mfd/ab3550-core.c
+++ b/drivers/mfd/ab3550-core.c
@@ -73,8 +73,8 @@ struct ab3550 {
u8 startup_events[AB3550_NUM_EVENT_REG];
bool startup_events_read;
#ifdef CONFIG_DEBUG_FS
- unsigned int debug_bank;
- unsigned int debug_address;
+ u8 debug_bank;
+ u8 debug_address;
#endif
};
@@ -879,20 +879,13 @@ static ssize_t ab3550_bank_write(struct file *file,
size_t count, loff_t *ppos)
{
struct ab3550 *ab = ((struct seq_file *)(file->private_data))->private;
- char buf[32];
- int buf_size;
- unsigned long user_bank;
+ u8 user_bank;
int err;
- /* Get userspace string and assure termination */
- buf_size = min(count, (sizeof(buf) - 1));
- if (copy_from_user(buf, user_buf, buf_size))
- return -EFAULT;
- buf[buf_size] = 0;
-
- err = strict_strtoul(buf, 0, &user_bank);
+ /* Get userspace string and convert to number */
+ err = kstrtou8_from_user(user_buf, count, 0, &user_bank);
if (err)
- return -EINVAL;
+ return err;
if (user_bank >= AB3550_NUM_BANKS) {
dev_err(&ab->i2c_client[0]->dev,
@@ -902,7 +895,7 @@ static ssize_t ab3550_bank_write(struct file *file,
ab->debug_bank = user_bank;
- return buf_size;
+ return count;
}
static int ab3550_address_print(struct seq_file *s, void *p)
@@ -923,27 +916,14 @@ static ssize_t ab3550_address_write(struct file *file,
size_t count, loff_t *ppos)
{
struct ab3550 *ab = ((struct seq_file *)(file->private_data))->private;
- char buf[32];
- int buf_size;
- unsigned long user_address;
int err;
- /* Get userspace string and assure termination */
- buf_size = min(count, (sizeof(buf) - 1));
- if (copy_from_user(buf, user_buf, buf_size))
- return -EFAULT;
- buf[buf_size] = 0;
-
- err = strict_strtoul(buf, 0, &user_address);
+ /* Get userspace string and convert to number */
+ err = kstrtou8_from_user(user_buf, count, 0, &ab->debug_address);
if (err)
- return -EINVAL;
- if (user_address > 0xff) {
- dev_err(&ab->i2c_client[0]->dev,
- "debugfs error input > 0xff\n");
- return -EINVAL;
- }
- ab->debug_address = user_address;
- return buf_size;
+ return err;
+
+ return count;
}
static int ab3550_val_print(struct seq_file *s, void *p)
@@ -952,8 +932,8 @@ static int ab3550_val_print(struct seq_file *s, void *p)
int err;
u8 regvalue;
- err = get_register_interruptible(ab, (u8)ab->debug_bank,
- (u8)ab->debug_address, ®value);
+ err = get_register_interruptible(ab, ab->debug_bank,
+ ab->debug_address, ®value);
if (err)
return -EINVAL;
seq_printf(s, "0x%02X\n", regvalue);
@@ -971,38 +951,27 @@ static ssize_t ab3550_val_write(struct file *file,
size_t count, loff_t *ppos)
{
struct ab3550 *ab = ((struct seq_file *)(file->private_data))->private;
- char buf[32];
- int buf_size;
- unsigned long user_val;
+ u8 user_val;
int err;
u8 regvalue;
- /* Get userspace string and assure termination */
- buf_size = min(count, (sizeof(buf)-1));
- if (copy_from_user(buf, user_buf, buf_size))
- return -EFAULT;
- buf[buf_size] = 0;
-
- err = strict_strtoul(buf, 0, &user_val);
+ /* Get userspace string and convert to number */
+ err = kstrtou8_from_user(user_buf, count, 0, &user_val);
if (err)
- return -EINVAL;
- if (user_val > 0xff) {
- dev_err(&ab->i2c_client[0]->dev,
- "debugfs error input > 0xff\n");
- return -EINVAL;
- }
+ return err;
+
err = mask_and_set_register_interruptible(
- ab, (u8)ab->debug_bank,
- (u8)ab->debug_address, 0xFF, (u8)user_val);
+ ab, ab->debug_bank,
+ ab->debug_address, 0xFF, user_val);
if (err)
return -EINVAL;
- get_register_interruptible(ab, (u8)ab->debug_bank,
- (u8)ab->debug_address, ®value);
+ get_register_interruptible(ab, ab->debug_bank,
+ ab->debug_address, ®value);
if (err)
return -EINVAL;
- return buf_size;
+ return count;
}
static const struct file_operations ab3550_bank_fops = {
--
1.7.3.4
This patch replaces the code for getting an number from a
userspace buffer by a simple call to kstrou8_from_user.
This makes it easier to read and less error prone.
Since the old buffers held only values up to 255, we don't need
kstrtoul, but rather kstrtou8.
Kernel Version: v3.0-rc3
Changes in v2:
- Use kstrtou8 instead of kstrtoul due to small numbers
- Dropped then unnecessary checks
Changes in v3:
- The local struct dev variable isn't needed anymore
Changes in v4:
- Drop unneccesary now local variables and casts
- Change the debug_address and debug_bank members to u8 since they hold
only values up to 255, and also remove the now superfluous u8 casts
(Remarks from Alexey Dobriyan, Thanks!)
Signed-off-by: Peter Huewe <[email protected]>
---
drivers/mfd/ab8500-debugfs.c | 74 ++++++++++++-----------------------------
1 files changed, 22 insertions(+), 52 deletions(-)
diff --git a/drivers/mfd/ab8500-debugfs.c b/drivers/mfd/ab8500-debugfs.c
index 64748e4..5d3cd35 100644
--- a/drivers/mfd/ab8500-debugfs.c
+++ b/drivers/mfd/ab8500-debugfs.c
@@ -14,8 +14,8 @@
#include <linux/mfd/abx500.h>
#include <linux/mfd/ab8500.h>
-static u32 debug_bank;
-static u32 debug_address;
+static u8 debug_bank;
+static u8 debug_address;
/**
* struct ab8500_reg_range
@@ -357,7 +357,7 @@ static int ab8500_registers_print(struct seq_file *s, void *p)
{
struct device *dev = s->private;
unsigned int i;
- u32 bank = debug_bank;
+ u8 bank = debug_bank;
seq_printf(s, AB8500_NAME_STRING " register values:\n");
@@ -372,7 +372,7 @@ static int ab8500_registers_print(struct seq_file *s, void *p)
int err;
err = abx500_get_register_interruptible(dev,
- (u8)bank, (u8)reg, &value);
+ bank, (u8)reg, &value);
if (err < 0) {
dev_err(dev, "ab->read fail %d\n", err);
return err;
@@ -419,20 +419,13 @@ static ssize_t ab8500_bank_write(struct file *file,
size_t count, loff_t *ppos)
{
struct device *dev = ((struct seq_file *)(file->private_data))->private;
- char buf[32];
- int buf_size;
- unsigned long user_bank;
+ u8 user_bank;
int err;
- /* Get userspace string and assure termination */
- buf_size = min(count, (sizeof(buf) - 1));
- if (copy_from_user(buf, user_buf, buf_size))
- return -EFAULT;
- buf[buf_size] = 0;
-
- err = strict_strtoul(buf, 0, &user_bank);
+ /* Get userspace string and convert to number */
+ err = kstrtou8_from_user(user_buf, count, 0, &user_bank);
if (err)
- return -EINVAL;
+ return err;
if (user_bank >= AB8500_NUM_BANKS) {
dev_err(dev, "debugfs error input > number of banks\n");
@@ -441,7 +434,7 @@ static ssize_t ab8500_bank_write(struct file *file,
debug_bank = user_bank;
- return buf_size;
+ return count;
}
static int ab8500_address_print(struct seq_file *s, void *p)
@@ -458,27 +451,14 @@ static ssize_t ab8500_address_write(struct file *file,
const char __user *user_buf,
size_t count, loff_t *ppos)
{
- struct device *dev = ((struct seq_file *)(file->private_data))->private;
- char buf[32];
- int buf_size;
- unsigned long user_address;
int err;
- /* Get userspace string and assure termination */
- buf_size = min(count, (sizeof(buf) - 1));
- if (copy_from_user(buf, user_buf, buf_size))
- return -EFAULT;
- buf[buf_size] = 0;
-
- err = strict_strtoul(buf, 0, &user_address);
+ /* Get userspace string and convert to number*/
+ err = kstrtou8_from_user(user_buf, count, 0, &debug_address);
if (err)
- return -EINVAL;
- if (user_address > 0xff) {
- dev_err(dev, "debugfs error input > 0xff\n");
- return -EINVAL;
- }
- debug_address = user_address;
- return buf_size;
+ return err;
+
+ return count;
}
static int ab8500_val_print(struct seq_file *s, void *p)
@@ -488,7 +468,7 @@ static int ab8500_val_print(struct seq_file *s, void *p)
u8 regvalue;
ret = abx500_get_register_interruptible(dev,
- (u8)debug_bank, (u8)debug_address, ®value);
+ debug_bank, debug_address, ®value);
if (ret < 0) {
dev_err(dev, "abx500_get_reg fail %d, %d\n",
ret, __LINE__);
@@ -509,32 +489,22 @@ static ssize_t ab8500_val_write(struct file *file,
size_t count, loff_t *ppos)
{
struct device *dev = ((struct seq_file *)(file->private_data))->private;
- char buf[32];
- int buf_size;
- unsigned long user_val;
+ u8 user_val;
int err;
- /* Get userspace string and assure termination */
- buf_size = min(count, (sizeof(buf)-1));
- if (copy_from_user(buf, user_buf, buf_size))
- return -EFAULT;
- buf[buf_size] = 0;
-
- err = strict_strtoul(buf, 0, &user_val);
+ /* Get userspace string and convert to number */
+ err = kstrtou8_from_user(user_buf, count, 0, &user_val);
if (err)
- return -EINVAL;
- if (user_val > 0xff) {
- dev_err(dev, "debugfs error input > 0xff\n");
- return -EINVAL;
- }
+ return err;
+
err = abx500_set_register_interruptible(dev,
- (u8)debug_bank, debug_address, (u8)user_val);
+ debug_bank, debug_address, user_val);
if (err < 0) {
printk(KERN_ERR "abx500_set_reg failed %d, %d", err, __LINE__);
return -EINVAL;
}
- return buf_size;
+ return count;
}
static const struct file_operations ab8500_bank_fops = {
--
1.7.3.4