| Message ID | 20231009165741.746184-1-max.kellermann@ionos.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/7] drivers/rtc/sysfs: move code to count_attribute_groups() | expand |
On Mon, Oct 09, 2023 at 06:57:34PM +0200, Max Kellermann wrote: > This avoids overloading the "groups" variable for three different > purposes What 3 different purposes? And you sent a patch series that hit a lot of different subsystems and maintainers all at once, but you didn't cc: all of us. Who is supposed to take these? But step back, _WHY_ are all of these changes needed? I don't see a cover letter explaining the rational, and on their own, they really don't do anything, so it's really confusing as to what is going on here. thanks, greg k-h
On Mon, Oct 9, 2023 at 7:24 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> What 3 different purposes?
The variable gets initialized 3 times with different (unrelated) values:
1. count old rtc->dev.groups
2. count grps
3. allocate new rtc->dev.groups
This coding style is confusing and error prone. The code gets simpler
with this patch, because the cognitive load of understanding a
"count_attribute_groups()" call is lower than reading the "while" loop
and figuring out why it's there.
But I really wrote this patch because after making "rtc->dev.groups"
const, I cannot any longer assign it to "groups", because that
variable must remain non-const.
On 09/10/2023 18:57:34+0200, Max Kellermann wrote: > This avoids overloading the "groups" variable for three different > purposes > > Signed-off-by: Max Kellermann <max.kellermann@ionos.com> > --- > drivers/rtc/sysfs.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/drivers/rtc/sysfs.c b/drivers/rtc/sysfs.c > index e3062c4d3f2c..617933d52324 100644 > --- a/drivers/rtc/sysfs.c > +++ b/drivers/rtc/sysfs.c > @@ -313,21 +313,25 @@ const struct attribute_group **rtc_get_dev_attribute_groups(void) > return rtc_attr_groups; > } > > +static size_t count_attribute_groups(const struct attribute_group *const*groups) Shouldn't that live out of the rtc subsystem? > +{ > + size_t count = 0; > + > + for (; *groups; ++groups) > + ++count; > + return count; > +} > + > int rtc_add_groups(struct rtc_device *rtc, const struct attribute_group **grps) > { > - size_t old_cnt = 0, add_cnt = 0, new_cnt; > + size_t old_cnt, add_cnt, new_cnt; > const struct attribute_group **groups, **old; > > if (!grps) > return -EINVAL; > > - groups = rtc->dev.groups; > - if (groups) > - for (; *groups; groups++) > - old_cnt++; > - > - for (groups = grps; *groups; groups++) > - add_cnt++; > + old_cnt = rtc->dev.groups ? count_attribute_groups(rtc->dev.groups) : 0; > + add_cnt = count_attribute_groups(grps); > > new_cnt = old_cnt + add_cnt + 1; > groups = devm_kcalloc(&rtc->dev, new_cnt, sizeof(*groups), GFP_KERNEL); > -- > 2.39.2 >
On Tue, Oct 10, 2023 at 10:15 AM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
> Shouldn't that live out of the rtc subsystem?
You suggest making this a global function? Sure, if that's preferred,
I'll change it. Whose decision is this?
diff --git a/drivers/rtc/sysfs.c b/drivers/rtc/sysfs.c index e3062c4d3f2c..617933d52324 100644 --- a/drivers/rtc/sysfs.c +++ b/drivers/rtc/sysfs.c @@ -313,21 +313,25 @@ const struct attribute_group **rtc_get_dev_attribute_groups(void) return rtc_attr_groups; } +static size_t count_attribute_groups(const struct attribute_group *const*groups) +{ + size_t count = 0; + + for (; *groups; ++groups) + ++count; + return count; +} + int rtc_add_groups(struct rtc_device *rtc, const struct attribute_group **grps) { - size_t old_cnt = 0, add_cnt = 0, new_cnt; + size_t old_cnt, add_cnt, new_cnt; const struct attribute_group **groups, **old; if (!grps) return -EINVAL; - groups = rtc->dev.groups; - if (groups) - for (; *groups; groups++) - old_cnt++; - - for (groups = grps; *groups; groups++) - add_cnt++; + old_cnt = rtc->dev.groups ? count_attribute_groups(rtc->dev.groups) : 0; + add_cnt = count_attribute_groups(grps); new_cnt = old_cnt + add_cnt + 1; groups = devm_kcalloc(&rtc->dev, new_cnt, sizeof(*groups), GFP_KERNEL);
This avoids overloading the "groups" variable for three different purposes Signed-off-by: Max Kellermann <max.kellermann@ionos.com> --- drivers/rtc/sysfs.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)