From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id DF595D37485 for ; Thu, 17 Oct 2024 12:59:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:Cc:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=HZiDCVFOuxxzr5P8U5CBm9plVkAnHkSqEVWe2EZOhms=; b=SrdWgSTZMr+jUge9clLqJCwSkr N4Smrww/2n5t8FNsT+etc13w+TYRkhxvyUO5fLoUYAtLq2yLTLzFjvnpllpyrx52Jl//KUOu/fUFA +bW7nVhJt3VIlrMPlTZyo891P9SSyKYx2nswxyawv2bYhpPRfKF13lZl0DOl49LM5xcud6dYZFYCo ulnN+6IYXKl4TWIas195BywFJIzZENsa+RlilxB4LP1WgRBg7lAXA1oPsGFjIalmf5fHsLsoRHErQ 5NvELduZH605CWjmoFJJiHB5noN3d8z60HReVEDPuK1F7YZ3vtJmzbVZke2ayIIB3qkZmD9GPCkAt mp2VYfJg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1t1Q6D-0000000EqrV-0Ml1; Thu, 17 Oct 2024 12:59:29 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1t1Ppi-0000000EoIL-3m4u for linux-arm-kernel@lists.infradead.org; Thu, 17 Oct 2024 12:42:28 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 70B1FFEC; Thu, 17 Oct 2024 05:42:55 -0700 (PDT) Received: from [10.1.34.186] (XHFQ2J9959.cambridge.arm.com [10.1.34.186]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 720173F71E; Thu, 17 Oct 2024 05:42:23 -0700 (PDT) Message-ID: <7aa84080-6845-496a-a394-30d334632298@arm.com> Date: Thu, 17 Oct 2024 13:42:22 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [RFC PATCH v1 34/57] sata_sil24: Remove PAGE_SIZE compile-time constant assumption Content-Language: en-GB To: Niklas Cassel Cc: Andrew Morton , Anshuman Khandual , Ard Biesheuvel , Catalin Marinas , Damien Le Moal , David Hildenbrand , Greg Marsden , Ivan Ivanov , Kalesh Singh , Marc Zyngier , Mark Rutland , Matthias Brugger , Miroslav Benes , Will Deacon , linux-arm-kernel@lists.infradead.org, linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, Kees Cook References: <20241014105514.3206191-1-ryan.roberts@arm.com> <20241014105912.3207374-1-ryan.roberts@arm.com> <20241014105912.3207374-34-ryan.roberts@arm.com> From: Ryan Roberts In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20241017_054227_071331_BE0131FA X-CRM114-Status: GOOD ( 35.06 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 17/10/2024 10:09, Niklas Cassel wrote: > 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: ". Noted; I'll fix this in the next version. > > 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 >> --- >> >> ***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'm vaguely aware of it. But as I understand it, __counted_by() nominates another member in the struct which keeps the count? In this case, there is no such member, it's size is implicit based on the value of PAGE_SIZE. So I'm not sure if it's practical to use it here? > > I haven't looked at the DEFINE_GLOBAL_PAGE_SIZE_VAR_CONST macro, but since DEFINE_GLOBAL_PAGE_SIZE_VAR_CONST(), when doing a boot-time page size build, defers the initialization of the global variable to kernel init time, when PAGE_SIZE is known. Because SIL24_MAX_SGE is defined in terms of PAGE_SIZE, this deferral is required. > 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. Thanks for the review! > > > Kind regards, > Niklas