Re: [PATCH] spapr: Move spapr_create_nvdimm_dr_connectors() to core machine code

[David Gibson]

On Tue, Oct 13, 2020 at 09:33:44AM +0200, Greg Kurz wrote:
> On Tue, 13 Oct 2020 11:40:14 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Mon, Oct 12, 2020 at 12:15:21PM +0200, Greg Kurz wrote:
> > > The spapr_create_nvdimm_dr_connectors() function doesn't need to access
> > > any internal details of the sPAPR NVDIMM implementation. Also, pretty
> > > much like for the LMBs, only spapr_machine_init() is responsible for the
> > > creation of DR connectors for NVDIMMs.
> > > 
> > > Make this clear by making this function static in hw/ppc/spapr.c.
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > 
> > Hrm, I'm not really seeing the advantage to moving this.  It doesn't
> > have to be in spapr_nvdimm for data hiding, but it is related, and
> > spapr.c is kind of huge.
> > 
> 
> The only advantage is to give an appropriate scope to this function,
> as many other functions that create internal devices, eg. other DRC
> types or the default PHB for which a similar change was accepted
> 2 years ago.
> 
> commit 999c9caf2eee66103195e1ec7580b379929db9d2
> Author: Greg Kurz <groug@kaod.org>
> Date:   Fri Dec 21 01:35:09 2018 +0100
> 
>     spapr: move spapr_create_phb() to core machine code
>     
>     This function is only used when creating the default PHB. Let's rename
>     it and move it to the core machine code for clarity.
>     
>     Signed-off-by: Greg Kurz <groug@kaod.org>
>     Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>     Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>     Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> I agree that spapr.c is huge indeed (4943 lines) but this increases its
> size by _just_ 0.2 %. And there are certainly good candidates that
> landed in spapr.c by _default_ over the years but should rather be
> moved to their own compilation unit (eg. a bunch of FDT building
> functions for various resources or some hotplug related functions
> that don't need to access machine internals).

Good points.  Applied to ppc-for-5.2.

> 
> > > ---
> > >  hw/ppc/spapr.c                |   10 ++++++++++
> > >  hw/ppc/spapr_nvdimm.c         |   11 -----------
> > >  include/hw/ppc/spapr_nvdimm.h |    1 -
> > >  3 files changed, 10 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 63315f2d0fa9..ee716a12af73 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -2641,6 +2641,16 @@ static hwaddr spapr_rma_size(SpaprMachineState 
> > > *spapr, Error **errp)
> > >      return rma_size;
> > >  }
> > >  
> > > +static void spapr_create_nvdimm_dr_connectors(SpaprMachineState *spapr)
> > > +{
> > > +    MachineState *machine = MACHINE(spapr);
> > > +    int i;
> > > +
> > > +    for (i = 0; i < machine->ram_slots; i++) {
> > > +        spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_PMEM, i);
> > > +    }
> > > +}
> > > +
> > >  /* pSeries LPAR / sPAPR hardware init */
> > >  static void spapr_machine_init(MachineState *machine)
> > >  {
> > > diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> > > index b3a489e9fe18..9e3d94071fe1 100644
> > > --- a/hw/ppc/spapr_nvdimm.c
> > > +++ b/hw/ppc/spapr_nvdimm.c
> > > @@ -106,17 +106,6 @@ void spapr_add_nvdimm(DeviceState *dev, uint64_t 
> > > slot, Error **errp)
> > >      }
> > >  }
> > >  
> > > -void spapr_create_nvdimm_dr_connectors(SpaprMachineState *spapr)
> > > -{
> > > -    MachineState *machine = MACHINE(spapr);
> > > -    int i;
> > > -
> > > -    for (i = 0; i < machine->ram_slots; i++) {
> > > -        spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_PMEM, i);
> > > -    }
> > > -}
> > > -
> > > -
> > >  static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt,
> > >                             int parent_offset, NVDIMMDevice *nvdimm)
> > >  {
> > > diff --git a/include/hw/ppc/spapr_nvdimm.h b/include/hw/ppc/spapr_nvdimm.h
> > > index b834d82f5545..490b19a009f4 100644
> > > --- a/include/hw/ppc/spapr_nvdimm.h
> > > +++ b/include/hw/ppc/spapr_nvdimm.h
> > > @@ -31,6 +31,5 @@ void spapr_dt_persistent_memory(SpaprMachineState 
> > > *spapr, void *fdt);
> > >  bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice 
> > > *nvdimm,
> > >                             uint64_t size, Error **errp);
> > >  void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp);
> > > -void spapr_create_nvdimm_dr_connectors(SpaprMachineState *spapr);
> > >  
> > >  #endif
> > > 
> > > 
> > 
> 



-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature