netdev - Re: [PATCH v3 2/3] cxl/region: Find free cxl decoder by device_for_each

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240827123006.00004527@Huawei.com>
Date: Tue, 27 Aug 2024 12:30:06 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Zijun Hu <zijun_hu@...oud.com>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, "Rafael J. Wysocki"
	<rafael@...nel.org>, Davidlohr Bueso <dave@...olabs.net>, Dave Jiang
	<dave.jiang@...el.com>, Alison Schofield <alison.schofield@...el.com>, Vishal
 Verma <vishal.l.verma@...el.com>, Ira Weiny <ira.weiny@...el.com>, Dan
 Williams <dan.j.williams@...el.com>, Takashi Sakamoto
	<o-takashi@...amocchi.jp>, Timur Tabi <timur@...nel.org>, "David S. Miller"
	<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	<kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	<linux-kernel@...r.kernel.org>, <linux-cxl@...r.kernel.org>,
	<netdev@...r.kernel.org>, Zijun Hu <quic_zijuhu@...cinc.com>
Subject: Re: [PATCH v3 2/3] cxl/region: Find free cxl decoder by
 device_for_each_child()

On Sat, 24 Aug 2024 17:07:44 +0800
Zijun Hu <zijun_hu@...oud.com> wrote:

> From: Zijun Hu <quic_zijuhu@...cinc.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, solved by using device_for_each_child() to
> implement relevant finding free cxl decoder function.
> 
> Suggested-by: Ira Weiny <ira.weiny@...el.com>
> Signed-off-by: Zijun Hu <quic_zijuhu@...cinc.com>
This seems to functionally do the same as before.

I'm not sure I like the original code though so a comment inline.

> ---
>  drivers/cxl/core/region.c | 30 ++++++++++++++++++++++++------
>  1 file changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 21ad5f242875..c2068e90bf2f 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,31 @@ 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)

Why do we carry on in this case?
Conditions are:
1. Start match_data->id == 0
2. First pass cxld->id == 0 (all good) or
   cxld->id == 1 say (and we skip until we match
   on cxld->id == 0 (perhaps on the second child if they are
   ordered (1, 0, 2) etc. 

If we skipped and then matched on second child but it was
already in use (so region set), we will increment match_data->id to 1
but never find that as it was the one we skipped.

So this can only work if the children are ordered.
So if that's the case and the line above is just a sanity check
on that, it should be noisier (so an error print) and might
as well fail as if it doesn't match all bets are off.

Jonathan
 
>  		return 0;
>  
> -	if (!cxld->region)
> +	if (!cxld->region) {
> +		match_data->target_device = get_device(dev);
>  		return 1;
> +	}
>  
> -	(*id)++;
> +	match_data->id++;
>  
>  	return 0;
>  }
>  
> +/* NOTE: need to drop the reference with put_device() after use. */
> +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 +859,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 +867,7 @@ 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;
>  	/*
>