Re: [PATCH 1/4] drivers/bus: Added Freescale Management Complex APIs

[Kim Phillips <kim.phillips@freescale.com>]

On Thu, 18 Sep 2014 15:14:03 +0200
Alexander Graf <agraf@suse.de> wrote:

> > Am 18.09.2014 um 06:17 schrieb German Rivera <German.Rivera@freescale.com>:
> > 
> >> On 09/15/2014 06:44 PM, Kim Phillips wrote:
> >> On Thu, 11 Sep 2014 12:34:21 -0500
> >> "J. German Rivera" <German.Rivera@freescale.com> wrote:
> >> 
> >>> From: "J. German Rivera" <German.Rivera@freescale.com>
> >>> 
> >>> APIs to access the Management Complex (MC) hardware
> >>> module of Freescale LS2 SoCs. This patch includes
> >>> APIs to check the MC firmware version and to manipulate
> >>> DPRC objects in the MC.
> >>> 
> >>> Signed-off-by: J. German Rivera <German.Rivera@freescale.com>
> >>> Signed-off-by: Stuart Yoder <stuart.yoder@freescale.com>
> >>> ---
> >>>  drivers/bus/fsl-mc/dpmng.c         |   93 +++++
> >>>  drivers/bus/fsl-mc/dprc.c          |  504 +++++++++++++++++++++++
> >>>  drivers/bus/fsl-mc/fsl_dpmng_cmd.h |   83 ++++
> >>>  drivers/bus/fsl-mc/fsl_dprc_cmd.h  |  545 +++++++++++++++++++++++++
> >>>  drivers/bus/fsl-mc/fsl_mc_sys.c    |  237 +++++++++++
> >>>  include/linux/fsl_dpmng.h          |  120 ++++++
> >>>  include/linux/fsl_dprc.h           |  790 ++++++++++++++++++++++++++++++++++++
> >>>  include/linux/fsl_mc_cmd.h         |  182 +++++++++
> >>>  include/linux/fsl_mc_sys.h         |   81 ++++
> >>>  9 files changed, 2635 insertions(+)
> >>>  create mode 100644 drivers/bus/fsl-mc/dpmng.c
> >>>  create mode 100644 drivers/bus/fsl-mc/dprc.c
> >>>  create mode 100644 drivers/bus/fsl-mc/fsl_dpmng_cmd.h
> >>>  create mode 100644 drivers/bus/fsl-mc/fsl_dprc_cmd.h
> >>>  create mode 100644 drivers/bus/fsl-mc/fsl_mc_sys.c
> >>>  create mode 100644 include/linux/fsl_dpmng.h
> >>>  create mode 100644 include/linux/fsl_dprc.h
> >>>  create mode 100644 include/linux/fsl_mc_cmd.h
> >>>  create mode 100644 include/linux/fsl_mc_sys.h
> >> 
> >> the fsl prefix in the filename fsl_dpmng_cmd.h is redundant with
> >> its directory name fsl-mc/.  Note that I find dashes ('-') in
> >> filenames make them easier to type: is there a reason we're using
> >> underscores here?
> > This is a convention that we decided early on '-' for directory names
> > and '_' for file names.

based on what?

> > unnecessarily complicated error path, plus a simpler
> >> implementation can be made if fn can return the mapped address, like
> >> so:
> >> 
> >> static void __iomem *map_mc_portal(phys_addr_t mc_portal_phys_addr,
> >>                                    uint32_t mc_portal_size)
> >> {
> >>         struct resource *res;
> >>         void __iomem *mapped_addr;
> >> 
> >>         res = request_mem_region(mc_portal_phys_addr, mc_portal_size,
> >>                                  "mc_portal");
> >>         if (!res)
> >>                 return NULL;
> >> 
> >>         mapped_addr = ioremap_nocache(mc_portal_phys_addr,
> >>                                       mc_portal_size);
> >>         if (!mapped_addr)
> >>                 release_mem_region(mc_portal_phys_addr, mc_portal_size);
> >> 
> >>         return mapped_addr;
> >> }
> >> 
> >> the callsite can return -ENOMEM to its caller if returned NULL.  This
> >> can be improved even further if devm_ functions are used:  this is
> >> just an example of how to simplify the code using early returns
> >> instead of goto error.
> > 
> > I disagree. Having a common error return point is more maintainable than having multiple returns as having the clean-up logic in one place is more maintainable and makes the min path (non-error) more readable.

my comment is not that much different from Joe's here:

https://lkml.org/lkml/2014/9/17/381

but hopefully all this will change with a devm_ based implementation.

