On Fri, Apr 02, 2021 at 08:30:18PM -0300, Jason Gunthorpe wrote: > On Fri, Apr 02, 2021 at 04:03:30PM -0700, Kees Cook wrote: > > > > relevant. It seems to me that the hw_counters 'struct attribute_group' > > > should probably be its own kobj within both of these structures so they > > > can have their own sysfs ops (unless there is some other way to do this > > > that I am missing). > > Err, yes, every subclass of the attribute should be accompanied by a > distinct kobject type to relay the show methods with typesafety, this > is how this design pattern is intended to be used. > > If I understand your report properly the hw_stats_attribute is being > assigned to a 'port_type' kobject and it only works by pure luck because > the show/store happens to overlap between port and hsa attributes? "happens to" :) Yeah, they're all like this, unfortunately: https://lore.kernel.org/lkml/202006112217.2E6CE093@keescook/ This is the first that I've seen that crossed kobj_types in the same group, though. :) > > > I would appreciate someone else taking a look and seeing if I am off > > > base or if there is an easier way to solve this. > > > > So, it seems that the reason for a custom kobj_type here is to use the > > .release callback. > > Every kobject should be associated with a specific, fixed, attribute > type. The purpose of the wrappers is to inject type safety so the > attribute implementations know they are working on the right stuff. Right -- though it's not specifically required to be a fixed attribute. It can just be a "generic" kobject. This seems to happen a lot when something wants to show up a global or const value in /sys > The answer is that the setup_hw_stats_() for port and device must > be split up and the attribute implementations for each of them have to > subclass starting from the correct type. I'm not convinced that just backing everything off to kobject isn't simpler? > And then two show/set functions that bounce through the correct types > to the data. I'd like to make these things compile-time safe (there is not type associated with use the __ATTR() macro, for example). That I haven't really figured out how to do right. -- Kees Cook
