For some platforms (e.g. Lenovo Yoga C630) we don't yet know a way to
update variables in the permanent storage. However being able to read
the vars is still useful as it allows us to get e.g. RTC offset.

Add a quirk for QSEECOM specifying that UEFI variables for this platform
should be registered in read-only mode.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/firmware/qcom/qcom_qseecom_uefisecapp.c | 18 +++++++++++++++++-
 include/linux/firmware/qcom/qcom_qseecom.h      |  2 ++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
index 98a463e9774bf04f2deb0f7fa1318bd0d2edfa49..05f700dcb8cf3189f640237ff0e045564abb8264 100644
--- a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
+++ b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
@@ -792,6 +792,12 @@ static efi_status_t qcuefi_query_variable_info(u32 attr, u64 *storage_space, u64
 	return status;
 }
 
+static const struct efivar_operations qcom_efivars_ro_ops = {
+	.get_variable = qcuefi_get_variable,
+	.get_next_variable = qcuefi_get_next_variable,
+	.query_variable_info = qcuefi_query_variable_info,
+};
+
 static const struct efivar_operations qcom_efivar_ops = {
 	.get_variable = qcuefi_get_variable,
 	.set_variable = qcuefi_set_variable,
@@ -804,7 +810,9 @@ static const struct efivar_operations qcom_efivar_ops = {
 static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev,
 				 const struct auxiliary_device_id *aux_dev_id)
 {
+	unsigned long *quirks = aux_dev->dev.platform_data;
 	struct qcom_tzmem_pool_config pool_config;
+	const struct efivar_operations *ops;
 	struct qcuefi_client *qcuefi;
 	int status;
 
@@ -829,7 +837,15 @@ static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev,
 	if (status)
 		return status;
 
-	status = efivars_register(&qcuefi->efivars, &qcom_efivar_ops);
+	if (quirks &&
+	    *quirks & QCOM_QSEECOM_QUIRK_RO_UEFIVARS) {
+		dev_dbg(&aux_dev->dev, "R/O UEFI vars implementation\n");
+		ops = &qcom_efivars_ro_ops;
+	} else {
+		ops = &qcom_efivar_ops;
+	}
+
+	status = efivars_register(&qcuefi->efivars, ops);
 	if (status)
 		qcuefi_set_reference(NULL);
 
diff --git a/include/linux/firmware/qcom/qcom_qseecom.h b/include/linux/firmware/qcom/qcom_qseecom.h
index 3387897bf36843cccd0bd933dd562390bf674b14..8d6d660e854fdb0fabbef10ab5ee6ff23ad79826 100644
--- a/include/linux/firmware/qcom/qcom_qseecom.h
+++ b/include/linux/firmware/qcom/qcom_qseecom.h
@@ -51,4 +51,6 @@ static inline int qcom_qseecom_app_send(struct qseecom_client *client,
 	return qcom_scm_qseecom_app_send(client->app_id, req, req_size, rsp, rsp_size);
 }
 
+#define QCOM_QSEECOM_QUIRK_RO_UEFIVARS		BIT(0)
+
 #endif /* __QCOM_QSEECOM_H */

-- 
2.39.5

On Sat, Jun 21, 2025 at 10:56:11PM +0300, Dmitry Baryshkov wrote:
> For some platforms (e.g. Lenovo Yoga C630) we don't yet know a way to
> update variables in the permanent storage. However being able to read
> the vars is still useful as it allows us to get e.g. RTC offset.
> 
> Add a quirk for QSEECOM specifying that UEFI variables for this platform
> should be registered in read-only mode.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---
>  drivers/firmware/qcom/qcom_qseecom_uefisecapp.c | 18 +++++++++++++++++-
>  include/linux/firmware/qcom/qcom_qseecom.h      |  2 ++
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
> index 98a463e9774bf04f2deb0f7fa1318bd0d2edfa49..05f700dcb8cf3189f640237ff0e045564abb8264 100644
> --- a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
> +++ b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
> @@ -792,6 +792,12 @@ static efi_status_t qcuefi_query_variable_info(u32 attr, u64 *storage_space, u64
>  	return status;
>  }
>  
> +static const struct efivar_operations qcom_efivars_ro_ops = {
> +	.get_variable = qcuefi_get_variable,
> +	.get_next_variable = qcuefi_get_next_variable,
> +	.query_variable_info = qcuefi_query_variable_info,
> +};

It looks like the efivars implementation does not support read-only
efivars and this will lead to NULL pointer dereferences whenever you try
to write a variable.

Also not sure how useful it is to only be able to read variables,
including for the RTC where you'll end up with an RTC that's always
slightly off due to drift (even if you can set it when booting into
Windows or possibly from the UEFI setup).

Don't you have any SDAM blocks in the PMICs that you can use instead for
a proper functioning RTC on these machines?

Johan

On Mon, Jun 23, 2025 at 04:45:30PM +0200, Johan Hovold wrote:
> On Sat, Jun 21, 2025 at 10:56:11PM +0300, Dmitry Baryshkov wrote:
> > For some platforms (e.g. Lenovo Yoga C630) we don't yet know a way to
> > update variables in the permanent storage. However being able to read
> > the vars is still useful as it allows us to get e.g. RTC offset.
> > 
> > Add a quirk for QSEECOM specifying that UEFI variables for this platform
> > should be registered in read-only mode.
> > 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > ---
> >  drivers/firmware/qcom/qcom_qseecom_uefisecapp.c | 18 +++++++++++++++++-
> >  include/linux/firmware/qcom/qcom_qseecom.h      |  2 ++
> >  2 files changed, 19 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
> > index 98a463e9774bf04f2deb0f7fa1318bd0d2edfa49..05f700dcb8cf3189f640237ff0e045564abb8264 100644
> > --- a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
> > +++ b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
> > @@ -792,6 +792,12 @@ static efi_status_t qcuefi_query_variable_info(u32 attr, u64 *storage_space, u64
> >  	return status;
> >  }
> >  
> > +static const struct efivar_operations qcom_efivars_ro_ops = {
> > +	.get_variable = qcuefi_get_variable,
> > +	.get_next_variable = qcuefi_get_next_variable,
> > +	.query_variable_info = qcuefi_query_variable_info,
> > +};
> 
> It looks like the efivars implementation does not support read-only
> efivars and this will lead to NULL pointer dereferences whenever you try
> to write a variable.

Ok, efivarfs seems to support it, but you'd crash when setting a
variable from the kernel (e.g. from the RTC driver).

> Also not sure how useful it is to only be able to read variables,
> including for the RTC where you'll end up with an RTC that's always
> slightly off due to drift (even if you can set it when booting into
> Windows or possibly from the UEFI setup).
> 
> Don't you have any SDAM blocks in the PMICs that you can use instead for
> a proper functioning RTC on these machines?

Johan

On Mon, Jun 23, 2025 at 04:50:27PM +0200, Johan Hovold wrote:
> On Mon, Jun 23, 2025 at 04:45:30PM +0200, Johan Hovold wrote:
> > On Sat, Jun 21, 2025 at 10:56:11PM +0300, Dmitry Baryshkov wrote:
> > > For some platforms (e.g. Lenovo Yoga C630) we don't yet know a way to
> > > update variables in the permanent storage. However being able to read
> > > the vars is still useful as it allows us to get e.g. RTC offset.
> > > 
> > > Add a quirk for QSEECOM specifying that UEFI variables for this platform
> > > should be registered in read-only mode.
> > > 
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > > ---
> > >  drivers/firmware/qcom/qcom_qseecom_uefisecapp.c | 18 +++++++++++++++++-
> > >  include/linux/firmware/qcom/qcom_qseecom.h      |  2 ++
> > >  2 files changed, 19 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
> > > index 98a463e9774bf04f2deb0f7fa1318bd0d2edfa49..05f700dcb8cf3189f640237ff0e045564abb8264 100644
> > > --- a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
> > > +++ b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
> > > @@ -792,6 +792,12 @@ static efi_status_t qcuefi_query_variable_info(u32 attr, u64 *storage_space, u64
> > >  	return status;
> > >  }
> > >  
> > > +static const struct efivar_operations qcom_efivars_ro_ops = {
> > > +	.get_variable = qcuefi_get_variable,
> > > +	.get_next_variable = qcuefi_get_next_variable,
> > > +	.query_variable_info = qcuefi_query_variable_info,
> > > +};
> > 
> > It looks like the efivars implementation does not support read-only
> > efivars and this will lead to NULL pointer dereferences whenever you try
> > to write a variable.
> 
> Ok, efivarfs seems to support it, but you'd crash when setting a
> variable from the kernel (e.g. from the RTC driver).

Ack, I'll fix it.

> 
> > Also not sure how useful it is to only be able to read variables,
> > including for the RTC where you'll end up with an RTC that's always
> > slightly off due to drift (even if you can set it when booting into
> > Windows or possibly from the UEFI setup).
> > 
> > Don't you have any SDAM blocks in the PMICs that you can use instead for
> > a proper functioning RTC on these machines?

I'd rather not poke into an SDAM, especially since we don't have docs
which SDAM blocks are used and which are not.

I think the slightly drifted RTC is still much better than ending up
with an RTC value which is significantly off, because it was set via the
file modification time.

Anyway, let me pick up some more patches in the next revision, maybe it
would be more obvious why I'd like to get R/O support.

-- 
With best wishes
Dmitry

On Tue, Jun 24, 2025 at 04:13:34AM +0300, Dmitry Baryshkov wrote:
> On Mon, Jun 23, 2025 at 04:50:27PM +0200, Johan Hovold wrote:
> > On Mon, Jun 23, 2025 at 04:45:30PM +0200, Johan Hovold wrote:

> > > Also not sure how useful it is to only be able to read variables,
> > > including for the RTC where you'll end up with an RTC that's always
> > > slightly off due to drift (even if you can set it when booting into
> > > Windows or possibly from the UEFI setup).
> > > 
> > > Don't you have any SDAM blocks in the PMICs that you can use instead for
> > > a proper functioning RTC on these machines?
> 
> I'd rather not poke into an SDAM, especially since we don't have docs
> which SDAM blocks are used and which are not.

You're with Qualcomm now so you should be able to dig up this
information like we did for the X13s (even if I'm quite aware that it
may still be easier said than done).

> I think the slightly drifted RTC is still much better than ending up
> with an RTC value which is significantly off, because it was set via the
> file modification time.

I measured drift of 1 second every 3.5 h on the X13s, so having an
almost correct time with massive drift that cannot be corrected for may
not necessarily be better.

> Anyway, let me pick up some more patches in the next revision, maybe it
> would be more obvious why I'd like to get R/O support.

I'll try to take a look.

Johan

On Thu, Jun 26, 2025 at 11:42:50AM +0200, Johan Hovold wrote:
> On Tue, Jun 24, 2025 at 04:13:34AM +0300, Dmitry Baryshkov wrote:
> > On Mon, Jun 23, 2025 at 04:50:27PM +0200, Johan Hovold wrote:
> > > On Mon, Jun 23, 2025 at 04:45:30PM +0200, Johan Hovold wrote:
> 
> > > > Also not sure how useful it is to only be able to read variables,
> > > > including for the RTC where you'll end up with an RTC that's always
> > > > slightly off due to drift (even if you can set it when booting into
> > > > Windows or possibly from the UEFI setup).
> > > > 
> > > > Don't you have any SDAM blocks in the PMICs that you can use instead for
> > > > a proper functioning RTC on these machines?
> > 
> > I'd rather not poke into an SDAM, especially since we don't have docs
> > which SDAM blocks are used and which are not.
> 
> You're with Qualcomm now so you should be able to dig up this
> information like we did for the X13s (even if I'm quite aware that it
> may still be easier said than done).

I'd rather try to find information on how to update UEFI vars on the
storage. Moreover, using the UEFI variable doesn't send the wrong
message to other developers (if I remember correctly, I've seen patches
poking to semi-random SDAM just because it seemed to be unused).

> > I think the slightly drifted RTC is still much better than ending up
> > with an RTC value which is significantly off, because it was set via the
> > file modification time.
> 
> I measured drift of 1 second every 3.5 h on the X13s, so having an
> almost correct time with massive drift that cannot be corrected for may
> not necessarily be better.

For me it provided a better user experience. Yes, I'm using C630 from
time to time, including the kernel development. A drifted but ticking
RTC is better than the RTC that rolls backs by several months at a
reboot, because of the missing RTC offset info.

> 
> > Anyway, let me pick up some more patches in the next revision, maybe it
> > would be more obvious why I'd like to get R/O support.
> 
> I'll try to take a look.
> 
> Johan

-- 
With best wishes
Dmitry

On Thu, Jun 26, 2025 at 02:15:26PM +0300, Dmitry Baryshkov wrote:
> On Thu, Jun 26, 2025 at 11:42:50AM +0200, Johan Hovold wrote:
> > On Tue, Jun 24, 2025 at 04:13:34AM +0300, Dmitry Baryshkov wrote:
> > > On Mon, Jun 23, 2025 at 04:50:27PM +0200, Johan Hovold wrote:
> > > > On Mon, Jun 23, 2025 at 04:45:30PM +0200, Johan Hovold wrote:
> > 
> > > > > Also not sure how useful it is to only be able to read variables,
> > > > > including for the RTC where you'll end up with an RTC that's always
> > > > > slightly off due to drift (even if you can set it when booting into
> > > > > Windows or possibly from the UEFI setup).
> > > > > 
> > > > > Don't you have any SDAM blocks in the PMICs that you can use instead for
> > > > > a proper functioning RTC on these machines?
> > > 
> > > I'd rather not poke into an SDAM, especially since we don't have docs
> > > which SDAM blocks are used and which are not.
> > 
> > You're with Qualcomm now so you should be able to dig up this
> > information like we did for the X13s (even if I'm quite aware that it
> > may still be easier said than done).
> 
> I'd rather try to find information on how to update UEFI vars on the
> storage.

You can do both, especially if it turns out you won't be able to have
persistent variables on these machines.

> Moreover, using the UEFI variable doesn't send the wrong
> message to other developers (if I remember correctly, I've seen patches
> poking to semi-random SDAM just because it seemed to be unused).

That's for the Qualcomm maintainers, and the rest of us, to catch during
review. And people putting random values into devicetrees is
unfortunately not limited to SDAM addresses.

Furthermore, getting an allocated block of addresses in SDAM for Linux
could be useful for other things too.
 
> > > I think the slightly drifted RTC is still much better than ending up
> > > with an RTC value which is significantly off, because it was set via the
> > > file modification time.
> > 
> > I measured drift of 1 second every 3.5 h on the X13s, so having an
> > almost correct time with massive drift that cannot be corrected for may
> > not necessarily be better.
> 
> For me it provided a better user experience. Yes, I'm using C630 from
> time to time, including the kernel development. A drifted but ticking
> RTC is better than the RTC that rolls backs by several months at a
> reboot, because of the missing RTC offset info.

Does it have to roll back? Can't you just keep it running after whatever
semi-random date it started at? And there is ntp and services like
fake-hwclock which saves the time on shutdown too.

Anyway, I still do no understand why you seem so reluctant to having a
proper functioning RTC using an SDAM offset.

Johan

On Thu, Jun 26, 2025 at 03:13:42PM +0200, Johan Hovold wrote:
> On Thu, Jun 26, 2025 at 02:15:26PM +0300, Dmitry Baryshkov wrote:
> > On Thu, Jun 26, 2025 at 11:42:50AM +0200, Johan Hovold wrote:
> > > On Tue, Jun 24, 2025 at 04:13:34AM +0300, Dmitry Baryshkov wrote:
> > > > On Mon, Jun 23, 2025 at 04:50:27PM +0200, Johan Hovold wrote:
> > > > > On Mon, Jun 23, 2025 at 04:45:30PM +0200, Johan Hovold wrote:
> > > 
> > > > > > Also not sure how useful it is to only be able to read variables,
> > > > > > including for the RTC where you'll end up with an RTC that's always
> > > > > > slightly off due to drift (even if you can set it when booting into
> > > > > > Windows or possibly from the UEFI setup).
> > > > > > 
> > > > > > Don't you have any SDAM blocks in the PMICs that you can use instead for
> > > > > > a proper functioning RTC on these machines?
> > > > 
> > > > I'd rather not poke into an SDAM, especially since we don't have docs
> > > > which SDAM blocks are used and which are not.
> > > 
> > > You're with Qualcomm now so you should be able to dig up this
> > > information like we did for the X13s (even if I'm quite aware that it
> > > may still be easier said than done).
> > 
> > I'd rather try to find information on how to update UEFI vars on the
> > storage.
> 
> You can do both, especially if it turns out you won't be able to have
> persistent variables on these machines.
> 
> > Moreover, using the UEFI variable doesn't send the wrong
> > message to other developers (if I remember correctly, I've seen patches
> > poking to semi-random SDAM just because it seemed to be unused).
> 
> That's for the Qualcomm maintainers, and the rest of us, to catch during
> review. And people putting random values into devicetrees is
> unfortunately not limited to SDAM addresses.

Yes. But here it might be more fun.

> Furthermore, getting an allocated block of addresses in SDAM for Linux
> could be useful for other things too.

Yes, but this obviously can't happen for released platforms.

>  
> > > > I think the slightly drifted RTC is still much better than ending up
> > > > with an RTC value which is significantly off, because it was set via the
> > > > file modification time.
> > > 
> > > I measured drift of 1 second every 3.5 h on the X13s, so having an
> > > almost correct time with massive drift that cannot be corrected for may
> > > not necessarily be better.
> > 
> > For me it provided a better user experience. Yes, I'm using C630 from
> > time to time, including the kernel development. A drifted but ticking
> > RTC is better than the RTC that rolls backs by several months at a
> > reboot, because of the missing RTC offset info.
> 
> Does it have to roll back? Can't you just keep it running after whatever
> semi-random date it started at?

It rolls back after reboot.

> And there is ntp and services like
> fake-hwclock which saves the time on shutdown too.

Likewise I can plug in the USB RTC or do something else. NTP requires
network access, which is fun to have if you are debugging modem of WiFi.

> Anyway, I still do no understand why you seem so reluctant to having a
> proper functioning RTC using an SDAM offset.

Because that would be a one-off solution for this particular laptop,
etc. I want something that other laptops can use without having to find
another magic value which suits a particular laptop instance.

-- 
With best wishes
Dmitry

On Fri, Jun 27, 2025 at 02:09:51AM +0300, Dmitry Baryshkov wrote:
> On Thu, Jun 26, 2025 at 03:13:42PM +0200, Johan Hovold wrote:

> > Furthermore, getting an allocated block of addresses in SDAM for Linux
> > could be useful for other things too.
> 
> Yes, but this obviously can't happen for released platforms.

We managed to get one for sc8280xp post release (was harder for x1e for
some reason).

> > > > > I think the slightly drifted RTC is still much better than ending up
> > > > > with an RTC value which is significantly off, because it was set via the
> > > > > file modification time.
> > > > 
> > > > I measured drift of 1 second every 3.5 h on the X13s, so having an
> > > > almost correct time with massive drift that cannot be corrected for may
> > > > not necessarily be better.
> > > 
> > > For me it provided a better user experience. Yes, I'm using C630 from
> > > time to time, including the kernel development. A drifted but ticking
> > > RTC is better than the RTC that rolls backs by several months at a
> > > reboot, because of the missing RTC offset info.
> > 
> > Does it have to roll back? Can't you just keep it running after whatever
> > semi-random date it started at?
> 
> It rolls back after reboot.

That's odd. Doesn't happen here with the X1E CRD if I drop the uefi
offset property in DT. I'm back in the seventies but time is strictly
increasing also after reboots.

Perhaps you have some user space setting that resets it?

> > And there is ntp and services like
> > fake-hwclock which saves the time on shutdown too.
> 
> Likewise I can plug in the USB RTC or do something else. NTP requires
> network access, which is fun to have if you are debugging modem of WiFi.
>
> > Anyway, I still do no understand why you seem so reluctant to having a
> > proper functioning RTC using an SDAM offset.
> 
> Because that would be a one-off solution for this particular laptop,
> etc. I want something that other laptops can use without having to find
> another magic value which suits a particular laptop instance.

My point is that it's not really a solution. These machines still do not
have persistent UEFI variables, and now they have an apparently
functional but still broken RTC.

I added support for the UEFI offset to the driver so that the time could
be set on Qualcomm machines. With this series that is still not the
case, even if people may now initially get the impression that it works
since the time is only off by a few seconds (until it becomes minutes
and hours and you starting missing your trains).

Johan

On 6/26/25 3:13 PM, Johan Hovold wrote:
> On Thu, Jun 26, 2025 at 02:15:26PM +0300, Dmitry Baryshkov wrote:
>> On Thu, Jun 26, 2025 at 11:42:50AM +0200, Johan Hovold wrote:
>>> On Tue, Jun 24, 2025 at 04:13:34AM +0300, Dmitry Baryshkov wrote:
>>>> On Mon, Jun 23, 2025 at 04:50:27PM +0200, Johan Hovold wrote:
>>>>> On Mon, Jun 23, 2025 at 04:45:30PM +0200, Johan Hovold wrote:
>>>
>>>>>> Also not sure how useful it is to only be able to read variables,
>>>>>> including for the RTC where you'll end up with an RTC that's always
>>>>>> slightly off due to drift (even if you can set it when booting into
>>>>>> Windows or possibly from the UEFI setup).
>>>>>>
>>>>>> Don't you have any SDAM blocks in the PMICs that you can use instead for
>>>>>> a proper functioning RTC on these machines?
>>>>
>>>> I'd rather not poke into an SDAM, especially since we don't have docs
>>>> which SDAM blocks are used and which are not.
>>>
>>> You're with Qualcomm now so you should be able to dig up this
>>> information like we did for the X13s (even if I'm quite aware that it
>>> may still be easier said than done).
>>
>> I'd rather try to find information on how to update UEFI vars on the
>> storage.
> 
> You can do both, especially if it turns out you won't be able to have
> persistent variables on these machines.

The danger here is that we only know what Qualcomm uses these cells
for, not necessarily what the vendors with a similar idea could
have come up with.

This is especially important since (unfortunately without going into
detail), you *really* don't want to mess up some existing values in
there.

Konrad

On Thu, Jun 26, 2025 at 03:49:32PM +0200, Konrad Dybcio wrote:
> On 6/26/25 3:13 PM, Johan Hovold wrote:
> > On Thu, Jun 26, 2025 at 02:15:26PM +0300, Dmitry Baryshkov wrote:
> >> On Thu, Jun 26, 2025 at 11:42:50AM +0200, Johan Hovold wrote:
> >>> On Tue, Jun 24, 2025 at 04:13:34AM +0300, Dmitry Baryshkov wrote:
> >>>> On Mon, Jun 23, 2025 at 04:50:27PM +0200, Johan Hovold wrote:
> >>>>> On Mon, Jun 23, 2025 at 04:45:30PM +0200, Johan Hovold wrote:
> >>>
> >>>>>> Also not sure how useful it is to only be able to read variables,
> >>>>>> including for the RTC where you'll end up with an RTC that's always
> >>>>>> slightly off due to drift (even if you can set it when booting into
> >>>>>> Windows or possibly from the UEFI setup).
> >>>>>>
> >>>>>> Don't you have any SDAM blocks in the PMICs that you can use instead for
> >>>>>> a proper functioning RTC on these machines?
> >>>>
> >>>> I'd rather not poke into an SDAM, especially since we don't have docs
> >>>> which SDAM blocks are used and which are not.
> >>>
> >>> You're with Qualcomm now so you should be able to dig up this
> >>> information like we did for the X13s (even if I'm quite aware that it
> >>> may still be easier said than done).
> >>
> >> I'd rather try to find information on how to update UEFI vars on the
> >> storage.
> > 
> > You can do both, especially if it turns out you won't be able to have
> > persistent variables on these machines.
> 
> The danger here is that we only know what Qualcomm uses these cells
> for, not necessarily what the vendors with a similar idea could
> have come up with.

Hmm. Good point.

But at least the address we used on sc8280xp appears to be cleared on
hard reset (holding down the power button) so it can't be used for
anything that useful it seems.

> This is especially important since (unfortunately without going into
> detail), you *really* don't want to mess up some existing values in
> there.

Yeah, I wouldn't pick a random address without getting an ack from
Qualcomm first.

Johan

On 6/23/25 4:45 PM, Johan Hovold wrote:
> On Sat, Jun 21, 2025 at 10:56:11PM +0300, Dmitry Baryshkov wrote:
>> For some platforms (e.g. Lenovo Yoga C630) we don't yet know a way to
>> update variables in the permanent storage. However being able to read
>> the vars is still useful as it allows us to get e.g. RTC offset.
>>
>> Add a quirk for QSEECOM specifying that UEFI variables for this platform
>> should be registered in read-only mode.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>> ---
>>  drivers/firmware/qcom/qcom_qseecom_uefisecapp.c | 18 +++++++++++++++++-
>>  include/linux/firmware/qcom/qcom_qseecom.h      |  2 ++
>>  2 files changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
>> index 98a463e9774bf04f2deb0f7fa1318bd0d2edfa49..05f700dcb8cf3189f640237ff0e045564abb8264 100644
>> --- a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
>> +++ b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
>> @@ -792,6 +792,12 @@ static efi_status_t qcuefi_query_variable_info(u32 attr, u64 *storage_space, u64
>>  	return status;
>>  }
>>  
>> +static const struct efivar_operations qcom_efivars_ro_ops = {
>> +	.get_variable = qcuefi_get_variable,
>> +	.get_next_variable = qcuefi_get_next_variable,
>> +	.query_variable_info = qcuefi_query_variable_info,
>> +};
> 
> It looks like the efivars implementation does not support read-only
> efivars and this will lead to NULL pointer dereferences whenever you try
> to write a variable.

There's efivar_supports_writes() that is used to set the EFIVAR_OPS_RDONLY
flag which then sets SB_RDONLY on all efivarfs superblocks

Konrad

On Mon, Jun 23, 2025 at 04:49:22PM +0200, Konrad Dybcio wrote:
> On 6/23/25 4:45 PM, Johan Hovold wrote:

> > It looks like the efivars implementation does not support read-only
> > efivars and this will lead to NULL pointer dereferences whenever you try
> > to write a variable.
> 
> There's efivar_supports_writes() that is used to set the EFIVAR_OPS_RDONLY
> flag which then sets SB_RDONLY on all efivarfs superblocks

Yep, but it doesn't help when attempting to store the RTC offset from
the kernel (since that does not use efivarfs).

Johan

On 6/23/25 4:52 PM, Johan Hovold wrote:
> On Mon, Jun 23, 2025 at 04:49:22PM +0200, Konrad Dybcio wrote:
>> On 6/23/25 4:45 PM, Johan Hovold wrote:
> 
>>> It looks like the efivars implementation does not support read-only
>>> efivars and this will lead to NULL pointer dereferences whenever you try
>>> to write a variable.
>>
>> There's efivar_supports_writes() that is used to set the EFIVAR_OPS_RDONLY
>> flag which then sets SB_RDONLY on all efivarfs superblocks
> 
> Yep, but it doesn't help when attempting to store the RTC offset from
> the kernel (since that does not use efivarfs).

You're right, that hole needs patching

Konrad