On 2020-04-11 20:32, James Smart wrote:
> + return EFC_SUCCESS;
> +}
Redefining 0 is unusual in the Linux kernel. I prefer to see "return 0;"
instead of "return ${DRIVER_NAME}_SUCCESS;".
> +static int efct_lio_tgt_session_data(struct efct *efct, u64 wwpn,
> + char *buf, int size)
> +{
> + struct efc_sli_port *sport = NULL;
> + struct efc_node *node = NULL;
> + struct efc *efc = efct->efcport;
> + u16 loop_id = 0;
> + int off = 0, rc = 0;
> +
> + if (!efc->domain) {
> + efc_log_err(efct, "failed to find efct/domain\n");
> + return EFC_FAIL;
> + }
> +
> + list_for_each_entry(sport, &efc->domain->sport_list, list_entry) {
> + if (sport->wwpn != wwpn)
> + continue;
> + list_for_each_entry(node, &sport->node_list,
> + list_entry) {
> + /* Dump only remote NPORT sessions */
> + if (!efct_lio_node_is_initiator(node))
> + continue;
> +
> + rc = snprintf(buf + off, size - off,
> + "0x%016llx,0x%08x,0x%04x\n",
> + get_unaligned_be64(node->wwpn),
> + node->rnode.fc_id, loop_id);
> + if (rc < 0)
> + break;
> + off += rc;
> + }
> + }
> +
> + return EFC_SUCCESS;
> +}
This is one of the most unfriendly debugfs data formats I have seen so
far: information about all sessions is dumped into one huge debugfs
attribute.
Is information about active sessions useful for other LIO target
drivers? Wasn't it promised that this functionality would be moved into
the LIO core instead of defining it for the efct driver only?
> +static int efct_debugfs_session_open(struct inode *inode, struct file *filp)
> +{
> + struct efct_lio_sport *sport = inode->i_private;
> + int size = 17 * PAGE_SIZE; /* 34 byte per session*2048 sessions */
> +
> + if (!(filp->f_mode & FMODE_READ)) {
> + filp->private_data = sport;
> + return EFC_SUCCESS;
> + }
> +
> + filp->private_data = kmalloc(size, GFP_KERNEL);
> + if (!filp->private_data)
> + return -ENOMEM;
> +
> + memset(filp->private_data, 0, size);
> + efct_lio_tgt_session_data(sport->efct, sport->wwpn, filp->private_data,
> + size);
> + return EFC_SUCCESS;
> +}
kmalloc() + memset() can be changed into kzalloc().
The above code allocates 68 KB physically contiguous memory? Kernel code
should not rely on higher order page allocations unless there is no
other choice.
Additionally, I see that the amount of memory allocated is independent
of the number of sessions. I think there are better approaches.
> +
> + lio_wq = create_singlethread_workqueue("efct_lio_worker");
> + if (!lio_wq) {
> + efc_log_err(efct, "workqueue create failed\n");
> + return -ENOMEM;
> + }
Is any work queued onto lio_wq that needs to be serialized against other
work queued onto the same queue? If not, can one of the system
workqueues be used, e.g. system_wq?
Thanks,
Bart.
