QStor SATA/RAID driver for 2.6.9-rc3

Here is the current release of the 2.6.xx driver source code for the Pacific Digital Corporation QStor SATA/RAID chip.

This 4-channel chip has hardware-assisted RAID0/RAID1/RAID10, host-queuing, per-request TCQ/NCQ support, support for hot insertion and removal of drives, etc.. The 64-bit/66Mhz chip shows throughput in excess of 220MByte/sec on my ancient P3-1GHz test system, and can do much better when installed in a PCI-X slot.

The driver (attached) supports most of the chip features, including host, native and legacy tagged queuing, but does not yet include boot-from-raid support (coming soon), or SATA port-multipliers.

Both hdparm and smartmontools are supported by this driver.

This patch is against linux-2.6.9-rc3.

Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

Okay, last call for comment on this driver.

Any other issues before initial kernel inclusion?

Cheers

Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

Yes, there's lots of issues, but I need some time to write them all up. You could start by removing all the silly typedefs and the EXPORT_SYMBOLs that have been vetoed.

Which typedefs do you mean? Most all of the original ones are gone.

And the EXPORT_SYMBOLS() have not been vetoed to my knowledge. They're required for addtional driver extensions that are being provided later one, as GPL'd source code.

Cheers

Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

Yes, they have been vetoed :) I explicitly used the word 'vetoed' in fact.

You can add the hooks when you add code to the kernel that uses them.

We don't add hooks on the _hope_ that _future_ code will (a) use the hooks and (b) be GPL'd.

Jeff

Sure we do. All of the time.

All of the other RAID drivers in the kernel have ioctl() hooks for external code to control driver interfaces and settings. Except with that kind of interface, we never get an open-source version of that external code.

With exported symbols to support a GPL source-code supplement, we get to see the code for all of it. In this case, that code is still being written, but it will be GPL in the end, simply because it will be a kernel module, which by definition is subject to the GPL.

This module will NOT be submitted to the stock kernel initially, though, so you won't see it on lkml for some time. That's because of the hoops that vendors must jump through (repeatedly) just to provide good open-source kernel support.

Given all of the fuss over this core driver (qstor.{ch}), there is simply no way the vendor wants to go through it all again for their RAID management module. So sure, it will be GPL and given away in source form (website, installation CD, etc..), but it won't appear here simply because we're making too hard for them to do so.

The exports are needed if we want that component to be open source. Otherwise, they'll be replaced by ioctl()s like all of the other drivers, and that part of the source code may then never be released.

Cheers

Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

You are missing the key point that an ioctl is _vastly_ differently from a kernel interface, from both a legal and technical perspective.

The userland<->kernel interface is a hard "line in the sand" where we don't presuppose you are "linking" (as the GPL defines it)

Hardly. You're requesting hooks for something that is (a) unreviewed and (b) doesn't even exist. So far as things stand, the need for the hooks has not been justified.

Furthermore, it has always been the Linus policy "do what you need, and no more." Adding hooks before they are used violates that credo.

Fundamentally we do not add kernel interfaces for code that isn't in the kernel.

Overall, I don't see why it is so damned difficult to delete the hooks then add them back when they _are_ needed. I would certainly support you in that effort.

Jeff

Okay, that can work.

Except that the hooks ARE needed NOW.

Right NOW, there is a programmer working on the RAID management interface, and he needs those hooks (or something similar) to compile and test his code against the driver.

Remember, the vendor wants to decouple the RAID management from the in-tree kernel code. They fully want to open-source (GPL) all of it, but in two pieces: (1) core driver to boot/run the system, and (2) loadable component to provide advanced RAID management.

It is anticipated that development of (2) will take some time and have many revisions, and they really want to decouple it's release from that of the stock kernel. Thus, the latest version of that code will be provided via website and installation CD to customers who actually buy/use the product.

I wonder if there's a better way to do this?

Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

You're the only person in the world that (a) needs these hooks NOW and (b) can utilize the hooks NOW, by your own admission ;-)

That's the best reasoning I've heard for why a piece of code _shouldn't_ be in the kernel. And I'm quite certain you are capable of compiling and testing the driver with a local patch held back from upstream.

Upstream is for stuff that's either finished, or at least usable...

Jeff

Actually, no.

There's a full-time programmer at PDC working on the RAID management layer for this, plus all of the folks there working on the O/S independent apps in userland for the card.

Perhaps I can get hold of an early snapshot of that code from them (the chardev driver), and submit that as a subsequent patch.

So, skipping the EXPORTs for now, how do you guys feel about the driver ?

Cheers

Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

- please kill the #ifndef SERVICE_ACTION_IN section
- please use <scsi/scsi.h> sense data ifdefs instead of adding your own
- please kill your scsi_to_pci_dma_dirusage, it's been obsoleted for a reason
- please remove DRPINTK in favour of pr_debug from <linux/kernel.h>
- please removæ the global host list, not just the exports for it
- dito for qs_notify
- code like
+ if (dev->raid) {
+ memset(dev->raid, 0, sizeof(struct qs_raid));
+ kfree(dev->raid);
+ }
is totally bogus, you defeat the slab poisoning with that
- qs_alloc duplicates kcalloc
- the !dev case in qs_scsi_queuecomman can't happen
- no need for the case in sc->host_scribble = (void *)(dev->uhba);
- never mess with eh_timeout from inside a driver
- please don't implemente the HDIO_ ioctls, Jeff said this can be done via SG_IO
- if ->info return a static string you can just store it into ->name
- please don't discard the exact error returned from pci_enable_device(pdev)
- never call scsi_set_device() in a new driver, scsi_add_host does that for you
- never use scsi_assign_lock in a new driver, the midlayer already has a per-host lock for you
- please use the kernel/kthread.c interface for your kernel thread
- never cast your ioregs before iounmap.
- in fact it looks like you want to run sparse over the driver and fix up everything it found - at least __iomem annotations are missing
- please don't use set_fs(KERNEL_DS), split up the underlying code for user and kernel buffers

This was just a quick 5 minute review, I'll give it a deeper look once I get a little bit more time.

