Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: 5WT7cSTrVJAciFHQF53BVlGCifldpedRDoSonAeO6vCjKkP2wE8X77swUeYZeNuPdRrVGVDvzb
 ct2TRZS9heFfE4HDPrKFrHOTWy/9w+pZ1Fvrny6uwZhFC74OE3fDD7FzvHXAXSI44llr0swjik
 J31xc5Uu5aaPHecg7o1/M2DcxGwFFeTBAVItKmtm5kAKJdtIo5XNOVpexCq2AFcguNmqp3R6gU
 tPiPF5HG15kYYrCh7CS7dkH1X1F59M6qNGes9AKU3TUl4EshvJ1WVJVQFsEbS6YnHg26UQKrDT
 WxvWl44QVSGjJi8LWlWeQpGG
IronPort-SDR: Im9fqODW3EMOMVpUWpxhntwLQlvraZUPqxC53GZk7jktxcjG/qiQrY5egt0RF4q7ASqHAr9UoX
 1WDuCeNZZgiSYukSh6Cmw+ivJqyJna0zrtSy/3M7UEUq7Ok5VBJhrihRia01l5tsrT2c7Mezgl
 1rsr81JHz/RjefdPoqD96Kh2ET8Dwh374s7zuH+ZTEEyu9QW2ltZHKf0YEpT3fSMmpmSPM1FQg
 HVerj0X6Imv70YCJrdVUxrGXGa6SVWiJCIpt9z8hmmXR2avkfcbDDl+v26zNGQLaYm5+bFWBiW
 mWo=
WDCIronportException: Internal
Message-ID: <1893d9ef-042b-af3b-74ea-dd4d0210c493@opensource.wdc.com>
Date:   Thu, 27 Jan 2022 15:37:41 +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 00/16] scsi: libsas and users: Factor out LLDD TMF code
Content-Language: en-US
To:     John Garry <john.garry@huawei.com>, jejb@linux.ibm.com,
        martin.petersen@oracle.com, artur.paszkiewicz@intel.com,
        jinpu.wang@cloud.ionos.com, chenxiang66@hisilicon.com,
        Ajish.Koshy@microchip.com
Cc:     yanaijie@huawei.com, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org,
        linuxarm@huawei.com, liuqi115@huawei.com, Viswas.G@microchip.com
References: <1643110372-85470-1-git-send-email-john.garry@huawei.com>
From:   Damien Le Moal <damien.lemoal@opensource.wdc.com>
Organization: Western Digital Research
In-Reply-To: <1643110372-85470-1-git-send-email-john.garry@huawei.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 1/25/22 20:32, John Garry wrote:
> The LLDD TMF code is almost identical between hisi_sas, pm8001, and mvsas
> drivers.
> 
> This series factors out that code into libsas, thus reducing much
> duplication and giving a net reduction of ~250 LoC.
> 
> There are some subtle differences between the core TMF handler and each
> of the LLDDs old implementation, so any review and testing is appreciated.
> 
> Some other minor patches are thrown in:
> - Delete unused macro in hisi_sas driver
> - Delete unused libsas callback
> - Add enum for response frame datapres field
> 
> I have another follow-up series to factor out the internal abort code,
> which is common to hisi_sas and pm8001 drivers.
> 
> Based on v5.17-rc1
> 
> John Garry (16):
>   scsi: libsas: Use enum for response frame DATAPRES field
>   scsi: libsas: Delete lldd_clear_aca callback
>   scsi: hisi_sas: Delete unused I_T_NEXUS_RESET_PHYUP_TIMEOUT
>   scsi: libsas: Move SMP task handlers to core
>   scsi: libsas: Add struct sas_tmf_task
>   scsi: libsas: Add sas_task.tmf
>   scsi: libsas: Add sas_execute_tmf()
>   scsi: libsas: Add sas_execute_ssp_tmf()
>   scsi: libsas: Add TMF handler exec complete callback
>   scsi: libsas: Add TMF handler aborted callback
>   scsi: libsas: Add sas_abort_task_set()
>   scsi: libsas: Add sas_clear_task_set()
>   scsi: libsas: Add sas_lu_reset()
>   scsi: libsas: Add sas_query_task()
>   scsi: libsas: Add sas_abort_task()
>   scsi: libsas: Add sas_execute_ata_cmd()
> 
>  Documentation/scsi/libsas.rst          |   2 -
>  drivers/scsi/aic94xx/aic94xx.h         |   1 -
>  drivers/scsi/aic94xx/aic94xx_init.c    |   1 -
>  drivers/scsi/aic94xx/aic94xx_tmf.c     |   9 -
>  drivers/scsi/hisi_sas/hisi_sas.h       |   9 +-
>  drivers/scsi/hisi_sas/hisi_sas_main.c  | 235 ++++---------------------
>  drivers/scsi/hisi_sas/hisi_sas_v1_hw.c |   2 +-
>  drivers/scsi/hisi_sas/hisi_sas_v2_hw.c |   9 +-
>  drivers/scsi/hisi_sas/hisi_sas_v3_hw.c |   2 +-
>  drivers/scsi/isci/init.c               |   1 -
>  drivers/scsi/isci/task.c               |  18 --
>  drivers/scsi/isci/task.h               |   4 -
>  drivers/scsi/libsas/sas_ata.c          |   8 +
>  drivers/scsi/libsas/sas_expander.c     |  24 +--
>  drivers/scsi/libsas/sas_internal.h     |   6 +
>  drivers/scsi/libsas/sas_scsi_host.c    | 220 +++++++++++++++++++++++
>  drivers/scsi/libsas/sas_task.c         |  12 +-
>  drivers/scsi/mvsas/mv_defs.h           |   5 -
>  drivers/scsi/mvsas/mv_init.c           |   5 +-
>  drivers/scsi/mvsas/mv_sas.c            | 177 +------------------
>  drivers/scsi/mvsas/mv_sas.h            |   3 -
>  drivers/scsi/pm8001/pm8001_hwi.c       |   4 +-
>  drivers/scsi/pm8001/pm8001_init.c      |   4 +-
>  drivers/scsi/pm8001/pm8001_sas.c       | 180 +++----------------
>  drivers/scsi/pm8001/pm8001_sas.h       |  13 +-
>  include/scsi/libsas.h                  |  23 ++-
>  26 files changed, 353 insertions(+), 624 deletions(-)
> 

John,

I did some light testing of this series (boot + some fio runs) and
everything looks good using my "ATTO Technology, Inc. ExpressSAS 12Gb/s
SAS/SATA HBA (rev 06)" HBA (x86_64 host).

Of note is that "make W=1 M=drivers/scsi" complains with:

drivers/scsi/pm8001/pm80xx_hwi.c:3938: warning: Function parameter or
member 'circularQ' not described in 'process_one_iomb'

And sparse/make C=1 complains about:

drivers/scsi/libsas/sas_port.c:77:13: warning: context imbalance in
'sas_form_port' - different lock contexts for basic block

But I have not checked if it is something that your series touch.

And there is a ton of complaints about __le32 use in the pm80xx code...
I can try to have a look at these if you want, on top of your series.

Cheers.

-- 
Damien Le Moal
Western Digital Research