RE: [PATCH 02/24] mpi3mr: base driver code

[Kashyap Desai <kashyap.desai@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 7260 bytes --]

> > +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@suse.de			                  +49 911 74053 688
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
> HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]