Re: [PATCH 2/3] Add disk hotswap support to libata RESEND #5

[Lukasz Kosewski <lkosewsk@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 900 bytes --]

On 9/27/05, Bernhard C. Schrenk <rlx@clemy.org> wrote:
> I tested your previous version of this patch on an AMD64. I found one 64 bit
> bug, crashing the kernel. After fixing it, it worked quite well.
>
> Please change in the file libata-core.c the line
>
> ap->hotplug_timer.data = (unsigned int)ap;
>
> to
>
> ap->hotplug_timer.data = (unsigned long)ap;

Hey Bernhard, Thomas, Jeff,

This bug was the product of my flagrant attempts to defy the C
standard of sizeof(long) == sizeof(void *).  Apparently, I have failed
to slip it into the kernel once again.  Super props go to me since I
even have an Athlon 64 machine and didn't commit it to testing this.

Jeff, please accept this revised patch in place of the original patch
2, the only difference being the change suggested above.

Thanks to Thomas Lustig and Bernhard Schrenk for both pointing this out!

Luke Kosewski

[-- Attachment #2: 02-libata_hotswap_infrastructure-2.6.14-rc2-git5-2.diff --]
[-- Type: application/octet-stream, Size: 11723 bytes --]

27.09.05    Luke Kosewski   <lkosewsk@nit.ca>

	* A patch to add a general-purpose hotswap framework to libata.  This is
	  resend #4 wherein we maintain send #3's API!  Driver writers call
	  ata_hotplug_plug or ata_hotplug_unplug when a plug/unplug event
	  occurs - it takes care of the rest.  The idea is to make the sequence
	  of events as straightforward as possible and not clutter the code
	  with exceptions; as a result, a series of 'hooks' can be provided at
	  points in the code for drivers that need it (such as the Silicon
	  Image drivers which need a reset performed on device removal).  See
	  'hotplug_(un)plug_janitor' for more info.
	* This edition is very streamlined.  I like it.  An area for improvement
	  might facilitate the removal of the ata_scsi_prepare_qc_abort function
	  in favour of changes to ata_qc_complete, but that can be up to debate.
	  Basically this could probably be slightly more readable if more EH
	  code were in place.
	* Small change here to ata_scsi_handle_unplug to set SDEV_CANCEL on a
	  drive before killing outstanding qc's, or else we might see a new qc
	  be queued before the CANCEL is set, which leads to timeouts.
	* This resend changes a pointer cast from (unsigned int) to (unsigned
	  long), thus making it work on non x86 machines.

diff -rpuN linux-2.6.14-rc1/drivers/scsi/libata-core.c linux-2.6.14-rc1-new/drivers/scsi/libata-core.c
--- linux-2.6.14-rc1/drivers/scsi/libata-core.c	2005-09-12 23:12:09.000000000 -0400
+++ linux-2.6.14-rc1-new/drivers/scsi/libata-core.c	2005-09-14 22:13:30.000000000 -0400
@@ -74,6 +74,7 @@ static void __ata_qc_complete(struct ata
 
 static unsigned int ata_unique_id = 1;
 static struct workqueue_struct *ata_wq;
+static struct workqueue_struct *ata_hotplug_wq;
 
 int atapi_enabled = 0;
 module_param(atapi_enabled, int, 0444);
@@ -1147,6 +1148,11 @@ static void ata_dev_identify(struct ata_
 		return;
 	}
 
+	/* Necessary if we had an LBA48 drive in, we pulled it out, and put a
+	 * non-LBA48 drive on the same ap.
+	 */
+	dev->flags &= ~ATA_DFLAG_LBA48;
+
 	if (ap->flags & (ATA_FLAG_SRST | ATA_FLAG_SATA_RESET))
 		using_edd = 0;
 	else
@@ -3692,6 +3698,102 @@ idle_irq:
 	return 0;	/* irq not handled */
 }
 
+void ata_check_kill_qc(struct ata_port *ap)
+{
+	struct ata_queued_cmd *qc = ata_qc_from_tag(ap, ap->active_tag);
+
+	if (unlikely(qc)) {
+		/* This is SO bad.  But we can't just run
+		 * ata_qc_complete without doing this, because
+		 * ata_scsi_qc_complete wants to talk to the device,
+		 * and we can't let it do that since it doesn't exist
+		 * anymore.
+		 */
+		ata_scsi_prepare_qc_abort(qc);
+		ata_qc_complete(qc, ATA_ERR);
+	}
+}
+
+static void ata_hotplug_task(void *_data)
+{
+	struct ata_port *ap = (struct ata_port *)_data;
+	enum hotplug_states hotplug_todo;
+
+	spin_lock(&ap->host_set->lock);
+	hotplug_todo = ap->plug; // Make sure we don't modify while reading!
+	spin_unlock(&ap->host_set->lock);
+
+	if (hotplug_todo == ATA_HOTPLUG_PLUG) {
+		DPRINTK("Got a plug request on port %d\n", ap->id);
+
+		/* This might be necessary if we unplug and plug in a drive
+		 * within ATA_TMOUT_HOTSWAP / HZ seconds... due to the debounce
+		 * timer, one event is generated.  Since the last event was a
+		 * plug, the unplug routine never gets called, so we need to
+		 * clean up the mess first.  If there was never a drive here in
+		 * the first place, this will just do nothing.  Otherwise, it
+		 * basically prevents 'plug' from being called twice with no
+		 * unplug in between.
+		 */
+		ata_scsi_handle_unplug(ap);
+
+		// The following flag is necessary on some Seagate drives.
+		ap->flags |= ATA_FLAG_SATA_RESET; 
+		ap->udma_mask = ap->orig_udma_mask;
+
+		if (!ata_bus_probe(ap))  //Drive found!  Tell the SCSI layer!
+			ata_scsi_handle_plug(ap);	
+	} else if (hotplug_todo == ATA_HOTPLUG_UNPLUG) {
+		DPRINTK("Got an unplug request on port %d\n", ap->id);
+
+		ata_scsi_handle_unplug(ap);
+	} else /* FIXME:  Some kind of error condition? */
+		BUG();
+}
+
+static void ata_hotplug_timer_func(unsigned long _data)
+{
+	struct ata_port *ap = (struct ata_port *)_data;
+
+	queue_work(ata_hotplug_wq, &ap->hotplug_task);
+}
+
+void ata_hotplug_plug(struct ata_port *ap)
+{
+	del_timer(&ap->hotplug_timer);
+
+	if (ap->ops->hotplug_plug_janitor)
+		ap->ops->hotplug_plug_janitor(ap);
+
+	/* This line should be protected by a host_set->lock.  If we follow the
+	 * call chain up from this, it should be called from ata_hotplug_unplug
+	 * or ata_hotplug_plug, which should be called from an interrupt
+	 * handler.  Make sure the call to either of those functions is
+	 * protected.
+	 */
+	ap->plug = ATA_HOTPLUG_PLUG;
+
+	ap->hotplug_timer.expires = jiffies + ATA_TMOUT_HOTSWAP;
+	add_timer(&ap->hotplug_timer);
+}
+
+void ata_hotplug_unplug(struct ata_port *ap)
+{
+	ata_port_disable(ap);  //Gone gone gone!
+	
+	del_timer(&ap->hotplug_timer);
+
+	if (ap->ops->hotplug_unplug_janitor)
+		ap->ops->hotplug_unplug_janitor(ap);
+	
+	/* See comment near similar line in ata_hotplug_plug function.
+	 */
+	ap->plug = ATA_HOTPLUG_UNPLUG;
+
+	ap->hotplug_timer.expires = jiffies + ATA_TMOUT_HOTSWAP;
+	add_timer(&ap->hotplug_timer);
+}
+
 /**
  *	ata_interrupt - Default ATA host interrupt handler
  *	@irq: irq line (unused)
@@ -3925,7 +4027,12 @@ static void ata_host_init(struct ata_por
 	ap->cbl = ATA_CBL_NONE;
 	ap->active_tag = ATA_TAG_POISON;
 	ap->last_ctl = 0xFF;
+	ap->orig_udma_mask = ent->udma_mask;
 
+	ap->hotplug_timer.function = ata_hotplug_timer_func;
+	ap->hotplug_timer.data = (unsigned long)ap;
+	init_timer(&ap->hotplug_timer);
+	INIT_WORK(&ap->hotplug_task, ata_hotplug_task, ap);
 	INIT_WORK(&ap->packet_task, atapi_packet_task, ap);
 	INIT_WORK(&ap->pio_task, ata_pio_task, ap);
 
@@ -4541,6 +4648,11 @@ static int __init ata_init(void)
 	ata_wq = create_workqueue("ata");
 	if (!ata_wq)
 		return -ENOMEM;
+	ata_hotplug_wq = create_workqueue("ata_hotplug");
+	if (!ata_hotplug_wq) {
+		destroy_workqueue(ata_wq);
+		return -ENOMEM;
+	}
 
 	printk(KERN_DEBUG "libata version " DRV_VERSION " loaded.\n");
 	return 0;
@@ -4549,6 +4661,7 @@ static int __init ata_init(void)
 static void __exit ata_exit(void)
 {
 	destroy_workqueue(ata_wq);
+	destroy_workqueue(ata_hotplug_wq);
 }
 
 module_init(ata_init);
@@ -4604,6 +4717,8 @@ EXPORT_SYMBOL_GPL(ata_dev_classify);
 EXPORT_SYMBOL_GPL(ata_dev_id_string);
 EXPORT_SYMBOL_GPL(ata_dev_config);
 EXPORT_SYMBOL_GPL(ata_scsi_simulate);
+EXPORT_SYMBOL_GPL(ata_hotplug_plug);
+EXPORT_SYMBOL_GPL(ata_hotplug_unplug);
 
 #ifdef CONFIG_PCI
 EXPORT_SYMBOL_GPL(pci_test_config_bits);
diff -rpuN linux-2.6.14-rc1/drivers/scsi/libata.h linux-2.6.14-rc1-new/drivers/scsi/libata.h
--- linux-2.6.14-rc1/drivers/scsi/libata.h	2005-09-12 23:12:09.000000000 -0400
+++ linux-2.6.14-rc1-new/drivers/scsi/libata.h	2005-09-14 19:57:53.000000000 -0400
@@ -44,6 +44,7 @@ extern struct ata_queued_cmd *ata_qc_new
 extern void ata_qc_free(struct ata_queued_cmd *qc);
 extern int ata_qc_issue(struct ata_queued_cmd *qc);
 extern int ata_check_atapi_dma(struct ata_queued_cmd *qc);
+extern void ata_check_kill_qc(struct ata_port *ap);
 extern void ata_dev_select(struct ata_port *ap, unsigned int device,
                            unsigned int wait, unsigned int can_sleep);
 extern void ata_tf_to_host_nolock(struct ata_port *ap, struct ata_taskfile *tf);
@@ -79,6 +80,9 @@ extern void ata_scsi_badcmd(struct scsi_
 extern void ata_scsi_rbuf_fill(struct ata_scsi_args *args,
                         unsigned int (*actor) (struct ata_scsi_args *args,
                                            u8 *rbuf, unsigned int buflen));
+extern void ata_scsi_prepare_qc_abort(struct ata_queued_cmd *qc);
+extern void ata_scsi_handle_plug(struct ata_port *ap);
+extern void ata_scsi_handle_unplug(struct ata_port *ap);
 
 static inline void ata_bad_scsiop(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *))
 {
diff -rpuN linux-2.6.14-rc1/drivers/scsi/libata-scsi.c linux-2.6.14-rc1-new/drivers/scsi/libata-scsi.c
--- linux-2.6.14-rc1/drivers/scsi/libata-scsi.c	2005-09-12 23:12:09.000000000 -0400
+++ linux-2.6.14-rc1-new/drivers/scsi/libata-scsi.c	2005-09-14 19:57:53.000000000 -0400
@@ -716,6 +716,53 @@ static int ata_scsi_qc_complete(struct a
 	return 0;
 }
 
+static int ata_scsi_qc_abort(struct ata_queued_cmd *qc, u8 drv_stat)
+{
+	struct scsi_cmnd *cmd = qc->scsicmd;
+
+	cmd->result = SAM_STAT_TASK_ABORTED; //FIXME:  Is this what we want?
+
+	qc->scsidone(cmd);
+
+	return 0;
+}
+
+void ata_scsi_prepare_qc_abort(struct ata_queued_cmd *qc)
+{
+	/* For some reason or another, we can't allow a normal scsi_qc_complete.
+	 * Note that at this point, we must be SURE the command is going to time
+	 * out, or else we might be changing this as it's being called.  Never a
+	 * good thing.
+	 */
+	if (qc->complete_fn == ata_scsi_qc_complete);
+		qc->complete_fn = ata_scsi_qc_abort;
+}
+
+void ata_scsi_handle_plug(struct ata_port *ap)
+{
+	//Currently SATA only
+	scsi_add_device(ap->host, 0, 0, 0);
+}
+
+void ata_scsi_handle_unplug(struct ata_port *ap)
+{
+	//SATA only, no PATA
+	struct scsi_device *scd = scsi_device_lookup(ap->host, 0, 0, 0);
+
+	if (scd)  // Set to cancel state to block further I/O
+		scsi_device_set_state(scd, SDEV_CANCEL);
+	// We might have a pending qc on I/O to a removed device.
+	ata_check_kill_qc(ap);
+	/* scd might not exist; someone did 'echo "scsi remove-single-device
+	 * ... " > /proc/scsi/scsi' or somebody  was turning the key in the
+	 * hotswap bay between on and off really really fast.
+	 */
+	if (scd) {
+		scsi_remove_device(scd);
+		scsi_device_put(scd);
+	}
+}
+
 /**
  *	ata_scsi_translate - Translate then issue SCSI command to ATA device
  *	@ap: ATA port to which the command is addressed
diff -rpuN linux-2.6.14-rc1/include/linux/libata.h linux-2.6.14-rc1-new/include/linux/libata.h
--- linux-2.6.14-rc1/include/linux/libata.h	2005-09-12 23:12:09.000000000 -0400
+++ linux-2.6.14-rc1-new/include/linux/libata.h	2005-09-14 22:12:20.000000000 -0400
@@ -32,6 +32,7 @@
 #include <asm/io.h>
 #include <linux/ata.h>
 #include <linux/workqueue.h>
+#include <linux/timer.h>
 
 /*
  * compile-time options
@@ -130,6 +131,7 @@ enum {
 	ATA_TMOUT_BOOT_QUICK	= 7 * HZ,	/* hueristic */
 	ATA_TMOUT_CDB		= 30 * HZ,
 	ATA_TMOUT_CDB_QUICK	= 5 * HZ,
