Re: [PATCH v4] bus: fix inconsistent representation of PCI numbers

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 5 Feb 2025 20:16:48 +0000
Shani Peretz <shperetz@nvidia.com> wrote:

> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Wednesday, 5 February 2025 20:46
> > To: Shani Peretz <shperetz@nvidia.com>
> > Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon (EXTERNAL)
> > <thomas@monjalon.net>; Tyler Retzlaff <roretzla@linux.microsoft.com>; Parav
> > Pandit <parav@nvidia.com>; Xueming Li <xuemingl@nvidia.com>; Nipun Gupta
> > <nipun.gupta@amd.com>; Nikhil Agarwal <nikhil.agarwal@amd.com>; Hemant
> > Agrawal <hemant.agrawal@nxp.com>; Sachin Saxena
> > <sachin.saxena@nxp.com>; Rosen Xu <rosen.xu@intel.com>; Chenbo Xia
> > <chenbox@nvidia.com>; Tomasz Duszynski <tduszynski@marvell.com>;
> > Chengwen Feng <fengchengwen@huawei.com>; NBU-Contact-longli
> > (EXTERNAL) <longli@microsoft.com>; Wei Hu <weh@microsoft.com>; Bruce
> > Richardson <bruce.richardson@intel.com>; Kevin Laatz
> > <kevin.laatz@intel.com>; Jan Blunck <jblunck@infradead.org>
> > Subject: Re: [PATCH v4] bus: fix inconsistent representation of PCI numbers
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Wed, 5 Feb 2025 17:37:54 +0000
> > Shani Peretz <shperetz@nvidia.com> wrote:
> >   
> > > > -----Original Message-----
> > > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > > Sent: Wednesday, 5 February 2025 18:42
> > > > To: Shani Peretz <shperetz@nvidia.com>
> > > > Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon (EXTERNAL)
> > > > <thomas@monjalon.net>; Tyler Retzlaff
> > > > <roretzla@linux.microsoft.com>; Parav Pandit <parav@nvidia.com>;
> > > > Xueming Li <xuemingl@nvidia.com>; Nipun Gupta  
> > <nipun.gupta@amd.com>;  
> > > > Nikhil Agarwal <nikhil.agarwal@amd.com>; Hemant Agrawal
> > > > <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>;
> > > > Rosen Xu <rosen.xu@intel.com>; Chenbo Xia <chenbox@nvidia.com>;
> > > > Tomasz Duszynski <tduszynski@marvell.com>; Chengwen Feng
> > > > <fengchengwen@huawei.com>; NBU-Contact-longli
> > > > (EXTERNAL) <longli@microsoft.com>; Wei Hu <weh@microsoft.com>; Bruce
> > > > Richardson <bruce.richardson@intel.com>; Kevin Laatz
> > > > <kevin.laatz@intel.com>; Jan Blunck <jblunck@infradead.org>
> > > > Subject: Re: [PATCH v4] bus: fix inconsistent representation of PCI
> > > > numbers
> > > >
> > > > External email: Use caution opening links or attachments
> > > >
> > > >
> > > > On Wed, 5 Feb 2025 16:36:11 +0000
> > > > Shani Peretz <shperetz@nvidia.com> wrote:
> > > >  
> > > > > > -----Original Message-----
> > > > > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > > > > Sent: Wednesday, 29 January 2025 18:25
> > > > > > To: Shani Peretz <shperetz@nvidia.com>
> > > > > > Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon (EXTERNAL)
> > > > > > <thomas@monjalon.net>; Tyler Retzlaff
> > > > > > <roretzla@linux.microsoft.com>; Parav Pandit <parav@nvidia.com>;
> > > > > > Xueming Li <xuemingl@nvidia.com>; Nipun Gupta  
> > > > <nipun.gupta@amd.com>;  
> > > > > > Nikhil Agarwal <nikhil.agarwal@amd.com>; Hemant Agrawal
> > > > > > <hemant.agrawal@nxp.com>; Sachin Saxena <sachin.saxena@nxp.com>;
> > > > > > Rosen Xu <rosen.xu@intel.com>; Chenbo Xia <chenbox@nvidia.com>;
> > > > > > Tomasz Duszynski <tduszynski@marvell.com>; Chengwen Feng
> > > > > > <fengchengwen@huawei.com>; NBU-Contact-longli
> > > > > > (EXTERNAL) <longli@microsoft.com>; Wei Hu <weh@microsoft.com>;
> > > > > > Bruce Richardson <bruce.richardson@intel.com>; Kevin Laatz
> > > > > > <kevin.laatz@intel.com>; Jan Blunck <jblunck@infradead.org>
> > > > > > Subject: Re: [PATCH v4] bus: fix inconsistent representation of
> > > > > > PCI numbers
> > > > > >
> > > > > > External email: Use caution opening links or attachments
> > > > > >
> > > > > >
> > > > > > On Wed, 29 Jan 2025 10:54:16 +0200 Shani Peretz
> > > > > > <shperetz@nvidia.com> wrote:
> > > > > >  
> > > > > > > +create_pci_dev(const char *name) {
> > > > > > > +     int port_id;
> > > > > > > +     uint8_t slave_mac1[] = {0x00, 0xFF, 0x00, 0xFF, 0x00, 0x00 };
> > > > > > > +     struct rte_ether_addr *mac_addr = (struct rte_ether_addr
> > > > > > > +*)slave_mac1;  
> > > > > >
> > > > > > Use different initializer and you can avoid the need for cast here.
> > > > > >
> > > > > >  
> > > > > > >
> > > > > > > +/**
> > > > > > > + * General device name comparison. Will compare by using the
> > > > > > > +specific bus
> > > > > > > + * compare function or by comparing the names directly.
> > > > > > > + *
> > > > > > > + * @param dev
> > > > > > > + *   Device handle.
> > > > > > > + * @param name
> > > > > > > + *   Name to compare against.
> > > > > > > + * @return
> > > > > > > + *   0 if the device matches the name. Nonzero otherwise.
> > > > > > > + */
> > > > > > > +__rte_internal
> > > > > > > +int rte_cmp_dev_name(const struct rte_device *dev, const void
> > > > > > > +*name);  
> > > > > >
> > > > > > It would make more sense to me if name was a character not void  
> > pointer.  
> > > > > >
> > > > > > The design might be clearer if bus address was more of an
> > > > > > typedef with a pointer and size together. Treat it more like an object.  
> > > > >
> > > > >
> > > > > Okay so just to understand,
> > > > > this is the struct I am adding:
> > > > >
> > > > >       struct rte_bus_address {
> > > > >              const void *addr;
> > > > >              size_t size;
> > > > >       };
> > > > >
> > > > > This is how I pass it to find_device:
> > > > >
> > > > >       struct rte_bus_address dev_addr = {
> > > > >                 .addr = da->name,
> > > > >                 .size = RTE_DEV_NAME_MAX_LEN
> > > > >       };
> > > > >       dev = da->bus->find_device(NULL, rte_cmp_dev_name,
> > > > > &dev_addr);
> > > > >
> > > > > And this is how I use it in rte_cmp_dev_name:
> > > > >
> > > > >       rte_cmp_dev_name(const struct rte_device *dev1, const void *addr)
> > > > >        {
> > > > >              const struct rte_bus_address *dev2_addr = addr;
> > > > >       …
> > > > >       }
> > > > >
> > > > > Is that what you meant?
> > > > > Also, I'm not sure if the size is really needed, because we check
> > > > > the size of the parsed name, which may be different than the size
> > > > > of the original name
> > > > >  
> > > >
> > > > It would be best to pass rte_bus_address to rte_cmp_dev_name rather
> > > > than having implied cast.
> > > > Not sure if that is possible without breaking API though.  
> > >
> > > I think you are right, also it will require to change the signature of
> > > find_device which will make it even bigger change  
> > 
> > Could you post patch with something "good enough" that builds and passes
> > tests?  
> 
> You mean with the rte_bus_address change or without?
> Without it it was tested and passed. With it I only ran few tests locally and they passed

With the bus address change how much impact? Not sure it really matter much though
both solutions are ok, it is more about dealing with other buses in future.