From patchwork Mon Jun 10 10:26:46 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Sylwester Nawrocki/Kernel \\(PLT\\) /SRPOL/Staff Engineer/Samsung Electronics" X-Patchwork-Id: 2696771 Return-Path: X-Original-To: patchwork-linux-media@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id B64353FD4E for ; Mon, 10 Jun 2013 10:26:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752090Ab3FJK0w (ORCPT ); Mon, 10 Jun 2013 06:26:52 -0400 Received: from mailout3.w1.samsung.com ([210.118.77.13]:9702 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751910Ab3FJK0w (ORCPT ); Mon, 10 Jun 2013 06:26:52 -0400 Received: from eucpsbgm1.samsung.com (unknown [203.254.199.244]) by mailout3.w1.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0MO600IARAC6EP50@mailout3.w1.samsung.com> for linux-media@vger.kernel.org; Mon, 10 Jun 2013 11:26:49 +0100 (BST) X-AuditID: cbfec7f4-b7fd76d0000035e1-57-51b5a9e9bdcc Received: from eusync4.samsung.com ( [203.254.199.214]) by eucpsbgm1.samsung.com (EUCPMTA) with SMTP id 2F.D1.13793.9E9A5B15; Mon, 10 Jun 2013 11:26:49 +0100 (BST) Received: from [106.116.147.32] by eusync4.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTPA id <0MO6007YQACOD120@eusync4.samsung.com>; Mon, 10 Jun 2013 11:26:49 +0100 (BST) Message-id: <51B5A9E6.20903@samsung.com> Date: Mon, 10 Jun 2013 12:26:46 +0200 From: Sylwester Nawrocki User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Thunderbird/17.0 MIME-version: 1.0 To: Sakari Ailus Cc: Sylwester Nawrocki , linux-media@vger.kernel.org, laurent.pinchart@ideasonboard.com, hj210.choi@samsung.com, sw0312.kim@samsung.com, a.hajda@samsung.com, Kyungmin Park Subject: Re: [RFC PATCH v2 1/2] media: Add function removing all media entity links References: <1368102573-16183-2-git-send-email-s.nawrocki@samsung.com> <1368180037-24091-1-git-send-email-s.nawrocki@samsung.com> <20130606194124.GB3103@valkosipuli.retiisi.org.uk> <51B4CB8D.1010507@gmail.com> <51B4FE9C.9020300@iki.fi> In-reply-to: <51B4FE9C.9020300@iki.fi> Content-type: text/plain; charset=ISO-8859-1 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrCLMWRmVeSWpSXmKPExsVy+t/xa7ovV24NNPjSIGFxa905VovHG68x W5xtesNu0TlxCbtFz4atrBZn9q9ks5gx+SWbxbzPO5kcODx2zrrL7jG7Yyarx+GvC1k8+ras YvT4vEkugDWKyyYlNSezLLVI3y6BK2N/2wr2grVqFTsP/WVuYLwi1cXIySEhYCKxZ+k6Rghb TOLCvfVsXYxcHEICSxkljp/6BpYQEvjEKNFytBbE5hXQkFj+8zxYnEVAVeL0uw5WEJtNwFCi 92gfWFxUIEBi8ZJz7BD1ghI/Jt9jAbFFBNQknm56yAKygFngNaPEtKPrwIqEBUIlPu5sZILY /JFR4tvJl2CTOIE6rk9ZAdbNLKAjsb91GhuELS+xec1b5gmMArOQLJmFpGwWkrIFjMyrGEVT S5MLipPScw31ihNzi0vz0vWS83M3MUJC/ssOxsXHrA4xCnAwKvHwPvi1JVCINbGsuDL3EKME B7OSCG/xrK2BQrwpiZVVqUX58UWlOanFhxiZODilGhjT5+aw75ogzSoTvqXAatrq5fmJiSx7 jr3mVpA6sbzQcOfUe+qXLnXwXr4gbB1kFfH94vGr4dcLBLWkTa0nl54QEFXZO+ud7LJ5pxjb H/CuzjZ+d8YxVuMk35vPK4TN9t+y+/B3+X2XfYxVpROKtVuOLNEX9Hf70TilW1qrePEmltKn 555zb/ipxFKckWioxVxUnAgAaZGJlVcCAAA= Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org Hi Sakari, On 06/10/2013 12:15 AM, Sakari Ailus wrote: > Sylwester Nawrocki wrote: > ... >>>> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c >>>> index e1cd132..bd85dc3 100644 >>>> --- a/drivers/media/media-entity.c >>>> +++ b/drivers/media/media-entity.c >>>> @@ -429,6 +429,57 @@ media_entity_create_link(struct media_entity >>>> *source, u16 source_pad, >>>> } >>>> EXPORT_SYMBOL_GPL(media_entity_create_link); >>>> >>>> +void __media_entity_remove_links(struct media_entity *entity) >>>> +{ >>>> + int i, r; >>> >>> u16? r can be defined inside the loop. >> >> I would argue 'unsigned int' would be more optimal type for i in most >> cases. Will move r inside the loop. >> >>>> + for (i = 0; i< entity->num_links; i++) { >>>> + struct media_link *link =&entity->links[i]; >>>> + struct media_entity *remote; >>>> + int num_links; >>> >>> num_links is u16 in struct media_entity. I'd use the same type. >> >> I would go with 'unsigned int', as a more natural type for the CPU in >> most cases. It's a minor issue, but I can't see how u16 would have been >> more optimal than unsigned int for a local variable like this, while >> this code is mostly used on 32-bit systems at least. >> >>>> + if (link->source->entity == entity) >>>> + remote = link->sink->entity; >>>> + else >>>> + remote = link->source->entity; >>>> + >>>> + num_links = remote->num_links; >>>> + >>>> + for (r = 0; r< num_links; r++) { >>> >>> Is caching remote->num_links needed, or do I miss something? >> >> Yes, it is needed, remote->num_links is decremented inside the loop. > > Oh, forgot this one... yes, remote->num_links is decremented, but so is > r it's compared to. So I don't think it's necessary to cache it, but > it's neither an error to do so. Indeed, it seems more correct to not cache it, thanks. >>>> + struct media_link *rlink =&remote->links[r]; >>>> + >>>> + if (rlink != link->reverse) >>>> + continue; >>>> + >>>> + if (link->source->entity == entity) >>>> + remote->num_backlinks--; >>>> + >>>> + remote->num_links--; >>>> + >>>> + if (remote->num_links< 1) >>> >>> How about: if (!remote->num_links) ? >> >> Hmm, perhaps squashing the above two lines to: >> >> if (--remote->num_links == 0) >> >> ? > > I'm not too fond of --/++ operators as part of more complex structures > so I'd keep it separate. Fewer lines doesn't always equate to more > readable code. :-) In general I agree, however it's quite simple statement in this case - only decrement and test, it's often only one instruction even in the assembly language... I'm going to squash following to this patch: So the loop looks something like: while (r < remote->num_links) { if (remote->links[r] != link->reverse) { r++; continue; } if (link->source->entity == entity) remote->num_backlinks--; if (--remote->num_links == 0) break; /* Insert last entry in place of the dropped link. */ remote->links[r] = remote->links[remote->num_links]; } Does it sound OK ? Regards, Sylwester --- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c index f5ea82e..1fb619d 100644 --- a/drivers/media/media-entity.c +++ b/drivers/media/media-entity.c @@ -436,18 +436,18 @@ void __media_entity_remove_links(struct media_entity *entity) for (i = 0; i < entity->num_links; i++) { struct media_link *link = &entity->links[i]; struct media_entity *remote; - unsigned int r, num_links; + unsigned int r; if (link->source->entity == entity) remote = link->sink->entity; else remote = link->source->entity; - num_links = remote->num_links; - - for (r = 0; r < num_links; r++) { - if (remote->links[r] != link->reverse) + while (r < remote->num_links; r++) { + if (remote->links[r] != link->reverse) { + r++; continue; + } if (link->source->entity == entity) remote->num_backlinks--; @@ -456,7 +456,7 @@ void __media_entity_remove_links(struct media_entity *entity) break; /* Insert last entry in place of the dropped link. */ - remote->links[r--] = remote->links[remote->num_links]; + remote->links[r] = remote->links[remote->num_links]; } }