| 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
| ||
|
Date: Fri, 06 Nov 2015 17:50:51 +0000
From: Marc Zyngier <marc.zyngier@....com>
To: "J. German Rivera" <German.Rivera@...escale.com>,
gregkh@...uxfoundation.org, arnd@...db.de,
devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org
CC: stuart.yoder@...escale.com, itai.katz@...escale.com,
lijun.pan@...escale.com, leoli@...escale.com,
scottwood@...escale.com, agraf@...e.de, bhamciu1@...escale.com,
R89243@...escale.com, bhupesh.sharma@...escale.com,
nir.erez@...escale.com, richard.schmitt@...escale.com,
dan.carpenter@...cle.com, jiang.liu@...ux.intel.com
Subject: Re: [PATCH v2 03/11] staging: fsl-mc: Added generic MSI support for
FSL-MC devices
On 30/10/15 19:43, J. German Rivera wrote:
> Created an MSI domain for the fsl-mc bus-- including functions
> to create a domain, find a domain, alloc/free domain irqs, and
> bus specific overrides for domain and irq_chip ops.
>
> Signed-off-by: J. German Rivera <German.Rivera@...escale.com>
> ---
> Changes in v2: none
>
> drivers/staging/fsl-mc/bus/Kconfig | 1 +
> drivers/staging/fsl-mc/bus/Makefile | 1 +
> drivers/staging/fsl-mc/bus/mc-msi.c | 276 ++++++++++++++++++++++++++++
> drivers/staging/fsl-mc/include/mc-private.h | 17 ++
> drivers/staging/fsl-mc/include/mc.h | 17 ++
> 5 files changed, 312 insertions(+)
> create mode 100644 drivers/staging/fsl-mc/bus/mc-msi.c
>
> diff --git a/drivers/staging/fsl-mc/bus/Kconfig b/drivers/staging/fsl-mc/bus/Kconfig
> index 0d779d9..c498ac6 100644
> --- a/drivers/staging/fsl-mc/bus/Kconfig
> +++ b/drivers/staging/fsl-mc/bus/Kconfig
> @@ -9,6 +9,7 @@
> config FSL_MC_BUS
> tristate "Freescale Management Complex (MC) bus driver"
> depends on OF && ARM64
> + select GENERIC_MSI_IRQ_DOMAIN
> help
> Driver to enable the bus infrastructure for the Freescale
> QorIQ Management Complex (fsl-mc). The fsl-mc is a hardware
> diff --git a/drivers/staging/fsl-mc/bus/Makefile b/drivers/staging/fsl-mc/bus/Makefile
> index 25433a9..a5f2ba4 100644
> --- a/drivers/staging/fsl-mc/bus/Makefile
> +++ b/drivers/staging/fsl-mc/bus/Makefile
> @@ -13,5 +13,6 @@ mc-bus-driver-objs := mc-bus.o \
> dpmng.o \
> dprc-driver.o \
> mc-allocator.o \
> + mc-msi.o \
> dpmcp.o \
> dpbp.o
> diff --git a/drivers/staging/fsl-mc/bus/mc-msi.c b/drivers/staging/fsl-mc/bus/mc-msi.c
> new file mode 100644
> index 0000000..81b53e3
> --- /dev/null
> +++ b/drivers/staging/fsl-mc/bus/mc-msi.c
> @@ -0,0 +1,276 @@
> +/*
> + * Freescale Management Complex (MC) bus driver MSI support
> + *
> + * Copyright (C) 2015 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.
> + */
> +
> +#include "../include/mc-private.h"
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
> +#include <linux/irqchip/arm-gic-v3.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/msi.h>
> +#include "../include/mc-sys.h"
> +#include "dprc-cmd.h"
> +
> +static void fsl_mc_msi_set_desc(msi_alloc_info_t *arg,
> + struct msi_desc *desc)
> +{
> + arg->desc = desc;
> + arg->hwirq = (irq_hw_number_t)desc->fsl_mc.msi_index;
> +}
> +
> +static void fsl_mc_msi_update_dom_ops(struct msi_domain_info *info)
> +{
> + struct msi_domain_ops *ops = info->ops;
> +
> + if (WARN_ON(!ops))
> + return;
> +
> + if (!ops->set_desc)
> + ops->set_desc = fsl_mc_msi_set_desc;
When would that be overridden by the MSI controller?
> +}
> +
> +static void __fsl_mc_msi_write_msg(struct fsl_mc_device *mc_bus_dev,
> + struct fsl_mc_device_irq *mc_dev_irq)
> +{
> + int error;
> + struct fsl_mc_device *owner_mc_dev = mc_dev_irq->mc_dev;
> + struct msi_desc *msi_desc = mc_dev_irq->msi_desc;
> + struct dprc_irq_cfg irq_cfg;
> +
> + /*
> + * msi_desc->msg.address is 0x0 when this function is invoked in
> + * the free_irq() code path. In this case, for the MC, we don't
> + * really need to "unprogram" the MSI, so we just return.
> + */
> + if (msi_desc->msg.address_lo == 0x0 && msi_desc->msg.address_hi == 0x0)
> + return;
> +
> + if (WARN_ON(!owner_mc_dev))
> + return;
> +
> + irq_cfg.paddr = ((u64)msi_desc->msg.address_hi << 32) |
> + msi_desc->msg.address_lo;
This should really be a phys_addr_t.
> + irq_cfg.val = msi_desc->msg.data;
> + irq_cfg.user_irq_id = msi_desc->irq;
> +
> + /*
> + * DPRCs and other objects use different commands to set up IRQs,
> + * so we have to differentiate them here.
> + */
> + if (owner_mc_dev == mc_bus_dev) {
> + /*
> + * IRQ is for the mc_bus_dev's DPRC itself
> + */
> + error = dprc_set_irq(mc_bus_dev->mc_io,
> + MC_CMD_FLAG_INTR_DIS | MC_CMD_FLAG_PRI,
> + mc_bus_dev->mc_handle,
> + mc_dev_irq->dev_irq_index,
> + &irq_cfg);
> + if (error < 0) {
> + dev_err(&owner_mc_dev->dev,
> + "dprc_set_irq() failed: %d\n", error);
> + }
> + } else {
> + error = dprc_set_obj_irq(mc_bus_dev->mc_io,
> + MC_CMD_FLAG_INTR_DIS | MC_CMD_FLAG_PRI,
> + mc_bus_dev->mc_handle,
> + owner_mc_dev->obj_desc.type,
> + owner_mc_dev->obj_desc.id,
> + mc_dev_irq->dev_irq_index,
> + &irq_cfg);
> + if (error < 0) {
> + dev_err(&owner_mc_dev->dev,
> + "dprc_obj_set_irq() failed: %d\n", error);
> + }
> + }
It feels a bit odd that you have all of this under a single MSI
umbrella, and yet have to differentiate between them. Do they have a
different programming interface? Or is that because these dprc_set_*_irq
functions do some other stuff behind the scene (I'm too lazy to check...)?
> +}
> +
> +/*
> + * NOTE: This function is invoked with interrupts disabled
> + */
> +static void fsl_mc_msi_write_msg(struct irq_data *irq_data,
> + struct msi_msg *msg)
> +{
> + struct msi_desc *msi_desc = irq_data_get_msi_desc(irq_data);
> + struct fsl_mc_device *mc_bus_dev = to_fsl_mc_device(msi_desc->dev);
> + struct fsl_mc_bus *mc_bus = to_fsl_mc_bus(mc_bus_dev);
> + struct fsl_mc_device_irq *mc_dev_irq =
> + &mc_bus->irq_resources[msi_desc->fsl_mc.msi_index];
> +
> + WARN_ON(mc_dev_irq->msi_desc != msi_desc);
> + msi_desc->msg = *msg;
I wonder why we don't do that in core code...
> +
> + /*
> + * Program the MSI (paddr, value) pair in the device:
> + */
> + __fsl_mc_msi_write_msg(mc_bus_dev, mc_dev_irq);
> +}
> +
> +static void fsl_mc_msi_update_chip_ops(struct msi_domain_info *info)
> +{
> + struct irq_chip *chip = info->chip;
> +
> + if (WARN_ON((!chip)))
> + return;
> +
> + if (!chip->irq_write_msi_msg)
> + chip->irq_write_msi_msg = fsl_mc_msi_write_msg;
Same as above. Do you foresee any circumstances where you you wouldn't
use this function? My hunch is that you should warn if
chip->irq_write_msi_msg is provided by the caller.
> +}
[...]
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
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