Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <48a28601-a3eb-8735-6a15-34436dcbd73e@redhat.com>
Date:   Tue, 1 Nov 2022 14:27:53 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.1
Subject: Re: [PATCH AUTOSEL 6.0 15/34] media: atomisp-ov2680: Fix
 ov2680_set_fmt()
Content-Language: en-US, nl
To:     Sasha Levin <sashal@kernel.org>, linux-kernel@vger.kernel.org,
        stable@vger.kernel.org
Cc:     Andy Shevchenko <andy.shevchenko@gmail.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        gregkh@linuxfoundation.org, linux-media@vger.kernel.org,
        linux-staging@lists.linux.dev
References: <20221101112726.799368-1-sashal@kernel.org>
 <20221101112726.799368-15-sashal@kernel.org>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20221101112726.799368-15-sashal@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Sasha,

I have no specific objections against the backporting of this
and other atomisp related patches.

But in general the atomisp driver is not yet in a state where
it is ready to be used by normal users. Progress is being made
but atm I don't really expect normal users to have it enabled /
in active use.

As such I'm also not sure if there is much value in backporting
atomisp changes to the stable series.

I don't know if you have a way to opt out certain drivers /
file-paths from stable series backporting, but if you do
you may want to consider opting out everything under:

drivers/staging/media/atomisp/

As said above I don't think doing the backports offers
much (if any) value to end users and I assume it does take
you some time, so opting this path out might be better.

Also given the fragile state of atomisp support atm
it is hard to say for me if partially backporting some of
the changes won't break the driver.

Regards,

Hans


On 11/1/22 12:27, Sasha Levin wrote:
> From: Hans de Goede <hdegoede@redhat.com>
> 
> [ Upstream commit adea153b4f6537f367fe77abada263fde8a1f7b6 ]
> 
> On sets actually store the set (closest) format inside ov2680_device.dev,
> so that it also properly gets returned by get_fmt.
> 
> This fixes the following problem:
> 
> 1. App does an VIDIOC_SET_FMT 640x480, calling ov2680_set_fmt()
> 2. Internal buffers (atomisp_create_pipes_stream()) get allocated
>    at 640x480 size by atomisp_set_fmt()
> 3. ov2680_get_fmt() gets called later on and returns 1600x1200
>    since ov2680_device.dev was not updated. So things get configured
>    to stream at 1600x1200, but the internal buffers created during
>    atomisp_create_pipes_stream() do not get updated in size
> 4. streaming starts, internal buffers overflow and the entire
>    machine freezes eventually due to memory being corrupted
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>  drivers/staging/media/atomisp/i2c/atomisp-ov2680.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> index 4ba99c660681..ab52e35266bb 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
> @@ -894,11 +894,7 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
>  	if (v_flag)
>  		ov2680_v_flip(sd, v_flag);
>  
> -	/*
> -	 * ret = startup(sd);
> -	 * if (ret)
> -	 * dev_err(&client->dev, "ov2680 startup err\n");
> -	 */
> +	dev->res = res;
>  err:
>  	mutex_unlock(&dev->input_lock);
>  	return ret;