Message ID | 20170822081543.30506-1-oohall@gmail.com |
---|---|
State | New |
Headers | show |
I reviewed this and I think I've convinced myself it's correct. That said it isn't the most obvious code in the world although I couldn't think of a way to simplify it further. It does make a lot of other code far more obvious/straight forward so assuming it's well tested (and I seem to recall Oliver tested pretty extensively) then I think it's worth merging. Reviewed-by: Alistair Popple <alistair@popple.id.au> On Tue, 22 Aug 2017 06:15:41 PM Oliver O'Halloran wrote: > Add an interation helper for PCI devices. This is functionally identical > to the pci_walk_devs() helper function, but it allows us to write linear > code rather than dropping us into the middle of callback hell. > > Signed-off-by: Oliver O'Halloran <oohall@gmail.com> > --- > core/pci.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > include/pci.h | 9 +++++++ > 2 files changed, 93 insertions(+) > > diff --git a/core/pci.c b/core/pci.c > index 4296180ad111..c5d2fba36508 100644 > --- a/core/pci.c > +++ b/core/pci.c > @@ -1798,6 +1798,90 @@ static struct pci_device *__pci_walk_dev(struct phb *phb, > return NULL; > } > > +/* > + * pci_next_dev(phb, prev, pass) - Iteration worker function > + * > + * phb - phb that we're iterating > + * prev - the last pci_device returned by pci_next_dev > + * > + * pass - a cookie used to track the iteration state. See below if you > + * really want to know the details. > + */ > +struct pci_device *pci_next_dev(struct phb *phb, struct pci_device *prev, > + int *pass) > +{ > + struct pci_device *next, *last; > + struct list_head *siblings; > + > + assert(phb); > + > + /* start of iteration */ > + if (!prev) { > + *pass = 0; > + return list_top(&phb->devices, struct pci_device, link); > + } > + > +up: > + assert(prev); > + > + if (prev->parent) > + siblings = &prev->parent->children; > + else > + siblings = &phb->devices; > + > + last = list_tail(siblings, struct pci_device, link); > + assert(last); > + > + /* > + * You made it this far so you get an explaination! > + * > + * We need to ensure that the iteration order is correct, that is we are > + * supposed to look at each *device* on a bus before looking at the > + * *child buses* below those devices, as is done in pci_walk_devs(). In > + * other words, we do two pases of every bus. > + * > + * The 'pass' variable is used to track which pass we are on. When it's > + * even we are just visiting each device and when odd we are descending > + * into child buses if we find any. > + */ > + if (prev == last) { > + if (*pass & 1) { /* end of the odd pass */ > + if (!prev->parent) { > + assert(*pass == 1); > + return NULL; > + } > + > + /* back to the odd pass across the parent's bus */ > + (*pass) -= 2; > + prev = prev->parent; > + goto up; > + } else { > + next = list_top(siblings, struct pci_device, link); > + (*pass)++; > + } > + } else { > + next = list_entry(prev->link.next, struct pci_device, link); > + } > + > + /* now find the device we should return */ > + if (!(*pass & 1)) > + return next; > + > + /* for an odd pass we scan for another child bus to descend into */ > + while (list_empty(&next->children)) { > + if (next == last) { > + prev = last; > + goto up; > + } > + > + next = list_entry(next->link.next, struct pci_device, link); > + } > + > + /* increment pass so it's even */ > + (*pass)++; > + return list_top(&next->children, struct pci_device, link); > +} > + > struct pci_device *pci_walk_dev(struct phb *phb, > struct pci_device *pd, > int (*cb)(struct phb *, > diff --git a/include/pci.h b/include/pci.h > index 54a62fd19a99..81ad62f8fa19 100644 > --- a/include/pci.h > +++ b/include/pci.h > @@ -460,6 +460,15 @@ static inline struct phb *__pci_next_phb_idx(uint64_t *phb_id) { > for (uint64_t __phb_idx = 0; \ > (phb = __pci_next_phb_idx(&__phb_idx)) ; ) > > + > +struct pci_device *pci_next_dev(struct phb *phb, struct pci_device *pd, > + int *pass); > + > +/* pass should be an int *, see pci_next_dev() for details */ > +#define for_each_pci_dev(phb, pd, pass) \ > + for (pd = pci_next_dev(phb, NULL, pass); pd; \ > + pd = pci_next_dev(phb, pd, pass)) > + > /* Device tree */ > extern void pci_std_swizzle_irq_map(struct dt_node *dt_node, > struct pci_device *pd, >
diff --git a/core/pci.c b/core/pci.c index 4296180ad111..c5d2fba36508 100644 --- a/core/pci.c +++ b/core/pci.c @@ -1798,6 +1798,90 @@ static struct pci_device *__pci_walk_dev(struct phb *phb, return NULL; } +/* + * pci_next_dev(phb, prev, pass) - Iteration worker function + * + * phb - phb that we're iterating + * prev - the last pci_device returned by pci_next_dev + * + * pass - a cookie used to track the iteration state. See below if you + * really want to know the details. + */ +struct pci_device *pci_next_dev(struct phb *phb, struct pci_device *prev, + int *pass) +{ + struct pci_device *next, *last; + struct list_head *siblings; + + assert(phb); + + /* start of iteration */ + if (!prev) { + *pass = 0; + return list_top(&phb->devices, struct pci_device, link); + } + +up: + assert(prev); + + if (prev->parent) + siblings = &prev->parent->children; + else + siblings = &phb->devices; + + last = list_tail(siblings, struct pci_device, link); + assert(last); + + /* + * You made it this far so you get an explaination! + * + * We need to ensure that the iteration order is correct, that is we are + * supposed to look at each *device* on a bus before looking at the + * *child buses* below those devices, as is done in pci_walk_devs(). In + * other words, we do two pases of every bus. + * + * The 'pass' variable is used to track which pass we are on. When it's + * even we are just visiting each device and when odd we are descending + * into child buses if we find any. + */ + if (prev == last) { + if (*pass & 1) { /* end of the odd pass */ + if (!prev->parent) { + assert(*pass == 1); + return NULL; + } + + /* back to the odd pass across the parent's bus */ + (*pass) -= 2; + prev = prev->parent; + goto up; + } else { + next = list_top(siblings, struct pci_device, link); + (*pass)++; + } + } else { + next = list_entry(prev->link.next, struct pci_device, link); + } + + /* now find the device we should return */ + if (!(*pass & 1)) + return next; + + /* for an odd pass we scan for another child bus to descend into */ + while (list_empty(&next->children)) { + if (next == last) { + prev = last; + goto up; + } + + next = list_entry(next->link.next, struct pci_device, link); + } + + /* increment pass so it's even */ + (*pass)++; + return list_top(&next->children, struct pci_device, link); +} + struct pci_device *pci_walk_dev(struct phb *phb, struct pci_device *pd, int (*cb)(struct phb *, diff --git a/include/pci.h b/include/pci.h index 54a62fd19a99..81ad62f8fa19 100644 --- a/include/pci.h +++ b/include/pci.h @@ -460,6 +460,15 @@ static inline struct phb *__pci_next_phb_idx(uint64_t *phb_id) { for (uint64_t __phb_idx = 0; \ (phb = __pci_next_phb_idx(&__phb_idx)) ; ) + +struct pci_device *pci_next_dev(struct phb *phb, struct pci_device *pd, + int *pass); + +/* pass should be an int *, see pci_next_dev() for details */ +#define for_each_pci_dev(phb, pd, pass) \ + for (pd = pci_next_dev(phb, NULL, pass); pd; \ + pd = pci_next_dev(phb, pd, pass)) + /* Device tree */ extern void pci_std_swizzle_irq_map(struct dt_node *dt_node, struct pci_device *pd,
Add an interation helper for PCI devices. This is functionally identical to the pci_walk_devs() helper function, but it allows us to write linear code rather than dropping us into the middle of callback hell. Signed-off-by: Oliver O'Halloran <oohall@gmail.com> --- core/pci.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ include/pci.h | 9 +++++++ 2 files changed, 93 insertions(+)