Re: [PATCH 4/6] scsi: pm8001: Use sas_task_find_rq() for tagging

[John Garry]

> > diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
> > index 0edc9857a8bd..0868836e7391 100644
> > --- a/drivers/scsi/pm8001/pm8001_init.c
> > +++ b/drivers/scsi/pm8001/pm8001_init.c
> > @@ -1208,17 +1208,14 @@ static int pm8001_init_ccb_tag(struct pm8001_hba_info *pm8001_ha)
> >   	struct Scsi_Host *shost = pm8001_ha->shost;
> >   	struct device *dev = pm8001_ha->dev;
> >   	u32 max_out_io, ccb_count;
> > -	u32 can_queue;
> >   	int i;
> >   
> >   	max_out_io = pm8001_ha->main_cfg_tbl.pm80xx_tbl.max_out_io;
> >   	ccb_count = min_t(int, PM8001_MAX_CCB, max_out_io);
> >   
> > -	/* Update to the scsi host*/
> > -	can_queue = ccb_count - PM8001_RESERVE_SLOT;
> > -	shost->can_queue = can_queue;
> > +	shost->can_queue = ccb_count - PM8001_RESERVE_SLOT;
> >   
> > -	pm8001_ha->tags = bitmap_zalloc(ccb_count, GFP_KERNEL);
> > +	pm8001_ha->tags = bitmap_zalloc(PM8001_RESERVE_SLOT, GFP_KERNEL);
>
> The "tags" name for this field is really confusing as it seems to be
> implying "all tags". Could we rename that to reserved_tags or similar ?

Sure

> >   	if (!pm8001_ha->tags)
> >   		goto err_out;
> >   
> > @@ -1244,9 +1241,10 @@ static int pm8001_init_ccb_tag(struct pm8001_hba_info *pm8001_ha)
> >   		pm8001_ha->ccb_info[i].task = NULL;
> >   		pm8001_ha->ccb_info[i].ccb_tag = PM8001_INVALID_TAG;
> >   		pm8001_ha->ccb_info[i].device = NULL;
> > -		++pm8001_ha->tags_num;
> >   	}
> >   
> > +	pm8001_ha->tags_num = PM8001_RESERVE_SLOT;
>
> Same here. reserved_tags_num ?
> But given that this seems to always be equal to PM8001_RESERVE_SLOT, do
> we even need this field at all ?

I don't think so. I can zap it.

> > +
> >   	return 0;
> >   
> >   err_out_noccb:
> > diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
> > index 066dfa9f4683..9d25855af657 100644
> > --- a/drivers/scsi/pm8001/pm8001_sas.c
> > +++ b/drivers/scsi/pm8001/pm8001_sas.c
> > @@ -68,6 +68,11 @@ void pm8001_tag_free(struct pm8001_hba_info *pm8001_ha, u32 tag)
> >   	void *bitmap = pm8001_ha->tags;
> >   	unsigned long flags;
> >   
> > +	if (tag < pm8001_ha->shost->can_queue)
> > +		return;
> > +
> > +	tag -= pm8001_ha->shost->can_queue;
> > +
> >   	spin_lock_irqsave(&pm8001_ha->bitmap_lock, flags);
> >   	__clear_bit(tag, bitmap);
> >   	spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
> > @@ -92,6 +97,9 @@ int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out)
> >   	}
> >   	__set_bit(tag, bitmap);
> >   	spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
> > +
> > +	/* reserved tags are in the upper region of the tagset */
> > +	tag += pm8001_ha->shost->can_queue;
> >   	*tag_out = tag;
> >   	return 0;
> >   }
> > diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
> > index 9acaadf02150..9ff8d1fa84b0 100644
> > --- a/drivers/scsi/pm8001/pm8001_sas.h
> > +++ b/drivers/scsi/pm8001/pm8001_sas.h
> > @@ -737,9 +737,13 @@ pm8001_ccb_alloc(struct pm8001_hba_info *pm8001_ha,
> >   		 struct pm8001_device *dev, struct sas_task *task)
> >   {
> >   	struct pm8001_ccb_info *ccb;
> > +	struct request *rq = NULL;
>
> I do not think you need the NULL initialization...

Right, but I will actually need to do it if I change sas_task_find_rq() 
to no deal with NULL task

> >   	u32 tag;
> >   
> > -	if (pm8001_tag_alloc(pm8001_ha, &tag)) {
> > +	rq = sas_task_find_rq(task);
> > +	if (rq) {
> > +		tag = rq->tag;
> > +	} else if (pm8001_tag_alloc(pm8001_ha, &tag)) {
> >   		pm8001_dbg(pm8001_ha, FAIL, "Failed to allocate a tag\n");
> >   		return NULL;
> >   	}

Thanks,
John