Re: [dpdk-dev] [PATCH 08/39] event/octeontx: add mailbox support

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

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>
> 
> >  +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>
> 
> >  +
> >  +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;
	}


> 
> >  +
> >  +	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>
> 
> >  +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"
>