Re: [PATCH net-next RESEND v5] net: qcom/emac: Find sgmii_ops by device_for_each_child()

[Paolo Abeni]

Hi,

On 9/30/24 13:32, Zijun Hu wrote:
> 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 emac_sgmii_acpi_match(), 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 implementing the same
> finding sgmii_ops function by correcting the function and using it
> as parameter of device_for_each_child() instead of device_find_child().
>
> By the way, this commit does not change any existing logic.
>
> Signed-off-by: Zijun Hu <quic_zijuhu@xxxxxxxxxxx>
> ---
> This patch is separated from the following patch series:
> https://lore.kernel.org/all/20240905-const_dfc_prepare-v4-0-4180e1d5a244@xxxxxxxxxxx/
>
> This patch is to prepare for constifying the following driver 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,
> 		int (*match)(struct device *dev, const void *data));
>
> How to constify the API ?
> There are total 30 usages of the API in current kernel tree:
>
> For 2/30 usages, the API's match function (*match)() will modify
> caller's match data @*data, and this patch will clean up one of both.
>
> For remaining 28/30, the following patch series will simply change its
> relevant parameter type to const void *.
> https://lore.kernel.org/all/20240811-const_dfc_done-v1-1-9d85e3f943cb@xxxxxxxxxxx/
>
> 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));
> ---
> Changes in v5:
> - Separate me for the series
> - Correct commit message and remove the inline comment
> - Link to v4: https://lore.kernel.org/r/20240905-const_dfc_prepare-v4-2-4180e1d5a244@xxxxxxxxxxx
>
> Changes in v4:
> - Correct title and commit message
> - Link to v3: https://lore.kernel.org/r/20240824-const_dfc_prepare-v3-3-32127ea32bba@xxxxxxxxxxx
>
> Changes in v3:
> - Make qcom/emac follow cxl/region solution suggested by Greg
> - Link to v2: https://lore.kernel.org/r/20240815-const_dfc_prepare-v2-0-8316b87b8ff9@xxxxxxxxxxx
>
> Changes in v2:
> - Give up introducing the API constify_device_find_child_helper()
> - Implement a driver specific and equivalent one instead of device_find_child()
> - Correct commit message
> - Link to v1: https://lore.kernel.org/r/20240811-const_dfc_prepare-v1-0-d67cc416b3d3@xxxxxxxxxxx
> ---
>   drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 21 ++++++++++++++++-----
>   1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
> index e4bc18009d08..e8265761c416 100644
> --- a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
> +++ b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
> @@ -293,6 +293,11 @@ static struct sgmii_ops qdf2400_ops = {
>   };
>   #endif
>   
> +struct emac_match_data {
> +	struct sgmii_ops **sgmii_ops;
> +	struct device *target_device;
> +};
> +
>   static int emac_sgmii_acpi_match(struct device *dev, void *data)
>   {
>   #ifdef CONFIG_ACPI
> @@ -303,7 +308,7 @@ static int emac_sgmii_acpi_match(struct device *dev, void *data)
>   		{}
>   	};
>   	const struct acpi_device_id *id = acpi_match_device(match_table, dev);
> -	struct sgmii_ops **ops = data;
> +	struct emac_match_data *match_data = data;
>   
>   	if (id) {
>   		acpi_handle handle = ACPI_HANDLE(dev);
> @@ -324,10 +329,12 @@ static int emac_sgmii_acpi_match(struct device *dev, void *data)
>   
>   		switch (hrv) {
>   		case 1:
> -			*ops = &qdf2432_ops;
> +			*match_data->sgmii_ops = &qdf2432_ops;
> +			match_data->target_device = get_device(dev);
>   			return 1;
>   		case 2:
> -			*ops = &qdf2400_ops;
> +			*match_data->sgmii_ops = &qdf2400_ops;
> +			match_data->target_device = get_device(dev);
>   			return 1;
>   		}
>   	}
> @@ -356,10 +363,14 @@ int emac_sgmii_config(struct platform_device *pdev, struct emac_adapter *adpt)
>   	int ret;
>   
>   	if (has_acpi_companion(&pdev->dev)) {
> +		struct emac_match_data match_data = {
> +			.sgmii_ops = &phy->sgmii_ops,
> +			.target_device = NULL,
> +		};
>   		struct device *dev;
>   
> -		dev = device_find_child(&pdev->dev, &phy->sgmii_ops,
> -					emac_sgmii_acpi_match);
> +		device_for_each_child(&pdev->dev, &match_data, emac_sgmii_acpi_match);
> +		dev = match_data.target_device;
>   
>   		if (!dev) {
>   			dev_warn(&pdev->dev, "cannot find internal phy node\n");


I'm sorry for the late feedback. I agree with Greg, I think it would 
more clear removing the get_device() from the match function and add it 
here, after the 'if (!dev) {' statement.

Thanks,

Paolo