libata & scsi rescan.

libata & scsi rescan.

[Andy Warner]

I've started looking at scsi device add/remove/rescan
and libata. Anyone have any groundrules or observations
that might save me wasting a bunch of cycles doing
something daft/unacceptable ?
-- 
andyw@pobox.com

Andy Warner		Voice: (612) 801-8549	Fax: (208) 575-5634

Re: libata & scsi rescan.

[Jeff Garzik]

Andy Warner wrote:
> I've started looking at scsi device add/remove/rescan
> and libata. Anyone have any groundrules or observations
> that might save me wasting a bunch of cycles doing
> something daft/unacceptable ?

I don't see why rescan is needed, since SATA hardware supports hotplug.

	Jeff

Re: libata & scsi rescan.

[Andy Warner]

Jeff Garzik wrote:
> [...]
> I don't see why rescan is needed, since SATA hardware supports hotplug.

OK - but at present, I can remove a quiescent drive
and /proc/scsi/scsi still serves up the cached info
about the drive. It seems there are at least a
couple of dots left that I could/should join.

Since they appear as scsi devices, I sort of expected
to be able to implement "scsi add-single-device n" and
"scsi remove-single-device n", but for all I know this
may be out of fashion or otherwise useless these days.

I can see how I may require HBA-driver-level
hooks to respond to phys going on/off-line, or
some such mechanism.
-- 
andyw@pobox.com

Andy Warner		Voice: (612) 801-8549	Fax: (208) 575-5634

Re: libata & scsi rescan.

[Jeff Garzik]

Andy Warner wrote:
> Jeff Garzik wrote:
> 
>>[...]
>>I don't see why rescan is needed, since SATA hardware supports hotplug.
> 
> 
> OK - but at present, I can remove a quiescent drive
> and /proc/scsi/scsi still serves up the cached info
> about the drive.

That's because libata doesn't call the proper hot-unplug hooks.  Call 
those hooks, and the problem goes away.

	Jeff

Re: libata & scsi rescan.

[Andy Warner]

Jeff Garzik wrote:
> [...]
> That's because libata doesn't call the proper hot-unplug hooks.  Call 
> those hooks, and the problem goes away.

Suggested reading, anyone ?
-- 
andyw@pobox.com

Andy Warner		Voice: (612) 801-8549	Fax: (208) 575-5634

Re: libata & scsi rescan.

[Jeff Garzik]

Andy Warner wrote:
> Jeff Garzik wrote:
> 
>>[...]
>>That's because libata doesn't call the proper hot-unplug hooks.  Call 
>>those hooks, and the problem goes away.
> 
> 
> Suggested reading, anyone ?


Documentation/scsi/scsi_mid_low_api.txt for 2.6.x.  "good luck" for 
2.4.x, and libata needs to work on 2.4.x as well (at least for the time 
being)

	Jeff

Re: libata & scsi rescan.

[Andy Warner]

Jeff Garzik wrote:
> [libata & hot-unplug...]
> Documentation/scsi/scsi_mid_low_api.txt for 2.6.x.  "good luck" for 
> 2.4.x, and libata needs to work on 2.4.x as well (at least for the time 
> being)

I've got phy-level hooks into interrupt handlers, all working
nicely. I can't find any good examples of scsi_add/remove_device
usage, and they do not like being called from an interrupt
context. Can anyone lend some advice/examples of usage ?
I've searched for guidelines on when/how to call these,
above and beyond scsi_mid_low_api.txt, and come up empty
handed.

If they need to be called from a helper-thread, I can do
that - just looking for ground rules.
-- 
andyw@pobox.com

Andy Warner		Voice: (612) 801-8549	Fax: (208) 575-5634

Re: libata & scsi rescan.

[Jeff Garzik]

Andy Warner wrote:
> Jeff Garzik wrote:
> 
>>[libata & hot-unplug...]
>>Documentation/scsi/scsi_mid_low_api.txt for 2.6.x.  "good luck" for 
>>2.4.x, and libata needs to work on 2.4.x as well (at least for the time 
>>being)
> 
> 
> I've got phy-level hooks into interrupt handlers, all working
> nicely. I can't find any good examples of scsi_add/remove_device
> usage, and they do not like being called from an interrupt
> context. Can anyone lend some advice/examples of usage ?
> I've searched for guidelines on when/how to call these,
> above and beyond scsi_mid_low_api.txt, and come up empty
> handed.
> 
> If they need to be called from a helper-thread, I can do
> that - just looking for ground rules.

