RE: [net-next v3 06/15] iecm: Implement mailbox functionality

["Brady, Alan" <alan.brady@intel.com>]

> -----Original Message-----
> From: Joe Perches <joe@perches.com>
> Sent: Thursday, June 25, 2020 7:58 PM
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; davem@davemloft.net
> Cc: Michael, Alice <alice.michael@intel.com>; netdev@vger.kernel.org;
> nhorman@redhat.com; sassmann@redhat.com; Brady, Alan
> <alan.brady@intel.com>; Burra, Phani R <phani.r.burra@intel.com>; Hay,
> Joshua A <joshua.a.hay@intel.com>; Chittim, Madhu
> <madhu.chittim@intel.com>; Linga, Pavan Kumar
> <pavan.kumar.linga@intel.com>; Skidmore, Donald C
> <donald.c.skidmore@intel.com>; Brandeburg, Jesse
> <jesse.brandeburg@intel.com>; Samudrala, Sridhar
> <sridhar.samudrala@intel.com>
> Subject: Re: [net-next v3 06/15] iecm: Implement mailbox functionality
> 
> On Thu, 2020-06-25 at 19:07 -0700, Jeff Kirsher wrote:
> > From: Alice Michael <alice.michael@intel.com>
> >
> > Implement mailbox setup, take down, and commands.
> []
> > diff --git a/drivers/net/ethernet/intel/iecm/iecm_controlq.c
> > b/drivers/net/ethernet/intel/iecm/iecm_controlq.c
> []
> > @@ -73,7 +142,74 @@ enum iecm_status iecm_ctlq_add(struct iecm_hw
> *hw,
> >  			       struct iecm_ctlq_create_info *qinfo,
> >  			       struct iecm_ctlq_info **cq)
> 
> Multiple functions using **cp and *cp can be error prone.
> 

We can see how this can be confusing.  Would it be acceptable/sufficient to change function parameter name to **cq_arr or **cq_list?

