[PATCH 1/4] RTC: fix double free in rtc_register_device() error path

[PATCH 1/4] RTC: fix double free in rtc_register_device() error path

[Dmitry Torokhov]

Commit 59cca865f21e9e7beab73fcf79ba4eb776a4c228 correctly noted that naked
kfree() should not be used after failed device_register() call, however,
while it added the needed put_device() it forgot to remove the original
kfree() causing double-free.

Cc: Vasiliy Kulikov <segooon@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/rtc/class.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index ea2a315..eb82ec2 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -234,8 +234,9 @@ struct rtc_device *rtc_device_register(const char *name, struct device *dev,
 
 	err = device_register(&rtc->dev);
 	if (err) {
+		/* This will free both memory and the ID */
 		put_device(&rtc->dev);
-		goto exit_kfree;
+		goto exit;
 	}
 
 	rtc_dev_add_device(rtc);
@@ -247,9 +248,6 @@ struct rtc_device *rtc_device_register(const char *name, struct device *dev,
 
 	return rtc;
 
-exit_kfree:
-	kfree(rtc);
-
 exit_ida:
 	ida_simple_remove(&rtc_ida, id);
 
-- 
2.4.3.573.g4eafbef

[PATCH 2/4] RTC: remove unnecessary device_get() in rtc_device_unregister

[Dmitry Torokhov]

Technically the address of rtc->dev can never be NULL, so get_device()
can never fail. Also caller of rtc_device_unregister() supposed to be
the owner of the device and thus have a valid reference. Therefore
call to get_device() is not needed here.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/rtc/class.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index eb82ec2..de7707f 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -266,19 +266,18 @@ EXPORT_SYMBOL_GPL(rtc_device_register);
  */
 void rtc_device_unregister(struct rtc_device *rtc)
 {
-	if (get_device(&rtc->dev) != NULL) {
-		mutex_lock(&rtc->ops_lock);
-		/* remove innards of this RTC, then disable it, before
-		 * letting any rtc_class_open() users access it again
-		 */
-		rtc_sysfs_del_device(rtc);
-		rtc_dev_del_device(rtc);
-		rtc_proc_del_device(rtc);
-		device_unregister(&rtc->dev);
-		rtc->ops = NULL;
-		mutex_unlock(&rtc->ops_lock);
-		put_device(&rtc->dev);
-	}
+	mutex_lock(&rtc->ops_lock);
+	/*
+	 * Remove innards of this RTC, then disable it, before
+	 * letting any rtc_class_open() users access it again
+	 */
+	rtc_sysfs_del_device(rtc);
+	rtc_dev_del_device(rtc);
+	rtc_proc_del_device(rtc);
+	device_del(&rtc->dev);
+	rtc->ops = NULL;
+	mutex_unlock(&rtc->ops_lock);
+	put_device(&rtc->dev);
 }
 EXPORT_SYMBOL_GPL(rtc_device_unregister);
 
-- 
2.4.3.573.g4eafbef

[PATCH 3/4] RTC: properly manage lifetime of dev and cdev in rtc device

[Dmitry Torokhov]

struct rtc embeds both struct dev and struct cdev.  Unfortunately character
device structure may outlive the parent rtc structure unless we set it up
as parent of character device so that it will stay pinned until character
device is freed.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/rtc/rtc-dev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c
index 799c34b..a6d9434 100644
--- a/drivers/rtc/rtc-dev.c
+++ b/drivers/rtc/rtc-dev.c
@@ -477,6 +477,7 @@ void rtc_dev_prepare(struct rtc_device *rtc)
 
 	cdev_init(&rtc->char_dev, &rtc_dev_fops);
 	rtc->char_dev.owner = rtc->owner;
+	rtc->char_dev.kobj.parent = &rtc->dev.kobj;
 }
 
 void rtc_dev_add_device(struct rtc_device *rtc)
-- 
2.4.3.573.g4eafbef

[PATCH 4/4] RTC: switch to using is_visible() to control sysfs attributes

[Dmitry Torokhov]

Instead of creating an attribute manually, after the device has been
registered, let's rely on facilities provided by the attribute groups to
control which attributes are visible and which are not. This allows to to
create all needed attributes at once, at the same time that we register
RTC class device.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/rtc/class.c     |  4 +--
 drivers/rtc/rtc-core.h  | 19 ++-----------
 drivers/rtc/rtc-sysfs.c | 75 +++++++++++++++++++++++++------------------------
 3 files changed, 42 insertions(+), 56 deletions(-)

diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index de7707f..de86578 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -202,6 +202,7 @@ struct rtc_device *rtc_device_register(const char *name, struct device *dev,
 	rtc->max_user_freq = 64;
 	rtc->dev.parent = dev;
 	rtc->dev.class = rtc_class;
+	rtc->dev.groups = rtc_get_dev_attribute_groups();
 	rtc->dev.release = rtc_device_release;
 
 	mutex_init(&rtc->ops_lock);
@@ -240,7 +241,6 @@ struct rtc_device *rtc_device_register(const char *name, struct device *dev,
 	}
 
 	rtc_dev_add_device(rtc);
-	rtc_sysfs_add_device(rtc);
 	rtc_proc_add_device(rtc);
 
 	dev_info(dev, "rtc core: registered %s as %s\n",
@@ -271,7 +271,6 @@ void rtc_device_unregister(struct rtc_device *rtc)
 	 * Remove innards of this RTC, then disable it, before
 	 * letting any rtc_class_open() users access it again
 	 */
-	rtc_sysfs_del_device(rtc);
 	rtc_dev_del_device(rtc);
 	rtc_proc_del_device(rtc);
 	device_del(&rtc->dev);
@@ -360,7 +359,6 @@ static int __init rtc_init(void)
 	}
 	rtc_class->pm = RTC_CLASS_DEV_PM_OPS;
 	rtc_dev_init();
-	rtc_sysfs_init(rtc_class);
 	return 0;
 }
 
diff --git a/drivers/rtc/rtc-core.h b/drivers/rtc/rtc-core.h
index 5f9df74..a098aea 100644
--- a/drivers/rtc/rtc-core.h
+++ b/drivers/rtc/rtc-core.h
@@ -48,23 +48,10 @@ static inline void rtc_proc_del_device(struct rtc_device *rtc)
 #endif
 
 #ifdef CONFIG_RTC_INTF_SYSFS
-
-extern void __init rtc_sysfs_init(struct class *);
-extern void rtc_sysfs_add_device(struct rtc_device *rtc);
-extern void rtc_sysfs_del_device(struct rtc_device *rtc);
-
+const struct attribute_group **rtc_get_dev_attribute_groups(void);
 #else
-
-static inline void rtc_sysfs_init(struct class *rtc)
-{
-}
-
-static inline void rtc_sysfs_add_device(struct rtc_device *rtc)
+static inline const struct attribute_group **rtc_get_dev_attribute_groups(void)
 {
+	return NULL;
 }
-
-static inline void rtc_sysfs_del_device(struct rtc_device *rtc)
-{
-}
-
 #endif
diff --git a/drivers/rtc/rtc-sysfs.c b/drivers/rtc/rtc-sysfs.c
index babd43b..0b4366c 100644
--- a/drivers/rtc/rtc-sysfs.c
+++ b/drivers/rtc/rtc-sysfs.c
@@ -122,20 +122,8 @@ hctosys_show(struct device *dev, struct device_attribute *attr, char *buf)
 }
 static DEVICE_ATTR_RO(hctosys);
 
-static struct attribute *rtc_attrs[] = {
-	&dev_attr_name.attr,
-	&dev_attr_date.attr,
-	&dev_attr_time.attr,
-	&dev_attr_since_epoch.attr,
-	&dev_attr_max_user_freq.attr,
-	&dev_attr_hctosys.attr,
-	NULL,
-};
-ATTRIBUTE_GROUPS(rtc);
-
 static ssize_t
-rtc_sysfs_show_wakealarm(struct device *dev, struct device_attribute *attr,
-		char *buf)
+wakealarm_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	ssize_t retval;
 	unsigned long alarm;
@@ -159,7 +147,7 @@ rtc_sysfs_show_wakealarm(struct device *dev, struct device_attribute *attr,
 }
 
 static ssize_t
-rtc_sysfs_set_wakealarm(struct device *dev, struct device_attribute *attr,
+wakealarm_store(struct device *dev, struct device_attribute *attr,
 		const char *buf, size_t n)
 {
 	ssize_t retval;
@@ -221,45 +209,58 @@ rtc_sysfs_set_wakealarm(struct device *dev, struct device_attribute *attr,
 	retval = rtc_set_alarm(rtc, &alm);
 	return (retval < 0) ? retval : n;
 }
-static DEVICE_ATTR(wakealarm, S_IRUGO | S_IWUSR,
-		rtc_sysfs_show_wakealarm, rtc_sysfs_set_wakealarm);
+static DEVICE_ATTR_RW(wakealarm);
 
+static struct attribute *rtc_attrs[] = {
+	&dev_attr_name.attr,
+	&dev_attr_date.attr,
+	&dev_attr_time.attr,
+	&dev_attr_since_epoch.attr,
+	&dev_attr_max_user_freq.attr,
+	&dev_attr_hctosys.attr,
+	&dev_attr_wakealarm.attr,
+	NULL,
+};
 
-/* The reason to trigger an alarm with no process watching it (via sysfs)
+/*
+ * The reason to trigger an alarm with no process watching it (via sysfs)
  * is its side effect:  waking from a system state like suspend-to-RAM or
  * suspend-to-disk.  So: no attribute unless that side effect is possible.
  * (Userspace may disable that mechanism later.)
  */
-static inline int rtc_does_wakealarm(struct rtc_device *rtc)
+static bool rtc_does_wakealarm(struct rtc_device *rtc)
 {
 	if (!device_can_wakeup(rtc->dev.parent))
-		return 0;
+		return false;
+
 	return rtc->ops->set_alarm != NULL;
 }
 
-
-void rtc_sysfs_add_device(struct rtc_device *rtc)
+static umode_t rtc_attr_is_visible(struct kobject *kobj,
+				   struct attribute *attr, int n)
 {
-	int err;
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct rtc_device *rtc = to_rtc_device(dev);
+	umode_t mode = attr->mode;
 
-	/* not all RTCs support both alarms and wakeup */
-	if (!rtc_does_wakealarm(rtc))
-		return;
+	if (attr == &dev_attr_wakealarm.attr)
+		if (!rtc_does_wakealarm(rtc))
+			mode = 0;
 
-	err = device_create_file(&rtc->dev, &dev_attr_wakealarm);
-	if (err)
-		dev_err(rtc->dev.parent,
-			"failed to create alarm attribute, %d\n", err);
+	return mode;
 }
 
-void rtc_sysfs_del_device(struct rtc_device *rtc)
-{
-	/* REVISIT did we add it successfully? */
-	if (rtc_does_wakealarm(rtc))
-		device_remove_file(&rtc->dev, &dev_attr_wakealarm);
-}
+static struct attribute_group rtc_attr_group = {
+	.is_visible	= rtc_attr_is_visible,
+	.attrs		= rtc_attrs,
+};
+
+static const struct attribute_group *rtc_attr_groups[] = {
+	&rtc_attr_group,
+	NULL
+};
 
-void __init rtc_sysfs_init(struct class *rtc_class)
+const struct attribute_group** rtc_get_dev_attribute_groups(void)
 {
-	rtc_class->dev_groups = rtc_groups;
+	return rtc_attr_groups;
 }
-- 
2.4.3.573.g4eafbef

Re: [rtc-linux] [PATCH 1/4] RTC: fix double free in rtc_register_device() error path

[Krzysztof Kozlowski]

2015-07-21 8:02 GMT+09:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> Commit 59cca865f21e9e7beab73fcf79ba4eb776a4c228 correctly noted that naked
> kfree() should not be used after failed device_register() call, however,
> while it added the needed put_device() it forgot to remove the original
> kfree() causing double-free.
>
> Cc: Vasiliy Kulikov <segooon@gmail.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/rtc/class.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)

I think it can be cc'ed stable (with fixes tag).

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

Re: [rtc-linux] [PATCH 2/4] RTC: remove unnecessary device_get() in rtc_device_unregister

[Krzysztof Kozlowski]

2015-07-21 8:02 GMT+09:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> Technically the address of rtc->dev can never be NULL, so get_device()
> can never fail. Also caller of rtc_device_unregister() supposed to be
> the owner of the device and thus have a valid reference. Therefore
> call to get_device() is not needed here.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/rtc/class.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)

