| Message ID | 20240905-const_dfc_prepare-v4-2-4180e1d5a244@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | driver core: Prevent device_find_child() from modifying caller's match data | expand |
On Thu, Sep 05, 2024 at 08:36:10AM +0800, Zijun Hu wrote: > From: Zijun Hu <quic_zijuhu@quicinc.com> > > 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 using device_for_each_child() > to implement relevant finding sgmii_ops function. > > By the way, this commit does not change any existing logic. > > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> > --- > drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 22 +++++++++++++++++----- > 1 file changed, 17 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..29392c63d115 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); Where is put_device() now called? > return 1; > } > } > @@ -356,10 +363,15 @@ 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); > + /* Need to put_device(@dev) after use */ > + dev = match_data.target_device; Why this new comment? That's always required and happens down below in the function, right? Otherwise, what changed? thanks, greg k-h
On Thu, Sep 05, 2024 at 07:29:10AM +0200, Greg Kroah-Hartman wrote: > On Thu, Sep 05, 2024 at 08:36:10AM +0800, Zijun Hu wrote: > > From: Zijun Hu <quic_zijuhu@quicinc.com> > > > > 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 using device_for_each_child() > > to implement relevant finding sgmii_ops function. > > > > By the way, this commit does not change any existing logic. > > > > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> > > --- > > drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 22 +++++++++++++++++----- > > 1 file changed, 17 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..29392c63d115 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); > > Where is put_device() now called? Nevermind, I see it now. That being said, this feels wrong still, why not just do this "set up the ops" logic _after_ you find the device and not here in the match function? thanks, greg k-h
On 9/5/2024 1:29 PM, Greg Kroah-Hartman wrote: > On Thu, Sep 05, 2024 at 08:36:10AM +0800, Zijun Hu wrote: >> From: Zijun Hu <quic_zijuhu@quicinc.com> >> >> 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 using device_for_each_child() >> to implement relevant finding sgmii_ops function. >> >> By the way, this commit does not change any existing logic. >> >> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> >> --- >> drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 22 +++++++++++++++++----- >> 1 file changed, 17 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..29392c63d115 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); > > Where is put_device() now called? > >> return 1; >> } >> } >> @@ -356,10 +363,15 @@ 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); >> + /* Need to put_device(@dev) after use */ >> + dev = match_data.target_device; > > > Why this new comment? That's always required and happens down below in > the function, right? Otherwise, what changed? > device_find_child() will get_device() by itself and that is obvious. device_for_each_child() will not get_device() by itself, we get_device() in its function parameter to make it equivalent with device_find_child() this get_device() is not obvious, so add the inline comments to prompt user put_device after use. yes, and the relevant put_device() don't happen immediately. > thanks, > > greg k-h
On 9/5/2024 1:33 PM, Greg Kroah-Hartman wrote: > On Thu, Sep 05, 2024 at 07:29:10AM +0200, Greg Kroah-Hartman wrote: >> On Thu, Sep 05, 2024 at 08:36:10AM +0800, Zijun Hu wrote: >>> From: Zijun Hu <quic_zijuhu@quicinc.com> >>> >>> 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 using device_for_each_child() >>> to implement relevant finding sgmii_ops function. >>> >>> By the way, this commit does not change any existing logic. >>> >>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> >>> --- >>> drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 22 +++++++++++++++++----- >>> 1 file changed, 17 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..29392c63d115 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); >> >> Where is put_device() now called? > > Nevermind, I see it now. > > That being said, this feels wrong still, why not just do this "set up > the ops" logic _after_ you find the device and not here in the match > function? > that will become more complex since 1) it need to change match_data->sgmii_ops type from struct sgmii_ops ** to struct sgmii_ops * which is not same as type of original @data 2) it also needs to implement a extra finding function find_xyz() similar as cxl/region find_free_decoder() 3) it need to extra condition assignment after find aim device. actually, this scenario is a bit different with cxl/region as shown below: for cxl/region, we only need to get aim device for this scenario, we need to get both aim device and relevant sgmii_ops > thanks, > > greg k-h
On 2024/9/5 13:33, Greg Kroah-Hartman wrote: > On Thu, Sep 05, 2024 at 07:29:10AM +0200, Greg Kroah-Hartman wrote: >> On Thu, Sep 05, 2024 at 08:36:10AM +0800, Zijun Hu wrote: >>> From: Zijun Hu <quic_zijuhu@quicinc.com> >>> >>> 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 using device_for_each_child() >>> to implement relevant finding sgmii_ops function. >>> >>> By the way, this commit does not change any existing logic. >>> >>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> >>> --- >>> drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 22 +++++++++++++++++----- >>> 1 file changed, 17 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..29392c63d115 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); >> >> Where is put_device() now called? > > Nevermind, I see it now. > > That being said, this feels wrong still, why not just do this "set up > the ops" logic _after_ you find the device and not here in the match > function? > sorry, let me complement a little reply to last one. This change will use emac_sgmii_acpi_match() as device_for_each_child() function parameter, so may not regard emac_sgmii_acpi_match() as match() function anymore. > thanks, > > greg k-h
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c index e4bc18009d08..29392c63d115 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,15 @@ 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); + /* Need to put_device(@dev) after use */ + dev = match_data.target_device; if (!dev) { dev_warn(&pdev->dev, "cannot find internal phy node\n");