| Message ID | 20210309205624.239253-1-sean.anderson@seco.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | cmd: fs: Use part_get_info_by_dev_and_name_or_num to parse partitions | expand |
Am 9. März 2021 21:56:24 MEZ schrieb Sean Anderson <sean.anderson@seco.com>: >This allows using dev#partlabel syntax. Allowing more widespread use of that syntax makes sense to me. Unfortunately you do not mention which commands and devices are affected. > >Signed-off-by: Sean Anderson <sean.anderson@seco.com> >--- > > fs/fs.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > >diff --git a/fs/fs.c b/fs/fs.c >index 900928c394..b7936fd4cf 100644 >--- a/fs/fs.c >+++ b/fs/fs.c >@@ -385,8 +385,8 @@ int fs_set_blk_dev(const char *ifname, const char >*dev_part_str, int fstype) > } > #endif > >- part = blk_get_device_part_str(ifname, dev_part_str, &fs_dev_desc, >- &fs_partition, 1); >+ part = part_get_info_by_dev_and_name_or_num(ifname, dev_part_str, >&fs_dev_desc, >+ &fs_partition, 1); > if (part < 0) > return -1; > It would be good if we could test the new capabilities. Can we make use of sandbox_mmc.c? Best regards Heinrich
On 3/9/21 4:21 PM, Heinrich Schuchardt wrote: > Am 9. März 2021 21:56:24 MEZ schrieb Sean Anderson <sean.anderson@seco.com>: >> This allows using dev#partlabel syntax. > > Allowing more widespread use of that syntax makes sense to me. > > Unfortunately you do not mention which commands and devices are affected. I enabled it so that cmd/fs.c can use it (e.g. CMD_FS_GENERIC). It seems like I overlooked that this was in fs/fs.c and not cmd/fs.c. Affected functionality should be most filesystem commands (due to do_load), DFU, fastboot, and splash screen. I suspect this affects almost every device using U-Boot. > >> >> Signed-off-by: Sean Anderson <sean.anderson@seco.com> >> --- >> >> fs/fs.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/fs/fs.c b/fs/fs.c >> index 900928c394..b7936fd4cf 100644 >> --- a/fs/fs.c >> +++ b/fs/fs.c >> @@ -385,8 +385,8 @@ int fs_set_blk_dev(const char *ifname, const char >> *dev_part_str, int fstype) >> } >> #endif >> >> - part = blk_get_device_part_str(ifname, dev_part_str, &fs_dev_desc, >> - &fs_partition, 1); >> + part = part_get_info_by_dev_and_name_or_num(ifname, dev_part_str, >> &fs_dev_desc, >> + &fs_partition, 1); >> if (part < 0) >> return -1; >> > > It would be good if we could test the new capabilities. Can we make use of sandbox_mmc.c? I suppose. There are already some tests in test/dm/fastboot.c along these lines. Fastboot calls into part_get_info_by_dev_and_name_or_num, so I think that is testing the same sort of thing. Perhaps they should be broken out into general partition tests. IMO this code path already has adequate tests. --Sean
On Tue, Mar 09, 2021 at 03:56:24PM -0500, Sean Anderson wrote: > This allows using dev#partlabel syntax. > > Signed-off-by: Sean Anderson <sean.anderson@seco.com> > --- > > fs/fs.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/fs.c b/fs/fs.c > index 900928c394..b7936fd4cf 100644 > --- a/fs/fs.c > +++ b/fs/fs.c > @@ -385,8 +385,8 @@ int fs_set_blk_dev(const char *ifname, const char *dev_part_str, int fstype) > } > #endif > > - part = blk_get_device_part_str(ifname, dev_part_str, &fs_dev_desc, > - &fs_partition, 1); > + part = part_get_info_by_dev_and_name_or_num(ifname, dev_part_str, &fs_dev_desc, > + &fs_partition, 1); > if (part < 0) > return -1; This breaks a small number of platforms that have FS_LOADER support without PARTITION support: xilinx_versal_mini xilinx_zynqmp_mini xilinx_zynqmp_mini_nand xilinx_zynqmp_mini_nand_single mt7620_mt7530_rfb mt7628_rfb and I suspect in all cases it's an intentional and valid config.
On 4/12/21 5:44 PM, Tom Rini wrote: > On Tue, Mar 09, 2021 at 03:56:24PM -0500, Sean Anderson wrote: > >> This allows using dev#partlabel syntax. >> >> Signed-off-by: Sean Anderson <sean.anderson@seco.com> >> --- >> >> fs/fs.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/fs/fs.c b/fs/fs.c >> index 900928c394..b7936fd4cf 100644 >> --- a/fs/fs.c >> +++ b/fs/fs.c >> @@ -385,8 +385,8 @@ int fs_set_blk_dev(const char *ifname, const char *dev_part_str, int fstype) >> } >> #endif >> >> - part = blk_get_device_part_str(ifname, dev_part_str, &fs_dev_desc, >> - &fs_partition, 1); >> + part = part_get_info_by_dev_and_name_or_num(ifname, dev_part_str, &fs_dev_desc, >> + &fs_partition, 1); >> if (part < 0) >> return -1; > > This breaks a small number of platforms that have FS_LOADER support > without PARTITION support: > xilinx_versal_mini xilinx_zynqmp_mini xilinx_zynqmp_mini_nand > xilinx_zynqmp_mini_nand_single mt7620_mt7530_rfb mt7628_rfb and I > suspect in all cases it's an intentional and valid config. > Ok, I will add a stub like what is done for blk_get_device_part_str in v2. --Sean
diff --git a/fs/fs.c b/fs/fs.c index 900928c394..b7936fd4cf 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -385,8 +385,8 @@ int fs_set_blk_dev(const char *ifname, const char *dev_part_str, int fstype) } #endif - part = blk_get_device_part_str(ifname, dev_part_str, &fs_dev_desc, - &fs_partition, 1); + part = part_get_info_by_dev_and_name_or_num(ifname, dev_part_str, &fs_dev_desc, + &fs_partition, 1); if (part < 0) return -1;
This allows using dev#partlabel syntax. Signed-off-by: Sean Anderson <sean.anderson@seco.com> --- fs/fs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)