linux-kernel - Re: [PATCH 0/4] staging: add ccree crypto driver

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] [day] [month] [year] [list]

Date:   Wed, 19 Apr 2017 17:40:22 +0300
From:   Gilad Ben-Yossef <gilad@...yossef.com>
To:     Mark Rutland <mark.rutland@....com>
Cc:     Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        Rob Herring <robh+dt@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        devel@...verdev.osuosl.org, linux-crypto@...r.kernel.org,
        devicetree@...r.kernel.org,
        Linux kernel mailing list <linux-kernel@...r.kernel.org>,
        Gilad Ben-Yossef <gilad.benyossef@....com>,
        Binoy Jayan <binoy.jayan@...aro.org>,
        Ofir Drang <ofir.drang@....com>
Subject: Re: [PATCH 0/4] staging: add ccree crypto driver

On Tue, Apr 18, 2017 at 6:43 PM, Mark Rutland <mark.rutland@....com> wrote:
...
>> >>
>> >> The code still needs some cleanup before maturing to a proper
>> >> upstream driver, which I am in the process of doing. However,
>> >> as discussion of some of the capabilities of the hardware and
>> >> its application to some dm-crypt and dm-verity features recently
>> >> took place I though it is better to do this in the open via the
>> >> staging tree.
>> >>
>> >> A Git repository based off of Linux 4.11-rc7 is available at
>> >> https://github.com/gby/linux.git branch ccree.
>> >>
...
>> >>  .../devicetree/bindings/crypto/arm-cryptocell.txt  |   23 +
>> >
>> > I'm interested in looking at the DT binding, but for some reason I've
>> > only recevied the cover letter so far.
>> >
>> > Are my mail servers being particularly slow today, or has something gone
>> > wrong with the Cc list for the remaining patches?
>> >
>>
>> Thanks a bunch for the willingness to review this.
>>
>> My apologies for not communicating this clearly enough -  Since it is
>> an pre-existing driver it is rather big.
>> I did not want to inflict a 600K patch on the mailing list so opted to
>> provide a git repo instead, as stated in the
>> email.
>
> Should this have been a [GIT PULL], then?
>
> I was confused by the [PATCH 0/4].

Yes, it should have. Sorry about that.
>
>> The patch in question is here:
>> https://github.com/gby/linux/commit/12d92e0bc19aa9bbd891cdab765eaaeabe0b6967
>>
>> Do let me know if you prefer that I will send at least the smaller
>> patch file via email in the normal fashion.
>
> Sending patches is the usual way to have things reviewed. Linking to an
> external site doesn't fit with the workflows of everyone.
>
> If you wish to have patches reviewed, please send them as patches in the
> usual fashion.

Thanks for the feedback.
I will break the driver up and post patches in the normal fashion.

>
> I will note that on the patch you linked, the compatible string is far
> too vague, per common conventions. Per your description above, that
> should be something like "arm,cryptocell-712-ree" (and each variant
> should have its own string).

Got it. Will change.

Thanks for the review even in this unconventional form :-) !

>
> I don't have documentation to hand to attempt to review the rest.
>
> I would appreciate if you could ensure that the DT binding was reviewed
> as part of the staging TODOs. That needs to follow the usual DT review
> process of sending patches to the list. Until that has occurred, it
> shouldn't be in Documentation/devicetree/.

Of course, will do.

Thanks,
Gilad
>
> Thanks,
> Mark.



-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru