| 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
| ||
|
Message-ID: <308126dc-1a5d-480c-b8a2-053f73865f86@redhat.com>
Date: Thu, 3 Oct 2024 15:39:16 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Zijun Hu <quic_zijuhu@...cinc.com>, Timur Tabi <timur@...nel.org>,
"David S. Miller" <davem@...emloft.net>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
Zijun Hu <zijun_hu@...oud.com>
Subject: Re: [PATCH net-next RESEND v5] net: qcom/emac: Find sgmii_ops by
device_for_each_child()
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@...cinc.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
Powered by blists - more mailing lists