Re: [dpdk-dev] [PATCH v3 2/2] common/octeontx2: add polling based response mbox message

[Gavin Hu <Gavin.Hu@arm.com>]

Hi Jerin,

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Tuesday, December 17, 2019 7:14 PM
> To: Gavin Hu <Gavin.Hu@arm.com>
> Cc: Sunil Kumar Kori <skori@marvell.com>; jerinj@marvell.com; Nithin
> Dabilpuram <ndabilpuram@marvell.com>; Vamsi Attunuru
> <vattunuru@marvell.com>; dev@dpdk.org; Harman Kalra
> <hkalra@marvell.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> Subject: Re: [dpdk-dev] [PATCH v3 2/2] common/octeontx2: add polling
> based response mbox message
> 
> > > +mbox_poll(struct otx2_mbox *mbox, uint32_t wait)
> > > +{
> > > +     uint32_t timeout = 0, sleep = 1;
> > > +     uint64_t rsp_reg = 0;
> > > +     uintptr_t reg_addr;
> > > +
> > > +     reg_addr = mbox->reg_base + mbox->intr_offset;
> > > +     while (!rsp_reg) {
> > The first iteration of (!rsp_reg) always evaluate to 'true'.
> > Why not use do .. while to save one iteration?
> > > +             rte_rmb();
> > Rte_rmb is overkill, how is reg_addr mapped(what is the memory attribute?
> WC? cacheable? )
> > If it is a PCI mmaped region, then rte_io_barrier is okay, I am proposing to
> relax the io barrier for aarch64 as a compiler barrier.
> >  http://patches.dpdk.org/patch/61662/
> 
> It is the device memory and Linux kernel(_CPU_ not IO) is updating the
> memory on the other side. I think,
> we would need rte_smp_rmb() here. Since it was slow-path code, added
> the full barrier here.
> I think it is OK to change rte_smp_rmb() not to rte_io_* version. Correct?
I want to summarize as follows:
rte_io_* should be used in this case as it is the device memory.
rte_smp_* is for normal memory, including DMA buffer resides in the normal memory and updated by devices.
Rte_cio_* should be used for ordering between normal memory and device memory, for example, updates to descriptors in the normal memory and a doorbell write.