linux-kernel - Re: [PATCH v4 04/11] driver core: Constify API device_find

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: <20241223203309.00004d51@huawei.com>
Date: Mon, 23 Dec 2024 20:33:09 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Zijun Hu <zijun_hu@...oud.com>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Linus Walleij
	<linus.walleij@...aro.org>, Bartosz Golaszewski <brgl@...ev.pl>, Uwe
 Kleine-König <ukleinek@...nel.org>, James Bottomley
	<James.Bottomley@...senPartnership.com>, Thomas Weißschuh <thomas@...ch.de>, <linux-kernel@...r.kernel.org>,
	<nvdimm@...ts.linux.dev>, <linux-sound@...r.kernel.org>,
	<sparclinux@...r.kernel.org>, <linux-block@...r.kernel.org>,
	<linux-cxl@...r.kernel.org>, <linux1394-devel@...ts.sourceforge.net>,
	<arm-scmi@...r.kernel.org>, <linux-efi@...r.kernel.org>,
	<linux-gpio@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
	<linux-mediatek@...ts.infradead.org>, <linux-hwmon@...r.kernel.org>,
	<linux-media@...r.kernel.org>, <linux-pwm@...r.kernel.org>,
	<linux-remoteproc@...r.kernel.org>, <linux-scsi@...r.kernel.org>,
	<linux-usb@...r.kernel.org>, <linux-serial@...r.kernel.org>,
	<netdev@...r.kernel.org>, Zijun Hu <quic_zijuhu@...cinc.com>, "Alison
 Schofield" <alison.schofield@...el.com>, Takashi Sakamoto
	<o-takashi@...amocchi.jp>
Subject: Re: [PATCH v4 04/11] driver core: Constify API device_find_child()
 then adapt for various usages

On Wed, 11 Dec 2024 08:08:06 +0800
Zijun Hu <zijun_hu@...oud.com> wrote:

> From: Zijun Hu <quic_zijuhu@...cinc.com>
> 
> 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);
> with the following reasons:
> 
> - 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 match functions can be directly taken
>   as the API's argument, they were exported by driver core.
> 
> Constify the API and adapt for various existing usages by simply making
> various match functions take 'const void *' as type of match data @data.

There are a couple of places I noticed where you changed types
that aren't related to the specific change this is making.
They are almost certainly fine but I'd either like a specific note
on that in this patch description or just change the elements
that are directly affected by this change.

> 
> Reviewed-by: Alison Schofield <alison.schofield@...el.com>
> Reviewed-by: Takashi Sakamoto <o-takashi@...amocchi.jp>
> Signed-off-by: Zijun Hu <quic_zijuhu@...cinc.com>


> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index d778996507984a759bbe84e7acac3774e0c7af98..bfecd71040c2f4373645380b4c31327d8b42d095 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c

> @@ -1722,10 +1722,12 @@ static struct cxl_port *next_port(struct cxl_port *port)
>  	return port->parent_dport->port;
>  }
>  
> -static int match_switch_decoder_by_range(struct device *dev, void *data)
> +static int match_switch_decoder_by_range(struct device *dev,
> +					 const void *data)
>  {
>  	struct cxl_switch_decoder *cxlsd;
> -	struct range *r1, *r2 = data;
> +	const struct range *r1, *r2 = data;

As below. I'd not touch type of things you don't need to touch.

> +
>  
>  	if (!is_switch_decoder(dev))
>  		return 0;
> @@ -3176,9 +3178,10 @@ static int devm_cxl_add_dax_region(struct cxl_region *cxlr)
>  	return rc;
>  }
>  
> -static int match_root_decoder_by_range(struct device *dev, void *data)
> +static int match_root_decoder_by_range(struct device *dev,
> +				       const void *data)
>  {
> -	struct range *r1, *r2 = data;
> +	const struct range *r1, *r2 = data;

From the point of view of keeping the patch to what it 'needs'
to touch, should leave type of r1 alone.
I've not looked at whether this causes any problems, just whether
it is relevant to this change.

>  	struct cxl_root_decoder *cxlrd;
>  
>  	if (!is_root_decoder(dev))
> @@ -3189,11 +3192,11 @@ static int match_root_decoder_by_range(struct device *dev, void *data)
>  	return range_contains(r1, r2);
>  }
>  
> -static int match_region_by_range(struct device *dev, void *data)
> +static int match_region_by_range(struct device *dev, const void *data)
>  {
>  	struct cxl_region_params *p;
>  	struct cxl_region *cxlr;
> -	struct range *r = data;
> +	const struct range *r = data;
>  	int rc = 0;
>