linux-kernel - Re: [PATCH 07/18] dmaengine/amba-pl08x: Enable/Disable amba

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: <CACRpkdZTsD=7woN1qV-sFpsJTyT0LQBq+q=xTOwuewAk=ijBjQ@mail.gmail.com>
Date:	Tue, 2 Aug 2011 11:05:38 +0200
From:	Linus Walleij <linus.walleij@...aro.org>
To:	Russell King - ARM Linux <linux@....linux.org.uk>
Cc:	Viresh Kumar <viresh.kumar@...com>, pratyush.anand@...com,
	rajeev-dlh.kumar@...com, armando.visconti@...com,
	bhupesh.sharma@...com, vinod.koul@...el.com,
	linux-kernel@...r.kernel.org, vipin.kumar@...com,
	shiraz.hashim@...com, amit.virdi@...com, vipulkumar.samar@...com,
	viresh.linux@...il.com, deepak.sikri@...com,
	dan.j.williams@...el.com, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 07/18] dmaengine/amba-pl08x: Enable/Disable amba_pclk with
 channel requests

On Sun, Jul 31, 2011 at 7:04 PM, Russell King - ARM Linux
<linux@....linux.org.uk> wrote:
> On Sun, Jul 31, 2011 at 02:04:47AM +0200, Linus Walleij wrote:
>>
>> ..and while it will just cause some double refcounts on the clock,
>> it makes sense to delete the pclk manipulation from the PL022
>> driver code as part of the patch, like this:
>
> Yes, this looks fine.  Shall I wrap it up as part of my patch?

Yes please.
Acked-by.

> Two other things I've spotted in this driver are:
>
> 1. The remove function doesn't undo what the probe function did to
> the pclk and vcore.  It needs to keep things balanced.  For a driver
> which doesn't manage its pclk, this is what happens:
>        - core gets pclk
>        - core enables pclk
>        - core calls driver's probe
>                - driver sets stuff up
> ...
>        - core calls driver's remove
>                - driver tidies up
>        - core disables pclk
>        - core puts pclk
>
> And PL022 does this:
>        - core gets pclk
>        - core enables pclk
>        - core calls driver's probe
>                - driver sets stuff up
>                - driver disables pclk
> ...
>        - core calls driver's remove
>                - driver tidies up
>        - core disables pclk
>        - core puts pclk
>
> Notice the double-disable of pclk in that sequence.  If ->probe disables
> pclk, ->remove needs to return with that disable balanced with an enable.

Ah yes. Can you fix this as part of the patch since it's pretty related?
Much appreciated if you do.

> 2. It thinks it can refuse 'remove' by returning an error code.  This
> is false.  removes can't be aborted - here's the code from drivers/base/dd.c:
>
> static void __device_release_driver(struct device *dev)
> {
> ...
>                if (dev->bus && dev->bus->remove)
>                        dev->bus->remove(dev);
>                else if (drv->remove)
>                        drv->remove(dev);
> ...
> }
>
> Notice how return codes go nowhere.  remove should _really_ be a void
> function to stop people thinking that it can be aborted.  It can't.

Correct, thanks for spotting this. I'll send a separate patch to Grant
fixing this up.

Yours,
Linus Walleij
--
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/