diff mbox series

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

Message ID 20240930-qcom_emac_fix-v5-1-e59c0ddbc8b4@quicinc.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net-next,RESEND,v5] net: qcom/emac: Find sgmii_ops by device_for_each_child() | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 9 this patch: 9
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/build_clang success Errors and warnings before: 9 this patch: 9
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 12 this patch: 12
netdev/checkpatch warning WARNING: line length of 86 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

quic_zijuhu Sept. 30, 2024, 11:32 a.m. UTC 
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@quicinc.com>
---
This patch is separated from the following patch series:
https://lore.kernel.org/all/20240905-const_dfc_prepare-v4-0-4180e1d5a244@quicinc.com/

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@quicinc.com/

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@quicinc.com

Changes in v4:
- Correct title and commit message
- Link to v3: https://lore.kernel.org/r/20240824-const_dfc_prepare-v3-3-32127ea32bba@quicinc.com

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@quicinc.com

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@quicinc.com
---
 drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)


---
base-commit: c824deb1a89755f70156b5cdaf569fca80698719
change-id: 20240923-qcom_emac_fix-0a03c6b9ea19

Best regards,

Comments

Paolo Abeni Oct. 3, 2024, 1:39 p.m. UTC | #1 
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@quicinc.com>
> ---
> This patch is separated from the following patch series:
> https://lore.kernel.org/all/20240905-const_dfc_prepare-v4-0-4180e1d5a244@quicinc.com/
> 
> 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@quicinc.com/
> 
> 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@quicinc.com
> 
> Changes in v4:
> - Correct title and commit message
> - Link to v3: https://lore.kernel.org/r/20240824-const_dfc_prepare-v3-3-32127ea32bba@quicinc.com
> 
> 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@quicinc.com
> 
> 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@quicinc.com
> ---
>   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
quic_zijuhu Oct. 3, 2024, 5:09 p.m. UTC | #2 
On 10/3/2024 9:39 PM, Paolo Abeni wrote:
>> @@ -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.
> 

Thank you for helping me understand those good suggestions.
will correct it within next revision.

> Thanks,
> 
> Paolo
diff mbox series

Patch

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