[PATCH v2] advansys: fix regression with request_firmware change

[PATCH v2] advansys: fix regression with request_firmware change

[Herton Ronaldo Krzesinski]

On newer kernels users of advansys module are reporting system hang when
trying to load it without firmware files present. After looking closely
at description on https://qa.mandriva.com/show_bug.cgi?id=53220, I think
this is related to commit "[SCSI] advansys: use request_firmware". The
problem is that after switch to request_firmware, asc_dvc->err_code
isn't being set when firmware files aren't found or loading fails.

err_code is used by the driver to judge if there was a fatal error or
not, as can be seen for example on advansys_board_found, which will only
return -ENODEV when err_code is set. Because err_code isn't being set
when request_firmware fails, this is a change of behaviour of the code
before request_firmware addition, making it continue to load and it
fails later as the firmware wasn't really loaded.

Also, error handling on advansys_board_found is fixed, because it's
buggy in the case we have an ASC_NARROW_BOARD set and failure happens on
AscInitAsc1000Driver step: it was freeing items of wrong struct in the
dvc_var union of struct asc_board, which could lead to an oops in the
case we set some of the fields in struct of narrow board as code was
choosing to always freeing wide board fields, and not everything wasn't
being freed/released properly.

Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
---
 drivers/scsi/advansys.c |   35 ++++++++++++++++++++++++++---------
 1 files changed, 26 insertions(+), 9 deletions(-)

v2 fixes error handling in advansys_board_found, so code will not oops anymore
if firmware files are not found. users tested this change and reported it to be
ok (no more freeze or oops when firmware files are not found)

diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
index 22626ab..9a3c68f 100644
--- a/drivers/scsi/advansys.c
+++ b/drivers/scsi/advansys.c
@@ -4781,12 +4781,14 @@ static ushort AscInitAsc1000Driver(ASC_DVC_VAR *asc_dvc)
 	if (err) {
 		printk(KERN_ERR "Failed to load image \"%s\" err %d\n",
 		       fwname, err);
+		asc_dvc->err_code |= ASC_IERR_MCODE_CHKSUM;
 		return err;
 	}
 	if (fw->size < 4) {
 		printk(KERN_ERR "Bogus length %zu in image \"%s\"\n",
 		       fw->size, fwname);
 		release_firmware(fw);
+		asc_dvc->err_code |= ASC_IERR_MCODE_CHKSUM;
 		return -EINVAL;
 	}
 	chksum = (fw->data[3] << 24) | (fw->data[2] << 16) |
@@ -5110,12 +5112,14 @@ static int AdvInitAsc3550Driver(ADV_DVC_VAR *asc_dvc)
 	if (err) {
 		printk(KERN_ERR "Failed to load image \"%s\" err %d\n",
 		       fwname, err);
+		asc_dvc->err_code = ASC_IERR_MCODE_CHKSUM;
 		return err;
 	}
 	if (fw->size < 4) {
 		printk(KERN_ERR "Bogus length %zu in image \"%s\"\n",
 		       fw->size, fwname);
 		release_firmware(fw);
+		asc_dvc->err_code = ASC_IERR_MCODE_CHKSUM;
 		return -EINVAL;
 	}
 	chksum = (fw->data[3] << 24) | (fw->data[2] << 16) |
@@ -5624,12 +5628,14 @@ static int AdvInitAsc38C0800Driver(ADV_DVC_VAR *asc_dvc)
 	if (err) {
 		printk(KERN_ERR "Failed to load image \"%s\" err %d\n",
 		       fwname, err);
+		asc_dvc->err_code = ASC_IERR_MCODE_CHKSUM;
 		return err;
 	}
 	if (fw->size < 4) {
 		printk(KERN_ERR "Bogus length %zu in image \"%s\"\n",
 		       fw->size, fwname);
 		release_firmware(fw);
+		asc_dvc->err_code = ASC_IERR_MCODE_CHKSUM;
 		return -EINVAL;
 	}
 	chksum = (fw->data[3] << 24) | (fw->data[2] << 16) |
@@ -6124,12 +6130,14 @@ static int AdvInitAsc38C1600Driver(ADV_DVC_VAR *asc_dvc)
 	if (err) {
 		printk(KERN_ERR "Failed to load image \"%s\" err %d\n",
 		       fwname, err);
+		asc_dvc->err_code = ASC_IERR_MCODE_CHKSUM;
 		return err;
 	}
 	if (fw->size < 4) {
 		printk(KERN_ERR "Bogus length %zu in image \"%s\"\n",
 		       fw->size, fwname);
 		release_firmware(fw);
+		asc_dvc->err_code = ASC_IERR_MCODE_CHKSUM;
 		return -EINVAL;
 	}
 	chksum = (fw->data[3] << 24) | (fw->data[2] << 16) |
@@ -12303,7 +12311,7 @@ static int __devinit advansys_board_found(struct Scsi_Host *shost,
 		asc_dvc_varp->overrun_buf = kzalloc(ASC_OVERRUN_BSIZE, GFP_KERNEL);
 		if (!asc_dvc_varp->overrun_buf) {
 			ret = -ENOMEM;
-			goto err_free_wide_mem;
+			goto err_free_irq;
 		}
 		warn_code = AscInitAsc1000Driver(asc_dvc_varp);
 
@@ -12314,28 +12322,37 @@ static int __devinit advansys_board_found(struct Scsi_Host *shost,
 					asc_dvc_varp->err_code);
 			if (asc_dvc_varp->err_code) {
 				ret = -ENODEV;
-				kfree(asc_dvc_varp->overrun_buf);
+				goto err_free_mem;
 			}
 		}
 	} else {
-		if (advansys_wide_init_chip(shost))
+		if (advansys_wide_init_chip(shost)) {
 			ret = -ENODEV;
+			goto err_free_mem;
+		}
 	}
 
-	if (ret)
-		goto err_free_wide_mem;
-
 	ASC_DBG_PRT_SCSI_HOST(2, shost);
 
 	ret = scsi_add_host(shost, boardp->dev);
 	if (ret)
-		goto err_free_wide_mem;
+		goto err_free_mem;
 
 	scsi_scan_host(shost);
 	return 0;
 
- err_free_wide_mem:
-	advansys_wide_free_mem(boardp);
+ err_free_mem:
+	if (ASC_NARROW_BOARD(boardp)) {
+		if ((asc_dvc_varp->err_code & ASC_IERR_SET_PC_ADDR) ||
+		    (asc_dvc_varp->err_code & ASC_IERR_START_STOP_CHIP) ||
+		    (!asc_dvc_varp->err_code))
+			dma_unmap_single(boardp->dev, asc_dvc_varp->overrun_dma,
+					 ASC_OVERRUN_BSIZE, DMA_FROM_DEVICE);
+		kfree(asc_dvc_varp->overrun_buf);
+	} else {
+		advansys_wide_free_mem(boardp);
+	}
+ err_free_irq:
 	free_irq(boardp->irq, shost);
  err_free_dma:
 #ifdef CONFIG_ISA
