linux-kernel - Re: [PATCH V13 05/10] mmc: cqhci: support for command queue enabled host

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 for Android: free password hash cracker in your pocket

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

Date:   Thu, 9 Nov 2017 14:55:34 +0200
From:   Adrian Hunter <adrian.hunter@...el.com>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     Ulf Hansson <ulf.hansson@...aro.org>,
        linux-mmc <linux-mmc@...r.kernel.org>,
        linux-block <linux-block@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Bough Chen <haibo.chen@....com>,
        Alex Lemberg <alex.lemberg@...disk.com>,
        Mateusz Nowak <mateusz.nowak@...el.com>,
        Yuliy Izrailov <Yuliy.Izrailov@...disk.com>,
        Jaehoon Chung <jh80.chung@...sung.com>,
        Dong Aisheng <dongas86@...il.com>,
        Das Asutosh <asutoshd@...eaurora.org>,
        Zhangfei Gao <zhangfei.gao@...il.com>,
        Sahitya Tummala <stummala@...eaurora.org>,
        Harjani Ritesh <riteshh@...eaurora.org>,
        Venu Byravarasu <vbyravarasu@...dia.com>,
        Shawn Lin <shawn.lin@...k-chips.com>,
        Christoph Hellwig <hch@....de>
Subject: Re: [PATCH V13 05/10] mmc: cqhci: support for command queue enabled
 host

On 09/11/17 14:26, Linus Walleij wrote:
> On Wed, Nov 8, 2017 at 3:14 PM, Adrian Hunter <adrian.hunter@...el.com> wrote:
>> On 08/11/17 11:22, Linus Walleij wrote:
>>> On Fri, Nov 3, 2017 at 2:20 PM, Adrian Hunter <adrian.hunter@...el.com> wrote:
> 
>>> (...)
> 
>>>> +EXPORT_SYMBOL(cqhci_resume);
>>>
>>> Why would the CQE case require special suspend/resume
>>> functionality?
>>
>> Seems like a very strange question.
> 
> Please realize that patch review is partly about education.
> 
> Educating me or anyone about your patch set involves
> being humbe and not seeing your peers as lesser.
> 
> Making your reviewer feel stupid by saying the ask
> "strange" or outright stupid questions is not helping
> your cause.

Please try to forgive me for being rude, it is just frustration.

> 
>> Obviously CQHCI has to be configured
>> after suspend.
> 
> Yeah. I think I misunderstood is such that:
> 
>> Also please don't confuse CQE and CQHCI.  CQHCI is an implementation of a
>> CQE.  We currently do not expect to have another implementation, but it is
>> not impossible.
> 
> OK now you educated me, see it's not that hard without
> using belitteling language.
> 
>>> This seems two much like on the side CQE-silo engineering,
>>> just use the device .[runtime]_suspend/resume callbacks like
>>> everyone else, make it possible for the host to figure out
>>> if it is in CQE mode or not (I guess it should know already
>>> since cqhci .enable() has been called?) and handle it
>>> from there.
>>
>> That is how it works!  The host controller has to decide how to handle
>> suspend / resume.
> 
> OK.
> 
>> cqhci_suspend() / cqhci_resume() are helper functions that the host
>> controller can use, but doesn't have to.
> 
> OK.
> 
>>> Why would CQE hosts need special accessors and the rest
>>> of the host not need it?
>>
>> Special accessors can be used to fix up registers that don't work exactly
>> the way the standard specified.
> 
> Yeah this is fine as it is for CQHCI, I didn't get that
> part :)
> 
>>> ->enable and ->disable() for just CQE seem reasonable.
>>> But that leaves just two new ops.
>>>
>>> So why not just put .cqe_enable() and .cqe_disable()
>>> ops into mmc_host_ops as optional and be done with it?
>>
>> Ok so you are not understanding this at all.
> 
> No I did not get it. But I do now (I think).
> 
>> As a CQE implementation, CQHCI interfaces with the upper layers through the
>> CQE ops etc.
>>
>> But CQHCI also has to work with any host controller driver, so it needs an
>> interface for that, which is what cqhci_host_ops is for.  All the ops serve
>> useful purposes.
> (...)
>> The whole point is to prove a library that can work with any host controller
>> driver.  That means it must provide functions and callbacks.
> 
> OK
> 
>>> I think the above approach to put any CQE-specific callbacks
>>> directly into the struct mmc_host_ops is way more viable.
>>
>> Nothing to do with CQE.  This is CQHCI.  Please try to get the difference.
> 
> I am trying, please try to think about your language.

I strongly disapprove of being rude but sadly it seems to get results.