All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jslaby@suse.com>,
	Serge Semin <fancer.lancer@gmail.com>,
	Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>,
	Paul Burton <paulburton@kernel.org>,
	Ralf Baechle <ralf@linux-mips.org>, Arnd Bergmann <arnd@arndb.de>,
	Long Cheng <long.cheng@mediatek.com>,
	Maxime Ripard <mripard@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Russell King <linux@armlinux.org.uk>,
	linux-mips@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org,
	Heikki Krogerus <heikki.krogerus@linux.intel.com>,
	Kefeng Wang <wangkefeng.wang@huawei.com>,
	linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 4/4] serial: 8250_dw: Fix common clocks usage race condition
Date: Fri, 15 May 2020 17:10:46 +0300	[thread overview]
Message-ID: <20200515141046.GF1634618@smile.fi.intel.com> (raw)
In-Reply-To: <20200506233136.11842-5-Sergey.Semin@baikalelectronics.ru>

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



WARNING: multiple messages have this Message-ID (diff)
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: Maxime Ripard <mripard@kernel.org>,
	Kefeng Wang <wangkefeng.wang@huawei.com>,
	Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Arnd Bergmann <arnd@arndb.de>,
	Paul Burton <paulburton@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-kernel@vger.kernel.org,
	Russell King <linux@armlinux.org.uk>,
	Serge Semin <fancer.lancer@gmail.com>,
	Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>,
	Long Cheng <long.cheng@mediatek.com>,
	linux-mediatek@lists.infradead.org,
	Ralf Baechle <ralf@linux-mips.org>,
	linux-serial@vger.kernel.org, Jiri Slaby <jslaby@suse.com>,
	Heikki Krogerus <heikki.krogerus@linux.intel.com>,
	linux-mips@vger.kernel.org, Will Deacon <will@kernel.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 4/4] serial: 8250_dw: Fix common clocks usage race condition
Date: Fri, 15 May 2020 17:10:46 +0300	[thread overview]
Message-ID: <20200515141046.GF1634618@smile.fi.intel.com> (raw)
In-Reply-To: <20200506233136.11842-5-Sergey.Semin@baikalelectronics.ru>

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



_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

WARNING: multiple messages have this Message-ID (diff)
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: Maxime Ripard <mripard@kernel.org>,
	Kefeng Wang <wangkefeng.wang@huawei.com>,
	Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Arnd Bergmann <arnd@arndb.de>,
	Paul Burton <paulburton@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-kernel@vger.kernel.org,
	Russell King <linux@armlinux.org.uk>,
	Serge Semin <fancer.lancer@gmail.com>,
	Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>,
	Long Cheng <long.cheng@mediatek.com>,
	linux-mediatek@lists.infradead.org,
	Ralf Baechle <ralf@linux-mips.org>,
	linux-serial@vger.kernel.org, Jiri Slaby <jslaby@suse.com>,
	Heikki Krogerus <heikki.krogerus@linux.intel.com>,
	linux-mips@vger.kernel.org, Will Deacon <will@kernel.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 4/4] serial: 8250_dw: Fix common clocks usage race condition
Date: Fri, 15 May 2020 17:10:46 +0300	[thread overview]
Message-ID: <20200515141046.GF1634618@smile.fi.intel.com> (raw)
In-Reply-To: <20200506233136.11842-5-Sergey.Semin@baikalelectronics.ru>

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



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2020-05-15 14:10 UTC|newest]

Thread overview: 69+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [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  2:46   ` Sergey.Semin
2020-03-23  9:20   ` Andy Shevchenko
2020-03-23  9:20     ` Andy Shevchenko
2020-03-23 11:11     ` Sergey Semin
2020-03-23 11:11       ` Sergey Semin
2020-03-23 11:52       ` Andy Shevchenko
2020-03-23 11:52         ` Andy Shevchenko
2020-03-23 17:07         ` Sergey Semin
2020-03-23 17:07           ` Sergey Semin
2020-03-23 10:01   ` Maxime Ripard
2020-03-23 10:01     ` Maxime Ripard
2020-03-23 13:50     ` Sergey Semin
2020-03-23 13:50       ` Sergey Semin
2020-03-24  9:41       ` Maxime Ripard
2020-03-24  9:41         ` Maxime Ripard
2020-03-24 10:12       ` Andy Shevchenko
2020-03-24 10:12         ` Andy Shevchenko
2020-03-25 17:11         ` Sergey Semin
2020-03-25 17:11           ` Sergey Semin
2020-03-26  1:44           ` Stephen Boyd
2020-03-26  1:44             ` Stephen Boyd
2020-03-27  9:12             ` Sergey Semin
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     ` Serge Semin
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
2020-05-06 23:31       ` Serge Semin
2020-05-06 23:31       ` Serge Semin
2020-05-15 13:48       ` Andy Shevchenko
2020-05-15 13:48         ` Andy Shevchenko
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-06 23:31       ` Serge Semin
2020-05-06 23:31       ` Serge Semin
2020-05-15 12:45       ` Greg Kroah-Hartman
2020-05-15 12:45         ` Greg Kroah-Hartman
2020-05-15 12:45         ` Greg Kroah-Hartman
2020-05-15 14:32         ` Serge Semin
2020-05-15 14:32           ` Serge Semin
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-06 23:31       ` Serge Semin
2020-05-06 23:31       ` Serge Semin
2020-05-15 14:05       ` Andy Shevchenko
2020-05-15 14:05         ` Andy Shevchenko
2020-05-15 14:05         ` Andy Shevchenko
2020-05-15 14:50         ` Serge Semin
2020-05-15 14:50           ` Serge Semin
2020-05-15 14:50           ` Serge Semin
2020-05-15 15:05           ` Andy Shevchenko
2020-05-15 15:05             ` Andy Shevchenko
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-06 23:31       ` Serge Semin
2020-05-06 23:31       ` Serge Semin
2020-05-15 14:10       ` Andy Shevchenko [this message]
2020-05-15 14:10         ` Andy Shevchenko
2020-05-15 14:10         ` Andy Shevchenko
2020-05-15 15:19         ` Serge Semin
2020-05-15 15:19           ` Serge Semin
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
2020-05-07 18:29       ` Andy Shevchenko
2020-05-07 18:29       ` Andy Shevchenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200515141046.GF1634618@smile.fi.intel.com \
    --to=andriy.shevchenko@linux.intel.com \
    --cc=Alexey.Malahov@baikalelectronics.ru \
    --cc=Sergey.Semin@baikalelectronics.ru \
    --cc=arnd@arndb.de \
    --cc=catalin.marinas@arm.com \
    --cc=fancer.lancer@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=heikki.krogerus@linux.intel.com \
    --cc=jslaby@suse.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=linux-mips@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=long.cheng@mediatek.com \
    --cc=mripard@kernel.org \
    --cc=paulburton@kernel.org \
    --cc=ralf@linux-mips.org \
    --cc=tsbogend@alpha.franken.de \
    --cc=wangkefeng.wang@huawei.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.