[PATCH 9-10/10] Allow subsystems to avoid using sysdevs for defining "core" PM callbacks

[PATCH 9-10/10] Allow subsystems to avoid using sysdevs for defining "core" PM callbacks

[Rafael J. Wysocki]

On Saturday, March 12, 2011, Rafael J. Wysocki wrote:
> Hi,
> 
> On Thursday, March 10, 2011, Rafael J. Wysocki wrote:
> > There are multiple problems with sysdevs, or struct sys_device objects to
> > be precise, that are so annoying that some people have started to think
> > of removind them entirely from the kernel.  To me, personally, the most
> > obvious issue is the way sysdevs are used for defining suspend/resume
> > callbacks to be executed with one CPU on-line and interrupts disabled.
> > Greg and Kay may tell you more about the other problems with sysdevs. :-)
> > 
> > Some subsystems need to carry out certain operations during suspend after
> > we've disabled non-boot CPUs and interrupts have been switched off on the
> > only on-line one.  Currently, the only way to achieve that is to define
> > sysdev suspend/resume callbacks, but this is cumbersome and inefficient.
> > Namely, to do that, one has to define a sysdev class providing the callbacks
> > and a sysdev actually using them, which is excessively complicated.  Moreover,
> > the sysdev suspend/resume callbacks take arguments that are not really used
> > by the majority of subsystems defining sysdev suspend/resume callbacks
> > (or even if they are used, they don't really _need_ to be used, so they
> > are simply unnecessary).  Of course, if a sysdev is only defined to provide
> > suspend/resume (and maybe shutdown) callbacks, there's no real reason why
> > it should show up in sysfs.
> > 
> > For this reason, I thought it would be a good idea to provide a simpler
> > interface for subsystems to define "very late" suspend callbacks and
> > "very early" resume callbacks (and "very late" shutdown callbacks as well)
> > without the entire bloat related to sysdevs.  The interface is introduced
> > by the first of the following patches, while the second patch converts some
> > sysdev users related to the x86 architecture to using the new interface.
> > 
> > I believe that call sysdev users who need to define suspend/resume/shutdown
> > callbacks may be converted to using the interface provided by the first patch,
> > which in turn should allow us to convert the remaining sysdev functionality
> > into "normal" struct device interfaces.  Still, even if that turns out to be
> > too complicated, the bloat reduction resulting from the second patch kind of
> > shows that moving at least some sysdev users to a simpler interface (like in
> > the first patch) is a good idea anyway.
> > 
> > This is a proof of concept, so the patches have not been tested.  Please be
> > extrememly careful, because they touch sensitive code, so to speak.  In the
> > majority of cases the changes are rather straightforward, but there are some
> > more interesting cases as well (io_apic.c most importantly).
> 
> Since Greg likes the idea and there haven't been any objections so far, here's
> the official submission.  The patches have been tested on HP nx6325 and
> Toshiba Portege R500.
> 
> Patch [1/8] is regareded as 2.6.38 material, following Greg's advice.  The
> other patches in the set are regarded as 2.6.39 material.  The last one
> obviously depends on all of the previous ones.
> 
> [1/8] - Introduce struct syscore_ops for registering operations to be run on
>         one CPU during suspend/resume/shutdown.
> 
> [2/8] - Convert sysdev users in arch/x86 to using struct syscore_ops.
> 
> [3/8] - Make ACPI use struct syscore_ops for irqrouter_resume().
> 
> [4/8] - Make timekeeping use struct syscore_ops for suspend/resume.
> 
> [5/8] - Make Intel IOMMU use struct syscore_ops for suspend/resume.
> 
> [6/8] - Make KVM use struct syscore_ops for suspend/resume.
> 
> [7/8] - Make cpufreq use struct syscore_ops for boot CPU suspend/resume.
> 
> [8/8] - Introduce config switch allowing architectures to skip sysdev
>         suspend/resume/shutdown code.
> 
> If there are no objectsions, I'd like to push these patches through the suspend
> tree.

A little followup with two ARM-related patches.

[9/10] - Make sh drivers use struct syscore_ops for suspend/resume (instead of
         sysdevs).
[10/10] - Use struct syscore_ops for suspend/resume (instead of sysdevs) in
          core ARM code.

Thanks,
Rafael

[PATCH 9/10] sh: Use struct syscore_ops instead of sysdev class and sysdev

