Messages in this thread | | | Date | Thu, 29 Sep 2022 08:41:14 +0100 | Subject | Re: [PATCH 4/6] scsi: pm8001: Use sas_task_find_rq() for tagging | From | 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
| |