linux-kernel - Re: [PATCH 4/6] scsi: pm8001: Use sas_task_find

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 29 Sep 2022 08:41:14 +0100
From:   John Garry <john.garry@...wei.com>
To:     Damien Le Moal <damien.lemoal@...nsource.wdc.com>,
        <jejb@...ux.ibm.com>, <martin.petersen@...cle.com>,
        <jinpu.wang@...ud.ionos.com>, <damien.lemoal@....com>
CC:     <hare@...e.de>, <linux-scsi@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linuxarm@...wei.com>,
        <ipylypiv@...gle.com>, <changyuanl@...gle.com>, <hch@....de>
Subject: Re: [PATCH 4/6] scsi: pm8001: Use sas_task_find_rq() for tagging


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