Received-SPF: pass (google.com: domain of linux-kernel+bounces-162366-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <4fd88c1c-6100-254e-ceec-98a4bcfa015c@amd.com>
Date: Mon, 29 Apr 2024 08:26:49 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
To: Dan Williams <dan.j.williams@intel.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>, Michael Roth <michael.roth@amd.com>,
 Ashish Kalra <ashish.kalra@amd.com>, Joel Becker <jlbec@evilplan.org>,
 Christoph Hellwig <hch@lst.de>
References: <cover.1713974291.git.thomas.lendacky@amd.com>
 <53e67e416fb4da085f32d8a3626c5c6e656554f9.1713974291.git.thomas.lendacky@amd.com>
 <662c2147c4fa3_b6e02948d@dwillia2-mobl3.amr.corp.intel.com.notmuch>
Content-Language: en-US
From: Tom Lendacky <thomas.lendacky@amd.com>
Subject: Re: [PATCH v4 12/15] fs/configfs: Add a callback to determine
 attribute visibility
In-Reply-To: <662c2147c4fa3_b6e02948d@dwillia2-mobl3.amr.corp.intel.com.notmuch>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?a0xKS09Qb0lwWCs3UEM4ZjdlcGVPTk1mZVJpNlFXcHo1YlhDY2JCVFg5c2w5?=
 =?utf-8?B?Snd2UkVra2tETnRUNjFwWlhMZ0Z0aDZCeEFYMHZNZkRObFFJdGNLRUUvbnFB?=
 =?utf-8?B?cy9ZOGM5eGdQeGY2dkZvNnN6cm10ZExGWmRhRGErZ2s0bXc4L2hOZUZhWlRX?=
 =?utf-8?B?MFQ5d2gwSFFjRTdsN2xSOFpPd1VPeHVERVVxTWdEVVljUXRzSEpxM216aXgy?=
 =?utf-8?B?VlZ2aHdyS1RwNFNsTkhlUXg4aCtVSk1lYnhLTHNKOUgyYU84SzN3ajV6RHRj?=
 =?utf-8?B?VURaMWZMbmJwTzhpN1lvOXV5SVVkMllLbFcwcWQ3TU5hKzJXSkdqdStzSWhX?=
 =?utf-8?B?Zmp6SVVsdHorcmNBZUdSc1pkVCtORzVrMlJ1OFlzdGc1b0ppSHBoaUxOYlU1?=
 =?utf-8?B?YzN0dlAvVTB1bEtocWJ4L0NYS1ZQSCtwYzFsT1VXeWVWd3lyQ1o4VE9VUy9U?=
 =?utf-8?B?YTBteGhaRVNLVmh3M3d4VytDQ3R1SFR1QzdGSTU2MWgyM1BpcjIvRGREeHZV?=
 =?utf-8?B?bXNvdU1wSHJXcmNpYmtIb0k0VFlPK2NRVjgxWStQVTlRU1F2WmRhSzFPZTNk?=
 =?utf-8?B?aGtjWUN1ZVNjeGE5NFFjZVBVaWsvSmRzSEVXU1RlL2Z1MW1aODNzVStmOWs4?=
 =?utf-8?B?NmEvVExGdElQVEhpeTBRc1FJL0F4dXYvcWtZbWMrSXJDcW14ZThmYU9IV2dE?=
 =?utf-8?B?emU0dmJCQm1Oa285cDhibmZ1b1JGZk44OVJTNUQwYUJtVzNyakw0WjRMSWpy?=
 =?utf-8?B?TVJYcG50dUp2S3RYZm5KZ2dwTlVVaXNwa2wyOTNsalh5azZNZnZtQnFLTnNH?=
 =?utf-8?B?VHdKTTRTZ3luMHhSazVYb1RQQ2VzbUxDZzRpTzlBSDVPMHRXNDM2R0UzRm9U?=
 =?utf-8?B?QjQ3a1Y1b0U3ZkJLNzVqZk1kSUJEL1FuK2VMQTliVmlveXhqWGdvQUJrZHpI?=
 =?utf-8?B?UDlhMkI4eWVGVWJwRHdlcGpkZ01qNzdONWQ4VnJmOThzYkxEc2daUzhsdjNU?=
 =?utf-8?B?UEFFSGNaYStlVW9VaWkzWWEyVytQWndWd1h2MDJKdU54Wm1uc1hKWFg5amVR?=
 =?utf-8?B?Tis3SVViV2JyeWJTekZsWEtUSXlYMzU3Z1VERHRCblBHZ25Lb1FuU21GUDNR?=
 =?utf-8?B?THFVdUl4dnMvcFlvbkhkUWVhZXFPMDdMYWczMVlaK2lTS3BGclZ6d3FyR1Fz?=
 =?utf-8?B?bitKOVh6TTdHeDRVd1A2Q2ErWnpYTnpqc2lrVll3WWFRZlU3TmkrcU1lSnlY?=
 =?utf-8?B?RG96WnZtQ001S0NkaFljaCtjV2h1RHhwVTNUeEYrZWxGNkNwelY1Z21ySlVw?=
 =?utf-8?B?bUE1RWxRajJZNHRGWExZNEdFSElCVU4wdnBCZU1GWmJPNzFHQmc2NDBxSndo?=
 =?utf-8?B?OVNXdU9SUE02SGorZTJrOEtRcUs2blpxRjNBSVVkL1dCRHZmWFZ1UEM0Nm1n?=
 =?utf-8?B?N2hpWHRwSE16L1cwUW9WWEl1WHN1bENZbWJ4WnJHa1ZXZExlTFp0NlNSUXVw?=
 =?utf-8?B?b3ZNWGR4T3FQN3BJZkh2WUlpN0tFNm1vZDgxVGU2TDFHRDN5WkRTMWZTanVO?=
 =?utf-8?B?b1FoWWtxZTU1dTVWeVVHb04zU2RvQThmUUhXb0N3K21aMHhpZWVOQi9YcE5Q?=
 =?utf-8?B?VjRWN0JiUzNmZ1RtWUc3ZGxiT3pFdUFGQzAwRkdFYWdlSFV6TzdkWXF4V3Fv?=
 =?utf-8?B?aXl1bmk3Wnh5MGJwclBkMnRFemh1MWtHNDgzZVlCeWZxWjEyUGdoYnRiQUpi?=
 =?utf-8?B?eStLMlU5bFdxdGxNekhvMGRMSXBzZytLKzVtNVRsakFueThSVG4rMGpRS3JI?=
 =?utf-8?B?Y08yRlgrY3ZKU1NxdWFxelFqZWk0UWgrbG9mYXpIa1pxTlVWVWJOQkcrYnls?=
 =?utf-8?B?akpKemhtVllnL3p0RXZDQ2xtVm9rVm14Q3NYRVEvMDAwUVM0Z0dNa3hpZnVB?=
 =?utf-8?B?dXNBQUZoVEd5MW1pTk81VVowRFZka1pEWEVUaW9BTW9CYmNJK2U3Um9ld1Q3?=
 =?utf-8?B?cSsxbmtzVUladW1uT0thTlZVTTRYSWFQazdnMXZ2QytTMnFaeHFBMVJNcXJP?=
 =?utf-8?B?VThpZGl3ckNCbUg4dU01Z2dMOTZRVzYzNFd6NVNSL1RLY3NYVzk3SHZOUGdE?=
 =?utf-8?Q?M1NCE1vcqQCZk5cW9ELeuBHjq?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c6ef3aa3-b22b-48aa-61d0-08dc6850083b
X-MS-Exchange-CrossTenant-AuthSource: BL1PR12MB5732.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Apr 2024 13:26:52.6186
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 1hbJyCQ1YMjLlkfV/HkyfBeCj8xkYKv5eC0l0+0stJi5uiCNWJ8PZxs2GawY5AATi5CIIA0prsN623gIJRffcg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR12MB6646

On 4/26/24 16:48, Dan Williams wrote:
> 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:

Right, that's the question. Do you use the default group ops that are 
created for TSM report or do you require the ops be part of the 
attributes. It is definitely easy to do the latter.

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

I'll update the patchset to do that.

Thanks,
Tom

>