linux-kernel - Re: [PATCH 3/4] scsi: pm8001: Use libsas internal abort support

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:   Mon, 7 Mar 2022 11:47:25 +0900
From:   Damien Le Moal <damien.lemoal@...nsource.wdc.com>
To:     John Garry <john.garry@...wei.com>, jejb@...ux.ibm.com,
        martin.petersen@...cle.com, jinpu.wang@...ud.ionos.com
Cc:     linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
        Ajish.Koshy@...rochip.com, linuxarm@...wei.com,
        Viswas.G@...rochip.com, hch@....de, liuqi115@...wei.com,
        chenxiang66@...ilicon.com
Subject: Re: [PATCH 3/4] scsi: pm8001: Use libsas internal abort support

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