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

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

On Thu, 18 Sep 2014 22:05:03 -0500
German Rivera <German.Rivera@freescale.com> wrote:

> On 09/18/2014 03:22 PM, Kim Phillips wrote:
> > On Thu, 18 Sep 2014 15:14:03 +0200
> 
> >>> 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.
> >
> I will refactor this function to use devm_ functions, to simplify the 
> error cleanup logic as you suggested, but still keep the current
> signature of the function, as I don't think it is a good practice
> to just return NULL in case of error, hiding the actual cause of the 
> error. Also, mixing returning a valid pointer and an error encoded
> as an invalid pointer is not clean and can be error-prone for callers, 
> if some caller just checks for NULL instead of using IS_ERR() ir
> IS_ERR_OR_NULL().

we'll make sure callers don't do that then :)

> >>>>> +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.
> >
> All this will be gone with the refactoring to use devm_ APIs.

sounds good.

> >>>>> +        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?
> >
> In this particular case, this comment doe snot apply anymore, as
> the magic filed will be removed.

it still applies for portal_virt_addr.

> >>>>> +/**
> >>>>> + * @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.
> >
> As I mentioned in the reply to Alex, I will remove the minor version check.

the code should be able to run on all subsequent versions of the
h/w, even in the major version case.

Kim