Makes sense.
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

Re: [rtc-linux] [PATCH 1/4] RTC: fix double free in rtc_register_device() error path

[Dmitry Torokhov]

On Mon, Jul 20, 2015 at 5:32 PM, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> 2015-07-21 8:02 GMT+09:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
>> Commit 59cca865f21e9e7beab73fcf79ba4eb776a4c228 correctly noted that naked
>> kfree() should not be used after failed device_register() call, however,
>> while it added the needed put_device() it forgot to remove the original
>> kfree() causing double-free.
>>
>> Cc: Vasiliy Kulikov <segooon@gmail.com>
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> ---
>>  drivers/rtc/class.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> I think it can be cc'ed stable (with fixes tag).

I considered that, but it does not happen on normally working systems.
I'll leave it up to maintainers to annotate as they see fit.

Thanks.

-- 
Dmitry

Re: [rtc-linux] [PATCH 3/4] RTC: properly manage lifetime of dev and cdev in rtc device

[Krzysztof Kozlowski]

2015-07-21 8:02 GMT+09:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> struct rtc embeds both struct dev and struct cdev.  Unfortunately character
> device structure may outlive the parent rtc structure unless we set it up
> as parent of character device so that it will stay pinned until character
> device is freed.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/rtc/rtc-dev.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

Re: [rtc-linux] [PATCH 4/4] RTC: switch to using is_visible() to control sysfs attributes

[Krzysztof Kozlowski]

2015-07-21 8:02 GMT+09:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> Instead of creating an attribute manually, after the device has been
> registered, let's rely on facilities provided by the attribute groups to
> control which attributes are visible and which are not. This allows to to
> create all needed attributes at once, at the same time that we register
> RTC class device.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/rtc/class.c     |  4 +--
>  drivers/rtc/rtc-core.h  | 19 ++-----------
>  drivers/rtc/rtc-sysfs.c | 75 +++++++++++++++++++++++++------------------------
>  3 files changed, 42 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
> index de7707f..de86578 100644
> --- a/drivers/rtc/class.c
> +++ b/drivers/rtc/class.c
> @@ -202,6 +202,7 @@ struct rtc_device *rtc_device_register(const char *name, struct device *dev,
>         rtc->max_user_freq = 64;
>         rtc->dev.parent = dev;
>         rtc->dev.class = rtc_class;
> +       rtc->dev.groups = rtc_get_dev_attribute_groups();
>         rtc->dev.release = rtc_device_release;
>
>         mutex_init(&rtc->ops_lock);
> @@ -240,7 +241,6 @@ struct rtc_device *rtc_device_register(const char *name, struct device *dev,
>         }
>
>         rtc_dev_add_device(rtc);
> -       rtc_sysfs_add_device(rtc);
>         rtc_proc_add_device(rtc);
>
>         dev_info(dev, "rtc core: registered %s as %s\n",
> @@ -271,7 +271,6 @@ void rtc_device_unregister(struct rtc_device *rtc)
>          * Remove innards of this RTC, then disable it, before
>          * letting any rtc_class_open() users access it again
>          */
> -       rtc_sysfs_del_device(rtc);
>         rtc_dev_del_device(rtc);
>         rtc_proc_del_device(rtc);
>         device_del(&rtc->dev);
> @@ -360,7 +359,6 @@ static int __init rtc_init(void)
>         }
>         rtc_class->pm = RTC_CLASS_DEV_PM_OPS;
>         rtc_dev_init();
> -       rtc_sysfs_init(rtc_class);
>         return 0;
>  }
>
> diff --git a/drivers/rtc/rtc-core.h b/drivers/rtc/rtc-core.h
> index 5f9df74..a098aea 100644
> --- a/drivers/rtc/rtc-core.h
> +++ b/drivers/rtc/rtc-core.h
> @@ -48,23 +48,10 @@ static inline void rtc_proc_del_device(struct rtc_device *rtc)
>  #endif
>
>  #ifdef CONFIG_RTC_INTF_SYSFS
> -
> -extern void __init rtc_sysfs_init(struct class *);
> -extern void rtc_sysfs_add_device(struct rtc_device *rtc);
> -extern void rtc_sysfs_del_device(struct rtc_device *rtc);
> -
> +const struct attribute_group **rtc_get_dev_attribute_groups(void);
>  #else
> -
> -static inline void rtc_sysfs_init(struct class *rtc)
> -{
> -}
> -
> -static inline void rtc_sysfs_add_device(struct rtc_device *rtc)
> +static inline const struct attribute_group **rtc_get_dev_attribute_groups(void)
>  {
> +       return NULL;
>  }
> -
> -static inline void rtc_sysfs_del_device(struct rtc_device *rtc)
> -{
> -}
> -
>  #endif
> diff --git a/drivers/rtc/rtc-sysfs.c b/drivers/rtc/rtc-sysfs.c
> index babd43b..0b4366c 100644
> --- a/drivers/rtc/rtc-sysfs.c
> +++ b/drivers/rtc/rtc-sysfs.c
> @@ -122,20 +122,8 @@ hctosys_show(struct device *dev, struct device_attribute *attr, char *buf)
>  }
>  static DEVICE_ATTR_RO(hctosys);
>
> -static struct attribute *rtc_attrs[] = {
> -       &dev_attr_name.attr,
> -       &dev_attr_date.attr,
> -       &dev_attr_time.attr,
> -       &dev_attr_since_epoch.attr,
> -       &dev_attr_max_user_freq.attr,
> -       &dev_attr_hctosys.attr,
> -       NULL,
> -};
> -ATTRIBUTE_GROUPS(rtc);
> -
>  static ssize_t
> -rtc_sysfs_show_wakealarm(struct device *dev, struct device_attribute *attr,
> -               char *buf)
> +wakealarm_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
>         ssize_t retval;
>         unsigned long alarm;
> @@ -159,7 +147,7 @@ rtc_sysfs_show_wakealarm(struct device *dev, struct device_attribute *attr,
>  }
>
>  static ssize_t
> -rtc_sysfs_set_wakealarm(struct device *dev, struct device_attribute *attr,
> +wakealarm_store(struct device *dev, struct device_attribute *attr,
>                 const char *buf, size_t n)
>  {
>         ssize_t retval;
> @@ -221,45 +209,58 @@ rtc_sysfs_set_wakealarm(struct device *dev, struct device_attribute *attr,
>         retval = rtc_set_alarm(rtc, &alm);
>         return (retval < 0) ? retval : n;
>  }
> -static DEVICE_ATTR(wakealarm, S_IRUGO | S_IWUSR,
> -               rtc_sysfs_show_wakealarm, rtc_sysfs_set_wakealarm);
> +static DEVICE_ATTR_RW(wakealarm);

This and renaming of show/store functions look unrelated

>
> +static struct attribute *rtc_attrs[] = {
> +       &dev_attr_name.attr,
> +       &dev_attr_date.attr,
> +       &dev_attr_time.attr,
> +       &dev_attr_since_epoch.attr,
> +       &dev_attr_max_user_freq.attr,
> +       &dev_attr_hctosys.attr,
> +       &dev_attr_wakealarm.attr,
> +       NULL,
> +};
>
> -/* The reason to trigger an alarm with no process watching it (via sysfs)
> +/*
> + * The reason to trigger an alarm with no process watching it (via sysfs)
>   * is its side effect:  waking from a system state like suspend-to-RAM or
>   * suspend-to-disk.  So: no attribute unless that side effect is possible.
>   * (Userspace may disable that mechanism later.)
>   */
> -static inline int rtc_does_wakealarm(struct rtc_device *rtc)
> +static bool rtc_does_wakealarm(struct rtc_device *rtc)
>  {
>         if (!device_can_wakeup(rtc->dev.parent))
> -               return 0;
> +               return false;
> +
>         return rtc->ops->set_alarm != NULL;
>  }

This looks unrelated too and confuses me. Could you split such cleanup
from main goal of the patch?

Best regards,
Krzysztof

Re: [PATCH 1/4] RTC: fix double free in rtc_register_device() error path

[Alexandre Belloni]

On 20/07/2015 at 16:02:49 -0700, Dmitry Torokhov wrote :
> Commit 59cca865f21e9e7beab73fcf79ba4eb776a4c228 correctly noted that naked
> kfree() should not be used after failed device_register() call, however,
> while it added the needed put_device() it forgot to remove the original
> kfree() causing double-free.
> 
> Cc: Vasiliy Kulikov <segooon@gmail.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/rtc/class.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
Applied after fixing the commit description in the commit log, thanks.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 2/4] RTC: remove unnecessary device_get() in rtc_device_unregister

[Alexandre Belloni]

On 20/07/2015 at 16:02:50 -0700, Dmitry Torokhov wrote :
> Technically the address of rtc->dev can never be NULL, so get_device()
> can never fail. Also caller of rtc_device_unregister() supposed to be
> the owner of the device and thus have a valid reference. Therefore
> call to get_device() is not needed here.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/rtc/class.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
Applied, thanks.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 3/4] RTC: properly manage lifetime of dev and cdev in rtc device

