Re: [PATCH V2 2/4] ofnet: move the allocation of the transmit buffer into a function

[Stanislav Kholmanskikh]

On 12/05/2016 06:52 PM, Daniel Kiper wrote:
> On Fri, Dec 02, 2016 at 06:10:03PM +0300, Stanislav Kholmanskikh wrote:
>> In the current code search_net_devices() uses the "alloc-mem" command
>> from the IEEE1275 User Interface for allocation of the transmit buffer
>> for the case when GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN is set.
>>
>> I don't have hardware where this flag is set to verify if this
>> workaround is still needed. However, further changes to ofnet will
>> require to execute this workaround one more time. Therefore, to
>> avoid possible duplication of code I'm moving this piece of
>> code into a function.
>>
>> Signed-off-by: Stanislav Kholmanskikh <address@hidden>
>> ---
>>  grub-core/net/drivers/ieee1275/ofnet.c |   71 
>> ++++++++++++++++++++------------
>>  1 files changed, 44 insertions(+), 27 deletions(-)
>>
>> diff --git a/grub-core/net/drivers/ieee1275/ofnet.c 
>> b/grub-core/net/drivers/ieee1275/ofnet.c
>> index 8332d34..25559c8 100644
>> --- a/grub-core/net/drivers/ieee1275/ofnet.c
>> +++ b/grub-core/net/drivers/ieee1275/ofnet.c
>> @@ -298,6 +298,48 @@ grub_ieee1275_net_config_real (const char *devpath, 
>> char **device, char **path,
>>    }
>>  }
>>
>> +/* Allocate memory with alloc-mem */
>> +static void *
>> +grub_ieee1275_alloc_mem (grub_size_t len)
>> +{
>> +  struct alloc_args
>> +  {
>> +    struct grub_ieee1275_common_hdr common;
>> +    grub_ieee1275_cell_t method;
>> +    grub_ieee1275_cell_t len;
>> +    grub_ieee1275_cell_t catch;
>> +    grub_ieee1275_cell_t result;
>> +  }
>> +  args;
>> +
>> +  if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_CANNOT_INTERPRET))
>> +    {
>> +      grub_error (GRUB_ERR_UNKNOWN_COMMAND, N_("interpret is not 
>> supported"));
>> +      return NULL;
>> +    }
>> +
>> +  INIT_IEEE1275_COMMON (&args.common, "interpret", 2, 2);
>> +  args.len = len;
>> +  args.method = (grub_ieee1275_cell_t) "alloc-mem";
>> +
>> +  if (IEEE1275_CALL_ENTRY_FN (&args) == -1 || args.catch)
>> +    {
>> +      grub_error (GRUB_ERR_INVALID_COMMAND, N_("alloc-mem failed"));
>> +      return NULL;
>> +    }
>> +  else
>> +    return (void *)args.result;
>> +}
>> +
>> +static void *
>> +ofnet_alloc_netbuf (grub_size_t len)
>> +{
>> +  if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN))
>> +    return grub_ieee1275_alloc_mem (len);
>> +  else
>> +    return grub_zalloc (len);
>> +}
>> +
>>  static int
>>  search_net_devices (struct grub_ieee1275_devalias *alias)
>>  {
>> @@ -414,39 +456,14 @@ search_net_devices (struct grub_ieee1275_devalias 
>> *alias)
>>
>>    card->txbufsize = ALIGN_UP (card->mtu, 64) + 256;
>>
>> -  if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN))
>> -    {
>> -      struct alloc_args
>> -      {
>> -    struct grub_ieee1275_common_hdr common;
>> -    grub_ieee1275_cell_t method;
>> -    grub_ieee1275_cell_t len;
>> -    grub_ieee1275_cell_t catch;
>> -    grub_ieee1275_cell_t result;
>> -      }
>> -      args;
>> -      INIT_IEEE1275_COMMON (&args.common, "interpret", 2, 2);
>> -      args.len = card->txbufsize;
>> -      args.method = (grub_ieee1275_cell_t) "alloc-mem";
>> -
>> -      if (IEEE1275_CALL_ENTRY_FN (&args) == -1
>> -      || args.catch)
>> -    {
>> -      card->txbuf = 0;
>> -      grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
>> -    }
>> -      else
>> -    card->txbuf = (void *) args.result;
>> -    }
>> -  else
>> -    card->txbuf = grub_zalloc (card->txbufsize);
>> +  card->txbuf = ofnet_alloc_netbuf (card->txbufsize);
>>    if (!card->txbuf)
>>      {
>>        grub_free (ofdata->path);
>>        grub_free (ofdata);
>>        grub_free (card);
>>        grub_print_error ();
>> -      return 0;
>> +      return 1;
> 
> Hmmm... This looks like an error...

Look, here is what I see.

The search_net_devices() function is called from grub_ofnet_findcards() as:

grub_ieee1275_devices_iterate (search_net_devices);

The grub_ieee1275_devices_iterate(hook) function is defined in
grub-core/kern/ieee1275/openfw.c and what it does is basically calling
the hook for each IEEE1275 device in the tree until:
a) there are no more devices
b) the hook returns a value != 1

So if search_net_devices() returns 1 it means that further probing for
network cards is stopped.

I think that stopping further probes when a memory allocation function
fails is fine and it aligns with the existing code at the top of the
function, i.e. handling of the cases when allocating memory for 'card'
and 'ofdata' fails.

If I'm not mistaken, we may also need to update the block:

  if (need_suffix)
    ofdata->path = grub_malloc (grub_strlen (alias->path) + sizeof
(SUFFIX));
  else
    ofdata->path = grub_malloc (grub_strlen (alias->path) + 1);
  if (!ofdata->path)
    {
      grub_print_error ();
      return 0;
    }

and add a 'return 1' + free some memory there.

As for the other block:

  pprop = (grub_uint8_t *) &prop;
  if (grub_ieee1275_get_property (devhandle, "mac-address",
                                  pprop, sizeof(prop), &prop_size)
      && grub_ieee1275_get_property (devhandle, "local-mac-address",
                                     pprop, sizeof(prop), &prop_size))
    {
      grub_error (GRUB_ERR_IO, "Couldn't retrieve mac address.");
      grub_print_error ();
      return 0;
    }

it seems we need to add free memory procedures here as well, but I'm not
sure we need to return 1 here.



> 
> Daniel
> 
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel
>