On 21.12.2021 13:56, Greg Kroah-Hartman wrote:
On Tue, Dec 21, 2021 at 01:24:00PM +0100, Rafał Miłecki wrote:
On 21.12.2021 08:13, Greg Kroah-Hartman wrote:
On Tue, Dec 21, 2021 at 07:53:32AM +0100, Rafał Miłecki wrote:
On 21.12.2021 07:45, Greg Kroah-Hartman wrote:
On Tue, Dec 21, 2021 at 07:39:24AM +0100, Rafał Miłecki wrote:
On 21.12.2021 07:33, Greg Kroah-Hartman wrote:
On Mon, Dec 20, 2021 at 09:39:43PM +0100, Rafał Miłecki wrote:
Hi Greg,
On 20.12.2021 09:00, Greg Kroah-Hartman wrote:
On Mon, Dec 20, 2021 at 07:47:30AM +0100, Rafał Miłecki wrote:
static void nvmem_cell_entry_add(struct nvmem_cell_entry *cell)
{
+ struct device *dev = &cell->nvmem->dev;
+ int err;
+
mutex_lock(&nvmem_mutex);
list_add_tail(&cell->node, &cell->nvmem->cells);
mutex_unlock(&nvmem_mutex);
+
+ sysfs_attr_init(&cell->battr.attr);
+ cell->battr.attr.name = cell->name;
+ cell->battr.attr.mode = 0400;
+ cell->battr.read = nvmem_cell_attr_read;
+ err = sysfs_add_bin_file_to_group(&dev->kobj, &cell->battr,
+ nvmem_cells_group.name);
Why not just use the is_bin_visible attribute instead to determine if
the attribute should be shown or not instead of having to add it
after-the-fact which will race with userspace and loose?
I'm sorry I really don't see how you suggest to get it done.
I can use .is_bin_visible() callback indeed to respect nvmem->root_only.
Great.
I don't understand addig-after-the-fact part. How is .is_bin_visible()
related to adding attributes for newly created cells?
You are adding a sysfs attribute to a device that is already registered
in the driver core, and so the creation of that attribute is never seen
by userspace. The attribute needs to be attached to the device _BEFORE_
it is registered.
Also, huge hint, if a driver has to call as sysfs_*() call, something is
wrong.
Do you mean I can
avoid calling sysfs_add_bin_file_to_group()?
Yes.
Do you recall any existing example of such solution?
Loads.
Just add this attribute group to your driver as a default attribute
group and the driver core will create it for you if needed.
Or if you always need it, no need to mess sith is_bin_visible() at all,
I can't really understand what you are trying to do here at all.
Thanks a lot! In nvmem_register() first there is a call to the
device_register() and only later cells get added. I suppose I just have
to rework nvmem_register() order so that:
1. Cells are collected earlier. For each cell I allocate group attribute
No, add all of the attributes to the device at the beginning before you
register it, there's no need to allocate anything.
If you mean static structures I can't do that, because cells almost
never are static. They are not known in advance. nvmem allows cells to
be:
1. Specified in OF
2. Submitted as list while registering a NVMEM device
So every cells gets its own structure allocated dynamically. My plan is
to put bin_attribute in that struct and then create a group collecting
all those cells.
A device has a driver associated with it, and that driver has default
groups associated with it. Use that, I am not saying to use static
structures, that is not how the driver model works at all.
I'm helpless on dealing with attributes.
I tried building a list of attributes dynamically but that of course
fails:
drivers/nvmem/core.c: In function ‘nvmem_register’:
drivers/nvmem/core.c:930:31: error: assignment of member ‘bin_attrs’ in read-only object
930 | nvmem_cells_group.bin_attrs = nvmem->cells_bin_attrs;
| ^
What I'm trying to achieve is having
/sys/bus/nvmem/devices/*/cells/*
with each file being an attribute.
What is the full path here that you are looking to add these attributes
to? Where is the struct device in play? What .c file should I look at?
Please kindly point me to a single example of "struct attribute_group"
that has a variable list of attributes with each attribute having
runtime set name.
Why would you ever want each attribute have a runtime-set name? That's
not what attributes are for. Think of them as "key/value" pairs. The
"key" part is the name (i.e. filename), that is well known to everyone,
unique to that struct device type, and documented in Documentation/ABI/.
The "value" part is the value you read from the file (or write to it.)
That's it, it's not all that complex.
Almost all cases in kernel look like:
static const struct attribute_group foo_attr_group = {
.attrs = foo_attrs,
};
with "foo_attrs" being a list of attributes with *predefined* names.
Yes, because that is what you really want.
Why do you feel this device is somehow unique to deserve attributes that
are not predefined? And if they are not predefined, how are you going
to define them when you create them in the code and document them? :)
Every example of dynamic attributes (runtime created) I see in a kernel
(e.g. drivers/base/node.c) uses sysfs_*().
drivers/base/* is not the best place to look at for how to implement
bus/driver logic, look at existing busses and drivers instead please.
We have a few hundred to choose from :)
So, let's break it down, what exactly are you wanting your device on
your bus to look like? What will be the attributes you want to expose,
and what are the values of those attributes? You have to start with
that, as Documentation/ABI/ is going to require you to write them down.
This patch subject / body is a basic summary. It's about
drivers/nvmem/core.c .
Let me explain it with more details:
NVMEM is a data blob.
NVMEM consists of entries called cells.
Example:
U-Boot environment variables is NVMEM. Example:
00000000 c4 09 30 54 62 6f 6f 74 63 6d 64 3d 74 66 74 70 |..0Tbootcmd=tftp|
00000010 00 62 6f 6f 74 64 65 6c 61 79 3d 35 00 65 74 68 |.bootdelay=5.eth|
A single environment variable is NVMEM cell. Example:
bootcmd=tftp
bootdelay=5
Every NVMEM device is exposed in sysfs as:
/sys/bus/nvmem/devices/*/
You can read NVMEM blob doing
cat /sys/bus/nvmem/devices/*/nvmem | hexdump -C
What I'm trying to do is to expose NVMEM cells in sysfs as:
/sys/bus/nvmem/devices/cells/*
Example:
$ cat /sys/bus/nvmem/devices/foo/cells/bootcmd
tftp
$ cat /sys/bus/nvmem/devices/foo/cells/bootdelay
5
As you can see above NVMEM cells are not known at compilation time.
So I believe the question is: how can I expose cells in sysfs?
