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:   Tue, 21 Jun 2022 14:14:17 -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: <YrI0qQrvM7MzKeLy@iweiny-desk3>
References: <20220610202259.3544623-1-ira.weiny@intel.com>
 <20220610202259.3544623-6-ira.weiny@intel.com>
 <62ad1fb69d742_8920729490@dwillia2-xfh.notmuch>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <62ad1fb69d742_8920729490@dwillia2-xfh.notmuch>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?PEMsxQEVtZqUfw3U3/O7d21jLU6hQUj0bI2X/hJBnY08PGPr7esbnEGe06pv?=
 =?us-ascii?Q?zPHo9OpDi0Dkht1bUf8/7guBrlcK/pUoYOGzE0fes6jsSjjQXhjiVmdkurNL?=
 =?us-ascii?Q?npkPIiSrADF5CkVfd2tkwR04OhN/kt8DVG+f6Zlog8yEpvEYf2HhRC9T1atY?=
 =?us-ascii?Q?k9pXDzpm0hOnuf9uYpFo88zXNVLREZB/orRr6I61pQq6q1qrnzxNMX8mn8YS?=
 =?us-ascii?Q?lEx7ISSxxoJlLavd41znOvnm3l/i2KHnGy+e0B+YJvNTjnsOft9R4erjesL/?=
 =?us-ascii?Q?2FH/G31gVWMoAP/24j7beMdK+KmLvEL1t1rCS1y4JTLE66sV/GHbRVpmFigQ?=
 =?us-ascii?Q?oahLjMzvmLRzLP6ev9tJhi/CEj/SZ8li8wMelpniLkP9dtVGWIHxt64LiTJ2?=
 =?us-ascii?Q?Y+C0/K2T+4Tn+l+dNkAFE9v0kMMnceT8DkhQyPBwolYsMSBikmGQC7v5AS6V?=
 =?us-ascii?Q?5zkurqoaolJviJzPuPDKiBTMk19l6usScDM3lkEcNxRpe7bE7iF6G89/UuiV?=
 =?us-ascii?Q?3g1d3u0fi3hKz2TPUBrvEzGw1stnkp0G0lIEdeNqNBQccMgRiApeyOsRKNSa?=
 =?us-ascii?Q?urauknjFXyLoN30rpO+KXSMul+mek9JQUyMp3yZ2YJiPXEyJK8/mhuVSFTCH?=
 =?us-ascii?Q?SlVSqmRoitstaLUsLA8n+LS7Lvz8UQa/hm9j7XKqsYDYrb0YqVVO1EDOjREy?=
 =?us-ascii?Q?//m/ZnjKsEueWNqh/V1ZCbPlnGI8155xv/wND1TYUhgp4REOCNi2kIZpzKqk?=
 =?us-ascii?Q?GazRDTRHX3d49X7EsZae+9E/tPdeBnpvD/gXCxlchrW/x11qE9x7KEM/S3sB?=
 =?us-ascii?Q?bOw828m18fgDwsOnk4aTROtRdezxPHF6gobPkqz34oaHNW8fw2qBy+UTHQwY?=
 =?us-ascii?Q?Hn+OjvAU787W6X6BhbKZ7/CnXMb22BrJmFpO2kjbe6GiMvsnFjgnpgvcj4bo?=
 =?us-ascii?Q?/MbqNfeC0RfMMZhCBQLHVtpsmWBSZG3Rw+dUgt8O7Cc9WimI+5y7lBzrUUhQ?=
 =?us-ascii?Q?sN5uqCFKYC0s2sO5acDuhP1+3+6ecy33fqWmDuTzctsk/CzgRFa2vItYh9so?=
 =?us-ascii?Q?gwoVKo8bDtwshQzair+rvefIgSAS3o/TXDJyGg3By+7YbOfkAI8QpuRhf8JZ?=
 =?us-ascii?Q?x51dmKqjOOMJ1JNHhfBuEOq91uSI+DOP4bWGVxDIzU3+Vb7egoKasZKLzJSp?=
 =?us-ascii?Q?pw0+oyM+Wx7VVVwLZA9oZ5GXgIRHatnfP1Caaoxou+1mTzpKjFf4YwThAbdK?=
 =?us-ascii?Q?ksFNZZ55bxhyWYYNzxyfxEkAzAia3z0A3aw78mpEhBD4twjkYFN7uXg6yfEj?=
 =?us-ascii?Q?4deMymaFQpWWZxv3U7nd2oO+M0qFZ+n42/jIJtc3PlS9l0w/NYvWKv5WrHBL?=
 =?us-ascii?Q?cjw3z9SmjDLzdyNG7zaWfdtCzWnQ5Z3DCzF4S/1ytPf88eAAuc9A0SmXcL1v?=
 =?us-ascii?Q?B8Zi16HS0vnJsSit3ECIVDUCQVEopVJgjuOND/Uq+8ZgFHcFo8FtS3lHvxfu?=
 =?us-ascii?Q?AmJWISO4yFUzsTjLRH5JmhnAojnW2tYO4Hx3EgrBHgiMhXch1o6JhFUHuGut?=
 =?us-ascii?Q?fWhX+dS7s9hT6i4UhcNQVP0TZZGaf8FzbKQy19L/vo34ymnsGOmkNVRYd9ZJ?=
 =?us-ascii?Q?AGg/asSpVb4ju9IuHhwEbqkQESuZ68Xq/Ta5XJSp+v9r6tvBsabnGz8dFY1C?=
 =?us-ascii?Q?NJzC7Bpg5tnVBLkDcbZzPFyYa1eVo18aV20C9lxhaZFH3kABRpI+ObxthFCL?=
 =?us-ascii?Q?BIn4V3l+9Q=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 112a6642-b627-436e-7226-08da53cb02c5
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB6311.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Jun 2022 21:14:21.7400
 (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: p7qH3VqYsNzfZxNqTk42HZfGDCEhz+jiu2fB4d6eF4aWsJgdY8hjdWlAfFf8lPGJKB78VxpJuwzeHc7wJ47dxA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB6042
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        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 Fri, Jun 17, 2022 at 05:43:34PM -0700, Dan Williams wrote:
> ira.weiny@ wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 

[snip]

> > +
> > +/**
> > + * read_cdat_data - Read the CDAT data on this port
> > + * @port: Port to read data from
> > + *
> > + * This call will sleep waiting for responses from the DOE mailbox.
> > + */
> > +void read_cdat_data(struct cxl_port *port)
> > +{
> > +	static struct pci_doe_mb *cdat_mb;
> > +	struct device *dev = &port->dev;
> > +	struct device *uport = port->uport;
> > +	size_t cdat_length;
> > +	int ret;
> > +
> > +	/*
> > +	 * Ensure a reference on the underlying uport device which has the
> > +	 * mailboxes in it
> > +	 */
> > +	get_device(uport);
> 
> I had written up a long description grumbling about "just in case"
> acquistions of device references, but then I lost that draft.
> 
> So I'll do the shorter response, but give you more homework in the
> process. How / Why is that get_device() needed? What are the guarantees that
> ensure you that the last reference has not been dropped just before that
> call? Hint: what context is this code running?

I'll check it out.  I suspect there is some reference on uport already taken
such that if port is valid uport must be valid.

> 
> > +
> > +	cdat_mb = find_cdat_mb(uport);
> > +	if (!cdat_mb) {
> > +		dev_dbg(dev, "No CDAT mailbox\n");
> > +		goto out;
> > +	}
> > +
> > +	if (cxl_cdat_get_length(dev, cdat_mb, &cdat_length)) {
> > +		dev_dbg(dev, "No CDAT length\n");
> > +		goto out;
> > +	}
> > +
> > +	port->cdat.table = devm_kzalloc(dev, cdat_length, GFP_KERNEL);
> > +	if (!port->cdat.table)
> > +		goto out;
> > +
> > +	port->cdat.length = cdat_length;
> > +	ret = cxl_cdat_read_table(dev, cdat_mb, &port->cdat);
> > +	if (ret) {
> > +		/* Don't leave table data allocated on error */
> > +		devm_kfree(dev, port->cdat.table);
> > +		port->cdat.table = NULL;
> > +		port->cdat.length = 0;
> > +		dev_err(dev, "CDAT data read error\n");
> 
> 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?

This message was to alert that a CDAT query was attempted but the read failed
vs finding no mailbox with CDAT capabilities for example.

[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?

> 
> > +
> > +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.  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?

> also per the comment about the error message in the CDAT table read case
> I think it makes sense to show an empty attribute. Only if the device
> does not claim to have a CDAT should the attribute not be visible.

Ok I can do that.

Thanks for the review,
Ira

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static struct bin_attribute *cxl_cdat_bin_attributes[] = {
> > +	&bin_attr_cdat,
> > +	NULL,
> > +};
> > +
> > +static struct attribute_group cxl_cdat_attribute_group = {
> > +	.name = "CDAT",
> > +	.bin_attrs = cxl_cdat_bin_attributes,
> > +	.is_bin_visible = cxl_port_bin_attr_is_visible,
> > +};
> > +
> > +static const struct attribute_group *cxl_port_attribute_groups[] = {
> > +	&cxl_cdat_attribute_group,
> > +	NULL,
> > +};
> > +
> >  static struct cxl_driver cxl_port_driver = {
> >  	.name = "cxl_port",
> >  	.probe = cxl_port_probe,
> >  	.id = CXL_DEVICE_PORT,
> > +	.drv = {
> > +		.dev_groups = cxl_port_attribute_groups,
> > +	},
> >  };
> >  
> >  module_cxl_driver(cxl_port_driver);
> > -- 
> > 2.35.1
> > 
> 
>