| 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: <20131030181755.GJ21983@codeaurora.org>
Date: Wed, 30 Oct 2013 11:17:55 -0700
From: Stephen Boyd <sboyd@...eaurora.org>
To: Josh Cartwright <joshc@...eaurora.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-arm-msm@...r.kernel.org,
Sagar Dharia <sdharia@...eaurora.org>,
Gilad Avidov <gavidov@...eaurora.org>,
Michael Bohan <mbohan@...eaurora.org>
Subject: Re: [PATCH v3 05/10] spmi: pmic_arb: add support for interrupt
handling
On 10/28, Josh Cartwright wrote:
> @@ -108,12 +111,17 @@ struct spmi_pmic_arb_dev {
> void __iomem *base;
> void __iomem *intr;
> void __iomem *cnfg;
> + unsigned int irq;
> bool allow_wakeup;
> spinlock_t lock;
> u8 channel;
> u8 min_apid;
> u8 max_apid;
> u32 mapping_table[SPMI_MAPPING_TABLE_LEN];
> + int bus_nr;
This looks unused.
> + struct irq_domain *domain;
> + struct spmi_controller *spmic;
> + u16 apid_to_ppid[256];
> };
>
> static inline u32 pmic_arb_base_read(struct spmi_pmic_arb_dev *dev, u32 offset)
[...]
> +
> +static void pmic_arb_chained_irq(unsigned int irq, struct irq_desc *desc)
> +{
> + struct spmi_pmic_arb_dev *pa = irq_get_handler_data(irq);
> + struct irq_chip *chip = irq_get_chip(irq);
> + void __iomem *intr = pa->intr;
> + int first = pa->min_apid >> 5;
> + int last = pa->max_apid >> 5;
> + int i, id;
> + u8 ee = 0; /*pa->owner;*/
TODO?
> + u32 status;
> +
> + chained_irq_enter(chip, desc);
> +
> + for (i = first; i <= last; ++i) {
> + status = readl_relaxed(intr + SPMI_PIC_OWNER_ACC_STATUS(ee, i));
> + while (status) {
> + id = ffs(status) - 1;
> + status &= ~(1 << id);
> + periph_interrupt(pa, id + i * 32);
> + }
> + }
> +
> + chained_irq_exit(chip, desc);
> +}
> +
[...]
> +static int qpnpint_irq_domain_dt_translate(struct irq_domain *d,
> + struct device_node *controller,
> + const u32 *intspec,
> + unsigned int intsize,
> + unsigned long *out_hwirq,
> + unsigned int *out_type)
> +{
> + struct spmi_pmic_arb_dev *pa = d->host_data;
> + struct spmi_pmic_arb_irq_spec spec;
> + int err;
> + u8 apid;
> +
> + dev_dbg(&pa->spmic->dev,
> + "intspec[0] 0x%1x intspec[1] 0x%02x intspec[2] 0x%02x\n",
> + intspec[0], intspec[1], intspec[2]);
> +
> + if (d->of_node != controller)
> + return -EINVAL;
> + if (intsize != 4)
> + return -EINVAL;
> + if (intspec[0] > 0xF || intspec[1] > 0xFF || intspec[2] > 0x7)
> + return -EINVAL;
> +
> + spec.slave = intspec[0];
> + spec.per = intspec[1];
> + spec.irq = intspec[2];
> +
> + err = search_mapping_table(pa, &spec, &apid);
> + if (err)
> + return err;
> +
> + pa->apid_to_ppid[apid] = spec.slave << 8 | spec.per;
> +
> + /* Keep track of {max,min}_apid for bounding search during interrupt */
> + if (apid > pa->max_apid)
> + pa->max_apid = apid;
> + if (apid < pa->min_apid)
> + pa->min_apid = apid;
Ah makes sense now why we set this to the opposite values in
probe. Please put a comment in patch 4 and maybe move that setup
in patch 4 to this patch.
> +
> + *out_hwirq = spec.slave << 24
> + | spec.per << 16
> + | spec.irq << 8
> + | apid;
> + *out_type = intspec[3] & IRQ_TYPE_SENSE_MASK;
> +
> + dev_dbg(&pa->spmic->dev, "out_hwirq = %lu\n", *out_hwirq);
> +
> + return 0;
> +}
> +
[...]
> static int spmi_pmic_arb_probe(struct platform_device *pdev)
> {
> struct spmi_pmic_arb_dev *pa;
> struct spmi_controller *ctrl;
> struct resource *res;
> - int err, i;
> + int err = 0, i;
>
> ctrl = spmi_controller_alloc(&pdev->dev, sizeof(*pa));
> if (!ctrl)
> return -ENOMEM;
>
> pa = spmi_controller_get_drvdata(ctrl);
> + pa->spmic = ctrl;
>
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core");
> pa->base = devm_ioremap_resource(&ctrl->dev, res);
> @@ -349,6 +679,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
> goto err_put_ctrl;
> }
>
> + pa->irq = platform_get_irq(pdev, 0);
> + if (pa->irq < 0) {
> + err = pa->irq;
> + goto err_put_ctrl;
> + }
> +
> for (i = 0; i < ARRAY_SIZE(pa->mapping_table); ++i)
> pa->mapping_table[i] = readl_relaxed(
> pa->cnfg + SPMI_MAPPING_TABLE_REG(i));
> @@ -364,15 +700,30 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
> ctrl->read_cmd = pmic_arb_read_cmd;
> ctrl->write_cmd = pmic_arb_write_cmd;
>
> + dev_dbg(&pdev->dev, "adding irq domain\n");
> + pa->domain = irq_domain_add_tree(pdev->dev.of_node,
> + &pmic_arb_irq_domain_ops, pa);
> + if (!pa->domain) {
> + dev_err(&pdev->dev, "unable to create irq_domain\n");
> + goto err_put_ctrl;
Why do we silently ignore the error here? Is the irqchip
functionality optional? Setting err here should allow us to skip
intializing err to 0 up at the top of this function.
> + }
> +
> + irq_set_handler_data(pa->irq, pa);
> + irq_set_chained_handler(pa->irq, pmic_arb_chained_irq);
> +
> err = spmi_controller_add(ctrl);
> if (err)
> - goto err_put_ctrl;
> + goto err_domain_remove;
>
> dev_dbg(&ctrl->dev, "PMIC Arb Version 0x%x\n",
> pmic_arb_base_read(pa, PMIC_ARB_VERSION));
>
> return 0;
>
> +err_domain_remove:
> + irq_set_chained_handler(pa->irq, NULL);
> + irq_set_handler_data(pa->irq, NULL);
> + irq_domain_remove(pa->domain);
> err_put_ctrl:
> spmi_controller_put(ctrl);
> return err;
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
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