| Message ID | 20240819223834.2049862-6-swboyd@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | platform/chrome: Add DT USB/DP muxing/topology support | expand |
On Mon, Aug 19, 2024 at 03:38:19PM -0700, Stephen Boyd wrote: > Simplify driver error paths by adding devm_typec_switch_register() which > will unregister the typec switch when the parent device is unbound. > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: <linux-usb@vger.kernel.org> > Cc: Pin-yen Lin <treapking@chromium.org> As per previous patches. ... > + ptr = devres_alloc(devm_typec_switch_unregister, sizeof(*ptr), GFP_KERNEL); > + if (!ptr) > + return ERR_PTR(-ENOMEM); > + > + switch_dev = typec_switch_register(parent ,desc); > + if (!IS_ERR(switch_dev)) { > + *ptr = switch_dev; > + devres_add(parent, ptr); > + } else { > + devres_free(ptr); > + } devm_add_action_or_reset() ?
Quoting Andy Shevchenko (2024-08-20 03:16:09) > On Mon, Aug 19, 2024 at 03:38:19PM -0700, Stephen Boyd wrote: > > + ptr = devres_alloc(devm_typec_switch_unregister, sizeof(*ptr), GFP_KERNEL); > > + if (!ptr) > > + return ERR_PTR(-ENOMEM); > > + > > + switch_dev = typec_switch_register(parent ,desc); > > + if (!IS_ERR(switch_dev)) { > > + *ptr = switch_dev; > > + devres_add(parent, ptr); > > + } else { > > + devres_free(ptr); > > + } > > devm_add_action_or_reset() ? > No. We don't want to call the 'action' devm_typec_switch_unregister() when it fails because that would unregister a switch that has never been registered.
On Tue, Aug 20, 2024 at 10:01:07AM -0700, Stephen Boyd wrote: > Quoting Andy Shevchenko (2024-08-20 03:16:09) > > On Mon, Aug 19, 2024 at 03:38:19PM -0700, Stephen Boyd wrote: > > > + ptr = devres_alloc(devm_typec_switch_unregister, sizeof(*ptr), GFP_KERNEL); > > > + if (!ptr) > > > + return ERR_PTR(-ENOMEM); > > > + > > > + switch_dev = typec_switch_register(parent ,desc); (Side note: wrong location of the white space) > > > + if (!IS_ERR(switch_dev)) { (Side note: positive conditional is okay) > > > + *ptr = switch_dev; > > > + devres_add(parent, ptr); > > > + } else { > > > + devres_free(ptr); > > > + } > > > > devm_add_action_or_reset() ? > > No. We don't want to call the 'action' devm_typec_switch_unregister() > when it fails because that would unregister a switch that has never been > registered. Hmm... With devm_add_action_or_reset() we first do things and then try to add them to the managed resources. In that case it won't be like you described. What do I miss?
Quoting Andy Shevchenko (2024-08-20 10:16:40) > On Tue, Aug 20, 2024 at 10:01:07AM -0700, Stephen Boyd wrote: > > Quoting Andy Shevchenko (2024-08-20 03:16:09) > > > On Mon, Aug 19, 2024 at 03:38:19PM -0700, Stephen Boyd wrote: > > > > + ptr = devres_alloc(devm_typec_switch_unregister, sizeof(*ptr), GFP_KERNEL); > > > > + if (!ptr) > > > > + return ERR_PTR(-ENOMEM); > > > > + > > > > + switch_dev = typec_switch_register(parent ,desc); > > (Side note: wrong location of the white space) Thanks. > > > > > + if (!IS_ERR(switch_dev)) { > > (Side note: positive conditional is okay) > > > > > + *ptr = switch_dev; > > > > + devres_add(parent, ptr); > > > > + } else { > > > > + devres_free(ptr); > > > > + } > > > > > > devm_add_action_or_reset() ? > > > > No. We don't want to call the 'action' devm_typec_switch_unregister() > > when it fails because that would unregister a switch that has never been > > registered. > > Hmm... With devm_add_action_or_reset() we first do things and then try to add > them to the managed resources. In that case it won't be like you described. > > What do I miss? > I believe you've missed that this is a wrapper around struct device and the error path is different (put_device() vs. device_unregister()).
Quoting Stephen Boyd (2024-08-20 10:19:48) > Quoting Andy Shevchenko (2024-08-20 10:16:40) > > On Tue, Aug 20, 2024 at 10:01:07AM -0700, Stephen Boyd wrote: > > > Quoting Andy Shevchenko (2024-08-20 03:16:09) > > > > On Mon, Aug 19, 2024 at 03:38:19PM -0700, Stephen Boyd wrote: > > > > > + ptr = devres_alloc(devm_typec_switch_unregister, sizeof(*ptr), GFP_KERNEL); > > > > > + if (!ptr) > > > > > + return ERR_PTR(-ENOMEM); > > > > > + > > > > > + switch_dev = typec_switch_register(parent ,desc); > > > > (Side note: wrong location of the white space) > > Thanks. > > > > > > > > + if (!IS_ERR(switch_dev)) { > > > > (Side note: positive conditional is okay) > > > > > > > + *ptr = switch_dev; > > > > > + devres_add(parent, ptr); > > > > > + } else { > > > > > + devres_free(ptr); > > > > > + } > > > > > > > > devm_add_action_or_reset() ? > > > > > > No. We don't want to call the 'action' devm_typec_switch_unregister() > > > when it fails because that would unregister a switch that has never been > > > registered. > > > > Hmm... With devm_add_action_or_reset() we first do things and then try to add > > them to the managed resources. In that case it won't be like you described. > > > > What do I miss? > > > > I believe you've missed that this is a wrapper around struct device and > the error path is different (put_device() vs. device_unregister()). Or you're suggesting devm_add_action_or_reset() after registering the switch? Sorry it wasn't clear to me.
On Tue, Aug 20, 2024 at 10:23:06AM -0700, Stephen Boyd wrote: > Quoting Stephen Boyd (2024-08-20 10:19:48) > > Quoting Andy Shevchenko (2024-08-20 10:16:40) > > > On Tue, Aug 20, 2024 at 10:01:07AM -0700, Stephen Boyd wrote: > > > > Quoting Andy Shevchenko (2024-08-20 03:16:09) > > > > > On Mon, Aug 19, 2024 at 03:38:19PM -0700, Stephen Boyd wrote: ... > > > > > > + ptr = devres_alloc(devm_typec_switch_unregister, sizeof(*ptr), GFP_KERNEL); > > > > > > + if (!ptr) > > > > > > + return ERR_PTR(-ENOMEM); > > > > > > + > > > > > > + switch_dev = typec_switch_register(parent ,desc); > > > > > > (Side note: wrong location of the white space) > > > > Thanks. > > > > > > > > > > > + if (!IS_ERR(switch_dev)) { > > > > > > (Side note: positive conditional is okay) > > > > > > > > > + *ptr = switch_dev; > > > > > > + devres_add(parent, ptr); > > > > > > + } else { > > > > > > + devres_free(ptr); > > > > > > + } > > > > > > > > > > devm_add_action_or_reset() ? > > > > > > > > No. We don't want to call the 'action' devm_typec_switch_unregister() > > > > when it fails because that would unregister a switch that has never been > > > > registered. > > > > > > Hmm... With devm_add_action_or_reset() we first do things and then try to add > > > them to the managed resources. In that case it won't be like you described. > > > > > > What do I miss? > > > > I believe you've missed that this is a wrapper around struct device and > > the error path is different (put_device() vs. device_unregister()). > > Or you're suggesting devm_add_action_or_reset() after registering the > switch? Sorry it wasn't clear to me. Yes, after successful switch registration.
diff --git a/drivers/usb/typec/mux.c b/drivers/usb/typec/mux.c index 65c60eb56428..3531ab03bac4 100644 --- a/drivers/usb/typec/mux.c +++ b/drivers/usb/typec/mux.c @@ -235,6 +235,33 @@ void typec_switch_unregister(struct typec_switch_dev *sw_dev) } EXPORT_SYMBOL_GPL(typec_switch_unregister); +static void devm_typec_switch_unregister(struct device *dev, void *switch_dev) +{ + typec_switch_unregister(*(struct typec_switch_dev **)switch_dev); +} + +struct typec_switch_dev * +devm_typec_switch_register(struct device *parent, + const struct typec_switch_desc *desc) +{ + struct typec_switch_dev **ptr, *switch_dev; + + ptr = devres_alloc(devm_typec_switch_unregister, sizeof(*ptr), GFP_KERNEL); + if (!ptr) + return ERR_PTR(-ENOMEM); + + switch_dev = typec_switch_register(parent ,desc); + if (!IS_ERR(switch_dev)) { + *ptr = switch_dev; + devres_add(parent, ptr); + } else { + devres_free(ptr); + } + + return switch_dev; +} +EXPORT_SYMBOL_GPL(devm_typec_switch_register); + void typec_switch_set_drvdata(struct typec_switch_dev *sw_dev, void *data) { dev_set_drvdata(&sw_dev->dev, data); diff --git a/include/linux/usb/typec_mux.h b/include/linux/usb/typec_mux.h index c6f49756530d..fe7a05dd71c8 100644 --- a/include/linux/usb/typec_mux.h +++ b/include/linux/usb/typec_mux.h @@ -34,6 +34,9 @@ int typec_switch_set(struct typec_switch *sw, struct typec_switch_dev * typec_switch_register(struct device *parent, const struct typec_switch_desc *desc); +struct typec_switch_dev * +devm_typec_switch_register(struct device *parent, + const struct typec_switch_desc *desc); void typec_switch_unregister(struct typec_switch_dev *sw); void typec_switch_set_drvdata(struct typec_switch_dev *sw, void *data); @@ -59,6 +62,12 @@ typec_switch_register(struct device *parent, { return ERR_PTR(-EOPNOTSUPP); } +static inline struct typec_switch_dev * +devm_typec_switch_register(struct device *parent, + const struct typec_switch_desc *desc) +{ + return typec_switch_register(parent, desc); +} static inline void typec_switch_unregister(struct typec_switch_dev *sw) {} static inline void typec_switch_set_drvdata(struct typec_switch_dev *sw, void *data) {}
Simplify driver error paths by adding devm_typec_switch_register() which will unregister the typec switch when the parent device is unbound. Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: <linux-usb@vger.kernel.org> Cc: Pin-yen Lin <treapking@chromium.org> Signed-off-by: Stephen Boyd <swboyd@chromium.org> --- drivers/usb/typec/mux.c | 27 +++++++++++++++++++++++++++ include/linux/usb/typec_mux.h | 9 +++++++++ 2 files changed, 36 insertions(+)