On Sat, 17 Feb 2024 at 17:03, Johan Hovold <johan+linaro@xxxxxxxxxx> wrote:
>
> Combining allocation and registration is an anti-pattern that should be
> avoided. Add two new functions for allocating and registering an dp-hpd
> bridge with a proper 'devm' prefix so that it is clear that these are
> device managed interfaces.
>
> devm_drm_dp_hpd_bridge_alloc()
> devm_drm_dp_hpd_bridge_add()
>
> The new interface will be used to fix a use-after-free bug in the
> Qualcomm PMIC GLINK driver and may prevent similar issues from being
> introduced elsewhere.
>
> The existing drm_dp_hpd_bridge_register() is reimplemented using the
> above and left in place for now.
>
> Signed-off-by: Johan Hovold <johan+linaro@xxxxxxxxxx>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
Minor nit below.
> ---
> drivers/gpu/drm/bridge/aux-hpd-bridge.c | 67 +++++++++++++++++++------
> include/drm/bridge/aux-bridge.h | 15 ++++++
> 2 files changed, 67 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/aux-hpd-bridge.c b/drivers/gpu/drm/bridge/aux-hpd-bridge.c
> index 9e71daf95bde..6886db2d9e00 100644
> --- a/drivers/gpu/drm/bridge/aux-hpd-bridge.c
> +++ b/drivers/gpu/drm/bridge/aux-hpd-bridge.c
> @@ -30,16 +30,13 @@ static void drm_aux_hpd_bridge_release(struct device *dev)
> kfree(adev);
> }
>
> -static void drm_aux_hpd_bridge_unregister_adev(void *_adev)
> +static void drm_aux_hpd_bridge_free_adev(void *_adev)
> {
> - struct auxiliary_device *adev = _adev;
> -
> - auxiliary_device_delete(adev);
> - auxiliary_device_uninit(adev);
> + auxiliary_device_uninit(_adev);
> }
>
> /**
> - * drm_dp_hpd_bridge_register - Create a simple HPD DisplayPort bridge
> + * devm_drm_dp_hpd_bridge_alloc - allocate a HPD DisplayPort bridge
> * @parent: device instance providing this bridge
> * @np: device node pointer corresponding to this bridge instance
> *
> @@ -47,11 +44,9 @@ static void drm_aux_hpd_bridge_unregister_adev(void *_adev)
> * DRM_MODE_CONNECTOR_DisplayPort, which terminates the bridge chain and is
> * able to send the HPD events.
> *
> - * Return: device instance that will handle created bridge or an error code
> - * encoded into the pointer.
> + * Return: bridge auxiliary device pointer or an error pointer
> */
> -struct device *drm_dp_hpd_bridge_register(struct device *parent,
> - struct device_node *np)
> +struct auxiliary_device *devm_drm_dp_hpd_bridge_alloc(struct device *parent, struct device_node *np)
> {
> struct auxiliary_device *adev;
> int ret;
> @@ -82,13 +77,55 @@ struct device *drm_dp_hpd_bridge_register(struct device *parent,
> return ERR_PTR(ret);
> }
>
> - ret = auxiliary_device_add(adev);
> - if (ret) {
> - auxiliary_device_uninit(adev);
> + ret = devm_add_action_or_reset(parent, drm_aux_hpd_bridge_free_adev, adev);
> + if (ret)
> return ERR_PTR(ret);
> - }
>
> - ret = devm_add_action_or_reset(parent, drm_aux_hpd_bridge_unregister_adev, adev);
> + return adev;
> +}
> +EXPORT_SYMBOL_GPL(devm_drm_dp_hpd_bridge_alloc);
> +
> +static void drm_aux_hpd_bridge_del_adev(void *_adev)
> +{
> + auxiliary_device_delete(_adev);
> +}
> +
> +/**
> + * devm_drm_dp_hpd_bridge_add - register a HDP DisplayPort bridge
> + * @dev: struct device to tie registration lifetime to
> + * @adev: bridge auxiliary device to be registered
> + *
> + * Returns: zero on success or a negative errno
> + */
> +int devm_drm_dp_hpd_bridge_add(struct device *dev, struct auxiliary_device *adev)
> +{
> + int ret;
> +
> + ret = auxiliary_device_add(adev);
> + if (ret)
> + return ret;
> +
> + return devm_add_action_or_reset(dev, drm_aux_hpd_bridge_del_adev, adev);
> +}
> +EXPORT_SYMBOL_GPL(devm_drm_dp_hpd_bridge_add);
> +
> +/**
> + * drm_dp_hpd_bridge_register - allocate and register a HDP DisplayPort bridge
> + * @parent: device instance providing this bridge
> + * @np: device node pointer corresponding to this bridge instance
> + *
> + * Return: device instance that will handle created bridge or an error pointer
> + */
> +struct device *drm_dp_hpd_bridge_register(struct device *parent, struct device_node *np)
> +{
> + struct auxiliary_device *adev;
> + int ret;
> +
> + adev = devm_drm_dp_hpd_bridge_alloc(parent, np);
> + if (IS_ERR(adev))
> + return ERR_CAST(adev);
> +
> + ret = devm_drm_dp_hpd_bridge_add(parent, adev);
> if (ret)
> return ERR_PTR(ret);
>
> diff --git a/include/drm/bridge/aux-bridge.h b/include/drm/bridge/aux-bridge.h
> index c4c423e97f06..4453906105ca 100644
> --- a/include/drm/bridge/aux-bridge.h
> +++ b/include/drm/bridge/aux-bridge.h
> @@ -9,6 +9,8 @@
>
> #include <drm/drm_connector.h>
>
> +struct auxiliary_device;
> +
> #if IS_ENABLED(CONFIG_DRM_AUX_BRIDGE)
> int drm_aux_bridge_register(struct device *parent);
> #else
> @@ -19,10 +21,23 @@ static inline int drm_aux_bridge_register(struct device *parent)
> #endif
>
> #if IS_ENABLED(CONFIG_DRM_AUX_HPD_BRIDGE)
> +struct auxiliary_device *devm_drm_dp_hpd_bridge_alloc(struct device *parent, struct device_node *np);
> +int devm_drm_dp_hpd_bridge_add(struct device *dev, struct auxiliary_device *adev);
I had a pretty close idea during prototyping, but I ended up doing it
as a single function for the following reasons:
First, this exports the implementation detail that internally the code
uses an aux device.
Also, by exporting the aux device the code becomes less type-safe. By
mistake one can call devm_drm_dp_hpd_bridge_add() on any aux device,
which is not necessarily the HPD bridge.
I'd prefer to see an opaque device-specific structure instead. WDYT?
> struct device *drm_dp_hpd_bridge_register(struct device *parent,
> struct device_node *np);
> void drm_aux_hpd_bridge_notify(struct device *dev, enum drm_connector_status status);
> #else
> +static inline struct auxiliary_device *devm_drm_dp_hpd_bridge_alloc(struct device *parent,
> + struct device_node *np)
> +{
> + return NULL;
> +}
> +
> +static inline int devm_drm_dp_hpd_bridge_add(struct auxiliary_device *adev)
> +{
> + return 0;
> +}
> +
> static inline struct device *drm_dp_hpd_bridge_register(struct device *parent,
> struct device_node *np)
> {
> --
> 2.43.0
>
--
With best wishes
Dmitry
