Re: [PATCH v3 5/5] scsi: ufs: Allow UFS host drivers to override the sg entry size

From: Eric Biggers <ebiggers@kernel.org>
To: Bart Van Assche <bvanassche@acm.org>
Cc: "Martin K . Petersen" <martin.petersen@oracle.com>,
	Jaegeuk Kim <jaegeuk@kernel.org>,
	linux-scsi@vger.kernel.org,
	Adrian Hunter <adrian.hunter@intel.com>,
	"James E.J. Bottomley" <jejb@linux.ibm.com>,
	Bean Huo <beanhuo@micron.com>, Avri Altman <avri.altman@wdc.com>,
	Jinyoung Choi <j-young.choi@samsung.com>,
	Geert Uytterhoeven <geert@linux-m68k.org>,
	Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>,
	Stanley Chu <stanley.chu@mediatek.com>,
	Keoseong Park <keosung.park@samsung.com>,
	Kiwoong Kim <kwmad.kim@samsung.com>
Subject: Re: [PATCH v3 5/5] scsi: ufs: Allow UFS host drivers to override the sg entry size
Date: Wed, 9 Nov 2022 10:24:31 -0800	[thread overview]
Message-ID: <Y2vwX3XlibvDi70/@sol.localdomain> (raw)
In-Reply-To: <20221108233339.412808-6-bvanassche@acm.org>

On Tue, Nov 08, 2022 at 03:33:39PM -0800, Bart Van Assche wrote:
> diff --git a/drivers/ufs/host/Kconfig b/drivers/ufs/host/Kconfig
> index 4cc2dbd79ed0..49017abdac92 100644
> --- a/drivers/ufs/host/Kconfig
> +++ b/drivers/ufs/host/Kconfig
> @@ -124,3 +124,13 @@ config SCSI_UFS_EXYNOS
>  
>  	  Select this if you have UFS host controller on Samsung Exynos SoC.
>  	  If unsure, say N.
> +
> +config SCSI_UFS_VARIABLE_SG_ENTRY_SIZE
> +	bool "Variable size UTP physical region descriptor"
> +	help
> +	  In the UFSHCI 3.0 standard the Physical Region Descriptor (PRD) is a
> +	  data structure used for transferring data between host and UFS
> +	  device. This data structure describes a single region in physical
> +	  memory. Although the standard requires that this data structure has a
> +	  size of 16 bytes, for some controllers this data structure has a
> +	  different size. Enable this option for UFS controllers that need it.

This shouldn't be a user-selectable option.  Just make it be enabled
automatically when it is needed.  Like this:

config SCSI_UFS_VARIABLE_SG_ENTRY_SIZE
	bool
	default y if SCSI_UFS_EXYNOS && SCSI_UFS_CRYPTO

Also, this patch doesn't make sense without the code that actually needs it, so
I hope you plan to send that upstream too.  I haven't been able to do so yet,
because no platform with this hardware actually can run the upstream kernel at
all, as far as I know.  Maybe commit 06874015327 ("arm64: dts: exynos: Add
initial device tree support for Exynos7885 SoC") changed that?  Any suggestions
would be greatly appreciated...  How are you testing this?

> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index bd45818bf0e8..c6854514e40e 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -754,6 +754,9 @@ struct ufs_hba_monitor {
>   * @vops: pointer to variant specific operations
>   * @vps: pointer to variant specific parameters
>   * @priv: pointer to variant specific private data
> +#ifdef CONFIG_SCSI_UFS_VARIABLE_SG_ENTRY_SIZE
> + * @sg_entry_size: size of struct ufshcd_sg_entry (may include variant fields)
> +#endif
>   * @irq: Irq number of the controller

It doesn't make sense to have an #ifdef in the middle of a comment.

- Eric