Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Justin T. Gibbs]

The latest installments of the Aic7xxx (Legacy -> U160) and Aic79xx (U320)
driver are now available here:

http://people.FreeBSD.org/~gibbs/linux/RPM  # RPMs for popular distributions
http://people.FreeBSD.org/~gibbs/linux/DUD  # Driver Update Diskettes
http://people.FreeBSD.org/~gibbs/linux/SRC  # Source tarballs for 2.4.X and 
					      2.5.x relative to today's
					      bk trees.

When compared to the versions of the driver currently embedded in either
the 2.4.X or the 2.5.X (at least for the U160 driver), the fixes and changes
are so numerous as to make it difficult to name them all.  Highlights
include:

o Auto-detection and fallback to PIO for broken VIA chipsets with byte
  merging enabled.
o Domain Validation
o Support for the 2.5.X dynamic queue depth facility
o Support for the 2.5.X slave alloc/configure/destroy API
o The shutdown hook has been disabled on 2.5.X to avoid a panic
  on shutdown since the hooks are called to early in that kernel series.
  I'm still working on an alternative.  In the mean time, aic785X based
  cards may not play nice with the Adaptec BIOS on a warm reboot.
o Even more workarounds for deadlocks in the 2.4.X SCSI error recovery code.
  We now "spoon feed" transactions that have failure codes back to the
  mid-layer so as to avoid the stack blow-out from recursion that can occur
  in scsi_queue_next_request.

--
Justin

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Christoph Hellwig]

On Mon, Dec 09, 2002 at 04:53:18PM -0700, Justin T. Gibbs wrote:
> http://people.FreeBSD.org/~gibbs/linux/RPM  # RPMs for popular distributions
> http://people.FreeBSD.org/~gibbs/linux/DUD  # Driver Update Diskettes
> http://people.FreeBSD.org/~gibbs/linux/SRC  # Source tarballs for 2.4.X and 
> 					      2.5.x relative to today's
> 					      bk trees.

Any chance you could provide patches?

I'm not sure how familar you are with BK, but if you didn't know it yet,
they are easily produced by

bk export -tpatch -r<last changeset from upstream>,<your last changeset>

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Justin T. Gibbs]

> On Mon, Dec 09, 2002 at 04:53:18PM -0700, Justin T. Gibbs wrote:
>> http://people.FreeBSD.org/~gibbs/linux/RPM  # RPMs for popular
>> distributions http://people.FreeBSD.org/~gibbs/linux/DUD  # Driver
>> Update Diskettes http://people.FreeBSD.org/~gibbs/linux/SRC  # Source
>> tarballs for 2.4.X and  2.5.x relative to today's
>> 					      bk trees.
> 
> Any chance you could provide patches?

I don't see why they are necessary.  The usual reasons for patches are:

1) To review the changes
   With the exception of Doug Ledford, I doubt that many others bother
   to understand how the aic7xxx drivers work

2) To avoid conflicts with other developers
   Who else is maintaining this driver?  Except for a few minor changes
   to the SCSI Makefile and Kconfig/Config.in file, all of the changes
   are contained within the aic7xxx directory.

3) Because the complete files will only work on a paticular kernel version.
   The drop in files should work in all 2.4.X and 2.5.X kernels until the
   next SCSI mid-layer API change comes down the pipe.

4) Because the patches are smaller.
   The aic79xx driver is all "added" files.  The changes since either
   6.2.4 or 6.2.8 are so large that unified diffs are larger than just
   providing the files wholesale.

5) So that when a new kernel version is released all of the patches stop
   working and I need to regenerate them. 8-)

Of course, I'll happily generate patches against whatever kernel versions
the power that be want me to if it will help a driver update to occur.
6.2.8 is so old that its silly and 2.5.X is still using 6.2.4!  The aic79xx
driver has been out since May and its still not in either 2.4.X or 2.5.X.
The BK generated patchsets I've made in the past didn't seem to help things,
but I'm willing to be proven wrong.

--
Justin

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Christoph Hellwig]

On Mon, Dec 09, 2002 at 05:33:40PM -0700, Justin T. Gibbs wrote:
> I don't see why they are necessary.  The usual reasons for patches are:
> 
> 1) To review the changes
>    With the exception of Doug Ledford, I doubt that many others bother
>    to understand how the aic7xxx drivers work

Even if I don't unserstand all of the aic7xxx driver I do understand the linux
scsi midlayer and DDI interface.

> 
> 2) To avoid conflicts with other developers
>    Who else is maintaining this driver?  Except for a few minor changes
>    to the SCSI Makefile and Kconfig/Config.in file, all of the changes
>    are contained within the aic7xxx directory.

Even if not many people are working on aic7xx directly the scsi midlayer
and the linux DDI are changing.  I.e. your tarball that I downloaded
yetserday doesn't seem to deal with Doug's slave_* changes yet.

Similarly I am working on making aic7xxx exploit the PCI driver API
properly and use scsi_add_host/scsi_remove_host to properly support
hotplugging.  Having the diffs to look at makes merging a lot easier.

> 
> Of course, I'll happily generate patches against whatever kernel versions
> the power that be want me to if it will help a driver update to occur.

I'll have a box with three aic7xxx of different types and a hotplug pci
controller at work and I'd like to help integrating both your updates and
my hotplug pci changes (which I'll send you and linux-scsi for review one
done of course) into mainline.

For 2.4 the APIs are stable so Alan and Marcelo should just grab it.  Of
course they prefer patches for applying because that's how linux development
works.. :)

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[James Bottomley]

The tarball contained a lot of extraneous files, but I think I got them all 
weeded out.  I've but the result at:

http://linux-scsi.bkbits.net/scsi-aic7xxx-2.5

it needs to be tested to make sure I captured everything.

Justin, this would be why patches are a lot easier to handle:  we have 
automated scripts to import them rather than having to do error prone manual 
steps.  All I need to know for a patch is where it was based, and I can get 
bitkeeper and the tools to do the rest.

> I'll have a box with three aic7xxx of different types and a hotplug
> pci controller at work and I'd like to help integrating both your
> updates and my hotplug pci changes (which I'll send you and linux-scsi
> for review one done of course) into mainline. 

Hey, I think you just got elected chief tester.  Let me know how the above 
archive fares (and any misc patches that are needed).

Thanks,

James

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Justin T. Gibbs]

> The tarball contained a lot of extraneous files, but I think I got them
> all  weeded out.  I've but the result at:
> 
> http://linux-scsi.bkbits.net/scsi-aic7xxx-2.5
> 
> it needs to be tested to make sure I captured everything.

I've updated the tarfiles.  For some reason my --exclude directive
didn't stick the first time.  <sigh>

> Justin, this would be why patches are a lot easier to handle:  we have 
> automated scripts to import them rather than having to do error prone
> manual  steps.  All I need to know for a patch is where it was based, and
> I can get  bitkeeper and the tools to do the rest.

Unfortunately, even though I've setup a BK repository in an attempt
to make this task easier, BK doesn't do what I want.  It seems that
only if I can push to a public repository or can export my repository
for others to pull from, will BK auto-matically figure out which change
sets don't exist where and generate the correct stuff.  Unfortunately
my IT department will not allow me to export a BK server, so that is
out of the question.  Anyone care to tell me the magic incantation to
get BK to generate a cumulative patch set against the parent repository?
Or must I do a second clone and manually create patches?

--
Justin

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[James Bottomley]

gibbs@scsiguy.com said:
> Unfortunately, even though I've setup a BK repository in an attempt to
> make this task easier, BK doesn't do what I want.  It seems that only
> if I can push to a public repository or can export my repository for
> others to pull from, will BK auto-matically figure out which change
> sets don't exist where and generate the correct stuff.  Unfortunately
> my IT department will not allow me to export a BK server, so that is
> out of the question.  Anyone care to tell me the magic incantation to
> get BK to generate a cumulative patch set against the parent
> repository? Or must I do a second clone and manually create patches? 

