[PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Mark Lord]

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

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.

Signed-off-by: Mark Lord <mlord@pobox.com>
-- 
Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

[-- Attachment #2: qstor-04oct04.patch.gz --]
[-- Type: application/x-gzip, Size: 37269 bytes --]

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Mark Lord]

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")

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Christoph Hellwig]

On Thu, Oct 07, 2004 at 09:42:14AM -0400, Mark Lord wrote:
> Okay, last call for comment on this driver.
> 
> Any other issues before initial kernel inclusion?

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.

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Mark Lord]

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")

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Jeff Garzik]

Mark Lord wrote:
> 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.

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

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Mark Lord]

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

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")

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Jeff Garzik]

Mark Lord wrote:
> Jeff Garzik wrote:
> 
>>
>> We don't add hooks on the _hope_ that _future_ code will (a) use the 
>> hooks and (b) be GPL'd.
> 
> 
> 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.

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)


> 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.

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.


> 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.

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

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Mark Lord]

Jeff Garzik wrote:

> 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.

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")

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Jeff Garzik]

Mark Lord wrote:
> Jeff Garzik wrote:
> 
>> 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.
> 
> 
> 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.

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

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Mark Lord]

 >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  ;-)

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")

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Christoph Hellwig]

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

 - 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.

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Mark Lord]

(Christoph Hellwig wrote:
 >
>  - the !dev case in qs_scsi_queuecomman can't happen

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.

>  - never mess with eh_timeout from inside a driver

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

>  - please don't implemente the HDIO_ ioctls, Jeff said this can
>    be done via SG_IO

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?

>  - if ->info return a static string you can just store it into ->name

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

>  - please use the kernel/kthread.c interface for your kernel thread

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")

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Christoph Hellwig]

On Thu, Oct 07, 2004 at 06:03:36PM -0400, Mark Lord wrote:
> (Christoph Hellwig wrote:
>  >
> >  - the !dev case in qs_scsi_queuecomman can't happen
> 
> 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.

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.

> >  - never mess with eh_timeout from inside a driver
> 
> Give us an interface for it, please.
> In the meanwhile, gone!

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

> >  - please don't implemente the HDIO_ ioctls, Jeff said this can
> >    be done via SG_IO
> 
> 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 please fixup userspace.  New hardware will require new system tools
once in a while.

> >  - if ->info return a static string you can just store it into ->name
> 
> So just nuke the _info() proc, and use .name = QS_DESC ?
> Okay, done.

Yes.

PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Mark Lord]

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

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

Signed-off-by: Mark Lord <mlord@pobox.com>

Thanks guys.
-- 
Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

[-- Attachment #2: qstor-07oct04.patch.gz --]
[-- Type: application/x-gzip, Size: 36855 bytes --]

[PATCH] QStor SATA/RAID driver for 2.6.9-rc4

[Mark Lord]

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

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.

Signed-off-by: Mark Lord <mlord@pobox.com>

Bug reports are appreciated.  Thanks guys.
-- 
Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

[-- Attachment #2: qstor-13oct04.patch.gz --]
[-- Type: application/x-gzip, Size: 34449 bytes --]

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[James Bottomley]

On Thu, 2004-10-07 at 16:15, Christoph Hellwig wrote:
>  - please use the kernel/kthread.c interface for your kernel thread

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

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Mark Lord]

James Bottomley wrote:
>
> 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.

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.

> 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

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.

> - 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.

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")

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[James Bottomley]

On Fri, 2004-10-08 at 10:15, Mark Lord wrote:
> > 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.
> 
> 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.

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

> > scsi_done() doesn't require a lock
> 
> Really?  I wonder why the mid-layer is so religious about
> doing the lock around every invocation of it today?

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.

> > - 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.
> 
> 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.

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

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

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

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Mark Lord]

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

Okay, done.

>>>scsi_done() doesn't require a lock
>>
>>Really?  I wonder why the mid-layer is so religious about
>>doing the lock around every invocation of it today?
> 
> It's not if you look at other drivers.

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.

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

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.

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

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")

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Jeff Garzik]

Mark Lord wrote:
> 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.


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

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[James Bottomley]

On Fri, 2004-10-08 at 10:34, Mark Lord wrote:
> If those locks are not needed, the scsi.c maintainer really should
> nuke'em.

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

> > Really, I suppose, libata should provide the interfaces for doing this
> > work for emulated commands.
> 
> 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.

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

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Mark Lord]

James Bottomley wrote:
> On Fri, 2004-10-08 at 10:34, Mark Lord wrote:
> 
>>If those locks are not needed, the scsi.c maintainer really should
>>nuke'em.
> 
> I think you can safely assume he has more important things to do.

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")

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Jeff Garzik]

On Tue, Oct 12, 2004 at 01:00:53PM -0400, Mark Lord wrote:
> James Bottomley wrote:
> >On Fri, 2004-10-08 at 10:34, Mark Lord wrote:
> >
> >>If those locks are not needed, the scsi.c maintainer really should
> >>nuke'em.
> >
> >I think you can safely assume he has more important things to do.
> 
> 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.


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

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[James Bottomley]

On Tue, 2004-10-12 at 12:05, Jeff Garzik wrote:
> 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.

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

James

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Jeff Garzik]

James Bottomley wrote:
> On Tue, 2004-10-12 at 12:05, Jeff Garzik wrote:
> 
>>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.
> 
> 
> So you'll be sending me the patches that do this?

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

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Mark Lord]

James Bottomley wrote:
> On Fri, 2004-10-08 at 10:15, Mark Lord wrote:
> 
>>>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.
>>
>>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.
> 
> 
> We use this already for other entities that require user context like
> domain validation.  It seems to work as advertised.

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")

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[James Bottomley]

On Fri, 2004-10-08 at 10:38, Mark Lord wrote:
> 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?
> 

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

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Jeff Garzik]

On Fri, Oct 08, 2004 at 10:47:40AM -0500, James Bottomley wrote:
> However, I assume you know you can't sleep in queuecommand since it may
> be run from the scsi tasklet?

Furthermore queuecommand is inside spin_lock_irqsave

	Jeff

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Mark Lord]

James Bottomley wrote:
> On Fri, 2004-10-08 at 10:38, Mark Lord wrote:
> 
>>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?
>>
> 
> 
> Since you wouldn't go straight from schedule_work->mid-layer, I assume
> you mean that when the workqueue thread runs the work?

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")

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Jeff Garzik]

On Tue, Oct 12, 2004 at 12:59:01PM -0400, Mark Lord wrote:
> James Bottomley wrote:
> >On Fri, 2004-10-08 at 10:38, Mark Lord wrote:
> >
> >>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?
> >>
> >
> >
> >Since you wouldn't go straight from schedule_work->mid-layer, I assume
> >you mean that when the workqueue thread runs the work?
> 
> 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.

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

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Mark Lord]

Jeff Garzik wrote:
>
>>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.
>
> The only schedule_work() call in the SCSI common code is for domain
> validation.

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")

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Jeff Garzik]

On Tue, Oct 12, 2004 at 01:14:01PM -0400, Mark Lord wrote:
> Jeff Garzik wrote:
> >
> >>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.
> >
> >The only schedule_work() call in the SCSI common code is for domain
> >validation.
> 
> 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?

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

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Jeff Garzik]

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

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[James Bottomley]

On Tue, 2004-10-12 at 11:59, Mark Lord wrote:
> 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?

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

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Mark Lord]

James Bottomley wrote:
 >
> 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.

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")

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[James Bottomley]

On Tue, 2004-10-12 at 12:22, Mark Lord wrote:
> 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.

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

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Mark Lord]

James Bottomley wrote:
>
> In that scenario, you use a separate workqueue.

Okay. But what thread should run that 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?

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")

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Jeff Garzik]

Mark Lord wrote:
> 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.


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

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Mark Lord]

Jeff Garzik wrote:
..
> The usual way to do what you want is either

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")

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[James Bottomley]

On Tue, 2004-10-12 at 12:51, Mark Lord 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.

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

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Mark Lord]

James Bottomley wrote:
..
> 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().

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")

driver hacking tips (was Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3)

[Jeff Garzik]

[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

Re: driver hacking tips (was Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3)

[Mark Lord]

 >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.

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")

Re: driver hacking tips (was Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3)

[Jeff Garzik]

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


flush_workqueue() [private workqueue] and flush_scheduled_work()

	Jeff

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Jeff Garzik]

Mark Lord wrote:
> James Bottomley wrote:
>  >
> 
>> 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.
> 
> 
> 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.

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.


> 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?

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

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Christoph Hellwig]

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

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Jeff Garzik]

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

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Mark Lord]

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")

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Jeff Garzik]

Mark Lord wrote:
> 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.

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

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Jeff Garzik]

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

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Jeff Garzik]

Mark Lord wrote:
> 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.

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

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Mark Lord]

 >>
 >> 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.
 >
 >
 >I recall this but don't see it in my inbox anymore... did I adequately respond?

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")

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Jeff Garzik]

Mark Lord wrote:
> 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.


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

	Jeff

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Mark Lord]

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

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")

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Jeff Garzik]

Mark Lord wrote:
> Jeff Garzik wrote:
> 
>> Fine with me but you'll need more than just the identify data, if you 
>> wanna do stuff like support MODE SELECT/SENSE.
> 
> 
> 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?

Seems sane, send a patch.

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

	Jeff

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Mark Lord]

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

 >Seems sane, send a patch.

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.

Signed-off-by: Mark Lord <mlord@pobox.com>
-- 
Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

[-- Attachment #2: libata_id.patch --]
[-- Type: text/plain, Size: 13842 bytes --]

diff -u --recursive --new-file --exclude='.*' linux-2.6.9-rc4/drivers/scsi/libata-core.c linux/drivers/scsi/libata-core.c
--- linux-2.6.9-rc4/drivers/scsi/libata-core.c	2004-10-13 14:47:26.000000000 -0400
+++ linux/drivers/scsi/libata-core.c	2004-10-14 12:04:48.000000000 -0400
@@ -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
--- linux-2.6.9-rc4/drivers/scsi/libata.h	2004-10-13 14:47:26.000000000 -0400
+++ linux/drivers/scsi/libata.h	2004-10-14 11:48:35.000000000 -0400
@@ -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/libata-scsi.c linux/drivers/scsi/libata-scsi.c
--- linux-2.6.9-rc4/drivers/scsi/libata-scsi.c	2004-10-13 14:47:26.000000000 -0400
+++ linux/drivers/scsi/libata-scsi.c	2004-10-14 12:06:49.000000000 -0400
@@ -34,7 +34,7 @@
 #include "libata.h"
 
 typedef unsigned int (*ata_xlat_func_t)(struct ata_queued_cmd *qc, u8 *scsicmd);
-static void ata_scsi_simulate(struct ata_port *ap, struct ata_device *dev,
+static void ata_scsi_simulate(u16 *id,
 			      struct scsi_cmnd *cmd,
 			      void (*done)(struct scsi_cmnd *));
 static struct ata_device *
@@ -411,7 +411,7 @@
 	tf->protocol = ATA_PROT_NODATA;
 
 	if ((tf->flags & ATA_TFLAG_LBA48) &&
-	    (ata_id_has_flush_ext(qc->dev)))
+	    (ata_id_has_flush_ext(qc->dev->id)))
 		tf->command = ATA_CMD_FLUSH_EXT;
 	else
 		tf->command = ATA_CMD_FLUSH;
@@ -758,7 +758,7 @@
 
 /**
  *	ata_scsi_rbuf_fill - wrapper for SCSI command simulators
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@actor: Callback hook for desired SCSI command simulator
  *
  *	Takes care of the hard work of simulating a SCSI command...
@@ -793,7 +793,7 @@
 
 /**
  *	ata_scsiop_inq_std - Simulate INQUIRY command
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
  *	@buflen: Response buffer length.
  *
@@ -807,8 +807,6 @@
 unsigned int ata_scsiop_inq_std(struct ata_scsi_args *args, u8 *rbuf,
 			       unsigned int buflen)
 {
-	struct ata_device *dev = args->dev;
-
 	u8 hdr[] = {
 		TYPE_DISK,
 		0,
@@ -818,7 +816,7 @@
 	};
 
 	/* set scsi removeable (RMB) bit per ata bit */
-	if (ata_id_removeable(dev))
+	if (ata_id_removeable(args->id))
 		hdr[1] |= (1 << 7);
 
 	VPRINTK("ENTER\n");
@@ -827,8 +825,8 @@
 
 	if (buflen > 35) {
 		memcpy(&rbuf[8], "ATA     ", 8);
-		ata_dev_id_string(dev, &rbuf[16], ATA_ID_PROD_OFS, 16);
-		ata_dev_id_string(dev, &rbuf[32], ATA_ID_FW_REV_OFS, 4);
+		ata_dev_id_string(args->id, &rbuf[16], ATA_ID_PROD_OFS, 16);
+		ata_dev_id_string(args->id, &rbuf[32], ATA_ID_FW_REV_OFS, 4);
 		if (rbuf[32] == 0 || rbuf[32] == ' ')
 			memcpy(&rbuf[32], "n/a ", 4);
 	}
@@ -852,7 +850,7 @@
 
 /**
  *	ata_scsiop_inq_00 - Simulate INQUIRY EVPD page 0, list of pages
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
  *	@buflen: Response buffer length.
  *
@@ -880,7 +878,7 @@
 
 /**
  *	ata_scsiop_inq_80 - Simulate INQUIRY EVPD page 80, device serial number
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
  *	@buflen: Response buffer length.
  *
@@ -902,7 +900,7 @@
 	memcpy(rbuf, hdr, sizeof(hdr));
 
 	if (buflen > (ATA_SERNO_LEN + 4))
-		ata_dev_id_string(args->dev, (unsigned char *) &rbuf[4],
+		ata_dev_id_string(args->id, (unsigned char *) &rbuf[4],
 				  ATA_ID_SERNO_OFS, ATA_SERNO_LEN);
 
 	return 0;
@@ -912,7 +910,7 @@
 
 /**
  *	ata_scsiop_inq_83 - Simulate INQUIRY EVPD page 83, device identity
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
  *	@buflen: Response buffer length.
  *
@@ -941,7 +939,7 @@
 
 /**
  *	ata_scsiop_noop -
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
  *	@buflen: Response buffer length.
  *
@@ -989,7 +987,7 @@
 
 /**
  *	ata_msense_caching - Simulate MODE SENSE caching info page
- *	@dev: Device associated with this MODE SENSE command
+ *	@id: device IDENTIFY data
  *	@ptr_io: (input/output) Location to store more output data
  *	@last: End of output data buffer
  *
@@ -1001,7 +999,7 @@
  *	None.
  */
 
-static unsigned int ata_msense_caching(struct ata_device *dev, u8 **ptr_io,
+static unsigned int ata_msense_caching(u16 *id, u8 **ptr_io,
 				       const u8 *last)
 {
 	u8 page[] = {
@@ -1011,9 +1009,9 @@
 		0, 0, 0, 0, 0, 0, 0, 0		/* 8 zeroes */
 	};
 
-	if (ata_id_wcache_enabled(dev))
+	if (ata_id_wcache_enabled(id))
 		page[2] |= (1 << 2);	/* write cache enable */
-	if (!ata_id_rahead_enabled(dev))
+	if (!ata_id_rahead_enabled(id))
 		page[12] |= (1 << 5);	/* disable read ahead */
 
 	ata_msense_push(ptr_io, last, page, sizeof(page));
@@ -1067,7 +1065,7 @@
 
 /**
  *	ata_scsiop_mode_sense - Simulate MODE SENSE 6, 10 commands
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
  *	@buflen: Response buffer length.
  *
@@ -1081,7 +1079,6 @@
 				  unsigned int buflen)
 {
 	u8 *scsicmd = args->cmd->cmnd, *p, *last;
-	struct ata_device *dev = args->dev;
 	unsigned int page_control, six_byte, output_len;
 
 	VPRINTK("ENTER\n");
@@ -1109,7 +1106,7 @@
 		break;
 
 	case 0x08:		/* caching */
-		output_len += ata_msense_caching(dev, &p, last);
+		output_len += ata_msense_caching(args->id, &p, last);
 		break;
 
 	case 0x0a: {		/* control mode */
@@ -1119,7 +1116,7 @@
 
 	case 0x3f:		/* all pages */
 		output_len += ata_msense_rw_recovery(&p, last);
-		output_len += ata_msense_caching(dev, &p, last);
+		output_len += ata_msense_caching(args->id, &p, last);
 		output_len += ata_msense_ctl_mode(&p, last);
 		break;
 
@@ -1141,7 +1138,7 @@
 
 /**
  *	ata_scsiop_read_cap - Simulate READ CAPACITY[ 16] commands
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
  *	@buflen: Response buffer length.
  *
@@ -1154,11 +1151,15 @@
 unsigned int ata_scsiop_read_cap(struct ata_scsi_args *args, u8 *rbuf,
 			        unsigned int buflen)
 {
-	u64 n_sectors = args->dev->n_sectors;
+	u64 n_sectors;
 	u32 tmp;
 
 	VPRINTK("ENTER\n");
 
+	if (ata_id_has_lba48(args->id))
+		n_sectors = ata_id_u64(args->id, 100);
+	else
+		n_sectors = ata_id_u32(args->id, 60);
 	n_sectors--;		/* ATA TotalUserSectors - 1 */
 
 	tmp = n_sectors;	/* note: truncates, if lba48 */
@@ -1196,7 +1197,7 @@
 
 /**
  *	ata_scsiop_report_luns - Simulate REPORT LUNS command
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
  *	@buflen: Response buffer length.
  *
@@ -1472,7 +1473,7 @@
 		if (xlat_func)
 			ata_scsi_translate(ap, dev, cmd, done, xlat_func);
 		else
-			ata_scsi_simulate(ap, dev, cmd, done);
+			ata_scsi_simulate(dev->id, cmd, done);
 	} else
 		ata_scsi_translate(ap, dev, cmd, done, atapi_xlat);
 
@@ -1482,8 +1483,7 @@
 
 /**
  *	ata_scsi_simulate - simulate SCSI command on ATA device
- *	@ap: Port to which ATA device is attached.
- *	@dev: Target device for CDB.
+ *	@id: current IDENTIFY data for target device.
  *	@cmd: SCSI command being sent to device.
  *	@done: SCSI command completion function.
  *
@@ -1494,15 +1494,14 @@
  *	spin_lock_irqsave(host_set lock)
  */
 
-static void ata_scsi_simulate(struct ata_port *ap, struct ata_device *dev,
+static void ata_scsi_simulate(u16 *id,
 			      struct scsi_cmnd *cmd,
 			      void (*done)(struct scsi_cmnd *))
 {
 	struct ata_scsi_args args;
 	u8 *scsicmd = cmd->cmnd;
 
-	args.ap = ap;
-	args.dev = dev;
+	args.id = id;
 	args.cmd = cmd;
 	args.done = done;
 
diff -u --recursive --new-file --exclude='.*' linux-2.6.9-rc4/drivers/scsi/sata_sil.c linux/drivers/scsi/sata_sil.c
--- linux-2.6.9-rc4/drivers/scsi/sata_sil.c	2004-10-13 14:47:26.000000000 -0400
+++ linux/drivers/scsi/sata_sil.c	2004-10-14 12:05:23.000000000 -0400
@@ -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
--- linux-2.6.9-rc4/include/linux/ata.h	2004-10-13 14:47:31.000000000 -0400
+++ linux/include/linux/ata.h	2004-10-14 11:51:26.000000000 -0400
@@ -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
--- linux-2.6.9-rc4/include/linux/libata.h	2004-10-13 14:47:31.000000000 -0400
+++ linux/include/linux/libata.h	2004-10-14 12:05:52.000000000 -0400
@@ -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__ */

Re: [PATCH] QStor SATA/RAID driver for 2.6.9-rc3

[Mark Lord]

Oh crap.. forgot to add the EXPORT..

Re-issue of this patch coming momentarily.
-- 
Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

[PATCH] Export ata_scsi_simulate() for use by non-libata drivers

[Mark Lord]

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

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().

Signed-off-by: Mark Lord <mlord@pobox.com>
-- 
Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

[-- Attachment #2: libata_id2.patch --]
[-- Type: text/plain, Size: 14503 bytes --]

diff -u --recursive --new-file --exclude='.*' linux-2.6.9-rc4/drivers/scsi/libata-core.c linux/drivers/scsi/libata-core.c
--- linux-2.6.9-rc4/drivers/scsi/libata-core.c	2004-10-13 14:47:26.000000000 -0400
+++ linux/drivers/scsi/libata-core.c	2004-10-14 12:50:16.000000000 -0400
@@ -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
--- linux-2.6.9-rc4/drivers/scsi/libata.h	2004-10-13 14:47:26.000000000 -0400
+++ linux/drivers/scsi/libata.h	2004-10-14 11:48:35.000000000 -0400
@@ -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/libata-scsi.c linux/drivers/scsi/libata-scsi.c
--- linux-2.6.9-rc4/drivers/scsi/libata-scsi.c	2004-10-13 14:47:26.000000000 -0400
+++ linux/drivers/scsi/libata-scsi.c	2004-10-14 12:40:57.000000000 -0400
@@ -34,9 +34,9 @@
 #include "libata.h"
 
 typedef unsigned int (*ata_xlat_func_t)(struct ata_queued_cmd *qc, u8 *scsicmd);
-static void ata_scsi_simulate(struct ata_port *ap, struct ata_device *dev,
-			      struct scsi_cmnd *cmd,
-			      void (*done)(struct scsi_cmnd *));
+void ata_scsi_simulate(u16 *id,
+		      struct scsi_cmnd *cmd,
+		      void (*done)(struct scsi_cmnd *));
 static struct ata_device *
 ata_scsi_find_dev(struct ata_port *ap, struct scsi_device *scsidev);
 
@@ -411,7 +411,7 @@
 	tf->protocol = ATA_PROT_NODATA;
 
 	if ((tf->flags & ATA_TFLAG_LBA48) &&
-	    (ata_id_has_flush_ext(qc->dev)))
+	    (ata_id_has_flush_ext(qc->dev->id)))
 		tf->command = ATA_CMD_FLUSH_EXT;
 	else
 		tf->command = ATA_CMD_FLUSH;
@@ -758,7 +758,7 @@
 
 /**
  *	ata_scsi_rbuf_fill - wrapper for SCSI command simulators
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@actor: Callback hook for desired SCSI command simulator
  *
  *	Takes care of the hard work of simulating a SCSI command...
@@ -793,7 +793,7 @@
 
 /**
  *	ata_scsiop_inq_std - Simulate INQUIRY command
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
  *	@buflen: Response buffer length.
  *
@@ -807,8 +807,6 @@
 unsigned int ata_scsiop_inq_std(struct ata_scsi_args *args, u8 *rbuf,
 			       unsigned int buflen)
 {
-	struct ata_device *dev = args->dev;
-
 	u8 hdr[] = {
 		TYPE_DISK,
 		0,
@@ -818,7 +816,7 @@
 	};
 
 	/* set scsi removeable (RMB) bit per ata bit */
-	if (ata_id_removeable(dev))
+	if (ata_id_removeable(args->id))
 		hdr[1] |= (1 << 7);
 
 	VPRINTK("ENTER\n");
@@ -827,8 +825,8 @@
 
 	if (buflen > 35) {
 		memcpy(&rbuf[8], "ATA     ", 8);
-		ata_dev_id_string(dev, &rbuf[16], ATA_ID_PROD_OFS, 16);
-		ata_dev_id_string(dev, &rbuf[32], ATA_ID_FW_REV_OFS, 4);
+		ata_dev_id_string(args->id, &rbuf[16], ATA_ID_PROD_OFS, 16);
+		ata_dev_id_string(args->id, &rbuf[32], ATA_ID_FW_REV_OFS, 4);
 		if (rbuf[32] == 0 || rbuf[32] == ' ')
 			memcpy(&rbuf[32], "n/a ", 4);
 	}
@@ -852,7 +850,7 @@
 
 /**
  *	ata_scsiop_inq_00 - Simulate INQUIRY EVPD page 0, list of pages
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
  *	@buflen: Response buffer length.
  *
@@ -880,7 +878,7 @@
 
 /**
  *	ata_scsiop_inq_80 - Simulate INQUIRY EVPD page 80, device serial number
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
  *	@buflen: Response buffer length.
  *
@@ -902,7 +900,7 @@
 	memcpy(rbuf, hdr, sizeof(hdr));
 
 	if (buflen > (ATA_SERNO_LEN + 4))
-		ata_dev_id_string(args->dev, (unsigned char *) &rbuf[4],
+		ata_dev_id_string(args->id, (unsigned char *) &rbuf[4],
 				  ATA_ID_SERNO_OFS, ATA_SERNO_LEN);
 
 	return 0;
@@ -912,7 +910,7 @@
 
 /**
  *	ata_scsiop_inq_83 - Simulate INQUIRY EVPD page 83, device identity
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
  *	@buflen: Response buffer length.
  *
@@ -941,7 +939,7 @@
 
 /**
  *	ata_scsiop_noop -
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
  *	@buflen: Response buffer length.
  *
@@ -989,7 +987,7 @@
 
 /**
  *	ata_msense_caching - Simulate MODE SENSE caching info page
- *	@dev: Device associated with this MODE SENSE command
+ *	@id: device IDENTIFY data
  *	@ptr_io: (input/output) Location to store more output data
  *	@last: End of output data buffer
  *
@@ -1001,7 +999,7 @@
  *	None.
  */
 
-static unsigned int ata_msense_caching(struct ata_device *dev, u8 **ptr_io,
+static unsigned int ata_msense_caching(u16 *id, u8 **ptr_io,
 				       const u8 *last)
 {
 	u8 page[] = {
@@ -1011,9 +1009,9 @@
 		0, 0, 0, 0, 0, 0, 0, 0		/* 8 zeroes */
 	};
 
-	if (ata_id_wcache_enabled(dev))
+	if (ata_id_wcache_enabled(id))
 		page[2] |= (1 << 2);	/* write cache enable */
-	if (!ata_id_rahead_enabled(dev))
+	if (!ata_id_rahead_enabled(id))
 		page[12] |= (1 << 5);	/* disable read ahead */
 
 	ata_msense_push(ptr_io, last, page, sizeof(page));
@@ -1067,7 +1065,7 @@
 
 /**
  *	ata_scsiop_mode_sense - Simulate MODE SENSE 6, 10 commands
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
  *	@buflen: Response buffer length.
  *
@@ -1081,7 +1079,6 @@
 				  unsigned int buflen)
 {
 	u8 *scsicmd = args->cmd->cmnd, *p, *last;
-	struct ata_device *dev = args->dev;
 	unsigned int page_control, six_byte, output_len;
 
 	VPRINTK("ENTER\n");
@@ -1109,7 +1106,7 @@
 		break;
 
 	case 0x08:		/* caching */
-		output_len += ata_msense_caching(dev, &p, last);
+		output_len += ata_msense_caching(args->id, &p, last);
 		break;
 
 	case 0x0a: {		/* control mode */
@@ -1119,7 +1116,7 @@
 
 	case 0x3f:		/* all pages */
 		output_len += ata_msense_rw_recovery(&p, last);
-		output_len += ata_msense_caching(dev, &p, last);
+		output_len += ata_msense_caching(args->id, &p, last);
 		output_len += ata_msense_ctl_mode(&p, last);
 		break;
 
@@ -1141,7 +1138,7 @@
 
 /**
  *	ata_scsiop_read_cap - Simulate READ CAPACITY[ 16] commands
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
  *	@buflen: Response buffer length.
  *
@@ -1154,11 +1151,15 @@
 unsigned int ata_scsiop_read_cap(struct ata_scsi_args *args, u8 *rbuf,
 			        unsigned int buflen)
 {
-	u64 n_sectors = args->dev->n_sectors;
+	u64 n_sectors;
 	u32 tmp;
 
 	VPRINTK("ENTER\n");
 
+	if (ata_id_has_lba48(args->id))
+		n_sectors = ata_id_u64(args->id, 100);
+	else
+		n_sectors = ata_id_u32(args->id, 60);
 	n_sectors--;		/* ATA TotalUserSectors - 1 */
 
 	tmp = n_sectors;	/* note: truncates, if lba48 */
@@ -1196,7 +1197,7 @@
 
 /**
  *	ata_scsiop_report_luns - Simulate REPORT LUNS command
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
  *	@buflen: Response buffer length.
  *
@@ -1472,7 +1473,7 @@
 		if (xlat_func)
 			ata_scsi_translate(ap, dev, cmd, done, xlat_func);
 		else
-			ata_scsi_simulate(ap, dev, cmd, done);
+			ata_scsi_simulate(dev->id, cmd, done);
 	} else
 		ata_scsi_translate(ap, dev, cmd, done, atapi_xlat);
 
@@ -1482,8 +1483,7 @@
 
 /**
  *	ata_scsi_simulate - simulate SCSI command on ATA device
- *	@ap: Port to which ATA device is attached.
- *	@dev: Target device for CDB.
+ *	@id: current IDENTIFY data for target device.
  *	@cmd: SCSI command being sent to device.
  *	@done: SCSI command completion function.
  *
@@ -1494,15 +1494,14 @@
  *	spin_lock_irqsave(host_set lock)
  */
 
-static void ata_scsi_simulate(struct ata_port *ap, struct ata_device *dev,
-			      struct scsi_cmnd *cmd,
-			      void (*done)(struct scsi_cmnd *))
+void ata_scsi_simulate(u16 *id,
+		      struct scsi_cmnd *cmd,
+		      void (*done)(struct scsi_cmnd *))
 {
 	struct ata_scsi_args args;
 	u8 *scsicmd = cmd->cmnd;
 
-	args.ap = ap;
-	args.dev = dev;
+	args.id = id;
 	args.cmd = cmd;
 	args.done = done;
 
diff -u --recursive --new-file --exclude='.*' linux-2.6.9-rc4/drivers/scsi/sata_sil.c linux/drivers/scsi/sata_sil.c
--- linux-2.6.9-rc4/drivers/scsi/sata_sil.c	2004-10-13 14:47:26.000000000 -0400
+++ linux/drivers/scsi/sata_sil.c	2004-10-14 12:05:23.000000000 -0400
@@ -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
--- linux-2.6.9-rc4/include/linux/ata.h	2004-10-13 14:47:31.000000000 -0400
+++ linux/include/linux/ata.h	2004-10-14 11:51:26.000000000 -0400
@@ -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
--- linux-2.6.9-rc4/include/linux/libata.h	2004-10-13 14:47:31.000000000 -0400
+++ linux/include/linux/libata.h	2004-10-14 12:05:52.000000000 -0400
@@ -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__ */

Re: [PATCH] Export ata_scsi_simulate() for use by non-libata drivers

[Jeff Garzik]

Mark Lord wrote:
> +void ata_scsi_simulate(u16 *id,
> +		      struct scsi_cmnd *cmd,
> +		      void (*done)(struct scsi_cmnd *));

That you are defining a public function in multiple files should be a 
hint that something is still missing...  :)  Put a prototype in 
linux/libata.h just like the other public functions, and the patch will 
be OK.

	Jeff

Re: [PATCH] Export ata_scsi_simulate() for use by non-libata drivers

[Mark Lord]

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

 >Put a prototype in linux/libata.h

Done.  Updated patch attached.

Signed-off-by: Mark Lord <mlord@pobox.com>
-- 
Mark Lord
(hdparm keeper & the original "Linux IDE Guy")

[-- Attachment #2: libata_id3.patch --]
[-- Type: text/plain, Size: 14460 bytes --]

diff -u --recursive --new-file --exclude='.*' linux-2.6.9-rc4/drivers/scsi/libata-core.c linux/drivers/scsi/libata-core.c
--- linux-2.6.9-rc4/drivers/scsi/libata-core.c	2004-10-13 14:47:26.000000000 -0400
+++ linux/drivers/scsi/libata-core.c	2004-10-14 14:33:54.000000000 -0400
@@ -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
--- linux-2.6.9-rc4/drivers/scsi/libata.h	2004-10-13 14:47:26.000000000 -0400
+++ linux/drivers/scsi/libata.h	2004-10-14 11:48:35.000000000 -0400
@@ -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/libata-scsi.c linux/drivers/scsi/libata-scsi.c
--- linux-2.6.9-rc4/drivers/scsi/libata-scsi.c	2004-10-13 14:47:26.000000000 -0400
+++ linux/drivers/scsi/libata-scsi.c	2004-10-14 14:36:45.000000000 -0400
@@ -34,9 +34,6 @@
 #include "libata.h"
 
 typedef unsigned int (*ata_xlat_func_t)(struct ata_queued_cmd *qc, u8 *scsicmd);
-static void ata_scsi_simulate(struct ata_port *ap, struct ata_device *dev,
-			      struct scsi_cmnd *cmd,
-			      void (*done)(struct scsi_cmnd *));
 static struct ata_device *
 ata_scsi_find_dev(struct ata_port *ap, struct scsi_device *scsidev);
 
@@ -411,7 +408,7 @@
 	tf->protocol = ATA_PROT_NODATA;
 
 	if ((tf->flags & ATA_TFLAG_LBA48) &&
-	    (ata_id_has_flush_ext(qc->dev)))
+	    (ata_id_has_flush_ext(qc->dev->id)))
 		tf->command = ATA_CMD_FLUSH_EXT;
 	else
 		tf->command = ATA_CMD_FLUSH;
@@ -758,7 +755,7 @@
 
 /**
  *	ata_scsi_rbuf_fill - wrapper for SCSI command simulators
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@actor: Callback hook for desired SCSI command simulator
  *
  *	Takes care of the hard work of simulating a SCSI command...
@@ -793,7 +790,7 @@
 
 /**
  *	ata_scsiop_inq_std - Simulate INQUIRY command
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
  *	@buflen: Response buffer length.
  *
@@ -807,8 +804,6 @@
 unsigned int ata_scsiop_inq_std(struct ata_scsi_args *args, u8 *rbuf,
 			       unsigned int buflen)
 {
-	struct ata_device *dev = args->dev;
-
 	u8 hdr[] = {
 		TYPE_DISK,
 		0,
@@ -818,7 +813,7 @@
 	};
 
 	/* set scsi removeable (RMB) bit per ata bit */
-	if (ata_id_removeable(dev))
+	if (ata_id_removeable(args->id))
 		hdr[1] |= (1 << 7);
 
 	VPRINTK("ENTER\n");
@@ -827,8 +822,8 @@
 
 	if (buflen > 35) {
 		memcpy(&rbuf[8], "ATA     ", 8);
-		ata_dev_id_string(dev, &rbuf[16], ATA_ID_PROD_OFS, 16);
-		ata_dev_id_string(dev, &rbuf[32], ATA_ID_FW_REV_OFS, 4);
+		ata_dev_id_string(args->id, &rbuf[16], ATA_ID_PROD_OFS, 16);
+		ata_dev_id_string(args->id, &rbuf[32], ATA_ID_FW_REV_OFS, 4);
 		if (rbuf[32] == 0 || rbuf[32] == ' ')
 			memcpy(&rbuf[32], "n/a ", 4);
 	}
@@ -852,7 +847,7 @@
 
 /**
  *	ata_scsiop_inq_00 - Simulate INQUIRY EVPD page 0, list of pages
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
  *	@buflen: Response buffer length.
  *
@@ -880,7 +875,7 @@
 
 /**
  *	ata_scsiop_inq_80 - Simulate INQUIRY EVPD page 80, device serial number
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
  *	@buflen: Response buffer length.
  *
@@ -902,7 +897,7 @@
 	memcpy(rbuf, hdr, sizeof(hdr));
 
 	if (buflen > (ATA_SERNO_LEN + 4))
-		ata_dev_id_string(args->dev, (unsigned char *) &rbuf[4],
+		ata_dev_id_string(args->id, (unsigned char *) &rbuf[4],
 				  ATA_ID_SERNO_OFS, ATA_SERNO_LEN);
 
 	return 0;
@@ -912,7 +907,7 @@
 
 /**
  *	ata_scsiop_inq_83 - Simulate INQUIRY EVPD page 83, device identity
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
  *	@buflen: Response buffer length.
  *
@@ -941,7 +936,7 @@
 
 /**
  *	ata_scsiop_noop -
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
  *	@buflen: Response buffer length.
  *
@@ -989,7 +984,7 @@
 
 /**
  *	ata_msense_caching - Simulate MODE SENSE caching info page
- *	@dev: Device associated with this MODE SENSE command
+ *	@id: device IDENTIFY data
  *	@ptr_io: (input/output) Location to store more output data
  *	@last: End of output data buffer
  *
@@ -1001,7 +996,7 @@
  *	None.
  */
 
-static unsigned int ata_msense_caching(struct ata_device *dev, u8 **ptr_io,
+static unsigned int ata_msense_caching(u16 *id, u8 **ptr_io,
 				       const u8 *last)
 {
 	u8 page[] = {
@@ -1011,9 +1006,9 @@
 		0, 0, 0, 0, 0, 0, 0, 0		/* 8 zeroes */
 	};
 
-	if (ata_id_wcache_enabled(dev))
+	if (ata_id_wcache_enabled(id))
 		page[2] |= (1 << 2);	/* write cache enable */
-	if (!ata_id_rahead_enabled(dev))
+	if (!ata_id_rahead_enabled(id))
 		page[12] |= (1 << 5);	/* disable read ahead */
 
 	ata_msense_push(ptr_io, last, page, sizeof(page));
@@ -1067,7 +1062,7 @@
 
 /**
  *	ata_scsiop_mode_sense - Simulate MODE SENSE 6, 10 commands
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
  *	@buflen: Response buffer length.
  *
@@ -1081,7 +1076,6 @@
 				  unsigned int buflen)
 {
 	u8 *scsicmd = args->cmd->cmnd, *p, *last;
-	struct ata_device *dev = args->dev;
 	unsigned int page_control, six_byte, output_len;
 
 	VPRINTK("ENTER\n");
@@ -1109,7 +1103,7 @@
 		break;
 
 	case 0x08:		/* caching */
-		output_len += ata_msense_caching(dev, &p, last);
+		output_len += ata_msense_caching(args->id, &p, last);
 		break;
 
 	case 0x0a: {		/* control mode */
@@ -1119,7 +1113,7 @@
 
 	case 0x3f:		/* all pages */
 		output_len += ata_msense_rw_recovery(&p, last);
-		output_len += ata_msense_caching(dev, &p, last);
+		output_len += ata_msense_caching(args->id, &p, last);
 		output_len += ata_msense_ctl_mode(&p, last);
 		break;
 
@@ -1141,7 +1135,7 @@
 
 /**
  *	ata_scsiop_read_cap - Simulate READ CAPACITY[ 16] commands
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
  *	@buflen: Response buffer length.
  *
@@ -1154,11 +1148,15 @@
 unsigned int ata_scsiop_read_cap(struct ata_scsi_args *args, u8 *rbuf,
 			        unsigned int buflen)
 {
-	u64 n_sectors = args->dev->n_sectors;
+	u64 n_sectors;
 	u32 tmp;
 
 	VPRINTK("ENTER\n");
 
+	if (ata_id_has_lba48(args->id))
+		n_sectors = ata_id_u64(args->id, 100);
+	else
+		n_sectors = ata_id_u32(args->id, 60);
 	n_sectors--;		/* ATA TotalUserSectors - 1 */
 
 	tmp = n_sectors;	/* note: truncates, if lba48 */
@@ -1196,7 +1194,7 @@
 
 /**
  *	ata_scsiop_report_luns - Simulate REPORT LUNS command
- *	@args: Port / device / SCSI command of interest.
+ *	@args: device IDENTIFY data / SCSI command of interest.
  *	@rbuf: Response buffer, to which simulated SCSI cmd output is sent.
  *	@buflen: Response buffer length.
  *
@@ -1472,7 +1470,7 @@
 		if (xlat_func)
 			ata_scsi_translate(ap, dev, cmd, done, xlat_func);
 		else
-			ata_scsi_simulate(ap, dev, cmd, done);
+			ata_scsi_simulate(dev->id, cmd, done);
 	} else
 		ata_scsi_translate(ap, dev, cmd, done, atapi_xlat);
 
@@ -1482,8 +1480,7 @@
 
 /**
  *	ata_scsi_simulate - simulate SCSI command on ATA device
- *	@ap: Port to which ATA device is attached.
- *	@dev: Target device for CDB.
+ *	@id: current IDENTIFY data for target device.
  *	@cmd: SCSI command being sent to device.
  *	@done: SCSI command completion function.
  *
@@ -1494,15 +1491,14 @@
  *	spin_lock_irqsave(host_set lock)
  */
 
-static void ata_scsi_simulate(struct ata_port *ap, struct ata_device *dev,
-			      struct scsi_cmnd *cmd,
-			      void (*done)(struct scsi_cmnd *))
+void ata_scsi_simulate(u16 *id,
+		      struct scsi_cmnd *cmd,
+		      void (*done)(struct scsi_cmnd *))
 {
 	struct ata_scsi_args args;
 	u8 *scsicmd = cmd->cmnd;
 
-	args.ap = ap;
-	args.dev = dev;
+	args.id = id;
 	args.cmd = cmd;
 	args.done = done;
 
diff -u --recursive --new-file --exclude='.*' linux-2.6.9-rc4/drivers/scsi/sata_sil.c linux/drivers/scsi/sata_sil.c
--- linux-2.6.9-rc4/drivers/scsi/sata_sil.c	2004-10-13 14:47:26.000000000 -0400
+++ linux/drivers/scsi/sata_sil.c	2004-10-14 12:05:23.000000000 -0400
@@ -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
--- linux-2.6.9-rc4/include/linux/ata.h	2004-10-13 14:47:31.000000000 -0400
+++ linux/include/linux/ata.h	2004-10-14 11:51:26.000000000 -0400
@@ -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
--- linux-2.6.9-rc4/include/linux/libata.h	2004-10-13 14:47:31.000000000 -0400
+++ linux/include/linux/libata.h	2004-10-14 14:35:41.000000000 -0400
@@ -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__ */

Re: [PATCH] Export ata_scsi_simulate() for use by non-libata drivers

[Jeff Garzik]

Mark Lord wrote:
>  >Put a prototype in linux/libata.h
> 
> Done.  Updated patch attached.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>


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

Re: [PATCH] Export ata_scsi_simulate() for use by non-libata drivers

[John W. Linville]

On Fri, Oct 15, 2004 at 01:04:50AM -0400, Jeff Garzik wrote:
> The full body of your email is pasted into the BitKeeper changeset 
> description.

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
linville@tuxdriver.com

Re: [PATCH] Export ata_scsi_simulate() for use by non-libata drivers

[Randy.Dunlap]

John W. Linville wrote:
> On Fri, Oct 15, 2004 at 01:04:50AM -0400, Jeff Garzik wrote:
> 
>>The full body of your email is pasted into the BitKeeper changeset 
>>description.
> 
> 
> 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?

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

> 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...


Thanks,
-- 
~Randy

Re: [PATCH] Export ata_scsi_simulate() for use by non-libata drivers

[Jeff Garzik]

Randy.Dunlap wrote:
> John W. Linville wrote:
> 
>> On Fri, Oct 15, 2004 at 01:04:50AM -0400, Jeff Garzik wrote:
>>
>>> The full body of your email is pasted into the BitKeeper changeset 
>>> description.
>>
>>
>>
>> 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?
> 
> 
> Jeff, are your scripts available somewhere (for the rest of us)?


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

	Jeff