-- 
1.7.0.3

Re: [PATCH v2] advansys: fix regression with request_firmware change

[James Bottomley]

On Fri, 2010-03-26 at 20:05 -0300, Herton Ronaldo Krzesinski wrote:
> On newer kernels users of advansys module are reporting system hang when
> trying to load it without firmware files present. After looking closely
> at description on https://qa.mandriva.com/show_bug.cgi?id=53220, I think
> this is related to commit "[SCSI] advansys: use request_firmware". The
> problem is that after switch to request_firmware, asc_dvc->err_code
> isn't being set when firmware files aren't found or loading fails.
> 
> err_code is used by the driver to judge if there was a fatal error or
> not, as can be seen for example on advansys_board_found, which will only
> return -ENODEV when err_code is set. Because err_code isn't being set
> when request_firmware fails, this is a change of behaviour of the code
> before request_firmware addition, making it continue to load and it
> fails later as the firmware wasn't really loaded.
> 
> Also, error handling on advansys_board_found is fixed, because it's
> buggy in the case we have an ASC_NARROW_BOARD set and failure happens on
> AscInitAsc1000Driver step: it was freeing items of wrong struct in the
> dvc_var union of struct asc_board, which could lead to an oops in the
> case we set some of the fields in struct of narrow board as code was
> choosing to always freeing wide board fields, and not everything wasn't
> being freed/released properly.

This piece is nothing to do with a request_firmware regression, so it
doesn't really belong here.

> Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
> ---
>  drivers/scsi/advansys.c |   35 ++++++++++++++++++++++++++---------
>  1 files changed, 26 insertions(+), 9 deletions(-)
> 
> v2 fixes error handling in advansys_board_found, so code will not oops anymore
> if firmware files are not found. users tested this change and reported it to be
> ok (no more freeze or oops when firmware files are not found)
> 
> diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
> index 22626ab..9a3c68f 100644
> --- a/drivers/scsi/advansys.c
> +++ b/drivers/scsi/advansys.c
> @@ -4781,12 +4781,14 @@ static ushort AscInitAsc1000Driver(ASC_DVC_VAR *asc_dvc)
>  	if (err) {
>  		printk(KERN_ERR "Failed to load image \"%s\" err %d\n",
>  		       fwname, err);
> +		asc_dvc->err_code |= ASC_IERR_MCODE_CHKSUM;
>  		return err;
>  	}
>  	if (fw->size < 4) {
>  		printk(KERN_ERR "Bogus length %zu in image \"%s\"\n",
>  		       fw->size, fwname);
>  		release_firmware(fw);
> +		asc_dvc->err_code |= ASC_IERR_MCODE_CHKSUM;
>  		return -EINVAL;
>  	}
>  	chksum = (fw->data[3] << 24) | (fw->data[2] << 16) |
> @@ -5110,12 +5112,14 @@ static int AdvInitAsc3550Driver(ADV_DVC_VAR *asc_dvc)
>  	if (err) {
>  		printk(KERN_ERR "Failed to load image \"%s\" err %d\n",
>  		       fwname, err);
> +		asc_dvc->err_code = ASC_IERR_MCODE_CHKSUM;
>  		return err;
>  	}
>  	if (fw->size < 4) {
>  		printk(KERN_ERR "Bogus length %zu in image \"%s\"\n",
>  		       fw->size, fwname);
>  		release_firmware(fw);
> +		asc_dvc->err_code = ASC_IERR_MCODE_CHKSUM;
>  		return -EINVAL;
>  	}
>  	chksum = (fw->data[3] << 24) | (fw->data[2] << 16) |
> @@ -5624,12 +5628,14 @@ static int AdvInitAsc38C0800Driver(ADV_DVC_VAR *asc_dvc)
>  	if (err) {
>  		printk(KERN_ERR "Failed to load image \"%s\" err %d\n",
>  		       fwname, err);
> +		asc_dvc->err_code = ASC_IERR_MCODE_CHKSUM;
>  		return err;
>  	}
>  	if (fw->size < 4) {
>  		printk(KERN_ERR "Bogus length %zu in image \"%s\"\n",
>  		       fw->size, fwname);
>  		release_firmware(fw);
> +		asc_dvc->err_code = ASC_IERR_MCODE_CHKSUM;
>  		return -EINVAL;
>  	}
>  	chksum = (fw->data[3] << 24) | (fw->data[2] << 16) |
> @@ -6124,12 +6130,14 @@ static int AdvInitAsc38C1600Driver(ADV_DVC_VAR *asc_dvc)
>  	if (err) {
>  		printk(KERN_ERR "Failed to load image \"%s\" err %d\n",
>  		       fwname, err);
> +		asc_dvc->err_code = ASC_IERR_MCODE_CHKSUM;
>  		return err;
>  	}
>  	if (fw->size < 4) {
>  		printk(KERN_ERR "Bogus length %zu in image \"%s\"\n",
>  		       fw->size, fwname);
>  		release_firmware(fw);
> +		asc_dvc->err_code = ASC_IERR_MCODE_CHKSUM;
>  		return -EINVAL;
>  	}
>  	chksum = (fw->data[3] << 24) | (fw->data[2] << 16) |
> @@ -12303,7 +12311,7 @@ static int __devinit advansys_board_found(struct Scsi_Host *shost,
>  		asc_dvc_varp->overrun_buf = kzalloc(ASC_OVERRUN_BSIZE, GFP_KERNEL);
>  		if (!asc_dvc_varp->overrun_buf) {
>  			ret = -ENOMEM;
> -			goto err_free_wide_mem;
> +			goto err_free_irq;
>  		}
>  		warn_code = AscInitAsc1000Driver(asc_dvc_varp);
>  
> @@ -12314,28 +12322,37 @@ static int __devinit advansys_board_found(struct Scsi_Host *shost,
>  					asc_dvc_varp->err_code);
>  			if (asc_dvc_varp->err_code) {
>  				ret = -ENODEV;
> -				kfree(asc_dvc_varp->overrun_buf);
> +				goto err_free_mem;
>  			}
>  		}
>  	} else {
> -		if (advansys_wide_init_chip(shost))
> +		if (advansys_wide_init_chip(shost)) {
>  			ret = -ENODEV;
> +			goto err_free_mem;
> +		}
>  	}
>  
> -	if (ret)
> -		goto err_free_wide_mem;
> -
>  	ASC_DBG_PRT_SCSI_HOST(2, shost);
>  
>  	ret = scsi_add_host(shost, boardp->dev);
>  	if (ret)
> -		goto err_free_wide_mem;
> +		goto err_free_mem;
>  
>  	scsi_scan_host(shost);
>  	return 0;
>  
> - err_free_wide_mem:
> -	advansys_wide_free_mem(boardp);
> + err_free_mem:
> +	if (ASC_NARROW_BOARD(boardp)) {
> +		if ((asc_dvc_varp->err_code & ASC_IERR_SET_PC_ADDR) ||
> +		    (asc_dvc_varp->err_code & ASC_IERR_START_STOP_CHIP) ||
> +		    (!asc_dvc_varp->err_code))
> +			dma_unmap_single(boardp->dev, asc_dvc_varp->overrun_dma,
> +					 ASC_OVERRUN_BSIZE, DMA_FROM_DEVICE);

This is both nasty and not really a fix:  the overrun buf is still
mapped many times on the reset path and only ever unmapped once, which
is still an unfixed bug in this code.  I think the map needs to be moved
out of AscInitMicroCodeVar() to somewhere in here.

James

> +		kfree(asc_dvc_varp->overrun_buf);
> +	} else {
> +		advansys_wide_free_mem(boardp);
> +	}
> + err_free_irq:
>  	free_irq(boardp->irq, shost);
>   err_free_dma:
>  #ifdef CONFIG_ISA

