diff mbox series

efi/libstub: Cast away type warning in use of max()

Message ID 20240326101850.914032-2-ardb+git@google.com
State New
Headers show
Series efi/libstub: Cast away type warning in use of max() | expand

Commit Message

Ard Biesheuvel March 26, 2024, 10:18 a.m. UTC 
From: Ard Biesheuvel <ardb@kernel.org>

Add a missing (u64) cast to alloc_min, which is passed into
efi_random_alloc() as unsigned long, while efi_physical_addr_t is u64.

Fixes: 3cb4a4827596abc82e ("efi/libstub: fix efi_random_alloc() ...")
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/libstub/randomalloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ard Biesheuvel March 28, 2024, 9:13 a.m. UTC | #1 
On Thu, 28 Mar 2024 at 10:21, Lukas Wunner <lukas@wunner.de> wrote:
>
> On Tue, Mar 26, 2024 at 11:18:51AM +0100, Ard Biesheuvel wrote:
> > Add a missing (u64) cast to alloc_min, which is passed into
> > efi_random_alloc() as unsigned long, while efi_physical_addr_t is u64.
> [...]
> > --- a/drivers/firmware/efi/libstub/randomalloc.c
> > +++ b/drivers/firmware/efi/libstub/randomalloc.c
> > @@ -120,7 +120,7 @@ efi_status_t efi_random_alloc(unsigned long size,
> >                       continue;
> >               }
> >
> > -             target = round_up(max(md->phys_addr, alloc_min), align) + target_slot * align;
> > +             target = round_up(max(md->phys_addr, (u64)alloc_min), align) + target_slot * align;
>
> Why not
>
>     max_t(u64, md->phys_addr, alloc_min)
>

Why is that better?
Ard Biesheuvel March 28, 2024, 1:57 p.m. UTC | #2 
On Thu, 28 Mar 2024 at 15:38, Lukas Wunner <lukas@wunner.de> wrote:
>
> On Thu, Mar 28, 2024 at 11:13:07AM +0200, Ard Biesheuvel wrote:
> > On Thu, 28 Mar 2024 at 10:21, Lukas Wunner <lukas@wunner.de> wrote:
> > > On Tue, Mar 26, 2024 at 11:18:51AM +0100, Ard Biesheuvel wrote:
> > > > Add a missing (u64) cast to alloc_min, which is passed into
> > > > efi_random_alloc() as unsigned long, while efi_physical_addr_t is u64.
> > > [...]
> > > > --- a/drivers/firmware/efi/libstub/randomalloc.c
> > > > +++ b/drivers/firmware/efi/libstub/randomalloc.c
> > > > @@ -120,7 +120,7 @@ efi_status_t efi_random_alloc(unsigned long size,
> > > >                       continue;
> > > >               }
> > > >
> > > > -             target = round_up(max(md->phys_addr, alloc_min), align) + target_slot * align;
> > > > +             target = round_up(max(md->phys_addr, (u64)alloc_min), align) + target_slot * align;
> > >
> > > Why not
> > >
> > >     max_t(u64, md->phys_addr, alloc_min)
> >
> > Why is that better?
>
> It just seems to be the idiomatic way to handle these casts in the kernel.
>

In this particular case, I prefer max() with the cast, because it
matches the other occurrence, where alloc_min is also used but there
it is u64 not unsigned long.

> It's also what checkpatch suggests, so by not using it you risk getting
> "helpful" fixup patches from the usual suspects.
>

Ugh yeah good point.

> It's your call buddy. :)

Thanks for the head's up
diff mbox series

Patch

diff --git a/drivers/firmware/efi/libstub/randomalloc.c b/drivers/firmware/efi/libstub/randomalloc.c
index 7e1852859550..fa81528150fe 100644
--- a/drivers/firmware/efi/libstub/randomalloc.c
+++ b/drivers/firmware/efi/libstub/randomalloc.c
@@ -120,7 +120,7 @@  efi_status_t efi_random_alloc(unsigned long size,
 			continue;
 		}
 
-		target = round_up(max(md->phys_addr, alloc_min), align) + target_slot * align;
+		target = round_up(max(md->phys_addr, (u64)alloc_min), align) + target_slot * align;
 		pages = size / EFI_PAGE_SIZE;
 
 		status = efi_bs_call(allocate_pages, EFI_ALLOCATE_ADDRESS,