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 C1609E77188 for ; Tue, 14 Jan 2025 09:55:51 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 574646B0083; Tue, 14 Jan 2025 04:55:51 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 524A76B0089; Tue, 14 Jan 2025 04:55:51 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3EC686B0093; Tue, 14 Jan 2025 04:55:51 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 224A06B0083 for ; Tue, 14 Jan 2025 04:55:51 -0500 (EST) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id BD233140813 for ; Tue, 14 Jan 2025 09:55:50 +0000 (UTC) X-FDA: 83005600860.23.2723EB4 Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) by imf08.hostedemail.com (Postfix) with ESMTP id F403816000A for ; Tue, 14 Jan 2025 09:55:47 +0000 (UTC) Authentication-Results: imf08.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf08.hostedemail.com: domain of jonathan.cameron@huawei.com designates 185.176.79.56 as permitted sender) smtp.mailfrom=jonathan.cameron@huawei.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1736848549; 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=m9UVEeOHTWbkiWj0ISEa73dJgpuy6/8RQ9AUQFi68U0=; b=2EKhWsAAoJF2BYS36JhS/2J0Ik2ru6ghFUSqazA2mqg+UWHQQ8npORM/ouaQrSL269B9sS Anp0uF1HUzX6yy+JAj4eXb7JXneiiBYBg47WvG9YWY8KDJF4++Q73TWgbxMG/aCsqTtDmB 7nqD8QM1+u4ZXaEjr4zXmC8qEtI250Y= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1736848549; a=rsa-sha256; cv=none; b=n8g2+pHWNh4SmkJQIGoaUIGtx6di4D2nL5xXx7K2xkb6mDtGRaQWbU2DFWyvHQxD8fdl+C k5uVuvR/3WEucIYryasZeX9tRZRM+r33otiPHxqjwPDIwxWPA1+tafX2XIm70kXaJUn9gz 6xs3hGw9y5bJxT8BYH8cyd6h3ZYN7uc= ARC-Authentication-Results: i=1; imf08.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf08.hostedemail.com: domain of jonathan.cameron@huawei.com designates 185.176.79.56 as permitted sender) smtp.mailfrom=jonathan.cameron@huawei.com Received: from mail.maildlp.com (unknown [172.18.186.231]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4YXPbY1PMRz6LDKg; Tue, 14 Jan 2025 17:54:01 +0800 (CST) Received: from frapeml500008.china.huawei.com (unknown [7.182.85.71]) by mail.maildlp.com (Postfix) with ESMTPS id 9D0DE140B3C; Tue, 14 Jan 2025 17:55:44 +0800 (CST) Received: from localhost (10.203.177.66) by frapeml500008.china.huawei.com (7.182.85.71) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.39; Tue, 14 Jan 2025 10:55:42 +0100 Date: Tue, 14 Jan 2025 09:55:41 +0000 From: Jonathan Cameron To: Mauro Carvalho Chehab CC: , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , Subject: Re: [PATCH v18 01/19] EDAC: Add support for EDAC device features control Message-ID: <20250114095541.000000a1@huawei.com> In-Reply-To: <20250113160611.39bdf3b3@foz.lan> References: <20250106121017.1620-1-shiju.jose@huawei.com> <20250106121017.1620-2-shiju.jose@huawei.com> <20250113160611.39bdf3b3@foz.lan> X-Mailer: Claws Mail 4.3.0 (GTK 3.24.42; x86_64-w64-mingw32) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.203.177.66] X-ClientProxiedBy: lhrpeml500002.china.huawei.com (7.191.160.78) To frapeml500008.china.huawei.com (7.182.85.71) X-Rspamd-Queue-Id: F403816000A X-Rspam-User: X-Rspamd-Server: rspam07 X-Stat-Signature: nti4xrogarhyhowz48y8na1pbxmhscfq X-HE-Tag: 1736848547-861273 X-HE-Meta: U2FsdGVkX192XpKwgjcmX/Z8oU6D5Xw86wjlfxSeC5btHBi8Et/yyXvcunher2hlKe7il2fnLUJu52+89VzfFvGhHIqhPcWiFIVS0g+DJoqy/8LEFCuua3o22LXNkk4c8Ucz+32zFfHM0gE5aLhJ2urAB+NGvUZPaeHTzveZPd0DseK4JKzlsh7DAkqA9BZT3k77KiAUO/vaUvwqt/JoK5dchQlfAvu9VK+mTyDbVFzWtD405E0Pw76gJi+VGaRG8FhP8KqNH2RO4W15Lqh5zFNzZMW5fcbYEZAvfOIflhVpV01pGHrLc3JidopHfxTP35qv1vfaLBY/hj8ET61I/25q/rdSbQZ5TYCSBQribDKVHNvQdDmUiSbM7VIMyipzu4F5JGcP9+JyQ6J0dZ7H9hE95yCB+OwAGMd+d3wl/5vAsYqOUmw5UIZ8irqRnP2yPVJBuC3CpY6ZgcJCOAuiyAZ7HF3fnpHv2rUskiZFbi/W0nckxZ2Vl5bDi4V61Aw4pDMQQCUJ6jk96phR9xqd5po0tsGn8caO7460VA6NqHCcUWOGsw6MfLjV6lgU4sVyY48do+aXX72xrTizKXm9AOEwzW0Cr17+7MzyRv8gqCzBG2cxDBVCy2WPGcX297WX1iFl0Wvtyv+GaUgJ7iBmoFpOrKjQWUqVd7Jj2MPzDZiLTy5FWvBm4dLxJc8c6hsXimYlMotBVZoYFYk6AUamsKzZ4jGYl2H2DIiPIUOA/iHPXT+5wWQtwz3ijDqXM7Iwu6ekhXJpI7zbgU22GEwER9yIhDAEjeEQmeKHmEqnsnJNKHDI6C0BaLnlzzpD2QnPA1wxW9xjpuRnOR203JBRLK3jPeGEO1wADvVFgC+577I21DelgzjpB7FCbbAPU2t5ROmsTzOadn0G+/oSAie/Ou6RrnrfetE3PD1hDAw1yCYQtBBsUkAzboCkU63gtPLVRQpWDaSOGPryAUgcFpw e3oZESEu oa6o9QmocC1DmHrFHcWMLhPpd8y7U7j+HZvnrnLSr6FfdUDdPWGtJ0FSXtzV6b5yF7yXIB3RIVzdjdfm9c3WWvRP6T4HTBtqwHawm5GwyIg9KKzExcOaiP73VziXEFSMUILPLhRD7tiH1P8kKYjd0tDrR9l0PBm2XosKGq69aJ5h58VZdBQJrXqH+LFca3K6xBrc3fLcBzkmofGSG0iZDz4/Wmw== 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: > > +int edac_dev_register(struct device *parent, char *name, > > + void *private, int num_features, > > + const struct edac_dev_feature *ras_features) > > +{ > > + const struct attribute_group **ras_attr_groups; > > + struct edac_dev_feat_ctx *ctx; > > + int attr_gcnt = 0; > > + int ret, feat; > > + > > + if (!parent || !name || !num_features || !ras_features) > > + return -EINVAL; > > + > > + /* Double parse to make space for attributes */ > > + for (feat = 0; feat < num_features; feat++) { > > + switch (ras_features[feat].ft_type) { > > + /* Add feature specific code */ > > + default: > > + return -EINVAL; > > + } > > + } > > + > > + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); > > + if (!ctx) > > + return -ENOMEM; > > + > > + ras_attr_groups = kcalloc(attr_gcnt + 1, sizeof(*ras_attr_groups), GFP_KERNEL); > > + if (!ras_attr_groups) { > > + ret = -ENOMEM; > > + goto ctx_free; > > + } > > + > > + attr_gcnt = 0; > > + for (feat = 0; feat < num_features; feat++, ras_features++) { > > + switch (ras_features->ft_type) { > > + /* Add feature specific code */ > > + default: > > + ret = -EINVAL; > > + goto groups_free; > > + } > > + } > > + > > + ctx->dev.parent = parent; > > + ctx->dev.bus = edac_get_sysfs_subsys(); > > + ctx->dev.type = &edac_dev_type; > > + ctx->dev.groups = ras_attr_groups; > > + ctx->private = private; > > + dev_set_drvdata(&ctx->dev, ctx); > > + > > + ret = dev_set_name(&ctx->dev, name); > > + if (ret) > > + goto groups_free; > > + > > + ret = device_register(&ctx->dev); > > + if (ret) { > > + put_device(&ctx->dev); > > > + return ret; > > As register failed, you need to change it to a goto groups_free, > as edac_dev_release() won't be called. Boris called this one out as well, so seems it is not that well understood. I've also tripped over this in the past and it's one of the most common things I catch in reviews of code calling this stuff. As discussed offline, it will be called. The device_register() docs make it clear that whether or not that call succeeds reference counting is enabled and put_device() is the correct way to free resources. The actual depends on the fact that device_register() is just a helper defined as device_initialize(); return device_add(); So for reasons lost to history (I guess there are cases where other cleanup needs to happen before the release) it does not handle side effects of device_initialize() on an error in device_add(). device_initialize() has called -> kobject_init(&dev->kobj, &device_type); -> kref_init_internal(kobj) + sets ktype (which has the release callback) kref_init_internal() sets the reference counter to 1 Hence when we do a device_put() in the error path, the reference counter drops to 0 and the release from the ktype is called. Here that is edac_dev_release(); If you want to verify replace device_register() with device_initialize() then call put_device(). If we were going back in history, I'd suggest device_register() should be side effect free and call put_device() on error and any driver that needs to handle other stuff before the release should just not use it. I guess that ship long sailed and maybe there are other reasons I've not thought of. I took a quick look and seems to go back into at least the 2.5 era. Jonathan