Re: [PATCH v2] advansys: fix regression with request_firmware change

[Herton Ronaldo Krzesinski]

Em Dom 28 Mar 2010, às 12:25:20, James Bottomley escreveu:
> On Fri, 2010-03-26 at 20:05 -0300, Herton Ronaldo Krzesinski wrote:
> > On newer kernels users of advansys module are reporting system hang when
> > trying to load it without firmware files present. After looking closely
> > at description on https://qa.mandriva.com/show_bug.cgi?id=53220, I think
> > this is related to commit "[SCSI] advansys: use request_firmware". The
> > problem is that after switch to request_firmware, asc_dvc->err_code
> > isn't being set when firmware files aren't found or loading fails.
> > 
> > err_code is used by the driver to judge if there was a fatal error or
> > not, as can be seen for example on advansys_board_found, which will only
> > return -ENODEV when err_code is set. Because err_code isn't being set
> > when request_firmware fails, this is a change of behaviour of the code
> > before request_firmware addition, making it continue to load and it
> > fails later as the firmware wasn't really loaded.
> > 
> > Also, error handling on advansys_board_found is fixed, because it's
> > buggy in the case we have an ASC_NARROW_BOARD set and failure happens on
> > AscInitAsc1000Driver step: it was freeing items of wrong struct in the
> > dvc_var union of struct asc_board, which could lead to an oops in the
> > case we set some of the fields in struct of narrow board as code was
> > choosing to always freeing wide board fields, and not everything wasn't
> > being freed/released properly.
> 
> This piece is nothing to do with a request_firmware regression, so it
> doesn't really belong here.
> 
> > Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
> > ---
> >  drivers/scsi/advansys.c |   35 ++++++++++++++++++++++++++---------
> >  1 files changed, 26 insertions(+), 9 deletions(-)
> > 
> > v2 fixes error handling in advansys_board_found, so code will not oops anymore
> > if firmware files are not found. users tested this change and reported it to be
> > ok (no more freeze or oops when firmware files are not found)
> > 
> > diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
> > index 22626ab..9a3c68f 100644
> > --- a/drivers/scsi/advansys.c
> > +++ b/drivers/scsi/advansys.c
> > @@ -4781,12 +4781,14 @@ static ushort AscInitAsc1000Driver(ASC_DVC_VAR *asc_dvc)
> >  	if (err) {
> >  		printk(KERN_ERR "Failed to load image \"%s\" err %d\n",
> >  		       fwname, err);
> > +		asc_dvc->err_code |= ASC_IERR_MCODE_CHKSUM;
> >  		return err;
> >  	}
> >  	if (fw->size < 4) {
> >  		printk(KERN_ERR "Bogus length %zu in image \"%s\"\n",
> >  		       fw->size, fwname);
> >  		release_firmware(fw);
> > +		asc_dvc->err_code |= ASC_IERR_MCODE_CHKSUM;
> >  		return -EINVAL;
> >  	}
> >  	chksum = (fw->data[3] << 24) | (fw->data[2] << 16) |
> > @@ -5110,12 +5112,14 @@ static int AdvInitAsc3550Driver(ADV_DVC_VAR *asc_dvc)
> >  	if (err) {
> >  		printk(KERN_ERR "Failed to load image \"%s\" err %d\n",
> >  		       fwname, err);
> > +		asc_dvc->err_code = ASC_IERR_MCODE_CHKSUM;
> >  		return err;
> >  	}
> >  	if (fw->size < 4) {
> >  		printk(KERN_ERR "Bogus length %zu in image \"%s\"\n",
> >  		       fw->size, fwname);
> >  		release_firmware(fw);
> > +		asc_dvc->err_code = ASC_IERR_MCODE_CHKSUM;
> >  		return -EINVAL;
> >  	}
> >  	chksum = (fw->data[3] << 24) | (fw->data[2] << 16) |
> > @@ -5624,12 +5628,14 @@ static int AdvInitAsc38C0800Driver(ADV_DVC_VAR *asc_dvc)
> >  	if (err) {
> >  		printk(KERN_ERR "Failed to load image \"%s\" err %d\n",
> >  		       fwname, err);
> > +		asc_dvc->err_code = ASC_IERR_MCODE_CHKSUM;
> >  		return err;
> >  	}
> >  	if (fw->size < 4) {
> >  		printk(KERN_ERR "Bogus length %zu in image \"%s\"\n",
> >  		       fw->size, fwname);
> >  		release_firmware(fw);
> > +		asc_dvc->err_code = ASC_IERR_MCODE_CHKSUM;
> >  		return -EINVAL;
> >  	}
> >  	chksum = (fw->data[3] << 24) | (fw->data[2] << 16) |
> > @@ -6124,12 +6130,14 @@ static int AdvInitAsc38C1600Driver(ADV_DVC_VAR *asc_dvc)
> >  	if (err) {
> >  		printk(KERN_ERR "Failed to load image \"%s\" err %d\n",
> >  		       fwname, err);
> > +		asc_dvc->err_code = ASC_IERR_MCODE_CHKSUM;
> >  		return err;
> >  	}
> >  	if (fw->size < 4) {
> >  		printk(KERN_ERR "Bogus length %zu in image \"%s\"\n",
> >  		       fw->size, fwname);
> >  		release_firmware(fw);
> > +		asc_dvc->err_code = ASC_IERR_MCODE_CHKSUM;
> >  		return -EINVAL;
> >  	}
> >  	chksum = (fw->data[3] << 24) | (fw->data[2] << 16) |
> > @@ -12303,7 +12311,7 @@ static int __devinit advansys_board_found(struct Scsi_Host *shost,
> >  		asc_dvc_varp->overrun_buf = kzalloc(ASC_OVERRUN_BSIZE, GFP_KERNEL);
> >  		if (!asc_dvc_varp->overrun_buf) {
> >  			ret = -ENOMEM;
> > -			goto err_free_wide_mem;
> > +			goto err_free_irq;
> >  		}
> >  		warn_code = AscInitAsc1000Driver(asc_dvc_varp);
> >  
> > @@ -12314,28 +12322,37 @@ static int __devinit advansys_board_found(struct Scsi_Host *shost,
> >  					asc_dvc_varp->err_code);
> >  			if (asc_dvc_varp->err_code) {
> >  				ret = -ENODEV;
> > -				kfree(asc_dvc_varp->overrun_buf);
> > +				goto err_free_mem;
> >  			}
> >  		}
> >  	} else {
> > -		if (advansys_wide_init_chip(shost))
> > +		if (advansys_wide_init_chip(shost)) {
> >  			ret = -ENODEV;
> > +			goto err_free_mem;
> > +		}
> >  	}
> >  
> > -	if (ret)
> > -		goto err_free_wide_mem;
> > -
> >  	ASC_DBG_PRT_SCSI_HOST(2, shost);
> >  
> >  	ret = scsi_add_host(shost, boardp->dev);
> >  	if (ret)
> > -		goto err_free_wide_mem;
> > +		goto err_free_mem;
> >  
> >  	scsi_scan_host(shost);
> >  	return 0;
> >  
> > - err_free_wide_mem:
> > -	advansys_wide_free_mem(boardp);
> > + err_free_mem:
> > +	if (ASC_NARROW_BOARD(boardp)) {
> > +		if ((asc_dvc_varp->err_code & ASC_IERR_SET_PC_ADDR) ||
> > +		    (asc_dvc_varp->err_code & ASC_IERR_START_STOP_CHIP) ||
> > +		    (!asc_dvc_varp->err_code))
> > +			dma_unmap_single(boardp->dev, asc_dvc_varp->overrun_dma,
> > +					 ASC_OVERRUN_BSIZE, DMA_FROM_DEVICE);
> 
> This is both nasty and not really a fix:  the overrun buf is still
> mapped many times on the reset path and only ever unmapped once, which
> is still an unfixed bug in this code.  I think the map needs to be moved
> out of AscInitMicroCodeVar() to somewhere in here.

I didn't want to do this since I think there will be much changes and I can't
test properly as I don't have the hardware (also may be too much for what
looks like a legacy driver/hardware). But I aggree this if is ugly and not
unmapping when needed on reset. What I thought can be better is this, to fix
error handling also on AscInitMicroCodeVar then:

diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
index 9201afe..01481d1 100644
--- a/drivers/scsi/advansys.c
+++ b/drivers/scsi/advansys.c
@@ -460,6 +460,7 @@ typedef struct asc_risc_sg_list_q {
 #define ASC_IERR_BIST_PRE_TEST		0x0800	/* BIST pre-test error */
 #define ASC_IERR_BIST_RAM_TEST		0x1000	/* BIST RAM test error */
 #define ASC_IERR_BAD_CHIPTYPE		0x2000	/* Invalid chip_type setting */
+#define ASC_IERR_DMA_MAP_SINGLE 	0x4000  /* Error with dma_map_single */
 
 #define ASC_DEF_MAX_TOTAL_QNG   (0xF0)
 #define ASC_MIN_TAG_Q_PER_DVC   (0x04)
@@ -4701,14 +4702,12 @@ static void AscInitQLinkVar(ASC_DVC_VAR *asc_dvc)
 static ushort AscInitMicroCodeVar(ASC_DVC_VAR *asc_dvc)
 {
 	int i;
-	ushort warn_code;
 	PortAddr iop_base;
 	ASC_PADDR phy_addr;
 	ASC_DCNT phy_size;
 	struct asc_board *board = asc_dvc_to_board(asc_dvc);
 
 	iop_base = asc_dvc->iop_base;
-	warn_code = 0;
 	for (i = 0; i <= ASC_MAX_TID; i++) {
 		AscPutMCodeInitSDTRAtID(iop_base, i,
 					asc_dvc->cfg->sdtr_period_offset[i]);
@@ -4724,6 +4723,10 @@ static ushort AscInitMicroCodeVar(ASC_DVC_VAR *asc_dvc)
 	BUG_ON((unsigned long)asc_dvc->overrun_buf & 7);
 	asc_dvc->overrun_dma = dma_map_single(board->dev, asc_dvc->overrun_buf,
 					ASC_OVERRUN_BSIZE, DMA_FROM_DEVICE);
+	if (dma_mapping_error(boad->dev, asc_dvc->overrun_dma)) {
+		asc_dvc->err_code |= ASC_IERR_DMA_MAP_SINGLE;
+		return -ENOMEM;
+	}
 	phy_addr = cpu_to_le32(asc_dvc->overrun_dma);
 	AscMemDWordCopyPtrToLram(iop_base, ASCV_OVERRUN_PADDR_D,
 				 (uchar *)&phy_addr, 1);
@@ -4739,14 +4742,19 @@ static ushort AscInitMicroCodeVar(ASC_DVC_VAR *asc_dvc)
 	AscSetPCAddr(iop_base, ASC_MCODE_START_ADDR);
 	if (AscGetPCAddr(iop_base) != ASC_MCODE_START_ADDR) {
 		asc_dvc->err_code |= ASC_IERR_SET_PC_ADDR;
-		return warn_code;
+		goto err_mcode_start;
 	}
 	if (AscStartChip(iop_base) != 1) {
 		asc_dvc->err_code |= ASC_IERR_START_STOP_CHIP;
-		return warn_code;
+		goto err_mcode_start;
 	}
 
-	return warn_code;
+	return 0;
+
+err_mcode_start:
+	dma_unmap_single(board->dev, asc_dvc->overrun_buf,
+			 ASC_OVERRUN_BSIZE, DMA_FROM_DEVICE);
+	return UW_ERR;
 }
 
 static ushort AscInitAsc1000Driver(ASC_DVC_VAR *asc_dvc)
@@ -4802,6 +4810,8 @@ static ushort AscInitAsc1000Driver(ASC_DVC_VAR *asc_dvc)
 	}
 	release_firmware(fw);
 	warn_code |= AscInitMicroCodeVar(asc_dvc);
+	if (asc_dvc->err_code != 0)
+		return warn_code;
 	asc_dvc->init_state |= ASC_INIT_STATE_END_LOAD_MC;
 	AscEnableInterrupt(iop_base);
 	return warn_code;
@@ -12311,7 +12321,7 @@ static int __devinit advansys_board_found(struct Scsi_Host *shost,
 		asc_dvc_varp->overrun_buf = kzalloc(ASC_OVERRUN_BSIZE, GFP_KERNEL);
 		if (!asc_dvc_varp->overrun_buf) {
 			ret = -ENOMEM;
-			goto err_free_wide_mem;
+			goto err_free_irq;
 		}
 		warn_code = AscInitAsc1000Driver(asc_dvc_varp);
 
@@ -12322,28 +12332,31 @@ static int __devinit advansys_board_found(struct Scsi_Host *shost,
 					asc_dvc_varp->err_code);
 			if (asc_dvc_varp->err_code) {
 				ret = -ENODEV;
-				kfree(asc_dvc_varp->overrun_buf);
+				goto err_free_mem;
 			}
 		}
 	} else {
-		if (advansys_wide_init_chip(shost))
+		if (advansys_wide_init_chip(shost)) {
 			ret = -ENODEV;
+			goto err_free_mem;
+		}
 	}
 
