[permalink] [raw]

Subject: [RFC v1 10/25] sh: intc: Use irq_desc_get_xxx() to avoid redundant lookup of irq_desc

Use irq_desc_get_xxx() to avoid redundant lookup of irq_desc while we
already have a pointer to corresponding irq_desc.

Also replace generic_handle_irq with generic_handle_irq_desc() to avoid
looking up irq_desc again.

Signed-off-by: Jiang Liu <[email protected]>
---
drivers/sh/intc/core.c | 2 +-
drivers/sh/intc/virq.c | 14 ++++++++------
2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/sh/intc/core.c b/drivers/sh/intc/core.c
index 81f22980b2de..e4ca964ca840 100644
--- a/drivers/sh/intc/core.c
+++ b/drivers/sh/intc/core.c
@@ -67,7 +67,7 @@ void intc_set_prio_level(unsigned int irq, unsigned int level)

static void intc_redirect_irq(unsigned int irq, struct irq_desc *desc)
{
- generic_handle_irq((unsigned int)irq_get_handler_data(irq));
+ generic_handle_irq_desc(irq, desc);
}

static void __init intc_register_irq(struct intc_desc *desc,
diff --git a/drivers/sh/intc/virq.c b/drivers/sh/intc/virq.c
index 3dbc21696924..8083882a91c0 100644
--- a/drivers/sh/intc/virq.c
+++ b/drivers/sh/intc/virq.c
@@ -109,7 +109,7 @@ static int add_virq_to_pirq(unsigned int irq, unsigned int virq)

static void intc_virq_handler(unsigned int irq, struct irq_desc *desc)
{
- struct irq_data *data = irq_get_irq_data(irq);
+ struct irq_data *data = irq_desc_get_irq_data(desc);
struct irq_chip *chip = irq_data_get_irq_chip(data);
struct intc_virq_list *entry, *vlist = irq_data_get_irq_handler_data(data);
struct intc_desc_int *d = get_intc_desc(irq);
@@ -118,12 +118,14 @@ static void intc_virq_handler(unsigned int irq, struct irq_desc *desc)

for_each_virq(entry, vlist) {
unsigned long addr, handle;
+ struct irq_desc *vdesc = irq_to_desc(entry->irq);

- handle = (unsigned long)irq_get_handler_data(entry->irq);
- addr = INTC_REG(d, _INTC_ADDR_E(handle), 0);
-
- if (intc_reg_fns[_INTC_FN(handle)](addr, handle, 0))
- generic_handle_irq(entry->irq);
+ if (vdesc) {
+ handle = (unsigned long)irq_desc_get_handler_data(vdesc);
+ addr = INTC_REG(d, _INTC_ADDR_E(handle), 0);
+ if (intc_reg_fns[_INTC_FN(handle)](addr, handle, 0))
+ generic_handle_irq_desc(entry->irq, vdesc);
+ }
}

chip->irq_unmask(data);
--
1.7.10.4

2015-05-20 10:00:54

by Jiang Liu

[permalink] [raw]

Subject: [RFC v1 11/25] keystone, irq: Use irq_data_get_xxx() to avoid redundant lookup of irq_data

Use irq_data_get_xxx() to avoid redundant lookup of irq_data while we
already have a pointer to corresponding irq_data.

Signed-off-by: Jiang Liu <[email protected]>
---
drivers/pci/host/pci-keystone-dw.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/host/pci-keystone-dw.c b/drivers/pci/host/pci-keystone-dw.c
index f34892e0edb4..e42d077039d3 100644
--- a/drivers/pci/host/pci-keystone-dw.c
+++ b/drivers/pci/host/pci-keystone-dw.c
@@ -104,14 +104,13 @@ static void ks_dw_pcie_msi_irq_ack(struct irq_data *d)
{
u32 offset, reg_offset, bit_pos;
struct keystone_pcie *ks_pcie;
- unsigned int irq = d->irq;
struct msi_desc *msi;
struct pcie_port *pp;

- msi = irq_get_msi_desc(irq);
+ msi = irq_data_get_msi_desc(d);
pp = sys_to_pcie(msi->dev->bus->sysdata);
ks_pcie = to_keystone_pcie(pp);
- offset = irq - irq_linear_revmap(pp->irq_domain, 0);
+ offset = d->irq - irq_linear_revmap(pp->irq_domain, 0);
update_reg_offset_bit_pos(offset, &reg_offset, &bit_pos);

writel(BIT(bit_pos),
@@ -142,15 +141,14 @@ void ks_dw_pcie_msi_clear_irq(struct pcie_port *pp, int irq)
static void ks_dw_pcie_msi_irq_mask(struct irq_data *d)
{
struct keystone_pcie *ks_pcie;
- unsigned int irq = d->irq;
struct msi_desc *msi;
struct pcie_port *pp;
u32 offset;

- msi = irq_get_msi_desc(irq);
+ msi = irq_data_get_msi_desc(d);
pp = sys_to_pcie(msi->dev->bus->sysdata);
ks_pcie = to_keystone_pcie(pp);
- offset = irq - irq_linear_revmap(pp->irq_domain, 0);
+ offset = d->irq - irq_linear_revmap(pp->irq_domain, 0);

/* Mask the end point if PVM implemented */
if (IS_ENABLED(CONFIG_PCI_MSI)) {
@@ -164,15 +162,14 @@ static void ks_dw_pcie_msi_irq_mask(struct irq_data *d)
static void ks_dw_pcie_msi_irq_unmask(struct irq_data *d)
{
struct keystone_pcie *ks_pcie;
- unsigned int irq = d->irq;
struct msi_desc *msi;
struct pcie_port *pp;
u32 offset;

- msi = irq_get_msi_desc(irq);
+ msi = irq_data_get_msi_desc(d);
pp = sys_to_pcie(msi->dev->bus->sysdata);
ks_pcie = to_keystone_pcie(pp);
- offset = irq - irq_linear_revmap(pp->irq_domain, 0);
+ offset = d->irq - irq_linear_revmap(pp->irq_domain, 0);

/* Mask the end point if PVM implemented */
if (IS_ENABLED(CONFIG_PCI_MSI)) {
--
1.7.10.4

2015-05-20 10:01:02

by Jiang Liu

[permalink] [raw]

Subject: [RFC v1 12/25] spmi: Use irq_desc_get_xxx() to avoid redundant lookup of irq_desc

Use irq_desc_get_xxx() to avoid redundant lookup of irq_desc while we
already have a pointer to corresponding irq_desc.

Signed-off-by: Jiang Liu <[email protected]>
---
drivers/spmi/spmi-pmic-arb.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index d7119db49cfe..b32cb0a813cb 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -453,8 +453,8 @@ static void periph_interrupt(struct spmi_pmic_arb_dev *pa, u8 apid)

static void pmic_arb_chained_irq(unsigned int irq, struct irq_desc *desc)
{
- struct spmi_pmic_arb_dev *pa = irq_get_handler_data(irq);
- struct irq_chip *chip = irq_get_chip(irq);
+ struct spmi_pmic_arb_dev *pa = irq_desc_get_handler_data(desc);
+ struct irq_chip *chip = irq_desc_get_chip(desc);
void __iomem *intr = pa->intr;
int first = pa->min_apid >> 5;
int last = pa->max_apid >> 5;
--
1.7.10.4

2015-05-20 10:01:08

by Jiang Liu

[permalink] [raw]

Subject: [RFC v1 13/25] genirq: Kill the parameter 'irq' of kstat_incr_irqs_this_cpu()

The first parameter 'irq' is never used by kstat_incr_irqs_this_cpu(),
so kill it. So one step further to kill the first parameter 'irq'
of irq_flow_handler_t.

Signed-off-by: Jiang Liu <[email protected]>
---
kernel/irq/chip.c | 16 ++++++++--------
kernel/irq/handle.c | 2 +-
kernel/irq/internals.h | 2 +-
kernel/irq/irqdesc.c | 2 +-
4 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index d7a50974b77d..fada8daa6b19 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -315,7 +315,7 @@ void handle_nested_irq(unsigned int irq)
raw_spin_lock_irq(&desc->lock);

desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING);
- kstat_incr_irqs_this_cpu(irq, desc);
+ kstat_incr_irqs_this_cpu(desc);

action = desc->action;
if (unlikely(!action || irqd_irq_disabled(&desc->irq_data))) {
@@ -391,7 +391,7 @@ handle_simple_irq(unsigned int irq, struct irq_desc *desc)
goto out_unlock;

desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING);
- kstat_incr_irqs_this_cpu(irq, desc);
+ kstat_incr_irqs_this_cpu(desc);

if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) {
desc->istate |= IRQS_PENDING;
@@ -443,7 +443,7 @@ handle_level_irq(unsigned int irq, struct irq_desc *desc)
goto out_unlock;

desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING);
- kstat_incr_irqs_this_cpu(irq, desc);
+ kstat_incr_irqs_this_cpu(desc);

/*
* If its disabled or no action available
@@ -515,7 +515,7 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc)
goto out;

desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING);
- kstat_incr_irqs_this_cpu(irq, desc);
+ kstat_incr_irqs_this_cpu(desc);

/*
* If its disabled or no action available
@@ -583,7 +583,7 @@ handle_edge_irq(unsigned int irq, struct irq_desc *desc)
goto out_unlock;
}

- kstat_incr_irqs_this_cpu(irq, desc);
+ kstat_incr_irqs_this_cpu(desc);

/* Start handling the irq */
desc->irq_data.chip->irq_ack(&desc->irq_data);
@@ -646,7 +646,7 @@ void handle_edge_eoi_irq(unsigned int irq, struct irq_desc *desc)
goto out_eoi;
}

- kstat_incr_irqs_this_cpu(irq, desc);
+ kstat_incr_irqs_this_cpu(desc);

do {
if (unlikely(!desc->action))
@@ -675,7 +675,7 @@ handle_percpu_irq(unsigned int irq, struct irq_desc *desc)
{
struct irq_chip *chip = irq_desc_get_chip(desc);

- kstat_incr_irqs_this_cpu(irq, desc);
+ kstat_incr_irqs_this_cpu(desc);

if (chip->irq_ack)
chip->irq_ack(&desc->irq_data);
@@ -705,7 +705,7 @@ void handle_percpu_devid_irq(unsigned int irq, struct irq_desc *desc)
void *dev_id = raw_cpu_ptr(action->percpu_dev_id);
irqreturn_t res;

- kstat_incr_irqs_this_cpu(irq, desc);
+ kstat_incr_irqs_this_cpu(desc);

if (chip->irq_ack)
chip->irq_ack(&desc->irq_data);
diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index 635480270858..4d37b96343e9 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -30,7 +30,7 @@
void handle_bad_irq(unsigned int irq, struct irq_desc *desc)
{
print_irq_desc(irq, desc);
- kstat_incr_irqs_this_cpu(irq, desc);
+ kstat_incr_irqs_this_cpu(desc);
ack_bad_irq(irq);
}

diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index d82a77d39aeb..ab675f282291 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -193,7 +193,7 @@ static inline bool irqd_has_set(struct irq_data *d, unsigned int mask)
return __irqd_to_state(d) & mask;
}

-static inline void kstat_incr_irqs_this_cpu(unsigned int irq, struct irq_desc *desc)
+static inline void kstat_incr_irqs_this_cpu(struct irq_desc *desc)
{
__this_cpu_inc(*desc->kstat_irqs);
__this_cpu_inc(kstat.irqs_sum);
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 358f620c035a..500e6fd11d78 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -586,7 +586,7 @@ int irq_set_percpu_devid(unsigned int irq)

void kstat_incr_irq_this_cpu(unsigned int irq)
{
- kstat_incr_irqs_this_cpu(irq, irq_to_desc(irq));
+ kstat_incr_irqs_this_cpu(irq_to_desc(irq));
}

/**
--
1.7.10.4

2015-05-20 10:01:27

by Jiang Liu

[permalink] [raw]

2015-05-20 10:04:25

by Jiang Liu

[permalink] [raw]

Subject: [RFC v1 18/25] genirq: Kill the first parameter 'irq' of irq_flow_handler_t

2015-05-20 10:04:51

by Jiang Liu

[permalink] [raw]

Subject: [RFC v1 19/25] genirq: Kill the first parameter 'irq' of irq_flow_handler_t

2015-05-20 10:05:35

by Jiang Liu

[permalink] [raw]

2015-05-20 10:07:11

by Jiang Liu

[permalink] [raw]

On Fri, 12 Jun 2015, Julia Lawall wrote:
> What is the status of this? I am close to having a semantic patch that
> works. The current version touches 133 files, but I haven't checked all
> of them. This includes both the local functions and the generic ones, but
> not the cases where the name of the handler function is a local variable
> or arbitrary expression. This occurs in around 30 files.

Jiang sent out a new patch series, but I'm really interested in doing
a fully automated check. If conversion is possible for some of them,
fine, but the checking part to find all the places where this needs to
be applied is the most important to me.

If you want, just send me that script and I'll give it a test ride.

Thanks,

tglx

2015-06-12 22:16:37

A new version of the semantic patch is below. The results are attached.

julia

virtual after_start

@initialize:ocaml@
@@

let tbl = Hashtbl.create(100)
let ltbl = Hashtbl.create(100)

let add_if_not_present f p =
try let _ = Hashtbl.find tbl f in ()
with Not_found ->
Hashtbl.add tbl f p;
let it = new iteration() in
it#add_virtual_rule After_start;
it#add_virtual_identifier Hf f;
it#register()

let local_add_if_not_present f file =
try let _ = Hashtbl.find ltbl (f,file) in ()
with Not_found ->
Hashtbl.add ltbl (f,file) ();
let it = new iteration() in
it#set_files [file];
it#add_virtual_rule After_start;
it#add_virtual_identifier Hf f;
it#register()

// ------------------------------------------------------------------------
// Getting started

@h depends on !after_start@
identifier ih != NULL;
local idexpression lih;
expression eh;
expression e1,e2,e3,e4,e5,e6,e7;
position p;
@@

(
__irq_set_handler@p(e1,\(lih\|ih\|eh\),e2,e3)
|
irq_set_handler@p(e1,\(lih\|ih\|eh\))
|
irq_set_chained_handler@p(e1,\(lih\|ih\|eh\))
|
irq_alloc_generic_chip@p(e1,e2,e3,\(lih\|ih\|eh\))
|
irq_alloc_domain_generic_chips@p(e1,e2,e3,e4,\(lih\|ih\|eh\),e5,e6,e7)
|
irq_set_chip_and_handler_name@p(e1,e2,\(lih\|ih\|eh\),e3)
|
irq_set_chip_and_handler@p(e1,e2,\(lih\|ih\|eh\))
|
__irq_set_handler_locked@p(e1,\(lih\|ih\|eh\))
|
__irq_set_chip_handler_name_locked@p(e1,e2,\(lih\|ih\|eh\),e3)
|
__irq_set_preflow_handler@p(e1,\(lih\|ih\|eh\))
)

@localfn@
identifier h.ih;
@@

ih(...) { ... }

@script:ocaml depends on !localfn@
p << h.p;
handler << h.ih;
@@

add_if_not_present handler p

@script:ocaml depends on localfn@
p << h.p;
handler << h.ih;
@@

local_add_if_not_present handler (List.hd p).file

@script:ocaml@
p << h.p;
handler << h.lih;
@@

Printf.eprintf "Local variable: %s:%d %s\n"
((List.hd p).file) ((List.hd p).line) handler

@script:ocaml@
p << h.p;
handler << h.eh;
@@

if not (handler = "NULL")
then Printf.eprintf "Arbitrary expression: %s:%d %s\n"
((List.hd p).file) ((List.hd p).line) handler

// ------------------------------------------------------------------------
// Adjusting functions

@@
identifier virtual.hf,irq;
fresh identifier firq = "__" ## irq;
type T;
@@

hf(T
- irq
+ firq
,...) {
... when != irq
when strict
}

@s@
identifier virtual.hf,irq;
position p1;
type T;
@@

hf(T irq,...) {
<...
(irq@p1 = <+...irq...+>)
...>
}

@s1@
identifier virtual.hf,irq;
position p1;
expression e;
statement S;
type T;
@@

hf(T irq,...) {
<...
for(...; ...; <+...irq@p1 = e...+>) S
...>
}

// first parameter is assigned at least once
@r exists@
identifier virtual.hf,irq;
expression e;
type T;
position p,p1 != {s.p1,s1.p1};
@@

hf(T@p irq,...) {
... when != irq
when strict
irq@p1 = e
... when any
}

// no uses of the first parameter before the assignment
@@
identifier virtual.hf,irq;
expression e;
type T;
fresh identifier firq = "__" ## irq;
position r.p,p1 != {s.p1,s1.p1};
@@

hf(T@p
- irq
+ firq
,...) {
... when != irq
when strict
? irq@p1 = e
... when any
}

// first parameter is used somewhere
@rr exists@
identifier virtual.hf,irq;
type T;
position p;
@@

hf(T@p irq,...) {
...
irq
... when any
}

@@
identifier virtual.hf,irq,desc;
fresh identifier firq = "__" ## irq;
type T,T1;
position rr.p;
@@

hf(T@p
- irq
+ firq
,T1 desc) {
+ unsigned int irq = irq_desc_get_irq(desc);
... when any
}

Attachments:

irq_handler.out (83.96 kB)

2015-06-13 14:53:47

> Now where my cocci foo ends is when I try to match both types in the
> same rule, i.e.
>
> @ fun exists @
> identifier hf,irq,desc;
> typedef u32;
> @@
>
> (
> void hf(unsigned irq, struct irq_desc *desc) { ... }
> |
> void hf(u32 irq, struct irq_desc *desc) { ... }
> )
>
> results in:
> Fatal error: exception Failure("minus: parse error:
> = File "../cocci/find-pot-handler.cocci", line 7, column 5, charpos = 63
> around = 'hf', whole content = void hf(unsigned irq, struct irq_desc *desc) { ... }
> ")

Yeah, sorry, it doesn't work. There is no way to put a disjunction on
a function definition or a parameter type.

> I tried a couple of other variants, but finally gave up and ran a
> cocci script first which does s/u32/unsigned/ on those functions, but
> of course because I'm lazy I wanted to do everything in one go. :)
>
> Aside of that the ruleset you gave me works nicely except for that
> part:
>
> // no uses of the first parameter before the assignment
> @@
> identifier fun.hf,irq;
> expression e;
> type T;
> fresh identifier firq = "__" ## irq;
> position r.p,p1 != {s.p1,s1.p1};
> @@
>
> hf(T@p
> - irq
> + firq
> ,...) {
> ... when != irq
> when strict
> ? irq@p1 = e
> ... when any
> }
>
> That lacks a:
>
> + unsigned int irq;
>
> and therefor missed to add the local variable 'unsigned int irq' in
> case the parameter is only used for local storage.

Oops, thanks.

julia

> That was simple enough to fix even for me :)
>
> Thanks,
>
> tglx
>
>