Received-SPF: pass (google.com: domain of linux-kernel+bounces-161101-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Sat, 27 Apr 2024 09:32:02 -0700
From: Dan Williams <dan.j.williams@intel.com>
To: Lukas Wunner <lukas@wunner.de>, Dan Williams <dan.j.williams@intel.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, "Rafael J. Wysocki"
	<rafael@kernel.org>, <linux-kernel@vger.kernel.org>, Alan Stern
	<stern@rowland.harvard.edu>, Jonathan Cameron <Jonathan.Cameron@huawei.com>
Subject: Re: [PATCH] sysfs: Allow bin_attributes to be added to groups
Message-ID: <662d288263cd5_b6e0294b5@dwillia2-mobl3.amr.corp.intel.com.notmuch>
References: <e140f6bf8dc99c24cc387f7ac3c7908c8830587f.1714030457.git.lukas@wunner.de>
 <662be72549aaf_db82d294c7@dwillia2-xfh.jf.intel.com.notmuch>
 <Ziv5p3SMEIw3XZkK@wunner.de>
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <Ziv5p3SMEIw3XZkK@wunner.de>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?yaNwrP6Xn9bRdaKSPw4qGE4JFFSakeeqMiKBtXBGb2blwmza30BmRIm44j?=
 =?iso-8859-1?Q?vM/f+1snh+pqUiwFeGuJCC3eBszh2FamfGHDMzjX02K4BToC3z3rG47+Fe?=
 =?iso-8859-1?Q?O0PtVEQ4ogznsUcj7j9lPyP7rJVR9qYjW46E1vghbza7NRA1bQra1IFDTo?=
 =?iso-8859-1?Q?gKp21PWlZRIvV4gq2YDhBVOny9g2nEXPImjWuak2bYotyMoEpJEYlV7BDK?=
 =?iso-8859-1?Q?4WKZ8IEUflmrAKhqX7vnvujA/k+zogzpq9VWNlVAvhpL7/gnWautlHppLX?=
 =?iso-8859-1?Q?XHB7JVdQ2cPOlmNKY2Irtji76xiSzWWRb/VJB9dcGyCxwgoysTVgLa7e5C?=
 =?iso-8859-1?Q?zP+/LHSA0GYCLCCKXYM1OKHbL7kZz75U+KFT2A8Ve1Vlb+g7rwDmioxDtY?=
 =?iso-8859-1?Q?9ddwK9A1GoBl3NMkAU08sx7Hq2Xq+uJ4Y+4JvD4Ic/JmQEF/7AnNGx7CbP?=
 =?iso-8859-1?Q?CKZgNWlYLjGPByMyED5V9iz1BttPikiFOCj0h6+BAS0zxXMqy+gEnppu4j?=
 =?iso-8859-1?Q?w85XOTextuqKHJpIdYaAdRvjEG0Ok58PrjnDSWK6/nPaEnBLYppBYkmD+g?=
 =?iso-8859-1?Q?fQGryrl8toxaRCH2jIVyVJyJa+VKatCyAdKsspQuGSn80vn/cySNuwi7NQ?=
 =?iso-8859-1?Q?99E2JGxcWpUAaPAcONL4sN9nW0nOzUUPH7mJ5DhiiNYrrjydmgUY1xhgeN?=
 =?iso-8859-1?Q?2KQW2S19jncpmoiSbEeBXo6WgwUznWe3CHfZGSNRsy85tqbDhnp1LrghJL?=
 =?iso-8859-1?Q?41HxfArY1zQ3G8kABTIyMuslV5vK4h9Qx2BJR2/MNxQz0HAWWykjbZhb+W?=
 =?iso-8859-1?Q?UAN2NzOkI1eQeGx/VsE+LSyqQ6FYD5G/HavpxRHe1CJwYBDY+awcGVmIdx?=
 =?iso-8859-1?Q?OSQq6BYiEAqoJ1tdPS5uIbPDhclaMfgDQYLmho4BpzJ0KMrVn0YUFQXQ+U?=
 =?iso-8859-1?Q?nR86f7BWu0zX8KDK0JwkxJSmLNOP9rVXY5NxVVsn7zX6mLn7kUBrUMaaq5?=
 =?iso-8859-1?Q?Pb8R45K5VF60rMwNG3ORX7+FSlSwaNmUJKPVC1t+nbaQPZbt+15R6eywEB?=
 =?iso-8859-1?Q?YoBWL6yaymn7TDIdK6ebLcfsovCJflIgdja1KBbpDFl1OA69z7hAyv3YuY?=
 =?iso-8859-1?Q?eJdE1dW55ARuGGQiKDN9ehZ/CSfH2+621aPuwT72lkE/GTdnaFSr+VYtNx?=
 =?iso-8859-1?Q?/nQjquX1RP49tAwXtJmfTodoMw/IISMDcFNRqVcRKlqEdnBm1P0mP6aswB?=
 =?iso-8859-1?Q?m0f+3wZrEtTrtsWP7/UYWNfpx7elddy4El2e7oOkF1K9iHmJfk5nyGsNyY?=
 =?iso-8859-1?Q?9dFgkgIrJYk/ENZWe3zF8F3S2aZc/LcPgUWcRbLL5IkFXr41gwivGklL6G?=
 =?iso-8859-1?Q?QHboK3tol/i3NaaSGv6yCWLWflMuYG+AwNdNdCzYXe5NzOF+lQctevubHr?=
 =?iso-8859-1?Q?vSnJGaYg+H8hxlx+cQx15Z8DYtpvcZL78RJhshHO58REeKfZwP1BgbwYtt?=
 =?iso-8859-1?Q?e+32ivQaFQSUbyRoPCFs392hswynD2dNuxcdgO04O9hRwEaNzjBA+EVLfF?=
 =?iso-8859-1?Q?wSzRIzf7MMiavHPQpyYIwIgTNSMvnbX2g5N97PWANAGATFjF+qF16nbQaT?=
 =?iso-8859-1?Q?qozuRNrTwwUvYZas8n2eLwrxnJX1ussUI7jOq/jtJ4S/bTrZbMb+aFsQ?=
 =?iso-8859-1?Q?=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f289ed44-09f8-4462-ace1-08dc66d792c7
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Apr 2024 16:32:04.7718
 (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: d9kJ8v8Ttaxx6ZOmu1TKOANXpryDQivxvTV8K1XaUayJo/phgY/uKvQk3XJWCfiH3/osj6t+WmkZo3e6yEk53s0Hj17V/xMNNxIV/j1TPFk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB6790
X-OriginatorOrg: intel.com

Lukas Wunner wrote:
> On Fri, Apr 26, 2024 at 10:40:53AM -0700, Dan Williams wrote:
> > Lukas Wunner wrote:
> > > Commit dfa87c824a9a ("sysfs: allow attributes to be added to groups")
> > > introduced dynamic addition of sysfs attributes to groups.
> > > 
> > > Allow the same for bin_attributes, in support of a forthcoming commit
> > > which adds various bin_attributes every time a PCI device is
> > > authenticated.
> > > 
> > > Addition of bin_attributes to groups differs from regular attributes in
> > > that different kernfs_ops are selected by sysfs_add_bin_file_mode_ns()
> > > vis-?-vis sysfs_add_file_mode_ns().
> > > 
> > > So call either of those two functions from sysfs_add_file_to_group()
> > > based on an additional boolean parameter and add two wrapper functions,
> > > one for bin_attributes and another for regular attributes.
> > > 
> > > Removal of bin_attributes from groups does not require a differentiation
> > > for bin_attributes and can use the same code path as regular attributes.
> > > 
> > > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > > Cc: Alan Stern <stern@rowland.harvard.edu>
> > > ---
> > > Submitting this ahead of my PCI device authentication v2 patches.
> > > Not sure if the patch is acceptable without an accompanying user,
> > > but even if it's not, perhaps someone has early review feedback
> > > or wants to provide an Acked-by?  Thank you!
> > 
> > On the one hand it makes sense from a symmetry perspective, on the other
> > hand the expectation and the infrastructure for dynamic sysfs visibilty
> > has increased since 2007.
> > 
> > That is why I would like to see the use case to understand why a
> > dynamically added a bin_attribute is needed compared with a statically
> > defined attribute with dynamic visibility.
> 
> I assume "would like to see" means "on the mailing list", which would
> (or will) be as part of the PCI device authentication v2 patches.

That works, yeah.

[..]
> Each signature needs a unique filename and the number of signatures
> that can be generated is essentially unlimited.  (Though older ones
> are likely uninteresting and can be culled.)  If you want to expose,
> say, up to 1000 signatures per device, you'd have to allocate an array
> of 1000 signatures (for each device!), even though the actual number
> of signatures received might be much lower.  It would be a waste of
> memory.  It is much more economical to add signature attributes
> dynamically on demand.

Ah, yeah. If it was a double-digit number of files then a static array
would be ok, but once it gets to 1000 potential files, then dynamic
makes complete sense.

> It has the additional benefit that it allows user space to dynamically
> adjust the maximum number of signatures retained in the log.
> That's more difficult to implement with static attributes, as you'd
> have to reallocate the attributes array and adjust all the pointers
> pointing to it.

To be clear, and for example, dynamic adjusting an array of attributes
based on userspace input is what cxl_region_target_group does, but again
with 1000 potential files the static array would be silly.

Thanks for the details!