| Message ID | 56A22B49.1040702@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hello Tomas, > -----Original Message----- > From: Tomas Henzl [mailto:thenzl@redhat.com] > Sent: Friday, January 22, 2016 5:15 AM > To: Raghava Aditya Renukunta; James.Bottomley@HansenPartnership.com; > martin.petersen@oracle.com; linux-scsi@vger.kernel.org > Cc: Mahesh Rajashekhara; Murthy Bhat; Gana Sridaran; aacraid@pmc- > sierra.com; Scott Benesh; jthumshirn@suse.de; shane.seymour@hpe.com; > zzzDavid Carroll > Subject: Re: [PATCH V3 7/9] aacraid: Fix AIF triggered IOP_RESET > > On 20.1.2016 21:32, Raghava Aditya Renukunta wrote: > > > >> -----Original Message----- > >> From: Tomas Henzl [mailto:thenzl@redhat.com] > >> Sent: Tuesday, January 19, 2016 8:14 AM > >> To: Raghava Aditya Renukunta; > James.Bottomley@HansenPartnership.com; > >> martin.petersen@oracle.com; linux-scsi@vger.kernel.org > >> Cc: Mahesh Rajashekhara; Murthy Bhat; Gana Sridaran; aacraid@pmc- > >> sierra.com; Scott Benesh; jthumshirn@suse.de; > shane.seymour@hpe.com; > >> David Carroll > >> Subject: Re: [PATCH V3 7/9] aacraid: Fix AIF triggered IOP_RESET > >> > >> On 15.1.2016 08:16, Raghava Aditya Renukunta wrote: > >>> From: Raghava Aditya Renukunta > <raghavaaditya.renukunta@pmcs.com> > >>> > >>> while driver removal is in progress or PCI shutdown is invoked, driver > >>> kills AIF aacraid thread, but IOCTL requests from the management tools > >>> re-start AIF thread leading to IOP_RESET. > >>> > >>> Fixed by setting adapter_shutdown flag when PCI shutdown is invoked. > >>> > >>> Changes in V2: > >>> Set adapter_shutdown flag before shutdown command is sent to \ > >>> controller > >>> > >>> Changes in V3: > >>> Call aac_send_shut_shutdown first thing in __aac_shutdown > >>> Convert adapter_shutdown to atomic_t variable to prevent \ > >>> SMP coherency issues(race conditions) > >> Hi, > >> I don't think that an atomic variable can change it, imagine that > >> thread just passed your test in aac_cfg_ioctl and another thread > >> enter a bit later the adapter_shutdown so both - an I/O and your > shutdown > >> code > >> will run in parallel. > >> Also you need to fix your compat_ioctl path too. > >> > >> --tm > > Hello Tomas, > > Yes that would not solve this problem. > > I can think of 2 solutions > > > > 1.Place a delay after setting adapter_shutdown and before sending the > actual > > shutdown command in aac_send_shutdown. This way any impending > commands will be > > processed before the adapter actually receives the shutdown command. > Since management > > commands are sync only , shutdown command will be sent last. This > solution uses an > > estimation of the delay > > > > 2.Since commands are sync , place a check in aac_fib_send to block > > commands once adapter_shutdown is set(only shutdown command will > be sent thru) > > This option looks better but I guess you still can find a tiny race window. > What do you think about a mutual exclusive access using a mutex, do you > think this could work? > diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c > index 54195a117f..b9505c58db 100644 > --- a/drivers/scsi/aacraid/commctrl.c > +++ b/drivers/scsi/aacraid/commctrl.c > @@ -858,6 +858,8 @@ int aac_do_ioctl(struct aac_dev * dev, int cmd, void > __user *arg) > /* > * HBA gets first crack > */ > + if (dev->adapter_shutdown) > + return -EACCES; > > status = aac_dev_ioctl(dev, cmd, arg); > if (status != -ENOTTY) > diff --git a/drivers/scsi/aacraid/comminit.c b/drivers/scsi/aacraid/comminit.c > index 0e954e37f0..02535c07a4 100644 > --- a/drivers/scsi/aacraid/comminit.c > +++ b/drivers/scsi/aacraid/comminit.c > @@ -212,6 +212,10 @@ int aac_send_shutdown(struct aac_dev * dev) > return -ENOMEM; > aac_fib_init(fibctx); > > + mutex_lock(&aac_mutex); > + dev->adapter_shutdown = 1; > + mutex_unlock(&aac_mutex); > + The intention here is to have mutually exclusive access to adapter_shutdown, or aac_do_Ioctl which has a check for adapter_shutdown so either one can be command is sent at one time if I am not mistaken?. Yes this solution will work. I will make the necessary changes. Thank you. Regards, Raghava Aditya > cmd = (struct aac_close *) fib_data(fibctx); > > cmd->command = cpu_to_le32(VM_CloseAll); > @@ -229,7 +233,6 @@ int aac_send_shutdown(struct aac_dev * dev) > /* FIB should be freed only after getting the response from the F/W > */ > if (status != -ERESTARTSYS) > aac_fib_free(fibctx); > - dev->adapter_shutdown = 1; > if ((dev->pdev->device == PMC_DEVICE_S7 || > dev->pdev->device == PMC_DEVICE_S8 || > dev->pdev->device == PMC_DEVICE_S9) && > diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c > index 6944560e22..a87880ab32 100644 > --- a/drivers/scsi/aacraid/linit.c > +++ b/drivers/scsi/aacraid/linit.c > @@ -706,8 +706,9 @@ static long aac_cfg_ioctl(struct file *file, > int ret; > struct aac_dev *aac; > aac = (struct aac_dev *)file->private_data; > - if (!capable(CAP_SYS_RAWIO) || aac->adapter_shutdown) > + if (!capable(CAP_SYS_RAWIO)) > return -EPERM; > + > mutex_lock(&aac_mutex); > ret = aac_do_ioctl(file->private_data, cmd, (void __user *)arg); > mutex_unlock(&aac_mutex); > > > > > I am more inclined to go with option 2. > > > > Regards, > > Raghava Aditya > > > >>> Signed-off-by: Raghava Aditya Renukunta > >> <raghavaaditya.renukunta@pmcs.com> > >>> --- > >>> drivers/scsi/aacraid/aacraid.h | 2 +- > >>> drivers/scsi/aacraid/comminit.c | 6 +++--- > >>> drivers/scsi/aacraid/linit.c | 15 ++++++++------- > >>> 3 files changed, 12 insertions(+), 11 deletions(-) > >>> > >>> diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h > >>> index 2916288..3473668 100644 > >>> --- a/drivers/scsi/aacraid/aacraid.h > >>> +++ b/drivers/scsi/aacraid/aacraid.h > >>> @@ -1234,7 +1234,7 @@ struct aac_dev > >>> int msi_enabled; /* MSI/MSI-X enabled */ > >>> struct msix_entry msixentry[AAC_MAX_MSIX]; > >>> struct aac_msix_ctx aac_msix[AAC_MAX_MSIX]; /* context */ > >>> - u8 adapter_shutdown; > >>> + atomic_t adapter_shutdown; > >>> u32 handle_pci_error; > >>> }; > >>> > >>> diff --git a/drivers/scsi/aacraid/comminit.c > >> b/drivers/scsi/aacraid/comminit.c > >>> index 0e954e3..d361673 100644 > >>> --- a/drivers/scsi/aacraid/comminit.c > >>> +++ b/drivers/scsi/aacraid/comminit.c > >>> @@ -212,8 +212,9 @@ int aac_send_shutdown(struct aac_dev * dev) > >>> return -ENOMEM; > >>> aac_fib_init(fibctx); > >>> > >>> - cmd = (struct aac_close *) fib_data(fibctx); > >>> + atomic_set(&dev->adapter_shutdown, 1); > >>> > >>> + cmd = (struct aac_close *) fib_data(fibctx); > >>> cmd->command = cpu_to_le32(VM_CloseAll); > >>> cmd->cid = cpu_to_le32(0xfffffffe); > >>> > >>> @@ -229,7 +230,6 @@ int aac_send_shutdown(struct aac_dev * dev) > >>> /* FIB should be freed only after getting the response from the F/W > >> */ > >>> if (status != -ERESTARTSYS) > >>> aac_fib_free(fibctx); > >>> - dev->adapter_shutdown = 1; > >>> if ((dev->pdev->device == PMC_DEVICE_S7 || > >>> dev->pdev->device == PMC_DEVICE_S8 || > >>> dev->pdev->device == PMC_DEVICE_S9) && > >>> @@ -468,7 +468,7 @@ struct aac_dev *aac_init_adapter(struct aac_dev > >> *dev) > >>> } > >>> dev->max_msix = 0; > >>> dev->msi_enabled = 0; > >>> - dev->adapter_shutdown = 0; > >>> + atomic_set(&dev->adapter_shutdown, 0); > >>> if ((!aac_adapter_sync_cmd(dev, > >> GET_COMM_PREFERRED_SETTINGS, > >>> 0, 0, 0, 0, 0, 0, > >>> status+0, status+1, status+2, status+3, status+4)) > >>> diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c > >>> index 6944560..27b3fcd 100644 > >>> --- a/drivers/scsi/aacraid/linit.c > >>> +++ b/drivers/scsi/aacraid/linit.c > >>> @@ -706,7 +706,7 @@ static long aac_cfg_ioctl(struct file *file, > >>> int ret; > >>> struct aac_dev *aac; > >>> aac = (struct aac_dev *)file->private_data; > >>> - if (!capable(CAP_SYS_RAWIO) || aac->adapter_shutdown) > >>> + if (!capable(CAP_SYS_RAWIO) || atomic_read(&aac- > >>> adapter_shutdown)) > >>> return -EPERM; > >>> mutex_lock(&aac_mutex); > >>> ret = aac_do_ioctl(file->private_data, cmd, (void __user *)arg); > >>> @@ -1078,6 +1078,8 @@ static void __aac_shutdown(struct aac_dev * > aac) > >>> int i; > >>> int cpu; > >>> > >>> + aac_send_shutdown(aac); > >>> + > >>> if (aac->aif_thread) { > >>> int i; > >>> /* Clear out events first */ > >>> @@ -1089,7 +1091,7 @@ static void __aac_shutdown(struct aac_dev * > aac) > >>> } > >>> kthread_stop(aac->thread); > >>> } > >>> - aac_send_shutdown(aac); > >>> + > >>> aac_adapter_disable_int(aac); > >>> cpu = cpumask_first(cpu_online_mask); > >>> if (aac->pdev->device == PMC_DEVICE_S6 || > >>> @@ -1474,7 +1476,7 @@ static int aac_resume(struct pci_dev *pdev) > >>> * reset this flag to unblock ioctl() as it was set at > >>> * aac_send_shutdown() to block ioctls from upperlayer > >>> */ > >>> - aac->adapter_shutdown = 0; > >>> + atomic_set(&aac->adapter_shutdown, 0); > >>> scsi_unblock_requests(shost); > >>> > >>> return 0; > >>> @@ -1553,9 +1555,8 @@ static pci_ers_result_t > >> aac_pci_error_detected(struct pci_dev *pdev, > >>> case pci_channel_io_normal: > >>> return PCI_ERS_RESULT_CAN_RECOVER; > >>> case pci_channel_io_frozen: > >>> - > >>> aac->handle_pci_error = 1; > >>> - aac->adapter_shutdown = 1; > >>> + atomic_set(&aac->adapter_shutdown, 1); > >>> > >>> scsi_block_requests(aac->scsi_host_ptr); > >>> aac_flush_ios(aac); > >>> @@ -1567,7 +1568,7 @@ static pci_ers_result_t > >> aac_pci_error_detected(struct pci_dev *pdev, > >>> return PCI_ERS_RESULT_NEED_RESET; > >>> case pci_channel_io_perm_failure: > >>> aac->handle_pci_error = 1; > >>> - aac->adapter_shutdown = 1; > >>> + atomic_set(&aac->adapter_shutdown, 1); > >>> > >>> aac_flush_ios(aac); > >>> return PCI_ERS_RESULT_DISCONNECT; > >>> @@ -1636,7 +1637,7 @@ static void aac_pci_resume(struct pci_dev > *pdev) > >>> * reset this flag to unblock ioctl() as it was set > >>> * at aac_send_shutdown() to block ioctls from upperlayer > >>> */ > >>> - aac->adapter_shutdown = 0; > >>> + atomic_set(&aac->adapter_shutdown, 0); > >>> aac->handle_pci_error = 0; > >>> > >>> shost_for_each_device(sdev, shost) > > -- > > 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
>>>>> "Tomas" == Tomas Henzl <thenzl@redhat.com> writes: >> 2.Since commands are sync , place a check in aac_fib_send to block >> commands once adapter_shutdown is set(only shutdown command will be >> sent thru) Tomas> This option looks better but I guess you still can find a tiny Tomas> race window. What do you think about a mutual exclusive access Tomas> using a mutex, do you think this could work? [...] Raghava?
Hello Martin, > -----Original Message----- > From: Martin K. Petersen [mailto:martin.petersen@oracle.com] > Sent: Tuesday, January 26, 2016 6:31 PM > To: Raghava Aditya Renukunta > Cc: Tomas Henzl; James.Bottomley@HansenPartnership.com; > martin.petersen@oracle.com; linux-scsi@vger.kernel.org; Mahesh > Rajashekhara; Murthy Bhat; Gana Sridaran; aacraid@pmc-sierra.com; Scott > Benesh; jthumshirn@suse.de; shane.seymour@hpe.com; zzzDavid Carroll > Subject: Re: [PATCH V3 7/9] aacraid: Fix AIF triggered IOP_RESET > > >>>>> "Tomas" == Tomas Henzl <thenzl@redhat.com> writes: > > >> 2.Since commands are sync , place a check in aac_fib_send to block > >> commands once adapter_shutdown is set(only shutdown command will > be > >> sent thru) > > Tomas> This option looks better but I guess you still can find a tiny > Tomas> race window. What do you think about a mutual exclusive access > Tomas> using a mutex, do you think this could work? > > [...] > > Raghava? I have replied that I will create a new set patches with this change shortly. Please expect it by tomorrow at the latest. Regards, Raghava Aditya > > -- > Martin K. Petersen Oracle Linux Engineering -- 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
diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c index 54195a117f..b9505c58db 100644 --- a/drivers/scsi/aacraid/commctrl.c +++ b/drivers/scsi/aacraid/commctrl.c @@ -858,6 +858,8 @@ int aac_do_ioctl(struct aac_dev * dev, int cmd, void __user *arg) /* * HBA gets first crack */ + if (dev->adapter_shutdown) + return -EACCES; status = aac_dev_ioctl(dev, cmd, arg); if (status != -ENOTTY) diff --git a/drivers/scsi/aacraid/comminit.c b/drivers/scsi/aacraid/comminit.c index 0e954e37f0..02535c07a4 100644 --- a/drivers/scsi/aacraid/comminit.c +++ b/drivers/scsi/aacraid/comminit.c @@ -212,6 +212,10 @@ int aac_send_shutdown(struct aac_dev * dev) return -ENOMEM; aac_fib_init(fibctx); + mutex_lock(&aac_mutex); + dev->adapter_shutdown = 1; + mutex_unlock(&aac_mutex); + cmd = (struct aac_close *) fib_data(fibctx); cmd->command = cpu_to_le32(VM_CloseAll); @@ -229,7 +233,6 @@ int aac_send_shutdown(struct aac_dev * dev) /* FIB should be freed only after getting the response from the F/W */ if (status != -ERESTARTSYS) aac_fib_free(fibctx); - dev->adapter_shutdown = 1; if ((dev->pdev->device == PMC_DEVICE_S7 || dev->pdev->device == PMC_DEVICE_S8 || dev->pdev->device == PMC_DEVICE_S9) && diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c index 6944560e22..a87880ab32 100644 --- a/drivers/scsi/aacraid/linit.c +++ b/drivers/scsi/aacraid/linit.c @@ -706,8 +706,9 @@ static long aac_cfg_ioctl(struct file *file, int ret; struct aac_dev *aac; aac = (struct aac_dev *)file->private_data; - if (!capable(CAP_SYS_RAWIO) || aac->adapter_shutdown) + if (!capable(CAP_SYS_RAWIO)) return -EPERM; + mutex_lock(&aac_mutex); ret = aac_do_ioctl(file->private_data, cmd, (void __user *)arg); mutex_unlock(&aac_mutex);