* [PATCH 2/3] blspec: add new blspec.compatible.extra variable
2023-09-20 12:37 [PATCH 1/3] blspec: take compatible name as argument Rouven Czerwinski
@ 2023-09-20 12:37 ` Rouven Czerwinski
2023-09-21 12:37 ` Ahmad Fatoum
2023-09-21 12:57 ` Sascha Hauer
2023-09-20 12:37 ` [PATCH 3/3] blspec: take extra entries into account Rouven Czerwinski
` (2 subsequent siblings)
3 siblings, 2 replies; 8+ messages in thread
From: Rouven Czerwinski @ 2023-09-20 12:37 UTC (permalink / raw)
To: barebox; +Cc: Rouven Czerwinski
The blspec.compatible.extra variable will be used to specify extra
compatibles that are also used to match during bootloader specification
entry parsing. This means that even if your internal barebox device tree
machine compatible is "vendor,bareboxcompatible", but your bootloader
specification device tree contains "vendor,linuxcompatible", it is
possible to boot this entry by setting blspec.compatible.extra to
"vendor,linuxcompatible".
More than one compatible is possible as well, the compatibles will need
to be space separated (since "," is already used for the vendor hardware
distinction).
Signed-off-by: Rouven Czerwinski <r.czerwinski@pengutronix.de>
---
common/blspec.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 56 insertions(+)
diff --git a/common/blspec.c b/common/blspec.c
index f8d47f20d2..e361a02333 100644
--- a/common/blspec.c
+++ b/common/blspec.c
@@ -19,10 +19,20 @@
#include <net.h>
#include <fs.h>
#include <of.h>
+#include <magicvar.h>
+#include <linux/list.h>
#include <linux/stat.h>
#include <linux/err.h>
#include <mtd/ubi-user.h>
+struct list_head *blspec_extra_list;
+char *blspec_extra_string;
+
+struct blspec_extra_entry {
+ char *compatible;
+ struct list_head list;
+};
+
/*
* blspec_entry_var_set - set a variable to a value
*/
@@ -830,8 +840,54 @@ static int blspec_bootentry_provider(struct bootentries *bootentries,
return found;
}
+static int blspec_extra_set(struct param_d *p, void *priv)
+{
+ struct blspec_extra_entry *entry, *tmp;
+ char *str = blspec_extra_string;
+ char *temp = str;
+ unsigned int len;
+
+ if (blspec_extra_list) {
+ list_for_each_entry_safe(entry, tmp, blspec_extra_list, list) {
+ list_del(&entry->list);
+ free(entry->compatible);
+ free(entry);
+ }
+ blspec_extra_list = NULL;
+ }
+
+ while(str++) {
+ if (*str == ' ' || *str == 0) {
+ len = temp - str;
+ if(len > 126) {
+ len = 127;
+ }
+ entry = calloc(1, sizeof(struct blspec_extra_entry));
+ entry->compatible = xzalloc(len + 1);
+ memcpy(entry->compatible, temp, len);
+
+ if (blspec_extra_list == NULL) {
+ INIT_LIST_HEAD(&entry->list);
+ blspec_extra_list = &entry->list;
+ } else {
+ list_add_tail(&entry->list, blspec_extra_list);
+ }
+
+ temp = str + 1;
+ }
+ if (*str == 0)
+ break;
+ }
+
+ return 0;
+}
+
static int blspec_init(void)
{
+ dev_add_param_string(&global_device, "blspec.compatible.extra",
+ blspec_extra_set, NULL, &blspec_extra_string, blspec_extra_list);
return bootentry_register_provider(blspec_bootentry_provider);
}
device_initcall(blspec_init);
+
+BAREBOX_MAGICVAR(global.blspec.compatible.extra, "Extra compatible to also match during bootloader entry matching");
--
2.42.0
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 2/3] blspec: add new blspec.compatible.extra variable
2023-09-20 12:37 ` [PATCH 2/3] blspec: add new blspec.compatible.extra variable Rouven Czerwinski
@ 2023-09-21 12:37 ` Ahmad Fatoum
2023-09-22 5:19 ` Ahmad Fatoum
2023-09-21 12:57 ` Sascha Hauer
1 sibling, 1 reply; 8+ messages in thread
From: Ahmad Fatoum @ 2023-09-21 12:37 UTC (permalink / raw)
To: Rouven Czerwinski, barebox
On 20.09.23 14:37, Rouven Czerwinski wrote:
> The blspec.compatible.extra variable will be used to specify extra
As this is only relevant when booting. Maybe prefix with boot.?
Not sure if that wouldn't be too long..
> compatibles that are also used to match during bootloader specification
> entry parsing. This means that even if your internal barebox device tree
> machine compatible is "vendor,bareboxcompatible", but your bootloader
> specification device tree contains "vendor,linuxcompatible", it is
> possible to boot this entry by setting blspec.compatible.extra to
> "vendor,linuxcompatible".
>
> More than one compatible is possible as well, the compatibles will need
> to be space separated (since "," is already used for the vendor hardware
> distinction).
>
> Signed-off-by: Rouven Czerwinski <r.czerwinski@pengutronix.de>
> ---
> common/blspec.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 56 insertions(+)
>
> diff --git a/common/blspec.c b/common/blspec.c
> index f8d47f20d2..e361a02333 100644
> --- a/common/blspec.c
> +++ b/common/blspec.c
> @@ -19,10 +19,20 @@
> #include <net.h>
> #include <fs.h>
> #include <of.h>
> +#include <magicvar.h>
> +#include <linux/list.h>
> #include <linux/stat.h>
> #include <linux/err.h>
> #include <mtd/ubi-user.h>
>
> +struct list_head *blspec_extra_list;
> +char *blspec_extra_string;
> +
> +struct blspec_extra_entry {
> + char *compatible;
> + struct list_head list;
> +};
> +
> /*
> * blspec_entry_var_set - set a variable to a value
> */
> @@ -830,8 +840,54 @@ static int blspec_bootentry_provider(struct bootentries *bootentries,
> return found;
> }
>
> +static int blspec_extra_set(struct param_d *p, void *priv)
> +{
> + struct blspec_extra_entry *entry, *tmp;
> + char *str = blspec_extra_string;
> + char *temp = str;
> + unsigned int len;
> +
> + if (blspec_extra_list) {
> + list_for_each_entry_safe(entry, tmp, blspec_extra_list, list) {
> + list_del(&entry->list);
> + free(entry->compatible);
> + free(entry);
> + }
> + blspec_extra_list = NULL;
> + }
> +
> + while(str++) {
> + if (*str == ' ' || *str == 0) {
Please use strsep (or strsep_unescaped) as elsewhere in blspec.c.
> + len = temp - str;
> + if(len > 126) {
> + len = 127;
> + }
> + entry = calloc(1, sizeof(struct blspec_extra_entry));
xzalloc if you don't check for !NULL.
> + entry->compatible = xzalloc(len + 1);
> + memcpy(entry->compatible, temp, len);
But here you could just use xstrdup?
> +
> + if (blspec_extra_list == NULL) {
> + INIT_LIST_HEAD(&entry->list);
> + blspec_extra_list = &entry->list;
This is unusal. Why not have blspec_extra_list be a list_head instead of a pointer?
> + } else {
> + list_add_tail(&entry->list, blspec_extra_list);
> + }
> +
> + temp = str + 1;
> + }
> + if (*str == 0)
> + break;
> + }
> +
> + return 0;
> +}
> +
> static int blspec_init(void)
> {
> + dev_add_param_string(&global_device, "blspec.compatible.extra",
> + blspec_extra_set, NULL, &blspec_extra_string, blspec_extra_list);
> return bootentry_register_provider(blspec_bootentry_provider);
> }
> device_initcall(blspec_init);
> +
> +BAREBOX_MAGICVAR(global.blspec.compatible.extra, "Extra compatible to also match during bootloader entry matching");
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 2/3] blspec: add new blspec.compatible.extra variable
2023-09-21 12:37 ` Ahmad Fatoum
@ 2023-09-22 5:19 ` Ahmad Fatoum
0 siblings, 0 replies; 8+ messages in thread
From: Ahmad Fatoum @ 2023-09-22 5:19 UTC (permalink / raw)
To: Rouven Czerwinski, barebox
Hi,
On 21.09.23 14:37, Ahmad Fatoum wrote:
> On 20.09.23 14:37, Rouven Czerwinski wrote:
>> The blspec.compatible.extra variable will be used to specify extra
>
> As this is only relevant when booting. Maybe prefix with boot.?
> Not sure if that wouldn't be too long..
We may also want to use this for other stuff than blspec in future,
e.g. FIT or matching overlays by compatible, so I think we should do away
with both blspec and boot and call it maybe global.of.compatible.extra?
If you agree, please define the variable in common/misc.c or some other
common location.
>
>> compatibles that are also used to match during bootloader specification
>> entry parsing. This means that even if your internal barebox device tree
>> machine compatible is "vendor,bareboxcompatible", but your bootloader
>> specification device tree contains "vendor,linuxcompatible", it is
>> possible to boot this entry by setting blspec.compatible.extra to
>> "vendor,linuxcompatible".
>>
>> More than one compatible is possible as well, the compatibles will need
>> to be space separated (since "," is already used for the vendor hardware
>> distinction).
>>
>> Signed-off-by: Rouven Czerwinski <r.czerwinski@pengutronix.de>
>> ---
>> common/blspec.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 56 insertions(+)
>>
>> diff --git a/common/blspec.c b/common/blspec.c
>> index f8d47f20d2..e361a02333 100644
>> --- a/common/blspec.c
>> +++ b/common/blspec.c
>> @@ -19,10 +19,20 @@
>> #include <net.h>
>> #include <fs.h>
>> #include <of.h>
>> +#include <magicvar.h>
>> +#include <linux/list.h>
>> #include <linux/stat.h>
>> #include <linux/err.h>
>> #include <mtd/ubi-user.h>
>>
>> +struct list_head *blspec_extra_list;
>> +char *blspec_extra_string;
>> +
>> +struct blspec_extra_entry {
>> + char *compatible;
>> + struct list_head list;
>> +};
>> +
>> /*
>> * blspec_entry_var_set - set a variable to a value
>> */
>> @@ -830,8 +840,54 @@ static int blspec_bootentry_provider(struct bootentries *bootentries,
>> return found;
>> }
>>
>> +static int blspec_extra_set(struct param_d *p, void *priv)
>> +{
>> + struct blspec_extra_entry *entry, *tmp;
>> + char *str = blspec_extra_string;
>> + char *temp = str;
>> + unsigned int len;
>> +
>> + if (blspec_extra_list) {
>> + list_for_each_entry_safe(entry, tmp, blspec_extra_list, list) {
>> + list_del(&entry->list);
>> + free(entry->compatible);
>> + free(entry);
>> + }
>> + blspec_extra_list = NULL;
>> + }
>> +
>> + while(str++) {
>> + if (*str == ' ' || *str == 0) {
>
> Please use strsep (or strsep_unescaped) as elsewhere in blspec.c.
>
>> + len = temp - str;
>> + if(len > 126) {
>> + len = 127;
>> + }
>> + entry = calloc(1, sizeof(struct blspec_extra_entry));
>
> xzalloc if you don't check for !NULL.
>
>> + entry->compatible = xzalloc(len + 1);
>> + memcpy(entry->compatible, temp, len);
>
> But here you could just use xstrdup?
>
>> +
>> + if (blspec_extra_list == NULL) {
>> + INIT_LIST_HEAD(&entry->list);
>> + blspec_extra_list = &entry->list;
>
> This is unusal. Why not have blspec_extra_list be a list_head instead of a pointer?
>
>> + } else {
>> + list_add_tail(&entry->list, blspec_extra_list);
>> + }
>> +
>> + temp = str + 1;
>> + }
>> + if (*str == 0)
>> + break;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int blspec_init(void)
>> {
>> + dev_add_param_string(&global_device, "blspec.compatible.extra",
>> + blspec_extra_set, NULL, &blspec_extra_string, blspec_extra_list);
>> return bootentry_register_provider(blspec_bootentry_provider);
>> }
>> device_initcall(blspec_init);
>> +
>> +BAREBOX_MAGICVAR(global.blspec.compatible.extra, "Extra compatible to also match during bootloader entry matching");
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] blspec: add new blspec.compatible.extra variable
2023-09-20 12:37 ` [PATCH 2/3] blspec: add new blspec.compatible.extra variable Rouven Czerwinski
2023-09-21 12:37 ` Ahmad Fatoum
@ 2023-09-21 12:57 ` Sascha Hauer
1 sibling, 0 replies; 8+ messages in thread
From: Sascha Hauer @ 2023-09-21 12:57 UTC (permalink / raw)
To: Rouven Czerwinski; +Cc: barebox
On Wed, Sep 20, 2023 at 02:37:19PM +0200, Rouven Czerwinski wrote:
> The blspec.compatible.extra variable will be used to specify extra
> compatibles that are also used to match during bootloader specification
> entry parsing. This means that even if your internal barebox device tree
> machine compatible is "vendor,bareboxcompatible", but your bootloader
> specification device tree contains "vendor,linuxcompatible", it is
> possible to boot this entry by setting blspec.compatible.extra to
> "vendor,linuxcompatible".
>
> More than one compatible is possible as well, the compatibles will need
> to be space separated (since "," is already used for the vendor hardware
> distinction).
>
> Signed-off-by: Rouven Czerwinski <r.czerwinski@pengutronix.de>
> ---
> common/blspec.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 56 insertions(+)
>
> diff --git a/common/blspec.c b/common/blspec.c
> index f8d47f20d2..e361a02333 100644
> --- a/common/blspec.c
> +++ b/common/blspec.c
> @@ -19,10 +19,20 @@
> #include <net.h>
> #include <fs.h>
> #include <of.h>
> +#include <magicvar.h>
> +#include <linux/list.h>
> #include <linux/stat.h>
> #include <linux/err.h>
> #include <mtd/ubi-user.h>
>
> +struct list_head *blspec_extra_list;
Replace with:
static LIST_HEAD(blspec_extra_list)
and fix the fallout.
> +char *blspec_extra_string;
Should also be static.
> +
> +struct blspec_extra_entry {
> + char *compatible;
> + struct list_head list;
> +};
> +
> /*
> * blspec_entry_var_set - set a variable to a value
> */
> @@ -830,8 +840,54 @@ static int blspec_bootentry_provider(struct bootentries *bootentries,
> return found;
> }
>
> +static int blspec_extra_set(struct param_d *p, void *priv)
> +{
Generally it seems unnecessary to use the setter to create a list from
these entries.
Instead you could just parse blspec_extra_string directly when you need it.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] blspec: take extra entries into account
2023-09-20 12:37 [PATCH 1/3] blspec: take compatible name as argument Rouven Czerwinski
2023-09-20 12:37 ` [PATCH 2/3] blspec: add new blspec.compatible.extra variable Rouven Czerwinski
@ 2023-09-20 12:37 ` Rouven Czerwinski
2023-09-21 12:33 ` [PATCH 1/3] blspec: take compatible name as argument Ahmad Fatoum
2023-09-21 12:38 ` Sascha Hauer
3 siblings, 0 replies; 8+ messages in thread
From: Rouven Czerwinski @ 2023-09-20 12:37 UTC (permalink / raw)
To: barebox; +Cc: Rouven Czerwinski
Wire up the possible extra entries during entry matching.
Signed-off-by: Rouven Czerwinski <r.czerwinski@pengutronix.de>
---
common/blspec.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/common/blspec.c b/common/blspec.c
index e361a02333..e950de4e99 100644
--- a/common/blspec.c
+++ b/common/blspec.c
@@ -512,7 +512,9 @@ int blspec_scan_file(struct bootentries *bootentries, const char *root,
const char *configname)
{
char *devname = NULL, *hwdevname = NULL;
+ struct blspec_extra_entry *extra;
struct blspec_entry *entry;
+ bool found;
if (blspec_have_entry(bootentries, configname))
return -EEXIST;
@@ -526,7 +528,16 @@ int blspec_scan_file(struct bootentries *bootentries, const char *root,
entry->configpath = xstrdup(configname);
entry->cdev = get_cdev_by_mountpath(root);
- if (!entry_is_of_compatible(entry, of_get_machine_compatible())) {
+ found = entry_is_of_compatible(entry, of_get_machine_compatible());
+ if (!found && blspec_extra_list) {
+ list_for_each_entry(extra, blspec_extra_list, list) {
+ found = entry_is_of_compatible(entry, extra->compatible);
+ if (found)
+ break;
+ }
+ }
+
+ if (!found) {
blspec_entry_free(&entry->entry);
return -ENODEV;
}
--
2.42.0
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/3] blspec: take compatible name as argument
2023-09-20 12:37 [PATCH 1/3] blspec: take compatible name as argument Rouven Czerwinski
2023-09-20 12:37 ` [PATCH 2/3] blspec: add new blspec.compatible.extra variable Rouven Czerwinski
2023-09-20 12:37 ` [PATCH 3/3] blspec: take extra entries into account Rouven Czerwinski
@ 2023-09-21 12:33 ` Ahmad Fatoum
2023-09-21 12:38 ` Sascha Hauer
3 siblings, 0 replies; 8+ messages in thread
From: Ahmad Fatoum @ 2023-09-21 12:33 UTC (permalink / raw)
To: Rouven Czerwinski, barebox
On 20.09.23 14:37, Rouven Czerwinski wrote:
> Instead of retrieving the root compatible itself, let
> entry_is_of_compatible take the compatible as an argument. Pass in
> of_get_machine_compatible for now, no functional changes.
>
> Signed-off-by: Rouven Czerwinski <r.czerwinski@pengutronix.de>
> ---
> common/blspec.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/common/blspec.c b/common/blspec.c
> index b70332a54c..f8d47f20d2 100644
> --- a/common/blspec.c
> +++ b/common/blspec.c
> @@ -422,13 +422,12 @@ static char *parse_nfs_url(const char *url)
> *
> * returns true if the entry is compatible, false otherwise
> */
> -static bool entry_is_of_compatible(struct blspec_entry *entry)
> +static bool entry_is_of_compatible(struct blspec_entry *entry, const char* compat)
> {
> const char *devicetree;
> const char *abspath;
> int ret;
> struct device_node *root = NULL, *barebox_root;
> - const char *compat;
> char *filename;
>
> /* If the entry doesn't specify a devicetree we are compatible */
> @@ -444,10 +443,6 @@ static bool entry_is_of_compatible(struct blspec_entry *entry)
> if (!barebox_root)
> return true;
>
> - ret = of_property_read_string(barebox_root, "compatible", &compat);
> - if (ret)
> - return false;
> -
> if (entry->rootpath)
> abspath = entry->rootpath;
> else
> @@ -521,7 +516,7 @@ int blspec_scan_file(struct bootentries *bootentries, const char *root,
> entry->configpath = xstrdup(configname);
> entry->cdev = get_cdev_by_mountpath(root);
>
> - if (!entry_is_of_compatible(entry)) {
> + if (!entry_is_of_compatible(entry, of_get_machine_compatible())) {
of_get_machine_compatible has an unfortunate name, as it strips away the vendor prefix,
so this change does introduce breakage.
> blspec_entry_free(&entry->entry);
> return -ENODEV;
> }
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/3] blspec: take compatible name as argument
2023-09-20 12:37 [PATCH 1/3] blspec: take compatible name as argument Rouven Czerwinski
` (2 preceding siblings ...)
2023-09-21 12:33 ` [PATCH 1/3] blspec: take compatible name as argument Ahmad Fatoum
@ 2023-09-21 12:38 ` Sascha Hauer
3 siblings, 0 replies; 8+ messages in thread
From: Sascha Hauer @ 2023-09-21 12:38 UTC (permalink / raw)
To: Rouven Czerwinski; +Cc: barebox
On Wed, Sep 20, 2023 at 02:37:18PM +0200, Rouven Czerwinski wrote:
> Instead of retrieving the root compatible itself, let
> entry_is_of_compatible take the compatible as an argument. Pass in
> of_get_machine_compatible for now, no functional changes.
>
> Signed-off-by: Rouven Czerwinski <r.czerwinski@pengutronix.de>
> ---
> common/blspec.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/common/blspec.c b/common/blspec.c
> index b70332a54c..f8d47f20d2 100644
> --- a/common/blspec.c
> +++ b/common/blspec.c
> @@ -422,13 +422,12 @@ static char *parse_nfs_url(const char *url)
> *
> * returns true if the entry is compatible, false otherwise
> */
> -static bool entry_is_of_compatible(struct blspec_entry *entry)
> +static bool entry_is_of_compatible(struct blspec_entry *entry, const char* compat)
> {
> const char *devicetree;
> const char *abspath;
> int ret;
> struct device_node *root = NULL, *barebox_root;
> - const char *compat;
> char *filename;
>
> /* If the entry doesn't specify a devicetree we are compatible */
> @@ -444,10 +443,6 @@ static bool entry_is_of_compatible(struct blspec_entry *entry)
> if (!barebox_root)
> return true;
>
> - ret = of_property_read_string(barebox_root, "compatible", &compat);
> - if (ret)
> - return false;
> -
> if (entry->rootpath)
> abspath = entry->rootpath;
> else
> @@ -521,7 +516,7 @@ int blspec_scan_file(struct bootentries *bootentries, const char *root,
> entry->configpath = xstrdup(configname);
> entry->cdev = get_cdev_by_mountpath(root);
>
> - if (!entry_is_of_compatible(entry)) {
> + if (!entry_is_of_compatible(entry, of_get_machine_compatible())) {
of_get_machine_compatible() returns the machine name without the vendor
prefix, i.e. from "fsl,imx6dl-sabrelite" it will make "imx6dl-sabrelite"
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 8+ messages in thread