Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 27 Jun 2022 20:24:35 -0700
From:   Ira Weiny <ira.weiny@intel.com>
To:     Dan Williams <dan.j.williams@intel.com>
CC:     Bjorn Helgaas <bhelgaas@google.com>,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        Dave Jiang <dave.jiang@intel.com>,
        Ben Widawsky <bwidawsk@kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-cxl@vger.kernel.org>,
        <linux-pci@vger.kernel.org>
Subject: Re: [PATCH V11 5/8] cxl/port: Read CDAT table
Message-ID: <Yrp0c2wD/L0HHvgG@iweiny-desk3>
References: <20220610202259.3544623-1-ira.weiny@intel.com>
 <20220610202259.3544623-6-ira.weiny@intel.com>
 <62ad1fb69d742_8920729490@dwillia2-xfh.notmuch>
 <YrI0qQrvM7MzKeLy@iweiny-desk3>
 <62b23c9b3726e_892072944d@dwillia2-xfh.notmuch>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <62b23c9b3726e_892072944d@dwillia2-xfh.notmuch>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?eg3+qacv+YBfxgxWO214IUAOs7BPFawuKzX+b9ZL6n9ewVCXQmM8tV3EVEkj?=
 =?us-ascii?Q?SScwdgV08mx+EEUVHGj3I4oCzkmPi/OZgxFe3rYvtGDYyTrQUcEOlaO8uKt2?=
 =?us-ascii?Q?bzpXUd2myweKRG5jD5RNxGizzerZAgh55XCQVQ3tUwDUspaFo8bh8EEMwabW?=
 =?us-ascii?Q?5T2Sc267Xocn4eY6SdhUaZq0qkcBhXKeU4tiYLeRebpQUXMfg6JvIlkMdDuo?=
 =?us-ascii?Q?7ghjMXGlHLEfMjYqka6mjT6UKw+NPF0+tBJq0oSPJsa8Fxn6Sb94jbVR8CtA?=
 =?us-ascii?Q?5T6ytiOvPMKuJUeIKAw38Nn8DLIJk+Wy/88AjorCu8stWeF9iWjiK+s1UaXm?=
 =?us-ascii?Q?aqSrWBIRcLbTM7Xovfnf0vDN6Drxxr08SG7XXsI7xaOXCT8RcEpG6Cp2E+PY?=
 =?us-ascii?Q?7V+dDwES+XXcnyKdJtV6brWEWrFhR45iiZ2mu40IO6bAt4Zfqdz10nzF7clX?=
 =?us-ascii?Q?FETnM2RMEDRsOWSfBI+nQcix1/xnublFIVbOzwuIfyrdhWdFwm4zieCv61Bo?=
 =?us-ascii?Q?q34njc3BFOBw92EF7xEhx0MXpDt/aKr7f3gAsguSbEVTW/KsQYJ6oUAjBX+w?=
 =?us-ascii?Q?z9c/BLweNBf91RlqJklE71n67YbmP6Ummk7PLWE/X/cQjkHlmVo+99UBhble?=
 =?us-ascii?Q?uxVMnA5qXzbLnvgdODiFIwa9khBlzBWVocU/rAUVRWtIkLRkYCfyz9StXZHh?=
 =?us-ascii?Q?j3WZLhrzrICOn8GtgGgHhOd2B2ukvfGq6bIvXj7lh4VgEBZL1/ua9TiDsXZ8?=
 =?us-ascii?Q?+AvsS7B7uiCKm6tJPTtNwSSXxmhKCjr/8nfrGDZbvr4J2WOreBKvoxwMAw0D?=
 =?us-ascii?Q?mYWlZ97PPROQHrrDCh66ZOL2KqCSfBWFgIDSMwT3uPxWD6OvAzlL2SdQRG4D?=
 =?us-ascii?Q?w03a0/usNm3iprSpLDFeL62tWRE0Fktp4lZvQkwF524g+TDLiDRLusH/rPrc?=
 =?us-ascii?Q?CZVkFl1BHJYXbv10pdjvfw7ne/NifbNkrnTvLr1Y95xO5oU1zdmJPPO60+jX?=
 =?us-ascii?Q?4NFuWHpypAeFc7iijMx7B2rvp99/B1v9YtUSMSeoow0A6kmf/ITA7G0peIzS?=
 =?us-ascii?Q?ayMgpp+jVcWqTj79TGIToKte/vaW/I2gL6aRrnH9jw0v4whlb6GFo3XWPnor?=
 =?us-ascii?Q?Jh7BUyxG4RInCTBDicvAEghIT9ntzOXmJ/n2smXSzgndI+kcKM7l/oBKnlJP?=
 =?us-ascii?Q?wHjBTEogItpLsKdYxtbjadf7gqJ9QghMyLP7aKL5c9fyep3NS0wWfmcTuXV2?=
 =?us-ascii?Q?vGUDDTNh9LOdwIjnXX0VZW80RM82wgLMbpePdzUjU/2euaPnSzfL/zM5mY3v?=
 =?us-ascii?Q?juQJTNpbsMzYVdhJzcdpDjLT5gJDekPiOo+XX8aSkXWdulnmdQ+LsmV15cEO?=
 =?us-ascii?Q?RRav78AYXRyCQHubLRu+Mxr1bbyjSlzwha7biWtKWHiEgU2DqfWNXy05Fj0+?=
 =?us-ascii?Q?Kkfd1iF2ZqkB+2/mDI2RNRcXMVUWbyCUIjmJHIHBW/o8Ehut+YX1NjYAT/WB?=
 =?us-ascii?Q?jC+ac0p7n6RBDBnVrkpnhudhxvbNINuybg0A0FpNlYOw7Vn+lEgQM7UW3PpP?=
 =?us-ascii?Q?8UU3roxCvDAV3oJpv4SGr3QYdj0U7yUwDvnZqJIjAyNb7BGjgg/yUMN59LgW?=
 =?us-ascii?Q?SA6O2HoyDgyUsfzDDL7LzriQKqYVqqAXsj6wOVm9iCiQ?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2e594c04-8dec-4486-5f40-08da58b5c740
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB6311.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Jun 2022 03:24:58.2649
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: v9RyfNyCyl6Jq+QYTILGEPkqmny7pNX+TtWmrFbrIKUgm9AhrCEukSG4eKx4DSdHjQbzTD26r4KpVsQXu8j6Rw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR11MB4818
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,
        SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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