> > +struct mpi3mr_ioc {
> > + struct list_head list;
> > + struct pci_dev *pdev;
> > + struct Scsi_Host *shost;
> > + u8 id;
> > + int cpu_count;
> > +
> > + char name[MPI3MR_NAME_LENGTH];
> > + char driver_name[MPI3MR_NAME_LENGTH];
> > +
> > + Mpi3SysIfRegs_t __iomem *sysif_regs;
> > + resource_size_t sysif_regs_phys;
> > + int bars;
> > + u64 dma_mask;
> > +
> > + u16 msix_count;
> > + u8 intr_enabled;
> > +
> > + u16 num_admin_req;
> > + u32 admin_req_q_sz;
> > + u16 admin_req_pi;
> > + u16 admin_req_ci;
> > + void *admin_req_base;
> > + dma_addr_t admin_req_dma;
> > + spinlock_t admin_req_lock;
> > +
> > + u16 num_admin_replies;
> > + u32 admin_reply_q_sz;
> > + u16 admin_reply_ci;
> > + u8 admin_reply_ephase;
> > + void *admin_reply_base;
> > + dma_addr_t admin_reply_dma;
> > +
> > + u32 ready_timeout;
> > +
> > + struct mpi3mr_intr_info *intr_info;
>
> Please, be consistent.
> If you must introduce typedefs for your internal structures, okay.
> But then introduce typedefs for _all_ internal structures.
> Or leave the typedefs and just use 'struct XXX'; which actually is the
> recommended way for linux.
Are you referring " typedef struct mpi3mr_drv_" ?. This is because of some
inter-operability issue of different kernel version. I will remove this
typedef in my V2.
Usually, our goal is not to have typedef in drivers except mpi3.0 header
files. I will scan such instances in and will update all the places.
>
> > + u16 intr_info_count;
> > +
> > + u16 num_queues;
> > + u16 num_op_req_q;
> > + struct op_req_qinfo *req_qinfo;
> > +
> > + u16 num_op_reply_q;
> > + struct op_reply_qinfo *op_reply_qinfo;
> > +
> > + struct mpi3mr_drv_cmd init_cmds;
> > + struct mpi3mr_ioc_facts facts;
> > + u16 op_reply_desc_sz;
> > +
> > + u32 num_reply_bufs;
> > + struct dma_pool *reply_buf_pool;
> > + u8 *reply_buf;
> > + dma_addr_t reply_buf_dma;
> > + dma_addr_t reply_buf_dma_max_address;
> > +
> > + u16 reply_free_qsz;
> > + struct dma_pool *reply_free_q_pool;
> > + U64 *reply_free_q;
> > + dma_addr_t reply_free_q_dma;
> > + spinlock_t reply_free_queue_lock;
> > + u32 reply_free_queue_host_index;
> > +
> > + u32 num_sense_bufs;
> > + struct dma_pool *sense_buf_pool;
> > + u8 *sense_buf;
> > + dma_addr_t sense_buf_dma;
> > +
> > + u16 sense_buf_q_sz;
> > + struct dma_pool *sense_buf_q_pool;
> > + U64 *sense_buf_q;
> > + dma_addr_t sense_buf_q_dma;
> > + spinlock_t sbq_lock;
> > + u32 sbq_host_index;
> > +
> > + u8 is_driver_loading;
> > +
> > + u16 max_host_ios;
> > +
> > + u32 chain_buf_count;
> > + struct dma_pool *chain_buf_pool;
> > + struct chain_element *chain_sgl_list;
> > + u16 chain_bitmap_sz;
> > + void *chain_bitmap;
> > +
> > + u8 reset_in_progress;
> > + u8 unrecoverable;
> > +
> > + int logging_level;
> > +
> > + struct mpi3mr_fwevt *current_event;
> > + Mpi3DriverInfoLayout_t driver_info;
>
> See my comment about struct typedefs above.
I will remove this typedef and similar instances.
> > +static inline int mpi3mr_request_irq(struct mpi3mr_ioc *mrioc, u16
index)
> > +{
> > + struct pci_dev *pdev = mrioc->pdev;
> > + struct mpi3mr_intr_info *intr_info = mrioc->intr_info + index;
> > + int retval = 0;
> > +
> > + intr_info->mrioc = mrioc;
> > + intr_info->msix_index = index;
> > + intr_info->op_reply_q = NULL;
> > +
> > + snprintf(intr_info->name, MPI3MR_NAME_LENGTH, "%s%d-msix%d",
> > + mrioc->driver_name, mrioc->id, index);
> > +
> > + retval = request_threaded_irq(pci_irq_vector(pdev, index),
> mpi3mr_isr,
> > + mpi3mr_isr_poll, IRQF_ONESHOT, intr_info->name, intr_info);
> > + if (retval) {
> > + ioc_err(mrioc, "%s: Unable to allocate interrupt %d!\n",
> > + intr_info->name, pci_irq_vector(pdev, index));
> > + return retval;
> > + }
> > +
>
> The point of having 'mpi3mr_isr_poll()' here is what exactly?
This is a place holder and actual use case is handled in " [17/24] mpi3mr:
add support of threaded isr"
For easy review, I have created separate patch " [17/24] mpi3mr: add
support of threaded isr"
> > + areq_entry = (u8 *)mrioc->admin_req_base +
> > + (areq_pi * MPI3MR_ADMIN_REQ_FRAME_SZ);
> > + memset(areq_entry, 0, MPI3MR_ADMIN_REQ_FRAME_SZ);
> > + memcpy(areq_entry, (u8 *)admin_req, admin_req_sz);
> > +
> > + if (++areq_pi == max_entries)
> > + areq_pi = 0;
> > + mrioc->admin_req_pi = areq_pi;
> > +
> > + writel(mrioc->admin_req_pi, &mrioc->sysif_regs-
> >AdminRequestQueuePI);
> > +
> > +out:
> > + spin_unlock_irqrestore(&mrioc->admin_req_lock, flags);
> > +
> > + return retval;
> > +}
> > +
>
> It might be an idea to have an 'admin' queue structure; keeping the
> values all within the main IOC structure might cause cache misses and a
> degraded performance.
Noted your point. We can do it in future update. I think it make sense for
code readability as well.
> > +int mpi3mr_init_ioc(struct mpi3mr_ioc *mrioc)
> > +{
> > + int retval = 0;
> > + enum mpi3mr_iocstate ioc_state;
> > + u64 base_info;
> > + u32 timeout;
> > + u32 ioc_status, ioc_config;
> > + Mpi3IOCFactsData_t facts_data;
> > +
> > + mrioc->change_count = 0;
> > + mrioc->cpu_count = num_online_cpus();
>
> What about CPU hotplug?
We have to use num_available_cpus() to get benefit of cpu hotplug. In next
update it will be available.
> > +
> > +/* global driver scop variables */
> > +LIST_HEAD(mrioc_list);
> > +DEFINE_SPINLOCK(mrioc_list_lock);
> > +static int mrioc_ids;
> > +static int warn_non_secure_ctlr;
> > +
> > +MODULE_AUTHOR(MPI3MR_DRIVER_AUTHOR);
> > +MODULE_DESCRIPTION(MPI3MR_DRIVER_DESC);
> > +MODULE_LICENSE(MPI3MR_DRIVER_LICENSE);
> > +MODULE_VERSION(MPI3MR_DRIVER_VERSION);
> > +
> > +/* Module parameters*/
> > +int logging_level;
> > +module_param(logging_level, int, 0);
> > +MODULE_PARM_DESC(logging_level,
> > + " bits for enabling additional logging info (default=0)");
> > +
> > +
> > +/**
> > + * mpi3mr_map_queues - Map queues callback handler
> > + * @shost: SCSI host reference
> > + *
> > + * Call the blk_mq_pci_map_queues with from which operational
> > + * queue the mapping has to be done
> > + *
> > + * Return: return of blk_mq_pci_map_queues
> > + */
> > +static int mpi3mr_map_queues(struct Scsi_Host *shost)
> > +{
> > + struct mpi3mr_ioc *mrioc = shost_priv(shost);
> > +
> > + return blk_mq_pci_map_queues(&shost-
> >tag_set.map[HCTX_TYPE_DEFAULT],
> > + mrioc->pdev, 0);
> > +}
> > +
>
> What happened to polling?
> You did some patches for megaraid_sas, so I would have expected them to
> be here, too ...
Internally, Io_uring iopoll is also completed for this driver as well, but
it is under testing and may be available in next update.
> > +module_init(mpi3mr_init);
> > +module_exit(mpi3mr_exit);
> >
> Cheers,
Hannes -
Thanks for the feedback. I am working on all the comments and soon I will
be posting V2.
Kashyap
>
> Hannes
> --
> Dr. Hannes Reinecke Kernel Storage Architect
> hare@xxxxxxx +49 911 74053 688
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
> HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
