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
