Hi David,
On 3/20/19 5:38 PM, David Gibson wrote:
> On Wed, Mar 20, 2019 at 04:10:03PM +0100, Thierry Reding wrote:
>> From: Thierry Reding <treding@xxxxxxxxxx>
>>
>> The new fdt_generate_phandle() function can be used to generate a new,
>> unused phandle given a specific device tree blob. The implementation is
>> somewhat naive in that it simply walks the entire device tree to find
>> the highest phandle value and then returns a phandle value one higher
>> than that. A more clever implementation might try to find holes in the
>> current set of phandle values and fill them. But this implementation is
>> relatively simple and works reliably.
>>
>> Also add a test that validates that phandles generated by this new API
>> are indeed unique.
>>
>> Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
>
> Applied, thanks.
I would like for you to think of possibly reverting this patch. Or doing
so in about two weeks. I have started discussing with Thierry whether
there is better way of handling the use case. But I am going to be off
grid for a week, so that conversation will be on hold.
-Frank
>
>> ---
>> Changes in v3:
>> - add fdt_generate_phandle() to version.lds
>>
>> Changes in v2:
>> - Handle the case where the number of phandles has been exhausted. Also
>> add more testing to exercise this corner case. Note that this is tied
>> to the current, naive implementation of fdt_generate_phandle(). If it
>> is ever changed to be more clevel, the tests will have to be updated.
>> ---
>> libfdt/fdt_ro.c | 31 +++++++++++++++++++++++++++++++
>> libfdt/libfdt.h | 19 +++++++++++++++++++
>> libfdt/libfdt_env.h | 1 +
>> libfdt/version.lds | 1 +
>> tests/get_phandle.c | 31 ++++++++++++++++++++++++++++++-
>> tests/multilabel.dts | 5 +++++
>> tests/multilabel_merge.dts | 5 +++++
>> tests/references.c | 19 +++++++++++++++++--
>> tests/references.dts | 5 +++++
>> 9 files changed, 114 insertions(+), 3 deletions(-)
>>
>> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
>> index eafc14282892..97cdcda0dbec 100644
>> --- a/libfdt/fdt_ro.c
>> +++ b/libfdt/fdt_ro.c
>> @@ -170,6 +170,37 @@ uint32_t fdt_get_max_phandle(const void *fdt)
>> return 0;
>> }
>>
>> +int fdt_generate_phandle(const void *fdt, uint32_t *phandle)
>> +{
>> + uint32_t max = 0;
>> + int offset = -1;
>> +
>> + while (true) {
>> + uint32_t value;
>> +
>> + offset = fdt_next_node(fdt, offset, NULL);
>> + if (offset < 0) {
>> + if (offset == -FDT_ERR_NOTFOUND)
>> + break;
>> +
>> + return offset;
>> + }
>> +
>> + value = fdt_get_phandle(fdt, offset);
>> +
>> + if (value > max)
>> + max = value;
>> + }
>> +
>> + if (max == FDT_MAX_PHANDLE)
>> + return -FDT_ERR_NOPHANDLES;
>> +
>> + if (phandle)
>> + *phandle = max + 1;
>> +
>> + return 0;
>> +}
>> +
>> static const struct fdt_reserve_entry *fdt_mem_rsv(const void *fdt, int n)
>> {
>> int offset = n * sizeof(struct fdt_reserve_entry);
>> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
>> index a470d1df6d2a..7e102d188e86 100644
>> --- a/libfdt/libfdt.h
>> +++ b/libfdt/libfdt.h
>> @@ -140,6 +140,10 @@
>>
>> #define FDT_ERR_MAX 17
>>
>> +/* constants */
>> +#define FDT_MAX_PHANDLE 0xfffffffe
>> + /* Valid values for phandles range from 1 to 2^32-2. */
>> +
>> /**********************************************************************/
>> /* Low-level functions (you probably don't need these) */
>> /**********************************************************************/
>> @@ -376,6 +380,21 @@ const char *fdt_string(const void *fdt, int stroffset);
>> */
>> uint32_t fdt_get_max_phandle(const void *fdt);
>>
>> +/**
>> + * fdt_generate_phandle - return a new, unused phandle for a device tree blob
>> + * @fdt: pointer to the device tree blob
>> + * @phandle: return location for the new phandle
>> + *
>> + * Walks the device tree blob and looks for the highest phandle value. On
>> + * success, the new, unused phandle value (one higher than the previously
>> + * highest phandle value in the device tree blob) will be returned in the
>> + * @phandle parameter.
>> + *
>> + * Returns:
>> + * 0 on success or a negative error-code on failure
>> + */
>> +int fdt_generate_phandle(const void *fdt, uint32_t *phandle);
>> +
>> /**
>> * fdt_num_mem_rsv - retrieve the number of memory reserve map entries
>> * @fdt: pointer to the device tree blob
>> diff --git a/libfdt/libfdt_env.h b/libfdt/libfdt_env.h
>> index eb2053845c9c..4d1cdfa58547 100644
>> --- a/libfdt/libfdt_env.h
>> +++ b/libfdt/libfdt_env.h
>> @@ -52,6 +52,7 @@
>> * EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>> */
>>
>> +#include <stdbool.h>
>> #include <stddef.h>
>> #include <stdint.h>
>> #include <stdlib.h>
>> diff --git a/libfdt/version.lds b/libfdt/version.lds
>> index 9f5d70880f24..5897f1e288a3 100644
>> --- a/libfdt/version.lds
>> +++ b/libfdt/version.lds
>> @@ -66,6 +66,7 @@ LIBFDT_1.2 {
>> fdt_resize;
>> fdt_overlay_apply;
>> fdt_get_string;
>> + fdt_generate_phandle;
>> local:
>> *;
>> };
>> diff --git a/tests/get_phandle.c b/tests/get_phandle.c
>> index 22bd7b81b3f0..d87aee50b065 100644
>> --- a/tests/get_phandle.c
>> +++ b/tests/get_phandle.c
>> @@ -17,6 +17,7 @@
>> * License along with this library; if not, write to the Free Software
>> * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
>> */
>> +#include <stdbool.h>
>> #include <stdlib.h>
>> #include <stdio.h>
>> #include <string.h>
>> @@ -42,10 +43,32 @@ static void check_phandle(void *fdt, const char *path, uint32_t checkhandle)
>> path, phandle, checkhandle);
>> }
>>
>> +static void check_phandle_unique(const void *fdt, uint32_t checkhandle)
>> +{
>> + uint32_t phandle;
>> + int offset = -1;
>> +
>> + while (true) {
>> + offset = fdt_next_node(fdt, offset, NULL);
>> + if (offset < 0) {
>> + if (offset == -FDT_ERR_NOTFOUND)
>> + break;
>> +
>> + FAIL("error looking for phandle %#x", checkhandle);
>> + }
>> +
>> + phandle = fdt_get_phandle(fdt, offset);
>> +
>> + if (phandle == checkhandle)
>> + FAIL("generated phandle already exists");
>> + }
>> +}
>> +
>> int main(int argc, char *argv[])
>> {
>> - uint32_t max;
>> + uint32_t max, phandle;
>> void *fdt;
>> + int err;
>>
>> test_init(argc, argv);
>> fdt = load_blob_arg(argc, argv);
>> @@ -59,5 +82,11 @@ int main(int argc, char *argv[])
>> FAIL("fdt_get_max_phandle returned 0x%x instead of 0x%x\n",
>> max, PHANDLE_2);
>>
>> + err = fdt_generate_phandle(fdt, &phandle);
>> + if (err < 0)
>> + FAIL("failed to generate phandle: %d", err);
>> +
>> + check_phandle_unique(fdt, phandle);
>> +
>> PASS();
>> }
>> diff --git a/tests/multilabel.dts b/tests/multilabel.dts
>> index 77da06cc4174..d80ebe1fb082 100644
>> --- a/tests/multilabel.dts
>> +++ b/tests/multilabel.dts
>> @@ -41,4 +41,9 @@ m1: mq: /memreserve/ 0 0x1000;
>> n2 = &n2;
>> n3 = &n3;
>> };
>> +
>> + node6 {
>> + linux,phandle = <0xfffffffe>;
>> + phandle = <0xfffffffe>;
>> + };
>> };
>> diff --git a/tests/multilabel_merge.dts b/tests/multilabel_merge.dts
>> index 3e8029897405..a27d856f80dd 100644
>> --- a/tests/multilabel_merge.dts
>> +++ b/tests/multilabel_merge.dts
>> @@ -39,6 +39,11 @@ m1: mq: /memreserve/ 0 0x1000;
>> n2 = &n2;
>> n3 = &n3;
>> };
>> +
>> + node6 {
>> + linux,phandle = <0xfffffffe>;
>> + phandle = <0xfffffffe>;
>> + };
>> };
>>
>> / {
>> diff --git a/tests/references.c b/tests/references.c
>> index cab70f6579af..5b233a4bda20 100644
>> --- a/tests/references.c
>> +++ b/tests/references.c
>> @@ -76,8 +76,8 @@ static void check_rref(const void *fdt)
>> int main(int argc, char *argv[])
>> {
>> void *fdt;
>> - int n1, n2, n3, n4, n5;
>> - uint32_t h1, h2, h4, h5;
>> + int n1, n2, n3, n4, n5, n6, err;
>> + uint32_t h1, h2, h4, h5, h6, hn;
>>
>> test_init(argc, argv);
>> fdt = load_blob_arg(argc, argv);
>> @@ -97,11 +97,15 @@ int main(int argc, char *argv[])
>> n5 = fdt_path_offset(fdt, "/node5");
>> if (n5 < 0)
>> FAIL("fdt_path_offset(/node5): %s", fdt_strerror(n5));
>> + n6 = fdt_path_offset(fdt, "/node6");
>> + if (n6 < 0)
>> + FAIL("fdt_path_offset(/node6): %s", fdt_strerror(n6));
>>
>> h1 = fdt_get_phandle(fdt, n1);
>> h2 = fdt_get_phandle(fdt, n2);
>> h4 = fdt_get_phandle(fdt, n4);
>> h5 = fdt_get_phandle(fdt, n5);
>> + h6 = fdt_get_phandle(fdt, n6);
>>
>> if (h1 != 0x2000)
>> FAIL("/node1 has wrong phandle, 0x%x instead of 0x%x",
>> @@ -109,6 +113,9 @@ int main(int argc, char *argv[])
>> if (h2 != 0x1)
>> FAIL("/node2 has wrong phandle, 0x%x instead of 0x%x",
>> h2, 0x1);
>> + if (h6 != FDT_MAX_PHANDLE)
>> + FAIL("/node6 has wrong phandle, 0x%x instead of 0x%x",
>> + h6, FDT_MAX_PHANDLE);
>> if ((h4 == 0x2000) || (h4 == 0x1) || (h4 == 0))
>> FAIL("/node4 has bad phandle, 0x%x", h4);
>>
>> @@ -117,6 +124,14 @@ int main(int argc, char *argv[])
>> if ((h5 == h4) || (h5 == h2) || (h5 == h1))
>> FAIL("/node5 has duplicate phandle, 0x%x", h5);
>>
>> + /*
>> + * /node6 has phandle FDT_MAX_PHANDLE, so fdt_generate_phandle() is
>> + * expected to fail.
>> + */
>> + err = fdt_generate_phandle(fdt, &hn);
>> + if (err != -FDT_ERR_NOPHANDLES)
>> + FAIL("generated invalid phandle 0x%x\n", hn);
>> +
>> check_ref(fdt, n1, h2);
>> check_ref(fdt, n2, h1);
>> check_ref(fdt, n3, h4);
>> diff --git a/tests/references.dts b/tests/references.dts
>> index f783e8bc8643..b39063999189 100644
>> --- a/tests/references.dts
>> +++ b/tests/references.dts
>> @@ -33,4 +33,9 @@
>> linux,phandle = <&n5>;
>> phandle = <&n5>;
>> };
>> +
>> + node6 {
>> + linux,phandle = <0xfffffffe>;
>> + phandle = <0xfffffffe>;
>> + };
>> };
>
