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

[Eric Biggers <ebiggers@kernel.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