Patchwork [5/5] sparc64: use up-to-data genirq functions

login
register
mail settings
Submitter Sam Ravnborg
Date Jan. 21, 2011, 10:26 p.m.
Message ID <1295648816-29516-5-git-send-email-sam@ravnborg.org>
Download mbox | patch
Permalink /patch/79958/
State RFC
Delegated to: David Miller
Headers show

Comments

Sam Ravnborg - Jan. 21, 2011, 10:26 p.m.
Drop all uses of deprecated genirq features

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
---
 arch/sparc/Kconfig         |    2 +
 arch/sparc/kernel/irq_64.c |  119 ++++++++++++++++++++++----------------------
 2 files changed, 62 insertions(+), 59 deletions(-)
Josip Rodin - Jan. 21, 2011, 11:43 p.m.
On Fri, Jan 21, 2011 at 11:26:56PM +0100, Sam Ravnborg wrote:
> Drop all uses of deprecated genirq features

> -static int sun4u_set_affinity(unsigned int virt_irq,
> -			       const struct cpumask *mask)
> +static int sun4u_set_affinity(struct irq_data *data,
> +			       const struct cpumask *mask, bool force)
> -static int sun4v_set_affinity(unsigned int virt_irq,
> -			       const struct cpumask *mask)
> +static int sun4v_set_affinity(struct irq_data *data,
> +			       const struct cpumask *mask, bool force)
> -static int sun4v_virt_set_affinity(unsigned int virt_irq,
> -				    const struct cpumask *mask)
> +static int sun4v_virt_set_affinity(struct irq_data *data,
> +				    const struct cpumask *mask, bool force)

For the benefit of future readers, maybe add a short comment to the commit
description to explain why these functions are now apparently ignoring the
'force' parameter, that is, does it not apply because it's not presently
implemented, or because it just doesn't make sense.

Just nitpicking, feel free to ignore :)
David Miller - Jan. 22, 2011, 12:02 a.m.
From: Josip Rodin <joy@entuzijast.net>
Date: Sat, 22 Jan 2011 00:43:44 +0100

> On Fri, Jan 21, 2011 at 11:26:56PM +0100, Sam Ravnborg wrote:
>> Drop all uses of deprecated genirq features
> 
>> -static int sun4u_set_affinity(unsigned int virt_irq,
>> -			       const struct cpumask *mask)
>> +static int sun4u_set_affinity(struct irq_data *data,
>> +			       const struct cpumask *mask, bool force)
>> -static int sun4v_set_affinity(unsigned int virt_irq,
>> -			       const struct cpumask *mask)
>> +static int sun4v_set_affinity(struct irq_data *data,
>> +			       const struct cpumask *mask, bool force)
>> -static int sun4v_virt_set_affinity(unsigned int virt_irq,
>> -				    const struct cpumask *mask)
>> +static int sun4v_virt_set_affinity(struct irq_data *data,
>> +				    const struct cpumask *mask, bool force)
> 
> For the benefit of future readers, maybe add a short comment to the commit
> description to explain why these functions are now apparently ignoring the
> 'force' parameter, that is, does it not apply because it's not presently
> implemented, or because it just doesn't make sense.

I don't see any implementation where the "force" parameter is used, and
I see no call of these methods that every sets "force" to anything other
than "false".
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Josip Rodin - Jan. 22, 2011, 1:21 a.m.
On Fri, Jan 21, 2011 at 04:02:52PM -0800, David Miller wrote:
> From: Josip Rodin <joy@entuzijast.net>
> Date: Sat, 22 Jan 2011 00:43:44 +0100
> 
> > On Fri, Jan 21, 2011 at 11:26:56PM +0100, Sam Ravnborg wrote:
> >> Drop all uses of deprecated genirq features
> > 
> >> -static int sun4u_set_affinity(unsigned int virt_irq,
> >> -			       const struct cpumask *mask)
> >> +static int sun4u_set_affinity(struct irq_data *data,
> >> +			       const struct cpumask *mask, bool force)
> >> -static int sun4v_set_affinity(unsigned int virt_irq,
> >> -			       const struct cpumask *mask)
> >> +static int sun4v_set_affinity(struct irq_data *data,
> >> +			       const struct cpumask *mask, bool force)
> >> -static int sun4v_virt_set_affinity(unsigned int virt_irq,
> >> -				    const struct cpumask *mask)
> >> +static int sun4v_virt_set_affinity(struct irq_data *data,
> >> +				    const struct cpumask *mask, bool force)
> > 
> > For the benefit of future readers, maybe add a short comment to the commit
> > description to explain why these functions are now apparently ignoring the
> > 'force' parameter, that is, does it not apply because it's not presently
> > implemented, or because it just doesn't make sense.
> 
> I don't see any implementation where the "force" parameter is used, and
> I see no call of these methods that every sets "force" to anything other
> than "false".

So the conversion to a non-deprecated API is introducing cruft?
That sounds like the change defeats its own point :)
Sam Ravnborg - Jan. 22, 2011, 6:51 a.m.
Hi Thomas.
Josip have a point here... Can you shed some light on this?

On Sat, Jan 22, 2011 at 02:21:24AM +0100, Josip Rodin wrote:
> On Fri, Jan 21, 2011 at 04:02:52PM -0800, David Miller wrote:
> > From: Josip Rodin <joy@entuzijast.net>
> > Date: Sat, 22 Jan 2011 00:43:44 +0100
> > 
> > > On Fri, Jan 21, 2011 at 11:26:56PM +0100, Sam Ravnborg wrote:
> > >> Drop all uses of deprecated genirq features
> > > 
> > >> -static int sun4u_set_affinity(unsigned int virt_irq,
> > >> -			       const struct cpumask *mask)
> > >> +static int sun4u_set_affinity(struct irq_data *data,
> > >> +			       const struct cpumask *mask, bool force)
> > >> -static int sun4v_set_affinity(unsigned int virt_irq,
> > >> -			       const struct cpumask *mask)
> > >> +static int sun4v_set_affinity(struct irq_data *data,
> > >> +			       const struct cpumask *mask, bool force)
> > >> -static int sun4v_virt_set_affinity(unsigned int virt_irq,
> > >> -				    const struct cpumask *mask)
> > >> +static int sun4v_virt_set_affinity(struct irq_data *data,
> > >> +				    const struct cpumask *mask, bool force)
> > > 
> > > For the benefit of future readers, maybe add a short comment to the commit
> > > description to explain why these functions are now apparently ignoring the
> > > 'force' parameter, that is, does it not apply because it's not presently
> > > implemented, or because it just doesn't make sense.
> > 
> > I don't see any implementation where the "force" parameter is used, and
> > I see no call of these methods that every sets "force" to anything other
> > than "false".
> 
> So the conversion to a non-deprecated API is introducing cruft?
> That sounds like the change defeats its own point :)

The commit that introduced the force parameter says:

    The replacement for set_affinity() has an extra argument "bool
    force". The reason for this is to notify the low level code, that the
    move has to be done right away and cannot be delayed until the next
    interrupt happens. That's necessary to handle the irq fixup on cpu
    unplug in the generic code.

The only callers of chip->irq_set_affinity() is in kernel/irq/manage.c

And like David I can only see irq_set_affinity() being called with force=false.
As force tell us to set the affinity and we do so on sparc in all
cases it looks safe to ignore the parameter.
I will update the changelog to include this info in next version,
if Thomas does not have other inputs.

	Sam
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Gleixner - Jan. 22, 2011, 9:27 a.m.
On Sat, 22 Jan 2011, Sam Ravnborg wrote:
> > So the conversion to a non-deprecated API is introducing cruft?
> > That sounds like the change defeats its own point :)
> 
> The commit that introduced the force parameter says:
> 
>     The replacement for set_affinity() has an extra argument "bool
>     force". The reason for this is to notify the low level code, that the
>     move has to be done right away and cannot be delayed until the next
>     interrupt happens. That's necessary to handle the irq fixup on cpu
>     unplug in the generic code.
> 
> The only callers of chip->irq_set_affinity() is in kernel/irq/manage.c
> 
> And like David I can only see irq_set_affinity() being called with force=false.
> As force tell us to set the affinity and we do so on sparc in all
> cases it looks safe to ignore the parameter.
> I will update the changelog to include this info in next version,
> if Thomas does not have other inputs.

