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

Message ID 20180710090710.9066-2-Denis.Osterland@diehl.com
State Superseded
Headers show
Series
  • rtc: isl1208: fixes, documentation and isl1219 support
Related show

Commit Message

Denis OSTERLAND July 10, 2018, 9:44 a.m.
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(+)

Comments

Alexandre Belloni July 18, 2018, 7:25 a.m. | #1
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.
Denis OSTERLAND July 18, 2018, 7:37 a.m. | #2
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

Patch

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)
+{
+	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);
+	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);
+}