From: Jinpu Wang <jinpu.wang@ionos.com>
To: Ajish Koshy <Ajish.Koshy@microchip.com>
Cc: Linux SCSI Mailinglist <linux-scsi@vger.kernel.org>,
Vasanthalakshmi.Tharmarajan@microchip.com,
Viswas G <Viswas.G@microchip.com>,
Ruksar Devadi <Ruksar.devadi@microchip.com>,
Ashokkumar N <Ashokkumar.N@microchip.com>,
Jinpu Wang <jinpu.wang@cloud.ionos.com>
Subject: Re: [PATCH v2 2/4] scsi: pm80xx: fix lockup due to commit <1f02beff224e>
Date: Tue, 7 Sep 2021 07:59:55 +0200 [thread overview]
Message-ID: <CAMGffEkm6kh96TKY2tKg0kNveQ5VO8ZrmcDOo6wq6NEi-RAubQ@mail.gmail.com> (raw)
In-Reply-To: <20210906170404.5682-3-Ajish.Koshy@microchip.com>
On Mon, Sep 6, 2021 at 6:09 PM Ajish Koshy <Ajish.Koshy@microchip.com> wrote:
>
> Commit 1f02beff224e ("scsi: pm80xx: Remove global lock from
> outbound queue processing") introduced a lock per outbound
> queue, where the driver before that was using a global lock
> for all outbound queues. While processing the IO responses
> and events, driver takes the outbound queue spinlock and
> later it is supposed to release the same spin lock in
> pm8001_ccb_task_free_done() before calling command done().
> Since the older code was using a global lock,
> pm8001_ccb_task_free_done() was also releasing the global
> spin lock. With the commit <1f02beff224e>,
> pm8001_ccb_task_free_done() remains the same and it was
> still using the global lock.
>
> So when driver completes a SATA command,the global spinlock
> will be in a locked state.
> mpi_sata_completion()->spin_lock(&pm8001_ha->lock);
>
> Later when driver gets a scsi command for SATA drive,
> pm8001_task_exec() tries to acquire the global lock and leads
> to lockup crash.
>
> Fixes: 1f02beff224e ("scsi: pm80xx: Remove global lock from outbound queue processing")
> Signed-off-by: Ajish Koshy <Ajish.Koshy@microchip.com>
> Signed-off-by: Viswas G <Viswas.G@microchip.com>
Acked-by: Jack Wang <jinpu.wang@ionos.com>
> ---
> drivers/scsi/pm8001/pm8001_sas.h | 3 +-
> drivers/scsi/pm8001/pm80xx_hwi.c | 53 ++++++++++++++++++++++++++------
> 2 files changed, 45 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
> index 1a016a421280..3274d88a9ccc 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.h
> +++ b/drivers/scsi/pm8001/pm8001_sas.h
> @@ -458,6 +458,7 @@ struct outbound_queue_table {
> __le32 producer_index;
> u32 consumer_idx;
> spinlock_t oq_lock;
> + unsigned long lock_flags;
> };
> struct pm8001_hba_memspace {
> void __iomem *memvirtaddr;
> @@ -740,9 +741,7 @@ pm8001_ccb_task_free_done(struct pm8001_hba_info *pm8001_ha,
> {
> pm8001_ccb_task_free(pm8001_ha, task, ccb, ccb_idx);
> smp_mb(); /*in order to force CPU ordering*/
> - spin_unlock(&pm8001_ha->lock);
> task->task_done(task);
> - spin_lock(&pm8001_ha->lock);
> }
>
> #endif
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
> index cec932f830b8..1ae2f5c6042c 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -2379,7 +2379,8 @@ static void mpi_ssp_event(struct pm8001_hba_info *pm8001_ha, void *piomb)
>
> /*See the comments for mpi_ssp_completion */
> static void
> -mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
> +mpi_sata_completion(struct pm8001_hba_info *pm8001_ha,
> + struct outbound_queue_table *circularQ, void *piomb)
> {
> struct sas_task *t;
> struct pm8001_ccb_info *ccb;
> @@ -2616,7 +2617,11 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
> IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS);
> ts->resp = SAS_TASK_UNDELIVERED;
> ts->stat = SAS_QUEUE_FULL;
> + spin_unlock_irqrestore(&circularQ->oq_lock,
> + circularQ->lock_flags);
> pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
> + spin_lock_irqsave(&circularQ->oq_lock,
> + circularQ->lock_flags);
> return;
> }
> break;
> @@ -2632,7 +2637,11 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
> IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS);
> ts->resp = SAS_TASK_UNDELIVERED;
> ts->stat = SAS_QUEUE_FULL;
> + spin_unlock_irqrestore(&circularQ->oq_lock,
> + circularQ->lock_flags);
> pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
> + spin_lock_irqsave(&circularQ->oq_lock,
> + circularQ->lock_flags);
> return;
> }
> break;
> @@ -2656,7 +2665,11 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
> IO_OPEN_CNX_ERROR_STP_RESOURCES_BUSY);
> ts->resp = SAS_TASK_UNDELIVERED;
> ts->stat = SAS_QUEUE_FULL;
> + spin_unlock_irqrestore(&circularQ->oq_lock,
> + circularQ->lock_flags);
> pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
> + spin_lock_irqsave(&circularQ->oq_lock,
> + circularQ->lock_flags);
> return;
> }
> break;
> @@ -2727,7 +2740,11 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
> IO_DS_NON_OPERATIONAL);
> ts->resp = SAS_TASK_UNDELIVERED;
> ts->stat = SAS_QUEUE_FULL;
> + spin_unlock_irqrestore(&circularQ->oq_lock,
> + circularQ->lock_flags);
> pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
> + spin_lock_irqsave(&circularQ->oq_lock,
> + circularQ->lock_flags);
> return;
> }
> break;
> @@ -2747,7 +2764,11 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
> IO_DS_IN_ERROR);
> ts->resp = SAS_TASK_UNDELIVERED;
> ts->stat = SAS_QUEUE_FULL;
> + spin_unlock_irqrestore(&circularQ->oq_lock,
> + circularQ->lock_flags);
> pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
> + spin_lock_irqsave(&circularQ->oq_lock,
> + circularQ->lock_flags);
> return;
> }
> break;
> @@ -2785,12 +2806,17 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
> pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);
> } else {
> spin_unlock_irqrestore(&t->task_state_lock, flags);
> + spin_unlock_irqrestore(&circularQ->oq_lock,
> + circularQ->lock_flags);
> pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
> + spin_lock_irqsave(&circularQ->oq_lock,
> + circularQ->lock_flags);
> }
> }
>
> /*See the comments for mpi_ssp_completion */
> -static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, void *piomb)
> +static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha,
> + struct outbound_queue_table *circularQ, void *piomb)
> {
> struct sas_task *t;
> struct task_status_struct *ts;
> @@ -2890,7 +2916,11 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, void *piomb)
> IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS);
> ts->resp = SAS_TASK_COMPLETE;
> ts->stat = SAS_QUEUE_FULL;
> + spin_unlock_irqrestore(&circularQ->oq_lock,
> + circularQ->lock_flags);
> pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
> + spin_lock_irqsave(&circularQ->oq_lock,
> + circularQ->lock_flags);
> return;
> }
> break;
> @@ -3002,7 +3032,11 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha, void *piomb)
> pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);
> } else {
> spin_unlock_irqrestore(&t->task_state_lock, flags);
> + spin_unlock_irqrestore(&circularQ->oq_lock,
> + circularQ->lock_flags);
> pm8001_ccb_task_free_done(pm8001_ha, t, ccb, tag);
> + spin_lock_irqsave(&circularQ->oq_lock,
> + circularQ->lock_flags);
> }
> }
>
> @@ -3906,7 +3940,8 @@ static int ssp_coalesced_comp_resp(struct pm8001_hba_info *pm8001_ha,
> * @pm8001_ha: our hba card information
> * @piomb: IO message buffer
> */
> -static void process_one_iomb(struct pm8001_hba_info *pm8001_ha, void *piomb)
> +static void process_one_iomb(struct pm8001_hba_info *pm8001_ha,
> + struct outbound_queue_table *circularQ, void *piomb)
> {
> __le32 pHeader = *(__le32 *)piomb;
> u32 opc = (u32)((le32_to_cpu(pHeader)) & 0xFFF);
> @@ -3948,11 +3983,11 @@ static void process_one_iomb(struct pm8001_hba_info *pm8001_ha, void *piomb)
> break;
> case OPC_OUB_SATA_COMP:
> pm8001_dbg(pm8001_ha, MSG, "OPC_OUB_SATA_COMP\n");
> - mpi_sata_completion(pm8001_ha, piomb);
> + mpi_sata_completion(pm8001_ha, circularQ, piomb);
> break;
> case OPC_OUB_SATA_EVENT:
> pm8001_dbg(pm8001_ha, MSG, "OPC_OUB_SATA_EVENT\n");
> - mpi_sata_event(pm8001_ha, piomb);
> + mpi_sata_event(pm8001_ha, circularQ, piomb);
> break;
> case OPC_OUB_SSP_EVENT:
> pm8001_dbg(pm8001_ha, MSG, "OPC_OUB_SSP_EVENT\n");
> @@ -4121,7 +4156,6 @@ static int process_oq(struct pm8001_hba_info *pm8001_ha, u8 vec)
> void *pMsg1 = NULL;
> u8 bc;
> u32 ret = MPI_IO_STATUS_FAIL;
> - unsigned long flags;
> u32 regval;
>
> if (vec == (pm8001_ha->max_q_num - 1)) {
> @@ -4138,7 +4172,7 @@ static int process_oq(struct pm8001_hba_info *pm8001_ha, u8 vec)
> }
> }
> circularQ = &pm8001_ha->outbnd_q_tbl[vec];
> - spin_lock_irqsave(&circularQ->oq_lock, flags);
> + spin_lock_irqsave(&circularQ->oq_lock, circularQ->lock_flags);
> do {
> /* spurious interrupt during setup if kexec-ing and
> * driver doing a doorbell access w/ the pre-kexec oq
> @@ -4149,7 +4183,8 @@ static int process_oq(struct pm8001_hba_info *pm8001_ha, u8 vec)
> ret = pm8001_mpi_msg_consume(pm8001_ha, circularQ, &pMsg1, &bc);
> if (MPI_IO_STATUS_SUCCESS == ret) {
> /* process the outbound message */
> - process_one_iomb(pm8001_ha, (void *)(pMsg1 - 4));
> + process_one_iomb(pm8001_ha, circularQ,
> + (void *)(pMsg1 - 4));
> /* free the message from the outbound circular buffer */
> pm8001_mpi_msg_free_set(pm8001_ha, pMsg1,
> circularQ, bc);
> @@ -4164,7 +4199,7 @@ static int process_oq(struct pm8001_hba_info *pm8001_ha, u8 vec)
> break;
> }
> } while (1);
> - spin_unlock_irqrestore(&circularQ->oq_lock, flags);
> + spin_unlock_irqrestore(&circularQ->oq_lock, circularQ->lock_flags);
> return ret;
> }
>
> --
> 2.27.0
>
next prev parent reply other threads:[~2021-09-07 6:00 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-06 17:04 [PATCH v2 0/4] pm80xx updates Ajish Koshy
2021-09-06 17:04 ` [PATCH v2 1/4] scsi: pm80xx: fix incorrect port value when registering a device Ajish Koshy
2021-09-07 5:59 ` Jinpu Wang
2021-09-06 17:04 ` [PATCH v2 2/4] scsi: pm80xx: fix lockup due to commit <1f02beff224e> Ajish Koshy
2021-09-07 5:59 ` Jinpu Wang [this message]
2021-09-06 17:04 ` [PATCH v2 3/4] scsi: pm80xx: Corrected Inbound and Outbound queue logging Ajish Koshy
2021-09-06 17:04 ` [PATCH v2 4/4] scsi: pm80xx: fix memory leak during rmmod Ajish Koshy
2021-09-15 2:29 ` [PATCH v2 0/4] pm80xx updates Martin K. Petersen
2021-09-22 4:45 ` Martin K. Petersen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAMGffEkm6kh96TKY2tKg0kNveQ5VO8ZrmcDOo6wq6NEi-RAubQ@mail.gmail.com \
--to=jinpu.wang@ionos.com \
--cc=Ajish.Koshy@microchip.com \
--cc=Ashokkumar.N@microchip.com \
--cc=Ruksar.devadi@microchip.com \
--cc=Vasanthalakshmi.Tharmarajan@microchip.com \
--cc=Viswas.G@microchip.com \
--cc=jinpu.wang@cloud.ionos.com \
--cc=linux-scsi@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).