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 46C29C021A9 for ; Mon, 17 Feb 2025 18:22:51 +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-Type:MIME-Version: Message-ID:Date:References:In-Reply-To:Subject:Cc:To:From:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=WYdQMpmWfwfRPZrNOq/JI6U8Fyr73ByGTD+9Af1nU7I=; b=cCPFJRzvwlhA9FyYB/+71rlts+ p8NCf4WI1y/VfqKWYPicFnGM5PYqdbnL8N1ac9ltebzFb4AWHdf0pmF7OfnyG/NTV/YxKZ8ZQa/xq Lis5OAeIuxKByZROv6j4ql5ZQypVoAknBtcERYGxxJYOYqPoKKADn4iH02IMC1O/SD7aUzOj13uMi pHXnzpaxcP5IJxAyp/6Qekb55JXoazJI5QMxOceA/oOesCtDTCeY7pxINzDrNIzzMdK9Qsu2t18jL /JQbfmDX71QCJMM5xGWjp0omqfEyBLpcGEdQuqPm7TbX7G0GaucX/muWOuQM1+ZJxyex7+sm3z2yy lZkF5NPw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1tk5lP-00000005ahG-2z9c; Mon, 17 Feb 2025 18:22:39 +0000 Received: from mail-wr1-x435.google.com ([2a00:1450:4864:20::435]) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1tk5a8-00000005ZHR-0fyX for linux-arm-kernel@lists.infradead.org; Mon, 17 Feb 2025 18:11:02 +0000 Received: by mail-wr1-x435.google.com with SMTP id ffacd0b85a97d-38f488f3161so517846f8f.3 for ; Mon, 17 Feb 2025 10:10:59 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre-com.20230601.gappssmtp.com; s=20230601; t=1739815858; x=1740420658; darn=lists.infradead.org; h=mime-version:message-id:date:user-agent:references:in-reply-to :subject:cc:to:from:from:to:cc:subject:date:message-id:reply-to; bh=WYdQMpmWfwfRPZrNOq/JI6U8Fyr73ByGTD+9Af1nU7I=; b=gcc8N/XPvKrfVMvrJ5ObQpRi+keSIMVacJZVCJxaPwKNWM1rvtYnijFU69DchoH0TY 8WP5edx6dUA7lSznjr7GBAiVzD6qltDu+IC9plIFHFZq+qBwjoCbdb9K/x9pXRJCVcmF f3Wi/QRzR0NYhOxmNibPZMX6HYJMsFBEorygjcCVBvqgPACfJDrTON9xArwQoRABqlkQ y6HUN32LLcE1n1BxGnFxyZk5V4Mo7iPSwq/HdHLqMd20fbZy1sW57OydlGNCfv/nPwCD Vs3FGio0oEno+11dga/jc/0oiHCA8SllDSnc3HS8fcl11o+5QthSG6SaPstFoOP0lK6M Q2PQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1739815858; x=1740420658; h=mime-version:message-id:date:user-agent:references:in-reply-to :subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=WYdQMpmWfwfRPZrNOq/JI6U8Fyr73ByGTD+9Af1nU7I=; b=oh7Y3473vWdRoboNYoixDAvrALswpt3j95cTo12cncrajc4eF03mBVIorqcrayNtFP DnAxB4/RMh9P+GwoMkbUwj1uVhus4SohClCCoZgk752/sO5OiTYtRGSrqcTctNeEe5bh UxEFkRg+jyNjGKc1igRANdJZk7ikulR5EAyBZuHVwiqyzwA5jj/980LXyozwa3HNEUzh TOg+MiHDBvzvSlnuYlzQqrPopK1tsFntZpvpqhh+GtQNX5oXMzZbcNA5U1PfVrltTta0 3t53eWDQh0aVR+KTDfo2HsdhRStZsrNAVqIcnfIUvvyu9TNvF4SEFcjrzcQscMWdz9DN o8jg== X-Forwarded-Encrypted: i=1; AJvYcCVmcQVCRPGdFs256/XV98q9AEM+aPdikpcQe6RU6miuifoSWZNAXFfChCq4i98Dd56Krx0jGDIocLmwjApOZANH@lists.infradead.org X-Gm-Message-State: AOJu0YwgTititsDCvV/wyMjD7mWdQ+9UM/+8EsHk27zDDGF8xWDoW2Es WOz8F08SGDp+3nL0hW4cETpWhZIr/Zm+v3j8NTyXAjN/XrqIp3m2XgnYrqIAUlg= X-Gm-Gg: ASbGnctNv9ViBPNHYamib07VHqjkqWsaJe4Sa2+6rGARJGuuDUH6+wVehY+5K120YZC B36KSxj4Q9GuS7pHOk4UnwS8pqq5k1qcEpJyr0fcadR0NeHhM6YLZiB4YF9k/6zBCSBNPVOrdVk Wj29zCU/0wkd/H/dPH723mOQdWjewOlJ5zl3WZvrLU9jIk7Hn80oTAkJGsN9zIy3yvfwOlww8+K 7rlXwXb4yIea9zQl72pZRRWZbv948bPPcY6bwJGTEff5jIiHm0xjGuxi01S+lkK8ea7Eo1t//Jf 0O4jTc6PXIVRTQ== X-Google-Smtp-Source: AGHT+IFTAKZ9BquOVY29sEYMChLDnuqHcqVlSqR4NiZlMjD4bUMe9qRI5EU9r/6iyIf02WCX+Y2KzA== X-Received: by 2002:a5d:4cca:0:b0:38a:2b34:e13e with SMTP id ffacd0b85a97d-38f33f28c01mr7768806f8f.18.1739815856721; Mon, 17 Feb 2025 10:10:56 -0800 (PST) Received: from localhost ([2a01:e0a:3c5:5fb1:a529:8667:e9bd:7f7]) by smtp.gmail.com with UTF8SMTPSA id 5b1f17b1804b1-4395afaf052sm155075695e9.13.2025.02.17.10.10.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 17 Feb 2025 10:10:55 -0800 (PST) From: Jerome Brunet To: Greg Kroah-Hartman Cc: Dave Ertman , Ira Weiny , "Rafael J. Wysocki" , Stephen Boyd , Arnd Bergmann , Danilo Krummrich , Conor Dooley , Daire McNamara , Philipp Zabel , Douglas Anderson , Andrzej Hajda , Neil Armstrong , Robert Foss , Laurent Pinchart , Jonas Karlman , Jernej Skrabec , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Simona Vetter , Hans de Goede , Ilpo =?utf-8?Q?J=C3=A4rvinen?= , Bryan O'Donoghue , Vladimir Kondratiev , Gregory CLEMENT , =?utf-8?Q?Th=C3=A9o?= Lebrun , Michael Turquette , Abel Vesa , Peng Fan , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam , Kevin Hilman , Martin Blumenstingl , linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org, dri-devel@lists.freedesktop.org, platform-driver-x86@vger.kernel.org, linux-mips@vger.kernel.org, linux-clk@vger.kernel.org, imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org, linux-amlogic@lists.infradead.org Subject: Re: [PATCH v3 1/7] driver core: auxiliary bus: add device creation helpers In-Reply-To: <2025021501-tamer-sank-142a@gregkh> (Greg Kroah-Hartman's message of "Sat, 15 Feb 2025 07:53:12 +0100") References: <20250211-aux-device-create-helper-v3-0-7edb50524909@baylibre.com> <20250211-aux-device-create-helper-v3-1-7edb50524909@baylibre.com> <2025021437-washout-stonewall-d13e@gregkh> <1jwmdsxugx.fsf@starbuckisacylon.baylibre.com> <2025021501-tamer-sank-142a@gregkh> User-Agent: mu4e 1.12.8; emacs 29.4 Date: Mon, 17 Feb 2025 19:10:54 +0100 Message-ID: <1jikp8xx01.fsf@starbuckisacylon.baylibre.com> MIME-Version: 1.0 Content-Type: text/plain X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20250217_101100_197121_578B6E9D X-CRM114-Status: GOOD ( 20.73 ) 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 Sat 15 Feb 2025 at 07:53, Greg Kroah-Hartman wrote: [...] >> >> > >> >> + int id) >> >> +{ >> >> + struct auxiliary_device *auxdev; >> >> + int ret; >> >> + >> >> + auxdev = kzalloc(sizeof(*auxdev), GFP_KERNEL); >> >> + if (!auxdev) >> >> + return ERR_PTR(-ENOMEM); >> > >> > Ick, who cares what the error value really is? Why not just do NULL or >> > a valid pointer? That makes the caller much simpler to handle, right? >> > >> >> Sure why not I have tried the 'NULL or valid' approach. In the consumers, which mostly return an integer from their various init function, I got this weird to come up with one from NULL. EINVAL, ENOMEM, etc ... can't really pick one. It is actually easier to pass something along. >> >> >> + >> >> + auxdev->id = id; >> >> + auxdev->name = devname; >> >> + auxdev->dev.parent = dev; >> >> + auxdev->dev.platform_data = platform_data; >> >> + auxdev->dev.release = auxiliary_device_release; >> >> + device_set_of_node_from_dev(&auxdev->dev, dev); >> >> + >> >> + ret = auxiliary_device_init(auxdev); >> > >> > Only way this will fail is if you forgot to set parent or a valid name. >> > So why not check for devname being non-NULL above this? >> >> If auxiliary_device_init() ever changes it would be easy to forget to >> update that and lead to something nasty to debug, don't you think ? > > Yes, this is being more defensive, I take back my objection, thanks. > >> >> + if (ret) { >> >> + kfree(auxdev); >> >> + return ERR_PTR(ret); >> >> + } >> >> + >> >> + ret = __auxiliary_device_add(auxdev, modname); >> >> + if (ret) { >> >> + /* >> >> + * NOTE: It may look odd but auxdev should not be freed >> >> + * here. auxiliary_device_uninit() calls device_put() >> >> + * which call the device release function, freeing auxdev. >> >> + */ >> >> + auxiliary_device_uninit(auxdev); >> > >> > Yes it is odd, are you SURE you should be calling device_del() on the >> > device if this fails? auxiliary_device_uninit(), makes sense so why not >> > just call that here? >> >> I'm confused ... I am call auxiliary_device_uninit() here. What do you >> mean ? > > Oh wow, I got this wrong, sorry, I was thinking you were calling > auxiliary_device_destroy(). Nevermind, ugh, it was a long day... > No worries > thanks, > > greg k-h -- Jerome