-	if (ret)
-		goto err_free_wide_mem;
-
 	ASC_DBG_PRT_SCSI_HOST(2, shost);
 
 	ret = scsi_add_host(shost, boardp->dev);
 	if (ret)
-		goto err_free_wide_mem;
+		goto err_free_mem;
 
 	scsi_scan_host(shost);
 	return 0;
 
- err_free_wide_mem:
-	advansys_wide_free_mem(boardp);
+ err_free_mem:
+	if (ASC_NARROW_BOARD(boardp))
+		kfree(asc_dvc_varp->overrun_buf);
+	else
+		advansys_wide_free_mem(boardp);
+ err_free_irq:
 	free_irq(boardp->irq, shost);
  err_free_dma:
 #ifdef CONFIG_ISA


To me looks safer than moving everything out from AscInitMicroCodeVar,
what do you think? If it's ok I submit it with changelog/signed-off

> 
> James
> 
> > +		kfree(asc_dvc_varp->overrun_buf);
> > +	} else {
> > +		advansys_wide_free_mem(boardp);
> > +	}
> > + err_free_irq:
> >  	free_irq(boardp->irq, shost);
> >   err_free_dma:
> >  #ifdef CONFIG_ISA

-- 
[]'s
Herton
--
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: [PATCH v2] advansys: fix regression with request_firmware change

[Herton Ronaldo Krzesinski]

Em Seg 29 Mar 2010, às 17:36:30, Herton Ronaldo Krzesinski escreveu:
> Em Dom 28 Mar 2010, às 12:25:20, James Bottomley escreveu:
> > On Fri, 2010-03-26 at 20:05 -0300, Herton Ronaldo Krzesinski wrote:

<snip>

> > This is both nasty and not really a fix:  the overrun buf is still
> > mapped many times on the reset path and only ever unmapped once, which
> > is still an unfixed bug in this code.  I think the map needs to be moved
> > out of AscInitMicroCodeVar() to somewhere in here.
> 
> I didn't want to do this since I think there will be much changes and I can't
> test properly as I don't have the hardware (also may be too much for what
> looks like a legacy driver/hardware). But I aggree this if is ugly and not
> unmapping when needed on reset. What I thought can be better is this, to fix
> error handling also on AscInitMicroCodeVar then:

