Re: [PATCH] efinet: check for broken firmware

[Andrei Borzenkov]

On Fri, Nov 13, 2015 at 1:07 AM, Josef Bacik <address@hidden> 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?

> 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 <address@hidden>
> ---
>  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.

> +    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.

>        if (! net)
>         /* This should not happen... Why?  */
>         continue;
> @@ -332,10 +329,6 @@ grub_efinet_findcards (void)
>        if (net->mode->state == GRUB_EFI_NETWORK_STOPPED)
>         continue;
>
> -      if (net->mode->state == GRUB_EFI_NETWORK_STARTED
> -         && efi_call_3 (net->initialize, net, 0, 0) != GRUB_EFI_SUCCESS)
> -       continue;
> -
>        card = grub_zalloc (sizeof (struct grub_net_card));
>        if (!card)
>         {
> --
> 1.8.1
>
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel