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

[Fabian Pflug <f.pflug@pengutronix.de>]

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.

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

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

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)

Kind regard
Fabian


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