Re: [PATCH v2 12/18] PCI/CMA: Expose certificates in sysfs

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

> >  
> > diff --git a/lib/spdm/req-sysfs.c b/lib/spdm/req-sysfs.c
> > index 9bbed7abc153..afba3c5a2e8f 100644
> > --- a/lib/spdm/req-sysfs.c
> > +++ b/lib/spdm/req-sysfs.c
> > @@ -93,3 +93,83 @@ const struct attribute_group spdm_attr_group = {
> >  	.attrs = spdm_attrs,
> >  	.is_visible = spdm_attrs_are_visible,
> >  };
> > +
> > +/* certificates attributes */
> > +
> > +static umode_t spdm_certificates_are_visible(struct kobject *kobj,
> > +					     struct bin_attribute *a, int n)
> > +{
> > +	struct device *dev = kobj_to_dev(kobj);
> > +	struct spdm_state *spdm_state = dev_to_spdm_state(dev);
> > +	u8 slot = a->attr.name[4] - '0';  
> 
> This is clever, but the @n parameter already conveys the index.

That's still fragile. I'd use a container structure so that we can
get the number directly from container_of() and appropriate field
in the container structure.

> 
> > +
> > +	if (IS_ERR_OR_NULL(spdm_state))
> > +		return SYSFS_GROUP_INVISIBLE;
> > +
> > +	if (!(spdm_state->supported_slots & BIT(slot)))
> > +		return 0;
> > +
> > +	return a->attr.mode;
> > +}
> > +
> > +static ssize_t spdm_cert_read(struct file *file, struct kobject *kobj,
> > +			      struct bin_attribute *a, char *buf, loff_t off,
> > +			      size_t count)
> > +{
> > +	struct device *dev = kobj_to_dev(kobj);
> > +	struct spdm_state *spdm_state = dev_to_spdm_state(dev);
> > +	u8 slot = a->attr.name[4] - '0';  
> 
> Similar comment on cleverness, I will note that the way this is
> typically handled is something like this which is just slightly less
> error prone if someone in the future changes the naming scheme.
> 
> #define CERT_ATTR(n) \
> static ssize_t slot##n##_show(struct file *file, struct kobject *kobj, \
>                               struct bin_attribute *a, char *buf, loff_t off, \
>                               size_t count) \
> { \
> 	return spdm_cert_read(kobj_to_dev(kobj), buf, off, count, (n)); \
> } \
> static BIN_ATTR_RO(slot##n);

Or augment the attribute by sticking it in a container structure with the slot
number as data and use container_of().  Either path works fine and avoids the
fragility issue of using the naming.

>