Re: [PATCH RFC 09/39] mmc: queue: Add a function to control wake-up on new requests

Linus Walleij
2017-02-15T13:07:37Z

On Fri, Feb 10, 2017 at 1:55 PM, Adrian Hunter <adrian.hunter@intel.com> wrote:

> Add a function to control wake-up on new requests. This will enable
> Software Command Queuing to choose whether or not to queue new
> requests immediately or wait for the current task to complete.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>

I think my patch set killed off the context and this whole thing
"just works" instead of needing more states and hacks like this.

The proliferation of states is reaching a all time high here:

+ if (cntx->is_waiting_last_req != wake_me) {
+ spin_lock_irq(q->queue_lock);
+ cntx->is_waiting_last_req = wake_me;
+ cntx->is_new_req = wake_me && mq->is_new_req;
(...)
@@ -43,6 +43,7 @@ struct mmc_queue {
(...)
+ bool is_new_req;

Now there is a is_new_req in the context AND an identically
named state variable in struct mmc_queue, and when reading the
code I suspect a reader would like to know the difference
between the two .is_new_req variables, one in the state
of the host and one in the state of the queue.

This is a cognitive nightmare, sorry. At least this would have to
have more specific names.

I think all these states come from the polling loop construction
and this constant spinning around and checking for errors, doing
and undoing the pre/post/bounce stuff, and compulsively flushing
the pipeline with NULL and so on. I was trying to
get rid of that stuff, but this is expanding it instead.

Hm, maybe I get a bit on the complainy side, sorry if so...

Yours,
Linus Walleij