pm8001: locking in mpi_sata_completion() is broken
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
RE: pm8001: locking in mpi_sata_completion() is broken
Quoted 5 lines
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)
Quoted 8 lines
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.
[Jack Wang]
Hi Dan,
Thanks for point out this. Other comments answer below.
Quoted 15 lines
[snip]2006 case
IO_OPEN_CNX_ERROR_IT_NEXUS_LOSS:
2007 PM8001_IO_DBG(pm8001_ha,
2008pm8001_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,
Quoted 8 lines
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?
[Jack Wang]
We hold spin_lock_irqsave when we process interrupt, so here we need restore it to do task done clean.
Quoted 20 lines
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); ^^^^^
Quoted 8 lines
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.
[Jack Wang]
Others are basically the same, what we want is make the lock balance.
Thanks.
Quoted 7 lines
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 10 lines · Jack Wang
Quoted 7 lines
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?[Jack Wang]
We holdspin_lock_irqsavewhen we process interrupt, so here we need restore it to do task done clean.
You're not restoring a previous saved state, flags is set to zero here. You're calling:
spin_unlock_irqrestore(&pm8001_ha->lock, 0);
The whole function is badly broken. mpi_sata_event() has similar problems.
With spin_lock_irqsave() and spin_unlock_irqrestore() you first have to save the state to "flags" before you can restore it. Doing it the other way round doesn't make any sort sense.
regards,
dan carpenter
So could you send a patch to not set the flags to 0, or I post a patch to do this later.
Thanks
Jack
Quoted 3 lines · Jack Wang
Quoted 1 line
Quoted 1 line
2018 mb();/*in order to force CPU
ordering*/
Quoted 6 lines
Quoted 1 line
Quoted 1 line
2019
spin_unlock_irqrestore(&pm8001_ha->lock,
Quoted 4 lines
Quoted 4 lines
flags);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Calling irqrestore before we've done an irqsave. What are we trying
to
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
restore?
[Jack Wang]
We holdspin_lock_irqsavewhen we process interrupt, so here we need
restore
Quoted 16 lines
Quoted 1 line
it to do task done clean.
You're not restoring a previous saved state, flags is set to zero here. You're calling:
spin_unlock_irqrestore(&pm8001_ha->lock, 0);The whole function is badly broken.
mpi_sata_event()has similar problems.With
spin_lock_irqsave()andspin_unlock_irqrestore()you first have to save the state to "flags" before you can restore it. Doing it the other way round doesn't make any sort sense.regards,
dan carpenter