Re: [RFC PATCH v1 34/57] sata_sil24: Remove PAGE_SIZE compile-time constant assumption

[Niklas Cassel <cassel@kernel.org>]

Hello Ryan,

While I realize that this has not always been consistent,
please prefix the subject with "ata: ", so that it becomes
"ata: sata_sil24: ".

On Mon, Oct 14, 2024 at 11:58:41AM +0100, Ryan Roberts wrote:
> To prepare for supporting boot-time page size selection, refactor code
> to remove assumptions about PAGE_SIZE being compile-time constant. Code
> intended to be equivalent when compile-time page size is active.
> 
> Convert "struct sil24_ata_block" and "struct sil24_atapi_block" to use a
> flexible array member for their sge[] array. The previous static size of
> SIL24_MAX_SGE depends on PAGE_SIZE so doesn't work for boot-time page
> size.
> 
> Wrap global variables that are initialized with PAGE_SIZE derived values
> using DEFINE_GLOBAL_PAGE_SIZE_VAR() so their initialization can be
> deferred for boot-time page size builds.
> 
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> ---
> 
> ***NOTE***
> Any confused maintainers may want to read the cover note here for context:
> https://lore.kernel.org/all/20241014105514.3206191-1-ryan.roberts@arm.com/
> 
>  drivers/ata/sata_sil24.c | 46 +++++++++++++++++++---------------------
>  1 file changed, 22 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
> index 72c03cbdaff43..85c6382976626 100644
> --- a/drivers/ata/sata_sil24.c
> +++ b/drivers/ata/sata_sil24.c
> @@ -42,26 +42,25 @@ struct sil24_sge {
>  	__le32	flags;
>  };
>  
> +/*
> + * sil24 fetches in chunks of 64bytes.  The first block
> + * contains the PRB and two SGEs.  From the second block, it's
> + * consisted of four SGEs and called SGT.  Calculate the
> + * number of SGTs that fit into one page.
> + */
> +#define SIL24_PRB_SZ	(sizeof(struct sil24_prb) + 2 * sizeof(struct sil24_sge))
> +#define SIL24_MAX_SGT	((PAGE_SIZE - SIL24_PRB_SZ) / (4 * sizeof(struct sil24_sge)))
> +
> +/*
> + * This will give us one unused SGEs for ATA.  This extra SGE
> + * will be used to store CDB for ATAPI devices.
> + */
> +#define SIL24_MAX_SGE	(4 * SIL24_MAX_SGT + 1)
>  
>  enum {
>  	SIL24_HOST_BAR		= 0,
>  	SIL24_PORT_BAR		= 2,
>  
> -	/* sil24 fetches in chunks of 64bytes.  The first block
> -	 * contains the PRB and two SGEs.  From the second block, it's
> -	 * consisted of four SGEs and called SGT.  Calculate the
> -	 * number of SGTs that fit into one page.
> -	 */
> -	SIL24_PRB_SZ		= sizeof(struct sil24_prb)
> -				  + 2 * sizeof(struct sil24_sge),
> -	SIL24_MAX_SGT		= (PAGE_SIZE - SIL24_PRB_SZ)
> -				  / (4 * sizeof(struct sil24_sge)),
> -
> -	/* This will give us one unused SGEs for ATA.  This extra SGE
> -	 * will be used to store CDB for ATAPI devices.
> -	 */
> -	SIL24_MAX_SGE		= 4 * SIL24_MAX_SGT + 1,
> -
>  	/*
>  	 * Global controller registers (128 bytes @ BAR0)
>  	 */
> @@ -244,13 +243,13 @@ enum {
>  
>  struct sil24_ata_block {
>  	struct sil24_prb prb;
> -	struct sil24_sge sge[SIL24_MAX_SGE];
> +	struct sil24_sge sge[];
>  };
>  
>  struct sil24_atapi_block {
>  	struct sil24_prb prb;
>  	u8 cdb[16];
> -	struct sil24_sge sge[SIL24_MAX_SGE];
> +	struct sil24_sge sge[];
>  };
>  
>  union sil24_cmd_block {
> @@ -373,7 +372,7 @@ static struct pci_driver sil24_pci_driver = {
>  #endif
>  };
>  
> -static const struct scsi_host_template sil24_sht = {
> +static DEFINE_GLOBAL_PAGE_SIZE_VAR_CONST(struct scsi_host_template, sil24_sht, {
>  	__ATA_BASE_SHT(DRV_NAME),
>  	.can_queue		= SIL24_MAX_CMDS,
>  	.sg_tablesize		= SIL24_MAX_SGE,
> @@ -382,7 +381,7 @@ static const struct scsi_host_template sil24_sht = {
>  	.sdev_groups		= ata_ncq_sdev_groups,
>  	.change_queue_depth	= ata_scsi_change_queue_depth,
>  	.device_configure	= ata_scsi_device_configure
> -};
> +});
>  
>  static struct ata_port_operations sil24_ops = {
>  	.inherits		= &sata_pmp_port_ops,
> @@ -1193,7 +1192,7 @@ static int sil24_port_start(struct ata_port *ap)
>  	struct device *dev = ap->host->dev;
>  	struct sil24_port_priv *pp;
>  	union sil24_cmd_block *cb;
> -	size_t cb_size = sizeof(*cb) * SIL24_MAX_CMDS;
> +	size_t cb_size = PAGE_SIZE * SIL24_MAX_CMDS;
>  	dma_addr_t cb_dma;
>  
>  	pp = devm_kzalloc(dev, sizeof(*pp), GFP_KERNEL);
> @@ -1258,7 +1257,6 @@ static void sil24_init_controller(struct ata_host *host)
>  
>  static int sil24_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  {
> -	extern int __MARKER__sil24_cmd_block_is_sized_wrongly;
>  	struct ata_port_info pi = sil24_port_info[ent->driver_data];
>  	const struct ata_port_info *ppi[] = { &pi, NULL };
>  	void __iomem * const *iomap;
> @@ -1266,9 +1264,9 @@ static int sil24_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	int rc;
>  	u32 tmp;
>  
> -	/* cause link error if sil24_cmd_block is sized wrongly */
> -	if (sizeof(union sil24_cmd_block) != PAGE_SIZE)
> -		__MARKER__sil24_cmd_block_is_sized_wrongly = 1;
> +	/* union sil24_cmd_block must be PAGE_SIZE */
> +	BUG_ON(struct_size_t(struct sil24_atapi_block, sge, SIL24_MAX_SGE) != PAGE_SIZE);
> +	BUG_ON(struct_size_t(struct sil24_ata_block, sge, SIL24_MAX_SGE) > PAGE_SIZE);
>  
>  	ata_print_version_once(&pdev->dev, DRV_VERSION);
>  
> -- 
> 2.43.0
> 

As you might know, there is an effort to annotate all flexible array
members with their run-time size information, see commit:
dd06e72e68bc ("Compiler Attributes: Add __counted_by macro")

I haven't looked at the DEFINE_GLOBAL_PAGE_SIZE_VAR_CONST macro, but since
sge[] now becomes a flexible array member, I think it would be nice if it
would be possible to somehow use the __counted_by macro.

Other than that, this looks good to me.


Kind regards,
Niklas