LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/4] avoid disabling interrupts where it is not required
@ 2018-05-04 14:24 Sebastian Andrzej Siewior
  2018-05-04 14:24 ` [PATCH 1/4] alim15x3: move irq-restore before pci_dev_put() Sebastian Andrzej Siewior
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-05-04 14:24 UTC (permalink / raw)
  To: linux-ide; +Cc: linux-kernel, tglx, David S. Miller

This series avoids/limits disabling of interrupts in drivers/ide.
This is from the -RT queue where it was reported initially (ata was not
widespread back then) and it hurts -RT.

While looking at alim15x3, there are some drivers which exist in both
worlds (alim15x3 included was far as I can tell). Is there a reason why
we still keep two drivers around for the same hardware? I know on mips
bigsur and swarm select IDE in their defconfig. I fixed (ages ago) ide
for swarm while I was using ata myself. So *I* think this just a relict.
If Ralf's bigsur is still alive, it is using SATA and not the onboard
IDE.

Sebastian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/4] alim15x3: move irq-restore before pci_dev_put()
  2018-05-04 14:24 [PATCH 0/4] avoid disabling interrupts where it is not required Sebastian Andrzej Siewior
@ 2018-05-04 14:24 ` Sebastian Andrzej Siewior
  2018-05-04 17:37   ` David Miller
  2018-05-04 14:24 ` [PATCH 2/4] ide: Handle irq disabling consistently Sebastian Andrzej Siewior
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-05-04 14:24 UTC (permalink / raw)
  To: linux-ide; +Cc: linux-kernel, tglx, David S. Miller, Sebastian Andrzej Siewior

init_chipset_ali15x3() initializes the chipset during init with disabled
interrupts. There is no need to keep the interrupts disabled during
pci_dev_put().
Move the irq-restore before pci_dev_put() is invoked.

Side note: The same init is performed in
drivers/ata/pata_ali.c::ali_init_chipset() without disabled interrupts.
It looks that the same hardware is supported in the ATA land. Would it
make sense to remove this driver since it is supported in the other
subsystem?

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/ide/alim15x3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ide/alim15x3.c b/drivers/ide/alim15x3.c
index 36f76e28a0bf..3265970aee34 100644
--- a/drivers/ide/alim15x3.c
+++ b/drivers/ide/alim15x3.c
@@ -323,9 +323,9 @@ static int init_chipset_ali15x3(struct pci_dev *dev)
 
 		pci_write_config_byte(dev, 0x53, tmpbyte);
 	}
+	local_irq_restore(flags);
 	pci_dev_put(north);
 	pci_dev_put(isa_dev);
-	local_irq_restore(flags);
 	return 0;
 }
 
-- 
2.17.0

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/4] ide: Handle irq disabling consistently
  2018-05-04 14:24 [PATCH 0/4] avoid disabling interrupts where it is not required Sebastian Andrzej Siewior
  2018-05-04 14:24 ` [PATCH 1/4] alim15x3: move irq-restore before pci_dev_put() Sebastian Andrzej Siewior
@ 2018-05-04 14:24 ` Sebastian Andrzej Siewior
  2018-05-04 17:38   ` David Miller
  2018-05-04 14:24 ` [PATCH 3/4] ide: don't disable interrupts during kmap_atomic() Sebastian Andrzej Siewior
  2018-05-04 14:24 ` [PATCH 4/4] ide: don't enable/disable interrupts in force threaded-IRQ mode Sebastian Andrzej Siewior
  3 siblings, 1 reply; 9+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-05-04 14:24 UTC (permalink / raw)
  To: linux-ide; +Cc: linux-kernel, tglx, David S. Miller, Sebastian Andrzej Siewior

ide_timer_expiry() disables interrupt at function entry when acquiring
hwif->lock. Before disabling the device interrupt it unlocks hwif->lock,
but interrupts stay disabled. After the call to disable_irq() interrupts
are disabled again, which is a pointless exercise.

After the device irq handler has been invoked with interrupts disabled,
hwif->lock is acquired again with spin_lock_irq() because the device irq
handler might have reenabled interrupts. This is not documented and
confusing for the casual reader.

Remove the redundant local_irq_disable() and add a comment which explains
why hwif->lock has to be reacquired with spin_lock_irq().

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/ide/ide-io.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index 6f25da56a169..a444bad7a2aa 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -659,8 +659,7 @@ void ide_timer_expiry (struct timer_list *t)
 		spin_unlock(&hwif->lock);
 		/* disable_irq_nosync ?? */
 		disable_irq(hwif->irq);
-		/* local CPU only, as if we were handling an interrupt */
-		local_irq_disable();
+
 		if (hwif->polling) {
 			startstop = handler(drive);
 		} else if (drive_is_ready(drive)) {
@@ -679,6 +678,7 @@ void ide_timer_expiry (struct timer_list *t)
 				startstop = ide_error(drive, "irq timeout",
 					hwif->tp_ops->read_status(hwif));
 		}
+		/* Disable interrupts again, `handler' might have enabled it */
 		spin_lock_irq(&hwif->lock);
 		enable_irq(hwif->irq);
 		if (startstop == ide_stopped && hwif->polling == 0) {
-- 
2.17.0

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 3/4] ide: don't disable interrupts during kmap_atomic()
  2018-05-04 14:24 [PATCH 0/4] avoid disabling interrupts where it is not required Sebastian Andrzej Siewior
  2018-05-04 14:24 ` [PATCH 1/4] alim15x3: move irq-restore before pci_dev_put() Sebastian Andrzej Siewior
  2018-05-04 14:24 ` [PATCH 2/4] ide: Handle irq disabling consistently Sebastian Andrzej Siewior
