Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 2 Nov 2021 14:33:32 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Tsuchiya Yuto <kitakar@gmail.com>
Cc:     Hans de Goede <hdegoede@redhat.com>,
        Patrik Gfeller <patrik.gfeller@gmail.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Sakari Ailus <sakari.ailus@linux.intel.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Hans Verkuil <hverkuil-cisco@xs4all.nl>,
        Aline Santana Cordeiro <alinesantanacordeiro@gmail.com>,
        Yang Yingliang <yangyingliang@huawei.com>,
        Alan <alan@linux.intel.com>,
        Dinghao Liu <dinghao.liu@zju.edu.cn>,
        Deepak R Varma <drv@mailo.com>,
        Alex Dewar <alex.dewar90@gmail.com>,
        linux-media@vger.kernel.org, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 05/17] media: atomisp: pci: fix inverted error check for
 ia_css_mipi_is_source_port_valid()
Message-ID: <20211102113332.GC2794@kadam>
References: <20211017161958.44351-1-kitakar@gmail.com>
 <20211017161958.44351-6-kitakar@gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211017161958.44351-6-kitakar@gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?AEHbXpYuCz6O0SSxzkJEIrGJsPseBPRbQezj0MlBg0B/Kc1dC2L9hpF6NDKH?=
 =?us-ascii?Q?BapbUAadDJUh+gi6ac+xLJMuIuzn6nqQNvdu9JWM5iyrA9OzwXwe2cvsUEEw?=
 =?us-ascii?Q?8XMSWpgzXU4KR4JEEnkJWx9VEOk8ivswlSrHIAPYgM2hxNVgGAqC2sCJ5dnK?=
 =?us-ascii?Q?xZU2bsdvjjO1ejI5k5BYQk/EcAIPpFHMgL9dEQzxq0knwKBHN5mPgdoCXp9m?=
 =?us-ascii?Q?QOrip1sB62XJruupznme4Phui2ul6Y7ZqvbLAhg1WhF98GJ6CPeqtyWIbqeM?=
 =?us-ascii?Q?5W+uCx+9xJ/n7V9C2E07Fg5TlWd4dNZN5ScrTf8BLCu3J/DI4erANex6Xhl0?=
 =?us-ascii?Q?nzAuV4/46AgUdJzMvfS9u2gb6IObBlmSbnWwir4otcRhNstwgNBipVVyQVv5?=
 =?us-ascii?Q?qM9sK3yCZcAqZCbcAF1+uAz62TKGPivm35ypm/77UU9uqn3urQbh3MdiQC4J?=
 =?us-ascii?Q?kOShSSXcjk5RQfSxUjXfrasRE8X5dggqXGpLGRseCR4xrVB/u990OXr4owHO?=
 =?us-ascii?Q?5JC6kOoZquHq5J9U0gS24S7r9ggqt+xkYnQoj7GuB9Fp2Erbsgwc+YC+TARF?=
 =?us-ascii?Q?ammEOPDlFUB3sadKYeoVYeNM+JQN9q2DlS9bbHUCqDn0DKbEwfjwXGjF4xzC?=
 =?us-ascii?Q?lEKwTFFl5h/0w3p1sOSru7o+Gi39FKlDSChfiz5pgf+Yhf7ps42v5AJJqv8h?=
 =?us-ascii?Q?deHRNIm0S69XsXRNxJ06jjBVhmbNHynMJ9lNJQmusOc/A1gn3S5RRD3psau3?=
 =?us-ascii?Q?O9wJYvnUvHFaUQrZmZYZObNZVHE6J8Lj7X6wRtqReE6RKU7CICD4RIUrrmF6?=
 =?us-ascii?Q?y/Qwk3L1KhPLGk1zjaICn8BbGSwOrbssZtKK1/kN/xgf7Vrsj+PAO3f7X1a+?=
 =?us-ascii?Q?/mTbA8z+VbRbMN/Ve6gsmFG2PhAa6covmTtD0ymqvp1MGjqG+FJLbp6idCUI?=
 =?us-ascii?Q?xbc8xUAFM2+fSOmUXAcKMxVd8xQqQ/tTYdqECnizY+QBL67oWyFD5V8Y1lWB?=
 =?us-ascii?Q?0cbP/WhS81vHFXF+vgUiBDR53hBqYQw0Fbqq1w3Hriy1GaOek3bbCsI6CVAG?=
 =?us-ascii?Q?uQBZWHxpSdCmwB6cbIpx/R3oPTrYW8DGORp46y3SI0PZCZs3UcacXbQjjF+Q?=
 =?us-ascii?Q?9/CedJzI7egahFPdNSiXP3dbU0TGV1+nxOEFjaur4A8U2CZTDMxJ1Jr6nOzi?=
 =?us-ascii?Q?0cQSwknkuI+O5IIIYLEg9GVDL78qhGn472h5m5QCE50LRl7Sy/VU4fUi1JiE?=
 =?us-ascii?Q?dq6dICXmOXHfqho0EX5r4oKG53oGsQC5zgv6PtKD6wAeSupWcaDlQFauPzNJ?=
 =?us-ascii?Q?r2p38SD+n4fYhoY1mqmD2OpNbBMhpj37nJy8HXgaEoBGhJgK8YORYAt7sEqo?=
 =?us-ascii?Q?+O6v2nO3V0nY2CzSpFzsvCEjYs3mnKTTBKifYTf8ShhoxpUq0rbwaLSHoIMO?=
 =?us-ascii?Q?mJ+AZuNSPFFD++XuGsn0ak4XEfvt9Za8BAS+8ibQP8/zQOzQbc33nOUGV2fT?=
 =?us-ascii?Q?23HItmF7gpI8usB8K53vEVioLvcG1XCXzL0HrWDTU46vwAlJhTE+Xc7Ij7zo?=
 =?us-ascii?Q?+BZbU4sr8jN0mfeBfL86Iu7XNPHJ7p0f2JtJZPWne2kniJkLFSBYYV9jwIEp?=
 =?us-ascii?Q?OwI7eBlaMfrIVlxVBhQ3SAq2UGJSiCzPCgPd5gzzWgxMEWwMR5aYMFKov31D?=
 =?us-ascii?Q?tNQsTg=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2f94fb65-dd39-462e-f74d-08d99df4a6d8
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Nov 2021 11:33:54.9402
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: i3SdlkHsbIldMdjjyrba15vCmV0PsOcrmWnVx1xn5axgKtPTyJo+9TP9Jv0jf8jPZulvd69wWLnrZaPpNxilwWxZ1d7LLTjaHBWVeHvQlic=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW5PR10MB5808
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10155 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 phishscore=0 malwarescore=0
 mlxscore=0 suspectscore=0 mlxlogscore=999 adultscore=0 spamscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2111020068
