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:the...@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 <the...@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 majord...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
