advansys: fix regression with request_firmware change

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 <[email protected]>

---
 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)

drivers/scsi/advansys.c +14 -7

index 22626ab..9a3c68f 100644
@@ -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;

drivers/scsi/advansys.c +5 -0

+		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

Quoted 14 lines

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.

Quoted 7 lines

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.

Quoted 111 lines

Signed-off-by Herton Ronaldo Krzesinski <[email protected]>

---
 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)

drivers/scsi/advansys.c +14 -7

index 22626ab..9a3c68f 100644
@@ -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;
 

drivers/scsi/advansys.c static int __devinit advansys_board_found(struct Scsi_Host *shost, · line 12350

Quoted 9 lines

drivers/scsi/advansys.c +7 -2

- 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

Quoted 8 lines

drivers/scsi/advansys.c +5 -0

+		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

Em Dom 28 Mar 2010, às 12:25:20, James Bottomley escreveu:

Quoted 1 line · Herton Ronaldo Krzesinski

Quoted 4 lines

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:

drivers/scsi/advansys.c +1 -2

index 9201afe..01481d1 100644
@@ -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]);

drivers/scsi/advansys.c +4 -1

+		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) {

drivers/scsi/advansys.c +16 -9

+			 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

Em Seg 29 Mar 2010, às 17:36:30, Herton Ronaldo Krzesinski escreveu:

Quoted 2 lines

Em Dom 28 Mar 2010, às 12:25:20, James Bottomley escreveu:

Quoted 1 line · Herton Ronaldo Krzesinski

<snip>

Quoted 5 lines

Quoted 4 lines

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.

Quoted 5 lines

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

Em Seg 29 Mar 2010, às 17:49:08, Herton Ronaldo Krzesinski escreveu:

Quoted 11 lines

Em Seg 29 Mar 2010, às 17:36:30, Herton Ronaldo Krzesinski escreveu:

Quoted 2 lines

Em Dom 28 Mar 2010, às 12:25:20, James Bottomley escreveu:

Quoted 1 line · Herton Ronaldo Krzesinski

<snip>

Quoted 5 lines

Quoted 4 lines

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.

Quoted 7 lines

Quoted 5 lines

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:

drivers/scsi/advansys.c +0 -0

index 9201afe..43289e8 100644

Quoted 23 lines

Em Seg 29 Mar 2010, às 17:49:08, Herton Ronaldo Krzesinski escreveu:

Quoted 11 lines

Em Seg 29 Mar 2010, às 17:36:30, Herton Ronaldo Krzesinski escreveu:

Quoted 2 lines

Em Dom 28 Mar 2010, às 12:25:20, James Bottomley escreveu:

Quoted 1 line · Herton Ronaldo Krzesinski

<snip>

Quoted 5 lines

Quoted 4 lines

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.

Quoted 7 lines

Quoted 5 lines

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:

drivers/scsi/advansys.c typedef struct asc_risc_sg_list_q { · line 463

Quoted 9 lines

drivers/scsi/advansys.c +1 -0

index 9201afe..43289e8 100644
@@ -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