[R. J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Convert the SuperH clocks framework and shared interrupt handling
code to using struct syscore_ops instead of a sysdev classes and
sysdevs for power managment.

This reduces the code size significantly and simplifies it.  The
optimizations causing things not to be restored after creating a
hibernation image are removed, but they might lead to undesirable
effects during resume from hibernation (e.g. the clocks would be left
as the boot kernel set them, which might be not the same way as the
hibernated kernel had seen them before the hibernation).

This also is necessary for removing sysdevs from the kernel entirely
in the future.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/sh/clk/core.c       |   68 +++++++--------------------
 drivers/sh/intc/core.c      |  108 ++++++++++++++------------------------------
 drivers/sh/intc/internals.h |    2 
 3 files changed, 53 insertions(+), 125 deletions(-)

Index: linux-2.6/drivers/sh/clk/core.c
=================================--- linux-2.6.orig/drivers/sh/clk/core.c
+++ linux-2.6/drivers/sh/clk/core.c
@@ -21,7 +21,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/list.h>
-#include <linux/sysdev.h>
+#include <linux/syscore_ops.h>
 #include <linux/seq_file.h>
 #include <linux/err.h>
 #include <linux/io.h>
@@ -630,68 +630,36 @@ long clk_round_parent(struct clk *clk, u
 EXPORT_SYMBOL_GPL(clk_round_parent);
 
 #ifdef CONFIG_PM
-static int clks_sysdev_suspend(struct sys_device *dev, pm_message_t state)
+static void clks_core_resume(void)
 {
-	static pm_message_t prev_state;
 	struct clk *clkp;
 
-	switch (state.event) {
-	case PM_EVENT_ON:
-		/* Resumeing from hibernation */
-		if (prev_state.event != PM_EVENT_FREEZE)
-			break;
-
-		list_for_each_entry(clkp, &clock_list, node) {
-			if (likely(clkp->ops)) {
-				unsigned long rate = clkp->rate;
-
-				if (likely(clkp->ops->set_parent))
-					clkp->ops->set_parent(clkp,
-						clkp->parent);
-				if (likely(clkp->ops->set_rate))
-					clkp->ops->set_rate(clkp, rate);
-				else if (likely(clkp->ops->recalc))
-					clkp->rate = clkp->ops->recalc(clkp);
-			}
+	list_for_each_entry(clkp, &clock_list, node) {
+		if (likely(clkp->ops)) {
+			unsigned long rate = clkp->rate;
+
+			if (likely(clkp->ops->set_parent))
+				clkp->ops->set_parent(clkp,
+					clkp->parent);
+			if (likely(clkp->ops->set_rate))
+				clkp->ops->set_rate(clkp, rate);
+			else if (likely(clkp->ops->recalc))
+				clkp->rate = clkp->ops->recalc(clkp);
 		}
-		break;
-	case PM_EVENT_FREEZE:
-		break;
-	case PM_EVENT_SUSPEND:
-		break;
 	}
-
-	prev_state = state;
-	return 0;
-}
-
-static int clks_sysdev_resume(struct sys_device *dev)
-{
-	return clks_sysdev_suspend(dev, PMSG_ON);
 }
 
-static struct sysdev_class clks_sysdev_class = {
-	.name = "clks",
-};
-
-static struct sysdev_driver clks_sysdev_driver = {
-	.suspend = clks_sysdev_suspend,
-	.resume = clks_sysdev_resume,
-};
-
-static struct sys_device clks_sysdev_dev = {
-	.cls = &clks_sysdev_class,
+static struct syscore_ops clks_syscore_ops = {
+	.resume = clks_core_resume,
 };
 
-static int __init clk_sysdev_init(void)
+static int __init clk_syscore_init(void)
 {
-	sysdev_class_register(&clks_sysdev_class);
-	sysdev_driver_register(&clks_sysdev_class, &clks_sysdev_driver);
-	sysdev_register(&clks_sysdev_dev);
+	register_syscore_ops(&clks_syscore_ops);
 
 	return 0;
 }
-subsys_initcall(clk_sysdev_init);
+subsys_initcall(clk_syscore_init);
 #endif
 
 /*
Index: linux-2.6/drivers/sh/intc/core.c
=================================--- linux-2.6.orig/drivers/sh/intc/core.c
+++ linux-2.6/drivers/sh/intc/core.c
@@ -24,7 +24,7 @@
 #include <linux/slab.h>
 #include <linux/interrupt.h>
 #include <linux/sh_intc.h>
-#include <linux/sysdev.h>
+#include <linux/syscore_ops.h>
 #include <linux/list.h>
 #include <linux/spinlock.h>
 #include <linux/radix-tree.h>
@@ -376,108 +376,70 @@ err0:
 	return -ENOMEM;
 }
 
-static ssize_t
-show_intc_name(struct sys_device *dev, struct sysdev_attribute *attr, char *buf)
+static int intc_suspend(void)
 {
 	struct intc_desc_int *d;
 
-	d = container_of(dev, struct intc_desc_int, sysdev);
+	list_for_each_entry(d, &intc_list, list) {
+		int irq;
 
-	return sprintf(buf, "%s\n", d->chip.name);
-}
+		/* enable wakeup irqs belonging to this intc controller */
+		for_each_active_irq(irq) {
+			struct irq_data *data;
+			struct irq_desc *desc;
+			struct irq_chip *chip;
+
+			data = irq_get_irq_data(irq);
+			chip = irq_data_get_irq_chip(data);
+			if (chip != &d->chip)
+				continue;
+			desc = irq_to_desc(irq);
+			if ((desc->status & IRQ_WAKEUP))
+				chip->irq_enable(data);
+		}
+	}
 
-static SYSDEV_ATTR(name, S_IRUGO, show_intc_name, NULL);
+	return 0;
+}
 
-static int intc_suspend(struct sys_device *dev, pm_message_t state)
+static void intc_resume(void)
 {
 	struct intc_desc_int *d;
-	struct irq_data *data;
-	struct irq_desc *desc;
-	struct irq_chip *chip;
-	int irq;
-
-	/* get intc controller associated with this sysdev */
-	d = container_of(dev, struct intc_desc_int, sysdev);
-
-	switch (state.event) {
-	case PM_EVENT_ON:
-		if (d->state.event != PM_EVENT_FREEZE)
-			break;
+
+	list_for_each_entry(d, &intc_list, list) {
+		int irq;
 
 		for_each_active_irq(irq) {
-			desc = irq_to_desc(irq);
+			struct irq_data *data;
+			struct irq_desc *desc;
+			struct irq_chip *chip;
+
 			data = irq_get_irq_data(irq);
 			chip = irq_data_get_irq_chip(data);
-
 			/*
 			 * This will catch the redirect and VIRQ cases
 			 * due to the dummy_irq_chip being inserted.
 			 */
 			if (chip != &d->chip)
 				continue;
+			desc = irq_to_desc(irq);
 			if (desc->status & IRQ_DISABLED)
 				chip->irq_disable(data);
 			else
 				chip->irq_enable(data);
 		}
-		break;
-	case PM_EVENT_FREEZE:
-		/* nothing has to be done */
-		break;
-	case PM_EVENT_SUSPEND:
-		/* enable wakeup irqs belonging to this intc controller */
-		for_each_active_irq(irq) {
-			desc = irq_to_desc(irq);
-			data = irq_get_irq_data(irq);
-			chip = irq_data_get_irq_chip(data);
-
-			if (chip != &d->chip)
-				continue;
-			if ((desc->status & IRQ_WAKEUP))
-				chip->irq_enable(data);
-		}
-		break;
 	}
-
-	d->state = state;
-
-	return 0;
 }
 
-static int intc_resume(struct sys_device *dev)
-{
-	return intc_suspend(dev, PMSG_ON);
-}
-
-struct sysdev_class intc_sysdev_class = {
-	.name		= "intc",
+struct syscore_ops intc_syscore_ops = {
 	.suspend	= intc_suspend,
 	.resume		= intc_resume,
 };
 
-/* register this intc as sysdev to allow suspend/resume */
-static int __init register_intc_sysdevs(void)
+static int __init intc_syscore_init(void)
 {
-	struct intc_desc_int *d;
-	int error;
+	register_syscore_ops(&intc_syscore_ops);
 
-	error = sysdev_class_register(&intc_sysdev_class);
-	if (!error) {
-		list_for_each_entry(d, &intc_list, list) {
-			d->sysdev.id = d->index;
-			d->sysdev.cls = &intc_sysdev_class;
-			error = sysdev_register(&d->sysdev);
-			if (error = 0)
-				error = sysdev_create_file(&d->sysdev,
-							   &attr_name);
-			if (error)
-				break;
-		}
-	}
-
-	if (error)
-		pr_err("sysdev registration error\n");
-
-	return error;
+	return 0;
 }
-device_initcall(register_intc_sysdevs);
+device_initcall(intc_syscore_init);
Index: linux-2.6/drivers/sh/intc/internals.h
=================================--- linux-2.6.orig/drivers/sh/intc/internals.h
+++ linux-2.6/drivers/sh/intc/internals.h
@@ -51,9 +51,7 @@ struct intc_subgroup_entry {
 
 struct intc_desc_int {
 	struct list_head list;
-	struct sys_device sysdev;
 	struct radix_tree_root tree;
-	pm_message_t state;
 	raw_spinlock_t lock;
 	unsigned int index;
 	unsigned long *reg;

[PATCH 10/10] ARM: Use struct syscore_ops instead of sysdevs for PM in timer and leds

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Convert arch/arm/kernel/time.c and arch/arm/kernel/leds.c to using
struct syscore_ops for power management instead of sysdev classes
and sysdevs.

This simplifies the code in arch/arm/kernel/time.c quite a bit and is
necessary for removing sysdevs from the kernel entirely in future.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 arch/arm/include/asm/mach/time.h |    1 -
 arch/arm/kernel/leds.c           |   28 ++++++++++++++++------------
 arch/arm/kernel/time.c           |   33 +++++++++++----------------------
 3 files changed, 27 insertions(+), 35 deletions(-)

Index: linux-2.6/arch/arm/kernel/time.c
=================================--- linux-2.6.orig/arch/arm/kernel/time.c
+++ linux-2.6/arch/arm/kernel/time.c
@@ -21,7 +21,7 @@
 #include <linux/timex.h>
 #include <linux/errno.h>
 #include <linux/profile.h>
-#include <linux/sysdev.h>
+#include <linux/syscore_ops.h>
 #include <linux/timer.h>
 #include <linux/irq.h>
 
@@ -117,48 +117,37 @@ void timer_tick(void)
 #endif
 
 #if defined(CONFIG_PM) && !defined(CONFIG_GENERIC_CLOCKEVENTS)
-static int timer_suspend(struct sys_device *dev, pm_message_t state)
+static int timer_suspend(void)
 {
-	struct sys_timer *timer = container_of(dev, struct sys_timer, dev);
-
-	if (timer->suspend != NULL)
+	if (system_timer->suspend)
 		timer->suspend();
 
 	return 0;
 }
 
-static int timer_resume(struct sys_device *dev)
+static void timer_resume(void)
 {
-	struct sys_timer *timer = container_of(dev, struct sys_timer, dev);
-
-	if (timer->resume != NULL)
-		timer->resume();
-
-	return 0;
+	if (system_timer->resume)
+		system_timer->resume();
 }
 #else
 #define timer_suspend NULL
 #define timer_resume NULL
 #endif
 
-static struct sysdev_class timer_sysclass = {
-	.name		= "timer",
+static struct syscore_ops timer_syscore_ops = {
 	.suspend	= timer_suspend,
 	.resume		= timer_resume,
 };
 
-static int __init timer_init_sysfs(void)
+static int __init timer_init_syscore_ops(void)
 {
-	int ret = sysdev_class_register(&timer_sysclass);
-	if (ret = 0) {
-		system_timer->dev.cls = &timer_sysclass;
-		ret = sysdev_register(&system_timer->dev);
-	}
+	register_syscore_ops(&timer_syscore_ops);
 
-	return ret;
+	return 0;
 }
 
-device_initcall(timer_init_sysfs);
+device_initcall(timer_init_syscore_ops);
 
 void __init time_init(void)
 {
Index: linux-2.6/arch/arm/include/asm/mach/time.h
=================================--- linux-2.6.orig/arch/arm/include/asm/mach/time.h
+++ linux-2.6/arch/arm/include/asm/mach/time.h
@@ -34,7 +34,6 @@
  *   timer interrupt which may be pending.
  */
 struct sys_timer {
-	struct sys_device	dev;
 	void			(*init)(void);
 	void			(*suspend)(void);
 	void			(*resume)(void);
Index: linux-2.6/arch/arm/kernel/leds.c
=================================--- linux-2.6.orig/arch/arm/kernel/leds.c
+++ linux-2.6/arch/arm/kernel/leds.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/sysdev.h>
+#include <linux/syscore_ops.h>
 
 #include <asm/leds.h>
 
@@ -69,36 +70,37 @@ static ssize_t leds_store(struct sys_dev
 
 static SYSDEV_ATTR(event, 0200, NULL, leds_store);
 
-static int leds_suspend(struct sys_device *dev, pm_message_t state)
+static struct sysdev_class leds_sysclass = {
+	.name		= "leds",
+};
+
+static struct sys_device leds_device = {
+	.id		= 0,
+	.cls		= &leds_sysclass,
+};
+
+static int leds_suspend(void)
 {
 	leds_event(led_stop);
 	return 0;
 }
 
-static int leds_resume(struct sys_device *dev)
+static void leds_resume(void)
 {
 	leds_event(led_start);
-	return 0;
 }
 
-static int leds_shutdown(struct sys_device *dev)
+static void leds_shutdown(void)
 {
 	leds_event(led_halted);
-	return 0;
 }
 
-static struct sysdev_class leds_sysclass = {
-	.name		= "leds",
+static struct syscore_ops leds_syscore_ops = {
 	.shutdown	= leds_shutdown,
 	.suspend	= leds_suspend,
 	.resume		= leds_resume,
 };
 
-static struct sys_device leds_device = {
-	.id		= 0,
-	.cls		= &leds_sysclass,
-};
-
 static int __init leds_init(void)
 {
 	int ret;
@@ -107,6 +109,8 @@ static int __init leds_init(void)
 		ret = sysdev_register(&leds_device);
 	if (ret = 0)
 		ret = sysdev_create_file(&leds_device, &attr_event);
+	if (ret = 0)
+		register_syscore_ops(&leds_syscore_ops);
 	return ret;
 }
 

Re: [PATCH 10/10] ARM: Use struct syscore_ops instead of sysdevs

[Stephen Boyd]

On 3/13/2011 6:04 AM, Rafael J. Wysocki wrote:
>   #if defined(CONFIG_PM)&&  !defined(CONFIG_GENERIC_CLOCKEVENTS)
> -static int timer_suspend(struct sys_device *dev, pm_message_t state)
> +static int timer_suspend(void)
>   {
> -	struct sys_timer *timer = container_of(dev, struct sys_timer, dev);
> -
> -	if (timer->suspend != NULL)
> +	if (system_timer->suspend)
>   		timer->suspend();
>

Shouldn't this be system_timer->suspend() ?

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[Update] Re: [PATCH 10/10] ARM: Use struct syscore_ops instead of sysdevs for PM in timer and leds

[Rafael J. Wysocki]

On Monday, March 14, 2011, Stephen Boyd wrote:
> On 3/13/2011 6:04 AM, Rafael J. Wysocki wrote:
> >   #if defined(CONFIG_PM)&&  !defined(CONFIG_GENERIC_CLOCKEVENTS)
> > -static int timer_suspend(struct sys_device *dev, pm_message_t state)
> > +static int timer_suspend(void)
> >   {
> > -	struct sys_timer *timer = container_of(dev, struct sys_timer, dev);
> > -
> > -	if (timer->suspend != NULL)
> > +	if (system_timer->suspend)
> >   		timer->suspend();
> >
> 
> Shouldn't this be system_timer->suspend() ?

Indeed, thanks a lot!

I must have forgotten to test it with CONFIG_GENERIC_CLOCKEVENTS unset.

Updated patch follows.

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: ARM: Use struct syscore_ops instead of sysdevs for PM in timer and leds

Convert arch/arm/kernel/time.c and arch/arm/kernel/leds.c to using
struct syscore_ops for power management instead of sysdev classes
and sysdevs.

This simplifies the code in arch/arm/kernel/time.c quite a bit and is
necessary for removing sysdevs from the kernel entirely in future.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 arch/arm/include/asm/mach/time.h |    1 -
 arch/arm/kernel/leds.c           |   28 ++++++++++++++++------------
 arch/arm/kernel/time.c           |   35 ++++++++++++-----------------------
 3 files changed, 28 insertions(+), 36 deletions(-)

Index: linux-2.6/arch/arm/kernel/time.c
=================================--- linux-2.6.orig/arch/arm/kernel/time.c
+++ linux-2.6/arch/arm/kernel/time.c
@@ -21,7 +21,7 @@
 #include <linux/timex.h>
 #include <linux/errno.h>
 #include <linux/profile.h>
-#include <linux/sysdev.h>
+#include <linux/syscore_ops.h>
 #include <linux/timer.h>
 #include <linux/irq.h>
 
@@ -117,48 +117,37 @@ void timer_tick(void)
 #endif
 
 #if defined(CONFIG_PM) && !defined(CONFIG_GENERIC_CLOCKEVENTS)
-static int timer_suspend(struct sys_device *dev, pm_message_t state)
+static int timer_suspend(void)
 {
-	struct sys_timer *timer = container_of(dev, struct sys_timer, dev);
-
-	if (timer->suspend != NULL)
-		timer->suspend();
+	if (system_timer->suspend)
+		system_timer->suspend();
 
 	return 0;
 }
 
-static int timer_resume(struct sys_device *dev)
+static void timer_resume(void)
 {
-	struct sys_timer *timer = container_of(dev, struct sys_timer, dev);
-
-	if (timer->resume != NULL)
-		timer->resume();
-
-	return 0;
+	if (system_timer->resume)
+		system_timer->resume();
 }
 #else
 #define timer_suspend NULL
 #define timer_resume NULL
 #endif
 
-static struct sysdev_class timer_sysclass = {
-	.name		= "timer",
+static struct syscore_ops timer_syscore_ops = {
 	.suspend	= timer_suspend,
 	.resume		= timer_resume,
 };
 
-static int __init timer_init_sysfs(void)
+static int __init timer_init_syscore_ops(void)
 {
-	int ret = sysdev_class_register(&timer_sysclass);
-	if (ret = 0) {
-		system_timer->dev.cls = &timer_sysclass;
-		ret = sysdev_register(&system_timer->dev);
-	}
+	register_syscore_ops(&timer_syscore_ops);
 
-	return ret;
+	return 0;
 }
 
-device_initcall(timer_init_sysfs);
+device_initcall(timer_init_syscore_ops);
 
 void __init time_init(void)
 {
Index: linux-2.6/arch/arm/include/asm/mach/time.h
=================================--- linux-2.6.orig/arch/arm/include/asm/mach/time.h
+++ linux-2.6/arch/arm/include/asm/mach/time.h
@@ -34,7 +34,6 @@
  *   timer interrupt which may be pending.
  */
 struct sys_timer {
-	struct sys_device	dev;
 	void			(*init)(void);
 	void			(*suspend)(void);
 	void			(*resume)(void);
Index: linux-2.6/arch/arm/kernel/leds.c
=================================--- linux-2.6.orig/arch/arm/kernel/leds.c
+++ linux-2.6/arch/arm/kernel/leds.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/sysdev.h>
+#include <linux/syscore_ops.h>
 
 #include <asm/leds.h>
 
@@ -69,36 +70,37 @@ static ssize_t leds_store(struct sys_dev
 
 static SYSDEV_ATTR(event, 0200, NULL, leds_store);
 
-static int leds_suspend(struct sys_device *dev, pm_message_t state)
+static struct sysdev_class leds_sysclass = {
+	.name		= "leds",
+};
+
+static struct sys_device leds_device = {
+	.id		= 0,
+	.cls		= &leds_sysclass,
+};
+
+static int leds_suspend(void)
 {
 	leds_event(led_stop);
 	return 0;
 }
 
-static int leds_resume(struct sys_device *dev)
+static void leds_resume(void)
 {
 	leds_event(led_start);
-	return 0;
 }
 
-static int leds_shutdown(struct sys_device *dev)
+static void leds_shutdown(void)
 {
 	leds_event(led_halted);
-	return 0;
 }
 
-static struct sysdev_class leds_sysclass = {
-	.name		= "leds",
+static struct syscore_ops leds_syscore_ops = {
 	.shutdown	= leds_shutdown,
 	.suspend	= leds_suspend,
 	.resume		= leds_resume,
 };
 
-static struct sys_device leds_device = {
-	.id		= 0,
-	.cls		= &leds_sysclass,
-};
-
 static int __init leds_init(void)
 {
 	int ret;
@@ -107,6 +109,8 @@ static int __init leds_init(void)
 		ret = sysdev_register(&leds_device);
 	if (ret = 0)
 		ret = sysdev_create_file(&leds_device, &attr_event);
+	if (ret = 0)
+		register_syscore_ops(&leds_syscore_ops);
 	return ret;
 }
 

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev class and sysdev

[Paul Mundt]

On Sun, Mar 13, 2011 at 02:03:49PM +0100, R. J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Convert the SuperH clocks framework and shared interrupt handling
> code to using struct syscore_ops instead of a sysdev classes and
> sysdevs for power managment.
> 
> This reduces the code size significantly and simplifies it.  The
> optimizations causing things not to be restored after creating a
> hibernation image are removed, but they might lead to undesirable
> effects during resume from hibernation (e.g. the clocks would be left
> as the boot kernel set them, which might be not the same way as the
> hibernated kernel had seen them before the hibernation).
> 
> This also is necessary for removing sysdevs from the kernel entirely
> in the future.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

This misses the use of the sysdev class by the userimask code, though I'm
open to suggestions for alternatives.

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev class and sysdev

[Rafael J. Wysocki]

On Thursday, March 17, 2011, Paul Mundt wrote:
> On Sun, Mar 13, 2011 at 02:03:49PM +0100, R. J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Convert the SuperH clocks framework and shared interrupt handling
> > code to using struct syscore_ops instead of a sysdev classes and
> > sysdevs for power managment.
> > 
> > This reduces the code size significantly and simplifies it.  The
> > optimizations causing things not to be restored after creating a
> > hibernation image are removed, but they might lead to undesirable
> > effects during resume from hibernation (e.g. the clocks would be left
> > as the boot kernel set them, which might be not the same way as the
> > hibernated kernel had seen them before the hibernation).
> > 
> > This also is necessary for removing sysdevs from the kernel entirely
> > in the future.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
> This misses the use of the sysdev class by the userimask code, though I'm
> open to suggestions for alternatives.

For now, I'd simply move the sysdev class definition to userimask.c, like
in the patch below.  The current goal is to eliminate the suspend/resume and
shutdown operations from sysdevs (and sysdev drivers), the next step will
be to replace the remaining sysdevs with alternative mechanisms.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: Drivers / sh: Use struct syscore_ops instead of sysdev class and sysdev (v2)

Convert the SuperH clocks framework and shared interrupt handling
code to using struct syscore_ops instead of a sysdev classes and
sysdevs for power managment.

This reduces the code size significantly and simplifies it.  The
optimizations causing things not to be restored after creating a
hibernation image are removed, but they might lead to undesirable
effects during resume from hibernation (e.g. the clocks would be left
as the boot kernel set them, which might be not the same way as the
hibernated kernel had seen them before the hibernation).

This also is necessary for removing sysdevs from the kernel entirely
in the future.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/sh/clk/core.c       |   68 +++++++--------------------
 drivers/sh/intc/core.c      |  108 ++++++++++++++------------------------------
 drivers/sh/intc/internals.h |    5 --
 drivers/sh/intc/userimask.c |    9 +++
 4 files changed, 63 insertions(+), 127 deletions(-)

Index: linux-2.6/drivers/sh/clk/core.c
=================================--- linux-2.6.orig/drivers/sh/clk/core.c
+++ linux-2.6/drivers/sh/clk/core.c
@@ -21,7 +21,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/list.h>
-#include <linux/sysdev.h>
+#include <linux/syscore_ops.h>
 #include <linux/seq_file.h>
 #include <linux/err.h>
 #include <linux/io.h>
@@ -630,68 +630,36 @@ long clk_round_parent(struct clk *clk, u
 EXPORT_SYMBOL_GPL(clk_round_parent);
 
 #ifdef CONFIG_PM
-static int clks_sysdev_suspend(struct sys_device *dev, pm_message_t state)
+static void clks_core_resume(void)
 {
-	static pm_message_t prev_state;
 	struct clk *clkp;
 
-	switch (state.event) {
-	case PM_EVENT_ON:
-		/* Resumeing from hibernation */
-		if (prev_state.event != PM_EVENT_FREEZE)
-			break;
-
-		list_for_each_entry(clkp, &clock_list, node) {
-			if (likely(clkp->ops)) {
-				unsigned long rate = clkp->rate;
-
-				if (likely(clkp->ops->set_parent))
-					clkp->ops->set_parent(clkp,
-						clkp->parent);
-				if (likely(clkp->ops->set_rate))
-					clkp->ops->set_rate(clkp, rate);
-				else if (likely(clkp->ops->recalc))
-					clkp->rate = clkp->ops->recalc(clkp);
-			}
+	list_for_each_entry(clkp, &clock_list, node) {
+		if (likely(clkp->ops)) {
+			unsigned long rate = clkp->rate;
+
+			if (likely(clkp->ops->set_parent))
+				clkp->ops->set_parent(clkp,
+					clkp->parent);
+			if (likely(clkp->ops->set_rate))
+				clkp->ops->set_rate(clkp, rate);
+			else if (likely(clkp->ops->recalc))
+				clkp->rate = clkp->ops->recalc(clkp);
 		}
-		break;
-	case PM_EVENT_FREEZE:
-		break;
-	case PM_EVENT_SUSPEND:
-		break;
 	}
-
-	prev_state = state;
-	return 0;
-}
-
-static int clks_sysdev_resume(struct sys_device *dev)
-{
-	return clks_sysdev_suspend(dev, PMSG_ON);
 }
 
-static struct sysdev_class clks_sysdev_class = {
-	.name = "clks",
-};
-
-static struct sysdev_driver clks_sysdev_driver = {
-	.suspend = clks_sysdev_suspend,
-	.resume = clks_sysdev_resume,
-};
-
-static struct sys_device clks_sysdev_dev = {
-	.cls = &clks_sysdev_class,
+static struct syscore_ops clks_syscore_ops = {
+	.resume = clks_core_resume,
 };
 
-static int __init clk_sysdev_init(void)
+static int __init clk_syscore_init(void)
 {
-	sysdev_class_register(&clks_sysdev_class);
-	sysdev_driver_register(&clks_sysdev_class, &clks_sysdev_driver);
-	sysdev_register(&clks_sysdev_dev);
+	register_syscore_ops(&clks_syscore_ops);
 
 	return 0;
 }
-subsys_initcall(clk_sysdev_init);
+subsys_initcall(clk_syscore_init);
 #endif
 
 /*
Index: linux-2.6/drivers/sh/intc/core.c
=================================--- linux-2.6.orig/drivers/sh/intc/core.c
+++ linux-2.6/drivers/sh/intc/core.c
@@ -24,7 +24,7 @@
 #include <linux/slab.h>
 #include <linux/interrupt.h>
 #include <linux/sh_intc.h>
-#include <linux/sysdev.h>
+#include <linux/syscore_ops.h>
 #include <linux/list.h>
 #include <linux/spinlock.h>
 #include <linux/radix-tree.h>
@@ -376,108 +376,70 @@ err0:
 	return -ENOMEM;
 }
 
-static ssize_t
-show_intc_name(struct sys_device *dev, struct sysdev_attribute *attr, char *buf)
+static int intc_suspend(void)
 {
 	struct intc_desc_int *d;
 
-	d = container_of(dev, struct intc_desc_int, sysdev);
+	list_for_each_entry(d, &intc_list, list) {
+		int irq;
 
-	return sprintf(buf, "%s\n", d->chip.name);
-}
+		/* enable wakeup irqs belonging to this intc controller */
+		for_each_active_irq(irq) {
+			struct irq_data *data;
+			struct irq_desc *desc;
+			struct irq_chip *chip;
+
+			data = irq_get_irq_data(irq);
+			chip = irq_data_get_irq_chip(data);
+			if (chip != &d->chip)
+				continue;
+			desc = irq_to_desc(irq);
+			if ((desc->status & IRQ_WAKEUP))
+				chip->irq_enable(data);
+		}
+	}
 
-static SYSDEV_ATTR(name, S_IRUGO, show_intc_name, NULL);
+	return 0;
+}
 
-static int intc_suspend(struct sys_device *dev, pm_message_t state)
+static void intc_resume(void)
 {
 	struct intc_desc_int *d;
-	struct irq_data *data;
-	struct irq_desc *desc;
-	struct irq_chip *chip;
-	int irq;
-
-	/* get intc controller associated with this sysdev */
-	d = container_of(dev, struct intc_desc_int, sysdev);
-
-	switch (state.event) {
-	case PM_EVENT_ON:
-		if (d->state.event != PM_EVENT_FREEZE)
-			break;
+
+	list_for_each_entry(d, &intc_list, list) {
+		int irq;
 
 		for_each_active_irq(irq) {
-			desc = irq_to_desc(irq);
+			struct irq_data *data;
+			struct irq_desc *desc;
+			struct irq_chip *chip;
+
 			data = irq_get_irq_data(irq);
 			chip = irq_data_get_irq_chip(data);
-
 			/*
 			 * This will catch the redirect and VIRQ cases
 			 * due to the dummy_irq_chip being inserted.
 			 */
 			if (chip != &d->chip)
 				continue;
+			desc = irq_to_desc(irq);
 			if (desc->status & IRQ_DISABLED)
 				chip->irq_disable(data);
 			else
 				chip->irq_enable(data);
 		}
-		break;
-	case PM_EVENT_FREEZE:
-		/* nothing has to be done */
-		break;
-	case PM_EVENT_SUSPEND:
-		/* enable wakeup irqs belonging to this intc controller */
-		for_each_active_irq(irq) {
-			desc = irq_to_desc(irq);
-			data = irq_get_irq_data(irq);
-			chip = irq_data_get_irq_chip(data);
-
-			if (chip != &d->chip)
-				continue;
-			if ((desc->status & IRQ_WAKEUP))
-				chip->irq_enable(data);
-		}
-		break;
 	}
-
-	d->state = state;
-
-	return 0;
 }
 
-static int intc_resume(struct sys_device *dev)
-{
-	return intc_suspend(dev, PMSG_ON);
-}
-
-struct sysdev_class intc_sysdev_class = {
-	.name		= "intc",
+struct syscore_ops intc_syscore_ops = {
 	.suspend	= intc_suspend,
 	.resume		= intc_resume,
 };
 
-/* register this intc as sysdev to allow suspend/resume */
-static int __init register_intc_sysdevs(void)
+static int __init intc_syscore_init(void)
 {
-	struct intc_desc_int *d;
-	int error;
+	register_syscore_ops(&intc_syscore_ops);
 
-	error = sysdev_class_register(&intc_sysdev_class);
-	if (!error) {
-		list_for_each_entry(d, &intc_list, list) {
-			d->sysdev.id = d->index;
-			d->sysdev.cls = &intc_sysdev_class;
-			error = sysdev_register(&d->sysdev);
-			if (error = 0)
-				error = sysdev_create_file(&d->sysdev,
-							   &attr_name);
-			if (error)
-				break;
-		}
-	}
-
-	if (error)
-		pr_err("sysdev registration error\n");
-
-	return error;
+	return 0;
 }
-device_initcall(register_intc_sysdevs);
+device_initcall(intc_syscore_init);
Index: linux-2.6/drivers/sh/intc/internals.h
=================================--- linux-2.6.orig/drivers/sh/intc/internals.h
+++ linux-2.6/drivers/sh/intc/internals.h
@@ -4,7 +4,7 @@
 #include <linux/kernel.h>
 #include <linux/types.h>
 #include <linux/radix-tree.h>
-#include <linux/sysdev.h>
+#include <linux/module.h>
 
 #define _INTC_MK(fn, mode, addr_e, addr_d, width, shift) \
 	((shift) | ((width) << 5) | ((fn) << 9) | ((mode) << 13) | \
@@ -51,9 +51,7 @@ struct intc_subgroup_entry {
 
 struct intc_desc_int {
 	struct list_head list;
-	struct sys_device sysdev;
 	struct radix_tree_root tree;
-	pm_message_t state;
 	raw_spinlock_t lock;
 	unsigned int index;
 	unsigned long *reg;
@@ -158,7 +156,6 @@ void _intc_enable(struct irq_data *data,
 extern struct list_head intc_list;
 extern raw_spinlock_t intc_big_lock;
 extern unsigned int nr_intc_controllers;
-extern struct sysdev_class intc_sysdev_class;
 
 unsigned int intc_get_dfl_prio_level(void);
 unsigned int intc_get_prio_level(unsigned int irq);
Index: linux-2.6/drivers/sh/intc/userimask.c
=================================--- linux-2.6.orig/drivers/sh/intc/userimask.c
+++ linux-2.6/drivers/sh/intc/userimask.c
@@ -57,12 +57,21 @@ store_intc_userimask(struct sysdev_class
 static SYSDEV_CLASS_ATTR(userimask, S_IRUSR | S_IWUSR,
 			 show_intc_userimask, store_intc_userimask);
 
+static struct sysdev_class intc_sysdev_class = {
+	.name		= "intc",
+};
 
 static int __init userimask_sysdev_init(void)
 {
+	int error;
+
 	if (unlikely(!uimask))
 		return -ENXIO;
 
+	error = sysdev_class_register(&intc_sysdev_class);
+	if (error)
+		return error;
+
 	return sysdev_class_create_file(&intc_sysdev_class, &attr_userimask);
 }
 late_initcall(userimask_sysdev_init);

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev class and sysdev

[Paul Mundt]

On Sat, Mar 19, 2011 at 01:47:27AM +0100, Rafael J. Wysocki wrote:
> On Thursday, March 17, 2011, Paul Mundt wrote:
> > On Sun, Mar 13, 2011 at 02:03:49PM +0100, R. J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > 
> > > Convert the SuperH clocks framework and shared interrupt handling
> > > code to using struct syscore_ops instead of a sysdev classes and
> > > sysdevs for power managment.
> > > 
> > > This reduces the code size significantly and simplifies it.  The
> > > optimizations causing things not to be restored after creating a
> > > hibernation image are removed, but they might lead to undesirable
> > > effects during resume from hibernation (e.g. the clocks would be left
> > > as the boot kernel set them, which might be not the same way as the
> > > hibernated kernel had seen them before the hibernation).
> > > 
> > > This also is necessary for removing sysdevs from the kernel entirely
> > > in the future.
> > > 
> > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > This misses the use of the sysdev class by the userimask code, though I'm
> > open to suggestions for alternatives.
> 
> For now, I'd simply move the sysdev class definition to userimask.c, like
> in the patch below.  The current goal is to eliminate the suspend/resume and
> shutdown operations from sysdevs (and sysdev drivers), the next step will
> be to replace the remaining sysdevs with alternative mechanisms.
> 
It's not quite that straightforward, you've also killed off the name
attribute for each of the intc sysdevs, so we no longer have a visible
way to map a given intc controller number to the controller name in a
user visible way.

I'm not opposed to the syscore thing for suspend/resume ops, but I'm not
willing to trash the userimask and name mapping interface in the process
with no alternatives.

userimask was the first global configuration item I added, but there are
other per-controller and global configuration knobs that I plan to export
through the interface, so there really needs to be a compelling reason
for moving off of sysdevs.

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev

[Kay Sievers]

On Tue, 2011-03-22 at 23:04 +0900, Paul Mundt wrote:
> On Sat, Mar 19, 2011 at 01:47:27AM +0100, Rafael J. Wysocki wrote:
> > On Thursday, March 17, 2011, Paul Mundt wrote:
> > > On Sun, Mar 13, 2011 at 02:03:49PM +0100, R. J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > 
> > > > Convert the SuperH clocks framework and shared interrupt handling
> > > > code to using struct syscore_ops instead of a sysdev classes and
> > > > sysdevs for power managment.
> > > > 
> > > > This reduces the code size significantly and simplifies it.  The
> > > > optimizations causing things not to be restored after creating a
> > > > hibernation image are removed, but they might lead to undesirable
> > > > effects during resume from hibernation (e.g. the clocks would be left
> > > > as the boot kernel set them, which might be not the same way as the
> > > > hibernated kernel had seen them before the hibernation).
> > > > 
> > > > This also is necessary for removing sysdevs from the kernel entirely
> > > > in the future.
> > > > 
> > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > 
> > > This misses the use of the sysdev class by the userimask code, though I'm
> > > open to suggestions for alternatives.
> > 
> > For now, I'd simply move the sysdev class definition to userimask.c, like
> > in the patch below.  The current goal is to eliminate the suspend/resume and
> > shutdown operations from sysdevs (and sysdev drivers), the next step will
> > be to replace the remaining sysdevs with alternative mechanisms.
> > 
> It's not quite that straightforward, you've also killed off the name
> attribute for each of the intc sysdevs, so we no longer have a visible
> way to map a given intc controller number to the controller name in a
> user visible way.
> 
> I'm not opposed to the syscore thing for suspend/resume ops, but I'm not
> willing to trash the userimask and name mapping interface in the process
> with no alternatives.
> 
> userimask was the first global configuration item I added, but there are
> other per-controller and global configuration knobs that I plan to export
> through the interface, so there really needs to be a compelling reason
> for moving off of sysdevs.

Yes, they don't fit into the model. They have been a dumb hack from the
first day, and never integrated into the kenrel driver core or hotplug
properly.

If you need the userspace visibility, better just add a "struct
bus_type" with a proper name for your subsystem and register a "struct
device" with the bus_type assigned for all of them, instead of using the
broken concept of sydevs. You can even make them show up
in /sys/devices/system/<bus_type name>/<struct device name>/ if you want
to.

That way userspace can properly enumerate them in a flat list
in /sys/bus/<bus_type name>/devices/*, and gets proper events on module
load and during system coldplug, and can hook into the usual hotplug
pathes to set/get these values instead of crawling magicly defined and
decoupled locations in /sys which can not express proper hierarchy,
classicication, or anything else that all other devices can just do.

There is really no reason for any device being a magic and conceptually
broken sysdev today - just to be different from any other device the
kernel exports to userspace.

Thanks,
Kay

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev class and sysdev

[Rafael J. Wysocki]

On Tuesday, March 22, 2011, Paul Mundt wrote:
> On Sat, Mar 19, 2011 at 01:47:27AM +0100, Rafael J. Wysocki wrote:
> > On Thursday, March 17, 2011, Paul Mundt wrote:
> > > On Sun, Mar 13, 2011 at 02:03:49PM +0100, R. J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > 
> > > > Convert the SuperH clocks framework and shared interrupt handling
> > > > code to using struct syscore_ops instead of a sysdev classes and
> > > > sysdevs for power managment.
> > > > 
> > > > This reduces the code size significantly and simplifies it.  The
> > > > optimizations causing things not to be restored after creating a
> > > > hibernation image are removed, but they might lead to undesirable
> > > > effects during resume from hibernation (e.g. the clocks would be left
> > > > as the boot kernel set them, which might be not the same way as the
> > > > hibernated kernel had seen them before the hibernation).
> > > > 
> > > > This also is necessary for removing sysdevs from the kernel entirely
> > > > in the future.
> > > > 
> > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > 
> > > This misses the use of the sysdev class by the userimask code, though I'm
> > > open to suggestions for alternatives.
> > 
> > For now, I'd simply move the sysdev class definition to userimask.c, like
> > in the patch below.  The current goal is to eliminate the suspend/resume and
> > shutdown operations from sysdevs (and sysdev drivers), the next step will
> > be to replace the remaining sysdevs with alternative mechanisms.
> > 
> It's not quite that straightforward, you've also killed off the name
> attribute for each of the intc sysdevs, so we no longer have a visible
> way to map a given intc controller number to the controller name in a
> user visible way.

Hmm.  So you actually want those empty directories to show up in sysfs?
I didn't think they were really useful ...

> I'm not opposed to the syscore thing for suspend/resume ops, but I'm not
> willing to trash the userimask and name mapping interface in the process
> with no alternatives.

OK

> userimask was the first global configuration item I added, but there are
> other per-controller and global configuration knobs that I plan to export
> through the interface, so there really needs to be a compelling reason
> for moving off of sysdevs.

Yes, there is.  They are simply unusable in other situations, but I agree
we'll need to find a clean way to replace them where there's a good reason to
use them.

Updated patch follows.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: Drivers / sh: Use struct syscore_ops instead of sysdevs

Convert the SuperH clocks framework and shared interrupt handling
code to using struct syscore_ops instead of a sysdev classes and
sysdevs for power managment.

This reduces the code size significantly and simplifies it.  The
optimizations causing things not to be restored after creating a
hibernation image are removed, but they might lead to undesirable
effects during resume from hibernation (e.g. the clocks would be left
as the boot kernel set them, which might be not the same way as the
hibernated kernel had seen them before the hibernation).

This also is necessary for removing sysdevs from the kernel entirely
in the future.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/sh/clk/core.c       |   68 ++++++++-----------------------
 drivers/sh/intc/core.c      |   95 +++++++++++++++++++++-----------------------
 drivers/sh/intc/internals.h |    1 
 3 files changed, 65 insertions(+), 99 deletions(-)

Index: linux-2.6/drivers/sh/clk/core.c
=================================--- linux-2.6.orig/drivers/sh/clk/core.c
+++ linux-2.6/drivers/sh/clk/core.c
@@ -21,7 +21,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/list.h>
-#include <linux/sysdev.h>
+#include <linux/syscore_ops.h>
 #include <linux/seq_file.h>
 #include <linux/err.h>
 #include <linux/io.h>
@@ -630,68 +630,36 @@ long clk_round_parent(struct clk *clk, u
 EXPORT_SYMBOL_GPL(clk_round_parent);
 
 #ifdef CONFIG_PM
-static int clks_sysdev_suspend(struct sys_device *dev, pm_message_t state)
+static void clks_core_resume(void)
 {
-	static pm_message_t prev_state;
 	struct clk *clkp;
 
-	switch (state.event) {
-	case PM_EVENT_ON:
-		/* Resumeing from hibernation */
-		if (prev_state.event != PM_EVENT_FREEZE)
-			break;
-
-		list_for_each_entry(clkp, &clock_list, node) {
-			if (likely(clkp->ops)) {
-				unsigned long rate = clkp->rate;
-
-				if (likely(clkp->ops->set_parent))
-					clkp->ops->set_parent(clkp,
-						clkp->parent);
-				if (likely(clkp->ops->set_rate))
-					clkp->ops->set_rate(clkp, rate);
-				else if (likely(clkp->ops->recalc))
-					clkp->rate = clkp->ops->recalc(clkp);
-			}
+	list_for_each_entry(clkp, &clock_list, node) {
+		if (likely(clkp->ops)) {
+			unsigned long rate = clkp->rate;
+
+			if (likely(clkp->ops->set_parent))
+				clkp->ops->set_parent(clkp,
+					clkp->parent);
+			if (likely(clkp->ops->set_rate))
+				clkp->ops->set_rate(clkp, rate);
+			else if (likely(clkp->ops->recalc))
+				clkp->rate = clkp->ops->recalc(clkp);
 		}
-		break;
-	case PM_EVENT_FREEZE:
-		break;
-	case PM_EVENT_SUSPEND:
-		break;
 	}
-
-	prev_state = state;
-	return 0;
-}
-
-static int clks_sysdev_resume(struct sys_device *dev)
-{
-	return clks_sysdev_suspend(dev, PMSG_ON);
 }
 
-static struct sysdev_class clks_sysdev_class = {
-	.name = "clks",
-};
-
-static struct sysdev_driver clks_sysdev_driver = {
-	.suspend = clks_sysdev_suspend,
-	.resume = clks_sysdev_resume,
-};
-
-static struct sys_device clks_sysdev_dev = {
-	.cls = &clks_sysdev_class,
+static struct syscore_ops clks_syscore_ops = {
+	.resume = clks_core_resume,
 };
 
-static int __init clk_sysdev_init(void)
+static int __init clk_syscore_init(void)
 {
-	sysdev_class_register(&clks_sysdev_class);
-	sysdev_driver_register(&clks_sysdev_class, &clks_sysdev_driver);
-	sysdev_register(&clks_sysdev_dev);
+	register_syscore_ops(&clks_syscore_ops);
 
 	return 0;
 }
-subsys_initcall(clk_sysdev_init);
+subsys_initcall(clk_syscore_init);
 #endif
 
 /*
Index: linux-2.6/drivers/sh/intc/core.c
=================================--- linux-2.6.orig/drivers/sh/intc/core.c
+++ linux-2.6/drivers/sh/intc/core.c
@@ -25,6 +25,7 @@
 #include <linux/interrupt.h>
 #include <linux/sh_intc.h>
 #include <linux/sysdev.h>
+#include <linux/syscore_ops.h>
 #include <linux/list.h>
 #include <linux/spinlock.h>
 #include <linux/radix-tree.h>
@@ -376,91 +377,89 @@ err0:
 	return -ENOMEM;
 }
 
-static ssize_t
-show_intc_name(struct sys_device *dev, struct sysdev_attribute *attr, char *buf)
+static int intc_suspend(void)
 {
 	struct intc_desc_int *d;
 
-	d = container_of(dev, struct intc_desc_int, sysdev);
+	list_for_each_entry(d, &intc_list, list) {
+		int irq;
 
-	return sprintf(buf, "%s\n", d->chip.name);
-}
+		/* enable wakeup irqs belonging to this intc controller */
+		for_each_active_irq(irq) {
+			struct irq_data *data;
+			struct irq_desc *desc;
+			struct irq_chip *chip;
 
-static SYSDEV_ATTR(name, S_IRUGO, show_intc_name, NULL);
+			data = irq_get_irq_data(irq);
+			chip = irq_data_get_irq_chip(data);
+			if (chip != &d->chip)
+				continue;
+			desc = irq_to_desc(irq);
+			if ((desc->status & IRQ_WAKEUP))
+				chip->irq_enable(data);
+		}
+	}
+
+	return 0;
+}
 
-static int intc_suspend(struct sys_device *dev, pm_message_t state)
+static void intc_resume(void)
 {
 	struct intc_desc_int *d;
-	struct irq_data *data;
-	struct irq_desc *desc;
-	struct irq_chip *chip;
-	int irq;
-
-	/* get intc controller associated with this sysdev */
-	d = container_of(dev, struct intc_desc_int, sysdev);
 
-	switch (state.event) {
-	case PM_EVENT_ON:
-		if (d->state.event != PM_EVENT_FREEZE)
-			break;
+	list_for_each_entry(d, &intc_list, list) {
+		int irq;
 
 		for_each_active_irq(irq) {
-			desc = irq_to_desc(irq);
+			struct irq_data *data;
+			struct irq_desc *desc;
+			struct irq_chip *chip;
+
 			data = irq_get_irq_data(irq);
 			chip = irq_data_get_irq_chip(data);
-
 			/*
 			 * This will catch the redirect and VIRQ cases
 			 * due to the dummy_irq_chip being inserted.
 			 */
 			if (chip != &d->chip)
 				continue;
+			desc = irq_to_desc(irq);
 			if (desc->status & IRQ_DISABLED)
 				chip->irq_disable(data);
 			else
 				chip->irq_enable(data);
 		}
-		break;
-	case PM_EVENT_FREEZE:
-		/* nothing has to be done */
-		break;
-	case PM_EVENT_SUSPEND:
-		/* enable wakeup irqs belonging to this intc controller */
-		for_each_active_irq(irq) {
-			desc = irq_to_desc(irq);
-			data = irq_get_irq_data(irq);
-			chip = irq_data_get_irq_chip(data);
-
-			if (chip != &d->chip)
-				continue;
-			if ((desc->status & IRQ_WAKEUP))
-				chip->irq_enable(data);
-		}
-		break;
 	}
-
-	d->state = state;
-
-	return 0;
 }
 
-static int intc_resume(struct sys_device *dev)
-{
-	return intc_suspend(dev, PMSG_ON);
-}
+struct syscore_ops intc_syscore_ops = {
+	.suspend	= intc_suspend,
+	.resume		= intc_resume,
+};
 
 struct sysdev_class intc_sysdev_class = {
 	.name		= "intc",
-	.suspend	= intc_suspend,
-	.resume		= intc_resume,
 };
 
-/* register this intc as sysdev to allow suspend/resume */
+static ssize_t
+show_intc_name(struct sys_device *dev, struct sysdev_attribute *attr, char *buf)
+{
+	struct intc_desc_int *d;
+
+	d = container_of(dev, struct intc_desc_int, sysdev);
+
+	return sprintf(buf, "%s\n", d->chip.name);
+}
+
+static SYSDEV_ATTR(name, S_IRUGO, show_intc_name, NULL);
+
 static int __init register_intc_sysdevs(void)
 {
 	struct intc_desc_int *d;
 	int error;
 
+	register_syscore_ops(&intc_syscore_ops);
+
 	error = sysdev_class_register(&intc_sysdev_class);
 	if (!error) {
 		list_for_each_entry(d, &intc_list, list) {
Index: linux-2.6/drivers/sh/intc/internals.h
=================================--- linux-2.6.orig/drivers/sh/intc/internals.h
+++ linux-2.6/drivers/sh/intc/internals.h
@@ -53,7 +53,6 @@ struct intc_desc_int {
 	struct list_head list;
 	struct sys_device sysdev;
 	struct radix_tree_root tree;
-	pm_message_t state;
 	raw_spinlock_t lock;
 	unsigned int index;
 	unsigned long *reg;

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev class and sysdev

[Rafael J. Wysocki]

On Tuesday, March 22, 2011, Kay Sievers wrote:
> On Tue, 2011-03-22 at 23:04 +0900, Paul Mundt wrote:
> > On Sat, Mar 19, 2011 at 01:47:27AM +0100, Rafael J. Wysocki wrote:
> > > On Thursday, March 17, 2011, Paul Mundt wrote:
> > > > On Sun, Mar 13, 2011 at 02:03:49PM +0100, R. J. Wysocki wrote:
> > > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > 
> > > > > Convert the SuperH clocks framework and shared interrupt handling
> > > > > code to using struct syscore_ops instead of a sysdev classes and
> > > > > sysdevs for power managment.
> > > > > 
> > > > > This reduces the code size significantly and simplifies it.  The
> > > > > optimizations causing things not to be restored after creating a
> > > > > hibernation image are removed, but they might lead to undesirable
> > > > > effects during resume from hibernation (e.g. the clocks would be left
> > > > > as the boot kernel set them, which might be not the same way as the
> > > > > hibernated kernel had seen them before the hibernation).
> > > > > 
> > > > > This also is necessary for removing sysdevs from the kernel entirely
> > > > > in the future.
> > > > > 
> > > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > > 
> > > > This misses the use of the sysdev class by the userimask code, though I'm
> > > > open to suggestions for alternatives.
> > > 
> > > For now, I'd simply move the sysdev class definition to userimask.c, like
> > > in the patch below.  The current goal is to eliminate the suspend/resume and
> > > shutdown operations from sysdevs (and sysdev drivers), the next step will
> > > be to replace the remaining sysdevs with alternative mechanisms.
> > > 
> > It's not quite that straightforward, you've also killed off the name
> > attribute for each of the intc sysdevs, so we no longer have a visible
> > way to map a given intc controller number to the controller name in a
> > user visible way.
> > 
> > I'm not opposed to the syscore thing for suspend/resume ops, but I'm not
> > willing to trash the userimask and name mapping interface in the process
> > with no alternatives.
> > 
> > userimask was the first global configuration item I added, but there are
> > other per-controller and global configuration knobs that I plan to export
> > through the interface, so there really needs to be a compelling reason
> > for moving off of sysdevs.
> 
> Yes, they don't fit into the model. They have been a dumb hack from the
> first day, and never integrated into the kenrel driver core or hotplug
> properly.
> 
> If you need the userspace visibility, better just add a "struct
> bus_type" with a proper name for your subsystem and register a "struct
> device" with the bus_type assigned for all of them, instead of using the
> broken concept of sydevs. You can even make them show up
> in /sys/devices/system/<bus_type name>/<struct device name>/ if you want
> to.

I don't really think that's going to be useful in this particular case.
The reason is, first, because the struct device would cause lots of other
stuff to show up in sysfs which would be totally redundant and confusing
and, second, because the things exported here are simply static attributes,
pretty much like the stuff in /sys/power/.

Perhaps there's a more straightforward way to make some files show up in
sysfs on a specific path than defininig an otherwise useless bus type and
device object?

> That way userspace can properly enumerate them in a flat list
> in /sys/bus/<bus_type name>/devices/*, and gets proper events on module
> load and during system coldplug, and can hook into the usual hotplug
> pathes to set/get these values instead of crawling magicly defined and
> decoupled locations in /sys which can not express proper hierarchy,
> classicication, or anything else that all other devices can just do.

There's no hotplug involved or anything remotely like that AFAICS.
There are simply static files as I said above, they are created
early during system initialization and simply stay there.

> There is really no reason for any device being a magic and conceptually
> broken sysdev today - just to be different from any other device the
> kernel exports to userspace.

It's not a "device being a sysdev", it's sysdevs being used for creating
a user space interface, which isn't broken by itself.

Thanks,
Rafael

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev

[Kay Sievers]

On Tue, 2011-03-22 at 21:30 +0100, Rafael J. Wysocki wrote:
> On Tuesday, March 22, 2011, Kay Sievers wrote:
> > On Tue, 2011-03-22 at 23:04 +0900, Paul Mundt wrote:
> > > On Sat, Mar 19, 2011 at 01:47:27AM +0100, Rafael J. Wysocki wrote:
> > > > On Thursday, March 17, 2011, Paul Mundt wrote:
> > > > > On Sun, Mar 13, 2011 at 02:03:49PM +0100, R. J. Wysocki wrote:
> > > > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > > 
> > > > > > Convert the SuperH clocks framework and shared interrupt handling
> > > > > > code to using struct syscore_ops instead of a sysdev classes and
> > > > > > sysdevs for power managment.
> > > > > > 
> > > > > > This reduces the code size significantly and simplifies it.  The
> > > > > > optimizations causing things not to be restored after creating a
> > > > > > hibernation image are removed, but they might lead to undesirable
> > > > > > effects during resume from hibernation (e.g. the clocks would be left
> > > > > > as the boot kernel set them, which might be not the same way as the
> > > > > > hibernated kernel had seen them before the hibernation).
> > > > > > 
> > > > > > This also is necessary for removing sysdevs from the kernel entirely
> > > > > > in the future.
> > > > > > 
> > > > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > 
> > > > > This misses the use of the sysdev class by the userimask code, though I'm
> > > > > open to suggestions for alternatives.
> > > > 
> > > > For now, I'd simply move the sysdev class definition to userimask.c, like
> > > > in the patch below.  The current goal is to eliminate the suspend/resume and
> > > > shutdown operations from sysdevs (and sysdev drivers), the next step will
> > > > be to replace the remaining sysdevs with alternative mechanisms.
> > > > 
> > > It's not quite that straightforward, you've also killed off the name
> > > attribute for each of the intc sysdevs, so we no longer have a visible
> > > way to map a given intc controller number to the controller name in a
> > > user visible way.
> > > 
> > > I'm not opposed to the syscore thing for suspend/resume ops, but I'm not
> > > willing to trash the userimask and name mapping interface in the process
> > > with no alternatives.
> > > 
> > > userimask was the first global configuration item I added, but there are
> > > other per-controller and global configuration knobs that I plan to export
> > > through the interface, so there really needs to be a compelling reason
> > > for moving off of sysdevs.
> > 
> > Yes, they don't fit into the model. They have been a dumb hack from the
> > first day, and never integrated into the kenrel driver core or hotplug
> > properly.
> > 
> > If you need the userspace visibility, better just add a "struct
> > bus_type" with a proper name for your subsystem and register a "struct
> > device" with the bus_type assigned for all of them, instead of using the
> > broken concept of sydevs. You can even make them show up
> > in /sys/devices/system/<bus_type name>/<struct device name>/ if you want
> > to.
> 
> I don't really think that's going to be useful in this particular case.
> The reason is, first, because the struct device would cause lots of other
> stuff to show up in sysfs which would be totally redundant and confusing
> and, second, because the things exported here are simply static attributes,
> pretty much like the stuff in /sys/power/.
> 
> Perhaps there's a more straightforward way to make some files show up in
> sysfs on a specific path than defininig an otherwise useless bus type and
> device object?

That's absolutely not the point. Please don't get yourself into that
thinking. If people want to "export stuff to userspace", they must not
invent new things. We need to get rid of the silly special cases.
Userspace is not meant to learn subsystem specific rules for every new
thing. There is _one_ way to export device attributes, and that is
"struct device" today.

If that's to expensive for anybody, just don't use sysfs. It's the rule
we have today. :)

> > That way userspace can properly enumerate them in a flat list
> > in /sys/bus/<bus_type name>/devices/*, and gets proper events on module
> > load and during system coldplug, and can hook into the usual hotplug
> > pathes to set/get these values instead of crawling magicly defined and
> > decoupled locations in /sys which can not express proper hierarchy,
> > classicication, or anything else that all other devices can just do.
> 
> There's no hotplug involved or anything remotely like that AFAICS.
> There are simply static files as I said above, they are created
> early during system initialization and simply stay there.

That's not the point. It's about a single way to retrieve information
about devices, extendability, and coldplug during bootup, where existing
devices need to be handled only after userspace is up. That is just a
case of "hotplug" that has the same codepath for userspace, even when
the devices can never really come and go.

> > There is really no reason for any device being a magic and conceptually
> > broken sysdev today - just to be different from any other device the
> > kernel exports to userspace.
> 
> It's not a "device being a sysdev", it's sysdevs being used for creating
> a user space interface, which isn't broken by itself.

Yeah , absolutely. But if any device wants to export anything is _must_
be a "struct device" today. If that does not fit, it must not use sysfs
at all.

Kay

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev class and sysdev

[Rafael J. Wysocki]

On Tuesday, March 22, 2011, Kay Sievers wrote:
> On Tue, 2011-03-22 at 21:30 +0100, Rafael J. Wysocki wrote:
> > On Tuesday, March 22, 2011, Kay Sievers wrote:
> > > On Tue, 2011-03-22 at 23:04 +0900, Paul Mundt wrote:
> > > > On Sat, Mar 19, 2011 at 01:47:27AM +0100, Rafael J. Wysocki wrote:
> > > > > On Thursday, March 17, 2011, Paul Mundt wrote:
> > > > > > On Sun, Mar 13, 2011 at 02:03:49PM +0100, R. J. Wysocki wrote:
> > > > > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > > > 
> > > > > > > Convert the SuperH clocks framework and shared interrupt handling
> > > > > > > code to using struct syscore_ops instead of a sysdev classes and
> > > > > > > sysdevs for power managment.
> > > > > > > 
> > > > > > > This reduces the code size significantly and simplifies it.  The
> > > > > > > optimizations causing things not to be restored after creating a
> > > > > > > hibernation image are removed, but they might lead to undesirable
> > > > > > > effects during resume from hibernation (e.g. the clocks would be left
> > > > > > > as the boot kernel set them, which might be not the same way as the
> > > > > > > hibernated kernel had seen them before the hibernation).
> > > > > > > 
> > > > > > > This also is necessary for removing sysdevs from the kernel entirely
> > > > > > > in the future.
> > > > > > > 
> > > > > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > > 
> > > > > > This misses the use of the sysdev class by the userimask code, though I'm
> > > > > > open to suggestions for alternatives.
> > > > > 
> > > > > For now, I'd simply move the sysdev class definition to userimask.c, like
> > > > > in the patch below.  The current goal is to eliminate the suspend/resume and
> > > > > shutdown operations from sysdevs (and sysdev drivers), the next step will
> > > > > be to replace the remaining sysdevs with alternative mechanisms.
> > > > > 
> > > > It's not quite that straightforward, you've also killed off the name
> > > > attribute for each of the intc sysdevs, so we no longer have a visible
> > > > way to map a given intc controller number to the controller name in a
> > > > user visible way.
> > > > 
> > > > I'm not opposed to the syscore thing for suspend/resume ops, but I'm not
> > > > willing to trash the userimask and name mapping interface in the process
> > > > with no alternatives.
> > > > 
> > > > userimask was the first global configuration item I added, but there are
> > > > other per-controller and global configuration knobs that I plan to export
> > > > through the interface, so there really needs to be a compelling reason
> > > > for moving off of sysdevs.
> > > 
> > > Yes, they don't fit into the model. They have been a dumb hack from the
> > > first day, and never integrated into the kenrel driver core or hotplug
> > > properly.
> > > 
> > > If you need the userspace visibility, better just add a "struct
> > > bus_type" with a proper name for your subsystem and register a "struct
> > > device" with the bus_type assigned for all of them, instead of using the
> > > broken concept of sydevs. You can even make them show up
> > > in /sys/devices/system/<bus_type name>/<struct device name>/ if you want
> > > to.
> > 
> > I don't really think that's going to be useful in this particular case.
> > The reason is, first, because the struct device would cause lots of other
> > stuff to show up in sysfs which would be totally redundant and confusing
> > and, second, because the things exported here are simply static attributes,
> > pretty much like the stuff in /sys/power/.
> > 
> > Perhaps there's a more straightforward way to make some files show up in
> > sysfs on a specific path than defininig an otherwise useless bus type and
> > device object?
> 
> That's absolutely not the point. Please don't get yourself into that
> thinking. If people want to "export stuff to userspace", they must not
> invent new things. We need to get rid of the silly special cases.

Why exactly?  Do they actually hurt anyone and if so then how?

> Userspace is not meant to learn subsystem specific rules for every new
> thing.

That depends a good deal of who's writing the user space in question.  If
that's the same person who's working on the particular part of the kernel,
I don't see a big problem.

> There is _one_ way to export device attributes, and that is
> "struct device" today.
> 
> If that's to expensive for anybody, just don't use sysfs. It's the rule
> we have today. :)

Oh, good to know.  It's changed a bit since I last heard.  Never mind.

Still, I won't let you change the things in /sys/power to struct devices,
sorry about that. ;-)

And I wonder how are you going to deal with clocksource exporting things
via the sysdev interface right now.  I'd simply create two directories and
put the two files into them and be done with that, but I guess that
wouldn't fit into the model somehow, right?

> > > That way userspace can properly enumerate them in a flat list
> > > in /sys/bus/<bus_type name>/devices/*, and gets proper events on module
> > > load and during system coldplug, and can hook into the usual hotplug
> > > pathes to set/get these values instead of crawling magicly defined and
> > > decoupled locations in /sys which can not express proper hierarchy,
> > > classicication, or anything else that all other devices can just do.
> > 
> > There's no hotplug involved or anything remotely like that AFAICS.
> > There are simply static files as I said above, they are created
> > early during system initialization and simply stay there.
> 
> That's not the point. It's about a single way to retrieve information
> about devices, extendability, and coldplug during bootup, where existing
> devices need to be handled only after userspace is up.

I'd say the case at hand has nothing to do with that.

> That is just a case of "hotplug" that has the same codepath for userspace,
> even when the devices can never really come and go.

My impression is that when you say "user space", you actually mean some
_specific_ user space, don't you?

> > > There is really no reason for any device being a magic and conceptually
> > > broken sysdev today - just to be different from any other device the
> > > kernel exports to userspace.
> > 
> > It's not a "device being a sysdev", it's sysdevs being used for creating
> > a user space interface, which isn't broken by itself.
> 
> Yeah , absolutely. But if any device wants to export anything is _must_
> be a "struct device" today. If that does not fit, it must not use sysfs
> at all.

Well, it's not a "device wants to export something".  It's platform code
wanting to export some information related to the platform and I really
don't see a reason why it should create bus types and device objects
_specifically_ for that.  It's just too wasteful, both in terms of memory
and time needed for handling that in the device core.

Thanks,
Rafael

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev

[Kay Sievers]

On Tue, 2011-03-22 at 22:00 +0100, Rafael J. Wysocki wrote:
> On Tuesday, March 22, 2011, Kay Sievers wrote:
> > On Tue, 2011-03-22 at 21:30 +0100, Rafael J. Wysocki wrote:
> > > 
> > > Perhaps there's a more straightforward way to make some files show up in
> > > sysfs on a specific path than defininig an otherwise useless bus type and
> > > device object?
> > 
> > That's absolutely not the point. Please don't get yourself into that
> > thinking. If people want to "export stuff to userspace", they must not
> > invent new things. We need to get rid of the silly special cases.
> 
> Why exactly?  Do they actually hurt anyone and if so then how?

Sure, "devices" are devices, and devices have well-defines set of
properties, not some magic directory, people can mess around with the
way they like.

> > Userspace is not meant to learn subsystem specific rules for every new
> > thing.
> 
> That depends a good deal of who's writing the user space in question.  If
> that's the same person who's working on the particular part of the kernel,
> I don't see a big problem.

Not for "devices". There are rules for devices, which are defined by the
driver core, and the sysdev stuff needs to go, because it does not fit
into that model.

> > There is _one_ way to export device attributes, and that is
> > "struct device" today.
> > 
> > If that's to expensive for anybody, just don't use sysfs. It's the rule
> > we have today. :)
> 
> Oh, good to know.  It's changed a bit since I last heard.  Never mind.

Oh, don't get me wrong, this is all is about "devices" not any other
controls.

> Still, I won't let you change the things in /sys/power to struct devices,
> sorry about that. ;-)

Fine as long as they are power specific things, and not "devices". You
don't have sysdevs there, right? :)

> And I wonder how are you going to deal with clocksource exporting things
> via the sysdev interface right now.  I'd simply create two directories and
> put the two files into them and be done with that, but I guess that
> wouldn't fit into the model somehow, right?

Nope, register a bus_type, and use struct device for all of them, Parent
them to /sys/devices/system/ if they should keep their location and
layout.

> > > > That way userspace can properly enumerate them in a flat list
> > > > in /sys/bus/<bus_type name>/devices/*, and gets proper events on module
> > > > load and during system coldplug, and can hook into the usual hotplug
> > > > pathes to set/get these values instead of crawling magicly defined and
> > > > decoupled locations in /sys which can not express proper hierarchy,
> > > > classicication, or anything else that all other devices can just do.
> > > 
> > > There's no hotplug involved or anything remotely like that AFAICS.
> > > There are simply static files as I said above, they are created
> > > early during system initialization and simply stay there.
> > 
> > That's not the point. It's about a single way to retrieve information
> > about devices, extendability, and coldplug during bootup, where existing
> > devices need to be handled only after userspace is up.
> 
> I'd say the case at hand has nothing to do with that.

It has. As for CPUs. We can not do proper CPU-dependent module
autoloading, because the events happen before userspace runs, and
clodplug can not see the broken sysdevs, because they have no events to
re-trigger, like all others have.

> > That is just a case of "hotplug" that has the same codepath for userspace,
> > even when the devices can never really come and go.
> 
> My impression is that when you say "user space", you actually mean some
> _specific_ user space, don't you?

On usual boxes it's udev/libudev and all the stuff around it. But
andreoid has the same stuff in their own way of doing it. So it's not
about an implementation in userspace, it's about a sane event and
classification interface for kernel-exported devices. Again tis is not
about any other stuff in /sys, only the "devices", and we want to have
only a single type, and a single way to handle it in userspace.

> > > > There is really no reason for any device being a magic and conceptually
> > > > broken sysdev today - just to be different from any other device the
> > > > kernel exports to userspace.
> > > 
> > > It's not a "device being a sysdev", it's sysdevs being used for creating
> > > a user space interface, which isn't broken by itself.
> > 
> > Yeah , absolutely. But if any device wants to export anything is _must_
> > be a "struct device" today. If that does not fit, it must not use sysfs
> > at all.
> 
> Well, it's not a "device wants to export something".  It's platform code
> wanting to export some information related to the platform and I really
> don't see a reason why it should create bus types and device objects
> _specifically_ for that.  It's just too wasteful, both in terms of memory
> and time needed for handling that in the device core.

Because they are devices, and there is a lot to win, if the kernel
exports all "devices" in the same way. This is not about saving an inode
in /sys, it's the ability to do runtime device configuration with common
tools.

Kay

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev class and sysdev

[Paul Mundt]

On Tue, Mar 22, 2011 at 10:12:36PM +0100, Kay Sievers wrote:
> On Tue, 2011-03-22 at 22:00 +0100, Rafael J. Wysocki wrote:
> > On Tuesday, March 22, 2011, Kay Sievers wrote:
> > > On Tue, 2011-03-22 at 21:30 +0100, Rafael J. Wysocki wrote:
> > > > 
> > > > Perhaps there's a more straightforward way to make some files show up in
> > > > sysfs on a specific path than defininig an otherwise useless bus type and
> > > > device object?
> > > 
> > > That's absolutely not the point. Please don't get yourself into that
> > > thinking. If people want to "export stuff to userspace", they must not
> > > invent new things. We need to get rid of the silly special cases.
> > 
> > Why exactly?  Do they actually hurt anyone and if so then how?
> 
> Sure, "devices" are devices, and devices have well-defines set of
> properties, not some magic directory, people can mess around with the
> way they like.
> 
This is starting to get silly. In the case of interrupt controllers as
with clocksources the bus abstraction is completely meaningless. I could
register a type of "system" bus, but how would that be any different from
what we have from sysdevs already today? All it does is force people to
duplicate crap all over the place instead and pretend like extra busses
exist in order to fit some arbitrary definition of how the driver model
should work.

You talk about inventing special interfaces to bypass the device model,
but that's not the case here. Rolling my own interface with kobjects and
attribute groups as with /sys/power or making an arbitrary bus type for a
single class of system devices seems infinitely more hackish than the
current sysdev model.

The comment at the top of sys.c says:

 * sys.c - pseudo-bus for system 'devices' (cpus, PICs, timers, etc)

Which is precisely where I would expect interrupt controllers and timers
and CPUs to go. I'm not going to make an IRQ bus or a timer bus and
arbitrarily map some things there and some things somewhere else in the
name of some abstraction insanity. These interrupt controllers all have
consistent attributes that make the sysdev class model work well, but
there are also many other types of interrupt controllers on the same CPUs
that use a different abstraction.

Beyond that, we also have sysdev class utilization for DMA controllers,
per-CPU store queues, etc, etc. all of which would need to be converted
to something else (see for example arch/sh/kernel/cpu/sh4/sq.c -- which
in turn was modelled after the cpufreq code, which also would need to
change to something else). It's not entirely obvious how to convert these
things, or why one should even bother. I can live with the struct device
overhead even if I find it to be a meaningless abstraction in this case,
but what sort of bus/class model to shoe horn these things in to is
rather beyond me.

Indeed it seems to me that these are precisely the sorts of things that
sysdevs are intended for, which is why I elected to use them from the
onset. Simply saying "don't use sysdevs" and "pretend like you have some
sort of a magical pseudo-bus that's not a system bus" doesn't quite do it
for me.

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev

[Kay Sievers]

On Wed, 2011-03-23 at 06:49 +0900, Paul Mundt wrote:
> On Tue, Mar 22, 2011 at 10:12:36PM +0100, Kay Sievers wrote:
> > On Tue, 2011-03-22 at 22:00 +0100, Rafael J. Wysocki wrote:
> > > On Tuesday, March 22, 2011, Kay Sievers wrote:
> > > > On Tue, 2011-03-22 at 21:30 +0100, Rafael J. Wysocki wrote:
> > > > > 
> > > > > Perhaps there's a more straightforward way to make some files show up in
> > > > > sysfs on a specific path than defininig an otherwise useless bus type and
> > > > > device object?
> > > > 
> > > > That's absolutely not the point. Please don't get yourself into that
> > > > thinking. If people want to "export stuff to userspace", they must not
> > > > invent new things. We need to get rid of the silly special cases.
> > > 
> > > Why exactly?  Do they actually hurt anyone and if so then how?
> > 
> > Sure, "devices" are devices, and devices have well-defines set of
> > properties, not some magic directory, people can mess around with the
> > way they like.
> > 
> This is starting to get silly. In the case of interrupt controllers as
> with clocksources the bus abstraction is completely meaningless. I could
> register a type of "system" bus, but how would that be any different from
> what we have from sysdevs already today? All it does is force people to
> duplicate crap all over the place instead and pretend like extra busses
> exist in order to fit some arbitrary definition of how the driver model
> should work.
> 
> You talk about inventing special interfaces to bypass the device model,
> but that's not the case here. Rolling my own interface with kobjects and
> attribute groups as with /sys/power or making an arbitrary bus type for a
> single class of system devices seems infinitely more hackish than the
> current sysdev model.
> 
> The comment at the top of sys.c says:
> 
>  * sys.c - pseudo-bus for system 'devices' (cpus, PICs, timers, etc)

Which is what we need to get rid of. It does not make any sense on the
global picture to have anything like that exported to userspace.

> Which is precisely where I would expect interrupt controllers and timers
> and CPUs to go. I'm not going to make an IRQ bus or a timer bus and
> arbitrarily map some things there and some things somewhere else in the
> name of some abstraction insanity. These interrupt controllers all have
> consistent attributes that make the sysdev class model work well, but
> there are also many other types of interrupt controllers on the same CPUs
> that use a different abstraction.
> 
> Beyond that, we also have sysdev class utilization for DMA controllers,
> per-CPU store queues, etc, etc. all of which would need to be converted
> to something else (see for example arch/sh/kernel/cpu/sh4/sq.c -- which
> in turn was modelled after the cpufreq code, which also would need to
> change to something else). It's not entirely obvious how to convert these
> things, or why one should even bother. I can live with the struct device
> overhead even if I find it to be a meaningless abstraction in this case,
> but what sort of bus/class model to shoe horn these things in to is
> rather beyond me.
> 
> Indeed it seems to me that these are precisely the sorts of things that
> sysdevs are intended for, which is why I elected to use them from the
> onset. Simply saying "don't use sysdevs" and "pretend like you have some
> sort of a magical pseudo-bus that's not a system bus" doesn't quite do it
> for me.

Nope, a device has a "name", a "subsystem" and a "devpath", has
well-defined core-maintained properties at the device directory. It is
not some random custom directory which people can put where they like
it. Userspace has expectations about devices which need to be met, and
that can only happen if these devices are "struct device".

All real devices sort into a hierarchy, possibly in different parent
locations, and have a single point of classification which is the
devices/ directory and contains symlinks. Only that way we can cope with
it in userspace.

People should really stop messing around in /sys for optimization
purposes. We have a common device model, and need to use it. Sysdevs do
not fit into that model.

I can't tell how that fits into your use case, but please use something
else than sysfs, if you need device information exported, but you can't
use "struct device".

Thanks,
Kay

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev class and sysdev

[Rafael J. Wysocki]

On Tuesday, March 22, 2011, Kay Sievers wrote:
> On Tue, 2011-03-22 at 22:00 +0100, Rafael J. Wysocki wrote:
> > On Tuesday, March 22, 2011, Kay Sievers wrote:
> > > On Tue, 2011-03-22 at 21:30 +0100, Rafael J. Wysocki wrote:
> > > > 
> > > > Perhaps there's a more straightforward way to make some files show up in
> > > > sysfs on a specific path than defininig an otherwise useless bus type and
> > > > device object?
> > > 
> > > That's absolutely not the point. Please don't get yourself into that
> > > thinking. If people want to "export stuff to userspace", they must not
> > > invent new things. We need to get rid of the silly special cases.
> > 
> > Why exactly?  Do they actually hurt anyone and if so then how?
> 
> Sure, "devices" are devices, and devices have well-defines set of
> properties, not some magic directory, people can mess around with the
> way they like.

So it looks like the the problem is that the exported attributes happen to
be under /sys/devices/.  Would it still be a problem if they were somewhere
else?

> > > Userspace is not meant to learn subsystem specific rules for every new
> > > thing.
> > 
> > That depends a good deal of who's writing the user space in question.  If
> > that's the same person who's working on the particular part of the kernel,
> > I don't see a big problem.
> 
> Not for "devices". There are rules for devices, which are defined by the
> driver core, and the sysdev stuff needs to go, because it does not fit
> into that model.

OK, I understand that.

Now, there's stuff that doesn't really match the "device" model.  Where is
the right place to export that?  Perhaps we should add something like
/sys/platform/ (in analogy with /sys/firmware/)?

> > > There is _one_ way to export device attributes, and that is
> > > "struct device" today.
> > > 
> > > If that's to expensive for anybody, just don't use sysfs. It's the rule
> > > we have today. :)
> > 
> > Oh, good to know.  It's changed a bit since I last heard.  Never mind.
> 
> Oh, don't get me wrong, this is all is about "devices" not any other
> controls.
>
> > Still, I won't let you change the things in /sys/power to struct devices,
> > sorry about that. ;-)
> 
> Fine as long as they are power specific things, and not "devices". You
> don't have sysdevs there, right? :)

No, I don't.

> > And I wonder how are you going to deal with clocksource exporting things
> > via the sysdev interface right now.  I'd simply create two directories and
> > put the two files into them and be done with that, but I guess that
> > wouldn't fit into the model somehow, right?
> 
> Nope, register a bus_type, and use struct device for all of them, Parent
> them to /sys/devices/system/ if they should keep their location and
> layout.

Well, I'll be watching what happens to the patch trying to do that, but I'm
not going to bet anything on its success. ;-)

> > > > > That way userspace can properly enumerate them in a flat list
> > > > > in /sys/bus/<bus_type name>/devices/*, and gets proper events on module
> > > > > load and during system coldplug, and can hook into the usual hotplug
> > > > > pathes to set/get these values instead of crawling magicly defined and
> > > > > decoupled locations in /sys which can not express proper hierarchy,
> > > > > classicication, or anything else that all other devices can just do.
> > > > 
> > > > There's no hotplug involved or anything remotely like that AFAICS.
> > > > There are simply static files as I said above, they are created
> > > > early during system initialization and simply stay there.
> > > 
> > > That's not the point. It's about a single way to retrieve information
> > > about devices, extendability, and coldplug during bootup, where existing
> > > devices need to be handled only after userspace is up.
> > 
> > I'd say the case at hand has nothing to do with that.
> 
> It has. As for CPUs. We can not do proper CPU-dependent module
> autoloading, because the events happen before userspace runs, and
> clodplug can not see the broken sysdevs, because they have no events to
> re-trigger, like all others have.

Well, as I said, would it be OK if the things in question happened to be
located somewhere outside of /sys/devices/ ?

> > > That is just a case of "hotplug" that has the same codepath for userspace,
> > > even when the devices can never really come and go.
> > 
> > My impression is that when you say "user space", you actually mean some
> > _specific_ user space, don't you?
> 
> On usual boxes it's udev/libudev and all the stuff around it. But
> andreoid has the same stuff in their own way of doing it. So it's not
> about an implementation in userspace, it's about a sane event and
> classification interface for kernel-exported devices. Again tis is not
> about any other stuff in /sys, only the "devices", and we want to have
> only a single type, and a single way to handle it in userspace.

OK

> > > > > There is really no reason for any device being a magic and conceptually
> > > > > broken sysdev today - just to be different from any other device the
> > > > > kernel exports to userspace.
> > > > 
> > > > It's not a "device being a sysdev", it's sysdevs being used for creating
> > > > a user space interface, which isn't broken by itself.
> > > 
> > > Yeah , absolutely. But if any device wants to export anything is _must_
> > > be a "struct device" today. If that does not fit, it must not use sysfs
> > > at all.
> > 
> > Well, it's not a "device wants to export something".  It's platform code
> > wanting to export some information related to the platform and I really
> > don't see a reason why it should create bus types and device objects
> > _specifically_ for that.  It's just too wasteful, both in terms of memory
> > and time needed for handling that in the device core.
> 
> Because they are devices, and there is a lot to win, if the kernel
> exports all "devices" in the same way. This is not about saving an inode
> in /sys, it's the ability to do runtime device configuration with common
> tools.

If I understand you correctly, the goal is to have everything under
/sys/devices/ been represented by struct device objects and there are good
reasons to do that.

In that case we either have to move the things exported via sysdevs somewhere
else (presumably having to create that "somewhere" before), or we have to
introduce struct device objects specifically for exporting them.  I don't
really think the latter approach will be very popular, so quite likely we'll
need to have a plan for moving those things to different locations.

Thanks,
Rafael

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev

[Kay Sievers]

On Tue, 2011-03-22 at 23:05 +0100, Rafael J. Wysocki wrote:
> On Tuesday, March 22, 2011, Kay Sievers wrote:
> > On Tue, 2011-03-22 at 22:00 +0100, Rafael J. Wysocki wrote:
> > > On Tuesday, March 22, 2011, Kay Sievers wrote:
> > > > On Tue, 2011-03-22 at 21:30 +0100, Rafael J. Wysocki wrote:
> > > > > 
> > > > > Perhaps there's a more straightforward way to make some files show up in
> > > > > sysfs on a specific path than defininig an otherwise useless bus type and
> > > > > device object?
> > > > 
> > > > That's absolutely not the point. Please don't get yourself into that
> > > > thinking. If people want to "export stuff to userspace", they must not
> > > > invent new things. We need to get rid of the silly special cases.
> > > 
> > > Why exactly?  Do they actually hurt anyone and if so then how?
> > 
> > Sure, "devices" are devices, and devices have well-defines set of
> > properties, not some magic directory, people can mess around with the
> > way they like.
> 
> So it looks like the the problem is that the exported attributes happen to
> be under /sys/devices/.  Would it still be a problem if they were somewhere
> else?

We are not going to invent another location for any devices. They need
to stay in /devices if they are devices. And all devices need to be
"struct device".

> > > > Userspace is not meant to learn subsystem specific rules for every new
> > > > thing.
> > > 
> > > That depends a good deal of who's writing the user space in question.  If
> > > that's the same person who's working on the particular part of the kernel,
> > > I don't see a big problem.
> > 
> > Not for "devices". There are rules for devices, which are defined by the
> > driver core, and the sysdev stuff needs to go, because it does not fit
> > into that model.
> 
> OK, I understand that.
> 
> Now, there's stuff that doesn't really match the "device" model.  Where is
> the right place to export that?  Perhaps we should add something like
> /sys/platform/ (in analogy with /sys/firmware/)?

No, add a subsystem (bus_type) for any of them, and register them. There
is no such thing as "devices which do not fit the device model", they
are all fine there. Please stop optimizing single bytes and creating a
mess in /sys. Every device is a "struct device".

Think of "struct bus_type" as "struct subsystem", we will rename that
when we are ready. It is just a group of devices which are of the same
type, it has nothing to do with a bus in the sense of hardware.

We need unified exports of _all devices to userspace, not custom layouts
in /sys.

There's is a pretty much outdated Documentation/sysfs-rules.txt, wich
covers part of the history and the plans.

> > > > There is _one_ way to export device attributes, and that is
> > > > "struct device" today.
> > > > 
> > > > If that's to expensive for anybody, just don't use sysfs. It's the rule
> > > > we have today. :)
> > > 
> > > Oh, good to know.  It's changed a bit since I last heard.  Never mind.
> > 
> > Oh, don't get me wrong, this is all is about "devices" not any other
> > controls.
> >
> > > Still, I won't let you change the things in /sys/power to struct devices,
> > > sorry about that. ;-)
> > 
> > Fine as long as they are power specific things, and not "devices". You
> > don't have sysdevs there, right? :)
> 
> No, I don't.

Then all is fine. All other stuff is more like /proc, and can never be
really unified. All we care about is devices, which have common methods
for userspace to trigger and consume, and need to be unified. Power
specific control files seems all fine in its kobject use.

> > > And I wonder how are you going to deal with clocksource exporting things
> > > via the sysdev interface right now.  I'd simply create two directories and
> > > put the two files into them and be done with that, but I guess that
> > > wouldn't fit into the model somehow, right?
> > 
> > Nope, register a bus_type, and use struct device for all of them, Parent
> > them to /sys/devices/system/ if they should keep their location and
> > layout.
> 
> Well, I'll be watching what happens to the patch trying to do that, but I'm
> not going to bet anything on its success. ;-)

It should be pretty straight-forward. We will need to do that for CPUs I
guess, because the interface is kinda commonly used.

> > > > > > That way userspace can properly enumerate them in a flat list
> > > > > > in /sys/bus/<bus_type name>/devices/*, and gets proper events on module
> > > > > > load and during system coldplug, and can hook into the usual hotplug
> > > > > > pathes to set/get these values instead of crawling magicly defined and
> > > > > > decoupled locations in /sys which can not express proper hierarchy,
> > > > > > classicication, or anything else that all other devices can just do.
> > > > > 
> > > > > There's no hotplug involved or anything remotely like that AFAICS.
> > > > > There are simply static files as I said above, they are created
> > > > > early during system initialization and simply stay there.
> > > > 
> > > > That's not the point. It's about a single way to retrieve information
> > > > about devices, extendability, and coldplug during bootup, where existing
> > > > devices need to be handled only after userspace is up.
> > > 
> > > I'd say the case at hand has nothing to do with that.
> > 
> > It has. As for CPUs. We can not do proper CPU-dependent module
> > autoloading, because the events happen before userspace runs, and
> > clodplug can not see the broken sysdevs, because they have no events to
> > re-trigger, like all others have.
> 
> Well, as I said, would it be OK if the things in question happened to be
> located somewhere outside of /sys/devices/ ?

No, no device directory can be outside of /sys/devices.

> > > > That is just a case of "hotplug" that has the same codepath for userspace,
> > > > even when the devices can never really come and go.
> > > 
> > > My impression is that when you say "user space", you actually mean some
> > > _specific_ user space, don't you?
> > 
> > On usual boxes it's udev/libudev and all the stuff around it. But
> > andreoid has the same stuff in their own way of doing it. So it's not
> > about an implementation in userspace, it's about a sane event and
> > classification interface for kernel-exported devices. Again tis is not
> > about any other stuff in /sys, only the "devices", and we want to have
> > only a single type, and a single way to handle it in userspace.
> 
> OK
> 
> > > > > > There is really no reason for any device being a magic and conceptually
> > > > > > broken sysdev today - just to be different from any other device the
> > > > > > kernel exports to userspace.
> > > > > 
> > > > > It's not a "device being a sysdev", it's sysdevs being used for creating
> > > > > a user space interface, which isn't broken by itself.
> > > > 
> > > > Yeah , absolutely. But if any device wants to export anything is _must_
> > > > be a "struct device" today. If that does not fit, it must not use sysfs
> > > > at all.
> > > 
> > > Well, it's not a "device wants to export something".  It's platform code
> > > wanting to export some information related to the platform and I really
> > > don't see a reason why it should create bus types and device objects
> > > _specifically_ for that.  It's just too wasteful, both in terms of memory
> > > and time needed for handling that in the device core.
> > 
> > Because they are devices, and there is a lot to win, if the kernel
> > exports all "devices" in the same way. This is not about saving an inode
> > in /sys, it's the ability to do runtime device configuration with common
> > tools.
> 
> If I understand you correctly, the goal is to have everything under
> /sys/devices/ been represented by struct device objects and there are good
> reasons to do that.
> 
> In that case we either have to move the things exported via sysdevs somewhere
> else (presumably having to create that "somewhere" before), or we have to
> introduce struct device objects specifically for exporting them.  I don't
> really think the latter approach will be very popular, so quite likely we'll
> need to have a plan for moving those things to different locations.

We can just keep the same location for anything that is expected to be
in /sys/devices/system. All outside visible difference is that these
devices (then struct device) also have a "subsystem" (bus_type) which
carries the flat list of devices, the devices have a "uevent" file and
with that they are coldpluggable, event re-triggerable, could have
modalias, ... and all the other 1000 things that just work today, and
what we need for many of them. They are then just handled like any other
exported device too.

Kay

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev class and sysdev

[Rafael J. Wysocki]

On Tuesday, March 22, 2011, Kay Sievers wrote:
> On Wed, 2011-03-23 at 06:49 +0900, Paul Mundt wrote:
...
> > You talk about inventing special interfaces to bypass the device model,
> > but that's not the case here. Rolling my own interface with kobjects and
> > attribute groups as with /sys/power or making an arbitrary bus type for a
> > single class of system devices seems infinitely more hackish than the
> > current sysdev model.
> > 
> > The comment at the top of sys.c says:
> > 
> >  * sys.c - pseudo-bus for system 'devices' (cpus, PICs, timers, etc)
> > 
> > Which is precisely where I would expect interrupt controllers and timers
> > and CPUs to go. I'm not going to make an IRQ bus or a timer bus and
> > arbitrarily map some things there and some things somewhere else in the
> > name of some abstraction insanity. These interrupt controllers all have
> > consistent attributes that make the sysdev class model work well, but
> > there are also many other types of interrupt controllers on the same CPUs
> > that use a different abstraction.
> > 
> > Beyond that, we also have sysdev class utilization for DMA controllers,
> > per-CPU store queues, etc, etc. all of which would need to be converted
> > to something else (see for example arch/sh/kernel/cpu/sh4/sq.c -- which
> > in turn was modelled after the cpufreq code, which also would need to
> > change to something else). It's not entirely obvious how to convert these
> > things, or why one should even bother.

The reason why is, if I understand it correctly, because user space tools
generally expect /sys/devices/ to be consistent in terms of the representation
of things and /sys/devices/system/ currently violates that expectation which
leads to all sorts of problems with device discovery, hotplug etc.

Now, whether or not to convert all the things currently exported through
sysdevs to struct device objects is not too obvious to me.  I think it simply
may be better to move them into a different direcory in sysfs (presumably
creating one for this purpose).

> > I can live with the struct device overhead even if I find it to be a
> > meaningless abstraction in this case, but what sort of bus/class model to
> > shoe horn these things in to is rather beyond me.
> > 
> > Indeed it seems to me that these are precisely the sorts of things that
> > sysdevs are intended for, which is why I elected to use them from the
> > onset. Simply saying "don't use sysdevs" and "pretend like you have some
> > sort of a magical pseudo-bus that's not a system bus" doesn't quite do it
> > for me.
> 
> Nope, a device has a "name", a "subsystem" and a "devpath", has
> well-defined core-maintained properties at the device directory. It is
> not some random custom directory which people can put where they like
> it. Userspace has expectations about devices which need to be met, and
> that can only happen if these devices are "struct device".
> 
> All real devices sort into a hierarchy, possibly in different parent
> locations, and have a single point of classification which is the
> devices/ directory and contains symlinks. Only that way we can cope with
> it in userspace.
> 
> People should really stop messing around in /sys for optimization
> purposes. We have a common device model, and need to use it. Sysdevs do
> not fit into that model.
> 
> I can't tell how that fits into your use case, but please use something
> else than sysfs, if you need device information exported, but you can't
> use "struct device".

I really think you shouldn't say "sysfs" when you in fact you mean
"/sys/devices/". :-)

Now, I can easily understand arguments about representing everything under
/sys/devices/ by struct device objects, no question about that.  However,
I also think there should be a place for things like those mentioned in the
comment in sys.c, presumably outside of /sys/devices/.

Thanks,
Rafael

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev class and sysdev

[Paul Mundt]

On Tue, Mar 22, 2011 at 11:00:56PM +0100, Kay Sievers wrote:
> On Wed, 2011-03-23 at 06:49 +0900, Paul Mundt wrote:
> > The comment at the top of sys.c says:
> > 
> >  * sys.c - pseudo-bus for system 'devices' (cpus, PICs, timers, etc)
> 
> Which is what we need to get rid of. It does not make any sense on the
> global picture to have anything like that exported to userspace.
> 
So far I haven't heard any rationale for why it doesn't. Exporting CPU
state to userspace certainly makes sense, and the sysdev model has worked
reasonably for CPUs, memory nodes, etc.

> People should really stop messing around in /sys for optimization
> purposes. We have a common device model, and need to use it. Sysdevs do
> not fit into that model.
> 
> I can't tell how that fits into your use case, but please use something
> else than sysfs, if you need device information exported, but you can't
> use "struct device".
> 
As long as CPU state is present in sysfs people will be tied to it for
per-CPU kobjects and the like, and until something concrete is proposed
for what to do about these cases there's not much chance of sysdevs going
away.

Once cpufreq, timekeeping, and NUMA node state have migrated to whatever
the driver model folks find acceptable, I'll happily follow suit.

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev class and sysdev

[Rafael J. Wysocki]

On Tuesday, March 22, 2011, Kay Sievers wrote:
> On Tue, 2011-03-22 at 23:05 +0100, Rafael J. Wysocki wrote:
> > On Tuesday, March 22, 2011, Kay Sievers wrote:
> > > On Tue, 2011-03-22 at 22:00 +0100, Rafael J. Wysocki wrote:
> > > > On Tuesday, March 22, 2011, Kay Sievers wrote:
> > > > > On Tue, 2011-03-22 at 21:30 +0100, Rafael J. Wysocki wrote:
> > > > > > 
> > > > > > Perhaps there's a more straightforward way to make some files show up in
> > > > > > sysfs on a specific path than defininig an otherwise useless bus type and
> > > > > > device object?
> > > > > 
> > > > > That's absolutely not the point. Please don't get yourself into that
> > > > > thinking. If people want to "export stuff to userspace", they must not
> > > > > invent new things. We need to get rid of the silly special cases.
> > > > 
> > > > Why exactly?  Do they actually hurt anyone and if so then how?
> > > 
> > > Sure, "devices" are devices, and devices have well-defines set of
> > > properties, not some magic directory, people can mess around with the
> > > way they like.
> > 
> > So it looks like the the problem is that the exported attributes happen to
> > be under /sys/devices/.  Would it still be a problem if they were somewhere
> > else?
> 
> We are not going to invent another location for any devices. They need
> to stay in /devices if they are devices. And all devices need to be
> "struct device".

_They_ _are_ _not_ _devices_.

Please take clocksource as an example.  It needs to export two attributes,
available_clocksource and current_clocksource, which are _useful_ user space
interfaces.  Why the heck are you trying to convince me it's a good idea to
create a special bus type and struct device _specifically_ for exporting them?!

Moreover, is there anything device-alike in those two attributes?  I don't
really think so.

So please stop arguing this way, because it simply isn't going to fly and
people will always say a big fat "no" to such ideas, for a good reason.

> > > > > Userspace is not meant to learn subsystem specific rules for every new
> > > > > thing.
> > > > 
> > > > That depends a good deal of who's writing the user space in question.  If
> > > > that's the same person who's working on the particular part of the kernel,
> > > > I don't see a big problem.
> > > 
> > > Not for "devices". There are rules for devices, which are defined by the
> > > driver core, and the sysdev stuff needs to go, because it does not fit
> > > into that model.
> > 
> > OK, I understand that.
> > 
> > Now, there's stuff that doesn't really match the "device" model.  Where is
> > the right place to export that?  Perhaps we should add something like
> > /sys/platform/ (in analogy with /sys/firmware/)?
> 
> No, add a subsystem (bus_type) for any of them, and register them. There
> is no such thing as "devices which do not fit the device model", they
> are all fine there. Please stop optimizing single bytes and creating a
> mess in /sys. Every device is a "struct device".

Again.  Those things are _not_ devices.  Am I not clear enough?

> Think of "struct bus_type" as "struct subsystem", we will rename that
> when we are ready. It is just a group of devices which are of the same
> type, it has nothing to do with a bus in the sense of hardware.
> 
> We need unified exports of _all devices to userspace, not custom layouts
> in /sys.
> 
> There's is a pretty much outdated Documentation/sysfs-rules.txt, wich
> covers part of the history and the plans.

You seem to be thinking that anything exported through sysfs needs to be
device, which I don't think is event approximately correct (what about
/sys/firmware/ or /sys/kernel/ or /sys/fs/ , for a few examples?).

Think this way: it is useful (and IMHO correct) to export some things to
user space that without necessarily regarding them as "devices", physical
or not.  Some of them _happen_ to be exported through sysdevs, but that
doesn't really mean the _are_ devices.  They are simply _software_ interfaces
to things that have no device representation and don't _need_ one.

> > > > > There is _one_ way to export device attributes, and that is
> > > > > "struct device" today.
> > > > > 
> > > > > If that's to expensive for anybody, just don't use sysfs. It's the rule
> > > > > we have today. :)
> > > > 
> > > > Oh, good to know.  It's changed a bit since I last heard.  Never mind.
> > > 
> > > Oh, don't get me wrong, this is all is about "devices" not any other
> > > controls.
> > >
> > > > Still, I won't let you change the things in /sys/power to struct devices,
> > > > sorry about that. ;-)
> > > 
> > > Fine as long as they are power specific things, and not "devices". You
> > > don't have sysdevs there, right? :)
> > 
> > No, I don't.
> 
> Then all is fine. All other stuff is more like /proc, and can never be
> really unified.

YES!  And _that_'s precisely what I'm (and Paul is) talking about.

> All we care about is devices, which have common methods
> for userspace to trigger and consume, and need to be unified. Power
> specific control files seems all fine in its kobject use.

I understand that, really.

> > > > And I wonder how are you going to deal with clocksource exporting things
> > > > via the sysdev interface right now.  I'd simply create two directories and
> > > > put the two files into them and be done with that, but I guess that
> > > > wouldn't fit into the model somehow, right?
> > > 
> > > Nope, register a bus_type, and use struct device for all of them, Parent
> > > them to /sys/devices/system/ if they should keep their location and
> > > layout.
> > 
> > Well, I'll be watching what happens to the patch trying to do that, but I'm
> > not going to bet anything on its success. ;-)
> 
> It should be pretty straight-forward. We will need to do that for CPUs I
> guess, because the interface is kinda commonly used.

No.  CPUs are _very_ special.

> > > > > > > That way userspace can properly enumerate them in a flat list
> > > > > > > in /sys/bus/<bus_type name>/devices/*, and gets proper events on module
> > > > > > > load and during system coldplug, and can hook into the usual hotplug
> > > > > > > pathes to set/get these values instead of crawling magicly defined and
> > > > > > > decoupled locations in /sys which can not express proper hierarchy,
> > > > > > > classicication, or anything else that all other devices can just do.
> > > > > > 
> > > > > > There's no hotplug involved or anything remotely like that AFAICS.
> > > > > > There are simply static files as I said above, they are created
> > > > > > early during system initialization and simply stay there.
> > > > > 
> > > > > That's not the point. It's about a single way to retrieve information
> > > > > about devices, extendability, and coldplug during bootup, where existing
> > > > > devices need to be handled only after userspace is up.
> > > > 
> > > > I'd say the case at hand has nothing to do with that.
> > > 
> > > It has. As for CPUs. We can not do proper CPU-dependent module
> > > autoloading, because the events happen before userspace runs, and
> > > clodplug can not see the broken sysdevs, because they have no events to
> > > re-trigger, like all others have.
> > 
> > Well, as I said, would it be OK if the things in question happened to be
> > located somewhere outside of /sys/devices/ ?
> 
> No, no device directory can be outside of /sys/devices.

Sorry, I'm repeating that for the last time.  I'm not talking about devices.
I'm talking about _totally_ _random_ _stuff_ which is "like /proc, and can
never be really unified" (your own words) which _happens_ to be exported
through the sysdev interface, because that happend to be _easy_ at one point.
Can we agree on that at least?

Thanks,
Rafael

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev

[Kay Sievers]

On Tue, 2011-03-22 at 23:23 +0100, Rafael J. Wysocki wrote:
> On Tuesday, March 22, 2011, Kay Sievers wrote:
> > On Wed, 2011-03-23 at 06:49 +0900, Paul Mundt wrote:
> ...
> > > You talk about inventing special interfaces to bypass the device model,
> > > but that's not the case here. Rolling my own interface with kobjects and
> > > attribute groups as with /sys/power or making an arbitrary bus type for a
> > > single class of system devices seems infinitely more hackish than the
> > > current sysdev model.
> > > 
> > > The comment at the top of sys.c says:
> > > 
> > >  * sys.c - pseudo-bus for system 'devices' (cpus, PICs, timers, etc)
> > > 
> > > Which is precisely where I would expect interrupt controllers and timers
> > > and CPUs to go. I'm not going to make an IRQ bus or a timer bus and
> > > arbitrarily map some things there and some things somewhere else in the
> > > name of some abstraction insanity. These interrupt controllers all have
> > > consistent attributes that make the sysdev class model work well, but
> > > there are also many other types of interrupt controllers on the same CPUs
> > > that use a different abstraction.
> > > 
> > > Beyond that, we also have sysdev class utilization for DMA controllers,
> > > per-CPU store queues, etc, etc. all of which would need to be converted
> > > to something else (see for example arch/sh/kernel/cpu/sh4/sq.c -- which
> > > in turn was modelled after the cpufreq code, which also would need to
> > > change to something else). It's not entirely obvious how to convert these
> > > things, or why one should even bother.
> 
> The reason why is, if I understand it correctly, because user space tools
> generally expect /sys/devices/ to be consistent in terms of the representation
> of things and /sys/devices/system/ currently violates that expectation which
> leads to all sorts of problems with device discovery, hotplug etc.
> 
> Now, whether or not to convert all the things currently exported through
> sysdevs to struct device objects is not too obvious to me.  I think it simply
> may be better to move them into a different direcory in sysfs (presumably
> creating one for this purpose).
> 
> > > I can live with the struct device overhead even if I find it to be a
> > > meaningless abstraction in this case, but what sort of bus/class model to
> > > shoe horn these things in to is rather beyond me.
> > > 
> > > Indeed it seems to me that these are precisely the sorts of things that
> > > sysdevs are intended for, which is why I elected to use them from the
> > > onset. Simply saying "don't use sysdevs" and "pretend like you have some
> > > sort of a magical pseudo-bus that's not a system bus" doesn't quite do it
> > > for me.
> > 
> > Nope, a device has a "name", a "subsystem" and a "devpath", has
> > well-defined core-maintained properties at the device directory. It is
> > not some random custom directory which people can put where they like
> > it. Userspace has expectations about devices which need to be met, and
> > that can only happen if these devices are "struct device".
> > 
> > All real devices sort into a hierarchy, possibly in different parent
> > locations, and have a single point of classification which is the
> > devices/ directory and contains symlinks. Only that way we can cope with
> > it in userspace.
> > 
> > People should really stop messing around in /sys for optimization
> > purposes. We have a common device model, and need to use it. Sysdevs do
> > not fit into that model.
> > 
> > I can't tell how that fits into your use case, but please use something
> > else than sysfs, if you need device information exported, but you can't
> > use "struct device".
> 
> I really think you shouldn't say "sysfs" when you in fact you mean
> "/sys/devices/". :-)
> 
> Now, I can easily understand arguments about representing everything under
> /sys/devices/ by struct device objects, no question about that.  However,
> I also think there should be a place for things like those mentioned in the
> comment in sys.c, presumably outside of /sys/devices/.

No, please. We have all we need. Let's do one example, which you might
apply to any other thing, because you never know what's the next big
thing in hardware. We need to be a future-proof-as-possible, and that's
not some second-class out-of-scope sysfs directory.

Lets' take CPUs:
  - they send events when registered
  - they want to export device specific properties
  - userspace wants to take actions when such devices are available

That all fits properly into the driver model in theory. Unless you do
coldplug and bootup a box.

These devices are already there before userspace even starts, hence we
find all these devices and "trigger" an fake uevent for all of them at
bootup. That will match execute all the rules specified for that device,
just as it would be hotplugges in that moment, hence we call it
coldplug, which works for all devices with the hotplug code, even when
they are never hot-pluggable.

What we do for coldplug is that we iterate over all flat lists of
subsystems and find the devices lists and trigger the event by poking in
the "uevent" sysfs file. Now all the sysdevs do not have a subsystem to
find, and do not have a standard "uevent" file.

Back to the CPUs, we have all the nice device directories which could
have all the CPU features in properties we need to make autoloading of
cpufreq, governer, kvm possible (patch exists from Andi Kleen already) 

But these dumb CPU sysfs device directories are completely invisible for
the *usual* logic, and should just join the model and all will just work
out-of-the-box.

When we started to clean up /sys (again only talking about devices, not
other stuff) we had:
  /block/*
  /class/<subsys>/*
  /bus/<subsys>/devices/*
  /devices/system/<subsys>/*
which are 4 different exports of exactly the same thing, a "device".
"Block" we converted to "class" already, "class" will be converted to
"bus", and "bus" will be renamed to "subsystem". All the current names
will be kept as compat symlinks, just as we did for "block". After that,
_all_ devices have a "subsystem" and a subsystem global directory where
people can add custom stuff shared by all devices-of-the-same-type. Ev

You can also argument from the other side, if a kernel device export is
not worth the few bytes of /sys/devices/ and a "subsystem" (struct
bus_type) it should not be in /sys at all, especially not hidden
somehwere outside of /sys/devices when it is something remotely close to
a device.

Kay

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev

[Kay Sievers]

On Tue, 2011-03-22 at 23:42 +0100, Rafael J. Wysocki wrote:
> On Tuesday, March 22, 2011, Kay Sievers wrote:
> > On Tue, 2011-03-22 at 23:05 +0100, Rafael J. Wysocki wrote:
> > > On Tuesday, March 22, 2011, Kay Sievers wrote:
> > > > On Tue, 2011-03-22 at 22:00 +0100, Rafael J. Wysocki wrote:
> > > > > On Tuesday, March 22, 2011, Kay Sievers wrote:
> > > > > > On Tue, 2011-03-22 at 21:30 +0100, Rafael J. Wysocki wrote:
> > > > > > > 
> > > > > > > Perhaps there's a more straightforward way to make some files show up in
> > > > > > > sysfs on a specific path than defininig an otherwise useless bus type and
> > > > > > > device object?
> > > > > > 
> > > > > > That's absolutely not the point. Please don't get yourself into that
> > > > > > thinking. If people want to "export stuff to userspace", they must not
> > > > > > invent new things. We need to get rid of the silly special cases.
> > > > > 
> > > > > Why exactly?  Do they actually hurt anyone and if so then how?
> > > > 
> > > > Sure, "devices" are devices, and devices have well-defines set of
> > > > properties, not some magic directory, people can mess around with the
> > > > way they like.
> > > 
> > > So it looks like the the problem is that the exported attributes happen to
> > > be under /sys/devices/.  Would it still be a problem if they were somewhere
> > > else?
> > 
> > We are not going to invent another location for any devices. They need
> > to stay in /devices if they are devices. And all devices need to be
> > "struct device".
> 
> _They_ _are_ _not_ _devices_.

Ah, I see. Then they should not ever have been created as a sysdev in
the first place. I thought you did only talk about stuff that needs
pm_ops. If they don't do anything like a device, they need to go
somewhere else, yes.

> Please take clocksource as an example.  It needs to export two attributes,
> available_clocksource and current_clocksource, which are _useful_ user space
> interfaces.  Why the heck are you trying to convince me it's a good idea to
> create a special bus type and struct device _specifically_ for exporting them?!
> 
> Moreover, is there anything device-alike in those two attributes?  I don't
> really think so.
> 
> So please stop arguing this way, because it simply isn't going to fly and
> people will always say a big fat "no" to such ideas, for a good reason.

I never expected anything like that to use a sysdev. I'm not tryin gto
convince you about anything, I'm just surprised what people do, well I'm
not after all the years. :)

> > > > > > Userspace is not meant to learn subsystem specific rules for every new
> > > > > > thing.
> > > > > 
> > > > > That depends a good deal of who's writing the user space in question.  If
> > > > > that's the same person who's working on the particular part of the kernel,
> > > > > I don't see a big problem.
> > > > 
> > > > Not for "devices". There are rules for devices, which are defined by the
> > > > driver core, and the sysdev stuff needs to go, because it does not fit
> > > > into that model.
> > > 
> > > OK, I understand that.
> > > 
> > > Now, there's stuff that doesn't really match the "device" model.  Where is
> > > the right place to export that?  Perhaps we should add something like
> > > /sys/platform/ (in analogy with /sys/firmware/)?
> > 
> > No, add a subsystem (bus_type) for any of them, and register them. There
> > is no such thing as "devices which do not fit the device model", they
> > are all fine there. Please stop optimizing single bytes and creating a
> > mess in /sys. Every device is a "struct device".
> 
> Again.  Those things are _not_ devices.  Am I not clear enough?

No that wasn't clear. They need to leave the driver core then, if they
are not devices. :)

> > Think of "struct bus_type" as "struct subsystem", we will rename that
> > when we are ready. It is just a group of devices which are of the same
> > type, it has nothing to do with a bus in the sense of hardware.
> > 
> > We need unified exports of _all devices to userspace, not custom layouts
> > in /sys.
> > 
> > There's is a pretty much outdated Documentation/sysfs-rules.txt, wich
> > covers part of the history and the plans.
> 
> You seem to be thinking that anything exported through sysfs needs to be
> device, which I don't think is event approximately correct (what about
> /sys/firmware/ or /sys/kernel/ or /sys/fs/ , for a few examples?).

All fine. Again, I'm only talking about "devices", which is class/,
block/, bus/, dev/, devices/ in /sys.

> Think this way: it is useful (and IMHO correct) to export some things to
> user space that without necessarily regarding them as "devices", physical
> or not.  Some of them _happen_ to be exported through sysdevs, but that
> doesn't really mean the _are_ devices.  They are simply _software_ interfaces
> to things that have no device representation and don't _need_ one.

Fine, they need to leave the driver core stuff alone, and not pretend to
be a device.

> > > > > > There is _one_ way to export device attributes, and that is
> > > > > > "struct device" today.
> > > > > > 
> > > > > > If that's to expensive for anybody, just don't use sysfs. It's the rule
> > > > > > we have today. :)
> > > > > 
> > > > > Oh, good to know.  It's changed a bit since I last heard.  Never mind.
> > > > 
> > > > Oh, don't get me wrong, this is all is about "devices" not any other
> > > > controls.
> > > >
> > > > > Still, I won't let you change the things in /sys/power to struct devices,
> > > > > sorry about that. ;-)
> > > > 
> > > > Fine as long as they are power specific things, and not "devices". You
> > > > don't have sysdevs there, right? :)
> > > 
> > > No, I don't.
> > 
> > Then all is fine. All other stuff is more like /proc, and can never be
> > really unified.
> 
> YES!  And _that_'s precisely what I'm (and Paul is) talking about.

Good.

> > All we care about is devices, which have common methods
> > for userspace to trigger and consume, and need to be unified. Power
> > specific control files seems all fine in its kobject use.
> 
> I understand that, really.
> 
> > > > > And I wonder how are you going to deal with clocksource exporting things
> > > > > via the sysdev interface right now.  I'd simply create two directories and
> > > > > put the two files into them and be done with that, but I guess that
> > > > > wouldn't fit into the model somehow, right?
> > > > 
> > > > Nope, register a bus_type, and use struct device for all of them, Parent
> > > > them to /sys/devices/system/ if they should keep their location and
> > > > layout.
> > > 
> > > Well, I'll be watching what happens to the patch trying to do that, but I'm
> > > not going to bet anything on its success. ;-)
> > 
> > It should be pretty straight-forward. We will need to do that for CPUs I
> > guess, because the interface is kinda commonly used.
> 
> No.  CPUs are _very_ special.

Not in the view of the driver core or the associated user space
interfaces. They are just like any other device.

> > > > > > > > That way userspace can properly enumerate them in a flat list
> > > > > > > > in /sys/bus/<bus_type name>/devices/*, and gets proper events on module
> > > > > > > > load and during system coldplug, and can hook into the usual hotplug
> > > > > > > > pathes to set/get these values instead of crawling magicly defined and
> > > > > > > > decoupled locations in /sys which can not express proper hierarchy,
> > > > > > > > classicication, or anything else that all other devices can just do.
> > > > > > > 
> > > > > > > There's no hotplug involved or anything remotely like that AFAICS.
> > > > > > > There are simply static files as I said above, they are created
> > > > > > > early during system initialization and simply stay there.
> > > > > > 
> > > > > > That's not the point. It's about a single way to retrieve information
> > > > > > about devices, extendability, and coldplug during bootup, where existing
> > > > > > devices need to be handled only after userspace is up.
> > > > > 
> > > > > I'd say the case at hand has nothing to do with that.
> > > > 
> > > > It has. As for CPUs. We can not do proper CPU-dependent module
> > > > autoloading, because the events happen before userspace runs, and
> > > > clodplug can not see the broken sysdevs, because they have no events to
> > > > re-trigger, like all others have.
> > > 
> > > Well, as I said, would it be OK if the things in question happened to be
> > > located somewhere outside of /sys/devices/ ?
> > 
> > No, no device directory can be outside of /sys/devices.
> 
> Sorry, I'm repeating that for the last time.  I'm not talking about devices.
> I'm talking about _totally_ _random_ _stuff_ which is "like /proc, and can
> never be really unified" (your own words) which _happens_ to be exported
> through the sysdev interface, because that happend to be _easy_ at one point.
> Can we agree on that at least?

Sure, they should leave the driver core alone, and should never been a
sysdev or any other device in the first place. We should not create
anything for them.

Kay

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev

[Kay Sievers]

On Tue, 2011-03-22 at 23:05 +0100, Rafael J. Wysocki wrote:
> On Tuesday, March 22, 2011, Kay Sievers wrote:

> >> Because they are devices, and there is a lot to win, if the kernel
> > exports all "devices" in the same way. This is not about saving an inode
> > in /sys, it's the ability to do runtime device configuration with common
> > tools.
> 
> If I understand you correctly, the goal is to have everything under
> /sys/devices/ been represented by struct device objects and there are good
> reasons to do that.
> 
> In that case we either have to move the things exported via sysdevs somewhere
> else (presumably having to create that "somewhere" before), or we have to
> introduce struct device objects specifically for exporting them.  I don't
> really think the latter approach will be very popular, so quite likely we'll
> need to have a plan for moving those things to different locations.

Ok, now that we sorted the misunderstanding out, let's start this topic
from here again. (I really read your reply as: "let's move the CPUS
somewhere else")


What's the list of stuff you discovered using sysdevs, which is not a
device at all, and has never more than a single instance of stuff of the
same type? The clocksource?

And let's find some place for them to put their properties instead of
messing around with device from the driver core.

Kay

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev class and sysdev

[Rafael J. Wysocki]

On Tuesday, March 22, 2011, Kay Sievers wrote:
> On Tue, 2011-03-22 at 23:23 +0100, Rafael J. Wysocki wrote:
> > On Tuesday, March 22, 2011, Kay Sievers wrote:
...
> > 
> > Now, I can easily understand arguments about representing everything under
> > /sys/devices/ by struct device objects, no question about that.  However,
> > I also think there should be a place for things like those mentioned in the
> > comment in sys.c, presumably outside of /sys/devices/.
> 
> No, please. We have all we need. Let's do one example, which you might
> apply to any other thing, because you never know what's the next big
> thing in hardware. We need to be a future-proof-as-possible, and that's
> not some second-class out-of-scope sysfs directory.
> 
> Lets' take CPUs:
>   - they send events when registered
>   - they want to export device specific properties
>   - userspace wants to take actions when such devices are available
> 
> That all fits properly into the driver model in theory. Unless you do
> coldplug and bootup a box.
> 
> These devices are already there before userspace even starts, hence we
> find all these devices and "trigger" an fake uevent for all of them at
> bootup. That will match execute all the rules specified for that device,
> just as it would be hotplugges in that moment, hence we call it
> coldplug, which works for all devices with the hotplug code, even when
> they are never hot-pluggable.
> 
> What we do for coldplug is that we iterate over all flat lists of
> subsystems and find the devices lists and trigger the event by poking in
> the "uevent" sysfs file. Now all the sysdevs do not have a subsystem to
> find, and do not have a standard "uevent" file.
> 
> Back to the CPUs, we have all the nice device directories which could
> have all the CPU features in properties we need to make autoloading of
> cpufreq, governer, kvm possible (patch exists from Andi Kleen already) 
> 
> But these dumb CPU sysfs device directories are completely invisible for
> the *usual* logic, and should just join the model and all will just work
> out-of-the-box.

That all is cool, but I'm not sure how it is related to things like
available_clocksource and current_clocksource (which happen to be located
under /sys/devices/system/clocksource/clocksource0/ being simply a path
in sysfs).

> When we started to clean up /sys (again only talking about devices, not
> other stuff) we had:
>   /block/*
>   /class/<subsys>/*
>   /bus/<subsys>/devices/*
>   /devices/system/<subsys>/*
> which are 4 different exports of exactly the same thing, a "device".
> "Block" we converted to "class" already, "class" will be converted to
> "bus", and "bus" will be renamed to "subsystem". All the current names
> will be kept as compat symlinks, just as we did for "block". After that,
> _all_ devices have a "subsystem" and a subsystem global directory where
> people can add custom stuff shared by all devices-of-the-same-type. Ev

OK, sounds good.

> You can also argument from the other side, if a kernel device export is
> not worth the few bytes of /sys/devices/ and a "subsystem" (struct
> bus_type) it should not be in /sys at all, especially not hidden
> somehwere outside of /sys/devices when it is something remotely close to
> a device.

Well, Greg apparently thinks that available_clocksource and current_clocksource
could be located under /sys/bus/clock/.  Perhaps other attributes now exported
through sysdevs could be moved to places like this?

Rafael

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev

[Kay Sievers]

On Wed, 2011-03-23 at 00:32 +0100, Rafael J. Wysocki wrote:
> On Tuesday, March 22, 2011, Kay Sievers wrote:
> > On Tue, 2011-03-22 at 23:23 +0100, Rafael J. Wysocki wrote:
> > > On Tuesday, March 22, 2011, Kay Sievers wrote:
> ...
> > > 
> > > Now, I can easily understand arguments about representing everything under
> > > /sys/devices/ by struct device objects, no question about that.  However,
> > > I also think there should be a place for things like those mentioned in the
> > > comment in sys.c, presumably outside of /sys/devices/.
> > 
> > No, please. We have all we need. Let's do one example, which you might
> > apply to any other thing, because you never know what's the next big
> > thing in hardware. We need to be a future-proof-as-possible, and that's
> > not some second-class out-of-scope sysfs directory.
> > 
> > Lets' take CPUs:
> >   - they send events when registered
> >   - they want to export device specific properties
> >   - userspace wants to take actions when such devices are available
> > 
> > That all fits properly into the driver model in theory. Unless you do
> > coldplug and bootup a box.
> > 
> > These devices are already there before userspace even starts, hence we
> > find all these devices and "trigger" an fake uevent for all of them at
> > bootup. That will match execute all the rules specified for that device,
> > just as it would be hotplugges in that moment, hence we call it
> > coldplug, which works for all devices with the hotplug code, even when
> > they are never hot-pluggable.
> > 
> > What we do for coldplug is that we iterate over all flat lists of
> > subsystems and find the devices lists and trigger the event by poking in
> > the "uevent" sysfs file. Now all the sysdevs do not have a subsystem to
> > find, and do not have a standard "uevent" file.
> > 
> > Back to the CPUs, we have all the nice device directories which could
> > have all the CPU features in properties we need to make autoloading of
> > cpufreq, governer, kvm possible (patch exists from Andi Kleen already) 
> > 
> > But these dumb CPU sysfs device directories are completely invisible for
> > the *usual* logic, and should just join the model and all will just work
> > out-of-the-box.
> 
> That all is cool, but I'm not sure how it is related to things like
> available_clocksource and current_clocksource (which happen to be located
> under /sys/devices/system/clocksource/clocksource0/ being simply a path
> in sysfs).

Sure, it isn't related to clocksource at all. I didn't really get the
idea that there are users that just fake core devices only to get a
place to put a couple of attributes. I was still in the context of the
$SUBJECT of this thread.

This stuff should just stay away from devices, not sysdev, not "struct
device".

For other things like CPUs, which are fine to be represented as driver
core devices, all the above is still valid, and they should be real
devices and have their own subsystem, which exposes them to coldplug and
usual event handling.

> Well, Greg apparently thinks that available_clocksource and current_clocksource
> could be located under /sys/bus/clock/.  Perhaps other attributes now exported
> through sysdevs could be moved to places like this?

Sure, we could do that. All such subsystems have a directory to put
subsystem-global stuff. In this case it would be a subsystem without any
registered device. But it leaves us open to add real devices to it
later, which might be the case for some similar subsystems.

The other option would be /sys/kernel/clocksource/ with the few
attributes to create.

We should decide if "clocksource" is kind of "device-related" or not. Do
you have any list of subsystems besides "clocksource", which would help
to get a bigger picture what we should expect?

Kay

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev class and sysdev

[Rafael J. Wysocki]

On Wednesday, March 23, 2011, Kay Sievers wrote:
> On Tue, 2011-03-22 at 23:05 +0100, Rafael J. Wysocki wrote:
> > On Tuesday, March 22, 2011, Kay Sievers wrote:
> 
> > >> Because they are devices, and there is a lot to win, if the kernel
> > > exports all "devices" in the same way. This is not about saving an inode
> > > in /sys, it's the ability to do runtime device configuration with common
> > > tools.
> > 
> > If I understand you correctly, the goal is to have everything under
> > /sys/devices/ been represented by struct device objects and there are good
> > reasons to do that.
> > 
> > In that case we either have to move the things exported via sysdevs somewhere
> > else (presumably having to create that "somewhere" before), or we have to
> > introduce struct device objects specifically for exporting them.  I don't
> > really think the latter approach will be very popular, so quite likely we'll
> > need to have a plan for moving those things to different locations.
> 
> Ok, now that we sorted the misunderstanding out, let's start this topic
> from here again. (I really read your reply as: "let's move the CPUS
> somewhere else")

Well, I didn't mean that. :-)

> What's the list of stuff you discovered using sysdevs, which is not a
> device at all, and has never more than a single instance of stuff of the
> same type? The clocksource?
> 
> And let's find some place for them to put their properties instead of
> messing around with device from the driver core.

On x86 the clocksource will be the only remaining one after my changes
recently posted.  In fact, it will be the only remaining one except for the
CPUs. :-)

The other architectures are a mixed bag, though.  First, there's some exported
stuff similar to the clocksource.  For one example, there's the "leds" sysdev
in arch/arm/kernel/leds.c that appears to export one attribute (in addition
to implementing suspend/resume/shutdown which I'm going to move to syscore_ops).
Second, there's some outright abuse here-and-there that may be removed entirely.
Finally, there are things that probably may be converted to struct devices,
in the powerpc tree in particular IIRC.

IMHO all of these things will have to be considered on the case-by-case basis
and discussed with the appropriate maintainers.

Thanks,
Rafael

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev class and sysdev

[Rafael J. Wysocki]

On Wednesday, March 23, 2011, Kay Sievers wrote:
> On Wed, 2011-03-23 at 00:32 +0100, Rafael J. Wysocki wrote:
> > On Tuesday, March 22, 2011, Kay Sievers wrote:
> > > On Tue, 2011-03-22 at 23:23 +0100, Rafael J. Wysocki wrote:
> > > > On Tuesday, March 22, 2011, Kay Sievers wrote:
> > ...
> > > > 
> > > > Now, I can easily understand arguments about representing everything under
> > > > /sys/devices/ by struct device objects, no question about that.  However,
> > > > I also think there should be a place for things like those mentioned in the
> > > > comment in sys.c, presumably outside of /sys/devices/.
> > > 
> > > No, please. We have all we need. Let's do one example, which you might
> > > apply to any other thing, because you never know what's the next big
> > > thing in hardware. We need to be a future-proof-as-possible, and that's
> > > not some second-class out-of-scope sysfs directory.
> > > 
> > > Lets' take CPUs:
> > >   - they send events when registered
> > >   - they want to export device specific properties
> > >   - userspace wants to take actions when such devices are available
> > > 
> > > That all fits properly into the driver model in theory. Unless you do
> > > coldplug and bootup a box.
> > > 
> > > These devices are already there before userspace even starts, hence we
> > > find all these devices and "trigger" an fake uevent for all of them at
> > > bootup. That will match execute all the rules specified for that device,
> > > just as it would be hotplugges in that moment, hence we call it
> > > coldplug, which works for all devices with the hotplug code, even when
> > > they are never hot-pluggable.
> > > 
> > > What we do for coldplug is that we iterate over all flat lists of
> > > subsystems and find the devices lists and trigger the event by poking in
> > > the "uevent" sysfs file. Now all the sysdevs do not have a subsystem to
> > > find, and do not have a standard "uevent" file.
> > > 
> > > Back to the CPUs, we have all the nice device directories which could
> > > have all the CPU features in properties we need to make autoloading of
> > > cpufreq, governer, kvm possible (patch exists from Andi Kleen already) 
> > > 
> > > But these dumb CPU sysfs device directories are completely invisible for
> > > the *usual* logic, and should just join the model and all will just work
> > > out-of-the-box.
> > 
> > That all is cool, but I'm not sure how it is related to things like
> > available_clocksource and current_clocksource (which happen to be located
> > under /sys/devices/system/clocksource/clocksource0/ being simply a path
> > in sysfs).
> 
> Sure, it isn't related to clocksource at all. I didn't really get the
> idea that there are users that just fake core devices only to get a
> place to put a couple of attributes. I was still in the context of the
> $SUBJECT of this thread.
> 
> This stuff should just stay away from devices, not sysdev, not "struct
> device".
> 
> For other things like CPUs, which are fine to be represented as driver
> core devices, all the above is still valid, and they should be real
> devices and have their own subsystem, which exposes them to coldplug and
> usual event handling.
> 
> > Well, Greg apparently thinks that available_clocksource and current_clocksource
> > could be located under /sys/bus/clock/.  Perhaps other attributes now exported
> > through sysdevs could be moved to places like this?
> 
> Sure, we could do that. All such subsystems have a directory to put
> subsystem-global stuff. In this case it would be a subsystem without any
> registered device. But it leaves us open to add real devices to it
> later, which might be the case for some similar subsystems.
> 
> The other option would be /sys/kernel/clocksource/ with the few
> attributes to create.
> 
> We should decide if "clocksource" is kind of "device-related" or not. Do
> you have any list of subsystems besides "clocksource", which would help
> to get a bigger picture what we should expect?

Not at the moment.  I'll prepare one while working on syscore_ops patches
for the non-x86 architectures.

Thanks,
Rafael

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev

[Russell King - ARM Linux]

On Tue, Mar 22, 2011 at 11:44:03PM +0100, Kay Sievers wrote:
> When we started to clean up /sys (again only talking about devices, not
> other stuff) we had:
>   /block/*
>   /class/<subsys>/*
>   /bus/<subsys>/devices/*
>   /devices/system/<subsys>/*
> which are 4 different exports of exactly the same thing, a "device".
> "Block" we converted to "class" already, "class" will be converted to
> "bus", and "bus" will be renamed to "subsystem". All the current names
> will be kept as compat symlinks, just as we did for "block".

I bet userspace will end up failing with that and need updating yet
again, inspite of the symlinks.

While Linus complains about the churn in the ARM subtree, I'd like to
officially complain about the pointless churn in sysfs which actively
breaks (and sometimes prevents) userspace booting when you upgrade
kernels.

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev class and sysdev

[Paul Mundt]

On Tue, Mar 22, 2011 at 09:19:28PM +0100, Rafael J. Wysocki wrote:
> Convert the SuperH clocks framework and shared interrupt handling
> code to using struct syscore_ops instead of a sysdev classes and
> sysdevs for power managment.
> 
> This reduces the code size significantly and simplifies it.  The
> optimizations causing things not to be restored after creating a
> hibernation image are removed, but they might lead to undesirable
> effects during resume from hibernation (e.g. the clocks would be left
> as the boot kernel set them, which might be not the same way as the
> hibernated kernel had seen them before the hibernation).
> 
> This also is necessary for removing sysdevs from the kernel entirely
> in the future.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/sh/clk/core.c       |   68 ++++++++-----------------------
>  drivers/sh/intc/core.c      |   95 +++++++++++++++++++++-----------------------
>  drivers/sh/intc/internals.h |    1 
>  3 files changed, 65 insertions(+), 99 deletions(-)
> 
This one looks good, and seems to work fine. Applied, thanks.

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev

[Mark Brown]

On Wed, Mar 23, 2011 at 07:23:48AM +0900, Paul Mundt wrote:
> On Tue, Mar 22, 2011 at 11:00:56PM +0100, Kay Sievers wrote:

> > Which is what we need to get rid of. It does not make any sense on the
> > global picture to have anything like that exported to userspace.

> So far I haven't heard any rationale for why it doesn't. Exporting CPU
> state to userspace certainly makes sense, and the sysdev model has worked
> reasonably for CPUs, memory nodes, etc.

FWIW it'd be really helpful to have CPUs (or at least SoCs) be regular
struct devices for integration with the regulator API so we can have all
things that might use a regulator (like DVFS) be struct devices but...

> Once cpufreq, timekeeping, and NUMA node state have migrated to whatever
> the driver model folks find acceptable, I'll happily follow suit.

...we're not precisely there yet :/

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev class and sysdev

[Paul Mundt]

On Wed, Mar 23, 2011 at 11:12:20AM +0000, Mark Brown wrote:
> On Wed, Mar 23, 2011 at 07:23:48AM +0900, Paul Mundt wrote:
> > On Tue, Mar 22, 2011 at 11:00:56PM +0100, Kay Sievers wrote:
> 
> > > Which is what we need to get rid of. It does not make any sense on the
> > > global picture to have anything like that exported to userspace.
> 
> > So far I haven't heard any rationale for why it doesn't. Exporting CPU
> > state to userspace certainly makes sense, and the sysdev model has worked
> > reasonably for CPUs, memory nodes, etc.
> 
> FWIW it'd be really helpful to have CPUs (or at least SoCs) be regular
> struct devices for integration with the regulator API so we can have all
> things that might use a regulator (like DVFS) be struct devices but...
> 
Sure, that makes sense. The easiest would probably be to just replace the
struct cpu sysdev with a struct device pointer and fix up drivers/base/cpu.c
accordingly. The linux/cpu.h API is unfortunately rather coupled to the
idea of having a sysdev, but this is purely for attributes and attribute
groups and primarily impacts powerpc, so the conversion shouldn't be too
painful. For simple topology registration the bulk of the architectures
ultimately don't care what's backing the struct cpu within the sysfs
context.

Re: [PATCH 9/10] sh: Use struct syscore_ops instead of sysdev class and sysdev

[Rafael J. Wysocki]

On Wednesday, March 23, 2011, Paul Mundt wrote:
> On Tue, Mar 22, 2011 at 09:19:28PM +0100, Rafael J. Wysocki wrote:
> > Convert the SuperH clocks framework and shared interrupt handling
> > code to using struct syscore_ops instead of a sysdev classes and
> > sysdevs for power managment.
> > 
> > This reduces the code size significantly and simplifies it.  The
> > optimizations causing things not to be restored after creating a
> > hibernation image are removed, but they might lead to undesirable
> > effects during resume from hibernation (e.g. the clocks would be left
> > as the boot kernel set them, which might be not the same way as the
> > hibernated kernel had seen them before the hibernation).
> > 
> > This also is necessary for removing sysdevs from the kernel entirely
> > in the future.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  drivers/sh/clk/core.c       |   68 ++++++++-----------------------
> >  drivers/sh/intc/core.c      |   95 +++++++++++++++++++++-----------------------
> >  drivers/sh/intc/internals.h |    1 
> >  3 files changed, 65 insertions(+), 99 deletions(-)
> > 
> This one looks good, and seems to work fine. Applied, thanks.

Cool, thanks!