Re: [PATCH v2 1/2] xen/riscv: initialize bootinfo from dtb

[oleksii.kurochko@xxxxxxxxx]

On Tue, 2024-10-15 at 14:32 +0200, Jan Beulich wrote:
> On 15.10.2024 11:11, oleksii.kurochko@xxxxxxxxx wrote:
> > On Tue, 2024-10-15 at 08:33 +0200, Jan Beulich wrote:
> > > On 10.10.2024 17:30, Oleksii Kurochko wrote:
> > > > --- a/xen/arch/riscv/setup.c
> > > > +++ b/xen/arch/riscv/setup.c
> > > > @@ -50,6 +50,8 @@ void __init noreturn start_xen(unsigned long
> > > > bootcpu_id,
> > > >                            _end - _start, false) )
> > > >          panic("Failed to add BOOTMOD_XEN\n");
> > > >  
> > > > +    BUG_ON(!boot_fdt_info(device_tree_flattened, dtb_addr));
> > > 
> > > We generally aim at avoiding side effects in BUG_ON() (or
> > > ASSERT()).
> > > With
> > > 
> > >     if (!boot_fdt_info(device_tree_flattened, dtb_addr))
> > >         BUG();
> > > 
> > > Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> > > 
> > > I can make the adjustment while committing, if desired.
> > It would be great if these changes could be made during the commit.
> 
> I've committed it with the adjustment, 
Thanks!

> yet once again I wondered: Why not
> panic()?
It could be panic() here; there's no specific reason. I agree that
using BUG() here isn't logically correct, as technically, a size of
zero for the FDT isn't a bug but rather indicates that someone provided
an incorrect FDT to Xen.

I will use panic() in the future for such cases.

It’s not always clear what should be used and where. Perhaps it would
be helpful to add some explanation somewhere.

~ Oleksii