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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 04036D37480 for ; Thu, 17 Oct 2024 12:42:29 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4A3406B007B; Thu, 17 Oct 2024 08:42:29 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 453696B0082; Thu, 17 Oct 2024 08:42:29 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3427A6B0083; Thu, 17 Oct 2024 08:42:29 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 13BDE6B007B for ; Thu, 17 Oct 2024 08:42:29 -0400 (EDT) Received: from smtpin01.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id DC1E2C1152 for ; Thu, 17 Oct 2024 12:42:16 +0000 (UTC) X-FDA: 82683057156.01.9EF3DBB Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf14.hostedemail.com (Postfix) with ESMTP id 95BC410000B for ; Thu, 17 Oct 2024 12:42:15 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=none; spf=pass (imf14.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com; dmarc=pass (policy=none) header.from=arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1729168898; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=HZiDCVFOuxxzr5P8U5CBm9plVkAnHkSqEVWe2EZOhms=; b=NaexgDPm2kVRujiUVhB9WOj4E2LZP+x3rYCbmY6rsybQ3++5isa1DAwpjjqvhDW2mBE5v3 RKpa9ckN1GPvA7XhjfclwP0eY5nX5PhRSCUlvFoenJ3JTAWonDvuVf43jvM6PHLwGmvb9X V8fmKLCsMUwv8Q/Y3mRaWlrCqP4z79c= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=none; spf=pass (imf14.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com; dmarc=pass (policy=none) header.from=arm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1729168898; a=rsa-sha256; cv=none; b=LVYhrwEGADoZga5TCNcVYr6b0XVNIXXljFrZs3Mp8VrEAi6gXdWuxYrbLPiCuGO0cMFgHI ecWRIwfk/lbIBtMbL2ZAVNaO+jqK6D888BW4tCqpGcuCevTXaPIqxTcw/kxE6jST0pZrlj 8JqHFVjbAIOdrvG6ApBXmc9yFfVbfJY= 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-Rspam-User: X-Stat-Signature: mbyrhnxz3tdxjib7eeob7szrucfwofzw X-Rspamd-Queue-Id: 95BC410000B X-Rspamd-Server: rspam11 X-HE-Tag: 1729168935-412147 X-HE-Meta: U2FsdGVkX19E163jvGlLzj6DmaZnb4ah4EkIMYFZGRefMnFBOts4RZ20kmtv5PMHwin80xT1RGlEtRzqunEGIpP1AAQmOA6+hd4+lrUrMZ5Pkf3vQtqUz5fM+Zow3IIVLClqtcaoPHqLDbU/KWZKqRqFVb8d1UUliKpfkpkeWTUKqnzmbGzSVu6cwm8aq4uoeRLfp6UAIeix+lS+88lbvnXXSXeN+RX17+9z2MIsTKGNTMeN/p2aSiJg95PpXSgdlWjSZcd4nwVCYXXqJczxxuzT6cK4yGOZLPkvEp55XBEMDVhAZjfumWtMAxfDckOdk0EwSSD+A/4X4HAU0jwEV2WxVGeMz7KzH2HefTysiwTI8B9HxQvEj/NFSK6N2/CE2WhGqDs/Q93c4D+rE1xWXzEgQrjoaeHfSTgxHCEZYMkFDmwEzRSYgZ4wYoEnpgU9T8UMPU/9qUrhxmbyYeeuhWSA4iuCTMyzbhRFQLCyTw0liasQdcn2vq79XbIpRcUk0DFvqtaohjapCGjC0SQUfSdK04/8V4hIdFBxor+2rfVWHisAuk4X6fwcWGBloAPdFM4t239o8B0lxUeTG/q8dJ+9L0ufOCPEh+dUr01dgSXjmWogH2N7nV+zoR0A+OyWNyv8/HL2oGSevUuFCyxDzRB+5ksLXaBarJP4L3/wvK0b5w80tx3jNBfkR3C/TsDiNw0E0i9ue+dtVtVG0V445bQhFQWohNN+efLWjW6fZQa9gUL15vQEX9+weVEvHIRxkH2etpLOIFaMQX6rlmfShQgBJGczE+a8VoqY1BuYsiopA/TO5YmTn3/jsbftM91pXhSfs5ytuDgx5QU9e0MHoVrRcujnDNRqCTkGevsS4pugFDiMHBXdSr5a4KgPAkRcWvJFRKFfxIhD45lVIPnW668G06O+IpvB6tdjAF8S6veQLdXTnIg19+PS3czJdnTJayfOtC/eNUnxEHD4CVF 4OvrmEgT 22Nq9qxhUbeI1JDRralUnzK/GagRCPv28x4IewwaMS6kW4iWpr5Q943idHZjEjNyA3DtqZXJUpo0OX8bJWdQYXqoEVD/N8bVM+mDi1Y6cN380jlnEGql7zm+hJGgZekDcR0eZIJmghOTp5rZg6+sW96be8agoyXdweRxsDroo2OLJIyDcrtHXFQc87U+hzW55qZOVcfSQEpAO7zEXGWwbaWS/Aujfz6qnaJFf/YdvvHSZovbzkzR3hs1+Mxz4/lUZxZ19ZmaYHKNiHVJvhUMPiTKSpA== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: 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