On Mon, 2014-09-15 at 13:50 +0200, Tomas Henzl wrote: > On 09/15/2014 12:36 PM, Ching Huang wrote: > > On Mon, 2014-09-15 at 12:25 +0200, Tomas Henzl wrote: > >> On 09/15/2014 04:56 AM, Ching Huang wrote: > >>> On Fri, 2014-09-12 at 15:34 +0200, Tomas Henzl wrote: > >>>> On 09/12/2014 09:29 AM, Ching Huang wrote: > >>>>> From: Ching Huang <ching2048@xxxxxxxxxxxx> > >>>>> > >>>>> This patch is to modify previous patch 13/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. for readability, rename firstindex to getIndex, rename lastindex to putIndex > >>>> For some reason, the names head+tail areusual for a circular buffer. > >>>> But let us ignore the names, I don't care. > >>>>> 2. define ARCMSR_API_DATA_BUFLEN as 1032 > >>>>> 3. simplify ioctl data read by macro CIRC_CNT_TO_END and CIRC_CNT > >>>> It's definitely better when you post renames and other non-functional changes in separate > >>>> patches, it's easier for the reviewer. > >>>>> Signed-off-by: Ching Huang <ching2048@xxxxxxxxxxxx> > >>>>> --- > >>>>> > >>>>> diff -uprN a/drivers/scsi/arcmsr/arcmsr_attr.c b/drivers/scsi/arcmsr/arcmsr_attr.c > >>>>> --- a/drivers/scsi/arcmsr/arcmsr_attr.c 2014-08-21 12:14:27.000000000 +0800 > >>>>> +++ b/drivers/scsi/arcmsr/arcmsr_attr.c 2014-09-12 15:18:46.659125000 +0800 > >>>>> @@ -50,6 +50,7 @@ > >>>>> #include <linux/errno.h> > >>>>> #include <linux/delay.h> > >>>>> #include <linux/pci.h> > >>>>> +#include <linux/circ_buf.h> > >>>>> > >>>>> #include <scsi/scsi_cmnd.h> > >>>>> #include <scsi/scsi_device.h> > >>>>> @@ -68,7 +69,7 @@ static ssize_t arcmsr_sysfs_iop_message_ > >>>>> struct device *dev = container_of(kobj,struct device,kobj); > >>>>> struct Scsi_Host *host = class_to_shost(dev); > >>>>> struct AdapterControlBlock *acb = (struct AdapterControlBlock *) host->hostdata; > >>>>> - uint8_t *pQbuffer,*ptmpQbuffer; > >>>>> + uint8_t *ptmpQbuffer; > >>>>> int32_t allxfer_len = 0; > >>>>> unsigned long flags; > >>>>> > >>>>> @@ -78,57 +79,22 @@ static ssize_t arcmsr_sysfs_iop_message_ > >>>>> /* do message unit read. */ > >>>>> ptmpQbuffer = (uint8_t *)buf; > >>>>> spin_lock_irqsave(&acb->rqbuffer_lock, flags); > >>>>> - if (acb->rqbuf_firstindex != acb->rqbuf_lastindex) { > >>>>> - pQbuffer = &acb->rqbuffer[acb->rqbuf_firstindex]; > >>>>> - if (acb->rqbuf_firstindex > acb->rqbuf_lastindex) { > >>>>> - if ((ARCMSR_MAX_QBUFFER - acb->rqbuf_firstindex) >= 1032) { > >>>>> - memcpy(ptmpQbuffer, pQbuffer, 1032); > >>>>> - acb->rqbuf_firstindex += 1032; > >>>>> - acb->rqbuf_firstindex %= ARCMSR_MAX_QBUFFER; > >>>>> - allxfer_len = 1032; > >>>>> - } else { > >>>>> - if (((ARCMSR_MAX_QBUFFER - acb->rqbuf_firstindex) > >>>>> - + acb->rqbuf_lastindex) > 1032) { > >>>>> - memcpy(ptmpQbuffer, pQbuffer, > >>>>> - ARCMSR_MAX_QBUFFER > >>>>> - - acb->rqbuf_firstindex); > >>>>> - ptmpQbuffer += ARCMSR_MAX_QBUFFER > >>>>> - - acb->rqbuf_firstindex; > >>>>> - memcpy(ptmpQbuffer, acb->rqbuffer, 1032 > >>>>> - - (ARCMSR_MAX_QBUFFER - > >>>>> - acb->rqbuf_firstindex)); > >>>>> - acb->rqbuf_firstindex = 1032 - > >>>>> - (ARCMSR_MAX_QBUFFER - > >>>>> - acb->rqbuf_firstindex); > >>>>> - allxfer_len = 1032; > >>>>> - } else { > >>>>> - memcpy(ptmpQbuffer, pQbuffer, > >>>>> - ARCMSR_MAX_QBUFFER - > >>>>> - acb->rqbuf_firstindex); > >>>>> - ptmpQbuffer += ARCMSR_MAX_QBUFFER - > >>>>> - acb->rqbuf_firstindex; > >>>>> - memcpy(ptmpQbuffer, acb->rqbuffer, > >>>>> - acb->rqbuf_lastindex); > >>>>> - allxfer_len = ARCMSR_MAX_QBUFFER - > >>>>> - acb->rqbuf_firstindex + > >>>>> - acb->rqbuf_lastindex; > >>>>> - acb->rqbuf_firstindex = > >>>>> - acb->rqbuf_lastindex; > >>>>> - } > >>>>> - } > >>>>> - } else { > >>>>> - if ((acb->rqbuf_lastindex - acb->rqbuf_firstindex) > 1032) { > >>>>> - memcpy(ptmpQbuffer, pQbuffer, 1032); > >>>>> - acb->rqbuf_firstindex += 1032; > >>>>> - allxfer_len = 1032; > >>>>> - } else { > >>>>> - memcpy(ptmpQbuffer, pQbuffer, acb->rqbuf_lastindex > >>>>> - - acb->rqbuf_firstindex); > >>>>> - allxfer_len = acb->rqbuf_lastindex - > >>>>> - acb->rqbuf_firstindex; > >>>>> - acb->rqbuf_firstindex = acb->rqbuf_lastindex; > >>>>> - } > >>>>> + if (acb->rqbuf_getIndex != acb->rqbuf_putIndex) { > >>>>> + unsigned int tail = acb->rqbuf_getIndex; > >>>>> + unsigned int head = acb->rqbuf_putIndex; > >>>>> + unsigned int cnt_to_end = CIRC_CNT_TO_END(head, tail, ARCMSR_MAX_QBUFFER); > >>>>> + > >>>>> + allxfer_len = CIRC_CNT(head, tail, ARCMSR_MAX_QBUFFER); > >>>>> + if (allxfer_len > ARCMSR_API_DATA_BUFLEN) > >>>>> + allxfer_len = ARCMSR_API_DATA_BUFLEN; > >>>>> + > >>>>> + if (allxfer_len <= cnt_to_end) > >>>>> + memcpy(ptmpQbuffer, acb->rqbuffer + tail, allxfer_len); > >>>>> + else { > >>>>> + memcpy(ptmpQbuffer, acb->rqbuffer + tail, cnt_to_end); > >>>>> + memcpy(ptmpQbuffer + cnt_to_end, acb->rqbuffer, allxfer_len - cnt_to_end); > >>>>> } > >>>>> + acb->rqbuf_getIndex = (acb->rqbuf_getIndex + allxfer_len) % ARCMSR_MAX_QBUFFER; > >>>>> } > >>>>> if (acb->acb_flags & ACB_F_IOPDATA_OVERFLOW) { > >>>>> struct QBUFFER __iomem *prbuffer; > >>>>> @@ -150,33 +116,32 @@ static ssize_t arcmsr_sysfs_iop_message_ > >>>>> struct device *dev = container_of(kobj,struct device,kobj); > >>>>> struct Scsi_Host *host = class_to_shost(dev); > >>>>> struct AdapterControlBlock *acb = (struct AdapterControlBlock *) host->hostdata; > >>>>> - int32_t my_empty_len, user_len, wqbuf_firstindex, wqbuf_lastindex; > >>>>> + int32_t my_empty_len, user_len, wqbuf_getIndex, wqbuf_putIndex; > >>>>> uint8_t *pQbuffer, *ptmpuserbuffer; > >>>>> unsigned long flags; > >>>>> > >>>>> if (!capable(CAP_SYS_ADMIN)) > >>>>> return -EACCES; > >>>>> - if (count > 1032) > >>>>> + if (count > ARCMSR_API_DATA_BUFLEN) > >>>>> return -EINVAL; > >>>>> /* do message unit write. */ > >>>>> ptmpuserbuffer = (uint8_t *)buf; > >>>>> user_len = (int32_t)count; > >>>>> spin_lock_irqsave(&acb->wqbuffer_lock, flags); > >>>>> - wqbuf_lastindex = acb->wqbuf_lastindex; > >>>>> - wqbuf_firstindex = acb->wqbuf_firstindex; > >>>>> - if (wqbuf_lastindex != wqbuf_firstindex) { > >>>>> + wqbuf_putIndex = acb->wqbuf_putIndex; > >>>>> + wqbuf_getIndex = acb->wqbuf_getIndex; > >>>>> + if (wqbuf_putIndex != wqbuf_getIndex) { > >>>>> arcmsr_write_ioctldata2iop(acb); > >>>>> spin_unlock_irqrestore(&acb->wqbuffer_lock, flags); > >>>>> return 0; /*need retry*/ > >>>>> } else { > >>>>> - my_empty_len = (wqbuf_firstindex-wqbuf_lastindex - 1) > >>>>> - &(ARCMSR_MAX_QBUFFER - 1); > >>>>> + my_empty_len = ARCMSR_MAX_QBUFFER - 1; > >>>> This^ doesn't look like like an rename can you explain? > >>> It is just a code simplification. > >> Yes it is of some kind. But I think it's also a bug, because you have replaced > >> 'firstindex - lastindex' with nothing. > > It will be not a bug. At here, firstindex == lastindex, so firstindex-lastindex is 0 > > Thanks, I haven't seen it before. In that case because user_len is max 1032 and ARCMSR_MAX_QBUFFER is 4k > the next if statement '(my_empty_len >= user_len)' will be true for any user_len (<1032), > and it looks like you could remove my_empty_len completely. Yes. You are right. thanks > > >>>> Let us stop here, or we end in an endless loop of corrections. The original 13/17 > >>>> you are trying to improve here was at least without bugs (better said I haven't noticed) so > >>>> improving it now only complicates the process. > >>>> My suggestion is -let us skip this patch and focus only on fixing the spinlock problem found in 16/17. > >>>> OKay? > >>> Agree. > >>>> Cheers, > >>>> Tomas > >>>> > >>> -- > >>> 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
