On Fri, Jun 12, 2009 at 8:23 AM, Brian King<brking@xxxxxxxxxxxxxxxxxx> wrote: ... > SCSI allows 8 byte LUNs these days. The reference you make here refers > to bits in CDB which are now reserved. OK - thanks (also say jejb's follow up - thanks too) > >>>> + spin_unlock_irqrestore(&pinstance->pending_pool_lock, lock_flags); >>>> + >>>> + /* Mulitple paths (IO path, control path) may be submitting IOARCBs, >>>> + * hence it is necessary to protect writes to IOA's ioarrin register. >>>> + * All writes to IOA ioarrin are synchronized with host_lock >>>> + */ >>>> + if (lock) >>>> + spin_lock_irqsave(pinstance->host->host_lock, >>>> + pinstance->host_lock_flags); >>>> + >>>> + /* apply memory barrier */ >>>> + mb(); >>>> + /* driver writes lower 32-bit value of IOARCB address only */ >>>> + write64(cmd->ioa_cb->ioarcb.ioarcb_bus_addr, pinstance->ioarrin); >>>> + >>>> + if (lock) >>>> + spin_unlock_irqrestore(pinstance->host->host_lock, >>>> + pinstance->host_lock_flags); >>> Any way to get rid of this lock flag getting passed in? >> >> And I believe due to spinlock/unlock, the mb() is not needed. >> Spin locks imply memory barriers. > > Incorrect. It's generally correct. Spin unlock implies a memory write barrier. OS's would be utterly broken if that were not true. See IA64-linux kernel by David Mosberger and Stephane Eranian. (Part of section 7.2.1 "Memory Mapped IO", page 303, "Ordering Memory Accesses on IA-64"). (And while some parts of the book describe APIs that are obsolete, still a good reference on OS internals/design). > The memory barrier here ensures that the command being > constructed for the adapter is in a consistent state and that all > the writes to the command buffer are flushed to memory before > the write64 happens, which will trigger the adapter to DMA the > command buffer and start executing the command. I understand the application. The mb() is NOT needed. The spin lock already guarantees cache coherency of the Memory write (not MMIO writes!). If anything, mmiowb() is needed. Off hand, I believe mmiowb() should be *after* the write64() since we want to make sure the write64() is delivered to PCI subsystem (where ordering is enforced) before the CPU releases the next lock or other semaphore. And AFAIK, mmiowb() is only required to run on SGI Altix machines due to the fact that releasing a spinlock does not guarantee ordering of MMIO transactions on their HW. (credit to jejb and willy for reminding me of the mmiowb() case). >>>> +/* Maximum number of adapters supported by current version of the driver */ >>>> +#define PMCRAID_MAX_ADAPTERS 32 >>> Why is there a limit on the max adapters supported? >> >> Because of this code I think: >> +DECLARE_BITMAP(pmcraid_minor, PMCRAID_MAX_ADAPTERS); >> ... >> + error = alloc_chrdev_region(&dev, 0, >> + PMCRAID_MAX_ADAPTERS, >> + PMCRAID_DEVFILE); >> >> I don't know offhand how to avoid this. Suggestions? > > Hopefully, this can be solved by removing the character device altogether. > AFAICS, its only used for the ioctls, which should be able to be converted > to use other interfaces such as sysfs or netlink. Ah ok...that sounds better to me too. thanks, grant -- 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