@ 2018-05-04 14:24 ` Sebastian Andrzej Siewior
  2018-05-04 17:38   ` David Miller
  2018-05-04 14:24 ` [PATCH 4/4] ide: don't enable/disable interrupts in force threaded-IRQ mode Sebastian Andrzej Siewior
  3 siblings, 1 reply; 9+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-05-04 14:24 UTC (permalink / raw)
  To: linux-ide; +Cc: linux-kernel, tglx, David S. Miller, Sebastian Andrzej Siewior

ide_pio_bytes() disables interrupts around kmap_atomic(). This is a
leftover from the old kmap_atomic() implementation which relied on fixed
mapping slots, so the caller had to make sure that the same slot could not
be reused from an interrupting context.

kmap_atomic() was changed to dynamic slots long ago and commit 1ec9c5ddc17a
("include/linux/highmem.h: remove the second argument of k[un]map_atomic()")
removed the slot assignements, but the callers were not checked for now
redundant interrupt disabling.

Remove the conditional interrupt disable.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/ide/ide-taskfile.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/ide/ide-taskfile.c b/drivers/ide/ide-taskfile.c
index abe0822dd429..49f9b4739779 100644
--- a/drivers/ide/ide-taskfile.c
+++ b/drivers/ide/ide-taskfile.c
@@ -237,7 +237,6 @@ void ide_pio_bytes(ide_drive_t *drive, struct ide_cmd *cmd,
 
 	while (len) {
 		unsigned nr_bytes = min(len, cursg->length - cmd->cursg_ofs);
-		int page_is_high;
 
 		page = sg_page(cursg);
 		offset = cursg->offset + cmd->cursg_ofs;
@@ -248,10 +247,6 @@ void ide_pio_bytes(ide_drive_t *drive, struct ide_cmd *cmd,
 
 		nr_bytes = min_t(unsigned, nr_bytes, (PAGE_SIZE - offset));
 
-		page_is_high = PageHighMem(page);
-		if (page_is_high)
-			local_irq_save(flags);
-
 		buf = kmap_atomic(page) + offset;
 
 		cmd->nleft -= nr_bytes;
@@ -270,9 +265,6 @@ void ide_pio_bytes(ide_drive_t *drive, struct ide_cmd *cmd,
 
 		kunmap_atomic(buf);
 
-		if (page_is_high)
-			local_irq_restore(flags);
-
 		len -= nr_bytes;
 	}
 }
-- 
2.17.0

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 4/4] ide: don't enable/disable interrupts in force threaded-IRQ mode
  2018-05-04 14:24 [PATCH 0/4] avoid disabling interrupts where it is not required Sebastian Andrzej Siewior
                   ` (2 preceding siblings ...)
  2018-05-04 14:24 ` [PATCH 3/4] ide: don't disable interrupts during kmap_atomic() Sebastian Andrzej Siewior
@ 2018-05-04 14:24 ` Sebastian Andrzej Siewior
  2018-05-04 17:39   ` David Miller
  3 siblings, 1 reply; 9+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-05-04 14:24 UTC (permalink / raw)
  To: linux-ide; +Cc: linux-kernel, tglx, David S. Miller, Sebastian Andrzej Siewior

The interrupts are enabled/disabled so the interrupt handler can run
with enabled interrupts while serving the interrupt and not lose other
interrupts especially the timer tick.
If the system runs with force-threaded interrupts then there is no need
to enable the interrupts.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/ide/ide-iops.c     | 13 +++++++++----
 drivers/ide/ide-taskfile.c |  2 +-
 kernel/irq/manage.c        |  1 +
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/ide/ide-iops.c b/drivers/ide/ide-iops.c
index 210a0887dd29..d55e9ebd5628 100644
--- a/drivers/ide/ide-iops.c
+++ b/drivers/ide/ide-iops.c
@@ -108,6 +108,7 @@ int __ide_wait_stat(ide_drive_t *drive, u8 good, u8 bad,
 	ide_hwif_t *hwif = drive->hwif;
 	const struct ide_tp_ops *tp_ops = hwif->tp_ops;
 	unsigned long flags;
+	bool irqs_threaded = force_irqthreads;
 	int i;
 	u8 stat;
 
@@ -115,8 +116,10 @@ int __ide_wait_stat(ide_drive_t *drive, u8 good, u8 bad,
 	stat = tp_ops->read_status(hwif);
 
 	if (stat & ATA_BUSY) {
-		local_save_flags(flags);
-		local_irq_enable_in_hardirq();
+		if (!irqs_threaded) {
+			local_save_flags(flags);
+			local_irq_enable_in_hardirq();
+		}
 		timeout += jiffies;
 		while ((stat = tp_ops->read_status(hwif)) & ATA_BUSY) {
 			if (time_after(jiffies, timeout)) {
@@ -129,12 +132,14 @@ int __ide_wait_stat(ide_drive_t *drive, u8 good, u8 bad,
 				if ((stat & ATA_BUSY) == 0)
 					break;
 
-				local_irq_restore(flags);
+				if (!irqs_threaded)
+					local_irq_restore(flags);
 				*rstat = stat;
 				return -EBUSY;
 			}
 		}
-		local_irq_restore(flags);
+		if (!irqs_threaded)
+			local_irq_restore(flags);
 	}
 	/*
 	 * Allow status to settle, then read it again.
diff --git a/drivers/ide/ide-taskfile.c b/drivers/ide/ide-taskfile.c
index 49f9b4739779..d42feeaebd36 100644
--- a/drivers/ide/ide-taskfile.c
+++ b/drivers/ide/ide-taskfile.c
@@ -405,7 +405,7 @@ static ide_startstop_t pre_task_out_intr(ide_drive_t *drive,
 		return startstop;
 	}
 
-	if ((drive->dev_flags & IDE_DFLAG_UNMASK) == 0)
+	if (!force_irqthreads && (drive->dev_flags & IDE_DFLAG_UNMASK) == 0)
 		local_irq_disable();
 
 	ide_set_handler(drive, &task_pio_intr, WAIT_WORSTCASE);
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index e3336d904f64..4c2ef8084e32 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -24,6 +24,7 @@
 
 #ifdef CONFIG_IRQ_FORCED_THREADING
 __read_mostly bool force_irqthreads;
+EXPORT_SYMBOL_GPL(force_irqthreads);
 
 static int __init setup_forced_irqthreads(char *arg)
 {
-- 
2.17.0

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/4] alim15x3: move irq-restore before pci_dev_put()
  2018-05-04 14:24 ` [PATCH 1/4] alim15x3: move irq-restore before pci_dev_put() Sebastian Andrzej Siewior