> >>> +int __must_check fsl_create_mc_io(phys_addr_t mc_portal_phys_addr,
> >>> +                  uint32_t mc_portal_size,
> >>> +                  uint32_t flags, struct fsl_mc_io **new_mc_io)
> >>> +{
> >>> +    int error = -EINVAL;
> >>> +    struct fsl_mc_io *mc_io = NULL;
> >>> +
> >>> +    mc_io = kzalloc(sizeof(*mc_io), GFP_KERNEL);
> >>> +    if (mc_io == NULL) {
> >>> +        error = -ENOMEM;
> >>> +        pr_err("No memory to allocate mc_io\n");
> >>> +        goto error;
> >>> +    }
> >>> +
> >>> +    mc_io->magic = FSL_MC_IO_MAGIC;
> >>> +    mc_io->flags = flags;
> >>> +    mc_io->portal_phys_addr = mc_portal_phys_addr;
> >>> +    mc_io->portal_size = mc_portal_size;
> >>> +    spin_lock_init(&mc_io->spinlock);
> >>> +    error = map_mc_portal(mc_portal_phys_addr,
> >>> +                  mc_portal_size, &mc_io->portal_virt_addr);
> >>> +    if (error < 0)
> >>> +        goto error;
> >>> +
> >>> +    *new_mc_io = mc_io;
> >>> +    return 0;
> >> 
> >> if a fn only returns an address or error, it can return ERR_PTR
> >> (e.g., -ENOMEM), and the callsite use IS_ERR() to determine whether
> >> there was an error or address returned.  This makes code much
> >> simpler instead of passing address values back by reference.
> > I disagree. I don't see why the alternative you suggest makes the code "much simpler".

because it eliminates the need for the extra pass-by-reference
argument struct fsl_mc_io **new_mc_io.

> >>> +void fsl_destroy_mc_io(struct fsl_mc_io *mc_io)
> >>> +{
> >>> +    if (WARN_ON(mc_io->magic != FSL_MC_IO_MAGIC))
> >>> +        return;
> >>> +
> >>> +    if (mc_io->portal_virt_addr != NULL) {
> >>> +        unmap_mc_portal(mc_io->portal_phys_addr,
> >>> +                mc_io->portal_size, mc_io->portal_virt_addr);
> >> 
> >> unmap_mc_portal already checks for virt_addr, this is another
> >> example where the code goes too far checking for NULL.
> > I disagree. Having the extra check is harmless and more importantly makes the intent explicit that we should only call unmap_mc_portal if we called map_mc_portal earlier.

the code is doing this:

        if (mc_io->portal_virt_addr != NULL) {
	        if (WARN_ON(mc_portal_virt_addr == NULL))
        	        return;

which is redundant and therefore makes it unnecessarily complicated,
after all, a stack trace will occur if mc_portal_virt_addr is
referenced anyway, making the WARN_ON clause redundant, too.

> >>> +        mc_io->portal_virt_addr = NULL;
> >>> +    }
> >>> +
> >>> +    mc_io->magic = 0x0;
> >>> +    kfree(mc_io);
> >>> +}

btw, what's the point of zeroing out things that are being freed?

> >>> +/**
> >>> + * @brief    Management Complex firmware version information
> >>> + */
> >>> +#define MC_VER_MAJOR 2
> >>> +#define MC_VER_MINOR 0
> >> 
> >> code should be adjusted to run on all *compatible* versions of h/w,
> >> not strictly the one set in these defines.
> > This comment is not precise enough be actionable.
> > What exactly you want to be changed  here?
> 
> I think the easy thing to do is to convert the exact version check into a ranged version check: have minimum and maximum versions you support. Or a list of exact versions you support. Or not check for the version at all - or only for the major version and guarantee that the major version indicates backwards compatibility.

yes, this was my point: elsewhere I noticed the code denies to run
iff those defines are not matched exactly: that code should change
to run as Alex describes.

> >>> +/**
> >>> + * @brief    Disconnects one endpoint to remove its network link
> >>> + *
> >>> + * @param[in]   mc_io        Pointer to opaque I/O object
> >>> + * @param[in]    dprc_handle    Handle to the DPRC object
> >>> + * @param[in]   endpoint    Endpoint configuration parameters.
> >>> + *
> >>> + * @returns    '0' on Success; Error code otherwise.
> >>> + * */
> >>> +int dprc_disconnect(struct fsl_mc_io *mc_io, uint16_t dprc_handle,
> >>> +            struct dprc_endpoint *endpoint);
> >>> +
> >>> +/*! @} */
> >> 
> >> this entire file is riddled with non-kernel-doc comment markers:  see
> >> Documentation/kernel-doc-nano-HOWTO.txt on how to write function and
> >> other types of comments in a kernel-doc compliant format.
> > This is because this file is using doxygen comments, as it was developed
> > by another team. Unless someone else has an objection, I will leave the doxygen comments alone and not make any change here.
> 
> Do you see any other source files in Linux using doxygen comments? Mixing different documentation styles can easily become a big mess, because you can't generate external documentation consistently for the whole tree.

Thanks Alex,

Kim