* [PATCH 3/4] pm8001: add a new spinlock to protect the CCB
@ 2014-06-26 15:17 Tomas Henzl
2014-06-27 6:09 ` Viswas G
0 siblings, 1 reply; 4+ messages in thread
From: Tomas Henzl @ 2014-06-26 15:17 UTC (permalink / raw)
To: linux-scsi
Cc: AnandKumar.Santhanam, Vasanthalakshmi.Tharmarajan,
Suresh.Thiagarajan, Viswas.G, Tomas Henzl
Patch adds a new spinlock to protect the ccb management.
It may happen that concurrent threads become the same tag value
from the 'alloc' function', the spinlock prevents this situation.
Signed-off-by: Tomas Henzl <thenzl@redhat.com>
---
drivers/scsi/pm8001/pm8001_init.c | 1 +
drivers/scsi/pm8001/pm8001_sas.c | 7 ++++++-
drivers/scsi/pm8001/pm8001_sas.h | 1 +
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
index c4f31b21..56b61e5 100644
--- a/drivers/scsi/pm8001/pm8001_init.c
+++ b/drivers/scsi/pm8001/pm8001_init.c
@@ -246,6 +246,7 @@ static int pm8001_alloc(struct pm8001_hba_info *pm8001_ha,
{
int i;
spin_lock_init(&pm8001_ha->lock);
+ spin_lock_init(&pm8001_ha->bitmap_lock);
PM8001_INIT_DBG(pm8001_ha,
pm8001_printk("pm8001_alloc: PHY:%x\n",
pm8001_ha->chip->n_phy));
diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index be55859..34cea82 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -77,11 +77,16 @@ inline int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out)
{
unsigned int tag;
void *bitmap = pm8001_ha->tags;
+ unsigned long flags;
+ spin_lock_irqsave(&pm8001_ha->bitmap_lock, flags);
tag = find_first_zero_bit(bitmap, pm8001_ha->tags_num);
- if (tag >= pm8001_ha->tags_num)
+ if (tag >= pm8001_ha->tags_num) {
+ spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
return -SAS_QUEUE_FULL;
+ }
set_bit(tag, bitmap);
+ spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
*tag_out = tag;
return 0;
}
diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
index 14106ad..f6b2ac5 100644
--- a/drivers/scsi/pm8001/pm8001_sas.h
+++ b/drivers/scsi/pm8001/pm8001_sas.h
@@ -475,6 +475,7 @@ struct pm8001_hba_info {
struct list_head list;
unsigned long flags;
spinlock_t lock;/* host-wide lock */
+ spinlock_t bitmap_lock;
struct pci_dev *pdev;/* our device */
struct device *dev;
struct pm8001_hba_memspace io_mem[6];
--
1.8.3.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* RE: [PATCH 3/4] pm8001: add a new spinlock to protect the CCB
2014-06-26 15:17 [PATCH 3/4] pm8001: add a new spinlock to protect the CCB Tomas Henzl
@ 2014-06-27 6:09 ` Viswas G
2014-06-27 10:21 ` Tomas Henzl
0 siblings, 1 reply; 4+ messages in thread
From: Viswas G @ 2014-06-27 6:09 UTC (permalink / raw)
To: Tomas Henzl, linux-scsi; +Cc: Vasanthalakshmi Tharmarajan, Suresh Thiagarajan
HI Tomas,
This lock is not needed as we are already protecting the tag allocation and freeing with the pm8001_ha->lock. We are always acquiring this lock before calling
pm8001_tag_alloc().
We are allocating and freeing the tag in following paths,
1) Request/Response path : This we are protecting using pm8001_ha->lock.
2)Driver initialization : We are not using the lock here . As there only one will be running, we don't need a lock here.
So there is no need of a specific lock for tag allocation.
Regards,
Viswas G
-----Original Message-----
From: Tomas Henzl [mailto:thenzl@redhat.com]
Sent: Thursday, June 26, 2014 8:48 PM
To: linux-scsi@vger.kernel.org
Cc: zzzAnand Kumar Santhanam(Nov-20-2013); Vasanthalakshmi Tharmarajan; Suresh Thiagarajan; Viswas G; Tomas Henzl
Subject: [PATCH 3/4] pm8001: add a new spinlock to protect the CCB
Patch adds a new spinlock to protect the ccb management.
It may happen that concurrent threads become the same tag value from the 'alloc' function', the spinlock prevents this situation.
Signed-off-by: Tomas Henzl <thenzl@redhat.com>
---
drivers/scsi/pm8001/pm8001_init.c | 1 + drivers/scsi/pm8001/pm8001_sas.c | 7 ++++++- drivers/scsi/pm8001/pm8001_sas.h | 1 +
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
index c4f31b21..56b61e5 100644
--- a/drivers/scsi/pm8001/pm8001_init.c
+++ b/drivers/scsi/pm8001/pm8001_init.c
@@ -246,6 +246,7 @@ static int pm8001_alloc(struct pm8001_hba_info *pm8001_ha, {
int i;
spin_lock_init(&pm8001_ha->lock);
+ spin_lock_init(&pm8001_ha->bitmap_lock);
PM8001_INIT_DBG(pm8001_ha,
pm8001_printk("pm8001_alloc: PHY:%x\n",
pm8001_ha->chip->n_phy));
diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index be55859..34cea82 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -77,11 +77,16 @@ inline int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out) {
unsigned int tag;
void *bitmap = pm8001_ha->tags;
+ unsigned long flags;
+ spin_lock_irqsave(&pm8001_ha->bitmap_lock, flags);
tag = find_first_zero_bit(bitmap, pm8001_ha->tags_num);
- if (tag >= pm8001_ha->tags_num)
+ if (tag >= pm8001_ha->tags_num) {
+ spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
return -SAS_QUEUE_FULL;
+ }
set_bit(tag, bitmap);
+ spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
*tag_out = tag;
return 0;
}
diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
index 14106ad..f6b2ac5 100644
--- a/drivers/scsi/pm8001/pm8001_sas.h
+++ b/drivers/scsi/pm8001/pm8001_sas.h
@@ -475,6 +475,7 @@ struct pm8001_hba_info {
struct list_head list;
unsigned long flags;
spinlock_t lock;/* host-wide lock */
+ spinlock_t bitmap_lock;
struct pci_dev *pdev;/* our device */
struct device *dev;
struct pm8001_hba_memspace io_mem[6];
--
1.8.3.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 3/4] pm8001: add a new spinlock to protect the CCB
2014-06-27 6:09 ` Viswas G
@ 2014-06-27 10:21 ` Tomas Henzl
2014-07-02 9:42 ` Suresh Thiagarajan
0 siblings, 1 reply; 4+ messages in thread
From: Tomas Henzl @ 2014-06-27 10:21 UTC (permalink / raw)
To: Viswas G, linux-scsi; +Cc: Vasanthalakshmi Tharmarajan, Suresh Thiagarajan
On 06/27/2014 08:09 AM, Viswas G wrote:
> HI Tomas,
>
> This lock is not needed as we are already protecting the tag allocation and freeing with the pm8001_ha->lock. We are always acquiring this lock before calling
> pm8001_tag_alloc().
>
> We are allocating and freeing the tag in following paths,
>
> 1) Request/Response path : This we are protecting using pm8001_ha->lock.
> 2)Driver initialization : We are not using the lock here . As there only one will be running, we don't need a lock here.
>
> So there is no need of a specific lock for tag allocation.
I looked at and tested the pm8001_ctl_bios_version_show. From this function the
pm8001_chip_get_nvmd_req is invoked and there is a new 'tag' allocated.
I have noticed your lock nowhere in the path, in addition I was able to trigger this -
[ 813.749060] pm80xx mpi_sata_completion 1991:task null
[ 813.758439] pm80xx mpi_sata_completion 1991:task null
[ 813.809356] pm80xx mpi_sata_completion 1991:task null
[ 813.819881] pm80xx mpi_sata_completion 1991:task null
[ 813.829086] pm80xx mpi_sata_completion 1991:task null
[ 844.376825] pm80xx pm8001_mpi_task_abort_resp 3776:task abort failed status 0x6 ,tag = 0x2, scp= 0x0
[ 844.387057] pm80xx pm8001_mpi_task_abort_resp 3776:task abort failed status 0x6 ,tag = 0x2, scp= 0x0
[ 844.397316] pm80xx pm8001_mpi_task_abort_resp 3776:task abort failed status 0x6 ,tag = 0x2, scp= 0x0
[ 844.406550] pm80xx pm8001_abort_task 1246:rc= 5
[ 844.411142] pm80xx pm8001_query_task 1169::rc= 5
and the system became 'unstable'.
--tm
>
> Regards,
> Viswas G
>
> -----Original Message-----
> From: Tomas Henzl [mailto:thenzl@redhat.com]
> Sent: Thursday, June 26, 2014 8:48 PM
> To: linux-scsi@vger.kernel.org
> Cc: zzzAnand Kumar Santhanam(Nov-20-2013); Vasanthalakshmi Tharmarajan; Suresh Thiagarajan; Viswas G; Tomas Henzl
> Subject: [PATCH 3/4] pm8001: add a new spinlock to protect the CCB
>
> Patch adds a new spinlock to protect the ccb management.
> It may happen that concurrent threads become the same tag value from the 'alloc' function', the spinlock prevents this situation.
>
> Signed-off-by: Tomas Henzl <thenzl@redhat.com>
> ---
> drivers/scsi/pm8001/pm8001_init.c | 1 + drivers/scsi/pm8001/pm8001_sas.c | 7 ++++++- drivers/scsi/pm8001/pm8001_sas.h | 1 +
> 3 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
> index c4f31b21..56b61e5 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -246,6 +246,7 @@ static int pm8001_alloc(struct pm8001_hba_info *pm8001_ha, {
> int i;
> spin_lock_init(&pm8001_ha->lock);
> + spin_lock_init(&pm8001_ha->bitmap_lock);
> PM8001_INIT_DBG(pm8001_ha,
> pm8001_printk("pm8001_alloc: PHY:%x\n",
> pm8001_ha->chip->n_phy));
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
> index be55859..34cea82 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -77,11 +77,16 @@ inline int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out) {
> unsigned int tag;
> void *bitmap = pm8001_ha->tags;
> + unsigned long flags;
>
> + spin_lock_irqsave(&pm8001_ha->bitmap_lock, flags);
> tag = find_first_zero_bit(bitmap, pm8001_ha->tags_num);
> - if (tag >= pm8001_ha->tags_num)
> + if (tag >= pm8001_ha->tags_num) {
> + spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
> return -SAS_QUEUE_FULL;
> + }
> set_bit(tag, bitmap);
> + spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
> *tag_out = tag;
> return 0;
> }
> diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
> index 14106ad..f6b2ac5 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.h
> +++ b/drivers/scsi/pm8001/pm8001_sas.h
> @@ -475,6 +475,7 @@ struct pm8001_hba_info {
> struct list_head list;
> unsigned long flags;
> spinlock_t lock;/* host-wide lock */
> + spinlock_t bitmap_lock;
> struct pci_dev *pdev;/* our device */
> struct device *dev;
> struct pm8001_hba_memspace io_mem[6];
> --
> 1.8.3.1
>
> --
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH 3/4] pm8001: add a new spinlock to protect the CCB
2014-06-27 10:21 ` Tomas Henzl
@ 2014-07-02 9:42 ` Suresh Thiagarajan
0 siblings, 0 replies; 4+ messages in thread
From: Suresh Thiagarajan @ 2014-07-02 9:42 UTC (permalink / raw)
To: Tomas Henzl, Viswas G, linux-scsi
Cc: Vasanthalakshmi Tharmarajan, Jack Wang (xjtuwjp@gmail.com)
On Fri, Jun 27, 2014 at 3:51 PM, Tomas Henzl <thenzl@redhat.com> wrote:
> On 06/27/2014 08:09 AM, Viswas G wrote:
>> HI Tomas,
>>
>> This lock is not needed as we are already protecting the tag
>> allocation and freeing with the pm8001_ha->lock. We are always acquiring this lock before calling pm8001_tag_alloc().
>>
>> We are allocating and freeing the tag in following paths,
>>
>> 1) Request/Response path : This we are protecting using pm8001_ha->lock.
>> 2)Driver initialization : We are not using the lock here . As there only one will be running, we don't need a lock here.
>>
>> So there is no need of a specific lock for tag allocation.
>
> I looked at and tested the pm8001_ctl_bios_version_show. From this
> function the
I agree. Instead of locking from multiple places where we call pm8001_tag_alloc, better way is to use this bitmap lock. Thanks for adding it.
Thanks Tomas. Looks good
Acked-by: Suresh Thiagarajan<Suresh.Thiagarajan@pmcs.com>
> pm8001_chip_get_nvmd_req is invoked and there is a new 'tag' allocated.
> I have noticed your lock nowhere in the path, in addition I was able
> to trigger this - [ 813.749060] pm80xx mpi_sata_completion 1991:task
> null [ 813.758439] pm80xx mpi_sata_completion 1991:task null [
> 813.809356] pm80xx mpi_sata_completion 1991:task null [ 813.819881]
> pm80xx mpi_sata_completion 1991:task null [ 813.829086] pm80xx
> mpi_sata_completion 1991:task null [ 844.376825] pm80xx
> pm8001_mpi_task_abort_resp 3776:task abort failed status 0x6 ,tag =
> 0x2, scp= 0x0 [ 844.387057] pm80xx pm8001_mpi_task_abort_resp
> 3776:task abort failed status 0x6 ,tag = 0x2, scp= 0x0 [ 844.397316]
> pm80xx pm8001_mpi_task_abort_resp 3776:task abort failed status 0x6
> ,tag = 0x2, scp= 0x0 [ 844.406550] pm80xx pm8001_abort_task 1246:rc=
> 5 [ 844.411142] pm80xx pm8001_query_task 1169::rc= 5 and the system became 'unstable'.
>
> --tm
>
>>
>> Regards,
>> Viswas G
>>
>> -----Original Message-----
>> From: Tomas Henzl [mailto:thenzl@redhat.com]
>> Sent: Thursday, June 26, 2014 8:48 PM
>> To: linux-scsi@vger.kernel.org
>> Cc: zzzAnand Kumar Santhanam(Nov-20-2013); Vasanthalakshmi
>> Tharmarajan; Suresh Thiagarajan; Viswas G; Tomas Henzl
>> Subject: [PATCH 3/4] pm8001: add a new spinlock to protect the CCB
>>
>> Patch adds a new spinlock to protect the ccb management.
>> It may happen that concurrent threads become the same tag value from the 'alloc' function', the spinlock prevents this situation.
>>
>> Signed-off-by: Tomas Henzl <thenzl@redhat.com>
>> ---
>> drivers/scsi/pm8001/pm8001_init.c | 1 +
>> drivers/scsi/pm8001/pm8001_sas.c | 7 ++++++-
>> drivers/scsi/pm8001/pm8001_sas.h | 1 +
>> 3 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/pm8001/pm8001_init.c
>> b/drivers/scsi/pm8001/pm8001_init.c
>> index c4f31b21..56b61e5 100644
>> --- a/drivers/scsi/pm8001/pm8001_init.c
>> +++ b/drivers/scsi/pm8001/pm8001_init.c
>> @@ -246,6 +246,7 @@ static int pm8001_alloc(struct pm8001_hba_info *pm8001_ha, {
>> int i;
>> spin_lock_init(&pm8001_ha->lock);
>> + spin_lock_init(&pm8001_ha->bitmap_lock);
>> PM8001_INIT_DBG(pm8001_ha,
>> pm8001_printk("pm8001_alloc: PHY:%x\n",
>> pm8001_ha->chip->n_phy)); diff --git
>> a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
>> index be55859..34cea82 100644
>> --- a/drivers/scsi/pm8001/pm8001_sas.c
>> +++ b/drivers/scsi/pm8001/pm8001_sas.c
>> @@ -77,11 +77,16 @@ inline int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out) {
>> unsigned int tag;
>> void *bitmap = pm8001_ha->tags;
>> + unsigned long flags;
>>
>> + spin_lock_irqsave(&pm8001_ha->bitmap_lock, flags);
>> tag = find_first_zero_bit(bitmap, pm8001_ha->tags_num);
>> - if (tag >= pm8001_ha->tags_num)
>> + if (tag >= pm8001_ha->tags_num) {
>> + spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
>> return -SAS_QUEUE_FULL;
>> + }
>> set_bit(tag, bitmap);
>> + spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
>> *tag_out = tag;
>> return 0;
>> }
>> diff --git a/drivers/scsi/pm8001/pm8001_sas.h
>> b/drivers/scsi/pm8001/pm8001_sas.h
>> index 14106ad..f6b2ac5 100644
>> --- a/drivers/scsi/pm8001/pm8001_sas.h
>> +++ b/drivers/scsi/pm8001/pm8001_sas.h
>> @@ -475,6 +475,7 @@ struct pm8001_hba_info {
>> struct list_head list;
>> unsigned long flags;
>> spinlock_t lock;/* host-wide lock */
>> + spinlock_t bitmap_lock;
>> struct pci_dev *pdev;/* our device */
>> struct device *dev;
>> struct pm8001_hba_memspace io_mem[6];
>> --
>> 1.8.3.1
>>
>> --
>> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-07-02 9:42 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-26 15:17 [PATCH 3/4] pm8001: add a new spinlock to protect the CCB Tomas Henzl
2014-06-27 6:09 ` Viswas G
2014-06-27 10:21 ` Tomas Henzl
2014-07-02 9:42 ` Suresh Thiagarajan
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.