LinuxLists.cc - DAC960 in 2.5.38, with new changes

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

Hi Dave,

>>>>> On Mon, 23 Sep 2002 12:04:00 -0700, Dave Olien <[email protected]> said:

Dave> #ifdef __ia64__
Dave> - writeq(Virtual_to_Bus64(CommandMailbox),
Dave> + writeq(CommandMailboxDMA,
Dave> ControllerBaseAddress + DAC960_LP_CommandMailboxBusAddressOffset);
Dave> #else
Dave> - writel(Virtual_to_Bus32(CommandMailbox),
Dave> + writel(CommandMailboxDMA,
Dave> ControllerBaseAddress + DAC960_LP_CommandMailboxBusAddressOffset);
Dave> #endif

This looks like a porting-nightmare in the making. There's got to be a
better way to determine whether you need a writeq() vs. a writel().

--david

2002-09-23 20:45:58

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

From: David Mosberger <[email protected]>
Date: Mon, 23 Sep 2002 12:19:02 -0700

This looks like a porting-nightmare in the making. There's got to be a
better way to determine whether you need a writeq() vs. a writel().

Or perhaps every platform should provide a writeq(), on 32-bit systems
it may merely be implemented as two consequetive writel() calls.

2002-09-23 20:48:11

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

>>>>> On Mon, 23 Sep 2002 13:40:00 -0700 (PDT), "David S. Miller" <[email protected]> said:

>> From: David Mosberger <[email protected]>
>> Date: Mon, 23 Sep 2002 12:19:02 -0700

>> This looks like a porting-nightmare in the making. There's got to be a
>> better way to determine whether you need a writeq() vs. a writel().

> Or perhaps every platform should provide a writeq(), on 32-bit systems
> it may merely be implemented as two consequetive writel() calls.

True, but I was wondering whether driver writers will have an implicit
assumption on readX/writeX being atomic. I don't think anyone ever
promised that, but I suspect all existing implementations are indeed
atomic (it's true even for old Alphas which don't have sub-word
load/stores).

--david

2002-09-23 21:00:44

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

From: Daniel Phillips <[email protected]>
Date: Mon, 23 Sep 2002 22:44:08 +0200

On Monday 23 September 2002 21:19, David Mosberger wrote:
> This looks like a porting-nightmare in the making. There's got to be a
> better way to determine whether you need a writeq() vs. a writel().

Even if an #ifdef is necessary here (and we are in trouble if it is) it
should not trigger on __ia64__, it should trigger on the size of (long).

There is nothing preventing a 32-bit long platform from being able
to store 64-bits at once to PCI space.

2002-09-23 21:03:02

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

From: David Mosberger <[email protected]>
Date: Mon, 23 Sep 2002 13:53:13 -0700

> Or perhaps every platform should provide a writeq(), on 32-bit systems
> it may merely be implemented as two consequetive writel() calls.

True, but I was wondering whether driver writers will have an implicit
assumption on readX/writeX being atomic. I don't think anyone ever
promised that, but I suspect all existing implementations are indeed
atomic (it's true even for old Alphas which don't have sub-word
load/stores).

On many platforms, two consequetive __raw_writel()'s might even
combine to an atomic 64-bit store to PCI space. :-)

I don't think the proposed 32-bit behavior is off the mark, and
anyways x86 can actually make the 64-bit store I believe if it
wants at least on more recent processors.

2002-09-23 20:33:41

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

On Monday 23 September 2002 21:04, Dave Olien wrote:
> Daniel, Jens
>
> Thanks for the feedback. Daniel, I've incorporated your patch into my
> driver changes, and moved to linux 2.5.38.
>
> Included are changes to put the memory mailbox and completion status
> mailboxes into dma mapped pages...

Applied, booted, up and running. I'll attempt to follow your work -
you are clearly way out in front of everybody in understanding this
driver.

Minor whitespace suggestion: don't worry too much about breaking up
lines to fit in 80 columns. It's nice where it works, but where it
just makes more lines, don't bother. We are going to go do spelling
patches to shorten a lot of those names anyway.

What test do you use to determine that some blocks are incorrectly
written?

> I made a decision to remove some behavior from the functions:
>
> DAC960_V1_EnableMemoryMailboxInterface
> DAC960_FinalizeController
>
> Following is a description of the behavior I have removed.
>
> In the original code, for the "LA" and "PG" controller types,
> the FinalizeController() function does NOT free the memory for the
> mailboxes for these controllers. Instead, the Finalize function calls
>
> DAC960_LA_SaveMemoryMailboxInfo
> or
> DAC960_PG_SaveMemoryMailboxInfo
>
> to instruct the controller to remember its memory mailbox addresses.
>
> The driver EnableMemoryMailboxInterface() function calls
>
> "DAC960_LA_RestoreMemoryMailboxInfo"
> or
> "DAC960_PG_RestoreMemoryMailboxInfo"
>
> functions to retrieve the controller's stored memory mailbox pointers.
>
> This behavior is "useful" only when the driver is unloaded, and then
> reloaded.
>
> I changed to code to just free the memory mailboxes during Finalize,
> and reallocate new ones when the driver is reloaded.
> I feel it is much better to release this resource when the driver
> isn't loaded.

I think you're right. It's hard to imagine why it would be good to
optimize the module unload/reload case for DAC, and if there is a
correctness argument, I personally would rather find out about it the
hard way than never know it.

> In 2.5. the driver needs both DMA addresses (that would be the address
> stored in the controller), and the CPU address. To preserve the old
> behavior in 2.5, the driver would need to save these CPU addresses
> somewhere. Strictly speaking, we shouldn't be translating between
> the two address types.

Yes, it seems a waste of effort.

> I don't have specifications for these devices.
> I don't know if there is some real reason for the old behavior.
>
> I don't have either of these controller types available to test this change
> either. So, if you can think of any reason I should preserve the original
> behavior, I'd appreciate the feedback.

Hopefully somebody with one of those will jump into the thread. Where
in the mass of DAC init output do we look for the controller type?

--
Daniel

2002-09-23 20:42:01

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

On Monday 23 September 2002 21:19, David Mosberger wrote:
> Hi Dave,
>
> >>>>> On Mon, 23 Sep 2002 12:04:00 -0700, Dave Olien <[email protected]> said:
>
> Dave> #ifdef __ia64__
> Dave> - writeq(Virtual_to_Bus64(CommandMailbox),
> Dave> + writeq(CommandMailboxDMA,
> Dave> ControllerBaseAddress + DAC960_LP_CommandMailboxBusAddressOffset);
> Dave> #else
> Dave> - writel(Virtual_to_Bus32(CommandMailbox),
> Dave> + writel(CommandMailboxDMA,
> Dave> ControllerBaseAddress + DAC960_LP_CommandMailboxBusAddressOffset);
> Dave> #endif
>
> This looks like a porting-nightmare in the making. There's got to be a
> better way to determine whether you need a writeq() vs. a writel().

Even if an #ifdef is necessary here (and we are in trouble if it is) it
should not trigger on __ia64__, it should trigger on the size of (long).

--
Daniel

2002-09-23 21:19:40

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

>>>>> On Mon, 23 Sep 2002 13:57:08 -0700 (PDT), "David S. Miller" <[email protected]> said:

>> On many platforms, two consequetive __raw_writel()'s might even
>> combine to an atomic 64-bit store to PCI space. :-)

Yes, but that's no guarantee.

>> I don't think the proposed 32-bit behavior is off the mark, and
>> anyways x86 can actually make the 64-bit store I believe if it
>> wants at least on more recent processors.

Surely we wouldn't want to define a new API that can't be supported on
all 32-bit platforms, no? Perhaps writeq_nonatomic()?

--david

2002-09-23 21:29:00

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

>>>>> On Mon, 23 Sep 2002 23:31:13 +0200, Daniel Phillips <[email protected]> said:

Daniel> Why attempt to write 8 bytes on ia32 when only 4 are needed?

Even on ia32 you'll need 8 bytes if the controller is operated in DAC
mode (which is what you want for a machine with >4GB of memory), no?

--david

2002-09-23 21:38:05

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

On Mon, Sep 23, 2002 at 10:39:00PM +0200, Daniel Phillips wrote:
>
> Applied, booted, up and running. I'll attempt to follow your work -
> you are clearly way out in front of everybody in understanding this
> driver.
>
> Minor whitespace suggestion: don't worry too much about breaking up
> lines to fit in 80 columns. It's nice where it works, but where it
> just makes more lines, don't bother. We are going to go do spelling
> patches to shorten a lot of those names anyway.

thanks.
I'd been wondering whether there was a guidline for this.
I'll relax my 80 column constraints.

>
> What test do you use to determine that some blocks are incorrectly
> written?

I wrote something really simple. The amazing thing was that it
turned up a problem relatively quickly, just running one instance
of the programs at a time. i.e. no parallelism.
I haven't walked through this code yet. But, I've noticed several
pdflush kernel daemons running on my 8-way system. So, I'm guessing
IF there ARE any SMP races (I'm not claiming that there ARE any races),
these daemons could possibly uncover them even though I'm running only once
instance of the reader/writer applcations.

I'll include the test programs source code at the bottom of the mail.
It's just a pair of C programs. The first one writes a unique
pattern (a device id, a block number, and a "sequence number") at the
beginning of each 512-byte block. The second one reads the blocks
back, checking the pattern.

I'm going to play with Iozone later this week.

>
> I think you're right. It's hard to imagine why it would be good to
> optimize the module unload/reload case for DAC, and if there is a
> correctness argument, I personally would rather find out about it the
> hard way than never know it.

..... stuff deleted..

>
> Hopefully somebody with one of those will jump into the thread. Where
> in the mass of DAC init output do we look for the controller type?
>
> --
> Daniel

I think that's a good approach. Hopefully people with more device
types will apprear as we get closer to completion.

The way to identify the type of controller is to read the current_status
file in the /proc/rd/c0 directory.

cd /dev/rd/c0; less current_status

The first lines of My current_status file reads:

***** DAC960 RAID Driver Version 2.4.11 of 11 October 2001 *****
Copyright 1998-2001 by Leonard N. Zubkoff <[email protected]>
Configuring Mylex eXtremeRAID 2000 PCI RAID Controller
Firmware Version: 7.00-03, Channels: 4, Memory Size: 64MB
PCI Bus: 11, Device: 8, Function: 0, I/O Address: Unassigned
PCI Address: 0xFC000000 mapped at 0xF8800000, IRQ Channel: 24
Controller Queue Depth: 512, Maximum Blocks per Command: 2048
Driver Queue Depth: 511, Scatter/Gather Limit: 128 of 257 Segments
Physical Devices:

>From the first line, it looks like we should change the Version
number and date of the driver. It Might be useful during testing.
Maybe change version number to 2.5.??, and the date to the date
of the latest patch applied.

On the third line, "eXtremeRAID 2000", can be mapped to the internal
device types by looking in DAC960.h, looking at the lines:

/*
Define the DAC960 Controller Hardware Types.
*/

typedef enum
{
DAC960_BA_Controller = 1, /* eXtremeRAID 2000 */
DAC960_LP_Controller = 2, /* AcceleRAID 352 */
DAC960_LA_Controller = 3, /* DAC1164P */
DAC960_PG_Controller = 4, /* DAC960PTL/PJ/PG */
DAC960_PD_Controller = 5, /* DAC960PU/PD/PL/P */
DAC960_P_Controller = 6 /* DAC960PU/PD/PL/P */
}
DAC960_HardwareType_T;

So my controller uses all the DAC960_BA_*, or DAC960_V2 routines.

The controller types DAC960_BA_Controller, and DAC960_LP_Controller
are both V2 type controllers. The others are type V1 controllers.

Here's the two test programs I ran to uncover the data problem I had last
week. (They're embarassingly simple). Both programs take three
arguments:

pathname to a partition to write, and then read.
a "device id" that uniquely identifies that partition
a "sequence number" that uniquely identifies the a partitular
invocation of the program.

So for example, I ran the programs in pairs:

wpath /dev/rd/c0d0p1 1010 1001; cpath /dev/rd/c0d0p1 1010 1001

The next time I ran the pair of programs on the same partition,
I'd change the "sequence number" argument to distinguish blocks
written previously, from blocks being newly written.

These programs write only a small amount of the partition.
But, this was enough to uncover a problem with 2.5.34. But,
the problem rarely appeared at the beginning of the partition.

Another thing I do is run mke2fs and e2fsck on the partitions.
The latest thing I tried successfully was to run mke2fs on all partitions
on all logical devices in parallel. So, there were 16 mke2fs programs
running in parallel. Then, run 16 e2fsck in parallel. On 2.5.38, the
e2fsck's all run successfully. Use the -f flag to force
checking on clean file systems.

--------------------------write pattern---------------------------------------

#include <sys/types.h>
#include <stdlib.h>
#include <fcntl.h>

char *dev_path;
int dev_id;
int seq_no;
int fd;

struct block_record {
int disk_id;
int block_no;
int sequence_no;
int pad;
};

#define BLK_SIZE 512
#define NBLKS_PER_BUF 1024
#define BUF_SIZE (BLK_SIZE*NBLKS_PER_BUF)
#define BUF_ALIGN 4096
#define NUM_BUFS 1024

void *io_buf;
int block_no;

fill_buffer()
{
int i;
void *bufp;
struct block_record *brp;

for (i = 0, bufp = io_buf; i < NBLKS_PER_BUF; i++) {
brp = (struct block_record *)bufp;
brp->disk_id = dev_id;
brp->block_no = block_no;
brp->sequence_no = seq_no;
bufp += BLK_SIZE;
block_no++;
}
}

int
write_buffer()
{
ssize_t wsize;

wsize = write(fd, io_buf, BUF_SIZE);

if (wsize < 0) {
perror("write error");
return -1;
}

if (wsize < BUF_SIZE)
printf("last size written\n", wsize);

return 0;
}

write_file()
{
int i;

for (i = 0; i < NUM_BUFS; i++) {
fill_buffer();
if (write_buffer())
break;
}
}

main(int argc, char **argv)
{

dev_path = argv[1];
dev_id = atoi(argv[2]);
seq_no = atoi(argv[3]);

printf("path %s id %d seq %d\n", dev_path, dev_id, seq_no);

if ((fd = open(dev_path, O_RDWR)) <= 0) {
perror("open failed");
exit(-1);
}

if (posix_memalign(&io_buf, BUF_ALIGN, BUF_SIZE)) {
perror("buffer alloc failed");
exit(-1);
}

write_file();
exit(0);
}

------------ check pattern -----------------------------------------------

#include <sys/types.h>
#include <stdlib.h>
#include <fcntl.h>

char *dev_path;
int dev_id;
int seq_no;
int fd;

struct block_record {
int disk_id;
int block_no;
int sequence_no;
int pad;
};

#define BLK_SIZE 512
#define NBLKS_PER_BUF 1024
#define BUF_SIZE (BLK_SIZE*NBLKS_PER_BUF)
#define BUF_ALIGN 4096
#define ERR_LIMIT 500
#define NUM_BUFS 1024

void *io_buf;
int block_no;
int err_cnt;

print_diff(int block_no, struct block_record *brp)
{
printf("disk_id %d block_no %d sequence_no %d\n",
brp->disk_id, brp->block_no, brp->sequence_no);
}

int err_flipflop = 0;

check_buffer(ssize_t rsize)
{
int i;
void *bufp;
struct block_record *brp;

printf("begin new buffer at block_no %d\n", block_no);

for (i = 0, bufp = io_buf; i < NBLKS_PER_BUF; i++) {
brp = (struct block_record *)bufp;
if ((brp->disk_id != dev_id) ||
(brp->block_no != block_no) ||
(brp->sequence_no != seq_no)) {
/*
* Mismatch! Print the error ONLY
* if the previous buffer was NOT
* a mismatch.
*/
if (err_flipflop == 0) {
err_flipflop = 1;
printf("difference in block_no %d\n", block_no);
print_diff(block_no, brp);
err_cnt++;
if (err_cnt > ERR_LIMIT) {
printf("err limit reached\n");
exit(-1);
}
}
} else if (err_flipflop == 1) {
/*
* We have a match, and the
* Previous block was a Mis-match.
*/
printf("Matching in block_no %d\n", block_no);
print_diff(block_no, brp);
err_flipflop = 0;
}
bufp += BLK_SIZE;
block_no++;
if (bufp - io_buf > rsize) {
printf("early check end\n");
break;
}
}
}

ssize_t
read_buffer()
{
ssize_t rsize;

rsize = read(fd, io_buf, BUF_SIZE);

if (rsize < 0) {
perror("rrite error");
return -1;
}

if (rsize < BUF_SIZE)
printf("last size read\n", rsize);

return rsize;
}

read_file()
{
ssize_t rsize;
int i;

for (i = 0; i < NUM_BUFS; i++) {
rsize = read_buffer();
check_buffer(rsize);
}
}

main(int argc, char **argv)
{

dev_path = argv[1];
dev_id = atoi(argv[2]);
seq_no = atoi(argv[3]);

printf("path %s id %d seq %d\n", dev_path, dev_id, seq_no);

if ((fd = open(dev_path, O_RDWR)) < 0) {
perror("open failed");
exit(-1);
}

if (posix_memalign(&io_buf, BUF_ALIGN, BUF_SIZE)) {
perror("buffer alloc failed");
exit(-1);
}

read_file();
exit(0);
}

2002-09-23 21:35:42

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

On Monday 23 September 2002 23:33, David Mosberger wrote:
> >>>>> On Mon, 23 Sep 2002 23:31:13 +0200, Daniel Phillips <[email protected]> said:
>
> Daniel> Why attempt to write 8 bytes on ia32 when only 4 are needed?
>
> Even on ia32 you'll need 8 bytes if the controller is operated in DAC
> mode (which is what you want for a machine with >4GB of memory), no?

Firm maybe. The current driver does not attempt to do that on ia32, and
you're saying it should? In that case should the driver not have #define
DAC_64, or similar?

Daniel

2002-09-23 21:24:56

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

From: David Mosberger <[email protected]>
Date: Mon, 23 Sep 2002 14:24:12 -0700

>> I don't think the proposed 32-bit behavior is off the mark, and
>> anyways x86 can actually make the 64-bit store I believe if it
>> wants at least on more recent processors.

Surely we wouldn't want to define a new API that can't be supported on
all 32-bit platforms, no? Perhaps writeq_nonatomic()?

I'm saying we define writeq() to be implementable as two 32-bit
transations, that will be the API.

I think adding some weird new writeq_nonatomic() will just create
more confusion than it saves.

2002-09-23 21:29:29

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

>>>>> On Mon, 23 Sep 2002 14:17:52 -0700 (PDT), "David S. Miller" <[email protected]> said:

DaveM> I'm saying we define writeq() to be implementable as two 32-bit
DaveM> transations, that will be the API.

Ah, OK, if it's documented that way (with the option of implementing
it as a single 64-bit store).

--david

2002-09-23 21:31:00

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

On Monday 23 September 2002 23:24, David Mosberger wrote:
> >>>>> On Mon, 23 Sep 2002 13:57:08 -0700 (PDT), "David S. Miller" <[email protected]> said:
>
> >> On many platforms, two consequetive __raw_writel()'s might even
> >> combine to an atomic 64-bit store to PCI space. :-)
>
> Yes, but that's no guarantee.
>
> >> I don't think the proposed 32-bit behavior is off the mark, and
> >> anyways x86 can actually make the 64-bit store I believe if it
> >> wants at least on more recent processors.
>
> Surely we wouldn't want to define a new API that can't be supported on
> all 32-bit platforms, no? Perhaps writeq_nonatomic()?

Why attempt to write 8 bytes on ia32 when only 4 are needed?

--
Daniel

2002-09-23 21:34:16

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

From: David Mosberger <[email protected]>
Date: Mon, 23 Sep 2002 14:33:46 -0700

>>>>> On Mon, 23 Sep 2002 23:31:13 +0200, Daniel Phillips <[email protected]> said:

Daniel> Why attempt to write 8 bytes on ia32 when only 4 are needed?

Even on ia32 you'll need 8 bytes if the controller is operated in DAC
mode (which is what you want for a machine with >4GB of memory), no?

Right. But if we know the top 32-bits will always be zero (f.e. if
CONFIG_HIGHMEM is disabled on x86) one might want to optimize this
to avoid the first 32-bit store.

2002-09-23 21:34:29

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

From: David Mosberger <[email protected]>
Date: Mon, 23 Sep 2002 14:32:32 -0700

Ah, OK, if it's documented that way (with the option of implementing
it as a single 64-bit store).

This is the idea, yes

2002-09-23 21:40:28

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

>>>>> On Mon, 23 Sep 2002 23:40:35 +0200, Daniel Phillips <[email protected]> said:

Daniel> Firm maybe. The current driver does not attempt to do that
Daniel> on ia32, and you're saying it should? In that case should
Daniel> the driver not have #define DAC_64, or similar?

I haven't looked at the complete driver, but yes, it probably needs to
have a #define DAC or its runtime-equivalent.

--david

2002-09-23 21:48:25

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

On Monday 23 September 2002 23:41, Dave Olien wrote:
> On Mon, Sep 23, 2002 at 10:39:00PM +0200, Daniel Phillips wrote:
> > Minor whitespace suggestion: don't worry too much about breaking up
> > lines to fit in 80 columns. It's nice where it works, but where it
> > just makes more lines, don't bother. We are going to go do spelling
> > patches to shorten a lot of those names anyway.
>
> thanks.
> I'd been wondering whether there was a guidline for this.
> I'll relax my 80 column constraints.

It's a lot more important for core kernel, and ever there, a few lines
tend to break loose here and there.

Thanks for the test code. Using your recipe, I confirmed I have the
same controller as you. Running on a dual PIII in my case.

Apparently Jens has a DAC as well. Jens, is it the same?

--
Daniel

2002-09-23 21:57:49

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

By the way, I don't know how extensive your code reviewing
of my changes has been.

This morning I sent you the complete driver patch from 2.5.38
to the latest version of the driver. That contains all of
the changes I sent you last week, plus new ones.

Would it be better for me to send you incremental patches
in the future? I could instead generate a patch relative to the
previous version of the driver I sent you.

Let me know what works better for you, based on where you are in code
reviewing.

Thanks!

On Mon, Sep 23, 2002 at 11:53:47PM +0200, Daniel Phillips wrote:
> On Monday 23 September 2002 23:41, Dave Olien wrote:
> > On Mon, Sep 23, 2002 at 10:39:00PM +0200, Daniel Phillips wrote:
> > > Minor whitespace suggestion: don't worry too much about breaking up
> > > lines to fit in 80 columns. It's nice where it works, but where it
> > > just makes more lines, don't bother. We are going to go do spelling
> > > patches to shorten a lot of those names anyway.
> >
> > thanks.
> > I'd been wondering whether there was a guidline for this.
> > I'll relax my 80 column constraints.
>
> It's a lot more important for core kernel, and ever there, a few lines
> tend to break loose here and there.
>
> Thanks for the test code. Using your recipe, I confirmed I have the
> same controller as you. Running on a dual PIII in my case.
>
> Apparently Jens has a DAC as well. Jens, is it the same?
>
> --
> Daniel

2002-09-23 22:16:40

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

On Tuesday 24 September 2002 00:03, Dave Olien wrote:
> By the way, I don't know how extensive your code reviewing
> of my changes has been.

Horribly unextensive.

> This morning I sent you the complete driver patch from 2.5.38
> to the latest version of the driver. That contains all of
> the changes I sent you last week, plus new ones.
>
> Would it be better for me to send you incremental patches
> in the future? I could instead generate a patch relative to the
> previous version of the driver I sent you.

Far better to send the whole patch. If anybody should be sending
incremental patches, it's me to you.

I have a number of code reading projects going at the moment, DAC960
is one of them. Rather than read the whole think I'll just start
with your bio changes, because it's fairly central and also fairly
focussed, plus I need the incentive to go in and figure out just what
Jens, Suparna et al have done in there.

--
Daniel

2002-09-24 16:49:44

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

I haven't been ignoring the discussion. Just thinking it over.
I was going to dig out an old PCI spec last night, but got involved
with other things. I wanted to review how 64-bit PCI and 32-bit PCI busses
handle 32 and 64-bit writes from the processor.

I don't have a spec for these controllers. IBM is selling Mylex
to LSI logic, so "All contracts, NDA's and agreements are on hold
until a transition to lsi is complete" But, I'm guessing this
controller actually implements a 64-bit register at this location.
The 32-bit write from ia32 works probably because the controller either
clears the upper-32 bits when the lower-32 bits are written, or
those upper-32 bits are zero because we never set them to non-zero.

According to the Documentation/DMA-mapping.txt file, the new
DMA mapping interfaces should allow all PCI transfers to use 32-bit DMA
addresses. Controllers on the PCI bus should never need to use DAC
PCI transfers. Based on this, writel() should work even on ia64.

It's possible there's something peculiar about the ia64 hardware
that lead to using writeq(). I might be able to get my hands on
an ia64 platform that I could try this on.

OR, the driver COULD always write 64 bits to this register. ia32 doesn't
have a writeq() macro. The only way I know to write 64 bits on ia32
is with one of the xchg instructions. But that's mostly used if you
really want an atomic 64-bit operation. Probably writeq() implemented as
two writel()'s would work. It depends on the behavior of this particular
register's implementation on the controller.

I'll experiment with this later, after the other changes
I'm working on. I'll try to identify a way to write this register
that is uniform across platforms, and won't require an #ifdef.

On Mon, Sep 23, 2002 at 01:54:47PM -0700, David S. Miller wrote:
> From: Daniel Phillips <[email protected]>
> Date: Mon, 23 Sep 2002 22:44:08 +0200
>
> On Monday 23 September 2002 21:19, David Mosberger wrote:
> > This looks like a porting-nightmare in the making. There's got to be a
> > better way to determine whether you need a writeq() vs. a writel().
>
> Even if an #ifdef is necessary here (and we are in trouble if it is) it
> should not trigger on __ia64__, it should trigger on the size of (long).
>
> There is nothing preventing a 32-bit long platform from being able
> to store 64-bits at once to PCI space.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

2002-09-24 17:06:38

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

>>>>> On Tue, 24 Sep 2002 09:54:56 -0700, Dave Olien <[email protected]> said:

Dave> According to the Documentation/DMA-mapping.txt file, the new
Dave> DMA mapping interfaces should allow all PCI transfers to use
Dave> 32-bit DMA addresses. Controllers on the PCI bus should never
Dave> need to use DAC PCI transfers. Based on this, writel() should
Dave> work even on ia64.

Warning: there is a big difference between *can* and *want*. On ia64
machines with an Intel chipset, the PCI DMA interface is implemented
via bounce buffers, so it will be *much* slower than DAC. For this
reason, it is preferable on ia64 to use DAC where possible (and just
in case Dave Miller starts asking about this: yes, the hp zx1 chipset
for Itanium 2 does have a hardware I/O TLB... ;-).

--david

2002-09-24 17:50:37

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

On Tuesday 24 September 2002 18:54, Dave Olien wrote:
> According to the Documentation/DMA-mapping.txt file, the new
> DMA mapping interfaces should allow all PCI transfers to use 32-bit DMA
> addresses. Controllers on the PCI bus should never need to use DAC
> PCI transfers. Based on this, writel() should work even on ia64.

A totally disgusting idea: MMX/x87 are also capable of transferring 8
bytes in one instruction on ia32.

--
Daniel

2002-09-24 17:46:57

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

On Tuesday 24 September 2002 19:11, David Mosberger wrote:
> (and just
> in case Dave Miller starts asking about this: yes, the hp zx1 chipset
> for Itanium 2 does have a hardware I/O TLB... ;-).

Excuse me, could you please elaborate on the significance of this
for the rest of us? ;-)

