On Tue, Jun 21, 2022 at 02:48:11PM -0700, Dan Williams wrote:
> Ira Weiny wrote:
> > On Fri, Jun 17, 2022 at 05:43:34PM -0700, Dan Williams wrote:
> > > ira.weiny@ wrote:
> > > > From: Ira Weiny <ira.weiny@xxxxxxxxx>
> > > >
> > >
[snip]
> > > Rather than a chatty / ephemeral error message I think this wants some
> > > indication in userspace, likely the 0-length CDAT binary attribute, so
> > > that userspace can debug why the kernel is picking sub-optimal QTG ids
> > > for newly provisioned CXL regions.
> >
> > I thought we agreed that 0-length or CDAT query failure would result in no
> > sysfs entry?
>
> Oh, I forgot about that, but some new rationale below...
>
> >
> > This message was to alert that a CDAT query was attempted but the read failed
> > vs finding no mailbox with CDAT capabilities for example.
>
> ...right, but that's an error message buried in the kernel log. I was
> hoping for something where tooling can query and say "oh, by the way,
> the driver tried and failed to get CDAT from this device that claimed to
> support CDAT, remedy that situation if you are seeing unexpected
> performance / behavior".
>
Ok I've added a flag which indicates if the device supported CDAT or not. If
so the sysfs will be visible but the data may be 0 length. Which means there
was some error in reading it.
> >
> > [snip]
> >
> > > >
> > > > +static ssize_t cdat_read(struct file *filp, struct kobject *kobj,
> > > > + struct bin_attribute *bin_attr, char *buf,
> > > > + loff_t offset, size_t count)
> > > > +{
> > > > + struct device *dev = kobj_to_dev(kobj);
> > > > + struct cxl_port *port = to_cxl_port(dev);
> > > > +
> > > > + if (!port->cdat.table)
> > > > + return 0;
> > > > +
> > > > + return memory_read_from_buffer(buf, count, &offset,
> > > > + port->cdat.table,
> > > > + port->cdat.length);
> > > > +}
> > > > +
> > > > +static BIN_ATTR_RO(cdat, 0);
> > >
> > > This should be BIN_ATTR_ADMIN_RO(), see:
> > >
> > > 3022c6a1b4b7 driver-core: Introduce DEVICE_ATTR_ADMIN_{RO,RW}
> >
> > Are you suggesting I add BIN_ATTR_ADMIN_* macros?
>
> Yes.
Done.
>
> >
> > >
> > > > +
> > > > +static umode_t cxl_port_bin_attr_is_visible(struct kobject *kobj,
> > > > + struct bin_attribute *attr, int i)
> > > > +{
> > > > + struct device *dev = kobj_to_dev(kobj);
> > > > + struct cxl_port *port = to_cxl_port(dev);
> > > > +
> > > > + if ((attr == &bin_attr_cdat) && port->cdat.table)
> > > > + return 0400;
> > >
> > > Per above change you only need to manage visibility and not permissions,
> >
> > But the permissions indicate visibility (In the kdoc for struct
> > attribute_group).
> >
> >
> > * ... Must
> > * return 0 if a binary attribute is not visible. The returned
> > * value will replace static permissions defined in
> > * struct bin_attribute.
> >
> > And the value returned overrides the mode.
> >
> > fs/sysfs/group.c:
> >
> > create_files()
> >
> > 82 if (grp->is_bin_visible) {
> > 83 mode = grp->is_bin_visible(kobj, *bin_attr, i);
> > 84 if (!mode)
> > 85 continue;
> > 86 }
> > 87
> > 88 WARN(mode & ~(SYSFS_PREALLOC | 0664),
> > 89 "Attribute %s: Invalid permissions 0%o\n",
> > 90 (*bin_attr)->attr.name, mode);
> > 91
> > 92 mode &= SYSFS_PREALLOC | 0664;
> >
> >
> > So I'm willing to add the macro but I'm not sure it is going to change anything
> > in this case.
>
> The change I was expecting is that with BIN_ATTR_ADMIN_RO() this
> implementation changes from:
>
> if ((attr == &bin_attr_cdat) && port->cdat.table)
> return 0400;
>
> ...to:
>
> if ((attr == &bin_attr_cdat) && port->cdat.table)
> return attr->mode;
>
> ...i.e. this routine only modifies visibility, you do not also need it
> to enforce the root-read-only permission change since that's already
> statically defined at attribute creation time.
Ok.
>
> > I think to make those _ADMIN_ macros work with is_visible()
> > create_files() needs to be changed. :-/ I'm not sure if the addition of
> > DEVICE_ATTR_ADMIN_{RO,RW} intended for is_visible() to be able to override the
> > mode?
>
> The intent was that one only needs to look in one place to read the
> permission, and is_visible() is (mostly*) only left to change the mode to
> 0.
>
> * changes from read-only to/from writable would still need is_visble()
> to manipulate permissions, but you get the idea.
Yep, done.
Ira
