On Thu, Jun 11, 2009 at 9:32 AM, Brian King<brking@xxxxxxxxxxxxxxxxxx> wrote:
> Anil Ravindranath wrote:
....
>> +static void pmcraid_init_res_table(struct pmcraid_cmd *);
>> +static void pmcraid_set_supported_devs(struct pmcraid_cmd *);
>> +static void pmcraid_fail_outstanding_cmds(struct pmcraid_instance *);
>
> Its generally suggested to structure your driver such that you minimize,
> if not avoid having to declare prototypes. Not sure if you can move some functions
> around to reduce the number of prototypes here.
Anil,
I was thinking the same thing that Brian mentions. Basically,
he's saying order the function declarations such that forward
declarations of functions are not needed.
>> +static int pmcraid_slave_alloc(struct scsi_device *scsi_dev)
>> +{
>> + struct pmcraid_resource_entry *temp, *res = NULL;
>> + struct pmcraid_instance *pinstance;
>> + u8 target, bus, lun;
>> + unsigned long lock_flags;
>> + int rc = -ENXIO;
>> +
>> + pinstance = (struct pmcraid_instance *)scsi_dev->host->hostdata;
>> +
>> + /* Driver exposes VSET and GSCSI resources only; all other device types
>> + * are not exposed. Resource list is synchronized using resource lock
>> + * so any traversal or modifications to the list should be done inside
>> + * this lock
>> + */
>> + spin_lock_irqsave(&pinstance->resource_lock, lock_flags);
>> + list_for_each_entry(temp, &pinstance->used_res_q, queue) {
>> +
>> + /* do not expose VSETs with order-ids >= 240 */
>> + if (RES_IS_VSET(temp->cfg_entry)) {
>> + target = temp->cfg_entry.unique_flags1;
>> + if (target >= PMCRAID_MAX_VSET_TARGETS)
>> + continue;
>> + bus = PMCRAID_VSET_BUS_ID;
>> + lun = 0;
>> + } else if (RES_IS_GSCSI(temp->cfg_entry)) {
>> + target = RES_TARGET(temp->cfg_entry.resource_address);
>> + bus = PMCRAID_PHYS_BUS_ID;
>> + lun = RES_LUN(temp->cfg_entry.resource_address);
>
> I assume this means this adapter only supports single byte LUNs...
ISTR, SCSI-3 spec only defines 5-bits for the LUN field...but my SCSI-foo
pretty old and I might misremember. It was easy to find this reference:
http://en.wikipedia.org/wiki/SCSI_Read_Commands
I'm sure there something better from t10.org but everything requires
a login now and I'm sure someone here will just know this.
...
>> +/* writing into a 64-bit iomemory address */
>> +static inline void write64(unsigned long val, void __iomem *addr)
>> +{
>> + /* write MSBytes first as writing to LSBytes starts IOA DMA. IOARCB
>> + * address is always 32-bit as it allocated by pci_alloc_consistent
>> + * hence first write is not required
>> + */
>> + /*iowrite32((u32) (val >> 32), (addr + 4)); */
>> + iowrite32(le32_to_cpu(val), addr);
>
> Wrapper functions like this that simply wrapper an existing Linux API
> are generally frowned upon, just call the function directly.
> Additionally, shouldn't this be calling writel instead? That's what almost every
> SCSI driver does.
Use writeq() or iowrite64() instead. That's a 64-bit MMIO write.
Two additional things bug me about this function:
1) Assumes pci_alloc_consistent() returns 32-bit DMA addresses.
That assumption is only guaranteed if the driver calls
pci_set_consistent_dma_mask()
with a 32-bit mask (which happens to be the current default). This
driver does call
pci_set_dma_mask() but does not call pci_set_consistent_dma_mask().
It should. New PCI-e device driver should be setting this to
64-bits and using
writeq() to push the address.
2) Comments further down claim write64() isn't actually writing 64-bits.
More bad assumption. Please remove those comments and use
writeq() instead. Assumptions like this will burn us later when the
code or behavior is changed and we miss something like this.
>> +static void _pmcraid_fire_command(struct pmcraid_cmd *cmd, u8 lock)
>> +{
>> + struct pmcraid_instance *pinstance = cmd->drv_inst;
>> + unsigned long lock_flags;
>> +
>> + /* Add this command block to pending cmd pool. We do this prior to
>> + * writting IOARCB to ioarrin because IOA might complete the command
>> + * by the time we are about to add it to the list. Response handler
>> + * (isr/tasklet) looks for cmb block in the pending pending list.
>> + */
>> + spin_lock_irqsave(&pinstance->pending_pool_lock, lock_flags);
>> + list_add_tail(&cmd->free_list, &pinstance->pending_cmd_pool);
>> + atomic_inc(&pinstance->outstanding_cmds);
atomic_inc doesn't need to be lock protected. This can be moved outside
the critical code (between lock/unlock).
>> + 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.
>> +static void pmcraid_start_bist(struct pmcraid_cmd *cmd)
>> +{
>> + struct pmcraid_instance *pinstance = cmd->drv_inst;
>> +
>> + /* proceed with bist and wait for 2 seconds */
>> + pci_block_user_cfg_access(pinstance->pdev);
>> + iowrite32(DOORBELL_IOA_START_BIST,
>> + pinstance->int_regs.host_ioa_interrupt_reg);
>
> Are you actually running BIST here or some other reset? BIST is typically
> initiated through PCI config space rather than memory space.
This might be something other than the PCI Config BIST.
And the iowrite32(host_ioa_interrupt_reg) needs to be followed by
a pci config read to flush the MMIO write. iowrite32() is a posted write.
...
>> +static void pmcraid_initiate_reset(struct pmcraid_instance *pinstance)
>> +{
>> + struct pmcraid_cmd *cmd;
>> + unsigned long lock_flags;
>> +
>> + /* If the reset is already in progress, just return, otherwise start
>> + * reset sequence and return
>> + */
>> + spin_lock_irqsave(&pinstance->reset_lock, lock_flags);
>> + if (pinstance->ioa_reset_in_progress) {
>> + spin_unlock_irqrestore(&pinstance->reset_lock, lock_flags);
>> + } else {
>> + spin_unlock_irqrestore(&pinstance->reset_lock, lock_flags);
>
> This looks wrong. If you need to hold the lock to check ioa_reset_in_progress,
> don't you need to hold it through the next couple of lines of code when you
> initiate the reset? How do you know the state doesn't change while you are
> executing the next few lines of code?
I agree this looks wrong.
He needs to set ioa_reset_in_progress in the "else" case and can then
release the spinlock.
>> +
>> + /* If this was a SCSI read/write command keep count of errors */
>> + if (SCSI_CMD_TYPE(scsi_cmd->cmnd[0]) == SCSI_READ_CMD)
>> + res->read_failures++;
>> + else if (SCSI_CMD_TYPE(scsi_cmd->cmnd[0]) == SCSI_WRITE_CMD)
>> + res->write_failures++;
>
> These are both getting incremented without locks, which could cause them
> to get corrupted.
atomic_inc() should be sufficient here.
(I didn't check the locking...assuming Brian is correct.)
>> +/* 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?
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
