RE: [PATCH 04/24] mpi3mr: add support of queue command processing

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

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

> > +
> > +/**
> > + * struct mpi3mr_stgt_priv_data - SCSI device private structure
> > + *
> > + * @tgt_priv_data: Scsi_target private data pointer
> > + * @lun_id: LUN ID of the device
> > + * @ncq_prio_enable: NCQ priority enable for SATA device  */ struct
> > +mpi3mr_sdev_priv_data {
> > +	struct mpi3mr_stgt_priv_data *tgt_priv_data;
> > +	u32 lun_id;
> > +	u8 ncq_prio_enable;
> > +};
> >
> Why is there a separate 'lun_id' field?
H/W is multi lun capable and driver store lun_id so that it can pass the
same to the FW in qcmd() and other path like task management etc.

>
> >   typedef struct mpi3mr_drv_cmd DRV_CMD;
> >   typedef void (*DRV_CMD_CALLBACK)(struct mpi3mr_ioc *mrioc, @@
> > -443,12 +481,16 @@ struct scmd_priv {
> >    * @sbq_lock: Sense buffer queue lock
> >    * @sbq_host_index: Sense buffer queuehost index
> >    * @is_driver_loading: Is driver still loading
> > + * @scan_started: Async scan started
> > + * @scan_failed: Asycn scan failed
> > + * @stop_drv_processing: Stop all command processing
> >    * @max_host_ios: Maximum host I/O count
> >    * @chain_buf_count: Chain buffer count
> >    * @chain_buf_pool: Chain buffer pool
> >    * @chain_sgl_list: Chain SGL list
> >    * @chain_bitmap_sz: Chain buffer allocator bitmap size
> >    * @chain_bitmap: Chain buffer allocator bitmap
> > + * @chain_buf_lock: Chain buffer list lock
> >    * @reset_in_progress: Reset in progress flag
> >    * @unrecoverable: Controller unrecoverable flag
> >    * @logging_level: Controller debug logging level @@ -533,6 +575,9
> > @@ struct mpi3mr_ioc {
> >   	u32 sbq_host_index;
> >
> >   	u8 is_driver_loading;
> > +	u8 scan_started;
> > +	u16 scan_failed;
> > +	u8 stop_drv_processing;
> >
> >   	u16 max_host_ios;
> >
> > @@ -541,6 +586,7 @@ struct mpi3mr_ioc {
> >   	struct chain_element *chain_sgl_list;
> >   	u16  chain_bitmap_sz;
> >   	void *chain_bitmap;
> > +	spinlock_t chain_buf_lock;
> >
> >   	u8 reset_in_progress;
> >   	u8 unrecoverable;
> > @@ -557,8 +603,11 @@ int mpi3mr_setup_resources(struct mpi3mr_ioc
> *mrioc);
> >   void mpi3mr_cleanup_resources(struct mpi3mr_ioc *mrioc);
> >   int mpi3mr_init_ioc(struct mpi3mr_ioc *mrioc);
> >   void mpi3mr_cleanup_ioc(struct mpi3mr_ioc *mrioc);
> > +int mpi3mr_issue_port_enable(struct mpi3mr_ioc *mrioc, u8 async);
> >   int mpi3mr_admin_request_post(struct mpi3mr_ioc *mrioc, void
> *admin_req,
> >   u16 admin_req_sz, u8 ignore_reset);
> > +int mpi3mr_op_request_post(struct mpi3mr_ioc *mrioc,
> > +			   struct op_req_qinfo *opreqq, u8 *req);
> >   void mpi3mr_add_sg_single(void *paddr, u8 flags, u32 length,
> >   			  dma_addr_t dma_addr);
> >   void mpi3mr_build_zero_len_sge(void *paddr); @@ -569,6 +618,9 @@
> > void *mpi3mr_get_reply_virt_addr(struct mpi3mr_ioc *mrioc,
> >   void mpi3mr_repost_sense_buf(struct mpi3mr_ioc *mrioc,
> >   				     u64 sense_buf_dma);
> >
> > +void mpi3mr_process_op_reply_desc(struct mpi3mr_ioc *mrioc,
> > +				  Mpi3DefaultReplyDescriptor_t
*reply_desc,
> > +				  u64 *reply_dma, u16 qidx);
> >   void mpi3mr_start_watchdog(struct mpi3mr_ioc *mrioc);
> >   void mpi3mr_stop_watchdog(struct mpi3mr_ioc *mrioc);
> >
> > diff --git a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> > b/drivers/scsi/mpi3mr/mpi3mr_fw.c index 6fb28983038e..abdf8c653e6b
> > 100644
> > --- a/drivers/scsi/mpi3mr/mpi3mr_fw.c
> > +++ b/drivers/scsi/mpi3mr/mpi3mr_fw.c
> > @@ -24,6 +24,23 @@ static inline void mpi3mr_writeq(__u64 b, volatile
> void __iomem *addr)
> >   }
> >   #endif
> >
> > +static inline bool
> > +mpi3mr_check_req_qfull(struct op_req_qinfo *op_req_q) {
> > +	u16 pi, ci, max_entries;
> > +	bool is_qfull = false;
> > +
> > +	pi = op_req_q->pi;
> > +	ci = op_req_q->ci;
> > +	max_entries = op_req_q->num_requests;
> > +
> > +	if ((ci == (pi + 1)) || ((!ci) && (pi == (max_entries - 1))))
> > +		is_qfull = true;
> > +
> > +	return is_qfull;
> > +}
> > + > +
>
> This needs to be protected by a lock to ensure the 'ci' and 'pi' values
won't
> change during processing; alternatively you'd need to use
> READ_ONCE() here.

I will add READ_ONCE() for op_req_q->ci.

> Please modify or add respective lockdep annotations.
I will discuss with you.

>
> >   static void mpi3mr_sync_irqs(struct mpi3mr_ioc *mrioc)
> >   {
> >   	u16 i, max_vectors;
> > @@ -282,6 +299,87 @@ static int mpi3mr_process_admin_reply_q(struct
> mpi3mr_ioc *mrioc)
> >   	return num_admin_replies;
> >   }
> >
> > +/**
> > + * mpi3mr_get_reply_desc - get reply descriptor frame corresponding
to
> > + *	queue's consumer index from operational reply descriptor queue.
> > + * @op_reply_q: op_reply_qinfo object
> > + * @reply_ci: operational reply descriptor's queue consumer index
> > + *
> > + * Returns reply descriptor frame address  */ static inline
> > +Mpi3DefaultReplyDescriptor_t * mpi3mr_get_reply_desc(struct
> > +op_reply_qinfo *op_reply_q, u32 reply_ci) {
> > +	void *segment_base_addr;
> > +	struct segments *segments = op_reply_q->q_segments;
> > +	Mpi3DefaultReplyDescriptor_t *reply_desc = NULL;
> > +
> > +	segment_base_addr =
> > +	    segments[reply_ci / op_reply_q->segment_qd].segment;
> > +	reply_desc = (Mpi3DefaultReplyDescriptor_t *)segment_base_addr +
> > +	    (reply_ci % op_reply_q->segment_qd);
> > +	return reply_desc;
> > +}
> > +
> > +
> > +static int mpi3mr_process_op_reply_q(struct mpi3mr_ioc *mrioc,
> > +	struct mpi3mr_intr_info *intr_info)
> > +{
> > +	struct op_reply_qinfo *op_reply_q = intr_info->op_reply_q;
> > +	struct op_req_qinfo *op_req_q;
> > +	u32 exp_phase;
> > +	u32 reply_ci;
> > +	u32 num_op_reply = 0;
> > +	u64 reply_dma = 0;
> > +	Mpi3DefaultReplyDescriptor_t *reply_desc;
> > +	u16 req_q_idx = 0, reply_qidx;
> > +
> > +	reply_qidx = op_reply_q->qid - 1;
> > +
> > +	exp_phase = op_reply_q->ephase;
> > +	reply_ci = op_reply_q->ci;
> > +
> > +	reply_desc = mpi3mr_get_reply_desc(op_reply_q, reply_ci);
> > +	if ((le16_to_cpu(reply_desc->ReplyFlags) &
> > +	    MPI3_REPLY_DESCRIPT_FLAGS_PHASE_MASK) != exp_phase) {
> > +		return 0;
> > +	}
> > +
> > +	do {
> > +		req_q_idx = le16_to_cpu(reply_desc->RequestQueueID) - 1;
> > +		op_req_q = &mrioc->req_qinfo[req_q_idx];
> > +
> > +		op_req_q->ci =
> > +		    le16_to_cpu(reply_desc->RequestQueueCI);
> > +
>
> This introduces an interlock with request processing; from what I can
see
> request processing is guarded by a spinlock, yet I don't see any
protection
> here.
As of now there is only one path to process reply queue (from interrupt).
Going forward, blk_mq_poll interface in this driver require interlock in
this area.

> How do you ensure a timely update of the 'ci' value if there is parallel
> execution of requests and responses (note: you cannot always assume that
> you will have a 1:1 relationship between queuepairs, CPUs, and
interrupts).
> I would have expected at least a WRITE_ONCE() here.

I will add WRITE_ONCE.
>
> Question is why you need to modify this at all; in most implementations
I
> know of the 'ci' index is modified by firmware, as really it's the
firmware
> consuming the request.
> Why not here?
Each request queue need separate memory space of queue_depth for that
particular request queue. Currently we are restricting 256 queue depth for
each request queue to get better memory utilization. If we allocate each
request queue same as can_queue, we may not have require this queue_full
check (there may be some corner case, but we can work on those.).  "ci" on
request queue is updated by FW only, but driver is reading those value to
know if there are enough space to post more request. If we see frequent
queue_full with 256 queue depth, we will increase the queue_depth value in
future.

~ Kashyap

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