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

[Gilad Ben-Yossef]

On Tue, Apr 18, 2017 at 6:43 PM, Mark Rutland  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

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

[Gilad Ben-Yossef]

On Tue, Apr 18, 2017 at 6:39 PM, Greg Kroah-Hartman
 wrote:
> On Tue, Apr 18, 2017 at 05:07:50PM +0300, Gilad Ben-Yossef wrote:
>> Arm TrustZone CryptoCell 700 is a family of cryptographic hardware
>> accelerators. It is supported by a long lived series of out of tree
>> drivers, which I am now in the process of unifying and upstreaming.
>> This is the first drop, supporting the new CryptoCell 712 REE.
>>
>> 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.
>>
>> Signed-off-by: Gilad Ben-Yossef 
>> CC: Binoy Jayan 
>> CC: Ofir Drang 
>>
>> Gilad Ben-Yossef (4):
>>   staging: add ccree crypto driver
>>   staging: ccree: add TODO list
>>   staging: ccree: add devicetree bindings
>>   MAINTAINERS: add Gilad BY as maintainer for ccree
>
> We can't do much without a real patch, sorry.  Digging in random git
> trees doesn't work :(
>
> I can't take this as-is, I need patches.

Got it.

I'll break the driver to a series of patches and post them .
Thanks for the feedback.

Gilad



-- 
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

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

[Mark Rutland]

On Tue, Apr 18, 2017 at 06:29:22PM +0300, Gilad Ben-Yossef wrote:
> On Tue, Apr 18, 2017 at 6:13 PM, Mark Rutland  wrote:
> > On Tue, Apr 18, 2017 at 05:07:50PM +0300, Gilad Ben-Yossef wrote:
> >> Arm TrustZone CryptoCell 700 is a family of cryptographic hardware
> >> accelerators. It is supported by a long lived series of out of tree
> >> drivers, which I am now in the process of unifying and upstreaming.
> >> This is the first drop, supporting the new CryptoCell 712 REE.
> >>
> >> 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.
> >>
> >> Signed-off-by: Gilad Ben-Yossef 
> >> CC: Binoy Jayan 
> >> CC: Ofir Drang 
> >>
> >> Gilad Ben-Yossef (4):
> >>   staging: add ccree crypto driver
> >>   staging: ccree: add TODO list
> >>   staging: ccree: add devicetree bindings
> >>   MAINTAINERS: add Gilad BY as maintainer for 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].

> 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.

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).

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/.

Thanks,
Mark.

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

[Greg Kroah-Hartman]

On Tue, Apr 18, 2017 at 05:07:50PM +0300, Gilad Ben-Yossef wrote:
> Arm TrustZone CryptoCell 700 is a family of cryptographic hardware
> accelerators. It is supported by a long lived series of out of tree
> drivers, which I am now in the process of unifying and upstreaming.
> This is the first drop, supporting the new CryptoCell 712 REE. 
> 
> 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.
> 
> Signed-off-by: Gilad Ben-Yossef 
> CC: Binoy Jayan 
> CC: Ofir Drang 
> 
> Gilad Ben-Yossef (4):
>   staging: add ccree crypto driver
>   staging: ccree: add TODO list
>   staging: ccree: add devicetree bindings
>   MAINTAINERS: add Gilad BY as maintainer for ccree

We can't do much without a real patch, sorry.  Digging in random git
trees doesn't work :(

I can't take this as-is, I need patches.

thanks,

greg k-h

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

[Gilad Ben-Yossef]

Hi Mark,

On Tue, Apr 18, 2017 at 6:13 PM, Mark Rutland  wrote:
> Hi,
>
> On Tue, Apr 18, 2017 at 05:07:50PM +0300, Gilad Ben-Yossef wrote:
>> Arm TrustZone CryptoCell 700 is a family of cryptographic hardware
>> accelerators. It is supported by a long lived series of out of tree
>> drivers, which I am now in the process of unifying and upstreaming.
>> This is the first drop, supporting the new CryptoCell 712 REE.
>>
>> 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.
>>
>> Signed-off-by: Gilad Ben-Yossef 
>> CC: Binoy Jayan 
>> CC: Ofir Drang 
>>
>> Gilad Ben-Yossef (4):
>>   staging: add ccree crypto driver
>>   staging: ccree: add TODO list
>>   staging: ccree: add devicetree bindings
>>   MAINTAINERS: add Gilad BY as maintainer for 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.

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.

Many thanks,
Gilad



-- 
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

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

[Mark Rutland]

Hi,

On Tue, Apr 18, 2017 at 05:07:50PM +0300, Gilad Ben-Yossef wrote:
> Arm TrustZone CryptoCell 700 is a family of cryptographic hardware
> accelerators. It is supported by a long lived series of out of tree
> drivers, which I am now in the process of unifying and upstreaming.
> This is the first drop, supporting the new CryptoCell 712 REE. 
> 
> 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.
> 
> Signed-off-by: Gilad Ben-Yossef 
> CC: Binoy Jayan 
> CC: Ofir Drang 
> 
> Gilad Ben-Yossef (4):
>   staging: add ccree crypto driver
>   staging: ccree: add TODO list
>   staging: ccree: add devicetree bindings
>   MAINTAINERS: add Gilad BY as maintainer for 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,
Mark.