Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
IronPort-SDR: bW1j+aO897BQFD/NWZ3XZjLMXp/uIJnXU79fL/66DZGagI0Sz0jjMrtQXumEubxEZe7G0j25wA
 aw2vvttWQa0YR5OlPgwbTygqU3hHhtCwmfbIWET7WXzNUw5GQQWqau3x5IXSZtxl+Xxg0VJPVV
 /8fX7IYN9nn2a6lisxak2PKA2dVduBwuDoAFHtkmfrL5l3XZSqXScD1UZvvGqA9v4R82jHkrFN
 UdJLLuVV+lwl5tCYRDk2fRk3rbultvzpOM/uqh0OdlxqTOBk/lapkjbaM4uopak2R8IpFzLaIZ
 x7KkU/YPstnB9cls8IhgQ0c2
IronPort-SDR: GT+oglLuk0Rq9vSv2qGmGH0hDpgrjzDceyIMiLSOCH/LAO9A9fpgmwAu4Y/CEoGGb3zIcj451D
 JGJJSfs6GVUNYGyA0ZeYKvC1bbByWg9mXFr8Oj4IHtuaJOpw2JCsYetFJbkfSk39/tP6D1+jZA
 Uj9CrXZ9Qz2yd5oB5A3LE2Z87+hWAzq7PWQw5ovOj64r2LWr19qw2kUzbDwCukPPzBYpaldszH
 VQj44+yHJKp2AXKCytz4QkmiyAMEbhXTM9f4ca2TVWK+WUyCczOj+dMvEjgTM7HaPRJ9JPDS3D
 lV4=
WDCIronportException: Internal
Message-ID: <e8d20a5d-8ca6-b1ff-20be-fb0c782345ca@opensource.wdc.com>
Date:   Thu, 29 Sep 2022 17:02:21 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.0
Subject: Re: [PATCH 6/6] scsi: mvsas: Use sas_task_find_rq() for tagging
Content-Language: en-US
To:     John Garry <john.garry@huawei.com>, jejb@linux.ibm.com,
        martin.petersen@oracle.com, jinpu.wang@cloud.ionos.com,
        damien.lemoal@wdc.com
Cc:     hare@suse.de, linux-scsi@vger.kernel.org,
        linux-kernel@vger.kernel.org, linuxarm@huawei.com,
        ipylypiv@google.com, changyuanl@google.com, hch@lst.de
References: <1664368034-114991-1-git-send-email-john.garry@huawei.com>
 <1664368034-114991-7-git-send-email-john.garry@huawei.com>
 <53e304b4-c025-e884-c8f5-6c2e96cc0052@opensource.wdc.com>
 <e4aa7b2b-3fab-14f9-8af5-8b4c37afb13f@huawei.com>
From:   Damien Le Moal <damien.lemoal@opensource.wdc.com>
Organization: Western Digital Research
In-Reply-To: <e4aa7b2b-3fab-14f9-8af5-8b4c37afb13f@huawei.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 9/29/22 16:49, John Garry wrote:
> On 29/09/2022 03:22, Damien Le Moal wrote:
>> On 9/28/22 21:27, John Garry wrote:
>>> The request associated with a scsi command coming from the block layer
>>> has a unique tag, so use that when possible for getting a slot.
>>>
>>> Unfortunately we don't support reserved commands in the SCSI midlayer yet.
>>> As such, SMP tasks - as an example - will not have a request associated, so
>>> in the interim continue to manage those tags for that type of sas_task
>>> internally.
>>>
>>> We reserve an arbitrary 4 tags for these internal tags. Indeed, we already
>>> decrement MVS_RSVD_SLOTS by 2 for the shost can_queue when flag
>>> MVF_FLAG_SOC is set. This change was made in commit 20b09c2992fef
>>> ("[PATCH] [SCSI] mvsas: add support for 94xx; layout change; bug fixes"),
>>> but what those 2 slots are used for is not obvious.
>>>
>>> Signed-off-by: John Garry <john.garry@huawei.com>
>>> ---
>>>   drivers/scsi/mvsas/mv_defs.h |  1 +
>>>   drivers/scsi/mvsas/mv_init.c |  4 ++--
>>>   drivers/scsi/mvsas/mv_sas.c  | 22 +++++++++++++++++-----
>>>   drivers/scsi/mvsas/mv_sas.h  |  1 -
>>>   4 files changed, 20 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/scsi/mvsas/mv_defs.h b/drivers/scsi/mvsas/mv_defs.h
>>> index 7123a2efbf58..8ef174cd4d37 100644
>>> --- a/drivers/scsi/mvsas/mv_defs.h
>>> +++ b/drivers/scsi/mvsas/mv_defs.h
>>> @@ -40,6 +40,7 @@ enum driver_configuration {
>>>   	MVS_ATA_CMD_SZ		= 96,	/* SATA command table buffer size */
>>>   	MVS_OAF_SZ		= 64,	/* Open address frame buffer size */
>>>   	MVS_QUEUE_SIZE		= 64,	/* Support Queue depth */
>>> +	MVS_RSVD_SLOTS		= 4,
>>>   	MVS_SOC_CAN_QUEUE	= MVS_SOC_SLOTS - 2,
>>>   };
>>>   
>>> diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
>>> index c85fb812ad43..d834ed9e8e4a 100644
>>> --- a/drivers/scsi/mvsas/mv_init.c
>>> +++ b/drivers/scsi/mvsas/mv_init.c
>>> @@ -284,7 +284,7 @@ static int mvs_alloc(struct mvs_info *mvi, struct Scsi_Host *shost)
>>>   			printk(KERN_DEBUG "failed to create dma pool %s.\n", pool_name);
>>>   			goto err_out;
>>>   	}
>>> -	mvi->tags_num = slot_nr;
>>> +	mvi->tags_num = MVS_RSVD_SLOTS;
>>
>> Same comment as for pm8001: do you really need this field if the value
>> is always MVS_RSVD_SLOTS ?
> 
> Right, I don't need this struct member. Again I can just use this macro 
> directly.
> 
>>
>>>   
>>>   	return 0;
>>>   err_out:
>>> @@ -367,7 +367,7 @@ static struct mvs_info *mvs_pci_alloc(struct pci_dev *pdev,
>>>   	mvi->sas = sha;
>>>   	mvi->shost = shost;
>>>   
>>> -	mvi->tags = kzalloc(MVS_CHIP_SLOT_SZ>>3, GFP_KERNEL);
>>> +	mvi->tags = kzalloc(MVS_RSVD_SLOTS, GFP_KERNEL);
>>
>> Field name ? reserved_tags ?
>> Also, the alloc seems wrong. This will allocate 4 bytes, but you only
>> need 4 bits. You could make this an unsigned long and not allocate
>> anything. 
> 
> Well spotted. I should have questioned more why they had >>3 previously.
> 
> But I would rather keep as a bitmap, i.e. *unsigned long for simplicity.>
>> Same remark for pm8001 by the way.
> 
> I think it's ok as it uses bitmap_zalloc()

Yes !

> 
>>
>> That would cap MVS_RSVD_SLOTS to BITS_PER_LONG maximum, but that is easy
>> to check at compile time with a #if/#error.
>>
> 
> As above, I'd rather keep as a bitmap. It's a little inefficient, but is 
> a one off in the driver.
> 
> Thanks,
> John
> 
> 

-- 
Damien Le Moal
Western Digital Research