You can "borrow" a public repository on bkbits.net.  See 
http://www.bitkeeper.com/Hosted.html

There isn't a way to diff two respositories, but you can see the differences 
with bk changes -{L|R} <other repository> and make diffs for each change set. 
(alternatively, if the child is just the parent with only your patches tacked 
on, you can diff from the common ancestor).

James

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Justin T. Gibbs]

> Okay, here's a lightly tested patch to get it in shape and
> compile/useable..

Why is this based on Alpha1 and not Alpha2.  Several of the things
you fixed are already fixed in Alpha2.  I just redownloaded what
I put up on my site and, as an example, verified that both the
pdev->driver_data and slave_alloc/configure/destroy stuff is already
in there (I wouldn't have been able to build on 2.5.X if it weren't).

Also, changes that remove compatibility ifdefs will be ignored.  This
driver has to build all the way back to 2.4.7 (RedHat 7.2 support).
Removing ifdefs just makes it harder for me to merge in changes from
external trees.  Yes, the ifdefs are ugly, but so is the Linux SCSI layer
and the unmanaged way that interfaces have been changed without any
consideration to backwards compatibility (eg. the HIGHIO stuff could have
been done with 0 impact to drivers, but wasn't for some strange reason).

>   - fix kbuild integration

Can you explain what failed before?  I don't mind using aic7xxx-y
or aic79xx-y instead of *-obj, but I would like to understand what
bug this fixes.

Thanks,
Justin

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Christoph Hellwig]

On Wed, Dec 11, 2002 at 08:18:26AM -0700, Justin T. Gibbs wrote:
> > Okay, here's a lightly tested patch to get it in shape and
> > compile/useable..
> 
> Why is this based on Alpha1 and not Alpha2.

Because that's a) what James put in the BK tree and b) that's what I downloaded
from your website today for reference.

> driver has to build all the way back to 2.4.7 (RedHat 7.2 support).

RedHat only supports kernel 2.4.18 for RH7.2/i386 and 2.4.9 with tons of
hacks for the other arches, so your argument is void.  Not to mention the only
support that is dropped by my changes is for builtin support < 2.2.18.

> Removing ifdefs just makes it harder for me to merge in changes from
> external trees.  Yes, the ifdefs are ugly, but so is the Linux SCSI layer
> and the unmanaged way that interfaces have been changed without any
> consideration to backwards compatibility (eg. the HIGHIO stuff could have
> been done with 0 impact to drivers, but wasn't for some strange reason).

linux driver interface change between stable series, face it.  And a driver
with tons of ifdefs is utter crap.  Interestingly only vendor driver use
that shitty scheme.

> 
> >   - fix kbuild integration
> 
> Can you explain what failed before?  I don't mind using aic7xxx-y
> or aic79xx-y instead of *-obj, but I would like to understand what
> bug this fixes.

AIC79xx support could be selected without PCI beeing set, letting
the makefile silently not build it, the driver wouldn't compile with
objdir != srcdir and it used more makefile code then nessecary.

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Justin T. Gibbs]

>> Why is this based on Alpha1 and not Alpha2.
> 
> Because that's a) what James put in the BK tree and b) that's what I
> downloaded from your website today for reference.

Where did you download it from.  This file is Alpha2 based:

http://people.FreeBSD.org/~gibbs/linux/SRC/aic79xx-linux-2.5.tar.gz

>> driver has to build all the way back to 2.4.7 (RedHat 7.2 support).
> 
> RedHat only supports kernel 2.4.18 for RH7.2/i386 and 2.4.9 with tons of
> hacks for the other arches, so your argument is void.

Tell that to the OEMs that Adaptec has to support.  They still test and
require 7.2 drivers.

> Not to mention the
> only support that is dropped by my changes is for builtin support <
> 2.2.18.

I'll have to verify that.

>> Removing ifdefs just makes it harder for me to merge in changes from
>> external trees.  Yes, the ifdefs are ugly, but so is the Linux SCSI layer
>> and the unmanaged way that interfaces have been changed without any
>> consideration to backwards compatibility (eg. the HIGHIO stuff could have
>> been done with 0 impact to drivers, but wasn't for some strange reason).
> 
> linux driver interface change between stable series, face it.

I have no problem with interfaces changing for good reason, but, for
example,
a driver that alread sets unchecked_isa_dma to 0 and uses the PCI dma mask
shouldn't have to set addition flags (with different names in different
vendor's trees) to enable HIGHIO.  It's yet-another *stupid* interface
change.

> And a driver with tons of ifdefs is utter crap.

I agree with that, but since we have to build for something like 35
different
kernel versions (major vendor releases plus all of their errata), there is
not much choice as a vendor.  Without the ifdefs, one of those builds 
invariably gets broken by a minor driver update because the person doing
the update doesn't understand how their change will effect the support for
the other kernels.

> Interestingly only vendor driver use that shitty scheme.

Yes, because the vendor actually has to support all of those versions
unlike some guy who hacks this stuff in his spare time and could care
less about anyone else's requirements but his own.  So your choice becomes
either accept the ifdefs so that the vendor can sell its products by meeting
OEM/Channel demands, or have the vendor exit the Linux market and not even
attempt to support their product.

--
Justin

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Christoph Hellwig]

On Wed, Dec 11, 2002 at 09:08:26AM -0700, Justin T. Gibbs wrote:
> >> Why is this based on Alpha1 and not Alpha2.
> > 
> > Because that's a) what James put in the BK tree and b) that's what I
> > downloaded from your website today for reference.
> 
> Where did you download it from.  This file is Alpha2 based:
> 
> http://people.FreeBSD.org/~gibbs/linux/SRC/aic79xx-linux-2.5.tar.gz

Yes, from exactly that location.  But it seems like a broken proxy cached
the old version, I've downloaded it on another host now and it's uptodate.

> >> driver has to build all the way back to 2.4.7 (RedHat 7.2 support).
> > 
> > RedHat only supports kernel 2.4.18 for RH7.2/i386 and 2.4.9 with tons of
> > hacks for the other arches, so your argument is void.
> 
> Tell that to the OEMs that Adaptec has to support.  They still test and
> require 7.2 drivers.

RedHat's current, supported kernels for RHL7.2 are 2.4.18-18.7.x (i386),
2.4.9-40 (ia64) and 2.4.9-38 (s390, but that one doesn't support pci
scsi cards anyway..).

> I have no problem with interfaces changing for good reason, but, for
> example,
> a driver that alread sets unchecked_isa_dma to 0 and uses the PCI dma mask
> shouldn't have to set addition flags (with different names in different
> vendor's trees) to enable HIGHIO.  It's yet-another *stupid* interface
> change.

Maybe you could have complained about that more than one year ago when the
patch came up first?

> > Interestingly only vendor driver use that shitty scheme.
> 
> Yes, because the vendor actually has to support all of those versions
> unlike some guy who hacks this stuff in his spare time and could care
> less about anyone else's requirements but his own.

Umm, if you would properly feed your patches upstream vendors could include
current versions easily and there would be no need for extra applied patches.

It's pretty simple.    If on the other hand the vendor thinks he should
support every stupid RedHat and SuSE release (and it really hurts to see
what these companies do to their release kernels) it's their problem.  But
having all those ifdefs in the source in mainline is horribly ugly.  Note
that there are a few drivers that properly use the newest API and use
a compat header to map it to older interface on older kernels (that's not
always possible for scsi because the interface is still to crappy to keep it,
I hope that will change once 2.6 is done).

Okay, let's stop that discussion that doesn't bring us forward anyway,
and try to get your changes in the mainline kernel.  I'll merge your
new driver into the BK tree where I did the other work and send your the full
diff to your release.  IT would be nice if you could submit small
incremental diffs for each new release after that, okay?

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Jens Axboe]

