* Re: [PATCH] serdev: Update drivers/tty/serdev/Kconfig for ACPI support
From: Ian W MORRISON @ 2017-10-08 3:20 UTC (permalink / raw)
To: Marcel Holtmann, Johan Hovold, Gustavo F. Padovan, Johan Hedberg,
bluez mailin list (linux-bluetooth@vger.kernel.org),
Hans de Goede, Frédéric Danis, Rob Herring,
Sebastian Reichel, Loic Poulain, Lukas Wunner, linux-serial,
linux-acpi, Greg Kroah-Hartman, Rafael J. Wysocki
In-Reply-To: <8D8274B0-4EF6-41A0-8B76-95023F6DD26C@holtmann.org>
On 10/8/17 6:57 AM, Marcel Holtmann wrote:
> Hi Johan,
>
>>> The current Kconfig for serdev is not compatible when adding ACPI support as it does not work when built as a module as it requires config SERIAL_DEV_CTRL_TTYPORT to be set. This patch makes serdev compiled into the kernel if selected so that config SERIAL_DEV_CTRL_TTYPORT can be correctly set if requiring ACPI support.
<snip>
>> I understand why you want this (to prevent hci_bcm from breaking), but we
>> should generally not have new entries default to y.
>>
<snip>
>> It may be better to have BT_HCIUART_BCM depend on (or select?)
>> SERIAL_DEV_CTRL_TTYPORT instead.
>
> if we move SERIAL_DEV_BUS to bool, then I would just have it be selected by BT_HCIUART_BCM. Frankly the SERIAL_DEV_BUS option is pretty hard to find in the kernel config. And if we depend on TTY, but then select SERIAL_DEV_BUS, I think that is a good compromise.
>
> Regards
>
> Marcel
>
Hi,
Many thanks for everyone's comments on my earlier patch to Kconfigs for ACPI serdev support.
I've submitted a revised patch set which addresses the points raised by making BT_HCIUART_BCM dependent on SERIAL_DEV_CTRL_TTYPORT which in turn is dependent on SERIAL_DEV_BUS and ensures that if SERIAL_DEV_BUS is selected is the code is build it.
Please can you review and let me know if any further changes are required?
Regards,
Ian
^ permalink raw reply
* Re: [PATCH v2 1/2] serdev: Update drivers/tty/serdev/Kconfig for ACPI serdev support
From: Greg KH @ 2017-10-08 7:26 UTC (permalink / raw)
To: Ian W MORRISON
Cc: Marcel Holtmann, Johan Hovold, Gustavo F. Padovan, Johan Hedberg,
bluez mailin list (linux-bluetooth@vger.kernel.org), hdegoede,
frederic.danis.oss, robh, sre, Loic Poulain, lukas, linux-serial,
linux-acpi, rafael
In-Reply-To: <82b477ef-757a-c953-45e8-fd68b28286e5@gmail.com>
On Sun, Oct 08, 2017 at 02:20:07PM +1100, Ian W MORRISON wrote:
> ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented for serdev. This is only possible if serdev support is compiled in as the code hooks into TTY. Otherwise PM will silently break as the corresponding platform devices would no longer be registered and as the tty class device is also gone and hciattach (btattach) will also fail.
>
> This patch set addresses this by making BT_HCIUART_BCM dependent on SERIAL_DEV_CTRL_TTYPORT which in turn is dependent on SERIAL_DEV_BUS and ensures that if SERIAL_DEV_BUS is selected is the code is build it.
Please properly line-wrap your changelog comments like your editor is
asking you to do...
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 2/2] ACPI / scan: Fix enumeration for special UART devices
From: Marcel Holtmann @ 2017-10-08 8:51 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Johan Hovold, Frédéric Danis, Rob Herring, Loic Poulain,
Lukas Wunner, Hans de Goede,
bluez mailin list (linux-bluetooth-u79uwXL29TY76Z2rM5mHXA@public.gmane.org),
linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20171007225311.lvli7rkhv3bebq2j@earth>
Hi Sebastian,
>>> UART devices is expected to be enumerated by SerDev subsystem.
>>>
>>> During ACPI scan, serial devices behind SPI, I2C or UART buses are not
>>> enumerated, allowing them to be enumerated by their respective parents.
>>>
>>> Rename *spi_i2c_slave* to *serial_bus_slave* as this will be used for serial
>>> devices on serial buses (SPI, I2C or UART).
>>>
>>> On Macs an empty ResourceTemplate is returned for uart slaves.
>>> Instead the device properties "baud", "parity", "dataBits", "stopBits" are
>>> provided. Add a check for "baud" in acpi_is_serial_bus_slave().
>>>
>>> Signed-off-by: Frédéric Danis <frederic.danis.oss-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> So just to reiterate what I just mentioned in a comment to one of Hans's
>> hci_bcm patches:
>>
>> This one would silently break PM for such devices on any system which
>> does not have serdev enabled (as the corresponding platform devices
>> would no longer be registered). And with serdev enabled, hciattach
>> (btattach) would start failing as the tty device would no longer be
>> registered (but I assume everyone is aware of that, and fine with it, by
>> now).
>>
>> Perhaps the hci_bcm driver should start depending on
>> SERIAL_DEV_CTRL_TTYPORT when ACPI is enabled?
>
> ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly,
> since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented
> for serdev. If any other controller is implemented that one could
> also be used.
>
> I wonder if we should just hide SERIAL_DEV_CTRL_TTYPORT and enable
> it together with SERDEV. I suspect that we won't see any other
> controller (it would be a UART device, that is not registered as
> tty device) in the next few years and the extra option seems to
> confuse people.
I wonder if we should just default SERIAL_DEV_CTRL_TTYPORT=y when DT or ACPI is enabled. Then no driver would have to select or depend on it.
Regards
Marcel
^ permalink raw reply
* [PATCH v3 1/2] serdev: Update drivers/tty/serdev/Kconfig for ACPI serdev support
From: Ian W MORRISON @ 2017-10-09 0:43 UTC (permalink / raw)
To: Marcel Holtmann, Johan Hovold, Gustavo F. Padovan, Johan Hedberg,
bluez mailin list (linux-bluetooth-u79uwXL29TY76Z2rM5mHXA@public.gmane.org),
hdegoede-H+wXaHxf7aLQT0dZR+AlfA,
frederic.danis.oss-Re5JQEeQqe8AvxtiuMwx3w,
robh-DgEjT+Ai2ygdnm+yROfE0A, sre-DgEjT+Ai2ygdnm+yROfE0A,
Loic Poulain, lukas-JFq808J9C/izQB+pC5nmwQ,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA, rafael-DgEjT+Ai2ygdnm+yROfE0A,
Greg Kroah-Hartman
In-Reply-To: <82b477ef-757a-c953-45e8-fd68b28286e5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly
since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented for
serdev. This is only possible if serdev support is compiled in as the code
hooks into TTY. Otherwise PM will silently break as the corresponding
platform devices would no longer be registered and as the tty class
device is also gone and hciattach (btattach) will also fail.
This patch set addresses this by making BT_HCIUART_BCM dependent on
SERIAL_DEV_CTRL_TTYPORT which in turn is dependent on SERIAL_DEV_BUS
and ensures that if SERIAL_DEV_BUS is selected is the code is build it.
Signed-off-by: Ian W MORRISON <ianwmorrison-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
drivers/tty/serdev/Kconfig | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serdev/Kconfig b/drivers/tty/serdev/Kconfig
index cdc6b820cf93..1144f4db5087 100644
--- a/drivers/tty/serdev/Kconfig
+++ b/drivers/tty/serdev/Kconfig
@@ -2,7 +2,7 @@
# Serial bus device driver configuration
#
menuconfig SERIAL_DEV_BUS
- tristate "Serial device bus"
+ bool "Serial device bus"
help
Core support for devices connected via a serial port.
@@ -11,6 +11,6 @@ if SERIAL_DEV_BUS
config SERIAL_DEV_CTRL_TTYPORT
bool "Serial device TTY port controller"
depends on TTY
- depends on SERIAL_DEV_BUS != m
+ depends on SERIAL_DEV_BUS
endif
--
2.11.0
^ permalink raw reply related
* [PATCH v3 2/2] serdev: Update drivers/bluetooth/Kconfig for ACPI serdev support
From: Ian W MORRISON @ 2017-10-09 0:43 UTC (permalink / raw)
To: Marcel Holtmann, Johan Hovold, gustavo-THi1TnShQwVAfugRpC6u6w,
johan.hedberg-Re5JQEeQqe8AvxtiuMwx3w,
linux-bluetooth-u79uwXL29TY76Z2rM5mHXA, Hans de Goede,
frederic.danis.oss-Re5JQEeQqe8AvxtiuMwx3w,
robh-DgEjT+Ai2ygdnm+yROfE0A, sre-DgEjT+Ai2ygdnm+yROfE0A,
loic.poulain-Re5JQEeQqe8AvxtiuMwx3w, Lukas Wunner,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA, rafael-DgEjT+Ai2ygdnm+yROfE0A,
Greg Kroah-Hartman
In-Reply-To: <dc9cecc0-d9ef-109f-5f05-cf77ab16e60e-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly
since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented for
serdev. This is only possible if serdev support is compiled in as the code
hooks into TTY. Otherwise PM will silently break as the corresponding
platform devices would no longer be registered and as the tty class
device is also gone and hciattach (btattach) will also fail.
This patch set addresses this by making BT_HCIUART_BCM dependent on
SERIAL_DEV_CTRL_TTYPORT which in turn is dependent on SERIAL_DEV_BUS
and ensures that if SERIAL_DEV_BUS is selected is the code is build it.
Signed-off-by: Ian W MORRISON <ianwmorrison-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
drivers/bluetooth/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index fae5a74dc737..8d432ee9f4bd 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -171,6 +171,7 @@ config BT_HCIUART_BCM
depends on BT_HCIUART_SERDEV
select BT_HCIUART_H4
select BT_BCM
+ select SERIAL_DEV_CTRL_TTYPORT
help
The Broadcom protocol support enables Bluetooth HCI over serial
port interface for Broadcom Bluetooth controllers.
--
2.11.0
^ permalink raw reply related
* Re: [PATCH v2 1/2] serdev: Update drivers/tty/serdev/Kconfig for ACPI serdev support
From: Ian W MORRISON @ 2017-10-09 0:43 UTC (permalink / raw)
To: Greg KH
Cc: Marcel Holtmann, Johan Hovold, Gustavo F. Padovan, Johan Hedberg,
bluez mailin list (linux-bluetooth-u79uwXL29TY76Z2rM5mHXA@public.gmane.org),
hdegoede-H+wXaHxf7aLQT0dZR+AlfA,
frederic.danis.oss-Re5JQEeQqe8AvxtiuMwx3w,
robh-DgEjT+Ai2ygdnm+yROfE0A, sre-DgEjT+Ai2ygdnm+yROfE0A,
Loic Poulain, lukas-JFq808J9C/izQB+pC5nmwQ,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA, rafael-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <20171008072655.GA3444-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
On 10/8/17 6:26 PM, Greg KH wrote:
<snip>
>
> Please properly line-wrap your changelog comments like your editor is
> asking you to do...
>
> thanks,
>
> greg k-h
>
Hi Greg,
Sorry, I had a brain snap. I purposely removed all the hard line feeds thinking I was sending an email with auto wrap completely forgetting that I was sending a patch with a changelog. I've resent them as v3.
Regards,
Ian
^ permalink raw reply
* Re: [PATCH 04/16] serial: mvebu-uart: support probe of multiple ports
From: Miquel RAYNAL @ 2017-10-09 7:17 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Greg Kroah-Hartman, Linus Walleij, Jason Cooper, Andrew Lunn,
Sebastian Hesselbarth, Jiri Slaby, Catalin Marinas, Will Deacon,
Thomas Petazzoni, devicetree-u79uwXL29TY76Z2rM5mHXA, Allen Yan,
Antoine Tenart, Nadav Haklai, linux-gpio-u79uwXL29TY76Z2rM5mHXA,
linux-serial-u79uwXL29TY76Z2rM5mHXA, Wilson Ding,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <87poa0foro.fsf-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
On Fri, 06 Oct 2017 14:23:55 +0200
Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Hi Miquel,
>
> On ven., oct. 06 2017, Miquel Raynal
> <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>
> > From: Allen Yan <yanwei-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> >
> > Until now, the mvebu-uart driver only supported probing a single
> > UART port. However, some platforms have multiple instances of this
> > UART controller, and therefore the driver should support multiple
> > ports.
> >
> > In order to achieve this, we make sure to assign port->line
> > properly, instead of hardcoding it to zero.
> >
> > Signed-off-by: Allen Yan <yanwei-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> > Signed-off-by: Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> > drivers/tty/serial/mvebu-uart.c | 13 +++++++++++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/tty/serial/mvebu-uart.c
> > b/drivers/tty/serial/mvebu-uart.c index 7e0a3e9fee15..25b11ede3a97
> > 100644 --- a/drivers/tty/serial/mvebu-uart.c
> > +++ b/drivers/tty/serial/mvebu-uart.c
> > @@ -560,7 +560,16 @@ static int mvebu_uart_probe(struct
> > platform_device *pdev) return -EINVAL;
> > }
> >
> > - port = &mvebu_uart_ports[0];
> > + if (pdev->dev.of_node)
> > + pdev->id = of_alias_get_id(pdev->dev.of_node,
> > "serial");
>
> If the id is retrieved using an of_ function, then I think that the
> driver would depend on OF_CONFIG.
Is this okay?
if (pdev->dev.of_node && IS_ENABLED(CONFIG_OF))
pdev->id = of_alias_get_id(pdev->dev.of_node, "serial");
else
pdev->id = 0;
BTW, I could not test without CONFIG_OF as it is defined by default in
our case: Selected by: ARM64 [=y]
I don't think there will be a 32-bit SoC with this UART IP?
Thanks,
Miquèl
>
> Gregory
>
>
> > +
> > + if (pdev->id >= MVEBU_NR_UARTS) {
> > + dev_err(&pdev->dev, "cannot have more than %d UART
> > ports\n",
> > + MVEBU_NR_UARTS);
> > + return -EINVAL;
> > + }
> > +
> > + port = &mvebu_uart_ports[pdev->id];
> >
> > spin_lock_init(&port->lock);
> >
> > @@ -572,7 +581,7 @@ static int mvebu_uart_probe(struct
> > platform_device *pdev) port->fifosize = 32;
> > port->iotype = UPIO_MEM32;
> > port->flags = UPF_FIXED_PORT;
> > - port->line = 0; /* single port: force line number
> > to 0 */
> > + port->line = pdev->id;
> >
> > port->irq = irq->start;
> > port->irqflags = 0;
> > --
> > 2.11.0
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
Miquel Raynal, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 16/16] arm64: dts: marvell: armada-3720-espressobin: fill UART nodes
From: Miquel RAYNAL @ 2017-10-09 7:30 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: Gregory CLEMENT, Greg Kroah-Hartman, Linus Walleij, Jason Cooper,
Andrew Lunn, Sebastian Hesselbarth, Jiri Slaby, Catalin Marinas,
Will Deacon, linux-serial, devicetree, linux-arm-kernel,
linux-gpio, Antoine Tenart, Nadav Haklai, Wilson Ding
In-Reply-To: <20171006151521.440418a2@windsurf.lan>
Hi Thomas, Gregory,
On Fri, 6 Oct 2017 15:15:21 +0200
Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:
> Hello,
>
> On Fri, 06 Oct 2017 15:01:18 +0200, Gregory CLEMENT wrote:
>
> > /*
> > * To enable the second UART on J17 (pins 24,26) refer to the uart1
> > * node from armada-3720-db.dts.
> > * Note that TX and RX signal are the ones coming directly from
> > the SoC:
> > * 1.8V TTL.
> > */
>
> One issue with this comment (and Miquèl's version as well) is that it
> does not explain why you don't enable this UART by default.
>
> The real reason is in the commit log from Miquèl, and should probably
> be part of the comment. Perhaps something like:
>
> /*
>
> * Connector J17 (pins X, Y, Z) exposes a number of different
> * features:
> * - UART1 (pins 24 = RX, pins 26 = TX), see armada-3720-db.dts for
> an
> * example on how to enable UART1. Beware that the signals are 1.8V
> * TTL.
> * - SPIxyz
> * - I2Cxyz
> */
Thanks for both your comments, there is my version, inspired from both
comments:
/*
* Connector J17 exposes a number of different features. Some pins are
* multiplexed. This is the case for the UART1 feature (pins 24 = RX,
* pins 26 = TX). See armada-3720-db.dts for an example of how to enable it.
* Beware that the signals are 1.8V TTL.
*/
Thanks,
Miquèl
>
> Otherwise, it's not clear at all why you don't just enable UART1. Or
> perhaps I misunderstood Miquèl's commit log ?
>
> Best regards,
>
> Thomas
--
Miquel Raynal, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH 2/2] ACPI / scan: Fix enumeration for special UART devices
From: Johan Hovold @ 2017-10-09 7:35 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Johan Hovold, Frédéric Danis, robh, marcel,
loic.poulain, lukas, hdegoede, linux-bluetooth, linux-serial,
linux-acpi
In-Reply-To: <20171007225311.lvli7rkhv3bebq2j@earth>
On Sun, Oct 08, 2017 at 12:53:11AM +0200, Sebastian Reichel wrote:
> Hi,
>
> On Sat, Oct 07, 2017 at 05:19:34PM +0200, Johan Hovold wrote:
> > On Wed, Oct 04, 2017 at 10:51:30AM +0200, Frédéric Danis wrote:
> > > UART devices is expected to be enumerated by SerDev subsystem.
> > >
> > > During ACPI scan, serial devices behind SPI, I2C or UART buses are not
> > > enumerated, allowing them to be enumerated by their respective parents.
> > >
> > > Rename *spi_i2c_slave* to *serial_bus_slave* as this will be used for serial
> > > devices on serial buses (SPI, I2C or UART).
> > >
> > > On Macs an empty ResourceTemplate is returned for uart slaves.
> > > Instead the device properties "baud", "parity", "dataBits", "stopBits" are
> > > provided. Add a check for "baud" in acpi_is_serial_bus_slave().
> > >
> > > Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
> >
> > So just to reiterate what I just mentioned in a comment to one of Hans's
> > hci_bcm patches:
> >
> > This one would silently break PM for such devices on any system which
> > does not have serdev enabled (as the corresponding platform devices
> > would no longer be registered). And with serdev enabled, hciattach
> > (btattach) would start failing as the tty device would no longer be
> > registered (but I assume everyone is aware of that, and fine with it, by
> > now).
> >
> > Perhaps the hci_bcm driver should start depending on
> > SERIAL_DEV_CTRL_TTYPORT when ACPI is enabled?
>
> ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly,
> since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented
> for serdev. If any other controller is implemented that one could
> also be used.
Not for hci_bcm, right? This particular driver specifically depends on
SERIAL_DEV_CTRL_TTYPORT for the ACPI devices and not just any (future)
serdev controller (or currently working systems soon breaks silently).
I don't think the same is true for the DT case where we do not already
have child nodes defined in firmware (and in fact, this driver did not
really support DT before serdev).
> I wonder if we should just hide SERIAL_DEV_CTRL_TTYPORT and enable
> it together with SERDEV. I suspect that we won't see any other
> controller (it would be a UART device, that is not registered as
> tty device) in the next few years and the extra option seems to
> confuse people.
I agree that it is somewhat confusing. But now that we have both,
perhaps simply having SERIAL_DEV_CTRL_TTYPORT default to "y" when
SERIAL_DEV_BUS is selected could be a compromise. The Kconfig entry
might need to be amended as well (e.g. if only to mention that you need
to select a controller as well).
And the bluetooth uart drivers already depend on SERIAL_DEV_BUS.
Johan
^ permalink raw reply
* Re: [PATCH] serdev: Update drivers/tty/serdev/Kconfig for ACPI support
From: Johan Hovold @ 2017-10-09 7:47 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Johan Hovold, Ian W MORRISON, Gustavo F. Padovan, Johan Hedberg,
bluez mailin list (linux-bluetooth@vger.kernel.org),
Hans de Goede, Frédéric Danis, Rob Herring,
Sebastian Reichel, Loic Poulain, Lukas Wunner, linux-serial,
linux-acpi, Greg Kroah-Hartman, Rafael J. Wysocki
In-Reply-To: <8D8274B0-4EF6-41A0-8B76-95023F6DD26C@holtmann.org>
On Sat, Oct 07, 2017 at 09:57:40PM +0200, Marcel Holtmann wrote:
> Hi Johan,
>
> >> The current Kconfig for serdev is not compatible when adding ACPI support as it does not work when built as a module as it requires config SERIAL_DEV_CTRL_TTYPORT to be set. This patch makes serdev compiled into the kernel if selected so that config SERIAL_DEV_CTRL_TTYPORT can be correctly set if requiring ACPI support.
> >> ---
> >> drivers/tty/serdev/Kconfig | 5 +++--
> >> 1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/tty/serdev/Kconfig b/drivers/tty/serdev/Kconfig
> >> index cdc6b820cf93..a9fb09a9c105 100644
> >> --- a/drivers/tty/serdev/Kconfig
> >> +++ b/drivers/tty/serdev/Kconfig
> >> @@ -2,7 +2,8 @@
> >> # Serial bus device driver configuration
> >> #
> >> menuconfig SERIAL_DEV_BUS
> >> - tristate "Serial device bus"
> >> + bool "Serial device bus"
> >> + default y
> >
> > I understand why you want this (to prevent hci_bcm from breaking), but we
> > should generally not have new entries default to y.
> >
> >> help
> >> Core support for devices connected via a serial port.
> >>
> >> @@ -11,6 +12,6 @@ if SERIAL_DEV_BUS
> >> config SERIAL_DEV_CTRL_TTYPORT
> >> bool "Serial device TTY port controller"
> >> depends on TTY
> >> - depends on SERIAL_DEV_BUS != m
> >> + default y
> >
> > Same here.
> >
> > It may be better to have BT_HCIUART_BCM depend on (or select?)
> > SERIAL_DEV_CTRL_TTYPORT instead.
>
> if we move SERIAL_DEV_BUS to bool, then I would just have it be
> selected by BT_HCIUART_BCM. Frankly the SERIAL_DEV_BUS option is
> pretty hard to find in the kernel config. And if we depend on TTY, but
> then select SERIAL_DEV_BUS, I think that is a good compromise.
The Bluetooth UART drivers already depend on on SERIAL_DEV_BUS (through
BT_HCIUART_SERDEV). And the problem with BT_HCIUART_BCM is that the
current ACPI devices really need SERIAL_DEV_CTRL_TTYPORT (and not just
serdev core).
And we should probably try to avoid selecting options if we can (to
avoid ending up with unmet dependencies).
Johan
^ permalink raw reply
* Re: [PATCH v3 1/2] serdev: Update drivers/tty/serdev/Kconfig for ACPI serdev support
From: Johan Hovold @ 2017-10-09 7:54 UTC (permalink / raw)
To: Ian W MORRISON
Cc: Marcel Holtmann, Johan Hovold, Gustavo F. Padovan, Johan Hedberg,
bluez mailin list (linux-bluetooth@vger.kernel.org), hdegoede,
frederic.danis.oss, robh, sre, Loic Poulain, lukas, linux-serial,
linux-acpi, rafael, Greg Kroah-Hartman
In-Reply-To: <7bdc8a8a-1a36-0494-ed0c-6e2052fb53d7@gmail.com>
On Mon, Oct 09, 2017 at 11:43:19AM +1100, Ian W MORRISON wrote:
> ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly
> since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented for
> serdev. This is only possible if serdev support is compiled in as the code
> hooks into TTY. Otherwise PM will silently break as the corresponding
> platform devices would no longer be registered and as the tty class
> device is also gone and hciattach (btattach) will also fail.
You have quoted and a few us in the sentences above, but I'm afraid the
end result is not correct (e.g. this patch has nothing to do with
platform devices being registered or not).
Pleas try to explain why you think this change is necessary with your
own words.
> This patch set addresses this by making BT_HCIUART_BCM dependent on
> SERIAL_DEV_CTRL_TTYPORT which in turn is dependent on SERIAL_DEV_BUS
> and ensures that if SERIAL_DEV_BUS is selected is the code is build it.
This patch does not touch BT_HCIUART_BCM, so the above paragraph is more
suited for a cover letter.
Also when resubmitting a patch series, make it self-contained rather
than send individual patches in response to their earlier counterparts
(which messes up threading).
> Signed-off-by: Ian W MORRISON <ianwmorrison@gmail.com>
> ---
> drivers/tty/serdev/Kconfig | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serdev/Kconfig b/drivers/tty/serdev/Kconfig
> index cdc6b820cf93..1144f4db5087 100644
> --- a/drivers/tty/serdev/Kconfig
> +++ b/drivers/tty/serdev/Kconfig
> @@ -2,7 +2,7 @@
> # Serial bus device driver configuration
> #
> menuconfig SERIAL_DEV_BUS
> - tristate "Serial device bus"
> + bool "Serial device bus"
So please explain why you think this is needed. I'm not sure it's
necessary.
> help
> Core support for devices connected via a serial port.
>
> @@ -11,6 +11,6 @@ if SERIAL_DEV_BUS
> config SERIAL_DEV_CTRL_TTYPORT
> bool "Serial device TTY port controller"
> depends on TTY
> - depends on SERIAL_DEV_BUS != m
> + depends on SERIAL_DEV_BUS
>
> endif
Johan
^ permalink raw reply
* Re: [PATCH v3 2/2] serdev: Update drivers/bluetooth/Kconfig for ACPI serdev support
From: Johan Hovold @ 2017-10-09 8:06 UTC (permalink / raw)
To: Ian W MORRISON
Cc: Marcel Holtmann, Johan Hovold, gustavo-THi1TnShQwVAfugRpC6u6w,
johan.hedberg-Re5JQEeQqe8AvxtiuMwx3w,
linux-bluetooth-u79uwXL29TY76Z2rM5mHXA, Hans de Goede,
frederic.danis.oss-Re5JQEeQqe8AvxtiuMwx3w,
robh-DgEjT+Ai2ygdnm+yROfE0A, sre-DgEjT+Ai2ygdnm+yROfE0A,
loic.poulain-Re5JQEeQqe8AvxtiuMwx3w, Lukas Wunner,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA, rafael-DgEjT+Ai2ygdnm+yROfE0A,
Greg Kroah-Hartman
In-Reply-To: <427a95f6-feda-e93e-9a0f-a05b7b339ee6-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Mon, Oct 09, 2017 at 11:43:31AM +1100, Ian W MORRISON wrote:
> ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly
> since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented for
> serdev. This is only possible if serdev support is compiled in as the code
> hooks into TTY. Otherwise PM will silently break as the corresponding
> platform devices would no longer be registered and as the tty class
> device is also gone and hciattach (btattach) will also fail.
>
> This patch set addresses this by making BT_HCIUART_BCM dependent on
> SERIAL_DEV_CTRL_TTYPORT which in turn is dependent on SERIAL_DEV_BUS
> and ensures that if SERIAL_DEV_BUS is selected is the code is build it.
Ok, so you didn't even bother to write two distinct commit messages for
your two patches, and my comments to the first patch apply also here.
> Signed-off-by: Ian W MORRISON <ianwmorrison-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> drivers/bluetooth/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index fae5a74dc737..8d432ee9f4bd 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -171,6 +171,7 @@ config BT_HCIUART_BCM
> depends on BT_HCIUART_SERDEV
> select BT_HCIUART_H4
> select BT_BCM
> + select SERIAL_DEV_CTRL_TTYPORT
I think
depends on (!ACPI || SERIAL_DEV_CTRL_TTYPORT)
or just
depends on SERIAL_DEV_CTRL_TTYPORT
is what we want here, but that's still being discussed.
[ In general, select should be avoided as you could end up with options
enabled without their dependencies also being enabled. In this case,
we're fine as BT_HCIUART_BCM as depends on SERIAL_DEV_BUS via
BT_HCIUART_SERDEV, and your first patch ruled out SERIAL_DEV_BUS=y, but
that's not obvious. ]
> help
> The Broadcom protocol support enables Bluetooth HCI over serial
> port interface for Broadcom Bluetooth controllers.
Johan
^ permalink raw reply
* Re: [PATCH 2/2] ACPI / scan: Fix enumeration for special UART devices
From: Sebastian Reichel @ 2017-10-09 8:55 UTC (permalink / raw)
To: Johan Hovold
Cc: Frédéric Danis, robh, marcel, loic.poulain, lukas,
hdegoede, linux-bluetooth, linux-serial, linux-acpi
In-Reply-To: <20171009073526.GL2618@localhost>
[-- Attachment #1: Type: text/plain, Size: 4062 bytes --]
Hi,
On Mon, Oct 09, 2017 at 09:35:26AM +0200, Johan Hovold wrote:
> On Sun, Oct 08, 2017 at 12:53:11AM +0200, Sebastian Reichel wrote:
> > On Sat, Oct 07, 2017 at 05:19:34PM +0200, Johan Hovold wrote:
> > > On Wed, Oct 04, 2017 at 10:51:30AM +0200, Frédéric Danis wrote:
> > > > UART devices is expected to be enumerated by SerDev subsystem.
> > > >
> > > > During ACPI scan, serial devices behind SPI, I2C or UART buses are not
> > > > enumerated, allowing them to be enumerated by their respective parents.
> > > >
> > > > Rename *spi_i2c_slave* to *serial_bus_slave* as this will be used for serial
> > > > devices on serial buses (SPI, I2C or UART).
> > > >
> > > > On Macs an empty ResourceTemplate is returned for uart slaves.
> > > > Instead the device properties "baud", "parity", "dataBits", "stopBits" are
> > > > provided. Add a check for "baud" in acpi_is_serial_bus_slave().
> > > >
> > > > Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
> > >
> > > So just to reiterate what I just mentioned in a comment to one of Hans's
> > > hci_bcm patches:
> > >
> > > This one would silently break PM for such devices on any system which
> > > does not have serdev enabled (as the corresponding platform devices
> > > would no longer be registered). And with serdev enabled, hciattach
> > > (btattach) would start failing as the tty device would no longer be
> > > registered (but I assume everyone is aware of that, and fine with it, by
> > > now).
> > >
> > > Perhaps the hci_bcm driver should start depending on
> > > SERIAL_DEV_CTRL_TTYPORT when ACPI is enabled?
> >
> > ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly,
> > since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented
> > for serdev. If any other controller is implemented that one could
> > also be used.
>
> Not for hci_bcm, right? This particular driver specifically depends on
> SERIAL_DEV_CTRL_TTYPORT for the ACPI devices and not just any (future)
> serdev controller (or currently working systems soon breaks silently).
>
> I don't think the same is true for the DT case where we do not already
> have child nodes defined in firmware (and in fact, this driver did not
> really support DT before serdev).
The serdev ACPI support has been added to the core and not to
the ttyport and the hci_bcm driver only uses functions from the
core. As far as I can see the ACPI part would also work fine with
a different serdev controller.
Of course DT and ACPI currently require SERIAL_DEV_CTRL_TTYPORT,
since it's the only serdev controller implementation. Also it
covers most use cases. When SERIAL_DEV_BUS is selected it's
very likely, that you also want SERIAL_DEV_CTRL_TTYPORT.
> > I wonder if we should just hide SERIAL_DEV_CTRL_TTYPORT and enable
> > it together with SERDEV. I suspect that we won't see any other
> > controller (it would be a UART device, that is not registered as
> > tty device) in the next few years and the extra option seems to
> > confuse people.
>
> I agree that it is somewhat confusing. But now that we have both,
> perhaps simply having SERIAL_DEV_CTRL_TTYPORT default to "y" when
> SERIAL_DEV_BUS is selected could be a compromise. The Kconfig entry
> might need to be amended as well (e.g. if only to mention that you
> need to select a controller as well).
I think we should at least add a default "y" if SERIAL_DEV_BUS.
> And the bluetooth uart drivers already depend on SERIAL_DEV_BUS.
Yes and that's the correct dependency. They only need the serdev
core and controller. The only reason they do not work without
SERIAL_DEV_CTRL_TTYPORT is, that there won't be any serdev
controller.
Note, that the default "y" if SERIAL_DEV_BUS in SERIAL_DEV_CTRL_TTYPORT's
config entry is only a partial fix. There is still the problem,
that SERIAL_DEV_CTRL_TTYPORT can only be enabled if SERIAL_DEV_BUS
is configured builtin. This is a limitation of the ttyport
implementation, that hooks into builtin TTY core code.
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] ACPI / scan: Fix enumeration for special UART devices
From: Sebastian Reichel @ 2017-10-09 8:59 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Johan Hovold, Frédéric Danis, Rob Herring, Loic Poulain,
Lukas Wunner, Hans de Goede,
bluez mailin list (linux-bluetooth@vger.kernel.org), linux-serial,
linux-acpi
In-Reply-To: <B8D6C3B8-8108-4372-88E6-6182314C7849@holtmann.org>
[-- Attachment #1: Type: text/plain, Size: 2330 bytes --]
Hi,
On Sun, Oct 08, 2017 at 10:51:52AM +0200, Marcel Holtmann wrote:
> Hi Sebastian,
>
> >>> UART devices is expected to be enumerated by SerDev subsystem.
> >>>
> >>> During ACPI scan, serial devices behind SPI, I2C or UART buses are not
> >>> enumerated, allowing them to be enumerated by their respective parents.
> >>>
> >>> Rename *spi_i2c_slave* to *serial_bus_slave* as this will be used for serial
> >>> devices on serial buses (SPI, I2C or UART).
> >>>
> >>> On Macs an empty ResourceTemplate is returned for uart slaves.
> >>> Instead the device properties "baud", "parity", "dataBits", "stopBits" are
> >>> provided. Add a check for "baud" in acpi_is_serial_bus_slave().
> >>>
> >>> Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
> >>
> >> So just to reiterate what I just mentioned in a comment to one of Hans's
> >> hci_bcm patches:
> >>
> >> This one would silently break PM for such devices on any system which
> >> does not have serdev enabled (as the corresponding platform devices
> >> would no longer be registered). And with serdev enabled, hciattach
> >> (btattach) would start failing as the tty device would no longer be
> >> registered (but I assume everyone is aware of that, and fine with it, by
> >> now).
> >>
> >> Perhaps the hci_bcm driver should start depending on
> >> SERIAL_DEV_CTRL_TTYPORT when ACPI is enabled?
> >
> > ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly,
> > since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented
> > for serdev. If any other controller is implemented that one could
> > also be used.
> >
> > I wonder if we should just hide SERIAL_DEV_CTRL_TTYPORT and enable
> > it together with SERDEV. I suspect that we won't see any other
> > controller (it would be a UART device, that is not registered as
> > tty device) in the next few years and the extra option seems to
> > confuse people.
>
> I wonder if we should just default SERIAL_DEV_CTRL_TTYPORT=y when
> DT or ACPI is enabled. Then no driver would have to select or
> depend on it.
This is not related to DT/ACPI. SERIAL_DEV_CTRL_TTYPORT is a serdev
controller driver, that registers serdev controller devices for each
tty. It will also be used by clients, that are registered via board
code.
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] ACPI / scan: Fix enumeration for special UART devices
From: Johan Hovold @ 2017-10-09 9:08 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Johan Hovold, Frédéric Danis, robh, marcel,
loic.poulain, lukas, hdegoede, linux-bluetooth, linux-serial,
linux-acpi
In-Reply-To: <20171009085534.c5smytfgckyx7mqy@earth>
On Mon, Oct 09, 2017 at 10:55:34AM +0200, Sebastian Reichel wrote:
> Hi,
>
> On Mon, Oct 09, 2017 at 09:35:26AM +0200, Johan Hovold wrote:
> > On Sun, Oct 08, 2017 at 12:53:11AM +0200, Sebastian Reichel wrote:
> > > On Sat, Oct 07, 2017 at 05:19:34PM +0200, Johan Hovold wrote:
> > > > On Wed, Oct 04, 2017 at 10:51:30AM +0200, Frédéric Danis wrote:
> > > > > UART devices is expected to be enumerated by SerDev subsystem.
> > > > >
> > > > > During ACPI scan, serial devices behind SPI, I2C or UART buses are not
> > > > > enumerated, allowing them to be enumerated by their respective parents.
> > > > >
> > > > > Rename *spi_i2c_slave* to *serial_bus_slave* as this will be used for serial
> > > > > devices on serial buses (SPI, I2C or UART).
> > > > >
> > > > > On Macs an empty ResourceTemplate is returned for uart slaves.
> > > > > Instead the device properties "baud", "parity", "dataBits", "stopBits" are
> > > > > provided. Add a check for "baud" in acpi_is_serial_bus_slave().
> > > > >
> > > > > Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
> > > >
> > > > So just to reiterate what I just mentioned in a comment to one of Hans's
> > > > hci_bcm patches:
> > > >
> > > > This one would silently break PM for such devices on any system which
> > > > does not have serdev enabled (as the corresponding platform devices
> > > > would no longer be registered). And with serdev enabled, hciattach
> > > > (btattach) would start failing as the tty device would no longer be
> > > > registered (but I assume everyone is aware of that, and fine with it, by
> > > > now).
> > > >
> > > > Perhaps the hci_bcm driver should start depending on
> > > > SERIAL_DEV_CTRL_TTYPORT when ACPI is enabled?
> > >
> > > ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly,
> > > since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented
> > > for serdev. If any other controller is implemented that one could
> > > also be used.
> >
> > Not for hci_bcm, right? This particular driver specifically depends on
> > SERIAL_DEV_CTRL_TTYPORT for the ACPI devices and not just any (future)
> > serdev controller (or currently working systems soon breaks silently).
> >
> > I don't think the same is true for the DT case where we do not already
> > have child nodes defined in firmware (and in fact, this driver did not
> > really support DT before serdev).
>
> The serdev ACPI support has been added to the core and not to
> the ttyport and the hci_bcm driver only uses functions from the
> core. As far as I can see the ACPI part would also work fine with
> a different serdev controller.
Indeed, but you need SERIAL_DEV_CTRL_TTYPORT to avoid silently breaking
current ACPI setups which breaks when this patch is applied (as these
devices all hang off of common serial ports managed by serial core).
> Of course DT and ACPI currently require SERIAL_DEV_CTRL_TTYPORT,
> since it's the only serdev controller implementation. Also it
> covers most use cases. When SERIAL_DEV_BUS is selected it's
> very likely, that you also want SERIAL_DEV_CTRL_TTYPORT.
> > > I wonder if we should just hide SERIAL_DEV_CTRL_TTYPORT and enable
> > > it together with SERDEV. I suspect that we won't see any other
> > > controller (it would be a UART device, that is not registered as
> > > tty device) in the next few years and the extra option seems to
> > > confuse people.
> >
> > I agree that it is somewhat confusing. But now that we have both,
> > perhaps simply having SERIAL_DEV_CTRL_TTYPORT default to "y" when
> > SERIAL_DEV_BUS is selected could be a compromise. The Kconfig entry
> > might need to be amended as well (e.g. if only to mention that you
> > need to select a controller as well).
>
> I think we should at least add a default "y" if SERIAL_DEV_BUS.
I'm preparing a patch.
> > And the bluetooth uart drivers already depend on SERIAL_DEV_BUS.
>
> Yes and that's the correct dependency. They only need the serdev
> core and controller. The only reason they do not work without
> SERIAL_DEV_CTRL_TTYPORT is, that there won't be any serdev
> controller.
In general, yes. Again, the only exception would be hci_bcm to avoid
breaking current setups without people noticing.
> Note, that the default "y" if SERIAL_DEV_BUS in SERIAL_DEV_CTRL_TTYPORT's
> config entry is only a partial fix. There is still the problem,
> that SERIAL_DEV_CTRL_TTYPORT can only be enabled if SERIAL_DEV_BUS
> is configured builtin. This is a limitation of the ttyport
> implementation, that hooks into builtin TTY core code.
I'm not saying it's a fix, but it is a sane default. I'm preparing a
patch also amending the Kconfig entries, and we can take it from there.
Johan
^ permalink raw reply
* Re: [PATCH v2 9/9] Bluetooth: hci_bcm: Add (runtime)pm support to the serdev driver
From: Hans de Goede @ 2017-10-09 14:02 UTC (permalink / raw)
To: Johan Hovold, Marcel Holtmann
Cc: Gustavo F. Padovan, Johan Hedberg, Frédéric Danis,
Sebastian Reichel, Rob Herring,
bluez mailin list (linux-bluetooth@vger.kernel.org), linux-serial,
linux-acpi
In-Reply-To: <20171007142631.GF2618@localhost>
Hi,
On 07-10-17 16:26, Johan Hovold wrote:
> On Thu, Oct 05, 2017 at 05:15:05PM +0200, Marcel Holtmann wrote:
>> Hi Hans,
>>
>>>>> Make the serdev driver use struct bcm_device as its driver data and share
>>>>> all the pm / GPIO / IRQ related code paths with the platform driver.
>>>>>
>>>>> After this commit the 2 drivers are in essence the same and the serdev
>>>>> driver interface can be used for all ACPI enumerated HCI UARTs.
>>>>>
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>> ---
>>>>> drivers/bluetooth/hci_bcm.c | 118 +++++++++++++++++++++++++-------------------
>>>>> 1 file changed, 68 insertions(+), 50 deletions(-)
>>>> all 9 patches have been applied to bluetooth-next tree.
>>>
>>> Excellent, thank you!
>
> Nice work here, Hans, allowing serdev to coexist with the
> platform-device hacks until the transition is complete and those hacks
> can finally be removed.
>
>>> So I guess this means we can also move forward with getting
>>> the 2 patches from Frédéric Danis merged ? There is a bit
>>> of a bisect-ability problem there, if the acpi pull-req
>>> gets merged first then uart attached bcm bt will stop
>>> working until the bluetooth subsys is also merged.
>>>
>>> But I don't think this will impact a lot of users
>>> (also given the need for a manual btattach so far),
>>> so I don't think this is a big problem... ?
>>
>> I wonder if we should just do this as all-in-one change for 4.15
>> kernel. We surely can get the ACPI changes into 4.15 and the Bluetooth
>> changes as well. Then it should just work.
>
> However, there are of course a couple of caveats. Once Frederic's ACPI
> patches land, there will be no more platform child devices. Unless
> serdev support is then compiled in, this means that PM will break
> (silently). And if serdev is enabled, of course the tty class device is
> gone and hciattach (btattach) will fail, but I guess everyone is aware
> of that issue by now.
>
> Should BT_HCIUART_BCM start depending on SERIAL_DEV_CTRL_TTYPORT (when
> ACPI is enabled) to avoid such silent breakage once ACPI-support is
> merged?
It seems that this was answered already in further discussions and
your recent patch to add a default y to SERIAL_DEV_CTRL_TTYPORT fixes
this, right ?
Regards,
Hans
^ permalink raw reply
* Re: [PATCH v2 1/9] Bluetooth: hci_uart_set_flow_control: Fix NULL deref when using serdev
From: Hans de Goede @ 2017-10-09 14:06 UTC (permalink / raw)
To: Johan Hovold
Cc: Marcel Holtmann, Gustavo Padovan, Johan Hedberg,
Frédéric Danis, Sebastian Reichel, robh,
linux-bluetooth, linux-serial, linux-acpi
In-Reply-To: <20171007143645.GG2618@localhost>
Hi,
On 07-10-17 16:36, Johan Hovold wrote:
> On Wed, Oct 04, 2017 at 08:43:35PM +0200, Hans de Goede wrote:
>> Fix a NULL pointer deref (hu->tty) when calling hci_uart_set_flow_control
>> on hci_uart-s using serdev.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -Also set RTS (Suggested-by: Sebastian Reichel <sre@kernel.org>)
>> ---
>> drivers/bluetooth/hci_ldisc.c | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
>> index a746627e784e..eec95019f15c 100644
>> --- a/drivers/bluetooth/hci_ldisc.c
>> +++ b/drivers/bluetooth/hci_ldisc.c
>> @@ -41,6 +41,7 @@
>> #include <linux/ioctl.h>
>> #include <linux/skbuff.h>
>> #include <linux/firmware.h>
>> +#include <linux/serdev.h>
>
> I know this is already merged, but do we really want to add a dependency
> on serdev from hci_ldisc.c?
>
> There is already a helper function host_set_baudrate() in hci_bcm to
> handle another case like this. If more drivers will need to support
> both then these could be moved to a common header, but we should at
> least try to be consistent here.
That is a valiud question, Marcel do you want me to do a follow
up series which:
1) Reverts this commit; and
2) Add a host_set_flow_control helper to hci_bcm.c ?
>> #include <net/bluetooth/bluetooth.h>
>> #include <net/bluetooth/hci_core.h>
>> @@ -298,6 +299,12 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
>> unsigned int set = 0;
>> unsigned int clear = 0;
>>
>> + if (hu->serdev) {
>> + serdev_device_set_flow_control(hu->serdev, !enable);
>> + serdev_device_set_rts(hu->serdev, !enable);
>
> The order here may matter; in the non-serdev case, rts is raised before
> enabling flow control.
AFAIK it should not matter, if RTS is not set before enabling, then CTS
may not be set by the other side yet and the hw flow-control will
wait for CTS to get raised.
Regards,
Hans
>
>> + return;
>> + }
>> +
>> if (enable) {
>> /* Disable hardware flow control */
>> ktermios = tty->termios;
>
> Johan
>
^ permalink raw reply
* Re: [PATCH v2 9/9] Bluetooth: hci_bcm: Add (runtime)pm support to the serdev driver
From: Johan Hovold @ 2017-10-09 14:22 UTC (permalink / raw)
To: Hans de Goede
Cc: Johan Hovold, Marcel Holtmann, Gustavo F. Padovan, Johan Hedberg,
Frédéric Danis, Sebastian Reichel, Rob Herring,
bluez mailin list (linux-bluetooth@vger.kernel.org), linux-serial,
linux-acpi
In-Reply-To: <ebd6e9dc-9c25-a2ea-619f-bd1cdf45b6a3@redhat.com>
On Mon, Oct 09, 2017 at 04:02:24PM +0200, Hans de Goede wrote:
> Hi,
>
> On 07-10-17 16:26, Johan Hovold wrote:
> > However, there are of course a couple of caveats. Once Frederic's ACPI
> > patches land, there will be no more platform child devices. Unless
> > serdev support is then compiled in, this means that PM will break
> > (silently). And if serdev is enabled, of course the tty class device is
> > gone and hciattach (btattach) will fail, but I guess everyone is aware
> > of that issue by now.
> >
> > Should BT_HCIUART_BCM start depending on SERIAL_DEV_CTRL_TTYPORT (when
> > ACPI is enabled) to avoid such silent breakage once ACPI-support is
> > merged?
>
> It seems that this was answered already in further discussions and
> your recent patch to add a default y to SERIAL_DEV_CTRL_TTYPORT fixes
> this, right ?
Only partially, as the serdev bus code could still be built as a module,
which would prevent SERIAL_DEV_CTRL_TTYPORT from being selected.
We could consider having BT_HCIUART_BCM depend on
SERIAL_DEV_CTRL_TTYPORT (or !ACPI || SERIAL_DEV_CTRL_TTYPORT) just to
avoid any hard-to-detect regressions.
Johan
^ permalink raw reply
* Re: [PATCH v2 1/9] Bluetooth: hci_uart_set_flow_control: Fix NULL deref when using serdev
From: Johan Hovold @ 2017-10-09 15:15 UTC (permalink / raw)
To: Hans de Goede
Cc: Johan Hovold, Marcel Holtmann, Gustavo Padovan, Johan Hedberg,
Frédéric Danis, Sebastian Reichel, robh,
linux-bluetooth, linux-serial, linux-acpi
In-Reply-To: <ce49b622-212a-0e6c-2c05-9cfe3db4bd47@redhat.com>
On Mon, Oct 09, 2017 at 04:06:02PM +0200, Hans de Goede wrote:
> On 07-10-17 16:36, Johan Hovold wrote:
> > On Wed, Oct 04, 2017 at 08:43:35PM +0200, Hans de Goede wrote:
> >> @@ -298,6 +299,12 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
> >> unsigned int set = 0;
> >> unsigned int clear = 0;
> >>
> >> + if (hu->serdev) {
> >> + serdev_device_set_flow_control(hu->serdev, !enable);
> >> + serdev_device_set_rts(hu->serdev, !enable);
> >
> > The order here may matter; in the non-serdev case, rts is raised before
> > enabling flow control.
>
> AFAIK it should not matter, if RTS is not set before enabling, then CTS
> may not be set by the other side yet and the hw flow-control will
> wait for CTS to get raised.
That's not necessarily how hardware flow control is implemented. When
using auto-RTS, RTS will simply be asserted whenever there's room in the
receive FIFO.
So in fact it seems the hci_ldisc should be doing this in the reverse
order (e.g. deassert RTS before disabling auto-RTS).
Probably doesn't matter in practice here, but I would at least expect
the enable and disable cases to be each others inverses (and for the
ldisc and serdev implementations to match).
Johan
^ permalink raw reply
* Re: [PATCH v2 1/9] Bluetooth: hci_uart_set_flow_control: Fix NULL deref when using serdev
From: Marcel Holtmann @ 2017-10-09 18:01 UTC (permalink / raw)
To: Hans de Goede
Cc: Johan Hovold, Gustavo F. Padovan, Johan Hedberg,
Frédéric Danis, Sebastian Reichel, Rob Herring,
bluez mailin list (linux-bluetooth@vger.kernel.org), linux-serial,
linux-acpi
In-Reply-To: <ce49b622-212a-0e6c-2c05-9cfe3db4bd47@redhat.com>
Hi Hans,
>>> Fix a NULL pointer deref (hu->tty) when calling hci_uart_set_flow_control
>>> on hci_uart-s using serdev.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> Changes in v2:
>>> -Also set RTS (Suggested-by: Sebastian Reichel <sre@kernel.org>)
>>> ---
>>> drivers/bluetooth/hci_ldisc.c | 7 +++++++
>>> 1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
>>> index a746627e784e..eec95019f15c 100644
>>> --- a/drivers/bluetooth/hci_ldisc.c
>>> +++ b/drivers/bluetooth/hci_ldisc.c
>>> @@ -41,6 +41,7 @@
>>> #include <linux/ioctl.h>
>>> #include <linux/skbuff.h>
>>> #include <linux/firmware.h>
>>> +#include <linux/serdev.h>
>> I know this is already merged, but do we really want to add a dependency
>> on serdev from hci_ldisc.c?
>> There is already a helper function host_set_baudrate() in hci_bcm to
>> handle another case like this. If more drivers will need to support
>> both then these could be moved to a common header, but we should at
>> least try to be consistent here.
>
> That is a valiud question, Marcel do you want me to do a follow
> up series which:
>
> 1) Reverts this commit; and
> 2) Add a host_set_flow_control helper to hci_bcm.c ?
don’t bother with a revert, lets just improve this with follow up patches.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 2/2] ACPI / scan: Fix enumeration for special UART devices
From: Marcel Holtmann @ 2017-10-09 18:09 UTC (permalink / raw)
To: Johan Hovold
Cc: Sebastian Reichel, Frédéric Danis,
robh-DgEjT+Ai2ygdnm+yROfE0A, loic.poulain-Re5JQEeQqe8AvxtiuMwx3w,
lukas-JFq808J9C/izQB+pC5nmwQ, hdegoede-H+wXaHxf7aLQT0dZR+AlfA,
linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20171009090835.GQ2618@localhost>
Hi Johan,
>>>>>> UART devices is expected to be enumerated by SerDev subsystem.
>>>>>>
>>>>>> During ACPI scan, serial devices behind SPI, I2C or UART buses are not
>>>>>> enumerated, allowing them to be enumerated by their respective parents.
>>>>>>
>>>>>> Rename *spi_i2c_slave* to *serial_bus_slave* as this will be used for serial
>>>>>> devices on serial buses (SPI, I2C or UART).
>>>>>>
>>>>>> On Macs an empty ResourceTemplate is returned for uart slaves.
>>>>>> Instead the device properties "baud", "parity", "dataBits", "stopBits" are
>>>>>> provided. Add a check for "baud" in acpi_is_serial_bus_slave().
>>>>>>
>>>>>> Signed-off-by: Frédéric Danis <frederic.danis.oss-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>>
>>>>> So just to reiterate what I just mentioned in a comment to one of Hans's
>>>>> hci_bcm patches:
>>>>>
>>>>> This one would silently break PM for such devices on any system which
>>>>> does not have serdev enabled (as the corresponding platform devices
>>>>> would no longer be registered). And with serdev enabled, hciattach
>>>>> (btattach) would start failing as the tty device would no longer be
>>>>> registered (but I assume everyone is aware of that, and fine with it, by
>>>>> now).
>>>>>
>>>>> Perhaps the hci_bcm driver should start depending on
>>>>> SERIAL_DEV_CTRL_TTYPORT when ACPI is enabled?
>>>>
>>>> ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly,
>>>> since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented
>>>> for serdev. If any other controller is implemented that one could
>>>> also be used.
>>>
>>> Not for hci_bcm, right? This particular driver specifically depends on
>>> SERIAL_DEV_CTRL_TTYPORT for the ACPI devices and not just any (future)
>>> serdev controller (or currently working systems soon breaks silently).
>>>
>>> I don't think the same is true for the DT case where we do not already
>>> have child nodes defined in firmware (and in fact, this driver did not
>>> really support DT before serdev).
>>
>> The serdev ACPI support has been added to the core and not to
>> the ttyport and the hci_bcm driver only uses functions from the
>> core. As far as I can see the ACPI part would also work fine with
>> a different serdev controller.
>
> Indeed, but you need SERIAL_DEV_CTRL_TTYPORT to avoid silently breaking
> current ACPI setups which breaks when this patch is applied (as these
> devices all hang off of common serial ports managed by serial core).
>
>> Of course DT and ACPI currently require SERIAL_DEV_CTRL_TTYPORT,
>> since it's the only serdev controller implementation. Also it
>> covers most use cases. When SERIAL_DEV_BUS is selected it's
>> very likely, that you also want SERIAL_DEV_CTRL_TTYPORT.
>
>>>> I wonder if we should just hide SERIAL_DEV_CTRL_TTYPORT and enable
>>>> it together with SERDEV. I suspect that we won't see any other
>>>> controller (it would be a UART device, that is not registered as
>>>> tty device) in the next few years and the extra option seems to
>>>> confuse people.
>>>
>>> I agree that it is somewhat confusing. But now that we have both,
>>> perhaps simply having SERIAL_DEV_CTRL_TTYPORT default to "y" when
>>> SERIAL_DEV_BUS is selected could be a compromise. The Kconfig entry
>>> might need to be amended as well (e.g. if only to mention that you
>>> need to select a controller as well).
>>
>> I think we should at least add a default "y" if SERIAL_DEV_BUS.
>
> I'm preparing a patch.
yes, please prepare a patch since the discussion spans multiple email threads now. Lets get this back on track and find a patch that we are all happy with.
Regards
Marcel
^ permalink raw reply
* [PATCH] serial: sh-sci: Remove __init attribute from static struct 'port_cfg'
From: Matthias Kaehlcke @ 2017-10-10 0:04 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: linux-serial, linux-kernel, Arnd Bergmann, Guenter Roeck,
Matthias Kaehlcke
This fixes the following error when building with clang:
drivers/tty/serial/sh-sci.c:3247:15: error: '__section__' attribute only
applies to functions, methods, properties, and global variables
static struct __init plat_sci_port port_cfg;
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
drivers/tty/serial/sh-sci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 784dd42002ea..0d1df1df9e89 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -3244,7 +3244,7 @@ early_platform_init_buffer("earlyprintk", &sci_driver,
early_serial_buf, ARRAY_SIZE(early_serial_buf));
#endif
#ifdef CONFIG_SERIAL_SH_SCI_EARLYCON
-static struct __init plat_sci_port port_cfg;
+static struct plat_sci_port port_cfg;
static int __init early_console_setup(struct earlycon_device *device,
int type)
--
2.14.2.920.gcf0c67979c-goog
^ permalink raw reply related
* Re: [PATCH] serial: sh-sci: Remove __init attribute from static struct 'port_cfg'
From: Guenter Roeck @ 2017-10-10 0:18 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, linux-kernel,
Arnd Bergmann, Guenter Roeck
In-Reply-To: <20171010000411.140016-1-mka@chromium.org>
On Mon, Oct 9, 2017 at 5:04 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> This fixes the following error when building with clang:
>
> drivers/tty/serial/sh-sci.c:3247:15: error: '__section__' attribute only
> applies to functions, methods, properties, and global variables
> static struct __init plat_sci_port port_cfg;
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> drivers/tty/serial/sh-sci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 784dd42002ea..0d1df1df9e89 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -3244,7 +3244,7 @@ early_platform_init_buffer("earlyprintk", &sci_driver,
> early_serial_buf, ARRAY_SIZE(early_serial_buf));
> #endif
> #ifdef CONFIG_SERIAL_SH_SCI_EARLYCON
> -static struct __init plat_sci_port port_cfg;
> +static struct plat_sci_port port_cfg;
__initdata ?
Guenter
>
> static int __init early_console_setup(struct earlycon_device *device,
> int type)
> --
> 2.14.2.920.gcf0c67979c-goog
>
^ permalink raw reply
* Re: [PATCH 0/2] ACPI serdev support
From: Rafael J. Wysocki @ 2017-10-10 0:27 UTC (permalink / raw)
To: Frédéric Danis
Cc: Rob Herring, Marcel Holtmann, Sebastian Reichel, Loic Poulain,
Johan Hovold, Lukas Wunner, Hans de Goede,
open list:BLUETOOTH DRIVERS,
linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
ACPI Devel Maling List
In-Reply-To: <1507107090-15992-1-git-send-email-frederic.danis.oss-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Wed, Oct 4, 2017 at 10:51 AM, Frédéric Danis
<frederic.danis.oss-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Add ACPI support for serial attached devices.
>
> Currently, serial devices are not set as enumerated during ACPI scan for SPI
> or i2c buses (but not for UART). This should also be done for UART serial
> devices.
> I renamed *spi_i2c_slave* to *serial_bus_slave* to reflect this.
>
> For hci_bcm, this needs Hans de Goede's "Bluetooth: hci_bcm: Add (runtime)pm
> support to the serdev drv" patches to work.
>
> Tested on T100TA with Broadcom BCM2E39.
>
> Since RFC:
> - Add or reword commit messages
> - Rename *serial_slave* to *serial_bus_slave*
> - Add specific check for Apple in acpi_is_serial_bus_slave(), thanks to
> Lukas Wunner
> - Update comment in acpi_default_enumeration()
> - Remove patch 3 "Bluetooth: hci_bcm: Add ACPI serdev support for BCM2E39"
> in favor of patches from Hans de Goede
>
> Frédéric Danis (2):
> serdev: Add ACPI support
> ACPI / scan: Fix enumeration for special UART devices
Some review comments you received on this series should be addressed
IMO, so I'm expecting an update of it.
When sending the next version, please add all of the tags you've
already received on it.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH] serial: sh-sci: Remove __init attribute from static struct 'port_cfg'
From: Matthias Kaehlcke @ 2017-10-10 0:35 UTC (permalink / raw)
To: Guenter Roeck
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, linux-kernel,
Arnd Bergmann, Guenter Roeck
In-Reply-To: <CABXOdTdEH-=n8nf6a=0sC1iBeXLDKSxTXxoj1wUYmLUX_U=BUw@mail.gmail.com>
El Mon, Oct 09, 2017 at 05:18:32PM -0700 Guenter Roeck ha dit:
> On Mon, Oct 9, 2017 at 5:04 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> > This fixes the following error when building with clang:
> >
> > drivers/tty/serial/sh-sci.c:3247:15: error: '__section__' attribute only
> > applies to functions, methods, properties, and global variables
> > static struct __init plat_sci_port port_cfg;
> >
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > drivers/tty/serial/sh-sci.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> > index 784dd42002ea..0d1df1df9e89 100644
> > --- a/drivers/tty/serial/sh-sci.c
> > +++ b/drivers/tty/serial/sh-sci.c
> > @@ -3244,7 +3244,7 @@ early_platform_init_buffer("earlyprintk", &sci_driver,
> > early_serial_buf, ARRAY_SIZE(early_serial_buf));
> > #endif
> > #ifdef CONFIG_SERIAL_SH_SCI_EARLYCON
> > -static struct __init plat_sci_port port_cfg;
> > +static struct plat_sci_port port_cfg;
>
> __initdata ?
Good point, thanks!
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox