Re: [PATCH 2/2] nvme: keyring: fix conditional compilation

[Arnd Bergmann]

On Fri, Oct 20, 2023, at 15:50, Hannes Reinecke wrote:
> On 10/20/23 15:05, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@xxxxxxxx>
>>   
>>   static void __exit nvme_core_exit(void)
>>   {
>> -	nvme_exit_auth();
>> -	nvme_keyring_exit();
>> +	if (IS_ENABLED(CONFIG_NVME_HOST_AUTH))
>> +		nvme_exit_auth();
>> +	if (IS_ENABLED(CONFIG_NVME_TCP_TLS))
>> +		nvme_keyring_exit();
>>   	class_destroy(nvme_ns_chr_class);
>>   	class_destroy(nvme_subsys_class);
>>   	class_destroy(nvme_class);
>
> Please add stub calls and avoid sprinkle the code with
> IS_ENABLED statements.

That seems to add a lot of complexity, but I can try. If I can't
figure it out, someone else might have to try it. Since we need
to check separately for the host and target options, this will
lead to having two extra stubs per function call, right?

key_serial_t nvme_tls_psk_default(struct key *keyring,
                const char *hostnqn, const char *subnqn);

static inline key_serial_t nvme_host_tls_psk_default(struct key *keyring,
                const char *hostnqn, const char *subnqn)
{  
        if (IS_ENABLED(CONFIG_NVME_TCP_TLS)
                return nvme_tls_psk_default(keyring, hostnqn, subnqn);

        return 0;
}

static inline key_serial_t nvme_target_tls_psk_default(struct key *keyring,
                const char *hostnqn, const char *subnqn)
{
        if (IS_ENABLED(CONFIG_NVME_TARGET_TCP_TLS))
                return nvme_host_tls_psk_default(keyring, hostnqn, subnqn);

        return 0;
}

>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>> index 4714a902f4caa..e2b90789c0407 100644
>> --- a/drivers/nvme/host/tcp.c
>> +++ b/drivers/nvme/host/tcp.c
>> @@ -1915,7 +1915,7 @@ static int nvme_tcp_alloc_admin_queue(struct nvme_ctrl *ctrl)
>>   	int ret;
>>   	key_serial_t pskid = 0;
>>   
>> -	if (ctrl->opts->tls) {
>> +	if (IS_ENABLED(CONFIG_NVME_TCP_TLS) && ctrl->opts->tls) {
>
> Why? '->tls' is not protected by a CONFIG options, and should be
> available in general ...
>
>>   		if (ctrl->opts->tls_key)
>>   			pskid = key_serial(ctrl->opts->tls_key);
>>   		else

It's the nvme_tls_psk_default() call that needs to be protected
here, but I found that the entire code block is dead if tls_key
is false, so it seemed more logical to make the entire block
conditional when we know the condition is always false at
compile time.

     Arnd