[PATCH 1/1] tty: n_gsm: Fix SW flow control encoding/handling

[PATCH 1/1] tty: n_gsm: Fix SW flow control encoding/handling

[Daniel Starke]

According to 3GPP 27.010 chapter 5.2.7.3 DC1 and DC3 (SW flow control) are to
be treated according to ISO/IEC 646. That means the MSB shall be ignored.
This patch applies the needed changes to handle this correctly.

Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
---
 drivers/tty/n_gsm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 0b96b14bbfe1..9ee0643fc9e2 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -322,6 +322,7 @@ static int addr_cnt;
 #define GSM1_ESCAPE_BITS	0x20
 #define XON			0x11
 #define XOFF			0x13
+#define ASCII_MASK		0x7F
 
 static const struct tty_port_operations gsm_port_ops;
 
@@ -521,7 +522,7 @@ static void gsm_print_packet(const char *hdr, int addr, int cr,
  *	@output: output buffer
  *	@len: length of input
  *
- *	Expand a buffer by bytestuffing it. The worst case size change
+ *	Expand a buffer by byte stuffing it. The worst case size change
  *	is doubling and the caller is responsible for handing out
  *	suitable sized buffers.
  */
@@ -531,7 +532,8 @@ static int gsm_stuff_frame(const u8 *input, u8 *output, int len)
 	int olen = 0;
 	while (len--) {
 		if (*input == GSM1_SOF || *input == GSM1_ESCAPE
-		    || *input == XON || *input == XOFF) {
+		    || (*input & ASCII_MASK) == XON
+		    || (*input & ASCII_MASK) == XOFF) {
 			*output++ = GSM1_ESCAPE;
 			*output++ = *input++ ^ GSM1_ESCAPE_BITS;
 			olen++;
-- 
2.25.1

Re: [PATCH 1/1] tty: n_gsm: Fix SW flow control encoding/handling

[Greg KH]

On Mon, Jan 10, 2022 at 11:23:17PM -0800, Daniel Starke wrote:
> According to 3GPP 27.010 chapter 5.2.7.3 DC1 and DC3 (SW flow control)

What is all of that?  Do you have a link to the document that this is
and where it says this?

> are to
> be treated according to ISO/IEC 646.

What is "ISO/IEC 646"?

> That means the MSB shall be ignored.

"MSB"?  Please spell it out, you have plenty of room here.

> This patch applies the needed changes to handle this correctly.

What changes are needed?  Please talk about what you are doing, as the
documentation asks you to so do.

> 
> Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> ---
>  drivers/tty/n_gsm.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 0b96b14bbfe1..9ee0643fc9e2 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -322,6 +322,7 @@ static int addr_cnt;
>  #define GSM1_ESCAPE_BITS	0x20
>  #define XON			0x11
>  #define XOFF			0x13
> +#define ASCII_MASK		0x7F

Where did "ASCII" come from?  You didn't say anything about that in the
changelog.

>  static const struct tty_port_operations gsm_port_ops;
>  
> @@ -521,7 +522,7 @@ static void gsm_print_packet(const char *hdr, int addr, int cr,
>   *	@output: output buffer
>   *	@len: length of input
>   *
> - *	Expand a buffer by bytestuffing it. The worst case size change
> + *	Expand a buffer by byte stuffing it. The worst case size change

This change is not described above, and is totally different and belongs
in a different change.


>   *	is doubling and the caller is responsible for handing out
>   *	suitable sized buffers.
>   */
> @@ -531,7 +532,8 @@ static int gsm_stuff_frame(const u8 *input, u8 *output, int len)
>  	int olen = 0;
>  	while (len--) {
>  		if (*input == GSM1_SOF || *input == GSM1_ESCAPE
> -		    || *input == XON || *input == XOFF) {
> +		    || (*input & ASCII_MASK) == XON
> +		    || (*input & ASCII_MASK) == XOFF) {
>  			*output++ = GSM1_ESCAPE;
>  			*output++ = *input++ ^ GSM1_ESCAPE_BITS;
>  			olen++;
> -- 
> 2.25.1
> 

What commit does this fix?

Please read the section entitled "The canonical patch format" in the
kernel file, Documentation/SubmittingPatches for a description of how
to do all of this better.

thanks,

greg k-h

Re: [PATCH 1/1] tty: n_gsm: Fix SW flow control encoding/handling

[Greg KH]

On Tue, Jan 11, 2022 at 11:54:01AM +0000, Starke, Daniel wrote:
> > > According to 3GPP 27.010 chapter 5.2.7.3 DC1 and DC3 (SW flow control)
> > 
> > What is all of that?  Do you have a link to the document that this is and where it says this?
> 
> n_gsm is based on the 3GPP 07.010 and its newer version is the 3GPP 27.010.
> See https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=1516
> The changes from 07.010 to 27.010 are non-functional. Therefore, I refer to
> the newer 27.010 here. Chapter 5.2.7.3 states that DC1 and DC3 data bytes
> shall be quoted to ensure transparent transmission of these bytes without
> setting software flow control. This is partly already the case in the
> current n_gsm implementation. This chapter refers also to ISO/IEC 646
> regarding the encoding of DC1 and DC3.
> 
> > > are to
> > > be treated according to ISO/IEC 646.
> > 
> > What is "ISO/IEC 646"?
> 
> ISO/IEC 646 refers to the set of ISO standards described as the ISO 7-bit
> coded character set for information interchange. Its final version is also
> known as ITU T.50. See https://www.itu.int/rec/T-REC-T.50-199209-I/en
> 
> > > That means the MSB shall be ignored.
> > 
> > "MSB"?  Please spell it out, you have plenty of room here.
> 
> MSB stands for "most significant bit" in this context.

Please put all of the above in the changelog text when you resubmit
this.

> > > This patch applies the needed changes to handle this correctly.
> > 
> > What changes are needed?  Please talk about what you are doing, as the documentation asks you to so do.
> 
> To abide the standard it is needed to quote DC1 and DC3 correctly if these
> are seen as data bytes and not as control characters. The current code
> already tries to enforces this but fails to catch all defined cases.
> 3GPP 27.010 chapter 5.2.7.3 clearly states that the most significant bit
> shall be ignored for DC1 and DC3 handling. The current implementation
> handles only the case with the most significant bit was set 0. Cases in
> which DC1 and DC3 have the most significant bit set 1 are left unhandled.
> This patch fixes this by masking the data bytes with ASCII_MASK (only the
> 7 least significant bits set 1) before comparing them with XON (a.k.a. DC1)
> and XOFF (a.k.a. DC3).

Great, again, please put this in the changelog text so that we can
properly understand it.

> > > Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> > > ---
> > >  drivers/tty/n_gsm.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c index 
> > > 0b96b14bbfe1..9ee0643fc9e2 100644
> > > --- a/drivers/tty/n_gsm.c
> > > +++ b/drivers/tty/n_gsm.c
> > > @@ -322,6 +322,7 @@ static int addr_cnt;
> > >  #define GSM1_ESCAPE_BITS	0x20
> > >  #define XON			0x11
> > >  #define XOFF			0x13
> > > +#define ASCII_MASK		0x7F
> > 
> > Where did "ASCII" come from?  You didn't say anything about that in the changelog.
> 
> The original version (ISO 646 IRV) differed from ASCII only in the code
> point for the currency symbol. Therefore, I used ASCII_MASK here to define
> the mask for the significant bits. I believe that this is easier to
> understand than ISO_IEC_646_MASK for example.

If this really is for ISO 646 then please use that text here.

> > >  static const struct tty_port_operations gsm_port_ops;
> > >  
> > > @@ -521,7 +522,7 @@ static void gsm_print_packet(const char *hdr, int addr, int cr,
> > >   *	@output: output buffer
> > >   *	@len: length of input
> > >   *
> > > - *	Expand a buffer by bytestuffing it. The worst case size change
> > > + *	Expand a buffer by byte stuffing it. The worst case size change
> > 
> > This change is not described above, and is totally different and belongs in a different change.
> 
> You are absolutely right. Shall I create a new patch?

Yes please.

> > >   *	is doubling and the caller is responsible for handing out
> > >   *	suitable sized buffers.
> > >   */
> > > @@ -531,7 +532,8 @@ static int gsm_stuff_frame(const u8 *input, u8 *output, int len)
> > >  	int olen = 0;
> > >  	while (len--) {
> > >  		if (*input == GSM1_SOF || *input == GSM1_ESCAPE
> > > -		    || *input == XON || *input == XOFF) {
> > > +		    || (*input & ASCII_MASK) == XON
> > > +		    || (*input & ASCII_MASK) == XOFF) {
> > >  			*output++ = GSM1_ESCAPE;
> > >  			*output++ = *input++ ^ GSM1_ESCAPE_BITS;
> > >  			olen++;
> > > --
> > > 2.25.1
> > > 
> > 
> > What commit does this fix?
> 
> It fixes the initial commit for the n_gsm:
> Commit e1eaea46bb40 (tty: n_gsm line discipline, 2010-03-26)

Great, please add that to the commit when you submit it.  Also it should
go to stable kernels so please add that marking as the documentation
asks for.

> However, this patch is based on the main branch from the TTY/Serial driver
> development tree.

That branch tracks Linus's tree, not the tty/serial driver changes, so
you might want to use a different branch if you think there are going to
be conflicts.

thanks,

greg k-h

RE: [PATCH 1/1] tty: n_gsm: Fix SW flow control encoding/handling

[Starke, Daniel]

> > According to 3GPP 27.010 chapter 5.2.7.3 DC1 and DC3 (SW flow control)
> 
> What is all of that?  Do you have a link to the document that this is and where it says this?

n_gsm is based on the 3GPP 07.010 and its newer version is the 3GPP 27.010.
See https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=1516
The changes from 07.010 to 27.010 are non-functional. Therefore, I refer to
the newer 27.010 here. Chapter 5.2.7.3 states that DC1 and DC3 data bytes
shall be quoted to ensure transparent transmission of these bytes without
setting software flow control. This is partly already the case in the
current n_gsm implementation. This chapter refers also to ISO/IEC 646
regarding the encoding of DC1 and DC3.

> > are to
> > be treated according to ISO/IEC 646.
> 
> What is "ISO/IEC 646"?

ISO/IEC 646 refers to the set of ISO standards described as the ISO 7-bit
coded character set for information interchange. Its final version is also
known as ITU T.50. See https://www.itu.int/rec/T-REC-T.50-199209-I/en

> > That means the MSB shall be ignored.
> 
> "MSB"?  Please spell it out, you have plenty of room here.

MSB stands for "most significant bit" in this context.

> > This patch applies the needed changes to handle this correctly.
> 
> What changes are needed?  Please talk about what you are doing, as the documentation asks you to so do.

To abide the standard it is needed to quote DC1 and DC3 correctly if these
are seen as data bytes and not as control characters. The current code
already tries to enforces this but fails to catch all defined cases.
3GPP 27.010 chapter 5.2.7.3 clearly states that the most significant bit
shall be ignored for DC1 and DC3 handling. The current implementation
handles only the case with the most significant bit was set 0. Cases in
which DC1 and DC3 have the most significant bit set 1 are left unhandled.
This patch fixes this by masking the data bytes with ASCII_MASK (only the
7 least significant bits set 1) before comparing them with XON (a.k.a. DC1)
and XOFF (a.k.a. DC3).

> > 
> > Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> > ---
> >  drivers/tty/n_gsm.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c index 
> > 0b96b14bbfe1..9ee0643fc9e2 100644
> > --- a/drivers/tty/n_gsm.c
> > +++ b/drivers/tty/n_gsm.c
> > @@ -322,6 +322,7 @@ static int addr_cnt;
> >  #define GSM1_ESCAPE_BITS	0x20
> >  #define XON			0x11
> >  #define XOFF			0x13
> > +#define ASCII_MASK		0x7F
> 
> Where did "ASCII" come from?  You didn't say anything about that in the changelog.

The original version (ISO 646 IRV) differed from ASCII only in the code
point for the currency symbol. Therefore, I used ASCII_MASK here to define
the mask for the significant bits. I believe that this is easier to
understand than ISO_IEC_646_MASK for example.

> >  static const struct tty_port_operations gsm_port_ops;
> >  
> > @@ -521,7 +522,7 @@ static void gsm_print_packet(const char *hdr, int addr, int cr,
> >   *	@output: output buffer
> >   *	@len: length of input
> >   *
> > - *	Expand a buffer by bytestuffing it. The worst case size change
> > + *	Expand a buffer by byte stuffing it. The worst case size change
> 
> This change is not described above, and is totally different and belongs in a different change.

You are absolutely right. Shall I create a new patch?

> >   *	is doubling and the caller is responsible for handing out
> >   *	suitable sized buffers.
> >   */
> > @@ -531,7 +532,8 @@ static int gsm_stuff_frame(const u8 *input, u8 *output, int len)
> >  	int olen = 0;
> >  	while (len--) {
> >  		if (*input == GSM1_SOF || *input == GSM1_ESCAPE
> > -		    || *input == XON || *input == XOFF) {
> > +		    || (*input & ASCII_MASK) == XON
> > +		    || (*input & ASCII_MASK) == XOFF) {
> >  			*output++ = GSM1_ESCAPE;
> >  			*output++ = *input++ ^ GSM1_ESCAPE_BITS;
> >  			olen++;
> > --
> > 2.25.1
> > 
> 
> What commit does this fix?

It fixes the initial commit for the n_gsm:
Commit e1eaea46bb40 (tty: n_gsm line discipline, 2010-03-26)
However, this patch is based on the main branch from the TTY/Serial driver
development tree.

With best regards,
Daniel Starke