linux-kernel - Re: [PATCH v1 1/1] device property: Add const qualifier to device_get_match

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 23 Sep 2022 11:25:19 +0000
From:   Sakari Ailus <sakari.ailus@...ux.intel.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
        Daniel Scally <djrscally@...il.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v1 1/1] device property: Add const qualifier to
 device_get_match_data() parameter

Hi Andy,

On Fri, Sep 23, 2022 at 01:25:42PM +0300, Andy Shevchenko wrote:
> On Thu, Sep 22, 2022 at 08:22:54PM +0000, Sakari Ailus wrote:
> > On Thu, Sep 22, 2022 at 04:54:10PM +0300, Andy Shevchenko wrote:
> > > Add const qualifier to the device_get_match_data() parameter.
> > > Some of the future users may utilize this function without
> > > forcing the type.
> > 
> > From const to non-const? This is what this patch does, right?
> 
> Right.
> 
> > > All the same, dev_fwnode() may be used with a const qualifier.
> > > 
> > > Reported-by: kernel test robot <lkp@...el.com>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> 
> > > -struct fwnode_handle *dev_fwnode(struct device *dev)
> > > +struct fwnode_handle *dev_fwnode(const struct device *dev)
> > 
> > If you have const struct device pointer, then the embedded fwnode handle in
> > that object sure is const, too. Isn't it?
> > 
> > If you really have const struct device pointer (where do you?), then I'd
> 
> device_get_match_data() expects the const parameter, but due to dev_fwnode()
> it can't be satisfied. This has been reported by LKP when I tried to submit
> a wrapper:
> https://lore.kernel.org/linux-spi/20220921204520.23984-1-andriy.shevchenko@linux.intel.com/
> 
> Yes, I probably can drop const there, but it will be again awkward to see
> almost all APIs beneath using const and upper one without it for unclear
> (to the reader) reasons.

dev could indeed be const there otherwise, I understand, but it's certainly
not better to force it non-const elsewhere for that.

> 
> > suggest to add another function, dev_fwnode_const() that is otherwise the
> > same except the argument as well as the return value are const.
> 
> Perhaps this is the case, but does it mean we need to duplicate APIs when
> we know we don't modify data? Seems road to bloating the code for peanuts.
> 
> > Or alternatively define it as a macro and use _Generic()?
> 
> Yeah, I have the mixed feelings about _Generic for generic APIs because
> it requires to convert some stuff to macros when type checking of the
> parameters can be missed (if a target takes two or more of them).

It's not uncommon to use wrapper functions in addition to get type checking
done properly.

> 
> It might work here (we have a single parameter), but in general...

If it works here, why not to do it then? :-)

-- 
Sakari Ailus