Re: [PATCH] tty/n_gsm.c: do not clear gsm_mux entry when the gsm is not closed

["xinhui.pan" <xinhuix.pan@intel.com>]

hi, Greg

于 2014年07月28日 02:09, Greg Kroah-Hartman 写道:
> On Thu, Jul 24, 2014 at 05:17:01PM +0800, xinhui.pan wrote:
>> If the gsmtty is still used by some process, we could not just
>> simply clear gsm_mux[gsm->num]. Clear it when gsm is being free.
>> Otherwise we will hit crashes when userspace close the gsmtty.
>>
>> Also add gsm_mux_get() and gsm_mux_put() to make gsm_mux[] is used safely.
>> We can do activation/deactivation with same gsm more than once now.
>> This is for fixing the FIXME.
>>
>> Signed-off-by: xinhui.pan <xinhuiX.pan@intel.com>
>> Reviewed-by: Zhang Yanmin <yanmin_zhang@linux.intel.com>
>> ---
>>  drivers/tty/n_gsm.c |   84 ++++++++++++++++++++++++++++++++++++---------------
>>  1 file changed, 60 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
>> index 81e7ccb..290df56 100644
>> --- a/drivers/tty/n_gsm.c
>> +++ b/drivers/tty/n_gsm.c
>> @@ -2020,6 +2020,58 @@ static void gsm_error(struct gsm_mux *gsm,
>>  }
>>  
>>  /**
>> + *	gsm_mux_get		-	get/fill one entry in gsm_mux
>> + *	@gsm: our gsm
>> + *
>> + *	Although its name end with get, it don't inc ref-count actually.
> 
> Then don't call it a 'get' function :(
> 

Thanks for you nice advices!
I will change its name. This is really a confusable name.

>> + *	get one entry is just like fill pte, first memory access will
>> + *	cause page_fault, the next accesses don't. So do here.
> 
> This doesn't make much sense to me, can you please explain it better?
> 
>> + */
>> +
> 
> blank line?
> 

Sorry about that. But I notice the other function's introduce in n_gsm.c is done in same way.
I just wanted to keep same code style.
OK, I will change it.
I think no blank line is better. :)

>> +static int gsm_mux_get(struct gsm_mux *gsm)
>> +{
>> +	int i;
>> +
>> +	if (gsm->num >= MAX_MUX) /* gsm is alloc by kzalloc, just be careful */
>> +		return -EIO;
> 
> -EIO?
> 

Thanks for pointing out the mistake.
perhaps -EINVAL is better.

>> +	if (gsm_mux[gsm->num] == gsm) /* We have already set gsm->num */
>> +		return 0;
>> +
>> +	spin_lock(&gsm_mux_lock);
>> +	for (i = 0; i < MAX_MUX; i++) {
>> +		if (gsm_mux[i] == NULL) {
>> +			gsm->num = i;
>> +			gsm_mux[i] = gsm;
>> +			break;
>> +		}
>> +	}
>> +	spin_unlock(&gsm_mux_lock);
>> +
>> +	if (i == MAX_MUX)
>> +		return -EBUSY;
>> +	return 0;
>> +}
>> +
>> +/**
>> + *	gsm_mux_put		-	put/clear one entry in gsm_mux
>> + *	@gsm: our gsm
>> + *
>> + *	Although its name end with put, it don't dec ref-count actually.
>> + *	put one entry is just like clear pte, So do here.
>> + */
>> +
>> +static void gsm_mux_put(struct gsm_mux *gsm)
>> +{
>> +	if (gsm->num >= MAX_MUX)
>> +		return;
>> +
>> +	spin_lock(&gsm_mux_lock);
>> +	if (gsm_mux[gsm->num] == gsm)
> 
> How can this not be true?

That is a big problem. let me explain it.

In current code, we add gsm into gsm_mux[] in gsm_activate_mux(). So if gsm_activate_mux() fails when gsm_mux[] is full, gsm->num is invaild, it is zero in fact.
So when will free this gsm, we have to check if gsm_mux[gsm->num] is really the one we are deleting.
The scenario is like below.
gsmld_open -> gsmld_attach_gsm -> gsm_activate_mux(fails) -> .... -> mux_put -> gsm_free_muxr -> gsm_mux_put(need check gsm_mux[gsm->num] == gsm).

Current code is a little hard to understand, So I suggest that move the *codes that changes gsm_mux[]* into gsm_alloc_mux(), then we don't need this check anymore.
Actually I have worked out a new patch with such idea.
And I am doing the normal tests set up by Intel test team and other trigger tests set up by myself. :)

> 
>> +		gsm_mux[gsm->num] = NULL;
>> +	spin_unlock(&gsm_mux_lock);
>> +}
> 
> Why can't you do dynamic reference counting of your structure, that
> would allow you to get rid of your global array, right?
> 

Thanks for your nice comments.
Struct gsm has a ref-count already. :)
And also adding a ref-count is a little hard to me. :(
This global array is used to keep tracking the gsms that stands for the gsmttyXX.
and it can tell us if we can create a new gsm. :)
In gsm_init we set *gsm_tty_driver = alloc_tty_driver(256);*

thanks,

xinhui

> thanks,
> 
> greg k-h
> 

Your reply really helps us a lot.
thanks,

xinhui