[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20180718072508.GG3211@piout.net>
Date: Wed, 18 Jul 2018 09:25:08 +0200
From: Alexandre Belloni <alexandre.belloni@...tlin.com>
To: Denis OSTERLAND <denis.osterland@...hl.com>
Cc: "a.zummo@...ertech.it" <a.zummo@...ertech.it>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"m.grzeschik@...gutronix.de" <m.grzeschik@...gutronix.de>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"mark.rutland@....com" <mark.rutland@....com>,
"linux-rtc@...r.kernel.org" <linux-rtc@...r.kernel.org>,
"kernel@...gutronix.de" <kernel@...gutronix.de>
Subject: Re: [PATCH v4 1/5] rtc: sysfs: facilitate attribute add to rtc device
Hello,
On 10/07/2018 09:44:15+0000, Denis OSTERLAND wrote:
> From: Denis Osterland <Denis.Osterland@...hl.com>
>
> This patches addresses following problem:
> rtc_allocate_device
> devm_device_add_group <-- kernel oops / null pointer, because
> sysfs entry does not yet exist
> rtc_register_device
> rc = devm_device_add_group
> if (rc)
> return rc; <-- forbidden to return error code
> after device register
>
> In case groups were not yet registered (kobj.state_in_sysfs == 0)
> rtc_add_group just addes given group to dev.groups,
> otherwise it uses devm_device_add_group.
>
> Signed-off-by: Denis Osterland <Denis.Osterland@...hl.com>
> ---
> drivers/rtc/rtc-core.h | 6 ++++++
> drivers/rtc/rtc-sysfs.c | 39 +++++++++++++++++++++++++++++++++++++++
> 2 files changed, 45 insertions(+)
>
> diff --git a/drivers/rtc/rtc-core.h b/drivers/rtc/rtc-core.h
> index 0abf98983e13..81d1c3ce7a96 100644
> --- a/drivers/rtc/rtc-core.h
> +++ b/drivers/rtc/rtc-core.h
> @@ -40,9 +40,15 @@ static inline void rtc_proc_del_device(struct rtc_device *rtc)
>
> #ifdef CONFIG_RTC_INTF_SYSFS
> const struct attribute_group **rtc_get_dev_attribute_groups(void);
> +int rtc_add_group(struct rtc_device *rtc, const struct attribute_group *grp);
> #else
> static inline const struct attribute_group **rtc_get_dev_attribute_groups(void)
> {
> return NULL;
> }
> +static inline
> +int rtc_add_group(struct rtc_device *rtc, const struct attribute_group *grp)
> +{
> + return 0;
> +}
> #endif
> diff --git a/drivers/rtc/rtc-sysfs.c b/drivers/rtc/rtc-sysfs.c
> index 454da38c6012..9a177b8f761b 100644
> --- a/drivers/rtc/rtc-sysfs.c
> +++ b/drivers/rtc/rtc-sysfs.c
> @@ -317,3 +317,42 @@ const struct attribute_group **rtc_get_dev_attribute_groups(void)
> {
> return rtc_attr_groups;
> }
> +
> +static size_t rtc_group_count(struct rtc_device *rtc)
> +{
I don't feel this function is necessary, you can include it in __rtc_add_group
> + const struct attribute_group **groups = rtc->dev.groups;
> + size_t cnt = 0;
> +
> + if (groups)
> + for (; *groups; groups++)
> + cnt++;
> +
> + return cnt;
> +}
> +
> +static inline int
> +__rtc_add_group(struct rtc_device *rtc, const struct attribute_group *grp)
> +{
> + size_t cnt = rtc_group_count(rtc);
> + const struct attribute_group **groups, **old;
> +
> + groups = devm_kzalloc(&rtc->dev, (cnt+2)*sizeof(*groups), GFP_KERNEL);
Please use devm_kcalloc
> + if (IS_ERR_OR_NULL(groups))
> + return PTR_ERR(groups);
> + memcpy(groups, rtc->dev.groups, cnt*sizeof(*groups));
> + groups[cnt] = grp;
> +
> + old = rtc->dev.groups;
> + rtc->dev.groups = groups;
> + if (old != rtc_attr_groups)
> + devm_kfree(&rtc->dev, old);
> +
> + return 0;
> +}
> +
> +int rtc_add_group(struct rtc_device *rtc, const struct attribute_group *grp)
> +{
> + return rtc->dev.kobj.state_in_sysfs ?
> + devm_device_add_group(&rtc->dev, grp) :
> + __rtc_add_group(rtc, grp);
Using devm_device_add_group after RTC registration is racy and should
not be allowed. I would merge __rtc_add_group in rtc_add_group and
return an error if rtc->registered is true.
--
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
Powered by blists - more mailing lists