Re: [PATCH 0/9] crypto: caam - Add RTA descriptor creation library
On 7/19/2014 4:23 AM, Kim Phillips wrote: On Sat, 19 Jul 2014 02:51:30 +0300 Horia Geantă horia.gea...@freescale.com wrote: On 7/19/2014 1:13 AM, Kim Phillips wrote: On Fri, 18 Jul 2014 19:37:17 +0300 Horia Geanta horia.gea...@freescale.com wrote: This patch set adds Run Time Assembler (RTA) SEC descriptor library. The main reason of replacing incumbent inline append is to have a single code base both for user space and kernel space. that's orthogonal to what this patchseries is doing from the kernel maintainer's perspective: it's polluting the driver with a CodingStyle-violating (see, e.g., Chapter 12) 6000+ lines of code - Regarding coding style - AFAICT that's basically: ERROR: Macros with complex values should be enclosed in parenthesis and I am wiling to find a different approach. There's that, too. which can only mean it's slower and more susceptible to bugs - and AFAICT for no superior technical advantage: NACK from me. The fact that the code size is bigger doesn't necessarily mean a bad thing: 1-code is better documented - cloc reports ~ 1000 more lines of comments; patch 09 even adds support for generating a docbook 2-pure code (i.e. no comments, white spaces) - cloc reports ~ 5000 more lines; this reflects two things, AFAICT: 2.1-more features: options (for e.g. new SEC instructions, little endian env. support), platform support includes Era 7 and Era 8, i.e. Layerscape LS1 and LS2; this is important to note, since plans are to run the very same CAAM driver on ARM platforms um, *those* features should not cost any driver *that many* lines of code! You are invited to comment on the code at hand. I am pretty sure it's not perfect. 2.2-more error-checking - from this perspective, I'd say driver is less susceptible to bugs, especially subtle ones in CAAM descriptors that are hard to identify / debug; RTA will complain when generating descriptors using features (say a new bit in an instruction opcode) that are not supported on the SEC on device ? The hardware does the error checking. This just tells me RTA is slow, inflexible, and requires unnecessary maintenance by design: all the more reason to keep it out of the kernel :) This is just like saying a toolchain isn't performing any checks and lets the user generate invalid machine code and deal with HW errors. Beside this, there are (quite a few) cases when SEC won't emit any error, but still the results are different than expected. SEC HW is complex enough to deserve descriptor error checking. RTA currently runs on: -QorIQ platforms - userspace (USDPAA) -Layerscape platforms - AIOP accelerator (obviously, plans are to run also on QorIQ/PowerPC and LS/ARM kernels) inline append runs elsewhere, too, but I don't see how this is related to the subject I'm bringing up. This is relevant, since having a piece of SW running in multiple environments should lead to better testing, more exposure, finding bugs faster. inline append *could run* elsewhere , but it doesn't AFAICT. Last time I checked, USDPAA and AIOP use RTA. Combined with: -comprehensive unit testing suite -RTA kernel port is bit-exact in terms of SEC descriptors hex dumps with inline append; besides this, it was tested with tcrypt and in IPsec scenarios I would say that RTA is tested more than inline append. In the end, this is a side effect of having a single code base. inline append has been proven stable for years now. RTA just adds redundant code and violates CodingStyle AFAICT. New platform support is not redundant. Error checking is not redundant, as explained above. kernel-doc is always helpful. Coding Style can be fixed. Thanks, Horia -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/9] crypto: caam - Add RTA descriptor creation library
On Mon, 21 Jul 2014 10:47:49 +0300 Horia Geantă horia.gea...@freescale.com wrote: On 7/19/2014 4:23 AM, Kim Phillips wrote: On Sat, 19 Jul 2014 02:51:30 +0300 Horia Geantă horia.gea...@freescale.com wrote: On 7/19/2014 1:13 AM, Kim Phillips wrote: On Fri, 18 Jul 2014 19:37:17 +0300 Horia Geanta horia.gea...@freescale.com wrote: This patch set adds Run Time Assembler (RTA) SEC descriptor library. which can only mean it's slower and more susceptible to bugs - and AFAICT for no superior technical advantage: NACK from me. The fact that the code size is bigger doesn't necessarily mean a bad thing: 1-code is better documented - cloc reports ~ 1000 more lines of comments; patch 09 even adds support for generating a docbook 2-pure code (i.e. no comments, white spaces) - cloc reports ~ 5000 more lines; this reflects two things, AFAICT: 2.1-more features: options (for e.g. new SEC instructions, little endian env. support), platform support includes Era 7 and Era 8, i.e. Layerscape LS1 and LS2; this is important to note, since plans are to run the very same CAAM driver on ARM platforms um, *those* features should not cost any driver *that many* lines of code! You are invited to comment on the code at hand. I am pretty sure it's not perfect. I can see RTA is responsible for the code size increase, not the features. And just because RTA has - or has plans for - those features don't justify the kernel driver adopting RTA over inline-append. 2.2-more error-checking - from this perspective, I'd say driver is less susceptible to bugs, especially subtle ones in CAAM descriptors that are hard to identify / debug; RTA will complain when generating descriptors using features (say a new bit in an instruction opcode) that are not supported on the SEC on device ? The hardware does the error checking. This just tells me RTA is slow, inflexible, and requires unnecessary maintenance by design: all the more reason to keep it out of the kernel :) This is just like saying a toolchain isn't performing any checks and lets the user generate invalid machine code and deal with HW errors. Beside this, there are (quite a few) cases when SEC won't emit any error, but still the results are different than expected. SEC HW is complex enough to deserve descriptor error checking. if part of RTA's objective is to cater to new SEC programmers, great, but that doesn't mean it belongs in the crypto API driver's limited input parameter set, and fixed descriptors operating environment: it's not the place to host an entire SEC toolchain. RTA currently runs on: -QorIQ platforms - userspace (USDPAA) -Layerscape platforms - AIOP accelerator (obviously, plans are to run also on QorIQ/PowerPC and LS/ARM kernels) inline append runs elsewhere, too, but I don't see how this is related to the subject I'm bringing up. This is relevant, since having a piece of SW running in multiple environments should lead to better testing, more exposure, finding bugs faster. that doesn't defeat the fact that more lines of code to do the same thing is always going to be a more bug-prone way of doing it. inline append *could run* elsewhere , but it doesn't AFAICT. Last time I checked, USDPAA and AIOP use RTA. inline append runs in ASF, and has been available for all upstream for years. Combined with: -comprehensive unit testing suite -RTA kernel port is bit-exact in terms of SEC descriptors hex dumps with inline append; besides this, it was tested with tcrypt and in IPsec scenarios I would say that RTA is tested more than inline append. In the end, this is a side effect of having a single code base. inline append has been proven stable for years now. RTA just adds redundant code and violates CodingStyle AFAICT. New platform support is not redundant. No, RTA is. Error checking is not redundant, as explained above. It is: the kernel has fixed descriptors. kernel-doc is always helpful. it doesn't matter how much you decorate it. Coding Style can be fixed. inline append isn't broken. Kim -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/9] crypto: caam - Add RTA descriptor creation library
On 7/19/2014 1:13 AM, Kim Phillips wrote: On Fri, 18 Jul 2014 19:37:17 +0300 Horia Geanta horia.gea...@freescale.com wrote: This patch set adds Run Time Assembler (RTA) SEC descriptor library. The main reason of replacing incumbent inline append is to have a single code base both for user space and kernel space. that's orthogonal to what this patchseries is doing from the kernel maintainer's perspective: it's polluting the driver with a CodingStyle-violating (see, e.g., Chapter 12) 6000+ lines of code - Regarding coding style - AFAICT that's basically: ERROR: Macros with complex values should be enclosed in parenthesis and I am wiling to find a different approach. which can only mean it's slower and more susceptible to bugs - and AFAICT for no superior technical advantage: NACK from me. The fact that the code size is bigger doesn't necessarily mean a bad thing: 1-code is better documented - cloc reports ~ 1000 more lines of comments; patch 09 even adds support for generating a docbook 2-pure code (i.e. no comments, white spaces) - cloc reports ~ 5000 more lines; this reflects two things, AFAICT: 2.1-more features: options (for e.g. new SEC instructions, little endian env. support), platform support includes Era 7 and Era 8, i.e. Layerscape LS1 and LS2; this is important to note, since plans are to run the very same CAAM driver on ARM platforms 2.2-more error-checking - from this perspective, I'd say driver is less susceptible to bugs, especially subtle ones in CAAM descriptors that are hard to identify / debug; RTA will complain when generating descriptors using features (say a new bit in an instruction opcode) that are not supported on the SEC on device RTA currently runs on: -QorIQ platforms - userspace (USDPAA) -Layerscape platforms - AIOP accelerator (obviously, plans are to run also on QorIQ/PowerPC and LS/ARM kernels) Combined with: -comprehensive unit testing suite -RTA kernel port is bit-exact in terms of SEC descriptors hex dumps with inline append; besides this, it was tested with tcrypt and in IPsec scenarios I would say that RTA is tested more than inline append. In the end, this is a side effect of having a single code base. Thanks, Horia -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/9] crypto: caam - Add RTA descriptor creation library
On Sat, 19 Jul 2014 02:51:30 +0300 Horia Geantă horia.gea...@freescale.com wrote: On 7/19/2014 1:13 AM, Kim Phillips wrote: On Fri, 18 Jul 2014 19:37:17 +0300 Horia Geanta horia.gea...@freescale.com wrote: This patch set adds Run Time Assembler (RTA) SEC descriptor library. The main reason of replacing incumbent inline append is to have a single code base both for user space and kernel space. that's orthogonal to what this patchseries is doing from the kernel maintainer's perspective: it's polluting the driver with a CodingStyle-violating (see, e.g., Chapter 12) 6000+ lines of code - Regarding coding style - AFAICT that's basically: ERROR: Macros with complex values should be enclosed in parenthesis and I am wiling to find a different approach. There's that, too. which can only mean it's slower and more susceptible to bugs - and AFAICT for no superior technical advantage: NACK from me. The fact that the code size is bigger doesn't necessarily mean a bad thing: 1-code is better documented - cloc reports ~ 1000 more lines of comments; patch 09 even adds support for generating a docbook 2-pure code (i.e. no comments, white spaces) - cloc reports ~ 5000 more lines; this reflects two things, AFAICT: 2.1-more features: options (for e.g. new SEC instructions, little endian env. support), platform support includes Era 7 and Era 8, i.e. Layerscape LS1 and LS2; this is important to note, since plans are to run the very same CAAM driver on ARM platforms um, *those* features should not cost any driver *that many* lines of code! 2.2-more error-checking - from this perspective, I'd say driver is less susceptible to bugs, especially subtle ones in CAAM descriptors that are hard to identify / debug; RTA will complain when generating descriptors using features (say a new bit in an instruction opcode) that are not supported on the SEC on device ? The hardware does the error checking. This just tells me RTA is slow, inflexible, and requires unnecessary maintenance by design: all the more reason to keep it out of the kernel :) RTA currently runs on: -QorIQ platforms - userspace (USDPAA) -Layerscape platforms - AIOP accelerator (obviously, plans are to run also on QorIQ/PowerPC and LS/ARM kernels) inline append runs elsewhere, too, but I don't see how this is related to the subject I'm bringing up. Combined with: -comprehensive unit testing suite -RTA kernel port is bit-exact in terms of SEC descriptors hex dumps with inline append; besides this, it was tested with tcrypt and in IPsec scenarios I would say that RTA is tested more than inline append. In the end, this is a side effect of having a single code base. inline append has been proven stable for years now. RTA just adds redundant code and violates CodingStyle AFAICT. Kim -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
