In order to support dynamic decisions as to whether an attribute should be
created, add a callback that returns a bool to indicate whether the
attribute should be display. If no callback is registered, the attribute
is displayed by default.
Cc: Joel Becker <jlbec@evilplan.org>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
fs/configfs/file.c | 13 +++++
include/linux/configfs.h | 114 +++++++++++++++++++++++++++------------
2 files changed, 93 insertions(+), 34 deletions(-)
diff --git a/fs/configfs/file.c b/fs/configfs/file.c
index 0ad32150611e..c758bcc11235 100644
--- a/fs/configfs/file.c
+++ b/fs/configfs/file.c
@@ -451,6 +451,12 @@ int configfs_create_file(struct config_item * item, const struct configfs_attrib
umode_t mode = (attr->ca_mode & S_IALLUGO) | S_IFREG;
int error = 0;
+ if (attr->ca_is_visible) {
+ mode = attr->ca_is_visible(item, attr);
+ if (!mode)
+ return 0;
+ }
+
inode_lock_nested(d_inode(dir), I_MUTEX_NORMAL);
error = configfs_make_dirent(parent_sd, NULL, (void *) attr, mode,
CONFIGFS_ITEM_ATTR, parent_sd->s_frag);
@@ -470,9 +476,16 @@ int configfs_create_bin_file(struct config_item *item,
{
struct dentry *dir = item->ci_dentry;
struct configfs_dirent *parent_sd = dir->d_fsdata;
+ const struct configfs_attribute *attr = &bin_attr->cb_attr;
umode_t mode = (bin_attr->cb_attr.ca_mode & S_IALLUGO) | S_IFREG;
int error = 0;
+ if (attr->ca_is_visible) {
+ mode = attr->ca_is_visible(item, attr);
+ if (!mode)
+ return 0;
+ }
+
inode_lock_nested(dir->d_inode, I_MUTEX_NORMAL);
error = configfs_make_dirent(parent_sd, NULL, (void *) bin_attr, mode,
CONFIGFS_ITEM_BIN_ATTR, parent_sd->s_frag);
diff --git a/include/linux/configfs.h b/include/linux/configfs.h
index 2606711adb18..18011f78ffde 100644
--- a/include/linux/configfs.h
+++ b/include/linux/configfs.h
@@ -112,39 +112,64 @@ static inline void configfs_add_default_group(struct config_group *new_group,
list_add_tail(&new_group->group_entry, &group->default_groups);
}
+typedef umode_t (*configfs_is_visible_t)(const struct config_item *item,
+ const struct configfs_attribute *attr);
+
struct configfs_attribute {
const char *ca_name;
struct module *ca_owner;
umode_t ca_mode;
+ configfs_is_visible_t ca_is_visible;
ssize_t (*show)(struct config_item *, char *);
ssize_t (*store)(struct config_item *, const char *, size_t);
};
-#define CONFIGFS_ATTR(_pfx, _name) \
+#define __CONFIGFS_ATTR(_pfx, _name, _vis) \
static struct configfs_attribute _pfx##attr_##_name = { \
.ca_name = __stringify(_name), \
.ca_mode = S_IRUGO | S_IWUSR, \
.ca_owner = THIS_MODULE, \
+ .ca_is_visible = _vis, \
.show = _pfx##_name##_show, \
.store = _pfx##_name##_store, \
}
-#define CONFIGFS_ATTR_RO(_pfx, _name) \
+#define __CONFIGFS_ATTR_RO(_pfx, _name, _vis) \
static struct configfs_attribute _pfx##attr_##_name = { \
.ca_name = __stringify(_name), \
.ca_mode = S_IRUGO, \
.ca_owner = THIS_MODULE, \
+ .ca_is_visible = _vis, \
.show = _pfx##_name##_show, \
}
-#define CONFIGFS_ATTR_WO(_pfx, _name) \
+#define __CONFIGFS_ATTR_WO(_pfx, _name, _vis) \
static struct configfs_attribute _pfx##attr_##_name = { \
.ca_name = __stringify(_name), \
.ca_mode = S_IWUSR, \
.ca_owner = THIS_MODULE, \
+ .ca_is_visible = _vis, \
.store = _pfx##_name##_store, \
}
+#define CONFIGFS_ATTR(_pfx, _name) \
+ __CONFIGFS_ATTR(_pfx, _name, NULL)
+
+#define CONFIGFS_ATTR_RO(_pfx, _name) \
+ __CONFIGFS_ATTR_RO(_pfx, _name, NULL)
+
+#define CONFIGFS_ATTR_WO(_pfx, _name) \
+ __CONFIGFS_ATTR_WO(_pfx, _name, NULL)
+
+#define CONFIGFS_ATTR_VISIBLE(_pfx, _name, _vis) \
+ __CONFIGFS_ATTR(_pfx, _name, _vis)
+
+#define CONFIGFS_ATTR_VISIBLE_RO(_pfx, _name, _vis) \
+ __CONFIGFS_ATTR_RO(_pfx, _name, _vis)
+
+#define CONFIGFS_ATTR_VISIBLE_WO(_pfx, _name, _vis) \
+ __CONFIGFS_ATTR_WO(_pfx, _name, _vis)
+
struct file;
struct vm_area_struct;
@@ -156,43 +181,64 @@ struct configfs_bin_attribute {
ssize_t (*write)(struct config_item *, const void *, size_t);
};
-#define CONFIGFS_BIN_ATTR(_pfx, _name, _priv, _maxsz) \
-static struct configfs_bin_attribute _pfx##attr_##_name = { \
- .cb_attr = { \
- .ca_name = __stringify(_name), \
- .ca_mode = S_IRUGO | S_IWUSR, \
- .ca_owner = THIS_MODULE, \
- }, \
- .cb_private = _priv, \
- .cb_max_size = _maxsz, \
- .read = _pfx##_name##_read, \
- .write = _pfx##_name##_write, \
+#define __CONFIGFS_BIN_ATTR(_pfx, _name, _priv, _maxsz, _vis) \
+static struct configfs_bin_attribute _pfx##attr_##_name = { \
+ .cb_attr = { \
+ .ca_name = __stringify(_name), \
+ .ca_mode = S_IRUGO | S_IWUSR, \
+ .ca_owner = THIS_MODULE, \
+ .ca_is_visible = _vis, \
+ }, \
+ .cb_private = _priv, \
+ .cb_max_size = _maxsz, \
+ .read = _pfx##_name##_read, \
+ .write = _pfx##_name##_write, \
}
-#define CONFIGFS_BIN_ATTR_RO(_pfx, _name, _priv, _maxsz) \
-static struct configfs_bin_attribute _pfx##attr_##_name = { \
- .cb_attr = { \
- .ca_name = __stringify(_name), \
- .ca_mode = S_IRUGO, \
- .ca_owner = THIS_MODULE, \
- }, \
- .cb_private = _priv, \
- .cb_max_size = _maxsz, \
- .read = _pfx##_name##_read, \
+#define __CONFIGFS_BIN_ATTR_RO(_pfx, _name, _priv, _maxsz, _vis) \
+static struct configfs_bin_attribute _pfx##attr_##_name = { \
+ .cb_attr = { \
+ .ca_name = __stringify(_name), \
+ .ca_mode = S_IRUGO, \
+ .ca_owner = THIS_MODULE, \
+ .ca_is_visible = _vis, \
+ }, \
+ .cb_private = _priv, \
+ .cb_max_size = _maxsz, \
+ .read = _pfx##_name##_read, \
}
-#define CONFIGFS_BIN_ATTR_WO(_pfx, _name, _priv, _maxsz) \
-static struct configfs_bin_attribute _pfx##attr_##_name = { \
- .cb_attr = { \
- .ca_name = __stringify(_name), \
- .ca_mode = S_IWUSR, \
- .ca_owner = THIS_MODULE, \
- }, \
- .cb_private = _priv, \
- .cb_max_size = _maxsz, \
- .write = _pfx##_name##_write, \
+#define __CONFIGFS_BIN_ATTR_WO(_pfx, _name, _priv, _maxsz, _vis) \
+static struct configfs_bin_attribute _pfx##attr_##_name = { \
+ .cb_attr = { \
+ .ca_name = __stringify(_name), \
+ .ca_mode = S_IWUSR, \
+ .ca_owner = THIS_MODULE, \
+ .ca_is_visible = _vis, \
+ }, \
+ .cb_private = _priv, \
+ .cb_max_size = _maxsz, \
+ .write = _pfx##_name##_write, \
}
+#define CONFIGFS_BIN_ATTR(_pfx, _name, _priv, _maxsz) \
+ __CONFIGFS_BIN_ATTR(_pfx, _name, _priv, _maxsz, NULL)
+
+#define CONFIGFS_BIN_ATTR_RO(_pfx, _name, _priv, _maxsz) \
+ __CONFIGFS_BIN_ATTR_RO(_pfx, _name, _priv, _maxsz, NULL)
+
+#define CONFIGFS_BIN_ATTR_WO(_pfx, _name, _priv, _maxsz) \
+ __CONFIGFS_BIN_ATTR_WO(_pfx, _name, _priv, _maxsz, NULL)
+
+#define CONFIGFS_BIN_ATTR_VISIBLE(_pfx, _name, _priv, _maxs, _vis) \
+ __CONFIGFS_BIN_ATTR(_pfx, _name, _priv, _maxsz, _vis)
+
+#define CONFIGFS_BIN_ATTR_VISIBLE_RO(_pfx, _name, _priv, _maxsz, _vis) \
+ __CONFIGFS_BIN_ATTR_RO(_pfx, _name, _priv, _maxsz, _vis)
+
+#define CONFIGFS_BIN_ATTR_VISIBLE_WO(_pfx, _name, _priv, _maxsz, _vis) \
+ __CONFIGFS_BIN_ATTR_WO(_pfx, _name, _priv, _maxsz, _vis)
+
/*
* If allow_link() exists, the item can symlink(2) out to other
* items. If the item is a group, it may support mkdir(2).
--
2.43.2On Fri, Mar 08, 2024 at 12:35:27PM -0600, Tom Lendacky wrote:
> In order to support dynamic decisions as to whether an attribute should be
> created, add a callback that returns a bool to indicate whether the
> attribute should be display. If no callback is registered, the attribute
> is displayed by default.
I'm curious what the strong value is in this extra callback. As opposed
to not generating the attribute in the absence of a TPM (why create a
config_item at all?), merely having an empty response from the attribute,
or having `->show()` return -ENODEV or similar.
>
> Cc: Joel Becker <jlbec@evilplan.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
> fs/configfs/file.c | 13 +++++
> include/linux/configfs.h | 114 +++++++++++++++++++++++++++------------
> 2 files changed, 93 insertions(+), 34 deletions(-)
>
> diff --git a/fs/configfs/file.c b/fs/configfs/file.c
> index 0ad32150611e..c758bcc11235 100644
> --- a/fs/configfs/file.c
> +++ b/fs/configfs/file.c
> @@ -451,6 +451,12 @@ int configfs_create_file(struct config_item * item, const struct configfs_attrib
> umode_t mode = (attr->ca_mode & S_IALLUGO) | S_IFREG;
> int error = 0;
>
> + if (attr->ca_is_visible) {
> + mode = attr->ca_is_visible(item, attr);
> + if (!mode)
> + return 0;
What value do we get from carrying the mode through here? The API
proposed is "visible or not", which is a boolean. Overloading that with
"also set the mode" is confusing, and it also can lead to the divergent
codepath problem you mentioned in your response, where
`->ca_is_visible()` fails to return the mode correctly. If this was simpl
a boolean hook, the code could read like so:
```
umode_t mode = (attr->ca_mode & S_IALLUGO) | S_IFREG;
int error = 0;
if (attr->ca_is_visible && !attr->ca_is_visible(item, attr))
return 0;
```
> diff --git a/include/linux/configfs.h b/include/linux/configfs.h
> index 2606711adb18..18011f78ffde 100644
> --- a/include/linux/configfs.h
> +++ b/include/linux/configfs.h
> @@ -112,39 +112,64 @@ static inline void configfs_add_default_group(struct config_group *new_group,
> list_add_tail(&new_group->group_entry, &group->default_groups);
> }
>
> +typedef umode_t (*configfs_is_visible_t)(const struct config_item *item,
> + const struct configfs_attribute *attr);
> +
We don't use typedefs of op functions anywhere else in configfs or
frankly the entire filesystem API. Adding one here would just introduce
confusion.
> struct configfs_attribute {
> const char *ca_name;
> struct module *ca_owner;
> umode_t ca_mode;
> + configfs_is_visible_t ca_is_visible;
> ssize_t (*show)(struct config_item *, char *);
> ssize_t (*store)(struct config_item *, const char *, size_t);
> };
>
Thanks,
Joel
--
Life's Little Instruction Book #306
"Take a nap on Sunday afternoons."
http://www.jlbec.org/
jlbec@evilplan.org
On 3/13/24 16:37, Joel Becker wrote:
> On Fri, Mar 08, 2024 at 12:35:27PM -0600, Tom Lendacky wrote:
>> In order to support dynamic decisions as to whether an attribute should be
>> created, add a callback that returns a bool to indicate whether the
>> attribute should be display. If no callback is registered, the attribute
>> is displayed by default.
>
> I'm curious what the strong value is in this extra callback. As opposed
> to not generating the attribute in the absence of a TPM (why create a
> config_item at all?), merely having an empty response from the attribute,
> or having `->show()` return -ENODEV or similar.
The value is to reduce the complexity of registering with the TSM support
across multiple vendors. There is a base set of attributes that are common
across vendors and some that are specific to vendors. Creating this
structure in the TSM support can get unwieldy. This would make it simple
to determine if support is provided since the attribute will either be
present or not.
This would also make the support similar to sysfs in the ability to
dynamically hide or show attributes.
>
>>
>> Cc: Joel Becker <jlbec@evilplan.org>
>> Cc: Christoph Hellwig <hch@lst.de>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>> fs/configfs/file.c | 13 +++++
>> include/linux/configfs.h | 114 +++++++++++++++++++++++++++------------
>> 2 files changed, 93 insertions(+), 34 deletions(-)
>>
>> diff --git a/fs/configfs/file.c b/fs/configfs/file.c
>> index 0ad32150611e..c758bcc11235 100644
>> --- a/fs/configfs/file.c
>> +++ b/fs/configfs/file.c
>> @@ -451,6 +451,12 @@ int configfs_create_file(struct config_item * item, const struct configfs_attrib
>> umode_t mode = (attr->ca_mode & S_IALLUGO) | S_IFREG;
>> int error = 0;
>>
>> + if (attr->ca_is_visible) {
>> + mode = attr->ca_is_visible(item, attr);
>> + if (!mode)
>> + return 0;
>
> What value do we get from carrying the mode through here? The API
> proposed is "visible or not", which is a boolean. Overloading that with
> "also set the mode" is confusing, and it also can lead to the divergent
> codepath problem you mentioned in your response, where
> `->ca_is_visible()` fails to return the mode correctly. If this was simpl
> a boolean hook, the code could read like so:
A boolean would work. There was a request to make this similar to the
sysfs attribute visibility. I certainly can make this a simple bool
function if that is preferable.
>
>
> ```
> umode_t mode = (attr->ca_mode & S_IALLUGO) | S_IFREG;
> int error = 0;
>
> if (attr->ca_is_visible && !attr->ca_is_visible(item, attr))
> return 0;
> ```
>
>> diff --git a/include/linux/configfs.h b/include/linux/configfs.h
>> index 2606711adb18..18011f78ffde 100644
>> --- a/include/linux/configfs.h
>> +++ b/include/linux/configfs.h
>> @@ -112,39 +112,64 @@ static inline void configfs_add_default_group(struct config_group *new_group,
>> list_add_tail(&new_group->group_entry, &group->default_groups);
>> }
>>
>> +typedef umode_t (*configfs_is_visible_t)(const struct config_item *item,
>> + const struct configfs_attribute *attr);
>> +
>
> We don't use typedefs of op functions anywhere else in configfs or
> frankly the entire filesystem API. Adding one here would just introduce
> confusion.
Sure, I can remove that and do something similar to show/store.
Thanks,
Tom
>
>> struct configfs_attribute {
>> const char *ca_name;
>> struct module *ca_owner;
>> umode_t ca_mode;
>> + configfs_is_visible_t ca_is_visible;
>> ssize_t (*show)(struct config_item *, char *);
>> ssize_t (*store)(struct config_item *, const char *, size_t);
>> };
>>
>
> Thanks,
> Joel
>
>
Tom Lendacky wrote:
> On 3/13/24 16:37, Joel Becker wrote:
> > On Fri, Mar 08, 2024 at 12:35:27PM -0600, Tom Lendacky wrote:
> >> In order to support dynamic decisions as to whether an attribute should be
> >> created, add a callback that returns a bool to indicate whether the
> >> attribute should be display. If no callback is registered, the attribute
> >> is displayed by default.
> >
> > I'm curious what the strong value is in this extra callback. As opposed
> > to not generating the attribute in the absence of a TPM (why create a
> > config_item at all?), merely having an empty response from the attribute,
> > or having `->show()` return -ENODEV or similar.
>
> The value is to reduce the complexity of registering with the TSM support
> across multiple vendors. There is a base set of attributes that are common
> across vendors and some that are specific to vendors. Creating this
> structure in the TSM support can get unwieldy. This would make it simple
> to determine if support is provided since the attribute will either be
> present or not.
>
> This would also make the support similar to sysfs in the ability to
> dynamically hide or show attributes.
>
> >
> >>
> >> Cc: Joel Becker <jlbec@evilplan.org>
> >> Cc: Christoph Hellwig <hch@lst.de>
> >> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> >> ---
> >> fs/configfs/file.c | 13 +++++
> >> include/linux/configfs.h | 114 +++++++++++++++++++++++++++------------
> >> 2 files changed, 93 insertions(+), 34 deletions(-)
> >>
> >> diff --git a/fs/configfs/file.c b/fs/configfs/file.c
> >> index 0ad32150611e..c758bcc11235 100644
> >> --- a/fs/configfs/file.c
> >> +++ b/fs/configfs/file.c
> >> @@ -451,6 +451,12 @@ int configfs_create_file(struct config_item * item, const struct configfs_attrib
> >> umode_t mode = (attr->ca_mode & S_IALLUGO) | S_IFREG;
> >> int error = 0;
> >>
> >> + if (attr->ca_is_visible) {
> >> + mode = attr->ca_is_visible(item, attr);
> >> + if (!mode)
> >> + return 0;
> >
> > What value do we get from carrying the mode through here? The API
> > proposed is "visible or not", which is a boolean. Overloading that with
> > "also set the mode" is confusing, and it also can lead to the divergent
> > codepath problem you mentioned in your response, where
> > `->ca_is_visible()` fails to return the mode correctly. If this was simpl
> > a boolean hook, the code could read like so:
>
> A boolean would work. There was a request to make this similar to the
> sysfs attribute visibility. I certainly can make this a simple bool
> function if that is preferable.
The inspiration was sysfs is_visible() and an idea to reproduce the
"static declaration + dynamic visibility" model that sysfs allows.
However, in the near term boolean visibility is sufficient since the
attributes are either on/off by vendor not read-write/read-only by
vendor implementation.
On 3/8/24 12:35, Tom Lendacky wrote:
> In order to support dynamic decisions as to whether an attribute should be
> created, add a callback that returns a bool to indicate whether the
> attribute should be display. If no callback is registered, the attribute
> is displayed by default.
>
> Cc: Joel Becker <jlbec@evilplan.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
> fs/configfs/file.c | 13 +++++
> include/linux/configfs.h | 114 +++++++++++++++++++++++++++------------
> 2 files changed, 93 insertions(+), 34 deletions(-)
>
> diff --git a/fs/configfs/file.c b/fs/configfs/file.c
> index 0ad32150611e..c758bcc11235 100644
> --- a/fs/configfs/file.c
> +++ b/fs/configfs/file.c
> @@ -451,6 +451,12 @@ int configfs_create_file(struct config_item * item, const struct configfs_attrib
> umode_t mode = (attr->ca_mode & S_IALLUGO) | S_IFREG;
I missed making this adjustment on the returned mode parameter below. So
this patch will change slightly to just initialize mode to attr->ca_mode
and then after the if statement, apply the masks as done above.
Thanks,
Tom
> int error = 0;
>
> + if (attr->ca_is_visible) {
> + mode = attr->ca_is_visible(item, attr);
> + if (!mode)
> + return 0;
> + }
> +
> inode_lock_nested(d_inode(dir), I_MUTEX_NORMAL);
> error = configfs_make_dirent(parent_sd, NULL, (void *) attr, mode,
> CONFIGFS_ITEM_ATTR, parent_sd->s_frag);
> @@ -470,9 +476,16 @@ int configfs_create_bin_file(struct config_item *item,
> {
> struct dentry *dir = item->ci_dentry;
> struct configfs_dirent *parent_sd = dir->d_fsdata;
> + const struct configfs_attribute *attr = &bin_attr->cb_attr;
> umode_t mode = (bin_attr->cb_attr.ca_mode & S_IALLUGO) | S_IFREG;
> int error = 0;
>
> + if (attr->ca_is_visible) {
> + mode = attr->ca_is_visible(item, attr);
> + if (!mode)
> + return 0;
> + }
> +
> inode_lock_nested(dir->d_inode, I_MUTEX_NORMAL);
> error = configfs_make_dirent(parent_sd, NULL, (void *) bin_attr, mode,
> CONFIGFS_ITEM_BIN_ATTR, parent_sd->s_frag);
© 2016 - 2026 Red Hat, Inc.