Date:   Tue, 6 Sep 2022 20:55:59 +0000
From:   "Michalik, Michal" <michal.michalik@...el.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:     Jakub Kicinski <kuba@...nel.org>,
        "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "richardcochran@...il.com" <richardcochran@...il.com>,
        "G, GurucharanX" <gurucharanx.g@...el.com>,
        Jiri Slaby <jirislaby@...nel.org>,
        Johan Hovold <johan@...nel.org>
Subject: RE: [PATCH net 3/3] ice: Add set_termios tty operations handle to
 GNSS

Greg,

Thanks - answer inline.

BR,
M^2

> 
> -----Original Message-----
> From: Greg Kroah-Hartman <gregkh@...uxfoundation.org> 
> Sent: Tuesday, September 6, 2022 8:16 AM
> To: Michalik, Michal <michal.michalik@...el.com>
> Cc: Jakub Kicinski <kuba@...nel.org>; Nguyen, Anthony L <anthony.l.nguyen@...el.com>; davem@...emloft.net; pabeni@...hat.com; edumazet@...gle.com; netdev@...r.kernel.org; richardcochran@...il.com; G, GurucharanX <gurucharanx.g@...el.com>; Jiri Slaby <jirislaby@...nel.org>; Johan Hovold <johan@...nel.org>
> Subject: Re: [PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS
> 
> On Mon, Sep 05, 2022 at 07:32:44PM +0000, Michalik, Michal wrote:
> > Hello Greg,
> > 
> > Much thanks for a feedback. Please excuse me for delayed answer, we tried to collect all
> > the required information before returning to you - but we are still working on it.
> > 
> > Best regards,
> > M^2
> > 
> > > 
> > > -----Original Message-----
> > > From: Greg Kroah-Hartman <gregkh@...uxfoundation.org> 
> > > Sent: Thursday, September 1, 2022 7:46 AM
> > > To: Jakub Kicinski <kuba@...nel.org>
> > > Cc: Nguyen, Anthony L <anthony.l.nguyen@...el.com>; davem@...emloft.net; pabeni@...hat.com; edumazet@...gle.com; Michalik, Michal <michal.michalik@...el.com>; netdev@...r.kernel.org; richardcochran@...il.com; G, GurucharanX <gurucharanx.g@...el.com>; Jiri Slaby <jirislaby@...nel.org>; Johan Hovold <johan@...nel.org>
> > > Subject: Re: [PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS
> > > 
> > > On Wed, Aug 31, 2022 at 02:54:39PM -0700, Jakub Kicinski wrote:
> > > > On Mon, 29 Aug 2022 15:00:49 -0700 Tony Nguyen wrote:
> > > > > From: Michal Michalik <michal.michalik@...el.com>
> > > > > 
> > > > > Some third party tools (ex. ubxtool) try to change GNSS TTY parameters
> > > > > (ex. speed). While being optional implementation, without set_termios
> > > > > handle this operation fails and prevents those third party tools from
> > > > > working.
> > > 
> > > What tools are "blocked" by this?  And what is the problem they have
> > > with just the default happening here?  You are now doing nothing, while
> > > if you do not have the callback, at least a basic "yes, we accepted
> > > these values" happens which was intended for userspace to not know that
> > > there was a problem here.
> > > 
> > 
> > As I stated in the commit message, the example tool is ubxtool - while trying to
> > connect to the GPS module the error appreared:
> > Traceback (most recent call last):
> > 
> > 	  File "/usr/local/bin/ubxtool", line 378, in <module>
> > 		io_handle = gps.gps_io(
> > 	  File "/usr/local/lib/python3.9/site-packages/gps/gps.py", line 309, in __init__
> > 		self.ser = Serial.Serial(
> > 	  File "/usr/local/lib/python3.9/site-packages/serial/serialutil.py", line 244, in __init__
> > 		self.open()
> > 	  File "/usr/local/lib/python3.9/site-packages/serial/serialposix.py", line 332, in open
> > 		self._reconfigure_port(force_update=True)
> > 	  File "/usr/local/lib/python3.9/site-packages/serial/serialposix.py", line 517, in _reconfigure_port
> > 		termios.tcsetattr(
> > 	termios.error: (22, 'Invalid argument')
> > 	
> > Adding this empty function solved the problem.
> 
> That seems very wrong, please work to fix this by NOT having an empty
> function like this as it should not be required.
> 

Thanks for sharing the feedback - I love the possibility to learn from you and
other community members, because I'm really new in here.

I don't get one thing, though. You are saying, it "seem" wrong and that
"should not" be required but I observe different behavior. I have prepared
a very simple code to reproduce the issue:
	#include <termios.h>
	#include <unistd.h>
	#include <stdio.h>
	#include <fcntl.h>
	#include <errno.h>

	int main()
	{
		struct termios tty;
		int fd;
		
		fd = open("/dev/ttyGNSS_0300", O_RDWR | O_NOCTTY | O_SYNC);

		if (fd < 0) {
				printf("Error - TTY not open.\n");
				return -1;
		}
				
		if (tcgetattr (fd, &tty) != 0) {
			printf("Error on get - errno=%i\n", errno);
			return -1;
		}
		tty.c_cflag |= CS8; // try to set 8 data bits 
		if (tcsetattr(fd, TCSANOW, &tty) != 0) {
			printf("Error on set - errno=%i\n", errno);
			return -1;
		}

		close(fd);
		printf("Done.\n");
	}

In this case, when I don't satisfy this API, I get an errno 22. If add this
empty function and therefore implement the full API it works as expected (no
error). In our case no action is needed, therefore we have an empty function.
At the moment, I'm not sure how I should fix it other way - since no action
on HW is neccessary.

Of course in the meantime we are working on investigating if we can easily
align to existing GNSS interface accroding to community suggestions. Still,
we believe that this fix is solving the problem at the moment. 

> thanks,
> 
> greg k-h
>

Powered by blists - more mailing lists