linux-kernel - Re: [PATCH 3/3] clk: keystone: Add sci-clk driver support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160831223101.GI12510@codeaurora.org>
Date:   Wed, 31 Aug 2016 15:31:01 -0700
From:   Stephen Boyd <sboyd@...eaurora.org>
To:     Tero Kristo <t-kristo@...com>
Cc:     Nishanth Menon <nm@...com>,
        Michael Turquette <mturquette@...libre.com>,
        Dave Gerlach <d-gerlach@...com>,
        Russell King <rmk+kernel@...linux.org.uk>,
        Sudeep Holla <sudeep.holla@....com>,
        Santosh Shilimkar <ssantosh@...nel.org>,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-pm@...r.kernel.org,
        linux-clk@...r.kernel.org
Subject: Re: [PATCH 3/3] clk: keystone: Add sci-clk driver support

On 08/31, Tero Kristo wrote:
> On 24/08/16 11:34, Stephen Boyd wrote:
> >On 08/19, Nishanth Menon wrote:
> >>diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
> >>new file mode 100644
> >>index 000000000000..6c43e097e6d6
> >>--- /dev/null
> >>+++ b/drivers/clk/keystone/sci-clk.c
> >>@@ -0,0 +1,539 @@
> >>+	return (int)new_rate;
> >
> >determine rate should return a negative number on failure and 0
> >on success. The actual rate that was found should go into
> >req->rate. This looks broken.
> 
> Yea it seems broken, I wonder how we haven't seen any issues with
> this in testing.... Apparently positive return values from this are
> interpreted as success. Having a quick look at clk.c seems to
> confirm this.
> 
> Anyway, will fix.

True, perhaps we should fix that so we don't use a long to hold
the int return value either.

> >>+ *
> >>+ * Gets a handle to an existing TI SCI clock, or builds a new clock
> >>+ * entry and registers it with the common clock framework. Called from
> >>+ * the common clock framework, when a corresponding of_clk_get call is
> >>+ * executed, or recursively from itself when parsing parent clocks.
> >>+ * Returns a pointer to the clock struct, or ERR_PTR value in failure.
> >>+ */
> >
> >Please move this driver to clk_hw_register() and friends. This on
> >the fly clk generation is scary considering how we hold locks
> >while the provider is asked to give us the pointer, so allocating
> >and registering clks (basically reentering the CCF again) could
> >lead to a locking nightmare. Best to avoid that.
> 
> Ok, so just converting the driver to use provider->get_hw should be
> enough? This seems to be a relatively new API in the CCF. Will look
> at that.

Hopefully it will simplify things greatly.

> 
> >>+	}
> >>+
> >>+	snprintf(name, 20, "%s:%d:%d", dev_name(provider->dev), sci_clk->dev_id,
> >>+		 sci_clk->clk_id);
> >
> >I hope we don't make dev_name() longer than 20 characters
> 
> Shall I just increase the size of the buffer and add a length check?
> Using kmalloc or something here seems overkill, as the name gets
> copied by CCF anyway.

There's kasprintf() which would always make it long enough. I
don't know if it really matters though.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project