From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751802Ab2B0FIU (ORCPT ); Mon, 27 Feb 2012 00:08:20 -0500 Received: from sr-smtp.usish.com ([210.5.144.203]:38985 "EHLO sr-smtp.usish.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750751Ab2B0FIS convert rfc822-to-8bit (ORCPT ); Mon, 27 Feb 2012 00:08:18 -0500 From: "Jack Wang" To: "'santosh prasad nayak'" Cc: , , , , References: <1330263303-25177-1-git-send-email-santoshprasadnayak@gmail.com> Subject: Re: [PATCH 2/2] [SCSI] pm8001: Fix possible racing codition. Date: Mon, 27 Feb 2012 13:06:32 +0800 Message-ID: <608226F8941E4DE49A175A8B3262BA41@usish.com.cn> MIME-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 8BIT X-Mailer: Microsoft Office Outlook 11 X-MimeOLE: Produced By Microsoft MimeOLE V6.00.2900.6157 In-Reply-To: Thread-Index: Acz1DLTh6R/pMhxtSx6kf8co2aMyQwAAJmeA Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org The first is not called, just implement a interface, just reserved. Thanks anyway. Jack > > Thanks Jack for your response. > > 'process_oq' is called at two places. > > > pm8001_8001_dispatch = { >          ....... > >         .isr             = pm8001_chip_isr --> process_oq, // 1st > place >         .isr_process_oq  = process_oq,                   // > 2nd place >         ..... > } > > regards > santosh > > On Mon, Feb 27, 2012 at 10:18 AM, Jack Wang wrote: > > Infact , process_oq is only called in pm8001_chip_isr, but this patch looks > > OK for me. Move down the lock to process_oq is OK. > > > > Thanks for fix. > > Acked-by : Jack Wang > > > >> From: Santosh Nayak > >> > >> There is a possble racing scenario. > >> > >> 'process_oq' is called by two routines, as shown below. > >> > >> pm8001_8001_dispatch = { > >>          ....... > >> > >>         .isr             = pm8001_chip_isr --> process_oq,// A > >>         .isr_process_oq  = process_oq,                   //  B > >>         ..... > >> } > >> > >> process_oq() --> process_one_iomb() --> mpi_sata_completion() > >> > >> In 'mpi_sata_completion', "pm8001_ha->lock" is first released. > >> It means lock is taken before,  which is true for > >> the context A, as 'pm8001_ha->lock' is taken in 'pm8001_chip_isr()' > >> > >> But for context B there is no lock taken before and pm8001_ha->lock > >> is unlocked in 'mpi_sata_completion()'. This may unlock the lock > >> taken in context A. Possible racing ?? > >> > >> If 'pm8001_ha->lock' is taken in 'process_oq()' instead of > >> 'pm8001_chip_isr' then the above issue can be avoided. > >> > >> Signed-off-by: Santosh Nayak > >> --- > >>  drivers/scsi/pm8001/pm8001_hwi.c |    6 +++--- > >>  1 files changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/scsi/pm8001/pm8001_hwi.c > >> b/drivers/scsi/pm8001/pm8001_hwi.c > >> index 838e3e2..6d9973b 100644 > >> --- a/drivers/scsi/pm8001/pm8001_hwi.c > >> +++ b/drivers/scsi/pm8001/pm8001_hwi.c > >> @@ -3663,7 +3663,9 @@ static int process_oq(struct pm8001_hba_info > > *pm8001_ha) > >>       void *pMsg1 = NULL; > >>       u8 uninitialized_var(bc); > >>       u32 ret = MPI_IO_STATUS_FAIL; > >> +     unsigned long flags; > >> > >> +     spin_lock_irqsave(&pm8001_ha->lock, flags); > >>       circularQ = &pm8001_ha->outbnd_q_tbl[0]; > >>       do { > >>               ret = mpi_msg_consume(pm8001_ha, circularQ, &pMsg1, &bc); > >> @@ -3682,6 +3684,7 @@ static int process_oq(struct pm8001_hba_info > > *pm8001_ha) > >>                               break; > >>               } > >>       } while (1); > >> +     spin_unlock_irqrestore(&pm8001_ha->lock, flags); > >>       return ret; > >>  } > >> > >> @@ -4087,12 +4090,9 @@ static u32 pm8001_chip_is_our_interupt(struct > >> pm8001_hba_info *pm8001_ha) > >>  static irqreturn_t > >>  pm8001_chip_isr(struct pm8001_hba_info *pm8001_ha) > >>  { > >> -     unsigned long flags; > >> -     spin_lock_irqsave(&pm8001_ha->lock, flags); > >>       pm8001_chip_interrupt_disable(pm8001_ha); > >>       process_oq(pm8001_ha); > >>       pm8001_chip_interrupt_enable(pm8001_ha); > >> -     spin_unlock_irqrestore(&pm8001_ha->lock, flags); > >>       return IRQ_HANDLED; > >>  } > >> > >> -- > >> 1.7.4.4 > >> > >> -- > >> 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