From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E7EEEC10DCE for ; Thu, 12 Mar 2020 18:53:40 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id B85DA20663 for ; Thu, 12 Mar 2020 18:53:40 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726569AbgCLSxk (ORCPT ); Thu, 12 Mar 2020 14:53:40 -0400 Received: from mail.baikalelectronics.com ([87.245.175.226]:57758 "EHLO mail.baikalelectronics.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726362AbgCLSxk (ORCPT ); Thu, 12 Mar 2020 14:53:40 -0400 X-Greylist: delayed 342 seconds by postgrey-1.27 at vger.kernel.org; Thu, 12 Mar 2020 14:53:37 EDT Received: from localhost (unknown [127.0.0.1]) by mail.baikalelectronics.ru (Postfix) with ESMTP id 4EFA2803078C; Thu, 12 Mar 2020 18:47:54 +0000 (UTC) X-Virus-Scanned: amavisd-new at baikalelectronics.ru Received: from mail.baikalelectronics.ru ([127.0.0.1]) by localhost (mail.baikalelectronics.ru [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id TRWXaDjO4H-y; Thu, 12 Mar 2020 21:47:51 +0300 (MSK) Date: Thu, 12 Mar 2020 21:47:02 +0300 From: Sergey Semin 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 Subject: Re: [PATCH] serial: 8250_dw: Fix common clocks usage race condition Message-ID: <20200312184450.co5o6zm6qniptfuv@ubsrv2.baikal.int> References: <20200306130231.05BBC8030795@mail.baikalelectronics.ru> <20200306134049.86F8180307C2@mail.baikalelectronics.ru> <20200310001444.B0F50803087C@mail.baikalelectronics.ru> <20200310141715.7063280307CA@mail.baikalelectronics.ru> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200310141715.7063280307CA@mail.baikalelectronics.ru> X-ClientProxiedBy: MAIL.baikal.int (192.168.51.25) To mail (192.168.51.25) Sender: linux-serial-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-serial@vger.kernel.org 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 > > > > > > > > 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 > > > > Signed-off-by: Alexey Malahov > > > > > > 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 > >