ultrastor.c is a bit-rot

ultrastor.c is a bit-rot

[Boaz Harrosh]

Inspecting ultrastor.c it is clear to me that this was never used for
a loooooooooong time. Not since a PC has more then 2^24 bit of memory.
Let me explain below.

Now I'm not saying it should be fixed. I'm saying that it should be dumped
in the account that it is not used by any one and that it does not work.

Why it never worked?
~~~~~~~~~~~~~~~~~~~~~

The driver's header says it supports 3 cards

 *	14F - ISA first-party DMA HA with floppy support and WD1003 emulation.
 *	24F - EISA Bus Master HA with floppy support and WD1003 emulation.
 *	34F - VL-Bus Bus Master HA with floppy support (no WD1003 emulation).

But Kconfig only specifies ISA. I'm not sure what a VL-Bus is.


now the driver defines a static array of structures like this:

	struct {
	  ...
	
	  struct mscp mscp[ULTRASTOR_MAX_CMDS];
	} config = {0};

and allocates a struct mscp in .queuecommand like this:
	    my_mscp = &config.mscp[mscp_index];

it will go on preparing this my_mscp structure including stuffing
some mapped pointers. Lets put that aside for now.
At the very end it will pass this my_mscp structure to the card's 
firmware like this:

	    /* Store pointer in OGM address bytes */
	outl(isa_virt_to_bus(my_mscp), config.ogm_address);

Now this is one hell of a smart ISA card. But putting this aside.

if the machine has more then 2^24 of memory. Then this will never
work, right? or I'm missing it completely?

(Also none of the emails in this file are valid)

Boaz

Re: ultrastor.c is a bit-rot

[James Bottomley]

On Mon, 2008-03-17 at 16:59 +0200, Boaz Harrosh wrote:
> Inspecting ultrastor.c it is clear to me that this was never used for
> a loooooooooong time. Not since a PC has more then 2^24 bit of memory.
> Let me explain below.
> 
> Now I'm not saying it should be fixed. I'm saying that it should be dumped
> in the account that it is not used by any one and that it does not work.
> 
> Why it never worked?
> ~~~~~~~~~~~~~~~~~~~~~
> 
> The driver's header says it supports 3 cards
> 
>  *	14F - ISA first-party DMA HA with floppy support and WD1003 emulation.
>  *	24F - EISA Bus Master HA with floppy support and WD1003 emulation.
>  *	34F - VL-Bus Bus Master HA with floppy support (no WD1003 emulation).
> 
> But Kconfig only specifies ISA. I'm not sure what a VL-Bus is.

VL is vesa local ... it was an ISA like graphics bus that was fast and
could reach > 16MB.

> now the driver defines a static array of structures like this:
> 
> 	struct {
> 	  ...
> 	
> 	  struct mscp mscp[ULTRASTOR_MAX_CMDS];
> 	} config = {0};
> 
> and allocates a struct mscp in .queuecommand like this:
> 	    my_mscp = &config.mscp[mscp_index];
> 
> it will go on preparing this my_mscp structure including stuffing
> some mapped pointers. Lets put that aside for now.
> At the very end it will pass this my_mscp structure to the card's 
> firmware like this:
> 
> 	    /* Store pointer in OGM address bytes */
> 	outl(isa_virt_to_bus(my_mscp), config.ogm_address);
> 
> Now this is one hell of a smart ISA card. But putting this aside.
> 
> if the machine has more then 2^24 of memory. Then this will never
> work, right? or I'm missing it completely?

It will definitely work for EISA and VL bus.  I think if you analyse the
placement of kernel data segments for compiled in drivers, it might also
work for ISA too, since I think the pfn will be low enough.  It should
fail as a module not just because the area will be out of range for ISA,
but also because the module data segment is in vmalloc space, so the
virt_to_bus assumptions of contiguity could be violated.

James

Re: ultrastor.c is a bit-rot

[Boaz Harrosh]

