Re: [PATCH v25 01/10] drivers/base: refactor cpu.c to use .is_visible()

From: Eric DeVolder
Date: Thu Aug 03 2023 - 14:22:17 EST




On 7/21/23 11:32, Eric DeVolder wrote:


On 7/3/23 11:53, Eric DeVolder wrote:


On 7/3/23 08:05, Greg KH wrote:
On Thu, Jun 29, 2023 at 03:21:10PM -0400, Eric DeVolder wrote:
  - the function body of the callback functions are now wrapped with
    IS_ENABLED(); as the callback function must exist now that the
    attribute is always compiled-in (though not necessarily visible).

Why do you need to do this last thing?  Is it a code savings goal?  Or
something else?  The file will not be present in the system if the
option is not enabled, so it should be safe to not do this unless you
feel it's necessary for some reason?

To accommodate the request, all DEVICE_ATTR() must be unconditionally present in this file. The DEVICE_ATTR() requires the .show() callback. As the callback is referenced from a data structure, the callback has to be present for link. All the callbacks for these attributes are in this file.

I have two basic choices for gutting the function body if the config feature is not enabled. I can either use #ifdef or IS_ENABLED(). Thomas has made it clear I need to use IS_ENABLED(). I can certainly use #ifdef (which is what I did in v24).


Not doing this would make the diff easier to read :)

I agree this is messy. I'm not really sure what this request/effort achieves as these attributes are not strongly related (unlike cacheinfo) and the way the file was before results in less code.

At any rate, please indicate if you'd rather I use #ifdef.
Thanks for your time!
eric


thanks,

greg k-h

Hi Greg,
I was wondering if you might weigh-in so that I can proceed.

I think there are three options on the table:
- use #ifdef to comment out these function bodies, which keeps the diff much more readable
- use IS_ENABLED() as Thomas has requested I do, but makes the diff more difficult to read
- remove this refactor altogether, perhaps post-poning until after this crash hotplug series merges, as this refactor is largely unrelated to crash hotplug.

Thank you for your time on this topic!
eric

Hi Greg,
If you have an opinion on how to proceed, please provide.
Thanks,
eric