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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html