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 linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <20140915184451.962a3a19c4940792d182f10a@freescale.com> Date: Mon, 15 Sep 2014 18:44:51 -0500 From: Kim Phillips <kim.phillips@...escale.com> To: "J. German Rivera" <German.Rivera@...escale.com> CC: <gregkh@...uxfoundation.org>, <arnd@...db.de>, <linux-kernel@...r.kernel.org>, <stuart.yoder@...escale.com>, <Kim.Phillips@...escale.com>, <scottwood@...escale.com>, <agraf@...e.de>, <linuxppc-release@...ux.freescale.net> Subject: Re: [PATCH 1/4] drivers/bus: Added Freescale Management Complex APIs On Thu, 11 Sep 2014 12:34:21 -0500 "J. German Rivera" <German.Rivera@...escale.com> wrote: > From: "J. German Rivera" <German.Rivera@...escale.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@...escale.com> > Signed-off-by: Stuart Yoder <stuart.yoder@...escale.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? Also, any reason why these and future include files aren't being put in include/linux/fsl/, so as to not pollute the top level include/linux/? That way, we can also remove the fsl- prefix from those filenames, too.. > diff --git a/drivers/bus/fsl-mc/dpmng.c b/drivers/bus/fsl-mc/dpmng.c > new file mode 100644 > index 0000000..c6ed27c > --- /dev/null > +++ b/drivers/bus/fsl-mc/dpmng.c > @@ -0,0 +1,93 @@ > +/* Copyright 2013-2014 Freescale Semiconductor Inc. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions are met: > + * * Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * * Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in the > + * documentation and/or other materials provided with the distribution. > + * * Neither the name of Freescale Semiconductor nor the > + * names of its contributors may be used to endorse or promote products > + * derived from this software without specific prior written permission. > + * > + * > + * ALTERNATIVELY, this software may be distributed under the terms of the > + * GNU General Public License ("GPL") as published by the Free Software > + * Foundation, either version 2 of that License or (at your option) any > + * later version. > + * > + * THIS SOFTWARE IS PROVIDED BY Freescale Semiconductor ``AS IS'' AND ANY interesting, normally this text reads: "THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS" ...does that mean we're excluding non-Freescale copyright holders and contributors from this warranty statement? That doesn't seem appropriate for an upstream kernel submission. > + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED > + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE > + * DISCLAIMED. IN NO EVENT SHALL Freescale Semiconductor BE LIABLE FOR ANY > + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES > + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; > + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND > + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS > + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + */ This dual BSD-3-clause/GPL license doesn't match that of patch 2's drivers/bus/fsl-mc/fsl_mc_bus.c, GPLv2: +/* + * Freescale Management Complex (MC) bus driver + * + * Copyright (C) 2014 Freescale Semiconductor, Inc. + * Author: German Rivera <German.Rivera@...escale.com> + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + */ any reason the licenses are different? > +int mc_get_version(struct fsl_mc_io *mc_io, struct mc_version *mc_ver_info) > +{ > + struct mc_command cmd = { 0 }; we can save some cycles if this initialization is not absolutely necessary: is it? i.e., does the h/w actually look at the params section when doing a get_version? not sure to what other commands this comment would apply to...at least get_container_id, but maybe more - all of them? > diff --git a/drivers/bus/fsl-mc/fsl_mc_sys.c b/drivers/bus/fsl-mc/fsl_mc_sys.c > +/** > + * Map an MC portal in the kernel virtual address space > + */ > +static int map_mc_portal(phys_addr_t mc_portal_phys_addr, > + uint32_t mc_portal_size, > + void __iomem **new_mc_portal_virt_addr) > +{ > + void __iomem *mc_portal_virt_addr = NULL; > + struct resource *res = NULL; > + int error = -EINVAL; > + > + res = > + request_mem_region(mc_portal_phys_addr, mc_portal_size, > + "mc_portal"); > + if (res == NULL) { > + pr_err("request_mem_region() failed for MC portal %#llx\n", > + mc_portal_phys_addr); > + error = -EBUSY; > + goto error; > + } > + > + mc_portal_virt_addr = ioremap_nocache(mc_portal_phys_addr, > + mc_portal_size); > + if (mc_portal_virt_addr == NULL) { > + pr_err("ioremap_nocache() failed for MC portal %#llx\n", > + mc_portal_phys_addr); > + error = -EFAULT; > + goto error; > + } > + > + *new_mc_portal_virt_addr = mc_portal_virt_addr; > + return 0; > +error: > + if (mc_portal_virt_addr != NULL) > + iounmap(mc_portal_virt_addr); > + > + if (res != NULL) > + release_mem_region(mc_portal_phys_addr, mc_portal_size); > + > + return error; > +} unnecessary initializations, bad error codes (both should be -ENOMEM), 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. > +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. > +error: > + if (mc_io != NULL) { > + if (mc_io->portal_virt_addr != NULL) { > + unmap_mc_portal(mc_portal_phys_addr, > + mc_portal_size, > + mc_io->portal_virt_addr); > + } > + > + kfree(mc_io); kfree can handle being passed NULL, but again, might want to consider using devm_ functions instead. > + } > + > + return error; > +} > +EXPORT_SYMBOL_GPL(fsl_create_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. > + mc_io->portal_virt_addr = NULL; > + } > + > + mc_io->magic = 0x0; > + kfree(mc_io); > +} > +EXPORT_SYMBOL_GPL(fsl_destroy_mc_io); > + > +static int mc_status_to_error(enum mc_cmd_status status) > +{ > + switch (status) { > + case MC_CMD_STATUS_OK: > + return 0; > + case MC_CMD_STATUS_AUTH_ERR: > + return -EACCES; > + case MC_CMD_STATUS_NO_PRIVILEGE: > + return -EPERM; > + case MC_CMD_STATUS_DMA_ERR: > + return -EIO; > + case MC_CMD_STATUS_CONFIG_ERR: > + return -EINVAL; > + case MC_CMD_STATUS_TIMEOUT: > + return -ETIMEDOUT; > + case MC_CMD_STATUS_NO_RESOURCE: > + return -ENAVAIL; > + case MC_CMD_STATUS_NO_MEMORY: > + return -ENOMEM; > + case MC_CMD_STATUS_BUSY: > + return -EBUSY; > + case MC_CMD_STATUS_UNSUPPORTED_OP: > + return -ENOTSUP; > + case MC_CMD_STATUS_INVALID_STATE: > + return -ENODEV; > + default: > + break; > + } > + > + /* Not expected to reach here */ > + return -EINVAL; but if it does, callsite can't disambiguate between that and e.g., MC_CMD_STATUS_CONFIG_ERR. Also, this would be more readable as a static const lookup table. > +} > + > +int mc_send_command(struct fsl_mc_io *mc_io, struct mc_command *cmd) > +{ > + enum mc_cmd_status status; > + unsigned long irqsave_flags = 0; > + int error = -EINVAL; > + bool lock_acquired = false; > + unsigned long jiffies_until_timeout = > + jiffies + MC_CMD_COMPLETION_TIMEOUT; > + > + if (WARN_ON(mc_io->magic != FSL_MC_IO_MAGIC)) > + goto out; if the h/w signals an error on the bad magic condition, s/w doesn't need to check it in its fast path. > + if (mc_io->flags & FSL_MC_IO_PORTAL_SHARED) { > + spin_lock_irqsave(&mc_io->spinlock, irqsave_flags); > + lock_acquired = true; > + } > + > + mc_write_command(mc_io->portal_virt_addr, cmd); > + > + for (;;) { > + status = mc_read_response(mc_io->portal_virt_addr, cmd); > + if (status != MC_CMD_STATUS_READY) > + break; > + > + /* > + * TODO: When MC command completion interrupts are supported > + * call wait function here instead of udelay() > + */ > + udelay(MC_CMD_COMPLETION_POLLING_INTERVAL); > + if (time_after_eq(jiffies, jiffies_until_timeout)) { > + error = -ETIMEDOUT; > + goto out; > + } > + } > + > + error = mc_status_to_error(status); > +out: > + if (lock_acquired) > + spin_unlock_irqrestore(&mc_io->spinlock, irqsave_flags); so if the portal is shared, we take a lock, disable interrupts, and then potentially udelay for a whopping 500usec, then check to see if _100_usec have passed, and thus _always_ issue a timeout error, even if the device took < 100usec to consume the command??? Not to mention this code will spin perpetually with IRQs disabled if the read_response never returns ready. I also don't see a reason why IRQs are being disabled in the first place - it's not being used in an IRQ handler...perhaps we need to wait until command completion IRQs are supported :) > +/** > + * @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. > +/** > + * @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. > +#ifdef FSL_MC_FIRMWARE > +/* > + * MC firmware decodes MC command parameters and encodes MC response parameters > + */ > + > +#define MC_CMD_PARAM_OP(_cmd, _param, _offset, _width, _type, _arg) \ > + ((_arg) = (_type)u64_dec((_cmd).params[_param], (_offset), (_width))) > + > +#define MC_RSP_PARAM_OP(_cmd, _param, _offset, _width, _type, _arg) \ > + ((_cmd).params[_param] |= u64_enc((_offset), (_width), (_type)(_arg))) > + > +#else > +/* > + * MC clients (GPP side) encode MC command parameters and decode MC response > + * parameters > + */ > + > +#define MC_CMD_PARAM_OP(_cmd, _param, _offset, _width, _type, _arg) \ > + ((_cmd).params[_param] |= u64_enc((_offset), (_width), (_type)(_arg))) > + > +#define MC_RSP_PARAM_OP(_cmd, _param, _offset, _width, _type, _arg) \ > + ((_arg) = (_type)u64_dec((_cmd).params[_param], (_offset), (_width))) > + > +#endif /* FSL_MC_FIRMWARE */ FSL_MC_FIRMWARE isn't being defined anywhere; remove. > diff --git a/include/linux/fsl_mc_sys.h b/include/linux/fsl_mc_sys.h ... > +#ifndef ENOTSUP > +#define ENOTSUP 95 > +#endif This is already being defined as EOPNOTSUPP: either use that, ENOTSUPP, or, if you can justify it, patch a more generic errno.h as a separate patch, earlier in the patchseries. > +#define ioread64(_p) readq(_p) > +#define iowrite64(_v, _p) writeq(_v, _p) these definitions have names that are too generic to belong in a FSL h/w header: conflicts will be introduced once the existing io{read,write}32 functions get promoted. Either use readq/writeq directly, or, if you can justify it, patch a more generic io.h. Also, is there a reason the 'relaxed' versions of the i/o accessors aren't being used? I'm stopping my review here, since I expect numerous changes in the subsequent patches as a result of changes to this one in the next version of the series. Thanks, 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