Re: [PATCH] efinet: check for broken firmware

[Josef Bacik <jbacik@fb.com>]

On 11/13/2015 08:10 AM, Andrei Borzenkov wrote:
> On Fri, Nov 13, 2015 at 1:07 AM, Josef Bacik <jbacik@fb.com> wrote:
>> The firmware bug I've been tracking down has been too extensive to work around
>> effectively.  Basically once we switch to EXCLUSIVE everything is completely
>> horked.  I can add a multicast filter but it's timing sensitive, so it has to be
>> done at least 10 seconds after initialization for it to take place, and we have
>> to continue to enable PROMISCUOUS_MULTICAST because otherwise we no longer get
>> unicast traffic.  I discovered however that if we do not make the EXCLUSIVE
>> switch over everything works fine.  So instead add a function that checks for
>
> Does your firmware use MNP at all?
>

I was going to look into that today, with as much problems as SNP has 
been giving us I'm wondering if MNP works better.  I'm going to rig up a 
barebones MNP driver and see if it behaves better.  The question is how 
to make us use MNP vs. SNP when it is supported, or to force us to go 
back to SNP if MNP is fuuuuucked up.

>> this broken firmware and uses GET_PROTOCOL instead of EXCLUSIVE.  This also
>> keeps the original protocol we use when scanning the cards and leaves the
>> initialization stuff for ->open.  Thanks,
>>
>> Signed-off-by: Josef Bacik <jbacik@fb.com>
>> ---
>>   grub-core/net/drivers/efi/efinet.c | 99 ++++++++++++++++++--------------------
>>   1 file changed, 46 insertions(+), 53 deletions(-)
>>
>> diff --git a/grub-core/net/drivers/efi/efinet.c b/grub-core/net/drivers/efi/efinet.c
>> index c8f80a1..5a207fd 100644
>> --- a/grub-core/net/drivers/efi/efinet.c
>> +++ b/grub-core/net/drivers/efi/efinet.c
>> @@ -156,59 +156,40 @@ get_card_packet (struct grub_net_card *dev)
>>   static grub_err_t
>>   open_card (struct grub_net_card *dev)
>>   {
>> -  grub_efi_simple_network_t *net;
>> +  grub_efi_simple_network_t *net = dev->efi_net;
>>
>> -  /* Try to reopen SNP exlusively to close any active MNP protocol instance
>> -     that may compete for packet polling
>> -   */
>> -  net = grub_efi_open_protocol (dev->efi_handle, &net_io_guid,
>> -                               GRUB_EFI_OPEN_PROTOCOL_BY_EXCLUSIVE);
>> -  if (net)
>> +  if (net->mode->state == GRUB_EFI_NETWORK_STOPPED)
>> +    return grub_error (GRUB_ERR_NET_NO_CARD, "%s: card stopped",
>> +                      dev->name);
>> +
>> +  if (net->mode->state == GRUB_EFI_NETWORK_STARTED
>> +      && efi_call_3 (net->initialize, net, 0, 0) != GRUB_EFI_SUCCESS)
>> +         return grub_error (GRUB_ERR_NET_NO_CARD, "%s: net initialize failed",
>> +                            dev->name);
>> +
>> +  /* Enable hardware receive filters if driver declares support for it.
>> +     We need unicast and broadcast and additionaly all nodes and
>> +     solicited multicast for IPv6. Solicited multicast is per-IPv6
>> +     address and we currently do not have API to do it so simply
>> +     try to enable receive of all multicast packets or evertyhing in
>> +     the worst case (i386 PXE driver always enables promiscuous too).
>> +
>> +     This does trust firmware to do what it claims to do.
>> +    */
>> +  if (net->mode->receive_filter_mask)
>>       {
>> -      if (net->mode->state == GRUB_EFI_NETWORK_STOPPED
>> -         && efi_call_1 (net->start, net) != GRUB_EFI_SUCCESS)
>> -       return grub_error (GRUB_ERR_NET_NO_CARD, "%s: net start failed",
>> -                          dev->name);
>> -
>> -      if (net->mode->state == GRUB_EFI_NETWORK_STOPPED)
>> -       return grub_error (GRUB_ERR_NET_NO_CARD, "%s: card stopped",
>> -                          dev->name);
>> -
>> -      if (net->mode->state == GRUB_EFI_NETWORK_STARTED
>> -         && efi_call_3 (net->initialize, net, 0, 0) != GRUB_EFI_SUCCESS)
>> -       return grub_error (GRUB_ERR_NET_NO_CARD, "%s: net initialize failed",
>> -                          dev->name);
>> -
>> -      /* Enable hardware receive filters if driver declares support for it.
>> -        We need unicast and broadcast and additionaly all nodes and
>> -        solicited multicast for IPv6. Solicited multicast is per-IPv6
>> -        address and we currently do not have API to do it so simply
>> -        try to enable receive of all multicast packets or evertyhing in
>> -        the worst case (i386 PXE driver always enables promiscuous too).
>> -
>> -        This does trust firmware to do what it claims to do.
>> -       */
>> -      if (net->mode->receive_filter_mask)
>> -       {
>> -         grub_uint32_t filters = GRUB_EFI_SIMPLE_NETWORK_RECEIVE_UNICAST   |
>> -                                 GRUB_EFI_SIMPLE_NETWORK_RECEIVE_BROADCAST |
>> -                                 GRUB_EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS_MULTICAST;
>> -
>> -         filters &= net->mode->receive_filter_mask;
>> -         if (!(filters & GRUB_EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS_MULTICAST))
>> -           filters |= (net->mode->receive_filter_mask &
>> -                       GRUB_EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS);
>> +      grub_uint32_t filters = GRUB_EFI_SIMPLE_NETWORK_RECEIVE_UNICAST   |
>> +             GRUB_EFI_SIMPLE_NETWORK_RECEIVE_BROADCAST |
>> +             GRUB_EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS_MULTICAST;
>>
>> -         efi_call_6 (net->receive_filters, net, filters, 0, 0, 0, NULL);
>> -       }
>> +      filters &= net->mode->receive_filter_mask;
>> +      if (!(filters & GRUB_EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS_MULTICAST))
>> +       filters |= (net->mode->receive_filter_mask &
>> +                   GRUB_EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS);
>>
>> -      efi_call_4 (grub_efi_system_table->boot_services->close_protocol,
>> -                 dev->efi_net, &net_io_guid,
>> -                 grub_efi_image_handle, dev->efi_handle);
>> -      dev->efi_net = net;
>> +      efi_call_6 (net->receive_filters, net, filters, 0, 0, 0, NULL);
>>       }
>>
>> -  /* If it failed we just try to run as best as we can */
>>     return GRUB_ERR_NONE;
>>   }
>>
>> @@ -278,12 +259,26 @@ grub_efinet_is_mac_device (struct grub_net_card *card)
>>     return 0;
>>   }
>>
>> +static grub_efi_uint32_t
>> +grub_snp_attributes (void)
>
> Attributes? I'd say
>
> use_snp ? GRUB_EFI_OPEN_PROTOCOL_BY_EXCLUSIVE :
> GRUB_EFI_OPEN_PROTOCOL_GET_PROTOCOL
>
> looks more readable.
>
>> +{
>> +  /* Some firmware will completely screw up the transition to exclusive SNP,
>> +     doing things like not honoring receive filters or taking multiple seconds
>> +     to make the switch over.  So don't bother using exclusive in this case.
>> +   */
>> +  if (!grub_memcmp(grub_efi_system_table->firmware_vendor, "A", 1) &&
>> +      grub_efi_system_table->firmware_revision == (grub_efi_uint32_t)262798)
>
> Well, I'm not thrilled by this check ... at least we need to compare
> full firmware_vendor then.
>

That is the full firmware_vendor for our firmware.  I suppose I should 
also do strlen(firmware_vendor) == 1 && memcmp() to make sure it doesn't 
just match something that starts with A, I can fix that up.

>> +    return GRUB_EFI_OPEN_PROTOCOL_GET_PROTOCOL;
>> +  return GRUB_EFI_OPEN_PROTOCOL_BY_EXCLUSIVE;
>> +}
>> +
>>   static void
>>   grub_efinet_findcards (void)
>>   {
>>     grub_efi_uintn_t num_handles;
>>     grub_efi_handle_t *handles;
>>     grub_efi_handle_t *handle;
>> +  grub_efi_uint32_t attributes;
>>     int i = 0;
>>
>>     /* Find handles which support the disk io interface.  */
>> @@ -291,6 +286,9 @@ grub_efinet_findcards (void)
>>                                      0, &num_handles);
>>     if (! handles)
>>       return;
>> +
>> +  attributes = grub_snp_attributes();
>> +
>>     for (handle = handles; num_handles--; handle++)
>>       {
>>         grub_efi_simple_network_t *net;
>> @@ -319,8 +317,7 @@ grub_efinet_findcards (void)
>>            && GRUB_EFI_DEVICE_PATH_SUBTYPE (parent) == GRUB_EFI_MAC_ADDRESS_DEVICE_PATH_SUBTYPE)
>>          continue;
>>
>> -      net = grub_efi_open_protocol (*handle, &net_io_guid,
>> -                                   GRUB_EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>> +      net = grub_efi_open_protocol (*handle, &net_io_guid, attributes);
>
> No, we cannot open exclusively here, it will destroy autocnfiguration
> information we need later. You need to add conditional in open_card.

The autoconfig stuff still works later for me but I can change it back. 
  Thanks,

Josef