+	ATA_TMOUT_HOTSWAP	= HZ / 2,	/* FIXME:  Guess value? */
 
 	/* ATA bus states */
 	BUS_UNKNOWN		= 0,
@@ -167,6 +169,11 @@ enum pio_task_states {
 	PIO_ST_ERR,
 };
 
+enum hotplug_states {
+	ATA_HOTPLUG_PLUG,
+	ATA_HOTPLUG_UNPLUG,
+};
+
 /* forward declarations */
 struct scsi_device;
 struct ata_port_operations;
@@ -316,6 +323,8 @@ struct ata_port {
 	struct ata_host_stats	stats;
 	struct ata_host_set	*host_set;
 
+	struct timer_list	hotplug_timer;
+	struct work_struct	hotplug_task;
 	struct work_struct	packet_task;
 
 	struct work_struct	pio_task;
@@ -323,6 +332,9 @@ struct ata_port {
 	unsigned long		pio_task_timeout;
 
 	void			*private_data;
+
+	unsigned int		orig_udma_mask;
+	enum hotplug_states	plug;
 };
 
 struct ata_port_operations {
@@ -369,6 +381,8 @@ struct ata_port_operations {
 
 	void (*bmdma_stop) (struct ata_queued_cmd *qc);
 	u8   (*bmdma_status) (struct ata_port *ap);
+	void (*hotplug_plug_janitor) (struct ata_port *ap);
+	void (*hotplug_unplug_janitor) (struct ata_port *ap);
 };
 
 struct ata_port_info {
@@ -399,6 +413,8 @@ extern int ata_scsi_queuecmd(struct scsi
 extern int ata_scsi_error(struct Scsi_Host *host);
 extern int ata_scsi_release(struct Scsi_Host *host);
 extern unsigned int ata_host_intr(struct ata_port *ap, struct ata_queued_cmd *qc);
+extern void ata_hotplug_plug(struct ata_port *ap);
+extern void ata_hotplug_unplug(struct ata_port *ap);
 /*
  * Default driver ops implementations
  */