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);

Why to constify the API?

- Protect caller's match data @*data which is for comparison and lookup
  and the API does not actually need to modify @*data.

- Make the API's parameters (@match)() and @data have the same type as
  all of other device finding APIs (bus|class|driver)_find_device().

- All kinds of existing device matching functions can be directly taken
  as the API's argument, they were exported by driver core.

How to constify the API?

- Now, no (@match)() argument of the API usages is modifying its match
  data @*data after previous cleanup, so it is easy and safe to make its
  parameter @data take const void * as type.

- Simplify involved codes further if it is possbile with benefits
  brought by constifying the API.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
Changes in v2:
- Series v1 have no code review comments and are posted a long time ago, so may ignore differences.
- Link to v1: https://lore.kernel.org/r/20240811-const_dfc_done-v1-0-9d85e3f943cb@quicinc.com
- Motivation link: https://lore.kernel.org/lkml/917359cc-a421-41dd-93f4-d28937fe2325@icloud.com

---
Zijun Hu (32):
      driver core: Constify API device_find_child()
      driver core: Introduce device_match_type() to match device with a device type
      drm/mediatek: Adapt for constified device_find_child()
      hwmon: Adapt for constified device_find_child()
      media: pci: mgb4: Adapt for constified device_find_child()
      thunderbolt: Adapt for constified device_find_child()
      gpio: sim: Remove gpio_sim_dev_match_fwnode()
      net: dsa: Adapt for constified device_find_child()
      pwm: Adapt for constified device_find_child()
      nvdimm: Adapt for constified device_find_child()
      libnvdimm: Simplify nd_namespace_store() implementation
      firewire: core: Adapt for constified device_find_child()
      serial: core: Adapt for constified device_find_child()
      usb: typec: class: Remove both cable_match() and partner_match()
      usb: typec: class: Adapt for constified device_find_child()
      slimbus: core: Simplify of_find_slim_device() implementation
      slimbus: core: Constify slim_eaddr_equal()
      slimbus: core: Adapt for constified device_find_child()
      scsi: iscsi: Constify API iscsi_find_flashnode_sess()
      scsi: qla4xxx: Adapt for constified iscsi_find_flashnode_sess()
      scsi: iscsi: Adapt for constified device_find_child()
      cxl/region: Adapt for constified device_find_child()
      cxl/pmem: Remove match_nvdimm_bridge()
      cxl/core/pci: Adapt for constified device_find_child()
      cxl/test: Adapt for constified device_find_child()
      sparc: vio: Adapt for constified device_find_child()
      bus: fsl-mc: Adapt for constified device_find_child()
      block: sunvdc: Adapt for constified device_find_child()
      firmware: arm_scmi: Adapt for constified device_find_child()
      efi: dev-path-parser: Adapt for constified device_find_child()
      rpmsg: core: Adapt for constified device_find_child()
      driver core: Simplify API device_find_child_by_name() implementation

 arch/sparc/kernel/vio.c                |  6 +++---
 drivers/base/core.c                    | 30 ++++++++++--------------------
 drivers/block/sunvdc.c                 |  6 +++---
 drivers/bus/fsl-mc/dprc-driver.c       |  6 +++---
 drivers/cxl/core/pci.c                 |  4 ++--
 drivers/cxl/core/pmem.c                |  9 +++------
 drivers/cxl/core/region.c              | 21 ++++++++++++---------
 drivers/firewire/core-device.c         |  4 ++--
 drivers/firmware/arm_scmi/bus.c        |  4 ++--
 drivers/firmware/efi/dev-path-parser.c |  4 ++--
 drivers/gpio/gpio-sim.c                |  7 +------
 drivers/gpu/drm/mediatek/mtk_drm_drv.c |  2 +-
 drivers/hwmon/hwmon.c                  |  2 +-
 drivers/media/pci/mgb4/mgb4_core.c     |  4 ++--
 drivers/nvdimm/bus.c                   |  2 +-
 drivers/nvdimm/claim.c                 |  9 +--------
 drivers/pwm/core.c                     |  2 +-
 drivers/rpmsg/rpmsg_core.c             |  4 ++--
 drivers/scsi/qla4xxx/ql4_os.c          |  3 ++-
 drivers/scsi/scsi_transport_iscsi.c    | 10 +++++-----
 drivers/slimbus/core.c                 | 17 +++++------------
 drivers/thunderbolt/retimer.c          |  2 +-
 drivers/thunderbolt/xdomain.c          |  2 +-
 drivers/tty/serial/serial_core.c       |  4 ++--
 drivers/usb/typec/class.c              | 31 ++++++++++++++-----------------
 include/linux/device.h                 |  4 ++--
 include/linux/device/bus.h             |  1 +
 include/scsi/scsi_transport_iscsi.h    |  4 ++--
 net/dsa/dsa.c                          |  2 +-
 tools/testing/cxl/test/cxl.c           |  2 +-
 30 files changed, 89 insertions(+), 119 deletions(-)
---
base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
change-id: 20241201-const_dfc_done-aaec71e3bbea

Best regards,
-- 
Zijun Hu <quic_zijuhu@quicinc.com>

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:

  CC      drivers/pwm/core.o
drivers/pwm/core.c: In function ‘pwm_unexport_child’:
drivers/pwm/core.c:1292:55: error: passing argument 3 of ‘device_find_child’ from incompatible pointer type [-Wincompatible-pointer-types]
 1292 |         pwm_dev = device_find_child(pwmchip_dev, pwm, pwm_unexport_match);
      |                                                       ^~~~~~~~~~~~~~~~~~
      |                                                       |
      |                                                       int (*)(struct device *, void *)
In file included from include/linux/acpi.h:14,
                 from drivers/pwm/core.c:11:
include/linux/device.h:1085:49: note: expected ‘device_match_t’ {aka ‘int (*)(struct device *, const void *)’} but argument is of type ‘int (*)(struct device *, void *)’
 1085 |                                  device_match_t match);
      |                                  ~~~~~~~~~~~~~~~^~~~~
drivers/pwm/core.c: In function ‘pwm_class_get_state’:
drivers/pwm/core.c:1386:55: error: passing argument 3 of ‘device_find_child’ from incompatible pointer type [-Wincompatible-pointer-types]
 1386 |         pwm_dev = device_find_child(pwmchip_dev, pwm, pwm_unexport_match);
      |                                                       ^~~~~~~~~~~~~~~~~~
      |                                                       |
      |                                                       int (*)(struct device *, void *)
include/linux/device.h:1085:49: note: expected ‘device_match_t’ {aka ‘int (*)(struct device *, const void *)’} but argument is of type ‘int (*)(struct device *, void *)’
 1085 |                                  device_match_t match);
      |                                  ~~~~~~~~~~~~~~~^~~~~
make[5]: *** [scripts/Makefile.build:194: drivers/pwm/core.o] Error 1
make[4]: *** [scripts/Makefile.build:440: drivers/pwm] Error 2
make[3]: *** [scripts/Makefile.build:440: drivers] Error 2
make[2]: *** [Makefile:1989: .] Error 2
make[1]: *** [Makefile:372: __build_one_by_one] Error 2
make: *** [Makefile:251: __sub-make] Error 2

Best regards
Uwe

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?

or squash such patch series into a single patch ?

various subsystem maintainers may not like squashing way.

> 
>   CC      drivers/pwm/core.o
> drivers/pwm/core.c: In function ‘pwm_unexport_child’:
> drivers/pwm/core.c:1292:55: error: passing argument 3 of ‘device_find_child’ from incompatible pointer type [-Wincompatible-pointer-types]
>  1292 |         pwm_dev = device_find_child(pwmchip_dev, pwm, pwm_unexport_match);
>       |                                                       ^~~~~~~~~~~~~~~~~~
>       |                                                       |
>       |                                                       int (*)(struct device *, void *)
> In file included from include/linux/acpi.h:14,
>                  from drivers/pwm/core.c:11:
> include/linux/device.h:1085:49: note: expected ‘device_match_t’ {aka ‘int (*)(struct device *, const void *)’} but argument is of type ‘int (*)(struct device *, void *)’
>  1085 |                                  device_match_t match);
>       |                                  ~~~~~~~~~~~~~~~^~~~~
> drivers/pwm/core.c: In function ‘pwm_class_get_state’:
> drivers/pwm/core.c:1386:55: error: passing argument 3 of ‘device_find_child’ from incompatible pointer type [-Wincompatible-pointer-types]
>  1386 |         pwm_dev = device_find_child(pwmchip_dev, pwm, pwm_unexport_match);
>       |                                                       ^~~~~~~~~~~~~~~~~~
>       |                                                       |
>       |                                                       int (*)(struct device *, void *)
> include/linux/device.h:1085:49: note: expected ‘device_match_t’ {aka ‘int (*)(struct device *, const void *)’} but argument is of type ‘int (*)(struct device *, void *)’
>  1085 |                                  device_match_t match);
>       |                                  ~~~~~~~~~~~~~~~^~~~~
> make[5]: *** [scripts/Makefile.build:194: drivers/pwm/core.o] Error 1
> make[4]: *** [scripts/Makefile.build:440: drivers/pwm] Error 2
> make[3]: *** [scripts/Makefile.build:440: drivers] Error 2
> make[2]: *** [Makefile:1989: .] Error 2
> make[1]: *** [Makefile:372: __build_one_by_one] Error 2
> make: *** [Makefile:251: __sub-make] Error 2
> 
> Best regards
> Uwe

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

