Re: [PATCH V2 2/2] nvme-core: use macro defination to define dev attr

[Chaitanya Kulkarni <chaitanyak@nvidia.com>]

On 6/5/23 00:55, Hannes Reinecke wrote:
> On 6/5/23 08:51, Chaitanya Kulkarni wrote:
>> Insated of duplicating the code for dhchap_secret & dhchap_ctrl_secret,
>> define a macro to register device attribute to create device attribute
>> that will also define store and show helpers.
>>
>> Note that this is not newly invented but followed the same format
>> preasent in block layer and nvme source code.
>> nvme_show_str_function/nvme_show_int_function/
>> nvme_subsys_show_str_function/ nullb_device_##NAME##_show/
>> nullb_device_##NAME##_store/LOOP_ATTR_RO
>>
>> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
>> ---
>>   drivers/nvme/host/sysfs.c | 67 +++++++++++++++------------------------
>>   1 file changed, 25 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
>> index 98becb7a7453..ff72bcfb0ff8 100644
>> --- a/drivers/nvme/host/sysfs.c
>> +++ b/drivers/nvme/host/sysfs.c
>> @@ -407,17 +407,6 @@ static ssize_t dctype_show(struct device *dev,
>>   static DEVICE_ATTR_RO(dctype);
>>     #ifdef CONFIG_NVME_AUTH
>> -static ssize_t nvme_ctrl_dhchap_secret_show(struct device *dev,
>> -        struct device_attribute *attr, char *buf)
>> -{
>> -    struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>> -    struct nvmf_ctrl_options *opts = ctrl->opts;
>> -
>> -    if (!opts->dhchap_secret)
>> -        return sysfs_emit(buf, "none\n");
>> -    return sysfs_emit(buf, "%s\n", opts->dhchap_secret);
>> -}
>> -
>>   static ssize_t nvme_dhchap_secret_store_common(struct nvme_ctrl *ctrl,
>>           const char *buf, size_t count, bool ctrl_secret)
>>   {
>> @@ -472,38 +461,32 @@ static ssize_t 
>> nvme_dhchap_secret_store_common(struct nvme_ctrl *ctrl,
>>       return count;
>>   }
>>   -static ssize_t nvme_ctrl_dhchap_secret_store(struct device *dev,
>> -        struct device_attribute *attr, const char *buf, size_t count)
>> -{
>> -    struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>> -
>> -    return nvme_dhchap_secret_store_common(ctrl, buf, count, false);
>> -}
>> -
>> -static DEVICE_ATTR(dhchap_secret, S_IRUGO | S_IWUSR,
>> -    nvme_ctrl_dhchap_secret_show, nvme_ctrl_dhchap_secret_store);
>> -
>> -static ssize_t nvme_ctrl_dhchap_ctrl_secret_show(struct device *dev,
>> -        struct device_attribute *attr, char *buf)
>> -{
>> -    struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>> -    struct nvmf_ctrl_options *opts = ctrl->opts;
>> -
>> -    if (!opts->dhchap_ctrl_secret)
>> -        return sysfs_emit(buf, "none\n");
>> -    return sysfs_emit(buf, "%s\n", opts->dhchap_ctrl_secret);
>> -}
>> -
>> -static ssize_t nvme_ctrl_dhchap_ctrl_secret_store(struct device *dev,
>> -        struct device_attribute *attr, const char *buf, size_t count)
>> -{
>> -    struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>> -
>> -    return nvme_dhchap_secret_store_common(ctrl, buf, count, false);
>> -}
>> +#define NVME_AUTH_DEVICE_ATTR(NAME, cs)                    \
>> +static ssize_t nvme_ctrl_##NAME##_show(struct device *dev,        \
>> +        struct device_attribute *attr, char *buf)        \
>> +{                                    \
>> +    struct nvme_ctrl *ctrl = dev_get_drvdata(dev);            \
>> +    struct nvmf_ctrl_options *opts = ctrl->opts; \
>> +                                    \
>> +    if (!opts->NAME)                        \
>> +        return sysfs_emit(buf, "none\n");            \
>> +    return sysfs_emit(buf, "%s\n", opts->NAME);            \
>> +}                                    \
>> +                                    \
>> +static ssize_t nvme_ctrl_##NAME##_store(struct device *dev,        \
>> +        struct device_attribute *attr, const char *buf, \
>> +        size_t count)                        \
>> +{                                    \
>> +    struct nvme_ctrl *ctrl = dev_get_drvdata(dev);            \
>> +                                    \
>> +    return nvme_dhchap_secret_store_common(ctrl, buf, count, cs);    \
>> +}                                    \
>> +                                    \
>> +static DEVICE_ATTR(NAME, S_IRUGO | S_IWUSR,                \
>> +    nvme_ctrl_##NAME##_show, nvme_ctrl_##NAME##_store); \
>>   -static DEVICE_ATTR(dhchap_ctrl_secret, S_IRUGO | S_IWUSR,
>> -    nvme_ctrl_dhchap_ctrl_secret_show, 
>> nvme_ctrl_dhchap_ctrl_secret_store);
>> +NVME_AUTH_DEVICE_ATTR(dhchap_secret, false);
>> +NVME_AUTH_DEVICE_ATTR(dhchap_ctrl_secret, true);
>>   #endif
>>     static struct attribute *nvme_dev_attrs[] = {
>
> Is it really worthwhile?
> These are just two sysfs attributes which are generalized that way, 
> and it's adding quite some boilerplate code to make it work.
>
> Plus I'll plan to rewrite authentication key handling to leverage 
> in-kernel keyrings once the TLS stuff has been accepted.
>
> So I'd rather not have this one. But might be convinced if someone has 
> a more compelling reason.
>
> Cheers,
>
> Hannes

let's drop this then, and merge first one ...

-ck