| 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: <20131030191034.GN20207@joshc.qualcomm.com>
Date: Wed, 30 Oct 2013 14:10:34 -0500
From: Josh Cartwright <joshc@...eaurora.org>
To: Stephen Boyd <sboyd@...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
Stephen-
Thanks for all the comments.
On Wed, Oct 30, 2013 at 11:17:55AM -0700, Stephen Boyd wrote:
> 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.
Indeed, will remove (I think this is a holdout from the split qpnpint
handling that exists in the vendor tree).
> > + 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?
Indeed, I removed this for some reason awhile back, but this really
should be put back into place, and the EE should be a required property
in the bindings.
[..]
> > +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.
Indeed, I'll move it and add a comment at init-time.
> > +
> > + *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.
Nice catch, it wasn't my intent to ignore it.
Thanks again,
Josh
--
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