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

[Stephen Hemminger <stephen@networkplumber.org>]

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.