Re: [PATCH v5 38/46] boot: Consider non-bootable partitions

[Tom Rini <trini@konsulko.com>]

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

On Thu, Apr 03, 2025 at 08:22:30AM +1300, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 3 Apr 2025 at 03:28, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sat, Mar 15, 2025 at 02:25:58PM +0000, Simon Glass wrote:
> >
> > > Any 'bootable' flag in a DOS partition causes boostd to only scan
> > > bootable partitions for that media. This can mean that extlinux.conf
> > > files on the root disk are missed.
> > >
> > > Put this logic behind a flag and update the documentation.
> > >
> > > For now, the flag is enabled, to preserve existing behaviour. Future
> > > work may provide a command (or some other mechanism) to control this.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >
> > > (no changes since v4)
> > >
> > > Changes in v4:
> > > - Rewrite the commit message
> > > - Enable the flag by default
> > >
> > > Changes in v3:
> > > - Add new patch to consider non-bootable partitions
> > >
> > >  boot/bootdev-uclass.c            | 4 +++-
> > >  cmd/bootflow.c                   | 2 +-
> > >  doc/develop/bootstd/overview.rst | 5 +++--
> > >  include/bootflow.h               | 2 ++
> > >  test/boot/bootdev.c              | 1 +
> > >  test/boot/bootflow.c             | 5 +++--
> > >  6 files changed, 13 insertions(+), 6 deletions(-)
> >
> > Having spent another 10 minutes just now trying to understand things,
> > again:
> > - The commit message, and the implementation are either in disagreement
> >   or just too vague. Saying "to preserve existing behaviour" is unclear
> >   about which behavior is being preserved, since we're setting the flag
> >   to continue to only check bootable flagged partitions
> >   (BOOTFLOWIF_ONLY_BOOTABLE). But then we would continue to miss
> >   extlinux.conf files on root filesystems, which would seem to be the
> >   bug that needed fixing?
> 
> How about 'to preserve the existing behaviour of bootstd which is to
> ignore non-bootable partitions so long as there is at least one
> bootable partition on the disk"  ?

Yes, that's better. I've reworded the commit message with that.

> > - It's still unclear if this makes bootstd match the exiting distro
> >   script behavior or not, and from your other emails it sounded like you
> >   were expecting someone else to dig around.
> 
> The distro scripts include the code below. My reading of it is that it
> only considers bootable partitions, except that if there is none, then
> it always scans partition 1.
> 
> "scan_dev_for_boot_part="                                         \
> "part list ${devtype} ${devnum} -bootable devplist; "     \
> "env exists devplist || setenv devplist 1; "              \
> "for distro_bootpart in ${devplist}; do "                 \
> "if fstype ${devtype} "                           \
> "${devnum}:${distro_bootpart} "   \
> "bootfstype; then "               \
> "part uuid ${devtype} "                   \
> "${devnum}:${distro_bootpart} "   \
> "distro_bootpart_uuid ; "         \
> "run scan_dev_for_boot; "                 \
> "fi; "                                            \
> "done; "                                                  \
> "setenv devplist\0"   \
> \

OK, thanks, I agree with your reading of that as well.

-- 
Tom

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