Re: [PATCH v2 3/4] firewire: core: Prevent device_find_child() from modifying caller's match data

[Takashi Sakamoto]

Hi,

On Thu, Aug 15, 2024 at 10:58:04PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@xxxxxxxxxxx>
> 
> 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 lookup_existing_device() 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
> fw_device_find_child() instead of the old API usage.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@xxxxxxxxxxx>
> ---
>  drivers/firewire/core-device.c | 37 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 35 insertions(+), 2 deletions(-)

Thanks for the patch.

> Why to constify the API ?
> 
> (1) It normally does not make sense, also does not need to, for
> such device finding operation to modify caller's match data which
> is mainly used for comparison.
> 
> (2) It will make the API's match function and match data parameter
> have the same type as all other APIs (bus|class|driver)_find_device().
> 
> (3) It will give driver author hints about choice between this API and
> the following one:
> int device_for_each_child(struct device *dev, void *data,
>                 int (*fn)(struct device *dev, void *data));

I have found another issue in respect to this subsystem.

The whole procedure in 'lookup_existing_device()' in the call of
'device_find_child()' is a bit superfluous, since it includes not only
finding but also updating. The helper function passed to
'device_find_child()' should do quick finding only.

I think we can change the relevant codes like the following patch. It
would solve your concern, too. If you prefer the change, I'm going to
evaluate it.

======== 8< --------