Re: [PATCH 4/6] mux: add missing mux_state_get

[Peter Rosin]

Hi!

On 2022-01-03 13:42, Greg Kroah-Hartman wrote:
> On Sun, Jan 02, 2022 at 11:38:36PM +0100, Peter Rosin wrote:
>> From: Peter Rosin <peda@xxxxxxxxxx>
>>
>> And implement devm_mux_state_get in terms of the new function.
>>
>> Tested-by: Aswath Govindraju <a-govindraju@xxxxxx>
>> Signed-off-by: Peter Rosin <peda@xxxxxxxxxx>
>> ---
>>  drivers/mux/core.c           | 41 ++++++++++++++++++++++++++----------
>>  include/linux/mux/consumer.h |  1 +
>>  2 files changed, 31 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
>> index 7d38e7c0c02e..90073ce01539 100644
>> --- a/drivers/mux/core.c
>> +++ b/drivers/mux/core.c
>> @@ -673,6 +673,33 @@ 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.
>> + *
>> + * Return: A pointer to the mux-state, or an ERR_PTR with a negative errno.
>> + */
>> +struct mux_state *mux_state_get(struct device *dev, const char *mux_name)
>> +{
>> +	struct mux_state *mstate;
>> +
>> +	mstate = kzalloc(sizeof(*mstate), GFP_KERNEL);
>> +	if (!mstate)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	mstate->mux = mux_get(dev, mux_name, &mstate->state);
> 
> will this build?  I haven't applied it but mux_get() in my tree right
> now is defined as:
> 	static inline void mux_get(struct gsm_mux *gsm)

Yes it builds. As mentioned in the cover letter, the patches have been
in -next for a couple of weeks. The static definition you are pointing
at is from n_gsm.c (which does not seem to be #included by any other
file). This definition of mux_get is again static and in a .c file
(which is not #included by anything). Surely not a conflict?

> 
>> +	if (IS_ERR(mstate->mux)) {
>> +		int err = PTR_ERR(mstate->mux);
>> +
>> +		kfree(mstate);
>> +		return ERR_PTR(err);
>> +	}
>> +
>> +	return mstate;
>> +}
>> +EXPORT_SYMBOL_GPL(mux_state_get);
> 
> No need to export it or make it global if no one is using it, right?

This is support code, and I think it should be up to the consumers
to decide if managed interfaced should be used or not, and would
rather not push anyone to managed interfaces if that does not work
or does not fit for some reason.

> Also, who frees this new memory you just allocated?

mux_state_put

> 
>> +
>>  /**
>>   * mux_state_put() - Put away the mux-state for good.
>>   * @mstate: The mux-state to put away.
>> @@ -705,25 +732,17 @@ struct mux_state *devm_mux_state_get(struct device *dev,
>>  				     const char *mux_name)
>>  {
>>  	struct mux_state **ptr, *mstate;
>> -	struct mux_control *mux_ctrl;
>> -	int state;
>> -
>> -	mstate = devm_kzalloc(dev, sizeof(struct mux_state), GFP_KERNEL);
>> -	if (!mstate)
>> -		return ERR_PTR(-ENOMEM);
> 
> Before this state memory was here, assigned to the device, so it was
> freed when the device was unbound.  I'm missing where this now
> happens...

Again, mux_state_put (via devm_mux_state_release).

Cheers,
Peter