| Message ID | 20241211-framer-core-fix-v1-1-0688c6905a0b@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: wan: framer: Simplify API framer_provider_simple_of_xlate() implementation | expand |
On Wed, Dec 11, 2024 at 09:28:20PM +0800, Zijun Hu wrote: > From: Zijun Hu <quic_zijuhu@quicinc.com> > > Simplify framer_provider_simple_of_xlate() implementation by API > class_find_device_by_of_node(). > > Also correct comments to mark its parameter @dev as unused instead of > @args in passing. > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> > --- > drivers/net/wan/framer/framer-core.c | 20 +++++++------------- > 1 file changed, 7 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/wan/framer/framer-core.c b/drivers/net/wan/framer/framer-core.c > index f547c22e26ac2b9986e48ed77143f12a0c8f62fb..7b369d9c41613314860753b7927b209e58f45a91 100644 > --- a/drivers/net/wan/framer/framer-core.c > +++ b/drivers/net/wan/framer/framer-core.c > @@ -732,8 +732,8 @@ EXPORT_SYMBOL_GPL(devm_framer_create); > > /** > * framer_provider_simple_of_xlate() - returns the framer instance from framer provider > - * @dev: the framer provider device > - * @args: of_phandle_args (not used here) > + * @dev: the framer provider device (not used here) > + * @args: of_phandle_args > * > * Intended to be used by framer provider for the common case where #framer-cells is > * 0. For other cases where #framer-cells is greater than '0', the framer provider > @@ -743,20 +743,14 @@ EXPORT_SYMBOL_GPL(devm_framer_create); > struct framer *framer_provider_simple_of_xlate(struct device *dev, > const struct of_phandle_args *args) > { > - struct class_dev_iter iter; > - struct framer *framer; > - > - class_dev_iter_init(&iter, &framer_class, NULL, NULL); > - while ((dev = class_dev_iter_next(&iter))) { > - framer = dev_to_framer(dev); > - if (args->np != framer->dev.of_node) > - continue; > + struct device *target_dev; > > - class_dev_iter_exit(&iter); > - return framer; > + target_dev = class_find_device_by_of_node(&framer_class, args->np); > + if (target_dev) { > + put_device(target_dev); > + return dev_to_framer(target_dev); > } > > - class_dev_iter_exit(&iter); > return ERR_PTR(-ENODEV); Hi Zijun Hu, FWIIW, I think it would be more idiomatic to have the non-error path in the main flow of execution, something like this (completely untested!): target_dev = class_find_device_by_of_node(&framer_class, args->np); if (!target_dev) return ERR_PTR(-ENODEV); put_device(target_dev); return dev_to_framer(target_dev); Also, is it safe to put_device(target_dev) before passing target_dev to dev_to_framer() ? > } > EXPORT_SYMBOL_GPL(framer_provider_simple_of_xlate); ...
On 2024/12/13 00:41, Simon Horman wrote: >> struct framer *framer_provider_simple_of_xlate(struct device *dev, >> const struct of_phandle_args *args) >> { >> - struct class_dev_iter iter; >> - struct framer *framer; >> - >> - class_dev_iter_init(&iter, &framer_class, NULL, NULL); >> - while ((dev = class_dev_iter_next(&iter))) { >> - framer = dev_to_framer(dev); >> - if (args->np != framer->dev.of_node) >> - continue; >> + struct device *target_dev; >> >> - class_dev_iter_exit(&iter); >> - return framer; >> + target_dev = class_find_device_by_of_node(&framer_class, args->np); >> + if (target_dev) { >> + put_device(target_dev); >> + return dev_to_framer(target_dev); >> } >> >> - class_dev_iter_exit(&iter); >> return ERR_PTR(-ENODEV); > Hi Zijun Hu, > > FWIIW, I think it would be more idiomatic to have the non-error path in the > main flow of execution, something like this (completely untested!): > > target_dev = class_find_device_by_of_node(&framer_class, args->np); > if (!target_dev) > return ERR_PTR(-ENODEV); > > put_device(target_dev); > return dev_to_framer(target_dev); > thank you Simon for code review. good suggestion. let me take it in v2. > Also, is it safe to put_device(target_dev) before > passing target_dev to dev_to_framer() ? Successful class_find_device_by_of_node() invocation will increase the refcount of @target_dev, so i put_device() here to keep the same logic as original. thank you.
diff --git a/drivers/net/wan/framer/framer-core.c b/drivers/net/wan/framer/framer-core.c index f547c22e26ac2b9986e48ed77143f12a0c8f62fb..7b369d9c41613314860753b7927b209e58f45a91 100644 --- a/drivers/net/wan/framer/framer-core.c +++ b/drivers/net/wan/framer/framer-core.c @@ -732,8 +732,8 @@ EXPORT_SYMBOL_GPL(devm_framer_create); /** * framer_provider_simple_of_xlate() - returns the framer instance from framer provider - * @dev: the framer provider device - * @args: of_phandle_args (not used here) + * @dev: the framer provider device (not used here) + * @args: of_phandle_args * * Intended to be used by framer provider for the common case where #framer-cells is * 0. For other cases where #framer-cells is greater than '0', the framer provider @@ -743,20 +743,14 @@ EXPORT_SYMBOL_GPL(devm_framer_create); struct framer *framer_provider_simple_of_xlate(struct device *dev, const struct of_phandle_args *args) { - struct class_dev_iter iter; - struct framer *framer; - - class_dev_iter_init(&iter, &framer_class, NULL, NULL); - while ((dev = class_dev_iter_next(&iter))) { - framer = dev_to_framer(dev); - if (args->np != framer->dev.of_node) - continue; + struct device *target_dev; - class_dev_iter_exit(&iter); - return framer; + target_dev = class_find_device_by_of_node(&framer_class, args->np); + if (target_dev) { + put_device(target_dev); + return dev_to_framer(target_dev); } - class_dev_iter_exit(&iter); return ERR_PTR(-ENODEV); } EXPORT_SYMBOL_GPL(framer_provider_simple_of_xlate);