[PATCH 1/3] err.h: add ERR_PTR_CONST macro

[PATCH 1/3] err.h: add ERR_PTR_CONST macro

[Christian Marangi]

Add ERR_PTR_CONST macro to initialize global variables with error
pointers. This might be useful for specific case where there is a global
variables initialized to an error condition and then later set to the
real handle once probe finish/completes.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 include/linux/err.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/err.h b/include/linux/err.h
index 1d60aa86db53..a38071881b20 100644
--- a/include/linux/err.h
+++ b/include/linux/err.h
@@ -41,6 +41,14 @@ static inline void * __must_check ERR_PTR(long error)
 	return (void *) error;
 }
 
+/**
+ * ERR_PTR_CONST - define a const error pointer.
+ * @error: A negative error code.
+ *
+ * Like ERR_PTR(), but usable to initialize global variables.
+ */
+#define ERR_PTR_CONST(error) ((void *)(error))
+
 /* Return the pointer in the percpu address space. */
 #define ERR_PTR_PCPU(error) ((void __percpu *)(unsigned long)ERR_PTR(error))
 
-- 
2.51.0

[PATCH 2/3] soc: qcom: smem: better track SMEM uninitialized state

[Christian Marangi]

There is currently a problem where, in the specific case of SMEM not
initialized by SBL, any SMEM API wrongly returns PROBE_DEFER
communicating wrong info to any user of this API.

A better way to handle this would be to track the SMEM state and return
a different kind of error than PROBE_DEFER.

Rework the __smem handle to always init it to the error pointer
-EPROBE_DEFER following what is already done by the SMEM API.
If we detect that the SBL didn't initialized SMEM, set the __smem handle
to the error pointer -ENODEV.
Also rework the SMEM API to handle the __smem handle to be an error
pointer and return it appropriately.

This way user of the API can react and return a proper error or use
fallback way for the failing API.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/soc/qcom/smem.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index 592819701809..d6136369262a 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -353,8 +353,12 @@ static void *cached_entry_to_item(struct smem_private_entry *e)
 	return p - le32_to_cpu(e->size);
 }
 
-/* Pointer to the one and only smem handle */
-static struct qcom_smem *__smem;
+/*
+ * Pointer to the one and only smem handle.
+ * Init to -EPROBE_DEFER to signal SMEM still has to be probed.
+ * Can be set to -ENODEV if SMEM is not initialized by SBL.
+ */
+static struct qcom_smem *__smem = ERR_PTR_CONST(-EPROBE_DEFER);
 
 /* Timeout (ms) for the trylock of remote spinlocks */
 #define HWSPINLOCK_TIMEOUT	1000
@@ -508,8 +512,8 @@ int qcom_smem_alloc(unsigned host, unsigned item, size_t size)
 	unsigned long flags;
 	int ret;
 
-	if (!__smem)
-		return -EPROBE_DEFER;
+	if (IS_ERR(__smem))
+		return PTR_ERR(__smem);
 
 	if (item < SMEM_ITEM_LAST_FIXED) {
 		dev_err(__smem->dev,
@@ -685,10 +689,10 @@ static void *qcom_smem_get_private(struct qcom_smem *smem,
 void *qcom_smem_get(unsigned host, unsigned item, size_t *size)
 {
 	struct smem_partition *part;
-	void *ptr = ERR_PTR(-EPROBE_DEFER);
+	void *ptr;
 
-	if (!__smem)
-		return ptr;
+	if (IS_ERR(__smem))
+		return __smem;
 
 	if (WARN_ON(item >= __smem->item_count))
 		return ERR_PTR(-EINVAL);
@@ -723,8 +727,8 @@ int qcom_smem_get_free_space(unsigned host)
 	struct smem_header *header;
 	unsigned ret;
 
-	if (!__smem)
-		return -EPROBE_DEFER;
+	if (IS_ERR(__smem))
+		return PTR_ERR(__smem);
 
 	if (host < SMEM_HOST_COUNT && __smem->partitions[host].virt_base) {
 		part = &__smem->partitions[host];
@@ -1182,6 +1186,7 @@ static int qcom_smem_probe(struct platform_device *pdev)
 	if (le32_to_cpu(header->initialized) != 1 ||
 	    le32_to_cpu(header->reserved)) {
 		dev_err(&pdev->dev, "SMEM is not initialized by SBL\n");
+		__smem = ERR_PTR(-ENODEV);
 		return -EINVAL;
 	}
 
-- 
2.51.0

Re: [PATCH 2/3] soc: qcom: smem: better track SMEM uninitialized state

[Andy Shevchenko]

On Wed, Oct 29, 2025 at 02:33:20PM +0100, Christian Marangi wrote:
> There is currently a problem where, in the specific case of SMEM not
> initialized by SBL, any SMEM API wrongly returns PROBE_DEFER
> communicating wrong info to any user of this API.
> 
> A better way to handle this would be to track the SMEM state and return
> a different kind of error than PROBE_DEFER.
> 
> Rework the __smem handle to always init it to the error pointer
> -EPROBE_DEFER following what is already done by the SMEM API.
> If we detect that the SBL didn't initialized SMEM, set the __smem handle
> to the error pointer -ENODEV.
> Also rework the SMEM API to handle the __smem handle to be an error
> pointer and return it appropriately.

...

>  	if (le32_to_cpu(header->initialized) != 1 ||
>  	    le32_to_cpu(header->reserved)) {
>  		dev_err(&pdev->dev, "SMEM is not initialized by SBL\n");
> +		__smem = ERR_PTR(-ENODEV);
>  		return -EINVAL;
>  	}

I find this a bit confusing. Why the error code returned to the upper layer is
different to the stored one?

...

Also, the series of patches should include the cover letter to explain not only
series background but additionally
- how it should be applied
- if it has dependencies
- etc

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] soc: qcom: smem: better track SMEM uninitialized state

[Christian Marangi]

On Wed, Oct 29, 2025 at 05:27:33PM +0200, Andy Shevchenko wrote:
> On Wed, Oct 29, 2025 at 02:33:20PM +0100, Christian Marangi wrote:
> > There is currently a problem where, in the specific case of SMEM not
> > initialized by SBL, any SMEM API wrongly returns PROBE_DEFER
> > communicating wrong info to any user of this API.
> > 
> > A better way to handle this would be to track the SMEM state and return
> > a different kind of error than PROBE_DEFER.
> > 
> > Rework the __smem handle to always init it to the error pointer
> > -EPROBE_DEFER following what is already done by the SMEM API.
> > If we detect that the SBL didn't initialized SMEM, set the __smem handle
> > to the error pointer -ENODEV.
> > Also rework the SMEM API to handle the __smem handle to be an error
> > pointer and return it appropriately.
> 
> ...
> 
> >  	if (le32_to_cpu(header->initialized) != 1 ||
> >  	    le32_to_cpu(header->reserved)) {
> >  		dev_err(&pdev->dev, "SMEM is not initialized by SBL\n");
> > +		__smem = ERR_PTR(-ENODEV);
> >  		return -EINVAL;
> >  	}
> 
> I find this a bit confusing. Why the error code returned to the upper layer is
> different to the stored one?
>

It's INVAL for probe. But for any user of SMEM it's NODEV as there isn't
an actual SMEM usable.

Totally ok to change the error condition in probe if maybe NODEV is
better suited. I assume there isn't a specific pattern of the correct
error condition in probe.

> ...
> 
> Also, the series of patches should include the cover letter to explain not only
> series background but additionally
> - how it should be applied
> - if it has dependencies
> - etc
> 

Didn't add one they are trivial patch but I can add it if needed... it's
pretty stable code so no dependency or branch target

> 
> 

-- 
	Ansuel

Re: [PATCH 2/3] soc: qcom: smem: better track SMEM uninitialized state

[Bjorn Andersson]

On Wed, Oct 29, 2025 at 04:32:35PM +0100, Christian Marangi wrote:
> On Wed, Oct 29, 2025 at 05:27:33PM +0200, Andy Shevchenko wrote:
> > On Wed, Oct 29, 2025 at 02:33:20PM +0100, Christian Marangi wrote:
> > > There is currently a problem where, in the specific case of SMEM not
> > > initialized by SBL, any SMEM API wrongly returns PROBE_DEFER
> > > communicating wrong info to any user of this API.
> > > 
> > > A better way to handle this would be to track the SMEM state and return
> > > a different kind of error than PROBE_DEFER.
> > > 
> > > Rework the __smem handle to always init it to the error pointer
> > > -EPROBE_DEFER following what is already done by the SMEM API.
> > > If we detect that the SBL didn't initialized SMEM, set the __smem handle
> > > to the error pointer -ENODEV.
> > > Also rework the SMEM API to handle the __smem handle to be an error
> > > pointer and return it appropriately.
> > 
> > ...
> > 
> > >  	if (le32_to_cpu(header->initialized) != 1 ||
> > >  	    le32_to_cpu(header->reserved)) {
> > >  		dev_err(&pdev->dev, "SMEM is not initialized by SBL\n");
> > > +		__smem = ERR_PTR(-ENODEV);
> > >  		return -EINVAL;
> > >  	}
> > 
> > I find this a bit confusing. Why the error code returned to the upper layer is
> > different to the stored one?
> >
> 
> It's INVAL for probe. But for any user of SMEM it's NODEV as there isn't
> an actual SMEM usable.
> 
> Totally ok to change the error condition in probe if maybe NODEV is
> better suited. I assume there isn't a specific pattern of the correct
> error condition in probe.
> 

I'd say ENODEV represents the error better than EINVAL, so I don't have
any concerns with you changing the return value.

> > ...
> > 
> > Also, the series of patches should include the cover letter to explain not only
> > series background but additionally
> > - how it should be applied
> > - if it has dependencies
> > - etc
> > 
> 
> Didn't add one they are trivial patch but I can add it if needed... it's
> pretty stable code so no dependency or branch target
> 

Specifically, I should merge patch 1 and 2 through the qcom/soc tree,
and patch 3 can be merged completely independently through the cpufreq
tree.

Regards,
Bjorn

> > 
> > 
> 
> -- 
> 	Ansuel

[PATCH 3/3] cpufreq: qcom-nvmem: add compatible fallback for ipq806x for no SMEM

[Christian Marangi]

On some IPQ806x SoC SMEM might be not initialized by SBL. This is the
case for some Google devices (the OnHub family) that can't make use of
SMEM to detect the SoC ID.

To handle these specific case, check if the SMEM is not initialized (by
checking if the qcom_smem_get_soc_id returns -ENODEV) and fallback to
OF machine compatible checking to identify the SoC variant.

Notice that the checking order is important as the machine compatible
are normally defined with the specific one following the generic SoC.
(for example compatible = "qcom,ipq8065", "qcom,ipq8064")

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/cpufreq/qcom-cpufreq-nvmem.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
index 3a8ed723a23e..c88a79a177b1 100644
--- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
+++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
@@ -257,8 +257,8 @@ static int qcom_cpufreq_ipq8064_name_version(struct device *cpu_dev,
 					     char **pvs_name,
 					     struct qcom_cpufreq_drv *drv)
 {
+	int msm_id = -1, ret = 0;
 	int speed = 0, pvs = 0;
-	int msm_id, ret = 0;
 	u8 *speedbin;
 	size_t len;
 
@@ -275,8 +275,21 @@ static int qcom_cpufreq_ipq8064_name_version(struct device *cpu_dev,
 	get_krait_bin_format_a(cpu_dev, &speed, &pvs, speedbin);
 
 	ret = qcom_smem_get_soc_id(&msm_id);
-	if (ret)
-		goto exit;
+	if (ret) {
+		if (ret != -ENODEV)
+			goto exit;
+
+		/* Fallback to compatible match with no SMEM initialized */
+		if (of_machine_is_compatible("qcom,ipq8062"))
+			msm_id = QCOM_ID_IPQ8062;
+		else if (of_machine_is_compatible("qcom,ipq8065") ||
+			 of_machine_is_compatible("qcom,ipq8069"))
+			msm_id = QCOM_ID_IPQ8065;
+		else if (of_machine_is_compatible("qcom,ipq8064") ||
+			 of_machine_is_compatible("qcom,ipq8066") ||
+			 of_machine_is_compatible("qcom,ipq8068"))
+			msm_id = QCOM_ID_IPQ8064;
+	}
 
 	switch (msm_id) {
 	case QCOM_ID_IPQ8062:
-- 
2.51.0

Re: [PATCH 3/3] cpufreq: qcom-nvmem: add compatible fallback for ipq806x for no SMEM

[Andy Shevchenko]

On Wed, Oct 29, 2025 at 02:33:21PM +0100, Christian Marangi wrote:
> On some IPQ806x SoC SMEM might be not initialized by SBL. This is the
> case for some Google devices (the OnHub family) that can't make use of
> SMEM to detect the SoC ID.
> 
> To handle these specific case, check if the SMEM is not initialized (by
> checking if the qcom_smem_get_soc_id returns -ENODEV) and fallback to
> OF machine compatible checking to identify the SoC variant.
> 
> Notice that the checking order is important as the machine compatible
> are normally defined with the specific one following the generic SoC.
> (for example compatible = "qcom,ipq8065", "qcom,ipq8064")

Misplaced period, should be at the end of closing parenthesis.

...

>  	ret = qcom_smem_get_soc_id(&msm_id);
> -	if (ret)
> -		goto exit;
> +	if (ret) {
> +		if (ret != -ENODEV)
> +			goto exit;

	if (ret == ...) {
		...
	} else if (ret)
		goto exit;

Even patch will look better after that.

> +		/* Fallback to compatible match with no SMEM initialized */
> +		if (of_machine_is_compatible("qcom,ipq8062"))
> +			msm_id = QCOM_ID_IPQ8062;
> +		else if (of_machine_is_compatible("qcom,ipq8065") ||
> +			 of_machine_is_compatible("qcom,ipq8069"))
> +			msm_id = QCOM_ID_IPQ8065;
> +		else if (of_machine_is_compatible("qcom,ipq8064") ||
> +			 of_machine_is_compatible("qcom,ipq8066") ||
> +			 of_machine_is_compatible("qcom,ipq8068"))
> +			msm_id = QCOM_ID_IPQ8064;
> +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] cpufreq: qcom-nvmem: add compatible fallback for ipq806x for no SMEM

[Konrad Dybcio]

On 10/29/25 2:33 PM, Christian Marangi wrote:
> On some IPQ806x SoC SMEM might be not initialized by SBL. This is the
> case for some Google devices (the OnHub family) that can't make use of
> SMEM to detect the SoC ID.

Oh this is (the unpleasant kind of ) interesting.. Is there any sort
of uboot/kernel tree for these machines available?

Konrad

Re: [PATCH 3/3] cpufreq: qcom-nvmem: add compatible fallback for ipq806x for no SMEM

[Christian Marangi]

On Thu, Oct 30, 2025 at 09:56:24AM +0100, Konrad Dybcio wrote:
> On 10/29/25 2:33 PM, Christian Marangi wrote:
> > On some IPQ806x SoC SMEM might be not initialized by SBL. This is the
> > case for some Google devices (the OnHub family) that can't make use of
> > SMEM to detect the SoC ID.
> 
> Oh this is (the unpleasant kind of ) interesting.. Is there any sort
> of uboot/kernel tree for these machines available?
>

There is some sort of source but quite confusing. From the info they use
coreboot and chromeos.

Looking at the source they comment everything related to SMEM
(confirming the fact that they actually don't init it)

[1] https://chromium.googlesource.com/chromiumos/platform/depthcharge/+/refs/heads/firmware-storm-6315.B/src/board/storm
[2] https://chromium.googlesource.com/chromiumos/third_party/coreboot/+/firmware-storm-6315.B

-- 
	Ansuel

Re: [PATCH 3/3] cpufreq: qcom-nvmem: add compatible fallback for ipq806x for no SMEM

[Konrad Dybcio]

On 10/30/25 11:28 AM, Christian Marangi wrote:
> On Thu, Oct 30, 2025 at 09:56:24AM +0100, Konrad Dybcio wrote:
>> On 10/29/25 2:33 PM, Christian Marangi wrote:
>>> On some IPQ806x SoC SMEM might be not initialized by SBL. This is the
>>> case for some Google devices (the OnHub family) that can't make use of
>>> SMEM to detect the SoC ID.
>>
>> Oh this is (the unpleasant kind of ) interesting.. Is there any sort
>> of uboot/kernel tree for these machines available?
>>
> 
> There is some sort of source but quite confusing. From the info they use
> coreboot and chromeos.
> 
> Looking at the source they comment everything related to SMEM
> (confirming the fact that they actually don't init it)
> 
> [1] https://chromium.googlesource.com/chromiumos/platform/depthcharge/+/refs/heads/firmware-storm-6315.B/src/board/storm
> [2] https://chromium.googlesource.com/chromiumos/third_party/coreboot/+/firmware-storm-6315.B

Hmm odd..

The patch itself looks mostly good, although you e.g. assign
qcom,ipq8069 -> QCOM_ID_IPQ8065 even though QCOM_ID_IPQ8069 exists

This doesn't cause any difference in behavior within this driver but
looks slightly funky

Should we perhaps do this patching in smem.c instead, in case other
drivers try to retrieve the ID in the future?

Konrad

Re: [PATCH 3/3] cpufreq: qcom-nvmem: add compatible fallback for ipq806x for no SMEM

[Christian Marangi]

On Thu, Oct 30, 2025 at 11:54:41AM +0100, Konrad Dybcio wrote:
> On 10/30/25 11:28 AM, Christian Marangi wrote:
> > On Thu, Oct 30, 2025 at 09:56:24AM +0100, Konrad Dybcio wrote:
> >> On 10/29/25 2:33 PM, Christian Marangi wrote:
> >>> On some IPQ806x SoC SMEM might be not initialized by SBL. This is the
> >>> case for some Google devices (the OnHub family) that can't make use of
> >>> SMEM to detect the SoC ID.
> >>
> >> Oh this is (the unpleasant kind of ) interesting.. Is there any sort
> >> of uboot/kernel tree for these machines available?
> >>
> > 
> > There is some sort of source but quite confusing. From the info they use
> > coreboot and chromeos.
> > 
> > Looking at the source they comment everything related to SMEM
> > (confirming the fact that they actually don't init it)
> > 
> > [1] https://chromium.googlesource.com/chromiumos/platform/depthcharge/+/refs/heads/firmware-storm-6315.B/src/board/storm
> > [2] https://chromium.googlesource.com/chromiumos/third_party/coreboot/+/firmware-storm-6315.B
> 
> Hmm odd..
> 
> The patch itself looks mostly good, although you e.g. assign
> qcom,ipq8069 -> QCOM_ID_IPQ8065 even though QCOM_ID_IPQ8069 exists
> 
> This doesn't cause any difference in behavior within this driver but
> looks slightly funky
>

Well yes I did to simplify the logic.

> Should we perhaps do this patching in smem.c instead, in case other
> drivers try to retrieve the ID in the future?
> 

Well we would hide the fact that SMEM is not available. SMEM gives
precise info while this operates on some kind of fallback measure. If
someone wrongly sets the compatible and use the most generic one
(qcom,ipq8064) then we would parse the wrong ID.

Also looking at the user of those API it's really just cpufreq and apss
for ipq60xx so maybe not worth? (we would also have to add additional
logic to fallback only for some specific SoC)

-- 
	Ansuel

Re: [PATCH 3/3] cpufreq: qcom-nvmem: add compatible fallback for ipq806x for no SMEM

[Konrad Dybcio]

On 10/30/25 12:11 PM, Christian Marangi wrote:
> On Thu, Oct 30, 2025 at 11:54:41AM +0100, Konrad Dybcio wrote:
>> On 10/30/25 11:28 AM, Christian Marangi wrote:
>>> On Thu, Oct 30, 2025 at 09:56:24AM +0100, Konrad Dybcio wrote:
>>>> On 10/29/25 2:33 PM, Christian Marangi wrote:
>>>>> On some IPQ806x SoC SMEM might be not initialized by SBL. This is the
>>>>> case for some Google devices (the OnHub family) that can't make use of
>>>>> SMEM to detect the SoC ID.
>>>>
>>>> Oh this is (the unpleasant kind of ) interesting.. Is there any sort
>>>> of uboot/kernel tree for these machines available?
>>>>
>>>
>>> There is some sort of source but quite confusing. From the info they use
>>> coreboot and chromeos.
>>>
>>> Looking at the source they comment everything related to SMEM
>>> (confirming the fact that they actually don't init it)
>>>
>>> [1] https://chromium.googlesource.com/chromiumos/platform/depthcharge/+/refs/heads/firmware-storm-6315.B/src/board/storm
>>> [2] https://chromium.googlesource.com/chromiumos/third_party/coreboot/+/firmware-storm-6315.B
>>
>> Hmm odd..
>>
>> The patch itself looks mostly good, although you e.g. assign
>> qcom,ipq8069 -> QCOM_ID_IPQ8065 even though QCOM_ID_IPQ8069 exists
>>
>> This doesn't cause any difference in behavior within this driver but
>> looks slightly funky
>>
> 
> Well yes I did to simplify the logic.

I'm fine with it I think.. it's just a small hack after all

>> Should we perhaps do this patching in smem.c instead, in case other
>> drivers try to retrieve the ID in the future?
>>
> 
> Well we would hide the fact that SMEM is not available. SMEM gives
> precise info while this operates on some kind of fallback measure. If
> someone wrongly sets the compatible and use the most generic one
> (qcom,ipq8064) then we would parse the wrong ID.
> 
> Also looking at the user of those API it's really just cpufreq and apss
> for ipq60xx so maybe not worth? (we would also have to add additional
> logic to fallback only for some specific SoC)

Right, maybe this patch is the right approach

Let's see if others have any reservations

Konrad

Re: [PATCH 1/3] err.h: add ERR_PTR_CONST macro

[Andy Shevchenko]

On Wed, Oct 29, 2025 at 02:33:19PM +0100, Christian Marangi wrote:
> Add ERR_PTR_CONST macro to initialize global variables with error

ERR_PTR_CONST()

> pointers. This might be useful for specific case where there is a global
> variables initialized to an error condition and then later set to the
> real handle once probe finish/completes.

Okay, this has two caveats:

1) naming is bad as it suggests something about const qualifier (and not, it's
not about that at all);

2) it doesn't explain what's wrong with ERR_PTR().

...

Note, I'm not objecting an idea as a whole.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] err.h: add ERR_PTR_CONST macro

[Christian Marangi]

On Wed, Oct 29, 2025 at 05:32:48PM +0200, Andy Shevchenko wrote:
> On Wed, Oct 29, 2025 at 02:33:19PM +0100, Christian Marangi wrote:
> > Add ERR_PTR_CONST macro to initialize global variables with error
> 
> ERR_PTR_CONST()
> 
> > pointers. This might be useful for specific case where there is a global
> > variables initialized to an error condition and then later set to the
> > real handle once probe finish/completes.
> 
> Okay, this has two caveats:
> 
> 1) naming is bad as it suggests something about const qualifier (and not, it's
> not about that at all);
> 
> 2) it doesn't explain what's wrong with ERR_PTR().
>

It can't be used for global variables as it does cause compilation
error.

I wanted to use ERR_PTR to set the __smem handle instead of freecode
(void *) -EPROBE_DEFER and notice the compiler doesn't like using
ERR_PTR().

Then the problem is clear as static declaration require constant value
for initialization and ERR_PTR is a inline function.

This is why ERR_PTR_CONST following the pattern that was used for
FIELD_PREP -> FIELD_PREP_CONST that was also introduced for similar
case.

So yes this is specific for case of static global variables.

> ...
> 
> Note, I'm not objecting an idea as a whole.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

-- 
	Ansuel

Re: [PATCH 1/3] err.h: add ERR_PTR_CONST macro

[Andy Shevchenko]

On Wed, Oct 29, 2025 at 04:38:53PM +0100, Christian Marangi wrote:
> On Wed, Oct 29, 2025 at 05:32:48PM +0200, Andy Shevchenko wrote:
> > On Wed, Oct 29, 2025 at 02:33:19PM +0100, Christian Marangi wrote:
> > > Add ERR_PTR_CONST macro to initialize global variables with error
> > 
> > ERR_PTR_CONST()
> > 
> > > pointers. This might be useful for specific case where there is a global
> > > variables initialized to an error condition and then later set to the
> > > real handle once probe finish/completes.
> > 
> > Okay, this has two caveats:
> > 
> > 1) naming is bad as it suggests something about const qualifier (and not, it's
> > not about that at all);
> > 
> > 2) it doesn't explain what's wrong with ERR_PTR().
> >
> 
> It can't be used for global variables as it does cause compilation
> error.

Can you show an example, please?

> I wanted to use ERR_PTR to set the __smem handle instead of freecode
> (void *) -EPROBE_DEFER and notice the compiler doesn't like using
> ERR_PTR().
> 
> Then the problem is clear as static declaration require constant value
> for initialization and ERR_PTR is a inline function.
> 
> This is why ERR_PTR_CONST following the pattern that was used for
> FIELD_PREP -> FIELD_PREP_CONST that was also introduced for similar
> case.
> 
> So yes this is specific for case of static global variables.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] err.h: add ERR_PTR_CONST macro

[Andy Shevchenko]

On Thu, Oct 30, 2025 at 10:27:38AM +0200, Andy Shevchenko wrote:
> On Wed, Oct 29, 2025 at 04:38:53PM +0100, Christian Marangi wrote:
> > On Wed, Oct 29, 2025 at 05:32:48PM +0200, Andy Shevchenko wrote:
> > > On Wed, Oct 29, 2025 at 02:33:19PM +0100, Christian Marangi wrote:
> > > > Add ERR_PTR_CONST macro to initialize global variables with error
> > > 
> > > ERR_PTR_CONST()
> > > 
> > > > pointers. This might be useful for specific case where there is a global
> > > > variables initialized to an error condition and then later set to the
> > > > real handle once probe finish/completes.
> > > 
> > > Okay, this has two caveats:
> > > 
> > > 1) naming is bad as it suggests something about const qualifier (and not, it's
> > > not about that at all);
> > > 
> > > 2) it doesn't explain what's wrong with ERR_PTR().
> > >
> > 
> > It can't be used for global variables as it does cause compilation
> > error.
> 
> Can you show an example, please?

Ah, it's probably due to static inline...

> > I wanted to use ERR_PTR to set the __smem handle instead of freecode
> > (void *) -EPROBE_DEFER and notice the compiler doesn't like using
> > ERR_PTR().
> > 
> > Then the problem is clear as static declaration require constant value
> > for initialization and ERR_PTR is a inline function.
> > 
> > This is why ERR_PTR_CONST following the pattern that was used for
> > FIELD_PREP -> FIELD_PREP_CONST that was also introduced for similar
> > case.
> > 
> > So yes this is specific for case of static global variables.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] err.h: add ERR_PTR_CONST macro

[Christian Marangi]

On Thu, Oct 30, 2025 at 10:27:38AM +0200, Andy Shevchenko wrote:
> On Wed, Oct 29, 2025 at 04:38:53PM +0100, Christian Marangi wrote:
> > On Wed, Oct 29, 2025 at 05:32:48PM +0200, Andy Shevchenko wrote:
> > > On Wed, Oct 29, 2025 at 02:33:19PM +0100, Christian Marangi wrote:
> > > > Add ERR_PTR_CONST macro to initialize global variables with error
> > > 
> > > ERR_PTR_CONST()
> > > 
> > > > pointers. This might be useful for specific case where there is a global
> > > > variables initialized to an error condition and then later set to the
> > > > real handle once probe finish/completes.
> > > 
> > > Okay, this has two caveats:
> > > 
> > > 1) naming is bad as it suggests something about const qualifier (and not, it's
> > > not about that at all);
> > > 
> > > 2) it doesn't explain what's wrong with ERR_PTR().
> > >
> > 
> > It can't be used for global variables as it does cause compilation
> > error.
> 
> Can you show an example, please?
>

