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, 7 Jul 2020 17:55:17 +0000
From:   Tomasz Figa <tfiga@chromium.org>
To:     Jonathan Bakker <xc-racer2@live.ca>
Cc:     kyungmin.park@samsung.com, s.nawrocki@samsung.com,
        mchehab@kernel.org, kgene@kernel.org, krzk@kernel.org,
        linux-media@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 03/11] media: exynos4-is: Fix nullptr when no CSIS device
 present
Message-ID: <20200707175517.GC2621465@chromium.org>
References: <20200426022650.10355-1-xc-racer2@live.ca>
 <BN6PR04MB0660EE7304C2BB2E603A8824A3AE0@BN6PR04MB0660.namprd04.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <BN6PR04MB0660EE7304C2BB2E603A8824A3AE0@BN6PR04MB0660.namprd04.prod.outlook.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Jonathan,

On Sat, Apr 25, 2020 at 07:26:42PM -0700, Jonathan Bakker wrote:
> Not all devices use the CSIS device, some may use the FIMC directly in
> which case the CSIS device isn't registered.  This leads to a nullptr
> exception when starting the stream as the CSIS device is always
> referenced.  Instead, if getting the CSIS device fails, try getting the
> FIMC directly to check if we are using the subdev API
> 
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> ---
>  drivers/media/platform/exynos4-is/media-dev.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 

Thank you for the patch. Please see my comments inline.

> diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
> index 9aaf3b8060d5..5c32abc7251b 100644
> --- a/drivers/media/platform/exynos4-is/media-dev.c
> +++ b/drivers/media/platform/exynos4-is/media-dev.c
> @@ -289,11 +289,26 @@ static int __fimc_pipeline_s_stream(struct exynos_media_pipeline *ep, bool on)
>  		{ IDX_CSIS, IDX_FLITE, IDX_FIMC, IDX_SENSOR, IDX_IS_ISP },
>  	};
>  	struct fimc_pipeline *p = to_fimc_pipeline(ep);
> -	struct fimc_md *fmd = entity_to_fimc_mdev(&p->subdevs[IDX_CSIS]->entity);
>  	enum fimc_subdev_index sd_id;
>  	int i, ret = 0;
>  
>  	if (p->subdevs[IDX_SENSOR] == NULL) {
> +		struct fimc_md *fmd;
> +		struct v4l2_subdev *sd = p->subdevs[IDX_CSIS];
> +
> +		if (!sd)
> +			sd = p->subdevs[IDX_FIMC];
> +
> +		if (!sd) {
> +			/*
> +			 * If neither CSIS nor FIMC was set up,
> +			 * it's impossible to have any sensors
> +			 */
> +			return -ENODEV;
> +		}
> +
> +		fmd = entity_to_fimc_mdev(&sd->entity);
> +

Are you sure this is the correct thing to do here? In general, the media
controller should be instantiated only if there are sensors in the system.

What do you mean by using "the FIMC directly"? Do you mean using it only as
an m2m image processor or with a sensor, but without the CSIS, which would
be the case for parallel I/F sensors?

Could you point me to the place where CSIS is always dereferenced? A quick
look through the code only revealed that everywhere it seems to be guarded
by a NULL check.

Another thought from looking at the implementation of
__fimc_pipeline_s_stream() is that it probably shouldn't call s_stream on
all the subdevices included in seq[], but only on those that are actually
included as a part of the pipeline. It would be quite a waste of power to
enable unnecessary hardware.

Best regards,
Tomasz