On 09/15/2014 06:34 AM, Ching Huang wrote: > On Fri, 2014-09-12 at 16:05 +0200, Tomas Henzl wrote: >> On 09/12/2014 10:22 AM, Ching Huang wrote: >>> From: Ching Huang <ching2048@xxxxxxxxxxxx> >>> >>> This patch is to modify previous patch 16/17 and it is relative to >>> http://git.infradead.org/users/hch/scsi-queue.git/tree/arcmsr-for-3.18:/drivers/scsi/arcmsr >>> >>> change in v4: >>> 1. clean up of duplicate variable declaration in switch. >>> 2. simplify of updating doneq_index and postq_index >>> 3. fix spin_lock area in arcmsr_hbaD_polling_ccbdone >> The intention of the doneq_lock is to protect the pmu->doneq_index and the associated buffer, >> right? Why is the spinlock not used on other places accessing the doneq_index >> like arcmsr_done4abort_postqueue or arcmsr_hbaD_postqueue_isr ? > In fact, in original code arcmsr_hbaD_postqueue_isr has spinlock with a larger area. > As to arcmsr_done4abort_postqueue, it should be protected as below. > > @@ -1182,35 +1178,25 @@ static void arcmsr_done4abort_postqueue( > break; > case ACB_ADAPTER_TYPE_D: { > struct MessageUnit_D *pmu = acb->pmuD; > - uint32_t ccb_cdb_phy, outbound_write_pointer; > - uint32_t doneq_index, index_stripped, addressLow, residual; > - bool error; > - struct CommandControlBlock *pCCB; > + uint32_t outbound_write_pointer; > + uint32_t doneq_index, index_stripped, addressLow, residual, toggle; > + unsigned long flags; > > - outbound_write_pointer = pmu->done_qbuffer[0].addressLow + 1; > - doneq_index = pmu->doneq_index; > residual = atomic_read(&acb->ccboutstandingcount); > for (i = 0; i < residual; i++) { > - while ((doneq_index & 0xFFF) != > + spin_lock_irqsave(&acb->doneq_lock, flags); > + outbound_write_pointer = > + pmu->done_qbuffer[0].addressLow + 1; > + doneq_index = pmu->doneq_index; > + if ((doneq_index & 0xFFF) != > (outbound_write_pointer & 0xFFF)) { > - if (doneq_index & 0x4000) { > - index_stripped = doneq_index & 0xFFF; > - index_stripped += 1; > - index_stripped %= > - ARCMSR_MAX_ARC1214_DONEQUEUE; > - pmu->doneq_index = index_stripped ? > - (index_stripped | 0x4000) : > - (index_stripped + 1); > - } else { > - index_stripped = doneq_index; > - index_stripped += 1; > - index_stripped %= > - ARCMSR_MAX_ARC1214_DONEQUEUE; > - pmu->doneq_index = index_stripped ? > - index_stripped : > - ((index_stripped | 0x4000) + 1); > - } > + toggle = doneq_index & 0x4000; > + index_stripped = (doneq_index & 0xFFF) + 1; > + index_stripped %= ARCMSR_MAX_ARC1214_DONEQUEUE; > + pmu->doneq_index = index_stripped ? (index_stripped | toggle) : > + ((toggle ^ 0x4000) + 1); > doneq_index = pmu->doneq_index; > + spin_unlock_irqrestore(&acb->doneq_lock, flags); > addressLow = pmu->done_qbuffer[doneq_index & > 0xFFF].addressLow; > ccb_cdb_phy = (addressLow & 0xFFFFFFF0); > @@ -1224,11 +1210,10 @@ static void arcmsr_done4abort_postqueue( > arcmsr_drain_donequeue(acb, pCCB, error); > writel(doneq_index, > pmu->outboundlist_read_pointer); > + } else { > + spin_unlock_irqrestore(&acb->doneq_lock, flags); > + mdelay(10); > } > - mdelay(10); > - outbound_write_pointer = > - pmu->done_qbuffer[0].addressLow + 1; > - doneq_index = pmu->doneq_index; > } > pmu->postq_index = 0; > pmu->doneq_index = 0x40FF; > > >> And in arcmsr_hbaD_polling_ccbdone the code looks so: >> >> spin_unlock_irqrestore(&acb->doneq_lock, flags); >> doneq_index = pmu->doneq_index; >> flag_ccb = pmu->done_qbuffer[doneq_index & 0xFFF].addressLow; >> you unlock^ and after that is the value read and used >> Seems to me that I probably don't understand what the spinlock should protect. >> Could you explain ? > You are right. above code should change to ok, awaiting a v5. > > doneq_index = pmu->doneq_index; > spin_unlock_irqrestore(&acb->doneq_lock, flags); > flag_ccb = pmu->done_qbuffer[doneq_index & 0xFFF].addressLow; >> Thanks, >> Tomas >> >> >> >>> Signed-off-by: Ching Huang <ching2048@xxxxxxxxxxxx> >>> --- >>> >>> diff -uprN a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c >>> --- a/drivers/scsi/arcmsr/arcmsr_hba.c 2014-09-12 12:43:16.956653000 +0800 >>> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c 2014-09-12 15:00:23.516069000 +0800 >>> @@ -1121,7 +1121,7 @@ static void arcmsr_drain_donequeue(struc >>> static void arcmsr_done4abort_postqueue(struct AdapterControlBlock *acb) >>> { >>> int i = 0; >>> - uint32_t flag_ccb; >>> + uint32_t flag_ccb, ccb_cdb_phy; >>> struct ARCMSR_CDB *pARCMSR_CDB; >>> bool error; >>> struct CommandControlBlock *pCCB; >>> @@ -1165,10 +1165,6 @@ static void arcmsr_done4abort_postqueue( >>> break; >>> case ACB_ADAPTER_TYPE_C: { >>> struct MessageUnit_C __iomem *reg = acb->pmuC; >>> - struct ARCMSR_CDB *pARCMSR_CDB; >>> - uint32_t flag_ccb, ccb_cdb_phy; >>> - bool error; >>> - struct CommandControlBlock *pCCB; >>> while ((readl(®->host_int_status) & ARCMSR_HBCMU_OUTBOUND_POSTQUEUE_ISR) && (i++ < ARCMSR_MAX_OUTSTANDING_CMD)) { >>> /*need to do*/ >>> flag_ccb = readl(®->outbound_queueport_low); >>> @@ -1182,10 +1178,8 @@ static void arcmsr_done4abort_postqueue( >>> break; >>> case ACB_ADAPTER_TYPE_D: { >>> struct MessageUnit_D *pmu = acb->pmuD; >>> - uint32_t ccb_cdb_phy, outbound_write_pointer; >>> - uint32_t doneq_index, index_stripped, addressLow, residual; >>> - bool error; >>> - struct CommandControlBlock *pCCB; >>> + uint32_t outbound_write_pointer; >>> + uint32_t doneq_index, index_stripped, addressLow, residual, toggle; >>> >>> outbound_write_pointer = pmu->done_qbuffer[0].addressLow + 1; >>> doneq_index = pmu->doneq_index; >>> @@ -1193,23 +1187,11 @@ static void arcmsr_done4abort_postqueue( >>> for (i = 0; i < residual; i++) { >>> while ((doneq_index & 0xFFF) != >>> (outbound_write_pointer & 0xFFF)) { >>> - if (doneq_index & 0x4000) { >>> - index_stripped = doneq_index & 0xFFF; >>> - index_stripped += 1; >>> - index_stripped %= >>> - ARCMSR_MAX_ARC1214_DONEQUEUE; >>> - pmu->doneq_index = index_stripped ? >>> - (index_stripped | 0x4000) : >>> - (index_stripped + 1); >>> - } else { >>> - index_stripped = doneq_index; >>> - index_stripped += 1; >>> - index_stripped %= >>> - ARCMSR_MAX_ARC1214_DONEQUEUE; >>> - pmu->doneq_index = index_stripped ? >>> - index_stripped : >>> - ((index_stripped | 0x4000) + 1); >>> - } >>> + toggle = doneq_index & 0x4000; >>> + index_stripped = (doneq_index & 0xFFF) + 1; >>> + index_stripped %= ARCMSR_MAX_ARC1214_DONEQUEUE; >>> + pmu->doneq_index = index_stripped ? (index_stripped | toggle) : >>> + ((toggle ^ 0x4000) + 1); >>> doneq_index = pmu->doneq_index; >>> addressLow = pmu->done_qbuffer[doneq_index & >>> 0xFFF].addressLow; >>> @@ -1461,7 +1443,7 @@ static void arcmsr_post_ccb(struct Adapt >>> case ACB_ADAPTER_TYPE_D: { >>> struct MessageUnit_D *pmu = acb->pmuD; >>> u16 index_stripped; >>> - u16 postq_index; >>> + u16 postq_index, toggle; >>> unsigned long flags; >>> struct InBound_SRB *pinbound_srb; >>> >>> @@ -1472,19 +1454,11 @@ static void arcmsr_post_ccb(struct Adapt >>> pinbound_srb->addressLow = dma_addr_lo32(cdb_phyaddr); >>> pinbound_srb->length = ccb->arc_cdb_size >> 2; >>> arcmsr_cdb->msgContext = dma_addr_lo32(cdb_phyaddr); >>> - if (postq_index & 0x4000) { >>> - index_stripped = postq_index & 0xFF; >>> - index_stripped += 1; >>> - index_stripped %= ARCMSR_MAX_ARC1214_POSTQUEUE; >>> - pmu->postq_index = index_stripped ? >>> - (index_stripped | 0x4000) : index_stripped; >>> - } else { >>> - index_stripped = postq_index; >>> - index_stripped += 1; >>> - index_stripped %= ARCMSR_MAX_ARC1214_POSTQUEUE; >>> - pmu->postq_index = index_stripped ? index_stripped : >>> - (index_stripped | 0x4000); >>> - } >>> + toggle = postq_index & 0x4000; >>> + index_stripped = postq_index + 1; >>> + index_stripped &= (ARCMSR_MAX_ARC1214_POSTQUEUE - 1); >>> + pmu->postq_index = index_stripped ? (index_stripped | toggle) : >>> + (toggle ^ 0x4000); >>> writel(postq_index, pmu->inboundlist_write_pointer); >>> spin_unlock_irqrestore(&acb->postq_lock, flags); >>> break; >>> @@ -1999,7 +1973,7 @@ static void arcmsr_hbaC_postqueue_isr(st >>> >>> static void arcmsr_hbaD_postqueue_isr(struct AdapterControlBlock *acb) >>> { >>> - u32 outbound_write_pointer, doneq_index, index_stripped; >>> + u32 outbound_write_pointer, doneq_index, index_stripped, toggle; >>> uint32_t addressLow, ccb_cdb_phy; >>> int error; >>> struct MessageUnit_D *pmu; >>> @@ -2013,21 +1987,11 @@ static void arcmsr_hbaD_postqueue_isr(st >>> doneq_index = pmu->doneq_index; >>> if ((doneq_index & 0xFFF) != (outbound_write_pointer & 0xFFF)) { >>> do { >>> - if (doneq_index & 0x4000) { >>> - index_stripped = doneq_index & 0xFFF; >>> - index_stripped += 1; >>> - index_stripped %= ARCMSR_MAX_ARC1214_DONEQUEUE; >>> - pmu->doneq_index = index_stripped >>> - ? (index_stripped | 0x4000) : >>> - (index_stripped + 1); >>> - } else { >>> - index_stripped = doneq_index; >>> - index_stripped += 1; >>> - index_stripped %= ARCMSR_MAX_ARC1214_DONEQUEUE; >>> - pmu->doneq_index = index_stripped >>> - ? index_stripped : >>> - ((index_stripped | 0x4000) + 1); >>> - } >>> + toggle = doneq_index & 0x4000; >>> + index_stripped = (doneq_index & 0xFFF) + 1; >>> + index_stripped %= ARCMSR_MAX_ARC1214_DONEQUEUE; >>> + pmu->doneq_index = index_stripped ? (index_stripped | toggle) : >>> + ((toggle ^ 0x4000) + 1); >>> doneq_index = pmu->doneq_index; >>> addressLow = pmu->done_qbuffer[doneq_index & >>> 0xFFF].addressLow; >>> @@ -2843,7 +2807,7 @@ static bool arcmsr_hbaD_get_config(struc >>> char __iomem *iop_firm_version; >>> char __iomem *iop_device_map; >>> u32 count; >>> - struct MessageUnit_D *reg ; >>> + struct MessageUnit_D *reg; >>> void *dma_coherent2; >>> dma_addr_t dma_coherent_handle2; >>> struct pci_dev *pdev = acb->pdev; >>> @@ -3176,7 +3140,7 @@ static int arcmsr_hbaD_polling_ccbdone(s >>> { >>> bool error; >>> uint32_t poll_ccb_done = 0, poll_count = 0, flag_ccb, ccb_cdb_phy; >>> - int rtn, doneq_index, index_stripped, outbound_write_pointer; >>> + int rtn, doneq_index, index_stripped, outbound_write_pointer, toggle; >>> unsigned long flags; >>> struct ARCMSR_CDB *arcmsr_cdb; >>> struct CommandControlBlock *pCCB; >>> @@ -3185,9 +3149,11 @@ static int arcmsr_hbaD_polling_ccbdone(s >>> polling_hbaD_ccb_retry: >>> poll_count++; >>> while (1) { >>> + spin_lock_irqsave(&acb->doneq_lock, flags); >>> outbound_write_pointer = pmu->done_qbuffer[0].addressLow + 1; >>> doneq_index = pmu->doneq_index; >>> if ((outbound_write_pointer & 0xFFF) == (doneq_index & 0xFFF)) { >>> + spin_unlock_irqrestore(&acb->doneq_lock, flags); >>> if (poll_ccb_done) { >>> rtn = SUCCESS; >>> break; >>> @@ -3200,21 +3166,11 @@ polling_hbaD_ccb_retry: >>> goto polling_hbaD_ccb_retry; >>> } >>> } >>> - spin_lock_irqsave(&acb->doneq_lock, flags); >>> - if (doneq_index & 0x4000) { >>> - index_stripped = doneq_index & 0xFFF; >>> - index_stripped += 1; >>> - index_stripped %= ARCMSR_MAX_ARC1214_DONEQUEUE; >>> - pmu->doneq_index = index_stripped ? >>> - (index_stripped | 0x4000) : >>> - (index_stripped + 1); >>> - } else { >>> - index_stripped = doneq_index; >>> - index_stripped += 1; >>> - index_stripped %= ARCMSR_MAX_ARC1214_DONEQUEUE; >>> - pmu->doneq_index = index_stripped ? index_stripped : >>> - ((index_stripped | 0x4000) + 1); >>> - } >>> + toggle = doneq_index & 0x4000; >>> + index_stripped = (doneq_index & 0xFFF) + 1; >>> + index_stripped %= ARCMSR_MAX_ARC1214_DONEQUEUE; >>> + pmu->doneq_index = index_stripped ? (index_stripped | toggle) : >>> + ((toggle ^ 0x4000) + 1); >>> spin_unlock_irqrestore(&acb->doneq_lock, flags); >>> doneq_index = pmu->doneq_index; >>> flag_ccb = pmu->done_qbuffer[doneq_index & 0xFFF].addressLow; >>> >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in >>> the body of a message to majordomo@xxxxxxxxxxxxxxx >>> 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@xxxxxxxxxxxxxxx > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
