| 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: <alpine.LFD.2.00.1103171004460.2787@localhost6.localdomain6>
Date: Thu, 17 Mar 2011 10:59:02 +0100 (CET)
From: Thomas Gleixner <tglx@...utronix.de>
To: Abhijeet Dharmapurikar <adharmap@...eaurora.org>
cc: davidb@...eaurora.org, dwalker@...o99.com,
"David S. Miller" <davem@...emloft.net>,
Andrew Morton <akpm@...ux-foundation.org>,
Bryan Huntsman <bryanh@...eaurora.org>,
David Collins <collinsd@...eaurora.org>,
Grant Likely <grant.likely@...retlab.ca>,
Greg Kroah-Hartman <gregkh@...e.de>,
Joe Perches <joe@...ches.com>,
Russell King <linux@....linux.org.uk>,
Samuel Ortiz <sameo@...ux.intel.com>,
Stepan Moskovchenko <stepanm@...eaurora.org>,
Mark Brown <broonie@...nsource.wolfsonmicro.com>,
Linus Walleij <linux.walleij@...aro.org>,
linux-arm-kernel@...ts.infradead.org,
linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PM8921 MFD V3 2/6] mfd: pm8xxx: Add irq support
On Wed, 16 Mar 2011, adharmap@...eaurora.org wrote:
> +static void pm8xxx_irq_handler(unsigned int irq, struct irq_desc *desc)
> +{
> + struct pm_irq_chip *chip = get_irq_data(irq);
> + struct irq_chip *irq_chip = get_irq_chip(irq);
Sigh. You have a reference to irq_desc, so why want you to get the
data with a sparse lookup ?
chip = irq_desc_get_handler_data(desc);
irq_chip = irq_desc_get_chip(desc);
> + u8 root;
> + int i, ret, masters = 0;
> +
> + ret = pm8xxx_read_root_irq(chip, &root);
> + if (ret) {
> + pr_err("Can't read root status ret=%d\n", ret);
> + return;
> + }
> +
> + /* on pm8xxx series masters start from bit 1 of the root */
> + masters = root >> 1;
> +
> + /* Read allowed masters for blocks. */
> + for (i = 0; i < chip->num_masters; i++)
> + if (masters & (1 << i))
> + pm8xxx_irq_master_handler(chip, i);
> +
> + irq_chip->irq_ack(&desc->irq_data);
> +}
> +
> +static void pm8xxx_irq_ack(struct irq_data *d)
> +{
> + struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
> + unsigned int pmirq = d->irq - chip->irq_base;
> + u8 block, config;
> +
> + block = pmirq / 8;
> +
> + config = chip->config[pmirq] | PM_IRQF_CLR;
> + /* Keep the mask */
> + if (!(chip->irqs_allowed[block] & (1 << (pmirq % 8))))
> + config |= PM_IRQF_MASK_ALL;
Why do you insist to keep that state thing around? Implement a
mask_ack() callback and get rid of it.
> + pm8xxx_config_irq(chip, block, config);
> +}
> +
> +static int pm8xxx_irq_set_wake(struct irq_data *d, unsigned int on)
> +{
> + struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
> + unsigned int pmirq = d->irq - chip->irq_base;
> +
> + if (on) {
> + set_bit(pmirq, chip->wake_enable);
> + chip->count_wakeable++;
> + } else {
> + clear_bit(pmirq, chip->wake_enable);
> + chip->count_wakeable--;
> + }
This bitfield and the count are completely useless. Get rid of it.
> + return 0;
> +}
> +
> +static struct irq_chip pm8xxx_irq_chip = {
> + .name = "pm8xxx",
> + .irq_ack = pm8xxx_irq_ack,
> + .irq_mask = pm8xxx_irq_mask,
> + .irq_unmask = pm8xxx_irq_unmask,
> + .irq_set_type = pm8xxx_irq_set_type,
> + .irq_set_wake = pm8xxx_irq_set_wake,
> + .flags = IRQCHIP_MASK_ON_SUSPEND,
> +};
> +
> +/**
> + * pm8xxx_get_irq_stat - get the status of the irq line
> + * @dev: the interrupt device
Please run this through the doc generator. dev is not an argument to
this function.
> + * @irq: the irq number
> + *
> + * The pm8xxx gpio and mpp rely on the interrupt block to read
> + * the values on their pins. This function is to facilitate reading
> + * the status of a gpio or an mpp line. The caller has to convert the
> + * gpio number to irq number.
> + *
> + * RETURNS:
> + * an int indicating the value read on that line
> + */
> +int pm8xxx_get_irq_stat(void *data, int irq)
Why is this void * ?
> +void * __devinit pm8xxx_irq_init(struct device *dev,
> + const struct pm8xxx_irq_platform_data *pdata)
Please return struct pm_irq_chip *
You don't have to make the struct definition public. All you need in
the shared header file is a forward declaration.
struct pm_irq_chip;
The you can operate with type safe pointers at the callsites, but you
cannot dereference them there.
> +{
> + struct pm_irq_chip *chip;
> + int devirq, rc;
> + unsigned int pmirq;
> +
> + if (!pdata) {
> + pr_err("No platform data\n");
> + return ERR_PTR(-EINVAL);
> + }
> +
> + devirq = pdata->devirq;
> + if (devirq < 0) {
> + pr_err("missing devirq\n");
> + rc = devirq;
> + return ERR_PTR(-EINVAL);
> + }
> +
> + chip = kzalloc(sizeof(struct pm_irq_chip), GFP_KERNEL);
> + if (!chip) {
> + pr_err("Cannot alloc pm_irq_chip struct\n");
> + return ERR_PTR(-EINVAL);
> + }
> +
> + chip->dev = dev;
> + chip->devirq = devirq;
> + chip->irq_base = pdata->irq_base;
> + chip->num_irqs = pdata->irq_cdata.nirqs;
> + chip->num_blocks = DIV_ROUND_UP(chip->num_irqs, 8);
> + chip->num_masters = DIV_ROUND_UP(chip->num_blocks, 8);
> + spin_lock_init(&chip->pm_irq_lock);
> +
> + chip->irqs_allowed = kzalloc(sizeof(u8) * chip->num_blocks, GFP_KERNEL);
> + if (!chip->irqs_allowed) {
> + pr_err("Cannot alloc irqs_allowed array\n");
> + rc = -ENOMEM;
> + goto free_all;
> + }
> +
> + chip->config = kzalloc(sizeof(u8) * chip->num_irqs, GFP_KERNEL);
> + if (!chip->config) {
> + pr_err("Cannot alloc config array\n");
> + rc = -ENOMEM;
> + goto free_all;
> + }
> + chip->wake_enable = kzalloc(sizeof(unsigned long)
> + * DIV_ROUND_UP(chip->num_irqs, BITS_PER_LONG),
> + GFP_KERNEL);
> + if (!chip->wake_enable) {
> + pr_err("Cannot alloc wake_enable array\n");
> + rc = -ENOMEM;
> + goto free_all;
> + }
If you got rid of all that state crap, then you can do:
struct pm_irq_chip {
...
...
u8 config[0];
};
and allocate the whole thing in one go.
chip = kzalloc(sizeof(struct pm_irq_chip) + sizeof(u8) * num_irqs, GFP_KERNEL);
>
> +static inline int pm8xxx_read_irq_stat(const struct device *dev, int irq)
> +{
> + struct pm8xxx_drvdata *dd = dev_get_drvdata(dev);
> +
> + BUG_ON(!dd);
No need for BUG_ON(). You can handle that gracefully with a proper
return code.
> + return dd->pmic_read_irq_stat(dev, irq);
> +}
Thanks,
tglx
--
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