Messages in this thread | | | Subject | Re: [PATCH v2 3/3] virtio-crypto: implement RSA algorithm | From | zhenwei pi <> | Date | Wed, 23 Feb 2022 18:17:21 +0800 |
| |
On 2/18/22 11:12 AM, zhenwei pi wrote: >>> +void virtio_crypto_akcipher_algs_unregister(struct virtio_crypto >>> +*vcrypto) { >>> + int i = 0; >>> + >>> + mutex_lock(&algs_lock); >>> + >>> + for (i = 0; i < ARRAY_SIZE(virtio_crypto_akcipher_algs); i++) { >>> + uint32_t service = virtio_crypto_akcipher_algs[i].service; >>> + uint32_t algonum = virtio_crypto_akcipher_algs[i].algonum; >>> + >>> + if (virtio_crypto_akcipher_algs[i].active_devs == 0 || >>> + !virtcrypto_algo_is_supported(vcrypto, service, algonum)) >>> + continue; >>> + >>> + if (virtio_crypto_akcipher_algs[i].active_devs == 1) >>> + >>> crypto_unregister_akcipher(&virtio_crypto_akcipher_algs[i].algo); >>> + >>> + virtio_crypto_akcipher_algs[i].active_devs--; >>> + } >>> + >>> + mutex_unlock(&algs_lock); >>> +} >> >> Why don't you reuse the virtio_crypto_algs_register/unregister functions? >> The current code is too repetitive. Maybe we don't need create the new >> file virtio_crypto_akcipher_algo.c >> because we had virtio_crypto_algs.c which includes all algorithms. >> > > Yes, this looks similar to virtio_crypto_algs_register/unregister. > > Let's look at the difference: > struct virtio_crypto_akcipher_algo { > uint32_t algonum; > uint32_t service; > unsigned int active_devs; > struct akcipher_alg algo; > }; > > struct virtio_crypto_algo { > uint32_t algonum; > uint32_t service; > unsigned int active_devs; > struct skcipher_alg algo; /* akcipher_alg VS skcipher_alg */ > }; > > If reusing virtio_crypto_algs_register/unregister, we need to modify the > data structure like this: > struct virtio_crypto_akcipher_algo { > uint32_t algonum; > uint32_t service; /* use service to distinguish > akcipher/skcipher */ > unsigned int active_devs; > union { > struct skcipher_alg skcipher; > struct akcipher_alg akcipher; > } alg; > }; > > int virtio_crypto_akcipher_algs_register(struct virtio_crypto *vcrypto) > { > ... > for (i = 0; i < ARRAY_SIZE(virtio_crypto_akcipher_algs); i++) { > uint32_t service = virtio_crypto_akcipher_algs[i].service; > ... > /* test service type then call > crypto_register_akcipher/crypto_register_skcipher */ > if (service == VIRTIO_CRYPTO_SERVICE_AKCIPHER) > > crypto_register_akcipher(&virtio_crypto_akcipher_algs[i].algo.akcipher); > else > > crypto_register_skcipher(&virtio_crypto_skcipher_algs[i].algo.skcipher); > ... > } > ... > } > > Also test service type and call > crypto_unregister_skcipher/crypto_unregister_akcipher. > > This gets unclear from current v2 version. > > On the other hand, the kernel side prefers to separate skcipher and > akcipher(separated header files and implementations). > Hi, Lei I also take a look at other crypto drivers at qat/ccp/hisilicon, they separate akcipher/skcipher algo. If you consider that reusing virtio_crypto_algs_register/unregister seems better, I will try to merge them into a single function.
-- zhenwei pi
| |