Re: [PATCH] mm/damon/sysfs: Remove misleading todo comment in nid_show()

[SeongJae Park <sj@kernel.org>]

On Tue, 21 Oct 2025 21:53:24 +0000 Swaraj Gaikwad <swarajgaikwad1925@gmail.com> wrote:

> The TODO comment in nid_show() suggested returning an error if the goal was
> not using nid. However, this comment was found to be inaccurate and
> misleading.This patch removes the TODO comment without changing any
> existing behavior.
> 
> This change follows feedback from SJ who pointed out [1] that wiring-order
> independence is expected and the function should simply show the last
> set value. and [2] checkpatch.pl complain about number of chars per line

This is another revision of your previous patch [1], right?  Thank you for
fixing the things I commented on.

That said, marking the fact that this is a new revision of other one on
subject, for example, setting the subject prefix as "[PATCH v2]", and adding
changelog from the previous version in commentary section could help review.
Please consider doing so from your next patch.  Please refer to the related
docs [2,3] for more details about that.

> 
> No functional code changes were made.
> 
> Tested with KUnit:
> - Built kernel with KUnit and DAMON sysfs tests enabled.
> - Executed KUnit tests:
>   ./tools/testing/kunit/kunit.py run --kunitconfig ./mm/damon/tests/
> - All 25 tests passed, including damon_sysfs_test_add_targets.
> 
> Signed-off-by: Swaraj Gaikwad <swarajgaikwad1925@gmail.com>
> Suggested-by: SeongJae Park <sj@kernel.org>

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

> 
> [1] https://lore.kernel.org/lkml/20251020151315.66260-1-sj@kernel.org/
> [2] https://lore.kernel.org/lkml/20251021010847.68473-1-sj@kernel.org/

And you should have put the above two lines before the above Signed-off-by:
line.  Please consider doing so from the next time.  Again, read the document
[1] for more details.

I found Andrew picked this patch on mm tree, adjusting the above two lines
properly [4].  So no new revision would be required.  But I'm noting this for
your next work.

> ---
>  mm/damon/sysfs-schemes.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> index 6536f16006c9..760279092b4f 100644
> --- a/mm/damon/sysfs-schemes.c
> +++ b/mm/damon/sysfs-schemes.c
> @@ -1112,7 +1112,6 @@ static ssize_t nid_show(struct kobject *kobj,
>  	struct damos_sysfs_quota_goal *goal = container_of(kobj, struct
>  			damos_sysfs_quota_goal, kobj);
> 
> -	/* todo: return error if the goal is not using nid */
> 
>  	return sysfs_emit(buf, "%d\n", goal->nid);
>  }

And this results in the code having two empty lines.  Just one single empty
line would be enough.  But that's too trivial, so I wouldn't request a new
revision to you, or in-queue direct fix to Andrew.

I only hope my trivial comments to help your future patches be easier to review
and more smoothly be merged :)

So, thank you for this nice patch.

[1] https://lore.kernel.org/20251021021712.59017-2-swarajgaikwad1925@gmail.com
[2] https://docs.kernel.org/process/submitting-patches.html#subject-line
[3] https://docs.kernel.org/process/submitting-patches.html#commentary
[4] https://lore.kernel.org/20251021183613.C4A6DC4CEF7@smtp.kernel.org


Thanks,
SJ

[...]