linux-kernel - Re: [PATCH 1/2] MMC Agressive clocking framework v5

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: <63386a3d0907231330h8695afdsdf7d642d3765b56c@mail.gmail.com>
Date:	Thu, 23 Jul 2009 22:30:40 +0200
From:	Linus Walleij <linus.ml.walleij@...il.com>
To:	David Vrabel <david.vrabel@....com>
Cc:	Linus Walleij <linus.walleij@...ricsson.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, Pierre Ossman <pierre@...man.eu>,
	linux-arm-kernel@...ts.arm.linux.org.uk
Subject: Re: [PATCH 1/2] MMC Agressive clocking framework v5

2009/7/23 David Vrabel <david.vrabel@....com>:
> Linus Walleij wrote:
>> 2009/7/22 David Vrabel <david.vrabel@....com>:

>>> 1. With some controllers (e.g., PXA270 I think) turning the clock on and
>>> off is slow.  This means if you're doing back-to-back commands you
>>> should leave the clock on for best performance.
>>
>> OK, when I've been testing this using the default workqueue and
>> schedule_work() covered these cases. Back-on-back commands
>> seemingly doesn't allow the timeout work to schedule, but I might be
>> overseeing the case of several CPU:s there though :-/
>
> Ok, with a configurable timeout your scheme is fine.  The timeout can be
> extended beyond 8 SDCLKs if this is beneficial.

Yep I have that in debugfs, but when I look at Adrians code I see he instead
added a disable delay field to the host struct so let's use his patch
instead then.

> I'm not sure what the best way to add this would be.  You could:
>
> 1. Have a special clock frequency to mean idle and fix up all existing
> controller drivers to interpret this as 400 kHz unless you know the
> controller handles SDIO interrupts with no SDCLK.
>
> or:
>
> 2. Add an additional controller method (set_bus_state?) and only provide
> this on controller drivers you're interested in.

As discussed with Adrian this is what his patch does (adding a new host->ops
function for enable/disable) so let's use his patch.

>>> 3. Regardless of point 1 above.  Using a workqueue item in this way
>>> seems overkill.  Consider using a timer and simply calling mod_timer()
>>> at the start of every command.  When the timer expires, idle the clock.
>>>  You will probably need a "command in progress" bit to ensure you don't
>>> idle the clock if the timer expires in the middle of a command.
>>
>> I would agree if I created a new workqueue, but the timeout of this
>> particular workqueue is unimportant and that's why I'm using the
>> global workqueue and just schedule_work(). This means no extra
>> overhead, no extra thread and basically does the exact same thing.
>
> You currently queue a work item and wake the workqueue every command.
> This is considerably more overhead (when doing back-to-back commands)
> than simply calling mod_timer().
>
> You also potentially delay for a considerable amount of time in the work
> item.

Yep that's the idea almost... But let's raise the timer debate again with
Adrian's patch instead :-)

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/