| Message ID | 1295648816-29516-5-git-send-email-sam@ravnborg.org |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
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 :)
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
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 :)
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
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
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
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); }
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(-)