On 2024/12/3 20:41, Greg Kroah-Hartman wrote:
> 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 :(
> 

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

> thanks,
> 
> greg k-h

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

On 2024-12-03 08:58:26-0500, James Bottomley wrote:
> 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.

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.
This also enables an incremental migration.


Thomas

On 2024/12/3 22:07, Thomas Weißschuh wrote:
> On 2024-12-03 08:58:26-0500, James Bottomley wrote:
>> 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.
> 
> 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.

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.

> Thomas

On Tue, 2024-12-03 at 22:56 +0800, Zijun Hu wrote:
> On 2024/12/3 22:07, Thomas Weißschuh wrote:
> > On 2024-12-03 08:58:26-0500, James Bottomley wrote:
> > > 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.
> > 
> > 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

On 12/3/2024 8:33 AM, 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);
> 
> Why to constify the API?
> 
> - Protect caller's match data @*data which is for comparison and lookup
>   and the API does not actually need to modify @*data.
> 
> - Make the API's parameters (@match)() and @data have the same type as
>   all of other device finding APIs (bus|class|driver)_find_device().
> 
> - All kinds of existing device matching functions can be directly taken
>   as the API's argument, they were exported by driver core.
> 
> How to constify the API?
> 
> - Now, no (@match)() argument of the API usages is modifying its match
>   data @*data after previous cleanup, so it is easy and safe to make its
>   parameter @data take const void * as type.
> 
> - Simplify involved codes further if it is possbile with benefits
>   brought by constifying the API.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
> Changes in v2:
> - Series v1 have no code review comments and are posted a long time ago, so may ignore differences.
> - Link to v1: https://lore.kernel.org/r/20240811-const_dfc_done-
> v1-0-9d85e3f943cb@quicinc.com
> - Motivation link: https://lore.kernel.org/lkml/917359cc-a421-41dd-93f4-d28937fe2325@icloud.com
> 
> ---
> Zijun Hu (32):
>       driver core: Constify API device_find_child()
>       driver core: Introduce device_match_type() to match device with a device type
>       drm/mediatek: Adapt for constified device_find_child()
>       hwmon: Adapt for constified device_find_child()
>       media: pci: mgb4: Adapt for constified device_find_child()
>       thunderbolt: Adapt for constified device_find_child()
>       gpio: sim: Remove gpio_sim_dev_match_fwnode()
>       net: dsa: Adapt for constified device_find_child()
>       pwm: Adapt for constified device_find_child()
>       nvdimm: Adapt for constified device_find_child()
>       libnvdimm: Simplify nd_namespace_store() implementation
>       firewire: core: Adapt for constified device_find_child()
>       serial: core: Adapt for constified device_find_child()
>       usb: typec: class: Remove both cable_match() and partner_match()
>       usb: typec: class: Adapt for constified device_find_child()
>       slimbus: core: Simplify of_find_slim_device() implementation
>       slimbus: core: Constify slim_eaddr_equal()
>       slimbus: core: Adapt for constified device_find_child()
>       scsi: iscsi: Constify API iscsi_find_flashnode_sess()
>       scsi: qla4xxx: Adapt for constified iscsi_find_flashnode_sess()
>       scsi: iscsi: Adapt for constified device_find_child()
>       cxl/region: Adapt for constified device_find_child()
>       cxl/pmem: Remove match_nvdimm_bridge()
>       cxl/core/pci: Adapt for constified device_find_child()
>       cxl/test: Adapt for constified device_find_child()
>       sparc: vio: Adapt for constified device_find_child()
>       bus: fsl-mc: Adapt for constified device_find_child()
>       block: sunvdc: Adapt for constified device_find_child()
>       firmware: arm_scmi: Adapt for constified device_find_child()
>       efi: dev-path-parser: Adapt for constified device_find_child()
>       rpmsg: core: Adapt for constified device_find_child()
>       driver core: Simplify API device_find_child_by_name() implementation

sorry for that only part of this series [0/32, 11/32] were sent out due
to mail account capability limitation.

will solve the limitation and send out whole patch series as v3.

thanks (^^)