Message ID | 20220318173320.320541-1-danielhb413@gmail.com (mailing list archive) |
---|---|
Headers | show |
Series | spapr: add drc->index, remove spapr_drc_index() | expand |
On Fri, Mar 18, 2022 at 02:33:11PM -0300, Daniel Henrique Barboza wrote: > Hi, > > I decided to make this change after realizing that (1) spapr_drc_index() > always return the same index value for the DRC regardless of machine or > device state and (2) we call spapr_drc_index() a lot throughout the > spapr code. Hmm.. so, spapr_drc_index() wasn't ever intended as an abstraction point. Rather, it's just there as a matter of data redundancy. The index can be derived from the drc->id and the type. Unless there's a compelling reason otherwise, it's usually a good idea to store data in just one form (if there's more it's an opportunity for bugs to let it get out of sync). > > This means that a new attribute to store the generated index in the DRC > object time will spare us from calling a function that always returns > the same value. > > No functional changes were made. > > > Daniel Henrique Barboza (9): > hw/ppc/spapr_drc.c: add drc->index > hw/ppc/spapr_drc.c: redefine 'index' SpaprDRC property > hw/ppc/spapr_drc.c: use drc->index in trace functions > hw/ppc/spapr_drc.c: use drc->index > hw/ppc/spapr.c: use drc->index > hw/ppc/spapr_events.c: use drc->index > hw/ppc/spapr_nvdimm.c: use drc->index > hw/ppc/spapr_pci.c: use drc->index > hw/ppc/spapr_drc.c: remove spapr_drc_index() > > hw/ppc/spapr.c | 18 ++++----- > hw/ppc/spapr_drc.c | 79 +++++++++++++++----------------------- > hw/ppc/spapr_events.c | 4 +- > hw/ppc/spapr_nvdimm.c | 10 ++--- > hw/ppc/spapr_pci.c | 5 +-- > include/hw/ppc/spapr_drc.h | 2 +- > 6 files changed, 48 insertions(+), 70 deletions(-) >
On 3/21/22 00:55, David Gibson wrote: > On Fri, Mar 18, 2022 at 02:33:11PM -0300, Daniel Henrique Barboza wrote: >> Hi, >> >> I decided to make this change after realizing that (1) spapr_drc_index() >> always return the same index value for the DRC regardless of machine or >> device state and (2) we call spapr_drc_index() a lot throughout the >> spapr code. > > Hmm.. so, spapr_drc_index() wasn't ever intended as an abstraction > point. Rather, it's just there as a matter of data redundancy. The > index can be derived from the drc->id and the type. Unless there's a > compelling reason otherwise, it's usually a good idea to store data in > just one form (if there's more it's an opportunity for bugs to let it > get out of sync). Hmm what if we store drc->index instead and derive drc->id from it? drc->index is read from several places, while drc->id is used just in spapr_drc_name() to write the DT (via spapr_dt_drc()). I'll think more about it. Thanks, Daniel > >> >> This means that a new attribute to store the generated index in the DRC >> object time will spare us from calling a function that always returns >> the same value. >> >> No functional changes were made. >> >> >> Daniel Henrique Barboza (9): >> hw/ppc/spapr_drc.c: add drc->index >> hw/ppc/spapr_drc.c: redefine 'index' SpaprDRC property >> hw/ppc/spapr_drc.c: use drc->index in trace functions >> hw/ppc/spapr_drc.c: use drc->index >> hw/ppc/spapr.c: use drc->index >> hw/ppc/spapr_events.c: use drc->index >> hw/ppc/spapr_nvdimm.c: use drc->index >> hw/ppc/spapr_pci.c: use drc->index >> hw/ppc/spapr_drc.c: remove spapr_drc_index() >> >> hw/ppc/spapr.c | 18 ++++----- >> hw/ppc/spapr_drc.c | 79 +++++++++++++++----------------------- >> hw/ppc/spapr_events.c | 4 +- >> hw/ppc/spapr_nvdimm.c | 10 ++--- >> hw/ppc/spapr_pci.c | 5 +-- >> include/hw/ppc/spapr_drc.h | 2 +- >> 6 files changed, 48 insertions(+), 70 deletions(-) >> >
On Mon, Mar 21, 2022 at 04:58:47AM -0300, Daniel Henrique Barboza wrote: > > > On 3/21/22 00:55, David Gibson wrote: > > On Fri, Mar 18, 2022 at 02:33:11PM -0300, Daniel Henrique Barboza wrote: > > > Hi, > > > > > > I decided to make this change after realizing that (1) spapr_drc_index() > > > always return the same index value for the DRC regardless of machine or > > > device state and (2) we call spapr_drc_index() a lot throughout the > > > spapr code. > > > > Hmm.. so, spapr_drc_index() wasn't ever intended as an abstraction > > point. Rather, it's just there as a matter of data redundancy. The > > index can be derived from the drc->id and the type. Unless there's a > > compelling reason otherwise, it's usually a good idea to store data in > > just one form (if there's more it's an opportunity for bugs to let it > > get out of sync). > > > Hmm what if we store drc->index instead and derive drc->id from it? drc->index > is read from several places, while drc->id is used just in spapr_drc_name() to > write the DT (via spapr_dt_drc()). That could work. It's still slightly redundant since the type part of the index can be derived from the class, but I think it's not unreasonable.