Re: [PATCH 24/32] elx: efct: LIO backend interface routines

James Smart
2019-10-28T17:49:39Z

Thank you Bart. We've gone through your comments and mostly agree with
them and will be making the corresponding changes.

For the few exceptions and where you had a couple of questios, see below.

-- james

On 10/24/2019 3:27 PM, Bart Van Assche wrote:
> Additionally, what is a "virtual target"?

The code meant virtual port (NPIV port). The comments will be updated.

>> +static ssize_t
>> +efct_lio_wwn_version_show(struct config_item *item, char *page)
>> +{
>> +    return sprintf(page, "Emulex EFCT fabric module version %s\n",
>> +               __stringify(EFCT_LIO_VERSION));
>> +}
>
> Version numbers are not useful in upstream code. Please remove this
> attribute and also the EFCT_LIO_VERSION constant.

From my time with lpfc, I disagree. It's true, if looking only at the
upstream kernel, version doesn't mean much. But when it comes to the
distros, who may cherry-pick a lot, it has certainly helped to have a
version string to get a general understanding of what's there. Granted
you must still look at the code for the actual content, but it's a good
indicator.

>> +static const struct file_operations efct_debugfs_session_fops = {
>> +    .owner        = THIS_MODULE,
>> +    .open        = efct_debugfs_session_open,
>> +    .release    = efct_debugfs_session_close,
>> +    .read        = efct_debugfs_session_read,
>> +    .write        = efct_debugfs_session_write,
>> +    .llseek        = default_llseek,
>> +};
>> +
>> +static const struct file_operations efct_npiv_debugfs_session_fops = {
>> +    .owner        = THIS_MODULE,
>> +    .open        = efct_npiv_debugfs_session_open,
>> +    .release    = efct_debugfs_session_close,
>> +    .read        = efct_debugfs_session_read,
>> +    .write        = efct_debugfs_session_write,
>> +    .llseek        = default_llseek,
>> +};
>
> Since the information that is exported through debugfs (logged in
> initiators) is information that is also useful for other target drivers,
> I think this functionality should be implemented in the target core
> instead of in this target driver.

Can you expand further on what you'd like to see and the format of the
data to be displayed ?

I'll see if it makes sense.

-- james