Re: [dpdk-dev] [PATCH v2 09/33] crypto/octeontx: adds symmetric capabilities

[Thomas Monjalon <thomas@monjalon.net>]

23/10/2018 10:48, Joseph, Anoob:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 22/10/2018 05:49, Joseph, Anoob:
> > > Hi Fiona,
> > >
> > > I do agree that your solution seems to be a neat way for organizing
> > capabilities. But Akhil & Thomas were against that idea and we had to come up
> > with one array with all capabilities. This would not scale well when we start
> > supporting devices with varying capabilities.
> > >
> > > If your plan is to follow the same approach for asym support, maybe we will
> > also follow suit and submit the required patches.
> > >
> > > @Akhil, Thomas, thoughts?
> > 
> > Generally speaking, macros are bad.
> > 
> > Why cannot you just write functions?
> > I don't understand your issue.
> 
> I had replaced macro with functions when I revised the patch. But when more devices (with varying capabilities) need to be supported, this can get complicated. Macro approach would be simpler in that case. Right now QAT has macros and we would like to stick to what is being followed in the community.
> 
> Following would be the use case for macros,
> 
> #define QAT_BASE_GEN1_SYM_CAPABILITIES, 	\
> {	CAPABILITES,				\
> 	...					\
> }
> 
> #define QAT_EXTRA_GEN2_SYM_CAPABILITIES  	\
> {	CAPABILITES,				\
> 	...					\
> }
> 
> static const struct rte_cryptodev_capabilities qat_gen1_sym_capabilities[] = {
> 	QAT_BASE_GEN1_SYM_CAPABILITIES,
> 	RTE_CRYPTODEV_END_OF_CAPABILITIES_LIST()
> };
> 
> static const struct rte_cryptodev_capabilities qat_gen2_sym_capabilities[] = {
> 	QAT_BASE_GEN1_SYM_CAPABILITIES,
> 	QAT_EXTRA_GEN2_SYM_CAPABILITIES,
> 	RTE_CRYPTODEV_END_OF_CAPABILITIES_LIST()
> };
> 
> Without the macros, we will be required to populate the array there itself and would mean repetition of GEN1 capabilities. Either approach is fine for us, but this could complicate things when we add support for ASYM. Since QAT is already doing this, is it fine to move to that approach as we add support for ASYM? Or if QAT is to follow any other scheme, I'm fine with adopting that as well. Whatever is simple and uniform would work.

You can use simple assignments in functions and avoid repetition.

There is a warning in checkpatch about macros.
I think we should be more strict with this warning.