[Alexandre Belloni]

On 20/07/2015 at 16:02:51 -0700, Dmitry Torokhov wrote :
> struct rtc embeds both struct dev and struct cdev.  Unfortunately character
> device structure may outlive the parent rtc structure unless we set it up
> as parent of character device so that it will stay pinned until character
> device is freed.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/rtc/rtc-dev.c | 1 +
>  1 file changed, 1 insertion(+)
Applied, thanks.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 4/4] RTC: switch to using is_visible() to control sysfs attributes

[Alexandre Belloni]

Hi,

Seems mostly good, a few comments on top of Krzysztof's below:

On 20/07/2015 at 16:02:52 -0700, Dmitry Torokhov wrote :
> Instead of creating an attribute manually, after the device has been
> registered, let's rely on facilities provided by the attribute groups to
> control which attributes are visible and which are not. This allows to to
too many 'to' here  --------------------------------------------------^

[...]

> -void __init rtc_sysfs_init(struct class *rtc_class)
> +const struct attribute_group** rtc_get_dev_attribute_groups(void)
That space is not properly placed ^, this should be:
const struct attribute_group **rtc_get_dev_attribute_groups(void)


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [rtc-linux] [PATCH 4/4] RTC: switch to using is_visible() to control sysfs attributes

[Alexandre Belloni]

