[PATCH 4/4] scsi: pm8001: fix pm8001_store_update_fw

[PATCH 4/4] scsi: pm8001: fix pm8001_store_update_fw

[Tomas Henzl]

The current implementation may mix the negative value returned
from pm8001_set_nvmd with with count. -(-ENOMEM) could be interpreted
as bytes programmed, this patch fixes it.

Signed-off-by: Tomas Henzl <thenzl@redhat.com>
---
 drivers/scsi/pm8001/pm8001_ctl.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_ctl.c b/drivers/scsi/pm8001/pm8001_ctl.c
index 8f1f882..847755b 100644
--- a/drivers/scsi/pm8001/pm8001_ctl.c
+++ b/drivers/scsi/pm8001/pm8001_ctl.c
@@ -650,14 +650,14 @@ static ssize_t pm8001_store_update_fw(struct device *cdev,
 	cmd_ptr = kzalloc(count*2, GFP_KERNEL);
 
 	if (!cmd_ptr) {
-		err = FAIL_OUT_MEMORY;
+		err = -FAIL_OUT_MEMORY;
 		goto out;
 	}
 
 	filename_ptr = cmd_ptr + count;
 	res = sscanf(buf, "%s %s", cmd_ptr, filename_ptr);
 	if (res != 2) {
-		err = FAIL_PARAMETERS;
+		err = -FAIL_PARAMETERS;
 		goto out1;
 	}
 
@@ -669,12 +669,12 @@ static ssize_t pm8001_store_update_fw(struct device *cdev,
 		}
 	}
 	if (flash_command == FLASH_CMD_NONE) {
-		err = FAIL_PARAMETERS;
+		err = -FAIL_PARAMETERS;
 		goto out1;
 	}
 
 	if (pm8001_ha->fw_status == FLASH_IN_PROGRESS) {
-		err = FLASH_IN_PROGRESS;
+		err = -FLASH_IN_PROGRESS;
 		goto out1;
 	}
 	err = request_firmware(&pm8001_ha->fw_image,
@@ -685,7 +685,7 @@ static ssize_t pm8001_store_update_fw(struct device *cdev,
 		PM8001_FAIL_DBG(pm8001_ha,
 			pm8001_printk("Failed to load firmware image file %s,"
 			" error %d\n", filename_ptr, err));
-		err = FAIL_OPEN_BIOS_FILE;
+		err = -FAIL_OPEN_BIOS_FILE;
 		goto out1;
 	}
 
@@ -700,7 +700,7 @@ static ssize_t pm8001_store_update_fw(struct device *cdev,
 		break;
 	default:
 		pm8001_ha->fw_status = FAIL_PARAMETERS;
-		err = FAIL_PARAMETERS;
+		err = -FAIL_PARAMETERS;
 		break;
 	}
 	release_firmware(pm8001_ha->fw_image);
@@ -709,10 +709,10 @@ out1:
 out:
 	pm8001_ha->fw_status = err;
 
-	if (!err)
-		return count;
-	else
-		return -err;
+	if (err)
+		return err;
+
+	return count;
 }
 
 static ssize_t pm8001_show_update_fw(struct device *cdev,
-- 
1.8.3.1

Re: [PATCH 4/4] scsi: pm8001: fix pm8001_store_update_fw

[Christoph Hellwig]

On Mon, Jul 07, 2014 at 05:20:01PM +0200, Tomas Henzl wrote:
> The current implementation may mix the negative value returned
> from pm8001_set_nvmd with with count. -(-ENOMEM) could be interpreted
> as bytes programmed, this patch fixes it.

This still doesn;t look correct to me as err mixes up the driver
internal FAIL_* codes with Linux error codes.  It seems like for the
FAIL_* codes should only go into ->fw_status and the return value
should be a proper Linux error code.

Funny fact: the FAIL_* / FLASH_IN_PROGRESS codes seems to be the same
between aic94xx and pm8001.

Re: [PATCH 4/4] scsi: pm8001: fix pm8001_store_update_fw

[Tomas Henzl]

On 07/10/2014 08:43 AM, Christoph Hellwig wrote:
> On Mon, Jul 07, 2014 at 05:20:01PM +0200, Tomas Henzl wrote:
>> The current implementation may mix the negative value returned
>> from pm8001_set_nvmd with with count. -(-ENOMEM) could be interpreted
>> as bytes programmed, this patch fixes it.
> This still doesn;t look correct to me as err mixes up the driver
> internal FAIL_* codes with Linux error codes.  It seems like for the
> FAIL_* codes should only go into ->fw_status and the return value
> should be a proper Linux error code.

And the fw_status might be later used to show error strings in pm8001_show_update_fw,
if it is so it depends on the flash utility but it seems likely. 

>
> Funny fact: the FAIL_* / FLASH_IN_PROGRESS codes seems to be the same
> between aic94xx and pm8001.

And similar story there too - asd_store_update_bios -...- asd_poll_flash(might return -ENOENT)

Maybe the flash utility ignores the return value or it has never happened.
---------------------

I'll try to find what seems to be the most probable way and post it in few days.
This patch is not related to the patches 1-3/4, so just wait with this one.

Thanks, Tomas

>
> --
> 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 4/4] scsi: pm8001: fix pm8001_store_update_fw

[Tomas Henzl]

On 07/10/2014 03:30 PM, Tomas Henzl wrote:
> On 07/10/2014 08:43 AM, Christoph Hellwig wrote:
>> On Mon, Jul 07, 2014 at 05:20:01PM +0200, Tomas Henzl wrote:
>>> The current implementation may mix the negative value returned
>>> from pm8001_set_nvmd with with count. -(-ENOMEM) could be interpreted
>>> as bytes programmed, this patch fixes it.
>> This still doesn;t look correct to me as err mixes up the driver
>> internal FAIL_* codes with Linux error codes.  It seems like for the
>> FAIL_* codes should only go into ->fw_status and the return value
>> should be a proper Linux error code.
> And the fw_status might be later used to show error strings in pm8001_show_update_fw,
> if it is so it depends on the flash utility but it seems likely. 
>
>> Funny fact: the FAIL_* / FLASH_IN_PROGRESS codes seems to be the same
>> between aic94xx and pm8001.
> And similar story there too - asd_store_update_bios -...- asd_poll_flash(might return -ENOENT)
>
> Maybe the flash utility ignores the return value or it has never happened.
> ---------------------
>
> I'll try to find what seems to be the most probable way and post it in few days.

Christoph,
from few days it is three weeks and the patch has been just replaced
by "[PATCH 1/3] pm8001: fix pm8001_store_update_fw" sent by Suresh,
so you can drop this patch.
(I've asked Suresh off-list to test it, I didn't want to brick my card by testing
fw flashes.)
Thanks,
Tomas

> This patch is not related to the patches 1-3/4, so just wait with this one.
>
> Thanks, Tomas
>
>> --
>> 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
> --
> 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