From patchwork Fri Jan 22 13:14:49 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tomas Henzl X-Patchwork-Id: 8089991 Return-Path: X-Original-To: patchwork-linux-scsi@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id CF3FDBEEE5 for ; Fri, 22 Jan 2016 13:14:58 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 8F5BA2038E for ; Fri, 22 Jan 2016 13:14:57 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id E3B5320320 for ; Fri, 22 Jan 2016 13:14:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753810AbcAVNOz (ORCPT ); Fri, 22 Jan 2016 08:14:55 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59993 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753405AbcAVNOy (ORCPT ); Fri, 22 Jan 2016 08:14:54 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 309DA8F87D; Fri, 22 Jan 2016 13:14:54 +0000 (UTC) Received: from dhcp-26-204.brq.redhat.com (dhcp-26-204.brq.redhat.com [10.34.26.204]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u0MDEoSE027507; Fri, 22 Jan 2016 08:14:50 -0500 Subject: Re: [PATCH V3 7/9] aacraid: Fix AIF triggered IOP_RESET To: Raghava Aditya Renukunta , "James.Bottomley@HansenPartnership.com" , "martin.petersen@oracle.com" , "linux-scsi@vger.kernel.org" References: <1452842182-684-1-git-send-email-RaghavaAditya.Renukunta@pmcs.com> <1452842182-684-8-git-send-email-RaghavaAditya.Renukunta@pmcs.com> <569E60E4.4000903@redhat.com> <198D06D448A18D4E93F08FB849C4E39D7D1F6AA9@BBYEXM01.pmc-sierra.internal> Cc: Mahesh Rajashekhara , Murthy Bhat , Gana Sridaran , "aacraid@pmc-sierra.com" , Scott Benesh , "jthumshirn@suse.de" , "shane.seymour@hpe.com" , zzzDavid Carroll From: Tomas Henzl Message-ID: <56A22B49.1040702@redhat.com> Date: Fri, 22 Jan 2016 14:14:49 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <198D06D448A18D4E93F08FB849C4E39D7D1F6AA9@BBYEXM01.pmc-sierra.internal> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 Sender: linux-scsi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-scsi@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 >>> >>> 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? > > I am more inclined to go with option 2. > > Regards, > Raghava Aditya > >>> Signed-off-by: Raghava Aditya Renukunta >> >>> --- >>> 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 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);