Re: [dpdk-dev] [PATCH 2/8] app/testpmd: use PCI memory resource access APIs

[Xia, Chenbo]

Hi,

> -----Original Message-----
> From: Li, Xiaoyun <xiaoyun...@intel.com>
> Sent: Thursday, September 16, 2021 2:10 PM
> To: Xia, Chenbo <chenbo....@intel.com>; dev@dpdk.org
> Subject: RE: [PATCH 2/8] app/testpmd: use PCI memory resource access APIs
> 
> Hi
> 
> > -----Original Message-----
> > From: Xia, Chenbo <chenbo....@intel.com>
> > Sent: Friday, September 10, 2021 10:24
> > To: dev@dpdk.org
> > Cc: Li, Xiaoyun <xiaoyun...@intel.com>
> > Subject: [PATCH 2/8] app/testpmd: use PCI memory resource access APIs
> >
> > Currently testpmd uses struct rte_pci_device to access PCI memory resource.
> > Since this structure will be internal later, this patch replaces use of
> > rte_pci_device with new PCI memory resource access APIs to read/write BAR 0.
> >
> > Signed-off-by: Chenbo Xia <chenbo....@intel.com>
> > ---
> >  app/test-pmd/config.c  | 38 +++++++++++-----------------------
> >  app/test-pmd/testpmd.h | 46 +++++++++++++++++++++---------------------
> >  2 files changed, 35 insertions(+), 49 deletions(-)
> >
> <snip>
> >
> > @@ -1081,7 +1063,9 @@ port_reg_bit_set(portid_t port_id, uint32_t reg_off,
> > uint8_t bit_pos,
> >                     (int) bit_v);
> >             return;
> >     }
> > -   reg_v = port_id_pci_reg_read(port_id, reg_off);
> > +   if (port_id_pci_reg_read(port_id, reg_off, &reg_v))
> > +           return;
> > +
> >     if (bit_v == 0)
> >             reg_v &= ~(1 << bit_pos);
> >     else
> > @@ -1123,7 +1107,9 @@ port_reg_bit_field_set(portid_t port_id, uint32_t
> > reg_off,
> >                             (unsigned)max_v, (unsigned)max_v);
> >             return;
> >     }
> > -   reg_v = port_id_pci_reg_read(port_id, reg_off);
> > +   if (port_id_pci_reg_read(port_id, reg_off, &reg_v))
> > +           return;
> > +
> >     reg_v &= ~(max_v << l_bit); /* Keep unchanged bits */
> >     reg_v |= (value << l_bit); /* Set changed bits */
> >     port_id_pci_reg_write(port_id, reg_off, reg_v); diff --git a/app/test-
> 
> In your implementation, port_id_pci_reg_write() can fail. Then the following
> display shouldn't be called.
> So change this function to have return value and check it before use it will
> be better.

Make sense. Will fix in v2.

Thanks,
Chenbo

> 
> > pmd/testpmd.h b/app/test-pmd/testpmd.h index 16a3598e48..7922807a6e
> > 100644
> > --- a/app/test-pmd/testpmd.h
> > +++ b/app/test-pmd/testpmd.h
> <snip>
> >  static inline void
> > -port_pci_reg_write(struct rte_port *port, uint32_t reg_off, uint32_t reg_v)
> > +port_id_pci_reg_write(portid_t pt_id, uint32_t reg_off, uint32_t reg_v)
> >  {
> > -   const struct rte_pci_device *pci_dev;
> > +   struct rte_port *port = &ports[(pt_id)];
> > +   char name[RTE_ETH_NAME_MAX_LEN];
> >     const struct rte_bus *bus;
> > -   void *reg_addr;
> >
> >     if (!port->dev_info.device) {
> >             fprintf(stderr, "Invalid device\n");
> > @@ -721,19 +721,19 @@ port_pci_reg_write(struct rte_port *port, uint32_t
> > reg_off, uint32_t reg_v)
> >
> >     bus = rte_bus_find_by_device(port->dev_info.device);
> >     if (bus && !strcmp(bus->name, "pci")) {
> > -           pci_dev = RTE_DEV_TO_PCI(port->dev_info.device);
> > +           rte_eth_dev_get_name_by_port(pt_id, name);
> >     } else {
> >             fprintf(stderr, "Not a PCI device\n");
> >             return;
> >     }
> >
> > -   reg_addr = ((char *)pci_dev->mem_resource[0].addr + reg_off);
> > -   *((volatile uint32_t *)reg_addr) = rte_cpu_to_le_32(reg_v);
> > +   reg_v = rte_cpu_to_le_32(reg_v);
> > +   if (rte_pci_mem_wr32(name, 0, &reg_v, reg_off)) {
> > +           fprintf(stderr, "Failed to write register\n");
> > +           return;
> > +   }
> >  }
> >
> > -#define port_id_pci_reg_write(pt_id, reg_off, reg_value) \
> > -   port_pci_reg_write(&ports[(pt_id)], (reg_off), (reg_value))
> > -
> >  static inline void
> >  get_start_cycles(uint64_t *start_tsc)
> >  {
> > --
> > 2.17.1