| Message ID | 20220208172514.3481-2-bvanassche@acm.org |
|---|---|
| State | New |
| Headers | show |
| Series | Remove the SCSI pointer from struct scsi_cmnd | expand |
On 08/02/2022 17:24, Bart Van Assche wrote: > This patch prepares for removal of the drivers/scsi/scsi.h header file. > That header file defines the 'TRUE' and 'FALSE' constants. > > Reviewed-by: Johannes Thumshirn<johannes.thumshirn@wdc.com> > Signed-off-by: Bart Van Assche<bvanassche@acm.org> Regardless of comment, below, feel free to add: Reviewed-by: John Garry <john.garry@huawei.com> > --- > drivers/scsi/ips.c | 42 ++++++++++++++++++++---------------------- > 1 file changed, 20 insertions(+), 22 deletions(-) > > diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c > index 498bf04499ce..b3532d290848 100644 > --- a/drivers/scsi/ips.c > +++ b/drivers/scsi/ips.c > @@ -655,13 +655,13 @@ ips_release(struct Scsi_Host *sh) This function and other places return an int, and not a bool, so that could be changed as well. Prob not worth the churn, though. > printk(KERN_WARNING > "(%s) release, invalid Scsi_Host pointer.\n", ips_name); > BUG(); > - return (FALSE); > + return false; > } > > ha = IPS_HA(sh); > > if (!ha) > - return (FALSE); > + return false; > > /* flush the cache on the controller */ > scb = &ha->scbs[ha->max_cmds - 1]; > @@ -700,7 +700,7 @@ ips_release(struct Scsi_Host *sh) > > ips_released_controllers++; > > - return (FALSE); > + return false;
> On Feb 8, 2022, at 9:24 AM, Bart Van Assche <bvanassche@acm.org> wrote: > > This patch prepares for removal of the drivers/scsi/scsi.h header file. > That header file defines the 'TRUE' and 'FALSE' constants. > > Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/scsi/ips.c | 42 ++++++++++++++++++++---------------------- > 1 file changed, 20 insertions(+), 22 deletions(-) > > diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c > index 498bf04499ce..b3532d290848 100644 > --- a/drivers/scsi/ips.c > +++ b/drivers/scsi/ips.c > @@ -655,13 +655,13 @@ ips_release(struct Scsi_Host *sh) > printk(KERN_WARNING > "(%s) release, invalid Scsi_Host pointer.\n", ips_name); > BUG(); > - return (FALSE); > + return false; > } > > ha = IPS_HA(sh); > > if (!ha) > - return (FALSE); > + return false; > > /* flush the cache on the controller */ > scb = &ha->scbs[ha->max_cmds - 1]; > @@ -700,7 +700,7 @@ ips_release(struct Scsi_Host *sh) > > ips_released_controllers++; > > - return (FALSE); > + return false; > } > > /****************************************************************************/ > @@ -949,7 +949,7 @@ static int __ips_eh_reset(struct scsi_cmnd *SC) > scsi_done(scsi_cmd); > } > > - ha->active = FALSE; > + ha->active = false; > return (FAILED); > } > > @@ -978,7 +978,7 @@ static int __ips_eh_reset(struct scsi_cmnd *SC) > scsi_done(scsi_cmd); > } > > - ha->active = FALSE; > + ha->active = false; > return (FAILED); > } > > @@ -1291,7 +1291,7 @@ ips_intr_copperhead(ips_ha_t * ha) > return 0; > } > > - while (TRUE) { > + while (true) { > sp = &ha->sp; > > intrstatus = (*ha->func.isintr) (ha); > @@ -1355,7 +1355,7 @@ ips_intr_morpheus(ips_ha_t * ha) > return 0; > } > > - while (TRUE) { > + while (true) { > sp = &ha->sp; > > intrstatus = (*ha->func.isintr) (ha); > @@ -3090,8 +3090,8 @@ ipsintr_blocking(ips_ha_t * ha, ips_scb_t * scb) > METHOD_TRACE("ipsintr_blocking", 2); > > ips_freescb(ha, scb); > - if ((ha->waitflag == TRUE) && (ha->cmd_in_progress == scb->cdb[0])) { > - ha->waitflag = FALSE; > + if (ha->waitflag && ha->cmd_in_progress == scb->cdb[0]) { > + ha->waitflag = false; > > return; > } > @@ -3391,7 +3391,7 @@ ips_send_wait(ips_ha_t * ha, ips_scb_t * scb, int timeout, int intr) > METHOD_TRACE("ips_send_wait", 1); > > if (intr != IPS_FFDC) { /* Won't be Waiting if this is a Time Stamp */ > - ha->waitflag = TRUE; > + ha->waitflag = true; > ha->cmd_in_progress = scb->cdb[0]; > } > scb->callback = ipsintr_blocking; > @@ -3468,10 +3468,8 @@ ips_send_cmd(ips_ha_t * ha, ips_scb_t * scb) > if (scb->bus > 0) { > /* Controller commands can't be issued */ > /* to real devices -- fail them */ > - if ((ha->waitflag == TRUE) && > - (ha->cmd_in_progress == scb->cdb[0])) { > - ha->waitflag = FALSE; > - } > + if (ha->waitflag && ha->cmd_in_progress == scb->cdb[0]) > + ha->waitflag = false; > > return (1); > } > @@ -4619,7 +4617,7 @@ ips_poll_for_flush_complete(ips_ha_t * ha) > { > IPS_STATUS cstatus; > > - while (TRUE) { > + while (true) { > cstatus.value = (*ha->func.statupd) (ha); > > if (cstatus.value == 0xffffffff) /* If No Interrupt to process */ > @@ -5542,26 +5540,26 @@ ips_wait(ips_ha_t * ha, int time, int intr) > METHOD_TRACE("ips_wait", 1); > > ret = IPS_FAILURE; > - done = FALSE; > + done = false; > > time *= IPS_ONE_SEC; /* convert seconds */ > > while ((time > 0) && (!done)) { > if (intr == IPS_INTR_ON) { > - if (ha->waitflag == FALSE) { > + if (!ha->waitflag) { > ret = IPS_SUCCESS; > - done = TRUE; > + done = true; > break; > } > } else if (intr == IPS_INTR_IORL) { > - if (ha->waitflag == FALSE) { > + if (!ha->waitflag) { > /* > * controller generated an interrupt to > * acknowledge completion of the command > * and ips_intr() has serviced the interrupt. > */ > ret = IPS_SUCCESS; > - done = TRUE; > + done = true; > break; > } > > @@ -5596,7 +5594,7 @@ ips_write_driver_status(ips_ha_t * ha, int intr) > { > METHOD_TRACE("ips_write_driver_status", 1); > > - if (!ips_readwrite_page5(ha, FALSE, intr)) { > + if (!ips_readwrite_page5(ha, false, intr)) { > IPS_PRINTK(KERN_WARNING, ha->pcidev, > "unable to read NVRAM page 5.\n"); > > @@ -5634,7 +5632,7 @@ ips_write_driver_status(ips_ha_t * ha, int intr) > ha->nvram->versioning = 0; /* Indicate the Driver Does Not Support Versioning */ > > /* now update the page */ > - if (!ips_readwrite_page5(ha, TRUE, intr)) { > + if (!ips_readwrite_page5(ha, true, intr)) { > IPS_PRINTK(KERN_WARNING, ha->pcidev, > "unable to write NVRAM page 5.\n"); > Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com> -- Himanshu Madhani Oracle Linux Engineering
On 2/8/22 10:04, John Garry wrote: > On 08/02/2022 17:24, Bart Van Assche wrote: >> diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c >> index 498bf04499ce..b3532d290848 100644 >> --- a/drivers/scsi/ips.c >> +++ b/drivers/scsi/ips.c >> @@ -655,13 +655,13 @@ ips_release(struct Scsi_Host *sh) > > This function and other places return an int, and not a bool, so that > could be changed as well. Prob not worth the churn, though. Because of this comment I took a closer look at the ips_release() function. It seems to me that that function only has one caller and that caller ignores the value returned by ips_release(). So how about changing the return type into 'void'? Thanks, Bart.
On 2/8/22 18:24, Bart Van Assche wrote: > This patch prepares for removal of the drivers/scsi/scsi.h header file. > That header file defines the 'TRUE' and 'FALSE' constants. > > Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/scsi/ips.c | 42 ++++++++++++++++++++---------------------- > 1 file changed, 20 insertions(+), 22 deletions(-) > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On 09/02/2022 00:04, Bart Van Assche wrote: >>> diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c >>> index 498bf04499ce..b3532d290848 100644 >>> --- a/drivers/scsi/ips.c >>> +++ b/drivers/scsi/ips.c >>> @@ -655,13 +655,13 @@ ips_release(struct Scsi_Host *sh) >> >> This function and other places return an int, and not a bool, so that >> could be changed as well. Prob not worth the churn, though. > > Because of this comment I took a closer look at the ips_release() > function. It seems to me that that function only has one caller and that > caller ignores the value returned by ips_release(). So how about > changing the return type into 'void'? You could do that. But then there are other places where FALSE is checked against at uint8_t - see ips_ha.waitflag, so by the same reason could be changed. However, as I said, changes like this are prob not worth it... thanks, John
diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c index 498bf04499ce..b3532d290848 100644 --- a/drivers/scsi/ips.c +++ b/drivers/scsi/ips.c @@ -655,13 +655,13 @@ ips_release(struct Scsi_Host *sh) printk(KERN_WARNING "(%s) release, invalid Scsi_Host pointer.\n", ips_name); BUG(); - return (FALSE); + return false; } ha = IPS_HA(sh); if (!ha) - return (FALSE); + return false; /* flush the cache on the controller */ scb = &ha->scbs[ha->max_cmds - 1]; @@ -700,7 +700,7 @@ ips_release(struct Scsi_Host *sh) ips_released_controllers++; - return (FALSE); + return false; } /****************************************************************************/ @@ -949,7 +949,7 @@ static int __ips_eh_reset(struct scsi_cmnd *SC) scsi_done(scsi_cmd); } - ha->active = FALSE; + ha->active = false; return (FAILED); } @@ -978,7 +978,7 @@ static int __ips_eh_reset(struct scsi_cmnd *SC) scsi_done(scsi_cmd); } - ha->active = FALSE; + ha->active = false; return (FAILED); } @@ -1291,7 +1291,7 @@ ips_intr_copperhead(ips_ha_t * ha) return 0; } - while (TRUE) { + while (true) { sp = &ha->sp; intrstatus = (*ha->func.isintr) (ha); @@ -1355,7 +1355,7 @@ ips_intr_morpheus(ips_ha_t * ha) return 0; } - while (TRUE) { + while (true) { sp = &ha->sp; intrstatus = (*ha->func.isintr) (ha); @@ -3090,8 +3090,8 @@ ipsintr_blocking(ips_ha_t * ha, ips_scb_t * scb) METHOD_TRACE("ipsintr_blocking", 2); ips_freescb(ha, scb); - if ((ha->waitflag == TRUE) && (ha->cmd_in_progress == scb->cdb[0])) { - ha->waitflag = FALSE; + if (ha->waitflag && ha->cmd_in_progress == scb->cdb[0]) { + ha->waitflag = false; return; } @@ -3391,7 +3391,7 @@ ips_send_wait(ips_ha_t * ha, ips_scb_t * scb, int timeout, int intr) METHOD_TRACE("ips_send_wait", 1); if (intr != IPS_FFDC) { /* Won't be Waiting if this is a Time Stamp */ - ha->waitflag = TRUE; + ha->waitflag = true; ha->cmd_in_progress = scb->cdb[0]; } scb->callback = ipsintr_blocking; @@ -3468,10 +3468,8 @@ ips_send_cmd(ips_ha_t * ha, ips_scb_t * scb) if (scb->bus > 0) { /* Controller commands can't be issued */ /* to real devices -- fail them */ - if ((ha->waitflag == TRUE) && - (ha->cmd_in_progress == scb->cdb[0])) { - ha->waitflag = FALSE; - } + if (ha->waitflag && ha->cmd_in_progress == scb->cdb[0]) + ha->waitflag = false; return (1); } @@ -4619,7 +4617,7 @@ ips_poll_for_flush_complete(ips_ha_t * ha) { IPS_STATUS cstatus; - while (TRUE) { + while (true) { cstatus.value = (*ha->func.statupd) (ha); if (cstatus.value == 0xffffffff) /* If No Interrupt to process */ @@ -5542,26 +5540,26 @@ ips_wait(ips_ha_t * ha, int time, int intr) METHOD_TRACE("ips_wait", 1); ret = IPS_FAILURE; - done = FALSE; + done = false; time *= IPS_ONE_SEC; /* convert seconds */ while ((time > 0) && (!done)) { if (intr == IPS_INTR_ON) { - if (ha->waitflag == FALSE) { + if (!ha->waitflag) { ret = IPS_SUCCESS; - done = TRUE; + done = true; break; } } else if (intr == IPS_INTR_IORL) { - if (ha->waitflag == FALSE) { + if (!ha->waitflag) { /* * controller generated an interrupt to * acknowledge completion of the command * and ips_intr() has serviced the interrupt. */ ret = IPS_SUCCESS; - done = TRUE; + done = true; break; } @@ -5596,7 +5594,7 @@ ips_write_driver_status(ips_ha_t * ha, int intr) { METHOD_TRACE("ips_write_driver_status", 1); - if (!ips_readwrite_page5(ha, FALSE, intr)) { + if (!ips_readwrite_page5(ha, false, intr)) { IPS_PRINTK(KERN_WARNING, ha->pcidev, "unable to read NVRAM page 5.\n"); @@ -5634,7 +5632,7 @@ ips_write_driver_status(ips_ha_t * ha, int intr) ha->nvram->versioning = 0; /* Indicate the Driver Does Not Support Versioning */ /* now update the page */ - if (!ips_readwrite_page5(ha, TRUE, intr)) { + if (!ips_readwrite_page5(ha, true, intr)) { IPS_PRINTK(KERN_WARNING, ha->pcidev, "unable to write NVRAM page 5.\n");