lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 19 Sep 2014 12:19:12 -0500 From: Kim Phillips <kim.phillips@...escale.com> To: German Rivera <German.Rivera@...escale.com> CC: Alexander Graf <agraf@...e.de>, "<gregkh@...uxfoundation.org>" <gregkh@...uxfoundation.org>, "<arnd@...db.de>" <arnd@...db.de>, "<linux-kernel@...r.kernel.org>" <linux-kernel@...r.kernel.org>, "<stuart.yoder@...escale.com>" <stuart.yoder@...escale.com>, "<scottwood@...escale.com>" <scottwood@...escale.com>, "<linuxppc-release@...ux.freescale.net>" <linuxppc-release@...ux.freescale.net> Subject: Re: [PATCH 1/4] drivers/bus: Added Freescale Management Complex APIs On Thu, 18 Sep 2014 22:05:03 -0500 German Rivera <German.Rivera@...escale.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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists