Re: [PATCH 00/16] scsi: libsas and users: Factor out LLDD TMF code

From: Damien Le Moal <damien.lemoal@opensource.wdc.com>
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
Subject: Re: [PATCH 00/16] scsi: libsas and users: Factor out LLDD TMF code
Date: Thu, 27 Jan 2022 15:37:41 +0900	[thread overview]
Message-ID: <1893d9ef-042b-af3b-74ea-dd4d0210c493@opensource.wdc.com> (raw)
In-Reply-To: <1643110372-85470-1-git-send-email-john.garry@huawei.com>

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