netdev - RE: [PATCH net 3/3] ice: Add set_termios tty operations handle to GNSS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 8 Sep 2022 13:44:33 +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

> From: Greg Kroah-Hartman <gregkh@...uxfoundation.org> 
> Sent: Wednesday, September 7, 2022 7:46 AM
> 
> On Tue, Sep 06, 2022 at 08:55:59PM +0000, Michalik, Michal wrote:
> > Greg,
> > 
> > Thanks - answer inline.
> 
> As is required, no need to put this on your emails, as top-posting is
> not allowed :)

Thanks - lesson learned.

> 
> > > -----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
> 
> Please fix your email client to not do this.
> 

Thanks, I will no longer send it. 

> > > > 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.
> > > 
> > 
> > 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.
> 
> You get the error on the first get or the set?
> 

On "set" - I'm doing get only to prove interface is working and read current
attributes to change only one of them.

> > 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.
> 
> This should not be needed as I thought the default would be "just ignore
> this", but maybe not.  Can you look into this problem please and figure
> out why this is required and fix that up?
> 
> > 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. 
> 
> Let's fix the root problem here, not paper over it.
> 

Ok - now I understand what you mean. If I get it correctly in your opinion the
kernel should not return error 22 if this handler is not set. I thought it is
an expected behavior, but I will take a look if that might be a bug.

Thanks,
M^2

> thanks,
> 
> greg k-h
>