lkml.org 
[lkml]   [2022]   [Jun]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH V11 5/8] cxl/port: Read CDAT table
    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@intel.com>
    > > > >
    > > >

    [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

    \
     
     \ /
      Last update: 2022-06-28 05:26    [W:5.468 / U:0.184 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site