--
Daniel

2002-09-24 17:41:27

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

On Tuesday 24 September 2002 18:54, Dave Olien wrote:
> I haven't been ignoring the discussion. Just thinking it over.
> I was going to dig out an old PCI spec last night, but got involved
> with other things. I wanted to review how 64-bit PCI and 32-bit PCI busses
> handle 32 and 64-bit writes from the processor.
>
> I don't have a spec for these controllers. IBM is selling Mylex
> to LSI logic, so "All contracts, NDA's and agreements are on hold
> until a transition to lsi is complete"

Note: I will sign an NDA to get the specs for this card (family) but I
would much prefer that somebody gets a clue and just releases the spec.
Frankly, this board needs all the help it can get, based on comparative
performance. Keeping the spec secret is just a brilliant way to ensure
it will die, and thus be worth $0.00 to the purchaser.

(I sincerely hope somebody in the DAC960 food chain notices this
comment, or somebody forwards it to one of the aforementioned: give
the world the spec and we will reciprocate by making the thing go as
fast as theoretically possible.)

Daniel

2002-09-24 17:41:28

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

Hmm, interesting. A big part of what I'm doing to the driver
right now is plugging in calls to the PCI DMA interfaces.
So unless the underlying implementation is aware of these
platform limitations and "doing the right thing", I'm adding
a bunch of bounce buffer activity for many ia64 platforms.
That would be too bad.

I distantly recall that the cost of doing PCI DAC isn't really
that high for bursty transfers, because the bus is able to
group packets of data cycles behind one initial address cycle.
But it's been many years since I last looked at this and I could have
this wrong.

On Tue, Sep 24, 2002 at 10:11:26AM -0700, David Mosberger wrote:
> >>>>> On Tue, 24 Sep 2002 09:54:56 -0700, Dave Olien <[email protected]> said:
>
> Dave> According to the Documentation/DMA-mapping.txt file, the new
> Dave> DMA mapping interfaces should allow all PCI transfers to use
> Dave> 32-bit DMA addresses. Controllers on the PCI bus should never
> Dave> need to use DAC PCI transfers. Based on this, writel() should
> Dave> work even on ia64.
>
> Warning: there is a big difference between *can* and *want*. On ia64
> machines with an Intel chipset, the PCI DMA interface is implemented
> via bounce buffers, so it will be *much* slower than DAC. For this
> reason, it is preferable on ia64 to use DAC where possible (and just
> in case Dave Miller starts asking about this: yes, the hp zx1 chipset
> for Itanium 2 does have a hardware I/O TLB... ;-).
>
> --david

2002-09-24 18:22:21

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

>>>>> On Tue, 24 Sep 2002 19:50:46 +0200, Daniel Phillips <[email protected]> said:

Daniel> On Tuesday 24 September 2002 19:11, David Mosberger wrote:
>> (and just
>> in case Dave Miller starts asking about this: yes, the hp zx1 chipset
>> for Itanium 2 does have a hardware I/O TLB... ;-).

Daniel> Excuse me, could you please elaborate on the significance of this
Daniel> for the rest of us? ;-)

Sorry, I didn't mean to be cryptic and it's really nothing deep. It's
just that in the past Dave Miller used, let's say, "colorful" language
whenever someone mentioned the fact that the original Itanium machines
didn't have a hardware I/O TLB. The new Itanium 2 machines from hp
have a chipset with heritage from pa-risc, so they don't have this
limitation (to be fair to Intel though, I don't think the lack of a hw
I/O TLB is a huge issue anymore, given that nowadays you can readily
find all kinds of PCI controllers that support DAC, including network
adapters).

--david

2002-09-24 18:40:29

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

I'm looking over the main body of the interrupt handler
for the DAC960 ( the DAC960_V[12]_ProcessCompletedCommand()).

The V1 version of this function is over 800 lines long.
The V2 version is 600 lines long.

First off I have an intense dislike for ANY function that
is this long.

Secondly, most of the stuff in these functions doesn't
seem appropriate for an interrupt handler. 90% of
the code in these functions is executed rarely, and
isn't performance critical. Most of
these functions involve health monitoring. In some cases,
where logical drives appear or disappear, the set of
disks visible through the controller must change.

It seems to me that most of this functionality
should be moved into a kernel daemon associated with the
driver. The interrupt handler could be shrunk to as few
instructions and "if then else" branches as possible, to
shorten the time the spent in the handler and the time the
controller's lock is held.

This is a bigger change than I'm ready to deal with
right now. First thing is to get the old code structure
to work with the DMA interfaces. But I think daemon-izing
this code would clean it up and make it more easily
understood as well.

Perhaps we could do this as part of a "stage-two"
set of changes.

2002-09-24 18:20:09

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

Ok, thanks for explaining!

At some point, I want to step back and look over the BIO and DMA code.
Possibly this will be my "project" for October :-)

On Tue, Sep 24, 2002 at 11:21:45AM -0700, David Mosberger wrote:
> >>>>> On Tue, 24 Sep 2002 10:28:43 -0700, Dave Olien <[email protected]> said:
>
> Dave> Hmm, interesting. A big part of what I'm doing to the driver
> Dave> right now is plugging in calls to the PCI DMA interfaces.
>
> That's fine (actually, it's more than that: it's great!).
>
> Dave> So unless the underlying implementation is aware of these
> Dave> platform limitations and "doing the right thing", I'm adding
> Dave> a bunch of bounce buffer activity for many ia64 platforms.
> Dave> That would be too bad.
>
> The idea is to do something along these lines:
>
> o Use PCA DMA interface for managing all DMA buffers.
>
> o If sizeof(dma_addr_t) > 4, turn on DAC mode and call
> pci_set_dma_mask(dev, MAX_ADDR), where MAX_ADDR is the maximum
> address that can be reached by the controller (i.e.,
> 0xffffffffffffffff for a truly 64-bit-capable PCI controller).
>
> This way, everything should work out correctly and with good
> performance on all imaginable platforms (ia64 with and without
> hardware I/O TLB, plain x86, x86 with >4GB RAM, SPARC64 with 32-bit
> dma_addr_t, etc.).
>
> --david

