[PATCH] Revert "cmd: pxe_utils: Check fdtcontroladdr in label_boot"

[PATCH] Revert "cmd: pxe_utils: Check fdtcontroladdr in label_boot"

[Tom Rini]

With the change here, all extlinux.conf files with only "KERNEL
/fitImage" don't work anymore. One common example of this would be those
files generated by thee Poky/OE WIC bootimg-partition bootloader
partition generator.

This reverts commit d5ba6188dfbf6bb68354bec86e483623f1f6dae2.

Reported-by: Neil Armstrong <neil.armstrong@linaro.org>
Reported-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
Signed-off-by: Tom Rini <trini@konsulko.com>
---
 boot/pxe_utils.c   | 8 +-------
 drivers/net/tsec.c | 2 +-
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
index 8133006875f9..96528aa14c03 100644
--- a/boot/pxe_utils.c
+++ b/boot/pxe_utils.c
@@ -616,10 +616,7 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label)
 	 * Scenario 2: If there is an fdt_addr specified, pass it along to
 	 * bootm, and adjust argc appropriately.
 	 *
-	 * Scenario 3: If there is an fdtcontroladdr specified, pass it along to
-	 * bootm, and adjust argc appropriately.
-	 *
-	 * Scenario 4: fdt blob is not available.
+	 * Scenario 3: fdt blob is not available.
 	 */
 	bootm_argv[3] = env_get("fdt_addr_r");
 
@@ -724,9 +721,6 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label)
 	if (!bootm_argv[3])
 		bootm_argv[3] = env_get("fdt_addr");
 
-	if (!bootm_argv[3])
-		bootm_argv[3] = env_get("fdtcontroladdr");
-
 	if (bootm_argv[3]) {
 		if (!bootm_argv[2])
 			bootm_argv[2] = "-";
diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c
index d69a9ff47736..519ea14b070e 100644
--- a/drivers/net/tsec.c
+++ b/drivers/net/tsec.c
@@ -156,7 +156,7 @@ static int tsec_mcast_addr(struct udevice *dev, const u8 *mcast_mac, int join)
 	return 0;
 }
 
-static int __maybe_unused tsec_set_promisc(struct udevice *dev, bool enable)
+static int tsec_set_promisc(struct udevice *dev, bool enable)
 {
 	struct tsec_private *priv = dev_get_priv(dev);
 	struct tsec __iomem *regs = priv->regs;
-- 
2.25.1

Re: [PATCH] Revert "cmd: pxe_utils: Check fdtcontroladdr in label_boot"

[Quentin Schulz]

Hi Tom,

On 12/13/22 15:29, Tom Rini wrote:
> With the change here, all extlinux.conf files with only "KERNEL
> /fitImage" don't work anymore. One common example of this would be those
> files generated by thee Poky/OE WIC bootimg-partition bootloader
> partition generator.
> 
> This reverts commit d5ba6188dfbf6bb68354bec86e483623f1f6dae2.
> 

FYI, this patch has been in U-Boot for three releases already, hence why 
I was reluctant to ask for a revert when I discovered the issue (that 
Neil already had reported in August, well before me).

Adding people who worked on the reverted patch in Cc so they can give 
their 2¢ on it.

Cheers,
Quentin

> Reported-by: Neil Armstrong <neil.armstrong@linaro.org>
> Reported-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> Signed-off-by: Tom Rini <trini@konsulko.com>
> ---
>   boot/pxe_utils.c   | 8 +-------
>   drivers/net/tsec.c | 2 +-
>   2 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
> index 8133006875f9..96528aa14c03 100644
> --- a/boot/pxe_utils.c
> +++ b/boot/pxe_utils.c
> @@ -616,10 +616,7 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label)
>   	 * Scenario 2: If there is an fdt_addr specified, pass it along to
>   	 * bootm, and adjust argc appropriately.
>   	 *
> -	 * Scenario 3: If there is an fdtcontroladdr specified, pass it along to
> -	 * bootm, and adjust argc appropriately.
> -	 *
> -	 * Scenario 4: fdt blob is not available.
> +	 * Scenario 3: fdt blob is not available.
>   	 */
>   	bootm_argv[3] = env_get("fdt_addr_r");
>   
> @@ -724,9 +721,6 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label)
>   	if (!bootm_argv[3])
>   		bootm_argv[3] = env_get("fdt_addr");
>   
> -	if (!bootm_argv[3])
> -		bootm_argv[3] = env_get("fdtcontroladdr");
> -
>   	if (bootm_argv[3]) {
>   		if (!bootm_argv[2])
>   			bootm_argv[2] = "-";
> diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c
> index d69a9ff47736..519ea14b070e 100644
> --- a/drivers/net/tsec.c
> +++ b/drivers/net/tsec.c
> @@ -156,7 +156,7 @@ static int tsec_mcast_addr(struct udevice *dev, const u8 *mcast_mac, int join)
>   	return 0;
>   }
>   
> -static int __maybe_unused tsec_set_promisc(struct udevice *dev, bool enable)
> +static int tsec_set_promisc(struct udevice *dev, bool enable)
>   {
>   	struct tsec_private *priv = dev_get_priv(dev);
>   	struct tsec __iomem *regs = priv->regs;

Re: [PATCH] Revert "cmd: pxe_utils: Check fdtcontroladdr in label_boot"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 834 bytes --]

On Tue, Dec 13, 2022 at 03:34:45PM +0100, Quentin Schulz wrote:
> Hi Tom,
> 
> On 12/13/22 15:29, Tom Rini wrote:
> > With the change here, all extlinux.conf files with only "KERNEL
> > /fitImage" don't work anymore. One common example of this would be those
> > files generated by thee Poky/OE WIC bootimg-partition bootloader
> > partition generator.
> > 
> > This reverts commit d5ba6188dfbf6bb68354bec86e483623f1f6dae2.
> > 
> 
> FYI, this patch has been in U-Boot for three releases already, hence why I
> was reluctant to ask for a revert when I discovered the issue (that Neil
> already had reported in August, well before me).
> 
> Adding people who worked on the reverted patch in Cc so they can give their
> 2¢ on it.

Sigh, I misreaad what git describe showed me as 202_3_.01 not 202_2_.01.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Revert "cmd: pxe_utils: Check fdtcontroladdr in label_boot"

[Peter Hoyes]

On 13/12/2022 14:34, Quentin Schulz wrote:
> Hi Tom,
>
> On 12/13/22 15:29, Tom Rini wrote:
>> With the change here, all extlinux.conf files with only "KERNEL
>> /fitImage" don't work anymore. One common example of this would be those
>> files generated by thee Poky/OE WIC bootimg-partition bootloader
>> partition generator.
>>
>> This reverts commit d5ba6188dfbf6bb68354bec86e483623f1f6dae2.
>>
>
> FYI, this patch has been in U-Boot for three releases already, hence 
> why I was reluctant to ask for a revert when I discovered the issue 
> (that Neil already had reported in August, well before me).
>
> Adding people who worked on the reverted patch in Cc so they can give 
> their 2¢ on it.

Sending a quick reply (as the original author of this patch) as I likely 
won't have bandwidth to investigate this properly until 2023.

The use case here (PXE with fdtcontroladdr) is still occasionally useful 
for us as we don't use FIT images but instead bundle U-Boot and the 
device tree in a prior boot stage, typically Trusted Firmware-A. As PXE 
is not regularly used in our team though, I think we'd be open to this 
revert in the short term so long as we can agree on a better design to 
be implemented in the medium term.

We are also active Poky/OE users here, but I think this more about 
supporting different options for device tree storage.

Peter

>
> Cheers,
> Quentin
>
>> Reported-by: Neil Armstrong <neil.armstrong@linaro.org>
>> Reported-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>> Signed-off-by: Tom Rini <trini@konsulko.com>
>> ---
>>   boot/pxe_utils.c   | 8 +-------
>>   drivers/net/tsec.c | 2 +-
>>   2 files changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
>> index 8133006875f9..96528aa14c03 100644
>> --- a/boot/pxe_utils.c
>> +++ b/boot/pxe_utils.c
>> @@ -616,10 +616,7 @@ static int label_boot(struct pxe_context *ctx, 
>> struct pxe_label *label)
>>        * Scenario 2: If there is an fdt_addr specified, pass it along to
>>        * bootm, and adjust argc appropriately.
>>        *
>> -     * Scenario 3: If there is an fdtcontroladdr specified, pass it 
>> along to
>> -     * bootm, and adjust argc appropriately.
>> -     *
>> -     * Scenario 4: fdt blob is not available.
>> +     * Scenario 3: fdt blob is not available.
>>        */
>>       bootm_argv[3] = env_get("fdt_addr_r");
>>   @@ -724,9 +721,6 @@ static int label_boot(struct pxe_context *ctx, 
>> struct pxe_label *label)
>>       if (!bootm_argv[3])
>>           bootm_argv[3] = env_get("fdt_addr");
>>   -    if (!bootm_argv[3])
>> -        bootm_argv[3] = env_get("fdtcontroladdr");
>> -
>>       if (bootm_argv[3]) {
>>           if (!bootm_argv[2])
>>               bootm_argv[2] = "-";
>> diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c
>> index d69a9ff47736..519ea14b070e 100644
>> --- a/drivers/net/tsec.c
>> +++ b/drivers/net/tsec.c
>> @@ -156,7 +156,7 @@ static int tsec_mcast_addr(struct udevice *dev, 
>> const u8 *mcast_mac, int join)
>>       return 0;
>>   }
>>   -static int __maybe_unused tsec_set_promisc(struct udevice *dev, 
>> bool enable)
>> +static int tsec_set_promisc(struct udevice *dev, bool enable)
>>   {
>>       struct tsec_private *priv = dev_get_priv(dev);
>>       struct tsec __iomem *regs = priv->regs;