From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1cGO0K-0008ST-Pb for mharc-grub-devel@gnu.org; Mon, 12 Dec 2016 05:38:44 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37085) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cGO0A-0008KY-Rt for grub-devel@gnu.org; Mon, 12 Dec 2016 05:38:36 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cGO07-0004Hw-LP for grub-devel@gnu.org; Mon, 12 Dec 2016 05:38:34 -0500 Received: from userp1040.oracle.com ([156.151.31.81]:42819) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1cGO07-0004HV-AU for grub-devel@gnu.org; Mon, 12 Dec 2016 05:38:31 -0500 Received: from userv0022.oracle.com (userv0022.oracle.com [156.151.31.74]) by userp1040.oracle.com (Sentrion-MTA-4.3.2/Sentrion-MTA-4.3.2) with ESMTP id uBCAcUZS031233 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Mon, 12 Dec 2016 10:38:30 GMT Received: from aserv0122.oracle.com (aserv0122.oracle.com [141.146.126.236]) by userv0022.oracle.com (8.14.4/8.14.4) with ESMTP id uBCAcT1p013492 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Mon, 12 Dec 2016 10:38:29 GMT Received: from abhmp0014.oracle.com (abhmp0014.oracle.com [141.146.116.20]) by aserv0122.oracle.com (8.14.4/8.14.4) with ESMTP id uBCAcT53020613 for ; Mon, 12 Dec 2016 10:38:29 GMT Received: from [10.162.81.165] (/10.162.81.165) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Mon, 12 Dec 2016 02:38:29 -0800 Subject: Re: [PATCH V2 2/4] ofnet: move the allocation of the transmit buffer into a function To: The development of GNU GRUB References: <1480691405-15118-1-git-send-email-stanislav.kholmanskikh@oracle.com> <1480691405-15118-3-git-send-email-stanislav.kholmanskikh@oracle.com> <20161205155204.GC6024@router-fw-old.local.net-space.pl> <584E7225.1020500@oracle.com> Cc: vasily.isaenko@oracle.com From: Stanislav Kholmanskikh Message-ID: <584E7E23.4000501@oracle.com> Date: Mon, 12 Dec 2016 13:38:27 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Source-IP: userv0022.oracle.com [156.151.31.74] X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.4.x-2.6.x [generic] [fuzzy] X-Received-From: 156.151.31.81 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 12 Dec 2016 10:38:36 -0000 On 12/12/2016 01:27 PM, Andrei Borzenkov wrote: > While I agree with your reasons, it belongs to separate commit, and > definitely is out of place if you say in commit message "I'm moving > piece of code". Actually, it is not related to this patch series, so > feel free to send this cleanup as separate patch. Thank you. I'll stick to 'return 0' for this series. > > On Mon, Dec 12, 2016 at 12:47 PM, Stanislav Kholmanskikh > wrote: >> >> >> 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 >>>> --- >>>> 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 *) ∝ >> 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 >>> Grub-devel@gnu.org >>> https://lists.gnu.org/mailman/listinfo/grub-devel >>> >> >> _______________________________________________ >> Grub-devel mailing list >> Grub-devel@gnu.org >> https://lists.gnu.org/mailman/listinfo/grub-devel > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel >