diff mbox

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

Message ID alpine.DEB.2.11.1506131130490.3786@nanos (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Thomas Gleixner June 13, 2015, 9:32 a.m. UTC 
On Sat, 13 Jun 2015, Julia Lawall wrote:

> >     unsigned irq = irq_desc_get_irq(desc);
> 
> Actually, it seems that this function doesn't exist in linux-next either.
> What is the correct name of the function, or is there some other tree that
> I should be working on?

That's a new helper function, which is introduced in that series.

Thanks,

	tglx

---
 include/linux/irqdesc.h |    5 +++++
 1 file changed, 5 insertions(+)

Comments

Jiang Liu June 13, 2015, 4:49 p.m. UTC | #1 
On 2015/6/13 22:53, Julia Lawall wrote:
> Better formatted diff.
Great thanks to you, Julia!
It seems really attractively, I will try to figure out
whether I have caught all the cases.

BTW, a new version of these patch set has been posted at:
http://comments.gmane.org/gmane.linux.kernel.pci/42320

I have also push the new version to github in case you need
to access it:
 https://github.com/jiangliu/linux.git irq:irqdesc_v2

Thanks!
Gerry
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Julia Lawall June 13, 2015, 8:15 p.m. UTC | #2 
The following are the cases that require some manual attention, either 
because the handler is a local variable or because it is some other kind 
of expression.

julia

Local variable: arch/x86/kernel/apic/io_apic.c:932 hdl
Local variable: arch/sparc/kernel/leon_kernel.c:232 flow_handler
Local variable: arch/sparc/kernel/leon_kernel.c:257 flow_handler
Local variable: arch/powerpc/platforms/52xx/mpc52xx_pic.c:199 handler
Local variable: arch/powerpc/platforms/52xx/mpc52xx_pic.c:364 hndlr
Local variable: arch/powerpc/sysdev/qe_lib/qe_ic.c:377 high_handler
Local variable: arch/powerpc/sysdev/qe_lib/qe_ic.c:372 low_handler
Local variable: arch/mips/cavium-octeon/octeon-irq.c:69 handler
Local variable: arch/mips/alchemy/common/irq.c:494 handler
Local variable: arch/mips/alchemy/common/irq.c:706 hdl
Local variable: arch/blackfin/mach-common/ints-priority.c:693 handle
Local variable: arch/tile/kernel/irq.c:239 handle
Local variable: arch/m68k/kernel/ints.c:124 handle
Local variable: drivers/irqchip/irq-clps711x.c:152 handler
Local variable: drivers/gpio/gpio-ep93xx.c:211 handler
Local variable: drivers/gpio/gpiolib.c:447 parent_handler
Local variable: drivers/pinctrl/intel/pinctrl-cherryview.c:1328 handler
Local variable: kernel/irq/irqdomain.c:946 handler
Local variable: kernel/irq/irqdomain.c:1251 handler
Local variable: kernel/irq/chip.c:787 handle
Arbitrary expression: arch/ia64/kernel/iosapic.c:613 trigger == IOSAPIC_EDGE ? handle_edge_irq : handle_level_irq
Arbitrary expression: arch/powerpc/sysdev/mpic.c:1669 & mpic_cascade
Arbitrary expression: arch/mips/dec/ioasic-irq.c:111 1 << ( i - base ) & IO_IRQ_DMA_INFO ? handle_edge_irq : handle_fasteoi_irq
Arbitrary expression: drivers/irqchip/irq-or1k-pic.c:131 pic -> handle
Arbitrary expression: drivers/iio/industrialio-trigger.c:449 & handle_simple_irq
Arbitrary expression: drivers/gpio/gpio-sta2x11.c:347 ct -> handler
Arbitrary expression: drivers/gpio/gpiolib.c:482 chip -> irq_handler
Arbitrary expression: drivers/pinctrl/samsung/pinctrl-s3c24xx.c:517 handlers [ i ]
Arbitrary expression: drivers/pinctrl/samsung/pinctrl-s3c64xx.c:734 s3c64xx_eint0_handlers [ i ]
Arbitrary expression: drivers/staging/iio/iio_dummy_evgen.c:89 & handle_simple_irq
Arbitrary expression: kernel/irq/generic-chip.c:461 ct -> handler
Arbitrary expression: kernel/irq/msi.c:167 info -> handler
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Gleixner June 13, 2015, 8:32 p.m. UTC | #3 
On Sat, 13 Jun 2015, Julia Lawall wrote:

> The following are the cases that require some manual attention, either 
> because the handler is a local variable or because it is some other kind 
> of expression.

yep, found it in the logs already and found a missing search expression:

|
gpiochip_set_chained_irqchip@p(e1,e2,e3,\(lih\|ih\|eh\))

This is really impressive work!!!!

Thanks

	tglx
 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Julia Lawall June 13, 2015, 8:42 p.m. UTC | #4 
On Sat, 13 Jun 2015, Thomas Gleixner wrote:

> On Sat, 13 Jun 2015, Julia Lawall wrote:
> 
> > The following are the cases that require some manual attention, either 
> > because the handler is a local variable or because it is some other kind 
> > of expression.
> 
> yep, found it in the logs already and found a missing search expression:
> 
> |
> gpiochip_set_chained_irqchip@p(e1,e2,e3,\(lih\|ih\|eh\))
> 
> This is really impressive work!!!!

Thanks :)

I'll add the above pattern, but I guess you already have the results for 
that case.

julia
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Julia Lawall June 13, 2015, 9:42 p.m. UTC | #5 
The semantic patch below considers the problem from another point of view, 
to find functions that may have a handler as an argument.  The results 
that are different than those considered already are as follows:

arch/x86/platform/uv/uv_irq.c:106 irq_domain_set_info
	has a possible handler in argument 6
arch/sparc/kernel/leon_pci_grpci2.c:815 leon_update_virq_handling
	has a possible handler in argument 2
arch/sparc/kernel/leon_kernel.c:79 leon_build_device_irq
	has a possible handler in argument 2
arch/arm/mach-omap2/prm_common.c:331 irq_alloc_generic_chip
	has a possible handler in argument 5
arch/mips/cavium-octeon/octeon-irq.c:1231 octeon_irq_set_ciu_mapping
	has a possible handler in argument 6
arch/blackfin/mach-common/ints-priority.c:805 bfin_set_irq_handler
	has a possible handler in argument 2
arch/m68k/q40/q40ints.c:84 m68k_setup_irq_controller
	has a possible handler in argument 2
drivers/gpio/gpio-pcf857x.c:362 gpiochip_irqchip_add
	has a possible handler in argument 4

The semantic patch looks for a function that has as argument a function 
that has arguments of type (unsigned int irq, struct irq_desc *desc) and a 
void return type.  It also looks for functions that have as argument a 
bunch of known irq handlers.

julia


@initialize:ocaml@
@@
let tbl = Hashtbl.create 101
let ln = Str.regexp "linux-next/" (* update as appropriate *)

let _ =
  List.iter (function x -> Hashtbl.add tbl x ())
    [("__irq_set_handler",2);("irq_set_handler",2);
      ("irq_set_chained_handler",2);("irq_alloc_generic_chip",4);
      ("irq_alloc_domain_generic_chips",5);("irq_set_chip_and_handler_name",3);
      ("irq_set_chip_and_handler",3);("__irq_set_handler_locked",2);
      ("__irq_set_chip_handler_name_locked",3);("__irq_set_preflow_handler",2);
      ("gpiochip_set_chained_irqchip",4)]

let update g f n p =
  let n = n + 1 in
  let entry = (g,n) in
  try let _ = Hashtbl.find tbl entry in ()
  with Not_found ->
    begin
      Hashtbl.add tbl entry ();
      let p = List.hd p in
      Printf.printf "%s:%d %s\n\thas a possible handler in argument %d\n"
	(List.nth (Str.split ln p.file) 1) p.line g
	n
    end

@possible_handler@
identifier f,irq,desc;
@@

void f(unsigned int irq, struct irq_desc *desc) { ... }

@call@
identifier g,possible_handler.f;
expression list[n] es;
position p;
@@

g(es,f@p,...)

@script:ocaml@
p << call.p;
f << possible_handler.f;
g << call.g;
n << call.n;
@@

update g f n p

@call2@
identifier g,f;
expression list[n] es;
position p;
symbol handle_level_irq,handle_simple_irq,handle_edge_irq,
       handle_fasteoi_irq,handle_percpu_irq;
@@

g(es,\(handle_level_irq@f@p\|handle_simple_irq@f@p\|handle_edge_irq@f@p\|
       handle_fasteoi_irq@f@p\|handle_percpu_irq@f@p\),...)

@script:ocaml@
p << call2.p;
f << call2.f;
g << call2.g;
n << call2.n;
@@

update g f n p
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Gleixner June 24, 2015, 9:10 p.m. UTC | #6 
On Sat, 13 Jun 2015, Julia Lawall wrote:
> On Sat, 13 Jun 2015, Thomas Gleixner wrote:
> 
> > On Sat, 13 Jun 2015, Julia Lawall wrote:
> > 
> > > The following are the cases that require some manual attention, either 
> > > because the handler is a local variable or because it is some other kind 
> > > of expression.
> > 
> > yep, found it in the logs already and found a missing search expression:
> > 
> > |
> > gpiochip_set_chained_irqchip@p(e1,e2,e3,\(lih\|ih\|eh\))
> > 
> > This is really impressive work!!!!
> 
> Thanks :)
> 
> I'll add the above pattern, but I guess you already have the results for 
> that case.

Yes.

I did some more experiments and analyzed all functions which have the
signature

[static] void f(T irq, struct irq_desc *desc)

where T is either 'unsigned int' or 'u32'

That throws up a handful of false positives, but those are functions
which are called from a real handler function, so we want to look at
those anyway.

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) { ... }
")

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.

That was simple enough to fix even for me :)

Thanks,

	tglx

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Julia Lawall June 24, 2015, 9:44 p.m. UTC | #7 
> 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
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index 8c84a2513665..a7aaef44a10e 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -100,6 +100,11 @@  static inline struct irq_desc *irq_data_to_desc(struct irq_data *data)
 #endif
 }
 
+static inline unsigned int irq_desc_get_irq(struct irq_desc *desc)
+{
+	return desc->irq_data.irq;
+}
+
 static inline struct irq_data *irq_desc_get_irq_data(struct irq_desc *desc)
 {
 	return &desc->irq_data;