On Thu, Mar 23, 2017 at 04:46:07PM +0000, Eads, Gage wrote:

Hi Jerin,

Thanks Gage for the review.

I identified a few issues below.

Thanks,
Gage

<snip>

quoted

 +static inline void
 +mbox_send_requeust(struct mbox *m, struct octeontx_mbox_hdr *hdr,
 +			const void *txmsg, uint16_t txsize)

"requeust" -> "request"

Will fix the typos in v2

<snip>

quoted

 +
 +static inline int
 +mbox_wait_response(struct mbox *m, struct octeontx_mbox_hdr *hdr,
 +			void *rxmsg, uint16_t rxsize)
 +{
 +	int res = 0, wait;
 +	uint16_t len;
 +	struct mbox_ram_hdr rx_hdr;
 +	uint64_t *ram_mbox_hdr = (uint64_t *)m->ram_mbox_base;
 +	uint8_t *ram_mbox_msg = m->ram_mbox_base + sizeof(struct
 +mbox_ram_hdr);
 +
 +	/* Wait for response */
 +	wait = MBOX_WAIT_TIME;
 +	while (wait > 0) {
 +		rte_delay_us(100);
 +		rx_hdr.u64 = rte_read64(ram_mbox_hdr);
 +		if (rx_hdr.chan_state == MBOX_CHAN_STATE_RES)
 +			break;
 +		wait -= 10;
 +	}

'wait' is in units of milliseconds ("Mbox operation timeout in milliseconds"), so the function subtracts 10 ms after spinning for 100 us. Is that intentional?

No. its unintentional. Thanks for pointing it out. I will fix it by

#define MBOX_WAIT_TIME_SEC 3
	wait = MBOX_WAIT_TIME_SEC * 1000 * 10;
	while (wait > 0) {
		rte_delay_us(100);

		wait -= 1;
	}

quoted

 +
 +	hdr->res_code = rx_hdr.res_code;
 +	m->tag_own++;
 +
 +	/* Tag mismatch */
 +	if (m->tag_own != rx_hdr.tag) {
 +		res = -EBADR;
 +		goto error;
 +	}
 +
 +	/* PF nacked the msg */
 +	if (rx_hdr.res_code != MBOX_RET_SUCCESS) {
 +		res = -EBADMSG;
 +		goto error;
 +	}
 +
 +	/* Timeout */
 +	if (wait <= 0) {
 +		res = -ETIMEDOUT;
 +		goto error;
 +	}

Will a timeout mean rx_hdr is invalid? If so, the timeout error should be checked before checking for a PF nack or tag mismatch.

rx_hdr is valid. But No harm in moving timeout to first. I will move the
timeout to first as you suggested.
 

<snip>

quoted

 +static inline int
 +mbox_send(struct mbox *m, struct octeontx_mbox_hdr *hdr, const void
 *txmsg,
 +		uint16_t txsize, void *rxmsg, uint16_t rxsize) {
 +	int res = -EINVAL;
 +
 +	if (m->init_once == 0 || hdr == NULL ||
 +		txsize > MAX_RAM_MBOX_LEN || rxsize >
 MAX_RAM_MBOX_LEN) {
 +		ssovf_log_err("Invalid init_once=%d hdr=%p txsz=%d rxsz=%d",
 +				m->init_once, hdr, txsize, rxsize);
 +		return res;
 +	}
 +
 +	rte_spinlock_lock(&m->lock);
 +
 +	mbox_send_requeust(m, hdr, txmsg, txsize);

"requeust" -> "request"