[PATCH] serial: don't announce CIR serial ports

[PATCH] serial: don't announce CIR serial ports

[Maciej S. Szmigiero]

CIR type serial ports aren't real serial ports.
This is just a way to prevent legacy serial driver
from probing and eventually binding some resources
so don't announce them like normal serial ports.

Signed-off-by: Maciej Szmigiero <mail@maciej.szmigiero.name>
---
 drivers/tty/serial/serial_core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index f368520..99f944d 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2237,7 +2237,7 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
 		port->ops->config_port(port, flags);
 	}
 
-	if (port->type != PORT_UNKNOWN) {
+	if (port->type != PORT_UNKNOWN && port->type != PORT_8250_CIR) {
 		unsigned long flags;
 
 		uart_report_port(drv, port);

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

[Greg Kroah-Hartman]

On Sun, Aug 02, 2015 at 11:09:57PM +0200, Maciej S. Szmigiero wrote:
> CIR type serial ports aren't real serial ports.
> This is just a way to prevent legacy serial driver
> from probing and eventually binding some resources
> so don't announce them like normal serial ports.
> 
> Signed-off-by: Maciej Szmigiero <mail@maciej.szmigiero.name>
> ---
>  drivers/tty/serial/serial_core.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index f368520..99f944d 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -2237,7 +2237,7 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
>  		port->ops->config_port(port, flags);
>  	}
>  
> -	if (port->type != PORT_UNKNOWN) {
> +	if (port->type != PORT_UNKNOWN && port->type != PORT_8250_CIR) {
>  		unsigned long flags;
>  
>  		uart_report_port(drv, port);

This does not seem correct, why is this type of "port" somehow special
that it should be skiped?

thanks,

greg k-h

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

[Maciej S. Szmigiero]

On 04.08.2015 01:40, Greg Kroah-Hartman wrote:
> On Sun, Aug 02, 2015 at 11:09:57PM +0200, Maciej S. Szmigiero wrote:
>> CIR type serial ports aren't real serial ports.
>> This is just a way to prevent legacy serial driver
>> from probing and eventually binding some resources
>> so don't announce them like normal serial ports.
>>
>> Signed-off-by: Maciej Szmigiero <mail@maciej.szmigiero.name>
>> ---
>>  drivers/tty/serial/serial_core.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>> index f368520..99f944d 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -2237,7 +2237,7 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
>>  		port->ops->config_port(port, flags);
>>  	}
>>  
>> -	if (port->type != PORT_UNKNOWN) {
>> +	if (port->type != PORT_UNKNOWN && port->type != PORT_8250_CIR) {
>>  		unsigned long flags;
>>  
>>  		uart_report_port(drv, port);
> 
> This does not seem correct, why is this type of "port" somehow special
> that it should be skiped?

PORT_8250_CIR is not an actual serial port, it is a way to tell serial driver
to not really bind to some resources (I/O port, memory range, IRQ). 

The 8250 driver does scan a few predefined locations (I/O ports on x86) to
discover legacy serial ports there.

The problem is that some of devices that shouldn't been driven by 8250 driver
implement enough of serial port interface to be identified as a serial port
(and bound to) during this scan by this driver.
This prevents their native driver from binding to them since their resources
are already taken.

When a serial port has PORT_8250_CIR type the relevant resources won't be
reserved by 8250 driver and trying to use this port will result in ENODEV or
EIO (drivers/tty/serial/8250/8250_core.c:serial8250_do_startup()
returns -ENODEV unconditionally for such type of port).

Marking port as PORT_8250_CIR type is done by 8250 PNP driver which
will do this for a some of PNP IDs
(besides marking them it also won't bind to these devices).

Currently only Winbond CIR port PNP ID is on the list,
but I have also submitted other patch to add SMSC IR port too, as it has the
same problem.

Overall, I guess the announcement purpose is for user to tell him what serial
ports he has on the system, and by extension, which ones he can use.
Since user can't really use this type of serial port it would be nice to not
announce it.
That's why I've submitted this patch.

> thanks,
> 
> greg k-h
> 

Thanks and best regards,
Maciej Szmigiero

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

[Peter Hurley]

Hi Maciej,

On 08/02/2015 05:09 PM, Maciej S. Szmigiero wrote:
> CIR type serial ports aren't real serial ports.
> This is just a way to prevent legacy serial driver
> from probing and eventually binding some resources
> so don't announce them like normal serial ports.

I'd like to keep some form of reporting so that we know the
port was properly probed; what about extending uart_report_port()
to including CIR + disabled status?

Secondly, good catch! Because we should not be trying to
register a console on this port, nor driving modem signals.

So maybe an early exit after uart_report_port?

Regards,
Peter Hurley

> Signed-off-by: Maciej Szmigiero <mail@maciej.szmigiero.name>
> ---
>  drivers/tty/serial/serial_core.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index f368520..99f944d 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -2237,7 +2237,7 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
>  		port->ops->config_port(port, flags);
>  	}
>  
> -	if (port->type != PORT_UNKNOWN) {
> +	if (port->type != PORT_UNKNOWN && port->type != PORT_8250_CIR) {
>  		unsigned long flags;
>  
>  		uart_report_port(drv, port);

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

[Maciej S. Szmigiero]

Hi Peter,

Thanks for looking into it.

On 04.08.2015 03:46, Peter Hurley wrote:
> Hi Maciej,
> 
> On 08/02/2015 05:09 PM, Maciej S. Szmigiero wrote:
>> CIR type serial ports aren't real serial ports.
>> This is just a way to prevent legacy serial driver
>> from probing and eventually binding some resources
>> so don't announce them like normal serial ports.
> 
> I'd like to keep some form of reporting so that we know the
> port was properly probed; what about extending uart_report_port()
> to including CIR + disabled status?

Currently the printed message looks like this:
"00:01: ttyS2 at I/O 0x3e8 (irq = 7, base_baud = 115200) is a CIR port".

I think it would be best to skip a device file name in this case,
since this is how user sees (and uses) a real serial port.
The message would be then:
"00:01 at I/O 0x3e8 (irq = 7, base_baud = 115200) is a CIR port".

The dev name will always be present since the only current
"source" of CIR ports is PNP 8250 driver which sets 
dev pointer uncondtionally.

> Secondly, good catch! Because we should not be trying to
> register a console on this port, nor driving modem signals.
> 
> So maybe an early exit after uart_report_port?

All right, I will resubmit updated patch tomorrow.

> Regards,
> Peter Hurley

Best regards,
Maciej Szmigiero

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

[Peter Hurley]

On 08/04/2015 07:25 PM, Maciej S. Szmigiero wrote:
> Hi Peter,
> 
> Thanks for looking into it.
> 
> On 04.08.2015 03:46, Peter Hurley wrote:
>> Hi Maciej,
>>
>> On 08/02/2015 05:09 PM, Maciej S. Szmigiero wrote:
>>> CIR type serial ports aren't real serial ports.
>>> This is just a way to prevent legacy serial driver
>>> from probing and eventually binding some resources
>>> so don't announce them like normal serial ports.
>>
>> I'd like to keep some form of reporting so that we know the
>> port was properly probed; what about extending uart_report_port()
>> to including CIR + disabled status?
> 
> Currently the printed message looks like this:
> "00:01: ttyS2 at I/O 0x3e8 (irq = 7, base_baud = 115200) is a CIR port".
> 
> I think it would be best to skip a device file name in this case,
> since this is how user sees (and uses) a real serial port.
> The message would be then:
> "00:01 at I/O 0x3e8 (irq = 7, base_baud = 115200) is a CIR port".
> 
> The dev name will always be present since the only current
> "source" of CIR ports is PNP 8250 driver which sets 
> dev pointer uncondtionally.
> 
>> Secondly, good catch! Because we should not be trying to
>> register a console on this port, nor driving modem signals.
>>
>> So maybe an early exit after uart_report_port?
> 
> All right, I will resubmit updated patch tomorrow.

In re-reviewing this, I think the proper solution is actually not
to add the uart port for a CIR port at all. It doesn't make
sense because the tty cannot be changed by setserial/ioctl(TIOCSSERIAL),
so the device node serves no purpose.

This problem is really an artifact of the 8250 driver port management,
and shouldn't involve the serial core at all.

An additional benefit of this approach is that a simple one-line
banner noting the port skip could be emitted instead from
serial8250_register_8250_port().

Regards,
Peter Hurley

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

[Maciej S. Szmigiero]

On 05.08.2015 04:03, Peter Hurley wrote:
> On 08/04/2015 07:25 PM, Maciej S. Szmigiero wrote:
>> Hi Peter,
>>
>> Thanks for looking into it.
>>
>> On 04.08.2015 03:46, Peter Hurley wrote:
>>> Hi Maciej,
>>>
>>> On 08/02/2015 05:09 PM, Maciej S. Szmigiero wrote:
>>>> CIR type serial ports aren't real serial ports.
>>>> This is just a way to prevent legacy serial driver
>>>> from probing and eventually binding some resources
>>>> so don't announce them like normal serial ports.
>>>
>>> I'd like to keep some form of reporting so that we know the
>>> port was properly probed; what about extending uart_report_port()
>>> to including CIR + disabled status?
>>
>> Currently the printed message looks like this:
>> "00:01: ttyS2 at I/O 0x3e8 (irq = 7, base_baud = 115200) is a CIR port".
>>
>> I think it would be best to skip a device file name in this case,
>> since this is how user sees (and uses) a real serial port.
>> The message would be then:
>> "00:01 at I/O 0x3e8 (irq = 7, base_baud = 115200) is a CIR port".
>>
>> The dev name will always be present since the only current
>> "source" of CIR ports is PNP 8250 driver which sets 
>> dev pointer uncondtionally.
>>
>>> Secondly, good catch! Because we should not be trying to
>>> register a console on this port, nor driving modem signals.
>>>
>>> So maybe an early exit after uart_report_port?
>>
>> All right, I will resubmit updated patch tomorrow.
> 
> In re-reviewing this, I think the proper solution is actually not
> to add the uart port for a CIR port at all. It doesn't make
> sense because the tty cannot be changed by setserial/ioctl(TIOCSSERIAL),
> so the device node serves no purpose.
> 
> This problem is really an artifact of the 8250 driver port management,
> and shouldn't involve the serial core at all.
> 
> An additional benefit of this approach is that a simple one-line
> banner noting the port skip could be emitted instead from
> serial8250_register_8250_port().

That's a good idea, checks for PORT_8250_CIR in serial port
callbacks can be removed too this way, since these callbacks
won't ever be called.

I've submitted an updated patch.

> Regards,
> Peter Hurley
 
Best regards,
Maciej Szmigiero