Received-SPF: pass (google.com: domain of linux-kernel+bounces-160713-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Fri, 26 Apr 2024 14:48:55 -0700
From: Dan Williams <dan.j.williams@intel.com>
To: Tom Lendacky <thomas.lendacky@amd.com>, <linux-kernel@vger.kernel.org>,
	<x86@kernel.org>, <linux-coco@lists.linux.dev>, <svsm-devel@coconut-svsm.dev>
CC: Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
	Borislav Petkov <bp@alien8.de>, Dave Hansen <dave.hansen@linux.intel.com>,
	"H. Peter Anvin" <hpa@zytor.com>, Andy Lutomirski <luto@kernel.org>, "Peter
 Zijlstra" <peterz@infradead.org>, Dan Williams <dan.j.williams@intel.com>,
	Michael Roth <michael.roth@amd.com>, Ashish Kalra <ashish.kalra@amd.com>,
	Joel Becker <jlbec@evilplan.org>, Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH v4 12/15] fs/configfs: Add a callback to determine
 attribute visibility
Message-ID: <662c2147c4fa3_b6e02948d@dwillia2-mobl3.amr.corp.intel.com.notmuch>
References: <cover.1713974291.git.thomas.lendacky@amd.com>
 <53e67e416fb4da085f32d8a3626c5c6e656554f9.1713974291.git.thomas.lendacky@amd.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <53e67e416fb4da085f32d8a3626c5c6e656554f9.1713974291.git.thomas.lendacky@amd.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?ZwqZoQylQduGp4Vcnbo+70d9/Hoz2k5T9ABmB/J/sj0ZtdkuKPf9Ks6DNaVT?=
 =?us-ascii?Q?zRIv9gBaXLGKteZVgq2dd2y3SU4+of4h7lz5No7MyopgitaDiL6X6lnuLZCH?=
 =?us-ascii?Q?t/echR0xihmEnU5PV5xqiN0bzY/C+fYni6cww8UC99nIOlazBcmyZy5lMkdQ?=
 =?us-ascii?Q?VEOWePcq1oJKDFPiAVB+wlJDA+43ZRxZpeT4McfkKJEGWGmvgxSfpckYFP4Z?=
 =?us-ascii?Q?i6WDS5Wcf+1esrgMSeX4Q6u04Lyt6E71TB5f45husC2qFRXbXWUkYanDq6oE?=
 =?us-ascii?Q?53XiKxj1yYzpbS2vX6Y2vBKpBqtJVSTybMuTj5H7GCIL2uywl9EvPPDh3ZNj?=
 =?us-ascii?Q?kGhQ8F1FUMu46DuK6BPW4AWp5I7vHcddoZ5ahYaGsIYsOLjNSgocyyQIlIHR?=
 =?us-ascii?Q?BH1Plpi8b73/9YbhbIbLZDqpiEN3bC7tUf/Ab+fhj62xOsa7BO2IWoN4+F2a?=
 =?us-ascii?Q?RVjRCK9zXF3iittZ+ORvnCcJkKsqEMX/I9Cclar6YLs/9cw7IkFePH2zcT4l?=
 =?us-ascii?Q?crGJrfCxdoQlQVKwMJzg8d3cKnWw+FBjNUti8XEGxBxhhHXDiTEF7jYzjcca?=
 =?us-ascii?Q?0PU5ysPlazl/GngU+GkmVBpWaU1Um7vgrfcoie5TlGugIZqYP7CUWc/3iytp?=
 =?us-ascii?Q?l3zsyXcRfJURH+JqQ2zjm+bcXGlkEhwJfvu7urx8YptQ8Atv1cYQyDjhsAOj?=
 =?us-ascii?Q?Y2oDXnZEIRp6v2bUPNLhdrZqWV4YkjiDwwBHEnq3XNkHbTCJFomnkC0q1JWu?=
 =?us-ascii?Q?RoR22DtzmDJrrCSDmJPVLokjKdA77FcJHy9zR+lPlZwseRmKvnmm2tBgY8FQ?=
 =?us-ascii?Q?D6Pntn4/hYvgUJKEmAx4c8TgIRJ3LMGi1TY7WGgxgIQBt9y0D2ltOBpsQ8OS?=
 =?us-ascii?Q?t9eHfY24I1eZSg8e71999QkgUf14Ae90aIVBzUKox0BbJWIvtHCZ9XT7viyP?=
 =?us-ascii?Q?jZRYuw8dexCTKTqk6yaBSWuNHSi3v9IeD4JYt0F2U6zxVymxizqxNg4K8Smd?=
 =?us-ascii?Q?rI4OXnGgibWrTFleA0ZLYprKnNkkOtfPph4+VxkTWPZ4ZagLshXCYmcx43Za?=
 =?us-ascii?Q?PMflDwnIu7gCj6WWtOd5VAwazOgfLeiD4iV0c78jlCsCzCXIXYi5uqCiOSyP?=
 =?us-ascii?Q?jaLQsOkKf49oRWVVfYdewcUr3CjlIawYoJZue5mQ1z0CPPw7i8b949Zc6Sm1?=
 =?us-ascii?Q?XQU/wWO+wVaTgT9i6x5IHfL2t+lk5SlK8u0Pp0P013nQDMvQ/sSf3gik1gMx?=
 =?us-ascii?Q?y4YH+hKUvDL++mFrEswsVtdZ04qGTKEFMyv/KRrVDlM69Z/+Ejx8sKvDRGjs?=
 =?us-ascii?Q?hhVMTElvs21nc5n5+RPksESzx/Gk+Qh8Frf0Z9kYGrdFCgo9tdbmLAp0+vu8?=
 =?us-ascii?Q?9VxPuo7F1zXrU5XMeRk4cgCxYo+7wdaLToqH6L5CqhkgXNffOjy0mbcIjIpt?=
 =?us-ascii?Q?0/zQOEmf56LFH+5wqWtyE9PaHjIWPLqWTJdx4lrBaP8an9xvHnr/rcn3gTVm?=
 =?us-ascii?Q?CFeb5suO32tEs2LTUQqAxdzA1fNqRpl2eNZ+ztx9rmBiVcpylWZXYFpTqQGx?=
 =?us-ascii?Q?27b/rEOgClChuJDEWRYsOXpyO7yFD5rGARdfuyUjd0W1GHShWi/HYTpQNMjl?=
 =?us-ascii?Q?FA=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: d2e9d478-5c62-4bb3-5e26-08dc663aae02
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Apr 2024 21:48:59.4729
 (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: a4OGgPq4ZvAxY2xutFEMhCtIHtgJGpE6pACWPLCHb/kaTKyPa8CNLyc6Zdd1/xJoAjI8VwHFYyPwGXJpqnaEzVFcQc7rQkwp7r+EyHCHX84=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY1PR11MB8031
X-OriginatorOrg: intel.com

Tom Lendacky wrote:
> In order to support dynamic decisions as to whether an attribute should be
> created, add a callback that returns a bool to indicate whether the
> attribute should be displayed. If no callback is registered, the attribute
> is displayed by default.
> 
> Cc: Joel Becker <jlbec@evilplan.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Co-developed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  fs/configfs/dir.c        | 20 ++++++++++++++++++++
>  include/linux/configfs.h |  3 +++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index 18677cd4e62f..463e66258507 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -580,6 +580,7 @@ static void detach_attrs(struct config_item * item)
>  static int populate_attrs(struct config_item *item)
>  {
>  	const struct config_item_type *t = item->ci_type;
> +	struct configfs_group_operations *ops;
>  	struct configfs_attribute *attr;
>  	struct configfs_bin_attribute *bin_attr;
>  	int error = 0;
> @@ -587,14 +588,33 @@ static int populate_attrs(struct config_item *item)
>  
>  	if (!t)
>  		return -EINVAL;
> +
> +	ops = t->ct_group_ops;
> +	if (!ops) {
> +		struct config_group *g = item->ci_group;
> +
> +		/*
> +		 * No item specific group operations, check if the item's group
> +		 * has group operations.
> +		 */
> +		if (g && g->cg_item.ci_type)
> +			ops = g->cg_item.ci_type->ct_group_ops;

Oh, I would not have expected to need to consider any alternate group
ops for attribute visibility beyond t->ct_group_ops. However in my RFC
example I made this mistake:

  static struct configfs_group_operations tsm_report_group_ops = {
         .make_item = tsm_report_make_item,
+        .is_visible = tsm_report_attr_visible,
+        .is_bin_visible = tsm_report_bin_attr_visible,
  };
 
Which in retrospect is the wrong level, and I suspect only reachable if
you do the the above awkward indirection ("ops =
g->cg_item.ci_type->ct_group_ops"). Instead, I was expecting symmetry
with sysfs where the object that carries ->attrs also carries
->is_visible, so something like this:

+ static struct configfs_group_operations tsm_report_attr_group_ops = {
+         .is_visible = tsm_report_attr_visible,
+         .is_bin_visible = tsm_report_bin_attr_visible,
+ };

  const struct config_item_type tsm_report_type = {
          .ct_owner = THIS_MODULE,
          .ct_bin_attrs = tsm_report_bin_attrs,
          .ct_attrs = tsm_report_attrs,
          .ct_item_ops = &tsm_report_item_ops,
+         .ct_group_ops = &tsm_report_attr_group_ops
  };
  EXPORT_SYMBOL_GPL(tsm_report_default_type);

..because is_visible() at the g->cg_item.ci_type->ct_group_ops level
would seem to mean parent directory visibility which is mismatched.

However as I stare at this a bit more it sinks in that configfs "group"
!= sysfs "group". So I am open to the suggestion that ci_item_ops is the
right place to house item attribute visibility callbacks, or even a new
"ci_attr_ops" expressly for this purpose. Either way my expectation is
that config_item_type can get to the visibilty callbacks for its
attributes without needing to traverse any other groups or items.