Re: [PATCH 10/15] MIPS: Add code for new system 'paravirt'.

[Andreas Herrmann <andreas.herrmann@caviumnetworks.com>]

On Wed, May 21, 2014 at 09:31:02AM -0700, David Daney wrote:
> On 05/21/2014 06:39 AM, James Hogan wrote:
> [...]
> >>diff --git a/arch/mips/paravirt/paravirt-irq.c b/arch/mips/paravirt/paravirt-irq.c
> >>new file mode 100644
> >>index 0000000..e1603dd
> >>--- /dev/null
> >>+++ b/arch/mips/paravirt/paravirt-irq.c
> [...]
> >
> >>+static void irq_core_set_enable_local(void *arg)
> >>+{
> >>+	struct irq_data *data = arg;
> >>+	struct core_chip_data *cd = irq_data_get_irq_chip_data(data);
> >>+	unsigned int mask = 0x100 << cd->bit;
> >>+
> >>+	/*
> >>+	 * Interrupts are already disabled, so these are atomic.
> >
> >Really? Even when called directly from irq_core_bus_sync_unlock with
> >only a single core online?
> >
> 
> Yes, but...
> 
> 
> >>+	 */
> >>+	if (cd->desired_en)
> >>+		set_c0_status(mask);
> >>+	else
> >>+		clear_c0_status(mask);
> >>+
> >>+}
> >>+
> >>+static void irq_core_disable(struct irq_data *data)
> >>+{
> >>+	struct core_chip_data *cd = irq_data_get_irq_chip_data(data);
> >>+	cd->desired_en = false;
> >>+}
> >>+
> >>+static void irq_core_enable(struct irq_data *data)
> >>+{
> >>+	struct core_chip_data *cd = irq_data_get_irq_chip_data(data);
> >>+	cd->desired_en = true;
> >>+}
> >>+
> >>+static void irq_core_bus_lock(struct irq_data *data)
> >>+{
> >>+	struct core_chip_data *cd = irq_data_get_irq_chip_data(data);
> >>+
> >>+	mutex_lock(&cd->core_irq_mutex);
> >>+}
> >>+
> >>+static void irq_core_bus_sync_unlock(struct irq_data *data)
> >>+{
> >>+	struct core_chip_data *cd = irq_data_get_irq_chip_data(data);
> >>+
> >>+	if (cd->desired_en != cd->current_en) {
> >>+		/*
> >>+		 * Can be called in early init when on_each_cpu() will
> >>+		 * unconditionally enable irqs, so handle the case
> >>+		 * where only a single CPU is online specially, and
> >>+		 * directly call.
> >>+		 */
> >>+		if (num_online_cpus() == 1)
> >>+			irq_core_set_enable_local(data);
> >>+		else
> >>+			on_each_cpu(irq_core_set_enable_local, data, 1);
> >>+
> 
> 
> ...  This code is not correct.  It was initially done as a
> workaround for the issues fixed in commit 202da4005.
> 
> Now that on_each_cpu() is less buggy, we can unconditionally use it
> and the assertion above about "Interrupts are already disabled" will
> be true.


I'll adapt this in the next version of the patch.


Andreas