From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f49.google.com (mail-wm1-f49.google.com [209.85.128.49]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 7C33D2D46D0 for ; Sun, 7 Dec 2025 23:33:08 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.49 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765150391; cv=none; b=HhkdEE7/NBY4O6ymVFndcLJAVIyqMQQuAf4qwky9juiaSlan0E0t3945K2zkvmCwhZIA4IXau1Vi6e3z1GUmwxIIeS+ZKWSyXf2X9MueN6zOv4GceGrM7xTscY/cYH9VP3cjehzEfVr3OkSz6Q8jwFTRMGTaGd+9BGPz3eamzg4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765150391; c=relaxed/simple; bh=IUBnPdBepuQxqsyw6VOLxWUVxW1uVa8REohoNzbmBWo=; h=Message-ID:Date:From:To:Cc:Subject:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=LfsV6R3JT1S6Wf44UJO0RIr+utrlRTEAWHsZxUkxIqNpL2jZjpLBKvr6acCTxiNuWt/vabwvSg8SHtpV7WY9eL5xciRY/do5AtyQkXBrjRkljkLrvc/B+pkqRHRoYtcugHVKGxiBl4BskucBPe2XSRnbDZpjN4GFRFNMsz9TgD0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=gORCP0hA; arc=none smtp.client-ip=209.85.128.49 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="gORCP0hA" Received: by mail-wm1-f49.google.com with SMTP id 5b1f17b1804b1-477619f8ae5so30528805e9.3 for ; Sun, 07 Dec 2025 15:33:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1765150387; x=1765755187; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:subject:cc :to:from:date:message-id:from:to:cc:subject:date:message-id:reply-to; bh=/1QB5AMwL5JyG53A9J+/NYEsONb+MuJ9XPqo/Mr2oxg=; b=gORCP0hAz6Va25M9MkwcviQejiNn/y4H7ldALaHZMeANruMpSVvJLpz/0cDdTml9HC FitQJ6tfE/K6cLfvJBF8ZtqXfeZ/0DVWUfvGs5tasOhb0JXIi2A3kSESZM8pxp+hXgMV psy8K/NHtHmlIxR02Td1KsQDmCwO8oB3LiTKRytDyOdLkgFoSzQTpO1k+yDCTk1C1SH5 vBblJ1zXnirxVfWhTJi4vjyIvnAIDoZiogcTV2yNPGjRP4GW9rzcBDeBiI+T/03aYDuj IlXuSJWaalSV8QFUQ/jcMGUYz9MIymPcgLXvvKCVuEruhHB97hXU2CK+LUsiKbCNPWGr y8PQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1765150387; x=1765755187; h=in-reply-to:content-disposition:mime-version:references:subject:cc :to:from:date:message-id:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=/1QB5AMwL5JyG53A9J+/NYEsONb+MuJ9XPqo/Mr2oxg=; b=trOzNsilxOAdqWhhpyziT8JysOiB4ko7zFF4y2oBoJsGsR5mnLlpGJh5fUren4ETk7 yPXKGnlWFSZIwE82ln74QzlrYzxjJRRQ5HMMX6HwfovGtOPoDREvPfAtcCHyMiH52F+U RNur6/N8LkECXDaoy9xJsQW0Ht63L+onQrdgZV+bIG1Nt0yec2jChbTc7rUUVRU6Mbyi 4x0p8UHJvEwvbfvRLBJNDXRBTeZvJw4IA+xNqqiV9Y+jiw+kDZWKx3gYKOeO+KNAEzz8 /0dqcxbv3ORdhYDgoaJSDarQdlOwcI1oHaVEZea4PW1OGpR8ZMGP0ifZWme13i3QqWJ3 JSPg== X-Forwarded-Encrypted: i=1; AJvYcCU2pUJnbTS1j5qv4zUvb7AQRcNwL5nzo1rzWLWKSbBpwnzgzd/2VBnabMHWpJcFdcRnElPQ0MzfPffZFY4=@vger.kernel.org X-Gm-Message-State: AOJu0YxzgGvMR9ujslOQb+QOV43dhXA3DuYXdQdZRbiFrZUPcVa2pmQq LB/A8caNturk3GRN7T7SoWreufdcFMBc8YB0+eNpXkQsefhTuZIXU/+b X-Gm-Gg: ASbGnctotH1K8UZz0nYAheVwOK8bLdvM/rMmJ9uujb0cGeyxpuEwHYINW2K/m584oqY fJHUHmPA6u7mRnom5O39Q2YO5kBxHRn/mYTUNfFTxjtm3E4LNHZKi7J9q3Fejjeg/+Z+BYQScUo QtbJWjP9cyc1hFE7EMIfEHpPp9mdzftt6z/YKcj5RyLqGH6sEiDWdfHr7HHZTQycVkQtj8016yX QOaDSQttkskV2aU/JF1HwLzJ8zy9Vak/0soFO56zbgUbqVu1aNhz6AxwvZDfMYSga8j9+zV3e3K UiUSCYjzC/ujHJuKZow703wLmhHCqapfo3+tAYZNVYUfNO667Rzr5QpE98t6/6naD6w2AanShl9 SvfNIBRduATlbvsOhHMi1Cuu5Oo+WtznqWoJBEonJkSNCUicUq0wrFg6RXbgkr3cxr5uQESROvI s9oMJfJsjRsV8FPV/ICuPyCRnfNdLWnKKGejpOR0ngOsXyKPHHNg== X-Google-Smtp-Source: AGHT+IGLUNEk+nz4eZd0cxsG7HFG7mM99SnnzoeXPdxW5LC2ztuNSH2YjTiMmtNxdHU5VMl4lTRNzw== X-Received: by 2002:a05:600c:3508:b0:477:89d5:fdb2 with SMTP id 5b1f17b1804b1-47939dfcb2amr75354295e9.14.1765150386730; Sun, 07 Dec 2025 15:33:06 -0800 (PST) Received: from Ansuel-XPS. (93-34-88-81.ip49.fastwebnet.it. [93.34.88.81]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-47930c90e67sm209800585e9.12.2025.12.07.15.33.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 07 Dec 2025 15:33:06 -0800 (PST) Message-ID: <69360eb2.050a0220.2b0cf3.b5a8@mx.google.com> X-Google-Original-Message-ID: Date: Mon, 8 Dec 2025 00:33:03 +0100 From: Christian Marangi To: Andy Shevchenko Cc: Andrew Morton , Dan Williams , Jonathan Cameron , Magnus Damm , linux-kernel@vger.kernel.org, "Rob Herring (Arm)" , stable@vger.kernel.org Subject: Re: [PATCH] resource: handle wrong resource_size value on zero start/end resource References: <20251207215359.28895-1-ansuelsmth@gmail.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: On Mon, Dec 08, 2025 at 01:23:37AM +0200, Andy Shevchenko wrote: > On Mon, Dec 08, 2025 at 01:12:08AM +0200, Andy Shevchenko wrote: > > On Sun, Dec 07, 2025 at 10:53:48PM +0100, Christian Marangi wrote: > > > Commit 900730dc4705 ("wifi: ath: Use > > > of_reserved_mem_region_to_resource() for "memory-region"") uncovered a > > > massive problem with the usage of resource_size() helper. > > > > > > The reported commit caused a regression with ath11k WiFi firmware > > > loading and the change was just a simple replacement of duplicate code > > > with a new helper of_reserved_mem_region_to_resource(). > > > > > > On reworking this, in the commit also a check for the presence of the > > > node was replaced with resource_size(&res). This was done following the > > > logic that if the node wasn't present then it's expected that also the > > > resource_size is zero, mimicking the same if-else logic. > > > > > > This was also the reason the regression was mostly hard to catch at > > > first sight as the rework is correctly done given the assumption on the > > > used helpers. > > > > > > BUT this is actually not the case. On further inspection on > > > resource_size() it was found that it NEVER actually returns 0. > > Actually this not true. Obviously if the end == start - 1, it will return 0. > So, you really need _carefully_ check users one-by-one and see how resource > is filled, before judging the test. It might or might not be broken. Each > case is individual, but the observation you made is quite valuable, thanks! > Yes sure there are case where it can return zero but are there real world scenario like that in the context of resource_size for PCI or resouce for MMIO? Again the idea of this patch was to start searching for error instead of simply fixing ath11k, I'm pretty sure there are other case that are currently working by luck. Another idea might be to introduce a new helper and add all kind of checks to understand if the resource we are testing is all zero. Something like resource_is_zero() that checks if start end and flags are all zero? (and fix all the case where the helper might be used in a wrong way?) Or maybe we can change the condition of this to: if (!res.flags && !res.start && !res.end) return 0; Just putting some ideas on what would be the proper solution to the problem without having to analyze all the 990 case where the helper is used ehehehhe > > > Even if the resource value of start and end are 0, the return value of > > > resource_size() will ALWAYS be 1, resulting in the broken if-else > > > condition ALWAYS going in the first if condition. > > > > > > This was simply confirmed by reading the resource_size() logic: > > > > > > return res->end - res->start + 1; > > > > > > Given the confusion, also other case of such usage were searched in the > > > kernel and with great suprise it seems LOTS of place assume > > > resource_size() should return zero in the context of the resource start > > > and end set to 0. > > > > > > Quoting for example comments in drivers/vfio/pci/vfio_pci_core.c: > > > > > > /* > > > * The PCI core shouldn't set up a resource with a > > > * type but zero size. But there may be bugs that > > > * cause us to do that. > > > */ > > > if (!resource_size(res)) > > > goto no_mmap; > > > > > > It really seems resource_size() was tought with the assumption that > > > resource struct was always correctly initialized before calling it and > > > never set to zero. > > > > > > But across the year this got lost and now there are lots of driver that > > > assume resource_size() returns 0 if start and end are also 0. > > > > > > To better handle this and make resource_size() returns correct value in > > > such case, add a simple check and return 0 if both resource start and > > > resource end are zero. > > > > Good catch! > > > > Now, let's unveil which drivers rely on "broken" behaviour... > > > > ... > > > > > static inline resource_size_t resource_size(const struct resource *res) > > > { > > > + if (!res->start && !res->end) > > > + return 0; > > > > I think this breaks or might brake some of the drivers that rely on the proper > > calculation. If you supply the start and end for the same (if it's not 0), you > > will get 1 and it's _correct_ result (surprise surprise). One of the thing that > > may be directly affected (and regress) is the amount of IRQs calculation (which > > on some platforms may start from 0). However, in practice I think it's none > > nowadays in the upstream kernel. > > > > > return res->end - res->start + 1; > > > } > > > > That said, unfortunately, I think, you want to fix drivers one-by-one and this > > patch is incorrect as it brings inconsistency to the logic (1 occupied address > > whatever unit it has may still be valid resource). > > > > Also a good start is to add test cases and add/update documentation. > > -- > With Best Regards, > Andy Shevchenko > > -- Ansuel