RE: [PATCH V3 7/9] aacraid: Fix AIF triggered IOP_RESET
Hello Martin, > -Original Message- > From: Martin K. Petersen [mailto:martin.peter...@oracle.com] > Sent: Tuesday, January 26, 2016 6:31 PM > To: Raghava Aditya Renukunta > Cc: Tomas Henzl; james.bottom...@hansenpartnership.com; > martin.peter...@oracle.com; linux-scsi@vger.kernel.org; Mahesh > Rajashekhara; Murthy Bhat; Gana Sridaran; aacr...@pmc-sierra.com; Scott > Benesh; jthumsh...@suse.de; shane.seym...@hpe.com; zzzDavid Carroll > Subject: Re: [PATCH V3 7/9] aacraid: Fix AIF triggered IOP_RESET > > >>>>> "Tomas" == Tomas Henzl <the...@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. PetersenOracle Linux Engineering -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 7/9] aacraid: Fix AIF triggered IOP_RESET
> "Tomas" == Tomas Henzlwrites: >> 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? -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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:the...@redhat.com] >> Sent: Tuesday, January 19, 2016 8:14 AM >> To: Raghava Aditya Renukunta; james.bottom...@hansenpartnership.com; >> martin.peter...@oracle.com; linux-scsi@vger.kernel.org >> Cc: Mahesh Rajashekhara; Murthy Bhat; Gana Sridaran; aacraid@pmc- >> sierra.com; Scott Benesh; jthumsh...@suse.de; shane.seym...@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.renuku...@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(_mutex); + dev->adapter_shutdown = 1; + mutex_unlock(_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(_mutex); ret = aac_do_ioctl(file->private_data, cmd, (void __user *)arg); mutex_unlock(_mutex); > > I am more inclined to go with option 2. > > Regards, > Raghava Aditya > >>> Signed-off-by: Raghava Aditya Renukunta >> <raghavaaditya.renuku...@pmcs.com> >>> --- >>> drivers/scsi/aacraid/aacraid.h
RE: [PATCH V3 7/9] aacraid: Fix AIF triggered IOP_RESET
Hello Tomas, > -Original Message- > From: Tomas Henzl [mailto:the...@redhat.com] > Sent: Friday, January 22, 2016 5:15 AM > To: Raghava Aditya Renukunta; james.bottom...@hansenpartnership.com; > martin.peter...@oracle.com; linux-scsi@vger.kernel.org > Cc: Mahesh Rajashekhara; Murthy Bhat; Gana Sridaran; aacraid@pmc- > sierra.com; Scott Benesh; jthumsh...@suse.de; shane.seym...@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:the...@redhat.com] > >> Sent: Tuesday, January 19, 2016 8:14 AM > >> To: Raghava Aditya Renukunta; > james.bottom...@hansenpartnership.com; > >> martin.peter...@oracle.com; linux-scsi@vger.kernel.org > >> Cc: Mahesh Rajashekhara; Murthy Bhat; Gana Sridaran; aacraid@pmc- > >> sierra.com; Scott Benesh; jthumsh...@suse.de; > shane.seym...@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.renuku...@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(_mutex); > + dev->adapter_shutdown = 1; > + mutex_unlock(_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) > /* F
RE: [PATCH V3 7/9] aacraid: Fix AIF triggered IOP_RESET
> -Original Message- > From: Tomas Henzl [mailto:the...@redhat.com] > Sent: Tuesday, January 19, 2016 8:14 AM > To: Raghava Aditya Renukunta; james.bottom...@hansenpartnership.com; > martin.peter...@oracle.com; linux-scsi@vger.kernel.org > Cc: Mahesh Rajashekhara; Murthy Bhat; Gana Sridaran; aacraid@pmc- > sierra.com; Scott Benesh; jthumsh...@suse.de; shane.seym...@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.renuku...@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) I am more inclined to go with option 2. Regards, Raghava Aditya > > > > Signed-off-by: Raghava Aditya Renukunta > <raghavaaditya.renuku...@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_tadapter_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(>adapter_shutdown, 1); > > > > + cmd = (struct aac_close *) fib_data(fibctx); > > cmd->command = cpu_to_le32(VM_CloseAll); > > cmd->cid = cpu_to_le32(0xfffe); > > > > @@ -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(>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/