Re: [PATCH] tty: n_gsm: Fix CR bit value when initiator=0

[赵振国]

Dear gregkh

1:  Documentation/driver-api/serial/n_gsm.rst

The text introduces the config of master ( c.initiator = 1), but the
config of as responder is different.
when set gsm->initiator=0 by GSMIOC_SETCONF ,ngsm driver should be
responder(slaver)

config：
	c.initiator = 0;    // set initiator=0，ngsm as responder
	ioctl(fd, GSMIOC_SETCONF, &c);

2:  if master side send SABM/DISC frame data by uart dev
    DLC0 control data frame:f9 03 3f 01 1c f9
    kernel log： gsmld_receive: 00000000: f9 03 3f 01 1c f9

{
	cr = gsm->address & 1;		/* C/R bit */
//CR value=1

	gsm_print_packet("<--", address, cr, gsm->control, gsm->buf, gsm->len);

	cr ^= 1 - gsm->initiator;	/* Flip so 1 always means command */
//when gsm->initiator is 0, CR value=0 by "^=" calculation
	dlci = gsm->dlci[address];

	switch (gsm->control) {
	case SABM|PF:
		if (cr == 0)
			goto invalid;                       //if CR value=0,ngsm will goto
invalid,but the dlc0 control frame data is right,if we can't modify
,ngsm can't send UA response data
}


2021-06-16 14:19 GMT+08:00, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>:
> On Wed, Jun 16, 2021 at 10:56:39AM +0800, Zhenguo Zhao wrote:
>> From: Zhenguo Zhao <zhenguo.zhao1@xxxxxxxxxx>
>>
>> 	When set initiator=0,switch to Responder,gsmld received dlci SABM/DISC
>> 	frame,CR bit should be 0 by calculation.
>>
>> 	receive DLC0 SABM CMD:
>> 	[69.740263] c1 gsmld_receive: 00000000: f9 03 3f 01 1c f9
>> 	[69.893247] c1 gsm_queue cr:1
>> 	[69.897629] c1 <-- 0) C: SABM(P)
>> 	[69.907516] c1 gsm_queue cr:0
>
> Why is this changelog text indented by tabs?
>
> And I do not understand the changelog text here, what is this showing?
> What is wrong here and what is being fixed?
>
>> Signed-off-by: Zhenguo Zhao <zhenguo.zhao1@xxxxxxxxxx>
>
> Does this fix a long-standing issue?  Should a "Fixes:" tag go here?  If
> so, please provide it.
>
> Should it also be sent to stable kernels?
>
>> ---
>>  drivers/tty/n_gsm.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
>> index 5fea02c..becca2c 100644
>> --- a/drivers/tty/n_gsm.c
>> +++ b/drivers/tty/n_gsm.c
>> @@ -1779,7 +1779,7 @@ static void gsm_queue(struct gsm_mux *gsm)
>>
>>  	switch (gsm->control) {
>>  	case SABM|PF:
>> -		if (cr == 0)
>> +		if (cr == 1)
>
> How did the original code ever work properly?
>
>>  			goto invalid;
>>  		if (dlci == NULL)
>>  			dlci = gsm_dlci_alloc(gsm, address);
>> @@ -1793,7 +1793,7 @@ static void gsm_queue(struct gsm_mux *gsm)
>>  		}
>>  		break;
>>  	case DISC|PF:
>> -		if (cr == 0)
>> +		if (cr == 1)
>
> Same here, how did this ever work?  Are you sure this change is correct?
>
> thanks,
>
> greg k-h
>