On Fri, 2010-03-26 at 20:05 -0300, Herton Ronaldo Krzesinski wrote:
quoted text > 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.
quoted text > 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
quoted text > + 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 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