Are you sure?
I have seen it occur immediately after hot-removal of a drive. There have been other structural changes since then, so perhaps it is no longer possible, but I'd rather have the test there than have the kernel ooops again. If you feel strongly about it, then away it goes.

Give us an interface for it, please.
In the meanwhile, gone!

SG_IO is incompatible with current user-mode toolsets. Once that interface becomes more mature, and the distributions gradually get updated with newer versions of the tools, then the HDIO_ stuff can go (as per the comments in the source). For now, it is essential for hdparm and smartmontools, among others.

Alternatively, as Jeff has suggested, we may be able to implement a generic HDIO_ mechanism in libata that re-issues the commands through SG_IO (perhaps that is what you meant). Is that there now?

So just nuke the _info() proc, and use .name = QS_DESC ? Okay, done.

Good -- I hadn't noticed that new interface before.

It's quite hard to find all of this stuff first time through, as there are practically no existing drivers that don't have many of the same comments applicable to them.

Perhaps this will be one of the first/few drivers to become totally compliant with the latest kernel APIs.

Cheers
--
Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

Hmm. It's freed in ->slave_destroy and the scsi state model shouldn't allow new command submission long before. If you still see it happen please send a bugreport to linux-scsi.

set scsi_device->timeout in ->slave_alloc to the value you want.

So please fixup userspace. New hardware will require new system tools once in a while.

Yes.

Here is an updated copy of the QStor driver patch
will all of Christoph's points addressed.

Thanks guys.

Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

Here is an updated copy of the QStor driver patch for 2.6.9-rc4, will most points to-date addressed.

The only possible sticky point I know of so far, is that the SCSI INQUIRY code could be shared with libata-scsi, if some changes were makde to libata-scsi. I've email'd Jeff a proposal for that. But in the meanwhile, we need to get this driver moving.

Bug reports are appreciated. Thanks guys.

Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

Actually, the driver has no need for a thread at all. Since you're simply using it to fire hotplug events, use schedule_work instead.

I also noticed the following in a lightening review:

- Kill these constructs:
+ /* scsi_done expects to be called while locked */
+ if (!in_interrupt())
+ spin_lock_irqsave(uhba->lock, flags);

scsi_done() doesn't require a lock

- Your emulated commands assume they're non-sg and issued through the kernel (i.e. you don't kmap and you don't do SG). This will blow up on the first inquiry submitted via SG_IO for instance.

I'm sure there are others, but I don't have time to do a thorough review at the moment.

James

That worries me some, because the mid-layer will perform blocking I/O and the like, and I'm not sure how much that stuff may depend on its own usage (any?) of workqueues. If you believe it to be safe, then I'll nuke the kthread entirely.

Really? I wonder why the mid-layer is so religious about doing the lock around every invocation of it today?

But again, if we're sure that this is the case, then it certainly make's life simpler in the driver.

The SG is tested for and simply failed -- there is no need today for SG usage on those code paths. If there turns out to be a need for that interface with this driver in the future, we can add it. Just like most of the other drivers currently treat it.

What is the "kmap" semantic, and how should it be applied here?

Thanks

Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

We use this already for other entities that require user context like domain validation. It seems to work as advertised.

It's not if you look at other drivers. There's no harm in taking the lock, so none of the old ones got updated, but the lock isn't needed. The idea is that if you're holding the lock naturally (say in an interrupt routine) there's no need to drop it artificially. However, you definitely shouldn't take it artificially like you do.

And you've tested this with things like SUSE's hwinfo utility which seems to send INQUIRIES on its own?

kmap is used to make a user page visible to the kernel.

Really, I suppose, libata should provide the interfaces for doing this work for emulated commands.

James

Okay, done.

Well, I was actually looking at scsi.c, where this kind of thing is common:

   spin_lock_irqsave(host->host_lock, flags);
   scsi_done(cmd);
   spin_unlock_irqrestore(host->host_lock, flags);

If those locks are not needed, the scsi.c maintainer really should nuke'em. Meanwhile, I'll take them out of qstor.c as well, thanks.

Not yet, but I have tested them with other scsiinfo-like tools. Again, when it is proven to be an issue with something, it will get fixed.

Well, after this driver submission work is done with, that's next on my list. Right now libata doesn't have the right interface for easy sharing of such functions.

And the driver will need it's own copy in some versions anyway, since this driver will be used on much older/earlier kernels than just the ones with the latest libata stuff.

Cheers

Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

Typically that's done with an out-of-tree compatibility module, such as something like kcompat (http://sf.net/projects/gkernel/), which provides a modern driver API to older kernels.

Jeff

I think you can safely assume he has more important things to do.

Not emulating an INQUIRY properly via SG_IO isn't acceptable since it's a mandatory command.

libata does all this correctly. I strongly suggest you find a way to share the code rather than trying to reinvent it yourself. But anyway, if you want to know how it should work, look in libata-scsi.c

James

I was actually working on the assumption that the lock might be there because it is/was necessary for something, like perhaps protecting access to the add_timer()/del_timer() calls associated with the scmd?

If not, no issue -- it can be removed from the driver.

Cheers
--
Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

I'll respectfully disagree with James... I think the most prudent course of action is to follow the example of SCSI common code.

If the SCSI core is doing something wrong, we should fix that _first_, not set a precedent of confusing dissociation.

Everyone knows that Linux programmers engineer with their cut-n-paste feature.

Jeff

So you'll be sending me the patches that do this?

James

I'm just saying you are encouraging inconsistency, which is wrong.

Mark should either
a) follow the style you request, _and_ submit patches to clean up the SCSI core, or

b) take the lock, just like the SCSI core is doing.

I disagree with the assertion that Mark's code should be different from the SCSI core.

Jeff

Can deadlock occur here, since qstor.c is already using schedule_work() as part of it's internal bottom-half handling for abnormal conditions?

Eg. hotplug event -> schedule_work -> mid-layer -> queuecommand

      --> sleep  :: interrupt -> schedule_work -> deadlock?

Just checking.. we all want this to function well.

Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

Since you wouldn't go straight from schedule_work->mid-layer, I assume you mean that when the workqueue thread runs the work?

With that assumption, this is legal and won't deadlock.

However, I assume you know you can't sleep in queuecommand since it may be run from the scsi tasklet?

James

Furthermore queuecommand is inside spin_lock_irqsave

Jeff

Yes. The workqueue thread will invoke the mid-layer function, which will do a queuecommand, which will return to the mid-layer, which will then SLEEP waiting for the command to complete, which will sleep that workqueue thread.

As part of the interrupt processing to complete the command in the LLD, it is possible that schedule_work may be necessary, requiring that a workqueue thread be run. If this means the same thread that is already sleeping courtesy of the mid-layer, then we could have a problem.

Comments?

Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

The only schedule_work() call in the SCSI common code is for domain validation.

All the other stuff is done by the totally independent error handling threads, or by non-process contexts such as tasklets.

Jeff

This particulare schedule_work() would be invoked from the interrupt handler in the LLD -- part of the qstor driver.

Is there a single thread (per CPU) for doing work from schedule_work(), or are there multiple such threads created on demand?

If there's just a single thread, then this scenario (described above) could indeed deadlock, in which case qstor cannot use schedule_work() to perform notification of drive hot insert/removal events.

What do you think, Jeff?
--
Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

Your assessment is correct, in that, on single-CPU systems there is only one thread for schedule_work() events. For each workqueue, there is one thread per CPU.

That does not preclude (a) using a private workqueue, rather than the general one or (b) using the kthread API as Christoph suggested.

Jeff

Christoph says that your driver doesn't do Domain Validation, which is the only place in the SCSI layer that uses schedule_work(). So you are fine.

Of course... the source code is there, you could have figured this out for yourself :)

Jeff

Erm, perhaps you don't understand the concept of a work queue. It's a queue of pending work. There's a back end daemon servicing the queue and executing all the items on the queue in sequence. schedule_work just adds an item of work to the queue and returns.

So, it's perfectly legal to call schedule_work from within the work queue function, because all you do is add the work to the list for the daemon thread to execute when it gets to it. There's nothing synchronous about a workqueue. If a work function sleeps, then its true, it takes the worker thread longer to get to the next work item, but as long as the work function awakes, it will get there.

James

Good. That is exactly how I suspected it worked.
In which case, deadlock *will* happen in the scenario described, and the qstor driver should therefore NOT use schedule_work() as the means of invoking the scsi_add_device()/scsi_remove_device() functions. A separate thread appears needed.

Again, the scenario is: schedule_work is used to have a work function invoked asynchronously, which then invokes scsi_add_device(), which then queues a command to the device and SLEEPS awaiting completion of the (INQUIRY) command.

As part of handling the command in the LLD, the qstor_intr() handler may use a (schedule_work) function to perform bottom-half processing for that very same command. If the worker thread is stuck sleeping in the mid-layer, then it will never get around to the qstor bottom-half processing that is needed to complete the original activity.

Dead-lock.

Right?

Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

In that scenario, you use a separate workqueue.

However, when I last looked at your driver you were only using the thread to provide user context for hotplug events ... where did this back end finishing thread come from?

James

Okay. But what thread should run that workqueue?

It's been there since day one. The interrupt handling sometimes requires more functionality than is available at interrupt time, so it uses schedule_work to have a bottom half re-run itself from thread context. This is needed in the error-processing and hot plug paths.

Cheers

Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

ewwww :) If you find yourself calling your irq path from non-irq-context code, back up, you're going down the wrong path.

The usual way to do what you want is either

1) wrap all code that _might_ be called from inside interrupt handler inside spin_lock_irqsave() [except when you are in the interrupt handler, of course, which is merely spin_lock()

Any code that checks "if (in_interrupt())" should be shot on sight :)

2) have both interrupt and non-interrupt contexts fire a tasklet using tasklet_schedule(). Your tasklet function then does the real work.

Jeff

Jeff Garzik wrote:
..

That's how it works already, thanks, except that it does have a few calls to in_interrupt() rather than simply passing itself a flag parameter to convey the same information -- I'll fix that now.

Except that it uses schedule_work() rather than a tasklet. The bottom half is only there for abnormal conditions like major chip errors and hotplug events.

So the only new suggestion here is to use a tasklet for the bottom-half processing rather than schedule_work()?

I thought work queues were the preferred mechanism for infrequent uses such as this these days? A tasklet is no problem here though, so long as worker threads (schedule_work) do not also rely on tasklets.

Cheers

Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

Really, no, you don't want to be doing what you are doing in qs_process_sff_entry()

At certain points you decide you have too much work, disable interrupts on the card and reschedule the routine in a work queue.

Please, please don't do this. It's a sure fire recipe for a hang. Suppose a prior task in the workqueue needs to page something in from swap and you're the swap device for instance....

What you need to do is to gather as much information as will reset the interrupt and then process the data as a tasklet. For your hotplug events, they should be fire and forget as schedule_work().

*never* disable interrupts and have re-enabling them contingent on a workqueue thread.

James

James Bottomley wrote:
..

Okay. Good find, thanks.

I'll rework that portion to remove the bh handling completely, doing everything possible directly from within the interrupt handler.

It will continue to use schedule_work to handle hotplug events.

Cheers

Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

[subject changed as this knowledge can benefit others as well]

In addition to agreeing with James' most recently assessment of qs_process_sff_entry(), I wanted to interject a bit of more general discussion...

Two main, but unrelated, driver-writing points:

1) Putting almost all your in-irq code into a tasklet can be quite convenient, if your situation is amenable to that. The benefits are

a) your irq handler is tiny,

ack irqs
tasklet_schedule()

b) data can be synchronized without a lock, if the data is only used in the tasklet or in between tasklet_disable/tasklet_enable pairs.

c) you can "call" your in-irq code from places other than your irq handler, and let the tasklet subsystem synchronize the tasklet schedule. no worries about disabling interrupts, they will just do (a) as
described above.

2) You want to avoid a model (like qs_process_sff_entry, alas) where you have one single, huge "event completion" path, called from various points in the driver. Instead, do your best to make sure event/action as fine-grained as possible.

Storage drivers that want to handle long-running events, or events that need process context, typically want to either fire off events _asynchronously_ via schedule_work(), or have a long-running thread that does nothing but processes an internal driver event queue. It's really programmer's preference at that point, as having a single (or per-host) event queue thread can sometimes be more simple than async work queue code.

Jeff

At driver module unload time, is there any way to guarantee that all pending "schedule_work()" events have been processed?

How?

Thanks

Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

flush_workqueue() [private workqueue] and flush_scheduled_work()

Jeff

Did you read my message???

QStor doesn't do domain validation, which is the only place where the SCSI core also calls schedule_work().

Your conclusion is incorrect AFAICS.

If you are creating a deadlock within your own driver, that's a separate issue. There is no deadlock with the SCSI core.

However... tasklets are for bottom-half processing, not threads.

Jeff

Pleasee stop the whining Mark. Either remove the exports or give up on the submission.

Maybe I can put it another way.

Without seeing the code that uses the hooks, we cannot evaluate whether the hooks are needed, useful, or even properly implemented. They are unreviewable.

Does this same argument hold true for ioctls? Yes. But (as noted in the previous email) ioctls and kernel API hooks are quite different.

Jeff

On a related note..

In the longer term, I'd like Jeff & I to get together and agree upon some interface changes in libata to make it easier for this driver (and others) to share more of the code dealing with the emulation of non-data SCSI commands like INQUIRY and friends.

Right now that's not as easy as it could be, due to the specialized libata struct parameters required, but I think it could be harmonized.

Cheers

Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

libata exists as it does simply due to how it evolved.

Please just submit patches containing the changes you want, I'm very receptive to improvements that increase the breadth of libata's coverage.

As the name implies, libata is just a library of code and nothing more.
A driver could choose to use the to/from FIS functions and none of the
driver architecture, for example. libata exists solely to concentrate ATA code into a single location.

(similarly, include/linux/ata.h exists to concentrate all ATA-related defines in one location)

Jeff

As another example, a piece of code that implements the HDIO_xxx ioctls in terms of a SCSI command can be quite generic, and separate from the libata driver API, while still in libata.

Jeff

I recall this but don't see it in my inbox anymore... did I adequately respond?

The easy answer is always: send patches. that's the best way to illustrate your design, and the quickest way to get the changes you want into the kernel.

Jeff

Well, that makes two of us that have lost the original. :)

No response from you yet, and I don't want to waste the effort on patches if they'll be rejected outright, thus the initial query:

The change would be as follows: Export ata_scsi_simulate(), and replace the first two parameters (struct ata_port, struct ata_device) with a pointer to the 512-byte drive IDENTIFY response data.

That way, ata_scsi_simulate() becomes usable from drivers (like qstor) that are not libata based. The identify data parameter would, of course, also be propogated down into all of the associated helper functions that get invoked by ata_scsi_simulate() and pals.

At present, this will work, since those routines are only interested in the identify data today, but I don't know about any future plans there might be for it all.

Of course, even to use this, QStor will still have to create fake identify blocks for each RAID device..

Cheers

Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

Fine with me but you'll need more than just the identify data, if you wanna do stuff like support MODE SELECT/SENSE.

Jeff

QStor has no need for MODE_SELECT, and the current implementation of MODE_SENSE in libata-scsi appears to use only the IDENTIFY data.

The READ_CAPACITY emulation in libata-scsi currently uses dev->n_sectors, but that could be changed to recalculate it from the IDENTIFY words.

So.. okay in concept?

Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

Seems sane, send a patch.

Typically we aren't in the business of pre-approving patches, much to ESR's chagrin :)

Jeff

Here you go. This patch modifies libata-scsi for easier sharing of the various ata_id_* functions and the ata_scsi_simulate() function with non-libata drivers.

Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

diff -u --recursive --new-file --exclude='.*' linux-2.6.9-rc4/drivers/scsi/libata-core.c linux/drivers/scsi/libata-core.c
@@ -829,17 +829,17 @@
  *	caller.
  */
 
-void ata_dev_id_string(struct ata_device *dev, unsigned char *s,
+void ata_dev_id_string(u16 *id, unsigned char *s,
 		       unsigned int ofs, unsigned int len)
 {
 	unsigned int c;
 
 	while (len > 0) {
-		c = dev->id[ofs] >> 8;
+		c = id[ofs] >> 8;
 		*s = c;
 		s++;
 
-		c = dev->id[ofs] & 0xff;
+		c = id[ofs] & 0xff;
 		*s = c;
 		s++;
 

@@ -1082,7 +1082,7 @@
 	 */
 
 	/* we require LBA and DMA support (bits 8 & 9 of word 49) */
-	if (!ata_id_has_dma(dev) || !ata_id_has_lba(dev)) {
+	if (!ata_id_has_dma(dev->id) || !ata_id_has_lba(dev->id)) {
 		printk(KERN_DEBUG "ata%u: no dma/lba\n", ap->id);
 		goto err_out_nosup;
 	}

@@ -1100,7 +1100,7 @@
 
 	/* ATA-specific feature tests */
 	if (dev->class == ATA_DEV_ATA) {
-		if (!ata_id_is_ata(dev))	/* sanity check */
+		if (!ata_id_is_ata(dev->id))	/* sanity check */
 			goto err_out_nosup;
 
 		tmp = dev->id[ATA_ID_MAJOR_VER];

@@ -1114,11 +1114,11 @@
 			goto err_out_nosup;
 		}
 
-		if (ata_id_has_lba48(dev)) {
+		if (ata_id_has_lba48(dev->id)) {
 			dev->flags |= ATA_DFLAG_LBA48;
-			dev->n_sectors = ata_id_u64(dev, 100);
+			dev->n_sectors = ata_id_u64(dev->id, 100);
 		} else {
-			dev->n_sectors = ata_id_u32(dev, 60);
+			dev->n_sectors = ata_id_u32(dev->id, 60);
 		}
 
 		ap->host->max_cmd_len = 16;

@@ -1133,7 +1133,7 @@
 
 	/* ATAPI-specific feature tests */
 	else {
-		if (ata_id_is_ata(dev))		/* sanity check */
+		if (ata_id_is_ata(dev->id))		/* sanity check */
 			goto err_out_nosup;
 
 		rc = atapi_cdb_len(dev->id);

diff -u --recursive --new-file --exclude='.*' linux-2.6.9-rc4/drivers/scsi/libata.h linux/drivers/scsi/libata.h
@@ -29,9 +29,8 @@
 #define DRV_VERSION	"1.02"	/* must be exactly four chars */
 
 struct ata_scsi_args {
-	struct ata_port		*ap;
-	struct ata_device	*dev;
-	struct scsi_cmnd		*cmd;
+	u16			*id;
+	struct scsi_cmnd	*cmd;
 	void			(*done)(struct scsi_cmnd *);
 };
 

diff -u --recursive --new-file --exclude='.*' linux-2.6.9-rc4/drivers/scsi/sata_sil.c linux/drivers/scsi/sata_sil.c
@@ -287,7 +287,7 @@
 	const char *s;
 	unsigned int len;
 
-	ata_dev_id_string(dev, model_num, ATA_ID_PROD_OFS,
+	ata_dev_id_string(dev->id, model_num, ATA_ID_PROD_OFS,
 			  sizeof(model_num));
 	s = &model_num[0];
 	len = strnlen(s, sizeof(model_num));

diff -u --recursive --new-file --exclude='.*' linux-2.6.9-rc4/include/linux/ata.h linux/include/linux/ata.h
@@ -217,24 +217,24 @@
 	u8			command;	/* IO operation */
 };
 
-#define ata_id_is_ata(dev)	(((dev)->id[0] & (1 << 15)) == 0)
-#define ata_id_rahead_enabled(dev) ((dev)->id[85] & (1 << 6))
-#define ata_id_wcache_enabled(dev) ((dev)->id[85] & (1 << 5))
-#define ata_id_has_flush(dev) ((dev)->id[83] & (1 << 12))
-#define ata_id_has_flush_ext(dev) ((dev)->id[83] & (1 << 13))
-#define ata_id_has_lba48(dev)	((dev)->id[83] & (1 << 10))
-#define ata_id_has_wcache(dev)	((dev)->id[82] & (1 << 5))
-#define ata_id_has_pm(dev)	((dev)->id[82] & (1 << 3))
-#define ata_id_has_lba(dev)	((dev)->id[49] & (1 << 9))
-#define ata_id_has_dma(dev)	((dev)->id[49] & (1 << 8))
-#define ata_id_removeable(dev)	((dev)->id[0] & (1 << 7))
-#define ata_id_u32(dev,n)	\
-	(((u32) (dev)->id[(n) + 1] << 16) | ((u32) (dev)->id[(n)]))
-#define ata_id_u64(dev,n)	\
-	( ((u64) dev->id[(n) + 3] << 48) |	\
-	  ((u64) dev->id[(n) + 2] << 32) |	\
-	  ((u64) dev->id[(n) + 1] << 16) |	\
-	  ((u64) dev->id[(n) + 0]) )
+#define ata_id_is_ata(id)	(((id)[0] & (1 << 15)) == 0)
+#define ata_id_rahead_enabled(id) ((id)[85] & (1 << 6))
+#define ata_id_wcache_enabled(id) ((id)[85] & (1 << 5))
+#define ata_id_has_flush(id) ((id)[83] & (1 << 12))
+#define ata_id_has_flush_ext(id) ((id)[83] & (1 << 13))
+#define ata_id_has_lba48(id)	((id)[83] & (1 << 10))
+#define ata_id_has_wcache(id)	((id)[82] & (1 << 5))
+#define ata_id_has_pm(id)	((id)[82] & (1 << 3))
+#define ata_id_has_lba(id)	((id)[49] & (1 << 9))
+#define ata_id_has_dma(id)	((id)[49] & (1 << 8))
+#define ata_id_removeable(id)	((id)[0] & (1 << 7))
+#define ata_id_u32(id,n)	\
+	(((u32) (id)[(n) + 1] << 16) | ((u32) (id)[(n)]))
+#define ata_id_u64(id,n)	\
+	( ((u64) (id)[(n) + 3] << 48) |	\
+	  ((u64) (id)[(n) + 2] << 32) |	\
+	  ((u64) (id)[(n) + 1] << 16) |	\
+	  ((u64) (id)[(n) + 0]) )
 
 static inline int atapi_cdb_len(u16 *dev_id)
 {

diff -u --recursive --new-file --exclude='.*' linux-2.6.9-rc4/include/linux/libata.h linux/include/linux/libata.h
@@ -403,7 +403,7 @@
 extern void ata_sg_init(struct ata_queued_cmd *qc, struct scatterlist *sg,
 		 unsigned int n_elem);
 extern unsigned int ata_dev_classify(struct ata_taskfile *tf);
-extern void ata_dev_id_string(struct ata_device *dev, unsigned char *s,
+extern void ata_dev_id_string(u16 *id, unsigned char *s,
 			      unsigned int ofs, unsigned int len);
 extern void ata_bmdma_setup (struct ata_queued_cmd *qc);
 extern void ata_bmdma_start (struct ata_queued_cmd *qc);

@@ -613,9 +613,9 @@
 
 static inline int ata_try_flush_cache(struct ata_device *dev)
 {
-	return ata_id_wcache_enabled(dev) ||
-	       ata_id_has_flush(dev) ||
-	       ata_id_has_flush_ext(dev);
+	return ata_id_wcache_enabled(dev->id) ||
+	       ata_id_has_flush(dev->id) ||
+	       ata_id_has_flush_ext(dev->id);
 }
 
 #endif /* __LINUX_LIBATA_H__ */

Oh crap.. forgot to add the EXPORT..

Re-issue of this patch coming momentarily.

Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

This patch modifies libata-scsi for easier sharing of the various ata_id_* functions and the ata_scsi_simulate() function with non-libata drivers.

This replaces an earlier patch which was missing the EXPORT_SYMBOL_GPL().

Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

diff -u --recursive --new-file --exclude='.*' linux-2.6.9-rc4/drivers/scsi/libata-core.c linux/drivers/scsi/libata-core.c
@@ -47,6 +47,9 @@
 
 #include "libata.h"
 
+void ata_scsi_simulate(u16 *id,
+		      struct scsi_cmnd *cmd,
+		      void (*done)(struct scsi_cmnd *));
 static unsigned int ata_busy_sleep (struct ata_port *ap,
 				    unsigned long tmout_pat,
 			    	    unsigned long tmout);

@@ -829,17 +832,17 @@
  *	caller.
  */
 
-void ata_dev_id_string(struct ata_device *dev, unsigned char *s,
+void ata_dev_id_string(u16 *id, unsigned char *s,
 		       unsigned int ofs, unsigned int len)
 {
 	unsigned int c;
 
 	while (len > 0) {
-		c = dev->id[ofs] >> 8;
+		c = id[ofs] >> 8;
 		*s = c;
 		s++;
 
-		c = dev->id[ofs] & 0xff;
+		c = id[ofs] & 0xff;
 		*s = c;
 		s++;
 

@@ -1082,7 +1085,7 @@
 	 */
 
 	/* we require LBA and DMA support (bits 8 & 9 of word 49) */
-	if (!ata_id_has_dma(dev) || !ata_id_has_lba(dev)) {
+	if (!ata_id_has_dma(dev->id) || !ata_id_has_lba(dev->id)) {
 		printk(KERN_DEBUG "ata%u: no dma/lba\n", ap->id);
 		goto err_out_nosup;
 	}

@@ -1100,7 +1103,7 @@
 
 	/* ATA-specific feature tests */
 	if (dev->class == ATA_DEV_ATA) {
-		if (!ata_id_is_ata(dev))	/* sanity check */
+		if (!ata_id_is_ata(dev->id))	/* sanity check */
 			goto err_out_nosup;
 
 		tmp = dev->id[ATA_ID_MAJOR_VER];

@@ -1114,11 +1117,11 @@
 			goto err_out_nosup;
 		}
 
-		if (ata_id_has_lba48(dev)) {
+		if (ata_id_has_lba48(dev->id)) {
 			dev->flags |= ATA_DFLAG_LBA48;
-			dev->n_sectors = ata_id_u64(dev, 100);
+			dev->n_sectors = ata_id_u64(dev->id, 100);
 		} else {
-			dev->n_sectors = ata_id_u32(dev, 60);
+			dev->n_sectors = ata_id_u32(dev->id, 60);
 		}
 
 		ap->host->max_cmd_len = 16;

@@ -1133,7 +1136,7 @@
 
 	/* ATAPI-specific feature tests */
 	else {
-		if (ata_id_is_ata(dev))		/* sanity check */
+		if (ata_id_is_ata(dev->id))		/* sanity check */
 			goto err_out_nosup;
 
 		rc = atapi_cdb_len(dev->id);

@@ -3655,3 +3658,4 @@
 EXPORT_SYMBOL_GPL(ata_host_intr);
 EXPORT_SYMBOL_GPL(ata_dev_classify);
 EXPORT_SYMBOL_GPL(ata_dev_id_string);
+EXPORT_SYMBOL_GPL(ata_scsi_simulate);

diff -u --recursive --new-file --exclude='.*' linux-2.6.9-rc4/drivers/scsi/libata.h linux/drivers/scsi/libata.h
@@ -29,9 +29,8 @@
 #define DRV_VERSION	"1.02"	/* must be exactly four chars */
 
 struct ata_scsi_args {
-	struct ata_port		*ap;
-	struct ata_device	*dev;
-	struct scsi_cmnd		*cmd;
+	u16			*id;
+	struct scsi_cmnd	*cmd;
 	void			(*done)(struct scsi_cmnd *);
 };
 

diff -u --recursive --new-file --exclude='.*' linux-2.6.9-rc4/drivers/scsi/sata_sil.c linux/drivers/scsi/sata_sil.c
@@ -287,7 +287,7 @@
 	const char *s;
 	unsigned int len;
 
-	ata_dev_id_string(dev, model_num, ATA_ID_PROD_OFS,
+	ata_dev_id_string(dev->id, model_num, ATA_ID_PROD_OFS,
 			  sizeof(model_num));
 	s = &model_num[0];
 	len = strnlen(s, sizeof(model_num));

diff -u --recursive --new-file --exclude='.*' linux-2.6.9-rc4/include/linux/ata.h linux/include/linux/ata.h
@@ -217,24 +217,24 @@
 	u8			command;	/* IO operation */
 };
 
-#define ata_id_is_ata(dev)	(((dev)->id[0] & (1 << 15)) == 0)
-#define ata_id_rahead_enabled(dev) ((dev)->id[85] & (1 << 6))
-#define ata_id_wcache_enabled(dev) ((dev)->id[85] & (1 << 5))
-#define ata_id_has_flush(dev) ((dev)->id[83] & (1 << 12))
-#define ata_id_has_flush_ext(dev) ((dev)->id[83] & (1 << 13))
-#define ata_id_has_lba48(dev)	((dev)->id[83] & (1 << 10))
-#define ata_id_has_wcache(dev)	((dev)->id[82] & (1 << 5))
-#define ata_id_has_pm(dev)	((dev)->id[82] & (1 << 3))
-#define ata_id_has_lba(dev)	((dev)->id[49] & (1 << 9))
-#define ata_id_has_dma(dev)	((dev)->id[49] & (1 << 8))
-#define ata_id_removeable(dev)	((dev)->id[0] & (1 << 7))
-#define ata_id_u32(dev,n)	\
-	(((u32) (dev)->id[(n) + 1] << 16) | ((u32) (dev)->id[(n)]))
-#define ata_id_u64(dev,n)	\
-	( ((u64) dev->id[(n) + 3] << 48) |	\
-	  ((u64) dev->id[(n) + 2] << 32) |	\
-	  ((u64) dev->id[(n) + 1] << 16) |	\
-	  ((u64) dev->id[(n) + 0]) )
+#define ata_id_is_ata(id)	(((id)[0] & (1 << 15)) == 0)
+#define ata_id_rahead_enabled(id) ((id)[85] & (1 << 6))
+#define ata_id_wcache_enabled(id) ((id)[85] & (1 << 5))
+#define ata_id_has_flush(id) ((id)[83] & (1 << 12))
+#define ata_id_has_flush_ext(id) ((id)[83] & (1 << 13))
+#define ata_id_has_lba48(id)	((id)[83] & (1 << 10))
+#define ata_id_has_wcache(id)	((id)[82] & (1 << 5))
+#define ata_id_has_pm(id)	((id)[82] & (1 << 3))
+#define ata_id_has_lba(id)	((id)[49] & (1 << 9))
+#define ata_id_has_dma(id)	((id)[49] & (1 << 8))
+#define ata_id_removeable(id)	((id)[0] & (1 << 7))
+#define ata_id_u32(id,n)	\
+	(((u32) (id)[(n) + 1] << 16) | ((u32) (id)[(n)]))
+#define ata_id_u64(id,n)	\
+	( ((u64) (id)[(n) + 3] << 48) |	\
+	  ((u64) (id)[(n) + 2] << 32) |	\
+	  ((u64) (id)[(n) + 1] << 16) |	\
+	  ((u64) (id)[(n) + 0]) )
 
 static inline int atapi_cdb_len(u16 *dev_id)
 {

diff -u --recursive --new-file --exclude='.*' linux-2.6.9-rc4/include/linux/libata.h linux/include/linux/libata.h
@@ -403,7 +403,7 @@
 extern void ata_sg_init(struct ata_queued_cmd *qc, struct scatterlist *sg,
 		 unsigned int n_elem);
 extern unsigned int ata_dev_classify(struct ata_taskfile *tf);
-extern void ata_dev_id_string(struct ata_device *dev, unsigned char *s,
+extern void ata_dev_id_string(u16 *id, unsigned char *s,
 			      unsigned int ofs, unsigned int len);
 extern void ata_bmdma_setup (struct ata_queued_cmd *qc);
 extern void ata_bmdma_start (struct ata_queued_cmd *qc);

@@ -613,9 +613,9 @@
 
 static inline int ata_try_flush_cache(struct ata_device *dev)
 {
-	return ata_id_wcache_enabled(dev) ||
-	       ata_id_has_flush(dev) ||
-	       ata_id_has_flush_ext(dev);
+	return ata_id_wcache_enabled(dev->id) ||
+	       ata_id_has_flush(dev->id) ||
+	       ata_id_has_flush_ext(dev->id);
 }
 
 #endif /* __LINUX_LIBATA_H__ */

Done. Updated patch attached.

Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

diff -u --recursive --new-file --exclude='.*' linux-2.6.9-rc4/drivers/scsi/libata-core.c linux/drivers/scsi/libata-core.c
@@ -829,17 +829,17 @@
  *	caller.
  */
 
-void ata_dev_id_string(struct ata_device *dev, unsigned char *s,
+void ata_dev_id_string(u16 *id, unsigned char *s,
 		       unsigned int ofs, unsigned int len)
 {
 	unsigned int c;
 
 	while (len > 0) {
-		c = dev->id[ofs] >> 8;
+		c = id[ofs] >> 8;
 		*s = c;
 		s++;
 
-		c = dev->id[ofs] & 0xff;
+		c = id[ofs] & 0xff;
 		*s = c;
 		s++;
 

@@ -1082,7 +1082,7 @@
 	 */
 
 	/* we require LBA and DMA support (bits 8 & 9 of word 49) */
-	if (!ata_id_has_dma(dev) || !ata_id_has_lba(dev)) {
+	if (!ata_id_has_dma(dev->id) || !ata_id_has_lba(dev->id)) {
 		printk(KERN_DEBUG "ata%u: no dma/lba\n", ap->id);
 		goto err_out_nosup;
 	}

@@ -1100,7 +1100,7 @@
 
 	/* ATA-specific feature tests */
 	if (dev->class == ATA_DEV_ATA) {
-		if (!ata_id_is_ata(dev))	/* sanity check */
+		if (!ata_id_is_ata(dev->id))	/* sanity check */
 			goto err_out_nosup;
 
 		tmp = dev->id[ATA_ID_MAJOR_VER];

@@ -1114,11 +1114,11 @@
 			goto err_out_nosup;
 		}
 
-		if (ata_id_has_lba48(dev)) {
+		if (ata_id_has_lba48(dev->id)) {
 			dev->flags |= ATA_DFLAG_LBA48;
-			dev->n_sectors = ata_id_u64(dev, 100);
+			dev->n_sectors = ata_id_u64(dev->id, 100);
 		} else {
-			dev->n_sectors = ata_id_u32(dev, 60);
+			dev->n_sectors = ata_id_u32(dev->id, 60);
 		}
 
 		ap->host->max_cmd_len = 16;

@@ -1133,7 +1133,7 @@
 
 	/* ATAPI-specific feature tests */
 	else {
-		if (ata_id_is_ata(dev))		/* sanity check */
+		if (ata_id_is_ata(dev->id))		/* sanity check */
 			goto err_out_nosup;
 
 		rc = atapi_cdb_len(dev->id);

@@ -3655,3 +3655,4 @@
 EXPORT_SYMBOL_GPL(ata_host_intr);
 EXPORT_SYMBOL_GPL(ata_dev_classify);
 EXPORT_SYMBOL_GPL(ata_dev_id_string);
+EXPORT_SYMBOL_GPL(ata_scsi_simulate);

diff -u --recursive --new-file --exclude='.*' linux-2.6.9-rc4/drivers/scsi/libata.h linux/drivers/scsi/libata.h
@@ -29,9 +29,8 @@
 #define DRV_VERSION	"1.02"	/* must be exactly four chars */
 
 struct ata_scsi_args {
-	struct ata_port		*ap;
-	struct ata_device	*dev;
-	struct scsi_cmnd		*cmd;
+	u16			*id;
+	struct scsi_cmnd	*cmd;
 	void			(*done)(struct scsi_cmnd *);
 };
 

diff -u --recursive --new-file --exclude='.*' linux-2.6.9-rc4/drivers/scsi/sata_sil.c linux/drivers/scsi/sata_sil.c
@@ -287,7 +287,7 @@
 	const char *s;
 	unsigned int len;
 
-	ata_dev_id_string(dev, model_num, ATA_ID_PROD_OFS,
+	ata_dev_id_string(dev->id, model_num, ATA_ID_PROD_OFS,
 			  sizeof(model_num));
 	s = &model_num[0];
 	len = strnlen(s, sizeof(model_num));

diff -u --recursive --new-file --exclude='.*' linux-2.6.9-rc4/include/linux/ata.h linux/include/linux/ata.h
@@ -217,24 +217,24 @@
 	u8			command;	/* IO operation */
 };
 
-#define ata_id_is_ata(dev)	(((dev)->id[0] & (1 << 15)) == 0)
-#define ata_id_rahead_enabled(dev) ((dev)->id[85] & (1 << 6))
-#define ata_id_wcache_enabled(dev) ((dev)->id[85] & (1 << 5))
-#define ata_id_has_flush(dev) ((dev)->id[83] & (1 << 12))
-#define ata_id_has_flush_ext(dev) ((dev)->id[83] & (1 << 13))
-#define ata_id_has_lba48(dev)	((dev)->id[83] & (1 << 10))
-#define ata_id_has_wcache(dev)	((dev)->id[82] & (1 << 5))
-#define ata_id_has_pm(dev)	((dev)->id[82] & (1 << 3))
-#define ata_id_has_lba(dev)	((dev)->id[49] & (1 << 9))
-#define ata_id_has_dma(dev)	((dev)->id[49] & (1 << 8))
-#define ata_id_removeable(dev)	((dev)->id[0] & (1 << 7))
-#define ata_id_u32(dev,n)	\
-	(((u32) (dev)->id[(n) + 1] << 16) | ((u32) (dev)->id[(n)]))
-#define ata_id_u64(dev,n)	\
-	( ((u64) dev->id[(n) + 3] << 48) |	\
-	  ((u64) dev->id[(n) + 2] << 32) |	\
-	  ((u64) dev->id[(n) + 1] << 16) |	\
-	  ((u64) dev->id[(n) + 0]) )
+#define ata_id_is_ata(id)	(((id)[0] & (1 << 15)) == 0)
+#define ata_id_rahead_enabled(id) ((id)[85] & (1 << 6))
+#define ata_id_wcache_enabled(id) ((id)[85] & (1 << 5))
+#define ata_id_has_flush(id) ((id)[83] & (1 << 12))
+#define ata_id_has_flush_ext(id) ((id)[83] & (1 << 13))
+#define ata_id_has_lba48(id)	((id)[83] & (1 << 10))
+#define ata_id_has_wcache(id)	((id)[82] & (1 << 5))
+#define ata_id_has_pm(id)	((id)[82] & (1 << 3))
+#define ata_id_has_lba(id)	((id)[49] & (1 << 9))
+#define ata_id_has_dma(id)	((id)[49] & (1 << 8))
+#define ata_id_removeable(id)	((id)[0] & (1 << 7))
+#define ata_id_u32(id,n)	\
+	(((u32) (id)[(n) + 1] << 16) | ((u32) (id)[(n)]))
+#define ata_id_u64(id,n)	\
+	( ((u64) (id)[(n) + 3] << 48) |	\
+	  ((u64) (id)[(n) + 2] << 32) |	\
+	  ((u64) (id)[(n) + 1] << 16) |	\
+	  ((u64) (id)[(n) + 0]) )
 
 static inline int atapi_cdb_len(u16 *dev_id)
 {

diff -u --recursive --new-file --exclude='.*' linux-2.6.9-rc4/include/linux/libata.h linux/include/linux/libata.h
@@ -403,7 +403,7 @@
 extern void ata_sg_init(struct ata_queued_cmd *qc, struct scatterlist *sg,
 		 unsigned int n_elem);
 extern unsigned int ata_dev_classify(struct ata_taskfile *tf);
-extern void ata_dev_id_string(struct ata_device *dev, unsigned char *s,
+extern void ata_dev_id_string(u16 *id, unsigned char *s,
 			      unsigned int ofs, unsigned int len);
 extern void ata_bmdma_setup (struct ata_queued_cmd *qc);
 extern void ata_bmdma_start (struct ata_queued_cmd *qc);

@@ -415,7 +415,9 @@
 			      struct block_device *bdev,
 			      sector_t capacity, int geom[]);
 extern int ata_scsi_slave_config(struct scsi_device *sdev);
-
+extern void ata_scsi_simulate(u16 *id,
+			      struct scsi_cmnd *cmd,
+			      void (*done)(struct scsi_cmnd *));
 
 static inline unsigned int ata_tag_valid(unsigned int tag)
 {

@@ -613,9 +615,9 @@
 
 static inline int ata_try_flush_cache(struct ata_device *dev)
 {
-	return ata_id_wcache_enabled(dev) ||
-	       ata_id_has_flush(dev) ||
-	       ata_id_has_flush_ext(dev);
+	return ata_id_wcache_enabled(dev->id) ||
+	       ata_id_has_flush(dev->id) ||
+	       ata_id_has_flush_ext(dev->id);
 }
 
 #endif /* __LINUX_LIBATA_H__ */

applied, but, you forgot rule #6:
http://linux.yyz.us/patch-format.html

Specifically, include the full description in each patch resend. Patch merging is largely automated by scripts these days, and failing to provide an adequate description means manual intervention is required.

The full body of your email is pasted into the BitKeeper changeset description.

Jeff

Jeff,

Andrews "The perfect patch"
(http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt) in section 3.e says:

Most people's patch receiving scripts will treat a ^--- string as the separator between the changelog and the patch itself. You can use this to ensure that any diffstat information is discarded when the patch is applied:

Do your scripts act this way as well?

It is nice to be able to send a single e-mail both w/ changelog-appropriate comments and with "this relates to the last message" comments as well...

John

P.S. Hopefully I didn't misunderstand Andrew...

John W. Linville
[email protected]

Jeff, are your scripts available somewhere (for the rest of us)?

Thanks,

~Randy

I use Linus's scripts, http://bktools.bkbits.net/

Jeff