* Re: [PATCH] serial: 8250_dw: Fix common clocks usage race condition [not found] ` <20200310001444.B0F50803087C@mail.baikalelectronics.ru> @ 2020-03-10 14:17 ` Andy Shevchenko [not found] ` <20200310141715.7063280307CA@mail.baikalelectronics.ru> 1 sibling, 0 replies; 29+ messages in thread From: Andy Shevchenko @ 2020-03-10 14:17 UTC (permalink / raw) To: Sergey Semin, linux-serial Cc: Greg Kroah-Hartman, Jiri Slaby, Alexey Malahov, Maxim Kaurkin, Pavel Parkhomenko, Ramil Zaripov, Ekaterina Skachko, Vadim Vlasov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle It's pity we are discussing this off-list. Cc linux-serial@ now and leaving original writings as is. On Tue, Mar 10, 2020 at 03:13:53AM +0300, Sergey Semin wrote: > On Fri, Mar 06, 2020 at 03:40:44PM +0200, Andy Shevchenko wrote: > > On Fri, Mar 06, 2020 at 04:02:20PM +0300, Sergey.Semin@baikalelectronics.ru wrote: > > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > > > There are races possible in the dw8250_set_termios() callback method > > > and while the device is in PM suspend state. A race condition may > > > happen if the baudrate clocks source device is shared with some other > > > device (in our machine it's another DW UART port). In this case if that > > > device changes the clocks rate while serial console is using it the > > > DW 8250 UART port might not only end up with an invalid uartclk value > > > saved, but might also experience a distorted output data since > > > baud-clock might have been changed. In order to fix this lets enable > > > an exclusive clocks rate access in case if "baudclk" device is > > > specified and is successfully enabled. > > > > > > So if some other device also acquires the rate exclusivity, then > > > DW UART 8250 driver won't be able to alter the baud-clock. It shall > > > just use the available clocks rate. Similarly another device also > > > won't manage to change the rate. If nothing else have the exclusive > > > rate access acquired except DW UART 8250 driver, then the driver > > > will be able to alter the rate as much as needs to in accordance > > > with the currently implemented logic. > > > > Thank you for the patch. > > > > I honestly consider that this is kinda bad hardware design. If two devices are > > fighting for the best rate, there is a room to make the second one always miss. > > > > (Imagine 1834200 UART clock and first asked for 115200, while second > > desperately tries for 4M). > > > > I think that the clock driver of the corresponding platform should actually > > keep track of the users and give the best for both. > > > > Hmm, this is discussable. > > While I do agree that two serial ports using a common reference clock > was a bad hw design (it was nothing for the hw designers to add an > extra clock divider), and do understand the concern you mentioned in the > example I disagree that the hw clock driver should handle the clock usage > races. There is a possible alternative but the current solution seemed > to me most suitable and here is why. > > First of all, if I implemented something like a users tracking algo in > the hw clock driver, It would be similar to the already developed and > suggested to be used in this patch clock exclusivity approach anyway, > since there would be no way to change the clock rate behind the uart drivers > back to adjust the clock frequency so making it suitable for both users. > In this case we would have the same race condition you described in the > parentheses. > > Secondly why would we need to have the clk_rate_exlusive_{get,put}() > interface exported from the clk subsystem anyway if not for the cases > like described in this commit message? These functions were designed for > devices, which require the exclusive rate lock at some point. As for me > I would have added the clock rate exclusive lock to all the drivers > handling devices (not only UARTs), which depend not only on a clock being > enabled but also on it having a particular rate value. But this is too > much work, which alas I don't have time for at the moment. > > Thirdly. Much easier solution would be to pretend, that the actual clock > device doesn't provide the rate change capability and work with a value > pre-initialized in the clock divider register (this is necessary because > on most of known to me Baikal-T1-based platforms one of these UARTs is > always used as a system/boot serial console). But in this case we would > have to artificially bind a different driver to the clock divider interface, > which wouldn't reflect the actual clock capability. > > Finally in my opinion the suggested in this patch solution is the most > suitable. It solves the problem on the common clock interface usage > by two independent UART ports. It also provides a portable solution for > platforms, which have the similar problem. And it doesn't affect the > devices lacking the described clock rate race condition. > > By the way I came up with an upgrade, which would make the solution even > better. Instead of using the clock exclusive rate lock/unlock in the > probe()/remove() methods, we could be calling them from the UART-port > startup()/shutdown() functions instead. In this case the clock rate would > be fixed only if both ports are opened at the same time. This would > lighten the limitations on the rates change conditions. What do you think > of this? The use of clk_set_rate_exclusive()/clk_rate_exclusive_put() seems better. But, while from our (Intel) perspective this looks like no-op, for others, especially zillions of ARM boards using DesignWare UART it might be crucial. I would suggest to Cc new version to some ARM people (homework: filter MAINTAINERS along with `git log -- drivers/tty/serial/8250/8520_dw.c` to see who may have relation to this driver). > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> > > > > Why second SoB is here, it's not the same as the submitter's (usually, > > submitter's one goes last in the chain)? Commit message or another tags, > > like Co-developed-by, doesn't shed a light either. > > Alexey as being our team-leader was in these patches delivery path, which > according to [1] permits to add the SoB tag with his address: > > "The Signed-off-by: tag indicates that the signer was involved in the > development of the patch, or that he/she was in the patch’s delivery path." > > So the SoB signify that all the changes provided by the patches I've sent > and will yet send have been passed through his revision and can be > opened at the corresponding license. > > [1] https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html Yes, and AFAIU the last person in a chain submits the work. So, it should reflect how patch went thru people's trees. If you send from your tree, where patches are created originally, there shouldn't be other SoBs (use suitable Reviewed-by, etc). At least this is safest side. However, better to ask maintainer (Greg KH in this case) and other senior developers. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 29+ messages in thread
[parent not found: <20200310141715.7063280307CA@mail.baikalelectronics.ru>]
* Re: [PATCH] serial: 8250_dw: Fix common clocks usage race condition [not found] ` <20200310141715.7063280307CA@mail.baikalelectronics.ru> @ 2020-03-12 18:47 ` Sergey Semin 2020-03-18 15:19 ` Andy Shevchenko 0 siblings, 1 reply; 29+ messages in thread From: Sergey Semin @ 2020-03-12 18:47 UTC (permalink / raw) To: Andy Shevchenko Cc: linux-serial, Greg Kroah-Hartman, Jiri Slaby, Alexey Malahov, Maxim Kaurkin, Pavel Parkhomenko, Ramil Zaripov, Ekaterina Skachko, Vadim Vlasov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle On Tue, Mar 10, 2020 at 04:17:08PM +0200, Andy Shevchenko wrote: > It's pity we are discussing this off-list. > Cc linux-serial@ now and leaving original writings as is. > > On Tue, Mar 10, 2020 at 03:13:53AM +0300, Sergey Semin wrote: > > On Fri, Mar 06, 2020 at 03:40:44PM +0200, Andy Shevchenko wrote: > > > On Fri, Mar 06, 2020 at 04:02:20PM +0300, Sergey.Semin@baikalelectronics.ru wrote: > > > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > > > > > There are races possible in the dw8250_set_termios() callback method > > > > and while the device is in PM suspend state. A race condition may > > > > happen if the baudrate clocks source device is shared with some other > > > > device (in our machine it's another DW UART port). In this case if that > > > > device changes the clocks rate while serial console is using it the > > > > DW 8250 UART port might not only end up with an invalid uartclk value > > > > saved, but might also experience a distorted output data since > > > > baud-clock might have been changed. In order to fix this lets enable > > > > an exclusive clocks rate access in case if "baudclk" device is > > > > specified and is successfully enabled. > > > > > > > > So if some other device also acquires the rate exclusivity, then > > > > DW UART 8250 driver won't be able to alter the baud-clock. It shall > > > > just use the available clocks rate. Similarly another device also > > > > won't manage to change the rate. If nothing else have the exclusive > > > > rate access acquired except DW UART 8250 driver, then the driver > > > > will be able to alter the rate as much as needs to in accordance > > > > with the currently implemented logic. > > > > > > Thank you for the patch. > > > > > > I honestly consider that this is kinda bad hardware design. If two devices are > > > fighting for the best rate, there is a room to make the second one always miss. > > > > > > (Imagine 1834200 UART clock and first asked for 115200, while second > > > desperately tries for 4M). > > > > > > I think that the clock driver of the corresponding platform should actually > > > keep track of the users and give the best for both. > > > > > > > Hmm, this is discussable. > > > > While I do agree that two serial ports using a common reference clock > > was a bad hw design (it was nothing for the hw designers to add an > > extra clock divider), and do understand the concern you mentioned in the > > example I disagree that the hw clock driver should handle the clock usage > > races. There is a possible alternative but the current solution seemed > > to me most suitable and here is why. > > > > First of all, if I implemented something like a users tracking algo in > > the hw clock driver, It would be similar to the already developed and > > suggested to be used in this patch clock exclusivity approach anyway, > > since there would be no way to change the clock rate behind the uart drivers > > back to adjust the clock frequency so making it suitable for both users. > > In this case we would have the same race condition you described in the > > parentheses. > > > > Secondly why would we need to have the clk_rate_exlusive_{get,put}() > > interface exported from the clk subsystem anyway if not for the cases > > like described in this commit message? These functions were designed for > > devices, which require the exclusive rate lock at some point. As for me > > I would have added the clock rate exclusive lock to all the drivers > > handling devices (not only UARTs), which depend not only on a clock being > > enabled but also on it having a particular rate value. But this is too > > much work, which alas I don't have time for at the moment. > > > > Thirdly. Much easier solution would be to pretend, that the actual clock > > device doesn't provide the rate change capability and work with a value > > pre-initialized in the clock divider register (this is necessary because > > on most of known to me Baikal-T1-based platforms one of these UARTs is > > always used as a system/boot serial console). But in this case we would > > have to artificially bind a different driver to the clock divider interface, > > which wouldn't reflect the actual clock capability. > > > > Finally in my opinion the suggested in this patch solution is the most > > suitable. It solves the problem on the common clock interface usage > > by two independent UART ports. It also provides a portable solution for > > platforms, which have the similar problem. And it doesn't affect the > > devices lacking the described clock rate race condition. > > > > By the way I came up with an upgrade, which would make the solution even > > better. Instead of using the clock exclusive rate lock/unlock in the > > probe()/remove() methods, we could be calling them from the UART-port > > startup()/shutdown() functions instead. In this case the clock rate would > > be fixed only if both ports are opened at the same time. This would > > lighten the limitations on the rates change conditions. What do you think > > of this? > > The use of clk_set_rate_exclusive()/clk_rate_exclusive_put() seems better. > But, while from our (Intel) perspective this looks like no-op, for others, > especially zillions of ARM boards using DesignWare UART it might be crucial. > Using clk_set_rate_exclusive() isn't that appropriate at the port->startup() callback. Firstly because it may fail due to not being able to update the rate, then I would have to try to perform just clk_rate_exclusive_get(), retrieve currently set clock rate and update the port uartclk field. Secondly an attempt to update the rate will be performed in set_termios() anyway, so the calling of clk_set_rate() in the port->startup() is redundant. Thirdly I also can't use clk_set_rate_exclusive() in set_termios(), because it doesn't have some antagonist like unset_termios() to perform the same number of clk_rate_exclusive_put() to appropriately free the exclusive lock of the clock. And I don't want to manually calculate the numbers of exclusive locking. So the easiest way would be to just lock the reference clock in the port->startup() method and update the port->uartclk field with current reference clock rate. Clock unlocking will be performed in the port-shutdown() callback. I'll send v2 patch with this solution. You'll see what it looks like. > I would suggest to Cc new version to some ARM people (homework: filter > MAINTAINERS along with `git log -- drivers/tty/serial/8250/8520_dw.c` > to see who may have relation to this driver). > Nice homework. Although I'd prefer to utilize the get_maintainers.pl script for the same purpose. It has the --git argument for it, which also tunable for parameters like deepness of the git-history, number of signatures, number of maintainers, etc. > > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> > > > > > > Why second SoB is here, it's not the same as the submitter's (usually, > > > submitter's one goes last in the chain)? Commit message or another tags, > > > like Co-developed-by, doesn't shed a light either. > > > > Alexey as being our team-leader was in these patches delivery path, which > > according to [1] permits to add the SoB tag with his address: > > > > "The Signed-off-by: tag indicates that the signer was involved in the > > development of the patch, or that he/she was in the patch’s delivery path." > > > > So the SoB signify that all the changes provided by the patches I've sent > > and will yet send have been passed through his revision and can be > > opened at the corresponding license. > > > > [1] https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html > > Yes, and AFAIU the last person in a chain submits the work. > > So, it should reflect how patch went thru people's trees. If you send from your > tree, where patches are created originally, there shouldn't be other SoBs (use > suitable Reviewed-by, etc). At least this is safest side. However, better to > ask maintainer (Greg KH in this case) and other senior developers. > Ok. Last time I read the patches posting procedure document, it didn't have anything regarding the order. It does since commit in 2018. Anyway I talked with Alexey regarding this issue. So I'll add his signature only to the patches/drivers in which actual development he was involved to with further Co-Developed-by tag supplied. Regards, -Sergey > -- > With Best Regards, > Andy Shevchenko > > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] serial: 8250_dw: Fix common clocks usage race condition 2020-03-12 18:47 ` Sergey Semin @ 2020-03-18 15:19 ` Andy Shevchenko 0 siblings, 0 replies; 29+ messages in thread From: Andy Shevchenko @ 2020-03-18 15:19 UTC (permalink / raw) To: Sergey Semin Cc: linux-serial, Greg Kroah-Hartman, Jiri Slaby, Alexey Malahov, Maxim Kaurkin, Pavel Parkhomenko, Ramil Zaripov, Ekaterina Skachko, Vadim Vlasov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle On Thu, Mar 12, 2020 at 09:47:02PM +0300, Sergey Semin wrote: > On Tue, Mar 10, 2020 at 04:17:08PM +0200, Andy Shevchenko wrote: > > On Tue, Mar 10, 2020 at 03:13:53AM +0300, Sergey Semin wrote: > > > On Fri, Mar 06, 2020 at 03:40:44PM +0200, Andy Shevchenko wrote: > > > > On Fri, Mar 06, 2020 at 04:02:20PM +0300, Sergey.Semin@baikalelectronics.ru wrote: ... > > > > > There are races possible in the dw8250_set_termios() callback method > > > > > and while the device is in PM suspend state. A race condition may > > > > > happen if the baudrate clocks source device is shared with some other > > > > > device (in our machine it's another DW UART port). In this case if that > > > > > device changes the clocks rate while serial console is using it the > > > > > DW 8250 UART port might not only end up with an invalid uartclk value > > > > > saved, but might also experience a distorted output data since > > > > > baud-clock might have been changed. In order to fix this lets enable > > > > > an exclusive clocks rate access in case if "baudclk" device is > > > > > specified and is successfully enabled. > > > > > > > > > > So if some other device also acquires the rate exclusivity, then > > > > > DW UART 8250 driver won't be able to alter the baud-clock. It shall > > > > > just use the available clocks rate. Similarly another device also > > > > > won't manage to change the rate. If nothing else have the exclusive > > > > > rate access acquired except DW UART 8250 driver, then the driver > > > > > will be able to alter the rate as much as needs to in accordance > > > > > with the currently implemented logic. > > > > > > > > Thank you for the patch. > > > > > > > > I honestly consider that this is kinda bad hardware design. If two devices are > > > > fighting for the best rate, there is a room to make the second one always miss. > > > > > > > > (Imagine 1834200 UART clock and first asked for 115200, while second > > > > desperately tries for 4M). > > > > > > > > I think that the clock driver of the corresponding platform should actually > > > > keep track of the users and give the best for both. > > > > > > > > > > Hmm, this is discussable. > > > > > > While I do agree that two serial ports using a common reference clock > > > was a bad hw design (it was nothing for the hw designers to add an > > > extra clock divider), and do understand the concern you mentioned in the > > > example I disagree that the hw clock driver should handle the clock usage > > > races. There is a possible alternative but the current solution seemed > > > to me most suitable and here is why. > > > > > > First of all, if I implemented something like a users tracking algo in > > > the hw clock driver, It would be similar to the already developed and > > > suggested to be used in this patch clock exclusivity approach anyway, > > > since there would be no way to change the clock rate behind the uart drivers > > > back to adjust the clock frequency so making it suitable for both users. > > > In this case we would have the same race condition you described in the > > > parentheses. > > > > > > Secondly why would we need to have the clk_rate_exlusive_{get,put}() > > > interface exported from the clk subsystem anyway if not for the cases > > > like described in this commit message? These functions were designed for > > > devices, which require the exclusive rate lock at some point. As for me > > > I would have added the clock rate exclusive lock to all the drivers > > > handling devices (not only UARTs), which depend not only on a clock being > > > enabled but also on it having a particular rate value. But this is too > > > much work, which alas I don't have time for at the moment. > > > > > > Thirdly. Much easier solution would be to pretend, that the actual clock > > > device doesn't provide the rate change capability and work with a value > > > pre-initialized in the clock divider register (this is necessary because > > > on most of known to me Baikal-T1-based platforms one of these UARTs is > > > always used as a system/boot serial console). But in this case we would > > > have to artificially bind a different driver to the clock divider interface, > > > which wouldn't reflect the actual clock capability. > > > > > > Finally in my opinion the suggested in this patch solution is the most > > > suitable. It solves the problem on the common clock interface usage > > > by two independent UART ports. It also provides a portable solution for > > > platforms, which have the similar problem. And it doesn't affect the > > > devices lacking the described clock rate race condition. > > > > > > By the way I came up with an upgrade, which would make the solution even > > > better. Instead of using the clock exclusive rate lock/unlock in the > > > probe()/remove() methods, we could be calling them from the UART-port > > > startup()/shutdown() functions instead. In this case the clock rate would > > > be fixed only if both ports are opened at the same time. This would > > > lighten the limitations on the rates change conditions. What do you think > > > of this? > > > > The use of clk_set_rate_exclusive()/clk_rate_exclusive_put() seems better. > > But, while from our (Intel) perspective this looks like no-op, for others, > > especially zillions of ARM boards using DesignWare UART it might be crucial. > > > > Using clk_set_rate_exclusive() isn't that appropriate at the port->startup() > callback. Firstly because it may fail due to not being able to update the rate, > then I would have to try to perform just clk_rate_exclusive_get(), > retrieve currently set clock rate and update the port uartclk field. Secondly > an attempt to update the rate will be performed in set_termios() anyway, > so the calling of clk_set_rate() in the port->startup() is redundant. Thirdly > I also can't use clk_set_rate_exclusive() in set_termios(), because it doesn't > have some antagonist like unset_termios() to perform the same number of > clk_rate_exclusive_put() to appropriately free the exclusive lock of the clock. > And I don't want to manually calculate the numbers of exclusive locking. > > So the easiest way would be to just lock the reference clock in the > port->startup() method and update the port->uartclk field with current > reference clock rate. Clock unlocking will be performed in the port-shutdown() > callback. I'll send v2 patch with this solution. You'll see what it > looks like. > > > I would suggest to Cc new version to some ARM people (homework: filter > > MAINTAINERS along with `git log -- drivers/tty/serial/8250/8520_dw.c` > > to see who may have relation to this driver). > > > > Nice homework. Although I'd prefer to utilize the get_maintainers.pl script > for the same purpose. It has the --git argument for it, which also tunable > for parameters like deepness of the git-history, number of signatures, number > of maintainers, etc. You may send a new version with a bit more elaboration in the commit message followed by Cc'ing few people from ARM side. I'm okay with it from Intel hardware perspective, but I can't tell about the rest of the world. Also it would be nice to see come clock framework guys' opinions... -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2] serial: 8250_dw: Fix common clocks usage race condition [not found] <20200306130231.05BBC8030795@mail.baikalelectronics.ru> [not found] ` <20200306134049.86F8180307C2@mail.baikalelectronics.ru> @ 2020-03-23 2:46 ` Sergey.Semin 2020-03-23 9:20 ` Andy Shevchenko ` (2 more replies) 1 sibling, 3 replies; 29+ messages in thread From: Sergey.Semin @ 2020-03-23 2:46 UTC (permalink / raw) To: Andy Shevchenko, Greg Kroah-Hartman, Jiri Slaby Cc: Serge Semin, Serge Semin, Alexey Malahov, Maxim Kaurkin, Pavel Parkhomenko, Ramil Zaripov, Ekaterina Skachko, Vadim Vlasov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Maxime Ripard, Chen-Yu Tsai, Ray Jui, Scott Branden, Florian Fainelli, Wei Xu, Jason Cooper, Andrew Lunn, Gregory Clement, Sebastian Hesselbarth, Jisheng Zhang, Heiko Stuebner, Catalin Marinas, Will Deacon, Russell King, linux-arm-kernel, Michael Turquette, Stephen Boyd, linux-clk, Heikki Krogerus, Kefeng Wang, linux-serial, linux-kernel From: Serge Semin <Sergey.Semin@baikalelectronics.ru> There are races possible in the dw8250_set_termios() callback method and while the device is in PM suspend state. A race condition may happen if the baudrate clock source device is shared with some other device (in our machine it's another DW UART port). In this case if that device changes the clock rate while serial console is using it the DW 8250 UART port might not only end up with an invalid uartclk value saved, but may also experience a distorted output data since baud-clock could have been changed. In order to fix this lets enable an exclusive reference clock rate access in case if "baudclk" device is specified. So if some other device also acquires the rate exclusivity during the time of a DW UART 8250 port being opened, then DW UART 8250 driver won't be able to alter the baud-clock. It shall just use the available clock rate. Similarly another device also won't manage to change the rate at that time. If nothing else have the exclusive rate access acquired except DW UART 8250 driver, then the driver will be able to alter the rate as much as it needs to in accordance with the currently implemented logic. Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> Cc: Maxim Kaurkin <Maxim.Kaurkin@baikalelectronics.ru> Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru> Cc: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru> Cc: Ekaterina Skachko <Ekaterina.Skachko@baikalelectronics.ru> Cc: Vadim Vlasov <V.Vlasov@baikalelectronics.ru> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> Cc: Paul Burton <paulburton@kernel.org> Cc: Ralf Baechle <ralf@linux-mips.org> Cc: Maxime Ripard <mripard@kernel.org> Cc: Chen-Yu Tsai <wens@csie.org> CC: Ray Jui <rjui@broadcom.com> Cc: Scott Branden <sbranden@broadcom.com> Cc: Florian Fainelli <f.fainelli@gmail.com> Cc: Wei Xu <xuwei5@hisilicon.com> Cc: Jason Cooper <jason@lakedaemon.net> Cc: Andrew Lunn <andrew@lunn.ch> Cc: Gregory Clement <gregory.clement@bootlin.com> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> Cc: Jisheng Zhang <Jisheng.Zhang@synaptics.com> Cc: Heiko Stuebner <heiko@sntech.de> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> Cc: Russell King <linux@armlinux.org.uk> Cc: linux-arm-kernel@lists.infradead.org Cc: Michael Turquette <mturquette@baylibre.com> Cc: Stephen Boyd <sboyd@kernel.org> Cc: linux-clk@vger.kernel.org --- Changelog v2: - Move exclusive ref clock lock/unlock precudures to the 8250 port startup/shutdown methods. - The changelog message has also been slightly modified due to the alteration. - Remove Alexey' SoB tag. - Cc someone from ARM who might be concerned regarding this change. - Cc someone from Clocks Framework to get their comments on this patch. --- drivers/tty/serial/8250/8250_dw.c | 36 +++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c index aab3cccc6789..08f3f745ed54 100644 --- a/drivers/tty/serial/8250/8250_dw.c +++ b/drivers/tty/serial/8250/8250_dw.c @@ -319,6 +319,40 @@ static void dw8250_set_ldisc(struct uart_port *p, struct ktermios *termios) serial8250_do_set_ldisc(p, termios); } +static int dw8250_startup(struct uart_port *p) +{ + struct dw8250_data *d = to_dw8250_data(p->private_data); + + /* + * Some platforms may provide a reference clock shared between several + * devices. In this case before using the serial port first we have to + * make sure nothing will change the rate behind our back and second + * the tty/serial subsystem knows the actual reference clock rate of + * the port. + */ + if (clk_rate_exclusive_get(d->clk)) { + dev_warn(p->dev, "Couldn't lock the clock rate\n"); + } else if (d->clk) { + p->uartclk = clk_get_rate(d->clk); + if (!p->uartclk) { + clk_rate_exclusive_put(d->clk); + dev_err(p->dev, "Clock rate not defined\n"); + return -EINVAL; + } + } + + return serial8250_do_startup(p); +} + +static void dw8250_shutdown(struct uart_port *p) +{ + struct dw8250_data *d = to_dw8250_data(p->private_data); + + serial8250_do_shutdown(p); + + clk_rate_exclusive_put(d->clk); +} + /* * dw8250_fallback_dma_filter will prevent the UART from getting just any free * channel on platforms that have DMA engines, but don't have any channels @@ -414,6 +448,8 @@ static int dw8250_probe(struct platform_device *pdev) p->serial_out = dw8250_serial_out; p->set_ldisc = dw8250_set_ldisc; p->set_termios = dw8250_set_termios; + p->startup = dw8250_startup; + p->shutdown = dw8250_shutdown; p->membase = devm_ioremap(dev, regs->start, resource_size(regs)); if (!p->membase) -- 2.25.1 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v2] serial: 8250_dw: Fix common clocks usage race condition 2020-03-23 2:46 ` [PATCH v2] " Sergey.Semin @ 2020-03-23 9:20 ` Andy Shevchenko 2020-03-23 11:11 ` Sergey Semin 2020-03-23 10:01 ` Maxime Ripard 2020-05-06 23:31 ` [PATCH v3 0/4] serial: 8250_dw: Fix ref clock usage Serge Semin 2 siblings, 1 reply; 29+ messages in thread From: Andy Shevchenko @ 2020-03-23 9:20 UTC (permalink / raw) To: Sergey.Semin Cc: Greg Kroah-Hartman, Jiri Slaby, Serge Semin, Alexey Malahov, Maxim Kaurkin, Pavel Parkhomenko, Ramil Zaripov, Ekaterina Skachko, Vadim Vlasov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Maxime Ripard, Chen-Yu Tsai, Ray Jui, Scott Branden, Florian Fainelli, Wei Xu, Jason Cooper, Andrew Lunn, Gregory Clement, Sebastian Hesselbarth, Jisheng Zhang, Heiko Stuebner, Catalin Marinas, Will Deacon, Russell King, linux-arm-kernel, Michael Turquette, Stephen Boyd, linux-clk, Heikki Krogerus, Kefeng Wang, linux-serial, linux-kernel On Mon, Mar 23, 2020 at 05:46:09AM +0300, Sergey.Semin@baikalelectronics.ru wrote: > From: Serge Semin <Sergey.Semin@baikalelectronics.ru> The question to CLK framework maintainers, is it correct approach in general for this case? > There are races possible in the dw8250_set_termios() callback method > and while the device is in PM suspend state. A race condition may > happen if the baudrate clock source device is shared with some other > device (in our machine it's another DW UART port). In this case if that > device changes the clock rate while serial console is using it the > DW 8250 UART port might not only end up with an invalid uartclk value > saved, but may also experience a distorted output data since baud-clock > could have been changed. In order to fix this lets enable an exclusive > reference clock rate access in case if "baudclk" device is specified. > > So if some other device also acquires the rate exclusivity during the > time of a DW UART 8250 port being opened, then DW UART 8250 driver > won't be able to alter the baud-clock. It shall just use the available > clock rate. Similarly another device also won't manage to change the > rate at that time. If nothing else have the exclusive rate access > acquired except DW UART 8250 driver, then the driver will be able to > alter the rate as much as it needs to in accordance with the currently > implemented logic. Thank you for an update, my comments below. ... > +static int dw8250_startup(struct uart_port *p) > +{ > + struct dw8250_data *d = to_dw8250_data(p->private_data); > + > + /* > + * Some platforms may provide a reference clock shared between several > + * devices. In this case before using the serial port first we have to > + * make sure nothing will change the rate behind our back and second > + * the tty/serial subsystem knows the actual reference clock rate of > + * the port. > + */ > + if (clk_rate_exclusive_get(d->clk)) { > + dev_warn(p->dev, "Couldn't lock the clock rate\n"); So, if this fails, in ->shutdown you will disbalance reference count, or did I miss something? > + } else if (d->clk) { > + p->uartclk = clk_get_rate(d->clk); > + if (!p->uartclk) { > + clk_rate_exclusive_put(d->clk); > + dev_err(p->dev, "Clock rate not defined\n"); > + return -EINVAL; > + } This operations I didn't get. If we have d->clk and suddenly get 0 as a rate (and note, that we still update uartclk member!), we try to put (why?) the exclusiveness of rate. > + } > + > + return serial8250_do_startup(p); > +} > + > +static void dw8250_shutdown(struct uart_port *p) > +{ > + struct dw8250_data *d = to_dw8250_data(p->private_data); > + > + serial8250_do_shutdown(p); > + > + clk_rate_exclusive_put(d->clk); > +} -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] serial: 8250_dw: Fix common clocks usage race condition 2020-03-23 9:20 ` Andy Shevchenko @ 2020-03-23 11:11 ` Sergey Semin 2020-03-23 11:52 ` Andy Shevchenko 0 siblings, 1 reply; 29+ messages in thread From: Sergey Semin @ 2020-03-23 11:11 UTC (permalink / raw) To: Andy Shevchenko Cc: Greg Kroah-Hartman, Jiri Slaby, Alexey Malahov, Maxim Kaurkin, Pavel Parkhomenko, Ramil Zaripov, Ekaterina Skachko, Vadim Vlasov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Maxime Ripard, Chen-Yu Tsai, Ray Jui, Scott Branden, Florian Fainelli, Wei Xu, Jason Cooper, Andrew Lunn, Gregory Clement, Sebastian Hesselbarth, Jisheng Zhang, Heiko Stuebner, Catalin Marinas, Will Deacon, Russell King, linux-arm-kernel, Michael Turquette, Stephen Boyd, linux-clk, Heikki Krogerus, Kefeng Wang, linux-serial, linux-kernel Hi On Mon, Mar 23, 2020 at 11:20:51AM +0200, Andy Shevchenko wrote: > On Mon, Mar 23, 2020 at 05:46:09AM +0300, Sergey.Semin@baikalelectronics.ru wrote: > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > The question to CLK framework maintainers, is it correct approach in general > for this case? > You should have been more specific then, if you wanted to see someone special. > On Wed, Mar 18, 2020 at 05:19:53PM +0200, Andy Shevchenko wrote: >> Also it would be nice to see come clock framework guys' opinions... Who can give a better comments regarding the clk API if not the subsystem maintainers? > > There are races possible in the dw8250_set_termios() callback method > > and while the device is in PM suspend state. A race condition may > > happen if the baudrate clock source device is shared with some other > > device (in our machine it's another DW UART port). In this case if that > > device changes the clock rate while serial console is using it the > > DW 8250 UART port might not only end up with an invalid uartclk value > > saved, but may also experience a distorted output data since baud-clock > > could have been changed. In order to fix this lets enable an exclusive > > reference clock rate access in case if "baudclk" device is specified. > > > > So if some other device also acquires the rate exclusivity during the > > time of a DW UART 8250 port being opened, then DW UART 8250 driver > > won't be able to alter the baud-clock. It shall just use the available > > clock rate. Similarly another device also won't manage to change the > > rate at that time. If nothing else have the exclusive rate access > > acquired except DW UART 8250 driver, then the driver will be able to > > alter the rate as much as it needs to in accordance with the currently > > implemented logic. > > Thank you for an update, my comments below. > > ... > > > +static int dw8250_startup(struct uart_port *p) > > +{ > > + struct dw8250_data *d = to_dw8250_data(p->private_data); > > + > > + /* > > + * Some platforms may provide a reference clock shared between several > > + * devices. In this case before using the serial port first we have to > > + * make sure nothing will change the rate behind our back and second > > + * the tty/serial subsystem knows the actual reference clock rate of > > + * the port. > > + */ > > > + if (clk_rate_exclusive_get(d->clk)) { > > + dev_warn(p->dev, "Couldn't lock the clock rate\n"); > > So, if this fails, in ->shutdown you will disbalance reference count, or did I > miss something? > Hm, you are right. I didn't fully thought this through. The thing is that according to the clk_rate_exclusive_get() function code currently it never fails. Though this isn't excuse for introducing a prone to future bugs code. Anyway if according to design a function may return an error we must take into account in the code using it. Due to this obligation and seeing we can't easily detect whether clk_rate_exclusive_get() has been failed while the driver is being executed in the shutdown method, the best approach would be to just return an error in startup method in case of the clock rate exclusivity acquisition failure. If you are ok with this, I'll have it fixed in v3 patchset. > > + } else if (d->clk) { > > > + p->uartclk = clk_get_rate(d->clk); > > + if (!p->uartclk) { > > + clk_rate_exclusive_put(d->clk); > > + dev_err(p->dev, "Clock rate not defined\n"); > > + return -EINVAL; > > + } > > This operations I didn't get. If we have d->clk and suddenly get 0 as a rate > (and note, that we still update uartclk member!), we try to put (why?) the > exclusiveness of rate. > Here is what I had in my mind while implementing this code. If d->clk isn't NULL, then there is a "baudclk" clock handler and we can use it to alter/retrieve the baud clock rate. But the same clock could be used by some other driver and that driver could have changed the rate while we didn't have this tty port started up (opened). In this case that driver could also have the clock exclusively acquired. So instead of trying to set the current p->uartclk rate to the clock, check the return value, if it's an error, try to get the current clock rate, check the return value, and so on, I just get the current baud clock rate and make sure the value is not zero (clk_get_rate() returns a zero rate in case of internal errors). At the same time dw8250_set_termios() will try to update the baud clock rate anyway (also by the serial core at the point of the port startup), so we don't need such complication in the DW 8250 port startup code. > (and note, that we still update uartclk member!), Yes, if we can't determine the current baud clock rate, then the there is a problem with the clock device, so we don't know at what rate it's currently working. Zero is the most appropriate value to be set in this case. > we try to put (why?) the > exclusiveness of rate. Yes, we put the exclusivity and return an error, because this if-branch has been taken only if the exclusivity has been successfully acquired. Regards, -Sergey > > + } > > + > > + return serial8250_do_startup(p); > > +} > > + > > +static void dw8250_shutdown(struct uart_port *p) > > +{ > > + struct dw8250_data *d = to_dw8250_data(p->private_data); > > + > > + serial8250_do_shutdown(p); > > + > > + clk_rate_exclusive_put(d->clk); > > +} > > -- > With Best Regards, > Andy Shevchenko > > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] serial: 8250_dw: Fix common clocks usage race condition 2020-03-23 11:11 ` Sergey Semin @ 2020-03-23 11:52 ` Andy Shevchenko 2020-03-23 17:07 ` Sergey Semin 0 siblings, 1 reply; 29+ messages in thread From: Andy Shevchenko @ 2020-03-23 11:52 UTC (permalink / raw) To: Sergey Semin Cc: Greg Kroah-Hartman, Jiri Slaby, Alexey Malahov, Maxim Kaurkin, Pavel Parkhomenko, Ramil Zaripov, Ekaterina Skachko, Vadim Vlasov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Maxime Ripard, Chen-Yu Tsai, Ray Jui, Scott Branden, Florian Fainelli, Wei Xu, Jason Cooper, Andrew Lunn, Gregory Clement, Sebastian Hesselbarth, Jisheng Zhang, Heiko Stuebner, Catalin Marinas, Will Deacon, Russell King, linux-arm-kernel, Michael Turquette, Stephen Boyd, linux-clk, Heikki Krogerus, Kefeng Wang, linux-serial, linux-kernel On Mon, Mar 23, 2020 at 02:11:49PM +0300, Sergey Semin wrote: > On Mon, Mar 23, 2020 at 11:20:51AM +0200, Andy Shevchenko wrote: > > On Mon, Mar 23, 2020 at 05:46:09AM +0300, Sergey.Semin@baikalelectronics.ru wrote: > > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > The question to CLK framework maintainers, is it correct approach in general > > for this case? > > You should have been more specific then, if you wanted to see someone > special. I didn't get your comment here. Since you put the question under a pile of words in the commit message, and actually in the changelog, not even in the message, I repeated it clearly that clock maintainers can see it. > > On Wed, Mar 18, 2020 at 05:19:53PM +0200, Andy Shevchenko wrote: > >> Also it would be nice to see come clock framework guys' opinions... > > Who can give a better comments regarding the clk API if not the > subsystem maintainers? You already got one from Maxime. ... > > > + /* > > > + * Some platforms may provide a reference clock shared between several > > > + * devices. In this case before using the serial port first we have to > > > + * make sure nothing will change the rate behind our back and second > > > + * the tty/serial subsystem knows the actual reference clock rate of > > > + * the port. > > > + */ > > > > > + if (clk_rate_exclusive_get(d->clk)) { > > > + dev_warn(p->dev, "Couldn't lock the clock rate\n"); > > > > So, if this fails, in ->shutdown you will disbalance reference count, or did I > > miss something? > > > > Hm, you are right. I didn't fully thought this through. The thing is > that according to the clk_rate_exclusive_get() function code currently > it never fails. Though this isn't excuse for introducing a prone to future > bugs code. > > Anyway if according to design a function may return an error we must take > into account in the code using it. Due to this obligation and seeing we can't > easily detect whether clk_rate_exclusive_get() has been failed while the > driver is being executed in the shutdown method, the best approach would be > to just return an error in startup method in case of the clock rate exclusivity > acquisition failure. If you are ok with this, I'll have it fixed in v3 > patchset. It needs to be carefully tested on other platforms than yours. > > > + } else if (d->clk) { > > > > > + p->uartclk = clk_get_rate(d->clk); > > > + if (!p->uartclk) { > > > + clk_rate_exclusive_put(d->clk); > > > + dev_err(p->dev, "Clock rate not defined\n"); > > > + return -EINVAL; > > > + } > > > > This operations I didn't get. If we have d->clk and suddenly get 0 as a rate > > (and note, that we still update uartclk member!), we try to put (why?) the > > exclusiveness of rate. > > > > Here is what I had in my mind while implementing this code. If d->clk > isn't NULL, then there is a "baudclk" clock handler and we can use it to > alter/retrieve the baud clock rate. But the same clock could be used by > some other driver and that driver could have changed the rate while we > didn't have this tty port started up (opened). In this case that driver > could also have the clock exclusively acquired. So instead of trying to > set the current p->uartclk rate to the clock, check the return value, > if it's an error, try to get the current clock rate, check the return > value, and so on, I just get the current baud clock rate and make sure > the value is not zero > (clk_get_rate() returns a zero rate in case of > internal errors). Have you considered !CLK case? > At the same time dw8250_set_termios() will try to update > the baud clock rate anyway (also by the serial core at the point of the port > startup), so we don't need such complication in the DW 8250 port startup > code. > > > (and note, that we still update uartclk member!), > > Yes, if we can't determine the current baud clock rate, then the there is > a problem with the clock device, so we don't know at what rate it's > currently working. Zero is the most appropriate value to be set in this case. > > > we try to put (why?) the > exclusiveness of rate. > > Yes, we put the exclusivity and return an error, because this if-branch has > been taken only if the exclusivity has been successfully acquired. So, this means that above code requires elaboration in the comments to explain how it supposed to work. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] serial: 8250_dw: Fix common clocks usage race condition 2020-03-23 11:52 ` Andy Shevchenko @ 2020-03-23 17:07 ` Sergey Semin 0 siblings, 0 replies; 29+ messages in thread From: Sergey Semin @ 2020-03-23 17:07 UTC (permalink / raw) To: Andy Shevchenko Cc: Greg Kroah-Hartman, Jiri Slaby, Alexey Malahov, Maxim Kaurkin, Pavel Parkhomenko, Ramil Zaripov, Ekaterina Skachko, Vadim Vlasov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Maxime Ripard, Chen-Yu Tsai, Ray Jui, Scott Branden, Florian Fainelli, Wei Xu, Jason Cooper, Andrew Lunn, Gregory Clement, Sebastian Hesselbarth, Jisheng Zhang, Heiko Stuebner, Catalin Marinas, Will Deacon, Russell King, linux-arm-kernel, Michael Turquette, Stephen Boyd, linux-clk, Heikki Krogerus, Kefeng Wang, linux-serial, linux-kernel On Mon, Mar 23, 2020 at 01:52:25PM +0200, Andy Shevchenko wrote: > On Mon, Mar 23, 2020 at 02:11:49PM +0300, Sergey Semin wrote: > > On Mon, Mar 23, 2020 at 11:20:51AM +0200, Andy Shevchenko wrote: > > > On Mon, Mar 23, 2020 at 05:46:09AM +0300, Sergey.Semin@baikalelectronics.ru wrote: > > > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > > > The question to CLK framework maintainers, is it correct approach in general > > > for this case? > > > > You should have been more specific then, if you wanted to see someone > > special. > > I didn't get your comment here. Since you put the question under a pile of > words in the commit message, and actually in the changelog, not even in the > message, I repeated it clearly that clock maintainers can see it. > > > > On Wed, Mar 18, 2020 at 05:19:53PM +0200, Andy Shevchenko wrote: > > >> Also it would be nice to see come clock framework guys' opinions... > > > > Who can give a better comments regarding the clk API if not the > > subsystem maintainers? > > You already got one from Maxime. > > ... > > > > > + /* > > > > + * Some platforms may provide a reference clock shared between several > > > > + * devices. In this case before using the serial port first we have to > > > > + * make sure nothing will change the rate behind our back and second > > > > + * the tty/serial subsystem knows the actual reference clock rate of > > > > + * the port. > > > > + */ > > > > > > > + if (clk_rate_exclusive_get(d->clk)) { > > > > + dev_warn(p->dev, "Couldn't lock the clock rate\n"); > > > > > > So, if this fails, in ->shutdown you will disbalance reference count, or did I > > > miss something? > > > > > > > Hm, you are right. I didn't fully thought this through. The thing is > > that according to the clk_rate_exclusive_get() function code currently > > it never fails. Though this isn't excuse for introducing a prone to future > > bugs code. > > > > Anyway if according to design a function may return an error we must take > > into account in the code using it. Due to this obligation and seeing we can't > > easily detect whether clk_rate_exclusive_get() has been failed while the > > driver is being executed in the shutdown method, the best approach would be > > to just return an error in startup method in case of the clock rate exclusivity > > acquisition failure. If you are ok with this, I'll have it fixed in v3 > > patchset. > > It needs to be carefully tested on other platforms than yours. > Alas I don't have one. But it can be done by other kernel users at rc-s stage of the next kernel release. > > > > + } else if (d->clk) { > > > > > > > + p->uartclk = clk_get_rate(d->clk); > > > > + if (!p->uartclk) { > > > > + clk_rate_exclusive_put(d->clk); > > > > + dev_err(p->dev, "Clock rate not defined\n"); > > > > + return -EINVAL; > > > > + } > > > > > > This operations I didn't get. If we have d->clk and suddenly get 0 as a rate > > > (and note, that we still update uartclk member!), we try to put (why?) the > > > exclusiveness of rate. > > > > > > > Here is what I had in my mind while implementing this code. If d->clk > > isn't NULL, then there is a "baudclk" clock handler and we can use it to > > alter/retrieve the baud clock rate. But the same clock could be used by > > some other driver and that driver could have changed the rate while we > > didn't have this tty port started up (opened). In this case that driver > > could also have the clock exclusively acquired. So instead of trying to > > set the current p->uartclk rate to the clock, check the return value, > > if it's an error, try to get the current clock rate, check the return > > value, and so on, I just get the current baud clock rate and make sure > > the value is not zero > > > (clk_get_rate() returns a zero rate in case of > > internal errors). > > Have you considered !CLK case? > Yes. It's a case of optional clock. Have a look at how the clock API works. You are already using it here in this driver when calling clk_prepare_enable()/clk_disable_unprepare(). > > At the same time dw8250_set_termios() will try to update > > the baud clock rate anyway (also by the serial core at the point of the port > > startup), so we don't need such complication in the DW 8250 port startup > > code. > > > > > (and note, that we still update uartclk member!), > > > > Yes, if we can't determine the current baud clock rate, then the there is > > a problem with the clock device, so we don't know at what rate it's > > currently working. Zero is the most appropriate value to be set in this case. > > > > > we try to put (why?) the > exclusiveness of rate. > > > > Yes, we put the exclusivity and return an error, because this if-branch has > > been taken only if the exclusivity has been successfully acquired. > > So, this means that above code requires elaboration in the comments to explain > how it supposed to work. > That's what I did by the comment: "... second the tty/serial subsystem knows the actual reference clock rate of the port." If you think, that checking a return value and undoing things in case of an error need elaboration in a comment I'll do it in v3. -Regards, Sergey > -- > With Best Regards, > Andy Shevchenko > > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] serial: 8250_dw: Fix common clocks usage race condition 2020-03-23 2:46 ` [PATCH v2] " Sergey.Semin 2020-03-23 9:20 ` Andy Shevchenko @ 2020-03-23 10:01 ` Maxime Ripard 2020-03-23 13:50 ` Sergey Semin 2020-05-06 23:31 ` [PATCH v3 0/4] serial: 8250_dw: Fix ref clock usage Serge Semin 2 siblings, 1 reply; 29+ messages in thread From: Maxime Ripard @ 2020-03-23 10:01 UTC (permalink / raw) To: Sergey.Semin Cc: Andy Shevchenko, Greg Kroah-Hartman, Jiri Slaby, Serge Semin, Alexey Malahov, Maxim Kaurkin, Pavel Parkhomenko, Ramil Zaripov, Ekaterina Skachko, Vadim Vlasov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Chen-Yu Tsai, Ray Jui, Scott Branden, Florian Fainelli, Wei Xu, Jason Cooper, Andrew Lunn, Gregory Clement, Sebastian Hesselbarth, Jisheng Zhang, Heiko Stuebner, Catalin Marinas, Will Deacon, Russell King, linux-arm-kernel, Michael Turquette, Stephen Boyd, linux-clk, Heikki Krogerus, Kefeng Wang, linux-serial, linux-kernel [-- Attachment #1: Type: text/plain, Size: 5274 bytes --] Hi, On Mon, Mar 23, 2020 at 05:46:09AM +0300, Sergey.Semin@baikalelectronics.ru wrote: > From: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > There are races possible in the dw8250_set_termios() callback method > and while the device is in PM suspend state. A race condition may > happen if the baudrate clock source device is shared with some other > device (in our machine it's another DW UART port). In this case if that > device changes the clock rate while serial console is using it the > DW 8250 UART port might not only end up with an invalid uartclk value > saved, but may also experience a distorted output data since baud-clock > could have been changed. In order to fix this lets enable an exclusive > reference clock rate access in case if "baudclk" device is specified. > > So if some other device also acquires the rate exclusivity during the > time of a DW UART 8250 port being opened, then DW UART 8250 driver > won't be able to alter the baud-clock. It shall just use the available > clock rate. Similarly another device also won't manage to change the > rate at that time. If nothing else have the exclusive rate access > acquired except DW UART 8250 driver, then the driver will be able to > alter the rate as much as it needs to in accordance with the currently > implemented logic. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> > Cc: Maxim Kaurkin <Maxim.Kaurkin@baikalelectronics.ru> > Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru> > Cc: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru> > Cc: Ekaterina Skachko <Ekaterina.Skachko@baikalelectronics.ru> > Cc: Vadim Vlasov <V.Vlasov@baikalelectronics.ru> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > Cc: Paul Burton <paulburton@kernel.org> > Cc: Ralf Baechle <ralf@linux-mips.org> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Chen-Yu Tsai <wens@csie.org> > CC: Ray Jui <rjui@broadcom.com> > Cc: Scott Branden <sbranden@broadcom.com> > Cc: Florian Fainelli <f.fainelli@gmail.com> > Cc: Wei Xu <xuwei5@hisilicon.com> > Cc: Jason Cooper <jason@lakedaemon.net> > Cc: Andrew Lunn <andrew@lunn.ch> > Cc: Gregory Clement <gregory.clement@bootlin.com> > Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> > Cc: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > Cc: Heiko Stuebner <heiko@sntech.de> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Russell King <linux@armlinux.org.uk> > Cc: linux-arm-kernel@lists.infradead.org > Cc: Michael Turquette <mturquette@baylibre.com> > Cc: Stephen Boyd <sboyd@kernel.org> > Cc: linux-clk@vger.kernel.org > > --- > > Changelog v2: > - Move exclusive ref clock lock/unlock precudures to the 8250 port > startup/shutdown methods. > - The changelog message has also been slightly modified due to the > alteration. > - Remove Alexey' SoB tag. > - Cc someone from ARM who might be concerned regarding this change. > - Cc someone from Clocks Framework to get their comments on this patch. > --- > drivers/tty/serial/8250/8250_dw.c | 36 +++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c > index aab3cccc6789..08f3f745ed54 100644 > --- a/drivers/tty/serial/8250/8250_dw.c > +++ b/drivers/tty/serial/8250/8250_dw.c > @@ -319,6 +319,40 @@ static void dw8250_set_ldisc(struct uart_port *p, struct ktermios *termios) > serial8250_do_set_ldisc(p, termios); > } > > +static int dw8250_startup(struct uart_port *p) > +{ > + struct dw8250_data *d = to_dw8250_data(p->private_data); > + > + /* > + * Some platforms may provide a reference clock shared between several > + * devices. In this case before using the serial port first we have to > + * make sure nothing will change the rate behind our back and second > + * the tty/serial subsystem knows the actual reference clock rate of > + * the port. > + */ > + if (clk_rate_exclusive_get(d->clk)) { > + dev_warn(p->dev, "Couldn't lock the clock rate\n"); > + } else if (d->clk) { > + p->uartclk = clk_get_rate(d->clk); > + if (!p->uartclk) { > + clk_rate_exclusive_put(d->clk); > + dev_err(p->dev, "Clock rate not defined\n"); > + return -EINVAL; > + } > + } > + > + return serial8250_do_startup(p); > +} I've been facing that issue, so it would be great to get it fixed, but I'm not sure this is the right solution. clk_rate_exclusive_get is pretty intrusive, and due to the usual topology of clock trees, this will lock down 3-4 parent clocks to their current rate as well. In the Allwinner SoCs case for example, this will lock down the same PLL than the one used by the CPU, preventing cpufreq from running. However, the 8250 has a pretty wide range of dividers and can adapt to any reasonable parent clock rate, so we don't really need to lock the rate either, we can simply react to a parent clock rate change using the clock notifiers, just like the SiFive UART is doing. I tried to do that, but given that I don't really have an extensive knowledge of the 8250, I couldn't find a way to stop the TX of chars while we change the clock rate. I'm not sure if this is a big deal or not, the SiFive UART doesn't seem to care. Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] serial: 8250_dw: Fix common clocks usage race condition 2020-03-23 10:01 ` Maxime Ripard @ 2020-03-23 13:50 ` Sergey Semin 2020-03-24 9:41 ` Maxime Ripard 2020-03-24 10:12 ` Andy Shevchenko 0 siblings, 2 replies; 29+ messages in thread From: Sergey Semin @ 2020-03-23 13:50 UTC (permalink / raw) To: Maxime Ripard Cc: Andy Shevchenko, Greg Kroah-Hartman, Jiri Slaby, Alexey Malahov, Maxim Kaurkin, Pavel Parkhomenko, Ramil Zaripov, Ekaterina Skachko, Vadim Vlasov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Chen-Yu Tsai, Ray Jui, Scott Branden, Florian Fainelli, Wei Xu, Jason Cooper, Andrew Lunn, Gregory Clement, Sebastian Hesselbarth, Jisheng Zhang, Heiko Stuebner, Catalin Marinas, Will Deacon, Russell King, linux-arm-kernel, Michael Turquette, Stephen Boyd, linux-clk, Heikki Krogerus, Kefeng Wang, linux-serial, linux-kernel Hello Maxime On Mon, Mar 23, 2020 at 11:01:09AM +0100, Maxime Ripard wrote: > Hi, > > On Mon, Mar 23, 2020 at 05:46:09AM +0300, Sergey.Semin@baikalelectronics.ru wrote: > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > There are races possible in the dw8250_set_termios() callback method > > and while the device is in PM suspend state. A race condition may > > happen if the baudrate clock source device is shared with some other > > device (in our machine it's another DW UART port). In this case if that > > device changes the clock rate while serial console is using it the > > DW 8250 UART port might not only end up with an invalid uartclk value > > saved, but may also experience a distorted output data since baud-clock > > could have been changed. In order to fix this lets enable an exclusive > > reference clock rate access in case if "baudclk" device is specified. > > > > So if some other device also acquires the rate exclusivity during the > > time of a DW UART 8250 port being opened, then DW UART 8250 driver > > won't be able to alter the baud-clock. It shall just use the available > > clock rate. Similarly another device also won't manage to change the > > rate at that time. If nothing else have the exclusive rate access > > acquired except DW UART 8250 driver, then the driver will be able to > > alter the rate as much as it needs to in accordance with the currently > > implemented logic. > > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> > > Cc: Maxim Kaurkin <Maxim.Kaurkin@baikalelectronics.ru> > > Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru> > > Cc: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru> > > Cc: Ekaterina Skachko <Ekaterina.Skachko@baikalelectronics.ru> > > Cc: Vadim Vlasov <V.Vlasov@baikalelectronics.ru> > > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > > Cc: Paul Burton <paulburton@kernel.org> > > Cc: Ralf Baechle <ralf@linux-mips.org> > > Cc: Maxime Ripard <mripard@kernel.org> > > Cc: Chen-Yu Tsai <wens@csie.org> > > CC: Ray Jui <rjui@broadcom.com> > > Cc: Scott Branden <sbranden@broadcom.com> > > Cc: Florian Fainelli <f.fainelli@gmail.com> > > Cc: Wei Xu <xuwei5@hisilicon.com> > > Cc: Jason Cooper <jason@lakedaemon.net> > > Cc: Andrew Lunn <andrew@lunn.ch> > > Cc: Gregory Clement <gregory.clement@bootlin.com> > > Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> > > Cc: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > > Cc: Heiko Stuebner <heiko@sntech.de> > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > Cc: Will Deacon <will@kernel.org> > > Cc: Russell King <linux@armlinux.org.uk> > > Cc: linux-arm-kernel@lists.infradead.org > > Cc: Michael Turquette <mturquette@baylibre.com> > > Cc: Stephen Boyd <sboyd@kernel.org> > > Cc: linux-clk@vger.kernel.org > > > > --- > > > > Changelog v2: > > - Move exclusive ref clock lock/unlock precudures to the 8250 port > > startup/shutdown methods. > > - The changelog message has also been slightly modified due to the > > alteration. > > - Remove Alexey' SoB tag. > > - Cc someone from ARM who might be concerned regarding this change. > > - Cc someone from Clocks Framework to get their comments on this patch. > > --- > > drivers/tty/serial/8250/8250_dw.c | 36 +++++++++++++++++++++++++++++++ > > 1 file changed, 36 insertions(+) > > > > diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c > > index aab3cccc6789..08f3f745ed54 100644 > > --- a/drivers/tty/serial/8250/8250_dw.c > > +++ b/drivers/tty/serial/8250/8250_dw.c > > @@ -319,6 +319,40 @@ static void dw8250_set_ldisc(struct uart_port *p, struct ktermios *termios) > > serial8250_do_set_ldisc(p, termios); > > } > > > > +static int dw8250_startup(struct uart_port *p) > > +{ > > + struct dw8250_data *d = to_dw8250_data(p->private_data); > > + > > + /* > > + * Some platforms may provide a reference clock shared between several > > + * devices. In this case before using the serial port first we have to > > + * make sure nothing will change the rate behind our back and second > > + * the tty/serial subsystem knows the actual reference clock rate of > > + * the port. > > + */ > > + if (clk_rate_exclusive_get(d->clk)) { > > + dev_warn(p->dev, "Couldn't lock the clock rate\n"); > > + } else if (d->clk) { > > + p->uartclk = clk_get_rate(d->clk); > > + if (!p->uartclk) { > > + clk_rate_exclusive_put(d->clk); > > + dev_err(p->dev, "Clock rate not defined\n"); > > + return -EINVAL; > > + } > > + } > > + > > + return serial8250_do_startup(p); > > +} > > I've been facing that issue, so it would be great to get it fixed, but > I'm not sure this is the right solution. > > clk_rate_exclusive_get is pretty intrusive, and due to the usual > topology of clock trees, this will lock down 3-4 parent clocks to > their current rate as well. In the Allwinner SoCs case for example, > this will lock down the same PLL than the one used by the CPU, > preventing cpufreq from running. > Speaking about weak design of a SoC' clock tree. Our problems are nothing with respect to the Allwinner SoC, in which case of changing the CPU-frequency may cause the UART glitches subsequently causing data transfer artefacts.) Moreover as I can see the same issue may raise for I2C, QSPI, PWM devices there. Anyway your concern does make sense. > However, the 8250 has a pretty wide range of dividers and can adapt to > any reasonable parent clock rate, so we don't really need to lock the > rate either, we can simply react to a parent clock rate change using > the clock notifiers, just like the SiFive UART is doing. > > I tried to do that, but given that I don't really have an extensive > knowledge of the 8250, I couldn't find a way to stop the TX of chars > while we change the clock rate. I'm not sure if this is a big deal or > not, the SiFive UART doesn't seem to care. > > Maxime Yes, your solution is also possible, but even in case of stopping Tx/Rx it doesn't lack drawbacks. First of all AFAIK there is no easy way to just pause the transfers. We'd have to first wait for the current transfers to be completed, then somehow lock the port usage (both Tx and Rx traffic), permit the reference clock rate change, accordingly adjust the UART clock divider, and finally unlock the port. While if we don't mind to occasionally have UART data glitches, we can just adjust the UART ref divider synchronously with ref clock rate change as you and SiFive UART driver suggest. So we are now at a zugzwang - a fork to three not that good solutions: 1) lock the whole clock branch and provide a glitchless interfaces. But by doing so we may (in case of Allwinner SoCs we will) lockup some very important functionality like CPU-frequency change while the UART port is started up. In this case we won't have the data glitches. 2) just adjust the UART clock divider in case of reference clock rate change (use the SiFive UART driver approach). In this case we may have the data corruption. 3) somehow implement the algo: wait for the transfers to be completed, lock UART interface (it's possible for Tx, but for Rx in case of no handshake enabled it's simply impossible), permit the ref clock rate change, adjust the UART divider, then unlock the UART interface. In this case the data glitches still may happen (if no modem control is available or handshakes are disabled). As for the cases of Baikal-T1 UARTs the first solutions is the most suitable. We don't lock anything valuable, since a base PLL output isn't directly connected to any device and it's rate once setup isn't changed during the system running. On the other hand I don't mind to implement the second solution, even though it's prone to data glitches. Regarding the solution 3) I won't even try. It's too complicated, I don't have time and test-infrastructure for this. So Andy what do you think? Regards, -Sergey ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] serial: 8250_dw: Fix common clocks usage race condition 2020-03-23 13:50 ` Sergey Semin @ 2020-03-24 9:41 ` Maxime Ripard 2020-03-24 10:12 ` Andy Shevchenko 1 sibling, 0 replies; 29+ messages in thread From: Maxime Ripard @ 2020-03-24 9:41 UTC (permalink / raw) To: Sergey Semin Cc: Andy Shevchenko, Greg Kroah-Hartman, Jiri Slaby, Alexey Malahov, Maxim Kaurkin, Pavel Parkhomenko, Ramil Zaripov, Ekaterina Skachko, Vadim Vlasov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Chen-Yu Tsai, Ray Jui, Scott Branden, Florian Fainelli, Wei Xu, Jason Cooper, Andrew Lunn, Gregory Clement, Sebastian Hesselbarth, Jisheng Zhang, Heiko Stuebner, Catalin Marinas, Will Deacon, Russell King, linux-arm-kernel, Michael Turquette, Stephen Boyd, linux-clk, Heikki Krogerus, Kefeng Wang, linux-serial, linux-kernel [-- Attachment #1: Type: text/plain, Size: 7559 bytes --] On Mon, Mar 23, 2020 at 04:50:17PM +0300, Sergey Semin wrote: > On Mon, Mar 23, 2020 at 11:01:09AM +0100, Maxime Ripard wrote: > > Hi, > > > > On Mon, Mar 23, 2020 at 05:46:09AM +0300, Sergey.Semin@baikalelectronics.ru wrote: > > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > > > There are races possible in the dw8250_set_termios() callback method > > > and while the device is in PM suspend state. A race condition may > > > happen if the baudrate clock source device is shared with some other > > > device (in our machine it's another DW UART port). In this case if that > > > device changes the clock rate while serial console is using it the > > > DW 8250 UART port might not only end up with an invalid uartclk value > > > saved, but may also experience a distorted output data since baud-clock > > > could have been changed. In order to fix this lets enable an exclusive > > > reference clock rate access in case if "baudclk" device is specified. > > > > > > So if some other device also acquires the rate exclusivity during the > > > time of a DW UART 8250 port being opened, then DW UART 8250 driver > > > won't be able to alter the baud-clock. It shall just use the available > > > clock rate. Similarly another device also won't manage to change the > > > rate at that time. If nothing else have the exclusive rate access > > > acquired except DW UART 8250 driver, then the driver will be able to > > > alter the rate as much as it needs to in accordance with the currently > > > implemented logic. > > > > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> > > > Cc: Maxim Kaurkin <Maxim.Kaurkin@baikalelectronics.ru> > > > Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru> > > > Cc: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru> > > > Cc: Ekaterina Skachko <Ekaterina.Skachko@baikalelectronics.ru> > > > Cc: Vadim Vlasov <V.Vlasov@baikalelectronics.ru> > > > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > > > Cc: Paul Burton <paulburton@kernel.org> > > > Cc: Ralf Baechle <ralf@linux-mips.org> > > > Cc: Maxime Ripard <mripard@kernel.org> > > > Cc: Chen-Yu Tsai <wens@csie.org> > > > CC: Ray Jui <rjui@broadcom.com> > > > Cc: Scott Branden <sbranden@broadcom.com> > > > Cc: Florian Fainelli <f.fainelli@gmail.com> > > > Cc: Wei Xu <xuwei5@hisilicon.com> > > > Cc: Jason Cooper <jason@lakedaemon.net> > > > Cc: Andrew Lunn <andrew@lunn.ch> > > > Cc: Gregory Clement <gregory.clement@bootlin.com> > > > Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> > > > Cc: Jisheng Zhang <Jisheng.Zhang@synaptics.com> > > > Cc: Heiko Stuebner <heiko@sntech.de> > > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > > Cc: Will Deacon <will@kernel.org> > > > Cc: Russell King <linux@armlinux.org.uk> > > > Cc: linux-arm-kernel@lists.infradead.org > > > Cc: Michael Turquette <mturquette@baylibre.com> > > > Cc: Stephen Boyd <sboyd@kernel.org> > > > Cc: linux-clk@vger.kernel.org > > > > > > --- > > > > > > Changelog v2: > > > - Move exclusive ref clock lock/unlock precudures to the 8250 port > > > startup/shutdown methods. > > > - The changelog message has also been slightly modified due to the > > > alteration. > > > - Remove Alexey' SoB tag. > > > - Cc someone from ARM who might be concerned regarding this change. > > > - Cc someone from Clocks Framework to get their comments on this patch. > > > --- > > > drivers/tty/serial/8250/8250_dw.c | 36 +++++++++++++++++++++++++++++++ > > > 1 file changed, 36 insertions(+) > > > > > > diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c > > > index aab3cccc6789..08f3f745ed54 100644 > > > --- a/drivers/tty/serial/8250/8250_dw.c > > > +++ b/drivers/tty/serial/8250/8250_dw.c > > > @@ -319,6 +319,40 @@ static void dw8250_set_ldisc(struct uart_port *p, struct ktermios *termios) > > > serial8250_do_set_ldisc(p, termios); > > > } > > > > > > +static int dw8250_startup(struct uart_port *p) > > > +{ > > > + struct dw8250_data *d = to_dw8250_data(p->private_data); > > > + > > > + /* > > > + * Some platforms may provide a reference clock shared between several > > > + * devices. In this case before using the serial port first we have to > > > + * make sure nothing will change the rate behind our back and second > > > + * the tty/serial subsystem knows the actual reference clock rate of > > > + * the port. > > > + */ > > > + if (clk_rate_exclusive_get(d->clk)) { > > > + dev_warn(p->dev, "Couldn't lock the clock rate\n"); > > > + } else if (d->clk) { > > > + p->uartclk = clk_get_rate(d->clk); > > > + if (!p->uartclk) { > > > + clk_rate_exclusive_put(d->clk); > > > + dev_err(p->dev, "Clock rate not defined\n"); > > > + return -EINVAL; > > > + } > > > + } > > > + > > > + return serial8250_do_startup(p); > > > +} > > > > I've been facing that issue, so it would be great to get it fixed, but > > I'm not sure this is the right solution. > > > > clk_rate_exclusive_get is pretty intrusive, and due to the usual > > topology of clock trees, this will lock down 3-4 parent clocks to > > their current rate as well. In the Allwinner SoCs case for example, > > this will lock down the same PLL than the one used by the CPU, > > preventing cpufreq from running. > > > > Speaking about weak design of a SoC' clock tree. Our problems are nothing > with respect to the Allwinner SoC, in which case of changing the > CPU-frequency may cause the UART glitches subsequently causing data > transfer artefacts.) I just remembered the RPi3/4 are in this situation too. > Moreover as I can see the same issue may raise for I2C, QSPI, PWM > devices there. We also have the I2C on the same parent clock in the Allwinner SoCs indeed :) > Anyway your concern does make sense. > > > However, the 8250 has a pretty wide range of dividers and can adapt to > > any reasonable parent clock rate, so we don't really need to lock the > > rate either, we can simply react to a parent clock rate change using > > the clock notifiers, just like the SiFive UART is doing. > > > > I tried to do that, but given that I don't really have an extensive > > knowledge of the 8250, I couldn't find a way to stop the TX of chars > > while we change the clock rate. I'm not sure if this is a big deal or > > not, the SiFive UART doesn't seem to care. > > Yes, your solution is also possible, but even in case of stopping Tx/Rx it > doesn't lack drawbacks. First of all AFAIK there is no easy way to just > pause the transfers. We'd have to first wait for the current transfers > to be completed, then somehow lock the port usage (both Tx and Rx > traffic), permit the reference clock rate change, accordingly adjust the > UART clock divider, and finally unlock the port. While if we don't mind > to occasionally have UART data glitches, we can just adjust the UART ref > divider synchronously with ref clock rate change as you and SiFive UART > driver suggest. > > So we are now at a zugzwang - a fork to three not that good solutions: > 1) lock the whole clock branch and provide a glitchless interfaces. But > by doing so we may (in case of Allwinner SoCs we will) lockup some very > important functionality like CPU-frequency change while the UART port is > started up. In this case we won't have the data glitches. This solution, at least without a whitelist of platforms where it makes sense, would be NAK'd for me. And I'm strongly suspecting that that whitelist would be very short, so the overall usefulness is pretty limited. Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] serial: 8250_dw: Fix common clocks usage race condition 2020-03-23 13:50 ` Sergey Semin 2020-03-24 9:41 ` Maxime Ripard @ 2020-03-24 10:12 ` Andy Shevchenko 2020-03-25 17:11 ` Sergey Semin 1 sibling, 1 reply; 29+ messages in thread From: Andy Shevchenko @ 2020-03-24 10:12 UTC (permalink / raw) To: Sergey Semin Cc: Maxime Ripard, Greg Kroah-Hartman, Jiri Slaby, Alexey Malahov, Maxim Kaurkin, Pavel Parkhomenko, Ramil Zaripov, Ekaterina Skachko, Vadim Vlasov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Chen-Yu Tsai, Ray Jui, Scott Branden, Florian Fainelli, Wei Xu, Jason Cooper, Andrew Lunn, Gregory Clement, Sebastian Hesselbarth, Jisheng Zhang, Heiko Stuebner, Catalin Marinas, Will Deacon, Russell King, linux-arm-kernel, Michael Turquette, Stephen Boyd, linux-clk, Heikki Krogerus, Kefeng Wang, linux-serial, linux-kernel On Mon, Mar 23, 2020 at 04:50:17PM +0300, Sergey Semin wrote: > On Mon, Mar 23, 2020 at 11:01:09AM +0100, Maxime Ripard wrote: > > On Mon, Mar 23, 2020 at 05:46:09AM +0300, Sergey.Semin@baikalelectronics.ru wrote: > > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > > > There are races possible in the dw8250_set_termios() callback method > > > and while the device is in PM suspend state. A race condition may > > > happen if the baudrate clock source device is shared with some other > > > device (in our machine it's another DW UART port). In this case if that > > > device changes the clock rate while serial console is using it the > > > DW 8250 UART port might not only end up with an invalid uartclk value > > > saved, but may also experience a distorted output data since baud-clock > > > could have been changed. In order to fix this lets enable an exclusive > > > reference clock rate access in case if "baudclk" device is specified. > > > > > > So if some other device also acquires the rate exclusivity during the > > > time of a DW UART 8250 port being opened, then DW UART 8250 driver > > > won't be able to alter the baud-clock. It shall just use the available > > > clock rate. Similarly another device also won't manage to change the > > > rate at that time. If nothing else have the exclusive rate access > > > acquired except DW UART 8250 driver, then the driver will be able to > > > alter the rate as much as it needs to in accordance with the currently > > > implemented logic. > > clk_rate_exclusive_get is pretty intrusive, and due to the usual > > topology of clock trees, this will lock down 3-4 parent clocks to > > their current rate as well. In the Allwinner SoCs case for example, > > this will lock down the same PLL than the one used by the CPU, > > preventing cpufreq from running. > > Speaking about weak design of a SoC' clock tree. Our problems are nothing > with respect to the Allwinner SoC, in which case of changing the > CPU-frequency may cause the UART glitches subsequently causing data > transfer artefacts.) Moreover as I can see the same issue may raise for > I2C, QSPI, PWM devices there. > > Anyway your concern does make sense. > > > However, the 8250 has a pretty wide range of dividers and can adapt to > > any reasonable parent clock rate, so we don't really need to lock the > > rate either, we can simply react to a parent clock rate change using > > the clock notifiers, just like the SiFive UART is doing. > > > > I tried to do that, but given that I don't really have an extensive > > knowledge of the 8250, I couldn't find a way to stop the TX of chars > > while we change the clock rate. I'm not sure if this is a big deal or > > not, the SiFive UART doesn't seem to care. > > Yes, your solution is also possible, but even in case of stopping Tx/Rx it > doesn't lack drawbacks. First of all AFAIK there is no easy way to just > pause the transfers. We'd have to first wait for the current transfers > to be completed, then somehow lock the port usage (both Tx and Rx > traffic), permit the reference clock rate change, accordingly adjust the > UART clock divider, and finally unlock the port. While if we don't mind > to occasionally have UART data glitches, we can just adjust the UART ref > divider synchronously with ref clock rate change as you and SiFive UART > driver suggest. > > So we are now at a zugzwang - a fork to three not that good solutions: > 1) lock the whole clock branch and provide a glitchless interfaces. But > by doing so we may (in case of Allwinner SoCs we will) lockup some very > important functionality like CPU-frequency change while the UART port is > started up. In this case we won't have the data glitches. > 2) just adjust the UART clock divider in case of reference clock rate > change (use the SiFive UART driver approach). In this case we may have the > data corruption. > 3) somehow implement the algo: wait for the transfers to be completed, > lock UART interface (it's possible for Tx, but for Rx in case of no handshake > enabled it's simply impossible), permit the ref clock rate change, > adjust the UART divider, then unlock the UART interface. In this case the data > glitches still may happen (if no modem control is available or > handshakes are disabled). > > As for the cases of Baikal-T1 UARTs the first solutions is the most suitable. > We don't lock anything valuable, since a base PLL output isn't directly > connected to any device and it's rate once setup isn't changed during the > system running. On the other hand I don't mind to implement the second > solution, even though it's prone to data glitches. Regarding the solution > 3) I won't even try. It's too complicated, I don't have time and > test-infrastructure for this. > > So Andy what do you think? From Intel HW perspective the first two are okay, but since Maxime is against first, you have the only option from your list. Perhaps somebody may give option 4) here... -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] serial: 8250_dw: Fix common clocks usage race condition 2020-03-24 10:12 ` Andy Shevchenko @ 2020-03-25 17:11 ` Sergey Semin 2020-03-26 1:44 ` Stephen Boyd 0 siblings, 1 reply; 29+ messages in thread From: Sergey Semin @ 2020-03-25 17:11 UTC (permalink / raw) To: Andy Shevchenko Cc: Maxime Ripard, Greg Kroah-Hartman, Jiri Slaby, Alexey Malahov, Maxim Kaurkin, Pavel Parkhomenko, Ramil Zaripov, Ekaterina Skachko, Vadim Vlasov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Chen-Yu Tsai, Ray Jui, Scott Branden, Florian Fainelli, Wei Xu, Jason Cooper, Andrew Lunn, Gregory Clement, Sebastian Hesselbarth, Jisheng Zhang, Heiko Stuebner, Catalin Marinas, Will Deacon, Russell King, linux-arm-kernel, Michael Turquette, Stephen Boyd, linux-clk, Heikki Krogerus, Kefeng Wang, linux-serial, linux-kernel On Tue, Mar 24, 2020 at 12:12:43PM +0200, Andy Shevchenko wrote: > On Mon, Mar 23, 2020 at 04:50:17PM +0300, Sergey Semin wrote: > > On Mon, Mar 23, 2020 at 11:01:09AM +0100, Maxime Ripard wrote: > > > On Mon, Mar 23, 2020 at 05:46:09AM +0300, Sergey.Semin@baikalelectronics.ru wrote: > > > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > > > > > There are races possible in the dw8250_set_termios() callback method > > > > and while the device is in PM suspend state. A race condition may > > > > happen if the baudrate clock source device is shared with some other > > > > device (in our machine it's another DW UART port). In this case if that > > > > device changes the clock rate while serial console is using it the > > > > DW 8250 UART port might not only end up with an invalid uartclk value > > > > saved, but may also experience a distorted output data since baud-clock > > > > could have been changed. In order to fix this lets enable an exclusive > > > > reference clock rate access in case if "baudclk" device is specified. > > > > > > > > So if some other device also acquires the rate exclusivity during the > > > > time of a DW UART 8250 port being opened, then DW UART 8250 driver > > > > won't be able to alter the baud-clock. It shall just use the available > > > > clock rate. Similarly another device also won't manage to change the > > > > rate at that time. If nothing else have the exclusive rate access > > > > acquired except DW UART 8250 driver, then the driver will be able to > > > > alter the rate as much as it needs to in accordance with the currently > > > > implemented logic. > > > > clk_rate_exclusive_get is pretty intrusive, and due to the usual > > > topology of clock trees, this will lock down 3-4 parent clocks to > > > their current rate as well. In the Allwinner SoCs case for example, > > > this will lock down the same PLL than the one used by the CPU, > > > preventing cpufreq from running. > > > > Speaking about weak design of a SoC' clock tree. Our problems are nothing > > with respect to the Allwinner SoC, in which case of changing the > > CPU-frequency may cause the UART glitches subsequently causing data > > transfer artefacts.) Moreover as I can see the same issue may raise for > > I2C, QSPI, PWM devices there. > > > > Anyway your concern does make sense. > > > > > However, the 8250 has a pretty wide range of dividers and can adapt to > > > any reasonable parent clock rate, so we don't really need to lock the > > > rate either, we can simply react to a parent clock rate change using > > > the clock notifiers, just like the SiFive UART is doing. > > > > > > I tried to do that, but given that I don't really have an extensive > > > knowledge of the 8250, I couldn't find a way to stop the TX of chars > > > while we change the clock rate. I'm not sure if this is a big deal or > > > not, the SiFive UART doesn't seem to care. > > > > Yes, your solution is also possible, but even in case of stopping Tx/Rx it > > doesn't lack drawbacks. First of all AFAIK there is no easy way to just > > pause the transfers. We'd have to first wait for the current transfers > > to be completed, then somehow lock the port usage (both Tx and Rx > > traffic), permit the reference clock rate change, accordingly adjust the > > UART clock divider, and finally unlock the port. While if we don't mind > > to occasionally have UART data glitches, we can just adjust the UART ref > > divider synchronously with ref clock rate change as you and SiFive UART > > driver suggest. > > > > So we are now at a zugzwang - a fork to three not that good solutions: > > 1) lock the whole clock branch and provide a glitchless interfaces. But > > by doing so we may (in case of Allwinner SoCs we will) lockup some very > > important functionality like CPU-frequency change while the UART port is > > started up. In this case we won't have the data glitches. > > 2) just adjust the UART clock divider in case of reference clock rate > > change (use the SiFive UART driver approach). In this case we may have the > > data corruption. > > 3) somehow implement the algo: wait for the transfers to be completed, > > lock UART interface (it's possible for Tx, but for Rx in case of no handshake > > enabled it's simply impossible), permit the ref clock rate change, > > adjust the UART divider, then unlock the UART interface. In this case the data > > glitches still may happen (if no modem control is available or > > handshakes are disabled). > > > > As for the cases of Baikal-T1 UARTs the first solutions is the most suitable. > > We don't lock anything valuable, since a base PLL output isn't directly > > connected to any device and it's rate once setup isn't changed during the > > system running. On the other hand I don't mind to implement the second > > solution, even though it's prone to data glitches. Regarding the solution > > 3) I won't even try. It's too complicated, I don't have time and > > test-infrastructure for this. > > > > So Andy what do you think? > > From Intel HW perspective the first two are okay, but since Maxime is against > first, you have the only option from your list. Perhaps somebody may give > option 4) here... > Ok then. I'll implement the option 2) in v3 if noone gives any alternatives before that. Regards, -Sergey > -- > With Best Regards, > Andy Shevchenko > > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] serial: 8250_dw: Fix common clocks usage race condition 2020-03-25 17:11 ` Sergey Semin @ 2020-03-26 1:44 ` Stephen Boyd 2020-03-27 9:12 ` Sergey Semin 0 siblings, 1 reply; 29+ messages in thread From: Stephen Boyd @ 2020-03-26 1:44 UTC (permalink / raw) To: Andy Shevchenko, Sergey Semin Cc: Maxime Ripard, Greg Kroah-Hartman, Jiri Slaby, Alexey Malahov, Maxim Kaurkin, Pavel Parkhomenko, Ramil Zaripov, Ekaterina Skachko, Vadim Vlasov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Chen-Yu Tsai, Ray Jui, Scott Branden, Florian Fainelli, Wei Xu, Jason Cooper, Andrew Lunn, Gregory Clement, Sebastian Hesselbarth, Jisheng Zhang, Heiko Stuebner, Catalin Marina s, Will Deacon, Russell King, linux-arm-kernel, Michael Turquette, linux-clk, Heikki Krogerus, Kefeng Wang, linux-serial, linux-kernel Quoting Sergey Semin (2020-03-25 10:11:09) > On Tue, Mar 24, 2020 at 12:12:43PM +0200, Andy Shevchenko wrote: > > On Mon, Mar 23, 2020 at 04:50:17PM +0300, Sergey Semin wrote: > > > On Mon, Mar 23, 2020 at 11:01:09AM +0100, Maxime Ripard wrote: > > > > > > clk_rate_exclusive_get is pretty intrusive, and due to the usual > > > > topology of clock trees, this will lock down 3-4 parent clocks to > > > > their current rate as well. In the Allwinner SoCs case for example, > > > > this will lock down the same PLL than the one used by the CPU, > > > > preventing cpufreq from running. > > > > > > Speaking about weak design of a SoC' clock tree. Our problems are nothing > > > with respect to the Allwinner SoC, in which case of changing the > > > CPU-frequency may cause the UART glitches subsequently causing data > > > transfer artefacts.) Moreover as I can see the same issue may raise for > > > I2C, QSPI, PWM devices there. > > > > > > Anyway your concern does make sense. > > > > > > > However, the 8250 has a pretty wide range of dividers and can adapt to > > > > any reasonable parent clock rate, so we don't really need to lock the > > > > rate either, we can simply react to a parent clock rate change using > > > > the clock notifiers, just like the SiFive UART is doing. > > > > > > > > I tried to do that, but given that I don't really have an extensive > > > > knowledge of the 8250, I couldn't find a way to stop the TX of chars > > > > while we change the clock rate. I'm not sure if this is a big deal or > > > > not, the SiFive UART doesn't seem to care. > > > > > > Yes, your solution is also possible, but even in case of stopping Tx/Rx it > > > doesn't lack drawbacks. First of all AFAIK there is no easy way to just > > > pause the transfers. We'd have to first wait for the current transfers > > > to be completed, then somehow lock the port usage (both Tx and Rx > > > traffic), permit the reference clock rate change, accordingly adjust the > > > UART clock divider, and finally unlock the port. While if we don't mind > > > to occasionally have UART data glitches, we can just adjust the UART ref > > > divider synchronously with ref clock rate change as you and SiFive UART > > > driver suggest. > > > > > > So we are now at a zugzwang - a fork to three not that good solutions: > > > 1) lock the whole clock branch and provide a glitchless interfaces. But > > > by doing so we may (in case of Allwinner SoCs we will) lockup some very > > > important functionality like CPU-frequency change while the UART port is > > > started up. In this case we won't have the data glitches. > > > 2) just adjust the UART clock divider in case of reference clock rate > > > change (use the SiFive UART driver approach). In this case we may have the > > > data corruption. > > > 3) somehow implement the algo: wait for the transfers to be completed, > > > lock UART interface (it's possible for Tx, but for Rx in case of no handshake > > > enabled it's simply impossible), permit the ref clock rate change, > > > adjust the UART divider, then unlock the UART interface. In this case the data > > > glitches still may happen (if no modem control is available or > > > handshakes are disabled). > > > > > > As for the cases of Baikal-T1 UARTs the first solutions is the most suitable. > > > We don't lock anything valuable, since a base PLL output isn't directly > > > connected to any device and it's rate once setup isn't changed during the > > > system running. On the other hand I don't mind to implement the second > > > solution, even though it's prone to data glitches. Regarding the solution > > > 3) I won't even try. It's too complicated, I don't have time and > > > test-infrastructure for this. > > > > > > So Andy what do you think? > > > > From Intel HW perspective the first two are okay, but since Maxime is against > > first, you have the only option from your list. Perhaps somebody may give > > option 4) here... > > > > Ok then. I'll implement the option 2) in v3 if noone gives any alternatives > before that. > Sorry, I haven't really read the thread but I'll quickly reply with this. Maybe option 4 is to make the uart driver a clk provider that consumes the single reference clk like it is already doing today? Then when the rate changes up above for the clk implemented here the clk set rate op for the newly implemented clk can go poke the uart registers to maintain the baud or whatever? That is close to how the notifier design would work, but it avoids keeping the notifiers around given that the notifiers are not preferred. It is also closer to reality, the uart has a divider or mux internally that we don't model as a clk, but we could just as easily model as such. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] serial: 8250_dw: Fix common clocks usage race condition 2020-03-26 1:44 ` Stephen Boyd @ 2020-03-27 9:12 ` Sergey Semin 0 siblings, 0 replies; 29+ messages in thread From: Sergey Semin @ 2020-03-27 9:12 UTC (permalink / raw) To: Stephen Boyd Cc: Andy Shevchenko, Maxime Ripard, Greg Kroah-Hartman, Jiri Slaby, Alexey Malahov, Maxim Kaurkin, Pavel Parkhomenko, Ramil Zaripov, Ekaterina Skachko, Vadim Vlasov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Chen-Yu Tsai, Ray Jui, Scott Branden, Florian Fainelli, Wei Xu, Jason Cooper, Andrew Lunn, Gregory Clement, Sebastian Hesselbarth, Jisheng Zhang, Heiko Stuebner, Catalin Marina s, Will Deacon, Russell King, linux-arm-kernel, Michael Turquette, linux-clk, Heikki Krogerus, Kefeng Wang, linux-serial, linux-kernel Hello Stephen Thanks for reply. My comment is below. On Wed, Mar 25, 2020 at 06:44:53PM -0700, Stephen Boyd wrote: > Quoting Sergey Semin (2020-03-25 10:11:09) > > On Tue, Mar 24, 2020 at 12:12:43PM +0200, Andy Shevchenko wrote: > > > On Mon, Mar 23, 2020 at 04:50:17PM +0300, Sergey Semin wrote: > > > > On Mon, Mar 23, 2020 at 11:01:09AM +0100, Maxime Ripard wrote: > > > > > > > > clk_rate_exclusive_get is pretty intrusive, and due to the usual > > > > > topology of clock trees, this will lock down 3-4 parent clocks to > > > > > their current rate as well. In the Allwinner SoCs case for example, > > > > > this will lock down the same PLL than the one used by the CPU, > > > > > preventing cpufreq from running. > > > > > > > > Speaking about weak design of a SoC' clock tree. Our problems are nothing > > > > with respect to the Allwinner SoC, in which case of changing the > > > > CPU-frequency may cause the UART glitches subsequently causing data > > > > transfer artefacts.) Moreover as I can see the same issue may raise for > > > > I2C, QSPI, PWM devices there. > > > > > > > > Anyway your concern does make sense. > > > > > > > > > However, the 8250 has a pretty wide range of dividers and can adapt to > > > > > any reasonable parent clock rate, so we don't really need to lock the > > > > > rate either, we can simply react to a parent clock rate change using > > > > > the clock notifiers, just like the SiFive UART is doing. > > > > > > > > > > I tried to do that, but given that I don't really have an extensive > > > > > knowledge of the 8250, I couldn't find a way to stop the TX of chars > > > > > while we change the clock rate. I'm not sure if this is a big deal or > > > > > not, the SiFive UART doesn't seem to care. > > > > > > > > Yes, your solution is also possible, but even in case of stopping Tx/Rx it > > > > doesn't lack drawbacks. First of all AFAIK there is no easy way to just > > > > pause the transfers. We'd have to first wait for the current transfers > > > > to be completed, then somehow lock the port usage (both Tx and Rx > > > > traffic), permit the reference clock rate change, accordingly adjust the > > > > UART clock divider, and finally unlock the port. While if we don't mind > > > > to occasionally have UART data glitches, we can just adjust the UART ref > > > > divider synchronously with ref clock rate change as you and SiFive UART > > > > driver suggest. > > > > > > > > So we are now at a zugzwang - a fork to three not that good solutions: > > > > 1) lock the whole clock branch and provide a glitchless interfaces. But > > > > by doing so we may (in case of Allwinner SoCs we will) lockup some very > > > > important functionality like CPU-frequency change while the UART port is > > > > started up. In this case we won't have the data glitches. > > > > 2) just adjust the UART clock divider in case of reference clock rate > > > > change (use the SiFive UART driver approach). In this case we may have the > > > > data corruption. > > > > 3) somehow implement the algo: wait for the transfers to be completed, > > > > lock UART interface (it's possible for Tx, but for Rx in case of no handshake > > > > enabled it's simply impossible), permit the ref clock rate change, > > > > adjust the UART divider, then unlock the UART interface. In this case the data > > > > glitches still may happen (if no modem control is available or > > > > handshakes are disabled). > > > > > > > > As for the cases of Baikal-T1 UARTs the first solutions is the most suitable. > > > > We don't lock anything valuable, since a base PLL output isn't directly > > > > connected to any device and it's rate once setup isn't changed during the > > > > system running. On the other hand I don't mind to implement the second > > > > solution, even though it's prone to data glitches. Regarding the solution > > > > 3) I won't even try. It's too complicated, I don't have time and > > > > test-infrastructure for this. > > > > > > > > So Andy what do you think? > > > > > > From Intel HW perspective the first two are okay, but since Maxime is against > > > first, you have the only option from your list. Perhaps somebody may give > > > option 4) here... > > > > > > > Ok then. I'll implement the option 2) in v3 if noone gives any alternatives > > before that. > > > > Sorry, I haven't really read the thread but I'll quickly reply with > this. > > Maybe option 4 is to make the uart driver a clk provider that consumes > the single reference clk like it is already doing today? Then when the > rate changes up above for the clk implemented here the clk set rate op > for the newly implemented clk can go poke the uart registers to maintain > the baud or whatever? > > That is close to how the notifier design would work, but it avoids > keeping the notifiers around given that the notifiers are not preferred. > It is also closer to reality, the uart has a divider or mux internally > that we don't model as a clk, but we could just as easily model as such. AFAIU your suggestion is pretty similar to the option 2), but it concerns the fixup implementation. So instead of subscribing to the reference clock change event and directly adjusting the UART clock divider when the change happens, you suggest to convert the divisor setting code into a clock provider, which in case of the parental clocks rate change shall automatically cause the rate adjustment of the clocks hierarchy below. While your proposal looks neat and better suits a common approach of the drivers design, it won't be that easy to be implemented for the serial subsystem. As far as I can see serial and 8250 code is too coupled with manual divisor and reference clock settings. Common 8250 port code gets and sets the divisor and relies on the reference clock value. Similarly the 8250-compatible vendor specific devices may also have custom divisor settings. Moreover uartclk field, which indicates the reference clock rate value, is used in many placed over the serial code, so if we implemented your design we would have to update it value anyway, which means to subscribe to the reference clock rate change event. So in order to do what you said, the serial subsystem would have to be seriously refactored, which taking into account the subsystem age and number of driver, will be very painful. -Sergey ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 0/4] serial: 8250_dw: Fix ref clock usage 2020-03-23 2:46 ` [PATCH v2] " Sergey.Semin 2020-03-23 9:20 ` Andy Shevchenko 2020-03-23 10:01 ` Maxime Ripard @ 2020-05-06 23:31 ` Serge Semin 2020-05-06 23:31 ` [PATCH v3 1/4] serial: 8250: Fix max baud limit in generic 8250 port Serge Semin ` (4 more replies) 2 siblings, 5 replies; 29+ messages in thread From: Serge Semin @ 2020-05-06 23:31 UTC (permalink / raw) To: Thomas Bogendoerfer, Greg Kroah-Hartman, Jiri Slaby Cc: Serge Semin, Serge Semin, Alexey Malahov, Maxim Kaurkin, Pavel Parkhomenko, Alexey Kolotnikov, Ramil Zaripov, Ekaterina Skachko, Vadim Vlasov, Paul Burton, Ralf Baechle, Arnd Bergmann, Long Cheng, Andy Shevchenko, Maxime Ripard, Catalin Marinas, Will Deacon, Russell King, linux-mips, linux-arm-kernel, linux-mediatek, linux-serial, linux-kernel It might be dangerous if an UART port reference clock rate is suddenly changed. In particular the 8250 port drivers (and AFAICS most of the tty drivers using common clock framework clocks) rely either on the exclusive reference clock utilization or on the ref clock rate being always constant. Needless to say that it turns out not true and if some other service suddenly changes the clock rate behind an UART port driver back it's no good. So the port might not only end up with an invalid uartclk value saved, but may also experience a distorted output/input data since such action will effectively update the programmed baud-clock. We discovered such problem on Baikal-T1 SoC where two DW 8250 ports have got a shared reference clock. Allwinner SoC is equipped with an UART, which clock is derived from the CPU PLL clock source, so the CPU frequency change might be propagated down up to the serial port reference clock. This patchset provides a way to fix the problem to the 8250 serial port controllers and mostly fixes it for the DW 8250-compatible UART. I say mostly because due to not having a facility to pause/stop and resume/ restart on-going transfers we implemented the UART clock rate update procedure executed post factum of the actual reference clock rate change. In addition the patchset includes a few fixes we discovered when were working the issue. First one concerns the maximum baud rate setting used to determine a serial port baud based on the current UART port clock rate. Another one simplifies the ref clock rate setting procedure a bit. This patchset is rebased and tested on the mainline Linux kernel 5.7-rc4: 0e698dfa2822 ("Linux 5.7-rc4") tag: v5.7-rc4 Changelog v3: - Refactor the original patch to adjust the UART port divisor instead of requesting an exclusive ref clock utilization. Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> Cc: Maxim Kaurkin <Maxim.Kaurkin@baikalelectronics.ru> Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru> Cc: Alexey Kolotnikov <Alexey.Kolotnikov@baikalelectronics.ru> Cc: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru> Cc: Ekaterina Skachko <Ekaterina.Skachko@baikalelectronics.ru> Cc: Vadim Vlasov <V.Vlasov@baikalelectronics.ru> Cc: Alexey Kolotnikov <Alexey.Kolotnikov@baikalelectronics.ru> Cc: Paul Burton <paulburton@kernel.org> Cc: Ralf Baechle <ralf@linux-mips.org> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Long Cheng <long.cheng@mediatek.com> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> Cc: Russell King <linux@armlinux.org.uk> Cc: linux-mips@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-mediatek@lists.infradead.org Cc: linux-serial@vger.kernel.org Cc: linux-kernel@vger.kernel.org Serge Semin (4): serial: 8250: Fix max baud limit in generic 8250 port serial: 8250: Add 8250 port clock update method serial: 8250_dw: Simplify the ref clock rate setting procedure serial: 8250_dw: Fix common clocks usage race condition drivers/tty/serial/8250/8250_dw.c | 125 +++++++++++++++++++++++++--- drivers/tty/serial/8250/8250_port.c | 42 +++++++++- include/linux/serial_8250.h | 2 + 3 files changed, 156 insertions(+), 13 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 1/4] serial: 8250: Fix max baud limit in generic 8250 port 2020-05-06 23:31 ` [PATCH v3 0/4] serial: 8250_dw: Fix ref clock usage Serge Semin @ 2020-05-06 23:31 ` Serge Semin 2020-05-15 13:48 ` Andy Shevchenko 2020-05-06 23:31 ` [PATCH v3 2/4] serial: 8250: Add 8250 port clock update method Serge Semin ` (3 subsequent siblings) 4 siblings, 1 reply; 29+ messages in thread From: Serge Semin @ 2020-05-06 23:31 UTC (permalink / raw) To: Thomas Bogendoerfer, Greg Kroah-Hartman, Jiri Slaby, Long Cheng Cc: Serge Semin, Serge Semin, Alexey Malahov, Paul Burton, Ralf Baechle, Arnd Bergmann, Andy Shevchenko, Maxime Ripard, Catalin Marinas, Will Deacon, Russell King, linux-mips, linux-arm-kernel, linux-mediatek, Lukas Wunner, Mika Westerberg, Stefan Roese, Vignesh Raghavendra, linux-serial, linux-kernel Standard 8250 UART ports are designed in a way so they can communicate with baud rates up to 1/16 of a reference frequency. It's expected from most of the currently supported UART controllers. That's why the former version of serial8250_get_baud_rate() method called uart_get_baud_rate() with min and max baud rates passed as (port->uartclk / 16 / UART_DIV_MAX) and ((port->uartclk + tolerance) / 16) respectively. Doing otherwise, like it was suggested in commit ("serial: 8250_mtk: support big baud rate."), caused acceptance of bauds, which was higher than the normal UART controllers actually supported. As a result if some user-space program requested to set a baud greater than (uartclk / 16) it would have been permitted without truncation, but then serial8250_get_divisor(baud) (which calls uart_get_divisor() to get the reference clock divisor) would have returned a zero divisor. Setting zero divisor will cause an unpredictable effect varying from chip to chip. In case of DW APB UART the communications just stop. Lets fix this problem by getting back the limitation of (uartclk + tolerance) / 16 maximum baud supported by the generic 8250 port. Mediatek 8250 UART ports driver developer shouldn't have touched it in the first place notably seeing he already provided a custom version of set_termios() callback in that glue-driver which took into account the extended baud rate values and accordingly updated the standard and vendor-specific divisor latch registers anyway. Fixes: 81bb549fdf14 ("serial: 8250_mtk: support big baud rate.") Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> Cc: Paul Burton <paulburton@kernel.org> Cc: Ralf Baechle <ralf@linux-mips.org> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Long Cheng <long.cheng@mediatek.com> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> Cc: Russell King <linux@armlinux.org.uk> Cc: linux-mips@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-mediatek@lists.infradead.org --- drivers/tty/serial/8250/8250_port.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c index f77bf820b7a3..4d83c85a7389 100644 --- a/drivers/tty/serial/8250/8250_port.c +++ b/drivers/tty/serial/8250/8250_port.c @@ -2615,6 +2615,8 @@ static unsigned int serial8250_get_baud_rate(struct uart_port *port, struct ktermios *termios, struct ktermios *old) { + unsigned int tolerance = port->uartclk / 100; + /* * Ask the core to calculate the divisor for us. * Allow 1% tolerance at the upper limit so uart clks marginally @@ -2623,7 +2625,7 @@ static unsigned int serial8250_get_baud_rate(struct uart_port *port, */ return uart_get_baud_rate(port, termios, old, port->uartclk / 16 / UART_DIV_MAX, - port->uartclk); + (port->uartclk + tolerance) / 16); } void -- 2.25.1 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v3 1/4] serial: 8250: Fix max baud limit in generic 8250 port 2020-05-06 23:31 ` [PATCH v3 1/4] serial: 8250: Fix max baud limit in generic 8250 port Serge Semin @ 2020-05-15 13:48 ` Andy Shevchenko 0 siblings, 0 replies; 29+ messages in thread From: Andy Shevchenko @ 2020-05-15 13:48 UTC (permalink / raw) To: Serge Semin Cc: Thomas Bogendoerfer, Greg Kroah-Hartman, Jiri Slaby, Long Cheng, Serge Semin, Alexey Malahov, Paul Burton, Ralf Baechle, Arnd Bergmann, Maxime Ripard, Catalin Marinas, Will Deacon, Russell King, linux-mips, linux-arm-kernel, linux-mediatek, Lukas Wunner, Mika Westerberg, Stefan Roese, Vignesh Raghavendra, linux-serial, linux-kernel On Thu, May 07, 2020 at 02:31:32AM +0300, Serge Semin wrote: > Standard 8250 UART ports are designed in a way so they can communicate > with baud rates up to 1/16 of a reference frequency. It's expected from > most of the currently supported UART controllers. That's why the former > version of serial8250_get_baud_rate() method called uart_get_baud_rate() > with min and max baud rates passed as (port->uartclk / 16 / UART_DIV_MAX) > and ((port->uartclk + tolerance) / 16) respectively. Doing otherwise, like > it was suggested in commit ("serial: 8250_mtk: support big baud rate."), > caused acceptance of bauds, which was higher than the normal UART > controllers actually supported. As a result if some user-space program > requested to set a baud greater than (uartclk / 16) it would have been > permitted without truncation, but then serial8250_get_divisor(baud) > (which calls uart_get_divisor() to get the reference clock divisor) would > have returned a zero divisor. Setting zero divisor will cause an > unpredictable effect varying from chip to chip. In case of DW APB UART the > communications just stop. > > Lets fix this problem by getting back the limitation of (uartclk + > tolerance) / 16 maximum baud supported by the generic 8250 port. Mediatek > 8250 UART ports driver developer shouldn't have touched it in the first > place notably seeing he already provided a custom version of set_termios() > callback in that glue-driver which took into account the extended baud > rate values and accordingly updated the standard and vendor-specific > divisor latch registers anyway. Some of the hardware support PS != 16 (8250_mid), but for now it lies to UART core for real UART clock because of its (core) hard cored assumption PS == 16 here and there. Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Fixes: 81bb549fdf14 ("serial: 8250_mtk: support big baud rate.") > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > Cc: Paul Burton <paulburton@kernel.org> > Cc: Ralf Baechle <ralf@linux-mips.org> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Long Cheng <long.cheng@mediatek.com> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Russell King <linux@armlinux.org.uk> > Cc: linux-mips@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-mediatek@lists.infradead.org > --- > drivers/tty/serial/8250/8250_port.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c > index f77bf820b7a3..4d83c85a7389 100644 > --- a/drivers/tty/serial/8250/8250_port.c > +++ b/drivers/tty/serial/8250/8250_port.c > @@ -2615,6 +2615,8 @@ static unsigned int serial8250_get_baud_rate(struct uart_port *port, > struct ktermios *termios, > struct ktermios *old) > { > + unsigned int tolerance = port->uartclk / 100; > + > /* > * Ask the core to calculate the divisor for us. > * Allow 1% tolerance at the upper limit so uart clks marginally > @@ -2623,7 +2625,7 @@ static unsigned int serial8250_get_baud_rate(struct uart_port *port, > */ > return uart_get_baud_rate(port, termios, old, > port->uartclk / 16 / UART_DIV_MAX, > - port->uartclk); > + (port->uartclk + tolerance) / 16); > } > > void > -- > 2.25.1 > -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 2/4] serial: 8250: Add 8250 port clock update method 2020-05-06 23:31 ` [PATCH v3 0/4] serial: 8250_dw: Fix ref clock usage Serge Semin 2020-05-06 23:31 ` [PATCH v3 1/4] serial: 8250: Fix max baud limit in generic 8250 port Serge Semin @ 2020-05-06 23:31 ` Serge Semin 2020-05-15 12:45 ` Greg Kroah-Hartman 2020-05-06 23:31 ` [PATCH v3 3/4] serial: 8250_dw: Simplify the ref clock rate setting procedure Serge Semin ` (2 subsequent siblings) 4 siblings, 1 reply; 29+ messages in thread From: Serge Semin @ 2020-05-06 23:31 UTC (permalink / raw) To: Thomas Bogendoerfer, Greg Kroah-Hartman, Jiri Slaby Cc: Serge Semin, Serge Semin, Alexey Malahov, Paul Burton, Ralf Baechle, Arnd Bergmann, Long Cheng, Andy Shevchenko, Maxime Ripard, Catalin Marinas, Will Deacon, Russell King, linux-mips, linux-arm-kernel, linux-mediatek, Lukas Wunner, Stefan Roese, Mika Westerberg, Vignesh Raghavendra, Allison Randal, Yegor Yefremov, Thomas Gleixner, Dmitry Safonov, linux-serial, linux-kernel Some platforms can be designed in a way so the UART port reference clock might be asynchronously changed at some point. In Baikal-T1 SoC this may happen due to the reference clock being shared between two UART ports, on the Allwinner SoC the reference clock is derived from the CPU clock, so any CPU frequency change should get to be known/reflected by/in the UART controller as well. But it's not enough to just update the uart_port->uartclk field of the corresponding UART port, the 8250 controller reference clock divisor should be altered so to preserve current baud rate setting. All of these things is done in a coherent way by calling the serial8250_update_uartclk() method provided in this patch. Though note that it isn't supposed to be called from within the UART port callbacks because the locks using to the protect the UART port data are already taken in there. Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> Cc: Paul Burton <paulburton@kernel.org> Cc: Ralf Baechle <ralf@linux-mips.org> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Long Cheng <long.cheng@mediatek.com> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> Cc: Russell King <linux@armlinux.org.uk> Cc: linux-mips@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-mediatek@lists.infradead.org --- drivers/tty/serial/8250/8250_port.c | 38 +++++++++++++++++++++++++++++ include/linux/serial_8250.h | 2 ++ 2 files changed, 40 insertions(+) diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c index 4d83c85a7389..484ff9df1432 100644 --- a/drivers/tty/serial/8250/8250_port.c +++ b/drivers/tty/serial/8250/8250_port.c @@ -2628,6 +2628,44 @@ static unsigned int serial8250_get_baud_rate(struct uart_port *port, (port->uartclk + tolerance) / 16); } +/* + * Note in order to avoid the tty port mutex deadlock don't use the next method + * within the uart port callbacks. Primarily it's supposed to be utilized to + * handle a sudden reference clock rate change. + */ +void serial8250_update_uartclk(struct uart_port *port, unsigned int uartclk) +{ + struct uart_8250_port *up = up_to_u8250p(port); + unsigned int baud, quot, frac = 0; + struct ktermios *termios; + unsigned long flags; + + mutex_lock(&port->state->port.mutex); + + if (port->uartclk == uartclk) + goto out_lock; + + port->uartclk = uartclk; + termios = &port->state->port.tty->termios; + + baud = serial8250_get_baud_rate(port, termios, NULL); + quot = serial8250_get_divisor(port, baud, &frac); + + spin_lock_irqsave(&port->lock, flags); + + uart_update_timeout(port, termios->c_cflag, baud); + + serial8250_set_divisor(port, baud, quot, frac); + serial_port_out(port, UART_LCR, up->lcr); + serial8250_out_MCR(up, UART_MCR_DTR | UART_MCR_RTS); + + spin_unlock_irqrestore(&port->lock, flags); + +out_lock: + mutex_unlock(&port->state->port.mutex); +} +EXPORT_SYMBOL(serial8250_update_uartclk); + void serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios, struct ktermios *old) diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h index 6545f8cfc8fa..2b70f736b091 100644 --- a/include/linux/serial_8250.h +++ b/include/linux/serial_8250.h @@ -155,6 +155,8 @@ extern int early_serial_setup(struct uart_port *port); extern int early_serial8250_setup(struct earlycon_device *device, const char *options); +extern void serial8250_update_uartclk(struct uart_port *port, + unsigned int uartclk); extern void serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios, struct ktermios *old); extern void serial8250_do_set_ldisc(struct uart_port *port, -- 2.25.1 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v3 2/4] serial: 8250: Add 8250 port clock update method 2020-05-06 23:31 ` [PATCH v3 2/4] serial: 8250: Add 8250 port clock update method Serge Semin @ 2020-05-15 12:45 ` Greg Kroah-Hartman 2020-05-15 14:32 ` Serge Semin 0 siblings, 1 reply; 29+ messages in thread From: Greg Kroah-Hartman @ 2020-05-15 12:45 UTC (permalink / raw) To: Serge Semin Cc: Thomas Bogendoerfer, Jiri Slaby, Serge Semin, Alexey Malahov, Paul Burton, Ralf Baechle, Arnd Bergmann, Long Cheng, Andy Shevchenko, Maxime Ripard, Catalin Marinas, Will Deacon, Russell King, linux-mips, linux-arm-kernel, linux-mediatek, Lukas Wunner, Stefan Roese, Mika Westerberg, Vignesh Raghavendra, Allison Randal, Yegor Yefremov, Thomas Gleixner, Dmitry Safonov, linux-serial, linux-kernel On Thu, May 07, 2020 at 02:31:33AM +0300, Serge Semin wrote: > Some platforms can be designed in a way so the UART port reference clock > might be asynchronously changed at some point. In Baikal-T1 SoC this may > happen due to the reference clock being shared between two UART ports, on > the Allwinner SoC the reference clock is derived from the CPU clock, so > any CPU frequency change should get to be known/reflected by/in the UART > controller as well. But it's not enough to just update the > uart_port->uartclk field of the corresponding UART port, the 8250 > controller reference clock divisor should be altered so to preserve > current baud rate setting. All of these things is done in a coherent > way by calling the serial8250_update_uartclk() method provided in this > patch. Though note that it isn't supposed to be called from within the > UART port callbacks because the locks using to the protect the UART port > data are already taken in there. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > Cc: Paul Burton <paulburton@kernel.org> > Cc: Ralf Baechle <ralf@linux-mips.org> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Long Cheng <long.cheng@mediatek.com> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Russell King <linux@armlinux.org.uk> > Cc: linux-mips@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-mediatek@lists.infradead.org > --- > drivers/tty/serial/8250/8250_port.c | 38 +++++++++++++++++++++++++++++ > include/linux/serial_8250.h | 2 ++ > 2 files changed, 40 insertions(+) > > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c > index 4d83c85a7389..484ff9df1432 100644 > --- a/drivers/tty/serial/8250/8250_port.c > +++ b/drivers/tty/serial/8250/8250_port.c > @@ -2628,6 +2628,44 @@ static unsigned int serial8250_get_baud_rate(struct uart_port *port, > (port->uartclk + tolerance) / 16); > } > > +/* > + * Note in order to avoid the tty port mutex deadlock don't use the next method > + * within the uart port callbacks. Primarily it's supposed to be utilized to > + * handle a sudden reference clock rate change. > + */ > +void serial8250_update_uartclk(struct uart_port *port, unsigned int uartclk) > +{ > + struct uart_8250_port *up = up_to_u8250p(port); > + unsigned int baud, quot, frac = 0; > + struct ktermios *termios; > + unsigned long flags; > + > + mutex_lock(&port->state->port.mutex); > + > + if (port->uartclk == uartclk) > + goto out_lock; > + > + port->uartclk = uartclk; > + termios = &port->state->port.tty->termios; > + > + baud = serial8250_get_baud_rate(port, termios, NULL); > + quot = serial8250_get_divisor(port, baud, &frac); > + > + spin_lock_irqsave(&port->lock, flags); > + > + uart_update_timeout(port, termios->c_cflag, baud); > + > + serial8250_set_divisor(port, baud, quot, frac); > + serial_port_out(port, UART_LCR, up->lcr); > + serial8250_out_MCR(up, UART_MCR_DTR | UART_MCR_RTS); > + > + spin_unlock_irqrestore(&port->lock, flags); > + > +out_lock: > + mutex_unlock(&port->state->port.mutex); > +} > +EXPORT_SYMBOL(serial8250_update_uartclk); EXPORT_SYMBOL_GPL() please. thanks, greg k-h ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 2/4] serial: 8250: Add 8250 port clock update method 2020-05-15 12:45 ` Greg Kroah-Hartman @ 2020-05-15 14:32 ` Serge Semin 0 siblings, 0 replies; 29+ messages in thread From: Serge Semin @ 2020-05-15 14:32 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Serge Semin, Thomas Bogendoerfer, Jiri Slaby, Alexey Malahov, Paul Burton, Ralf Baechle, Arnd Bergmann, Long Cheng, Andy Shevchenko, Maxime Ripard, Catalin Marinas, Will Deacon, Russell King, linux-mips, linux-arm-kernel, linux-mediatek, Lukas Wunner, Stefan Roese, Mika Westerberg, Vignesh Raghavendra, Allison Randal, Yegor Yefremov, Thomas Gleixner, Dmitry Safonov, linux-serial, linux-kernel On Fri, May 15, 2020 at 02:45:25PM +0200, Greg Kroah-Hartman wrote: > On Thu, May 07, 2020 at 02:31:33AM +0300, Serge Semin wrote: > > Some platforms can be designed in a way so the UART port reference clock > > might be asynchronously changed at some point. In Baikal-T1 SoC this may > > happen due to the reference clock being shared between two UART ports, on > > the Allwinner SoC the reference clock is derived from the CPU clock, so > > any CPU frequency change should get to be known/reflected by/in the UART > > controller as well. But it's not enough to just update the > > uart_port->uartclk field of the corresponding UART port, the 8250 > > controller reference clock divisor should be altered so to preserve > > current baud rate setting. All of these things is done in a coherent > > way by calling the serial8250_update_uartclk() method provided in this > > patch. Though note that it isn't supposed to be called from within the > > UART port callbacks because the locks using to the protect the UART port > > data are already taken in there. > > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> > > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > > Cc: Paul Burton <paulburton@kernel.org> > > Cc: Ralf Baechle <ralf@linux-mips.org> > > Cc: Arnd Bergmann <arnd@arndb.de> > > Cc: Long Cheng <long.cheng@mediatek.com> > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Cc: Maxime Ripard <mripard@kernel.org> > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > Cc: Will Deacon <will@kernel.org> > > Cc: Russell King <linux@armlinux.org.uk> > > Cc: linux-mips@vger.kernel.org > > Cc: linux-arm-kernel@lists.infradead.org > > Cc: linux-mediatek@lists.infradead.org > > --- > > drivers/tty/serial/8250/8250_port.c | 38 +++++++++++++++++++++++++++++ > > include/linux/serial_8250.h | 2 ++ > > 2 files changed, 40 insertions(+) > > > > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c > > index 4d83c85a7389..484ff9df1432 100644 > > --- a/drivers/tty/serial/8250/8250_port.c > > +++ b/drivers/tty/serial/8250/8250_port.c > > @@ -2628,6 +2628,44 @@ static unsigned int serial8250_get_baud_rate(struct uart_port *port, > > (port->uartclk + tolerance) / 16); > > } > > > > +/* > > + * Note in order to avoid the tty port mutex deadlock don't use the next method > > + * within the uart port callbacks. Primarily it's supposed to be utilized to > > + * handle a sudden reference clock rate change. > > + */ > > +void serial8250_update_uartclk(struct uart_port *port, unsigned int uartclk) > > +{ > > + struct uart_8250_port *up = up_to_u8250p(port); > > + unsigned int baud, quot, frac = 0; > > + struct ktermios *termios; > > + unsigned long flags; > > + > > + mutex_lock(&port->state->port.mutex); > > + > > + if (port->uartclk == uartclk) > > + goto out_lock; > > + > > + port->uartclk = uartclk; > > + termios = &port->state->port.tty->termios; > > + > > + baud = serial8250_get_baud_rate(port, termios, NULL); > > + quot = serial8250_get_divisor(port, baud, &frac); > > + > > + spin_lock_irqsave(&port->lock, flags); > > + > > + uart_update_timeout(port, termios->c_cflag, baud); > > + > > + serial8250_set_divisor(port, baud, quot, frac); > > + serial_port_out(port, UART_LCR, up->lcr); > > + serial8250_out_MCR(up, UART_MCR_DTR | UART_MCR_RTS); > > + > > + spin_unlock_irqrestore(&port->lock, flags); > > + > > +out_lock: > > + mutex_unlock(&port->state->port.mutex); > > +} > > +EXPORT_SYMBOL(serial8250_update_uartclk); > > EXPORT_SYMBOL_GPL() please. Ok. I guess I've just copied the line from some of the export symbol statements below. My mistake. Sorry. -Sergey > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 3/4] serial: 8250_dw: Simplify the ref clock rate setting procedure 2020-05-06 23:31 ` [PATCH v3 0/4] serial: 8250_dw: Fix ref clock usage Serge Semin 2020-05-06 23:31 ` [PATCH v3 1/4] serial: 8250: Fix max baud limit in generic 8250 port Serge Semin 2020-05-06 23:31 ` [PATCH v3 2/4] serial: 8250: Add 8250 port clock update method Serge Semin @ 2020-05-06 23:31 ` Serge Semin 2020-05-15 14:05 ` Andy Shevchenko 2020-05-06 23:31 ` [PATCH v3 4/4] serial: 8250_dw: Fix common clocks usage race condition Serge Semin 2020-05-07 18:29 ` [PATCH v3 0/4] serial: 8250_dw: Fix ref clock usage Andy Shevchenko 4 siblings, 1 reply; 29+ messages in thread From: Serge Semin @ 2020-05-06 23:31 UTC (permalink / raw) To: Thomas Bogendoerfer, Greg Kroah-Hartman, Jiri Slaby, Andy Shevchenko Cc: Serge Semin, Serge Semin, Alexey Malahov, Paul Burton, Ralf Baechle, Arnd Bergmann, Long Cheng, Maxime Ripard, Catalin Marinas, Will Deacon, Russell King, linux-mips, linux-arm-kernel, linux-mediatek, Heikki Krogerus, Kefeng Wang, linux-serial, linux-kernel Really instead of twice checking the clk_round_rate() return value we could do it once, and if it isn't error the clock rate can be changed. By doing so we decrease a number of ret-value tests and remove a weird goto-based construction implemented in the dw8250_set_termios() method. Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> Cc: Paul Burton <paulburton@kernel.org> Cc: Ralf Baechle <ralf@linux-mips.org> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Long Cheng <long.cheng@mediatek.com> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> Cc: Russell King <linux@armlinux.org.uk> Cc: linux-mips@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-mediatek@lists.infradead.org --- drivers/tty/serial/8250/8250_dw.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c index aab3cccc6789..12866083731d 100644 --- a/drivers/tty/serial/8250/8250_dw.c +++ b/drivers/tty/serial/8250/8250_dw.c @@ -282,20 +282,13 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios, clk_disable_unprepare(d->clk); rate = clk_round_rate(d->clk, baud * 16); - if (rate < 0) - ret = rate; - else if (rate == 0) - ret = -ENOENT; - else + if (rate > 0) { ret = clk_set_rate(d->clk, rate); + if (!ret) + p->uartclk = rate; + } clk_prepare_enable(d->clk); - if (ret) - goto out; - - p->uartclk = rate; - -out: p->status &= ~UPSTAT_AUTOCTS; if (termios->c_cflag & CRTSCTS) p->status |= UPSTAT_AUTOCTS; -- 2.25.1 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v3 3/4] serial: 8250_dw: Simplify the ref clock rate setting procedure 2020-05-06 23:31 ` [PATCH v3 3/4] serial: 8250_dw: Simplify the ref clock rate setting procedure Serge Semin @ 2020-05-15 14:05 ` Andy Shevchenko 2020-05-15 14:50 ` Serge Semin 0 siblings, 1 reply; 29+ messages in thread From: Andy Shevchenko @ 2020-05-15 14:05 UTC (permalink / raw) To: Serge Semin Cc: Thomas Bogendoerfer, Greg Kroah-Hartman, Jiri Slaby, Serge Semin, Alexey Malahov, Paul Burton, Ralf Baechle, Arnd Bergmann, Long Cheng, Maxime Ripard, Catalin Marinas, Will Deacon, Russell King, linux-mips, linux-arm-kernel, linux-mediatek, Heikki Krogerus, Kefeng Wang, linux-serial, linux-kernel On Thu, May 07, 2020 at 02:31:34AM +0300, Serge Semin wrote: > Really instead of twice checking the clk_round_rate() return value > we could do it once, and if it isn't error the clock rate can be changed. > By doing so we decrease a number of ret-value tests and remove a weird > goto-based construction implemented in the dw8250_set_termios() method. > rate = clk_round_rate(d->clk, baud * 16); > - if (rate < 0) > - ret = rate; > - else if (rate == 0) > - ret = -ENOENT; This case now handled differently. I don't think it's good idea to change semantics. So, I don't see how this, after leaving the rate==0 case, would be better than original one. > - else > + if (rate > 0) { > ret = clk_set_rate(d->clk, rate); > + if (!ret) > + p->uartclk = rate; > + } > clk_prepare_enable(d->clk); > > - if (ret) > - goto out; > - > - p->uartclk = rate; > - > -out: > p->status &= ~UPSTAT_AUTOCTS; > if (termios->c_cflag & CRTSCTS) > p->status |= UPSTAT_AUTOCTS; -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 3/4] serial: 8250_dw: Simplify the ref clock rate setting procedure 2020-05-15 14:05 ` Andy Shevchenko @ 2020-05-15 14:50 ` Serge Semin 2020-05-15 15:05 ` Andy Shevchenko 0 siblings, 1 reply; 29+ messages in thread From: Serge Semin @ 2020-05-15 14:50 UTC (permalink / raw) To: Andy Shevchenko Cc: Serge Semin, Thomas Bogendoerfer, Greg Kroah-Hartman, Jiri Slaby, Alexey Malahov, Paul Burton, Ralf Baechle, Arnd Bergmann, Long Cheng, Maxime Ripard, Catalin Marinas, Will Deacon, Russell King, linux-mips, linux-arm-kernel, linux-mediatek, Heikki Krogerus, Kefeng Wang, linux-serial, linux-kernel On Fri, May 15, 2020 at 05:05:47PM +0300, Andy Shevchenko wrote: > On Thu, May 07, 2020 at 02:31:34AM +0300, Serge Semin wrote: > > Really instead of twice checking the clk_round_rate() return value > > we could do it once, and if it isn't error the clock rate can be changed. > > By doing so we decrease a number of ret-value tests and remove a weird > > goto-based construction implemented in the dw8250_set_termios() method. > > > rate = clk_round_rate(d->clk, baud * 16); > > - if (rate < 0) > > - ret = rate; > > > - else if (rate == 0) > > - ret = -ENOENT; > > This case now handled differently. > I don't think it's good idea to change semantics. > > So, I don't see how this, after leaving the rate==0 case, would be better than > original one. Semantic doesn't change. The code does exactly the same as before. If it didn't I either would have provided a comment about this or just didn't introduce the change in the first place. I guess you just don't see the whole picture of the method. Take a look in the code. The ret variable's been used to skip the "p->uartclk = rate" assignment. That's it. So the (rate == 0) will still be considered as error condition, which causes the clock rate left unchanged. Here is the code diff so you wouldn't need to dive deep into the driver sources: < clk_disable_unprepare(d->clk); < rate = clk_round_rate(d->clk, baud * 16); < if (rate < 0) < ret = rate; < else if (rate == 0) < ret = -ENOENT; < else < ret = clk_set_rate(d->clk, rate); < clk_prepare_enable(d->clk); < < if (ret) < goto out; < < p->uartclk = rate; < <out: --- > clk_disable_unprepare(d->clk); > rate = clk_round_rate(d->clk, baud * 16); > if (rate > 0) { > ret = clk_set_rate(d->clk, rate); > if (!ret) > p->uartclk = rate; > } > clk_prepare_enable(d->clk); -Sergey > > > - else > > + if (rate > 0) { > > ret = clk_set_rate(d->clk, rate); > > + if (!ret) > > + p->uartclk = rate; > > + } > > clk_prepare_enable(d->clk); > > > > - if (ret) > > - goto out; > > - > > - p->uartclk = rate; > > - > > -out: > > p->status &= ~UPSTAT_AUTOCTS; > > if (termios->c_cflag & CRTSCTS) > > p->status |= UPSTAT_AUTOCTS; > > -- > With Best Regards, > Andy Shevchenko > > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 3/4] serial: 8250_dw: Simplify the ref clock rate setting procedure 2020-05-15 14:50 ` Serge Semin @ 2020-05-15 15:05 ` Andy Shevchenko 0 siblings, 0 replies; 29+ messages in thread From: Andy Shevchenko @ 2020-05-15 15:05 UTC (permalink / raw) To: Serge Semin Cc: Serge Semin, Thomas Bogendoerfer, Greg Kroah-Hartman, Jiri Slaby, Alexey Malahov, Paul Burton, Ralf Baechle, Arnd Bergmann, Long Cheng, Maxime Ripard, Catalin Marinas, Will Deacon, Russell King, linux-mips, linux-arm-kernel, linux-mediatek, Heikki Krogerus, Kefeng Wang, linux-serial, linux-kernel On Fri, May 15, 2020 at 05:50:07PM +0300, Serge Semin wrote: > On Fri, May 15, 2020 at 05:05:47PM +0300, Andy Shevchenko wrote: > > On Thu, May 07, 2020 at 02:31:34AM +0300, Serge Semin wrote: > > > Really instead of twice checking the clk_round_rate() return value > > > we could do it once, and if it isn't error the clock rate can be changed. > > > By doing so we decrease a number of ret-value tests and remove a weird > > > goto-based construction implemented in the dw8250_set_termios() method. > > > > > rate = clk_round_rate(d->clk, baud * 16); > > > - if (rate < 0) > > > - ret = rate; > > > > > - else if (rate == 0) > > > - ret = -ENOENT; > > > > This case now handled differently. > > I don't think it's good idea to change semantics. > > > > So, I don't see how this, after leaving the rate==0 case, would be better than > > original one. > > Semantic doesn't change. The code does exactly the same as before. If it didn't > I either would have provided a comment about this or just didn't introduce the > change in the first place. I guess you just don't see the whole picture of the > method. Take a look in the code. The ret variable's been used to skip the > "p->uartclk = rate" assignment. That's it. So the (rate == 0) will still be > considered as error condition, which causes the clock rate left unchanged. > Here is the code diff so you wouldn't need to dive deep into the driver > sources: > > < clk_disable_unprepare(d->clk); > < rate = clk_round_rate(d->clk, baud * 16); > < if (rate < 0) > < ret = rate; > < else if (rate == 0) > < ret = -ENOENT; > < else > < ret = clk_set_rate(d->clk, rate); > < clk_prepare_enable(d->clk); > < > < if (ret) > < goto out; > < > < p->uartclk = rate; > < > <out: > --- > > clk_disable_unprepare(d->clk); > > rate = clk_round_rate(d->clk, baud * 16); > > if (rate > 0) { > > ret = clk_set_rate(d->clk, rate); > > if (!ret) > > p->uartclk = rate; > > } > > clk_prepare_enable(d->clk); Thanks. Indeed, in the above it looks clear. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 4/4] serial: 8250_dw: Fix common clocks usage race condition 2020-05-06 23:31 ` [PATCH v3 0/4] serial: 8250_dw: Fix ref clock usage Serge Semin ` (2 preceding siblings ...) 2020-05-06 23:31 ` [PATCH v3 3/4] serial: 8250_dw: Simplify the ref clock rate setting procedure Serge Semin @ 2020-05-06 23:31 ` Serge Semin 2020-05-15 14:10 ` Andy Shevchenko 2020-05-07 18:29 ` [PATCH v3 0/4] serial: 8250_dw: Fix ref clock usage Andy Shevchenko 4 siblings, 1 reply; 29+ messages in thread From: Serge Semin @ 2020-05-06 23:31 UTC (permalink / raw) To: Thomas Bogendoerfer, Greg Kroah-Hartman, Jiri Slaby, Andy Shevchenko Cc: Serge Semin, Serge Semin, Alexey Malahov, Paul Burton, Ralf Baechle, Arnd Bergmann, Long Cheng, Maxime Ripard, Catalin Marinas, Will Deacon, Russell King, linux-mips, linux-arm-kernel, linux-mediatek, Heikki Krogerus, Kefeng Wang, linux-serial, linux-kernel The race condition may happen if the UART reference clock is shared with some other device (on Baikal-T1 SoC it's another DW UART port). In this case if that device changes the clock rate while serial console is using it the DW 8250 UART port might not only end up with an invalid uartclk value saved, but may also experience a distorted output data since baud-clock could have been changed. In order to fix this lets at least try to adjust the 8250 port setting like UART clock rate in case if the reference clock rate change is discovered. The driver will call the new method to update 8250 UART port clock rate settings. It's done by means of the clock event notifier registered at the port startup and unregistered in the shutdown callback method. Note 1. In order to avoid deadlocks we had to execute the UART port update method in a dedicated deferred work. This is due to (in my opinion redundant) the clock update implemented in the dw8250_set_termios() method. Note 2. Before the ref clock is manually changed by the custom set_termios() function we swap the port uartclk value with new rate adjusted to be suitable for the requested baud. It is necessary in order to effectively disable a functionality of the ref clock events handler for the current UART port, since uartclk update will be done a bit further in the generic serial8250_do_set_termios() function. Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> Cc: Paul Burton <paulburton@kernel.org> Cc: Ralf Baechle <ralf@linux-mips.org> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Long Cheng <long.cheng@mediatek.com> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> Cc: Russell King <linux@armlinux.org.uk> Cc: linux-mips@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-mediatek@lists.infradead.org --- Changelog v2: - Move exclusive ref clock lock/unlock precudures to the 8250 port startup/shutdown methods. - The changelog message has also been slightly modified due to the alteration. - Remove Alexey' SoB tag. - Cc someone from ARM who might be concerned regarding this change. - Cc someone from Clocks Framework to get their comments on this patch. Changelog v3: - Refactor the original patch to adjust the UART port divisor instead of requesting an exclusive ref clock utilization. --- drivers/tty/serial/8250/8250_dw.c | 114 +++++++++++++++++++++++++++++- 1 file changed, 111 insertions(+), 3 deletions(-) diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c index 12866083731d..cf4de510ab1b 100644 --- a/drivers/tty/serial/8250/8250_dw.c +++ b/drivers/tty/serial/8250/8250_dw.c @@ -19,6 +19,8 @@ #include <linux/of_irq.h> #include <linux/of_platform.h> #include <linux/platform_device.h> +#include <linux/workqueue.h> +#include <linux/notifier.h> #include <linux/slab.h> #include <linux/acpi.h> #include <linux/clk.h> @@ -43,6 +45,8 @@ struct dw8250_data { int msr_mask_off; struct clk *clk; struct clk *pclk; + struct notifier_block clk_notifier; + struct work_struct clk_work; struct reset_control *rst; unsigned int skip_autocfg:1; @@ -54,6 +58,16 @@ static inline struct dw8250_data *to_dw8250_data(struct dw8250_port_data *data) return container_of(data, struct dw8250_data, data); } +static inline struct dw8250_data *clk_to_dw8250_data(struct notifier_block *nb) +{ + return container_of(nb, struct dw8250_data, clk_notifier); +} + +static inline struct dw8250_data *work_to_dw8250_data(struct work_struct *work) +{ + return container_of(work, struct dw8250_data, clk_work); +} + static inline int dw8250_modify_msr(struct uart_port *p, int offset, int value) { struct dw8250_data *d = to_dw8250_data(p->private_data); @@ -260,6 +274,45 @@ static int dw8250_handle_irq(struct uart_port *p) return 0; } +static void dw8250_clk_work_cb(struct work_struct *work) +{ + struct dw8250_data *d = work_to_dw8250_data(work); + struct uart_8250_port *up; + unsigned long rate; + + rate = clk_get_rate(d->clk); + if (rate) { + up = serial8250_get_port(d->data.line); + + serial8250_update_uartclk(&up->port, rate); + } +} + +static int dw8250_clk_notifier_cb(struct notifier_block *nb, + unsigned long event, void *data) +{ + struct dw8250_data *d = clk_to_dw8250_data(nb); + + /* + * We have no choice but to defer the uartclk update due to two + * deadlocks. First one is caused by a recursive mutex lock which + * happens when clk_set_rate() is called from dw8250_set_termios(). + * Second deadlock is more tricky and is caused by an inverted order of + * the clk and tty-port mutexes lock. It happens if clock rate change + * is requested asynchronously while set_termios() is executed between + * tty-port mutex lock and clk_set_rate() function invocation and + * vise-versa. Anyway if we didn't have the reference clock alteration + * in the dw8250_set_termios() method we wouldn't have needed this + * deferred event handling complication. + */ + if (event == POST_RATE_CHANGE) { + queue_work(system_unbound_wq, &d->clk_work); + return NOTIFY_OK; + } + + return NOTIFY_DONE; +} + static void dw8250_do_pm(struct uart_port *port, unsigned int state, unsigned int old) { @@ -283,9 +336,16 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios, clk_disable_unprepare(d->clk); rate = clk_round_rate(d->clk, baud * 16); if (rate > 0) { - ret = clk_set_rate(d->clk, rate); - if (!ret) - p->uartclk = rate; + /* + * Premilinary set the uartclk to the new clock rate so the + * clock update event handler caused by the clk_set_rate() + * calling wouldn't actually update the UART divisor since + * we about to do this anyway. + */ + swap(p->uartclk, rate); + ret = clk_set_rate(d->clk, p->uartclk); + if (ret) + swap(p->uartclk, rate); } clk_prepare_enable(d->clk); @@ -312,6 +372,49 @@ static void dw8250_set_ldisc(struct uart_port *p, struct ktermios *termios) serial8250_do_set_ldisc(p, termios); } +static int dw8250_startup(struct uart_port *p) +{ + struct dw8250_data *d = to_dw8250_data(p->private_data); + int ret; + + /* + * Some platforms may provide a reference clock shared between several + * devices. In this case before using the serial port first we have to + * make sure that any clock state change is known to the UART port at + * least post factum. + */ + if (d->clk) { + ret = clk_notifier_register(d->clk, &d->clk_notifier); + if (ret) + dev_warn(p->dev, "Failed to set the clock notifier\n"); + + /* + * Get current reference clock rate to make sure the UART port + * is equipped with an up-to-date value before it's started up. + */ + p->uartclk = clk_get_rate(d->clk); + if (!p->uartclk) { + dev_err(p->dev, "Clock rate not defined\n"); + return -EINVAL; + } + } + + return serial8250_do_startup(p); +} + +static void dw8250_shutdown(struct uart_port *p) +{ + struct dw8250_data *d = to_dw8250_data(p->private_data); + + serial8250_do_shutdown(p); + + if (d->clk) { + clk_notifier_unregister(d->clk, &d->clk_notifier); + + flush_work(&d->clk_work); + } +} + /* * dw8250_fallback_dma_filter will prevent the UART from getting just any free * channel on platforms that have DMA engines, but don't have any channels @@ -407,6 +510,8 @@ static int dw8250_probe(struct platform_device *pdev) p->serial_out = dw8250_serial_out; p->set_ldisc = dw8250_set_ldisc; p->set_termios = dw8250_set_termios; + p->startup = dw8250_startup; + p->shutdown = dw8250_shutdown; p->membase = devm_ioremap(dev, regs->start, resource_size(regs)); if (!p->membase) @@ -468,6 +573,9 @@ static int dw8250_probe(struct platform_device *pdev) if (IS_ERR(data->clk)) return PTR_ERR(data->clk); + INIT_WORK(&data->clk_work, dw8250_clk_work_cb); + data->clk_notifier.notifier_call = dw8250_clk_notifier_cb; + err = clk_prepare_enable(data->clk); if (err) dev_warn(dev, "could not enable optional baudclk: %d\n", err); -- 2.25.1 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v3 4/4] serial: 8250_dw: Fix common clocks usage race condition 2020-05-06 23:31 ` [PATCH v3 4/4] serial: 8250_dw: Fix common clocks usage race condition Serge Semin @ 2020-05-15 14:10 ` Andy Shevchenko 2020-05-15 15:19 ` Serge Semin 0 siblings, 1 reply; 29+ messages in thread From: Andy Shevchenko @ 2020-05-15 14:10 UTC (permalink / raw) To: Serge Semin Cc: Thomas Bogendoerfer, Greg Kroah-Hartman, Jiri Slaby, Serge Semin, Alexey Malahov, Paul Burton, Ralf Baechle, Arnd Bergmann, Long Cheng, Maxime Ripard, Catalin Marinas, Will Deacon, Russell King, linux-mips, linux-arm-kernel, linux-mediatek, Heikki Krogerus, Kefeng Wang, linux-serial, linux-kernel On Thu, May 07, 2020 at 02:31:35AM +0300, Serge Semin wrote: > The race condition may happen if the UART reference clock is shared with > some other device (on Baikal-T1 SoC it's another DW UART port). In this > case if that device changes the clock rate while serial console is using > it the DW 8250 UART port might not only end up with an invalid uartclk > value saved, but may also experience a distorted output data since > baud-clock could have been changed. In order to fix this lets at least > try to adjust the 8250 port setting like UART clock rate in case if the > reference clock rate change is discovered. The driver will call the new > method to update 8250 UART port clock rate settings. It's done by means of > the clock event notifier registered at the port startup and unregistered > in the shutdown callback method. I'm wondering if clock framework itself can provide such a notifier? > Note 1. In order to avoid deadlocks we had to execute the UART port update > method in a dedicated deferred work. This is due to (in my opinion > redundant) the clock update implemented in the dw8250_set_termios() > method. So, and how you propose to update the clock when ->set_termios() is called? > Note 2. Before the ref clock is manually changed by the custom > set_termios() function we swap the port uartclk value with new rate > adjusted to be suitable for the requested baud. It is necessary in > order to effectively disable a functionality of the ref clock events > handler for the current UART port, since uartclk update will be done > a bit further in the generic serial8250_do_set_termios() function. ... > + struct notifier_block clk_notifier; > + struct work_struct clk_work; Oh, this seems too much. Perhaps, the compatible based quirk with your initial approach is much better for time being. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 4/4] serial: 8250_dw: Fix common clocks usage race condition 2020-05-15 14:10 ` Andy Shevchenko @ 2020-05-15 15:19 ` Serge Semin 0 siblings, 0 replies; 29+ messages in thread From: Serge Semin @ 2020-05-15 15:19 UTC (permalink / raw) To: Andy Shevchenko Cc: Serge Semin, Thomas Bogendoerfer, Greg Kroah-Hartman, Jiri Slaby, Alexey Malahov, Paul Burton, Ralf Baechle, Arnd Bergmann, Long Cheng, Maxime Ripard, Catalin Marinas, Will Deacon, Russell King, linux-mips, linux-arm-kernel, linux-mediatek, Heikki Krogerus, Kefeng Wang, linux-serial, linux-kernel On Fri, May 15, 2020 at 05:10:46PM +0300, Andy Shevchenko wrote: > On Thu, May 07, 2020 at 02:31:35AM +0300, Serge Semin wrote: > > The race condition may happen if the UART reference clock is shared with > > some other device (on Baikal-T1 SoC it's another DW UART port). In this > > case if that device changes the clock rate while serial console is using > > it the DW 8250 UART port might not only end up with an invalid uartclk > > value saved, but may also experience a distorted output data since > > baud-clock could have been changed. In order to fix this lets at least > > try to adjust the 8250 port setting like UART clock rate in case if the > > reference clock rate change is discovered. The driver will call the new > > method to update 8250 UART port clock rate settings. It's done by means of > > the clock event notifier registered at the port startup and unregistered > > in the shutdown callback method. > > I'm wondering if clock framework itself can provide such a notifier? > > > Note 1. In order to avoid deadlocks we had to execute the UART port update > > method in a dedicated deferred work. This is due to (in my opinion > > redundant) the clock update implemented in the dw8250_set_termios() > > method. > > So, and how you propose to update the clock when ->set_termios() is called? First of all If you are worried about the current implementation, please don't, it still updates the clock in set_termios (please see the set_termios code). The method hasn't changed much and does the updating the same way it did before. Secondly, 8250 driver should be using the same reference clock as it is pre-defined by the platform with no change. The baud rate updates are supposed to be performed by the divider embedded into the 8250 controller, otherwise the divisor functionality is left completely unused. If a platform engineer needs to speed the uart up, the ref clock rate can be tuned by for instance the "assigned-clock-rates" property. > > > Note 2. Before the ref clock is manually changed by the custom > > set_termios() function we swap the port uartclk value with new rate > > adjusted to be suitable for the requested baud. It is necessary in > > order to effectively disable a functionality of the ref clock events > > handler for the current UART port, since uartclk update will be done > > a bit further in the generic serial8250_do_set_termios() function. > > ... > > > + struct notifier_block clk_notifier; > > + struct work_struct clk_work; > > Oh, this seems too much. > Perhaps, the compatible based quirk with your initial approach is much better for time being. It's already in 8250_dw, useful not for a single platform and won't hurt any other one. So I'll leave it here and wait for the Greg feedback. -Sergey > > -- > With Best Regards, > Andy Shevchenko > > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 0/4] serial: 8250_dw: Fix ref clock usage 2020-05-06 23:31 ` [PATCH v3 0/4] serial: 8250_dw: Fix ref clock usage Serge Semin ` (3 preceding siblings ...) 2020-05-06 23:31 ` [PATCH v3 4/4] serial: 8250_dw: Fix common clocks usage race condition Serge Semin @ 2020-05-07 18:29 ` Andy Shevchenko 4 siblings, 0 replies; 29+ messages in thread From: Andy Shevchenko @ 2020-05-07 18:29 UTC (permalink / raw) To: Serge Semin Cc: Thomas Bogendoerfer, Greg Kroah-Hartman, Jiri Slaby, Serge Semin, Alexey Malahov, Maxim Kaurkin, Pavel Parkhomenko, Alexey Kolotnikov, Ramil Zaripov, Ekaterina Skachko, Vadim Vlasov, Paul Burton, Ralf Baechle, Arnd Bergmann, Long Cheng, Maxime Ripard, Catalin Marinas, Will Deacon, Russell King, linux-mips, linux-arm-kernel, linux-mediatek, linux-serial, linux-kernel On Thu, May 07, 2020 at 02:31:31AM +0300, Serge Semin wrote: > It might be dangerous if an UART port reference clock rate is suddenly > changed. In particular the 8250 port drivers (and AFAICS most of the tty > drivers using common clock framework clocks) rely either on the > exclusive reference clock utilization or on the ref clock rate being > always constant. Needless to say that it turns out not true and if some > other service suddenly changes the clock rate behind an UART port driver > back it's no good. So the port might not only end up with an invalid > uartclk value saved, but may also experience a distorted output/input > data since such action will effectively update the programmed baud-clock. > We discovered such problem on Baikal-T1 SoC where two DW 8250 ports have > got a shared reference clock. Allwinner SoC is equipped with an UART, > which clock is derived from the CPU PLL clock source, so the CPU frequency > change might be propagated down up to the serial port reference clock. > This patchset provides a way to fix the problem to the 8250 serial port > controllers and mostly fixes it for the DW 8250-compatible UART. I say > mostly because due to not having a facility to pause/stop and resume/ > restart on-going transfers we implemented the UART clock rate update > procedure executed post factum of the actual reference clock rate change. > > In addition the patchset includes a few fixes we discovered when were > working the issue. First one concerns the maximum baud rate setting used > to determine a serial port baud based on the current UART port clock rate. > Another one simplifies the ref clock rate setting procedure a bit. > > This patchset is rebased and tested on the mainline Linux kernel 5.7-rc4: > 0e698dfa2822 ("Linux 5.7-rc4") > tag: v5.7-rc4 Thanks! I will look at them later, but first impression that the first approach narrowed to the certain SoC (by matching compatible string) looks better solution for time being. > Changelog v3: > - Refactor the original patch to adjust the UART port divisor instead of > requesting an exclusive ref clock utilization. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> > Cc: Maxim Kaurkin <Maxim.Kaurkin@baikalelectronics.ru> > Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru> > Cc: Alexey Kolotnikov <Alexey.Kolotnikov@baikalelectronics.ru> > Cc: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru> > Cc: Ekaterina Skachko <Ekaterina.Skachko@baikalelectronics.ru> > Cc: Vadim Vlasov <V.Vlasov@baikalelectronics.ru> > Cc: Alexey Kolotnikov <Alexey.Kolotnikov@baikalelectronics.ru> > Cc: Paul Burton <paulburton@kernel.org> > Cc: Ralf Baechle <ralf@linux-mips.org> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Long Cheng <long.cheng@mediatek.com> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Russell King <linux@armlinux.org.uk> > Cc: linux-mips@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-mediatek@lists.infradead.org > Cc: linux-serial@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > > Serge Semin (4): > serial: 8250: Fix max baud limit in generic 8250 port > serial: 8250: Add 8250 port clock update method > serial: 8250_dw: Simplify the ref clock rate setting procedure > serial: 8250_dw: Fix common clocks usage race condition > > drivers/tty/serial/8250/8250_dw.c | 125 +++++++++++++++++++++++++--- > drivers/tty/serial/8250/8250_port.c | 42 +++++++++- > include/linux/serial_8250.h | 2 + > 3 files changed, 156 insertions(+), 13 deletions(-) > > -- > 2.25.1 > -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2020-05-15 15:20 UTC | newest] Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20200306130231.05BBC8030795@mail.baikalelectronics.ru> [not found] ` <20200306134049.86F8180307C2@mail.baikalelectronics.ru> [not found] ` <20200310001444.B0F50803087C@mail.baikalelectronics.ru> 2020-03-10 14:17 ` [PATCH] serial: 8250_dw: Fix common clocks usage race condition Andy Shevchenko [not found] ` <20200310141715.7063280307CA@mail.baikalelectronics.ru> 2020-03-12 18:47 ` Sergey Semin 2020-03-18 15:19 ` Andy Shevchenko 2020-03-23 2:46 ` [PATCH v2] " Sergey.Semin 2020-03-23 9:20 ` Andy Shevchenko 2020-03-23 11:11 ` Sergey Semin 2020-03-23 11:52 ` Andy Shevchenko 2020-03-23 17:07 ` Sergey Semin 2020-03-23 10:01 ` Maxime Ripard 2020-03-23 13:50 ` Sergey Semin 2020-03-24 9:41 ` Maxime Ripard 2020-03-24 10:12 ` Andy Shevchenko 2020-03-25 17:11 ` Sergey Semin 2020-03-26 1:44 ` Stephen Boyd 2020-03-27 9:12 ` Sergey Semin 2020-05-06 23:31 ` [PATCH v3 0/4] serial: 8250_dw: Fix ref clock usage Serge Semin 2020-05-06 23:31 ` [PATCH v3 1/4] serial: 8250: Fix max baud limit in generic 8250 port Serge Semin 2020-05-15 13:48 ` Andy Shevchenko 2020-05-06 23:31 ` [PATCH v3 2/4] serial: 8250: Add 8250 port clock update method Serge Semin 2020-05-15 12:45 ` Greg Kroah-Hartman 2020-05-15 14:32 ` Serge Semin 2020-05-06 23:31 ` [PATCH v3 3/4] serial: 8250_dw: Simplify the ref clock rate setting procedure Serge Semin 2020-05-15 14:05 ` Andy Shevchenko 2020-05-15 14:50 ` Serge Semin 2020-05-15 15:05 ` Andy Shevchenko 2020-05-06 23:31 ` [PATCH v3 4/4] serial: 8250_dw: Fix common clocks usage race condition Serge Semin 2020-05-15 14:10 ` Andy Shevchenko 2020-05-15 15:19 ` Serge Semin 2020-05-07 18:29 ` [PATCH v3 0/4] serial: 8250_dw: Fix ref clock usage Andy Shevchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).