@ 2018-05-04 17:37   ` David Miller
  0 siblings, 0 replies; 9+ messages in thread
From: David Miller @ 2018-05-04 17:37 UTC (permalink / raw)
  To: bigeasy; +Cc: linux-ide, linux-kernel, tglx

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Date: Fri,  4 May 2018 16:24:43 +0200

> init_chipset_ali15x3() initializes the chipset during init with disabled
> interrupts. There is no need to keep the interrupts disabled during
> pci_dev_put().
> Move the irq-restore before pci_dev_put() is invoked.
> 
> Side note: The same init is performed in
> drivers/ata/pata_ali.c::ali_init_chipset() without disabled interrupts.
> It looks that the same hardware is supported in the ATA land. Would it
> make sense to remove this driver since it is supported in the other
> subsystem?
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Acked-by: David S. Miller <davem@davemloft.net>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/4] ide: Handle irq disabling consistently
  2018-05-04 14:24 ` [PATCH 2/4] ide: Handle irq disabling consistently Sebastian Andrzej Siewior
@ 2018-05-04 17:38   ` David Miller
  0 siblings, 0 replies; 9+ messages in thread
From: David Miller @ 2018-05-04 17:38 UTC (permalink / raw)
  To: bigeasy; +Cc: linux-ide, linux-kernel, tglx

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Date: Fri,  4 May 2018 16:24:44 +0200

> ide_timer_expiry() disables interrupt at function entry when acquiring
> hwif->lock. Before disabling the device interrupt it unlocks hwif->lock,
> but interrupts stay disabled. After the call to disable_irq() interrupts
> are disabled again, which is a pointless exercise.
> 
> After the device irq handler has been invoked with interrupts disabled,
> hwif->lock is acquired again with spin_lock_irq() because the device irq
> handler might have reenabled interrupts. This is not documented and
> confusing for the casual reader.
> 
> Remove the redundant local_irq_disable() and add a comment which explains
> why hwif->lock has to be reacquired with spin_lock_irq().
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Acked-by: David S. Miller <davem@davemloft.net>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/4] ide: don't disable interrupts during kmap_atomic()
  2018-05-04 14:24 ` [PATCH 3/4] ide: don't disable interrupts during kmap_atomic() Sebastian Andrzej Siewior