On Wed, Dec 11 2002, Christoph Hellwig wrote:
> > I have no problem with interfaces changing for good reason, but, for
> > example,
> > a driver that alread sets unchecked_isa_dma to 0 and uses the PCI dma mask
> > shouldn't have to set addition flags (with different names in different
> > vendor's trees) to enable HIGHIO.  It's yet-another *stupid* interface
> > change.
> 
> Maybe you could have complained about that more than one year ago when the
> patch came up first?

And I still dont see a better way to do it. Remember that this is 2.4
and we must be able to toggle the highmem io capability on a per-driver
basis easily and default to off until a given piece of hardware (and
driver) has been verified.

Saying the high io stuff could have been done with zero impact to
drivers just shows that you have no idea what you are talking about
here and are living in your Justin world again. Tons of drivers needed
to be changed to be able to deal with highmem pages sanely.

-- 
Jens Axboe

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Justin T. Gibbs]

> On Wed, Dec 11 2002, Christoph Hellwig wrote:
>> > I have no problem with interfaces changing for good reason, but, for
>> > example,
>> > a driver that alread sets unchecked_isa_dma to 0 and uses the PCI dma
>> > mask shouldn't have to set addition flags (with different names in
>> > different vendor's trees) to enable HIGHIO.  It's yet-another *stupid*
>> > interface change.
>> 
>> Maybe you could have complained about that more than one year ago when
>> the patch came up first?
> 
> And I still dont see a better way to do it. Remember that this is 2.4
> and we must be able to toggle the highmem io capability on a per-driver
> basis easily and default to off until a given piece of hardware (and
> driver) has been verified.

>From the perspective of a driver that already meets the requirements
for highio, it is simply frustrating that:

1) You have to set a flag when you've already told the system you
   dma capabilities.

2) That flag is not documented in hosts.h or in the Documentation directory.

3) No warning is given if you use pci_set_dma_mask without setting
highmem_io
   so that you know that your driver needs to be updated.

4) highio requires that all SCSI drivers support single length S/G lists,
   but since single buffers are still allowed by the interfaces, even
   compliant SCSI drivers cannot strip out this code.
 
> Saying the high io stuff could have been done with zero impact to
> drivers just shows that you have no idea what you are talking about
> here and are living in your Justin world again. Tons of drivers needed
> to be changed to be able to deal with highmem pages sanely.

I'm not doubting that "lots of drivers needed to be updated", but since you
had to touch these drivers anyway, you could have deprecated all of these
older stupid interfaces and effected a real cleanup without needing a
positive "highmem enabled" flag.  The subset of devices that used
pci_set_dma_mask() and were not compliant was probably small.  Fixing
them with your initial patch set would have made the proper use of the
PCI dma API the "marker" for a highmem enabled device. Instead, drivers
that already were compliant to the PCI dma spec silently started bouncing
pages again unless you knew about this poorly documented flag.  <ARGGGGHHHH>

--
Justin

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Jens Axboe]

On Thu, Dec 12 2002, Justin T. Gibbs wrote:
> > On Wed, Dec 11 2002, Christoph Hellwig wrote:
> >> > I have no problem with interfaces changing for good reason, but, for
> >> > example,
> >> > a driver that alread sets unchecked_isa_dma to 0 and uses the PCI dma
> >> > mask shouldn't have to set addition flags (with different names in
> >> > different vendor's trees) to enable HIGHIO.  It's yet-another *stupid*
> >> > interface change.
> >> 
> >> Maybe you could have complained about that more than one year ago when
> >> the patch came up first?
> > 
> > And I still dont see a better way to do it. Remember that this is 2.4
> > and we must be able to toggle the highmem io capability on a per-driver
> > basis easily and default to off until a given piece of hardware (and
> > driver) has been verified.
> 
> >From the perspective of a driver that already meets the requirements
> for highio, it is simply frustrating that:
> 
> 1) You have to set a flag when you've already told the system you
>    dma capabilities.

All drivers blindly copy the setting of the dma mask, typically means
nothing.

> 2) That flag is not documented in hosts.h or in the Documentation directory.

That is true. Name of the member is a good clue, though.

> 3) No warning is given if you use pci_set_dma_mask without setting
> highmem_io so that you know that your driver needs to be updated.

Two different layers. One is the block layer entry, other deals with the
hardware.

> 4) highio requires that all SCSI drivers support single length S/G lists,
>    but since single buffers are still allowed by the interfaces, even
>    compliant SCSI drivers cannot strip out this code.

This is not a new requirement, it's just spelled out now. The special
casing of an sg request with one entry was silly, imho. And yes you do
have to support both single entry sg requests and non-sg requests. I
wouldn't mind getting rid of that, but this is the 2.4 series and
changes must be kept small(ish).

> > Saying the high io stuff could have been done with zero impact to
> > drivers just shows that you have no idea what you are talking about
> > here and are living in your Justin world again. Tons of drivers needed
> > to be changed to be able to deal with highmem pages sanely.
> 
> I'm not doubting that "lots of drivers needed to be updated", but since you
> had to touch these drivers anyway, you could have deprecated all of these
> older stupid interfaces and effected a real cleanup without needing a
> positive "highmem enabled" flag.  The subset of devices that used

If I had done that, there was no way we could have had this feature for
2.4. It is just too invasive.

> pci_set_dma_mask() and were not compliant was probably small.  Fixing
> them with your initial patch set would have made the proper use of the
> PCI dma API the "marker" for a highmem enabled device. Instead,

Again, way too much work. The concept of the highmem_io flag is simple.
If your driver complies with the pci dma api, set the flag and you are
now handed pages for io up to your pci dma mask. If you either don't
know about the flag or don't use the pci dma api, behaviour is
unchanged.

See this way I didn't need to touch _any_ drivers, and I didn't break
_any_ drivers. Drivers that are compliant just have to set the flag.

> drivers that already were compliant to the PCI dma spec silently
> started bouncing pages again unless you knew about this poorly
> documented flag.  <ARGGGGHHHH>

What are you talking about? If you didn't set the flag, behaviour is
*unchanged* from before. You always got highmem pages bounced.

As Christoph said, if you want to influence changes made to the kernel
it doesn't help to whine about them years after they were developed. If
you want to take the back seat to the linux kernel, fine, just dont come
complaining when you miss out on something. Your timing is just
excellent, too. You've even had months of 2.4.20-pre time (highmem stuff
was merged in 2.4.20-pre2, iirc), and the block-highmem patch had
existed in the public and in vendor kernels about a year before that.

-- 
Jens Axboe

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Christoph Hellwig]

On Thu, Dec 12, 2002 at 06:38:18PM +0100, Jens Axboe wrote:
> > 1) You have to set a flag when you've already told the system you
> >    dma capabilities.
> 
> All drivers blindly copy the setting of the dma mask, typically means
> nothing.

Maybe it's a bit to dangerous for 2.4, but for 2.5 Justin's suggestion
looks very nice, IMHO.


> > 2) That flag is not documented in hosts.h or in the Documentation directory.
> 
> That is true. Name of the member is a good clue, though.

Hmm, actually I documented it at some point, I just can't remember
in what tree that was..

> > 4) highio requires that all SCSI drivers support single length S/G lists,
> >    but since single buffers are still allowed by the interfaces, even
> >    compliant SCSI drivers cannot strip out this code.
> 
> This is not a new requirement, it's just spelled out now. The special
> casing of an sg request with one entry was silly, imho. And yes you do
> have to support both single entry sg requests and non-sg requests. I
> wouldn't mind getting rid of that, but this is the 2.4 series and
> changes must be kept small(ish).

IIRC Alan planned to kill non-sg requests for 2.5.  But I don't know how
far he got.

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Jens Axboe]

On Fri, Dec 13 2002, Christoph Hellwig wrote:
> On Thu, Dec 12, 2002 at 06:38:18PM +0100, Jens Axboe wrote:
> > > 1) You have to set a flag when you've already told the system you
> > >    dma capabilities.
> > 
> > All drivers blindly copy the setting of the dma mask, typically means
> > nothing.
> 
> Maybe it's a bit to dangerous for 2.4, but for 2.5 Justin's suggestion
> looks very nice, IMHO.

