| Message ID | 20220211073704.963993-22-damien.lemoal@opensource.wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libsas and pm8001 fixes | expand |
On 11/02/2022 07:37, Damien Le Moal wrote: Hi Damien, > The n_elem local variable in pm8001_task_exec() is initialized to 0 and > changed set to the number of DMA scatter elements for a needed for a > task command only for ATA commands and for SAS commands that have a > non-zero number of sg segments. n_elem is never initialized to 0 for SAS Do you mean re-initialized? I thought the current code was ok, as we init n_elem = 0 and we only ever loop once. Am I missing something? Thanks, john > commands that do not no sg segments, potentially leading to an incorrect > value of n_elem being used for a task. Add the missing 0 initialization. > > Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> > --- > drivers/scsi/pm8001/pm8001_sas.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c > index 7b749da82a61..8cd7e7837f41 100644 > --- a/drivers/scsi/pm8001/pm8001_sas.c > +++ b/drivers/scsi/pm8001/pm8001_sas.c > @@ -383,7 +383,7 @@ static int pm8001_task_exec(struct sas_task *task, > struct sas_task *t = task; > struct task_status_struct *ts = &t->task_status; > struct pm8001_ccb_info *ccb; > - u32 tag = 0xdeadbeef, rc = 0, n_elem = 0; > + u32 tag = 0xdeadbeef, rc = 0, n_elem; > unsigned long flags = 0; > enum sas_protocol task_proto = t->task_proto; > > @@ -440,6 +440,8 @@ static int pm8001_task_exec(struct sas_task *task, > rc = -ENOMEM; > goto err_out_tag; > } > + } else { > + n_elem = 0; > } > } else { > n_elem = t->num_scatter;
On 2/11/22 21:51, John Garry wrote: > On 11/02/2022 07:37, Damien Le Moal wrote: > > Hi Damien, > >> The n_elem local variable in pm8001_task_exec() is initialized to 0 and >> changed set to the number of DMA scatter elements for a needed for a >> task command only for ATA commands and for SAS commands that have a >> non-zero number of sg segments. n_elem is never initialized to 0 for SAS > > Do you mean re-initialized? > > I thought the current code was ok, as we init n_elem = 0 and we only > ever loop once. Am I missing something? It was not clear to me because of the loop. If the loop is done only once, why the loop in the first place ? Hold on... Oh ! It is a while(0)... OK, this too ugly to live. We need to do something about this. The continue at the beginning of the loop seems totally crazy as it may lead to the same task being reused, so multiple ->task_done() calls for the same task. Is that sane ?
On 11/02/2022 12:57, Damien Le Moal wrote: >> I thought the current code was ok, as we init n_elem = 0 and we only >> ever loop once. Am I missing something? > It was not clear to me because of the loop. If the loop is done only > once, why the loop in the first place ? > > Hold on... > > Oh ! It is a while(0)... OK, this too ugly to live. I didn't see the point of the while (0) loop at all. We need to do > something about this. The continue at the beginning of the loop seems > totally crazy as it may lead to the same task being reused, so multiple > ->task_done() calls for the same task. Is that sane ? No. And I think continue in while(0) has has effect as break - it falls out. Thanks, John
On 2/11/22 22:05, John Garry wrote: > On 11/02/2022 12:57, Damien Le Moal wrote: >>> I thought the current code was ok, as we init n_elem = 0 and we only >>> ever loop once. Am I missing something? >> It was not clear to me because of the loop. If the loop is done only >> once, why the loop in the first place ? >> >> Hold on... >> >> Oh ! It is a while(0)... OK, this too ugly to live. > > I didn't see the point of the while (0) loop at all. > > We need to do >> something about this. The continue at the beginning of the loop seems >> totally crazy as it may lead to the same task being reused, so multiple >> ->task_done() calls for the same task. Is that sane ? > > No. > > And I think continue in while(0) has has effect as break - it falls out. OK. So the loop is really useless, and confusing. Will clean that up to make the code cleaner and easier to understand. > > Thanks, > John > >
diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c index 7b749da82a61..8cd7e7837f41 100644 --- a/drivers/scsi/pm8001/pm8001_sas.c +++ b/drivers/scsi/pm8001/pm8001_sas.c @@ -383,7 +383,7 @@ static int pm8001_task_exec(struct sas_task *task, struct sas_task *t = task; struct task_status_struct *ts = &t->task_status; struct pm8001_ccb_info *ccb; - u32 tag = 0xdeadbeef, rc = 0, n_elem = 0; + u32 tag = 0xdeadbeef, rc = 0, n_elem; unsigned long flags = 0; enum sas_protocol task_proto = t->task_proto; @@ -440,6 +440,8 @@ static int pm8001_task_exec(struct sas_task *task, rc = -ENOMEM; goto err_out_tag; } + } else { + n_elem = 0; } } else { n_elem = t->num_scatter;
The n_elem local variable in pm8001_task_exec() is initialized to 0 and changed set to the number of DMA scatter elements for a needed for a task command only for ATA commands and for SAS commands that have a non-zero number of sg segments. n_elem is never initialized to 0 for SAS commands that do not no sg segments, potentially leading to an incorrect value of n_elem being used for a task. Add the missing 0 initialization. Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> --- drivers/scsi/pm8001/pm8001_sas.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)