| Message ID | 20171221173420.8213-9-RaghavaAditya.Renukunta@microsemi.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, 2017-12-21 at 09:33 -0800, Raghava Aditya Renukunta wrote: > +static void aac_wait_for_io_completion(struct aac_dev *aac) > +{ > + unsigned long flagv = 0; > + int i = 0; > + > + for (i = 60; i; --i) { > + struct scsi_device *dev; > + struct scsi_cmnd *command; > + int active = 0; > + > + __shost_for_each_device(dev, aac->scsi_host_ptr) { > + spin_lock_irqsave(&dev->list_lock, flagv); > + list_for_each_entry(command, &dev->cmd_list, list) { > + if (command->SCp.phase == AAC_OWNER_FIRMWARE) { > + active++; > + break; > + } > + } > + spin_unlock_irqrestore(&dev->list_lock, flagv); > + if (active) > + break; > + > + } > + /* > + * We can exit If all the commands are complete > + */ > + if (active == 0) > + break; > + ssleep(1); > + } > +} Have you considered to call scsi_target_block() and scsi_target_unblock() instead of implementing functionality like the above in a SCSI LLD? Thanks, Bart.
On Thu, 2017-12-21 at 17:59 +0000, Bart Van Assche wrote: > On Thu, 2017-12-21 at 09:33 -0800, Raghava Aditya Renukunta wrote: > > +static void aac_wait_for_io_completion(struct aac_dev *aac) > > +{ > > + unsigned long flagv = 0; > > + int i = 0; > > + > > + for (i = 60; i; --i) { > > + struct scsi_device *dev; > > + struct scsi_cmnd *command; > > + int active = 0; > > + > > + __shost_for_each_device(dev, aac->scsi_host_ptr) { > > + spin_lock_irqsave(&dev->list_lock, flagv); > > + list_for_each_entry(command, &dev->cmd_list, list) { > > + if (command->SCp.phase == AAC_OWNER_FIRMWARE) { > > + active++; > > + break; > > + } > > + } > > + spin_unlock_irqrestore(&dev->list_lock, flagv); > > + if (active) > > + break; > > + > > + } > > + /* > > + * We can exit If all the commands are complete > > + */ > > + if (active == 0) > > + break; > > + ssleep(1); > > + } > > +} > > Have you considered to call scsi_target_block() and scsi_target_unblock() instead > of implementing functionality like the above in a SCSI LLD? (replying to my own e-mail) It seems like I misread your code - calling scsi_target_block() and scsi_target_unblock() would not be sufficient. But calling blk_mq_freeze_queue() and blk_mq_unfreeze_queue() should be sufficient. The following commit made these functions work not only for scsi-mq but also for legacy scsi queues: commit 055f6e18e08f ("block: Make q_usage_counter also track legacy requests"). Bart.
> -----Original Message----- > From: Bart Van Assche [mailto:Bart.VanAssche@wdc.com] > Sent: Friday, December 22, 2017 8:27 AM > To: jejb@linux.vnet.ibm.com; Raghava Aditya Renukunta > <RaghavaAditya.Renukunta@microsemi.com>; linux-scsi@vger.kernel.org; > martin.petersen@oracle.com > Cc: dl-esc-Aacraid Linux Driver <aacraid@microsemi.com>; > gpiccoli@linux.vnet.ibm.com; Tom White <tom.white@microsemi.com>; > Scott Benesh <scott.benesh@microsemi.com> > Subject: Re: [PATCH 08/29] scsi: aacraid: Move code to wait for IO completion > to shutdown func > > EXTERNAL EMAIL > > > On Thu, 2017-12-21 at 17:59 +0000, Bart Van Assche wrote: > > On Thu, 2017-12-21 at 09:33 -0800, Raghava Aditya Renukunta wrote: > > > +static void aac_wait_for_io_completion(struct aac_dev *aac) > > > +{ > > > + unsigned long flagv = 0; > > > + int i = 0; > > > + > > > + for (i = 60; i; --i) { > > > + struct scsi_device *dev; > > > + struct scsi_cmnd *command; > > > + int active = 0; > > > + > > > + __shost_for_each_device(dev, aac->scsi_host_ptr) { > > > + spin_lock_irqsave(&dev->list_lock, flagv); > > > + list_for_each_entry(command, &dev->cmd_list, list) { > > > + if (command->SCp.phase == AAC_OWNER_FIRMWARE) { > > > + active++; > > > + break; > > > + } > > > + } > > > + spin_unlock_irqrestore(&dev->list_lock, flagv); > > > + if (active) > > > + break; > > > + > > > + } > > > + /* > > > + * We can exit If all the commands are complete > > > + */ > > > + if (active == 0) > > > + break; > > > + ssleep(1); > > > + } > > > +} > > > > Have you considered to call scsi_target_block() and scsi_target_unblock() > instead > > of implementing functionality like the above in a SCSI LLD? > > (replying to my own e-mail) > > It seems like I misread your code - calling scsi_target_block() and > scsi_target_unblock() would not be sufficient. But calling > blk_mq_freeze_queue() > and blk_mq_unfreeze_queue() should be sufficient. The following commit > made these > functions work not only for scsi-mq but also for legacy scsi queues: commit > 055f6e18e08f ("block: Make q_usage_counter also track legacy requests"). Hi Bart, That piece of code is very much legacy that I removed and placed in a function that would benefit all code paths that shutdown the controller. The 2 functions you mentioned are a god send but I think I will hold off on this patchset and do a full refactor with them in the next patchset. ( I suspect that I will have to touch a bunch of different code paths and perform extensive testing to be fully confident) Regards, Raghava Aditya > Bart.
diff --git a/drivers/scsi/aacraid/comminit.c b/drivers/scsi/aacraid/comminit.c index 9eff246..0dc7b5a 100644 --- a/drivers/scsi/aacraid/comminit.c +++ b/drivers/scsi/aacraid/comminit.c @@ -42,6 +42,8 @@ #include <linux/completion.h> #include <linux/mm.h> #include <scsi/scsi_host.h> +#include <scsi/scsi_device.h> +#include <scsi/scsi_cmnd.h> #include "aacraid.h" @@ -284,6 +286,38 @@ static void aac_queue_init(struct aac_dev * dev, struct aac_queue * q, u32 *mem, q->entries = qsize; } +static void aac_wait_for_io_completion(struct aac_dev *aac) +{ + unsigned long flagv = 0; + int i = 0; + + for (i = 60; i; --i) { + struct scsi_device *dev; + struct scsi_cmnd *command; + int active = 0; + + __shost_for_each_device(dev, aac->scsi_host_ptr) { + spin_lock_irqsave(&dev->list_lock, flagv); + list_for_each_entry(command, &dev->cmd_list, list) { + if (command->SCp.phase == AAC_OWNER_FIRMWARE) { + active++; + break; + } + } + spin_unlock_irqrestore(&dev->list_lock, flagv); + if (active) + break; + + } + /* + * We can exit If all the commands are complete + */ + if (active == 0) + break; + ssleep(1); + } +} + /** * aac_send_shutdown - shutdown an adapter * @dev: Adapter to shutdown @@ -306,6 +340,8 @@ int aac_send_shutdown(struct aac_dev * dev) mutex_unlock(&dev->ioctl_mutex); } + aac_wait_for_io_completion(dev); + fibctx = aac_fib_alloc(dev); if (!fibctx) return -ENOMEM; diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c index 32b8bdb..9840bd3 100644 --- a/drivers/scsi/aacraid/commsup.c +++ b/drivers/scsi/aacraid/commsup.c @@ -1701,31 +1701,6 @@ int aac_reset_adapter(struct aac_dev *aac, int forced, u8 reset_type) */ host = aac->scsi_host_ptr; scsi_block_requests(host); - if (forced < 2) for (retval = 60; retval; --retval) { - struct scsi_device * dev; - struct scsi_cmnd * command; - int active = 0; - - __shost_for_each_device(dev, host) { - spin_lock_irqsave(&dev->list_lock, flagv); - list_for_each_entry(command, &dev->cmd_list, list) { - if (command->SCp.phase == AAC_OWNER_FIRMWARE) { - active++; - break; - } - } - spin_unlock_irqrestore(&dev->list_lock, flagv); - if (active) - break; - - } - /* - * We can exit If all the commands are complete - */ - if (active == 0) - break; - ssleep(1); - } /* Quiesce build, flush cache, write through mode */ if (forced < 2)
Ideally driver needs to wait for IO to be submitted or responded to before shutdown. Move code to wait for IO completion into shutdown path Signed-off-by: Raghava Aditya Renukunta <RaghavaAditya.Renukunta@microsemi.com> --- drivers/scsi/aacraid/comminit.c | 36 ++++++++++++++++++++++++++++++++++++ drivers/scsi/aacraid/commsup.c | 25 ------------------------- 2 files changed, 36 insertions(+), 25 deletions(-)