On Mon, Mar 17 2008 at 17:23 +0200, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> On Mon, 2008-03-17 at 16:59 +0200, Boaz Harrosh wrote:
>> Inspecting ultrastor.c it is clear to me that this was never used for
>> a loooooooooong time. Not since a PC has more then 2^24 bit of memory.
>> Let me explain below.
>>
>> Now I'm not saying it should be fixed. I'm saying that it should be dumped
>> in the account that it is not used by any one and that it does not work.
>>
>> Why it never worked?
>> ~~~~~~~~~~~~~~~~~~~~~
>>
>> The driver's header says it supports 3 cards
>>
>>  *	14F - ISA first-party DMA HA with floppy support and WD1003 emulation.
>>  *	24F - EISA Bus Master HA with floppy support and WD1003 emulation.
>>  *	34F - VL-Bus Bus Master HA with floppy support (no WD1003 emulation).
>>
>> But Kconfig only specifies ISA. I'm not sure what a VL-Bus is.
> 
> VL is vesa local ... it was an ISA like graphics bus that was fast and
> could reach > 16MB.
> 
>> now the driver defines a static array of structures like this:
>>
>> 	struct {
>> 	  ...
>> 	
>> 	  struct mscp mscp[ULTRASTOR_MAX_CMDS];
>> 	} config = {0};
>>
>> and allocates a struct mscp in .queuecommand like this:
>> 	    my_mscp = &config.mscp[mscp_index];
>>
>> it will go on preparing this my_mscp structure including stuffing
>> some mapped pointers. Lets put that aside for now.
>> At the very end it will pass this my_mscp structure to the card's 
>> firmware like this:
>>
>> 	    /* Store pointer in OGM address bytes */
>> 	outl(isa_virt_to_bus(my_mscp), config.ogm_address);
>>
>> Now this is one hell of a smart ISA card. But putting this aside.
>>
>> if the machine has more then 2^24 of memory. Then this will never
>> work, right? or I'm missing it completely?
> 
> It will definitely work for EISA and VL bus.  I think if you analyse the
> placement of kernel data segments for compiled in drivers, it might also
> work for ISA too, since I think the pfn will be low enough.  It should
> fail as a module not just because the area will be out of range for ISA,
> but also because the module data segment is in vmalloc space, so the
> virt_to_bus assumptions of contiguity could be violated.
> 
> James
> 

So what is the verdict? is it removed? marked broken for ISA?

can I safely say that unchecked_isa_dma can be removed?

Boaz

Re: ultrastor.c is a bit-rot

[James Bottomley]

On Mon, 2008-03-17 at 18:00 +0200, Boaz Harrosh wrote:
> On Mon, Mar 17 2008 at 17:23 +0200, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > On Mon, 2008-03-17 at 16:59 +0200, Boaz Harrosh wrote:
> >> Inspecting ultrastor.c it is clear to me that this was never used for
> >> a loooooooooong time. Not since a PC has more then 2^24 bit of memory.
> >> Let me explain below.
> >>
> >> Now I'm not saying it should be fixed. I'm saying that it should be dumped
> >> in the account that it is not used by any one and that it does not work.
> >>
> >> Why it never worked?
> >> ~~~~~~~~~~~~~~~~~~~~~
> >>
> >> The driver's header says it supports 3 cards
> >>
> >>  *	14F - ISA first-party DMA HA with floppy support and WD1003 emulation.
> >>  *	24F - EISA Bus Master HA with floppy support and WD1003 emulation.
> >>  *	34F - VL-Bus Bus Master HA with floppy support (no WD1003 emulation).
> >>
> >> But Kconfig only specifies ISA. I'm not sure what a VL-Bus is.
> > 
> > VL is vesa local ... it was an ISA like graphics bus that was fast and
> > could reach > 16MB.
> > 
> >> now the driver defines a static array of structures like this:
> >>
> >> 	struct {
> >> 	  ...
> >> 	
> >> 	  struct mscp mscp[ULTRASTOR_MAX_CMDS];
> >> 	} config = {0};
> >>
> >> and allocates a struct mscp in .queuecommand like this:
> >> 	    my_mscp = &config.mscp[mscp_index];
> >>
> >> it will go on preparing this my_mscp structure including stuffing
> >> some mapped pointers. Lets put that aside for now.
> >> At the very end it will pass this my_mscp structure to the card's 
> >> firmware like this:
> >>
> >> 	    /* Store pointer in OGM address bytes */
> >> 	outl(isa_virt_to_bus(my_mscp), config.ogm_address);
> >>
> >> Now this is one hell of a smart ISA card. But putting this aside.
> >>
> >> if the machine has more then 2^24 of memory. Then this will never
> >> work, right? or I'm missing it completely?
> > 
> > It will definitely work for EISA and VL bus.  I think if you analyse the
> > placement of kernel data segments for compiled in drivers, it might also
> > work for ISA too, since I think the pfn will be low enough.  It should
> > fail as a module not just because the area will be out of range for ISA,
> > but also because the module data segment is in vmalloc space, so the
> > virt_to_bus assumptions of contiguity could be violated.
> > 
> > James
> > 
> 
> So what is the verdict? is it removed? marked broken for ISA?

It's probably obvious enough to apply the best straight line fix.

> can I safely say that unchecked_isa_dma can be removed?

No ... ISA definitely requires it.

James

Re: ultrastor.c is a bit-rot

[Boaz Harrosh]

On Mon, Mar 17 2008 at 18:03 +0200, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> On Mon, 2008-03-17 at 18:00 +0200, Boaz Harrosh wrote:
>> On Mon, Mar 17 2008 at 17:23 +0200, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
>>> On Mon, 2008-03-17 at 16:59 +0200, Boaz Harrosh wrote:
>>>> Inspecting ultrastor.c it is clear to me that this was never used for
>>>> a loooooooooong time. Not since a PC has more then 2^24 bit of memory.
>>>> Let me explain below.
>>>>
>>>> Now I'm not saying it should be fixed. I'm saying that it should be dumped
>>>> in the account that it is not used by any one and that it does not work.
>>>>
>>>> Why it never worked?
>>>> ~~~~~~~~~~~~~~~~~~~~~
>>>>
>>>> The driver's header says it supports 3 cards
>>>>
>>>>  *	14F - ISA first-party DMA HA with floppy support and WD1003 emulation.
>>>>  *	24F - EISA Bus Master HA with floppy support and WD1003 emulation.
>>>>  *	34F - VL-Bus Bus Master HA with floppy support (no WD1003 emulation).
>>>>
>>>> But Kconfig only specifies ISA. I'm not sure what a VL-Bus is.
>>> VL is vesa local ... it was an ISA like graphics bus that was fast and
>>> could reach > 16MB.
>>>
>>>> now the driver defines a static array of structures like this:
>>>>
>>>> 	struct {
>>>> 	  ...
>>>> 	
>>>> 	  struct mscp mscp[ULTRASTOR_MAX_CMDS];
>>>> 	} config = {0};
>>>>
>>>> and allocates a struct mscp in .queuecommand like this:
>>>> 	    my_mscp = &config.mscp[mscp_index];
>>>>
>>>> it will go on preparing this my_mscp structure including stuffing
>>>> some mapped pointers. Lets put that aside for now.
>>>> At the very end it will pass this my_mscp structure to the card's 
>>>> firmware like this:
>>>>
>>>> 	    /* Store pointer in OGM address bytes */
>>>> 	outl(isa_virt_to_bus(my_mscp), config.ogm_address);
>>>>
>>>> Now this is one hell of a smart ISA card. But putting this aside.
>>>>
>>>> if the machine has more then 2^24 of memory. Then this will never
>>>> work, right? or I'm missing it completely?
>>> It will definitely work for EISA and VL bus.  I think if you analyse the
>>> placement of kernel data segments for compiled in drivers, it might also
>>> work for ISA too, since I think the pfn will be low enough.  It should
>>> fail as a module not just because the area will be out of range for ISA,
>>> but also because the module data segment is in vmalloc space, so the
>>> virt_to_bus assumptions of contiguity could be violated.
>>>
>>> James
>>>
>> So what is the verdict? is it removed? marked broken for ISA?
> 
> It's probably obvious enough to apply the best straight line fix.
> 
>> can I safely say that unchecked_isa_dma can be removed?
> 
> No ... ISA definitely requires it.
> 
> James
> 

