Re: [PATCH 4/4] ufs: Let the SCSI core allocate per-command UFS data

[Bart Van Assche <bvanassche@xxxxxxx>]

On 1/9/20 1:48 AM, Avri Altman wrote:
> Bart Van Assche wrote:
> > +       /* See also ufshcd_is_scsi() */
> > +       switch (req_op(cmd->request)) {
> > +       case REQ_OP_DRV_IN:
> > +       case REQ_OP_DRV_OUT:
> > +               WARN_ON_ONCE(true);
>
> Maybe just WARN_ON_ONCE(!ufshcd_is_scsi(cmd->request))

Good idea. Will do.

> > +static int ufshcd_init_cmd_priv(struct Scsi_Host *shost, struct
> > +scsi_cmnd *cmd) {
> > +       struct ufs_hba *hba = shost_priv(shost);
> > +
> > +       ufshcd_init_lrb(hba, scsi_cmd_priv(cmd), cmd->tag);
>
> So ufshcd_init_lrb() is called now for every new request?

ufshcd_init_lrb() is only called from inside scsi_add_host(), namely as 
follows:

scsi_add_host()
-> scsi_add_host_with_dma()
  -> scsi_mq_setup_tags()
    -> blk_mq_alloc_tag_set()
      -> blk_mq_alloc_rq_maps()
        -> __blk_mq_alloc_rq_maps()
          -> __blk_mq_alloc_rq_map()
            -> blk_mq_alloc_rqs()
              -> blk_mq_init_request()
                -> scsi_mq_init_request()
                  -> ufshcd_init_cmd_priv()

> > @@ -6074,7 +6132,8 @@ static int ufshcd_eh_device_reset_handler(struct
> > scsi_cmnd *cmd)
> >
> >          /* clear the commands that were pending for corresponding LUN */
> >          for_each_set_bit(pos, &hba->outstanding_reqs, hba->nutrs) {
> > -               if (hba->lrb[pos].lun == lrbp->lun) {
> > +               lrbp2 = ufshcd_tag_to_lrb(hba, pos);
>
> Can lrpb2 be null here?

lrpb2 can only be NULL if the 'pos' argument passed to 
ufshcd_tag_to_lrb() is not a valid tag. for_each_set_bit() however 
guarantees that 0 <= pos < hba->nutrs and hence guarantees that 'pos' is 
a valid tag.

Thanks,

Bart.