linux-kernel - Re: R: R: [PATCH v1] serial: 8250

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Tue, 15 Dec 2020 15:38:05 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Flavio Suligoi <f.suligoi@...m.it>
Cc:     Jiri Slaby <jirislaby@...nel.org>,
        "Gustavo A . R . Silva" <gustavoars@...nel.org>,
        Ji-Ze Hong <hpeter@...il.com>,
        "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: R: R: [PATCH v1] serial: 8250_fintek: Print Fintek chip name

On Tue, Dec 15, 2020 at 02:06:09PM +0000, Flavio Suligoi wrote:
> Hi Greg,
> 
> 
> > > > > +		chip_name = "F81216H";
> > > > > +		break;
> > > > >  	case CHIP_ID_F81216:
> > > > > +		chip_name = "F81216";
> > > > >  		break;
> > > > >  	default:
> > > > >  		return -ENODEV;
> > > > >  	}
> > > > >
> > > > >  	pdata->pid = chip;
> > > > > +
> > > > > +	pr_info("%s%s%s Fintek %s\n",
> > > > > +		uart->port.dev ? dev_name(uart->port.dev) : "",
> > > > > +		uart->port.dev ? ": " : "",
> > > > > +		uart->port.name,
> > > > > +		chip_name);
> > > >
> > > > Drivers, if all goes well, should not print anything to the kernel
> > log.
> > > > This isn't ok.
> > > >
> > > > And even if it was, dev_info() would be the correct thing to do...
> > >
> > > Ok, too many information in the driver.
> > >
> > > But what do you think about the possibility to introduce
> > > a new additional field, in "serial8250_config" structure,
> > > such as "extra_name" or something like this:
> > >
> > > struct serial8250_config {
> > > 	const char		*name;
> > > 	const char		*extra_name;
> > > 	unsigned short	fifo_size;
> > > 	unsigned short	tx_loadsz;
> > > 	unsigned char	fcr;
> > > 	unsigned char	rxtrig_bytes[UART_FCR_R_TRIG_MAX_STATE];
> > > 	unsigned int	flags;
> > > };
> > >
> > > In this way, if required, each driver can fill this
> > > additional field, for example adding the name of
> > > the particular uart chip or other useful info.
> > >
> > > As result, for example, the "uart_report_port" function output
> > > could be something like this:
> > >
> > > 00:01: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 115200) is a 16550A -
> > Fintek F81216AD
> > > 00:02: ttyS3 at I/O 0x2e8 (irq = 11, base_baud = 115200) is a 16550A -
> > Fintek F81216AD
> > >
> > > where the "extra_name", if not empty, is printed
> > > at the end of the line.
> > > For practical space reasons, the "extra_name" length
> > > can be limited to 16 chars.
> > 
> > Why?  What tool will use this, and why would userspace care about it?
> > 
> > What problem are you trying to solve here?
> 
> I try to explain my requirement:
> 
> we produce some x86 boards with multistandard RS232/422/485 ports
> and, to have this feature, in some of these boards, we use a
> Fintek uart or superIO.
> So this additional info "extra_name" can be useful for
> a quick check if the serial ports are multistandard or not,
> without any other investigations, but using only a simple command
> like:
> 
> dmesg| grep ttyS

But as they work the same, why does it matter?

Userspace should not care here.  Isn't there some other id you can
read/query for a hardware database tool to determine this?

Printing a random string to the kernel log is not a good way to do
hardware descriptions in a format that everyone can easily parse them :)

thanks,

greg k-h