RE: 2.6.6 + Adaptec I2O

RE: 2.6.6 + Adaptec I2O

[Salyzyn, Mark]

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

Please read CHANGELOG.dpt_i2o for change documentation.

Two changes recently posted by Christoph Hellwig have been incorporated
in this patch.

This patch has aimed towards 2.6 purity, and although the sources
maintained at Adaptec are unified (2.4 & 2.6), scripts were used to
remove the ifdef KERNEL_VERSION from this release. This source is in
synchronized with the patch on May 19th submitted for dpt_i2o in the 2.4
tree.

I look forward to comments regarding this patch.

Sincerely -- Mark Salyzyn

[-- Attachment #2: dpt_i2o-2.5.0-2333.patch.gz --]
[-- Type: application/x-gzip, Size: 34177 bytes --]

Re: 2.6.6 + Adaptec I2O

[Christoph Hellwig]

On Tue, Jun 01, 2004 at 11:11:22AM -0400, Salyzyn, Mark wrote:
> Please read CHANGELOG.dpt_i2o for change documentation.
> 
> Two changes recently posted by Christoph Hellwig have been incorporated
> in this patch.
> 
> This patch has aimed towards 2.6 purity, and although the sources
> maintained at Adaptec are unified (2.4 & 2.6), scripts were used to
> remove the ifdef KERNEL_VERSION from this release. This source is in
> synchronized with the patch on May 19th submitted for dpt_i2o in the 2.4
> tree.
> 
> I look forward to comments regarding this patch.

Patch is mostly okay although it still won't win a beaty contest.

Two showstoppers though:

 - the spin_is_locked magic is really, really bogus.  Easiest way to
   avoid this is to have your own spinlock in adpt_hba and use
   scsi_assign_lock to make it the host_lock, too.  With a little
   magic you'll also get per-host locks in the various 2.4 vendor
   trees with that..
 - you're still trying to sneak DPT_TARGET_BUSY back in.

Please fix those two issues and the patch can go in.

Re: 2.6.6 + Adaptec I2O

[Christoph Hellwig]

On Tue, Jun 01, 2004 at 05:20:20PM +0200, Christoph Hellwig wrote:
> Two showstoppers though:
> 
>  - the spin_is_locked magic is really, really bogus.  Easiest way to
>    avoid this is to have your own spinlock in adpt_hba and use
>    scsi_assign_lock to make it the host_lock, too.  With a little
>    magic you'll also get per-host locks in the various 2.4 vendor
>    trees with that..
>  - you're still trying to sneak DPT_TARGET_BUSY back in.
> 
> Please fix those two issues and the patch can go in.

Ah, and one more:  README.dpti is misplaced in drivers/scsi/, it should
update Documentation/scsi/dpti.txt instead.

RE: 2.6.6 + Adaptec I2O

[Salyzyn, Mark]

As noted by you before, the spin_is_locked could probably be dropped
entirely. This was a bug noted in the 2.4 tree that migrated into the
code for the 2.6 tree. I believe this lock is held at this point in the
code.

*this* DPT_TARGET_BUSY did the bd_claim you indicated would be
appropriate ... the bus_for_each_dev() call could be dropped though, it
was simply wishful thinking that we could expand on that idea to get the
busy check functionality back.

Sincerely -- Mark Salyzyn

How about this post-patch (untested):

diff -Nru dpt_i2o.1/dpt_i2o.c dpt_i2o/dpt_i2o.c
--- dpt_i2o.1/dpt_i2o.c Tue Jun  1 07:22:27 2004
+++ dpt_i2o/dpt_i2o.c   Tue Jun  1 08:31:19 2004
@@ -1285,7 +1226,6 @@
        msg[2] |= 0x80000000 | ((u32)wait_data->id);
        timeout *= HZ;
        if((status = adpt_i2o_post_this(pHba, msg, len)) == 0){
-               spinlock_t * was_locked = (spinlock_t *)NULL;
                set_current_state(TASK_INTERRUPTIBLE);
                /*
                 *      We are called before the host & host lock has
been
@@ -1293,11 +1233,6 @@
                 * held. We need to free the lock, if held, before going
                 * to sleep.
                 */
-               if ((pHba->host != NULL) /* Sad */
-                && (spin_is_locked(pHba->host->host_lock))) {
-                       was_locked = pHba->host->host_lock;
-                       spin_unlock_irq(was_locked);
+               spin_unlock_irq(pHba->host->host_lock);
-               }
                if(!timeout){
                        schedule();
                } else {
@@ -1309,8 +1244,6 @@
                                status = -ETIME;
                        }
                }
-               if (was_locked)
-                       spin_lock_irq(was_locked);
+               spin_lock_irq(pHba->host->host_lock);
                if (signal_pending(current)) {
                        printk("adpt_i2o_post_wait: interrupted\n");
                        status = -EINTR;
@@ -2067,10 +2000,6 @@

 #endif

-static int adpt_busy(struct device * dev, void * arg)
-{
-       return 0;
-}

 static int adpt_ioctl(struct inode *inode, struct file *file, uint cmd,
              ulong arg)
@@ -2162,12 +2091,6 @@
                if (bd_claim(inode->i_bdev, pHba))
                        busy.isBusy = 1;
-               else {
-                       extern struct bus_type scsi_bus_type;
-                       bd_release(inode->i_bdev);
-                       busy.isBusy = pHba->unit;
-                       busy.isBusy = bus_for_each_dev(&scsi_bus_type,
NULL, &bu
sy, adpt_busy);
-               }
                if (copy_to_user ((char*)arg, &busy, sizeof(busy))) {
                        return -EFAULT;
                }

-----Original Message-----
From: Christoph Hellwig [mailto:hch@lst.de] 
Sent: Tuesday, June 01, 2004 11:20 AM
To: Salyzyn, Mark
Cc: Andrew Morton; linux-scsi@vger.kernel.org; Leon Toh; Christoph
Hellwig
Subject: Re: 2.6.6 + Adaptec I2O

On Tue, Jun 01, 2004 at 11:11:22AM -0400, Salyzyn, Mark wrote:
> Please read CHANGELOG.dpt_i2o for change documentation.
> 
> Two changes recently posted by Christoph Hellwig have been
incorporated
> in this patch.
> 
> This patch has aimed towards 2.6 purity, and although the sources
> maintained at Adaptec are unified (2.4 & 2.6), scripts were used to
> remove the ifdef KERNEL_VERSION from this release. This source is in
> synchronized with the patch on May 19th submitted for dpt_i2o in the
2.4
> tree.
> 
> I look forward to comments regarding this patch.

Patch is mostly okay although it still won't win a beaty contest.

Two showstoppers though:

 - the spin_is_locked magic is really, really bogus.  Easiest way to
   avoid this is to have your own spinlock in adpt_hba and use
   scsi_assign_lock to make it the host_lock, too.  With a little
   magic you'll also get per-host locks in the various 2.4 vendor
   trees with that..
 - you're still trying to sneak DPT_TARGET_BUSY back in.

Please fix those two issues and the patch can go in.

Re: 2.6.6 + Adaptec I2O

[Christoph Hellwig]

On Tue, Jun 01, 2004 at 11:38:16AM -0400, Salyzyn, Mark wrote:
> *this* DPT_TARGET_BUSY did the bd_claim you indicated would be
> appropriate ... the bus_for_each_dev() call could be dropped though, it
> was simply wishful thinking that we could expand on that idea to get the
> busy check functionality back.

bd_claim/bd_release is paired for a reason.  So you must call bd_claim
before you start your exclusive operation and bd_release afterwards,
your combination just doesn't make sense.

In addition you're calling bd_claim on the inode of the managment
_character_ device.  I honestly can't see how you ever tested this as
it should immediately crash.

RE: 2.6.6 + Adaptec I2O

[Salyzyn, Mark]

No crash (lucky me ;-/ ), but the other fix is in jeopardy in testing,
the host structure is not defined at all calls to adpt_i2o_post_wait :-(
as the call is made before some initializations have been performed.

Here is an updated post-patch in the interest of turnaround.

Sincerely -- Mark Salyzyn

diff -Nru dpt_i2o.1/dpt_i2o.c dpt_i2o/dpt_i2o.c
--- dpt_i2o.1/dpt_i2o.c Tue Jun  1 07:22:27 2004
+++ dpt_i2o/dpt_i2o.c   Tue Jun  1 09:02:48 2004
@@ -1293,8 +1234,7 @@
                 * held. We need to free the lock, if held, before going
                 * to sleep.
                 */
-               if ((pHba->host != NULL) /* Sad */
-                && (spin_is_locked(pHba->host->host_lock))) {
+               if (pHba->host != NULL) { /* Sad */
                        was_locked = pHba->host->host_lock;
                        spin_unlock_irq(was_locked);
                }
@@ -2067,10 +2007,6 @@

 #endif

-static int adpt_busy(struct device * dev, void * arg)
-{
-       return 0;
-}

 static int adpt_ioctl(struct inode *inode, struct file *file, uint cmd,
              ulong arg)
@@ -2160,14 +2096,7 @@
                        return -EFAULT;
                }

-               if (bd_claim(inode->i_bdev, pHba))
-                       busy.isBusy = 1;
-               else {
-                       extern struct bus_type scsi_bus_type;
-                       bd_release(inode->i_bdev);
-                       busy.isBusy = pHba->unit;
-                       busy.isBusy = bus_for_each_dev(&scsi_bus_type,
NULL, &busy, adpt_busy);
-               }
+               busy.isBusy = 0;
                if (copy_to_user ((char*)arg, &busy, sizeof(busy))) {
                        return -EFAULT;
                }

-----Original Message-----
From: Christoph Hellwig [mailto:hch@lst.de] 
Sent: Tuesday, June 01, 2004 11:54 AM
To: Salyzyn, Mark
Cc: Christoph Hellwig; Andrew Morton; linux-scsi@vger.kernel.org; Leon
Toh
Subject: Re: 2.6.6 + Adaptec I2O

On Tue, Jun 01, 2004 at 11:38:16AM -0400, Salyzyn, Mark wrote:
> *this* DPT_TARGET_BUSY did the bd_claim you indicated would be
> appropriate ... the bus_for_each_dev() call could be dropped though,
it
> was simply wishful thinking that we could expand on that idea to get
the
> busy check functionality back.

bd_claim/bd_release is paired for a reason.  So you must call bd_claim
before you start your exclusive operation and bd_release afterwards,
your combination just doesn't make sense.

In addition you're calling bd_claim on the inode of the managment
_character_ device.  I honestly can't see how you ever tested this as
it should immediately crash.

Re: 2.6.6 + Adaptec I2O

[Andi Kleen]

"Salyzyn, Mark" <mark_salyzyn@adaptec.com> writes:

> +	- Added __x86_64__ ifdef for signature, reporting PROC_SEXIUM for
> +	  processor type.

PROC_QUINDECIUM would be more appropiate (both Nocona and Opteron use x86 
model 15) 

-Andi

RE: 2.6.6 + Adaptec I2O

[Salyzyn, Mark]

PROC_SEXIUM was reserved by our application software group when we added
PROC_PENTIUM. PROC_SEXIUM was simply a `followon' processor of a
different feather. The dptsig.h file actually is shared with (many)
other sources, so it carries an impact.

Branching this file, notwithstanding ... sooo ... if it is to change,
maybe it should be set to PROC_EM64T ...

Sincerely -- Mark Salyzyn

-----Original Message-----
From: Andi Kleen [mailto:ak@muc.de] 
Sent: Tuesday, June 01, 2004 1:09 PM
To: Salyzyn, Mark
Cc: linux-scsi@vger.kernel.org
Subject: Re: 2.6.6 + Adaptec I2O

"Salyzyn, Mark" <mark_salyzyn@adaptec.com> writes:

> +	- Added __x86_64__ ifdef for signature, reporting PROC_SEXIUM
for
> +	  processor type.

PROC_QUINDECIUM would be more appropiate (both Nocona and Opteron use
x86 
model 15) 

-Andi

Re: 2.6.6 + Adaptec I2O

[Andi Kleen]

"Salyzyn, Mark" <mark_salyzyn@adaptec.com> writes:

> PROC_SEXIUM was reserved by our application software group when we added
> PROC_PENTIUM. PROC_SEXIUM was simply a `followon' processor of a
> different feather. The dptsig.h file actually is shared with (many)
> other sources, so it carries an impact.
>
> Branching this file, notwithstanding ... sooo ... if it is to change,
> maybe it should be set to PROC_EM64T ...

Better call it PROC_X86_64 then. That's the standard name for the 
architecture on Linux.

What is this used for anyways? I'm not really sure why a driver should
know about the CPU type. How about just dropping it?

-Andi

RE: 2.6.6 + Adaptec I2O

[Salyzyn, Mark]

It's not the driver that needs to know, it's an information box on the
RAID applications that need to know via a management ioctl (see the
public domain raidutil tool for the cli variant). It's the `dptsig' tool
that reads the binary and provides version and processor type the binary
is compiled for.

Sincerely -- Mark Salyzyn

-----Original Message-----
From: Andi Kleen [mailto:ak@muc.de] 
Sent: Tuesday, June 01, 2004 2:21 PM
To: Salyzyn, Mark
Cc: linux-scsi@vger.kernel.org
Subject: Re: 2.6.6 + Adaptec I2O

"Salyzyn, Mark" <mark_salyzyn@adaptec.com> writes:

> PROC_SEXIUM was reserved by our application software group when we
added
> PROC_PENTIUM. PROC_SEXIUM was simply a `followon' processor of a
> different feather. The dptsig.h file actually is shared with (many)
> other sources, so it carries an impact.
>
> Branching this file, notwithstanding ... sooo ... if it is to change,
> maybe it should be set to PROC_EM64T ...

Better call it PROC_X86_64 then. That's the standard name for the 
architecture on Linux.

What is this used for anyways? I'm not really sure why a driver should
know about the CPU type. How about just dropping it?

-Andi

Re: 2.6.6 + Adaptec I2O

[Matthew Wilcox]

On Tue, Jun 01, 2004 at 02:27:44PM -0400, Salyzyn, Mark wrote:
> It's not the driver that needs to know, it's an information box on the
> RAID applications that need to know via a management ioctl (see the
> public domain raidutil tool for the cli variant). It's the `dptsig' tool
> that reads the binary and provides version and processor type the binary
> is compiled for.

Why can't it get this information via uname?

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

RE: 2.6.6 + Adaptec I2O

[Salyzyn, Mark]

There are two parts to the dptsig needs, one is a runtime need by the
management tools, and other is a binary inspection tool.

Dptsig inspects the module binary under any OS. It also inspects all of
the other dpt products for all other operating systems as well. It was
standard started in 1992 by DPT for their binaries. There is no running
linux kernel for this information to be acquired by the binary itself.
One could run the dptsig tool on windows and inspect a linux module
binary.

The management tools could (but doesn't for linux, this is for sake of
argument) report the driver was built for an i386 processor, yet we
could be running on an i686 kernel. Other drivers from DPT/Adaptec for
other operating systems have the same ioctl call populated with the same
information.

The management tools themselves contain a hierarchy of dptsig
structures, each reporting the purpose, the compile environment and the
runtime environment of each product in layers. For instance, the public
domain raidutil will report the driver, ioctl engine as well as the
raidutil's dptsig information.

Sincerely -- Mark Salyzyn

-----Original Message-----
From: willy@www.linux.org.uk [mailto:willy@www.linux.org.uk] On Behalf
Of Matthew Wilcox
Sent: Tuesday, June 01, 2004 2:29 PM
To: Salyzyn, Mark
Cc: Andi Kleen; linux-scsi@vger.kernel.org
Subject: Re: 2.6.6 + Adaptec I2O

On Tue, Jun 01, 2004 at 02:27:44PM -0400, Salyzyn, Mark wrote:
> It's not the driver that needs to know, it's an information box on the
> RAID applications that need to know via a management ioctl (see the
> public domain raidutil tool for the cli variant). It's the `dptsig'
tool
> that reads the binary and provides version and processor type the
binary
> is compiled for.

Why can't it get this information via uname?

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and
refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: 2.6.6 + Adaptec I2O

[Andi Kleen]

> The management tools could (but doesn't for linux, this is for sake of
> argument) report the driver was built for an i386 processor, yet we
> could be running on an i686 kernel. Other drivers from DPT/Adaptec for
> other operating systems have the same ioctl call populated with the same
> information.

Sounds useless to me. At least for linux you would need to report
a lot more than what you currently do to figure out if the driver
is compatible to a given kernel.

 e.g. how the kernel was built (with SMP, without, with PREEMPT etc., 
with what compiler etc.) See what the linux module loader checks.
In addition the distribution vendor may be needed.

Also PROC_SEXIUM is definitely wrong then since an x86-64 driver
is completely different from a i686 driver.

-Andi

RE: 2.6.6 + Adaptec I2O

[Salyzyn, Mark]

The driver reports that if it is an x86_64, that it is an Intel based
processor that is *not* an i686 driver. It replaces PROC_PENTIUM with
PROC_SEXIUM. Isn't this what you want? Or is it the *name* of this bit
that offends you?

>From the management tool's perspective, the x86-64 driver is identical
to the i686 driver without this bit of information. This driver responds
to 32 bit (legacy and/or remote) management application. The
applications are given the clue that the driver is different via the
PROC_SEXIUM dptsig information. In fact, if we did not touch adding the
bit, the applications would be tricked into thinking they are talking to
a pure i686 driver.

I believe you are reading far too much into the significance of
utilizing this `sub processor class' bit in the management ioctl
channel; and certainly into the significance of this change.

Chose to not make this change, then the management applications
blissfully are reporting incorrect information about the driver.

Sincerely -- Mark Salyzyn

-----Original Message-----
From: Andi Kleen [mailto:ak@muc.de] 
Sent: Tuesday, June 01, 2004 3:26 PM
To: Salyzyn, Mark
Cc: Matthew Wilcox; Andi Kleen; linux-scsi@vger.kernel.org
Subject: Re: 2.6.6 + Adaptec I2O

> The management tools could (but doesn't for linux, this is for sake of
> argument) report the driver was built for an i386 processor, yet we
> could be running on an i686 kernel. Other drivers from DPT/Adaptec for
> other operating systems have the same ioctl call populated with the
same
> information.

Sounds useless to me. At least for linux you would need to report
a lot more than what you currently do to figure out if the driver
is compatible to a given kernel.

 e.g. how the kernel was built (with SMP, without, with PREEMPT etc., 
with what compiler etc.) See what the linux module loader checks.
In addition the distribution vendor may be needed.

Also PROC_SEXIUM is definitely wrong then since an x86-64 driver
is completely different from a i686 driver.

-Andi

Re: 2.6.6 + Adaptec I2O

[Andi Kleen]

On Tue, Jun 01, 2004 at 03:52:26PM -0400, Salyzyn, Mark wrote:
> The driver reports that if it is an x86_64, that it is an Intel based
> processor that is *not* an i686 driver. It replaces PROC_PENTIUM with
> PROC_SEXIUM. Isn't this what you want? Or is it the *name* of this bit
> that offends you?

No, not at all. I like that bit.

I'm just objecting to having a driver do such useless things,
especially when it used to #error when the system type is 
unknown.

(I remember having to fix that for some drivers early in the
x86-64 port and I certainly cursed the inventor of these things
at that time)

But I will shut up on this issue now.

-Andi

Re: 2.6.6 + Adaptec I2O

[Leon Toh]

>>  Two showstoppers though:
>>
>>   - the spin_is_locked magic is really, really bogus.  Easiest way to
>>     avoid this is to have your own spinlock in adpt_hba and use
>>     scsi_assign_lock to make it the host_lock, too.  With a little
>>     magic you'll also get per-host locks in the various 2.4 vendor
>>     trees with that..
>>   - you're still trying to sneak DPT_TARGET_BUSY back in.
>>
>>  Please fix those two issues and the patch can go in.
>
>Ah, and one more:  README.dpti is misplaced in drivers/scsi/, it should
>update Documentation/scsi/dpti.txt instead.

Is there a placement recommendation of driver README files in 2.4 than ?  

Re: 2.6.6 + Adaptec I2O

[Christoph Hellwig]

> Is there a placement recommendation of driver README files in 2.4 than ?  

2.4 still had them in drivers/scsi/