Re: [PATCH v10 3/9] mux: Add helper functions for getting optional and selected mux-state

[Josua Mayer <josua@solid-run.com>]

Am 25.02.26 um 12:34 schrieb Josua Mayer:
> In-tree phy-can-transceiver and phy_rcar_gen3_usb2 have already
> implemented local versions of devm_mux_state_get_optional.
>
> The omap-i2c driver gets and selects an optional mux in its probe
> function without using any helper.
>
> Add new helper functions covering both aforementioned use-cases:
>
> - mux_control_get_optional:
>   Get a mux-control if specified in dt, return NULL otherwise.
> - devm_mux_state_get_optional:
>   Get a mux-state if specified in dt, return NULL otherwise.
> - devm_mux_state_get_selected:
>   Get and select a mux-state specified in dt, return error otherwise.
> - devm_mux_state_get_optional_selected:
>   Get and select a mux-state if specified in dt, return error or NULL.
>
> Existing mux_get helper function is changed to take an extra argument
> indicating whether the mux is optional.
> In this case no error is printed, and NULL returned in case of ENOENT.
>
> Calling code is adapted to handle NULL return case, and to pass optional
> argument as required.
>
> To support automatic deselect for _selected helper, a new structure is
> created storing an exit pointer similar to clock core which is called on
> release.
>
> To facilitate code sharing between optional/mandatory/selected helpers,
> a new internal helper function is added to handle quiet (optional) and
> verbose (mandatory) errors, as well as storing the correct callback for
> devm release: __devm_mux_state_get
>
> Due to this structure devm_mux_state_get_*_selected can no longer print
> a useful error message when select fails. Instead callers should print
> errors where needed.
>
> Commit e153fdea9db04 ("phy: can-transceiver: Re-instate "mux-states"
> property presence check") noted that "mux_get() always prints an error
> message in case of an error, including when the property is not present,
> confusing the user."
>
> The first error message covers the case that a mux name is not matched
> in dt. The second error message is based on of_parse_phandle_with_args
> return value.
>
> In optional case no error is printed and NULL is returned.
> This ensures that the new helper functions will not confuse the user
> either.
>
> With the addition of optional helper functions it became clear that
> drivers should compile and link even if CONFIG_MULTIPLEXER was not enabled.
> Add stubs for all symbols exported by mux core.
>
> Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Signed-off-by: Josua Mayer <josua@solid-run.com>
> ---
>  drivers/mux/core.c           | 206 ++++++++++++++++++++++++++++++++++++-------
>  include/linux/mux/consumer.h | 108 ++++++++++++++++++++++-
>  2 files changed, 279 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
> index f09ee8782e3d..6033da0a9e17 100644
> --- a/drivers/mux/core.c
> +++ b/drivers/mux/core.c
> @@ -46,6 +46,16 @@ static const struct class mux_class = {
>  	.name = "mux",
>  };
>  
> +/**
> + * struct devm_mux_state_state -	Tracks managed resources for mux-state objects.
> + * @mstate:				Pointer to a mux state.
> + * @exit:				An optional callback to execute before free.
> + */
> +struct devm_mux_state_state {
> +	struct mux_state *mstate;
> +	int (*exit)(struct mux_state *mstate);
> +};
> +
>  static DEFINE_IDA(mux_ida);
>  
>  static int __init mux_init(void)
> @@ -516,17 +526,19 @@ static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
>  	return dev ? to_mux_chip(dev) : NULL;
>  }
>  
> -/*
> +/**
>   * mux_get() - Get the mux-control for a device.
>   * @dev: The device that needs a mux-control.
>   * @mux_name: The name identifying the mux-control.
>   * @state: Pointer to where the requested state is returned, or NULL when
>   *         the required multiplexer states are handled by other means.
> + * @optional: Whether to return NULL and silence errors when mux doesn't exist.
>   *
> - * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
> + * Return: Pointer to the mux-control on success, an ERR_PTR with a negative errno on error,
> + * or NULL if optional is true and mux doesn't exist.
>   */
>  static struct mux_control *mux_get(struct device *dev, const char *mux_name,
> -				   unsigned int *state)
> +				   unsigned int *state, bool optional)
>  {
>  	struct device_node *np = dev->of_node;
>  	struct of_phandle_args args;
> @@ -542,7 +554,9 @@ static struct mux_control *mux_get(struct device *dev, const char *mux_name,
>  		else
>  			index = of_property_match_string(np, "mux-control-names",
>  							 mux_name);
> -		if (index < 0) {
> +		if (index < 0 && optional) {
> +			return NULL;
> +		} else if (index < 0) {
>  			dev_err(dev, "mux controller '%s' not found\n",
>  				mux_name);
>  			return ERR_PTR(index);
> @@ -558,8 +572,12 @@ static struct mux_control *mux_get(struct device *dev, const char *mux_name,
>  						 "mux-controls", "#mux-control-cells",
>  						 index, &args);
>  	if (ret) {
> +		if (optional && ret == -ENOENT)
> +			return NULL;
> +
>  		dev_err(dev, "%pOF: failed to get mux-%s %s(%i)\n",
> -			np, state ? "state" : "control", mux_name ?: "", index);
> +			np, state ? "state" : "control",
> +			mux_name ?: "", index);
>  		return ERR_PTR(ret);
>  	}
>  
> @@ -617,10 +635,29 @@ static struct mux_control *mux_get(struct device *dev, const char *mux_name,
>   */
>  struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>  {
> -	return mux_get(dev, mux_name, NULL);
> +	struct mux_control *mux = mux_get(dev, mux_name, NULL, false);
> +
> +	if (!mux)
> +		return ERR_PTR(-ENOENT);
> +
> +	return mux;
>  }
>  EXPORT_SYMBOL_GPL(mux_control_get);
>  
> +/**
> + * mux_control_get_optional() - Get the optional mux-control for a device.
> + * @dev: The device that needs a mux-control.
> + * @mux_name: The name identifying the mux-control.
> + *
> + * Return: Pointer to the mux-state on success, an ERR_PTR with a negative errno on error,
> + * or NULL if mux doesn't exist.
> + */
> +struct mux_control *mux_control_get_optional(struct device *dev, const char *mux_name)
> +{
> +	return mux_get(dev, mux_name, NULL, true);
> +}
> +EXPORT_SYMBOL_GPL(mux_control_get_optional);
> +
>  /**
>   * mux_control_put() - Put away the mux-control for good.
>   * @mux: The mux-control to put away.
> @@ -657,10 +694,13 @@ struct mux_control *devm_mux_control_get(struct device *dev,
>  	if (!ptr)
>  		return ERR_PTR(-ENOMEM);
>  
> -	mux = mux_control_get(dev, mux_name);
> +	mux = mux_get(dev, mux_name, NULL, false);
>  	if (IS_ERR(mux)) {
>  		devres_free(ptr);
>  		return mux;
> +	} else if (!mux) {
> +		devres_free(ptr);
> +		return ERR_PTR(-ENOENT);
>  	}
>  
>  	*ptr = mux;
> @@ -670,14 +710,16 @@ struct mux_control *devm_mux_control_get(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(devm_mux_control_get);
>  
> -/*
> +/**
>   * mux_state_get() - Get the mux-state for a device.
>   * @dev: The device that needs a mux-state.
>   * @mux_name: The name identifying the mux-state.
> + * @optional: Whether to return NULL and silence errors when mux doesn't exist.
>   *
> - * Return: A pointer to the mux-state, or an ERR_PTR with a negative errno.
> + * Return: Pointer to the mux-state on success, an ERR_PTR with a negative errno on error,
> + * or NULL if optional is true and mux doesn't exist.
>   */
> -static struct mux_state *mux_state_get(struct device *dev, const char *mux_name)
> +static struct mux_state *mux_state_get(struct device *dev, const char *mux_name, bool optional)
>  {
>  	struct mux_state *mstate;
>  
> @@ -685,12 +727,16 @@ static struct mux_state *mux_state_get(struct device *dev, const char *mux_name)
>  	if (!mstate)
>  		return ERR_PTR(-ENOMEM);
>  
> -	mstate->mux = mux_get(dev, mux_name, &mstate->state);
> +	mstate->mux = mux_get(dev, mux_name, &mstate->state, optional);
>  	if (IS_ERR(mstate->mux)) {
> -		int err = PTR_ERR(mstate->mux);
> -
>  		kfree(mstate);
> -		return ERR_PTR(err);
> +		return ERR_CAST(mstate->mux);
kernel robot found a use-after-free here which I will resolve for v11.
> +	} else if (optional && !mstate->mux) {
> +		kfree(mstate);
> +		return NULL;
> +	} else if (!mstate->mux) {
> +		kfree(mstate);
> +		return ERR_PTR(-ENOENT);
>  	}
>  
>  	return mstate;
> @@ -710,9 +756,66 @@ static void mux_state_put(struct mux_state *mstate)
>  
>  static void devm_mux_state_release(struct device *dev, void *res)
>  {
> -	struct mux_state *mstate = *(struct mux_state **)res;
> +	struct devm_mux_state_state *devm_state = res;
>  
> +	if (devm_state->exit)
> +		devm_state->exit(devm_state->mstate);
> +
> +	mux_state_put(devm_state->mstate);
> +}
> +
> +/**
> + * __devm_mux_state_get() - Get the optional mux-state for a device,
> + *			    with resource management.
> + * @dev: The device that needs a mux-state.
> + * @mux_name: The name identifying the mux-state.
> + * @optional: Whether to return NULL and silence errors when mux doesn't exist.
> + * @init: Optional function pointer for mux-state object initialisation.
> + * @exit: Optional function pointer for mux-state object cleanup on release.
> + *
> + * Return: Pointer to the mux-state on success, an ERR_PTR with a negative errno on error,
> + * or NULL if optional is true and mux doesn't exist.
> + */
> +static struct mux_state *__devm_mux_state_get(struct device *dev, const char *mux_name,
> +					      bool optional,
> +					      int (*init)(struct mux_state *mstate),
> +					      int (*exit)(struct mux_state *mstate))
> +{
> +	struct devm_mux_state_state *devm_state;
> +	struct mux_state *mstate;
> +	int ret;
> +
> +	mstate = mux_state_get(dev, mux_name, optional);
> +	if (IS_ERR(mstate))
> +		return ERR_CAST(mstate);
> +	else if (optional && !mstate)
> +		return NULL;
> +	else if (!mstate)
> +		return ERR_PTR(-ENOENT);
> +
> +	devm_state = devres_alloc(devm_mux_state_release, sizeof(*devm_state), GFP_KERNEL);
> +	if (!devm_state) {
> +		ret = -ENOMEM;
> +		goto err_devres_alloc;
> +	}
> +
> +	if (init) {
> +		ret = init(mstate);
> +		if (ret)
> +			goto err_mux_state_init;
> +	}
> +
> +	devm_state->mstate = mstate;
> +	devm_state->exit = exit;
> +	devres_add(dev, devm_state);
> +
> +	return mstate;
> +
> +err_mux_state_init:
> +	devres_free(devm_state);
> +err_devres_alloc:
>  	mux_state_put(mstate);
> +	return ERR_PTR(ret);
>  }
>  
>  /**
> @@ -722,28 +825,69 @@ static void devm_mux_state_release(struct device *dev, void *res)
>   * @mux_name: The name identifying the mux-control.
>   *
>   * Return: Pointer to the mux-state, or an ERR_PTR with a negative errno.
> + *
> + * The mux-state will automatically be freed on release.
>   */
> -struct mux_state *devm_mux_state_get(struct device *dev,
> -				     const char *mux_name)
> +struct mux_state *devm_mux_state_get(struct device *dev, const char *mux_name)
>  {
> -	struct mux_state **ptr, *mstate;
> -
> -	ptr = devres_alloc(devm_mux_state_release, sizeof(*ptr), GFP_KERNEL);
> -	if (!ptr)
> -		return ERR_PTR(-ENOMEM);
> +	return __devm_mux_state_get(dev, mux_name, false, NULL, NULL);
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_state_get);
>  
> -	mstate = mux_state_get(dev, mux_name);
> -	if (IS_ERR(mstate)) {
> -		devres_free(ptr);
> -		return mstate;
> -	}
> +/**
> + * devm_mux_state_get_optional() - Get the optional mux-state for a device,
> + *				   with resource management.
> + * @dev: The device that needs a mux-state.
> + * @mux_name: The name identifying the mux-state.
> + *
> + * Return: Pointer to the mux-state on success, an ERR_PTR with a negative errno on error,
> + * or NULL if mux doesn't exist.
> + *
> + * The mux-state will automatically be freed on release.
> + */
> +struct mux_state *devm_mux_state_get_optional(struct device *dev, const char *mux_name)
> +{
> +	return __devm_mux_state_get(dev, mux_name, true, NULL, NULL);
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_state_get_optional);
>  
> -	*ptr = mstate;
> -	devres_add(dev, ptr);
> +/**
> + * devm_mux_state_get_selected() - Get the mux-state for a device, with
> + *				   resource management.
> + * @dev: The device that needs a mux-state.
> + * @mux_name: The name identifying the mux-state.
> + *
> + * Return: Pointer to the mux-state, or an ERR_PTR with a negative errno.
> + *
> + * The returned mux-state (if valid) is already selected.
> + *
> + * The mux-state will automatically be deselected and freed on release.
> + */
> +struct mux_state *devm_mux_state_get_selected(struct device *dev, const char *mux_name)
> +{
> +	return __devm_mux_state_get(dev, mux_name, false, mux_state_select, mux_state_deselect);
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_state_get_selected);
>  
> -	return mstate;
> +/**
> + * devm_mux_state_get_optional_selected() - Get the optional mux-state for
> + *					    a device, with resource management.
> + * @dev: The device that needs a mux-state.
> + * @mux_name: The name identifying the mux-state.
> + *
> + * Return: Pointer to the mux-state on success, an ERR_PTR with a negative errno on error,
> + * or NULL if mux doesn't exist.
> + *
> + * The returned mux-state (if valid) is already selected.
> + *
> + * The mux-state will automatically be deselected and freed on release.
> + */
> +struct mux_state *devm_mux_state_get_optional_selected(struct device *dev,
> +						       const char *mux_name)
> +{
> +	return __devm_mux_state_get(dev, mux_name, true, mux_state_select, mux_state_deselect);
>  }
> -EXPORT_SYMBOL_GPL(devm_mux_state_get);
> +EXPORT_SYMBOL_GPL(devm_mux_state_get_optional_selected);
>  
>  /*
>   * Using subsys_initcall instead of module_init here to try to ensure - for
> diff --git a/include/linux/mux/consumer.h b/include/linux/mux/consumer.h
> index 2e25c838f831..a961861a503b 100644
> --- a/include/linux/mux/consumer.h
> +++ b/include/linux/mux/consumer.h
> @@ -16,6 +16,8 @@ struct device;
>  struct mux_control;
>  struct mux_state;
>  
> +#if IS_ENABLED(CONFIG_MULTIPLEXER)
> +
>  unsigned int mux_control_states(struct mux_control *mux);
>  int __must_check mux_control_select_delay(struct mux_control *mux,
>  					  unsigned int state,
> @@ -54,11 +56,109 @@ int mux_control_deselect(struct mux_control *mux);
>  int mux_state_deselect(struct mux_state *mstate);
>  
>  struct mux_control *mux_control_get(struct device *dev, const char *mux_name);
> +struct mux_control *mux_control_get_optional(struct device *dev, const char *mux_name);
>  void mux_control_put(struct mux_control *mux);
>  
> -struct mux_control *devm_mux_control_get(struct device *dev,
> -					 const char *mux_name);
> -struct mux_state *devm_mux_state_get(struct device *dev,
> -				     const char *mux_name);
> +struct mux_control *devm_mux_control_get(struct device *dev, const char *mux_name);
> +struct mux_state *devm_mux_state_get(struct device *dev, const char *mux_name);
> +struct mux_state *devm_mux_state_get_optional(struct device *dev, const char *mux_name);
> +struct mux_state *devm_mux_state_get_selected(struct device *dev, const char *mux_name);
> +struct mux_state *devm_mux_state_get_optional_selected(struct device *dev, const char *mux_name);
> +
> +#else
> +
> +static inline unsigned int mux_control_states(struct mux_control *mux)
> +{
> +	return 0;
> +}
> +static inline int __must_check mux_control_select_delay(struct mux_control *mux,
> +							unsigned int state, unsigned int delay_us)
> +{
> +	return -EOPNOTSUPP;
> +}
> +static inline int __must_check mux_state_select_delay(struct mux_state *mstate,
> +						      unsigned int delay_us)
> +{
> +	return -EOPNOTSUPP;
> +}
> +static inline int __must_check mux_control_try_select_delay(struct mux_control *mux,
> +							    unsigned int state,
> +							    unsigned int delay_us)
> +{
> +	return -EOPNOTSUPP;
> +}
> +static inline int __must_check mux_state_try_select_delay(struct mux_state *mstate,
> +							  unsigned int delay_us)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static inline int __must_check mux_control_select(struct mux_control *mux,
> +						  unsigned int state)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static inline int __must_check mux_state_select(struct mux_state *mstate)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static inline int __must_check mux_control_try_select(struct mux_control *mux,
> +						      unsigned int state)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static inline int __must_check mux_state_try_select(struct mux_state *mstate)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static inline int mux_control_deselect(struct mux_control *mux)
> +{
> +	return -EOPNOTSUPP;
> +}
> +static inline int mux_state_deselect(struct mux_state *mstate)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static inline struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
> +{
> +	return ERR_PTR(-EOPNOTSUPP);
> +}
> +static inline struct mux_control *mux_control_get_optional(struct device *dev,
> +							   const char *mux_name)
> +{
> +	return NULL;
> +}
> +static inline void mux_control_put(struct mux_control *mux) {}
> +
> +static inline struct mux_control *devm_mux_control_get(struct device *dev, const char *mux_name)
> +{
> +	return ERR_PTR(-EOPNOTSUPP);
> +}
> +static inline struct mux_state *devm_mux_state_get(struct device *dev, const char *mux_name)
> +{
> +	return ERR_PTR(-EOPNOTSUPP);
> +}
> +static inline struct mux_state *devm_mux_state_get_optional(struct device *dev,
> +							    const char *mux_name)
> +{
> +	return NULL;
> +}
> +static inline struct mux_state *devm_mux_state_get_selected(struct device *dev,
> +							    const char *mux_name)
> +{
> +	return ERR_PTR(-EOPNOTSUPP);
> +}
> +static inline struct mux_state *devm_mux_state_get_optional_selected(struct device *dev,
> +								     const char *mux_name)
> +{
> +	return NULL;
> +}
> +
> +#endif /* CONFIG_MULTIPLEXER */
>  
>  #endif /* _LINUX_MUX_CONSUMER_H */
>