Correct, they must be called from process context.

	Jeff

Re: libata & scsi rescan.

[Andy Warner]

Jeff Garzik wrote:
> [...]
> > If they need to be called from a helper-thread, I can do
> > that - just looking for ground rules.
> 
> Correct, they must be called from process context.

Thanks for the confirmation - I can take it from there..
Does anyone have any opinions/advice about reusing existing
helper tasks ?

Also - I need help from someone with access to Promise
chip manuals. I have Silicon Image docs, and am able
to enable interrupts on PHY status changes by setting
the correct bit in the SEIN register(s). This patchette
shows what I do:

===== drivers/scsi/sata_sil.c 1.35 vs edited =====
--- 1.35/drivers/scsi/sata_sil.c	2004-09-16 01:45:15 -05:00
+++ edited/drivers/scsi/sata_sil.c	2004-10-06 21:20:42 -05:00
@@ -415,9 +415,10 @@
 	}
 
 	/* mask all SATA phy-related interrupts */
-	/* TODO: unmask bit 6 (SError N bit) for hotplug */
-	for (i = 0; i < probe_ent->n_ports; i++)
-		writel(0, mmio_base + sil_port[i].sien);
+	/* except (SError N bit) for hotplug */
+	for (i = 0; i < probe_ent->n_ports; i++) {
+		writel(SCR_DIAG_N, mmio_base + sil_port[i].sien);
+	}
 
 	pci_set_master(pdev);
 
[NOTE: SCR_DIAG_N is (1 << 16)]

If anyone can provide me with either the promise Tx2/4 docs,
or a corresponding patch for sata_promise.c, I'd be most
grateful.

The handling of the SATA error/status registers is all
standardised, and taken care of by me. I just need to
enable SATA PHY interrupts from the promise chips.

I can only test with SiI/Promise right now, I have Marvell
and 3124 hardware, but libata drivers for them are not
available yet (*please* correct me if I'm wrong.)
For anyone with access to the right docs, adding matching
functionality to sata_vsc.c etc will be trivial once I
get past calling scsi_add/remove_device() correctly.
-- 
andyw@pobox.com

Andy Warner		Voice: (612) 801-8549	Fax: (208) 575-5634

Re: libata & scsi rescan.

[Jeff Garzik]

Well, the PDC2037x docs are under NDA, but the following information is 
obviously required to implement hotplug, and therefore will obviously be 
public once implemented:

bits of Promise hotplug register, PDC_SATA_PLUG_CSR:

23:20	mask unplug irq (r/w)
19:16	mask plug irq (r/w)
11:8	SATAn is well connected (r/o)
7:4	SATAn is plugged again (rwc)
3:0	SATAn is disconnected (rwc)

Re: libata & scsi rescan.

[Andy Warner]

Jeff Garzik wrote:
> 
> Well, the PDC2037x docs are under NDA, but the following information is 
> obviously required to implement hotplug, and therefore will obviously be 
> public once implemented:
> 
> bits of Promise hotplug register, PDC_SATA_PLUG_CSR:
> 
> 23:20	mask unplug irq (r/w)
> 19:16	mask plug irq (r/w)
> 11:8	SATAn is well connected (r/o)
> 7:4	SATAn is plugged again (rwc)
> 3:0	SATAn is disconnected (rwc)

There's a fine line between being terse and being cryptic,
as "man man" used to say .. :)

Is it also true that the standard SATA control, error and
status registers exist and function pretty much as the
spec says (at least for (1 << 16) in error and the
IPM & DET fields in Status) ?
-- 
andyw@pobox.com

Andy Warner		Voice: (612) 801-8549	Fax: (208) 575-5634

Re: libata & scsi rescan.

[Jeff Garzik]

Andy Warner wrote:
> Is it also true that the standard SATA control, error and
> status registers exist and function pretty much as the
> spec says (at least for (1 << 16) in error and the
> IPM & DET fields in Status) ?

SStatus and SControl yes.  The SError register seems to be for error 
conditions only; no mention of PhyRdy being directly manipulated.

	Jeff

Re: libata & scsi rescan.

[Jeff Garzik]

BTW, the SiI folks recommend a debounce timer.