drivers/soc/qcom/smem.c:361:35: error: initializer element is not constant
  361 | static struct qcom_smem *__smem = ERR_PTR(-EPROBE_DEFER);
      |                                   ^~~~~~~
make[9]: *** [scripts/Makefile.build:229: drivers/soc/qcom/smem.o] Error 1

You want me to add this to the commit? Or any hint to better reword this
so it's more understandable?

> > I wanted to use ERR_PTR to set the __smem handle instead of freecode
> > (void *) -EPROBE_DEFER and notice the compiler doesn't like using
> > ERR_PTR().
> > 
> > Then the problem is clear as static declaration require constant value
> > for initialization and ERR_PTR is a inline function.
> > 
> > This is why ERR_PTR_CONST following the pattern that was used for
> > FIELD_PREP -> FIELD_PREP_CONST that was also introduced for similar
> > case.
> > 
> > So yes this is specific for case of static global variables.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

-- 
	Ansuel

Re: [PATCH 1/3] err.h: add ERR_PTR_CONST macro

[Andy Shevchenko]

On Thu, Oct 30, 2025 at 11:22:11AM +0100, Christian Marangi wrote:
> On Thu, Oct 30, 2025 at 10:27:38AM +0200, Andy Shevchenko wrote:
> > On Wed, Oct 29, 2025 at 04:38:53PM +0100, Christian Marangi wrote:
> > > On Wed, Oct 29, 2025 at 05:32:48PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Oct 29, 2025 at 02:33:19PM +0100, Christian Marangi wrote:
> > > > > Add ERR_PTR_CONST macro to initialize global variables with error
> > > > 
> > > > ERR_PTR_CONST()
> > > > 
> > > > > pointers. This might be useful for specific case where there is a global
> > > > > variables initialized to an error condition and then later set to the
> > > > > real handle once probe finish/completes.
> > > > 
> > > > Okay, this has two caveats:
> > > > 
> > > > 1) naming is bad as it suggests something about const qualifier (and not, it's
> > > > not about that at all);
> > > > 
> > > > 2) it doesn't explain what's wrong with ERR_PTR().
> > > >
> > > 
> > > It can't be used for global variables as it does cause compilation
> > > error.
> > 
> > Can you show an example, please?
> 
> drivers/soc/qcom/smem.c:361:35: error: initializer element is not constant
>   361 | static struct qcom_smem *__smem = ERR_PTR(-EPROBE_DEFER);
>       |                                   ^~~~~~~
> make[9]: *** [scripts/Makefile.build:229: drivers/soc/qcom/smem.o] Error 1
> 
> You want me to add this to the commit? Or any hint to better reword this
> so it's more understandable?

