compile warning fix for advansys (trivial)

compile warning fix for advansys (trivial)

[Guennadi Liakhovetski]

Hi

This was earlier sent with a pretty masking subject to LKML, so, in case,
it wasn't noticed by those, who collect stuff for 2.6.1+, here it is
again.

On Fri, 19 Dec 2003, Gene Heskett wrote:

> Hey, how about us advansis users?  There is at least one, me...  And
> thats the only driver in the whole compile that spits out visible
> warnings about check_region() being deprecated.  But its still
> working, so far.  I presume it will until such time as check_region()
> is actually removed from wherever it lives.

Well, wouldn't it suffice just to remove the deprecated checks? The only
(for recent kernels) request_region() does check the return code. So, this
should do:

--- drivers/scsi/advansys.c~	Mon Nov 24 20:43:42 2003
+++ drivers/scsi/advansys.c	Sat Dec 20 15:34:00 2003
@@ -4619,13 +4619,7 @@
                         ASC_DBG1(1,
                                 "advansys_detect: probing I/O port 0x%x...\n",
                             iop);
-                        if (check_region(iop, ASC_IOADR_GAP) != 0) {
-                            printk(
-"AdvanSys SCSI: specified I/O Port 0x%X is busy\n", iop);
-                            /* Don't try this I/O port twice. */
-                            asc_ioport[ioport] = 0;
-                            goto ioport_try_again;
-                        } else if (AscFindSignature(iop) == ASC_FALSE) {
+			if (AscFindSignature(iop) == ASC_FALSE) {
                             printk(
 "AdvanSys SCSI: specified I/O Port 0x%X has no adapter\n", iop);
                             /* Don't try this I/O port twice. */
@@ -10003,12 +9997,6 @@
     }
     for (; i < ASC_IOADR_TABLE_MAX_IX; i++) {
         iop_base = _asc_def_iop_base[i];
-        if (check_region(iop_base, ASC_IOADR_GAP) != 0) {
-            ASC_DBG1(1,
-               "AscSearchIOPortAddr11: check_region() failed I/O port 0x%x\n",
-                     iop_base);
-            continue;
-        }
         ASC_DBG1(1, "AscSearchIOPortAddr11: probing I/O port 0x%x\n", iop_base);
         if (AscFindSignature(iop_base)) {
             return (iop_base);

Regards
Guennadi
---
Guennadi Liakhovetski

Re: compile warning fix for advansys (trivial)

[Christoph Hellwig]

On Sun, Dec 21, 2003 at 10:40:49PM +0100, Guennadi Liakhovetski wrote:
> Hi
> 
> This was earlier sent with a pretty masking subject to LKML, so, in case,
> it wasn't noticed by those, who collect stuff for 2.6.1+, here it is
> again.

It's not that easy.  You need to move the request_region as early as
the check_region is currently and make sure you properly release the
I/O region again on an initialization failure later on.

Re: compile warning fix for advansys (trivial)

[Guennadi Liakhovetski]

On Sun, 28 Dec 2003, Christoph Hellwig wrote:

> It's not that easy.  You need to move the request_region as early as
> the check_region is currently and make sure you properly release the
> I/O region again on an initialization failure later on.

Hm, my assumption was, that if the old version, doing

if (check_region())
	fail();

if (!request_region())
	fail();

was correct, it would mean, that if check_region succeeds, but due to the
race, the following request_region() fails, the error-processing and
bailing-out should work correct. So, we can just safely assume, that the
check_region() does succeed, and let the actual request_region decide. So,
I think, the patch would only be wrong if either the original driver
behaviour was wrong in the race-case, or there are other paths, except
the above, where only the result of check_region() is used, without
actually doing request_region(), which would be a bit strange. Or is this
the case?

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: compile warning fix for advansys (trivial)

[Christoph Hellwig]

On Mon, Dec 29, 2003 at 12:26:39AM +0100, Guennadi Liakhovetski wrote:
> > It's not that easy.  You need to move the request_region as early as
> > the check_region is currently and make sure you properly release the
> > I/O region again on an initialization failure later on.
> 
> Hm, my assumption was, that if the old version, doing
> 
> if (check_region())
> 	fail();
> 
> if (!request_region())
> 	fail();
> 
> was correct, it would mean, that if check_region succeeds, but due to the
> race, the following request_region() fails, the error-processing and
> bailing-out should work correct. So, we can just safely assume, that the
> check_region() does succeed, and let the actual request_region decide. So,
> I think, the patch would only be wrong if either the original driver
> behaviour was wrong in the race-case,

it was.  That's why we're changing it after all :)

Re: compile warning fix for advansys (trivial)

[Guennadi Liakhovetski]

On Sun, 28 Dec 2003, Christoph Hellwig wrote:

> On Mon, Dec 29, 2003 at 12:26:39AM +0100, Guennadi Liakhovetski wrote:
> > I think, the patch would only be wrong if either the original driver
> > behaviour was wrong in the race-case,
>
> it was.  That's why we're changing it after all :)

Ok, then, I think, the driver is doing the following: for ISA / VL busses
it does check_region for ASC_IOADR_GAP (16) bytes, does chip-detection,
and if a card is found - then issues request_region() with the correct
length.

So, I see 2 possibilities:
1) replace check_region with request_region for the same size, and then
release the region before requesting the correct size. (Well,
theoretically, one could request the rest of the larger area in a second
request_region, but that's pretty ugly)
2) replace check_region with request_region - but immediately of correct
length. Finding this correct length involves identifying the chip... But,
reading the code, it looks like only 38C-1600 can be used in ISA and VL
cards, right? Sorry, I didn't find any data sheets.

