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:   Mon, 8 Nov 2021 18:14:55 +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: <20211108151455.GI2001@kadam>
References: <20211017161958.44351-1-kitakar@gmail.com>
 <20211017161958.44351-6-kitakar@gmail.com>
 <20211102113332.GC2794@kadam>
 <ed17420d96bd302479b528f7ae8694ff9cd2e72a.camel@gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ed17420d96bd302479b528f7ae8694ff9cd2e72a.camel@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?Ws/HybgEpF72yrxQ2vqmE5QURUCfOnHlMtrbgC5bT8rBplSHNpPGAAd4vVsU?=
 =?us-ascii?Q?DX+TMq5lgs8L0LQURzE7pDsNOOsCbq7ii1OZm7+MDh5YzGSt8Z+MuY/73ofH?=
 =?us-ascii?Q?6Up4hjf7iBIkUY2dHmIjV+WWX3pExr92oneFs0LyzCa5CF1HZGajrjcqfhde?=
 =?us-ascii?Q?zWz0gaz+mGGRsK30oi9FsObELv08Zuka0Cnq44DNhd7nYlRczT5dBEfOXDl8?=
 =?us-ascii?Q?owVuyF2Qp+V3kw/nCD8/sQ/oqVnzUTHgNx4aBcbre33Zbc0ST0BzyoMuKQ1B?=
 =?us-ascii?Q?vj5qN1L+Vj6JGY6SOjtIvdHLo4l1U7AB3mzPi8D3lB99R+QjyzAfduhFtg9X?=
 =?us-ascii?Q?/pQjTdeWZtKhzxhDQ6TM9py/n61qOJFnCDpqQjK/DGeJ1Se1+Vwv64LBt8Ol?=
 =?us-ascii?Q?Ikd6TKnc8yQbu796YDkcy3RUbR4r0GevZK60eKYsgPrA2U+AObkX+kqpb036?=
 =?us-ascii?Q?A+6A97s08cnB3RbqWwUAyHRvq2sMk01sMWlIoVuRT69e7ob0CFYSyR6FSYZb?=
 =?us-ascii?Q?8bxG0jXZFdE26KnGZsjRusgZJNSz8q4HmZWTZvWslYMCKAaGGjlQfkj/VJrM?=
 =?us-ascii?Q?4xb3sb/g9PRy9Yk+yB8QFjR2tA20+27rOdQaFmmvQo1x3bwjz61fywBx8WX+?=
 =?us-ascii?Q?vlxbQvhpb57ihKnSfkI/cVt7UCYhjzfVDaCAk2Kl6ump9m7iUNyX3FKOklEM?=
 =?us-ascii?Q?naML41XibmOJ2btVNFz+5q36f0sEenNZbHHESeBwJcNaStq6ORPE563praen?=
 =?us-ascii?Q?v1V9JDg72lFrewi3uens4asjrDq84+8DEkk5RsJxpp1E6GlZIJJ/8Wfq9ePL?=
 =?us-ascii?Q?PnUU7GZFNI/cZJArIi3/DP9NL6u9xMhFZQkC2V+BNpQ/VefCkgLua20dMNR8?=
 =?us-ascii?Q?3tTMpA7gnKjJFuQYqcoWDAMU1UEqHZ5z94qypBk+8TrDEPvNfI/KCf7R0mbL?=
 =?us-ascii?Q?yV9eHF1ydJE1UPawoWXmAv96pAGCTTvjGtlYz6NMIVspYUvRS8PE7DIFcMFb?=
 =?us-ascii?Q?r1cfEmlI7MBE07qJc+a/eVdrMGEYPtYBGTPCL3z4CPeybFIMPxmsdh7Ztlk0?=
 =?us-ascii?Q?OKO7M5cZKgGXHkAX+12wdw60B8pmTOxR+1lqLfycJ0TDV00L5czDBDiy/kx1?=
 =?us-ascii?Q?kEhriFTnQ4C95ufpH7Opes3+Lfds2WeJcD/6qzQaQNX86PeciwggTxjLlr+d?=
 =?us-ascii?Q?svkhPwxy9fCn9y0lSChDDRjn4ZR8i8x0u0V+bW8xVr/pM3pHkgONxudQ0eGn?=
 =?us-ascii?Q?g99FM/j+a/zCwEHx78w2osFMeUTCujP/iBIPa+p/Hvitd6YGZVh5P7gfUtHK?=
 =?us-ascii?Q?GKXdkXkNQXsvUQ1Wdu3ajsoH9j1AwzE5gXEYsUiS8Tr/3GtzI1SvcLMo+Ps4?=
 =?us-ascii?Q?NKLlXyp/or33SKLKlnCKz+OWxMmbmbs13l7zVncSYPdCDCmfSNs6Y38yko2n?=
 =?us-ascii?Q?bus3u9k111BXTWiBXQkKcHi+3iNuAyEhGVf2i75KBuwNoeHFX6GxepOhyGfs?=
 =?us-ascii?Q?YsuhG0ChKoCokNLWTOhHeE0tr95+4vDP1Eun6ZfmZhB6N5tWJt1I32yedzSa?=
 =?us-ascii?Q?ypXiYOH/06CX5Us7Bn6RKXkjCH76yp62zUzat9YPp6T1iE5OR9poW0Iqkl8j?=
 =?us-ascii?Q?K5AGq60OYWZtbD7fIsYLpDzzNosxeV4XsoFxcp/MwPGq4kBoGbiE6inubQGA?=
 =?us-ascii?Q?GD1ZsYATMBjYoHdmVSauKbcp5Z4=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e1158f22-65b5-40a6-c51d-08d9a2ca991a
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Nov 2021 15:15:28.6966
 (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: t6mD9Sq1D/h69dQ+Gt2RcOiHXHcA0GMiaCnUN5bk3fvF1oiqgkhsvvR3YSykVgII7h/FA+nnnL9Nw0/SCfTAkJKKyTnHKLbunUHG0fFHl28=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR1001MB2206
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10161 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 adultscore=0
 mlxlogscore=999 malwarescore=0 phishscore=0 mlxscore=0 suspectscore=0
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2110150000 definitions=main-2111080094
X-Proofpoint-ORIG-GUID: aQHAhjRFiD0t_USO4emAi7ZgtbsOzXHh
X-Proofpoint-GUID: aQHAhjRFiD0t_USO4emAi7ZgtbsOzXHh
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Nov 09, 2021 at 12:00:29AM +0900, Tsuchiya Yuto wrote:
> On Tue, 2021-11-02 at 14:33 +0300, Dan Carpenter wrote:
> > 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.
> 
> You mean I should instead modify the return value of
> ia_css_mipi_is_source_port_valid() ?
> 

No.  ia_css_mipi_is_source_port_valid() is fine.  It has a boolean name
so returning bool is fine.  What I'm saying is that allocate_mipi_frames()
should do:

	if (!ia_css_mipi_is_source_port_valid(pipe, &port))
		err = -EINVAL;

Otherwise it returns negative error codes and 1 on failure.

regards,
dan carpenter