[dpdk-dev,v6,09/11] pci: implement find_device bus operation
Checks
Commit Message
Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
lib/librte_eal/common/eal_common_pci.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
Comments
On Tue, 27 Jun 2017 18:11:16 +0200
Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> + int start_found = !!(start == NULL);
> +
> + FOREACH_DEVICE_ON_PCIBUS(dev) {
> + if (!start_found) {
> + if (&dev->device == start)
> + start_found = 1;
> + continue;
> + }
> + if (cmp(&dev->device, data) == 0)
> + return &dev->device;
> + }
> + return NULL;
> +}
> +
Why is start_found not a boolean?
Do you really need start_found at all? Why not just reuse existing
pointer?
FOREACH_DEVICE_ON_PCIBUS(dev) {
if (start) {
if (&dev->device == start)
start = NULL
continue;
}
if (cmp(&dev->device, data) == 0)
return &dev->device;
}
return NULL;
}
On Tue, Jun 27, 2017 at 04:35:14PM -0700, Stephen Hemminger wrote:
> On Tue, 27 Jun 2017 18:11:16 +0200
> Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
>
> > + int start_found = !!(start == NULL);
> > +
> > + FOREACH_DEVICE_ON_PCIBUS(dev) {
> > + if (!start_found) {
> > + if (&dev->device == start)
> > + start_found = 1;
> > + continue;
> > + }
> > + if (cmp(&dev->device, data) == 0)
> > + return &dev->device;
> > + }
> > + return NULL;
> > +}
> > +
>
> Why is start_found not a boolean?
>
Ah, yes, I wrote this a few times over in rte_bus and rte_vdev, and
mostly used the same scheme in the PCI implementation, without checking
for the use of stdbool in the vincinity otherwise.
I would not be opposed to using a bool if I was rewriting it, but I
don't feel this change warrants a new version.
> Do you really need start_found at all? Why not just reuse existing
> pointer?
>
You are right, it could be reduced. But I find it a little too "clever"
in a sense, and I prefer usually to avoid rewriting input parameters. If this
function had to be refactored later, the writer would need to be careful
about start having changed. The advantage of using one variable less
does not outweight this drawback I think.
> FOREACH_DEVICE_ON_PCIBUS(dev) {
> if (start) {
> if (&dev->device == start)
> start = NULL
> continue;
> }
> if (cmp(&dev->device, data) == 0)
> return &dev->device;
> }
> return NULL;
> }
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Gaëtan Rivet
> Sent: Wednesday, June 28, 2017 10:18 AM
> To: Stephen Hemminger <stephen@networkplumber.org>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v6 09/11] pci: implement find_device bus
> operation
>
> On Tue, Jun 27, 2017 at 04:35:14PM -0700, Stephen Hemminger wrote:
> > On Tue, 27 Jun 2017 18:11:16 +0200
> > Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> >
> > > + int start_found = !!(start == NULL);
> > > +
> > > + FOREACH_DEVICE_ON_PCIBUS(dev) {
> > > + if (!start_found) {
> > > + if (&dev->device == start)
> > > + start_found = 1;
> > > + continue;
> > > + }
> > > + if (cmp(&dev->device, data) == 0)
> > > + return &dev->device;
> > > + }
> > > + return NULL;
> > > +}
> > > +
> >
> > Why is start_found not a boolean?
> >
>
> Ah, yes, I wrote this a few times over in rte_bus and rte_vdev, and mostly
> used the same scheme in the PCI implementation, without checking for the
> use of stdbool in the vincinity otherwise.
>
> I would not be opposed to using a bool if I was rewriting it, but I don't
> feel this change warrants a new version.
>
> > Do you really need start_found at all? Why not just reuse existing
> > pointer?
> >
>
> You are right, it could be reduced. But I find it a little too "clever"
> in a sense, and I prefer usually to avoid rewriting input parameters. If
> this function had to be refactored later, the writer would need to be
> careful about start having changed. The advantage of using one variable
> less does not outweight this drawback I think.
>
+1 for having an extra variable for clarity.
@@ -488,10 +488,31 @@ rte_pci_remove_device(struct rte_pci_device *pci_dev)
TAILQ_REMOVE(&rte_pci_bus.device_list, pci_dev, next);
}
+static struct rte_device *
+pci_find_device(rte_dev_cmp_t cmp,
+ const void *data,
+ const struct rte_device *start)
+{
+ struct rte_pci_device *dev;
+ int start_found = !!(start == NULL);
+
+ FOREACH_DEVICE_ON_PCIBUS(dev) {
+ if (!start_found) {
+ if (&dev->device == start)
+ start_found = 1;
+ continue;
+ }
+ if (cmp(&dev->device, data) == 0)
+ return &dev->device;
+ }
+ return NULL;
+}
+
struct rte_pci_bus rte_pci_bus = {
.bus = {
.scan = rte_pci_scan,
.probe = rte_pci_probe,
+ .find_device = pci_find_device,
},
.device_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.device_list),
.driver_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.driver_list),