[PATCH v5 03/14] efi_loader: device_path: add efi_dp_from_ipv4

[Adriano Córdova]

El lun, 25 nov 2024 a las 5:42, Heinrich Schuchardt (<xypron.glpk at gmx.de>)
escribió:

> On 11/25/24 01:58, Adriano Cordova wrote:
> > Add efi_dp_from_ipv4 to form a device path from an ipv4 address.
> >
> > Signed-off-by: Adriano Cordova <adrianox at gmail.com>
> > ---
> >
> > Changes in v5:
> > - Add function description to efi_dp_from_ipv4
> > - Allocate ipv4dp in the stack
> > - Make efi_dp_from_ipv4 static
> > - Remove u-boot log messages inside efi routine
> > - Change (void *)(unitptr_t) to char *
>
> Nits:
>
> uintptr_t
>
> >
> > Changes in v4:
> > - Fix memcpy mistake
> >
> > Changes in v3:
> > - Remove some unnecessary void* casts.
> > - Change sizeof(struct efi_device_path_ipv4) to sizeof(*ipv4dp)
> >    in efi_dp_from_ipv4.
> >
> >   lib/efi_loader/efi_device_path.c | 43 ++++++++++++++++++++++++++++++++
> >   1 file changed, 43 insertions(+)
> >
> > diff --git a/lib/efi_loader/efi_device_path.c
> b/lib/efi_loader/efi_device_path.c
> > index ee387e1dfd..96e72f72fc 100644
> > --- a/lib/efi_loader/efi_device_path.c
> > +++ b/lib/efi_loader/efi_device_path.c
> > @@ -974,6 +974,49 @@ struct efi_device_path __maybe_unused
> *efi_dp_from_eth(void)
> >       return start;
> >   }
> >
> > +/**
> > + * efi_dp_from_ipv4() - set device path from IPv4 address
> > + *
> > + * Set the device path to an ethernet device path as provided by
> > + * efi_dp_from_eth() concatenated with a device path of subtype
> > + * DEVICE_PATH_SUB_TYPE_MSG_IPV4, and an END node.
> > + *
> > + * @ip:              IPv4 local address
> > + * @mask:    network mask
> > + * @srv:     IPv4 remote/server address
>
> The return value is not described, e.g.
>
> Return: device-path or NULL in case of an error
>
> > + */
> > +static struct efi_device_path *efi_dp_from_ipv4(struct efi_ipv4_address
> *ip,
> > +                                      struct efi_ipv4_address *mask,
> > +                                      struct efi_ipv4_address *srv)
>
> Nits:
> scripts/checkpatch.pl complains here.
> Alignment should match open parenthesis.
>
> If I align it the line gets too long and I get another checkpatch.pl
complain. Similar things happen
in a couple of other patches in this series. I could move it a bit to the
right if you want.

> +{
> > +     struct efi_device_path *dp1, *dp2, *pos;
> > +     struct {
> > +             struct efi_device_path_ipv4 ipv4dp;
> > +             struct efi_device_path end;
> > +     } dp;
> > +
> > +     memset(&dp.ipv4dp, 0, sizeof(dp.ipv4dp));
> > +     dp.ipv4dp.dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
> > +     dp.ipv4dp.dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_IPV4;
> > +     dp.ipv4dp.dp.length = sizeof(dp.ipv4dp);
> > +     dp.ipv4dp.protocol = 6;
> > +     if (ip)
> > +             memcpy(&dp.ipv4dp.local_ip_address, ip, sizeof(*ip));
> > +     if (mask)
> > +             memcpy(&dp.ipv4dp.subnet_mask, mask, sizeof(*mask));
> > +     if (srv)
> > +             memcpy(&dp.ipv4dp.remote_ip_address, srv, sizeof(*srv));
> > +     pos = &dp.end;
> > +     memcpy(pos, &END, sizeof(END));
> > +
> > +     dp1 = efi_dp_from_eth();
>
> If dp1 == NULL due to out of memory, shouldn't we return NULL to the
> caller?
>
> Best regards
>
> Heinrich


Sure, makes sense

Best,
Adriano

>
>
> +     dp2 = efi_dp_concat(dp1, (const struct efi_device_path *)&dp, 0);
> > +
> > +     efi_free_pool(dp1);
> > +
> > +     return dp2;
> > +}
> > +
> >   /* Construct a device-path for memory-mapped image */
> >   struct efi_device_path *efi_dp_from_mem(uint32_t memory_type,
> >                                       uint64_t start_address,
>
>