From patchwork Sat May 26 11:28:18 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mauro Carvalho Chehab X-Patchwork-Id: 10428953 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 58C2760249 for ; Sat, 26 May 2018 11:28:48 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 7625F28DE9 for ; Sat, 26 May 2018 11:28:45 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 6B213290AE; Sat, 26 May 2018 11:28:45 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI, T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 6AC2728DE9 for ; Sat, 26 May 2018 11:28:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1031746AbeEZL20 (ORCPT ); Sat, 26 May 2018 07:28:26 -0400 Received: from bombadil.infradead.org ([198.137.202.133]:37006 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1031606AbeEZL2Y (ORCPT ); Sat, 26 May 2018 07:28:24 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20170209; h=Content-Transfer-Encoding: Content-Type:MIME-Version:References:In-Reply-To:Message-ID:Subject:Cc:To: From:Date:Sender:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=r0BJHpwydjEHHT9cxddUzuEUVcztO/WlWMaTXF+vzRQ=; b=caSGuXKtva3SQlkU74Wf+Uc7p U/DlCLvDFQ5dtB/FtgHqR0fvFcYqWOT6qwR3MP3JuOlfgrF7/komPU0D3pPqwBkjqY1vOAOx7WvFe I9dh8dVKKiQKSkjMLrvpcXohbIb2PMaKULEkDPVRxGJ275kyqkpHcdAP53z04BLpfSDjdOigPHTjJ 29UJknpNj/TJa2P9kxG6lfAAudaQDrVATzyODSjzQ2cVtjP//hU3cIdVucw0sjEsAsw/nKlWsR/MR 3vnIEfFxKIBMzWPhKgtBANF0vVddh/TKnI/KWDSNNFPSRDNd1BPYEg4c6COtHZ9iYlFOjOevekEWU FTINHhILw==; Received: from [179.95.17.250] (helo=vento.lan) by bombadil.infradead.org with esmtpsa (Exim 4.90_1 #2 (Red Hat Linux)) id 1fMXN0-0007UR-9O; Sat, 26 May 2018 11:28:22 +0000 Date: Sat, 26 May 2018 08:28:18 -0300 From: Mauro Carvalho Chehab To: Laurent Pinchart Cc: linux-media@vger.kernel.org, Kieran Bingham , Dan Carpenter Subject: Re: [GIT PULL FOR v4.18] R-Car VSP1 TLB optimisation Message-ID: <20180526082818.70a369b5@vento.lan> In-Reply-To: <1657947.LKPPaiEoOV@avalon> References: <10831984.07PNLvckhh@avalon> <20180525201027.1d5c82eb@vento.lan> <4867226.Y05TeWaCcJ@avalon> <1657947.LKPPaiEoOV@avalon> X-Mailer: Claws Mail 3.16.0 (GTK+ 2.24.32; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Em Sat, 26 May 2018 03:24:00 +0300 Laurent Pinchart escreveu: > Hi Mauro, > > On Saturday, 26 May 2018 02:39:16 EEST Laurent Pinchart wrote: > > On Saturday, 26 May 2018 02:10:27 EEST Mauro Carvalho Chehab wrote: > > > Em Sun, 20 May 2018 15:10:50 +0300 Laurent Pinchart escreveu: > > >> Hi Mauro, > > >> > > >> The following changes since commit > > >> > > >> 8ed8bba70b4355b1ba029b151ade84475dd12991: > > >> media: imx274: remove non-indexed pointers from mode_table (2018-05-17 > > >> > > >> 06:22:08 -0400) > > >> > > >> are available in the Git repository at: > > >> git://linuxtv.org/pinchartl/media.git v4l2/vsp1/next > > >> > > >> for you to fetch changes up to 429f256501652c90a4ed82f2416618f82a77d37c: > > >> media: vsp1: Move video configuration to a cached dlb (2018-05-20 > > >> 09:46:51 +0300) > > >> > > >> The branch passes the VSP and DU test suites, both on its own and when > > >> merged with the drm-next branch. > > > > > > This series added a new warning: > > > > > > drivers/media/platform/vsp1/vsp1_dl.c:69: warning: Function parameter or > > > member 'refcnt' not described in 'vsp1_dl_body' > > > > We'll fix that. Kieran, as you authored the code, would you like to give it > > a go ? > > > > > To the already existing one: > > > > > > drivers/media/platform/vsp1/vsp1_drm.c:336 vsp1_du_pipeline_setup_brx() > > > error: we previously assumed 'pipe->brx' could be null (see line 244) > > > > That's still on my todo list. I tried to give it a go but received plenty of > > SQL errors. How do you run smatch ? > > Nevermind, I found out what was wrong (had to specify the data directory > manually). > > I've reproduced the issue and created a minimal test case. > > 1. struct vsp1_pipeline; > 2. > 3. struct vsp1_entity { > 4. struct vsp1_pipeline *pipe; > 5. struct vsp1_entity *sink; > 6. unsigned int source_pad; > 7. }; > 8. > 9. struct vsp1_pipeline { > 10. struct vsp1_entity *brx; > 11. }; > 12. > 13. struct vsp1_brx { > 14. struct vsp1_entity entity; > 15. }; > 16. > 17. struct vsp1_device { > 18. struct vsp1_brx *bru; > 19. struct vsp1_brx *brs; > 20. }; > 21. > 22. unsigned int frob(struct vsp1_device *vsp1, struct vsp1_pipeline *pipe) > 23. { > 24. struct vsp1_entity *brx; > 25. > 26. if (pipe->brx) > 27. brx = pipe->brx; > 28. else if (!vsp1->bru->entity.pipe) > 29. brx = &vsp1->bru->entity; > 30. else > 31. brx = &vsp1->brs->entity; > 32. > 33. if (brx != pipe->brx) > 34. pipe->brx = brx; > 35. > 36. return pipe->brx->source_pad; > 37. } > > The reason why smatch complains is that it has no guarantee that vsp1->brs is > not NULL. It's quite tricky: > > - On line 26, smatch assumes that pipe->brx can be NULL > - On line 27, brx is assigned a non-NULL value (as pipe->brx is not NULL due > to line 26) > - On line 28, smatch assumes that vsp1->bru is not NULL > - On line 29, brx is assigned a non-NULL value (as vsp1->bru is not NULL due > to line 28) > - On line 31, brx is assigned a possibly NULL value (as there's no information > regarding vsp1->brs) > - On line 34, pipe->brx is not assigned a non-NULL value if brx is NULL > - On line 36 pipe->brx is dereferenced > > The problem comes from the fact that smatch assumes that vsp1->brs isn't NULL. > Adding a "(void)vsp1->brs->entity;" statement on line 25 makes the warning > disappear. > > So how do we know that vsp1->brs isn't NULL in the original code ? > > if (pipe->num_inputs > 2) > brx = &vsp1->bru->entity; > else if (pipe->brx && !drm_pipe->force_brx_release) > brx = pipe->brx; > else if (!vsp1->bru->entity.pipe) > brx = &vsp1->bru->entity; > else > brx = &vsp1->brs->entity; > > A VSP1 instance can have no brs, so in general vsp1->brs can be NULL. However, > when that's the case, the following conditions are fulfilled. > > - drm_pipe->force_brx_release will be false > - either pipe->brx will be non-NULL, or vsp1->bru->entity.pipe will be NULL > > The fourth branch should thus never be taken. I don't think that adding a forth branch there would solve. The thing is that Smatch knows that pipe->brx can be NULL, as the function explicly checks if pipe->brx != NULL. When Smatch handles this if: if (brx != pipe->brx) { It wrongly assumes that this could be false if pipe->brx is NULL. I don't know why, as Smatch should know that brx can't be NULL. On such case, the next code to be executed would be: format.pad = pipe->brx->source_pad; With would be trying to de-ref a NULL pointer. There are two ways to fix it: 1) with my patch. It is based to the fact that, if pipe->brx is null, then brx won't be NULL. So, the logic that "Switch BRx if needed." will always be called: The right fix would be, instead, to fix Smatch to handle the: if (brx != pipe->brx) for the cases where one var can be NULL while the other can't be NULL, but, as I said before, I suspect that this can be a way more complex. Thanks, Mauro diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c index 095dc48aa25a..cb6b60843400 100644 --- a/drivers/media/platform/vsp1/vsp1_drm.c +++ b/drivers/media/platform/vsp1/vsp1_drm.c @@ -185,7 +185,7 @@ static int vsp1_du_pipeline_setup_brx(struct vsp1_device *vsp1, brx = &vsp1->brs->entity; /* Switch BRx if needed. */ - if (brx != pipe->brx) { + if (brx != pipe->brx || !pipe->brx) { struct vsp1_entity *released_brx = NULL; /* Release our BRx if we have one. */ The code with switches BRx ensures that pipe->brx won't be null, as in the end, it sets: pipe->brx = brx; And brx can't be NULL. From my PoV, this patch has the advantage of explicitly showing to humans that the code inside the if statement will always be executed when pipe->brx is NULL. - Another way to solve would be to explicitly check if pipe->brx is still null before de-referencing: diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c index edb35a5c57ea..9fe063d6df31 100644 --- a/drivers/media/platform/vsp1/vsp1_drm.c +++ b/drivers/media/platform/vsp1/vsp1_drm.c @@ -327,6 +327,9 @@ static int vsp1_du_pipeline_setup_brx(struct vsp1_device *vsp1, list_add_tail(&pipe->brx->list_pipe, &pipe->entities); } + if (!pipe->brx) + return -EINVAL; + /* * Configure the format on the BRx source and verify that it matches the * requested format. We don't set the media bus code as it is configured