Which of the 2 options (or a third one) would be preferrable, and is my
assumption about the 1600 chip correct? The first one is easier to
implement. Ideally, one should properly identify the chip and then request
the required number of bytes, of course.

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: compile warning fix for advansys (trivial)

[Guennadi Liakhovetski]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 735 bytes --]

On Sun, 28 Dec 2003, Christoph Hellwig wrote:

> On Mon, Dec 29, 2003 at 12:26:39AM +0100, Guennadi Liakhovetski wrote:
> > I think, the patch would only be wrong if either the original driver
> > behaviour was wrong in the race-case,
>
> it was.  That's why we're changing it after all :)

Ok, the attached patch (carefully, I hope) does request_region instead of
check_region for the detection address-range, and then release_region -
when needed. Hope I got it right. Compiles, otherwise completely untested.
Yes, I saw the other patch from Omkhar Arasaratnam, sent to LKML, - I
think, it is incomplete and not quite accurate too. However, the idea is
the same (if I understood his idea right).

Guennadi
---
Guennadi Liakhovetski


[-- Attachment #2: Type: TEXT/PLAIN, Size: 3563 bytes --]

--- linux-2.6.0-test11/drivers/scsi/advansys.c~	Wed Dec 31 17:36:05 2003
+++ linux-2.6.0-test11/drivers/scsi/advansys.c	Wed Dec 31 18:47:12 2003
@@ -2071,6 +2071,7 @@
 STATIC ASC_DCNT  AscGetEisaProductID(PortAddr);
 STATIC PortAddr  AscSearchIOPortAddrEISA(PortAddr);
 STATIC PortAddr  AscSearchIOPortAddr11(PortAddr);
+STATIC void      AscReleaseIOPortAddr11(PortAddr);
 STATIC PortAddr  AscSearchIOPortAddr(PortAddr, ushort);
 STATIC void      AscSetISAPNPWaitForKey(void);
 #endif /* CONFIG_ISA */
@@ -4619,7 +4620,7 @@
                         ASC_DBG1(1,
                                 "advansys_detect: probing I/O port 0x%x...\n",
                             iop);
-                        if (check_region(iop, ASC_IOADR_GAP) != 0) {
+                        if (request_region(iop, ASC_IOADR_GAP, "advansys-detect") == NULL) {
                             printk(
 "AdvanSys SCSI: specified I/O Port 0x%X is busy\n", iop);
                             /* Don't try this I/O port twice. */
@@ -4630,6 +4631,7 @@
 "AdvanSys SCSI: specified I/O Port 0x%X has no adapter\n", iop);
                             /* Don't try this I/O port twice. */
                             asc_ioport[ioport] = 0;
+                            release_region(iop, ASC_IOADR_GAP);
                             goto ioport_try_again;
                         } else {
                             /*
@@ -4647,6 +4649,7 @@
                                   * 'ioport' past this board.
                                   */
                                  ioport++;
+                                 release_region(iop, ASC_IOADR_GAP);
                                  goto ioport_try_again;
                             }
                         }
@@ -5370,6 +5373,12 @@
                 }
             }
 
+#ifdef CONFIG_ISA
+            /* on ISA and VL have to release the detection-region */
+            if (asc_bus[bus] & (ASC_IS_ISA | ASC_IS_VL))
+                    release_region(iop, ASC_IOADR_GAP);
+#endif
+
             /*
              * Register Board Resources - I/O Port, DMA, IRQ
              */
@@ -9962,6 +9971,7 @@
             if (AscGetChipVersion(iop_beg, bus_type) <= ASC_CHIP_MAX_VER_VL) {
                 return (iop_beg);
             }
+            AscReleaseIOPortAddr11(iop_beg);
         }
         return (0);
     }
@@ -9974,6 +9984,7 @@
             if ((AscGetChipVersion(iop_beg, bus_type) & ASC_CHIP_VER_ISA_BIT) != 0) {
                 return (iop_beg);
             }
+            AscReleaseIOPortAddr11(iop_beg);
         }
         return (0);
     }
@@ -9987,6 +9998,16 @@
 }
 
 ASC_INITFUNC(
+STATIC void,
+AscReleaseIOPortAddr11(
+                         PortAddr iop_base
+)
+)
+{
+    release_region(iop_base, ASC_IOADR_GAP);
+}
+
+ASC_INITFUNC(
 STATIC PortAddr,
 AscSearchIOPortAddr11(
                          PortAddr s_addr
@@ -10003,7 +10024,7 @@
     }
     for (; i < ASC_IOADR_TABLE_MAX_IX; i++) {
         iop_base = _asc_def_iop_base[i];
-        if (check_region(iop_base, ASC_IOADR_GAP) != 0) {
+        if (request_region(iop_base, ASC_IOADR_GAP, "advansys-detect") == NULL) {
             ASC_DBG1(1,
                "AscSearchIOPortAddr11: check_region() failed I/O port 0x%x\n",
                      iop_base);
@@ -10013,6 +10034,7 @@
         if (AscFindSignature(iop_base)) {
             return (iop_base);
         }
+        release_region(iop_base, ASC_IOADR_GAP);
     }
     return (0);
 }