Just the first line would be enough.
And perhaps better naming for the macro, but I have no ideas from top of my
head right now. Ah, actually I do. We call those either INIT_*() or DEFINE_*()
with the difference that INIT_*() works like your proposed idea. i.e. returns
a suitable value, but DEFINE_*() incorporates a variable and a type.

I think the INIT_ERR_PTR() is what we want as a name.

> > > I wanted to use ERR_PTR to set the __smem handle instead of freecode
> > > (void *) -EPROBE_DEFER and notice the compiler doesn't like using
> > > ERR_PTR().
> > > 
> > > Then the problem is clear as static declaration require constant value
> > > for initialization and ERR_PTR is a inline function.
> > > 
> > > This is why ERR_PTR_CONST following the pattern that was used for
> > > FIELD_PREP -> FIELD_PREP_CONST that was also introduced for similar
> > > case.
> > > 
> > > So yes this is specific for case of static global variables.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] err.h: add ERR_PTR_CONST macro

[Arnd Bergmann]

On Thu, Oct 30, 2025, at 15:00, Andy Shevchenko wrote:
> On Thu, Oct 30, 2025 at 11:22:11AM +0100, Christian Marangi wrote:
>> On Thu, Oct 30, 2025 at 10:27:38AM +0200, Andy Shevchenko wrote:
>> > On Wed, Oct 29, 2025 at 04:38:53PM +0100, Christian Marangi wrote:

>> drivers/soc/qcom/smem.c:361:35: error: initializer element is not constant
>>   361 | static struct qcom_smem *__smem = ERR_PTR(-EPROBE_DEFER);
>>       |                                   ^~~~~~~
>> make[9]: *** [scripts/Makefile.build:229: drivers/soc/qcom/smem.o] Error 1
>> 
>> You want me to add this to the commit? Or any hint to better reword this
>> so it's more understandable?
>
> Just the first line would be enough.
> And perhaps better naming for the macro, but I have no ideas from top of my
> head right now. Ah, actually I do. We call those either INIT_*() or DEFINE_*()
> with the difference that INIT_*() works like your proposed idea. i.e. returns
> a suitable value, but DEFINE_*() incorporates a variable and a type.
>
> I think the INIT_ERR_PTR() is what we want as a name.

Agreed, that seems better than ERR_PTR_CONST(). I'm still not sure
there is much benefit in using this in static initializers, but
I don't mind it either.

    Arnd