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: dLeUCSy3p6hCkS+rdwtjRRICbcXma4aXL7qZr3J5w/j0iIncIxirKI4Eskz3URbmokhqZx6rYP
 h8RgpOQgGbEnEHjpJr1C2ATmi11CS1Mm9oGjmzft91yLAQAUH5Bjgyn+PSECoL/q0whIF4gRjc
 3GgLrJWuwvqI3Hn+kvmvk/l7PNMwyu2I+WePNWnB2QJnCl1QVM8IX75g9A0kW/f/uu1l4Lg01z
 OdJ59NKgTHG4r2VteCQScziTBDa2GQJQF+UtzU6l2q1cvtCrKvLdwNxhRlF1LzJTNQ7k772G0a
 GoHL4+K2osufkSUp0+tlV9aq
IronPort-SDR: KBYLt6Z+ojWYOT/z2hhr/eMTcuFVSpN2QbbqLbSDH0xp5yXmo+4/tbMuySNRxE4bmoDJNOsBDH
 TPCxXbT9osnSGX+RsG/7CU9eMrlprOg9PFTEHI+ZADCPWgCOPsiJeu2fnBS+5UB7a8uVqY0aQd
 m4FJt1fuuVdTn1rLL53aS+8+3F5L+Giv652h8ab5d3mKNPjIP8PG6ohCcaQjWwX9Go8tHbC3vH
 CuXwqZeK6o5Z2/qlT9dsR5YYxed+58XbktMvcjRMCLPw+hs4AlqUfBvdhiJc+JiYzNurTtUj6R
 iUE=
WDCIronportException: Internal
Message-ID: <d5145a6b-0985-030a-0288-1f7853b518d4@opensource.wdc.com>
Date:   Mon, 7 Mar 2022 11:47:25 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH 3/4] scsi: pm8001: Use libsas internal abort support
Content-Language: en-US
To:     John Garry <john.garry@huawei.com>, jejb@linux.ibm.com,
        martin.petersen@oracle.com, jinpu.wang@cloud.ionos.com
Cc:     linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
        Ajish.Koshy@microchip.com, linuxarm@huawei.com,
        Viswas.G@microchip.com, hch@lst.de, liuqi115@huawei.com,
        chenxiang66@hisilicon.com
References: <1646309930-138960-1-git-send-email-john.garry@huawei.com>
 <1646309930-138960-4-git-send-email-john.garry@huawei.com>
 <85a33515-043d-00f4-3bd3-ecb9a1349a68@opensource.wdc.com>
 <966a1048-cd14-d796-8b9d-734605796652@huawei.com>
From:   Damien Le Moal <damien.lemoal@opensource.wdc.com>
Organization: Western Digital Research
In-Reply-To: <966a1048-cd14-d796-8b9d-734605796652@huawei.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 3/4/22 18:41, John Garry wrote:
>>>   /**
>>>     * pm8001_queue_command - register for upper layer used, all IO commands sent
>>>     * to HBA are from this interface.
>>> @@ -379,16 +428,15 @@ int pm8001_queue_command(struct sas_task *task, gfp_t gfp_flags)
>>>   	enum sas_protocol task_proto = task->task_proto;
>>>   	struct domain_device *dev = task->dev;
>>>   	struct pm8001_device *pm8001_dev = dev->lldd_dev;
>>> +	bool internal_abort = sas_is_internal_abort(task);
>>>   	struct pm8001_hba_info *pm8001_ha;
>>>   	struct pm8001_port *port = NULL;
>>>   	struct pm8001_ccb_info *ccb;
>>> -	struct sas_tmf_task *tmf = task->tmf;
>>> -	int is_tmf = !!task->tmf;
>>>   	unsigned long flags;
>>>   	u32 n_elem = 0;
>>>   	int rc = 0;
>>>   
>>> -	if (!dev->port) {
>>> +	if (!internal_abort && !dev->port) {
>>>   		ts->resp = SAS_TASK_UNDELIVERED;
>>>   		ts->stat = SAS_PHY_DOWN;
>>>   		if (dev->dev_type != SAS_SATA_DEV)
>>> @@ -410,7 +458,8 @@ int pm8001_queue_command(struct sas_task *task, gfp_t gfp_flags)
>>>   	pm8001_dev = dev->lldd_dev;
>>>   	port = &pm8001_ha->port[sas_find_local_port_id(dev)];
>>>   
>>> -	if (DEV_IS_GONE(pm8001_dev) || !port->port_attached) {
>>> +	if (!internal_abort && (DEV_IS_GONE(pm8001_dev) ||
>>> +				!port->port_attached)) {
>> Wrapping the line after "&&" would make this a lot cleaner and easier to read.
> 
> Agreed, I can do it.
> 
> But if you can see any neater ways to skip these checks for internal 
> abort in the common queue command path then I would be glad to hear it.

Would need to check the locking context, but if there is no race
possible with the context setting the device as gone, libata should
already be aware of it and not issuing the command in the first place.
So these checks should not be needed at all.

Will try to have a look when I have some time.

There are several things that needs better integration with libata
anyway, like the "manual" read log on error. We should try to address
these for 5.19.


-- 
Damien Le Moal
Western Digital Research