Thanks, I'll check it out. On Tue, Apr 4, 2023 at 11:32 AM Thomas Weißschuh <thomas@xxxxxxxx> wrote: > > On 2023-04-03 15:18:31-0500, Jorge Lopez wrote: > > Hi Thomas, > > > > > > > > > > Currently the driver stores all its state in driver-global static data. > > > The kobjects are stored without any state. > > > Inside the kobject attribute operations is some fiddly logic that tries > > > to figure out the corresponding state with a fiddly mechansims. > > > > > > The more correct way would be to attach the corresponding state > > > directly to the kobject. > > > > > > Let me know if you want to give this a shot and I'll give an example. > > > > Yes. I would like to give it a shot. I can take a look at the code > > and determine when we can implement it. > > No promises. > > /* data for each kernel object */ > struct bios_property { > /* This is *not* a pointer, it will be used by the core sysfs > * code framework to manage this "object" */ > struct kobject kobj; > int instance_id; /* instance ID to pass to WMI functions */ > /* common members to all properties */ > u8 display_name[MAX_BUFF]; > u8 path[MAX_BUFF]; > /* all the other common stuff */ > > const struct *property_ops ops; > union { > struct string_property_data { > u8 current_value[MAX_BUFF]; > u8 new_value[MAX_BUFF]; > u32 min_length; > u32 max_length; > } string_data; > /* same for other data types... */ > }; > }; > > struct property_ops { > ssize_t (*show_current_value)(struct bios_property *, char *); > ssize_t (*store_current_value)(struct bios_property *, const char *, size_t); > }; > > static ssize_t string_property_show_current_value(struct bios_property *prop, char *buf) > { > /* or read from WMI. Does it need to be cached? */ > return sysfs_emit(buf, prop->string_data.current_value); > } > > ssize_t string_property_store_current_value(struct bios_property *prop, const char *buf, size_t count) > { > int ret; > > if (strlen(buf) > prop->string_data.max_length) > return -ERANGE; > > ret = do_string_specifc_wmi_stuff(buf, count); > if (ret) > return ret; > > strcpy(prop->current_value, buf); > return count; > } > > /* different show/store functionality per property type */ > static const struct property_ops string_property_ops = { > .store_current_value = string_property_show_current_value, > .show_current_value = string_property_show_current_value, > }; > > struct bioscfg_attribute { > struct attribute attr; > ssize_t (*show)(struct bioscfg_prop *prop, char *buf); > ssize_t (*store)(struct bioscfg_prop *prop, const char *buf, size_t count); > }; > > /* this is one implementation for *all* property types */ > static ssize_t display_name_show(struct bioscfg_prop *prop, char *buf) > { > return sysfs_emit(buf, prop->display_name); > } > static struct bioscfg_attribute display_name = __ATTR_RO(display_name); > > /* and all the other ones */ > > /* this dispatches into the type-specific property handlers */ > static ssize_t current_value_show(struct bioscfg_prop *prop, char *buf) > { > return prop->ops->show_current_value(prop, buf); > } > static struct bioscfg_attribute current_value = __ATTR(current_value); > > static struct attribute *attrs[] = { > &display_name.attr, > /* other attrs here */ > NULL > }; > > /* reflect read-only mode in sysfs */ > static umode_t bioscfg_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n) > { > struct bios_property *prop = container_of(kobj, struct bios_property, kobj); > > if (attr == ¤t_value.attr && prop->read_only) > return attr->mode ^ 0222; /* clear writable bits */ > return attr->mode; > } > > static const struct attribute_group attr_group = { > .attrs = attrs, > .is_visible = bioscfg_attr_is_visible, > }; > > /* the following two functions dispatch from your the core kobj pointer > * to your custom callbacks operating on nice bioscfg_attribute > */ > static ssize_t bioscfg_attr_show(struct kobject *kobj, struct attribute *attr, > char *buf) > { > struct bioscfg_attribute *kattr; > ssize_t ret = -EIO; > > kattr = container_of(attr, struct bioscfg_attribute, attr); > if (kattr->show) > ret = kattr->show(kobj, kattr, buf); > return ret; > } > > static ssize_t bioscfg_attr_store(struct kobject *kobj, struct attribute *attr, > const char *buf, size_t count) > { > struct bioscfg_attribute *kattr; > ssize_t ret = -EIO; > > kattr = container_of(attr, struct bioscfg_attribute, attr); > if (kattr->store) > ret = kattr->store(kobj, kattr, buf, count); > return ret; > } > > static const struct sysfs_ops bioscfg_kobj_sysfs_ops = { > .show = bioscfg_attr_show, > .store = bioscfg_attr_store, > }; > > /* to hook this into the generic kobject machinery */ > static const struct kobj_type bioscfg_kobj_type = { > .release = free_struct_bios_property, > .sysfs_ops = &bios_property_sysfs_ops, > .default_groups = attr_groups, > }; > > static int probe(void) > { > struct bios_property *prop; > > for (each property discovered via WMI) { > prop = kzalloc(sizeof(*prop)); > prop->readonly = is_read_only(property); > /* other common properties */ > if (is_string_property(property)) { > prop->ops = string_property_ops; > prop->string_data.current_value = ""; > /* other type-specific properties */ > } else { > ; /* and so on for other types */ > } > > kobject_init(&prop->kobj, &bioscfg_kobj_type); > kobject_add(&prop->kobj, parent, name); > } > > /* Now all properties and their memory are managed by the kernel */ > } > > Instead of having one kobj_type for all properties it would also be > possible to create a new one for each. But I don't think it's worth it.