X-Proofpoint-GUID: u_TdqyWwXLHeLY6rXCevwrd6ps9jupLL
X-Proofpoint-ORIG-GUID: u_TdqyWwXLHeLY6rXCevwrd6ps9jupLL
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, Oct 18, 2021 at 01:19:45AM +0900, Tsuchiya Yuto wrote:
> The function ia_css_mipi_is_source_port_valid() returns true if the port
> is valid. So, we can't use the existing err variable as is.
> 
> To fix this issue while reusing that variable, invert the return value
> when assigning it to the variable.
> 
> Fixes: 3c0538fbad9f ("media: atomisp: get rid of most checks for ISP2401 version")
> Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
> ---
>  .../staging/media/atomisp/pci/sh_css_mipi.c   | 24 ++++++++++++-------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> index 65fc93c5d56b..c1f2f6151c5f 100644
> --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> @@ -423,10 +423,12 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
>  		return 0; /* AM TODO: Check  */
>  	}
>  
> -	if (!IS_ISP2401)
> +	if (!IS_ISP2401) {
>  		port = (unsigned int)pipe->stream->config.source.port.port;
> -	else
> -		err = ia_css_mipi_is_source_port_valid(pipe, &port);
> +	} else {
> +		/* Returns true if port is valid. So, invert it */
> +		err = !ia_css_mipi_is_source_port_valid(pipe, &port);

Don't invert it...  This isn't supposed to return 1 on failure it's
supposed to return negative error codes.


> +	}
>  
>  	assert(port < N_CSI_PORTS);
>  
> @@ -553,10 +555,12 @@ free_mipi_frames(struct ia_css_pipe *pipe)
>  			return err;
>  		}
>  
> -		if (!IS_ISP2401)
> +		if (!IS_ISP2401) {
>  			port = (unsigned int)pipe->stream->config.source.port.port;
> -		else
> -			err = ia_css_mipi_is_source_port_valid(pipe, &port);
> +		} else {
> +			/* Returns true if port is valid. So, invert it */
> +			err = !ia_css_mipi_is_source_port_valid(pipe, &port);

Presumably the same here?

> +		}
>  
>  		assert(port < N_CSI_PORTS);
>  
> @@ -665,10 +669,12 @@ send_mipi_frames(struct ia_css_pipe *pipe)
>  		/* TODO: AM: maybe this should be returning an error. */
>  	}
>  
> -	if (!IS_ISP2401)
> +	if (!IS_ISP2401) {
>  		port = (unsigned int)pipe->stream->config.source.port.port;
> -	else
> -		err = ia_css_mipi_is_source_port_valid(pipe, &port);
> +	} else {
> +		/* Returns true if port is valid. So, invert it */
> +		err = !ia_css_mipi_is_source_port_valid(pipe, &port);

Same?

> +	}
>  
>  	assert(port < N_CSI_PORTS);

regards,
dan carpenter