In Hebrew we say: "You make me drink Kerosene". 
An "obvious enough to apply the best straight line fix" submitted below:

I say dump it, it's unused.

Boaz
---
From: Boaz Harrosh <bharrosh@panasas.com>
Date: Mon, 17 Mar 2008 18:40:03 +0200
Subject: [PATCH] ultrastor: Fix for ISA DMA allocation

"obvious enough to apply the best straight line fix" submitted
below.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
CC: Andi Kleen <ak@suse.de>
---
 drivers/scsi/ultrastor.c |   24 +++++++++++++++++++++---
 1 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ultrastor.c b/drivers/scsi/ultrastor.c
index f385dce..04441eb 100644
--- a/drivers/scsi/ultrastor.c
+++ b/drivers/scsi/ultrastor.c
@@ -255,8 +255,9 @@ static struct ultrastor_config
   unsigned long mscp_free;
 #endif
   volatile unsigned char aborted[ULTRASTOR_MAX_CMDS];
-  struct mscp mscp[ULTRASTOR_MAX_CMDS];
-} config = {0};
+  struct mscp *mscp;
+  dma_addr_t dma;
+} config;
 
 /* Set this to 1 to reset the SCSI bus on error.  */
 static int ultrastor_bus_reset;
@@ -646,12 +647,29 @@ static int ultrastor_24f_detect(struct scsi_host_template * tpnt)
 
 static int ultrastor_detect(struct scsi_host_template * tpnt)
 {
+	int ret;
+
 	tpnt->proc_name = "ultrastor";
-	return ultrastor_14f_detect(tpnt) || ultrastor_24f_detect(tpnt);
+
+	if (!config.mscp)
+		config.mscp = dma_alloc_coherent(NULL,
+			sizeof(*config.mscp) * ULTRASTOR_MAX_CMDS,
+			&config.dma, GFP_KERNEL);
+
+	ret = ultrastor_14f_detect(tpnt) || ultrastor_24f_detect(tpnt);
+
+	if (!ret)
+		dma_free_coherent(NULL,
+			sizeof(*config.mscp) * ULTRASTOR_MAX_CMDS,
+			config.mscp, config.dma);
+	return ret;
 }
 
 static int ultrastor_release(struct Scsi_Host *shost)
 {
+	dma_free_coherent(NULL, sizeof(*config.mscp) * ULTRASTOR_MAX_CMDS,
+			  config.mscp, config.dma);
+
 	if (shost->irq)
 		free_irq(shost->irq, NULL);
 	if (shost->dma_channel != 0xff)
-- 
1.5.3.3

Re: ultrastor.c is a bit-rot

[James Bottomley]

On Mon, 2008-03-17 at 19:01 +0200, Boaz Harrosh wrote:
> In Hebrew we say: "You make me drink Kerosene". 

Slàinte mhath as we say in English.

> An "obvious enough to apply the best straight line fix" submitted below:
> 
> I say dump it, it's unused.
> 
> Boaz
> ---
> From: Boaz Harrosh <bharrosh@panasas.com>
> Date: Mon, 17 Mar 2008 18:40:03 +0200
> Subject: [PATCH] ultrastor: Fix for ISA DMA allocation
> 
> "obvious enough to apply the best straight line fix" submitted
> below.
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> CC: Andi Kleen <ak@suse.de>
> ---
>  drivers/scsi/ultrastor.c |   24 +++++++++++++++++++++---
>  1 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/ultrastor.c b/drivers/scsi/ultrastor.c
> index f385dce..04441eb 100644
> --- a/drivers/scsi/ultrastor.c
> +++ b/drivers/scsi/ultrastor.c
> @@ -255,8 +255,9 @@ static struct ultrastor_config
>    unsigned long mscp_free;
>  #endif
>    volatile unsigned char aborted[ULTRASTOR_MAX_CMDS];
> -  struct mscp mscp[ULTRASTOR_MAX_CMDS];
> -} config = {0};
> +  struct mscp *mscp;
> +  dma_addr_t dma;
> +} config;
>  
>  /* Set this to 1 to reset the SCSI bus on error.  */
>  static int ultrastor_bus_reset;
> @@ -646,12 +647,29 @@ static int ultrastor_24f_detect(struct scsi_host_template * tpnt)
>  
>  static int ultrastor_detect(struct scsi_host_template * tpnt)
>  {
> +	int ret;
> +
>  	tpnt->proc_name = "ultrastor";
> -	return ultrastor_14f_detect(tpnt) || ultrastor_24f_detect(tpnt);
> +
> +	if (!config.mscp)
> +		config.mscp = dma_alloc_coherent(NULL,
> +			sizeof(*config.mscp) * ULTRASTOR_MAX_CMDS,
> +			&config.dma, GFP_KERNEL);

Error handling here, I'm afraid; dma_alloc_coherent can return NULL.

Other than that, looks great.

James

Re: ultrastor.c is a bit-rot

[Matthew Wilcox]

On Mon, Mar 17, 2008 at 04:59:02PM +0200, Boaz Harrosh wrote:
> The driver's header says it supports 3 cards
> 
>  *	14F - ISA first-party DMA HA with floppy support and WD1003 emulation.
>  *	24F - EISA Bus Master HA with floppy support and WD1003 emulation.
>  *	34F - VL-Bus Bus Master HA with floppy support (no WD1003 emulation).
> 
> But Kconfig only specifies ISA. I'm not sure what a VL-Bus is.

VESA Local Bus.  It was (in some sense) the predecessor of AGP.  We
treat it like ISA inside the kernel.  On x86, EISA depends on ISA, so
the dependency, while wrong, does not affect any x86 users who have an
EISA card.

> now the driver defines a static array of structures like this:
> 
> 	struct {
> 	  ...
> 	
> 	  struct mscp mscp[ULTRASTOR_MAX_CMDS];
> 	} config = {0};
> 
> and allocates a struct mscp in .queuecommand like this:
> 	    my_mscp = &config.mscp[mscp_index];
> 
> it will go on preparing this my_mscp structure including stuffing
> some mapped pointers. Lets put that aside for now.
> At the very end it will pass this my_mscp structure to the card's 
> firmware like this:
> 
> 	    /* Store pointer in OGM address bytes */
> 	outl(isa_virt_to_bus(my_mscp), config.ogm_address);
> 
> Now this is one hell of a smart ISA card. But putting this aside.
> 
> if the machine has more then 2^24 of memory. Then this will never
> work, right? or I'm missing it completely?

I believe this will work for VESA and EISA cards, though not, indeed for
ISA cards.

> (Also none of the emails in this file are valid)

That doesn't mean it doesn't have users ...

On the other hand, the u14-34f driver is supposed to be preferred to
the ultrastor drivers.  Only problem: it doesn't support the 24F card.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: ultrastor.c is a bit-rot

[H. Peter Anvin]

Matthew Wilcox wrote:
> 
> VESA Local Bus.  It was (in some sense) the predecessor of AGP.  We
> treat it like ISA inside the kernel.  On x86, EISA depends on ISA, so
> the dependency, while wrong, does not affect any x86 users who have an
> EISA card.
> 

Well, one can say VLB was to ISA (a souped-up ISA bus with some of the 
worst limitations removed) what AGP is to PCI...

	-hpa

Re: ultrastor.c is a bit-rot

[Boaz Harrosh]

On Fri, Mar 21 2008 at 22:41 +0200, "H. Peter Anvin" <hpa@zytor.com> wrote:
> Matthew Wilcox wrote:
>> VESA Local Bus.  It was (in some sense) the predecessor of AGP.  We
>> treat it like ISA inside the kernel.  On x86, EISA depends on ISA, so
>> the dependency, while wrong, does not affect any x86 users who have an
>> EISA card.
>>
> 
> Well, one can say VLB was to ISA (a souped-up ISA bus with some of the 
> worst limitations removed) what AGP is to PCI...
> 
> 	-hpa

Hmm interesting, so someone took the VGA thing and made a storage device
for it. Did that ever happen with AGP? Any AGP scsi cards. I guess I can
Google for it.

Boaz

Re: ultrastor.c is a bit-rot

[H. Peter Anvin]

Boaz Harrosh wrote:
> On Fri, Mar 21 2008 at 22:41 +0200, "H. Peter Anvin" <hpa@zytor.com> wrote:
>> Matthew Wilcox wrote:
>>> VESA Local Bus.  It was (in some sense) the predecessor of AGP.  We
>>> treat it like ISA inside the kernel.  On x86, EISA depends on ISA, so
>>> the dependency, while wrong, does not affect any x86 users who have an
>>> EISA card.
>>>
>> Well, one can say VLB was to ISA (a souped-up ISA bus with some of the 
>> worst limitations removed) what AGP is to PCI...
>>
>> 	-hpa
> 
> Hmm interesting, so someone took the VGA thing and made a storage device
> for it. Did that ever happen with AGP? Any AGP scsi cards. I guess I can
> Google for it.

A lot less likely, IMO.  Unlike AGP-PCI, VLB slots were compatible with 
ISA cards, and the electricals were less complex, so several motherboard 
manufacturers built boards with 3 or more VESA slots (I think I saw a 
board with 6 at one point.)

AGP electricals pretty much require that it be point to point.

	-hpa

Re: ultrastor.c is a bit-rot

[Al Viro]

On Sun, Mar 23, 2008 at 09:24:03AM -0700, H. Peter Anvin wrote:

> >Hmm interesting, so someone took the VGA thing and made a storage device
> >for it. Did that ever happen with AGP? Any AGP scsi cards. I guess I can
> >Google for it.
> 
> A lot less likely, IMO.  Unlike AGP-PCI, VLB slots were compatible with 
> ISA cards, and the electricals were less complex, so several motherboard 
> manufacturers built boards with 3 or more VESA slots (I think I saw a 
> board with 6 at one point.)
> 
> AGP electricals pretty much require that it be point to point.

... not that VLB didn't suck badly with several devices attached at the same
time.  I would be rather surprised if that board would work if fully populated
with VLB cards, all in active use.  IOW, a likely explanation is that it
had a warning along the lines of "use of more than <number> of VLB cards
at the same time may use instability" buried in documentation and proud
"6 VLB slots!!!" touted by marketing...

I can't find VLB specs online, but IIRC 3 had been the limit and anything
past that had been very much out of spec and likely to screw you.

Re: ultrastor.c is a bit-rot

[H. Peter Anvin]

Al Viro wrote:
> 
> ... not that VLB didn't suck badly with several devices attached at the same
> time.  I would be rather surprised if that board would work if fully populated
> with VLB cards, all in active use.  IOW, a likely explanation is that it
> had a warning along the lines of "use of more than <number> of VLB cards
> at the same time may use instability" buried in documentation and proud
> "6 VLB slots!!!" touted by marketing...
> 
> I can't find VLB specs online, but IIRC 3 had been the limit and anything
> past that had been very much out of spec and likely to screw you.

I think that particular board had multiple root drivers (it was a P5 
board, so it wasn't a "local" bus anyway.)

	-hpa

Re: ultrastor.c is a bit-rot

[Alan Cox]

> if the machine has more then 2^24 of memory. Then this will never
> work, right? or I'm missing it completely?

It will work compiled in, and probably users of such cards if any don't
have > 16MB. I doubt anyone is using the driver however.

Alan

Re: ultrastor.c is a bit-rot

[Benny Amorsen]

Boaz Harrosh <bharrosh@panasas.com> writes:

> if the machine has more then 2^24 of memory. Then this will never
> work, right? or I'm missing it completely?

I used to have a machine with the VL-bus version of that card. I spent
a fortune upgrading that machine from 8x1MB to 4x4MB + 4x1MB, so it
did indeed work with larger than 16MB. The card got awfully hot and it
only worked when clocked at 33MHz (not the 40MHz the PC could do
without the card), but otherwise it worked fine.

I'm not sure whether that machine made it to kernel 2.0 though.


/Benny