[RFC 0/1] serdes: Add whitelist to bring back missing serial port

[RFC 0/1] serdes: Add whitelist to bring back missing serial port

[Punit Agrawal]

Hi,

While booting v5.5-rc1 on Apollo Lake based UP2[0], I ran into an
issue with the primary serial port. The kernel is able to output to
ttyS0 but systemd isn't able to raise a login prompt. On further
investigation, it turns out that no serial device (/dev/ttyS0) is
being created as the device is claimed by serdev sub-system.

The issue has been reported in a few different places[0][1]. A patch
was proposed to solve the issue but there doesn't seem to be any
further progress[2]. Feedback on the thread suggested implementing a
whitelist based approach - which is what this RFC does.

With this patch, systemd is able to create a login prompt. The
whitelist has intentionally been left blank as it's not clear which
devices go in there.

Feedback welcome.

Thanks,
Punit

[0] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=911831
[1] https://bugs.launchpad.net/ubuntu/+source/linux-oem/+bug/1769610
[2] https://marc.info/?l=linux-serial&m=152455861101408&w=2

Punit Agrawal (1):
  serdev: Only claim supported devices

 drivers/tty/serdev/core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

-- 
2.24.0

[RFC ] serdev: Only claim supported devices

[Punit Agrawal]

Serdev sub-system claims all serial devices that are not already
enumerated. As a result, no device node is created for serial port on
certain devices such as the Apollo Lake based UP2. This has the
unintended consequence of unable to raise the login prompt via serial
connection.

Introduce a whitelist to only register devices that are supported by
the sub-system.

Signed-off-by: Punit Agrawal <punit1.agrawal@toshiba.co.jp>
Cc: Rob Herring <robh@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Johan Hovold <johan@kernel.org>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 drivers/tty/serdev/core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index 226adeec2aed..0f414aa4d870 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -663,6 +663,10 @@ static acpi_status acpi_serdev_register_device(struct serdev_controller *ctrl,
 	return AE_OK;
 }
 
