| Message ID | 20151216110746.GB21018@mwanda (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wed, Dec 16, 2015 at 02:07:46PM +0300, Dan Carpenter wrote: > After the loop we test for "if (!retries) " as a failure, but actually > the post-op here will end with retries set to -1. I have fixed this by > using a pre-op instead. > > Fixes: 7ec0effd30bb ('[SCSI] qla2xxx: Add support for ISP8044.') > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > diff --git a/drivers/scsi/qla2xxx/qla_nx2.c b/drivers/scsi/qla2xxx/qla_nx2.c > index 007192d..66d629e 100644 > --- a/drivers/scsi/qla2xxx/qla_nx2.c > +++ b/drivers/scsi/qla2xxx/qla_nx2.c > @@ -3491,7 +3491,7 @@ qla8044_poll_flash_status_reg(struct scsi_qla_host *vha) > int retries = QLA8044_FLASH_READ_RETRY_COUNT; > int ret_val = QLA_SUCCESS; > > - while (retries--) { > + while (--retries) { > ret_val = qla8044_rd_reg_indirect(vha, QLA8044_FLASH_STATUS, > &flash_status); > if (ret_val) { > -- > 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 Good catch. Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
>>>>> "Dan" == Dan Carpenter <dan.carpenter@oracle.com> writes:
Dan> After the loop we test for "if (!retries) " as a failure, but
Dan> actually the post-op here will end with retries set to -1. I have
Dan> fixed this by using a pre-op instead.
Applied to 4.4/scsi-fixes.
Hi Dan, On 12/16/15, 3:07 AM, "linux-scsi-owner@vger.kernel.org on behalf of Dan Carpenter" <linux-scsi-owner@vger.kernel.org on behalf of dan.carpenter@oracle.com> wrote: >After the loop we test for "if (!retries) " as a failure, but actually >the post-op here will end with retries set to -1. I have fixed this by >using a pre-op instead. > >Fixes: 7ec0effd30bb ('[SCSI] qla2xxx: Add support for ISP8044.') >Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > >diff --git a/drivers/scsi/qla2xxx/qla_nx2.c >b/drivers/scsi/qla2xxx/qla_nx2.c >index 007192d..66d629e 100644 >--- a/drivers/scsi/qla2xxx/qla_nx2.c >+++ b/drivers/scsi/qla2xxx/qla_nx2.c >@@ -3491,7 +3491,7 @@ qla8044_poll_flash_status_reg(struct scsi_qla_host >*vha) > int retries = QLA8044_FLASH_READ_RETRY_COUNT; > int ret_val = QLA_SUCCESS; > >- while (retries--) { >+ while (--retries) { > ret_val = qla8044_rd_reg_indirect(vha, QLA8044_FLASH_STATUS, > &flash_status); > if (ret_val) { There is flaw in this pre-decrement logic here for following 2 reasons 1. The pre-decrement reduces the total number of retries by one 2. If the retries is already Zero, then the pre-decrement would cause loop to iterate for a very large number of times. The correct fix for this would be to change the if(!retries) to if (retries == -1) or if (retries < 0) We¹ll send a follow up patch to fix this. Thanks, Himanshu >-- >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
On Wed, Dec 16, 2015 at 05:50:17PM -0500, Martin K. Petersen wrote: > >>>>> "Dan" == Dan Carpenter <dan.carpenter@oracle.com> writes: > > Dan> After the loop we test for "if (!retries) " as a failure, but > Dan> actually the post-op here will end with retries set to -1. I have > Dan> fixed this by using a pre-op instead. > > Applied to 4.4/scsi-fixes. > The qlogic devs asked me to redo this patch but you have already applied it. Should I resend or redo it on top of the earlier patch. (James's tree was rebaseable for a day after a patch was "applied".) regards, dan carpenter -- 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
>>>>> "Dan" == Dan Carpenter <dan.carpenter@oracle.com> writes:
Dan> The qlogic devs asked me to redo this patch but you have already
Dan> applied it. Should I resend or redo it on top of the earlier
Dan> patch. (James's tree was rebaseable for a day after a patch was
Dan> "applied".)
I dropped it for now.
diff --git a/drivers/scsi/qla2xxx/qla_nx2.c b/drivers/scsi/qla2xxx/qla_nx2.c index 007192d..66d629e 100644 --- a/drivers/scsi/qla2xxx/qla_nx2.c +++ b/drivers/scsi/qla2xxx/qla_nx2.c @@ -3491,7 +3491,7 @@ qla8044_poll_flash_status_reg(struct scsi_qla_host *vha) int retries = QLA8044_FLASH_READ_RETRY_COUNT; int ret_val = QLA_SUCCESS; - while (retries--) { + while (--retries) { ret_val = qla8044_rd_reg_indirect(vha, QLA8044_FLASH_STATUS, &flash_status); if (ret_val) {
After the loop we test for "if (!retries) " as a failure, but actually the post-op here will end with retries set to -1. I have fixed this by using a pre-op instead. Fixes: 7ec0effd30bb ('[SCSI] qla2xxx: Add support for ISP8044.') Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> -- 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