From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1ZxFN2-0007mP-LT for mharc-grub-devel@gnu.org; Fri, 13 Nov 2015 09:30:32 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60610) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZxFMz-0007jb-AP for grub-devel@gnu.org; Fri, 13 Nov 2015 09:30:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZxFMv-00056A-3k for grub-devel@gnu.org; Fri, 13 Nov 2015 09:30:29 -0500 Received: from mx0a-00082601.pphosted.com ([67.231.145.42]:57714) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZxFMu-000560-S2 for grub-devel@gnu.org; Fri, 13 Nov 2015 09:30:25 -0500 Received: from pps.filterd (m0044010.ppops.net [127.0.0.1]) by mx0a-00082601.pphosted.com (8.15.0.59/8.15.0.59) with SMTP id tADEU4E5023896; Fri, 13 Nov 2015 06:30:22 -0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=fb.com; h=subject : to : references : cc : from : message-id : date : mime-version : in-reply-to : content-type : content-transfer-encoding; s=facebook; bh=9iJPqO+MWfwUPUq5UjHCnbGy3hxDL5SvG99xwViIDcs=; b=g5TwvsbXEirW4VZlLYZI/mAc0b2A4yyXJc8tenMiMMiR43EJQe3+aXpO9UW1+5mOQjqf 2wiX5FllXjWLPKK9XwCEQJe9PcG23NC2FY4EMgAnw+fkTRIyGznuxrYHr+cWApDTPgw9 n/gBev7D0Y8LBbT+odlb0vZbz/U+pS9IQD0= Received: from mail.thefacebook.com ([199.201.64.23]) by mx0a-00082601.pphosted.com with ESMTP id 1y523taw73-1 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=NOT); Fri, 13 Nov 2015 06:30:22 -0800 Received: from localhost.localdomain (192.168.52.123) by mail.thefacebook.com (192.168.16.24) with Microsoft SMTP Server (TLS) id 14.3.248.2; Fri, 13 Nov 2015 06:30:19 -0800 Subject: Re: [PATCH] efinet: check for broken firmware To: Andrei Borzenkov , The development of GNU GRUB References: <1447366056-3328165-1-git-send-email-jbacik@fb.com> From: Josef Bacik Message-ID: <5645F3F9.7030405@fb.com> Date: Fri, 13 Nov 2015 09:30:17 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [192.168.52.123] X-Proofpoint-Spam-Reason: safe X-FB-Internal: Safe X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2015-11-13_12:, , signatures=0 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x [generic] X-Received-From: 67.231.145.42 Cc: kernel-team@fb.com X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 13 Nov 2015 14:30:30 -0000 On 11/13/2015 08:10 AM, Andrei Borzenkov wrote: > On Fri, Nov 13, 2015 at 1:07 AM, Josef Bacik 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 >> --- >> 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