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@xxxxxxxxxxxxxxxxxxxx wrote:
> > > > From: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxxxxxxxxxxx>
> > > > Signed-off-by: Alexey Malahov <Alexey.Malahov@xxxxxxxxxxxxxxxxxxxx>
> > >
> > > 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
>
>
