Re: [PATCH] serial: don't register CIR serial ports

["Maciej S. Szmigiero" <mail@maciej.szmigiero.name>]

On 05.08.2015 16:51, Greg Kroah-Hartman wrote:
> On Wed, Aug 05, 2015 at 02:33:33PM +0200, Maciej S. Szmigiero wrote:
>> CIR type serial ports aren't real serial ports.
>>
>> This is just a way to prevent legacy 8250 serial
>> driver from probing and eventually binding some
>> resources.
>>
>> Since in current state such ports aren't providing
>> any real functionality and it is not possible
>> to change their type via setserial/ioctl(TIOCSSERIAL)
>> (due to UPF_FIXED_PORT flag set on them)
>> it is simpler and cleaner to not register them at all
>> with serial core.
>>
>> Print a short message in this case so it is known
>> to user what has happened.
>>
>> This way checks for PORT_8250_CIR in serial port
>> callbacks can be removed too, since they won't
>> ever be called.
>>
>> Signed-off-by: Maciej Szmigiero <mail@maciej.szmigiero.name>
>> ---
>> This replaces "serial: don't announce CIR serial ports"
>> submission.
>>
>>  drivers/tty/serial/8250/8250_core.c |   38 +++++++++++++++++-----------------
>>  1 files changed, 19 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
>> index 37fff12..b843b83 100644
>> --- a/drivers/tty/serial/8250/8250_core.c
>> +++ b/drivers/tty/serial/8250/8250_core.c
>> @@ -2129,9 +2129,6 @@ int serial8250_do_startup(struct uart_port *port)
>>  	unsigned char lsr, iir;
>>  	int retval;
>>  
>> -	if (port->type == PORT_8250_CIR)
>> -		return -ENODEV;
>> -
>>  	if (!port->fifosize)
>>  		port->fifosize = uart_config[port->type].fifo_size;
>>  	if (!up->tx_loadsz)
>> @@ -2858,14 +2855,8 @@ static void serial8250_release_port(struct uart_port *port)
>>  static int serial8250_request_port(struct uart_port *port)
>>  {
>>  	struct uart_8250_port *up = up_to_u8250p(port);
>> -	int ret;
>> -
>> -	if (port->type == PORT_8250_CIR)
>> -		return -ENODEV;
>> -
>> -	ret = serial8250_request_std_resource(up);
>>  
>> -	return ret;
>> +	return serial8250_request_std_resource(up);
>>  }
>>  
>>  static int fcr_get_rxtrig_bytes(struct uart_8250_port *up)
>> @@ -3013,9 +3004,6 @@ static void serial8250_config_port(struct uart_port *port, int flags)
>>  	struct uart_8250_port *up = up_to_u8250p(port);
>>  	int ret;
>>  
>> -	if (port->type == PORT_8250_CIR)
>> -		return;
>> -
>>  	/*
>>  	 * Find the region that we can probe for.  This in turn
>>  	 * tells us whether we can probe for the type of port.
>> @@ -3889,13 +3877,25 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
>>  		if (up->dl_write)
>>  			uart->dl_write = up->dl_write;
>>  
>> -		if (serial8250_isa_config != NULL)
>> -			serial8250_isa_config(0, &uart->port,
>> -					&uart->capabilities);
>> +		if (uart->port.type != PORT_8250_CIR) {
>> +			if (serial8250_isa_config != NULL)
>> +				serial8250_isa_config(0, &uart->port,
>> +						&uart->capabilities);
>>  
>> -		ret = uart_add_one_port(&serial8250_reg, &uart->port);
>> -		if (ret == 0)
>> -			ret = uart->port.line;
>> +			ret = uart_add_one_port(&serial8250_reg,
>> +						&uart->port);
>> +			if (ret == 0)
>> +				ret = uart->port.line;
>> +		} else {
>> +			pr_info("%s%sskipping CIR port at 0x%lx / 0x%llx, IRQ %d\n",
>> +				uart->port.dev ? dev_name(uart->port.dev) : "",
>> +				uart->port.dev ? ": " : "",
>> +				uart->port.iobase,
>> +				(unsigned long long)uart->port.mapbase,
>> +				uart->port.irq);
> 
> dev_info() perhaps?  It provides much of what you are trying to say
> here.

I thought about it, but while currently uart->port.dev is always set
(because these ports are "registered" by PNP 8250 driver) maybe
one day other sources will also make use of this port type.

Then dev_info() will result in "(NULL device *): " prefix.
That's why it is a similar construct as in uart_report_port() to
skip device name altogether in such case.

Best regards,
Maciej Szmigiero