netdev - 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: <534318f9-b040-4839-b6dc-585810ad8362@icloud.com>
Date: Tue, 24 Dec 2024 20:47:06 +0800
From: Zijun Hu <zijun_hu@...oud.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.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 2024/12/24 04:33, Jonathan Cameron wrote:
> 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.
> 

okay, will correct commit message in v5.

v5 will mention these changes will bring extra code improvement as side
effects during achieving main purpose.

>>
>> 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.
> 
explained below.

>> +
>>  
>>  	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.
> 

1) i have checked that will not cause problem, may have code improvement

2) this change (2 lines) is simpler than below(3 lines)
   -	struct range *r1, *r2 = data;
   +    struct range *r1;
   +    const struct range *r2 = data;

3) r1 and r2 are used for range_contains() whose prototype was changed
to takes 2 const pointers recently.

4) make r1 & r2 keep the same type as original.

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