| Message ID | 20181023033528.31256-5-alistair@popple.id.au |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libpdbg: Add an api to get target address by class | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | master/apply_patch Successfully applied |
| snowpatch_ozlabs/build-multiarch | fail | Test build-multiarch on branch master |
On Tue, 2018-10-23 at 14:35 +1100, Alistair Popple wrote: > Signed-off-by: Alistair Popple <alistair@popple.id.au> > --- > libpdbg/libpdbg.h | 1 + > libpdbg/target.c | 10 ++++++++++ > 2 files changed, 11 insertions(+) > > diff --git a/libpdbg/libpdbg.h b/libpdbg/libpdbg.h > index b1d6655..ce11fff 100644 > --- a/libpdbg/libpdbg.h > +++ b/libpdbg/libpdbg.h > @@ -91,6 +91,7 @@ const char *pdbg_target_dn_name(struct pdbg_target > *target); > void *pdbg_target_priv(struct pdbg_target *target); > void pdbg_target_priv_set(struct pdbg_target *target, void *priv); > struct pdbg_target *pdbg_target_root(void); > +struct pdbg_target *pdbg_class_target_addr(struct pdbg_target > *target, const char *name, uint64_t *addr); > > /* Procedures */ > int fsi_read(struct pdbg_target *target, uint32_t addr, uint32_t > *val); > diff --git a/libpdbg/target.c b/libpdbg/target.c > index 975ecec..fbcf792 100644 > --- a/libpdbg/target.c > +++ b/libpdbg/target.c > @@ -32,6 +32,16 @@ static struct pdbg_target > *get_class_target_addr(struct pdbg_target *target, con > return target; > } > > +struct pdbg_target *pdbg_class_target_addr(struct pdbg_target > *target, const char *name, uint64_t *addr) > +{ > + uint64_t tmp; > + > + if (!addr) > + addr = &tmp; > + > + return get_class_target_addr(target, name, addr); > +} > + I think this api is badly named. Isn't this same as parent = pdbg_target_parent(target, name) pdbg_get_address(parent, &addr) May be we don't want to add an api that does the same thing as the combination of existing apis. Unless there is a specific corner case which is not covered. Amitay.
On Monday, 5 November 2018 12:15:25 PM AEDT Amitay Isaacs wrote: > On Tue, 2018-10-23 at 14:35 +1100, Alistair Popple wrote: > > Signed-off-by: Alistair Popple <alistair@popple.id.au> > > --- > > > > libpdbg/libpdbg.h | 1 + > > libpdbg/target.c | 10 ++++++++++ > > 2 files changed, 11 insertions(+) > > > > diff --git a/libpdbg/libpdbg.h b/libpdbg/libpdbg.h > > index b1d6655..ce11fff 100644 > > --- a/libpdbg/libpdbg.h > > +++ b/libpdbg/libpdbg.h > > @@ -91,6 +91,7 @@ const char *pdbg_target_dn_name(struct pdbg_target > > *target); > > > > void *pdbg_target_priv(struct pdbg_target *target); > > void pdbg_target_priv_set(struct pdbg_target *target, void *priv); > > struct pdbg_target *pdbg_target_root(void); > > > > +struct pdbg_target *pdbg_class_target_addr(struct pdbg_target > > *target, const char *name, uint64_t *addr); > > > > /* Procedures */ > > int fsi_read(struct pdbg_target *target, uint32_t addr, uint32_t > > > > *val); > > diff --git a/libpdbg/target.c b/libpdbg/target.c > > index 975ecec..fbcf792 100644 > > --- a/libpdbg/target.c > > +++ b/libpdbg/target.c > > @@ -32,6 +32,16 @@ static struct pdbg_target > > *get_class_target_addr(struct pdbg_target *target, con > > > > return target; > > > > } > > > > +struct pdbg_target *pdbg_class_target_addr(struct pdbg_target > > *target, const char *name, uint64_t *addr) > > +{ > > + uint64_t tmp; > > + > > + if (!addr) > > + addr = &tmp; > > + > > + return get_class_target_addr(target, name, addr); > > +} > > + > > I think this api is badly named. Isn't this same as > > parent = pdbg_target_parent(target, name) > pdbg_get_address(parent, &addr) > > May be we don't want to add an api that does the same thing as the > combination of existing apis. Unless there is a specific corner case > which is not covered. I think you're right. The less API functions to maintain the better, it's not an especially common usecase so I will drop this. - Alistair > Amitay.
On Wed, 2018-11-07 at 15:51 +1100, Alistair Popple wrote: > On Monday, 5 November 2018 12:15:25 PM AEDT Amitay Isaacs wrote: > > On Tue, 2018-10-23 at 14:35 +1100, Alistair Popple wrote: > > > Signed-off-by: Alistair Popple <alistair@popple.id.au> > > > --- > > > > > > libpdbg/libpdbg.h | 1 + > > > libpdbg/target.c | 10 ++++++++++ > > > 2 files changed, 11 insertions(+) > > > > > > diff --git a/libpdbg/libpdbg.h b/libpdbg/libpdbg.h > > > index b1d6655..ce11fff 100644 > > > --- a/libpdbg/libpdbg.h > > > +++ b/libpdbg/libpdbg.h > > > @@ -91,6 +91,7 @@ const char *pdbg_target_dn_name(struct > > > pdbg_target > > > *target); > > > > > > void *pdbg_target_priv(struct pdbg_target *target); > > > void pdbg_target_priv_set(struct pdbg_target *target, void > > > *priv); > > > struct pdbg_target *pdbg_target_root(void); > > > > > > +struct pdbg_target *pdbg_class_target_addr(struct pdbg_target > > > *target, const char *name, uint64_t *addr); > > > > > > /* Procedures */ > > > int fsi_read(struct pdbg_target *target, uint32_t addr, uint32_t > > > > > > *val); > > > diff --git a/libpdbg/target.c b/libpdbg/target.c > > > index 975ecec..fbcf792 100644 > > > --- a/libpdbg/target.c > > > +++ b/libpdbg/target.c > > > @@ -32,6 +32,16 @@ static struct pdbg_target > > > *get_class_target_addr(struct pdbg_target *target, con > > > > > > return target; > > > > > > } > > > > > > +struct pdbg_target *pdbg_class_target_addr(struct pdbg_target > > > *target, const char *name, uint64_t *addr) > > > +{ > > > + uint64_t tmp; > > > + > > > + if (!addr) > > > + addr = &tmp; > > > + > > > + return get_class_target_addr(target, name, addr); > > > +} > > > + > > > > I think this api is badly named. Isn't this same as > > > > parent = pdbg_target_parent(target, name) > > pdbg_get_address(parent, &addr) > > > > May be we don't want to add an api that does the same thing as the > > combination of existing apis. Unless there is a specific corner > > case > > which is not covered. > > I think you're right. The less API functions to maintain the better, > it's not > an especially common usecase so I will drop this. I think I read the code wrong. It's doing address translation relative to a particular parent of specified class. It's not the same as pdbg_get_address(parent, &addr). Since it's doing address translation, I don't think there should be any users for such a functionality outside libpdbg. If there is a need for such a functionality, it probably means we have some missing abstration in libpdbg. Amitay.
diff --git a/libpdbg/libpdbg.h b/libpdbg/libpdbg.h index b1d6655..ce11fff 100644 --- a/libpdbg/libpdbg.h +++ b/libpdbg/libpdbg.h @@ -91,6 +91,7 @@ const char *pdbg_target_dn_name(struct pdbg_target *target); void *pdbg_target_priv(struct pdbg_target *target); void pdbg_target_priv_set(struct pdbg_target *target, void *priv); struct pdbg_target *pdbg_target_root(void); +struct pdbg_target *pdbg_class_target_addr(struct pdbg_target *target, const char *name, uint64_t *addr); /* Procedures */ int fsi_read(struct pdbg_target *target, uint32_t addr, uint32_t *val); diff --git a/libpdbg/target.c b/libpdbg/target.c index 975ecec..fbcf792 100644 --- a/libpdbg/target.c +++ b/libpdbg/target.c @@ -32,6 +32,16 @@ static struct pdbg_target *get_class_target_addr(struct pdbg_target *target, con return target; } +struct pdbg_target *pdbg_class_target_addr(struct pdbg_target *target, const char *name, uint64_t *addr) +{ + uint64_t tmp; + + if (!addr) + addr = &tmp; + + return get_class_target_addr(target, name, addr); +} + /* The indirect access code was largely stolen from hw/xscom.c in skiboot */ #define PIB_IND_MAX_RETRIES 10 #define PIB_IND_READ PPC_BIT(0)
Signed-off-by: Alistair Popple <alistair@popple.id.au> --- libpdbg/libpdbg.h | 1 + libpdbg/target.c | 10 ++++++++++ 2 files changed, 11 insertions(+)