> -----Original Message----- > From: Christoph Hellwig [mailto:hch@xxxxxxxxxxxxx] > Sent: Monday, February 23, 2015 2:25 PM > To: Don Brace > Cc: Scott Teel; Kevin Barnett; james.bottomley@xxxxxxxxxxxxx; > hch@xxxxxxxxxxxxx; Justin Lindley; brace; linux-scsi@xxxxxxxxxxxxxxx > Subject: Re: [PATCH 37/43] hpsa: use block layer tag for command allocation > > > @@ -4691,27 +4701,31 @@ static int hpsa_scsi_queue_command(struct > Scsi_Host *sh, struct scsi_cmnd *cmd) > > > > /* Get the ptr to our adapter structure out of cmd->host. */ > > h = sdev_to_hba(cmd->device); > > - dev = cmd->device->hostdata; > > - if (!dev) { > > + > > + if (cmd->request->tag < 0) { > > + dev_crit(&h->pdev->dev, > > + "sdev %p scmd %p: bad tag %d -- rejecting\n", > > + cmd->device, cmd, cmd->request->tag); > > cmd->result = DID_NO_CONNECT << 16; > > cmd->scsi_done(cmd); > > return 0; > > Shouldn't happen with a modern kernel, but if you really want to > assert this I'd recommend a BUG_ON(). > Done. Left over from early block tag adoption. > > - if (unlikely(lockup_detected(h))) { > > + dev = cmd->device->hostdata; > > + if (!dev) { > > cmd->result = DID_NO_CONNECT << 16; > > cmd->scsi_done(cmd); > > return 0; > > } > > How could this happen (see my earlier question in one of the first > patches). This has happened to us in the past. I'd like to leave it in for now. > > > @@ -4838,6 +4852,11 @@ static int hpsa_register_scsi(struct ctlr_info *h) > > sh->hostdata[0] = (unsigned long) h; > > sh->irq = h->intr[h->intr_mode]; > > sh->unique_id = sh->irq; > > + if (!shost_use_blk_mq(sh)) { > > + error = scsi_init_shared_tag_map(sh, sh->can_queue); > > + if (error) > > + goto fail_host_put; > > + } > > scsi_init_shared_tag_map alredy has the blk-mq check, so you can call > it unconditionally. > > > +static void hpsa_print_scsi_cmd(struct ctlr_info *h, struct scsi_cmnd *scmd, > > + char *prefix) > > Maybe just use scsi_print_command? Done. > > > +static void cmd_tagged_free(struct ctlr_info *h, struct CommandList *c) > > +{ > > + /* > > + * Release our reference to the block. We don't need to do anything > > + * else to free it, because it is accessed by index. (There's no point > > + * in checking the result of the decrement, since we cannot guarantee > > + * that there isn't a concurrent abort which is also accessing it.) > > + */ > > + (void)atomic_dec_and_test(&c->refcount); > > Just use atomic_dec(). > > Any reason you're implementing your own allocator instead of setting > ->cmd_size? I need to do some more work on command management. Our legacy driver still has to manage its own allocation. Would like to leave this alone for now. -- 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
