lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 25 Aug 2020 17:11:16 +0300 From: Abel Vesa <abel.vesa@....com> To: Philipp Zabel <p.zabel@...gutronix.de> Cc: Dong Aisheng <aisheng.dong@....com>, Rob Herring <robh@...nel.org>, Peng Fan <peng.fan@....com>, Fugang Duan <fugang.duan@....com>, Jacky Bai <ping.bai@....com>, Anson Huang <anson.huang@....com>, devicetree@...r.kernel.org, Stephen Boyd <sboyd@...nel.org>, Mike Turquette <mturquette@...libre.com>, Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, NXP Linux Team <linux-imx@....com>, Sascha Hauer <kernel@...gutronix.de>, Fabio Estevam <fabio.estevam@....com>, Shawn Guo <shawnguo@...nel.org>, linux-clk@...r.kernel.org, linux-arm-kernel@...ts.infradead.org Subject: Re: [PATCH v2 11/17] clk: imx: Add blk_ctrl combo driver On 20-08-25 14:07:29, Philipp Zabel wrote: > On Tue, 2020-08-25 at 14:24 +0300, Abel Vesa wrote: > [...] > > > > +static int imx_blk_ctrl_reset_set(struct reset_controller_dev *rcdev, > > > > + unsigned long id, bool assert) > > > > +{ > > > > + struct imx_blk_ctrl_drvdata *drvdata = container_of(rcdev, > > > > + struct imx_blk_ctrl_drvdata, rcdev); > > > > + unsigned int offset = drvdata->rst_hws[id].offset; > > > > + unsigned int shift = drvdata->rst_hws[id].shift; > > > > + unsigned int mask = drvdata->rst_hws[id].mask; > > > > + void __iomem *reg_addr = drvdata->base + offset; > > > > + unsigned long flags; > > > > + unsigned int asserted_before = 0, asserted_after = 0; > > > > + u32 reg; > > > > + int i; > > > > + > > > > + spin_lock_irqsave(&drvdata->lock, flags); > > > > + > > > > + for (i = 0; i < drvdata->rcdev.nr_resets; i++) > > > > + if (drvdata->rst_hws[i].asserted) > > > > + asserted_before++; > > > > + > > > > + if (asserted_before == 0 && assert) > > > > + pm_runtime_get(rcdev->dev); > > > > > > Shouldn't that be pm_runtime_get_sync() ? > > > > > > I would do that unconditionally before locking drvdata->lock and then > > > drop unnecessary refcounts afterwards. > > > > > > > I thought we already discussed this on the last's version thread. > > This is about something different. pm_runtime_get() just queues the > device to be enabled at a later point, but I presume you want to have it > enabled before writing to its registers. (The question here is can you > write to the registers, and have the device update its internal state, > while the power domain is disabled?) > Either way, if you want the reset to be asserted after the function > returns (as is required by the reset API), as I understand it, you have > to make sure that the power domain is activated before the function > returns. > Therefore pm_runtime_get_sync() is required instead of pm_runtime_get(), > and that must be called outside of the spin locked section. My > suggestion would be: > > if (assert) > pm_runtime_get_sync(); > spin_lock_irqsave(); > /* ... */ > spin_unlock_irqrestore(); > if (assert && asserted_before) > pm_runtime_put(); > You're right this makes more sense. > unless the following might be an issue: > > > > > + > > > > + if (assert) { > > > > + reg = readl(reg_addr); > > > > + writel(reg & ~(mask << shift), reg_addr); > > > > + drvdata->rst_hws[id].asserted = true; > > > > + } else { > > > > + reg = readl(reg_addr); > > > > + writel(reg | (mask << shift), reg_addr); > > Could this cause problems if the power domain is already disabled? If > so, it would be best to either temporarily enable power, or to skip the > register writes if asserted_before == 0 && !assert. I'll go with the latter one since it leaves the PD off. > > > > > + drvdata->rst_hws[id].asserted = false; > > > > + } > > > > + > > > > + for (i = 0; i < drvdata->rcdev.nr_resets; i++) > > > > + if (drvdata->rst_hws[i].asserted) > > > > + asserted_after++; > > > > + > > > > + if (asserted_before == 1 && asserted_after == 0) > > > > + pm_runtime_put(rcdev->dev); > > > > + > > > > + spin_unlock_irqrestore(&drvdata->lock, flags); > > > > + > > > > + return 0; > > > > +} > > regards > Philipp
Powered by blists - more mailing lists