> >  {
> > -	/* stub */
> > +	enum iecm_status status = 0;
> > +	bool is_rxq = false;
> > +
> > +	if (!qinfo->len || !qinfo->buf_size ||
> > +	    qinfo->len > IECM_CTLQ_MAX_RING_SIZE ||
> > +	    qinfo->buf_size > IECM_CTLQ_MAX_BUF_LEN)
> > +		return IECM_ERR_CFG;
> > +
> > +	*cq = kcalloc(1, sizeof(struct iecm_ctlq_info), GFP_KERNEL);
> > +	if (!(*cq))
> > +		return IECM_ERR_NO_MEMORY;
> 
> pity there is an iecm_status and not just a generic -ENOMEM.
> Is there much value in the separate enum?

For controlq interactions we don't have much choice because that API is OS agnostic and we need some common set of error codes to fall back to.  This is similar to how other Intel NIC drivers have AQ error codes.

> []
> @@ -152,7 +388,58 @@ enum iecm_status iecm_ctlq_clean_sq(struct
> iecm_ctlq_info *cq,
> >  				    u16 *clean_count,
> >  				    struct iecm_ctlq_msg *msg_status[])  {
> > -	/* stub */
> > +	enum iecm_status ret = 0;
> > +	struct iecm_ctlq_desc *desc;
> > +	u16 i = 0, num_to_clean;
> > +	u16 ntc, desc_err;
> > +
> > +	if (!cq || !cq->ring_size)
> > +		return IECM_ERR_CTLQ_EMPTY;
> > +
> > +	if (*clean_count == 0)
> > +		return 0;
> > +	else if (*clean_count > cq->ring_size)
> > +		return IECM_ERR_PARAM;
> 
> unnecessary else

Will fix.

> 
> > +
> > +	mutex_lock(&cq->cq_lock);
> > +
> > +	ntc = cq->next_to_clean;
> > +
> > +	num_to_clean = *clean_count;
> > +
> > +	for (i = 0; i < num_to_clean; i++) {
> > +		/* Fetch next descriptor and check if marked as done */
> > +		desc = IECM_CTLQ_DESC(cq, ntc);
> > +		if (!(le16_to_cpu(desc->flags) & IECM_CTLQ_FLAG_DD))
> > +			break;
> > +
> > +		desc_err = le16_to_cpu(desc->ret_val);
> > +		if (desc_err) {
> > +			/* strip off FW internal code */
> > +			desc_err &= 0xff;
> > +		}
> > +
> > +		msg_status[i] = cq->bi.tx_msg[ntc];
> > +		msg_status[i]->status = desc_err;
> > +
> > +		cq->bi.tx_msg[ntc] = NULL;
> > +
> > +		/* Zero out any stale data */
> > +		memset(desc, 0, sizeof(*desc));
> > +
> > +		ntc++;
> > +		if (ntc == cq->ring_size)
> > +			ntc = 0;
> > +	}
> > +
> > +	cq->next_to_clean = ntc;
> > +
> > +	mutex_unlock(&cq->cq_lock);
> > +
> > +	/* Return number of descriptors actually cleaned */
> > +	*clean_count = i;
> > +
> > +	return ret;
> >  }
> >
> >  /**
> > @@ -175,7 +462,111 @@ enum iecm_status iecm_ctlq_post_rx_buffs(struct
> iecm_hw *hw,
> >  					 u16 *buff_count,
> >  					 struct iecm_dma_mem **buffs)
> >  {
> > -	/* stub */
> > +	enum iecm_status status = 0;
> > +	struct iecm_ctlq_desc *desc;
> > +	u16 ntp = cq->next_to_post;
> > +	bool buffs_avail = false;
> > +	u16 tbp = ntp + 1;
> > +	int i = 0;
> > +
> > +	if (*buff_count > cq->ring_size)
> > +		return IECM_ERR_PARAM;
> > +
> > +	if (*buff_count > 0)
> > +		buffs_avail = true;
> > +
> > +	mutex_lock(&cq->cq_lock);
> > +
> > +	if (tbp >= cq->ring_size)
> > +		tbp = 0;
> > +
> > +	if (tbp == cq->next_to_clean)
> > +		/* Nothing to do */
> > +		goto post_buffs_out;
> > +
> > +	/* Post buffers for as many as provided or up until the last one used */
> > +	while (ntp != cq->next_to_clean) {
> > +		desc = IECM_CTLQ_DESC(cq, ntp);
> > +
> > +		if (!cq->bi.rx_buff[ntp]) {
> 
> 		if (cq->bi.rx_buff[ntp])
> 			continue;
> 
> and unindent?

Yeah this is weird, not sure how this got indented like that.  Will fix.

> 
> > +			if (!buffs_avail) {
> > +				/* If the caller hasn't given us any buffers or
> > +				 * there are none left, search the ring itself
> > +				 * for an available buffer to move to this
> > +				 * entry starting at the next entry in the ring
> > +				 */
> > +				tbp = ntp + 1;
> > +
> > +				/* Wrap ring if necessary */
> > +				if (tbp >= cq->ring_size)
> > +					tbp = 0;
> > +
> > +				while (tbp != cq->next_to_clean) {
> > +					if (cq->bi.rx_buff[tbp]) {
> > +						cq->bi.rx_buff[ntp] =
> > +							cq->bi.rx_buff[tbp];
> > +						cq->bi.rx_buff[tbp] = NULL;
> > +
> > +						/* Found a buffer, no need to
> > +						 * search anymore
> > +						 */
> > +						break;
> > +					}
> > +
> > +					/* Wrap ring if necessary */
> > +					tbp++;
> > +					if (tbp >= cq->ring_size)
> > +						tbp = 0;
> > +				}
> > +
> > +				if (tbp == cq->next_to_clean)
> > +					goto post_buffs_out;
> > +			} else {
> > +				/* Give back pointer to DMA buffer */
> > +				cq->bi.rx_buff[ntp] = buffs[i];
> > +				i++;
> > +
> > +				if (i >= *buff_count)
> > +					buffs_avail = false;
> > +			}
> > +		}
> > +
> > +		desc->flags =
> > +			cpu_to_le16(IECM_CTLQ_FLAG_BUF |
> IECM_CTLQ_FLAG_RD);
> > +
> > +		/* Post buffers to descriptor */
> > +		desc->datalen = cpu_to_le16(cq->bi.rx_buff[ntp]->size);
> > +		desc->params.indirect.addr_high =
> > +			cpu_to_le32(IECM_HI_DWORD(cq->bi.rx_buff[ntp]-
> >pa));
> > +		desc->params.indirect.addr_low =
> > +			cpu_to_le32(IECM_LO_DWORD(cq->bi.rx_buff[ntp]-
> >pa));
> > +
> > +		ntp++;
> > +		if (ntp == cq->ring_size)
> > +			ntp = 0;
> > +	}
> 
> []
> 
> > @@ -186,7 +555,27 @@ iecm_wait_for_event(struct iecm_adapter *adapter,
> >  		    enum iecm_vport_vc_state state,
> >  		    enum iecm_vport_vc_state err_check)  {
> > -	/* stub */
> > +	enum iecm_status status;
> > +	int event;
> > +
> > +	event = wait_event_timeout(adapter->vchnl_wq,
> > +				   test_and_clear_bit(state, adapter->vc_state),
> > +				   msecs_to_jiffies(500));
> > +	if (event) {
> > +		if (test_and_clear_bit(err_check, adapter->vc_state)) {
> > +			dev_err(&adapter->pdev->dev,
> > +				"VC response error %d", err_check);
> 
> missing format terminating newlines

Oops, nice catch.  We'll see if we can grep for any other missing terminating newlines.  Will fix.

> 
> > +			status = IECM_ERR_CFG;
> > +		} else {
> > +			status = 0;
> > +		}
> > +	} else {
> > +		/* Timeout occurred */
> > +		status = IECM_ERR_NOT_READY;
> > +		dev_err(&adapter->pdev->dev, "VC timeout, state = %u", state);
> 
> here too
> 

Will fix. Thanks

Alan