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

[James Smart <jsmart2021@gmail.com>]

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