Thanks for the review Thomas!
On 23/04/13 16:09, Thomas Gleixner wrote:
> On Tue, 23 Apr 2013, James Hogan wrote:
>> +/**
>> + * struct pdc_intc_priv - private pdc interrupt data.
>> + * @nr_perips: Number of peripheral interrupt signals.
>> + * @nr_syswakes: Number of syswake signals.
>> + * @perip_irqs: List of peripheral IRQ numbers handled.
>> + * @syswake_irq: Shared PDC syswake IRQ number.
>> + * @domain: IRQ domain for PDC peripheral and syswake IRQs.
>> + * @pdc_base: Base of PDC registers.
>> + * @lock: Lock to protect the PDC syswake registers.
>> + */
>> +struct pdc_intc_priv {
>> + unsigned int nr_perips;
>> + unsigned int nr_syswakes;
>> + unsigned int *perip_irqs;
>> + unsigned int syswake_irq;
>> + struct irq_domain *domain;
>> + void __iomem *pdc_base;
>> +
>> + spinlock_t lock;
>
> raw_spinlock_t please
Okay.
If I understand right, this would be because on RT, spinlock_t becomes a
mutex and won't work correctly with irqs actually disabled for the irq
callbacks below, is that right?
>> +static void perip_irq_mask(struct irq_data *data)
>> +{
>> + struct pdc_intc_priv *priv = irqd_to_priv(data);
>> + unsigned int irq_route;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&priv->lock, flags);
>
> This is always called with interrupts disabled.
Okay, I'll switch to raw_spin_lock
>> + irq_route = pdc_read(priv, PDC_IRQ_ROUTE);
>> + irq_route &= ~(1 << data->hwirq);
>
> Why not cache the mask value ?
Yes, caching PDC_IRQ_ROUTE should be possible since it should only be
used by this driver (hence the driver local spinlock).
>
>> + pdc_write(priv, PDC_IRQ_ROUTE, irq_route);
>
>> + spin_unlock_irqrestore(&priv->lock, flags);
>> +}
>> +
>> +static void perip_irq_unmask(struct irq_data *data)
>> +{
>> + struct pdc_intc_priv *priv = irqd_to_priv(data);
>> + unsigned int irq_route;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&priv->lock, flags);
>> + irq_route = pdc_read(priv, PDC_IRQ_ROUTE);
>> + irq_route |= 1 << data->hwirq;
>> + pdc_write(priv, PDC_IRQ_ROUTE, irq_route);
>
> This code is another slightly different copy of stuff which is
> available in kernel/irq/generic-chip.c
>
> Can we please stop the code duplication and reuse existing
> infrastructure? Don't tell me it does not work for you. I sent out a
> patch yesterday which makes the code suitable for irq domains.
I'll look into this. kernel/irq/generic-chip.c was added after this
driver was written.
>> +static void pdc_intc_perip_isr(unsigned int irq, struct irq_desc *desc)
>> +{
>> + struct pdc_intc_priv *priv;
>> + unsigned int i, irq_no;
>> +
>> + priv = (struct pdc_intc_priv *)irq_desc_get_handler_data(desc);
>> +
>> + /* find the peripheral number */
>> + for (i = 0; i < priv->nr_perips; ++i)
>> + if (irq == priv->perip_irqs[i])
>> + goto found;
>
> Whee. Aren't these numbers linear ?
Not necessarily as they're virtual irq numbers obtained via
platform_get_irq(), which come individually from device tree. Even their
hardware irq numbers aren't linear as they're not wired to their irqchip
in the same order:
> pdc: pdc@0x02006000 {
> interrupt-controller;
> #interrupt-cells = <3>;
>
> reg = <0x02006000 0x1000>;
> compatible = "img,pdc-intc";
>
> num-perips = <3>;
> num-syswakes = <3>;
>
> interrupts = <18 4 /* level */>, /* Syswakes */
> <30 4 /* level */>, /* Perip 0 (RTC) */
> <29 4 /* level */>, /* Perip 1 (IR) */
> <31 4 /* level */>; /* Perip 2 (WDT) */
> };
>> +static int pdc_intc_probe(struct platform_device *pdev)
>> +{
>> + struct pdc_intc_priv *priv;
>> + struct device_node *node = pdev->dev.of_node;
>> + struct resource *res_regs;
>> + unsigned int i;
>> + int irq, ret;
>> + u32 val;
>> +
>> + if (!node)
>> + return -ENOENT;
>> +
>> + /* Get registers */
>> + res_regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (res_regs == NULL) {
>> + dev_err(&pdev->dev, "cannot find registers resource\n");
>> + return -ENOENT;
>> + }
>> +
>> + /* Allocate driver data */
>> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv) {
>> + dev_err(&pdev->dev, "cannot allocate device data\n");
>> + return -ENOMEM;
>> + }
>> + spin_lock_init(&priv->lock);
>> + platform_set_drvdata(pdev, priv);
>> +
>> + /* Ioremap the registers */
>> + priv->pdc_base = devm_ioremap(&pdev->dev, res_regs->start,
>> + res_regs->end - res_regs->start);
>> + if (!priv->pdc_base)
>> + return -EIO;
>
> Leaks priv.
>
>> + /* Get number of peripherals */
>> + ret = of_property_read_u32(node, "num-perips", &val);
>> + if (ret) {
>> + dev_err(&pdev->dev, "No num-perips node property found\n");
>> + return -EINVAL;
>
> Leaks priv and mapping
>
>> + }
>> + if (val > SYS0_HWIRQ) {
>> + dev_err(&pdev->dev, "num-perips (%u) out of range\n", val);
>> + return -EINVAL;
>
> Error handling is optional, right?
>
>> +static int pdc_intc_remove(struct platform_device *pdev)
>> +{
>> + struct pdc_intc_priv *priv = platform_get_drvdata(pdev);
>> +
>> + irq_domain_remove(priv->domain);
>
> And the rest of the resources is still there?
I was under the impression devm_kzalloc and devm_ioremap took care of
that in both the probe error case and the remove case:
> * devm_kzalloc - Resource-managed kzalloc
> * Managed kzalloc. Memory allocated with this function is
> * automatically freed on driver detach.
> * devm_ioremap - Managed ioremap()
> * Managed ioremap(). Map is automatically unmapped on driver detach.
I may have misunderstood the whole point of their existence though?
Thanks
James
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
