All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dan Carpenter <dan.carpenter@oracle.com>
To: jack_wang@usish.com
Cc: lindar_liu@usish.com, linux-scsi@vger.kernel.org
Subject: pm8001: locking in mpi_sata_completion() is broken
Date: Wed, 14 Dec 2011 12:29:47 +0300	[thread overview]
Message-ID: <20111214092947.GA1537@elgon.mountain> (raw)

The locking in mpi_sata_completion() is broken.  I'm not sure what was
intended at all.  Here is what it does:

  1876  mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
  1877  {
  1878          struct sas_task *t;
  1879          struct pm8001_ccb_info *ccb;
  1880          unsigned long flags = 0;
                ^^^^^^^^^^^^^^^^^^^^^^^
This is bogus.  The flags should be set with irqsave.  This implies a
knowledge of the internals which should be abstracted away.

	[snip]

  2006          case IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS:
  2007                  PM8001_IO_DBG(pm8001_ha,
  2008                          pm8001_printk("IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS\n"));
  2009                  ts->resp = SAS_TASK_COMPLETE;
  2010                  ts->stat = SAS_DEV_NO_RESPONSE;
  2011                  if (!t->uldd_task) {
  2012                          pm8001_handle_event(pm8001_ha,
  2013                                  pm8001_dev,
  2014                                  IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS);
  2015                          ts->resp = SAS_TASK_UNDELIVERED;
  2016                          ts->stat = SAS_QUEUE_FULL;
  2017                          pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);
  2018                          mb();/*in order to force CPU ordering*/
  2019                          spin_unlock_irqrestore(&pm8001_ha->lock, flags);
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Calling irqrestore before we've done an irqsave.  What are we trying to
restore?

  2020                          t->task_done(t);
  2021                          spin_lock_irqsave(&pm8001_ha->lock, flags);
  2022                          return;
  2023                  }

	[snip]

  2197                  spin_unlock_irqrestore(&t->task_state_lock, flags);
  2198                  pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);
  2199                  mb();/*ditto*/
  2200                  spin_unlock_irqrestore(&pm8001_ha->lock, flags);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
We just called irqrestore so there is no need to restore it twice.

  2201                  t->task_done(t);
  2202                  spin_lock_irqsave(&pm8001_ha->lock, flags);
                                                            ^^^^^
We're saving the irq status but we're at the end of the function so we
never use it again.

  2203          }
  2204  }

I've just picked out a couple examples, but basically the whole function
is like that.

regards,
dan carpenter

             reply	other threads:[~2011-12-14  9:30 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-12-14  9:29 Dan Carpenter [this message]
2011-12-15  1:26 ` pm8001: locking in mpi_sata_completion() is broken Jack Wang
2011-12-15  6:15   ` Dan Carpenter
2011-12-15  9:03     ` Jack Wang

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=20111214092947.GA1537@elgon.mountain \
    --to=dan.carpenter@oracle.com \
    --cc=jack_wang@usish.com \
    --cc=lindar_liu@usish.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.