Re: libata & scsi rescan.

[Andy Warner]

Jeff Garzik wrote:
> BTW, the SiI folks recommend a debounce timer.

OK, got that. No problem, queue_delayed_work() and some
state handles that. Now I've got the following issue,
calling scsi_remove_device() seems to try and flush
the disk - not a very productive operation:

<drive removed>
ata1: drive not present
Synchronizing SCSI cache for disk sdb: 
ATA: abnormal status 0x7F on port 0xF8A3EC87
ATA: abnormal status 0x7F on port 0xF8A3EC87
ATA: abnormal status 0x7F on port 0xF8A3EC87
ata1: command 0xea timeout, stat 0x50 host_stat 0x0

<drive re-added>
ata1: drive present
  Vendor: ATA       Model: ST3160023AS       Rev: 3.17
  Type:   Direct-Access                      ANSI SCSI revision: 05
SCSI device sdb: 312581808 512-byte hdwr sectors (160042 MB)
SCSI device sdb: drive cache: write back
ATA: abnormal status 0xFF on port 0xF8A3EC87
ATA: abnormal status 0xFF on port 0xF8A3EC87
ata1: command 0x25 timeout, stat 0x50 host_stat 0x1
 unknown partition table
Attached scsi disk sdb at scsi14, channel 0, id 0, lun 0
Attached scsi generic sg2 at scsi14, channel 0, id 0, lun 0,  type 0


I'm not too concerned about the errors when the drive
appears yet, but trying to access the disk that just
vanished is never going to work well. Am I missing
something ?

Here's the code (ignore hardcoded values and sloppy error
checking for now):

if (ap->hotplug_task_state == HOTPLUG_ST_PENDING) {
	struct scsi_device *sdev ;
	/*
	 * Need to read the status register to determine
	 * the current drive state.
	 */
	status = ap->ops->scr_read(ap, SCR_STATUS) ;
	if ((status & (SCR_STAT_IPM_MASK | SCR_STAT_DET_MASK)) ==
			(SCR_STAT_IPM_ACTIVE | SCR_STAT_DET_GOOD)) {
		/*
		 * Drive present.
		 */
		printk("ata%u: drive present\n", ap->id) ;

		/*
		 * Add it.
		 */
		sdev = scsi_add_device(ap->host, 0, 0, 0) ;
	} else {
		/*
		 * Drive not present.
		 */
		printk("ata%u: drive not present\n", ap->id) ;

		/*
		 * Remove it.
		 */
		sdev = scsi_device_lookup(ap->host, 0, 0, 0);
		if (sdev) {
			scsi_remove_device(sdev);
			scsi_device_put(sdev) ;
		}
	}
	ap->hotplug_task_state = HOTPLUG_ST_IDLE ;
}

-- 
andyw@pobox.com

Andy Warner		Voice: (612) 801-8549	Fax: (208) 575-5634

Re: libata & scsi rescan.

[Jeff Garzik]

On Thu, Oct 07, 2004 at 05:14:57PM -0500, Andy Warner wrote:
> Jeff Garzik wrote:
> > BTW, the SiI folks recommend a debounce timer.
> 
> OK, got that. No problem, queue_delayed_work() and some
> state handles that. Now I've got the following issue,
> calling scsi_remove_device() seems to try and flush
> the disk - not a very productive operation:
> 
> <drive removed>
> ata1: drive not present
> Synchronizing SCSI cache for disk sdb: 
> ATA: abnormal status 0x7F on port 0xF8A3EC87
> ATA: abnormal status 0x7F on port 0xF8A3EC87
> ATA: abnormal status 0x7F on port 0xF8A3EC87
> ata1: command 0xea timeout, stat 0x50 host_stat 0x0
> 
> <drive re-added>
> ata1: drive present
>   Vendor: ATA       Model: ST3160023AS       Rev: 3.17
>   Type:   Direct-Access                      ANSI SCSI revision: 05
> SCSI device sdb: 312581808 512-byte hdwr sectors (160042 MB)
> SCSI device sdb: drive cache: write back
> ATA: abnormal status 0xFF on port 0xF8A3EC87
> ATA: abnormal status 0xFF on port 0xF8A3EC87
> ata1: command 0x25 timeout, stat 0x50 host_stat 0x1
>  unknown partition table
> Attached scsi disk sdb at scsi14, channel 0, id 0, lun 0
> Attached scsi generic sg2 at scsi14, channel 0, id 0, lun 0,  type 0
> 
> 
> I'm not too concerned about the errors when the drive
> appears yet, but trying to access the disk that just
> vanished is never going to work well. Am I missing
> something ?

