Re: [PATCH v2 00/32] driver core: Constify API device_find_child() and adapt for various existing usages

[Greg Kroah-Hartman]

On Tue, Dec 03, 2024 at 08:23:45PM +0800, Zijun Hu wrote:
> On 2024/12/3 20:00, Uwe Kleine-König wrote:
> > Hello,
> >
> > On Tue, Dec 03, 2024 at 08:33:22AM +0800, Zijun Hu wrote:
> >> This patch series is to constify the following API:
> >> struct device *device_find_child(struct device *dev, void *data,
> >> int (*match)(struct device *dev, void *data));
> >> To :
> >> struct device *device_find_child(struct device *dev, const void *data,
> >> device_match_t match);
> >> typedef int (*device_match_t)(struct device *dev, const void *data);
> >
> > This series isn't bisectible. With only the first two patches applied I
> > hit:
>
> yes. such patch series needs to be merge as atomic way.
>
> Hi Greg,
>
> is it possible to ONLY merge such patch series by atomic way into your
> driver-core tree?

Nope!

> or squash such patch series into a single patch ?
>
> various subsystem maintainers may not like squashing way.

Agreed, so look into either doing it in a bisectable way if at all
possible. As I don't see a full series here, I can't suggest how it
needs to happen :(

thanks,

greg k-h

[James Bottomley]

On Tue, 2024-12-03 at 21:02 +0800, Zijun Hu wrote:

> On 2024/12/3 20:41, Greg Kroah-Hartman wrote:
> > On Tue, Dec 03, 2024 at 08:23:45PM +0800, Zijun Hu wrote:

[...]

> > > or squash such patch series into a single patch ?
> > >
> > > various subsystem maintainers may not like squashing way.
> >
> > Agreed, so look into either doing it in a bisectable way if at all
> > possible.  As I don't see a full series here, I can't suggest how
> > it needs to happen :(
> >
>

> let me send you a full series later and discuss how to solve this
> issue.

It's only slightly more complex than what we normally do: modify all
instances and then change the API. In this case you have an additional
problem because the prototype "const void *" will cause a mismatch if a
function has "void *". The easiest way to solve this is probably to
make device_find_child a macro that coerces its function argument to
having a non const "void *" and then passes off to the real function.
If you do that in the first patch, then you can constify all the
consumers and finally remove the macro coercion in the last patch.

James

[James Bottomley]

On Tue, 2024-12-03 at 22:56 +0800, Zijun Hu wrote:
> On 2024/12/3 22:07, Thomas Weißschuh wrote:

> > Casting function pointers like that should be detected and trapped
> > by control flow integrity checking (KCFI).
> >
> > Another possibility would be to use a macro and _Generic to
> > dispatch to two different backing functions. See __BIN_ATTR() in
> > include/linux/sysfs.h for an inspiration.

That's way over complicated for this conversion: done properly there
should be no need for _Generic() compile time type matching at all.

> this way may fix building error issue but does not achieve our
> purpose. our purpose is that there are only constified
> device_find_child().
>
>
> > This also enables an incremental migration.
>
> change the API prototype from:
> device_find_child(..., void *data_0, int (*match)(struct device *dev,
> void *data));
>
> to:
> device_find_child(..., const void *data_0, int (*match)(struct device
> *dev, const void *data));
>
> For @data_0,  void * -> const void * is okay.
> but for @match, the problem is function pointer type incompatibility.
>
> there are two solutions base on discussions.
>
> 1) squashing likewise Greg mentioned.
>    Do all of the "prep work" first, and then
>    do the const change at the very end, all at once.
>
> 2)  as changing platform_driver's remove() prototype.
> Commit: e70140ba0d2b ("Get rid of 'remove_new' relic from platform
> driver struct")
>
>  introduce extra device_find_child_new() which is constified  -> use
> *_new() replace ALL device_find_child() instances one by one -> 
> remove device_find_child() -> rename *_new() to device_find_child()
> once.

Why bother with the last step, which churns the entire code base again?
Why not call the new function device_find_child_const() and simply keep
it (it's descriptive of its function). That way you can have a patch
series without merging and at the end simply remove the old function.

Regards,

James

[James Bottomley]

On Wed, 2024-12-04 at 20:26 +0800, Zijun Hu wrote:

> On 2024/12/3 23:34, James Bottomley wrote:
> > > > This also enables an incremental migration.
> > > change the API prototype from:
> > > device_find_child(..., void *data_0, int (*match)(struct device
> > > *dev, void *data));
> > >
> > > to:
> > > device_find_child(..., const void *data_0, int (*match)(struct
> > > device *dev, const void *data));
> > >
> > > For @data_0,  void * -> const void * is okay.
> > > but for @match, the problem is function pointer type
> > > incompatibility.
> > >
> > > there are two solutions base on discussions.
> > >
> > > 1) squashing likewise Greg mentioned.
> > >    Do all of the "prep work" first, and then
> > >    do the const change at the very end, all at once.
> > >
> > > 2)  as changing platform_driver's remove() prototype.
> > > Commit: e70140ba0d2b ("Get rid of 'remove_new' relic from
> > > platform driver struct")
> > >
> > >  introduce extra device_find_child_new() which is constified  ->
> > > use *_new() replace ALL device_find_child() instances one by one
> > > -> remove device_find_child() -> rename *_new() to
> > > device_find_child() once.
> > Why bother with the last step, which churns the entire code base
> > again?
>

> keep the good API name device_find_child().

Well, I think it's a good opportunity to rename the API better, but if
that's the goal, you can still do it with _Generic() without churning
the code base a second time. The example is in
slab.h:kmem_cache_create

> > Why not call the new function device_find_child_const() and simply
> > keep it (it's descriptive of its function).  That way you can have
> > a patch series without merging and at the end simply remove the old
> > function.
>

> device_find_child is a good name for the API, 'find' already means
> const.

Not to me it doesn't, but that's actually not what I think is wrong
with the API name: it actually only returns the first match, so I'd
marginally prefer it to be called device_find_first_child() ... not
enough to churn the code to change it, but since you're doing that
anyway it might make sense as an update.

Regards,

James