(Krzysztof, be careful, Dmitry was not in copy of your maili, you should
probably check your mailer config)

On 21/07/2015 at 10:21:11 +0900, Krzysztof Kozlowski wrote :
> 2015-07-21 8:02 GMT+09:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> >  static ssize_t
> > -rtc_sysfs_set_wakealarm(struct device *dev, struct device_attribute *attr,
> > +wakealarm_store(struct device *dev, struct device_attribute *attr,
> >                 const char *buf, size_t n)
> >  {
> >         ssize_t retval;
> > @@ -221,45 +209,58 @@ rtc_sysfs_set_wakealarm(struct device *dev, struct device_attribute *attr,
> >         retval = rtc_set_alarm(rtc, &alm);
> >         return (retval < 0) ? retval : n;
> >  }
> > -static DEVICE_ATTR(wakealarm, S_IRUGO | S_IWUSR,
> > -               rtc_sysfs_show_wakealarm, rtc_sysfs_set_wakealarm);
> > +static DEVICE_ATTR_RW(wakealarm);
> 
> This and renaming of show/store functions look unrelated
> 

I don't really mind that one but I would also prefer if it could be
separated.

> >
> > +static struct attribute *rtc_attrs[] = {
> > +       &dev_attr_name.attr,
> > +       &dev_attr_date.attr,
> > +       &dev_attr_time.attr,
> > +       &dev_attr_since_epoch.attr,
> > +       &dev_attr_max_user_freq.attr,
> > +       &dev_attr_hctosys.attr,
> > +       &dev_attr_wakealarm.attr,
> > +       NULL,
> > +};
> >
> > -/* The reason to trigger an alarm with no process watching it (via sysfs)
> > +/*
> > + * The reason to trigger an alarm with no process watching it (via sysfs)
> >   * is its side effect:  waking from a system state like suspend-to-RAM or
> >   * suspend-to-disk.  So: no attribute unless that side effect is possible.
> >   * (Userspace may disable that mechanism later.)
> >   */
> > -static inline int rtc_does_wakealarm(struct rtc_device *rtc)
> > +static bool rtc_does_wakealarm(struct rtc_device *rtc)
> >  {
> >         if (!device_can_wakeup(rtc->dev.parent))
> > -               return 0;
> > +               return false;
> > +
> >         return rtc->ops->set_alarm != NULL;
> >  }
> 
> This looks unrelated too and confuses me. Could you split such cleanup
> from main goal of the patch?
> 

That one is bothering me too.



-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [rtc-linux] [PATCH 4/4] RTC: switch to using is_visible() to control sysfs attributes

[Dmitry Torokhov]

On Wed, Jul 22, 2015 at 10:57:35PM +0200, Alexandre Belloni wrote:
> (Krzysztof, be careful, Dmitry was not in copy of your maili, you should
> probably check your mailer config)
> 
> On 21/07/2015 at 10:21:11 +0900, Krzysztof Kozlowski wrote :
> > 2015-07-21 8:02 GMT+09:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> > >  static ssize_t
> > > -rtc_sysfs_set_wakealarm(struct device *dev, struct device_attribute *attr,
> > > +wakealarm_store(struct device *dev, struct device_attribute *attr,
> > >                 const char *buf, size_t n)
> > >  {
> > >         ssize_t retval;
> > > @@ -221,45 +209,58 @@ rtc_sysfs_set_wakealarm(struct device *dev, struct device_attribute *attr,
> > >         retval = rtc_set_alarm(rtc, &alm);
> > >         return (retval < 0) ? retval : n;
> > >  }
> > > -static DEVICE_ATTR(wakealarm, S_IRUGO | S_IWUSR,
> > > -               rtc_sysfs_show_wakealarm, rtc_sysfs_set_wakealarm);
> > > +static DEVICE_ATTR_RW(wakealarm);
> > 
> > This and renaming of show/store functions look unrelated
> > 
> 
> I don't really mind that one but I would also prefer if it could be
> separated.

OK, I will.

> 
> > >
> > > +static struct attribute *rtc_attrs[] = {
> > > +       &dev_attr_name.attr,
> > > +       &dev_attr_date.attr,
> > > +       &dev_attr_time.attr,
> > > +       &dev_attr_since_epoch.attr,
> > > +       &dev_attr_max_user_freq.attr,
> > > +       &dev_attr_hctosys.attr,
> > > +       &dev_attr_wakealarm.attr,
> > > +       NULL,
> > > +};
> > >
> > > -/* The reason to trigger an alarm with no process watching it (via sysfs)
> > > +/*
> > > + * The reason to trigger an alarm with no process watching it (via sysfs)
> > >   * is its side effect:  waking from a system state like suspend-to-RAM or
> > >   * suspend-to-disk.  So: no attribute unless that side effect is possible.
> > >   * (Userspace may disable that mechanism later.)
> > >   */
> > > -static inline int rtc_does_wakealarm(struct rtc_device *rtc)
> > > +static bool rtc_does_wakealarm(struct rtc_device *rtc)
> > >  {
> > >         if (!device_can_wakeup(rtc->dev.parent))
> > > -               return 0;
> > > +               return false;
> > > +
> > >         return rtc->ops->set_alarm != NULL;
> > >  }
> > 
> > This looks unrelated too and confuses me. Could you split such cleanup
> > from main goal of the patch?
> > 
> 
> That one is bothering me too.

Will separate this too.

Thanks.

-- 
Dmitry

Re: [rtc-linux] [PATCH 4/4] RTC: switch to using is_visible() to control sysfs attributes

[Krzysztof Kozlowski]

2015-07-23 5:57 GMT+09:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> (Krzysztof, be careful, Dmitry was not in copy of your maili, you should
> probably check your mailer config)

Right, I need to fix my Google group settings. Thank you for pointing
this. Probably my other emails also were not sent to Dmitry...

Best regards,
Krzysztof