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