2002-09-24 18:16:41

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

>>>>> On Tue, 24 Sep 2002 10:28:43 -0700, Dave Olien <[email protected]> said:

Dave> Hmm, interesting. A big part of what I'm doing to the driver
Dave> right now is plugging in calls to the PCI DMA interfaces.

That's fine (actually, it's more than that: it's great!).

Dave> So unless the underlying implementation is aware of these
Dave> platform limitations and "doing the right thing", I'm adding
Dave> a bunch of bounce buffer activity for many ia64 platforms.
Dave> That would be too bad.

The idea is to do something along these lines:

o Use PCA DMA interface for managing all DMA buffers.

o If sizeof(dma_addr_t) > 4, turn on DAC mode and call
pci_set_dma_mask(dev, MAX_ADDR), where MAX_ADDR is the maximum
address that can be reached by the controller (i.e.,
0xffffffffffffffff for a truly 64-bit-capable PCI controller).

This way, everything should work out correctly and with good
performance on all imaginable platforms (ia64 with and without
hardware I/O TLB, plain x86, x86 with >4GB RAM, SPARC64 with 32-bit
dma_addr_t, etc.).

--david

2002-09-24 19:07:26

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

>>>>> On Tue, 24 Sep 2002 11:25:19 -0700, Dave Olien <[email protected]> said:

