[v4,1/5] rtc: sysfs: facilitate attribute add to rtc device

From: Denis Osterland <Denis.Osterland@diehl.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@diehl.com>
---
 drivers/rtc/rtc-core.h  |  6 ++++++
 drivers/rtc/rtc-sysfs.c | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

Hello,

On 10/07/2018 09:44:15+0000, Denis OSTERLAND wrote:
> From: Denis Osterland <Denis.Osterland@diehl.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@diehl.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.

Hello,

thanks for your comments.

Am Mittwoch, den 18.07.2018, 09:25 +0200 schrieb Alexandre Belloni:
> Hello,
> 
> On 10/07/2018 09:44:15+0000, Denis OSTERLAND wrote:
> > 
> > +
> > +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
I think that it is easier to read, but sure I can include it.
> 
> > 
> > +
> > +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
okay
> 
> > 
> > +	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.
You are right. An error in this case is better.
> 
> 

Regards Denis

Diehl Connectivity Solutions GmbH
Geschäftsführung: Horst Leonberger
Sitz der Gesellschaft: Nürnberg - Registergericht: Amtsgericht
Nürnberg: HRB 32315