lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <97fbf01c-6cfb-7ab9-f045-383a1e4053c2@microchip.com>
Date:   Wed, 17 Nov 2021 16:52:40 +0000
From:   <Eugen.Hristev@...rochip.com>
To:     <sakari.ailus@...ux.intel.com>
CC:     <leonl@...pardimaging.com>, <linux-media@...r.kernel.org>,
        <skomatineni@...dia.com>, <luca@...aceresoli.net>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] media: i2c: imx274: fix s_frame_interval runtime resume
 not requested

On 11/17/21 6:11 PM, Sakari Ailus wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Eugen,
> 
> On Wed, Nov 17, 2021 at 05:40:09PM +0200, Eugen Hristev wrote:
>> pm_runtime_resume_and_get should be called when the s_frame_interval
>> is called.
>>
>> The driver will try to access device registers to configure VMAX, coarse
>> time and exposure.
>>
>> Currently if the runtime is not resumed, this fails:
>>   # media-ctl -d /dev/media0 --set-v4l2 '"IMX274 1-001a":0[fmt:SRGGB10_1X10/3840x2
>> 160@...0]'
>>
>> IMX274 1-001a: imx274_binning_goodness: ask 3840x2160, size 3840x2160, goodness 0
>> IMX274 1-001a: imx274_binning_goodness: ask 3840x2160, size 1920x1080, goodness -3000
>> IMX274 1-001a: imx274_binning_goodness: ask 3840x2160, size 1280x720, goodness -4000
>> IMX274 1-001a: imx274_binning_goodness: ask 3840x2160, size 1280x540, goodness -4180
>> IMX274 1-001a: __imx274_change_compose: selected 1x1 binning
>> IMX274 1-001a: imx274_set_frame_interval: input frame interval = 1 / 10
>> IMX274 1-001a: imx274_read_mbreg : addr 0x300e, val=0x1 (2 bytes)
>> IMX274 1-001a: imx274_set_frame_interval : register SVR = 1
>> IMX274 1-001a: imx274_read_mbreg : addr 0x30f6, val=0x6a8 (2 bytes)
>> IMX274 1-001a: imx274_set_frame_interval : register HMAX = 1704
>> IMX274 1-001a: imx274_set_frame_length : input length = 2112
>> IMX274 1-001a: imx274_write_mbreg : i2c bulk write failed, 30f8 = 884 (3 bytes)
>> IMX274 1-001a: imx274_set_frame_length error = -121
>> IMX274 1-001a: imx274_set_frame_interval error = -121
>> Unable to setup formats: Remote I/O error (121)
>>
>> The device is not resumed thus the remote I/O error.
>>
>> Setting the frame interval works at streaming time, because
>> pm_runtime_resume_and_get is called at s_stream time before sensor setup.
>> The failure happens when only the s_frame_interval is called separately
>> independently on streaming time.
>>
>> Fixes: ad97bc37426c ("media: i2c: imx274: Add IMX274 power on and off sequence"
>> Signed-off-by: Eugen Hristev <eugen.hristev@...rochip.com>
>> ---
>>   drivers/media/i2c/imx274.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
>> index e89ef35a71c5..6e63fdcc5e46 100644
>> --- a/drivers/media/i2c/imx274.c
>> +++ b/drivers/media/i2c/imx274.c
>> @@ -1420,6 +1420,10 @@ static int imx274_s_frame_interval(struct v4l2_subdev *sd,
>>        int min, max, def;
>>        int ret;
>>
>> +     ret = pm_runtime_resume_and_get(&imx274->client->dev);
>> +     if (ret < 0)
>> +             return ret;
>> +
>>        mutex_lock(&imx274->lock);
>>        ret = imx274_set_frame_interval(imx274, fi->interval);
>>
>> @@ -1451,6 +1455,7 @@ static int imx274_s_frame_interval(struct v4l2_subdev *sd,
>>
>>   unlock:
>>        mutex_unlock(&imx274->lock);
>> +     pm_runtime_put(&imx274->client->dev);
>>
>>        return ret;
>>   }
> 
> If the device is powered off in the end, could you instead not power it on
> in the first place? I.e. see how this works for the s_ctrl() callback.


Hi Sakari,

I tried this initially, as in s_ctrl,

         if (!pm_runtime_get_if_in_use(&imx274->client->dev)) 

                 return 0;


However, if the device is powered off, the s_frame_interval does not do 
anything (return 0), and the frame interval is not changed. Not even the 
internal structure frame_interval is updated (as this is updated after 
configuring the actual device).
And in consequence media-ctl -p will still print the old frame interval.

So either we power on the device to set everything, or, things have to 
be set in the software struct and written once streaming starts.
I am in favor of the first option (hence the patch), to avoid having 
configuration that was requested but not written to the device itself.
The second option would require some rework to move the software part 
before the hardware part, and to assume that the hardware part never 
fails in bounds or by other reason (or the software part would be no 
longer consistent)

What do you think ?

Eugen

> 
> --
> Kind regards,
> 
> Sakari Ailus
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