Dave> Ok, thanks for explaining!

Dave> On Tue, Sep 24, 2002 at 11:21:45AM -0700, David Mosberger wrote:
>> >>>>> On Tue, 24 Sep 2002 10:28:43 -0700, Dave Olien <[email protected]> said:
>>
>> The idea is to do something along these lines:
>>
>> o Use PCA DMA interface for managing all DMA buffers.
>>
>> o If sizeof(dma_addr_t) > 4, turn on DAC mode and call
>> pci_set_dma_mask(dev, MAX_ADDR), where MAX_ADDR is the maximum
>> address that can be reached by the controller (i.e.,
>> 0xffffffffffffffff for a truly 64-bit-capable PCI controller).
>>
>> This way, everything should work out correctly and with good
>> performance on all imaginable platforms (ia64 with and without
>> hardware I/O TLB, plain x86, x86 with >4GB RAM, SPARC64 with 32-bit
>> dma_addr_t, etc.).

Come to think of it, the following refinement may be preferable:

o Use PCA DMA interface for managing all DMA buffers.

o limit = min(~(dma_addr_t) 0, MAX_ADDR), where MAX_ADDR is the maximum
address that can be reached by the controller (i.e.,
0xffffffffffffffff for a truly 64-bit-capable PCI controller).

o Call pci_set_dma_mask(dev, limit).