(you should probably CC linux-scsi@vger.kernel.org as well, when
mentioning SCSI-related stuff)

I've seen this behavior before.  It's weird, but we must deal with it
anyway because

Since scsi_remove_device() must be called in process context, there will
ALWAYS be a window where a command could get issued.  Therefore, we must
create and set a "device is gone" flag in our local structures, and
check that in the queuecommand handler.

You must also make sure to clean up any currently-executing commands
properly, when a device is removed.

	Jeff

Re: libata & scsi rescan.

[Jeff Garzik]

Andy Warner wrote:
> 		sdev = scsi_add_device(ap->host, 0, 0, 0) ;

> 		sdev = scsi_device_lookup(ap->host, 0, 0, 0);

These args are wrong.  Consider master/slave configurations.

Also, remember my short message about a debounce timer.  It's not as 
simple as this... you need to make sure the driver doesn't "scream" 
add_device/remove_device while the SATA bus settles.

	Jeff

Re: libata & scsi rescan.

[Andy Warner]

Jeff Garzik wrote:
> Andy Warner wrote:
> > 		sdev = scsi_add_device(ap->host, 0, 0, 0) ;
> 
> > 		sdev = scsi_device_lookup(ap->host, 0, 0, 0);
> 
> These args are wrong.  Consider master/slave configurations.

Like I said: "... (ignore hardcoded values and sloppy error
checking for now)"

> Also, remember my short message about a debounce timer.  It's not as 
> simple as this... you need to make sure the driver doesn't "scream" 
> add_device/remove_device while the SATA bus settles.

I didn't show that logic, but I think the code in the interrupt
handler deals with that:

if ((err & SCR_DIAG_N) && (ap->hotplug_task_state == HOTPLUG_ST_IDLE)) {
	/*
	 * Schedule hotplug task to deal with it.
	 */
	ap->hotplug_task_state = HOTPLUG_ST_PENDING;
	queue_delayed_work(ata_wq, &ap->hotplug_task, 20);
}

[again, ignore locking issues for now]

What this effectively does is start a one-shot on the first
interrupt due to bit 16, and ignore any subsequent transitions.
>= 20 jiffies later (TODO: figure out the optimal value) the other
hotplug_task runs, by which time the bus will have settled. It reads
the value and does what it must. I think this will accomplish
what is needed.

I too want to cache dev in *ap, but that's another bunch of issues
(like either unrolling scsi_scan_host() as called from ata_device_add(),
figuring out ap->dev after scsi_scan_host() has run, or just
letting the hotswap logic add the drives after initting the
host.) I'll get to that after I fixup the scsi_remove_device()
problems.

I think I also need logic to ensure that I don't call scsi_add_device()
twice [if (!(ap->dev)) { ...} springs to mind].

Thanks for confirming that the "sync the disk that just went
away"/scsi_remove_device() problem is structural, not just me
calling the wrong function. I'll look into implementing flags
as you suggest.
-- 
andyw@pobox.com

Andy Warner		Voice: (612) 801-8549	Fax: (208) 575-5634

Re: libata & scsi rescan.

[Andy Warner]

Jeff Garzik wrote:
> [...]
> These args are wrong.  Consider master/slave configurations.

Can I check that I've got these things right:

 o *ap is a per sata-HBA-port structure ?
 o each port can have 0, 1 or 2 drives attached ?
   Though 0 & 1 are the only practical values
   for current SATA hardware ?
   Does this become 0 .. n when port expanders
   enter into the picture ?
 o struct scsi_device is a per drive structure ?

So *ap may need to support an aribrary list of devs,
just like struct Scsi_host does today ?

Since I bring up port expanders, will we still get hot-plug
notification from the hardware when a downstream drive changes
state ?
-- 
andyw@pobox.com

Andy Warner		Voice: (612) 801-8549	Fax: (208) 575-5634

Re: libata & scsi rescan.

[Jeff Garzik]

Also, to avoid scsi_device_lookup(), it may be nice to cache struct 
scsi_device pointer in struct ata_device.

	Jeff