@ 2018-05-04 17:38   ` David Miller
  0 siblings, 0 replies; 9+ messages in thread
From: David Miller @ 2018-05-04 17:38 UTC (permalink / raw)
  To: bigeasy; +Cc: linux-ide, linux-kernel, tglx

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Date: Fri,  4 May 2018 16:24:45 +0200

> ide_pio_bytes() disables interrupts around kmap_atomic(). This is a
> leftover from the old kmap_atomic() implementation which relied on fixed
> mapping slots, so the caller had to make sure that the same slot could not
> be reused from an interrupting context.
> 
> kmap_atomic() was changed to dynamic slots long ago and commit 1ec9c5ddc17a
> ("include/linux/highmem.h: remove the second argument of k[un]map_atomic()")
> removed the slot assignements, but the callers were not checked for now
> redundant interrupt disabling.
> 
> Remove the conditional interrupt disable.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Acked-by: David S. Miller <davem@davemloft.net>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 4/4] ide: don't enable/disable interrupts in force threaded-IRQ mode
  2018-05-04 14:24 ` [PATCH 4/4] ide: don't enable/disable interrupts in force threaded-IRQ mode Sebastian Andrzej Siewior
@ 2018-05-04 17:39   ` David Miller
  0 siblings, 0 replies; 9+ messages in thread
From: David Miller @ 2018-05-04 17:39 UTC (permalink / raw)
  To: bigeasy; +Cc: linux-ide, linux-kernel, tglx

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Date: Fri,  4 May 2018 16:24:46 +0200

> The interrupts are enabled/disabled so the interrupt handler can run
> with enabled interrupts while serving the interrupt and not lose other
> interrupts especially the timer tick.
> If the system runs with force-threaded interrupts then there is no need
> to enable the interrupts.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Acked-by: David S. Miller <davem@davemloft.net>

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-05-04 17:39 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-04 14:24 [PATCH 0/4] avoid disabling interrupts where it is not required Sebastian Andrzej Siewior
2018-05-04 14:24 ` [PATCH 1/4] alim15x3: move irq-restore before pci_dev_put() Sebastian Andrzej Siewior
2018-05-04 17:37   ` David Miller
2018-05-04 14:24 ` [PATCH 2/4] ide: Handle irq disabling consistently Sebastian Andrzej Siewior
2018-05-04 17:38   ` David Miller
2018-05-04 14:24 ` [PATCH 3/4] ide: don't disable interrupts during kmap_atomic() Sebastian Andrzej Siewior
2018-05-04 17:38   ` David Miller
2018-05-04 14:24 ` [PATCH 4/4] ide: don't enable/disable interrupts in force threaded-IRQ mode Sebastian Andrzej Siewior
2018-05-04 17:39   ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).