From: Ira Weiny <ira.weiny@intel.com>
To: Zijun Hu <zijun_hu@icloud.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
"Rafael J. Wysocki" <rafael@kernel.org>,
Davidlohr Bueso <dave@stgolabs.net>,
Jonathan Cameron <jonathan.cameron@huawei.com>,
Dave Jiang <dave.jiang@intel.com>,
"Alison Schofield" <alison.schofield@intel.com>,
Vishal Verma <vishal.l.verma@intel.com>,
Ira Weiny <ira.weiny@intel.com>,
Dan Williams <dan.j.williams@intel.com>,
Takashi Sakamoto <o-takashi@sakamocchi.jp>,
Timur Tabi <timur@kernel.org>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>,
"Paolo Abeni" <pabeni@redhat.com>
Cc: Zijun Hu <zijun_hu@icloud.com>, <linux-kernel@vger.kernel.org>,
<linux-cxl@vger.kernel.org>,
<linux1394-devel@lists.sourceforge.net>, <netdev@vger.kernel.org>,
Zijun Hu <quic_zijuhu@quicinc.com>
Subject: Re: [PATCH v2 2/4] cxl/region: Prevent device_find_child() from modifying caller's match data
Date: Tue, 20 Aug 2024 08:59:18 -0500 [thread overview]
Message-ID: <66c4a136d9764_2ddc2429435@iweiny-mobl.notmuch> (raw)
In-Reply-To: <20240815-const_dfc_prepare-v2-2-8316b87b8ff9@quicinc.com>
Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
>
> To prepare for constifying the following old driver core API:
>
> struct device *device_find_child(struct device *dev, void *data,
> int (*match)(struct device *dev, void *data));
> to new:
> struct device *device_find_child(struct device *dev, const void *data,
> int (*match)(struct device *dev, const void *data));
>
> The new API does not allow its match function (*match)() to modify
> caller's match data @*data, but match_free_decoder() as the old API's
> match function indeed modifies relevant match data, so it is not
> suitable for the new API any more, fixed by implementing a equivalent
> cxl_device_find_child() instead of the old API usage.
Generally it seems ok but I think some name changes will make this more
clear. See below.
Also for those working on CXL I'm questioning the use of ID here and the
dependence on the id's being added to the parent in order. Is that a
guarantee?
>
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
> drivers/cxl/core/region.c | 36 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 21ad5f242875..8d8f0637f7ac 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -134,6 +134,39 @@ static const struct attribute_group *get_cxl_region_access1_group(void)
> return &cxl_region_access1_coordinate_group;
> }
>
> +struct cxl_dfc_data {
struct cxld_match_data
'cxld' == cxl decoder in our world.
> + int (*match)(struct device *dev, void *data);
> + void *data;
> + struct device *target_device;
> +};
> +
> +static int cxl_dfc_match_modify(struct device *dev, void *data)
Why not just put this logic into match_free_decoder?
> +{
> + struct cxl_dfc_data *dfc_data = data;
> + int res;
> +
> + res = dfc_data->match(dev, dfc_data->data);
> + if (res && get_device(dev)) {
> + dfc_data->target_device = dev;
> + return res;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * I have the same function as device_find_child() but allow to modify
> + * caller's match data @*data.
> + */
No need for this comment after the new API is established.
> +static struct device *cxl_device_find_child(struct device *parent, void *data,
> + int (*match)(struct device *dev, void *data))
> +{
> + struct cxl_dfc_data dfc_data = {match, data, NULL};
> +
> + device_for_each_child(parent, &dfc_data, cxl_dfc_match_modify);
> + return dfc_data.target_device;
> +}
> +
> static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> @@ -849,7 +882,8 @@ cxl_region_find_decoder(struct cxl_port *port,
> dev = device_find_child(&port->dev, &cxlr->params,
> match_auto_decoder);
> else
> - dev = device_find_child(&port->dev, &id, match_free_decoder);
> + dev = cxl_device_find_child(&port->dev, &id,
> + match_free_decoder);
This is too literal. How about the following (passes basic cxl-tests).
Ira
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 21ad5f242875..c1e46254efb8 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -794,10 +794,15 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos)
return rc;
}
+struct cxld_match_data {
+ int id;
+ struct device *target_device;
+};
+
static int match_free_decoder(struct device *dev, void *data)
{
+ struct cxld_match_data *match_data = data;
struct cxl_decoder *cxld;
- int *id = data;
if (!is_switch_decoder(dev))
return 0;
@@ -805,17 +810,30 @@ static int match_free_decoder(struct device *dev, void *data)
cxld = to_cxl_decoder(dev);
/* enforce ordered allocation */
- if (cxld->id != *id)
+ if (cxld->id != match_data->id)
return 0;
- if (!cxld->region)
+ if (!cxld->region && get_device(dev)) {
+ match_data->target_device = dev;
return 1;
+ }
- (*id)++;
+ match_data->id++;
return 0;
}
+static struct device *find_free_decoder(struct device *parent)
+{
+ struct cxld_match_data match_data = {
+ .id = 0,
+ .target_device = NULL,
+ };
+
+ device_for_each_child(parent, &match_data, match_free_decoder);
+ return match_data.target_device;
+}
+
static int match_auto_decoder(struct device *dev, void *data)
{
struct cxl_region_params *p = data;
@@ -840,7 +858,6 @@ cxl_region_find_decoder(struct cxl_port *port,
struct cxl_region *cxlr)
{
struct device *dev;
- int id = 0;
if (port == cxled_to_port(cxled))
return &cxled->cxld;
@@ -849,7 +866,8 @@ cxl_region_find_decoder(struct cxl_port *port,
dev = device_find_child(&port->dev, &cxlr->params,
match_auto_decoder);
else
- dev = device_find_child(&port->dev, &id, match_free_decoder);
+ dev = find_free_decoder(&port->dev);
+
if (!dev)
return NULL;
/*
next prev parent reply other threads:[~2024-08-20 13:59 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-08-15 14:58 [PATCH v2 0/4] driver core: Prevent device_find_child() from modifying caller's match data Zijun Hu
2024-08-15 14:58 ` [PATCH v2 1/4] driver core: Make parameter check consistent for API cluster device_(for_each|find)_child() Zijun Hu
2024-08-20 12:53 ` Ira Weiny
2024-08-20 13:40 ` Zijun Hu
2024-08-20 14:14 ` Ira Weiny
2024-08-21 14:44 ` Zijun Hu
2024-08-23 17:19 ` Ira Weiny
2024-08-23 21:45 ` Zijun Hu
2024-08-24 3:21 ` Greg Kroah-Hartman
2024-08-24 3:23 ` Greg Kroah-Hartman
2024-08-15 14:58 ` [PATCH v2 2/4] cxl/region: Prevent device_find_child() from modifying caller's match data Zijun Hu
2024-08-20 13:59 ` Ira Weiny [this message]
2024-08-21 12:46 ` Zijun Hu
2024-08-23 18:10 ` Ira Weiny
2024-08-23 22:18 ` Zijun Hu
2024-08-24 3:29 ` Greg Kroah-Hartman
2024-08-15 14:58 ` [PATCH v2 3/4] firewire: core: " Zijun Hu
2024-08-17 9:57 ` Takashi Sakamoto
2024-08-18 14:24 ` Zijun Hu
2024-08-19 8:58 ` Takashi Sakamoto
2024-08-19 11:41 ` Zijun Hu
2024-08-15 14:58 ` [PATCH v2 4/4] net: qcom/emac: " Zijun Hu
2024-08-24 3:29 ` Greg Kroah-Hartman
2024-08-24 7:11 ` Zijun Hu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=66c4a136d9764_2ddc2429435@iweiny-mobl.notmuch \
--to=ira.weiny@intel.com \
--cc=alison.schofield@intel.com \
--cc=dan.j.williams@intel.com \
--cc=dave.jiang@intel.com \
--cc=dave@stgolabs.net \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=gregkh@linuxfoundation.org \
--cc=jonathan.cameron@huawei.com \
--cc=kuba@kernel.org \
--cc=linux-cxl@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux1394-devel@lists.sourceforge.net \
--cc=netdev@vger.kernel.org \
--cc=o-takashi@sakamocchi.jp \
--cc=pabeni@redhat.com \
--cc=quic_zijuhu@quicinc.com \
--cc=rafael@kernel.org \
--cc=timur@kernel.org \
--cc=vishal.l.verma@intel.com \
--cc=zijun_hu@icloud.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox