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

John Garry
2022-09-29T07:42:01Z

>> 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