Hello,
On 10/07/2018 09:44:15+0000, Denis OSTERLAND wrote:
> From: Denis Osterland <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
