linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] mm/damon/sysfs: return 'err' value when call kstrtoul() failed
@ 2022-09-20  2:51 Xin Hao
  2022-09-20 16:33 ` SeongJae Park
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Xin Hao @ 2022-09-20  2:51 UTC (permalink / raw)
  To: sj; +Cc: akpm, damon, linux-mm, linux-kernel, xhao

We had better return the 'err' value when calling kstrtoul() failed, so
the user will know why it really fails, there do little change, let it
return the 'err' value when failed.

Suggested-by: SeongJae Park <sj@kernel.org>
Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
Changes from v1
https://lore.kernel.org/linux-mm/20220919170305.61335-1-sj@kernel.org/T/
- keep the 'err' variable, and return the 'err' value when failed.
---
 mm/damon/sysfs.c | 46 ++++++++++++++--------------------------------
 1 file changed, 14 insertions(+), 32 deletions(-)

diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
index 0cca1909bf67..455215a5c059 100644
--- a/mm/damon/sysfs.c
+++ b/mm/damon/sysfs.c
@@ -58,7 +58,7 @@ static ssize_t min_store(struct kobject *kobj, struct kobj_attribute *attr,

 	err = kstrtoul(buf, 0, &min);
 	if (err)
-		return -EINVAL;
+		return err;

 	range->min = min;
 	return count;
@@ -83,7 +83,7 @@ static ssize_t max_store(struct kobject *kobj, struct kobj_attribute *attr,

 	err = kstrtoul(buf, 0, &max);
 	if (err)
-		return -EINVAL;
+		return err;

 	range->max = max;
 	return count;
@@ -291,9 +291,7 @@ static ssize_t interval_us_store(struct kobject *kobj,
 			struct damon_sysfs_watermarks, kobj);
 	int err = kstrtoul(buf, 0, &watermarks->interval_us);

-	if (err)
-		return -EINVAL;
-	return count;
+	return err ? err : count;
 }

 static ssize_t high_show(struct kobject *kobj,
@@ -312,9 +310,7 @@ static ssize_t high_store(struct kobject *kobj,
 			struct damon_sysfs_watermarks, kobj);
 	int err = kstrtoul(buf, 0, &watermarks->high);

-	if (err)
-		return -EINVAL;
-	return count;
+	return err ? err : count;
 }

 static ssize_t mid_show(struct kobject *kobj,
@@ -333,9 +329,7 @@ static ssize_t mid_store(struct kobject *kobj,
 			struct damon_sysfs_watermarks, kobj);
 	int err = kstrtoul(buf, 0, &watermarks->mid);

-	if (err)
-		return -EINVAL;
-	return count;
+	return err ? err : count;
 }

 static ssize_t low_show(struct kobject *kobj,
@@ -354,9 +348,7 @@ static ssize_t low_store(struct kobject *kobj,
 			struct damon_sysfs_watermarks, kobj);
 	int err = kstrtoul(buf, 0, &watermarks->low);

-	if (err)
-		return -EINVAL;
-	return count;
+	return err ? err : count;
 }

 static void damon_sysfs_watermarks_release(struct kobject *kobj)
@@ -437,9 +429,7 @@ static ssize_t sz_permil_store(struct kobject *kobj,
 			struct damon_sysfs_weights, kobj);
 	int err = kstrtouint(buf, 0, &weights->sz);

-	if (err)
-		return -EINVAL;
-	return count;
+	return err ? err : count;
 }

 static ssize_t nr_accesses_permil_show(struct kobject *kobj,
@@ -458,9 +448,7 @@ static ssize_t nr_accesses_permil_store(struct kobject *kobj,
 			struct damon_sysfs_weights, kobj);
 	int err = kstrtouint(buf, 0, &weights->nr_accesses);

-	if (err)
-		return -EINVAL;
-	return count;
+	return err ? err : count;
 }

 static ssize_t age_permil_show(struct kobject *kobj,
@@ -479,9 +467,7 @@ static ssize_t age_permil_store(struct kobject *kobj,
 			struct damon_sysfs_weights, kobj);
 	int err = kstrtouint(buf, 0, &weights->age);

-	if (err)
-		return -EINVAL;
-	return count;
+	return err ? err : count;
 }

 static void damon_sysfs_weights_release(struct kobject *kobj)
@@ -1111,9 +1097,7 @@ static ssize_t start_store(struct kobject *kobj, struct kobj_attribute *attr,
 			struct damon_sysfs_region, kobj);
 	int err = kstrtoul(buf, 0, &region->start);

-	if (err)
-		return -EINVAL;
-	return count;
+	return err ? err : count;
 }

 static ssize_t end_show(struct kobject *kobj, struct kobj_attribute *attr,
@@ -1132,9 +1116,7 @@ static ssize_t end_store(struct kobject *kobj, struct kobj_attribute *attr,
 			struct damon_sysfs_region, kobj);
 	int err = kstrtoul(buf, 0, &region->end);

-	if (err)
-		return -EINVAL;
-	return count;
+	return err ? err : count;
 }

 static void damon_sysfs_region_release(struct kobject *kobj)
@@ -1528,7 +1510,7 @@ static ssize_t sample_us_store(struct kobject *kobj,
 	int err = kstrtoul(buf, 0, &us);

 	if (err)
-		return -EINVAL;
+		return err;

 	intervals->sample_us = us;
 	return count;
@@ -1552,7 +1534,7 @@ static ssize_t aggr_us_store(struct kobject *kobj, struct kobj_attribute *attr,
 	int err = kstrtoul(buf, 0, &us);

 	if (err)
-		return -EINVAL;
+		return err;

 	intervals->aggr_us = us;
 	return count;
@@ -1576,7 +1558,7 @@ static ssize_t update_us_store(struct kobject *kobj,
 	int err = kstrtoul(buf, 0, &us);

 	if (err)
-		return -EINVAL;
+		return err;

 	intervals->update_us = us;
 	return count;
--
2.31.0

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH v2] mm/damon/sysfs: return 'err' value when call kstrtoul() failed
  2022-09-20  2:51 [PATCH v2] mm/damon/sysfs: return 'err' value when call kstrtoul() failed Xin Hao
@ 2022-09-20 16:33 ` SeongJae Park
  2022-09-20 16:35 ` [PATCH v3] " SeongJae Park
  2022-09-21  1:28 ` haoxin
  2 siblings, 0 replies; 4+ messages in thread
