| Message ID | 20241211-const_dfc_done-v4-0-583cc60329df@quicinc.com |
|---|---|
| Headers | show |
| Series | driver core: Constify API device_find_child() | expand |
On Wed, 11 Dec 2024 08:08:03 +0800 Zijun Hu <zijun_hu@icloud.com> wrote: > From: Zijun Hu <quic_zijuhu@quicinc.com> > > Simplify nd_namespace_store() implementation by > using device_find_child_by_name(). > > Reviewed-by: Alison Schofield <alison.schofield@intel.com> > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> LGTM Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/nvdimm/claim.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/drivers/nvdimm/claim.c b/drivers/nvdimm/claim.c > index 030dbde6b0882050c90fb8db106ec15b1baef7ca..9e84ab411564f9d5e7ceb687c6491562564552e3 100644 > --- a/drivers/nvdimm/claim.c > +++ b/drivers/nvdimm/claim.c > @@ -67,13 +67,6 @@ bool nd_attach_ndns(struct device *dev, struct nd_namespace_common *attach, > return claimed; > } > > -static int namespace_match(struct device *dev, void *data) > -{ > - char *name = data; > - > - return strcmp(name, dev_name(dev)) == 0; > -} > - > static bool is_idle(struct device *dev, struct nd_namespace_common *ndns) > { > struct nd_region *nd_region = to_nd_region(dev->parent); > @@ -168,7 +161,7 @@ ssize_t nd_namespace_store(struct device *dev, > goto out; > } > > - found = device_find_child(dev->parent, name, namespace_match); > + found = device_find_child_by_name(dev->parent, name); > if (!found) { > dev_dbg(dev, "'%s' not found under %s\n", name, > dev_name(dev->parent)); >
On Wed, 11 Dec 2024 08:08:05 +0800 Zijun Hu <zijun_hu@icloud.com> wrote: > From: Zijun Hu <quic_zijuhu@quicinc.com> > > fsl_mc_device_match() does not modify caller's inputs. > > Constify it by simply changing its parameter types to const pointer. > > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> Similar to previous patch, I'd say why you are making this change. There are may places in the kernel where pointers are constant but not marked so. Why does this one matter? With that info added Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/bus/fsl-mc/dprc-driver.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/bus/fsl-mc/dprc-driver.c b/drivers/bus/fsl-mc/dprc-driver.c > index 4b68c84ef485055c9b300b4f7912a20f959b8ac1..11c8fadcf85148b4e4ea6b97b7efb6d4ddf22d3c 100644 > --- a/drivers/bus/fsl-mc/dprc-driver.c > +++ b/drivers/bus/fsl-mc/dprc-driver.c > @@ -22,8 +22,8 @@ struct fsl_mc_child_objs { > struct fsl_mc_obj_desc *child_array; > }; > > -static bool fsl_mc_device_match(struct fsl_mc_device *mc_dev, > - struct fsl_mc_obj_desc *obj_desc) > +static bool fsl_mc_device_match(const struct fsl_mc_device *mc_dev, > + const struct fsl_mc_obj_desc *obj_desc) > { > return mc_dev->obj_desc.id == obj_desc->id && > strcmp(mc_dev->obj_desc.type, obj_desc->type) == 0; >
On Wed, 11 Dec 2024 08:08:07 +0800 Zijun Hu <zijun_hu@icloud.com> wrote: > From: Zijun Hu <quic_zijuhu@quicinc.com> > > Simplify device_find_child_by_name() implementation by both existing > API device_find_child() and device_match_name(). There is a subtle difference. In theory old code could dereference a NULL if parent->p == NULL, now it can't. Sounds at most like a harmless change but maybe you should mention it. Otherwise LGTM Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> > --- > drivers/base/core.c | 13 +------------ > 1 file changed, 1 insertion(+), 12 deletions(-) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index bc3b523a4a6366080c3c9fd190e54c7fd13c8ded..8116bc8dd6e9eba0653ca686a90c7008de9e2840 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -4110,18 +4110,7 @@ EXPORT_SYMBOL_GPL(device_find_child); > struct device *device_find_child_by_name(struct device *parent, > const char *name) > { > - struct klist_iter i; > - struct device *child; > - > - if (!parent) > - return NULL; > - > - klist_iter_init(&parent->p->klist_children, &i); > - while ((child = next_device(&i))) > - if (sysfs_streq(dev_name(child), name) && get_device(child)) > - break; > - klist_iter_exit(&i); > - return child; > + return device_find_child(parent, name, device_match_name); > } > EXPORT_SYMBOL_GPL(device_find_child_by_name); > >
On Wed, 11 Dec 2024 08:08:09 +0800 Zijun Hu <zijun_hu@icloud.com> wrote: > From: Zijun Hu <quic_zijuhu@quicinc.com> > > static of_slim_match_dev() has same function as API device_match_of_node(). > > Remove the former and use the later instead. > > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> Nice tidy up given the current code is dance up and down containing structure to exactly the same device it started with. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/slimbus/core.c | 10 +--------- > 1 file changed, 1 insertion(+), 9 deletions(-) > > diff --git a/drivers/slimbus/core.c b/drivers/slimbus/core.c > index ab927fd077cb4fe1e29c004269fe52b2896c302f..005fa2ef100f526df5603d212b6334c06a366c94 100644 > --- a/drivers/slimbus/core.c > +++ b/drivers/slimbus/core.c > @@ -385,21 +385,13 @@ struct slim_device *slim_get_device(struct slim_controller *ctrl, > } > EXPORT_SYMBOL_GPL(slim_get_device); > > -static int of_slim_match_dev(struct device *dev, const void *data) > -{ > - const struct device_node *np = data; > - struct slim_device *sbdev = to_slim_device(dev); > - > - return (sbdev->dev.of_node == np); > -} > - > static struct slim_device *of_find_slim_device(struct slim_controller *ctrl, > struct device_node *np) > { > struct slim_device *sbdev; > struct device *dev; > > - dev = device_find_child(ctrl->dev, np, of_slim_match_dev); > + dev = device_find_child(ctrl->dev, np, device_match_of_node); > if (dev) { > sbdev = to_slim_device(dev); > return sbdev; >
On Wed, 11 Dec 2024 08:08:11 +0800 Zijun Hu <zijun_hu@icloud.com> wrote: > From: Zijun Hu <quic_zijuhu@quicinc.com> > > Introduce device_match_type() for purposes below: > > - Test if a device matches with a specified device type. > - As argument of various device finding APIs to find a device with > specified type. > > device_find_child() will use it to simplify operations later. > > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> Seems useful enough to have a generic helper. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/base/core.c | 6 ++++++ > include/linux/device/bus.h | 1 + > 2 files changed, 7 insertions(+) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 289f2dafa8f3831931d0f316d66ee12c2cb8a2e1..8bdbc9e657e832a063542391426f570ccb5c18b9 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -5228,6 +5228,12 @@ int device_match_name(struct device *dev, const void *name) > } > EXPORT_SYMBOL_GPL(device_match_name); > > +int device_match_type(struct device *dev, const void *type) > +{ > + return dev->type == type; > +} > +EXPORT_SYMBOL_GPL(device_match_type); > + > int device_match_of_node(struct device *dev, const void *np) > { > return dev->of_node == np; > diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h > index cdc4757217f9bb4b36b5c3b8a48bab45737e44c5..bc3fd74bb763e6d2d862859bd2ec3f0d443f2d7a 100644 > --- a/include/linux/device/bus.h > +++ b/include/linux/device/bus.h > @@ -131,6 +131,7 @@ typedef int (*device_match_t)(struct device *dev, const void *data); > > /* Generic device matching functions that all busses can use to match with */ > int device_match_name(struct device *dev, const void *name); > +int device_match_type(struct device *dev, const void *type); > int device_match_of_node(struct device *dev, const void *np); > int device_match_fwnode(struct device *dev, const void *fwnode); > int device_match_devt(struct device *dev, const void *pdevt); >
On Wed, 11 Dec 2024 08:08:13 +0800 Zijun Hu <zijun_hu@icloud.com> wrote: > From: Zijun Hu <quic_zijuhu@quicinc.com> > > cable_match(), as matching function of device_find_child(), matches > a device with device type @typec_cable_dev_type, and its task can be > simplified by the recently introduced API device_match_type(). > > partner_match() is similar with cable_match() but with a different > device type @typec_partner_dev_type. > > Remove both functions and use the API plus respective device type instead. > > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> Looks good, but there is the same trade off here between internal detail of type identification and reducing the use of helpers where the generic ones are fine. Here is less obvious even than the CXL one as the helper macros do have other uses in these files. So, it's on for USB folk to decide on and I won't be giving a tag as a result. Jonathan > --- > drivers/usb/typec/class.c | 27 ++++++++++++--------------- > 1 file changed, 12 insertions(+), 15 deletions(-) > > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c > index 601a81aa1e1024265f2359393dee531a7779c6ea..3a4e0bd0131774afd0d746d2f0a306190219feec 100644 > --- a/drivers/usb/typec/class.c > +++ b/drivers/usb/typec/class.c > @@ -1282,11 +1282,6 @@ const struct device_type typec_cable_dev_type = { > .release = typec_cable_release, > }; > > -static int cable_match(struct device *dev, const void *data) > -{ > - return is_typec_cable(dev); > -} > - > /** > * typec_cable_get - Get a reference to the USB Type-C cable > * @port: The USB Type-C Port the cable is connected to > @@ -1298,7 +1293,8 @@ struct typec_cable *typec_cable_get(struct typec_port *port) > { > struct device *dev; > > - dev = device_find_child(&port->dev, NULL, cable_match); > + dev = device_find_child(&port->dev, &typec_cable_dev_type, > + device_match_type); > if (!dev) > return NULL; > > @@ -2028,16 +2024,12 @@ const struct device_type typec_port_dev_type = { > /* --------------------------------------- */ > /* Driver callbacks to report role updates */ > > -static int partner_match(struct device *dev, const void *data) > -{ > - return is_typec_partner(dev); > -} > - > static struct typec_partner *typec_get_partner(struct typec_port *port) > { > struct device *dev; > > - dev = device_find_child(&port->dev, NULL, partner_match); > + dev = device_find_child(&port->dev, &typec_partner_dev_type, > + device_match_type); > if (!dev) > return NULL; > > @@ -2170,7 +2162,9 @@ void typec_set_pwr_opmode(struct typec_port *port, > sysfs_notify(&port->dev.kobj, NULL, "power_operation_mode"); > kobject_uevent(&port->dev.kobj, KOBJ_CHANGE); > > - partner_dev = device_find_child(&port->dev, NULL, partner_match); > + partner_dev = device_find_child(&port->dev, > + &typec_partner_dev_type, > + device_match_type); > if (partner_dev) { > struct typec_partner *partner = to_typec_partner(partner_dev); > > @@ -2334,7 +2328,9 @@ int typec_get_negotiated_svdm_version(struct typec_port *port) > enum usb_pd_svdm_ver svdm_version; > struct device *partner_dev; > > - partner_dev = device_find_child(&port->dev, NULL, partner_match); > + partner_dev = device_find_child(&port->dev, > + &typec_partner_dev_type, > + device_match_type); > if (!partner_dev) > return -ENODEV; > > @@ -2361,7 +2357,8 @@ int typec_get_cable_svdm_version(struct typec_port *port) > enum usb_pd_svdm_ver svdm_version; > struct device *cable_dev; > > - cable_dev = device_find_child(&port->dev, NULL, cable_match); > + cable_dev = device_find_child(&port->dev, &typec_cable_dev_type, > + device_match_type); > if (!cable_dev) > return -ENODEV; > >
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. What to do? - Patches [1/11, 3/11] prepare for constifying the API. - Patch 4/11 constifies the API and adapt for its various subsystem usages. - Remaining do cleanup for several usages with benefits brought above. --- Changes in v4: - Correct title and commit messages according to review comments - Link to v3: https://lore.kernel.org/r/20241205-const_dfc_done-v3-0-1611f1486b5a@quicinc.com Changes in v3: - Solve build broken issue by squashing changes of various subsystem. - Reduce recipients to try to send out full patch serial. - Correct tiles and commit messages. - Link to v2: https://lore.kernel.org/all/20241203-const_dfc_done-v2-0-7436a98c497f@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 (11): libnvdimm: Replace namespace_match() with device_find_child_by_name() slimbus: core: Constify slim_eaddr_equal() bus: fsl-mc: Constify fsl_mc_device_match() driver core: Constify API device_find_child() then adapt for various usages driver core: Simplify API device_find_child_by_name() implementation driver core: Remove match_any() slimbus: core: Remove of_slim_match_dev() gpio: sim: Remove gpio_sim_dev_match_fwnode() driver core: Introduce an device matching API device_match_type() cxl/pmem: Replace match_nvdimm_bridge() with API device_match_type() usb: typec: class: Remove both cable_match() and partner_match() arch/sparc/kernel/vio.c | 6 +++--- drivers/base/core.c | 30 ++++++++++-------------------- drivers/block/sunvdc.c | 6 +++--- drivers/bus/fsl-mc/dprc-driver.c | 8 ++++---- 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, 90 insertions(+), 120 deletions(-) --- base-commit: cdd30ebb1b9f36159d66f088b61aee264e649d7a change-id: 20241201-const_dfc_done-aaec71e3bbea Best regards,