The whole discussion was about the 2.4 patch. For 2.5 I'm not adverse to
doing it this way.

-- 
Jens Axboe

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Alan Cox]

On Wed, 2002-12-11 at 16:08, Justin T. Gibbs wrote:
> Tell that to the OEMs that Adaptec has to support.  They still test and
> require 7.2 drivers.

Joyous. I guess you need the 2.4.7 drivers to install to update to the
errata though

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Justin T. Gibbs]

>> >   - fix kbuild integration
>> 
>> Can you explain what failed before?  I don't mind using aic7xxx-y
>> or aic79xx-y instead of *-obj, but I would like to understand what
>> bug this fixes.
> 
> AIC79xx support could be selected without PCI beeing set, letting
> the makefile silently not build it

Okay.  Sine the choice directive is now gone, is there a compelling reason
to put the "old" aic7xxx driver Kconfig directives in the same Kconfig
file as the new driver?  It is kind of nice to have the separation since
Adaptec cannot support the old driver.  I also take it that you were also
unable to make the choice directive do the right thing?  That was the
original reason for removing the "old driver" Kconfig directives.  The
way your patch stands now, I don't believe there is anything to prevent
both drivers from being compiled statically into the kernel.  If so, the
resulting kernel will not boot.

--
Justin

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Christoph Hellwig]

On Wed, Dec 11, 2002 at 10:31:05AM -0700, Justin T. Gibbs wrote:
> Okay.  Sine the choice directive is now gone, is there a compelling reason
> to put the "old" aic7xxx driver Kconfig directives in the same Kconfig
> file as the new driver?

I wanted to have it just below the other one.

> It is kind of nice to have the separation since
> Adaptec cannot support the old driver.

Well, it usually just works (TM) :)  But I don't really see a relation
between Kconfig entries and what's supported by whom.  The MAINTAINERS
file in the toplevel directory is the only place where certain drivers/
subsystems are claimed supported.

I also take it that you were also
> unable to make the choice directive do the right thing?  That was the
> original reason for removing the "old driver" Kconfig directives.  The
> way your patch stands now, I don't believe there is anything to prevent
> both drivers from being compiled statically into the kernel.  If so, the
> resulting kernel will not boot.

Yupp, it currently crashes when I have both compiled in.  Dough, any chance
you could fix that?  A PCI driver is not supposed to stop over already
claimed device.
I'll try to find out how to get the old depency for both not beeing allowed
to be compiled in into the new Kconfig scheme until then.

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Justin T. Gibbs]

> On Wed, Dec 11, 2002 at 10:31:05AM -0700, Justin T. Gibbs wrote:
>> Okay.  Sine the choice directive is now gone, is there a compelling
>> reason to put the "old" aic7xxx driver Kconfig directives in the same
>> Kconfig file as the new driver?
> 
> I wanted to have it just below the other one.

Well, what I originally did was split the Kconfig into Kconfig.aic7xxx
and Kconfig.aic79xx and source both of them.  Assuming the choice syntax
worked as expected, this should have allowed me to choose between the
new and old driver and just have the aic79xx driver handled separately.
I never got the choice stuff to work, so I punted.  I'd rather have the
split Kconfig's than have the aic7xxx_old stuff in the new driver's
Kconfig source.

>> It is kind of nice to have the separation since
>> Adaptec cannot support the old driver.
> 
> Well, it usually just works (TM) :)  But I don't really see a relation
> between Kconfig entries and what's supported by whom.  The MAINTAINERS
> file in the toplevel directory is the only place where certain drivers/
> subsystems are claimed supported.

I just mean in terms of having to maintain any new or changed settings
for Doug's driver in the Kconfig file that is largely for the new driver.
It just creates another point of coordination.

> I also take it that you were also
>> unable to make the choice directive do the right thing?  That was the
>> original reason for removing the "old driver" Kconfig directives.  The
>> way your patch stands now, I don't believe there is anything to prevent
>> both drivers from being compiled statically into the kernel.  If so, the
>> resulting kernel will not boot.
> 
> Yupp, it currently crashes when I have both compiled in.  Dough, any
> chance you could fix that?  A PCI driver is not supposed to stop over
> already claimed device.

If you use the sanctioned PCI entry points, the PCI code seems to take
care of this.  Unfortunately, aic7xxx_old still manually pokes at devices.
The bug will have to be fixed there.

> I'll try to find out how to get the old depency for both not beeing
> allowed to be compiled in into the new Kconfig scheme until then.

Okay.

--
Justin

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Doug Ledford]

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

On Wed, Dec 11, 2002 at 06:17:46PM +0000, Christoph Hellwig wrote:
> 
> Yupp, it currently crashes when I have both compiled in.  Dough, any chance
> you could fix that?  A PCI driver is not supposed to stop over already
> claimed device.

If you've got a test machine, the attached patch would I think fix the 
problem.  Let me know if it does in fact work for you and I'll commit it 
to the tree.

-- 
  Doug Ledford <dledford@redhat.com>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606
  

[-- Attachment #2: aic-playnice.patch --]
[-- Type: text/plain, Size: 3647 bytes --]

===== drivers/scsi/aic7xxx_old.c 1.37 vs edited =====
--- 1.37/drivers/scsi/aic7xxx_old.c	Fri Nov 22 00:34:44 2002
+++ edited/drivers/scsi/aic7xxx_old.c	Thu Dec 12 13:35:23 2002
@@ -9246,12 +9246,22 @@
 	    {
               /* duplicate PCI entry, skip it */
 	      kfree(temp_p);
-	      temp_p = NULL;
+              continue;
 	    }
 	    current_p = current_p->next;
 	  }
-	  if ( temp_p == NULL )
+          if(!pci_request_regions(temp_p->pdev, "aic7xxx"))
+          {
+            printk("aic7xxx: <%s> at PCI %d/%d/%d\n", 
+              board_names[aic_pdevs[i].board_name_index],
+              temp_p->pci_bus,
+              PCI_SLOT(temp_p->pci_device_fn),
+              PCI_FUNC(temp_p->pci_device_fn));
+            printk("aic7xxx: I/O ports already in use, ignoring.\n");
+            kfree(temp_p);
             continue;
+          }
+
           if (aic7xxx_verbose & VERBOSE_PROBE2)
             printk("aic7xxx: <%s> at PCI %d/%d\n", 
               board_names[aic_pdevs[i].board_name_index],
@@ -9283,20 +9293,6 @@
           pci_write_config_dword(pdev, DEVCONFIG, devconfig);
 #endif /* AIC7XXX_STRICT_PCI_SETUP */
 
-          if(temp_p->base && !request_region(temp_p->base, MAXREG - MINREG,
-				  "aic7xxx"))
-          {
-            printk("aic7xxx: <%s> at PCI %d/%d/%d\n", 
-              board_names[aic_pdevs[i].board_name_index],
-              temp_p->pci_bus,
-              PCI_SLOT(temp_p->pci_device_fn),
-              PCI_FUNC(temp_p->pci_device_fn));
-            printk("aic7xxx: I/O ports already in use, ignoring.\n");
-            kfree(temp_p);
-            temp_p = NULL;
-            continue;
-          }
-
           temp_p->unpause = INTEN;
           temp_p->pause = temp_p->unpause | PAUSE;
           if ( ((temp_p->base == 0) &&
@@ -9309,9 +9305,7 @@
               PCI_SLOT(temp_p->pci_device_fn),
               PCI_FUNC(temp_p->pci_device_fn));
             printk("aic7xxx: Controller disabled by BIOS, ignoring.\n");
-            kfree(temp_p);
-            temp_p = NULL;
-            continue;
+            goto skip_pci_controller;
           }
 
 #ifdef MMAPIO
@@ -9353,9 +9347,7 @@
                     PCI_SLOT(temp_p->pci_device_fn),
                     PCI_FUNC(temp_p->pci_device_fn));
                   printk("aic7xxx: Controller disabled by BIOS, ignoring.\n");