+static const struct acpi_device_id serdev_supported_devices[] = {
+	{ },
+};
+
 static acpi_status acpi_serdev_add_device(acpi_handle handle, u32 level,
 					  void *data, void **return_value)
 {
@@ -675,6 +679,10 @@ static acpi_status acpi_serdev_add_device(acpi_handle handle, u32 level,
 	if (acpi_device_enumerated(adev))
 		return AE_OK;
 
+	/* Skip if not supported */
+	if (acpi_match_device_ids(adev, serdev_supported_devices) == -ENOENT)
+		return AE_OK;
+
 	if (acpi_serdev_check_resources(ctrl, adev))
 		return AE_OK;
 
-- 
2.24.0

Re: [RFC 0/1] serdes: Add whitelist to bring back missing serial port

[Hans de Goede]

HI,

On 16-12-2019 05:08, Punit Agrawal wrote:
> 
> Hi,
> 
> While booting v5.5-rc1 on Apollo Lake based UP2[0], I ran into an
> issue with the primary serial port. The kernel is able to output to
> ttyS0 but systemd isn't able to raise a login prompt. On further
> investigation, it turns out that no serial device (/dev/ttyS0) is
> being created as the device is claimed by serdev sub-system.
> 
> The issue has been reported in a few different places[0][1]. A patch
> was proposed to solve the issue but there doesn't seem to be any
> further progress[2]. Feedback on the thread suggested implementing a
> whitelist based approach - which is what this RFC does.
> 
> With this patch, systemd is able to create a login prompt. The
> whitelist has intentionally been left blank as it's not clear which
> devices go in there.

As I already mentioned when discussing this upstream:

https://marc.info/?l=linux-serial&m=152460460418058&w=2

I am afraid that a whitelist is not going to fly, that means
duplicating all the device-ids in all the relevant drivers and that
everytime we add a device-id we need to do so in 2 places. Just take
a look at drivers/bluetooth/hci_bcm.c at the device-id list starting
at line 1187 and that is just 1 driver.

I also mention a hack for RTL8723BS devices there, but those have
gotten a proper in kernel driver in the mean time.

Looking at the ACPI device id list in the proposed upstream fix
with a "hsuart serdev driver":
https://www.spinics.net/lists/linux-serial/msg30035.html

+static const struct acpi_device_id hsuart_acpi_match[] = {
+	{"INT3511", 0},
+	{"INT3512", 0},
+	{ },
+};

Then blacklist with just these 2 ids would clearly be a much better
approach, as we are talking 2 ids vs 50+ ids here for whitelist vs
blacklist.

The whitepaper on this:
https://www.intel.com/content/dam/www/public/us/en/documents/white-papers/enabling-multi-com-port-white-paper.pdf
Also mentions these 2 as "the default Hardware IDs (INT3511 and INT3512) used here are Intel HS-UART COM
port peripheral device IDs." as the hardware ids to use if the port has no
specific function, in other words to use these 2 ids when under Linux the
serial-port should just show up as a /dev/ttyS* device.

So I believe that the fix here is using a blacklist with just these 2
ids in there.

Regards,

Hans

Re: [RFC 0/1] serdes: Add whitelist to bring back missing serial port

[Punit Agrawal]

Hi Hans,

Thanks for chiming in.

Hans de Goede <hdegoede@redhat.com> writes:

> HI,
>
> On 16-12-2019 05:08, Punit Agrawal wrote:
>>
>> Hi,
>>
>> While booting v5.5-rc1 on Apollo Lake based UP2[0], I ran into an
>> issue with the primary serial port. The kernel is able to output to
>> ttyS0 but systemd isn't able to raise a login prompt. On further
>> investigation, it turns out that no serial device (/dev/ttyS0) is
>> being created as the device is claimed by serdev sub-system.
>>
>> The issue has been reported in a few different places[0][1]. A patch
>> was proposed to solve the issue but there doesn't seem to be any
>> further progress[2]. Feedback on the thread suggested implementing a
>> whitelist based approach - which is what this RFC does.
>>
>> With this patch, systemd is able to create a login prompt. The
>> whitelist has intentionally been left blank as it's not clear which
>> devices go in there.
>
> As I already mentioned when discussing this upstream:
>
> https://marc.info/?l=linux-serial&m=152460460418058&w=2
>
> I am afraid that a whitelist is not going to fly, that means
> duplicating all the device-ids in all the relevant drivers and that
> everytime we add a device-id we need to do so in 2 places. Just take
> a look at drivers/bluetooth/hci_bcm.c at the device-id list starting
> at line 1187 and that is just 1 driver.

I had seen the linked mail but was missing the context given here. I am
not that familiar with the serial devices framework.

>
> I also mention a hack for RTL8723BS devices there, but those have
> gotten a proper in kernel driver in the mean time.
>
> Looking at the ACPI device id list in the proposed upstream fix
> with a "hsuart serdev driver":
> https://www.spinics.net/lists/linux-serial/msg30035.html
>
> +static const struct acpi_device_id hsuart_acpi_match[] = {
> +	{"INT3511", 0},
> +	{"INT3512", 0},
> +	{ },
> +};
>
> Then blacklist with just these 2 ids would clearly be a much better
> approach, as we are talking 2 ids vs 50+ ids here for whitelist vs
> blacklist.
>
> The whitepaper on this:
> https://www.intel.com/content/dam/www/public/us/en/documents/white-papers/enabling-multi-com-port-white-paper.pdf
> Also mentions these 2 as "the default Hardware IDs (INT3511 and INT3512) used here are Intel HS-UART COM
> port peripheral device IDs." as the hardware ids to use if the port has no
> specific function, in other words to use these 2 ids when under Linux the
> serial-port should just show up as a /dev/ttyS* device.
>
> So I believe that the fix here is using a blacklist with just these 2
> ids in there.

That makes sense.

A shorter list of exceptions is better than the longer list of supported
device list that is going to be duplicated.

I will respin the patches taking the blacklist approach if there is no
other feedback.

Thanks,
Punit