On Mon, Oct 10, 2022 at 08:00:09AM +0100, John Garry wrote: > On 07/10/2022 14:23, Niklas Cassel wrote: > > At the request of the libata maintainer, introduce a ata_port_is_frozen() > > helper function. > > > > Signed-off-by: Niklas Cassel <niklas.cassel@xxxxxxx> > > --- > > include/linux/libata.h | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/include/linux/libata.h b/include/linux/libata.h > > index a505cfb92ab3..d5ac52654b42 100644 > > --- a/include/linux/libata.h > > +++ b/include/linux/libata.h > > @@ -1043,6 +1043,11 @@ static inline int ata_port_is_dummy(struct ata_port *ap) > > return ap->ops == &ata_dummy_port_ops; > > } > > Hi Niklas, Hello John, > > > > > +static inline bool ata_port_is_frozen(const struct ata_port *ap) > > The majority of libata APIs don't use const in this way, so I think that > consistency is better. Well, right now there is no consistency :) $ git grep "static inline" include/linux/libata.h | grep "(const struct" include/linux/libata.h:static inline bool ata_port_is_frozen(const struct ata_port *ap) include/linux/libata.h:static inline int ata_acpi_stm(const struct ata_port *ap, include/linux/libata.h:static inline int ata_acpi_gtm(const struct ata_port *ap, include/linux/libata.h:static inline bool ata_is_host_link(const struct ata_link *link) include/linux/libata.h:static inline bool ata_is_host_link(const struct ata_link *link) include/linux/libata.h:static inline unsigned int ata_dev_enabled(const struct ata_device *dev) include/linux/libata.h:static inline unsigned int ata_dev_disabled(const struct ata_device *dev) include/linux/libata.h:static inline unsigned int ata_dev_absent(const struct ata_device *dev) include/linux/libata.h:static inline int ata_link_max_devices(const struct ata_link *link) include/linux/libata.h:static inline int ata_try_flush_cache(const struct ata_device *dev) There are 10 uses (9 without my addition) that uses a const struct pointer. So since both are used in libata, I chose the one that seemed most correct. > > Indeed, this is not const data which you're pointing at, so maybe it's > better to be honest with the compiler. And since this is inlined, could the > compiler optimise out multiple reads on ap->flags in a caller function since > we tell it it's const? "This is not const data which you're pointing at" Well, according to 6.7.6.1 Pointer declarators in https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3054.pdf A "const struct *ptr" means that the contents of any object pointed to cannot be modified through that pointer. "And since this is inlined, could the compiler optimise out multiple reads on ap->flags in a caller function since we tell it it's const?" I'm far from a compiler expert, but because an optimising compiler is free to inline whatever function it wants, not just functions marked inline, I would assume that the compiler would "do the right thing" regardless if a function is marked as inline or not. Doing a: git grep "static inline" include/ | grep "(const struct" | wc -l 2055 Makes me quite confident that this should be fine. Sure, the data it points to might never change. But seeing e.g.: $ git grep "static inline" include/ | grep "empty(const struct" Especially used in tcp and qdisc makes me even more confident that this will work fine. Looking at e.g. __dev_xmit_skb(): https://elixir.bootlin.com/linux/v6.0/source/net/core/dev.c#L3803 we can see that it uses nolock_qdisc_is_empty() multiple times within the same function. So now I'm very confident that this will be fine :) Kind regards, Niklas
