Re: [PATCH] drivers: base: add cpu_device_create to support per-cpu devices
From: Sudeep Holla
Date: Tue Aug 26 2014 - 12:54:43 EST
Hi David,
On 22/08/14 13:33, Sudeep Holla wrote:
On 22/08/14 12:41, David Herrmann wrote:
Hi
On Fri, Aug 22, 2014 at 1:37 PM, David Herrmann <dh.herrmann@xxxxxxxxx> wrote:
Hi
On Fri, Aug 22, 2014 at 1:29 PM, Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
From: Sudeep Holla <sudeep.holla@xxxxxxx>
This patch adds a new function to create per-cpu devices.
This helps in:
1. reusing the device infrastructure to create any cpu related
attributes and corresponding sysfs instead of creating and
dealing with raw kobjects directly
2. retaining the legacy path(/sys/devices/system/cpu/..) to support
existing sysfs ABI
3. avoiding to create links in the bus directory pointing to the
device as there would be per-cpu instance of these devices with
the same name since dev->bus is not populated to cpu_sysbus on
purpose
Signed-off-by: Sudeep Holla <sudeep.holla@xxxxxxx>
Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
---
drivers/base/cpu.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/cpu.h | 4 ++++
2 files changed, 58 insertions(+)
Hi Greg,
Here is the alternate solution I could come up with instead of
creating cpu class. cpu_device_create is very similar to
device_create_groups_vargs w/o class support, but I could not
reuse anything else to avoid creating similar function.
Let me know your thoughts/suggestions on this.
Regards,
Sudeep
diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 277a9cfa9040..53f0c4141d05 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -363,6 +363,60 @@ struct device *get_cpu_device(unsigned cpu)
}
EXPORT_SYMBOL_GPL(get_cpu_device);
+static void device_create_release(struct device *dev)
+{
+ kfree(dev);
+}
+
+static struct device *
+__cpu_device_create(struct device *parent, void *drvdata,
+ const struct attribute_group **groups,
+ const char *fmt, va_list args)
+{
+ struct device *dev = NULL;
+ int retval = -ENODEV;
+
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev) {
+ retval = -ENOMEM;
+ goto error;
+ }
+
+ device_initialize(dev);
+ dev->parent = parent;
+ dev->groups = groups;
+ dev->release = device_create_release;
+ dev_set_drvdata(dev, drvdata);
+
+ retval = kobject_set_name_vargs(&dev->kobj, fmt, args);
+ if (retval)
+ goto error;
+
+ retval = device_add(dev);
+ if (retval)
+ goto error;
Exactly! As I said, simply setting dev->groups before calling device_add().
However, I really don't understand why we need this as global API.
Skimming over the other patches, you use cpu_device_create() only in
one place. Why not hard-code this all there? It is totally OK to do
device initialization in drivers. All the helpers (like
device_create(), device_create_with_groups(), and so on) are just
convenience functions. The driver-core API explicitly allows drivers
to initialize devices manually.
Nevertheless, this patch looks fine.
Wait, no. Why don't you set dev->bus to cpu_subsys? Is this thing
supposed to create child-devices of CPUs? Can you describe what your
topology is supposed to look like?
Yes, it's not done on purpose as mentioned in the commit log.
E.g.: cacheinfo topology will be as below
/sys/devices/system/cpu/cpuX/cache/index0/<attributes>
/sys/devices/system/cpu/cpuX/cache/index1/<attributes>
..
/sys/devices/system/cpu/cpuX/cache/index<Y/<attributes>
Does the above topology looks fine to you ? Since the parent is set
properly, not setting bus will not cause any issue to the topology.
In this case 'cache' is cpuX's child and index<0..Y> are children of
cache in cpuX. The main problem with per-cpu device is that they have
same name for each cpu's instance and when the bus is set to this
devices, the driver model tries to create symlink to each of these
devices in /sys/bus/cpu/... which fails.
Here is the exact issue, probably kernel dump is easier to explain.
It fails for second CPU as the sysfs path already exists.
WARNING: CPU: 1 PID: 1 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x51/0x5c()
sysfs: cannot create duplicate filename '/bus/cpu/devices/cache'
Modules linked in:
CPU: 1 PID: 1 Comm: swapper/0 Not tainted
3.17.0-rc2-00013-g7956a439b183-dirty #89
[<c0013c3d>] (unwind_backtrace) from [<c0010581>] (show_stack+0x11/0x14)
[<c0010581>] (show_stack) from [<c04e9419>] (dump_stack+0x69/0x74)
[<c04e9419>] (dump_stack) from [<c00204cb>] (warn_slowpath_common+0x5f/0x78)
[<c00204cb>] (warn_slowpath_common) from [<c0020507>]
(warn_slowpath_fmt+0x23/0x2c)
[<c0020507>] (warn_slowpath_fmt) from [<c013b921>]
(sysfs_warn_dup+0x51/0x5c)
[<c013b921>] (sysfs_warn_dup) from [<c013bb6d>]
(sysfs_do_create_link_sd.isra.2+0x91/0x94)
[<c013bb6d>] (sysfs_do_create_link_sd.isra.2) from [<c031d4f3>]
(bus_add_device+0xab/0x134)
[<c031d4f3>] (bus_add_device) from [<c031c16d>] (device_add+0x2a1/0x3dc)
[<c031c16d>] (device_add) from [<c031f905>] (cpu_device_create+0x85/0x94)
Hi Greg,
Any suggestions to proceed on this ?
Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/