| 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: <20151119094127.7db3522c@arm.com>
Date: Thu, 19 Nov 2015 09:41:27 +0000
From: Marc Zyngier <marc.zyngier@....com>
To: MaJun <majun258@...wei.com>
Cc: <Catalin.Marinas@....com>, <linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>, <Will.Deacon@....com>,
<mark.rutland@....com>, <jason@...edaemon.net>,
<tglx@...utronix.de>, <lizefan@...wei.com>, <huxinwei@...wei.com>,
<dingtianhong@...wei.com>, <zhaojunhua@...ilicon.com>,
<liguozhu@...ilicon.com>, <xuwei5@...ilicon.com>,
<wei.chenwei@...ilicon.com>, <guohanjun@...wei.com>,
<wuyun.wu@...wei.com>, <guodong.xu@...aro.org>,
<haojian.zhuang@...aro.org>, <zhangfei.gao@...aro.org>,
<usman.ahmad@...aro.org>, <klimov.linux@...il.com>,
<gabriele.paoloni@...wei.com>
Subject: Re: [PATCH v8 4/4] irqchip:implement the mbigen irq chip operation
functions
On Fri, 6 Nov 2015 16:28:42 +0800
MaJun <majun258@...wei.com> wrote:
> From: Ma Jun <majun258@...wei.com>
>
> Add the interrupt controller chip operation functions of mbigen chip.
>
> Signed-off-by: Ma Jun <majun258@...wei.com>
> ---
> drivers/irqchip/irq-mbigen.c | 102 +++++++++++++++++++++++++++++++++++++++--
> 1 files changed, 97 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
> index 71291cb..155c210 100644
> --- a/drivers/irqchip/irq-mbigen.c
> +++ b/drivers/irqchip/irq-mbigen.c
> @@ -46,6 +46,19 @@
> /* offset of vector register in mbigen node */
> #define REG_MBIGEN_VEC_OFFSET 0x200
>
> +/**
> + * offset of clear register in mbigen node
> + * This register is used to clear the status
> + * of interrupt
> + */
> +#define REG_MBIGEN_CLEAR_OFFSET 0xa00
> +
> +/**
> + * offset of interrupt type register
> + * This register is used to configure interrupt
> + * trigger type
> + */
> +#define REG_MBIGEN_TYPE_OFFSET 0x0
>
> /**
> * struct mbigen_device - holds the information of mbigen device.
> @@ -64,11 +77,19 @@ struct mbigen_device {
> * struct mbigen_irq_data - private data of each irq
> *
> * @base: mapped address of mbigen chip
> + * @pin_offset: local pin offset of interrupt.
> * @reg_vec: addr offset of interrupt vector register.
> + * @reg_type: addr offset of interrupt trigger type register.
> + * @reg_clear: addr offset of interrupt clear register.
> + * @type: interrupt trigger type.
> */
> struct mbigen_irq_data {
> void __iomem *base;
> + unsigned int pin_offset;
> unsigned int reg_vec;
> + unsigned int reg_type;
> + unsigned int reg_clear;
> + unsigned int type;
> };
I have the same comments here as for patch #3. You're storing
information that are just a pure function of hwirq, and essentially
free to compute at runtime. Please fix it in a similar way.
>
> static inline int get_mbigen_vec_reg(u32 nid, u32 offset)
> @@ -77,8 +98,68 @@ static inline int get_mbigen_vec_reg(u32 nid, u32 offset)
> + REG_MBIGEN_VEC_OFFSET;
> }
>
> +static int get_mbigen_type_reg(u32 nid, u32 offset)
> +{
> + int ofst;
> +
> + ofst = offset / 32 * 4;
> + return ofst + nid * MBIGEN_NODE_OFFSET
> + + REG_MBIGEN_TYPE_OFFSET;
> +}
> +
> +static int get_mbigen_clear_reg(u32 nid, u32 offset)
> +{
> + int ofst;
> +
> + ofst = offset / 32 * 4;
> + return ofst + nid * MBIGEN_NODE_OFFSET
> + + REG_MBIGEN_CLEAR_OFFSET;
> +}
> +
> +static void mbigen_eoi_irq(struct irq_data *data)
> +{
> + struct mbigen_irq_data *mgn_irq_data = irq_data_get_irq_chip_data(data);
> + u32 mask;
> +
> + /* only level triggered interrupt need to clear status */
> + if (mgn_irq_data->type == IRQ_TYPE_LEVEL_HIGH) {
> + mask = 1 << (mgn_irq_data->pin_offset % 32);
> + writel_relaxed(mask, mgn_irq_data->reg_clear + mgn_irq_data->base);
> + }
Does this have the effect of regenerating an edge if the level is still
active?
> +
> + irq_chip_eoi_parent(data);
> +}
> +
> +static int mbigen_set_type(struct irq_data *d, unsigned int type)
> +{
> + struct mbigen_irq_data *mgn_irq_data = irq_data_get_irq_chip_data(d);
> + u32 mask;
> + int val;
> +
> + if (type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
> + return -EINVAL;
If these are the only two interrupt triggers you support, then you
should update the documentation (DT binding) to reflect this, as all it
says is "The 2nd cell is the interrupt trigger type" which is pretty
vague.
> +
> + mask = 1 << (mgn_irq_data->pin_offset % 32);
> +
> + val = readl_relaxed(mgn_irq_data->reg_type + mgn_irq_data->base);
> +
> + if (type == IRQ_TYPE_LEVEL_HIGH)
> + val |= mask;
> + else if (type == IRQ_TYPE_EDGE_RISING)
> + val &= ~mask;
Given that you've excluded anything but LEVEL_HIGH and EDGE_RISING
already, the second if() is superfluous.
> +
> + writel_relaxed(val, mgn_irq_data->reg_type + mgn_irq_data->base);
> +
> + return 0;
> +}
> +
> static struct irq_chip mbigen_irq_chip = {
> .name = "mbigen-v2",
> + .irq_mask = irq_chip_mask_parent,
> + .irq_unmask = irq_chip_unmask_parent,
> + .irq_eoi = mbigen_eoi_irq,
> + .irq_set_type = mbigen_set_type,
> + .irq_set_affinity = irq_chip_set_affinity_parent,
> };
>
> static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg)
> @@ -94,10 +175,11 @@ static void mbigen_write_msg(struct msi_desc *desc, struct msi_msg *msg)
> writel_relaxed(val, mgn_irq_data->reg_vec + mgn_irq_data->base);
> }
>
> -static struct mbigen_irq_data *set_mbigen_irq_data(int hwirq)
> +static struct mbigen_irq_data *set_mbigen_irq_data(int hwirq,
> + unsigned int type)
> {
> struct mbigen_irq_data *datap;
> - unsigned int nid, pin_offset;
> + unsigned int nid;
>
> datap = kzalloc(sizeof(*datap), GFP_KERNEL);
> if (!datap)
> @@ -106,10 +188,20 @@ static struct mbigen_irq_data *set_mbigen_irq_data(int hwirq)
> /* get the mbigen node number */
> nid = (hwirq - RESERVED_IRQ_PER_MBIGEN_CHIP) / IRQS_PER_MBIGEN_NODE + 1;
>
> - pin_offset = (hwirq - RESERVED_IRQ_PER_MBIGEN_CHIP)
> + datap->pin_offset = (hwirq - RESERVED_IRQ_PER_MBIGEN_CHIP)
> % IRQS_PER_MBIGEN_NODE;
>
> - datap->reg_vec = get_mbigen_vec_reg(nid, pin_offset);
> + datap->reg_vec = get_mbigen_vec_reg(nid, datap->pin_offset);
> + datap->reg_type = get_mbigen_type_reg(nid, datap->pin_offset);
> +
> + /* no clear register for edge triggered interrupt */
> + if (type == IRQ_TYPE_EDGE_RISING)
> + datap->reg_clear = 0;
> + else
> + datap->reg_clear = get_mbigen_clear_reg(nid,
> + datap->pin_offset);
> +
> + datap->type = type;
> return datap;
> }
That function can entirely go.
>
> @@ -151,7 +243,7 @@ static int mbigen_irq_domain_alloc(struct irq_domain *domain,
> return err;
>
> /* set related information of this irq */
> - mgn_irq_data = set_mbigen_irq_data(hwirq);
> + mgn_irq_data = set_mbigen_irq_data(hwirq, type);
> if (!mgn_irq_data)
> return err;
>
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