Sorry the diff had some typos (didn't test it), but was only for the idea

> 
> diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
> index 9201afe..01481d1 100644
> --- a/drivers/scsi/advansys.c
> +++ b/drivers/scsi/advansys.c
> @@ -460,6 +460,7 @@ typedef struct asc_risc_sg_list_q {
>  #define ASC_IERR_BIST_PRE_TEST		0x0800	/* BIST pre-test error */
>  #define ASC_IERR_BIST_RAM_TEST		0x1000	/* BIST RAM test error */
>  #define ASC_IERR_BAD_CHIPTYPE		0x2000	/* Invalid chip_type setting */
> +#define ASC_IERR_DMA_MAP_SINGLE 	0x4000  /* Error with dma_map_single */
>  
>  #define ASC_DEF_MAX_TOTAL_QNG   (0xF0)
>  #define ASC_MIN_TAG_Q_PER_DVC   (0x04)
> @@ -4701,14 +4702,12 @@ static void AscInitQLinkVar(ASC_DVC_VAR *asc_dvc)
>  static ushort AscInitMicroCodeVar(ASC_DVC_VAR *asc_dvc)
>  {
>  	int i;
> -	ushort warn_code;
>  	PortAddr iop_base;
>  	ASC_PADDR phy_addr;
>  	ASC_DCNT phy_size;
>  	struct asc_board *board = asc_dvc_to_board(asc_dvc);
>  
>  	iop_base = asc_dvc->iop_base;
> -	warn_code = 0;
>  	for (i = 0; i <= ASC_MAX_TID; i++) {
>  		AscPutMCodeInitSDTRAtID(iop_base, i,
>  					asc_dvc->cfg->sdtr_period_offset[i]);
> @@ -4724,6 +4723,10 @@ static ushort AscInitMicroCodeVar(ASC_DVC_VAR *asc_dvc)
>  	BUG_ON((unsigned long)asc_dvc->overrun_buf & 7);
>  	asc_dvc->overrun_dma = dma_map_single(board->dev, asc_dvc->overrun_buf,
>  					ASC_OVERRUN_BSIZE, DMA_FROM_DEVICE);
> +	if (dma_mapping_error(boad->dev, asc_dvc->overrun_dma)) {

should be board->dev

> +		asc_dvc->err_code |= ASC_IERR_DMA_MAP_SINGLE;
> +		return -ENOMEM;
> +	}
>  	phy_addr = cpu_to_le32(asc_dvc->overrun_dma);
>  	AscMemDWordCopyPtrToLram(iop_base, ASCV_OVERRUN_PADDR_D,
>  				 (uchar *)&phy_addr, 1);
> @@ -4739,14 +4742,19 @@ static ushort AscInitMicroCodeVar(ASC_DVC_VAR *asc_dvc)
>  	AscSetPCAddr(iop_base, ASC_MCODE_START_ADDR);
>  	if (AscGetPCAddr(iop_base) != ASC_MCODE_START_ADDR) {
>  		asc_dvc->err_code |= ASC_IERR_SET_PC_ADDR;
> -		return warn_code;
> +		goto err_mcode_start;
>  	}
>  	if (AscStartChip(iop_base) != 1) {
>  		asc_dvc->err_code |= ASC_IERR_START_STOP_CHIP;
> -		return warn_code;
> +		goto err_mcode_start;
>  	}
>  
> -	return warn_code;
> +	return 0;
> +
> +err_mcode_start:
> +	dma_unmap_single(board->dev, asc_dvc->overrun_buf,

Replace asc_dvc->overrun_buf by asc_dvc->overrun_dma

> +			 ASC_OVERRUN_BSIZE, DMA_FROM_DEVICE);
> +	return UW_ERR;
>  }
>  
>  static ushort AscInitAsc1000Driver(ASC_DVC_VAR *asc_dvc)
> @@ -4802,6 +4810,8 @@ static ushort AscInitAsc1000Driver(ASC_DVC_VAR *asc_dvc)
>  	}
>  	release_firmware(fw);
>  	warn_code |= AscInitMicroCodeVar(asc_dvc);
> +	if (asc_dvc->err_code != 0)
> +		return warn_code;
>  	asc_dvc->init_state |= ASC_INIT_STATE_END_LOAD_MC;
>  	AscEnableInterrupt(iop_base);
>  	return warn_code;
> @@ -12311,7 +12321,7 @@ static int __devinit advansys_board_found(struct Scsi_Host *shost,
>  		asc_dvc_varp->overrun_buf = kzalloc(ASC_OVERRUN_BSIZE, GFP_KERNEL);
>  		if (!asc_dvc_varp->overrun_buf) {
>  			ret = -ENOMEM;
> -			goto err_free_wide_mem;
> +			goto err_free_irq;
>  		}
>  		warn_code = AscInitAsc1000Driver(asc_dvc_varp);
>  
> @@ -12322,28 +12332,31 @@ static int __devinit advansys_board_found(struct Scsi_Host *shost,
>  					asc_dvc_varp->err_code);
>  			if (asc_dvc_varp->err_code) {
>  				ret = -ENODEV;
> -				kfree(asc_dvc_varp->overrun_buf);
> +				goto err_free_mem;
>  			}
>  		}
>  	} else {
> -		if (advansys_wide_init_chip(shost))
> +		if (advansys_wide_init_chip(shost)) {
>  			ret = -ENODEV;
> +			goto err_free_mem;
> +		}
>  	}
>  
> -	if (ret)
> -		goto err_free_wide_mem;
> -
>  	ASC_DBG_PRT_SCSI_HOST(2, shost);
>  
>  	ret = scsi_add_host(shost, boardp->dev);
>  	if (ret)
> -		goto err_free_wide_mem;
> +		goto err_free_mem;
>  
>  	scsi_scan_host(shost);
>  	return 0;
>  
> - err_free_wide_mem:
> -	advansys_wide_free_mem(boardp);
> + err_free_mem:
> +	if (ASC_NARROW_BOARD(boardp))
> +		kfree(asc_dvc_varp->overrun_buf);
> +	else
> +		advansys_wide_free_mem(boardp);
> + err_free_irq:
>  	free_irq(boardp->irq, shost);
>   err_free_dma:
>  #ifdef CONFIG_ISA
> 
> 
> To me looks safer than moving everything out from AscInitMicroCodeVar,
> what do you think? If it's ok I submit it with changelog/signed-off
> 

<snip>

-- 
[]'s
Herton
--
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: [PATCH v2] advansys: fix regression with request_firmware change

[Herton Ronaldo Krzesinski]

Em Seg 29 Mar 2010, às 17:49:08, Herton Ronaldo Krzesinski escreveu:
> Em Seg 29 Mar 2010, às 17:36:30, Herton Ronaldo Krzesinski escreveu:
> > Em Dom 28 Mar 2010, às 12:25:20, James Bottomley escreveu:
> > > On Fri, 2010-03-26 at 20:05 -0300, Herton Ronaldo Krzesinski wrote:
> 
> <snip>
> 
> > > This is both nasty and not really a fix:  the overrun buf is still
> > > mapped many times on the reset path and only ever unmapped once, which
> > > is still an unfixed bug in this code.  I think the map needs to be moved
> > > out of AscInitMicroCodeVar() to somewhere in here.
> > 
> > I didn't want to do this since I think there will be much changes and I can't
> > test properly as I don't have the hardware (also may be too much for what
> > looks like a legacy driver/hardware). But I aggree this if is ugly and not
> > unmapping when needed on reset. What I thought can be better is this, to fix
> > error handling also on AscInitMicroCodeVar then:
> 
> Sorry the diff had some typos (didn't test it), but was only for the idea

Damn, and in the diff I also forgot about dma unmap on scsi_add_host error...
please consider version below, also compile tested:

diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
index 9201afe..43289e8 100644
--- a/drivers/scsi/advansys.c
+++ b/drivers/scsi/advansys.c
@@ -460,6 +460,7 @@ typedef struct asc_risc_sg_list_q {
 #define ASC_IERR_BIST_PRE_TEST		0x0800	/* BIST pre-test error */
 #define ASC_IERR_BIST_RAM_TEST		0x1000	/* BIST RAM test error */
 #define ASC_IERR_BAD_CHIPTYPE		0x2000	/* Invalid chip_type setting */
+#define ASC_IERR_DMA_MAP_SINGLE 	0x4000  /* Error with dma_map_single */
 
 #define ASC_DEF_MAX_TOTAL_QNG   (0xF0)
 #define ASC_MIN_TAG_Q_PER_DVC   (0x04)
@@ -4701,14 +4702,12 @@ static void AscInitQLinkVar(ASC_DVC_VAR *asc_dvc)
 static ushort AscInitMicroCodeVar(ASC_DVC_VAR *asc_dvc)
 {
 	int i;
-	ushort warn_code;
 	PortAddr iop_base;
 	ASC_PADDR phy_addr;
 	ASC_DCNT phy_size;
 	struct asc_board *board = asc_dvc_to_board(asc_dvc);
 
 	iop_base = asc_dvc->iop_base;
-	warn_code = 0;
 	for (i = 0; i <= ASC_MAX_TID; i++) {
 		AscPutMCodeInitSDTRAtID(iop_base, i,
 					asc_dvc->cfg->sdtr_period_offset[i]);
@@ -4724,6 +4723,10 @@ static ushort AscInitMicroCodeVar(ASC_DVC_VAR *asc_dvc)
 	BUG_ON((unsigned long)asc_dvc->overrun_buf & 7);
 	asc_dvc->overrun_dma = dma_map_single(board->dev, asc_dvc->overrun_buf,
 					ASC_OVERRUN_BSIZE, DMA_FROM_DEVICE);
+	if (dma_mapping_error(board->dev, asc_dvc->overrun_dma)) {
+		asc_dvc->err_code |= ASC_IERR_DMA_MAP_SINGLE;
+		return -ENOMEM;
+	}
 	phy_addr = cpu_to_le32(asc_dvc->overrun_dma);
 	AscMemDWordCopyPtrToLram(iop_base, ASCV_OVERRUN_PADDR_D,
 				 (uchar *)&phy_addr, 1);
@@ -4739,14 +4742,19 @@ static ushort AscInitMicroCodeVar(ASC_DVC_VAR *asc_dvc)
 	AscSetPCAddr(iop_base, ASC_MCODE_START_ADDR);
 	if (AscGetPCAddr(iop_base) != ASC_MCODE_START_ADDR) {
 		asc_dvc->err_code |= ASC_IERR_SET_PC_ADDR;
-		return warn_code;
+		goto err_mcode_start;
 	}
 	if (AscStartChip(iop_base) != 1) {
 		asc_dvc->err_code |= ASC_IERR_START_STOP_CHIP;
-		return warn_code;
+		goto err_mcode_start;
 	}
 
-	return warn_code;
+	return 0;
+
+err_mcode_start:
+	dma_unmap_single(board->dev, asc_dvc->overrun_dma,
+			 ASC_OVERRUN_BSIZE, DMA_FROM_DEVICE);
+	return UW_ERR;
 }
 
 static ushort AscInitAsc1000Driver(ASC_DVC_VAR *asc_dvc)
@@ -4802,6 +4810,8 @@ static ushort AscInitAsc1000Driver(ASC_DVC_VAR *asc_dvc)
 	}
 	release_firmware(fw);
 	warn_code |= AscInitMicroCodeVar(asc_dvc);
+	if (asc_dvc->err_code != 0)
+		return warn_code;
 	asc_dvc->init_state |= ASC_INIT_STATE_END_LOAD_MC;
 	AscEnableInterrupt(iop_base);
 	return warn_code;
@@ -12311,7 +12321,7 @@ static int __devinit advansys_board_found(struct Scsi_Host *shost,
 		asc_dvc_varp->overrun_buf = kzalloc(ASC_OVERRUN_BSIZE, GFP_KERNEL);
 		if (!asc_dvc_varp->overrun_buf) {
 			ret = -ENOMEM;
-			goto err_free_wide_mem;
+			goto err_free_irq;
 		}
 		warn_code = AscInitAsc1000Driver(asc_dvc_varp);
 
@@ -12323,27 +12333,34 @@ static int __devinit advansys_board_found(struct Scsi_Host *shost,
 			if (asc_dvc_varp->err_code) {
 				ret = -ENODEV;
 				kfree(asc_dvc_varp->overrun_buf);
+				goto err_free_irq;
 			}
 		}
 	} else {
-		if (advansys_wide_init_chip(shost))
+		if (advansys_wide_init_chip(shost)) {
 			ret = -ENODEV;
+			advansys_wide_free_mem(boardp);
+			goto err_free_irq;
+		}
 	}
 
-	if (ret)
-		goto err_free_wide_mem;
-
 	ASC_DBG_PRT_SCSI_HOST(2, shost);
 
 	ret = scsi_add_host(shost, boardp->dev);
 	if (ret)
-		goto err_free_wide_mem;
+		goto err_add_host;
 
 	scsi_scan_host(shost);
 	return 0;
 
- err_free_wide_mem:
-	advansys_wide_free_mem(boardp);
+ err_add_host:
+	if (ASC_NARROW_BOARD(boardp)) {
+		dma_unmap_single(boardp->dev, asc_dvc_varp->overrun_dma,
+				 ASC_OVERRUN_BSIZE, DMA_FROM_DEVICE);
+		kfree(asc_dvc_varp->overrun_buf);
+	} else
+		advansys_wide_free_mem(boardp);
+ err_free_irq:
 	free_irq(boardp->irq, shost);
  err_free_dma:
 #ifdef CONFIG_ISA


-- 
[]'s
Herton
--
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: [PATCH v2] advansys: fix regression with request_firmware change

[James Bottomley]

On Mon, 2010-03-29 at 18:22 -0300, Herton Ronaldo Krzesinski wrote:
> Em Seg 29 Mar 2010, às 17:49:08, Herton Ronaldo Krzesinski escreveu:
> > Em Seg 29 Mar 2010, às 17:36:30, Herton Ronaldo Krzesinski escreveu:
> > > Em Dom 28 Mar 2010, às 12:25:20, James Bottomley escreveu:
> > > > On Fri, 2010-03-26 at 20:05 -0300, Herton Ronaldo Krzesinski wrote:
> > 
> > <snip>
> > 
> > > > This is both nasty and not really a fix:  the overrun buf is still
> > > > mapped many times on the reset path and only ever unmapped once, which
> > > > is still an unfixed bug in this code.  I think the map needs to be moved
> > > > out of AscInitMicroCodeVar() to somewhere in here.
> > > 
> > > I didn't want to do this since I think there will be much changes and I can't
> > > test properly as I don't have the hardware (also may be too much for what
> > > looks like a legacy driver/hardware). But I aggree this if is ugly and not
> > > unmapping when needed on reset. What I thought can be better is this, to fix
> > > error handling also on AscInitMicroCodeVar then:
> > 
> > Sorry the diff had some typos (didn't test it), but was only for the idea
> 
> Damn, and in the diff I also forgot about dma unmap on scsi_add_host error...
> please consider version below, also compile tested:
> 
> diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
> index 9201afe..43289e8 100644
> --- a/drivers/scsi/advansys.c
> +++ b/drivers/scsi/advansys.c
> @@ -460,6 +460,7 @@ typedef struct asc_risc_sg_list_q {
>  #define ASC_IERR_BIST_PRE_TEST		0x0800	/* BIST pre-test error */
>  #define ASC_IERR_BIST_RAM_TEST		0x1000	/* BIST RAM test error */
>  #define ASC_IERR_BAD_CHIPTYPE		0x2000	/* Invalid chip_type setting */
> +#define ASC_IERR_DMA_MAP_SINGLE 	0x4000  /* Error with dma_map_single */

You don't actually need another flag for this.  asc_board is in
shost_priv and, as such, is zero initialised, so you can rely on
overrun_dma being NULL unless it's mapped on a narrow board.

James


--
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: [PATCH v2] advansys: fix regression with request_firmware change

[Herton Ronaldo Krzesinski]

Em Seg 29 Mar 2010, às 20:16:05, James Bottomley escreveu:
> On Mon, 2010-03-29 at 18:22 -0300, Herton Ronaldo Krzesinski wrote:
> > Em Seg 29 Mar 2010, às 17:49:08, Herton Ronaldo Krzesinski escreveu:
> > > Em Seg 29 Mar 2010, às 17:36:30, Herton Ronaldo Krzesinski escreveu:
> > > > Em Dom 28 Mar 2010, às 12:25:20, James Bottomley escreveu:
> > > > > On Fri, 2010-03-26 at 20:05 -0300, Herton Ronaldo Krzesinski wrote:
> > > 
> > > <snip>
> > > 
> > > > > This is both nasty and not really a fix:  the overrun buf is still
> > > > > mapped many times on the reset path and only ever unmapped once, which
> > > > > is still an unfixed bug in this code.  I think the map needs to be moved
> > > > > out of AscInitMicroCodeVar() to somewhere in here.
> > > > 
> > > > I didn't want to do this since I think there will be much changes and I can't
> > > > test properly as I don't have the hardware (also may be too much for what
> > > > looks like a legacy driver/hardware). But I aggree this if is ugly and not
> > > > unmapping when needed on reset. What I thought can be better is this, to fix
> > > > error handling also on AscInitMicroCodeVar then:
> > > 
> > > Sorry the diff had some typos (didn't test it), but was only for the idea
> > 
> > Damn, and in the diff I also forgot about dma unmap on scsi_add_host error...
> > please consider version below, also compile tested:
> > 
> > diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
> > index 9201afe..43289e8 100644
> > --- a/drivers/scsi/advansys.c
> > +++ b/drivers/scsi/advansys.c
> > @@ -460,6 +460,7 @@ typedef struct asc_risc_sg_list_q {
> >  #define ASC_IERR_BIST_PRE_TEST		0x0800	/* BIST pre-test error */
> >  #define ASC_IERR_BIST_RAM_TEST		0x1000	/* BIST RAM test error */
> >  #define ASC_IERR_BAD_CHIPTYPE		0x2000	/* Invalid chip_type setting */
> > +#define ASC_IERR_DMA_MAP_SINGLE 	0x4000  /* Error with dma_map_single */
> 
> You don't actually need another flag for this.  asc_board is in
> shost_priv and, as such, is zero initialised, so you can rely on
> overrun_dma being NULL unless it's mapped on a narrow board.

Ok, this is new diff avoiding adding an extra flag:

diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
index 9201afe..7f87979 100644
--- a/drivers/scsi/advansys.c
+++ b/drivers/scsi/advansys.c
@@ -4724,6 +4724,10 @@ static ushort AscInitMicroCodeVar(ASC_DVC_VAR *asc_dvc)
 	BUG_ON((unsigned long)asc_dvc->overrun_buf & 7);
 	asc_dvc->overrun_dma = dma_map_single(board->dev, asc_dvc->overrun_buf,
 					ASC_OVERRUN_BSIZE, DMA_FROM_DEVICE);
+	if (dma_mapping_error(board->dev, asc_dvc->overrun_dma)) {
+		warn_code = -ENOMEM;
+		goto err_dma_map;
+	}
 	phy_addr = cpu_to_le32(asc_dvc->overrun_dma);
 	AscMemDWordCopyPtrToLram(iop_base, ASCV_OVERRUN_PADDR_D,
 				 (uchar *)&phy_addr, 1);
@@ -4739,14 +4743,23 @@ static ushort AscInitMicroCodeVar(ASC_DVC_VAR *asc_dvc)
 	AscSetPCAddr(iop_base, ASC_MCODE_START_ADDR);
 	if (AscGetPCAddr(iop_base) != ASC_MCODE_START_ADDR) {
 		asc_dvc->err_code |= ASC_IERR_SET_PC_ADDR;
-		return warn_code;
+		warn_code = UW_ERR;
+		goto err_mcode_start;
 	}
 	if (AscStartChip(iop_base) != 1) {
 		asc_dvc->err_code |= ASC_IERR_START_STOP_CHIP;
-		return warn_code;
+		warn_code = UW_ERR;
+		goto err_mcode_start;
 	}
 
 	return warn_code;
+
+err_mcode_start:
+	dma_unmap_single(board->dev, asc_dvc->overrun_dma,
+			 ASC_OVERRUN_BSIZE, DMA_FROM_DEVICE);
+err_dma_map:
+	asc_dvc->overrun_dma = 0;
+	return warn_code;
 }
 
 static ushort AscInitAsc1000Driver(ASC_DVC_VAR *asc_dvc)
@@ -4802,6 +4815,8 @@ static ushort AscInitAsc1000Driver(ASC_DVC_VAR *asc_dvc)
 	}
 	release_firmware(fw);
 	warn_code |= AscInitMicroCodeVar(asc_dvc);
+	if (!asc_dvc->overrun_dma)
+		return warn_code;
 	asc_dvc->init_state |= ASC_INIT_STATE_END_LOAD_MC;
 	AscEnableInterrupt(iop_base);
 	return warn_code;
@@ -7978,9 +7993,10 @@ static int advansys_reset(struct scsi_cmnd *scp)
 		status = AscInitAsc1000Driver(asc_dvc);
 
 		/* Refer to ASC_IERR_* definitions for meaning of 'err_code'. */
-		if (asc_dvc->err_code) {
+		if (asc_dvc->err_code || !asc_dvc->overrun_dma) {
 			scmd_printk(KERN_INFO, scp, "SCSI bus reset error: "
-				    "0x%x\n", asc_dvc->err_code);
+				    "0x%x, status: 0x%x\n", asc_dvc->err_code,
+				    status);
 			ret = FAILED;
 		} else if (status) {
 			scmd_printk(KERN_INFO, scp, "SCSI bus reset warning: "
@@ -12311,7 +12327,7 @@ static int __devinit advansys_board_found(struct Scsi_Host *shost,
 		asc_dvc_varp->overrun_buf = kzalloc(ASC_OVERRUN_BSIZE, GFP_KERNEL);
 		if (!asc_dvc_varp->overrun_buf) {
 			ret = -ENOMEM;
-			goto err_free_wide_mem;
+			goto err_free_irq;
 		}
 		warn_code = AscInitAsc1000Driver(asc_dvc_varp);
 
@@ -12320,30 +12336,36 @@ static int __devinit advansys_board_found(struct Scsi_Host *shost,
 					"warn 0x%x, error 0x%x\n",
 					asc_dvc_varp->init_state, warn_code,
 					asc_dvc_varp->err_code);
-			if (asc_dvc_varp->err_code) {
+			if (!asc_dvc_varp->overrun_dma) {
 				ret = -ENODEV;
-				kfree(asc_dvc_varp->overrun_buf);
+				goto err_free_mem;
 			}
 		}
 	} else {
-		if (advansys_wide_init_chip(shost))
+		if (advansys_wide_init_chip(shost)) {
 			ret = -ENODEV;
+			goto err_free_mem;
+		}
 	}
 
-	if (ret)
-		goto err_free_wide_mem;
-
 	ASC_DBG_PRT_SCSI_HOST(2, shost);
 
 	ret = scsi_add_host(shost, boardp->dev);
 	if (ret)
-		goto err_free_wide_mem;
+		goto err_free_mem;
 
 	scsi_scan_host(shost);
 	return 0;
 
- err_free_wide_mem:
-	advansys_wide_free_mem(boardp);
+ err_free_mem:
+	if (ASC_NARROW_BOARD(boardp)) {
+		if (asc_dvc_varp->overrun_dma)
+			dma_unmap_single(boardp->dev, asc_dvc_varp->overrun_dma,
+					 ASC_OVERRUN_BSIZE, DMA_FROM_DEVICE);
+		kfree(asc_dvc_varp->overrun_buf);
+	} else
+		advansys_wide_free_mem(boardp);
+ err_free_irq:
 	free_irq(boardp->irq, shost);
  err_free_dma:
 #ifdef CONFIG_ISA


> 
> James
> 
> 
> 

-- 
[]'s
Herton
--
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