Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1446604 > unrolled thread
| Started by | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| First post | 2016-07-19 19:00 +0200 |
| Last post | 2016-07-20 16:50 +0200 |
| Articles | 4 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [RFC PATCH v2] irqchip: add support for SMP irq router Thomas Gleixner <tglx@linutronix.de> - 2016-07-19 19:00 +0200
Re: [RFC PATCH v2] irqchip: add support for SMP irq router Sebastian Frias <sf84@laposte.net> - 2016-07-20 13:10 +0200
Re: [RFC PATCH v2] irqchip: add support for SMP irq router Marc Zyngier <marc.zyngier@arm.com> - 2016-07-20 15:30 +0200
Re: [RFC PATCH v2] irqchip: add support for SMP irq router Thomas Gleixner <tglx@linutronix.de> - 2016-07-20 16:50 +0200
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2016-07-19 19:00 +0200 |
| Subject | Re: [RFC PATCH v2] irqchip: add support for SMP irq router |
| Message-ID | <rWGxc-6sA-25@gated-at.bofh.it> |
On Tue, 19 Jul 2016, Sebastian Frias wrote:
> +
> +#define DBGERR(__format, ...) panic("[%s:%d] %s(): " __format, \
> + __FILE__, __LINE__, \
> + __func__, ##__VA_ARGS__)
Please get rid of this macro mess. Use the functions (panic, ...)
directly. There is no value for this file, line, func crappola. Think about
proper texts which can be grepped for.
> +
> +#define DBGWARN(__format, ...) pr_err("[%s:%d] %s(): " __format, \
Very consistent. WARN == err ....
> + __FILE__, __LINE__, \
> + __func__, ##__VA_ARGS__)
> +
> +
> +#if 0
Don't even think about posting code with "#if 0" or such in it.
> +#define DBGLOG(__format, ...) pr_info("[%s:%d] %s(): " __format, \
> + __FILE__, __LINE__, \
> + __func__, ##__VA_ARGS__)
> +#else
> +#define DBGLOG(__format, ...) do {} while (0)
> +#endif
pr_debug() is there for a reason.
> +#if 0
> +#define SHORT_OR_FULL_NAME full_name
> +#else
> +#define SHORT_OR_FULL_NAME name
> +#endif
Use one and be done with it. Really.
> +
> +#define NODE_NAME(__node__) (__node__ ? __node__->SHORT_OR_FULL_NAME : \
> + "<no-node>")
Sigh. pr_xxx("%s", NULL) prints <null>. That's really sufficient for your
debug/error handling.
> +
> +#define BITMASK_VECTOR_SIZE(__count__) (__count__ / 32)
> +#define IRQ_TO_OFFSET(__hwirq__) (__hwirq__ * 4)
> +
> +struct tango_irqrouter;
> +
> +/*
> + * Maintains the mapping between a Linux virq and a hwirq
> + * on the parent controller.
> + * It is used by tango_irqdomain_map() or tango_irqdomain_hierarchy_alloc()
> + * to setup the route between input IRQ and output IRQ
> + */
> +struct tango_irqrouter_output {
> + struct tango_irqrouter *context;
> +
> + u32 domain_id;
> +
> + u32 hwirq;
> + u32 hwirq_level;
> + u32 virq;
> +
> + int shared_count;
> + int *shared_irqs;
> +};
> +
> +/*
> + * Context for the driver
> + */
> +struct tango_irqrouter {
> + raw_spinlock_t lock;
> + struct device_node *node;
> + void __iomem *base;
Plase align struct members proper
raw_spinlock_t lock;
struct device_node *node;
void __iomem *base;
> +
> + int input_count;
> + u32 irq_mask[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
> + u32 irq_invert_mask[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
You only ever use BITMASK_VECTOR_SIZE(ROUTER_INPUTS). Why can't you simply
do:
#define ROUTER_INPUTs_MASK_SIZE (ROUTER__INPUTS / 32) ????
> +static inline u32 tango_readl(struct tango_irqrouter *irqrouter,
> + int reg)
> +{
> + u32 val = readl_relaxed(irqrouter->base + reg);
> + /*DBGLOG("r[0x%08x + 0x%08x = 0x%08x] = 0x%08x\n",
> + irqrouter->base, reg, irqrouter->base + reg, val);*/
> + return val;
Please get rid of this debug nonsense.
> +}
> +
> +
> +static inline void tango_writel(struct tango_irqrouter *irqrouter,
> + int reg,
> + u32 val)
> +{
> + /*DBGLOG("w[0x%08x + 0x%08x = 0x%08x] = 0x%08x\n",
> + irqrouter->base, reg, irqrouter->base + reg, val);*/
> + writel_relaxed(val, irqrouter->base + reg);
> +}
> +
> +
> +static inline void tango_set_swirq_enable(struct tango_irqrouter *irqrouter,
> + int swirq,
> + bool enable)
> +{
> + u32 offset = SWIRQ_ENABLE;
If you name that varible 'reg' then it becomes obvious what it does.
> + u32 value = tango_readl(irqrouter, offset);
> + u32 swirq_bit_index = swirq % SWIRQ_COUNT;
> +
> +#if 1
> + DBGLOG("%smask swirq(in) %d : current regvalue 0x%x\n",
> + enable ? "un":"",
> + swirq, value);
> +#endif
> +
> + if (enable) {
> + /* unmask swirq */
The whole code lacks comments, but here you document the obvious ....
> + irqrouter->swirq_mask |= (1 << swirq_bit_index);
> + value |= (1 << swirq_bit_index);
> + } else {
> + /* mask swirq */
> + irqrouter->swirq_mask &= ~(1 << swirq_bit_index);
> + value &= ~(1 << swirq_bit_index);
> + }
> +
> + tango_writel(irqrouter, offset, value);
> +}
> +
> +
> +static inline void tango_set_hwirq_enable(struct tango_irqrouter *irqrouter,
> + int hwirq,
> + bool enable)
> +{
> + u32 offset = IRQ_TO_OFFSET(hwirq);
> + u32 value = tango_readl(irqrouter, offset);
> + u32 hwirq_reg_index = hwirq / 32;
> + u32 hwirq_bit_index = hwirq % 32;
> + u32 *enable_mask = &(irqrouter->irq_mask[hwirq_reg_index]);
> +
> +#if 1
> + DBGLOG("%smask hwirq(in) %d : current regvalue 0x%x\n",
> + enable ? "un":"",
> + hwirq, value);
> +#endif
> +
> + if (enable) {
> + /* unmask irq */
> + *enable_mask |= (1 << hwirq_bit_index);
> + value |= IRQ_ROUTER_ENABLE_MASK;
> + } else {
> + /* mask irq */
> + *enable_mask &= ~(1 << hwirq_bit_index);
> + value &= ~(IRQ_ROUTER_ENABLE_MASK);
> + }
> +
> + tango_writel(irqrouter, offset, value);
> +}
> +
> +
> +static inline void tango_set_swirq_inversion(struct tango_irqrouter *irqrouter,
> + int swirq,
> + bool invert)
> +{
> +
> + DBGLOG("swirq(in) %d %s inverted\n", swirq, invert ? "":"not");
> +
> + if (invert)
> + DBGERR("SW IRQs cannot be inverted!\n");
Groan.
> +}
> +
> +
> +static inline void tango_set_hwirq_inversion(struct tango_irqrouter *irqrouter,
> + int hwirq,
> + bool invert)
> +{
> + u32 offset = IRQ_TO_OFFSET(hwirq);
> + u32 value = tango_readl(irqrouter, offset);
> + u32 hwirq_reg_index = hwirq / 32;
> + u32 hwirq_bit_index = hwirq % 32;
> + u32 *invert_mask = &(irqrouter->irq_invert_mask[hwirq_reg_index]);
> +
> + if (invert) {
> + *invert_mask |= (1 << hwirq_bit_index);
> + value |= IRQ_ROUTER_INVERT_MASK;
> + } else {
> + *invert_mask &= ~(1 << hwirq_bit_index);
> + value &= ~(IRQ_ROUTER_INVERT_MASK);
> + }
> +
> + DBGLOG("hwirq(in) %d %s inverted\n", hwirq, invert ? "":"not");
> +
> + tango_writel(irqrouter, offset, value);
> +}
> +
> +
> +static inline void tango_set_swirq_route(struct tango_irqrouter *irqrouter,
> + int swirq_in,
> + int irq_out)
> +{
> + u32 swirq_reg_index = swirq_in / 4;
> + u32 swirq_bit_index = (swirq_in % 4) * 8;
> + u32 mask = ~(0x1f << swirq_bit_index);
> + u32 offset = SWIRQ_MAP_GROUP0 + (swirq_reg_index * 4);
> + u32 value = tango_readl(irqrouter, offset);
> +
> + DBGLOG("ri %d, bi %d, mask 0x%x, offset 0x%x, val 0x%x\n",
> + swirq_reg_index,
> + swirq_bit_index,
> + mask,
> + offset,
> + value);
> +
> + DBGLOG("route swirq %d => hwirq(out) %d\n", swirq_in, irq_out);
> +
> + value &= mask;
> +
> + if (irq_out < 0) {
This logic is utterly confusing, really. What's a negative interrupt number?
> + tango_set_irq_enable(irqrouter,
> + swirq_in + irqrouter->input_count,
> + 0);
Now you write back 'value' with the bit for this irq masked. Is that correct?
> + } else
} else {
please
> + value |= ((irq_out & 0x1f) << swirq_bit_index);
> +
> + tango_writel(irqrouter, offset, value);
> +}
> +
> +
> +static inline void tango_set_hwirq_route(struct tango_irqrouter *irqrouter,
> + int irq_in,
> + int irq_out)
> +{
> + u32 offset = IRQ_TO_OFFSET(irq_in);
> + u32 value;
> +
> + DBGLOG("route hwirq(in) %d => hwirq(out) %d\n", irq_in, irq_out);
> +
> + if (irq_out < 0) {
> + tango_set_irq_enable(irqrouter,
> + irq_in,
> + 0);
This fits in a single line.
> + value = 0;
Please document why you are setting the value to 0 here.
> + } else
> + value = (irq_out & 0x1f);
> +
> + tango_writel(irqrouter, offset, value);
> +}
> +
> +
> +static inline int tango_set_irq_enable(struct tango_irqrouter *irqrouter,
> + int irq,
> + bool enable)
> +{
> + if (irq >= (irqrouter->input_count + irqrouter->swirq_count))
> + return -EINVAL;
How can that happen? Paranoia programming?
> + else if (irq >= irqrouter->input_count)
That "else" is pointless. You return above.
> + tango_set_swirq_enable(irqrouter,
> + irq - irqrouter->input_count,
> + enable);
> + else
> + tango_set_hwirq_enable(irqrouter,
> + irq,
> + enable);
This can be written way simpler.
if (irq >= irqrouter->input_count)
irq -= irqrouter->input_count;
tango_set_hwirq_enable(irqrouter, irq, enable);
Hmm?
> +static int tango_set_irq_type(struct tango_irqrouter *irqrouter,
> + int hwirq_in,
> + u32 type,
> + u32 parent_type)
> +{
> + int err;
> +
> + if (parent_type & (type & IRQ_TYPE_SENSE_MASK))
> + /* same polarity */
> + err = tango_set_irq_inversion(irqrouter, hwirq_in, 0);
> + else
> + /* invert polarity */
> + err = tango_set_irq_inversion(irqrouter, hwirq_in, 1);
bool invert = parent_type & (type & IRQ_TYPE_SENSE_MASK);
err = tango_set_irq_inversion(irqrouter, hwirq_in, invert);
> +
> + if (err < 0) {
> + DBGWARN("Failed to setup IRQ %d polarity\n", hwirq_in);
> + return err;
> + }
> +
> + switch (type & IRQ_TYPE_SENSE_MASK) {
> + case IRQ_TYPE_EDGE_RISING:
> + case IRQ_TYPE_EDGE_FALLING:
> + DBGERR("Does not support edge triggers\n");
This really sucks. In fact you panic here while the code suggests that you
print a DEBUG error and continue .....
> + break;
> + case IRQ_TYPE_LEVEL_HIGH:
> + break;
You can spare that break.
> + case IRQ_TYPE_LEVEL_LOW:
> + break;
> + default:
> + DBGWARN("Invalid trigger mode 0x%x for hwirq(in) %d\n",
> + type, hwirq_in);
So here you continue with an error code, but above for EDGE you panic. I
really don't get that logic.
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +
> +static int tango_get_output_for_hwirq(struct tango_irqrouter *irqrouter,
> + int hwirq_in,
> + struct tango_irqrouter_output **out_val)
> +{
> + struct tango_irqrouter_output *irqrouter_output;
> + int i;
> +
> + if (!out_val)
> + return -EINVAL;
This is utter crap. Look at the call site:
> + struct tango_irqrouter_output *irqrouter_output = NULL;
> +
> + tango_get_output_for_hwirq(irqrouter, hwirq_in, &irqrouter_output);
> + if (!irqrouter_output)
> + goto handle_parent;
So out_val CANNOT be NULL. What's the purpose of your return values if you
ignore them?
> +
> + /* Get the irqrouter_output for the hwirq */
This is another really helpful comment.
> + for (i = 0; i < irqrouter->output_count; i++) {
> + int j;
> +
> + irqrouter_output = &(irqrouter->output[i]);
> +
> + for (j = 0; j < irqrouter_output->shared_count; j++) {
> + if (hwirq_in == irqrouter_output->shared_irqs[j])
> + goto found_router_output;
> + }
> + }
> + if (i == irqrouter->output_count) {
What other reason would be to get here than this?
> + DBGWARN("Couldn't find hwirq mapping\n");
> + return -ENODEV;
> + }
> +
> +found_router_output:
> +
> + *out_val = irqrouter_output;
> + return 0;
> +}
This whole thing can be condensed to:
static struct tango_irqrouter_output *
tango_get_output_for_hwirq(struct tango_irqrouter *irqr, int hwirq_in)
{
struct tango_irqrouter_output *rout;
int i, j;
/* Scan the outputs for a matching hwirq number */
for (i = 0, rout = irqr->output; i < irqr->output_count; i++, rout++) {
for (j = 0; j < rout->shared_count; j++) {
if (rout->shared_irqs[j] == hwirq_in)
return rout;
}
}
return NULL;
}
And the call site would become:
struct tango_irqrouter_output *rout;
rout = tango_get_output_for_hwirq(irqrouter, hwirq_in);
if (!rout)
goto handle_parent;
Hmm?
> +/* 'irqchip' handling callbacks
> + * Used for 'shared' IRQs, i.e.: IRQs that share a GIC input
> + * This driver performs the IRQ dispatch based on the flags
> + */
> +
Multiline comments have this format:
/*
* First line
* ....
* Last line
*/
> +static void tango_irqchip_mask_irq(struct irq_data *data)
> +{
> + struct irq_domain *domain = irq_data_get_irq_chip_data(data);
> + struct tango_irqrouter_output *irqrouter_output = domain->host_data;
> + struct tango_irqrouter *irqrouter = irqrouter_output->context;
> + int hwirq_in = (int)data->hwirq;
Huch? What's that silly typecast for? Why can't you use u32 for hwirq_in?
> + tango_set_irq_enable(irqrouter, hwirq_in, 0);
> +}
> +#ifdef CONFIG_SMP
> +static int tango_irqchip_set_irq_affinity(struct irq_data *data,
> + const struct cpumask *mask_val,
> + bool force)
> +{
> + struct irq_domain *domain = irq_data_get_irq_chip_data(data);
> + struct tango_irqrouter_output *irqrouter_output = domain->host_data;
> + struct irq_chip *parent_chip = irq_get_chip(irqrouter_output->virq);
> + struct irq_data *parent_data = irq_get_irq_data(irqrouter_output->virq);
> +
> + DBGLOG("%s:%s(0x%p)\n",
> + NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain);
> +
> + if (parent_chip && parent_chip->irq_set_affinity)
> + return parent_chip->irq_set_affinity(parent_data,
> + mask_val,
> + force);
> + else
> + return -EINVAL;
You really can spare identation levels by writing it a bit more clever:
if (!parent || !parent->irq_set_affinity)
return -EINVAL;
return parent->irq_set_affinity(parent, mask, force);
You might notice that I shortened the variable names and it still is entirely
clear what the code does.
Aside of that mask_val is a clear misnomer.
> +}
> +#endif
> +
> +
> +static inline u32 tango_dispatch_irqs(struct irq_domain *domain,
> + struct irq_desc *desc,
> + u32 status,
> + int base)
> +{
> + u32 hwirq;
> + u32 virq;
Please put same types onto a single line
> +
> + while (status) {
> + hwirq = __ffs(status);
> + virq = irq_find_mapping(domain, base + hwirq);
> + if (unlikely(!virq))
> + handle_bad_irq(desc);
> + else
> + generic_handle_irq(virq);
> +
> + status &= ~BIT(hwirq);
> + }
> +
> + return status;
Huch? status is guaranteed to be 0 here. So what's the point of this return
value.
> +}
> +
> +static void tango_irqdomain_handle_cascade_irq(struct irq_desc *desc)
> +{
> + struct irq_domain *domain = irq_desc_get_handler_data(desc);
> + struct tango_irqrouter_output *irqrouter_output = domain->host_data;
> + struct tango_irqrouter *irqrouter = irqrouter_output->context;
> + struct irq_chip *host_chip = irq_desc_get_chip(desc);
> + u32 i, status;
> + u32 swirq_status, irq_status[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
> +
> +#if 0
> + DBGLOG("%s:%s(0x%p): irqrouter_output 0x%p, hwirq(out) %d\n",
> + NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain,
> + irqrouter_output, irqrouter_output->hwirq);
> +#endif
> +
> + chained_irq_enter(host_chip, desc);
> +
> + raw_spin_lock(&(irqrouter->lock));
> + swirq_status = tango_readl(irqrouter, READ_SWIRQ_STATUS);
> + for (i = 0; i < BITMASK_VECTOR_SIZE(ROUTER_INPUTS); i++)
> + irq_status[i] = tango_readl(irqrouter,
> + READ_SYS_IRQ_GROUP0 + i*4);
i * 4 please
> + raw_spin_unlock(&(irqrouter->lock));
> +
> + /* HW irqs */
And that comment tells us what?
> + for (i = 0; i < BITMASK_VECTOR_SIZE(ROUTER_INPUTS); i++) {
> +#if 0
> + DBGLOG("%d: 0x%08x (en 0x%08x, inv 0x%08x)\n",
> + i,
> + irq_status[i],
> + irqrouter->irq_mask[0],
> + irqrouter->irq_invert_mask[0]);
> +#endif
> +
> +#define HANDLE_INVERTED_LINES(__irqstatus__, __x__) ((((~__irqstatus__) & irqrouter->irq_invert_mask[__x__]) & irqrouter->irq_mask[__x__]) | __irqstatus__)
> +#define HANDLE_EN_AND_INV_MASKS(__irqstatus__, __y__) (HANDLE_INVERTED_LINES(__irqstatus__, __y__) & irqrouter->irq_mask[__y__])
What the hell does this unparseable macro mess? What's so wrong with a proper
inline function?
> +
> + irq_status[i] = HANDLE_EN_AND_INV_MASKS(irq_status[i], i);
> + status = tango_dispatch_irqs(domain, desc, irq_status[i], i*32);
> + if (status & irq_status[i])
Can you pretty please explain how this can happen? It can't. Because you clear
every single bit of status in tango_dispatch_irqs(). See above.
Please clean up that unholy mess of nonsensical checks and debug code.
> + DBGERR("%s: %d unhandled IRQs (as a mask) 0x%x\n",
> + NODE_NAME(irq_domain_get_of_node(domain)),
> + i,
> + status & irq_status[i]);
> + }
> +
> + /* SW irqs */
> + swirq_status &= irqrouter->swirq_mask;
> + status = tango_dispatch_irqs(domain, desc, swirq_status, 128);
128 is the magic number pulled out of thin air or what?
> + if (status & swirq_status)
> + DBGERR("%s: Unhandled IRQs (as a mask) 0x%x\n",
> + NODE_NAME(irq_domain_get_of_node(domain)),
> + status & swirq_status);
See above.
> +
> + chained_irq_exit(host_chip, desc);
> +}
> +
> +
> +/**
> + * tango_irqdomain_map - route a hwirq(in) to a hwirq(out).
> + * NOTE: The hwirq(out) must have been already allocated and enabled on
> + * the parent controller.
Please put notes AFTER the argument descriptions
> + * @hwirq: HW IRQ of the device requesting an IRQ
> + * if hwirq > inputs it is a SW IRQ
> + * @virq: Linux IRQ (associated to the domain) to be given to the device
> + * @domain: IRQ domain (from the domain, we get the irqrouter_output
> + * in order to know to which output we need to route hwirq to)
And if you spend a few seconds to align this proper, then it becomes suddenly
readable.
* @hwirq: HW IRQ of the device requesting an IRQ
* if hwirq > inputs it is a SW IRQ
* @virq: Linux IRQ (associated to the domain) to be given to the device
* @domain: IRQ domain (from the domain, we get the irqrouter_output
* in order to know to which output we need to route hwirq to)
Hmm?
> + */
> +static int tango_irqdomain_map(struct irq_domain *domain,
> + unsigned int virq,
> + irq_hw_number_t hwirq)
> +{
> + struct tango_irqrouter_output *irqrouter_output = domain->host_data;
> + struct tango_irqrouter *irqrouter = irqrouter_output->context;
> +
> + DBGLOG("%s:%s(0x%p): hwirq(in) %d := virq %d, and route "
> + "hwirq(in) %d => hwirq(out) %d (virq %d)\n",
> + NODE_NAME(irq_domain_get_of_node(domain)),
> + domain->name,
> + domain,
> + (u32)hwirq,
> + virq,
> + (u32)hwirq,
> + irqrouter_output->hwirq,
> + irqrouter_output->virq);
> +
> + if (hwirq >= (irqrouter->input_count + irqrouter->swirq_count))
> + DBGERR("%s: Invalid hwirq(in) %d >= %d + %d\n",
> + NODE_NAME(irq_domain_get_of_node(domain)),
> + (u32)hwirq,
> + irqrouter->input_count,
> + irqrouter->swirq_count);
> + else if (hwirq >= irqrouter->input_count)
> + DBGLOG("Map swirq %ld\n", hwirq - irqrouter->input_count);
> +
> + irq_set_chip_and_handler(virq,
> + &tango_irq_chip_shared_ops,
> + handle_level_irq);
> + irq_set_chip_data(virq, domain);
> + irq_set_probe(virq);
> +
> + tango_set_irq_route(irqrouter, hwirq, irqrouter_output->hwirq);
> +
> + return 0;
> +}
> +
> +
> +/**
> + * tango_irqdomain_translate - used to select the domain for a given
> + * irq_fwspec
It hardly selects the domain. @domain is handed in as a argument. And please
get rid of that silly 'used to'. That just adds characters and no information.
- Select the router section for a firmware spec
Hmm?
> + * @domain: a registered domain
> + * @fwspec: an IRQ specification. This callback is used to translate the
> + * parameters given as irq_fwspec into a HW IRQ and Type values.
fwspec is a callback?
> + * @out_hwirq:
> + * @out_type:
Missing docs.....
> + */
> +static int tango_irqdomain_translate(struct irq_domain *domain,
> + struct irq_fwspec *fwspec,
> + unsigned long *out_hwirq,
> + unsigned int *out_type)
> +{
> + struct tango_irqrouter_output *irqrouter_output = domain->host_data;
> + struct tango_irqrouter *irqrouter = irqrouter_output->context;
> + irq_hw_number_t irq;
> + u32 domain_id, type;
> + int err;
> +
> + DBGLOG("%s:%s(0x%p): argc %d for hwirq(out) %d\n",
> + NODE_NAME(irq_domain_get_of_node(domain)),
> + domain->name,
> + domain,
> + fwspec->param_count,
> + irqrouter_output->hwirq);
> +
> + err = tango_parse_fwspec(domain,
> + fwspec,
> + &domain_id,
> + &irq,
> + &type);
Please use the full 80 characters. Your patch does not become more valuable
when you inflate the line count. It's more valuable when it is easy to read
and understand.
> + if (err < 0) {
> + DBGWARN("Failed to parse fwspec\n");
> + return err;
> + }
> +
> + switch (domain_id) {
> + case SIGMA_HWIRQ:
> + DBGLOG("Request is for SIGMA_HWIRQ\n");
> + break;
> + case SIGMA_SWIRQ:
> + DBGLOG("Request is for SIGMA_SWIRQ\n");
> + irq += irqrouter->input_count;
> + break;
> + default:
> + DBGLOG("Request is for domain ID 0x%x (we are 0x%x)\n",
> + domain_id,
> + irqrouter_output->domain_id);
Huch what? You only ever handle HW and SW irqs and now you happily proceed if
the id is something else. Consistent error handling is optional and can be
replaced by random debug prints, right?
> + break;
> + };
> +
> + *out_hwirq = irq;
> + *out_type = type & IRQ_TYPE_SENSE_MASK;
> +
> + DBGLOG("hwirq %d type 0x%x\n", (u32)*out_hwirq, (u32)*out_type);
> +
> + return 0;
> +}
> +
> +
> +/**
> + * tango_irqdomain_select - used to select the domain for a given irq_fwspec
> + * @domain: a registered domain
> + * @fwspec: an IRQ specification. This callback should return zero if the
> + * irq_fwspec does not belong to the given domain. If it does, it should
> + * return non-zero.
Callback????
> + * In practice it will return non-zero if the irq_fwspec matches one of the
> + * IRQs shared within the given domain.
> + * @bus_token: a bus token
> + */
> +static int tango_irqdomain_select(struct irq_domain *domain,
> + struct irq_fwspec *fwspec,
> + enum irq_domain_bus_token bus_token)
> +{
> + struct tango_irqrouter_output *irqrouter_output = domain->host_data;
> + struct tango_irqrouter *irqrouter = irqrouter_output->context;
> + irq_hw_number_t irq;
> + u32 domain_id, type;
> + int err;
> +
> + DBGLOG("%s:%s(0x%p): argc %d, 0x%p, bus 0x%x\n",
> + NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain,
> + fwspec->param_count, fwspec->fwnode, bus_token);
> +
> + DBGLOG("router 0x%p, output 0x%p\n", irqrouter, irqrouter_output);
> +
> + err = tango_parse_fwspec(domain, fwspec, &domain_id, &irq, &type);
> + if (err < 0)
> + return 0;
> +
> + switch (domain_id) {
> + case SIGMA_HWIRQ:
> + DBGLOG("Request is for SIGMA_HWIRQ\n");
> + break;
> + case SIGMA_SWIRQ:
> + DBGLOG("Request is for SIGMA_SWIRQ\n");
> + break;
> + default:
> + DBGLOG("Request is for domain ID 0x%x (we are 0x%x)\n",
> + domain_id,
> + irqrouter_output->domain_id);
> + break;
> + };
> +
> + if (!irqrouter->implicit_groups) {
> + int i;
> +
> + /* Check if the requested IRQ belongs to those listed
> + * to be sharing the output assigned to this domain
Your using of 'domain' is utterly confusing. Please use something like
hwdomain or whatever which makes it clear that this is something else than the
irq domain. I tripped over this several times now.
> + */
> + if (irqrouter_output->shared_count <= 0) {
> + DBGLOG("Not shared IRQ line?\n");
> + return 0;
> + }
> +
> + for (i = 0; i < irqrouter_output->shared_count; i++) {
> + if (irq == irqrouter_output->shared_irqs[i]) {
> + DBGLOG("Match: IRQ %lu\n", irq);
> + return 1;
> + }
> + }
> + } else {
> + /* Otherwise, check if the domain_id given matches
> + * the one assigned to this output
> + */
> + if (domain_id == irqrouter_output->domain_id) {
> + DBGLOG("Match: Domain ID %d\n", domain_id);
> + return 1;
> + }
> + }
> +
> + return 0;
> +}
> +
> +
> +/* 'irqrouter' handling callbacks
> + * Used for 'direct' IRQs, i.e.: IRQs that are directly routed to the GIC
> + * This driver does not dispatch the IRQs, the GIC does.
> + */
> +
> +
> +static void tango_irqrouter_mask_irq(struct irq_data *data)
> +{
> + struct irq_domain *domain = irq_data_get_irq_chip_data(data);
> + struct tango_irqrouter *irqrouter = domain->host_data;
> + int hwirq_in = (int)data->hwirq;
> +
> + DBGLOG("%s:%s(0x%p) hwirq(in) %d\n",
> + NODE_NAME(irq_domain_get_of_node(domain)),
> + domain->name,
> + domain,
> + hwirq_in);
> +
> + tango_set_irq_enable(irqrouter, hwirq_in, 0);
> +
> + irq_chip_mask_parent(data);
> +}
> +
> +
> +static void tango_irqrouter_unmask_irq(struct irq_data *data)
> +{
> + struct irq_domain *domain = irq_data_get_irq_chip_data(data);
> + struct tango_irqrouter *irqrouter = domain->host_data;
> + int hwirq_in = (int)data->hwirq;
> +
> + DBGLOG("%s:%s(0x%p) hwirq(in) %d\n",
> + NODE_NAME(irq_domain_get_of_node(domain)),
> + domain->name,
> + domain,
> + hwirq_in);
> +
> + tango_set_irq_enable(irqrouter, hwirq_in, 1);
> +
> + irq_chip_unmask_parent(data);
> +}
> +
> +
> +static int tango_irqrouter_set_irq_type(struct irq_data *data,
> + unsigned int type)
> +{
> + struct irq_domain *domain = irq_data_get_irq_chip_data(data);
> + struct tango_irqrouter_output *irqrouter_output = NULL;
> + struct tango_irqrouter *irqrouter = domain->host_data;
> + int hwirq_in = (int)data->hwirq;
> + u32 parent_type;
> +
> + DBGLOG("%s:%s(0x%p) type 0x%x for hwirq(in) %d\n",
> + NODE_NAME(irq_domain_get_of_node(domain)),
> + domain->name,
> + domain,
> + type,
> + hwirq_in);
> +
> + tango_get_output_for_hwirq(irqrouter, hwirq_in, &irqrouter_output);
> + if (!irqrouter_output)
> + goto handle_parent;
So if there is no output, then you unconditionally set the type on the
parent. If that's correct, this needs a big fat comment at least.
> + parent_type = (irqrouter_output->hwirq_level & IRQ_TYPE_SENSE_MASK);
> + tango_set_irq_type(irqrouter, hwirq_in, type, parent_type);
> +
> +handle_parent:
> + return irq_chip_set_type_parent(data, type);
> +}
I'm stopping here. You can apply the above to the rest of the code as well.
Thanks,
tglx
[toc] | [next] | [standalone]
| From | Sebastian Frias <sf84@laposte.net> |
|---|---|
| Date | 2016-07-20 13:10 +0200 |
| Message-ID | <rWXy1-Dc-7@gated-at.bofh.it> |
| In reply to | #1446604 |
Hi Thomas,
Thanks for your comments.
I appreciate the time you dedicated to the review.
I will take your comments into account, in the meanwhile, and since this was
a RFC, would it be possible to also get some feedback/comments from a
conceptual point of view?
I'm copy/pasting some of the questions I attached to the RFC email here:
----
2) In order to get a virq mapping for the domains associated with the outputs
(the outputs connected to the GIC) I request a sort of "Fake HW IRQ"
See irq-tango_v4.c:1400
/* To request a virq we need a HW IRQ, use a "Fake HW IRQ" */
hwirq = index + irqrouter->input_count + irqrouter->swirq_count;
This feels a bit like a hack, so suggestions are welcome.
3) The file is called 'irq-tango_v4.c' but I think it should match the
compatible string, so I was thinking of renaming:
irq-tango_v4.c => irq-sigma_smp_irqrouter.c
irq-tango_v4.h => irq-sigma_smp_irqrouter.h
What do you think?
4) Do I have to do something more to handle the affinity stuff?
6) More of a theoretical question:
I have to #include <dt-bindings/interrupt-controller/irq-tango_v4.h> from
code that is not in the Linux tree. That's fine for now, but if in the future
there are other irq controllers and another header is required, how can the
code know which header to #include ?
Are we supposed to #include all of them, and then somehow detect which module
is actually active and act accordingly? If so, can we detect which module
matched and probed correctly to know which controller version we need to
talk to?
----
What the driver does is:
- creates a domain hierarchy attached to its parent domain (the GIC)
- the callbacks for this domain hierarchy will be used to deal with IRQs
"directly" routed to the GIC (i.e.: exclusive IRQ lines routed to the GIC
and not shared).
In this case the GIC dispatches the interrupts.
- create linear domains attached to this driver's node.
- the callbacks for these linear domains will be used to deal with shared
IRQs (they share the routing to one of the GIC inputs).
In this case this driver dispatches the interrupts after reading the flags.
It is such concept, and then its implementation, that I would need some more
advise with, because, as stated in the questions above, it still does not
feels right, especially question 2.
I will reply to your comments below and will post a v3 later, hopefully after
dealing also with the answers to the doubts outlined above.
On 07/19/2016 06:49 PM, Thomas Gleixner wrote:
> On Tue, 19 Jul 2016, Sebastian Frias wrote:
>> +
>> +#define DBGERR(__format, ...) panic("[%s:%d] %s(): " __format, \
>> + __FILE__, __LINE__, \
>> + __func__, ##__VA_ARGS__)
>
> Please get rid of this macro mess. Use the functions (panic, ...)
> directly. There is no value for this file, line, func crappola. Think about
> proper texts which can be grepped for.
Ok.
>
>> +
>> +#define DBGWARN(__format, ...) pr_err("[%s:%d] %s(): " __format, \
>
> Very consistent. WARN == err ....
>
>> + __FILE__, __LINE__, \
>> + __func__, ##__VA_ARGS__)
>> +
>> +
>> +#if 0
>
> Don't even think about posting code with "#if 0" or such in it.
>
The idea behind these macros was:
- to easily enable/disable them.
- to be more consistent with other code written at Sigma: this is a
Sigma-only driver.
>> +#define DBGLOG(__format, ...) pr_info("[%s:%d] %s(): " __format, \
>> + __FILE__, __LINE__, \
>> + __func__, ##__VA_ARGS__)
>> +#else
>> +#define DBGLOG(__format, ...) do {} while (0)
>> +#endif
>
> pr_debug() is there for a reason.
Yes, but doesn't pr_debug() requires changes to the Makefile?
>
>> +#if 0
>> +#define SHORT_OR_FULL_NAME full_name
>> +#else
>> +#define SHORT_OR_FULL_NAME name
>> +#endif
>
> Use one and be done with it. Really.
By making the choice explicit, the person reading/debugging is aware of the possibilities.
Picking "full_name" or "name" would likely prevent the person from knowing it has other debug options.
But I can change this if upstreaming depends on it.
>
>> +
>> +#define NODE_NAME(__node__) (__node__ ? __node__->SHORT_OR_FULL_NAME : \
>> + "<no-node>")
>
> Sigh. pr_xxx("%s", NULL) prints <null>. That's really sufficient for your
> debug/error handling.
The code is checking for __node__ to be non-NULL before dereferencing it.
>
>> +
>> +#define BITMASK_VECTOR_SIZE(__count__) (__count__ / 32)
>> +#define IRQ_TO_OFFSET(__hwirq__) (__hwirq__ * 4)
>> +
>> +struct tango_irqrouter;
>> +
>> +/*
>> + * Maintains the mapping between a Linux virq and a hwirq
>> + * on the parent controller.
>> + * It is used by tango_irqdomain_map() or tango_irqdomain_hierarchy_alloc()
>> + * to setup the route between input IRQ and output IRQ
>> + */
>> +struct tango_irqrouter_output {
>> + struct tango_irqrouter *context;
>> +
>> + u32 domain_id;
>> +
>> + u32 hwirq;
>> + u32 hwirq_level;
>> + u32 virq;
>> +
>> + int shared_count;
>> + int *shared_irqs;
>> +};
>> +
>> +/*
>> + * Context for the driver
>> + */
>> +struct tango_irqrouter {
>> + raw_spinlock_t lock;
>> + struct device_node *node;
>> + void __iomem *base;
>
> Plase align struct members proper
>
> raw_spinlock_t lock;
> struct device_node *node;
> void __iomem *base;
Ok.
>
>> +
>> + int input_count;
>> + u32 irq_mask[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
>> + u32 irq_invert_mask[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
>
> You only ever use BITMASK_VECTOR_SIZE(ROUTER_INPUTS). Why can't you simply
> do:
>
> #define ROUTER_INPUTs_MASK_SIZE (ROUTER__INPUTS / 32) ????
I guess I can do that.
>
>> +static inline u32 tango_readl(struct tango_irqrouter *irqrouter,
>> + int reg)
>> +{
>> + u32 val = readl_relaxed(irqrouter->base + reg);
>> + /*DBGLOG("r[0x%08x + 0x%08x = 0x%08x] = 0x%08x\n",
>> + irqrouter->base, reg, irqrouter->base + reg, val);*/
>> + return val;
>
> Please get rid of this debug nonsense.
Ok, although I can guarantee it was really useful to determine and prove
that the HW documentation of the module was wrong...
>
>> +}
>> +
>> +
>> +static inline void tango_writel(struct tango_irqrouter *irqrouter,
>> + int reg,
>> + u32 val)
>> +{
>> + /*DBGLOG("w[0x%08x + 0x%08x = 0x%08x] = 0x%08x\n",
>> + irqrouter->base, reg, irqrouter->base + reg, val);*/
>> + writel_relaxed(val, irqrouter->base + reg);
>> +}
>> +
>> +
>> +static inline void tango_set_swirq_enable(struct tango_irqrouter *irqrouter,
>> + int swirq,
>> + bool enable)
>> +{
>> + u32 offset = SWIRQ_ENABLE;
>
> If you name that varible 'reg' then it becomes obvious what it does.
Well, it was to add a sort of "consistency" between the code for SW IRQs and HW IRQs.
>
>> + u32 value = tango_readl(irqrouter, offset);
>> + u32 swirq_bit_index = swirq % SWIRQ_COUNT;
>> +
>> +#if 1
>> + DBGLOG("%smask swirq(in) %d : current regvalue 0x%x\n",
>> + enable ? "un":"",
>> + swirq, value);
>> +#endif
>> +
>> + if (enable) {
>> + /* unmask swirq */
>
> The whole code lacks comments, but here you document the obvious ....
I will remove the comments.
>
>> + irqrouter->swirq_mask |= (1 << swirq_bit_index);
>> + value |= (1 << swirq_bit_index);
>> + } else {
>> + /* mask swirq */
>> + irqrouter->swirq_mask &= ~(1 << swirq_bit_index);
>> + value &= ~(1 << swirq_bit_index);
>> + }
>> +
>> + tango_writel(irqrouter, offset, value);
>> +}
>> +
>> +
>> +static inline void tango_set_hwirq_enable(struct tango_irqrouter *irqrouter,
>> + int hwirq,
>> + bool enable)
>> +{
>> + u32 offset = IRQ_TO_OFFSET(hwirq);
>> + u32 value = tango_readl(irqrouter, offset);
>> + u32 hwirq_reg_index = hwirq / 32;
>> + u32 hwirq_bit_index = hwirq % 32;
>> + u32 *enable_mask = &(irqrouter->irq_mask[hwirq_reg_index]);
>> +
>> +#if 1
>> + DBGLOG("%smask hwirq(in) %d : current regvalue 0x%x\n",
>> + enable ? "un":"",
>> + hwirq, value);
>> +#endif
>> +
>> + if (enable) {
>> + /* unmask irq */
>> + *enable_mask |= (1 << hwirq_bit_index);
>> + value |= IRQ_ROUTER_ENABLE_MASK;
>> + } else {
>> + /* mask irq */
>> + *enable_mask &= ~(1 << hwirq_bit_index);
>> + value &= ~(IRQ_ROUTER_ENABLE_MASK);
>> + }
>> +
>> + tango_writel(irqrouter, offset, value);
>> +}
>> +
>> +
>> +static inline void tango_set_swirq_inversion(struct tango_irqrouter *irqrouter,
>> + int swirq,
>> + bool invert)
>> +{
>> +
>> + DBGLOG("swirq(in) %d %s inverted\n", swirq, invert ? "":"not");
>> +
>> + if (invert)
>> + DBGERR("SW IRQs cannot be inverted!\n");
>
> Groan.
I rather keep this to catch typos in code requesting SW IRQs.
>
>> +}
>> +
>> +
>> +static inline void tango_set_hwirq_inversion(struct tango_irqrouter *irqrouter,
>> + int hwirq,
>> + bool invert)
>> +{
>> + u32 offset = IRQ_TO_OFFSET(hwirq);
>> + u32 value = tango_readl(irqrouter, offset);
>> + u32 hwirq_reg_index = hwirq / 32;
>> + u32 hwirq_bit_index = hwirq % 32;
>> + u32 *invert_mask = &(irqrouter->irq_invert_mask[hwirq_reg_index]);
>> +
>> + if (invert) {
>> + *invert_mask |= (1 << hwirq_bit_index);
>> + value |= IRQ_ROUTER_INVERT_MASK;
>> + } else {
>> + *invert_mask &= ~(1 << hwirq_bit_index);
>> + value &= ~(IRQ_ROUTER_INVERT_MASK);
>> + }
>> +
>> + DBGLOG("hwirq(in) %d %s inverted\n", hwirq, invert ? "":"not");
>> +
>> + tango_writel(irqrouter, offset, value);
>> +}
>> +
>> +
>> +static inline void tango_set_swirq_route(struct tango_irqrouter *irqrouter,
>> + int swirq_in,
>> + int irq_out)
>> +{
>> + u32 swirq_reg_index = swirq_in / 4;
>> + u32 swirq_bit_index = (swirq_in % 4) * 8;
>> + u32 mask = ~(0x1f << swirq_bit_index);
>> + u32 offset = SWIRQ_MAP_GROUP0 + (swirq_reg_index * 4);
>> + u32 value = tango_readl(irqrouter, offset);
>> +
>> + DBGLOG("ri %d, bi %d, mask 0x%x, offset 0x%x, val 0x%x\n",
>> + swirq_reg_index,
>> + swirq_bit_index,
>> + mask,
>> + offset,
>> + value);
>> +
>> + DBGLOG("route swirq %d => hwirq(out) %d\n", swirq_in, irq_out);
>> +
>> + value &= mask;
>> +
>> + if (irq_out < 0) {
>
> This logic is utterly confusing, really. What's a negative interrupt number?
It is a sort of "magic value" to disable the IRQ and the routing.
>
>> + tango_set_irq_enable(irqrouter,
>> + swirq_in + irqrouter->input_count,
>> + 0);
>
> Now you write back 'value' with the bit for this irq masked. Is that correct?
Yes, tango_set_irq_enable() is used to setup the mask (in this case disable the IRQ
by masking it) to one register, and later we write 'value' (which deals with the
routing) to a different register.
Since up to 4 SW IRQ routes can be programmed in one of multiple routing registers,
to "disable" the routing for one of them, we need to mask the other 3 and leave them
untouched.
'value' is ANDed with 'mask' which is keeps the 3 other routing intact and resetting
the routing for the given IRQ.
>
>> + } else
>
> } else {
>
> please
I think I'm confused, we need to use {} for single statements on "else" blocks,
but not on "for" blocks?
>
>> + value |= ((irq_out & 0x1f) << swirq_bit_index);
>> +
>> + tango_writel(irqrouter, offset, value);
>> +}
>> +
>> +
>> +static inline void tango_set_hwirq_route(struct tango_irqrouter *irqrouter,
>> + int irq_in,
>> + int irq_out)
>> +{
>> + u32 offset = IRQ_TO_OFFSET(irq_in);
>> + u32 value;
>> +
>> + DBGLOG("route hwirq(in) %d => hwirq(out) %d\n", irq_in, irq_out);
>> +
>> + if (irq_out < 0) {
>> + tango_set_irq_enable(irqrouter,
>> + irq_in,
>> + 0);
>
> This fits in a single line.
Ok.
Actually, I was wondering if for consistency it wouldn't be better if all
function calls had one argument per line?
>
>> + value = 0;
>
> Please document why you are setting the value to 0 here.
Ok.
(it is for the same reason than for SW IRQs, to "disable" the routing, or more
precisely, to set disabled IRQs to a known value, in this case 0)
>
>> + } else
>> + value = (irq_out & 0x1f);
>> +
>> + tango_writel(irqrouter, offset, value);
>> +}
>> +
>> +
>> +static inline int tango_set_irq_enable(struct tango_irqrouter *irqrouter,
>> + int irq,
>> + bool enable)
>> +{
>> + if (irq >= (irqrouter->input_count + irqrouter->swirq_count))
>> + return -EINVAL;
>
> How can that happen? Paranoia programming?
No, actually it can happen, that's the reason for question 2 of my RFC:
----
2) In order to get a virq mapping for the domains associated with the outputs
(the outputs connected to the GIC) I request a sort of "Fake HW IRQ"
See irq-tango_v4.c:1400
/* To request a virq we need a HW IRQ, use a "Fake HW IRQ" */
hwirq = index + irqrouter->input_count + irqrouter->swirq_count;
This feels a bit like a hack, so suggestions are welcome.
----
>
>> + else if (irq >= irqrouter->input_count)
>
> That "else" is pointless. You return above.
Ok.
>
>> + tango_set_swirq_enable(irqrouter,
>> + irq - irqrouter->input_count,
>> + enable);
>> + else
>> + tango_set_hwirq_enable(irqrouter,
>> + irq,
>> + enable);
>
> This can be written way simpler.
>
> if (irq >= irqrouter->input_count)
> irq -= irqrouter->input_count;
> tango_set_hwirq_enable(irqrouter, irq, enable);
>
> Hmm?
Ok, but is it ok to modify function parameters?
I thought it was not recommended.
Indeed, some people may assume they are constants.
>
>> +static int tango_set_irq_type(struct tango_irqrouter *irqrouter,
>> + int hwirq_in,
>> + u32 type,
>> + u32 parent_type)
>> +{
>> + int err;
>> +
>> + if (parent_type & (type & IRQ_TYPE_SENSE_MASK))
>> + /* same polarity */
>> + err = tango_set_irq_inversion(irqrouter, hwirq_in, 0);
>> + else
>> + /* invert polarity */
>> + err = tango_set_irq_inversion(irqrouter, hwirq_in, 1);
>
> bool invert = parent_type & (type & IRQ_TYPE_SENSE_MASK);
>
> err = tango_set_irq_inversion(irqrouter, hwirq_in, invert);
>
Ok.
>> +
>> + if (err < 0) {
>> + DBGWARN("Failed to setup IRQ %d polarity\n", hwirq_in);
>> + return err;
>> + }
>> +
>> + switch (type & IRQ_TYPE_SENSE_MASK) {
>> + case IRQ_TYPE_EDGE_RISING:
>> + case IRQ_TYPE_EDGE_FALLING:
>> + DBGERR("Does not support edge triggers\n");
>
> This really sucks. In fact you panic here while the code suggests that you
> print a DEBUG error and continue .....
>
If I use panic() here, do I still have to use 'break' as well?
>> + break;
>> + case IRQ_TYPE_LEVEL_HIGH:
>> + break;
>
> You can spare that break.
Ok.
(before there were logs so the 'break' was necessary)
>
>> + case IRQ_TYPE_LEVEL_LOW:
>> + break;
>> + default:
>> + DBGWARN("Invalid trigger mode 0x%x for hwirq(in) %d\n",
>> + type, hwirq_in);
>
> So here you continue with an error code, but above for EDGE you panic. I
> really don't get that logic.
Ok.
Do you suggest I just make the code panic for any error? I'm fine with that.
>
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +
>> +static int tango_get_output_for_hwirq(struct tango_irqrouter *irqrouter,
>> + int hwirq_in,
>> + struct tango_irqrouter_output **out_val)
>> +{
>> + struct tango_irqrouter_output *irqrouter_output;
>> + int i;
>> +
>> + if (!out_val)
>> + return -EINVAL;
>
> This is utter crap. Look at the call site:
I think I'm confused, so it is not recommended to check for NULL pointers before
dereferencing them? Regardless of how the current code calls the function?
>
>> + struct tango_irqrouter_output *irqrouter_output = NULL;
>> +
>> + tango_get_output_for_hwirq(irqrouter, hwirq_in, &irqrouter_output);
>> + if (!irqrouter_output)
>> + goto handle_parent;
>
> So out_val CANNOT be NULL.
You are right, with current code it can't.
But I thought that it was a good practice to:
"be lenient with your input and strict with your output"
>What's the purpose of your return values if you
> ignore them?
I can make the function 'void'.
>
>> +
>> + /* Get the irqrouter_output for the hwirq */
>
> This is another really helpful comment.
It is true that now that this is a function, the name of the function
speaks for itself, so I can remove it.
>
>> + for (i = 0; i < irqrouter->output_count; i++) {
>> + int j;
>> +
>> + irqrouter_output = &(irqrouter->output[i]);
>> +
>> + for (j = 0; j < irqrouter_output->shared_count; j++) {
>> + if (hwirq_in == irqrouter_output->shared_irqs[j])
>> + goto found_router_output;
>> + }
>> + }
>> + if (i == irqrouter->output_count) {
>
> What other reason would be to get here than this?
You are right, relic of a refactoring.
>
>> + DBGWARN("Couldn't find hwirq mapping\n");
>> + return -ENODEV;
>> + }
>> +
>> +found_router_output:
>> +
>> + *out_val = irqrouter_output;
>> + return 0;
>> +}
>
> This whole thing can be condensed to:
>
> static struct tango_irqrouter_output *
> tango_get_output_for_hwirq(struct tango_irqrouter *irqr, int hwirq_in)
> {
> struct tango_irqrouter_output *rout;
> int i, j;
>
> /* Scan the outputs for a matching hwirq number */
> for (i = 0, rout = irqr->output; i < irqr->output_count; i++, rout++) {
> for (j = 0; j < rout->shared_count; j++) {
> if (rout->shared_irqs[j] == hwirq_in)
> return rout;
> }
> }
> return NULL;
> }
>
> And the call site would become:
>
> struct tango_irqrouter_output *rout;
>
> rout = tango_get_output_for_hwirq(irqrouter, hwirq_in);
> if (!rout)
> goto handle_parent;
>
> Hmm?
Ok.
>
>> +/* 'irqchip' handling callbacks
>> + * Used for 'shared' IRQs, i.e.: IRQs that share a GIC input
>> + * This driver performs the IRQ dispatch based on the flags
>> + */
>> +
>
> Multiline comments have this format:
>
> /*
> * First line
> * ....
> * Last line
> */
Ok.
>
>> +static void tango_irqchip_mask_irq(struct irq_data *data)
>> +{
>> + struct irq_domain *domain = irq_data_get_irq_chip_data(data);
>> + struct tango_irqrouter_output *irqrouter_output = domain->host_data;
>> + struct tango_irqrouter *irqrouter = irqrouter_output->context;
>> + int hwirq_in = (int)data->hwirq;
>
> Huch? What's that silly typecast for? Why can't you use u32 for hwirq_in?
data->hwirq is of type irq_hw_number_t (unsigned long), I don't know if that
can change in the future.
So do you think it is safe to use u32 for data->hwirq (and all other "irq"
occurrences)?
>
>> + tango_set_irq_enable(irqrouter, hwirq_in, 0);
>> +}
>
>> +#ifdef CONFIG_SMP
>> +static int tango_irqchip_set_irq_affinity(struct irq_data *data,
>> + const struct cpumask *mask_val,
>> + bool force)
>> +{
>> + struct irq_domain *domain = irq_data_get_irq_chip_data(data);
>> + struct tango_irqrouter_output *irqrouter_output = domain->host_data;
>> + struct irq_chip *parent_chip = irq_get_chip(irqrouter_output->virq);
>> + struct irq_data *parent_data = irq_get_irq_data(irqrouter_output->virq);
>> +
>> + DBGLOG("%s:%s(0x%p)\n",
>> + NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain);
>> +
>> + if (parent_chip && parent_chip->irq_set_affinity)
>> + return parent_chip->irq_set_affinity(parent_data,
>> + mask_val,
>> + force);
>> + else
>> + return -EINVAL;
>
> You really can spare identation levels by writing it a bit more clever:
>
> if (!parent || !parent->irq_set_affinity)
> return -EINVAL;
>
> return parent->irq_set_affinity(parent, mask, force);
>
> You might notice that I shortened the variable names and it still is entirely
> clear what the code does.
Ok.
>
> Aside of that mask_val is a clear misnomer.
Actually, this code comes from exynos-combiner.c and is part of the reason for
question 4 of the RFC:
----
4) Do I have to do something more to handle the affinity stuff?
----
>
>> +}
>> +#endif
>> +
>> +
>> +static inline u32 tango_dispatch_irqs(struct irq_domain *domain,
>> + struct irq_desc *desc,
>> + u32 status,
>> + int base)
>> +{
>> + u32 hwirq;
>> + u32 virq;
>
> Please put same types onto a single line
Ok.
>
>> +
>> + while (status) {
>> + hwirq = __ffs(status);
>> + virq = irq_find_mapping(domain, base + hwirq);
>> + if (unlikely(!virq))
>> + handle_bad_irq(desc);
>> + else
>> + generic_handle_irq(virq);
>> +
>> + status &= ~BIT(hwirq);
>> + }
>> +
>> + return status;
>
> Huch? status is guaranteed to be 0 here. So what's the point of this return
> value.
You are right, relic of a refactoring.
I wanted to do:
+ if (unlikely(!virq))
+ handle_bad_irq(desc);
+ else {
+ generic_handle_irq(virq);
+ status_out &= ~BIT(hwirq);
+ }
+ status &= ~BIT(hwirq);
with status_out=status at the start of the function.
>
>> +}
>> +
>> +static void tango_irqdomain_handle_cascade_irq(struct irq_desc *desc)
>> +{
>> + struct irq_domain *domain = irq_desc_get_handler_data(desc);
>> + struct tango_irqrouter_output *irqrouter_output = domain->host_data;
>> + struct tango_irqrouter *irqrouter = irqrouter_output->context;
>> + struct irq_chip *host_chip = irq_desc_get_chip(desc);
>> + u32 i, status;
>> + u32 swirq_status, irq_status[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
>> +
>> +#if 0
>> + DBGLOG("%s:%s(0x%p): irqrouter_output 0x%p, hwirq(out) %d\n",
>> + NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain,
>> + irqrouter_output, irqrouter_output->hwirq);
>> +#endif
>> +
>> + chained_irq_enter(host_chip, desc);
>> +
>> + raw_spin_lock(&(irqrouter->lock));
>> + swirq_status = tango_readl(irqrouter, READ_SWIRQ_STATUS);
>> + for (i = 0; i < BITMASK_VECTOR_SIZE(ROUTER_INPUTS); i++)
>> + irq_status[i] = tango_readl(irqrouter,
>> + READ_SYS_IRQ_GROUP0 + i*4);
>
> i * 4 please
Ok.
>
>> + raw_spin_unlock(&(irqrouter->lock));
>> +
>> + /* HW irqs */
>
> And that comment tells us what?
>
>> + for (i = 0; i < BITMASK_VECTOR_SIZE(ROUTER_INPUTS); i++) {
>> +#if 0
>> + DBGLOG("%d: 0x%08x (en 0x%08x, inv 0x%08x)\n",
>> + i,
>> + irq_status[i],
>> + irqrouter->irq_mask[0],
>> + irqrouter->irq_invert_mask[0]);
>> +#endif
>> +
>> +#define HANDLE_INVERTED_LINES(__irqstatus__, __x__) ((((~__irqstatus__) & irqrouter->irq_invert_mask[__x__]) & irqrouter->irq_mask[__x__]) | __irqstatus__)
>> +#define HANDLE_EN_AND_INV_MASKS(__irqstatus__, __y__) (HANDLE_INVERTED_LINES(__irqstatus__, __y__) & irqrouter->irq_mask[__y__])
>
> What the hell does this unparseable macro mess? What's so wrong with a proper
> inline function?
Ok, I'll inline it.
>
>> +
>> + irq_status[i] = HANDLE_EN_AND_INV_MASKS(irq_status[i], i);
>> + status = tango_dispatch_irqs(domain, desc, irq_status[i], i*32);
>> + if (status & irq_status[i])
>
> Can you pretty please explain how this can happen? It can't. Because you clear
> every single bit of status in tango_dispatch_irqs(). See above.
You are right that with the current code of tango_dispatch_irqs() it cannot happen,
but that's more of a typo. I wanted to have a way to print something from this
driver in the event of unhandled IRQs.
Just to understand something, is leaving some debug or checks somehow not recommended?
>
> Please clean up that unholy mess of nonsensical checks and debug code.
>
Ok.
>> + DBGERR("%s: %d unhandled IRQs (as a mask) 0x%x\n",
>> + NODE_NAME(irq_domain_get_of_node(domain)),
>> + i,
>> + status & irq_status[i]);
>> + }
>> +
>> + /* SW irqs */
>> + swirq_status &= irqrouter->swirq_mask;
>> + status = tango_dispatch_irqs(domain, desc, swirq_status, 128);
>
> 128 is the magic number pulled out of thin air or what?
I will replace that with irqrouter->input_count.
>
>> + if (status & swirq_status)
>> + DBGERR("%s: Unhandled IRQs (as a mask) 0x%x\n",
>> + NODE_NAME(irq_domain_get_of_node(domain)),
>> + status & swirq_status);
>
> See above.
>
>> +
>> + chained_irq_exit(host_chip, desc);
>> +}
>> +
>> +
>> +/**
>> + * tango_irqdomain_map - route a hwirq(in) to a hwirq(out).
>> + * NOTE: The hwirq(out) must have been already allocated and enabled on
>> + * the parent controller.
>
> Please put notes AFTER the argument descriptions
The note refers to the title:
"tango_irqdomain_map - route a hwirq(in) to a hwirq(out)."
>
>> + * @hwirq: HW IRQ of the device requesting an IRQ
>> + * if hwirq > inputs it is a SW IRQ
>> + * @virq: Linux IRQ (associated to the domain) to be given to the device
>> + * @domain: IRQ domain (from the domain, we get the irqrouter_output
>> + * in order to know to which output we need to route hwirq to)
>
> And if you spend a few seconds to align this proper, then it becomes suddenly
> readable.
>
> * @hwirq: HW IRQ of the device requesting an IRQ
> * if hwirq > inputs it is a SW IRQ
> * @virq: Linux IRQ (associated to the domain) to be given to the device
> * @domain: IRQ domain (from the domain, we get the irqrouter_output
> * in order to know to which output we need to route hwirq to)
>
> Hmm?
Ok.
>
>> + */
>> +static int tango_irqdomain_map(struct irq_domain *domain,
>> + unsigned int virq,
>> + irq_hw_number_t hwirq)
>> +{
>> + struct tango_irqrouter_output *irqrouter_output = domain->host_data;
>> + struct tango_irqrouter *irqrouter = irqrouter_output->context;
>> +
>> + DBGLOG("%s:%s(0x%p): hwirq(in) %d := virq %d, and route "
>> + "hwirq(in) %d => hwirq(out) %d (virq %d)\n",
>> + NODE_NAME(irq_domain_get_of_node(domain)),
>> + domain->name,
>> + domain,
>> + (u32)hwirq,
>> + virq,
>> + (u32)hwirq,
>> + irqrouter_output->hwirq,
>> + irqrouter_output->virq);
>> +
>> + if (hwirq >= (irqrouter->input_count + irqrouter->swirq_count))
>> + DBGERR("%s: Invalid hwirq(in) %d >= %d + %d\n",
>> + NODE_NAME(irq_domain_get_of_node(domain)),
>> + (u32)hwirq,
>> + irqrouter->input_count,
>> + irqrouter->swirq_count);
>> + else if (hwirq >= irqrouter->input_count)
>> + DBGLOG("Map swirq %ld\n", hwirq - irqrouter->input_count);
>> +
>> + irq_set_chip_and_handler(virq,
>> + &tango_irq_chip_shared_ops,
>> + handle_level_irq);
>> + irq_set_chip_data(virq, domain);
>> + irq_set_probe(virq);
>> +
>> + tango_set_irq_route(irqrouter, hwirq, irqrouter_output->hwirq);
>> +
>> + return 0;
>> +}
>> +
>> +
>> +/**
>> + * tango_irqdomain_translate - used to select the domain for a given
>> + * irq_fwspec
>
> It hardly selects the domain. @domain is handed in as a argument. And please
> get rid of that silly 'used to'. That just adds characters and no information.
>
> - Select the router section for a firmware spec
>
> Hmm?
Ok, maybe I did not express it correctly.
The callback is called from irqdomain.c:irq_find_matching_fwspec() and each
domain is tested, if the callback returns non-zero, the current domain is considered
a "match" for the fwspec and returned by irq_find_matching_fwspec().
>
>> + * @domain: a registered domain
>> + * @fwspec: an IRQ specification. This callback is used to translate the
>> + * parameters given as irq_fwspec into a HW IRQ and Type values.
>
> fwspec is a callback?
No, the sentence refers to the callback below (tango_irqdomain_translate) that is
being described by this comment.
I can reformulate or remove the comment.
>
>> + * @out_hwirq:
>> + * @out_type:
>
> Missing docs.....
Ok.
>
>> + */
>> +static int tango_irqdomain_translate(struct irq_domain *domain,
>> + struct irq_fwspec *fwspec,
>> + unsigned long *out_hwirq,
>> + unsigned int *out_type)
>> +{
>> + struct tango_irqrouter_output *irqrouter_output = domain->host_data;
>> + struct tango_irqrouter *irqrouter = irqrouter_output->context;
>> + irq_hw_number_t irq;
>> + u32 domain_id, type;
>> + int err;
>> +
>> + DBGLOG("%s:%s(0x%p): argc %d for hwirq(out) %d\n",
>> + NODE_NAME(irq_domain_get_of_node(domain)),
>> + domain->name,
>> + domain,
>> + fwspec->param_count,
>> + irqrouter_output->hwirq);
>> +
>> + err = tango_parse_fwspec(domain,
>> + fwspec,
>> + &domain_id,
>> + &irq,
>> + &type);
>
> Please use the full 80 characters. Your patch does not become more valuable
> when you inflate the line count. It's more valuable when it is easy to read
> and understand.
Ok.
As a side note, does the 80 characters limit still makes sense? I mean, I would
imagine that anybody now is using wide screens capable of at least double that
amount.
>
>> + if (err < 0) {
>> + DBGWARN("Failed to parse fwspec\n");
>> + return err;
>> + }
>> +
>> + switch (domain_id) {
>> + case SIGMA_HWIRQ:
>> + DBGLOG("Request is for SIGMA_HWIRQ\n");
>> + break;
>> + case SIGMA_SWIRQ:
>> + DBGLOG("Request is for SIGMA_SWIRQ\n");
>> + irq += irqrouter->input_count;
>> + break;
>> + default:
>> + DBGLOG("Request is for domain ID 0x%x (we are 0x%x)\n",
>> + domain_id,
>> + irqrouter_output->domain_id);
>
> Huch what? You only ever handle HW and SW irqs and now you happily proceed if
> the id is something else. Consistent error handling is optional and can be
> replaced by random debug prints, right?
You are right that this case should never happen as the 'select' callback must
have replied correctly.
This is more of a relic when I did not had access to the 'select' callback.
(I was working on 4.4)
>
>> + break;
>> + };
>> +
>> + *out_hwirq = irq;
>> + *out_type = type & IRQ_TYPE_SENSE_MASK;
>> +
>> + DBGLOG("hwirq %d type 0x%x\n", (u32)*out_hwirq, (u32)*out_type);
>> +
>> + return 0;
>> +}
>> +
>> +
>> +/**
>> + * tango_irqdomain_select - used to select the domain for a given irq_fwspec
>> + * @domain: a registered domain
>> + * @fwspec: an IRQ specification. This callback should return zero if the
>> + * irq_fwspec does not belong to the given domain. If it does, it should
>> + * return non-zero.
>
> Callback????
Ok.
>
>> + * In practice it will return non-zero if the irq_fwspec matches one of the
>> + * IRQs shared within the given domain.
>> + * @bus_token: a bus token
>> + */
>> +static int tango_irqdomain_select(struct irq_domain *domain,
>> + struct irq_fwspec *fwspec,
>> + enum irq_domain_bus_token bus_token)
>> +{
>> + struct tango_irqrouter_output *irqrouter_output = domain->host_data;
>> + struct tango_irqrouter *irqrouter = irqrouter_output->context;
>> + irq_hw_number_t irq;
>> + u32 domain_id, type;
>> + int err;
>> +
>> + DBGLOG("%s:%s(0x%p): argc %d, 0x%p, bus 0x%x\n",
>> + NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain,
>> + fwspec->param_count, fwspec->fwnode, bus_token);
>> +
>> + DBGLOG("router 0x%p, output 0x%p\n", irqrouter, irqrouter_output);
>> +
>> + err = tango_parse_fwspec(domain, fwspec, &domain_id, &irq, &type);
>> + if (err < 0)
>> + return 0;
>> +
>> + switch (domain_id) {
>> + case SIGMA_HWIRQ:
>> + DBGLOG("Request is for SIGMA_HWIRQ\n");
>> + break;
>> + case SIGMA_SWIRQ:
>> + DBGLOG("Request is for SIGMA_SWIRQ\n");
>> + break;
>> + default:
>> + DBGLOG("Request is for domain ID 0x%x (we are 0x%x)\n",
>> + domain_id,
>> + irqrouter_output->domain_id);
>> + break;
>> + };
>> +
>> + if (!irqrouter->implicit_groups) {
>> + int i;
>> +
>> + /* Check if the requested IRQ belongs to those listed
>> + * to be sharing the output assigned to this domain
>
> Your using of 'domain' is utterly confusing. Please use something like
> hwdomain or whatever which makes it clear that this is something else than the
> irq domain. I tripped over this several times now.
Ok.
>
>> + */
>> + if (irqrouter_output->shared_count <= 0) {
>> + DBGLOG("Not shared IRQ line?\n");
>> + return 0;
>> + }
>> +
>> + for (i = 0; i < irqrouter_output->shared_count; i++) {
>> + if (irq == irqrouter_output->shared_irqs[i]) {
>> + DBGLOG("Match: IRQ %lu\n", irq);
>> + return 1;
>> + }
>> + }
>> + } else {
>> + /* Otherwise, check if the domain_id given matches
>> + * the one assigned to this output
>> + */
>> + if (domain_id == irqrouter_output->domain_id) {
>> + DBGLOG("Match: Domain ID %d\n", domain_id);
>> + return 1;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +
>> +/* 'irqrouter' handling callbacks
>> + * Used for 'direct' IRQs, i.e.: IRQs that are directly routed to the GIC
>> + * This driver does not dispatch the IRQs, the GIC does.
>> + */
>> +
>> +
>> +static void tango_irqrouter_mask_irq(struct irq_data *data)
>> +{
>> + struct irq_domain *domain = irq_data_get_irq_chip_data(data);
>> + struct tango_irqrouter *irqrouter = domain->host_data;
>> + int hwirq_in = (int)data->hwirq;
>> +
>> + DBGLOG("%s:%s(0x%p) hwirq(in) %d\n",
>> + NODE_NAME(irq_domain_get_of_node(domain)),
>> + domain->name,
>> + domain,
>> + hwirq_in);
>> +
>> + tango_set_irq_enable(irqrouter, hwirq_in, 0);
>> +
>> + irq_chip_mask_parent(data);
>> +}
>> +
>> +
>> +static void tango_irqrouter_unmask_irq(struct irq_data *data)
>> +{
>> + struct irq_domain *domain = irq_data_get_irq_chip_data(data);
>> + struct tango_irqrouter *irqrouter = domain->host_data;
>> + int hwirq_in = (int)data->hwirq;
>> +
>> + DBGLOG("%s:%s(0x%p) hwirq(in) %d\n",
>> + NODE_NAME(irq_domain_get_of_node(domain)),
>> + domain->name,
>> + domain,
>> + hwirq_in);
>> +
>> + tango_set_irq_enable(irqrouter, hwirq_in, 1);
>> +
>> + irq_chip_unmask_parent(data);
>> +}
>> +
>> +
>> +static int tango_irqrouter_set_irq_type(struct irq_data *data,
>> + unsigned int type)
>> +{
>> + struct irq_domain *domain = irq_data_get_irq_chip_data(data);
>> + struct tango_irqrouter_output *irqrouter_output = NULL;
>> + struct tango_irqrouter *irqrouter = domain->host_data;
>> + int hwirq_in = (int)data->hwirq;
>> + u32 parent_type;
>> +
>> + DBGLOG("%s:%s(0x%p) type 0x%x for hwirq(in) %d\n",
>> + NODE_NAME(irq_domain_get_of_node(domain)),
>> + domain->name,
>> + domain,
>> + type,
>> + hwirq_in);
>> +
>> + tango_get_output_for_hwirq(irqrouter, hwirq_in, &irqrouter_output);
>> + if (!irqrouter_output)
>> + goto handle_parent;
>
> So if there is no output, then you unconditionally set the type on the
> parent. If that's correct, this needs a big fat comment at least.
Ok.
This is related to question 2 of my RFC:
----
2) In order to get a virq mapping for the domains associated with the outputs
(the outputs connected to the GIC) I request a sort of "Fake HW IRQ"
See irq-tango_v4.c:1400
/* To request a virq we need a HW IRQ, use a "Fake HW IRQ" */
hwirq = index + irqrouter->input_count + irqrouter->swirq_count;
This feels a bit like a hack, so suggestions are welcome.
----
Indeed, this callback will be called once when requesting a "fake HW IRQ" so
that alloc() callback is called and we get a free output (a GIC input) so
that we can later route IRQs to it.
And another time when an actual HW IRQ is requested, in which case the
tango_set_irq_type() needs to be called so that we can see if inversion needs
to be implemented.
Actually, if I don't deal with inversion and consider there will never be
mismatches between the level setup on the GIC input and that one of the
device requesting the IRQ to be routed to said GIC input, then I could just
use irq_chip_set_type_parent() as callback directly.
>
>> + parent_type = (irqrouter_output->hwirq_level & IRQ_TYPE_SENSE_MASK);
>> + tango_set_irq_type(irqrouter, hwirq_in, type, parent_type);
>> +
>> +handle_parent:
>> + return irq_chip_set_type_parent(data, type);
>> +}
>
> I'm stopping here. You can apply the above to the rest of the code as well.
>
>
Thanks again for your review.
Best regards,
Sebastian
[toc] | [prev] | [next] | [standalone]
| From | Marc Zyngier <marc.zyngier@arm.com> |
|---|---|
| Date | 2016-07-20 15:30 +0200 |
| Message-ID | <rWZJv-1YB-9@gated-at.bofh.it> |
| In reply to | #1447161 |
Thomas already commented on some aspects of the code, so I'm not going to do another review (I'll do the next round). On 20/07/16 12:06, Sebastian Frias wrote: > Hi Thomas, > > Thanks for your comments. > I appreciate the time you dedicated to the review. > I will take your comments into account, in the meanwhile, and since this was > a RFC, would it be possible to also get some feedback/comments from a > conceptual point of view? > > I'm copy/pasting some of the questions I attached to the RFC email here: > > ---- > 2) In order to get a virq mapping for the domains associated with the outputs > (the outputs connected to the GIC) I request a sort of "Fake HW IRQ" > See irq-tango_v4.c:1400 > > /* To request a virq we need a HW IRQ, use a "Fake HW IRQ" */ > hwirq = index + irqrouter->input_count + irqrouter->swirq_count; Why are you even trying to establish a route from a made up interrupt to a GIC line as the driver is just creating domains? Haven't you realized that everything gets created on demand, as DT interrupt specifiers gets parsed? So the only time where you create a irq_fwspec out of thin air is when you actually allocate *one* interrupt. And that's the only time. You don't pre-populate stuff, you don't pre-allocate stuff. > This feels a bit like a hack, so suggestions are welcome. Just look at all the existing drivers in the tree that use stacked domains. They are all based on the same template. At least try and follow it. > > 3) The file is called 'irq-tango_v4.c' but I think it should match the > compatible string, so I was thinking of renaming: > irq-tango_v4.c => irq-sigma_smp_irqrouter.c > irq-tango_v4.h => irq-sigma_smp_irqrouter.h > > What do you think? And why should it match? Am I going to rename the GIC driver to be "arm,gic-400_or_arm,arm11mp-gic_or_arm,arm1176jzf-devchip-gic_or_...c"? No. > > 4) Do I have to do something more to handle the affinity stuff? > > 6) More of a theoretical question: > I have to #include <dt-bindings/interrupt-controller/irq-tango_v4.h> from No you don't. This is for DT people to play with, and I don't care about it in the irqchip code. Thanks, M. -- Jazz is not dead. It just smells funny...
[toc] | [prev] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2016-07-20 16:50 +0200 |
| Message-ID | <rX0YW-2Gu-19@gated-at.bofh.it> |
| In reply to | #1447161 |
On Wed, 20 Jul 2016, Sebastian Frias wrote:
> ----
> 2) In order to get a virq mapping for the domains associated with the outputs
> (the outputs connected to the GIC) I request a sort of "Fake HW IRQ"
> See irq-tango_v4.c:1400
>
> /* To request a virq we need a HW IRQ, use a "Fake HW IRQ" */
> hwirq = index + irqrouter->input_count + irqrouter->swirq_count;
>
> This feels a bit like a hack, so suggestions are welcome.
Well, look at it from the hardware level
|----------------|
| 0|---- Device irqs
| 1|----
|----------| | 2|----
| 0 |-----| 3|----
| 1 |-----| Router .|----
| GIC ...|-----| .|----
| N |-----| .|----
|----------| | .|----
| .|----
| X|----
|----------------|
So you have X hardware irqs in your router chip and the GIC has N. When an
interrupt is created then we
- allocate the virq number
- associate the hwirq number of the router (input side)
- route it to a hwirq number of the GIC (output side)
So what kind of magic numbers do you need for that? I don't see any.
The only information which is interesting is whether the router output is
shared between several router inputs or not because that might short cut
interrupt handling. Though you can start with the simple assumption that every
output is shared. That's a few cpu cycles overhead in the irq delivery path,
but a great simplification of the code in the first place.
Your device tree describes
1) the association of the device irq to the router irq
2) the routing between router input and output
#1 is standard device tree magic
#2 is a property of your router irq chip
There is no requirement to describe all routes, you can limit it to those
routes which you want to be exclusive. The rest can be assigned
automatically.
Pseuso code:
#define ROUTE_NONE 0xFFFFFFFF
struct router {
u32 routes[NR_INPUTS];
bool exclusive[NR_OUTPUTS];
u32 nextout;
};
init()
struct router *router = kzalloc(sizeof(*router));
/* Clear all routes */
memset(router->routes, 0xFF, sizeof(router->routes);
/* Scan device tree entries */
for (info = node_get_next_route(node); info; ) {
/* TODO: Sanity check info->in and info->out */
router->routes[info->in] = info->out;
router->exclusive[info->out] = true;
info = node_get_next_route(node);
}
interrupt setup()
/*
* We get the hwirq of the router input in the DT/FW descriptor.
* Check whether the interrupt is routed by DT or should be
* assigned a route dynamically.
*/
if (router->routes[hwirq] != ROUTE_NONE) {
outirq = router->routes[hwirq];
} else {
/*
* Try to find the next non exclusive output
*/
outirq = router->nextout;
while (router->exclusive[outirq]) {
outirq++;
if (outirq == NR_OUTPUTS)
outirq = 0;
if (outirq == router->nextout)
return -ENOSPC;
}
router->nextout = outirq + 1;
if (router->nextout == NR_OUTPUTS)
router->nextout = 0;
}
/* Do the magic hardware initialization .... */
Hmm?
> 3) The file is called 'irq-tango_v4.c' but I think it should match the
> compatible string, so I was thinking of renaming:
> irq-tango_v4.c => irq-sigma_smp_irqrouter.c
> irq-tango_v4.h => irq-sigma_smp_irqrouter.h
>
> What do you think?
That's the least of my worries as long as the filename is unique and can be
associated to the hardware in a sensible way.
> 4) Do I have to do something more to handle the affinity stuff?
Well, I have no idea how your hardware works, but I assume that the affinity
is solely handled by the GIC. So delegating the affinity setting to the GIC is
all you can do.
> 6) More of a theoretical question:
> I have to #include <dt-bindings/interrupt-controller/irq-tango_v4.h> from
> code that is not in the Linux tree. That's fine for now, but if in the future
> there are other irq controllers and another header is required, how can the
> code know which header to #include ?
> Are we supposed to #include all of them, and then somehow detect which module
> is actually active and act accordingly? If so, can we detect which module
> matched and probed correctly to know which controller version we need to
> talk to?
-ENOPARSE
> What the driver does is:
> - creates a domain hierarchy attached to its parent domain (the GIC)
> - the callbacks for this domain hierarchy will be used to deal with IRQs
> "directly" routed to the GIC (i.e.: exclusive IRQ lines routed to the GIC
> and not shared).
> In this case the GIC dispatches the interrupts.
> - create linear domains attached to this driver's node.
> - the callbacks for these linear domains will be used to deal with shared
> IRQs (they share the routing to one of the GIC inputs).
> In this case this driver dispatches the interrupts after reading the flags.
>
> It is such concept, and then its implementation, that I would need some more
> advise with, because, as stated in the questions above, it still does not
> feels right, especially question 2.
The non shared case is merily an optimization of the shared case, but I
recommend to omit it for now and just deal with a very straight forward
implementation of the recommendation I outlined above. Making the non-shared
case special is simple once you got the above right.
> >> +#if 0
> >
> > Don't even think about posting code with "#if 0" or such in it.
> >
>
> The idea behind these macros was:
> - to easily enable/disable them.
> - to be more consistent with other code written at Sigma: this is a
> Sigma-only driver.
I don't care about Sigmas coding habits. This is kernel code and not
Sigma-only playground.
> > pr_debug() is there for a reason.
>
> Yes, but doesn't pr_debug() requires changes to the Makefile?
# git grep pr_debug Documentation/
> >
> >> +#if 0
> >> +#define SHORT_OR_FULL_NAME full_name
> >> +#else
> >> +#define SHORT_OR_FULL_NAME name
> >> +#endif
> >
> > Use one and be done with it. Really.
>
> By making the choice explicit, the person reading/debugging is aware of the possibilities.
> Picking "full_name" or "name" would likely prevent the person from knowing it has other debug options.
>
This does prevent absolutely nothing. Random #if 0 all over the place is a
nightmare and nothing else. And what do you win by printing one or the other
name? Absolutely nothing, really. Again, it might be Sigmas way to obfuscate
code and at the same time claiming that it makes it better to read/debug, but
that choice of coding style/habits have absolutely no place in proper kernel
code.
> >> +
> >> +#define NODE_NAME(__node__) (__node__ ? __node__->SHORT_OR_FULL_NAME : \
> >> + "<no-node>")
> >
> > Sigh. pr_xxx("%s", NULL) prints <null>. That's really sufficient for your
> > debug/error handling.
>
> The code is checking for __node__ to be non-NULL before dereferencing it.
pr_xxx("%s", node->name ? node->name: NULL)
What's wrong with explicitely writing stuff into the code where people can see
it instead of obfuscating everything with gazillion of macros.
> >> +static inline u32 tango_readl(struct tango_irqrouter *irqrouter,
> >> + int reg)
> >> +{
> >> + u32 val = readl_relaxed(irqrouter->base + reg);
> >> + /*DBGLOG("r[0x%08x + 0x%08x = 0x%08x] = 0x%08x\n",
> >> + irqrouter->base, reg, irqrouter->base + reg, val);*/
> >> + return val;
> >
> > Please get rid of this debug nonsense.
>
> Ok, although I can guarantee it was really useful to determine and prove
> that the HW documentation of the module was wrong...
That's fine. But that commented out stuff has no place in proper code. Use
pr_debug() if you really think that this information is valuable for debugging
beyond the initial experimentation. If it's of no use for the daily trouble
shooting with users, get rid of it.
> >> +static inline void tango_set_swirq_enable(struct tango_irqrouter *irqrouter,
> >> + int swirq,
> >> + bool enable)
> >> +{
> >> + u32 offset = SWIRQ_ENABLE;
> >
> > If you name that varible 'reg' then it becomes obvious what it does.
>
> Well, it was to add a sort of "consistency" between the code for SW IRQs and
> HW IRQs.
You try to force different things into the same implementation, which is
always a bad sign.
> >
>> + u32 value = tango_readl(irqrouter, offset);
> >> + u32 swirq_bit_index = swirq % SWIRQ_COUNT;
> >> +
> >> +#if 1
> >> + DBGLOG("%smask swirq(in) %d : current regvalue 0x%x\n",
> >> + enable ? "un":"",
> >> + swirq, value);
> >> +#endif
> >> +
> >> + if (enable) {
> >> + /* unmask swirq */
> >
> > The whole code lacks comments, but here you document the obvious ....
>
> I will remove the comments.
Instead of just removing the comments, please add comments where the stuff is
non obvious.
> >> +static inline void tango_set_swirq_inversion(struct tango_irqrouter *irqrouter,
> >> + int swirq,
> >> + bool invert)
> >> +{
> >> +
> >> + DBGLOG("swirq(in) %d %s inverted\n", swirq, invert ? "":"not");
> >> +
> >> + if (invert)
> >> + DBGERR("SW IRQs cannot be inverted!\n");
> >
> > Groan.
>
> I rather keep this to catch typos in code requesting SW IRQs.
Come on. Is that coding kindergarden or what?
> >> + if (irq_out < 0) {
> >
> > This logic is utterly confusing, really. What's a negative interrupt number?
>
> It is a sort of "magic value" to disable the IRQ and the routing.
And of course completely undocumented. Again you try to implement stuff which
should be seperate into a single implementation and end up with an unreadable
and therefor unmaintainable mess.
> >> + } else
> >
> > } else {
> >
> > please
>
> I think I'm confused, we need to use {} for single statements on "else" blocks,
> but not on "for" blocks?
for (i = 0; i < MAX; i++)
do_stuff(i);
for (i = 0; i < MAX; i++) {
do_stuff(i);
do_more_stuff(i);
}
if (bla < MAX)
do_stuff(bla);
else
do_something_else(bla);
if (bla < MAX) {
do_stuff(bla);
do_more_stuff(bla);
} else {
do_something_else(bla);
do_more_stuff(bla);
}
if (bla < MAX) {
do_stuff(bla);
do_more_stuff(bla);
} else {
do_something_else(bla);
}
if (bla < MAX) {
do_stuff(bla);
} else {
do_something_else(bla);
do_more_stuff(bla);
}
Now compare that to:
if (bla < MAX)
do_stuff(bla);
else {
do_something_else(bla);
do_more_stuff(bla);
}
It's harder to parse simply because the 'else {' is assymetric. Ditto for
if (bla < MAX) {
do_stuff(bla);
do_more_stuff(bla);
} else
do_something_else(bla);
Ok?
> >> + if (irq_out < 0) {
> >> + tango_set_irq_enable(irqrouter,
> >> + irq_in,
> >> + 0);
> >
> > This fits in a single line.
>
> Ok.
> Actually, I was wondering if for consistency it wouldn't be better if all
> function calls had one argument per line?
Err no. You waste precious vertical space and therefor context.
> >> +static inline int tango_set_irq_enable(struct tango_irqrouter *irqrouter,
> >> + int irq,
> >> + bool enable)
> >> +{
> >> + if (irq >= (irqrouter->input_count + irqrouter->swirq_count))
> >> + return -EINVAL;
> >
> > How can that happen? Paranoia programming?
>
> No, actually it can happen, that's the reason for question 2 of my RFC:
>
> ----
> 2) In order to get a virq mapping for the domains associated with the outputs
> (the outputs connected to the GIC) I request a sort of "Fake HW IRQ"
> See irq-tango_v4.c:1400
>
> /* To request a virq we need a HW IRQ, use a "Fake HW IRQ" */
> hwirq = index + irqrouter->input_count + irqrouter->swirq_count;
>
> This feels a bit like a hack, so suggestions are welcome.
It is a hack. See above.
> > This can be written way simpler.
> >
> > if (irq >= irqrouter->input_count)
> > irq -= irqrouter->input_count;
> > tango_set_hwirq_enable(irqrouter, irq, enable);
> >
> > Hmm?
>
> Ok, but is it ok to modify function parameters?
> I thought it was not recommended.
> Indeed, some people may assume they are constants.
They are only constant when they have a const qualifier in the function
prototype. Random assumptions of random people are irrelevant. You can use an
intermediate local variable and do the above if that makes you feel
better. That still will be way better readable and understandable code.
> >> +
> >> + if (err < 0) {
> >> + DBGWARN("Failed to setup IRQ %d polarity\n", hwirq_in);
> >> + return err;
> >> + }
> >> +
> >> + switch (type & IRQ_TYPE_SENSE_MASK) {
> >> + case IRQ_TYPE_EDGE_RISING:
> >> + case IRQ_TYPE_EDGE_FALLING:
> >> + DBGERR("Does not support edge triggers\n");
> >
> > This really sucks. In fact you panic here while the code suggests that you
> > print a DEBUG error and continue .....
> >
>
> If I use panic() here, do I still have to use 'break' as well?
It does not matter much.
> >> + default:
> >> + DBGWARN("Invalid trigger mode 0x%x for hwirq(in) %d\n",
> >> + type, hwirq_in);
> >
> > So here you continue with an error code, but above for EDGE you panic. I
> > really don't get that logic.
>
> Ok.
> Do you suggest I just make the code panic for any error? I'm fine with that.
I ask for consistent handling. Either you always panic or you try to handle
all errors gracefully. I prefer the latter because if it's not a essential
interrupt then you still have the chance to retrieve debug info.
panic or BUG() are the last resort when there is no way to keep the machine
alive and any action would perhaps cause even more fatal problems. That's
hardly the case when an interrupt cannot be set up.
> >> + if (!out_val)
> >> + return -EINVAL;
> >
> > This is utter crap. Look at the call site:
>
> I think I'm confused, so it is not recommended to check for NULL pointers
> before dereferencing them? Regardless of how the current code calls the
> function?
There is nothing wrong with defensive programming, but just adding checks to
every function bloats the code for no value. If you have a helper with a
single callsite, then such a check is just pointless and only there to make
you feel good or whatever. Global functions which can be called by random code
certainly want to have such checks, but then the callers are also required to
check the return values.
> >
> >> + struct tango_irqrouter_output *irqrouter_output = NULL;
> >> +
> >> + tango_get_output_for_hwirq(irqrouter, hwirq_in, &irqrouter_output);
> >> + if (!irqrouter_output)
> >> + goto handle_parent;
> >
> > So out_val CANNOT be NULL.
>
> You are right, with current code it can't.
> But I thought that it was a good practice to:
> "be lenient with your input and strict with your output"
See above.
> >What's the purpose of your return values if you
> > ignore them?
>
> I can make the function 'void'.
I made you a proposal for a useful replacement of that thing. See below.
> > This whole thing can be condensed to:
> >
> > static struct tango_irqrouter_output *
> > tango_get_output_for_hwirq(struct tango_irqrouter *irqr, int hwirq_in)
> > {
> > struct tango_irqrouter_output *rout;
> > int i, j;
> >
> > /* Scan the outputs for a matching hwirq number */
> > for (i = 0, rout = irqr->output; i < irqr->output_count; i++, rout++) {
> > for (j = 0; j < rout->shared_count; j++) {
> > if (rout->shared_irqs[j] == hwirq_in)
> > return rout;
> > }
> > }
> > return NULL;
> > }
> >
> > And the call site would become:
> >
> > struct tango_irqrouter_output *rout;
> >
> > rout = tango_get_output_for_hwirq(irqrouter, hwirq_in);
> > if (!rout)
> > goto handle_parent;
> >
> > Hmm?
>
> Ok.
> >> +static void tango_irqchip_mask_irq(struct irq_data *data)
> >> +{
> >> + struct irq_domain *domain = irq_data_get_irq_chip_data(data);
> >> + struct tango_irqrouter_output *irqrouter_output = domain->host_data;
> >> + struct tango_irqrouter *irqrouter = irqrouter_output->context;
> >> + int hwirq_in = (int)data->hwirq;
> >
> > Huch? What's that silly typecast for? Why can't you use u32 for hwirq_in?
>
> data->hwirq is of type irq_hw_number_t (unsigned long), I don't know if that
> can change in the future.
And because something might change in the future you slap a type cast on
it. Which will nicely prevent a compiler warning when the conversion after the
change is crap.
No. We only do type casts when there is a real reason and not something like
'might change in the future'.
Guess what happens if we change data->hwirq to be a pointer? Your typecast
still works ....
> So do you think it is safe to use u32 for data->hwirq (and all other "irq"
> occurrences)?
If your hwirq numbers are not going to exceed 2^32 ....
> >
> > Aside of that mask_val is a clear misnomer.
>
> Actually, this code comes from exynos-combiner.c and is part of the reason for
Copy and paste is not an excuse for bad code.
> >> +
> >> + irq_status[i] = HANDLE_EN_AND_INV_MASKS(irq_status[i], i);
> >> + status = tango_dispatch_irqs(domain, desc, irq_status[i], i*32);
> >> + if (status & irq_status[i])
> >
> > Can you pretty please explain how this can happen? It can't. Because you
> > clear every single bit of status in tango_dispatch_irqs(). See above.
>
> You are right that with the current code of tango_dispatch_irqs() it cannot
> happen, but that's more of a typo. I wanted to have a way to print something
> from this driver in the event of unhandled IRQs.
That's the fricking wrong place. If the dispatcher cannot handle an irq then
the dispatcher should issue the printout, not some random call site.
> >> +/**
> >> + * tango_irqdomain_map - route a hwirq(in) to a hwirq(out).
> >> + * NOTE: The hwirq(out) must have been already allocated and enabled on
> >> + * the parent controller.
> >
> > Please put notes AFTER the argument descriptions
>
> The note refers to the title:
> "tango_irqdomain_map - route a hwirq(in) to a hwirq(out)."
That does not make the comment a valid kernel doc comment. Format is:
/**
* function_name - Short function description
* @arg1: Description of arg1
* @argument2: Description of argument2
*
* Long description of the function including return values.
*/
Hint. You can build the kerneldoc stuff. It will tell you what's wrong.
> >> + * @domain: a registered domain
> >> + * @fwspec: an IRQ specification. This callback is used to translate the
> >> + * parameters given as irq_fwspec into a HW IRQ and Type values.
> >
> > fwspec is a callback?
>
> No, the sentence refers to the callback below (tango_irqdomain_translate) that is
> being described by this comment.
> I can reformulate or remove the comment.
Please do not remove useful comments. Make them proper.
> >> + err = tango_parse_fwspec(domain,
> >> + fwspec,
> >> + &domain_id,
> >> + &irq,
> >> + &type);
> >
> > Please use the full 80 characters. Your patch does not become more valuable
> > when you inflate the line count. It's more valuable when it is easy to read
> > and understand.
>
> Ok.
>
> As a side note, does the 80 characters limit still makes sense? I mean, I would
> imagine that anybody now is using wide screens capable of at least double that
> amount.
That's horrible. Did you ever think about why stuff is printed in columns or
the width is restricted?
There is a very simple reason: Your eyes cannot parse wide printed lines fast
without moving the focus while you can scan a limited width in one go.
> This is more of a relic when I did not had access to the 'select' callback.
> (I was working on 4.4)
Please remove relics. They are just a burden for review and they will confuse
the hell out of you 3 month from now.
Thanks,
tglx
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web