* Re: [RFC/PATCH 0/5] DVFS in the OPP core
From: Rafael J. Wysocki @ 2019-01-31 10:36 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael J. Wysocki, Stephen Boyd, Michael Turquette, grahamr,
Linux Kernel Mailing List, linux-arm-msm, Linux PM, linux-serial,
linux-spi, Rajendra Nayak, Ulf Hansson, Doug Anderson,
Vincent Guittot
In-Reply-To: <20190131100628.6ex6ic3kspuxxxt7@vireshk-i7>
On Thu, Jan 31, 2019 at 11:06 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 31-01-19, 10:58, Rafael J. Wysocki wrote:
> > On Thu, Jan 31, 2019 at 10:23 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > Adding few folks to the thread who might be interested in this stuff.
> > >
> > > On 28-01-19, 17:55, Stephen Boyd wrote:
> > > > This patch series is an RFC around how we can implement DVFS for devices
> > > > that aren't your typical OPPish device (i.e. GPU/CPU). They don't have a
> > > > strict set of frequencies that they have been tested at to derive some
> > > > operating performance point. Instead they have a coarser set of
> > > > frequency max or 'fmax' OPPs that describe the maiximum frequency the
> > > > device can operate at with a given voltage.
> > > >
> > > > The approach we take is to let the devm_pm_opp_set_rate() API accept 0
> > > > as a valid frequency indicating the frequency isn't required anymore and
> > > > to make the same API use the clk framework to round the frequency passed
> > > > in instead of relying on the OPP table to specify each frequency that
> > > > can be used. Once we have these two patches in place, we can use the OPP
> > > > API to change clk rates instead of clk_set_rate() and use all the recent
> > > > OPP enhancements that have been made around required-opps and genpd
> > > > performance states to do DVFS for all devices.
> > >
> > > Generally speaking I am fine with such an approach but I am not sure
> > > about what others would say on this as they had objections to using
> > > OPP core for setting the rate itself.
> > >
> > > FWIW, I suggested exactly this solution sometime back [1]
> > >
> > > - Drivers need to use two API sets to change clock rate (OPP helpers)
> > > and enable/disable them (CLK framework helpers) and this leads us to
> > > exactly that combination. Is that acceptable ? It doesn't look great
> > > to me as well..
> >
> > I agree here.
> >
> > > - Do we expect the callers will disable clk before calling
> > > opp-set-rate with 0 ? We should remove the regulator requirements as
> > > well along with perf-state.
> >
> > Well, disabling clk affects HW in general, doesn't it?
>
> Yeah, but the regulator may be shared and is running at higher
> voltages just because of the clock requirement of the device getting
> disabled here. Or did I misunderstood what you wanted to say ?
What I wanted to say is that if the caller is required to disable clk
beforehand, that may be as good as setting its rate to zero already.
> > > - What about enabling/disabling clock as well from OPP framework. We
> > > can enable it on the very first call to opp-set-rate and disable
> > > when freq is 0. That will simplify the drivers as well.
> >
> > That sounds compelling, but I guess there are cases in which you can
> > gate the clock regardless of the frequency setting. How would that
> > work then?
>
> Can you give any example here ? I am not sure I understood the concern
> here.
It looks like I was confused somehow, never mind. :-)
^ permalink raw reply
* Re: [RFC/PATCH 0/5] DVFS in the OPP core
From: Viresh Kumar @ 2019-01-31 10:06 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Stephen Boyd, Michael Turquette, grahamr,
Linux Kernel Mailing List, linux-arm-msm, Linux PM, linux-serial,
linux-spi, Rajendra Nayak, Ulf Hansson, Doug Anderson,
Vincent Guittot
In-Reply-To: <CAJZ5v0jY5Zjg0R7uC6ewJT5Pi-15Mohotj=im9k8w4THJSM0Dw@mail.gmail.com>
On 31-01-19, 10:58, Rafael J. Wysocki wrote:
> On Thu, Jan 31, 2019 at 10:23 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > Adding few folks to the thread who might be interested in this stuff.
> >
> > On 28-01-19, 17:55, Stephen Boyd wrote:
> > > This patch series is an RFC around how we can implement DVFS for devices
> > > that aren't your typical OPPish device (i.e. GPU/CPU). They don't have a
> > > strict set of frequencies that they have been tested at to derive some
> > > operating performance point. Instead they have a coarser set of
> > > frequency max or 'fmax' OPPs that describe the maiximum frequency the
> > > device can operate at with a given voltage.
> > >
> > > The approach we take is to let the devm_pm_opp_set_rate() API accept 0
> > > as a valid frequency indicating the frequency isn't required anymore and
> > > to make the same API use the clk framework to round the frequency passed
> > > in instead of relying on the OPP table to specify each frequency that
> > > can be used. Once we have these two patches in place, we can use the OPP
> > > API to change clk rates instead of clk_set_rate() and use all the recent
> > > OPP enhancements that have been made around required-opps and genpd
> > > performance states to do DVFS for all devices.
> >
> > Generally speaking I am fine with such an approach but I am not sure
> > about what others would say on this as they had objections to using
> > OPP core for setting the rate itself.
> >
> > FWIW, I suggested exactly this solution sometime back [1]
> >
> > - Drivers need to use two API sets to change clock rate (OPP helpers)
> > and enable/disable them (CLK framework helpers) and this leads us to
> > exactly that combination. Is that acceptable ? It doesn't look great
> > to me as well..
>
> I agree here.
>
> > - Do we expect the callers will disable clk before calling
> > opp-set-rate with 0 ? We should remove the regulator requirements as
> > well along with perf-state.
>
> Well, disabling clk affects HW in general, doesn't it?
Yeah, but the regulator may be shared and is running at higher
voltages just because of the clock requirement of the device getting
disabled here. Or did I misunderstood what you wanted to say ?
> > - What about enabling/disabling clock as well from OPP framework. We
> > can enable it on the very first call to opp-set-rate and disable
> > when freq is 0. That will simplify the drivers as well.
>
> That sounds compelling, but I guess there are cases in which you can
> gate the clock regardless of the frequency setting. How would that
> work then?
Can you give any example here ? I am not sure I understood the concern
here.
--
viresh
^ permalink raw reply
* Re: [RFC/PATCH 0/5] DVFS in the OPP core
From: Rafael J. Wysocki @ 2019-01-31 9:58 UTC (permalink / raw)
To: Viresh Kumar
Cc: Stephen Boyd, Michael Turquette, grahamr,
Linux Kernel Mailing List, linux-arm-msm, Linux PM, linux-serial,
linux-spi, Rajendra Nayak, Ulf Hansson, Doug Anderson,
Vincent Guittot
In-Reply-To: <20190131092349.fksfqemm23qddkhw@vireshk-i7>
On Thu, Jan 31, 2019 at 10:23 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> Adding few folks to the thread who might be interested in this stuff.
>
> On 28-01-19, 17:55, Stephen Boyd wrote:
> > This patch series is an RFC around how we can implement DVFS for devices
> > that aren't your typical OPPish device (i.e. GPU/CPU). They don't have a
> > strict set of frequencies that they have been tested at to derive some
> > operating performance point. Instead they have a coarser set of
> > frequency max or 'fmax' OPPs that describe the maiximum frequency the
> > device can operate at with a given voltage.
> >
> > The approach we take is to let the devm_pm_opp_set_rate() API accept 0
> > as a valid frequency indicating the frequency isn't required anymore and
> > to make the same API use the clk framework to round the frequency passed
> > in instead of relying on the OPP table to specify each frequency that
> > can be used. Once we have these two patches in place, we can use the OPP
> > API to change clk rates instead of clk_set_rate() and use all the recent
> > OPP enhancements that have been made around required-opps and genpd
> > performance states to do DVFS for all devices.
>
> Generally speaking I am fine with such an approach but I am not sure
> about what others would say on this as they had objections to using
> OPP core for setting the rate itself.
>
> FWIW, I suggested exactly this solution sometime back [1]
>
> - Drivers need to use two API sets to change clock rate (OPP helpers)
> and enable/disable them (CLK framework helpers) and this leads us to
> exactly that combination. Is that acceptable ? It doesn't look great
> to me as well..
I agree here.
> - Do we expect the callers will disable clk before calling
> opp-set-rate with 0 ? We should remove the regulator requirements as
> well along with perf-state.
Well, disabling clk affects HW in general, doesn't it?
> - What about enabling/disabling clock as well from OPP framework. We
> can enable it on the very first call to opp-set-rate and disable
> when freq is 0. That will simplify the drivers as well.
That sounds compelling, but I guess there are cases in which you can
gate the clock regardless of the frequency setting. How would that
work then?
^ permalink raw reply
* [PATCH][V5] tty: fix race between flush_to_ldisc and tty_open
From: Li RongQing @ 2019-01-31 9:43 UTC (permalink / raw)
To: linux-serial, linux-kernel, jslaby, Greg Kroah-Hartman, gkohli
There still is a race window after the commit b027e2298bd588
("tty: fix data race between tty_init_dev and flush of buf"),
and we encountered this crash issue if receive_buf call comes
before tty initialization completes in tty_open and
tty->driver_data may be NULL.
CPU0 CPU1
---- ----
tty_open
tty_init_dev
tty_ldisc_unlock
schedule
flush_to_ldisc
receive_buf
tty_port_default_receive_buf
tty_ldisc_receive_buf
n_tty_receive_buf_common
__receive_buf
uart_flush_chars
uart_start
/*tty->driver_data is NULL*/
tty->ops->open
/*init tty->driver_data*/
it can be fixed by extending ldisc semaphore lock in tty_init_dev
to driver_data initialized completely after tty->ops->open(), but
this will lead to get lock on one function and unlock in some other
function, and hard to maintain, so fix this race only by checking
tty->driver_data when receiving, and return if tty->driver_data
is NULL, and n_tty_receive_buf_common maybe calls uart_unthrottle,
so add the same check
Signed-off-by: Wang Li <wangli39@baidu.com>
Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
V5: move check into uart_start from n_tty_receive_buf_common
V4: add version information
V3: not used ldisc semaphore lock, only checking tty->driver_data with NULL
V2: fix building error by EXPORT_SYMBOL tty_ldisc_unlock
V1: extend ldisc lock to protect that tty->driver_data is inited
drivers/tty/serial/serial_core.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 5c01bb6d1c24..556f50aa1b58 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -130,6 +130,9 @@ static void uart_start(struct tty_struct *tty)
struct uart_port *port;
unsigned long flags;
+ if (!state)
+ return;
+
port = uart_port_lock(state, flags);
__uart_start(tty);
uart_port_unlock(port, flags);
@@ -727,6 +730,9 @@ static void uart_unthrottle(struct tty_struct *tty)
upstat_t mask = UPSTAT_SYNC_FIFO;
struct uart_port *port;
+ if (!state)
+ return;
+
port = uart_port_ref(state);
if (!port)
return;
--
2.16.2
^ permalink raw reply related
* Re: [RFC/PATCH 0/5] DVFS in the OPP core
From: Viresh Kumar @ 2019-01-31 9:23 UTC (permalink / raw)
To: Stephen Boyd, mturquette, grahamr
Cc: linux-kernel, linux-arm-msm, linux-pm, linux-serial, linux-spi,
Rajendra Nayak, Ulf Hansson, Doug Anderson, vincent.guittot
In-Reply-To: <20190129015547.213276-1-swboyd@chromium.org>
Adding few folks to the thread who might be interested in this stuff.
On 28-01-19, 17:55, Stephen Boyd wrote:
> This patch series is an RFC around how we can implement DVFS for devices
> that aren't your typical OPPish device (i.e. GPU/CPU). They don't have a
> strict set of frequencies that they have been tested at to derive some
> operating performance point. Instead they have a coarser set of
> frequency max or 'fmax' OPPs that describe the maiximum frequency the
> device can operate at with a given voltage.
>
> The approach we take is to let the devm_pm_opp_set_rate() API accept 0
> as a valid frequency indicating the frequency isn't required anymore and
> to make the same API use the clk framework to round the frequency passed
> in instead of relying on the OPP table to specify each frequency that
> can be used. Once we have these two patches in place, we can use the OPP
> API to change clk rates instead of clk_set_rate() and use all the recent
> OPP enhancements that have been made around required-opps and genpd
> performance states to do DVFS for all devices.
Generally speaking I am fine with such an approach but I am not sure
about what others would say on this as they had objections to using
OPP core for setting the rate itself.
FWIW, I suggested exactly this solution sometime back [1]
- Drivers need to use two API sets to change clock rate (OPP helpers)
and enable/disable them (CLK framework helpers) and this leads us to
exactly that combination. Is that acceptable ? It doesn't look great
to me as well..
- Do we expect the callers will disable clk before calling
opp-set-rate with 0 ? We should remove the regulator requirements as
well along with perf-state.
- What about enabling/disabling clock as well from OPP framework. We
can enable it on the very first call to opp-set-rate and disable
when freq is 0. That will simplify the drivers as well.
> One nice feature of this approach is that we don't need to change the
> OPP binding to support this. We can specify only the max frequencies and
> the voltage requirements in DT with the existing binding and slightly
> tweak the OPP code to achieve these results.
>
> This series includes a conversion of the uart and spi drivers on
> qcom sdm845 where these patches are being developed. It shows how a
> driver is converted from the clk APIs to the OPP APIs and how
> enable/disable state of the clk is communicated to the OPP layer.
>
> Some open topics and initial points for discussion are:
>
> 1) The dev_pm_opp_set_rate() API changes may break something that's
> relying on the rate rounding that OPP provides. If those exist,
> we may need to implement another API that is more explicit about using
> the clk API instead of the OPP tables.
>
> 2) dev_pm_opp_set_rate(0) is an interesting solution to the problem of
> dropping the rate requirement. Is there anything better than this?
>
> 3) How do we handle devices that already have power-domains specified in
> DT? The opp binding for required-opps doesn't let us specify the power
> domain to target, instead it assumes that whatever power domain is
> attached to a device is the one that OPP needs to use to change the
> genpd performance state. Do we need a
> dev_pm_opp_set_required_opps_name() or something to be explicit about
> this? Can we have some way for the power domain that required-opps
> correspond to be expressed in the OPP tables themselves?
>
> 4) How do we achieve the "full constraint" state? i.e. what do we do
> about some devices being active and others being inactive during boot
> and making sure that the voltage for the shared power domains doesn't
> drop until all devices requiring it have informed OPP about their
> power requirements?
>
> Rajendra Nayak (4):
> OPP: Make dev_pm_opp_set_rate() with freq=0 as valid
> tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state
> spi: spi-geni-qcom: Use OPP API to set clk/perf state
> arm64: dts: sdm845: Add OPP table for all qup devices
>
> Stephen Boyd (1):
> OPP: Don't overwrite rounded clk rate
>
> arch/arm64/boot/dts/qcom/sdm845.dtsi | 115 ++++++++++++++++++++++++++
> drivers/opp/core.c | 26 ++++--
> drivers/spi/spi-geni-qcom.c | 12 ++-
> drivers/tty/serial/qcom_geni_serial.c | 15 +++-
> 4 files changed, 155 insertions(+), 13 deletions(-)
>
> For the interested, these patches are located here:
>
> https://github.com/rrnayak/linux/ v5.0-rc3/opp-corners-wip
>
> I've generated these patches by cutting off the top of that tree and
> massaging the commit text a bit for the first patch.
>
> base-commit: 49a57857aeea06ca831043acbb0fa5e0f50602fd
> prerequisite-patch-id: 9c3ee728603596b8b0ba06ffd66084bdc21b1271
> prerequisite-patch-id: f160e050bcd74f5de6fad66329381853869a6a97
> prerequisite-patch-id: aa23548d2b486c29489b4304d85799d08762254e
> prerequisite-patch-id: 40dd117c45fecb4308298352346546612db94b64
> prerequisite-patch-id: cd102fa42bab19897c2295e8b990b2156626054a
> prerequisite-patch-id: 3b9e5c8ed65ee96cc0f1c50166cf6bbb597ef582
> prerequisite-patch-id: 7e71b957b90ad51d0619944d5ebc859380e8e3e4
> prerequisite-patch-id: 5abd2bd6b3ae3e91551e2b8f9295169019ba82c7
> prerequisite-patch-id: 68bb3e44cf4e5dbd363a1a1750e4d378971ed393
> prerequisite-patch-id: 882b14ef9527b15d22cfddbb5fa2b9d43df1ff04
> prerequisite-patch-id: 6fc14ddeb074fb0826f1f40031e9d161f1361666
> --
> Sent by a computer through tubes
--
viresh
[1] https://lore.kernel.org/linux-clk/20180704065522.p4qpfnpayeobaok3@vireshk-i7/
^ permalink raw reply
* Re: [PATCH v6 5/6] dt-bindings: pinctrl: mt8183: add binding document
From: Zhiyong Tao @ 2019-01-31 8:30 UTC (permalink / raw)
To: Rob Herring
Cc: Erin Lo, Matthias Brugger, Mark Rutland, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Greg Kroah-Hartman, Stephen Boyd,
devicetree, srv_heupstream, linux-kernel, linux-serial,
linux-mediatek, linux-arm-kernel, yingjoe.chen, mars.cheng,
eddie.huang, linux-clk
In-Reply-To: <20190130161714.GA24352@bogus>
On Wed, 2019-01-30 at 10:17 -0600, Rob Herring wrote:
> On Thu, Jan 24, 2019 at 04:07:19PM +0800, Erin Lo wrote:
> > From: Zhiyong Tao <zhiyong.tao@mediatek.com>
> >
> > The commit adds mt8183 compatible node in binding document.
> >
> > Signed-off-by: Zhiyong Tao <zhiyong.tao@mediatek.com>
> > Signed-off-by: Erin Lo <erin.lo@mediatek.com>
> > ---
> > .../devicetree/bindings/pinctrl/pinctrl-mt8183.txt | 115 +++++++++++++++++++++
> > 1 file changed, 115 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-mt8183.txt
> >
> > diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-mt8183.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-mt8183.txt
> > new file mode 100644
> > index 0000000..364e673
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-mt8183.txt
> > @@ -0,0 +1,115 @@
> > +* Mediatek MT8183 Pin Controller
> > +
> > +The Mediatek's Pin controller is used to control SoC pins.
> > +
> > +Required properties:
> > +- compatible: value should be one of the following.
> > + "mediatek,mt8183-pinctrl", compatible with mt8183 pinctrl.
> > +- gpio-controller : Marks the device node as a gpio controller.
> > +- #gpio-cells: number of cells in GPIO specifier. Since the generic GPIO
> > + binding is used, the amount of cells must be specified as 2. See the below
> > + mentioned gpio binding representation for description of particular cells.
> > +- gpio-ranges : gpio valid number range.
> > +- reg: physicall address base for gpio base registers. There are nine
> > + physicall address base in mt8183. They are 0x10005000, 0x11F20000,
>
> Still have a typo in 'physicall'
==>sorry, we will change it in next version.
>
> > + 0x11E80000, 0x11E70000, 0x11E90000, 0x11D30000, 0x11D20000, 0x11C50000,
> > + 0x11F30000.
>
> You don't need to list the address values. Only how many and what each
> one is.
==>ok, because every base address don't have the detailed name. We will
remove the address values list and change it like this:
- reg: physical address base for gpio base registers. There are nine
physical address base in mt8183.
>
> > +
> > + Eg: <&pio 6 0>
> > + <[phandle of the gpio controller node]
> > + [line number within the gpio controller]
> > + [flags]>
> > +
> > + Values for gpio specifier:
> > + - Line number: is a value between 0 to 202.
> > + - Flags: bit field of flags, as defined in <dt-bindings/gpio/gpio.h>.
> > + Only the following flags are supported:
> > + 0 - GPIO_ACTIVE_HIGH
> > + 1 - GPIO_ACTIVE_LOW
> > +
> > +Optional properties:
> > +- reg-names: gpio base register names. There are nine gpio base register
> > + names in mt8183. They are "iocfg0", "iocfg1", "iocfg2", "iocfg3", "iocfg4",
> > + "iocfg5", "iocfg6", "iocfg7", "iocfg8".
>
> As I said before, these names aren't useful. There's already
> inheritently an index with 'reg'.
>
> Unless some are optional and can be sparsely populated.
==> Do you mean that we can remove Optional properties the reg-names
description?
>
> > +- interrupt-controller: Marks the device node as an interrupt controller
> > +- #interrupt-cells: Should be two.
> > +- interrupts : The interrupt outputs from the controller.
> > +
> > +Please refer to pinctrl-bindings.txt in this directory for details of the
> > +common pinctrl bindings used by client devices.
> > +
> > +Subnode format
> > +A pinctrl node should contain at least one subnodes representing the
> > +pinctrl groups available on the machine. Each subnode will list the
> > +pins it needs, and how they should be configured, with regard to muxer
> > +configuration, pullups, drive strength, input enable/disable and input schmitt.
> > +
> > + node {
> > + pinmux = <PIN_NUMBER_PINMUX>;
> > + GENERIC_PINCONFIG;
> > + };
> > +
> > +Required properties:
> > +- pinmux: integer array, represents gpio pin number and mux setting.
> > + Supported pin number and mux varies for different SoCs, and are defined
> > + as macros in boot/dts/<soc>-pinfunc.h directly.
> > +
> > +Optional properties:
> > +- GENERIC_PINCONFIG: is the generic pinconfig options to use, bias-disable,
> > + bias-pull-down, bias-pull-up, input-enable, input-disable, output-low, output-high,
> > + input-schmitt-enable, input-schmitt-disable and drive-strength are valid.
> > +
> > + Some special pins have extra pull up strength, there are R0 and R1 pull-up
> > + resistors available, but for user, it's only need to set R1R0 as 00, 01, 10 or 11.
> > + So when config mediatek,pull-up-adv or mediatek,pull-down-adv,
> > + it support arguments for those special pins.
> > +
> > + When config drive-strength, it can support some arguments, such as
> > + MTK_DRIVE_4mA, MTK_DRIVE_6mA, etc. See dt-bindings/pinctrl/mt65xx.h.
> > +
> > +Examples:
> > +
> > +#include "mt8183-pinfunc.h"
> > +
> > +...
> > +{
> > + pio: pinctrl@10005000 {
> > + compatible = "mediatek,mt8183-pinctrl";
> > + reg = <0 0x10005000 0 0x1000>,
> > + <0 0x11F20000 0 0x1000>,
> > + <0 0x11E80000 0 0x1000>,
> > + <0 0x11E70000 0 0x1000>,
> > + <0 0x11E90000 0 0x1000>,
> > + <0 0x11D30000 0 0x1000>,
> > + <0 0x11D20000 0 0x1000>,
> > + <0 0x11C50000 0 0x1000>,
> > + <0 0x11F30000 0 0x1000>;
>
> Use lowercase hex.
==> we will change it in the next version.
>
> > + reg-names = "iocfg0", "iocfg1", "iocfg2",
> > + "iocfg3", "iocfg4", "iocfg5",
> > + "iocfg6", "iocfg7", "iocfg8";
> > + gpio-controller;
> > + #gpio-cells = <2>;
> > + gpio-ranges = <&pio 0 0 192>;
> > + interrupt-controller;
> > + interrupts = <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>;
> > + interrupt-parent = <&gic>;
> > + #interrupt-cells = <2>;
> > +
> > + i2c0_pins_a: i2c0 {
> > + pins1 {
> > + pinmux = <PINMUX_GPIO48__FUNC_SCL5>,
> > + <PINMUX_GPIO49__FUNC_SDA5>;
> > + mediatek,pull-up-adv = <11>;
> > + };
> > + };
> > +
> > + i2c1_pins_a: i2c1 {
> > + pins {
> > + pinmux = <PINMUX_GPIO50__FUNC_SCL3>,
> > + <PINMUX_GPIO51__FUNC_SDA3>;
> > + mediatek,pull-down-adv = <10>;
> > + };
> > + };
> > + ...
> > + };
> > +};
> > --
> > 1.9.1
> >
^ permalink raw reply
* 答复: 答复: 答复: [PATCH][v4] tty: fix race between flush_to_ldisc and tty_open
From: Li,Rongqing @ 2019-01-31 7:40 UTC (permalink / raw)
To: Greg KH
Cc: jslaby@suse.com, linux-kernel@vger.kernel.org,
gkohli@codeaurora.org, linux-serial@vger.kernel.org
In-Reply-To: <20190131065215.GB30992@kroah.com>
> -----邮件原件-----
> 发件人: Greg KH [mailto:gregkh@linuxfoundation.org]
> 发送时间: 2019年1月31日 14:52
> 收件人: Li,Rongqing <lirongqing@baidu.com>
> 抄送: jslaby@suse.com; linux-kernel@vger.kernel.org; gkohli@codeaurora.org;
> linux-serial@vger.kernel.org
> 主题: Re: 答复: 答复: [PATCH][v4] tty: fix race between flush_to_ldisc and
> tty_open
>
> On Thu, Jan 31, 2019 at 02:15:35AM +0000, Li,Rongqing wrote:
> >
> >
> > > -----邮件原件-----
> > > 发件人: Greg KH [mailto:gregkh@linuxfoundation.org]
> > > 发送时间: 2019年1月30日 21:17
> > > 收件人: Li,Rongqing <lirongqing@baidu.com>
> > > 抄送: jslaby@suse.com; linux-kernel@vger.kernel.org;
> > > gkohli@codeaurora.org; linux-serial@vger.kernel.org
> > > 主题: Re: 答复: [PATCH][v4] tty: fix race between flush_to_ldisc and
> > > tty_open
> > >
> > > On Wed, Jan 30, 2019 at 12:48:42PM +0000, Li,Rongqing wrote:
> > > >
> > > >
> > > > > -----邮件原件-----
> > > > > 发件人: linux-kernel-owner@vger.kernel.org
> > > > > [mailto:linux-kernel-owner@vger.kernel.org] 代表 Greg KH
> > > > > 发送时间: 2019年1月30日 18:19
> > > > > 收件人: Li,Rongqing <lirongqing@baidu.com>
> > > > > 抄送: jslaby@suse.com; linux-kernel@vger.kernel.org;
> > > > > gkohli@codeaurora.org
> > > > > 主题: Re: [PATCH][v4] tty: fix race between flush_to_ldisc and
> > > > > tty_open
> > > > >
> > > > > On Fri, Jan 18, 2019 at 05:27:17PM +0800, Li RongQing wrote:
> > > > > > There still is a race window after the commit b027e2298bd588
> > > > > > ("tty: fix data race between tty_init_dev and flush of buf"),
> > > > > > and we encountered this crash issue if receive_buf call comes
> > > > > > before tty initialization completes in n_tty_open and
> > > > > > tty->driver_data may be NULL.
> > > > > >
> > > > > > CPU0 CPU1
> > > > > > ---- ----
> > > > > > n_tty_open
> > > > > > tty_init_dev
> > > > > > tty_ldisc_unlock
> > > > > > schedule
> flush_to_ldisc
> > > > > > receive_buf
> > > > > > tty_port_default_receive_buf
> > > > > > tty_ldisc_receive_buf
> > > > > > n_tty_receive_buf_common
> > > > > > __receive_buf
> > > > > > uart_flush_chars
> > > > > > uart_start
> > > > > > /*tty->driver_data is NULL*/
> > > > > > tty->ops->open
> > > > > > /*init tty->driver_data*/
> > > > > >
> > > > > > it can be fixed by extending ldisc semaphore lock in
> > > > > > tty_init_dev to driver_data initialized completely after
> > > > > > tty->ops->open(), but this will lead to put lock on one
> > > > > > function and unlock in some other function, and hard to
> > > > > > maintain, so fix this race only by checking
> > > > > > tty->driver_data when receiving, and return if
> > > > > > tty->tty->driver_data
> > > > > > is NULL
> > > > > >
> > > > > > Signed-off-by: Wang Li <wangli39@baidu.com>
> > > > > > Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
> > > > > > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > > > > > ---
> > > > > > V4: add version information
> > > > > > V3: not used ldisc semaphore lock, only checking
> > > > > > tty->driver_data with NULL
> > > > > > V2: fix building error by EXPORT_SYMBOL tty_ldisc_unlock
> > > > > > V1: extend ldisc lock to protect that tty->driver_data is
> > > > > > inited
> > > > > >
> > > > > > drivers/tty/tty_port.c | 3 +++
> > > > > > 1 file changed, 3 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> > > > > > index
> > > > > > 044c3cbdcfa4..86d0bec38322 100644
> > > > > > --- a/drivers/tty/tty_port.c
> > > > > > +++ b/drivers/tty/tty_port.c
> > > > > > @@ -31,6 +31,9 @@ static int
> > > > > > tty_port_default_receive_buf(struct
> > > > > > tty_port
> > > > > *port,
> > > > > > if (!tty)
> > > > > > return 0;
> > > > > >
> > > > > > + if (!tty->driver_data)
> > > > > > + return 0;
> > > > > > +
> > > > >
> > > > > How is this working? What is setting driver_data to NULL to
> > > > > "stop" this
> > > race?
> > > > >
> > > >
> > > >
> > > > if tty->driver_data is NULL and return,
> > > > tty_port_default_receive_buf will not step to uart_start which
> > > > access tty->driver_data and trigger panic before tty_open, so it
> > > > can fix the system panic
> > > >
> > > > > There's no requirement that a tty driver set this field to NULL
> > > > > when it is
> > > "done"
> > > > > with the tty device, so I think you are just getting lucky in
> > > > > that your specific driver happens to be doing this.
> > > > >
> > > >
> > > > when tty_open is running, tty is allocated by kzalloc in
> > > > tty_init_dev which called by tty_open_by_driver, tty is inited to
> > > > 0
> > > >
> > > > > What driver are you testing this against?
> > > > >
> > > >
> > > > 8250
> > >
> > > Ok, as this is specific to the uart core, how about this patch instead:
> > >
> > > diff --git a/drivers/tty/serial/serial_core.c
> > > b/drivers/tty/serial/serial_core.c
> > > index 5c01bb6d1c24..b56a6250df3f 100644
> > > --- a/drivers/tty/serial/serial_core.c
> > > +++ b/drivers/tty/serial/serial_core.c
> > > @@ -130,6 +130,9 @@ static void uart_start(struct tty_struct *tty)
> > > struct uart_port *port;
> > > unsigned long flags;
> > >
> > > + if (!state)
> > > + return;
> > > +
> > > port = uart_port_lock(state, flags);
> > > __uart_start(tty);
> > > uart_port_unlock(port, flags);
> >
> >
> > If move the check into uart_start, i am afraid that it maybe not fully
> > fix this issue, Since n_tty_receive_buf_common maybe call
> > n_tty_check_throttle/ tty_unthrottle_safe which maybe use the
> > tty->driver_data
> >
> > if tty is not fully opened, I think no gain to step into more function
>
> But as I said, the tty core has no knowledge of the "driver_data", field. It
> does not know if a driver really is even using that field, so it means nothing to
> the tty core, so it can not check it. Your specific tty driver does happen to use
> it, so it can check it.
>
> If you also need to check this in unthrottle, how about this patch too?
> Does the combination of these two patches solve the problem for your
> systems?
>
> thanks,
>
> greg k-h
>
Thanks for you explanation, I see now
Your suggestion should work, I will send V5 based on your suggestion
-RongQing
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 5c01bb6d1c24..e33d4c181123 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -727,6 +727,9 @@ static void uart_unthrottle(struct tty_struct *tty)
> upstat_t mask = UPSTAT_SYNC_FIFO;
> struct uart_port *port;
>
> + if (!state)
> + return;
> +
> port = uart_port_ref(state);
> if (!port)
> return;
^ permalink raw reply
* Re: 答复: 答复: [PATCH][v4] tty: fix race between flush_to_ldisc and tty_open
From: Greg KH @ 2019-01-31 6:52 UTC (permalink / raw)
To: Li,Rongqing
Cc: jslaby@suse.com, linux-kernel@vger.kernel.org,
gkohli@codeaurora.org, linux-serial@vger.kernel.org
In-Reply-To: <b1388c835686492fbbfa91e9711470d4@baidu.com>
On Thu, Jan 31, 2019 at 02:15:35AM +0000, Li,Rongqing wrote:
>
>
> > -----邮件原件-----
> > 发件人: Greg KH [mailto:gregkh@linuxfoundation.org]
> > 发送时间: 2019年1月30日 21:17
> > 收件人: Li,Rongqing <lirongqing@baidu.com>
> > 抄送: jslaby@suse.com; linux-kernel@vger.kernel.org; gkohli@codeaurora.org;
> > linux-serial@vger.kernel.org
> > 主题: Re: 答复: [PATCH][v4] tty: fix race between flush_to_ldisc and tty_open
> >
> > On Wed, Jan 30, 2019 at 12:48:42PM +0000, Li,Rongqing wrote:
> > >
> > >
> > > > -----邮件原件-----
> > > > 发件人: linux-kernel-owner@vger.kernel.org
> > > > [mailto:linux-kernel-owner@vger.kernel.org] 代表 Greg KH
> > > > 发送时间: 2019年1月30日 18:19
> > > > 收件人: Li,Rongqing <lirongqing@baidu.com>
> > > > 抄送: jslaby@suse.com; linux-kernel@vger.kernel.org;
> > > > gkohli@codeaurora.org
> > > > 主题: Re: [PATCH][v4] tty: fix race between flush_to_ldisc and
> > > > tty_open
> > > >
> > > > On Fri, Jan 18, 2019 at 05:27:17PM +0800, Li RongQing wrote:
> > > > > There still is a race window after the commit b027e2298bd588
> > > > > ("tty: fix data race between tty_init_dev and flush of buf"), and
> > > > > we encountered this crash issue if receive_buf call comes before
> > > > > tty initialization completes in n_tty_open and
> > > > > tty->driver_data may be NULL.
> > > > >
> > > > > CPU0 CPU1
> > > > > ---- ----
> > > > > n_tty_open
> > > > > tty_init_dev
> > > > > tty_ldisc_unlock
> > > > > schedule flush_to_ldisc
> > > > > receive_buf
> > > > > tty_port_default_receive_buf
> > > > > tty_ldisc_receive_buf
> > > > > n_tty_receive_buf_common
> > > > > __receive_buf
> > > > > uart_flush_chars
> > > > > uart_start
> > > > > /*tty->driver_data is NULL*/
> > > > > tty->ops->open
> > > > > /*init tty->driver_data*/
> > > > >
> > > > > it can be fixed by extending ldisc semaphore lock in tty_init_dev
> > > > > to driver_data initialized completely after tty->ops->open(), but
> > > > > this will lead to put lock on one function and unlock in some
> > > > > other function, and hard to maintain, so fix this race only by
> > > > > checking
> > > > > tty->driver_data when receiving, and return if tty->driver_data
> > > > > is NULL
> > > > >
> > > > > Signed-off-by: Wang Li <wangli39@baidu.com>
> > > > > Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
> > > > > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > > > > ---
> > > > > V4: add version information
> > > > > V3: not used ldisc semaphore lock, only checking tty->driver_data
> > > > > with NULL
> > > > > V2: fix building error by EXPORT_SYMBOL tty_ldisc_unlock
> > > > > V1: extend ldisc lock to protect that tty->driver_data is inited
> > > > >
> > > > > drivers/tty/tty_port.c | 3 +++
> > > > > 1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c index
> > > > > 044c3cbdcfa4..86d0bec38322 100644
> > > > > --- a/drivers/tty/tty_port.c
> > > > > +++ b/drivers/tty/tty_port.c
> > > > > @@ -31,6 +31,9 @@ static int tty_port_default_receive_buf(struct
> > > > > tty_port
> > > > *port,
> > > > > if (!tty)
> > > > > return 0;
> > > > >
> > > > > + if (!tty->driver_data)
> > > > > + return 0;
> > > > > +
> > > >
> > > > How is this working? What is setting driver_data to NULL to "stop" this
> > race?
> > > >
> > >
> > >
> > > if tty->driver_data is NULL and return, tty_port_default_receive_buf
> > > will not step to uart_start which access tty->driver_data and trigger
> > > panic before tty_open, so it can fix the system panic
> > >
> > > > There's no requirement that a tty driver set this field to NULL when it is
> > "done"
> > > > with the tty device, so I think you are just getting lucky in that
> > > > your specific driver happens to be doing this.
> > > >
> > >
> > > when tty_open is running, tty is allocated by kzalloc in tty_init_dev
> > > which called by tty_open_by_driver, tty is inited to 0
> > >
> > > > What driver are you testing this against?
> > > >
> > >
> > > 8250
> >
> > Ok, as this is specific to the uart core, how about this patch instead:
> >
> > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> > index 5c01bb6d1c24..b56a6250df3f 100644
> > --- a/drivers/tty/serial/serial_core.c
> > +++ b/drivers/tty/serial/serial_core.c
> > @@ -130,6 +130,9 @@ static void uart_start(struct tty_struct *tty)
> > struct uart_port *port;
> > unsigned long flags;
> >
> > + if (!state)
> > + return;
> > +
> > port = uart_port_lock(state, flags);
> > __uart_start(tty);
> > uart_port_unlock(port, flags);
>
>
> If move the check into uart_start, i am afraid that it maybe not fully fix this issue,
> Since n_tty_receive_buf_common maybe call n_tty_check_throttle/
> tty_unthrottle_safe which maybe use the tty->driver_data
>
> if tty is not fully opened, I think no gain to step into more function
But as I said, the tty core has no knowledge of the "driver_data",
field. It does not know if a driver really is even using that field, so
it means nothing to the tty core, so it can not check it. Your specific
tty driver does happen to use it, so it can check it.
If you also need to check this in unthrottle, how about this patch too?
Does the combination of these two patches solve the problem for your
systems?
thanks,
greg k-h
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 5c01bb6d1c24..e33d4c181123 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -727,6 +727,9 @@ static void uart_unthrottle(struct tty_struct *tty)
upstat_t mask = UPSTAT_SYNC_FIFO;
struct uart_port *port;
+ if (!state)
+ return;
+
port = uart_port_ref(state);
if (!port)
return;
^ permalink raw reply related
* Re: [PATCH v6 6/6] arm64: dts: Add Mediatek SoC MT8183 and evaluation board dts and Makefile
From: Erin Lo @ 2019-01-31 2:34 UTC (permalink / raw)
To: Rob Herring
Cc: Mark Rutland, Ben Ho, mars.cheng-NuS5LvNUpcJWk0Htik3J/w,
Mengqi Zhang, linux-clk-u79uwXL29TY76Z2rM5mHXA, Hsin-Hsiung Wang,
Weiyi Lu, Marc Zyngier, linux-serial-u79uwXL29TY76Z2rM5mHXA,
yingjoe.chen-NuS5LvNUpcJWk0Htik3J/w,
devicetree-u79uwXL29TY76Z2rM5mHXA, Jason Cooper, Seiya Wang,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Matthias Brugger,
Thomas Gleixner, eddie.huang-NuS5LvNUpcJWk0Htik3J/w,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, srv_heupstream,
Greg Kroah-Hartman, Stephen Boyd,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Zhiyong Tao
In-Reply-To: <20190130162204.GA1521@bogus>
On Wed, 2019-01-30 at 10:22 -0600, Rob Herring wrote:
> On Thu, Jan 24, 2019 at 04:07:20PM +0800, Erin Lo wrote:
> > From: Ben Ho <Ben.Ho-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> >
> > Add basic chip support for Mediatek 8183, include
> > pinctrl file, uart node with correct uart clocks, pwrap device
> >
> > Add clock controller nodes, include topckgen, infracfg,
> > apmixedsys and subsystem.
> >
> > Signed-off-by: Ben Ho <Ben.Ho-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > Signed-off-by: Erin Lo <erin.lo-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > Signed-off-by: Seiya Wang <seiya.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > Signed-off-by: Zhiyong Tao <zhiyong.tao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > Signed-off-by: Weiyi Lu <weiyi.lu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > Signed-off-by: Mengqi Zhang <Mengqi.Zhang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > Signed-off-by: Hsin-Hsiung Wang <hsin-hsiung.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > Signed-off-by: Eddie Huang <eddie.huang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > ---
>
>
> > + sysirq: intpol-controller@c530a80 {
>
> interrupt-controller@...
I will modify it in next version.
>
>
> Place all the MMIO peripherals under one or more simple-bus nodes.
>
> Rob
>
Do you mean need to add simple-bus like this?
+ soc: soc {
+ #address-cells = <0x1>;
+ #size-cells = <0x1>;
+ ranges = <0 0 0 0xffffffff>;
+ compatible = "simple-bus";
soc_data: soc_data@08000000 {
compatible = "mediatek,mt8183-efuse",
"mediatek,efuse";
reg = <0 0x08000000 0 0x0010>;
#address-cells = <1>;
#size-cells = <1>;
status = "disabled";
};
gic: interrupt-controller@0c000000 {
compatible = "arm,gic-v3";
#interrupt-cells = <4>;
Best Regards,
Erin
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply
* 答复: 答复: [PATCH][v4] tty: fix race between flush_to_ldisc and tty_open
From: Li,Rongqing @ 2019-01-31 2:15 UTC (permalink / raw)
To: Greg KH
Cc: jslaby@suse.com, linux-kernel@vger.kernel.org,
gkohli@codeaurora.org, linux-serial@vger.kernel.org
In-Reply-To: <20190130131641.GA10795@kroah.com>
> -----邮件原件-----
> 发件人: Greg KH [mailto:gregkh@linuxfoundation.org]
> 发送时间: 2019年1月30日 21:17
> 收件人: Li,Rongqing <lirongqing@baidu.com>
> 抄送: jslaby@suse.com; linux-kernel@vger.kernel.org; gkohli@codeaurora.org;
> linux-serial@vger.kernel.org
> 主题: Re: 答复: [PATCH][v4] tty: fix race between flush_to_ldisc and tty_open
>
> On Wed, Jan 30, 2019 at 12:48:42PM +0000, Li,Rongqing wrote:
> >
> >
> > > -----邮件原件-----
> > > 发件人: linux-kernel-owner@vger.kernel.org
> > > [mailto:linux-kernel-owner@vger.kernel.org] 代表 Greg KH
> > > 发送时间: 2019年1月30日 18:19
> > > 收件人: Li,Rongqing <lirongqing@baidu.com>
> > > 抄送: jslaby@suse.com; linux-kernel@vger.kernel.org;
> > > gkohli@codeaurora.org
> > > 主题: Re: [PATCH][v4] tty: fix race between flush_to_ldisc and
> > > tty_open
> > >
> > > On Fri, Jan 18, 2019 at 05:27:17PM +0800, Li RongQing wrote:
> > > > There still is a race window after the commit b027e2298bd588
> > > > ("tty: fix data race between tty_init_dev and flush of buf"), and
> > > > we encountered this crash issue if receive_buf call comes before
> > > > tty initialization completes in n_tty_open and
> > > > tty->driver_data may be NULL.
> > > >
> > > > CPU0 CPU1
> > > > ---- ----
> > > > n_tty_open
> > > > tty_init_dev
> > > > tty_ldisc_unlock
> > > > schedule flush_to_ldisc
> > > > receive_buf
> > > > tty_port_default_receive_buf
> > > > tty_ldisc_receive_buf
> > > > n_tty_receive_buf_common
> > > > __receive_buf
> > > > uart_flush_chars
> > > > uart_start
> > > > /*tty->driver_data is NULL*/
> > > > tty->ops->open
> > > > /*init tty->driver_data*/
> > > >
> > > > it can be fixed by extending ldisc semaphore lock in tty_init_dev
> > > > to driver_data initialized completely after tty->ops->open(), but
> > > > this will lead to put lock on one function and unlock in some
> > > > other function, and hard to maintain, so fix this race only by
> > > > checking
> > > > tty->driver_data when receiving, and return if tty->driver_data
> > > > is NULL
> > > >
> > > > Signed-off-by: Wang Li <wangli39@baidu.com>
> > > > Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
> > > > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > > > ---
> > > > V4: add version information
> > > > V3: not used ldisc semaphore lock, only checking tty->driver_data
> > > > with NULL
> > > > V2: fix building error by EXPORT_SYMBOL tty_ldisc_unlock
> > > > V1: extend ldisc lock to protect that tty->driver_data is inited
> > > >
> > > > drivers/tty/tty_port.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c index
> > > > 044c3cbdcfa4..86d0bec38322 100644
> > > > --- a/drivers/tty/tty_port.c
> > > > +++ b/drivers/tty/tty_port.c
> > > > @@ -31,6 +31,9 @@ static int tty_port_default_receive_buf(struct
> > > > tty_port
> > > *port,
> > > > if (!tty)
> > > > return 0;
> > > >
> > > > + if (!tty->driver_data)
> > > > + return 0;
> > > > +
> > >
> > > How is this working? What is setting driver_data to NULL to "stop" this
> race?
> > >
> >
> >
> > if tty->driver_data is NULL and return, tty_port_default_receive_buf
> > will not step to uart_start which access tty->driver_data and trigger
> > panic before tty_open, so it can fix the system panic
> >
> > > There's no requirement that a tty driver set this field to NULL when it is
> "done"
> > > with the tty device, so I think you are just getting lucky in that
> > > your specific driver happens to be doing this.
> > >
> >
> > when tty_open is running, tty is allocated by kzalloc in tty_init_dev
> > which called by tty_open_by_driver, tty is inited to 0
> >
> > > What driver are you testing this against?
> > >
> >
> > 8250
>
> Ok, as this is specific to the uart core, how about this patch instead:
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 5c01bb6d1c24..b56a6250df3f 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -130,6 +130,9 @@ static void uart_start(struct tty_struct *tty)
> struct uart_port *port;
> unsigned long flags;
>
> + if (!state)
> + return;
> +
> port = uart_port_lock(state, flags);
> __uart_start(tty);
> uart_port_unlock(port, flags);
If move the check into uart_start, i am afraid that it maybe not fully fix this issue,
Since n_tty_receive_buf_common maybe call n_tty_check_throttle/
tty_unthrottle_safe which maybe use the tty->driver_data
if tty is not fully opened, I think no gain to step into more function
thanks
-RongQing
^ permalink raw reply
* Re: [PATCH] tty: Fix WARNING in tty_set_termios
From: shuah @ 2019-01-31 0:35 UTC (permalink / raw)
To: Johan Hovold
Cc: Al Viro, linux-wireless, chris, devel, robh, sameo, linux-serial,
jslaby, santhameena13, kirk, johan.hedberg, arnd, marcel,
samuel.thibault, m.maya.nakamura, zhongjiang, gregkh, speakup,
linux-kernel, linux-bluetooth, netdev, nishka.dasgupta_ug18,
davem, shuah
In-Reply-To: <20190130103227.GR3691@localhost>
On 1/30/19 3:32 AM, Johan Hovold wrote:
> On Mon, Jan 28, 2019 at 02:29:22PM -0700, shuah wrote:
>> On 1/25/19 9:14 PM, Al Viro wrote:
>>> On Fri, Jan 25, 2019 at 04:29:05PM -0700, Shuah Khan wrote:
>>>> tty_set_termios() has the following WARMN_ON which can be triggered with a
>>>> syscall to invoke TIOCGETD __NR_ioctl.
>
> You meant TIOCSETD here, and in fact its the call which sets the uart
> protocol that triggers the warning.
Right. It is a TIOCSETD.
>
>>>> WARN_ON(tty->driver->type == TTY_DRIVER_TYPE_PTY &&
>>>> tty->driver->subtype == PTY_TYPE_MASTER);
>>>> Reference: https://syzkaller.appspot.com/bug?id=2410d22f1d8e5984217329dd0884b01d99e3e48d
>>>>
>>>> A simple change would have been to print error message instead of WARN_ON.
>>>> However, the callers assume that tty_set_termios() always returns 0 and
>>>> don't check return value. The complete solution is fixing all the callers
>>>> to check error and bail out to fix the WARN_ON.
>>>>
>>>> This fix changes tty_set_termios() to return error and all the callers
>>>> to check error and bail out. The reproducer is used to reproduce the
>>>> problem and verify the fix.
>>>
>>>> --- a/drivers/bluetooth/hci_ldisc.c
>>>> +++ b/drivers/bluetooth/hci_ldisc.c
>>>> @@ -321,6 +321,8 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
>>>> status = tty_set_termios(tty, &ktermios);
>>>> BT_DBG("Disabling hardware flow control: %s",
>>>> status ? "failed" : "success");
>>>> + if (status)
>>>> + return;
>>>
>>> Can that ldisc end up set on pty master? And does it make any sense there?
>>
>> The initial objective of the patch is to prevent the WARN_ON by making
>> the change to return error instead of WARN_ON. However, without changes
>> to places that don't check the return and keep making progress, there
>> will be secondary problems.
>>
>> Without this change to return here, instead of WARN_ON, it will fail
>> with the following NULL pointer dereference at the next thing
>> hci_uart_set_flow_control() attempts.
>>
>> status = tty->driver->ops->tiocmget(tty);
>>
>> kernel: [10140.649783] BUG: unable to handle kernel NULL pointer
>
> That's a separate issue, which is being fixed:
>
> https://lkml.kernel.org/r/20190130095938.GP3691@localhost
>
Ah good to know.
>>> IOW, I don't believe that this patch makes any sense. If anything,
>>> we need to prevent unconditional tty_set_termios() on the path
>>> that *does* lead to calling it for pty.
>>
>> I don't think preventing unconditional tty_set_termios() is enough to
>> prevent secondary problems such as the one above.
>>
>> For example, the following call chain leads to the WARN_ON that was
>> reported. Even if void hci_uart_set_baudrate() prevents the very first
>> tty_set_termios() call, its caller hci_uart_setup() continues with
>> more tty setup. It goes ahead to call driver setup callback. The
>> driver callback goes on to do more setup calling tty_set_termios().
>>
>> WARN_ON call path:
>> hci_uart_set_baudrate+0x1cc/0x250 drivers/bluetooth/hci_ldisc.c:378
>> hci_uart_setup+0xa2/0x490 drivers/bluetooth/hci_ldisc.c:401
>> hci_dev_do_open+0x6b1/0x1920 net/bluetooth/hci_core.c:1423
>>
>> Once this WARN_ON is changed to return error, the following
>> happens, when hci_uart_setup() does driver setup callback.
>>
>> kernel: [10140.649836] mrvl_setup+0x17/0x80 [hci_uart]
>> kernel: [10140.649840] hci_uart_setup+0x56/0x160 [hci_uart]
>> kernel: [10140.649850] hci_dev_do_open+0xe6/0x630 [bluetooth]
>> kernel: [10140.649860] hci_power_on+0x52/0x220 [bluetooth]
>>
>> I think continuing to catch the invalid condition in tty_set_termios()
>> and preventing progress by checking return value is a straight forward
>> change to avoid secondary problems, and it might be difficult to catch
>> all the cases where it could fail.
>
> I agree with Al that this change doesn't make much sense. The WARN_ON
> is there to catch any bugs leading to the termios being changed for a
> master side pty. Those should bugs should be fixed, and not worked
> around in order to silence a WARN_ON.
>
> The problem started with 7721383f4199 ("Bluetooth: hci_uart: Support
> operational speed during setup") which introduced a new way for how
> tty_set_termios() could end up being called for a master pty.
>
Ah. Thanks for the context.
> As Al hinted at, setting these ldiscs for a master pty really makes no
> sense and perhaps that is what we should prevent unless simply making
> sure they do not call tty_set_termios() is sufficient for the time
> being.
>
I will take a look to see if not calling tty_set_termios() is enough.
> Finally, note that serdev never operates on a pty, and that this is only
> an issue for (the three) line disciplines.
>
Thanks for the detailed message explaining the evolution. Now it makes
sense.
-- Shuah
^ permalink raw reply
* Re: [PATCH 1/6] dt-bindings: soc: qcom: Add interconnect binding for GENI QUP
From: alokc @ 2019-01-30 22:29 UTC (permalink / raw)
To: Georgi Djakov
Cc: linux-arm-msm, devicetree, linux-kernel, linux-i2c, linux-spi,
linux-serial, Andy Gross, David Brown, Rob Herring, Mark Rutland,
dianders, swboyd, bjorn.andersson
In-Reply-To: <c4260d66-5ca4-b194-0cbc-2a682a99db4b@linaro.org>
On 2019-01-23 10:35, Georgi Djakov wrote:
> Hi Alok,
>
> Thanks for the patches!
>
> On 1/22/19 08:33, Alok Chauhan wrote:
>> Add documentation for the interconnect and interconnect-names bindings
>
> s/interconnect /interconnects /
>
>> for the GENI QUP as detailed by
>> bindings/interconnect/interconnect.txt.
>>
>> Signed-off-by: Alok Chauhan <alokc@codeaurora.org>
>> ---
>> Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt | 10
>> ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>> b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>> index dab7ca9..44d7e02 100644
>> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>> @@ -17,6 +17,12 @@ Required properties if child node exists:
>> - #address-cells: Must be <1> for Serial Engine Address
>> - #size-cells: Must be <1> for Serial Engine Address Size
>> - ranges: Must be present
>> +- interconnects: phandle to a interconnect provider. Please refer
>
> s/a interconnect/an interconnect/
sure, will do.
>
>> + ../interconnect/interconnect.txt for details.
>> + Must be 2 paths corresponding to 2 AXI ports.
>> +- interconnect-names: Port names to differentiate between the
>> + 2 interconnect paths defined with interconnect
>> + specifier.
>>
>> Properties for children:
>>
>> @@ -67,6 +73,10 @@ Example:
>> #size-cells = <1>;
>> ranges;
>>
>> + interconnects = <&qnoc 11 &qnoc 512>,
>> + <&qnoc 0 &qnoc 543>;
>
> Please take a snippet from your patch 6/6 and put it here instead of
> the
> hard-coded integers above.
sure
>
>> + interconnect-names = "qup-memory", "qup-config";
>> +
>> i2c0: i2c@a94000 {
>> compatible = "qcom,geni-i2c";
>> reg = <0xa94000 0x4000>;
>>
>
> When you post a new version, please change the subject of the patch
> series to PATCH v2, PATCH v3 etc, in order to be able to distinguish
> between different versions.
sure, will do this.
>
> Thanks,
> Georgi
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH 1/6] dt-bindings: soc: qcom: Add interconnect binding for GENI QUP
From: alokc @ 2019-01-30 22:27 UTC (permalink / raw)
To: Evan Green
Cc: linux-arm-msm, devicetree, LKML, linux-i2c, linux-spi,
linux-serial, Andy Gross, David Brown, Rob Herring, Mark Rutland,
Georgi Djakov, Doug Anderson, Stephen Boyd, Bjorn Andersson,
linux-kernel-owner
In-Reply-To: <CAE=gft5+OJCHTVszSW2Tt9HX3UA-LuTzK95Q4-R3v=DUqff9OA@mail.gmail.com>
On 2019-01-23 17:10, Evan Green wrote:
> On Mon, Jan 21, 2019 at 10:34 PM Alok Chauhan <alokc@codeaurora.org>
> wrote:
>>
>> Add documentation for the interconnect and interconnect-names bindings
>> for the GENI QUP as detailed by
>> bindings/interconnect/interconnect.txt.
>>
>> Signed-off-by: Alok Chauhan <alokc@codeaurora.org>
>> ---
>> Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt | 10
>> ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>> b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>> index dab7ca9..44d7e02 100644
>> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>> @@ -17,6 +17,12 @@ Required properties if child node exists:
>> - #address-cells: Must be <1> for Serial Engine Address
>> - #size-cells: Must be <1> for Serial Engine Address
>> Size
>> - ranges: Must be present
>> +- interconnects: phandle to a interconnect provider. Please
>> refer
>> + ../interconnect/interconnect.txt for details.
>
> This path to the interconnect documentation is not correct.
sorry, i will correct this in next patch.
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v3 2/2] Dt-bindings: Serial: Add X1000 serial bindings.
From: Rob Herring @ 2019-01-30 18:46 UTC (permalink / raw)
To: Zhou Yanjie
Cc: linux-mips, linux-kernel, linux-serial, devicetree, robh+dt,
paul.burton, gregkh, jslaby, mark.rutland, syq, jiaxun.yang,
772753199
In-Reply-To: <1548695029-11900-3-git-send-email-zhouyanjie@zoho.com>
On Tue, 29 Jan 2019 01:03:49 +0800, Zhou Yanjie wrote:
> Add the serial bindings for the X1000 Soc from Ingenic.
>
> Signed-off-by: Zhou Yanjie <zhouyanjie@zoho.com>
> ---
> Documentation/devicetree/bindings/serial/ingenic,uart.txt | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v6 6/6] arm64: dts: Add Mediatek SoC MT8183 and evaluation board dts and Makefile
From: Rob Herring @ 2019-01-30 16:22 UTC (permalink / raw)
To: Erin Lo
Cc: Matthias Brugger, Mark Rutland, Thomas Gleixner, Jason Cooper,
Marc Zyngier, Greg Kroah-Hartman, Stephen Boyd, devicetree,
srv_heupstream, linux-kernel, linux-serial, linux-mediatek,
linux-arm-kernel, yingjoe.chen, mars.cheng, eddie.huang,
linux-clk, Ben Ho, Seiya Wang, Zhiyong Tao, Weiyi Lu,
Mengqi Zhang <Mengqi.Zhang@
In-Reply-To: <1548317240-44682-7-git-send-email-erin.lo@mediatek.com>
On Thu, Jan 24, 2019 at 04:07:20PM +0800, Erin Lo wrote:
> From: Ben Ho <Ben.Ho@mediatek.com>
>
> Add basic chip support for Mediatek 8183, include
> pinctrl file, uart node with correct uart clocks, pwrap device
>
> Add clock controller nodes, include topckgen, infracfg,
> apmixedsys and subsystem.
>
> Signed-off-by: Ben Ho <Ben.Ho@mediatek.com>
> Signed-off-by: Erin Lo <erin.lo@mediatek.com>
> Signed-off-by: Seiya Wang <seiya.wang@mediatek.com>
> Signed-off-by: Zhiyong Tao <zhiyong.tao@mediatek.com>
> Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> Signed-off-by: Mengqi Zhang <Mengqi.Zhang@mediatek.com>
> Signed-off-by: Hsin-Hsiung Wang <hsin-hsiung.wang@mediatek.com>
> Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
> ---
> + sysirq: intpol-controller@c530a80 {
interrupt-controller@...
Place all the MMIO peripherals under one or more simple-bus nodes.
Rob
^ permalink raw reply
* Re: [PATCH v6 5/6] dt-bindings: pinctrl: mt8183: add binding document
From: Rob Herring @ 2019-01-30 16:17 UTC (permalink / raw)
To: Erin Lo
Cc: Matthias Brugger, Mark Rutland, Thomas Gleixner, Jason Cooper,
Marc Zyngier, Greg Kroah-Hartman, Stephen Boyd, devicetree,
srv_heupstream, linux-kernel, linux-serial, linux-mediatek,
linux-arm-kernel, yingjoe.chen, mars.cheng, eddie.huang,
linux-clk, Zhiyong Tao
In-Reply-To: <1548317240-44682-6-git-send-email-erin.lo@mediatek.com>
On Thu, Jan 24, 2019 at 04:07:19PM +0800, Erin Lo wrote:
> From: Zhiyong Tao <zhiyong.tao@mediatek.com>
>
> The commit adds mt8183 compatible node in binding document.
>
> Signed-off-by: Zhiyong Tao <zhiyong.tao@mediatek.com>
> Signed-off-by: Erin Lo <erin.lo@mediatek.com>
> ---
> .../devicetree/bindings/pinctrl/pinctrl-mt8183.txt | 115 +++++++++++++++++++++
> 1 file changed, 115 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-mt8183.txt
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-mt8183.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-mt8183.txt
> new file mode 100644
> index 0000000..364e673
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-mt8183.txt
> @@ -0,0 +1,115 @@
> +* Mediatek MT8183 Pin Controller
> +
> +The Mediatek's Pin controller is used to control SoC pins.
> +
> +Required properties:
> +- compatible: value should be one of the following.
> + "mediatek,mt8183-pinctrl", compatible with mt8183 pinctrl.
> +- gpio-controller : Marks the device node as a gpio controller.
> +- #gpio-cells: number of cells in GPIO specifier. Since the generic GPIO
> + binding is used, the amount of cells must be specified as 2. See the below
> + mentioned gpio binding representation for description of particular cells.
> +- gpio-ranges : gpio valid number range.
> +- reg: physicall address base for gpio base registers. There are nine
> + physicall address base in mt8183. They are 0x10005000, 0x11F20000,
Still have a typo in 'physicall'
> + 0x11E80000, 0x11E70000, 0x11E90000, 0x11D30000, 0x11D20000, 0x11C50000,
> + 0x11F30000.
You don't need to list the address values. Only how many and what each
one is.
> +
> + Eg: <&pio 6 0>
> + <[phandle of the gpio controller node]
> + [line number within the gpio controller]
> + [flags]>
> +
> + Values for gpio specifier:
> + - Line number: is a value between 0 to 202.
> + - Flags: bit field of flags, as defined in <dt-bindings/gpio/gpio.h>.
> + Only the following flags are supported:
> + 0 - GPIO_ACTIVE_HIGH
> + 1 - GPIO_ACTIVE_LOW
> +
> +Optional properties:
> +- reg-names: gpio base register names. There are nine gpio base register
> + names in mt8183. They are "iocfg0", "iocfg1", "iocfg2", "iocfg3", "iocfg4",
> + "iocfg5", "iocfg6", "iocfg7", "iocfg8".
As I said before, these names aren't useful. There's already
inheritently an index with 'reg'.
Unless some are optional and can be sparsely populated.
> +- interrupt-controller: Marks the device node as an interrupt controller
> +- #interrupt-cells: Should be two.
> +- interrupts : The interrupt outputs from the controller.
> +
> +Please refer to pinctrl-bindings.txt in this directory for details of the
> +common pinctrl bindings used by client devices.
> +
> +Subnode format
> +A pinctrl node should contain at least one subnodes representing the
> +pinctrl groups available on the machine. Each subnode will list the
> +pins it needs, and how they should be configured, with regard to muxer
> +configuration, pullups, drive strength, input enable/disable and input schmitt.
> +
> + node {
> + pinmux = <PIN_NUMBER_PINMUX>;
> + GENERIC_PINCONFIG;
> + };
> +
> +Required properties:
> +- pinmux: integer array, represents gpio pin number and mux setting.
> + Supported pin number and mux varies for different SoCs, and are defined
> + as macros in boot/dts/<soc>-pinfunc.h directly.
> +
> +Optional properties:
> +- GENERIC_PINCONFIG: is the generic pinconfig options to use, bias-disable,
> + bias-pull-down, bias-pull-up, input-enable, input-disable, output-low, output-high,
> + input-schmitt-enable, input-schmitt-disable and drive-strength are valid.
> +
> + Some special pins have extra pull up strength, there are R0 and R1 pull-up
> + resistors available, but for user, it's only need to set R1R0 as 00, 01, 10 or 11.
> + So when config mediatek,pull-up-adv or mediatek,pull-down-adv,
> + it support arguments for those special pins.
> +
> + When config drive-strength, it can support some arguments, such as
> + MTK_DRIVE_4mA, MTK_DRIVE_6mA, etc. See dt-bindings/pinctrl/mt65xx.h.
> +
> +Examples:
> +
> +#include "mt8183-pinfunc.h"
> +
> +...
> +{
> + pio: pinctrl@10005000 {
> + compatible = "mediatek,mt8183-pinctrl";
> + reg = <0 0x10005000 0 0x1000>,
> + <0 0x11F20000 0 0x1000>,
> + <0 0x11E80000 0 0x1000>,
> + <0 0x11E70000 0 0x1000>,
> + <0 0x11E90000 0 0x1000>,
> + <0 0x11D30000 0 0x1000>,
> + <0 0x11D20000 0 0x1000>,
> + <0 0x11C50000 0 0x1000>,
> + <0 0x11F30000 0 0x1000>;
Use lowercase hex.
> + reg-names = "iocfg0", "iocfg1", "iocfg2",
> + "iocfg3", "iocfg4", "iocfg5",
> + "iocfg6", "iocfg7", "iocfg8";
> + gpio-controller;
> + #gpio-cells = <2>;
> + gpio-ranges = <&pio 0 0 192>;
> + interrupt-controller;
> + interrupts = <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-parent = <&gic>;
> + #interrupt-cells = <2>;
> +
> + i2c0_pins_a: i2c0 {
> + pins1 {
> + pinmux = <PINMUX_GPIO48__FUNC_SCL5>,
> + <PINMUX_GPIO49__FUNC_SDA5>;
> + mediatek,pull-up-adv = <11>;
> + };
> + };
> +
> + i2c1_pins_a: i2c1 {
> + pins {
> + pinmux = <PINMUX_GPIO50__FUNC_SCL3>,
> + <PINMUX_GPIO51__FUNC_SDA3>;
> + mediatek,pull-down-adv = <10>;
> + };
> + };
> + ...
> + };
> +};
> --
> 1.9.1
>
^ permalink raw reply
* Re: 答复: [PATCH][v4] tty: fix race between flush_to_ldisc and tty_open
From: Greg KH @ 2019-01-30 13:16 UTC (permalink / raw)
To: Li,Rongqing
Cc: jslaby@suse.com, linux-kernel@vger.kernel.org,
gkohli@codeaurora.org, linux-serial
In-Reply-To: <f13cb07ed77d428b8b43459ff71adcae@baidu.com>
On Wed, Jan 30, 2019 at 12:48:42PM +0000, Li,Rongqing wrote:
>
>
> > -----邮件原件-----
> > 发件人: linux-kernel-owner@vger.kernel.org
> > [mailto:linux-kernel-owner@vger.kernel.org] 代表 Greg KH
> > 发送时间: 2019年1月30日 18:19
> > 收件人: Li,Rongqing <lirongqing@baidu.com>
> > 抄送: jslaby@suse.com; linux-kernel@vger.kernel.org; gkohli@codeaurora.org
> > 主题: Re: [PATCH][v4] tty: fix race between flush_to_ldisc and tty_open
> >
> > On Fri, Jan 18, 2019 at 05:27:17PM +0800, Li RongQing wrote:
> > > There still is a race window after the commit b027e2298bd588
> > > ("tty: fix data race between tty_init_dev and flush of buf"), and we
> > > encountered this crash issue if receive_buf call comes before tty
> > > initialization completes in n_tty_open and
> > > tty->driver_data may be NULL.
> > >
> > > CPU0 CPU1
> > > ---- ----
> > > n_tty_open
> > > tty_init_dev
> > > tty_ldisc_unlock
> > > schedule flush_to_ldisc
> > > receive_buf
> > > tty_port_default_receive_buf
> > > tty_ldisc_receive_buf
> > > n_tty_receive_buf_common
> > > __receive_buf
> > > uart_flush_chars
> > > uart_start
> > > /*tty->driver_data is NULL*/
> > > tty->ops->open
> > > /*init tty->driver_data*/
> > >
> > > it can be fixed by extending ldisc semaphore lock in tty_init_dev to
> > > driver_data initialized completely after tty->ops->open(), but this
> > > will lead to put lock on one function and unlock in some other
> > > function, and hard to maintain, so fix this race only by checking
> > > tty->driver_data when receiving, and return if tty->driver_data
> > > is NULL
> > >
> > > Signed-off-by: Wang Li <wangli39@baidu.com>
> > > Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
> > > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > > ---
> > > V4: add version information
> > > V3: not used ldisc semaphore lock, only checking tty->driver_data with
> > > NULL
> > > V2: fix building error by EXPORT_SYMBOL tty_ldisc_unlock
> > > V1: extend ldisc lock to protect that tty->driver_data is inited
> > >
> > > drivers/tty/tty_port.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c index
> > > 044c3cbdcfa4..86d0bec38322 100644
> > > --- a/drivers/tty/tty_port.c
> > > +++ b/drivers/tty/tty_port.c
> > > @@ -31,6 +31,9 @@ static int tty_port_default_receive_buf(struct tty_port
> > *port,
> > > if (!tty)
> > > return 0;
> > >
> > > + if (!tty->driver_data)
> > > + return 0;
> > > +
> >
> > How is this working? What is setting driver_data to NULL to "stop" this race?
> >
>
>
> if tty->driver_data is NULL and return, tty_port_default_receive_buf will not step to
> uart_start which access tty->driver_data and trigger panic before tty_open, so it can
> fix the system panic
>
> > There's no requirement that a tty driver set this field to NULL when it is "done"
> > with the tty device, so I think you are just getting lucky in that your specific
> > driver happens to be doing this.
> >
>
> when tty_open is running, tty is allocated by kzalloc in tty_init_dev which called
> by tty_open_by_driver, tty is inited to 0
>
> > What driver are you testing this against?
> >
>
> 8250
Ok, as this is specific to the uart core, how about this patch instead:
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 5c01bb6d1c24..b56a6250df3f 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -130,6 +130,9 @@ static void uart_start(struct tty_struct *tty)
struct uart_port *port;
unsigned long flags;
+ if (!state)
+ return;
+
port = uart_port_lock(state, flags);
__uart_start(tty);
uart_port_unlock(port, flags);
^ permalink raw reply related
* Re: [PATCH] tty: Fix WARNING in tty_set_termios
From: Johan Hovold @ 2019-01-30 10:32 UTC (permalink / raw)
To: shuah
Cc: Al Viro, linux-wireless, chris, devel, robh, sameo, linux-serial,
jslaby, santhameena13, kirk, johan.hedberg, arnd, marcel,
samuel.thibault, m.maya.nakamura, zhongjiang, gregkh, speakup,
linux-kernel, linux-bluetooth, netdev, nishka.dasgupta_ug18,
davem
In-Reply-To: <fe4018a7-19e4-cdd1-3095-bbe88a82788e@kernel.org>
On Mon, Jan 28, 2019 at 02:29:22PM -0700, shuah wrote:
> On 1/25/19 9:14 PM, Al Viro wrote:
> > On Fri, Jan 25, 2019 at 04:29:05PM -0700, Shuah Khan wrote:
> >> tty_set_termios() has the following WARMN_ON which can be triggered with a
> >> syscall to invoke TIOCGETD __NR_ioctl.
You meant TIOCSETD here, and in fact its the call which sets the uart
protocol that triggers the warning.
> >> WARN_ON(tty->driver->type == TTY_DRIVER_TYPE_PTY &&
> >> tty->driver->subtype == PTY_TYPE_MASTER);
> >> Reference: https://syzkaller.appspot.com/bug?id=2410d22f1d8e5984217329dd0884b01d99e3e48d
> >>
> >> A simple change would have been to print error message instead of WARN_ON.
> >> However, the callers assume that tty_set_termios() always returns 0 and
> >> don't check return value. The complete solution is fixing all the callers
> >> to check error and bail out to fix the WARN_ON.
> >>
> >> This fix changes tty_set_termios() to return error and all the callers
> >> to check error and bail out. The reproducer is used to reproduce the
> >> problem and verify the fix.
> >
> >> --- a/drivers/bluetooth/hci_ldisc.c
> >> +++ b/drivers/bluetooth/hci_ldisc.c
> >> @@ -321,6 +321,8 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
> >> status = tty_set_termios(tty, &ktermios);
> >> BT_DBG("Disabling hardware flow control: %s",
> >> status ? "failed" : "success");
> >> + if (status)
> >> + return;
> >
> > Can that ldisc end up set on pty master? And does it make any sense there?
>
> The initial objective of the patch is to prevent the WARN_ON by making
> the change to return error instead of WARN_ON. However, without changes
> to places that don't check the return and keep making progress, there
> will be secondary problems.
>
> Without this change to return here, instead of WARN_ON, it will fail
> with the following NULL pointer dereference at the next thing
> hci_uart_set_flow_control() attempts.
>
> status = tty->driver->ops->tiocmget(tty);
>
> kernel: [10140.649783] BUG: unable to handle kernel NULL pointer
That's a separate issue, which is being fixed:
https://lkml.kernel.org/r/20190130095938.GP3691@localhost
> > IOW, I don't believe that this patch makes any sense. If anything,
> > we need to prevent unconditional tty_set_termios() on the path
> > that *does* lead to calling it for pty.
>
> I don't think preventing unconditional tty_set_termios() is enough to
> prevent secondary problems such as the one above.
>
> For example, the following call chain leads to the WARN_ON that was
> reported. Even if void hci_uart_set_baudrate() prevents the very first
> tty_set_termios() call, its caller hci_uart_setup() continues with
> more tty setup. It goes ahead to call driver setup callback. The
> driver callback goes on to do more setup calling tty_set_termios().
>
> WARN_ON call path:
> hci_uart_set_baudrate+0x1cc/0x250 drivers/bluetooth/hci_ldisc.c:378
> hci_uart_setup+0xa2/0x490 drivers/bluetooth/hci_ldisc.c:401
> hci_dev_do_open+0x6b1/0x1920 net/bluetooth/hci_core.c:1423
>
> Once this WARN_ON is changed to return error, the following
> happens, when hci_uart_setup() does driver setup callback.
>
> kernel: [10140.649836] mrvl_setup+0x17/0x80 [hci_uart]
> kernel: [10140.649840] hci_uart_setup+0x56/0x160 [hci_uart]
> kernel: [10140.649850] hci_dev_do_open+0xe6/0x630 [bluetooth]
> kernel: [10140.649860] hci_power_on+0x52/0x220 [bluetooth]
>
> I think continuing to catch the invalid condition in tty_set_termios()
> and preventing progress by checking return value is a straight forward
> change to avoid secondary problems, and it might be difficult to catch
> all the cases where it could fail.
I agree with Al that this change doesn't make much sense. The WARN_ON
is there to catch any bugs leading to the termios being changed for a
master side pty. Those should bugs should be fixed, and not worked
around in order to silence a WARN_ON.
The problem started with 7721383f4199 ("Bluetooth: hci_uart: Support
operational speed during setup") which introduced a new way for how
tty_set_termios() could end up being called for a master pty.
As Al hinted at, setting these ldiscs for a master pty really makes no
sense and perhaps that is what we should prevent unless simply making
sure they do not call tty_set_termios() is sufficient for the time
being.
Finally, note that serdev never operates on a pty, and that this is only
an issue for (the three) line disciplines.
Johan
^ permalink raw reply
* [PATCH v2 2/2] serial: mps2-uart: support combined irq
From: Vladimir Murzin @ 2019-01-30 9:55 UTC (permalink / raw)
To: linux-serial; +Cc: greg, linux-arm-kernel, sudeep.holla
In-Reply-To: <1548842115-14782-1-git-send-email-vladimir.murzin@arm.com>
It turns out that some designs went for implementing only combined
interrupt for rx, tx and overrun, which is currently not supported
by the driver. Support of combined irq is built on top of existent
irq handlers and activated automatically if only single irq was
specified in device tree.
Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
---
Changelog
v1 -> v2
- overrun interrupt also combined with rx and tx
- count number of irqs provided via dt to detect
use of combined irq.
drivers/tty/serial/mps2-uart.c | 91 ++++++++++++++++++++++++++++++------------
1 file changed, 65 insertions(+), 26 deletions(-)
diff --git a/drivers/tty/serial/mps2-uart.c b/drivers/tty/serial/mps2-uart.c
index 6da0633..4404eb7 100644
--- a/drivers/tty/serial/mps2-uart.c
+++ b/drivers/tty/serial/mps2-uart.c
@@ -66,11 +66,14 @@
#define MPS2_MAX_PORTS 3
+#define UART_PORT_COMBINED_IRQ BIT(0)
+
struct mps2_uart_port {
struct uart_port port;
struct clk *clk;
unsigned int tx_irq;
unsigned int rx_irq;
+ unsigned int flags;
};
static inline struct mps2_uart_port *to_mps2_port(struct uart_port *port)
@@ -265,6 +268,20 @@ static irqreturn_t mps2_uart_oerrirq(int irq, void *data)
return handled;
}
+static irqreturn_t mps2_uart_combinedirq(int irq, void *data)
+{
+ if (mps2_uart_rxirq(irq, data) == IRQ_HANDLED)
+ return IRQ_HANDLED;
+
+ if (mps2_uart_txirq(irq, data) == IRQ_HANDLED)
+ return IRQ_HANDLED;
+
+ if (mps2_uart_oerrirq(irq, data) == IRQ_HANDLED)
+ return IRQ_HANDLED;
+
+ return IRQ_NONE;
+}
+
static int mps2_uart_startup(struct uart_port *port)
{
struct mps2_uart_port *mps_port = to_mps2_port(port);
@@ -275,26 +292,37 @@ static int mps2_uart_startup(struct uart_port *port)
mps2_uart_write8(port, control, UARTn_CTRL);
- ret = request_irq(mps_port->rx_irq, mps2_uart_rxirq, 0,
- MAKE_NAME(-rx), mps_port);
- if (ret) {
- dev_err(port->dev, "failed to register rxirq (%d)\n", ret);
- return ret;
- }
+ if (mps_port->flags & UART_PORT_COMBINED_IRQ) {
+ ret = request_irq(port->irq, mps2_uart_combinedirq, 0,
+ MAKE_NAME(-combined), mps_port);
- ret = request_irq(mps_port->tx_irq, mps2_uart_txirq, 0,
- MAKE_NAME(-tx), mps_port);
- if (ret) {
- dev_err(port->dev, "failed to register txirq (%d)\n", ret);
- goto err_free_rxirq;
- }
+ if (ret) {
+ dev_err(port->dev, "failed to register combinedirq (%d)\n", ret);
+ return ret;
+ }
+ } else {
+ ret = request_irq(port->irq, mps2_uart_oerrirq, IRQF_SHARED,
+ MAKE_NAME(-overrun), mps_port);
+
+ if (ret) {
+ dev_err(port->dev, "failed to register oerrirq (%d)\n", ret);
+ return ret;
+ }
- ret = request_irq(port->irq, mps2_uart_oerrirq, IRQF_SHARED,
- MAKE_NAME(-overrun), mps_port);
+ ret = request_irq(mps_port->rx_irq, mps2_uart_rxirq, 0,
+ MAKE_NAME(-rx), mps_port);
+ if (ret) {
+ dev_err(port->dev, "failed to register rxirq (%d)\n", ret);
+ goto err_free_oerrirq;
+ }
+
+ ret = request_irq(mps_port->tx_irq, mps2_uart_txirq, 0,
+ MAKE_NAME(-tx), mps_port);
+ if (ret) {
+ dev_err(port->dev, "failed to register txirq (%d)\n", ret);
+ goto err_free_rxirq;
+ }
- if (ret) {
- dev_err(port->dev, "failed to register oerrirq (%d)\n", ret);
- goto err_free_txirq;
}
control |= UARTn_CTRL_RX_GRP | UARTn_CTRL_TX_GRP;
@@ -303,10 +331,10 @@ static int mps2_uart_startup(struct uart_port *port)
return 0;
-err_free_txirq:
- free_irq(mps_port->tx_irq, mps_port);
err_free_rxirq:
free_irq(mps_port->rx_irq, mps_port);
+err_free_oerrirq:
+ free_irq(port->irq, mps_port);
return ret;
}
@@ -320,8 +348,11 @@ static void mps2_uart_shutdown(struct uart_port *port)
mps2_uart_write8(port, control, UARTn_CTRL);
- free_irq(mps_port->rx_irq, mps_port);
- free_irq(mps_port->tx_irq, mps_port);
+ if (!mps_port->flags & UART_PORT_COMBINED_IRQ) {
+ free_irq(mps_port->rx_irq, mps_port);
+ free_irq(mps_port->tx_irq, mps_port);
+ }
+
free_irq(port->irq, mps_port);
}
@@ -511,6 +542,10 @@ static int mps2_of_get_port(struct platform_device *pdev,
if (id < 0)
return id;
+ /* Only combined irq is presesnt */
+ if (platform_irq_count(pdev) == 1)
+ mps_port->flags |= UART_PORT_COMBINED_IRQ;
+
mps_port->port.line = id;
return 0;
@@ -529,11 +564,6 @@ static int mps2_init_port(struct platform_device *pdev,
mps_port->port.mapbase = res->start;
mps_port->port.mapsize = resource_size(res);
-
- mps_port->rx_irq = platform_get_irq(pdev, 0);
- mps_port->tx_irq = platform_get_irq(pdev, 1);
- mps_port->port.irq = platform_get_irq(pdev, 2);
-
mps_port->port.iotype = UPIO_MEM;
mps_port->port.flags = UPF_BOOT_AUTOCONF;
mps_port->port.fifosize = 1;
@@ -552,6 +582,15 @@ static int mps2_init_port(struct platform_device *pdev,
clk_disable_unprepare(mps_port->clk);
+
+ if (mps_port->flags & UART_PORT_COMBINED_IRQ) {
+ mps_port->port.irq = platform_get_irq(pdev, 0);
+ } else {
+ mps_port->rx_irq = platform_get_irq(pdev, 0);
+ mps_port->tx_irq = platform_get_irq(pdev, 1);
+ mps_port->port.irq = platform_get_irq(pdev, 2);
+ }
+
return ret;
}
--
2.7.4
^ permalink raw reply related
* [PATCH v2 1/2] serial: mps2-uart: move to dynamic port allocation
From: Vladimir Murzin @ 2019-01-30 9:55 UTC (permalink / raw)
To: linux-serial; +Cc: greg, linux-arm-kernel, sudeep.holla
In-Reply-To: <1548842115-14782-1-git-send-email-vladimir.murzin@arm.com>
Some designs, like MPS3, expose number of virtual serial ports which
already close or exceeds MPS2_MAX_PORTS. Increasing MPS2_MAX_PORTS
would have negative impact (in terms of memory consumption) on tiny
MPS2 platform which, in fact, has only one physically populated UART.
Start with converting existent static port array to idr. As a bonus it
make driver not to fail in case when no alias was specified in device
tree.
Note: there is no need in idr_destroy() because code doesn't unload
since ce87122911f8 ("serial: mps2-uart: make driver explicitly non-modular")
Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
---
Changelog
v1 -> v2
- added a note about idr_destroy()
drivers/tty/serial/mps2-uart.c | 47 ++++++++++++++++++++++++++++--------------
1 file changed, 31 insertions(+), 16 deletions(-)
diff --git a/drivers/tty/serial/mps2-uart.c b/drivers/tty/serial/mps2-uart.c
index 9f8f637..6da0633 100644
--- a/drivers/tty/serial/mps2-uart.c
+++ b/drivers/tty/serial/mps2-uart.c
@@ -22,6 +22,7 @@
#include <linux/serial_core.h>
#include <linux/tty_flip.h>
#include <linux/types.h>
+#include <linux/idr.h>
#define SERIAL_NAME "ttyMPS"
#define DRIVER_NAME "mps2-uart"
@@ -397,7 +398,7 @@ static const struct uart_ops mps2_uart_pops = {
.verify_port = mps2_uart_verify_port,
};
-static struct mps2_uart_port mps2_uart_ports[MPS2_MAX_PORTS];
+static DEFINE_IDR(ports_idr);
#ifdef CONFIG_SERIAL_MPS2_UART_CONSOLE
static void mps2_uart_console_putchar(struct uart_port *port, int ch)
@@ -410,7 +411,8 @@ static void mps2_uart_console_putchar(struct uart_port *port, int ch)
static void mps2_uart_console_write(struct console *co, const char *s, unsigned int cnt)
{
- struct uart_port *port = &mps2_uart_ports[co->index].port;
+ struct mps2_uart_port *mps_port = idr_find(&ports_idr, co->index);
+ struct uart_port *port = &mps_port->port;
uart_console_write(port, s, cnt, mps2_uart_console_putchar);
}
@@ -426,7 +428,10 @@ static int mps2_uart_console_setup(struct console *co, char *options)
if (co->index < 0 || co->index >= MPS2_MAX_PORTS)
return -ENODEV;
- mps_port = &mps2_uart_ports[co->index];
+ mps_port = idr_find(&ports_idr, co->index);
+
+ if (!mps_port)
+ return -ENODEV;
if (options)
uart_parse_options(options, &baud, &parity, &bits, &flow);
@@ -487,27 +492,32 @@ static struct uart_driver mps2_uart_driver = {
.cons = MPS2_SERIAL_CONSOLE,
};
-static struct mps2_uart_port *mps2_of_get_port(struct platform_device *pdev)
+static int mps2_of_get_port(struct platform_device *pdev,
+ struct mps2_uart_port *mps_port)
{
struct device_node *np = pdev->dev.of_node;
int id;
if (!np)
- return NULL;
+ return -ENODEV;
id = of_alias_get_id(np, "serial");
+
+ if (id < 0)
+ id = idr_alloc_cyclic(&ports_idr, (void *)mps_port, 0, MPS2_MAX_PORTS, GFP_KERNEL);
+ else
+ id = idr_alloc(&ports_idr, (void *)mps_port, id, MPS2_MAX_PORTS, GFP_KERNEL);
+
if (id < 0)
- id = 0;
+ return id;
- if (WARN_ON(id >= MPS2_MAX_PORTS))
- return NULL;
+ mps_port->port.line = id;
- mps2_uart_ports[id].port.line = id;
- return &mps2_uart_ports[id];
+ return 0;
}
-static int mps2_init_port(struct mps2_uart_port *mps_port,
- struct platform_device *pdev)
+static int mps2_init_port(struct platform_device *pdev,
+ struct mps2_uart_port *mps_port)
{
struct resource *res;
int ret;
@@ -550,11 +560,16 @@ static int mps2_serial_probe(struct platform_device *pdev)
struct mps2_uart_port *mps_port;
int ret;
- mps_port = mps2_of_get_port(pdev);
- if (!mps_port)
- return -ENODEV;
+ mps_port = devm_kzalloc(&pdev->dev, sizeof(struct mps2_uart_port), GFP_KERNEL);
+
+ if (!mps_port)
+ return -ENOMEM;
+
+ ret = mps2_of_get_port(pdev, mps_port);
+ if (ret)
+ return ret;
- ret = mps2_init_port(mps_port, pdev);
+ ret = mps2_init_port(pdev, mps_port);
if (ret)
return ret;
--
2.7.4
^ permalink raw reply related
* [PATCH v2 0/2] serial: mps2-uart: minor improvements
From: Vladimir Murzin @ 2019-01-30 9:55 UTC (permalink / raw)
To: linux-serial; +Cc: greg, linux-arm-kernel, sudeep.holla
This mini series is supposed to improve MPS2 uart driver for:
1. platforms with number of virtual serial ports, like MPS3
2. platforms with combined irq
Vladimir Murzin (2):
serial: mps2-uart: move to dynamic port allocation
serial: mps2-uart: support combined irq
drivers/tty/serial/mps2-uart.c | 138 ++++++++++++++++++++++++++++-------------
1 file changed, 96 insertions(+), 42 deletions(-)
--
2.7.4
^ permalink raw reply
* [PATCH] serdev: ttyport: call tiocmget and tiocmset ops directly
From: Johan Hovold @ 2019-01-30 9:52 UTC (permalink / raw)
To: Rob Herring
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, linux-kernel,
Johan Hovold
The tty struct holds a pointer to the driver's tty operations so drop
the unnecessary driver dereference when calling tiocmget and tiocmset.
Note that this also makes the calls match the preceding sanity checks as
expected.
Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/tty/serdev/serdev-ttyport.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index fa1672993b4c..d1cdd2ab8b4c 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -233,7 +233,7 @@ static int ttyport_get_tiocm(struct serdev_controller *ctrl)
if (!tty->ops->tiocmget)
return -ENOTSUPP;
- return tty->driver->ops->tiocmget(tty);
+ return tty->ops->tiocmget(tty);
}
static int ttyport_set_tiocm(struct serdev_controller *ctrl, unsigned int set, unsigned int clear)
@@ -244,7 +244,7 @@ static int ttyport_set_tiocm(struct serdev_controller *ctrl, unsigned int set, u
if (!tty->ops->tiocmset)
return -ENOTSUPP;
- return tty->driver->ops->tiocmset(tty, set, clear);
+ return tty->ops->tiocmset(tty, set, clear);
}
static const struct serdev_controller_ops ctrl_ops = {
--
2.20.1
^ permalink raw reply related
* Re: [PATCH 1/2] serial: mps2-uart: move to dynamic port allocation
From: Vladimir Murzin @ 2019-01-30 9:33 UTC (permalink / raw)
To: Greg KH; +Cc: linux-serial, linux-arm-kernel, sudeep.holla
In-Reply-To: <20190130092405.GA27385@kroah.com>
On 1/30/19 9:24 AM, Greg KH wrote:
> On Wed, Jan 30, 2019 at 08:58:53AM +0000, Vladimir Murzin wrote:
>> On 1/30/19 8:27 AM, Greg KH wrote:
>>> On Fri, Jan 25, 2019 at 02:13:16PM +0000, Vladimir Murzin wrote:
>>>> Some designs, like MPS3, expose number of virtual serial ports which
>>>> already close or exceeds MPS2_MAX_PORTS. Increasing MPS2_MAX_PORTS
>>>> would have negative impact (in terms of memory consumption) on tiny
>>>> MPS2 platform which, in fact, has only one physically populated UART.
>>>>
>>>> Start with converting existent static port array to idr. As a bonus it
>>>> make driver not to fail in case when no alias was specified in device
>>>> tree.
>>>>
>>>> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
>>>> ---
>>>> drivers/tty/serial/mps2-uart.c | 47 ++++++++++++++++++++++++++++--------------
>>>> 1 file changed, 31 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/drivers/tty/serial/mps2-uart.c b/drivers/tty/serial/mps2-uart.c
>>>> index 9f8f637..6da0633 100644
>>>> --- a/drivers/tty/serial/mps2-uart.c
>>>> +++ b/drivers/tty/serial/mps2-uart.c
>>>> @@ -22,6 +22,7 @@
>>>> #include <linux/serial_core.h>
>>>> #include <linux/tty_flip.h>
>>>> #include <linux/types.h>
>>>> +#include <linux/idr.h>
>>>>
>>>> #define SERIAL_NAME "ttyMPS"
>>>> #define DRIVER_NAME "mps2-uart"
>>>> @@ -397,7 +398,7 @@ static const struct uart_ops mps2_uart_pops = {
>>>> .verify_port = mps2_uart_verify_port,
>>>> };
>>>>
>>>> -static struct mps2_uart_port mps2_uart_ports[MPS2_MAX_PORTS];
>>>> +static DEFINE_IDR(ports_idr);
>>>
>>> You forgot to call idr_destroy() when your code unloads :(
>>
>> Hmm, but code doesn't unload since ce87122911f8 ("serial: mps2-uart: make driver explicitly non-modular")
>> or I'm missing something?
>
> Ugh, ok, nevermind, I missed that, sorry.
No problem, I'll add a note about that in commit message ;)
Thanks
Vladimir
>
> greg k-h
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply
* 答复: [PATCH][v4] tty: fix race between flush_to_ldisc and tty_open
From: Li,Rongqing @ 2019-01-30 9:29 UTC (permalink / raw)
To: Kohli, Gaurav, jslaby@suse.com, linux-kernel@vger.kernel.org
Cc: Greg Kroah-Hartman, linux-serial@vger.kernel.org
In-Reply-To: <ae44b1c5-4d33-110b-a4da-f526d156be2a@codeaurora.org>
> -----邮件原件-----
> 发件人: Kohli, Gaurav [mailto:gkohli@codeaurora.org]
> 发送时间: 2019年1月18日 20:51
> 收件人: Li,Rongqing <lirongqing@baidu.com>; gregkh@linuxfoundation.org;
> jslaby@suse.com; linux-kernel@vger.kernel.org
> 主题: Re: [PATCH][v4] tty: fix race between flush_to_ldisc and tty_open
>
>
>
> On 1/18/2019 2:57 PM, Li RongQing wrote:
> > There still is a race window after the commit b027e2298bd588
> > ("tty: fix data race between tty_init_dev and flush of buf"), and we
> > encountered this crash issue if receive_buf call comes before tty
> > initialization completes in n_tty_open and
> > tty->driver_data may be NULL.
> >
> > CPU0 CPU1
> > ---- ----
> > n_tty_open
> > tty_init_dev
> > tty_ldisc_unlock
> > schedule flush_to_ldisc
> > receive_buf
> > tty_port_default_receive_buf
> > tty_ldisc_receive_buf
> > n_tty_receive_buf_common
> > __receive_buf
> > uart_flush_chars
> > uart_start
> > /*tty->driver_data is NULL*/
> > tty->ops->open
> > /*init tty->driver_data*/
> >
> > it can be fixed by extending ldisc semaphore lock in tty_init_dev to
> > driver_data initialized completely after tty->ops->open(), but this
> > will lead to put lock on one function and unlock in some other
> > function, and hard to maintain, so fix this race only by checking
> > tty->driver_data when receiving, and return if tty->driver_data
> > is NULL
> >
> > Signed-off-by: Wang Li <wangli39@baidu.com>
> > Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > ---
> > V4: add version information
> > V3: not used ldisc semaphore lock, only checking tty->driver_data with
> > NULL
> > V2: fix building error by EXPORT_SYMBOL tty_ldisc_unlock
> > V1: extend ldisc lock to protect that tty->driver_data is inited
> >
> > drivers/tty/tty_port.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c index
> > 044c3cbdcfa4..86d0bec38322 100644
> > --- a/drivers/tty/tty_port.c
> > +++ b/drivers/tty/tty_port.c
> > @@ -31,6 +31,9 @@ static int tty_port_default_receive_buf(struct tty_port
> *port,
> > if (!tty)
> > return 0;
> >
> > + if (!tty->driver_data)
> > + return 0;
> > +
> > disc = tty_ldisc_ref(tty);
> > if (!disc)
> > return 0;
> >
> Acked-by: Gaurav Kohli <gkohli@codeaurora.org>
>
> It looks good to me w.r.t previous approach, but Let's Maintainer decide once.
>
Thanks for your review, this one is simple and safe, it is used as live-patch online
-RongQing
> Regards
> Gaurav
> --
> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
> is a member of the Code Aurora Forum, a Linux Foundation Collaborative
> Project.
^ permalink raw reply
* Re: [PATCH 1/2] serial: mps2-uart: move to dynamic port allocation
From: Greg KH @ 2019-01-30 9:24 UTC (permalink / raw)
To: Vladimir Murzin; +Cc: linux-arm-kernel, linux-serial, sudeep.holla
In-Reply-To: <0bea8e14-9ade-aed4-c41e-f02ab0b81d24@arm.com>
On Wed, Jan 30, 2019 at 08:58:53AM +0000, Vladimir Murzin wrote:
> On 1/30/19 8:27 AM, Greg KH wrote:
> > On Fri, Jan 25, 2019 at 02:13:16PM +0000, Vladimir Murzin wrote:
> >> Some designs, like MPS3, expose number of virtual serial ports which
> >> already close or exceeds MPS2_MAX_PORTS. Increasing MPS2_MAX_PORTS
> >> would have negative impact (in terms of memory consumption) on tiny
> >> MPS2 platform which, in fact, has only one physically populated UART.
> >>
> >> Start with converting existent static port array to idr. As a bonus it
> >> make driver not to fail in case when no alias was specified in device
> >> tree.
> >>
> >> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
> >> ---
> >> drivers/tty/serial/mps2-uart.c | 47 ++++++++++++++++++++++++++++--------------
> >> 1 file changed, 31 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/tty/serial/mps2-uart.c b/drivers/tty/serial/mps2-uart.c
> >> index 9f8f637..6da0633 100644
> >> --- a/drivers/tty/serial/mps2-uart.c
> >> +++ b/drivers/tty/serial/mps2-uart.c
> >> @@ -22,6 +22,7 @@
> >> #include <linux/serial_core.h>
> >> #include <linux/tty_flip.h>
> >> #include <linux/types.h>
> >> +#include <linux/idr.h>
> >>
> >> #define SERIAL_NAME "ttyMPS"
> >> #define DRIVER_NAME "mps2-uart"
> >> @@ -397,7 +398,7 @@ static const struct uart_ops mps2_uart_pops = {
> >> .verify_port = mps2_uart_verify_port,
> >> };
> >>
> >> -static struct mps2_uart_port mps2_uart_ports[MPS2_MAX_PORTS];
> >> +static DEFINE_IDR(ports_idr);
> >
> > You forgot to call idr_destroy() when your code unloads :(
>
> Hmm, but code doesn't unload since ce87122911f8 ("serial: mps2-uart: make driver explicitly non-modular")
> or I'm missing something?
Ugh, ok, nevermind, I missed that, sorry.
greg k-h
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox