linux-kernel - Re: [PATCH v4 2/2] Input: bcm-keypad: Add Broadcom keypad controller

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]

Date:	Mon, 2 Mar 2015 12:23:42 -0800
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	Scott Branden <sbranden@...adcom.com>
Cc:	bcm-kernel-feedback-list@...adcom.com,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Grant Likely <grant.likely@...aro.org>,
	Ray Jui <rjui@...adcom.com>,
	Jonathan Richardson <jonathar@...adcom.com>,
	Dmitry Torokhov <dtor@...gle.com>,
	Anatol Pomazao <anatol@...gle.com>, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/2] Input: bcm-keypad: Add Broadcom keypad controller

Hi Scott,

On Mon, Mar 02, 2015 at 11:37:57AM -0800, Scott Branden wrote:
> Hi Dmitry,
> 
> I don't see the need for a check of the clock in bcm_kp_start or
> stop. See comment below.
> 
> On 15-02-28 02:15 PM, Dmitry Torokhov wrote:
> >On Sat, Feb 28, 2015 at 02:10:22PM -0800, Dmitry Torokhov wrote:
> >>Hi Scott,
> >>
> >>On Sat, Feb 28, 2015 at 08:35:57AM -0800, Scott Branden wrote:
> >>>+	/* Enable clock */
> >>>+
> >>>+	kp->clk = devm_clk_get(&pdev->dev, "peri_clk");
> >>>+	if (IS_ERR(kp->clk)) {
> >>>+		dev_info(&pdev->dev,
> >>>+			"No clock specified. Assuming it's enabled\n");
> >>
> >>I was doing the final pass before applying and I think that this is not
> >>quite correct, as it does not handle -EPROBE_DEFER properly which is
> >>quite common. I think we should do something like this:
> >>
> >>		error = PTR_ERR(kp->clk);
> >>		if (error != -ENOENT) {
> >>			if (error != -EPROBE_DEFER)
> >>				dev_err(.. "Failed to get clock" ...);
> >>			return error;
> >>		}
> >>		dev_dbg(... "No clock specified" ...);
> >>
> >>No need to resubmit if you are OK with this, I can make the change
> >>locally.
> >
> >Hmm, also bcm_kp_start() and bcm_kp_stop() should check if kp->clk is
> >valid before trying to enable/disable it.
> >
> I checked and other keyboard drivers do not check this.  I return an
> error in bcm_kp_start if the clk enable fails.  On stop, if the clk
> is not valid something is really, really wrong as well.

The other drivers simply abort probe() if they can't get clock, you
decided to allow probe() to finish and assume that clock is already
enabled, leaving kp->clk == ERR_PTR(-ENOENT) in your version. If you try
then calling clk_prepare_enable() with that pointer it is going to bomb,
that is why I said you need to check pointer validity in bcm_kp_start()
and bcm_kp_stop().

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/