From: SeongJae Park @ 2022-09-20 16:33 UTC (permalink / raw)
  To: Xin Hao; +Cc: sj, akpm, damon, linux-mm, linux-kernel

Hi Xin,

On Tue, 20 Sep 2022 10:51:58 +0800 Xin Hao <xhao@linux.alibaba.com> wrote:

> We had better return the 'err' value when calling kstrtoul() failed, so
> the user will know why it really fails, there do little change, let it
> return the 'err' value when failed.
> 
> Suggested-by: SeongJae Park <sj@kernel.org>
> Signed-off-by: Xin Hao <xhao@linux.alibaba.com>

Looks good to me overall, but...

> Changes from v1
> https://lore.kernel.org/linux-mm/20220919170305.61335-1-sj@kernel.org/T/
> - keep the 'err' variable, and return the 'err' value when failed.

Patch changelog shouldn't be here as a part of the commit message,

> ---

But here.

Other than that,

Reviewed-by: SeongJae Park <sj@kernel.org>

I will revise and post it on behalf of Xin, for reducing unnecessary Andrew's
burden.


Thanks,
SJ

[...]

^ permalink raw reply	[flat|nested] 4+ messages in thread
* [PATCH v3] mm/damon/sysfs: return 'err' value when call kstrtoul() failed
  2022-09-20  2:51 [PATCH v2] mm/damon/sysfs: return 'err' value when call kstrtoul() failed Xin Hao
  2022-09-20 16:33 ` SeongJae Park
@ 2022-09-20 16:35 ` SeongJae Park
  2022-09-21  1:28 ` haoxin
  2 siblings, 0 replies; 4+ messages in thread
From: SeongJae Park @ 2022-09-20 16:35 UTC (permalink / raw)
  To: sj; +Cc: Xin Hao, akpm, damon, linux-mm, linux-kernel

From: Xin Hao <xhao@linux.alibaba.com>

We had better return the 'err' value when calling kstrtoul() failed, so
the user will know why it really fails, there do little change, let it
return the 'err' value when failed.

Suggested-by: SeongJae Park <sj@kernel.org>
Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
Reviewed-by: SeongJae Park <sj@kernel.org>
Signed-off-by: SeongJae Park <sj@kernel.org>

---
Changes from v2
(https://lore.kernel.org/damon/20220920025158.70293-1-xhao@linux.alibaba.com/)
- Move patch changelog outside of the commit message area

Changes from v1
(https://lore.kernel.org/linux-mm/20220919170305.61335-1-sj@kernel.org/T/)
- keep the 'err' variable, and return the 'err' value when failed.

 mm/damon/sysfs.c | 46 ++++++++++++++--------------------------------
 1 file changed, 14 insertions(+), 32 deletions(-)

diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
index 0cca1909bf67..455215a5c059 100644
--- a/mm/damon/sysfs.c
+++ b/mm/damon/sysfs.c
@@ -58,7 +58,7 @@ static ssize_t min_store(struct kobject *kobj, struct kobj_attribute *attr,

 	err = kstrtoul(buf, 0, &min);
 	if (err)
-		return -EINVAL;
+		return err;

 	range->min = min;
 	return count;
@@ -83,7 +83,7 @@ static ssize_t max_store(struct kobject *kobj, struct kobj_attribute *attr,

 	err = kstrtoul(buf, 0, &max);
 	if (err)
-		return -EINVAL;
+		return err;

 	range->max = max;
 	return count;
@@ -291,9 +291,7 @@ static ssize_t interval_us_store(struct kobject *kobj,
 			struct damon_sysfs_watermarks, kobj);
 	int err = kstrtoul(buf, 0, &watermarks->interval_us);

-	if (err)
-		return -EINVAL;
-	return count;
+	return err ? err : count;
 }

 static ssize_t high_show(struct kobject *kobj,
@@ -312,9 +310,7 @@ static ssize_t high_store(struct kobject *kobj,
 			struct damon_sysfs_watermarks, kobj);
 	int err = kstrtoul(buf, 0, &watermarks->high);

-	if (err)
-		return -EINVAL;
-	return count;
+	return err ? err : count;
 }

 static ssize_t mid_show(struct kobject *kobj,
@@ -333,9 +329,7 @@ static ssize_t mid_store(struct kobject *kobj,
 			struct damon_sysfs_watermarks, kobj);
 	int err = kstrtoul(buf, 0, &watermarks->mid);

-	if (err)
-		return -EINVAL;
-	return count;
+	return err ? err : count;
 }

 static ssize_t low_show(struct kobject *kobj,
@@ -354,9 +348,7 @@ static ssize_t low_store(struct kobject *kobj,
 			struct damon_sysfs_watermarks, kobj);
 	int err = kstrtoul(buf, 0, &watermarks->low);

-	if (err)
-		return -EINVAL;
-	return count;
+	return err ? err : count;
 }

 static void damon_sysfs_watermarks_release(struct kobject *kobj)
@@ -437,9 +429,7 @@ static ssize_t sz_permil_store(struct kobject *kobj,
 			struct damon_sysfs_weights, kobj);
 	int err = kstrtouint(buf, 0, &weights->sz);

-	if (err)
-		return -EINVAL;
-	return count;
+	return err ? err : count;
 }

 static ssize_t nr_accesses_permil_show(struct kobject *kobj,
@@ -458,9 +448,7 @@ static ssize_t nr_accesses_permil_store(struct kobject *kobj,
 			struct damon_sysfs_weights, kobj);
 	int err = kstrtouint(buf, 0, &weights->nr_accesses);

-	if (err)
-		return -EINVAL;
-	return count;
+	return err ? err : count;
 }

 static ssize_t age_permil_show(struct kobject *kobj,
@@ -479,9 +467,7 @@ static ssize_t age_permil_store(struct kobject *kobj,
 			struct damon_sysfs_weights, kobj);
 	int err = kstrtouint(buf, 0, &weights->age);

-	if (err)
-		return -EINVAL;
-	return count;
+	return err ? err : count;
 }

 static void damon_sysfs_weights_release(struct kobject *kobj)
@@ -1111,9 +1097,7 @@ static ssize_t start_store(struct kobject *kobj, struct kobj_attribute *attr,
 			struct damon_sysfs_region, kobj);
 	int err = kstrtoul(buf, 0, &region->start);

-	if (err)
-		return -EINVAL;
-	return count;
+	return err ? err : count;
 }

 static ssize_t end_show(struct kobject *kobj, struct kobj_attribute *attr,
@@ -1132,9 +1116,7 @@ static ssize_t end_store(struct kobject *kobj, struct kobj_attribute *attr,
 			struct damon_sysfs_region, kobj);
 	int err = kstrtoul(buf, 0, &region->end);

-	if (err)
-		return -EINVAL;
-	return count;
+	return err ? err : count;
 }

 static void damon_sysfs_region_release(struct kobject *kobj)
@@ -1528,7 +1510,7 @@ static ssize_t sample_us_store(struct kobject *kobj,
 	int err = kstrtoul(buf, 0, &us);

 	if (err)
-		return -EINVAL;
+		return err;

 	intervals->sample_us = us;
 	return count;
@@ -1552,7 +1534,7 @@ static ssize_t aggr_us_store(struct kobject *kobj, struct kobj_attribute *attr,
 	int err = kstrtoul(buf, 0, &us);

 	if (err)
-		return -EINVAL;
+		return err;

 	intervals->aggr_us = us;
 	return count;
@@ -1576,7 +1558,7 @@ static ssize_t update_us_store(struct kobject *kobj,
 	int err = kstrtoul(buf, 0, &us);

 	if (err)
-		return -EINVAL;
+		return err;

 	intervals->update_us = us;
 	return count;
--
2.31.0

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH v3] mm/damon/sysfs: return 'err' value when call kstrtoul() failed
  2022-09-20  2:51 [PATCH v2] mm/damon/sysfs: return 'err' value when call kstrtoul() failed Xin Hao
  2022-09-20 16:33 ` SeongJae Park
  2022-09-20 16:35 ` [PATCH v3] " SeongJae Park
@ 2022-09-21  1:28 ` haoxin
  2 siblings, 0 replies; 4+ messages in thread
From: haoxin @ 2022-09-21  1:28 UTC (permalink / raw)
  To: SeongJae Park; +Cc: akpm, damon, linux-mm, linux-kernel

Hi SJ,


在 2022/9/21 上午12:35, SeongJae Park 写道:
> From: Xin Hao <xhao@linux.alibaba.com>
>
> We had better return the 'err' value when calling kstrtoul() failed, so
> the user will know why it really fails, there do little change, let it
> return the 'err' value when failed.
>
> Suggested-by: SeongJae Park <sj@kernel.org>
> Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
> Reviewed-by: SeongJae Park <sj@kernel.org>
> Signed-off-by: SeongJae Park <sj@kernel.org>
>
> ---
> Changes from v2
> (https://lore.kernel.org/damon/20220920025158.70293-1-xhao@linux.alibaba.com/)
> - Move patch changelog outside of the commit message area

Thanks for fix this stupid bug，i will nerver make it happen once again.

Reviewed-by: Xin Hao <xhao@linux.alibaba.com>

>
> Changes from v1
> (https://lore.kernel.org/linux-mm/20220919170305.61335-1-sj@kernel.org/T/)
> - keep the 'err' variable, and return the 'err' value when failed.
>
>   mm/damon/sysfs.c | 46 ++++++++++++++--------------------------------
>   1 file changed, 14 insertions(+), 32 deletions(-)
>
> diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
> index 0cca1909bf67..455215a5c059 100644
> --- a/mm/damon/sysfs.c
> +++ b/mm/damon/sysfs.c
> @@ -58,7 +58,7 @@ static ssize_t min_store(struct kobject *kobj, struct kobj_attribute *attr,
>
>   	err = kstrtoul(buf, 0, &min);
>   	if (err)
> -		return -EINVAL;
> +		return err;
>
>   	range->min = min;
>   	return count;
> @@ -83,7 +83,7 @@ static ssize_t max_store(struct kobject *kobj, struct kobj_attribute *attr,
>
>   	err = kstrtoul(buf, 0, &max);
>   	if (err)
> -		return -EINVAL;
> +		return err;
>
>   	range->max = max;
>   	return count;
> @@ -291,9 +291,7 @@ static ssize_t interval_us_store(struct kobject *kobj,
>   			struct damon_sysfs_watermarks, kobj);
>   	int err = kstrtoul(buf, 0, &watermarks->interval_us);
>
> -	if (err)
> -		return -EINVAL;
> -	return count;
> +	return err ? err : count;
>   }
>
>   static ssize_t high_show(struct kobject *kobj,
> @@ -312,9 +310,7 @@ static ssize_t high_store(struct kobject *kobj,
>   			struct damon_sysfs_watermarks, kobj);
>   	int err = kstrtoul(buf, 0, &watermarks->high);
>
> -	if (err)
> -		return -EINVAL;
> -	return count;
> +	return err ? err : count;
>   }
>
>   static ssize_t mid_show(struct kobject *kobj,
> @@ -333,9 +329,7 @@ static ssize_t mid_store(struct kobject *kobj,
>   			struct damon_sysfs_watermarks, kobj);
>   	int err = kstrtoul(buf, 0, &watermarks->mid);
>
> -	if (err)
> -		return -EINVAL;
> -	return count;
> +	return err ? err : count;
>   }
>
>   static ssize_t low_show(struct kobject *kobj,
> @@ -354,9 +348,7 @@ static ssize_t low_store(struct kobject *kobj,
>   			struct damon_sysfs_watermarks, kobj);
>   	int err = kstrtoul(buf, 0, &watermarks->low);
>
> -	if (err)
> -		return -EINVAL;
> -	return count;
> +	return err ? err : count;
>   }
>
>   static void damon_sysfs_watermarks_release(struct kobject *kobj)
> @@ -437,9 +429,7 @@ static ssize_t sz_permil_store(struct kobject *kobj,
>   			struct damon_sysfs_weights, kobj);
>   	int err = kstrtouint(buf, 0, &weights->sz);
>
> -	if (err)
> -		return -EINVAL;
> -	return count;
> +	return err ? err : count;
>   }
>
>   static ssize_t nr_accesses_permil_show(struct kobject *kobj,
> @@ -458,9 +448,7 @@ static ssize_t nr_accesses_permil_store(struct kobject *kobj,
>   			struct damon_sysfs_weights, kobj);
>   	int err = kstrtouint(buf, 0, &weights->nr_accesses);
>
> -	if (err)
> -		return -EINVAL;
> -	return count;
> +	return err ? err : count;
>   }
>
>   static ssize_t age_permil_show(struct kobject *kobj,
> @@ -479,9 +467,7 @@ static ssize_t age_permil_store(struct kobject *kobj,
>   			struct damon_sysfs_weights, kobj);
>   	int err = kstrtouint(buf, 0, &weights->age);
>
> -	if (err)
> -		return -EINVAL;
> -	return count;
> +	return err ? err : count;
>   }
>
>   static void damon_sysfs_weights_release(struct kobject *kobj)
> @@ -1111,9 +1097,7 @@ static ssize_t start_store(struct kobject *kobj, struct kobj_attribute *attr,
>   			struct damon_sysfs_region, kobj);
>   	int err = kstrtoul(buf, 0, &region->start);
>
> -	if (err)
> -		return -EINVAL;
> -	return count;
> +	return err ? err : count;
>   }
>
>   static ssize_t end_show(struct kobject *kobj, struct kobj_attribute *attr,
> @@ -1132,9 +1116,7 @@ static ssize_t end_store(struct kobject *kobj, struct kobj_attribute *attr,
>   			struct damon_sysfs_region, kobj);
>   	int err = kstrtoul(buf, 0, &region->end);
>
> -	if (err)
> -		return -EINVAL;
> -	return count;
> +	return err ? err : count;
>   }
>
>   static void damon_sysfs_region_release(struct kobject *kobj)
> @@ -1528,7 +1510,7 @@ static ssize_t sample_us_store(struct kobject *kobj,
>   	int err = kstrtoul(buf, 0, &us);
>
>   	if (err)
> -		return -EINVAL;
> +		return err;
>
>   	intervals->sample_us = us;
>   	return count;
> @@ -1552,7 +1534,7 @@ static ssize_t aggr_us_store(struct kobject *kobj, struct kobj_attribute *attr,
>   	int err = kstrtoul(buf, 0, &us);
>
>   	if (err)
> -		return -EINVAL;
> +		return err;
>
>   	intervals->aggr_us = us;
>   	return count;
> @@ -1576,7 +1558,7 @@ static ssize_t update_us_store(struct kobject *kobj,
>   	int err = kstrtoul(buf, 0, &us);
>
>   	if (err)
> -		return -EINVAL;
> +		return err;
>
>   	intervals->update_us = us;
>   	return count;
> --
> 2.31.0

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-09-21  1:29 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-20  2:51 [PATCH v2] mm/damon/sysfs: return 'err' value when call kstrtoul() failed Xin Hao
2022-09-20 16:33 ` SeongJae Park
2022-09-20 16:35 ` [PATCH v3] " SeongJae Park
2022-09-21  1:28 ` haoxin

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).