-                  kfree(temp_p);
-                  temp_p = NULL;
-                  continue;
+                  goto skip_pci_controller;
                 }
               }
             }
@@ -9398,10 +9390,7 @@
 
           if (aic7xxx_chip_reset(temp_p) == -1)
           {
-            release_region(temp_p->base, MAXREG - MINREG);
-            kfree(temp_p);
-            temp_p = NULL;
-            continue;
+            goto skip_pci_controller;
           }
           /*
            * Very quickly put the term setting back into the register since
@@ -9687,6 +9676,10 @@
           }
           temp_p->next = NULL;
           found++;
+	  continue;
+skip_pci_controller:
+	  pci_release_regions(temp_p->pdev);
+	  kfree(temp_p);
         }  /* Found an Adaptec PCI device. */
         else /* Well, we found one, but we couldn't get any memory */
         {
@@ -10969,14 +10962,16 @@
 
   if(p->irq)
     free_irq(p->irq, p);
-  if(p->base)
-    release_region(p->base, MAXREG - MINREG);
 #ifdef MMAPIO
   if(p->maddr)
   {
     iounmap((void *) (((unsigned long) p->maddr) & PAGE_MASK));
   }
 #endif /* MMAPIO */
+  if(!p->pdev)
+    release_region(p->base, MAXREG - MINREG);
+  else
+    pci_release_regions(p->pdev);
   prev = NULL;
   next = first_aic7xxx;
   while(next != NULL)

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Christoph Hellwig]

On Thu, Dec 12, 2002 at 03:20:52PM -0500, Doug Ledford wrote:
> > you could fix that?  A PCI driver is not supposed to stop over already
> > claimed device.
> 
> If you've got a test machine, the attached patch would I think fix the 
> problem.  Let me know if it does in fact work for you and I'll commit it 
> to the tree.

I'll test it as soon as I recovered from the company christmas party today..

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Justin T. Gibbs]

> On Wed, Dec 11, 2002 at 06:17:46PM +0000, Christoph Hellwig wrote:
>> 
>> Yupp, it currently crashes when I have both compiled in.  Dough, any
>> chance you could fix that?  A PCI driver is not supposed to stop over
>> already claimed device.
> 
> If you've got a test machine, the attached patch would I think fix the 
> problem.  Let me know if it does in fact work for you and I'll commit it 
> to the tree.

I would have to review the aic7xxx driver's probe and attach routine
to see if in the case of aic7xxx_old probing first all is well.  If you
could somehow get pci_dev->driver to get set, in the above load order,
aic7xxx would never even have its probe routine called.

--
Justin

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Christoph Hellwig]

On Thu, Dec 12, 2002 at 03:20:52PM -0500, Doug Ledford wrote:
> On Wed, Dec 11, 2002 at 06:17:46PM +0000, Christoph Hellwig wrote:
> > 
> > Yupp, it currently crashes when I have both compiled in.  Dough, any chance
> > you could fix that?  A PCI driver is not supposed to stop over already
> > claimed device.
> 
> If you've got a test machine, the attached patch would I think fix the 
> problem.  Let me know if it does in fact work for you and I'll commit it 
> to the tree.

Still doesn't work.  I think you really need to set and check the driver
field like justin suggested.

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Doug Ledford]

On Fri, Dec 13, 2002 at 09:02:15PM +0000, Christoph Hellwig wrote:
> On Thu, Dec 12, 2002 at 03:20:52PM -0500, Doug Ledford wrote:
> > On Wed, Dec 11, 2002 at 06:17:46PM +0000, Christoph Hellwig wrote:
> > > 
> > > Yupp, it currently crashes when I have both compiled in.  Dough, any chance
> > > you could fix that?  A PCI driver is not supposed to stop over already
> > > claimed device.
> > 
> > If you've got a test machine, the attached patch would I think fix the 
> > problem.  Let me know if it does in fact work for you and I'll commit it 
> > to the tree.
> 
> Still doesn't work.  I think you really need to set and check the driver
> field like justin suggested.

Hmmm...what's the failure mode here?  And if it doesn't work this way, 
then how's it ever suppossed to work on the non-PCI controllers both of 
our modules support since last I knew, you never get called by the pci hot 
plug core on non-PCI controllers so the lockout method between our drivers 
can *not* be pci specific and still work in all cases.  The request_region 
stuff always was and still is the only truly generic io space protection 
mechanism that works on all devices on all busses.

-- 
  Doug Ledford <dledford@redhat.com>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606
  

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Justin T. Gibbs]

> On Fri, Dec 13, 2002 at 09:02:15PM +0000, Christoph Hellwig wrote:
>> On Thu, Dec 12, 2002 at 03:20:52PM -0500, Doug Ledford wrote:
>> > On Wed, Dec 11, 2002 at 06:17:46PM +0000, Christoph Hellwig wrote:
>> > > 
>> > > Yupp, it currently crashes when I have both compiled in.  Dough, any
>> > > chance you could fix that?  A PCI driver is not supposed to stop
>> > > over already claimed device.
>> > 
>> > If you've got a test machine, the attached patch would I think fix the 
>> > problem.  Let me know if it does in fact work for you and I'll commit
>> > it  to the tree.
>> 
>> Still doesn't work.  I think you really need to set and check the driver
>> field like justin suggested.
> 
> Hmmm...what's the failure mode here?

I haven't tested it locally yet, so I'm not sure.  My concern was that
the PCI subsystem would simply not know that your driver had attached
which could lead to a second attach playing with BARs or other configuration
space stuff before the aic7xxx driver even tries to map registers.
This doesn't occur in the non-PCI case, so the request region stuff is
probably sufficient there.

--
Justin

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Christoph Hellwig]

On Fri, Dec 13, 2002 at 04:23:52PM -0500, Doug Ledford wrote:
> Hmmm...what's the failure mode here?

It complains that it can't reserve the mem regions but still continues to
setup, download sequencer code, and even register scsi3ff.

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Doug Ledford]

On Fri, Dec 13, 2002 at 09:51:13PM +0000, Christoph Hellwig wrote:
> On Fri, Dec 13, 2002 at 04:23:52PM -0500, Doug Ledford wrote:
> > Hmmm...what's the failure mode here?
> 
> It complains that it can't reserve the mem regions but still continues to
> setup, download sequencer code, and even register scsi3ff.

Hmmm...are you loading mine or Justin's driver first?  Mine should bail, 
not continue, when it can't reserve the requested region.

-- 
  Doug Ledford <dledford@redhat.com>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606
  

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Justin T. Gibbs]

> On Fri, Dec 13, 2002 at 09:51:13PM +0000, Christoph Hellwig wrote:
>> On Fri, Dec 13, 2002 at 04:23:52PM -0500, Doug Ledford wrote:
>> > Hmmm...what's the failure mode here?
>> 
>> It complains that it can't reserve the mem regions but still continues to
>> setup, download sequencer code, and even register scsi3ff.
> 
> Hmmm...are you loading mine or Justin's driver first?  Mine should bail, 
> not continue, when it can't reserve the requested region.

If the new driver is loaded second, you'll wind up with a config
space COMMAND register with both IO space and MEM space disabled.
The reason for this was to ensure that the aic7xxx controller was
not responding to a memory or I/O port already requested for a
*different device*.  I wasn't thinking about a *different driver*
accessing the same device.  There are other PCI api functions
that are called prior to the region check too (set powerstate, enable
device, set dma mask, etc.).  I just don't know how safe you can
really make it unless the old driver uses the PCI registration API.
The new driver will fail though if *both* its memory and port addresses
cannot be mapped.

In the other load order, the driver only reserves the region type it is
using.
The other region is simply disabled in the COMMAND register.  This is
probably why things are failing.  One driver is using mem I/O.  The other
is using PIO.

--
Justin

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Doug Ledford]

On Fri, Dec 13, 2002 at 04:08:17PM -0700, Justin T. Gibbs wrote:
> > On Fri, Dec 13, 2002 at 09:51:13PM +0000, Christoph Hellwig wrote:
> >> On Fri, Dec 13, 2002 at 04:23:52PM -0500, Doug Ledford wrote:
> >> > Hmmm...what's the failure mode here?
> >> 
> >> It complains that it can't reserve the mem regions but still continues to
> >> setup, download sequencer code, and even register scsi3ff.
> > 
> > Hmmm...are you loading mine or Justin's driver first?  Mine should bail, 
> > not continue, when it can't reserve the requested region.
> 
> If the new driver is loaded second, you'll wind up with a config
> space COMMAND register with both IO space and MEM space disabled.
> The reason for this was to ensure that the aic7xxx controller was
> not responding to a memory or I/O port already requested for a
> *different device*.

Two PCI devices sharing the same I/O or Mem space either one is invalid.  
If the space is allocated to the card, then it *belongs* to that card and
there should never be a conflict.  So, in my driver now, I switched to
using pci_request_regions() (pretty nice helper function actually, it
grabs *all* the pci resources that are assigned to that card and fails if
it can't get all of them).  That's the only thing that really made sense
to me.  Now, I think the *only* thing I might do differently is to move
the pci_enable_device() call to after the pci_request_regions() since
right now the pci_enable_device() is the first thing my driver does (I
wasn't sure, but I thought you might have to call pci_enable_device() to
trigger some resource allocations so that pci_request_regions() would have
all the regions it needs to reserve properly allocated).  But, that should
be fairly safe from a 2 driver standpoint (I'm making the assumption that
calling pci_enabled_device() on an already enabled and live card isn't a
problem, which I think is right but probably warrants a deeper look into
the pci subsys to be sure).  It's also pretty close to identical to what I
do in the eisa/vlb case.

>  I wasn't thinking about a *different driver*
> accessing the same device.  There are other PCI api functions
> that are called prior to the region check too (set powerstate, enable
> device, set dma mask, etc.).  I just don't know how safe you can
> really make it unless the old driver uses the PCI registration API.
> The new driver will fail though if *both* its memory and port addresses
> cannot be mapped.

I had a few things in there before the region check as well.  I just moved 
the region check up to the top, right after pci_enable_device().

> In the other load order, the driver only reserves the region type it is
> using.

Which I think is technically wrong IMHO.  Whether you use the I/O space or
not, it's been allocated to you by the BIOS/PCI subsystem.  If you can't
have control over an area allocated to you then there is a bogon hiding
somewhere in the woodpile.

> The other region is simply disabled in the COMMAND register.  This is
> probably why things are failing.  One driver is using mem I/O.  The other
> is using PIO.
> 
> --
> Justin

-- 
  Doug Ledford <dledford@redhat.com>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606
  

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Justin T. Gibbs]

>> If the new driver is loaded second, you'll wind up with a config
>> space COMMAND register with both IO space and MEM space disabled.
>> The reason for this was to ensure that the aic7xxx controller was
>> not responding to a memory or I/O port already requested for a
>> *different device*.
> 
> Two PCI devices sharing the same I/O or Mem space either one is invalid.

Sure.  But I've seen BIOSes that screw this up before, especially if
you have more devices in the system then you have I/O space to map them
all.  This becomes even more important in a PCI hot-plug environment.
I dont' want to reserve regions that I'm not going to use because that
resource space might be vital for some future hot-plug device.  The fact
that the BIOSes allocate all regions is a holdover from when OSes were
not PCI PNP capable.

>> In the other load order, the driver only reserves the region type it is
>> using.
> 
> Which I think is technically wrong IMHO.  Whether you use the I/O space or
> not, it's been allocated to you by the BIOS/PCI subsystem.  If you can't
> have control over an area allocated to you then there is a bogon hiding
> somewhere in the woodpile.

There are lots of PCI devices that have extra bars that a driver may or
may not choose to use.  In some cases, the memory regions supported by
those bars are *huge* but only one bar need be active at a time.  On some
devices it may be safe to allocate and just not use all BARs on the card,
but I don't think that it applies to all devices.  I would rather the PCI
subsystem defer to the device driver to tell it which regions are required
rather than attempt to allocate them all upfront and exhaust what is
a limited pool of resources.

--
Justin

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Gérard Roudier]

On Wed, 11 Dec 2002, Christoph Hellwig wrote:

[...]

> linux driver interface change between stable series, face it.  And a driver
> with tons of ifdefs is utter crap.  Interestingly only vendor driver use
> that shitty scheme.

You just miss that new hardware must be supported for both stable and
current kernels and that differences in driver interface use to be large
between adjacent stable and current in Linux. On the other hand driver
interfaces in current kernel seem to be changing as new features come to
mind to developpers. Add to that mess that not all Linux based O/S are
using kernel at same level.

Btw, people who want to learn about how to design and maintain a smart
driver interface that can evolve gracefully without breaking driver
zillions of times should ask Justin how it does this so well with
FreeBSD/CAM, in my opinion.

  Gérard.

-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Justin T. Gibbs]

> Btw, people who want to learn about how to design and maintain a smart
> driver interface that can evolve gracefully without breaking driver
> zillions of times should ask Justin how it does this so well with
> FreeBSD/CAM, in my opinion.
> 
>   Gérard.

Gerard,

The whole reason CAM has been sucessful was that the decision was made,
however painful it was, to completely redo a subsystem to fix the blatent
architectural flaws in it.  CAM was only integrated once all of the drivers
were ported and it had received extensive external testing.  I don't know
that I or anyone else can do much better with the Linux SCSI layer until
such time that someone decides to just nuke the whole thing and start over.
A hack, on top of a hack, on top of a hack is no way to build a solid 
foundation.  What's even more frustrating for me is that the hacks show up
in places you never expect and in forms that don't warn you if you are out
of compliance with the "new hack".  It makes it very difficult to follow
interface changes successfully.

--
Justin

-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: scsi_scan.c complaints...

[Doug Ledford]

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

OK, so I've been working on that patch.  I've got it close to done, but it 
oopses on me at bootup (somewhere I think there is a single init that I'm 
missing that causes the first command we insert into the request queue to 
oops).  So, I've got to go up to the office to get any further on this 
since I need my test machine and serial console setup.  But, in case 
people were curious what I did about the problem, I've attached the 
current form of the patch to this email.  If anyone sees an obvious 
oversight that is causing the problem, feel free to clue me in, otherwise 
I'll be working on it some more later today ;-)

-- 
  Doug Ledford <dledford@redhat.com>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606
  

[-- Attachment #2: scsi_scan-test.patch.gz --]
[-- Type: application/x-gzip, Size: 8841 bytes --]

Re: scsi_scan.c complaints...

[Doug Ledford]

On Thu, Dec 19, 2002 at 01:56:57PM -0500, Doug Ledford wrote:
> OK, so I've been working on that patch.  I've got it close to done, but it 
> oopses on me at bootup (somewhere I think there is a single init that I'm 
> missing that causes the first command we insert into the request queue to 
> oops).

And I was right.  One little q = NULL; is all that was missing.  Anyway, 
here's a printout of what startup looks like with this patch in place 
under 2.5.52.  This should make you happy Justin ;-)

[root@aladin root]# insmod /lib/modules/2.5.52/kernel/drivers/scsi/scsi_mod.ko
[root@aladin root]# insmod /lib/modules/2.5.52/kernel/drivers/scsi/sd_mod.ko
[root@aladin root]# insmod /lib/modules/2.5.52/kernel/drivers/scsi/aic7xxx_old.ko
[root@aladin root]# dmesg | tail -41
device class 'scsi-host': registering
(scsi0) <Adaptec AIC-7850 SCSI host adapter> found at PCI 0/9/0
(scsi0) Narrow Channel, SCSI ID=7, 3/255 SCBs
(scsi0) Cables present (Int-50 NO, Ext-50 YES)
(scsi0) Downloading sequencer code... 415 instructions downloaded
scsi0 : Adaptec AHA274x/284x/294x (EISA/VLB/PCI-Fast SCSI) 5.2.6/5.2.0
       <Adaptec AIC-7850 SCSI host adapter>
aic7xxx_old: slave_alloc 0/0/0
  Vendor: iomega    Model: jaz 2GB           Rev: E.17
  Type:   Direct-Access                      ANSI SCSI revision: 02
aic7xxx_old: slave_configure 0/0/0
aic7xxx_old: slave_alloc 0/1/0
  Vendor: IOMEGA    Model: ZIP 100           Rev: J.03
  Type:   Direct-Access                      ANSI SCSI revision: 02
aic7xxx_old: slave_configure 0/1/0
aic7xxx_old: slave_alloc 0/2/0
  Vendor: ARCHIVE   Model: Python 04106-XXX  Rev: 7350
  Type:   Sequential-Access                  ANSI SCSI revision: 02
aic7xxx_old: slave_configure 0/2/0
aic7xxx_old: slave_alloc 0/3/0
  Vendor: QUANTUM   Model: FIREBALL_TM3200S  Rev: 301B
  Type:   Direct-Access                      ANSI SCSI revision: 02
aic7xxx_old: slave_configure 0/3/0
aic7xxx_old: slave_alloc 0/4/0
  Vendor: PLEXTOR   Model: CD-ROM PX-4XCS    Rev: 1.04
  Type:   CD-ROM                             ANSI SCSI revision: 02
aic7xxx_old: slave_configure 0/4/0
aic7xxx_old: slave_alloc 0/5/0
aic7xxx_old: slave_destroy 0/5/0
aic7xxx_old: slave_alloc 0/6/0
aic7xxx_old: slave_destroy 0/6/0
(scsi0:0:0:0) Synchronous at 10.0 Mbyte/sec, offset 15.
SCSI device sda: drive cache: write back
Attached scsi removable disk sda at scsi0, channel 0, id 0, lun 0
SCSI device sdb: cache data unavailable
Attached scsi removable disk sdb at scsi0, channel 0, id 1, lun 0
(scsi0:0:3:0) Synchronous at 10.0 Mbyte/sec, offset 15.
SCSI device sdc: drive cache: write back
SCSI device sdc: 6281856 512-byte hdwr sectors (3216 MB)
 sdc: sdc1
Attached scsi disk sdc at scsi0, channel 0, id 3, lun 0
[root@aladin root]#



-- 
  Doug Ledford <dledford@redhat.com>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606
  

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[Andrew Morton]

James Bottomley wrote:
> 
> The tarball contained a lot of extraneous files, but I think I got them all
> weeded out.  I've but the result at:
> 
> http://linux-scsi.bkbits.net/scsi-aic7xxx-2.5
> 

Well I would test (and distribute) it too, but the above format is useless
to me, as it is to most others.

Re: Aic7xxx v6.2.22 and Aic79xx v1.3.0Alpha2 Released

[James Bottomley]

akpm@digeo.com said:
> Well I would test (and distribute) it too, but the above format is
> useless to me, as it is to most others.

If someone has an ftp server they could lend me, I'll dump patches against 
2.5.51 (or the later BK's) on it.  Alternatively, I could use my 
hansenpartnership website, but I don't want to run up against my (rather low) 
bandwidth limits.

James

Re: scsi_scan.c complaints...

[Christoph Hellwig]

On Fri, Dec 20, 2002 at 08:29:23PM -0500, Doug Ledford wrote:
> And I was right.  One little q = NULL; is all that was missing.  Anyway, 
> here's a printout of what startup looks like with this patch in place 
> under 2.5.52.  This should make you happy Justin ;-)

Okay, I looked at the patches that are in mainline and they look pretty
cool to me.  When looking over the code (in preparation of implementing
Justin's suggestion to get rid of the highmem_io flag) I found quite
a bit small stuff to make the code in that area a lot cleaner:

- new helper scsi_calculate_bounce_limit to calculate the bounce
  limit for a scsi host, remove a copy of that code ni st.c
- scsi_initialize_queue gets replace with scsi_alloc_queue, this
  one now takes only a struct Scsi_Host and returns the request queue,
  it's paired with a small scsi_free_queue helper.

Diffstat:

 hosts.h     |    3 -
 scsi.c      |   43 ----------------------
 scsi.h      |    1
 scsi_scan.c |  113 ++++++++++++++++++++++++++++++++++--------------------------
 scsi_syms.c |    5 ++
 st.c        |   16 +-------
 6 files changed, 73 insertions(+), 108 deletions(-)


--- 1.46/drivers/scsi/hosts.h	Thu Nov 28 14:36:44 2002
+++ edited/drivers/scsi/hosts.h	Sat Dec 21 16:47:36 2002
@@ -554,9 +554,6 @@
     struct device_driver scsi_driverfs_driver;
 };
 
-void  scsi_initialize_queue(Scsi_Device *, struct Scsi_Host *);
-
-
 /*
  * Highlevel driver registration/unregistration.
  */
--- 1.81/drivers/scsi/scsi.c	Fri Dec 20 20:59:59 2002
+++ edited/drivers/scsi/scsi.c	Sat Dec 21 16:34:24 2002
@@ -147,49 +147,6 @@
 extern void scsi_times_out(Scsi_Cmnd * SCpnt);
 void scsi_build_commandblocks(Scsi_Device * SDpnt);
 
-/*
- * Function:    scsi_initialize_queue()
- *
- * Purpose:     Sets up the block queue for a device.
- *
- * Arguments:   SDpnt   - device for which we need a handler function.
- *
- * Returns:     Nothing
- *
- * Lock status: No locking assumed or required.
- */
-void  scsi_initialize_queue(Scsi_Device * SDpnt, struct Scsi_Host * SHpnt)
-{
-	request_queue_t *q = SDpnt->request_queue;
-
-	/*
-	 * tell block layer about assigned host_lock for this host
-	 */
-	blk_init_queue(q, scsi_request_fn, SHpnt->host_lock);
-
-	/* Hardware imposed limit. */
-	blk_queue_max_hw_segments(q, SHpnt->sg_tablesize);
-
-	/*
-	 * scsi_alloc_sgtable max
-	 */
-	blk_queue_max_phys_segments(q, MAX_PHYS_SEGMENTS);
-
-	if(!SHpnt->max_sectors)
-		/* driver imposes no hard sector transfer limit.
-		 * start at machine infinity initially */
-		SHpnt->max_sectors = SCSI_DEFAULT_MAX_SECTORS;
-
-	/* FIXME: we should also adjust this limit later on
-	 * after we know what the device capabilities are */
-	blk_queue_max_sectors(q, SHpnt->max_sectors);
-
-	if (!SHpnt->use_clustering)
-		clear_bit(QUEUE_FLAG_CLUSTER, &q->queue_flags);
-
-        blk_queue_prep_rq(q, scsi_prep_fn);
-}
-
 #ifdef MODULE
 MODULE_PARM(scsi_logging_level, "i");
 MODULE_PARM_DESC(scsi_logging_level, "SCSI logging level; should be zero or nonzero");
--- 1.53/drivers/scsi/scsi.h	Fri Dec 20 20:59:59 2002
+++ edited/drivers/scsi/scsi.h	Sat Dec 21 16:54:22 2002
@@ -511,6 +511,7 @@
  */
 extern int scsi_add_single_device(uint, uint, uint, uint);
 extern int scsi_remove_single_device(uint, uint, uint, uint);
+extern u64 scsi_calculate_bounce_limit(struct Scsi_Host *);
 
 /*
  * Prototypes for functions in constants.c
--- 1.50/drivers/scsi/scsi_scan.c	Fri Dec 20 21:00:00 2002
+++ edited/drivers/scsi/scsi_scan.c	Sat Dec 21 17:44:03 2002
@@ -28,7 +28,6 @@
 #include <linux/config.h>
 #include <linux/module.h>
 #include <linux/init.h>
-
 #include <linux/blk.h>
 
 #include "scsi.h"
@@ -365,34 +364,60 @@
 		printk("\n");
 }
 
-/**
- * scsi_initialize_merge_fn() -Æ£initialize merge function for a host
- * @sd:		host descriptor
- */
-static void scsi_initialize_merge_fn(struct scsi_device *sd)
+u64 scsi_calculate_bounce_limit(struct Scsi_Host *shost)
 {
-	request_queue_t *q = sd->request_queue;
-	struct Scsi_Host *sh = sd->host;
-	struct device *dev = scsi_get_device(sh);
-	u64 bounce_limit;
-
-	if (sh->highmem_io) {
-		if (dev && dev->dma_mask && PCI_DMA_BUS_IS_PHYS) {
-			bounce_limit = *dev->dma_mask;
-		} else {
-			/*
-			 * Platforms with virtual-DMA translation
- 			 * hardware have no practical limit.
-			 */
-			bounce_limit = BLK_BOUNCE_ANY;
-		}
-	} else if (sh->unchecked_isa_dma) {
-		bounce_limit = BLK_BOUNCE_ISA;
-	} else {
-		bounce_limit = BLK_BOUNCE_HIGH;
+	if (shost->highmem_io) {
+		struct device *host_dev = scsi_get_device(shost);
+
+		if (PCI_DMA_BUS_IS_PHYS && host_dev && host_dev->dma_mask)
+			return *host_dev->dma_mask;
+
+		/*
+		 * Platforms with virtual-DMA translation
+ 		 * hardware have no practical limit.
+		 */
+		return BLK_BOUNCE_ANY;
+	} else if (shost->unchecked_isa_dma)
+		return BLK_BOUNCE_ISA;
+
+	return BLK_BOUNCE_HIGH;
+}
+
+static request_queue_t *scsi_alloc_queue(struct Scsi_Host *shost)
+{
+	request_queue_t *q;
+
+	q = kmalloc(sizeof(*q), GFP_ATOMIC);
+	if (!q)
+		return NULL;
+	memset(q, 0, sizeof(*q));
+
+	if (!shost->max_sectors) {
+		/*
+		 * Driver imposes no hard sector transfer limit.
+		 * start at machine infinity initially.
+		 */
+		shost->max_sectors = SCSI_DEFAULT_MAX_SECTORS;
 	}
 
-	blk_queue_bounce_limit(q, bounce_limit);
+	blk_init_queue(q, scsi_request_fn, shost->host_lock);
+	blk_queue_prep_rq(q, scsi_prep_fn);
+
+	blk_queue_max_hw_segments(q, shost->sg_tablesize);
+	blk_queue_max_phys_segments(q, MAX_PHYS_SEGMENTS);
+	blk_queue_max_sectors(q, shost->max_sectors);
+	blk_queue_bounce_limit(q, scsi_calculate_bounce_limit(shost));
+
+	if (!shost->use_clustering)
+		clear_bit(QUEUE_FLAG_CLUSTER, &q->queue_flags);
+
+	return q;
+}
+
+static void scsi_free_queue(request_queue_t *q)
+{
+	blk_cleanup_queue(q);
+	kfree(q);
 }
 
 /**
@@ -435,19 +460,15 @@
 		 */
 		sdev->borken = 1;
 
-		if(!q || *q == NULL) {
-			sdev->request_queue = kmalloc(sizeof(struct request_queue), GFP_ATOMIC);
-			if(sdev->request_queue == NULL) {
+		if (!q || *q == NULL) {
+			sdev->request_queue = scsi_alloc_queue(shost);
+			if (!sdev->request_queue)
 				goto out_bail;
-			}
-			memset(sdev->request_queue, 0,
-				       	sizeof(struct request_queue));
-			scsi_initialize_queue(sdev, shost);
-			scsi_initialize_merge_fn(sdev);
 		} else {
 			sdev->request_queue = *q;
 			*q = NULL;
 		}
+
 		sdev->request_queue->queuedata = sdev;
 		scsi_adjust_queue_depth(sdev, 0, sdev->host->cmd_per_lun);
 		scsi_build_commandblocks(sdev);
@@ -488,13 +509,12 @@
 	}
 out_bail:
 	printk(ALLOC_FAILURE_MSG, __FUNCTION__);
-	if(q && sdev->request_queue) {
+	if (q && sdev->request_queue) {
 		*q = sdev->request_queue;
 		sdev->request_queue = NULL;
-	} else if(sdev->request_queue) {
-		blk_cleanup_queue(sdev->request_queue);
-		kfree(sdev->request_queue);
-	}
+	} else if (sdev->request_queue)
+		scsi_free_queue(sdev->request_queue);
+
 	scsi_release_commandblocks(sdev);
 	kfree(sdev);
 	return NULL;
@@ -513,14 +533,12 @@
 	list_del(&sdev->siblings);
 	list_del(&sdev->same_target_siblings);
 
-	if(sdev->request_queue != NULL) {
-		blk_cleanup_queue(sdev->request_queue);
-		kfree(sdev->request_queue);
-	}
+	if (sdev->request_queue)
+		scsi_free_queue(sdev->request_queue);
 	scsi_release_commandblocks(sdev);
 	if (sdev->host->hostt->slave_destroy)
 		sdev->host->hostt->slave_destroy(sdev);
-	if (sdev->inquiry != NULL)
+	if (sdev->inquiry)
 		kfree(sdev->inquiry);
 	kfree(sdev);
 }
@@ -1946,10 +1964,9 @@
 			scsi_scan_target(shost, &q, channel, order_id);
 		}
 	}
-	if(q) {
-		blk_cleanup_queue(q);
-		kfree(q);
-	}
+
+	if (q)
+		scsi_free_queue(q);
 }
 
 void scsi_forget_host(struct Scsi_Host *shost)
--- 1.23/drivers/scsi/scsi_syms.c	Thu Nov 28 09:09:53 2002
+++ edited/drivers/scsi/scsi_syms.c	Sat Dec 21 17:52:49 2002
@@ -98,6 +98,11 @@
 EXPORT_SYMBOL(scsi_device_types);
 
 /*
+ * This is for st to find the bounce limit
+ */
+EXPORT_SYMBOL(scsi_calculate_bounce_limit);
+
+/*
  * Externalize timers so that HBAs can safely start/restart commands.
  */
 extern void scsi_add_timer(Scsi_Cmnd *, int, void ((*) (Scsi_Cmnd *)));
--- 1.51/drivers/scsi/st.c	Mon Dec 16 10:55:37 2002
+++ edited/drivers/scsi/st.c	Sat Dec 21 16:53:05 2002
@@ -3764,21 +3764,9 @@
 	tpnt->nbr_partitions = 0;
 	tpnt->timeout = ST_TIMEOUT;
 	tpnt->long_timeout = ST_LONG_TIMEOUT;
-
 	tpnt->try_dio = try_direct_io && !SDp->host->unchecked_isa_dma;
-	bounce_limit = BLK_BOUNCE_HIGH; /* Borrowed from scsi_merge.c */
-	if (SDp->host->highmem_io) {
-		struct device *dev = scsi_get_device(SDp->host);
-		if (!PCI_DMA_BUS_IS_PHYS)
-			/* Platforms with virtual-DMA translation
-			 * hardware have no practical limit.
-			 */
-			bounce_limit = BLK_BOUNCE_ANY;
-		else if (dev && dev->dma_mask)
-			bounce_limit = *dev->dma_mask;
-	} else if (SDp->host->unchecked_isa_dma)
-		bounce_limit = BLK_BOUNCE_ISA;
-	bounce_limit >>= PAGE_SHIFT;
+
+	bounce_limit = scsi_calculate_bounce_limit(SDp->host) >> PAGE_SHIFT;
 	if (bounce_limit > ULONG_MAX)
 		bounce_limit = ULONG_MAX;
 	tpnt->max_pfn = bounce_limit;




-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html