The change was done as a preparatory for further cleanups down the
road. I did not want to add irq_set_affinity() now and then change all
those instances again 6 month later down the road.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sam Ravnborg - Jan. 22, 2011, 6:29 p.m.
On Sat, Jan 22, 2011 at 10:27:06AM +0100, Thomas Gleixner wrote:
> On Sat, 22 Jan 2011, Sam Ravnborg wrote:
> > > So the conversion to a non-deprecated API is introducing cruft?
> > > That sounds like the change defeats its own point :)
> > 
> > The commit that introduced the force parameter says:
> > 
> >     The replacement for set_affinity() has an extra argument "bool
> >     force". The reason for this is to notify the low level code, that the
> >     move has to be done right away and cannot be delayed until the next
> >     interrupt happens. That's necessary to handle the irq fixup on cpu
> >     unplug in the generic code.
> > 
> > The only callers of chip->irq_set_affinity() is in kernel/irq/manage.c
> > 
> > And like David I can only see irq_set_affinity() being called with force=false.
> > As force tell us to set the affinity and we do so on sparc in all
> > cases it looks safe to ignore the parameter.
> > I will update the changelog to include this info in next version,
> > if Thomas does not have other inputs.
> 
> The change was done as a preparatory for further cleanups down the
> road. I did not want to add irq_set_affinity() now and then change all
> those instances again 6 month later down the road.

Thanks for info.
It would have been good to have had this info in the changelog.

	Sam
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
index 45d9c87..7990a3c 100644
--- a/arch/sparc/Kconfig
+++ b/arch/sparc/Kconfig
@@ -50,6 +50,8 @@  config SPARC64
 	select RTC_DRV_STARFIRE
 	select HAVE_PERF_EVENTS
 	select PERF_USE_VMALLOC
+	select HAVE_GENERIC_HARDIRQS
+	select GENERIC_HARDIRQS_NO_DEPRECATED
 
 config ARCH_DEFCONFIG
 	string
diff --git a/arch/sparc/kernel/irq_64.c b/arch/sparc/kernel/irq_64.c
index b07fc79..f356e4c 100644
--- a/arch/sparc/kernel/irq_64.c
+++ b/arch/sparc/kernel/irq_64.c
@@ -275,16 +275,15 @@  static int irq_choose_cpu(unsigned int virt_irq, const struct cpumask *affinity)
 	real_hard_smp_processor_id()
 #endif
 
-static void sun4u_irq_enable(unsigned int virt_irq)
+static void sun4u_irq_enable(struct irq_data *data)
 {
-	struct irq_handler_data *handler_data = get_irq_data(virt_irq);
+	struct irq_handler_data *handler_data = data->handler_data;
 
 	if (likely(handler_data)) {
 		unsigned long cpuid, imap, val;
 		unsigned int tid;
 
-		cpuid = irq_choose_cpu(virt_irq,
-				       irq_desc[virt_irq].irq_data.affinity);
+		cpuid = irq_choose_cpu(data->irq, data->affinity);
 		imap = handler_data->imap;
 
 		tid = sun4u_compute_tid(imap, cpuid);
@@ -298,16 +297,16 @@  static void sun4u_irq_enable(unsigned int virt_irq)
 	}
 }
 
-static int sun4u_set_affinity(unsigned int virt_irq,
-			       const struct cpumask *mask)
+static int sun4u_set_affinity(struct irq_data *data,
+			       const struct cpumask *mask, bool force)
 {
-	struct irq_handler_data *handler_data = get_irq_data(virt_irq);
+	struct irq_handler_data *handler_data = data->handler_data;
 
 	if (likely(handler_data)) {
 		unsigned long cpuid, imap, val;
 		unsigned int tid;
 
-		cpuid = irq_choose_cpu(virt_irq, mask);
+		cpuid = irq_choose_cpu(data->irq, mask);
 		imap = handler_data->imap;
 
 		tid = sun4u_compute_tid(imap, cpuid);
@@ -340,14 +339,14 @@  static int sun4u_set_affinity(unsigned int virt_irq,
  * sees that, it also hooks up a default ->shutdown method which
  * invokes ->mask() which we do not want.  See irq_chip_set_defaults().
  */
-static void sun4u_irq_disable(unsigned int virt_irq)
+static void sun4u_irq_disable(struct irq_data *data)
 {
 }
 
-static void sun4u_irq_eoi(unsigned int virt_irq)
+static void sun4u_irq_eoi(struct irq_data *data)
 {
-	struct irq_handler_data *handler_data = get_irq_data(virt_irq);
-	struct irq_desc *desc = irq_desc + virt_irq;
+	struct irq_handler_data *handler_data = data->handler_data;
+	struct irq_desc *desc = irq_desc + data->irq;
 
 	if (unlikely(desc->status & (IRQ_DISABLED|IRQ_INPROGRESS)))
 		return;
@@ -356,11 +355,10 @@  static void sun4u_irq_eoi(unsigned int virt_irq)
 		upa_writeq(ICLR_IDLE, handler_data->iclr);
 }
 
-static void sun4v_irq_enable(unsigned int virt_irq)
+static void sun4v_irq_enable(struct irq_data *data)
 {
-	unsigned int ino = virt_irq_table[virt_irq].dev_ino;
-	unsigned long cpuid = irq_choose_cpu(virt_irq,
-					     irq_desc[virt_irq].irq_data.affinity);
+	unsigned int ino = virt_irq_table[data->irq].dev_ino;
+	unsigned long cpuid = irq_choose_cpu(data->irq, data->affinity);
 	int err;
 
 	err = sun4v_intr_settarget(ino, cpuid);
@@ -377,11 +375,11 @@  static void sun4v_irq_enable(unsigned int virt_irq)
 		       ino, err);
 }
 
-static int sun4v_set_affinity(unsigned int virt_irq,
-			       const struct cpumask *mask)
+static int sun4v_set_affinity(struct irq_data *data,
+			       const struct cpumask *mask, bool force)
 {
-	unsigned int ino = virt_irq_table[virt_irq].dev_ino;
-	unsigned long cpuid = irq_choose_cpu(virt_irq, mask);
+	unsigned int ino = virt_irq_table[data->irq].dev_ino;
+	unsigned long cpuid = irq_choose_cpu(data->irq, mask);
 	int err;
 
 	err = sun4v_intr_settarget(ino, cpuid);
@@ -392,9 +390,9 @@  static int sun4v_set_affinity(unsigned int virt_irq,
 	return 0;
 }
 
-static void sun4v_irq_disable(unsigned int virt_irq)
+static void sun4v_irq_disable(struct irq_data *data)
 {
-	unsigned int ino = virt_irq_table[virt_irq].dev_ino;
+	unsigned int ino = virt_irq_table[data->irq].dev_ino;
 	int err;
 
 	err = sun4v_intr_setenabled(ino, HV_INTR_DISABLED);
@@ -403,10 +401,10 @@  static void sun4v_irq_disable(unsigned int virt_irq)
 		       "err(%d)\n", ino, err);
 }
 
-static void sun4v_irq_eoi(unsigned int virt_irq)
+static void sun4v_irq_eoi(struct irq_data *data)
 {
-	unsigned int ino = virt_irq_table[virt_irq].dev_ino;
-	struct irq_desc *desc = irq_desc + virt_irq;
+	unsigned int ino = virt_irq_table[data->irq].dev_ino;
+	struct irq_desc *desc = irq_desc + data->irq;
 	int err;
 
 	if (unlikely(desc->status & (IRQ_DISABLED|IRQ_INPROGRESS)))
@@ -418,15 +416,15 @@  static void sun4v_irq_eoi(unsigned int virt_irq)
 		       "err(%d)\n", ino, err);
 }
 
-static void sun4v_virq_enable(unsigned int virt_irq)
+static void sun4v_virq_enable(struct irq_data *data)
 {
 	unsigned long cpuid, dev_handle, dev_ino;
 	int err;
 
-	cpuid = irq_choose_cpu(virt_irq, irq_desc[virt_irq].irq_data.affinity);
+	cpuid = irq_choose_cpu(data->irq, data->affinity);
 
-	dev_handle = virt_irq_table[virt_irq].dev_handle;
-	dev_ino = virt_irq_table[virt_irq].dev_ino;
+	dev_handle = virt_irq_table[data->irq].dev_handle;
+	dev_ino = virt_irq_table[data->irq].dev_ino;
 
 	err = sun4v_vintr_set_target(dev_handle, dev_ino, cpuid);
 	if (err != HV_EOK)
@@ -447,16 +445,16 @@  static void sun4v_virq_enable(unsigned int virt_irq)
 		       dev_handle, dev_ino, err);
 }
 
-static int sun4v_virt_set_affinity(unsigned int virt_irq,
-				    const struct cpumask *mask)
+static int sun4v_virt_set_affinity(struct irq_data *data,
+				    const struct cpumask *mask, bool force)
 {
 	unsigned long cpuid, dev_handle, dev_ino;
 	int err;
 
-	cpuid = irq_choose_cpu(virt_irq, mask);
+	cpuid = irq_choose_cpu(data->irq, mask);
 
-	dev_handle = virt_irq_table[virt_irq].dev_handle;
-	dev_ino = virt_irq_table[virt_irq].dev_ino;
+	dev_handle = virt_irq_table[data->irq].dev_handle;
+	dev_ino = virt_irq_table[data->irq].dev_ino;
 
 	err = sun4v_vintr_set_target(dev_handle, dev_ino, cpuid);
 	if (err != HV_EOK)
@@ -467,13 +465,13 @@  static int sun4v_virt_set_affinity(unsigned int virt_irq,
 	return 0;
 }
 
-static void sun4v_virq_disable(unsigned int virt_irq)
+static void sun4v_virq_disable(struct irq_data *data)
 {
 	unsigned long dev_handle, dev_ino;
 	int err;
 
-	dev_handle = virt_irq_table[virt_irq].dev_handle;
-	dev_ino = virt_irq_table[virt_irq].dev_ino;
+	dev_handle = virt_irq_table[data->irq].dev_handle;
+	dev_ino = virt_irq_table[data->irq].dev_ino;
 
 	err = sun4v_vintr_set_valid(dev_handle, dev_ino,
 				    HV_INTR_DISABLED);
@@ -483,17 +481,17 @@  static void sun4v_virq_disable(unsigned int virt_irq)
 		       dev_handle, dev_ino, err);
 }
 
-static void sun4v_virq_eoi(unsigned int virt_irq)
+static void sun4v_virq_eoi(struct irq_data *data)
 {
-	struct irq_desc *desc = irq_desc + virt_irq;
+	struct irq_desc *desc = irq_desc + data->irq;
 	unsigned long dev_handle, dev_ino;
 	int err;
 
 	if (unlikely(desc->status & (IRQ_DISABLED|IRQ_INPROGRESS)))
 		return;
 
-	dev_handle = virt_irq_table[virt_irq].dev_handle;
-	dev_ino = virt_irq_table[virt_irq].dev_ino;
+	dev_handle = virt_irq_table[data->irq].dev_handle;
+	dev_ino = virt_irq_table[data->irq].dev_ino;
 
 	err = sun4v_vintr_set_state(dev_handle, dev_ino,
 				    HV_INTR_STATE_IDLE);
@@ -504,27 +502,27 @@  static void sun4v_virq_eoi(unsigned int virt_irq)
 }
 
 static struct irq_chip sun4u_irq = {
-	.name		= "sun4u",
-	.enable		= sun4u_irq_enable,
-	.disable	= sun4u_irq_disable,
-	.eoi		= sun4u_irq_eoi,
-	.set_affinity	= sun4u_set_affinity,
+	.name			= "sun4u",
+	.irq_enable		= sun4u_irq_enable,
+	.irq_disable		= sun4u_irq_disable,
+	.irq_eoi		= sun4u_irq_eoi,
+	.irq_set_affinity	= sun4u_set_affinity,
 };
 
 static struct irq_chip sun4v_irq = {
-	.name		= "sun4v",
-	.enable		= sun4v_irq_enable,
-	.disable	= sun4v_irq_disable,
-	.eoi		= sun4v_irq_eoi,
-	.set_affinity	= sun4v_set_affinity,
+	.name			= "sun4v",
+	.irq_enable		= sun4v_irq_enable,
+	.irq_disable		= sun4v_irq_disable,
+	.irq_eoi		= sun4v_irq_eoi,
+	.irq_set_affinity	= sun4v_set_affinity,
 };
 
 static struct irq_chip sun4v_virq = {
-	.name		= "vsun4v",
-	.enable		= sun4v_virq_enable,
-	.disable	= sun4v_virq_disable,
-	.eoi		= sun4v_virq_eoi,
-	.set_affinity	= sun4v_virt_set_affinity,
+	.name			= "vsun4v",
+	.irq_enable		= sun4v_virq_enable,
+	.irq_disable		= sun4v_virq_disable,
+	.irq_eoi		= sun4v_virq_eoi,
+	.irq_set_affinity	= sun4v_virt_set_affinity,
 };
 
 static void pre_flow_handler(unsigned int virt_irq,
@@ -798,9 +796,12 @@  void fixup_irqs(void)
 		raw_spin_lock_irqsave(&irq_desc[irq].lock, flags);
 		if (irq_desc[irq].action &&
 		    !(irq_desc[irq].status & IRQ_PER_CPU)) {
-			if (irq_desc[irq].irq_data.chip->set_affinity)
-				irq_desc[irq].irq_data.chip->set_affinity(irq,
-					irq_desc[irq].irq_data.affinity);
+			struct irq_data *data = irq_get_irq_data(irq);
+
+			if (data->chip->irq_set_affinity)
+				data->chip->irq_set_affinity(data,
+				                             data->affinity,
+				                             false);
 		}
 		raw_spin_unlock_irqrestore(&irq_desc[irq].lock, flags);
 	}