Re: [PATCH 3/4] bootm: use new api to get kernel command line params

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hi,

On 11/26/25 12:29 PM, Fabian Pflug wrote:
> Hey :)
> 
> On Wed, 2025-11-26 at 11:25 +0100, Ahmad Fatoum wrote:
>>
>> On 11/26/25 7:42 AM, Fabian Pflug wrote:
>>> Getting root device adn root kernel options seperatily any combining
>>
>> device and root kernel options seperatly and combining
>>
>>> them in this function instead of getting the full string from the helper
>>> functions. This is in preperation for replacing the root= string in the
>>
>> preparation
>>
>>> kernel commandline with something defined during runtime.
>>>
>>> Signed-off-by: Fabian Pflug <f.pflug@pengutronix.de>
>>> ---
>>>  common/bootm.c | 34 ++++++++++++++++++++--------------
>>>  1 file changed, 20 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/common/bootm.c b/common/bootm.c
>>> index 17792b2a1d..5d7dc00e3e 100644
>>> --- a/common/bootm.c
>>> +++ b/common/bootm.c
>>> @@ -4,6 +4,7 @@
>>>  #include <bootm.h>
>>>  #include <bootm-overrides.h>
>>>  #include <fs.h>
>>> +#include <fcntl.h>
>>>  #include <malloc.h>
>>>  #include <memory.h>
>>>  #include <block.h>
>>> @@ -841,33 +842,38 @@ int bootm_boot(struct bootm_data *bootm_data)
>>>  	}
>>>  
>>>  	if (bootm_data->appendroot) {
>>> -		char *rootarg;
>>> +		char *root;
>>> +		char *rootopts;
>>>  
>>>  		if (bootm_data->root_dev) {
>>>  			const char *root_dev_name = devpath_to_name(bootm_data->root_dev);
>>>  			struct cdev *root_cdev = cdev_open_by_name(root_dev_name, O_RDONLY);
>>>  
>>> -			rootarg = cdev_get_linux_rootarg(root_cdev);
>>> -			if (!rootarg) {
>>> -				rootarg = ERR_PTR(-EINVAL);
>>> -
>>> -				if (!root_cdev)
>>> -					pr_err("no cdev found for %s, cannot set root= option\n",
>>> -						root_dev_name);
>>> -				else if (!root_cdev->partuuid[0])
>>> -					pr_err("%s doesn't have a PARTUUID, cannot set root= option\n",
>>> -						root_dev_name);
>>
>> This warning is useful, because it's very confusing otherwise when a MBR
>> disk image is created without a NT signature.
>>
>>> -			}
>>> +			cdev_get_linux_root_and_opts(root_cdev, &root, &rootopts);
>>>  
>>>  			if (root_cdev)
>>>  				cdev_close(root_cdev);
>>> +			else
>>> +				pr_err("no cdev found for %s, cannot set root= option\n", root_dev_name);
>>>  		} else {
>>> -			rootarg = path_get_linux_rootarg(data->os_file);
>>
>> path_get_linux_rootarg() is stale now right? Can you squash its removal
>> into the patch that gets rid of linux.bootargs?
> 
> Then we would have a case, where the function is used, but not available anymore resulting in an unbuildable system,
> which I would like to avoid. I can remove it in an extra commit after this one.

You will have of course to change the users not to call it anymore.
If you add a later commit removing it, you still have an intermittent
breakage that would trip up a bisect or am I missing something?

> 
>>
>>> +			struct fs_device *fsdev = get_fsdevice_by_path(AT_FDCWD, data->os_file);
>>> +			if(fsdev)
>>> +				fsdev_get_linux_root_options(fsdev, &root, &rootopts);
>>> +			else
>>> +				pr_err("no fsdevice under path: %s\n", data->os_file);
>>>  		}
>>>  
>>> -		if (IS_ERR(rootarg)) {
>>> +		if (!root) {
>>>  			pr_err("Failed to append kernel cmdline parameter 'root='\n");
>>>  		} else {
>>> +			char *rootarg;
>>> +			if(rootopts) {
>>> +				rootarg = xasprintf("root=%s %s", root, rootopts);
>>> +				free(rootopts);
>>> +			} else {
>>> +				rootarg = xasprintf("root=%s", root);
>>> +			}
>>
>> I wonder if we should add a helper into a header that does this, given
>> how many times this is repeated...
>>
>> char *format_root_bootarg(char *root, char *rootopts);
>>
>> which consumes (and frees) two pointers and returns a new buffer..
> 
> Good thinking. But where do we add it?
> * fs.h
> * block.h
> * bootargs.h

bootargs.h sounds suitable.

> 
> And also, do we add a third parameter? because I will re replaced in the next commit with "%s=%s %s"

Noticed that too when I read that far. Ye, three arguments sounds good
to me.

> And since you proposed to get rid of path_get_linux_rootarg it will be used with two parameters in one place
> (cdev_get_linux_rootarg) and with three at the other. (bootm_boot)

Can't follow, but I might when I see the code :-)

Cheers,
Ahmad

> 
> Kind regard
> Fabian
> 
> 
>>
>> Cheers,
>> Ahmad
>>
>>> +			free(root);
>>>  			pr_info("Adding \"%s\" to Kernel commandline\n", rootarg);
>>>  			globalvar_add_simple("linux.bootargs.bootm.appendroot",
>>>  					     rootarg);
> 

-- 
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 |