lkml.org 
[lkml]   [2019]   [Jan]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC 1/6] powerpc:/drc Define interface to acquire arch-specific drc info
    From
    Date
    On 1/29/19 3:31 AM, Michael Ellerman wrote:
    > Tyrel Datwyler <turtle.in.the.kernel@gmail.com> writes:
    >> On 12/14/2018 12:50 PM, Michael Bringmann wrote:
    >>> Define interface to acquire arch-specific drc info to match against
    >>> hotpluggable devices. The current implementation exposes several
    >>> pseries-specific dynamic memory properties in generic kernel code.
    >>> This patch set provides an interface to pull that code out of the
    >>> generic kernel.
    >>>
    >>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
    >>> ---
    >>> include/linux/topology.h | 9 +++++++++
    >>> 1 file changed, 9 insertions(+)
    >>>
    >>> diff --git a/include/linux/topology.h b/include/linux/topology.h
    >>> index cb0775e..df97f5f 100644
    >>> --- a/include/linux/topology.h
    >>> +++ b/include/linux/topology.h
    >>> @@ -44,6 +44,15 @@
    >>
    >> As far as I know pseries is the only platform that uses DR connectors, and I
    >> highly doubt that any other powerpc platform or arch ever will. So, I'm not sure
    >> that this is really generic enough to belong in topology.h.
    >
    > Right. This does not belong in include/linux.
    >
    >> If anything I would
    >> suggest putting this in an include in arch/powerpc/include/ named something like
    >> drcinfo.h or pseries-drc.h. That will make it visible to modules like rpaphp
    >> that want/need to use this functionality.
    >
    > Yeah that would make more sense.

    If you see no objection to referencing a powerpc-specific function from
    the code ...

    >
    > Using "arch" in the name is wrong, it's pseries specific so
    > pseries_find_drc_match() would be more appropriate.
    >
    >>> +int arch_find_drc_match(struct device_node *dn,
    >>> + bool (*usercb)(struct device_node *dn,
    >>> + u32 drc_index, char *drc_name,
    >>> + char *drc_type, u32 drc_power_domain,
    >>> + void *data),
    >>> + char *opt_drc_type, char *opt_drc_name,
    >>> + bool match_drc_index, bool ck_php_type,
    >>> + void *data);
    >
    > This function signature is kind of insane.
    >
    > You end with calls like:
    >
    > + return arch_find_drc_match(dn, rpaphp_add_slot_cb,
    > + NULL, NULL, false, true, NULL);
    >
    > Which is impossible to parse.
    >
    > I feel like maybe this isn't the right level of abstraction.

    ...
    I had already been considering simplifying the interface for these
    calls to something like the following:

    int rpaphp_check_drc_props(struct device_node *dn, char *drc_name,
    char *drc_type)
    {
    return pseries_find_drc_match(dn, drc_type, drc_name);
    }
    ...
    int rpaphp_add_slot(struct device_node *dn)
    {
    if (!dn->name || strcmp(dn->name, "pci"))
    return 0;

    return pseries_add_drc_slot(dn, rpaphp_add_slot_cb);
    }
    ...

    Further details would be hidden within the pseries code.


    >
    > cheers

    Regards

    --
    Michael W. Bringmann
    Linux Technology Center
    IBM Corporation
    Tie-Line 363-5196
    External: (512) 286-5196
    Cell: (512) 466-0650
    mwb@linux.vnet.ibm.com

    \
     
     \ /
      Last update: 2019-01-29 17:21    [W:2.608 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site