o If limit > 0xffffffff, turn on DAC mode.

This "algorithm" should be correct for all PCI controllers.

--david

2002-09-25 21:37:19

by Davide Rossetti

[permalink] [raw]

Subject: Re: DAC960 in 2.5.38, with new changes

On Tue, 24 Sep 2002, Daniel Phillips wrote:

> On Tuesday 24 September 2002 18:54, Dave Olien wrote:
> > According to the Documentation/DMA-mapping.txt file, the new
> > DMA mapping interfaces should allow all PCI transfers to use 32-bit DMA
> > addresses. Controllers on the PCI bus should never need to use DAC
> > PCI transfers. Based on this, writel() should work even on ia64.
>
> A totally disgusting idea: MMX/x87 are also capable of transferring 8
> bytes in one instruction on ia32.

to the north bridge. but then it's a bridge matter how to translate that
access on the PCI side... I guess.
regards

--
______/ Rossetti Davide INFN - Roma I - APE group \______________
pho +390649914507/412 web: http://apegate.roma1.infn.it/~rossetti
fax +390649914423 email: [email protected]

2002-09-25 22:17:51

[permalink] [raw]

Subject: Re: DAC960, documentation links

On Thursday 26 September 2002 00:20, Daniel I wrote
> This appears to be the best that Mylex can offer:
>
> http://www.mylex.com/support/index.html
> (contact technical support)
>
> As you said.

Wait:

http://www.mylex.com/products/hpr/ex2000/eXtremeRAID2000.pdf

--
Daniel

2002-09-25 22:15:29