of: Fix of_empty_ranges_quirk()
PATCH 1/1 of: Fix of_empty_ranges_quirk()
From: Marek Vasut <[email protected]>
The of_empty_ranges_quirk() returns a mix of boolean and signed integer types, which cannot work well. Replace that with boolean only and fix usage logic in of_translate_one() -- the check should trigger when the ranges are NULL and the quirk is applicable on the hardware.
--- drivers/of/address.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/of/address.c +5 -4
@@ -616,7 +616,7 @@ static struct of_bus *of_match_bus(struct device_node *np)return NULL ; } -static int of_empty_ranges_quirk (struct device_node *np) +static bool of_empty_ranges_quirk (struct device_node *np) {if (IS_ENABLED (CONFIG_PPC )) {/* To save cycles, we cache the result for global "Mac" setting */
@@ -631,7 +631,8 @@ static int of_empty_ranges_quirk(struct device_node *np) quirk_state =of_machine_is_compatible ("Power Macintosh" ) ||of_machine_is_compatible ("MacRISC" ); -return quirk_state; +if (quirk_state >0 ) +return true; }return false; }
@@ -662,8 +663,8 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,* This code is only enabled on powerpc. --gcl */ ranges =of_get_property (parent, rprop, &rlen); -if (ranges ==NULL && !of_empty_ranges_quirk (parent)) { -pr_debug ("no ranges; cannot translate\n" ); +if (ranges ==NULL &&of_empty_ranges_quirk (parent)) { +pr_err ("no ranges; cannot translate\n" );return 1 ; }if (ranges ==NULL || rlen ==0 ) {
Signature · 1 line
2.20.1
Quoted 7 lines · On Fri, Aug 09, 2019 at 07:33:21PM +0200, [email protected] wrote:
From: Marek Vasut <[email protected]>
The
of_empty_ranges_quirk()returns a mix of boolean and signed integer types, which cannot work well. Replace that with boolean only and fix usage logic inof_translate_one()-- the check should trigger when the ranges are NULL and the quirk is applicable on the hardware.
Quoted 5 lines
Signed-off-by Marek Vasut <[email protected]>Cc Rob Herring <[email protected]>Cc Frank Rowand <[email protected]>
Quoted 41 lines
--- drivers/of/address.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)drivers/of/address.c +5
-4@@ -616,7 +616,7 @@ static struct of_bus *of_match_bus(struct device_node *np)return NULL ; } -static int of_empty_ranges_quirk (struct device_node *np) +static bool of_empty_ranges_quirk (struct device_node *np) {if (IS_ENABLED (CONFIG_PPC )) {/* To save cycles, we cache the result for global "Mac" setting */ @@ -631,7 +631,8 @@ static int of_empty_ranges_quirk(struct device_node *np) quirk_state =of_machine_is_compatible ("Power Macintosh" ) ||of_machine_is_compatible ("MacRISC" ); -return quirk_state; +if (quirk_state >0 ) +return true; }return false; }@@ -662,8 +663,8 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,* This code is only enabled on powerpc. --gcl */ ranges =of_get_property (parent, rprop, &rlen); -if (ranges ==NULL && !of_empty_ranges_quirk (parent)) { -pr_debug ("no ranges; cannot translate\n" ); +if (ranges ==NULL &&of_empty_ranges_quirk (parent)) { +pr_err ("no ranges; cannot translate\n" );return 1 ; }if (ranges ==NULL || rlen ==0 ) {Signature · 1 line
2.20.1
Quoted 5 lines · On Fri, Aug 9, 2019 at 11:33 AM <[email protected]> wrote:
From: Marek Vasut <[email protected]>
The
of_empty_ranges_quirk()returns a mix of boolean and signed integer types, which cannot work well.
It never returns a negative. The negative is used as an uninitialized flag. Note quirk_state is static.
Quoted 37 lines
Replace that with boolean only and fix
usage logic inof_translate_one()-- the check should trigger when the ranges are NULL and the quirk is applicable on the hardware.Signed-off-by Marek Vasut <[email protected]>Cc Rob Herring <[email protected]>Cc Frank Rowand <[email protected]>--- drivers/of/address.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)drivers/of/address.c +3
-2@@ -616,7 +616,7 @@ static struct of_bus *of_match_bus(struct device_node *np)return NULL ; } -static int of_empty_ranges_quirk (struct device_node *np) +static bool of_empty_ranges_quirk (struct device_node *np) {if (IS_ENABLED (CONFIG_PPC )) {/* To save cycles, we cache the result for global "Mac" setting */ @@ -631,7 +631,8 @@ static int of_empty_ranges_quirk(struct device_node *np) quirk_state =of_machine_is_compatible ("Power Macintosh" ) ||of_machine_is_compatible ("MacRISC" ); -return quirk_state; +if (quirk_state >0 ) +return true; }return false; }
drivers/of/address.c static int of_translate_one(struct device_node *parent, struct of_bus *bus, · line 667
Quoted 7 lines
drivers/of/address.c +2
-2@@ -662,8 +663,8 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,* This code is only enabled on powerpc. --gcl */ ranges =of_get_property (parent, rprop, &rlen); -if (ranges ==NULL && !of_empty_ranges_quirk (parent)) { -pr_debug ("no ranges; cannot translate\n" ); +if (ranges ==NULL &&of_empty_ranges_quirk (parent)) { +pr_err ("no ranges; cannot translate\n" );
This is wrong. If you have NULL ranges and not the quirk, then no ranges is an error. IOW, if you are getting an error here, you have an error in your DT (because I assume you are not working on a PASemi or Apple system).
Rob
Quoted 9 lines · Rob Herring
Quoted 5 lines · On Fri, Aug 9, 2019 at 11:33 AM <[email protected]> wrote:
From: Marek Vasut <[email protected]>
The
of_empty_ranges_quirk()returns a mix of boolean and signed integer types, which cannot work well.It never returns a negative. The negative is used as an uninitialized flag. Note
quirk_stateis static.
It's still mixing boolean and signed int types though, which isn't right.
Quoted 41 lines
Quoted 41 lines
Replace that with boolean only and fix
usage logic inof_translate_one()-- the check should trigger when the ranges are NULL and the quirk is applicable on the hardware.Signed-off-by Marek Vasut <[email protected]>Cc Rob Herring <[email protected]>Cc Frank Rowand <[email protected]>--- drivers/of/address.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)drivers/of/address.c +3
-3@@ -616,7 +616,7 @@ static struct of_bus *of_match_bus(struct device_node *np)return NULL ; } -static int of_empty_ranges_quirk (struct device_node *np) +static bool of_empty_ranges_quirk (struct device_node *np) {if (IS_ENABLED (CONFIG_PPC )) {/* To save cycles, we cache the result for global "Mac" setting */ @@ -631,7 +631,8 @@ static int of_empty_ranges_quirk(struct device_node *np) quirk_state =of_machine_is_compatible ("Power Macintosh" ) ||of_machine_is_compatible ("MacRISC" ); -return quirk_state; +if (quirk_state >0 ) +return true; }return false; }@@ -662,8 +663,8 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,* This code is only enabled on powerpc. --gcl */ ranges =of_get_property (parent, rprop, &rlen); -if (ranges ==NULL && !of_empty_ranges_quirk (parent)) {
Quoted 8 lines
drivers/of/address.c static int of_translate_one(struct device_node *parent, struct of_bus *bus, · line 667
Quoted 3 lines
drivers/of/address.c +2
-1-pr_debug ("no ranges; cannot translate\n" ); +if (ranges ==NULL &&of_empty_ranges_quirk (parent)) { +pr_err ("no ranges; cannot translate\n" );This is wrong. If you have NULL ranges and not the quirk, then no ranges is an error. IOW, if you are getting an error here, you have an error in your DT (because I assume you are not working on a PASemi or Apple system).
The way I understand the code is that
if (you have no ranges in the DT) AND (the quirk is applicable) then print the message. Which is what this patch does.
Am I missing something ?
Signature · 2 lines
Best regards, Marek Vasut
Quoted 8 lines · Marek Vasut
Quoted 7 lines · Rob Herring
Quoted 5 lines · On Fri, Aug 9, 2019 at 11:33 AM <[email protected]> wrote:
From: Marek Vasut <[email protected]>
The
of_empty_ranges_quirk()returns a mix of boolean and signed integer types, which cannot work well.
Quoted 6 lines
Quoted 2 lines
It never returns a negative. The negative is used as an uninitialized flag. Note
quirk_stateis static.It's still mixing boolean and signed int types though, which isn't right.
From a code readability aspect, Marek is correct.
The code author used "stupid (or clever) coding tricks" (tm) to save a little bit of memory. A more readable implementation would be:
static bool of_empty_ranges_quirk(struct device_node *np)
{
/*
* As far as we know, the missing "ranges" problem only exists on Apple
* machines, so only enable the exception on powerpc. --gcl
*/
if (IS_ENABLED(CONFIG_PPC)) {
/* Cache the result for global "Mac" setting */
static int quirk_state_initialized = 0;
static bool quirk_state;
/* PA-SEMI sdc DT bug */
if (of_device_is_compatible(np, "1682m-sdc"))
return true;
if (!quirk_state_initialized)
quirk_state_initialized = 1;
quirk_state =
of_machine_is_compatible("Power Macintosh") ||
of_machine_is_compatible("MacRISC");
return quirk_state;
}
return false;
}I would also rename of_empty_ranges_quirk() to something like of_missing_ranges_is_ok() or of_missing_ranges_allowed(). "quirk" does not convey any useful information while my proposed rename describes what the function is actually checking for.
The comment that I added is currently in the caller of of_empty_ranges_quirk(), but instead belongs in of_empty_ranges_quirk(). When I read that comment in of_translate_one(), my reaction was to look for the check for powerpc in of_translate_one() and to be puzzled when I could not find it. I also modified the comment for the changed context. Thus the "--gcl" portion of the comment should also be removed from of_translate_one().
The more readable implementation (IMNSHO) uses slightly more memory and slightly more code, but it is more direct about what it is doing and thus more readable.
-Frank
Quoted 57 lines
Quoted 42 lines
Quoted 42 lines
Replace that with boolean only and fix
usage logic inof_translate_one()-- the check should trigger when the ranges are NULL and the quirk is applicable on the hardware.Signed-off-by Marek Vasut <[email protected]>Cc Rob Herring <[email protected]>Cc Frank Rowand <[email protected]>--- drivers/of/address.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)drivers/of/address.c +3
-3@@ -616,7 +616,7 @@ static struct of_bus *of_match_bus(struct device_node *np)return NULL ; } -static int of_empty_ranges_quirk (struct device_node *np) +static bool of_empty_ranges_quirk (struct device_node *np) {if (IS_ENABLED (CONFIG_PPC )) {/* To save cycles, we cache the result for global "Mac" setting */ @@ -631,7 +631,8 @@ static int of_empty_ranges_quirk(struct device_node *np) quirk_state =of_machine_is_compatible ("Power Macintosh" ) ||of_machine_is_compatible ("MacRISC" ); -return quirk_state; +if (quirk_state >0 ) +return true; }return false; }@@ -662,8 +663,8 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,* This code is only enabled on powerpc. --gcl */ ranges =of_get_property (parent, rprop, &rlen); -if (ranges ==NULL && !of_empty_ranges_quirk (parent)) {Quoted 8 lines
drivers/of/address.c static int of_translate_one(struct device_node *parent, struct of_bus *bus, · line 667
Quoted 3 lines
drivers/of/address.c +2
-1-pr_debug ("no ranges; cannot translate\n" ); +if (ranges ==NULL &&of_empty_ranges_quirk (parent)) { +pr_err ("no ranges; cannot translate\n" );This is wrong. If you have NULL ranges and not the quirk, then no ranges is an error. IOW, if you are getting an error here, you have an error in your DT (because I assume you are not working on a PASemi or Apple system).
The way I understand the code is that
if (you have no ranges in the DT) AND (the quirk is applicable) then print the message. Which is what this patch does.Am I missing something ?
Quoted 59 lines · Frank Rowand
Quoted 5 lines · Marek Vasut
Quoted 4 lines · Rob Herring
Quoted 3 lines · On Fri, Aug 9, 2019 at 11:33 AM <[email protected]> wrote:
From: Marek Vasut <[email protected]>
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
The
of_empty_ranges_quirk()returns a mix of boolean and signed integer types, which cannot work well.It never returns a negative. The negative is used as an uninitialized flag. Note
quirk_stateis static.It's still mixing boolean and signed int types though, which isn't right.
From a code readability aspect, Marek is correct.
The code author used "stupid (or clever) coding tricks" (tm) to save a little bit of memory. A more readable implementation would be:
static bool
of_empty_ranges_quirk(struct device_node *np){ /* * As far as we know, the missing "ranges" problem only exists on Apple * machines, so only enable the exception on powerpc. --gcl */ if (IS_ENABLED(CONFIG_PPC)) { /* Cache the result for global "Mac" setting */ static int quirk_state_initialized = 0; static bool quirk_state; /* PA-SEMI sdc DT bug */ if (of_device_is_compatible(np, "1682m-sdc")) return true; if (!quirk_state_initialized) quirk_state_initialized = 1; quirk_state = of_machine_is_compatible("Power Macintosh") || of_machine_is_compatible("MacRISC"); return quirk_state; } return false; }I would also rename
of_empty_ranges_quirk()to something likeof_missing_ranges_is_ok()orof_missing_ranges_allowed(). "quirk" does not convey any useful information while my proposed rename describes what the function is actually checking for.The comment that I added is currently in the caller of
of_empty_ranges_quirk(), but instead belongs inof_empty_ranges_quirk(). When I read that comment inof_translate_one(), my reaction was to look for the check for powerpc inof_translate_one()and to be puzzled when I could not find it. I also modified the comment for the changed context. Thus the "--gcl" portion of the comment should also be removed fromof_translate_one().
Quoted 3 lines
The more readable implementation (IMNSHO) uses slightly more memory and slightly more code, but it is more direct about what it is doing and thus more readable.
Thanks for the input, sorry for the delay, let me send a V2.
Signature · 2 lines
Best regards, Marek Vasut
Quoted 6 lines · Marek Vasut
Quoted 4 lines · Rob Herring
Quoted 3 lines · On Fri, Aug 9, 2019 at 11:33 AM <[email protected]> wrote:
From: Marek Vasut <[email protected]>
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
The
of_empty_ranges_quirk()returns a mix of boolean and signed integer types, which cannot work well.It never returns a negative. The negative is used as an uninitialized flag. Note
quirk_stateis static.It's still mixing boolean and signed int types though, which isn't right.
I'm really only interested in touching this code if it is too remove it. But some reason people still run 1990s Macs.
Quoted 50 lines
Quoted 41 lines
Quoted 41 lines
Replace that with boolean only and fix
usage logic inof_translate_one()-- the check should trigger when the ranges are NULL and the quirk is applicable on the hardware.Signed-off-by Marek Vasut <[email protected]>Cc Rob Herring <[email protected]>Cc Frank Rowand <[email protected]>--- drivers/of/address.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)drivers/of/address.c +3
-3@@ -616,7 +616,7 @@ static struct of_bus *of_match_bus(struct device_node *np)return NULL ; } -static int of_empty_ranges_quirk (struct device_node *np) +static bool of_empty_ranges_quirk (struct device_node *np) {if (IS_ENABLED (CONFIG_PPC )) {/* To save cycles, we cache the result for global "Mac" setting */ @@ -631,7 +631,8 @@ static int of_empty_ranges_quirk(struct device_node *np) quirk_state =of_machine_is_compatible ("Power Macintosh" ) ||of_machine_is_compatible ("MacRISC" ); -return quirk_state; +if (quirk_state >0 ) +return true; }return false; }@@ -662,8 +663,8 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,* This code is only enabled on powerpc. --gcl */ ranges =of_get_property (parent, rprop, &rlen); -if (ranges ==NULL && !of_empty_ranges_quirk (parent)) {Quoted 8 lines
drivers/of/address.c static int of_translate_one(struct device_node *parent, struct of_bus *bus, · line 667
Quoted 3 lines
drivers/of/address.c +2
-1-pr_debug ("no ranges; cannot translate\n" ); +if (ranges ==NULL &&of_empty_ranges_quirk (parent)) { +pr_err ("no ranges; cannot translate\n" );This is wrong. If you have NULL ranges and not the quirk, then no ranges is an error. IOW, if you are getting an error here, you have an error in your DT (because I assume you are not working on a PASemi or Apple system).
Quoted 3 lines
The way I understand the code is that
if (you have no ranges in the DT) AND (the quirk is applicable) then print the message. Which is what this patch does.
Your understanding is wrong.
Quoted 1 line
Am I missing something ?
The normal case is you must have ranges to translate addresses. If you don't have ranges (say for I2C addresses), then you shouldn't be in this code. The quirk is for when there is not a ranges property but should be. IOW, if the quirk is true, then pretend there is an empty ranges (1:1 translation) property and continue to translate the address.
Rob