linux-kernel - Re: [PATCH v17 03/12] power: reset: reboot-mode: Add support for 64 bit magic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <zejaqakbtufwzlzs7xc7xzxezcylqjkmu4nne2mro4riuhgbkc@hlgu3u2w36bb>
Date: Mon, 10 Nov 2025 12:33:10 -0600
From: Bjorn Andersson <andersson@...nel.org>
To: Shivendra Pratap <shivendra.pratap@....qualcomm.com>
Cc: Mukesh Ojha <mukesh.ojha@....qualcomm.com>, 
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>, Sebastian Reichel <sre@...nel.org>, 
	Rob Herring <robh@...nel.org>, Sudeep Holla <sudeep.holla@....com>, 
	Souvik Chakravarty <Souvik.Chakravarty@....com>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Andy Yan <andy.yan@...k-chips.com>, 
	Mark Rutland <mark.rutland@....com>, Lorenzo Pieralisi <lpieralisi@...nel.org>, 
	Arnd Bergmann <arnd@...db.de>, Konrad Dybcio <konradybcio@...nel.org>, 
	cros-qcom-dts-watchers@...omium.org, Vinod Koul <vkoul@...nel.org>, 
	Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>, 
	Florian Fainelli <florian.fainelli@...adcom.com>, Moritz Fischer <moritz.fischer@...us.com>, 
	John Stultz <john.stultz@...aro.org>, Matthias Brugger <matthias.bgg@...il.com>, 
	Krzysztof Kozlowski <krzk@...nel.org>, Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>, 
	Stephen Boyd <swboyd@...omium.org>, Andre Draszik <andre.draszik@...aro.org>, 
	Kathiravan Thirumoorthy <kathiravan.thirumoorthy@....qualcomm.com>, linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-arm-msm@...r.kernel.org, Elliot Berman <quic_eberman@...cinc.com>, 
	Xin Liu <xin.liu@....qualcomm.com>, Srinivas Kandagatla <srini@...nel.org>, 
	Umang Chheda <umang.chheda@....qualcomm.com>, Nirmesh Kumar Singh <nirmesh.singh@....qualcomm.com>
Subject: Re: [PATCH v17 03/12] power: reset: reboot-mode: Add support for 64
 bit magic

On Mon, Nov 10, 2025 at 11:22:40PM +0530, Shivendra Pratap wrote:
> 
> 
> On 11/10/2025 10:00 PM, Bjorn Andersson wrote:
> > On Mon, Nov 10, 2025 at 07:15:29PM +0530, Mukesh Ojha wrote:
> >> On Sun, Nov 09, 2025 at 08:07:16PM +0530, Shivendra Pratap wrote:
> >>> Current reboot-mode supports a single 32-bit argument for any
> >>> supported mode. Some reboot-mode based drivers may require
> >>> passing two independent 32-bit arguments during a reboot
> >>> sequence, for uses-cases, where a mode requires an additional
> >>> argument. Such drivers may not be able to use the reboot-mode
> >>> driver. For example, ARM PSCI vendor-specific resets, need two
> >>> arguments for its operation – reset_type and cookie, to complete
> >>> the reset operation. If a driver wants to implement this
> >>> firmware-based reset, it cannot use reboot-mode framework.
> >>>
> >>> Introduce 64-bit magic values in reboot-mode driver to
> >>> accommodate dual 32-bit arguments when specified via device tree.
> >>> In cases, where no second argument is passed from device tree,
> >>> keep the upper 32-bit of magic un-changed(0) to maintain backward
> >>> compatibility.
> >>>
> >>> Update the current drivers using reboot-mode for a 64-bit magic
> >>> value.
> 
> [SNIP..]
> 
> >>> +	if (magic > U32_MAX)
> >>> +		return -EINVAL;
> >>> +
> >>> +	magic_32 = magic;
> >>> +
> >>>  	syscon_rbm = container_of(reboot, struct syscon_reboot_mode, reboot);
> >>>  
> >>>  	ret = regmap_update_bits(syscon_rbm->map, syscon_rbm->offset,
> >>> -				 syscon_rbm->mask, magic);
> >>> +				 syscon_rbm->mask, magic_32);
> > 
> > As above, if we're no longer silently discarding bits, I think we should
> > compare the magic against syscon_rbm->mask.
> > 
> > No need for a local variable, just type cast after checking the validity.
> 
> Trying to summarize below why we added these check-
> 
> the patch in v11 used typecasting and did not have any of these checks(link below):
> https://lore.kernel.org/all/20250717-arm-psci-system_reset2-vendor-reboots-v11-2-df3e2b2183c3@oss.qualcomm.com/
> 
> As per below upstream review, type cast was removed and bound checks were added all-over patchset:
> "As a general rule of thumb, code with casts is poor quality code. Try
> to write the code without casts." - 
> https://lore.kernel.org/all/8d4a42b6-657f-4c30-8e25-4213d8d53a89@lunn.ch/
> 
> We can revert to the typecast way. Please suggest.
> 

Okay, I'm okay with Andrew's original request, stick to that for the
nvmem case. Although I don't fancy the name "magic_32", and would prefer
that you just call it "value" or something.


For pon and syscon however, I'm wondering why you have ignored Andrew's
other request from that same email:

"""
You might be able to go further, and validate that magic actually fits
into the field when you consider the << pon->reason_shift.
"""

Writing "if (magic > U32_MAX)" in a snippet of code where magic isn't
allowed to be more than either 32 or 64 is misleading.

For syscon, it's true that the parameter is an unsigned long, but the
actual limit better be based on syscon_rbm->mask.

Regards,
Bjorn

> thanks,
> Shivendra