linux-kernel - Re: [PATCH v6 4/4] media: i2c: add MAX96714 driver

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: <9075808c-783b-4af6-a92f-2a6d3f25d225@collabora.com>
Date: Wed, 24 Apr 2024 10:42:11 +0200
From: Julien Massot <julien.massot@...labora.com>
To: Sakari Ailus <sakari.ailus@....fi>
Cc: linux-media@...r.kernel.org, devicetree@...r.kernel.org,
 kernel@...labora.com, linux-kernel@...r.kernel.org, mchehab@...nel.org,
 robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org
Subject: Re: [PATCH v6 4/4] media: i2c: add MAX96714 driver

Hi Sakari,

On 4/24/24 10:30, Sakari Ailus wrote:
> Hi Julien,
> 
> On Tue, Apr 23, 2024 at 04:01:16PM +0200, Julien Massot wrote:
> 
> ...
> 
>>>> +static int max96714_enable_streams(struct v4l2_subdev *sd,
>>>> +				   struct v4l2_subdev_state *state,
>>>> +				   u32 source_pad, u64 streams_mask)
>>>> +{
>>>> +	struct max96714_priv *priv = sd_to_max96714(sd);
>>>> +	u64 sink_streams;
>>>> +	int ret;
>>>> +
>>>> +	if (!priv->enabled_source_streams)
>>>> +		max96714_enable_tx_port(priv);
>>>> +
>>>> +	ret = max96714_apply_patgen(priv, state);
>>>> +	if (ret)
>>>> +		goto err;
>>>> +
>>>> +	if (!priv->pattern) {
>>>> +		if (!priv->rxport.source.sd) {
>>>> +			ret = -ENODEV;
>>>> +			goto err;
>>>> +		}
On enable streams the check is here :)
Streaming is not possible without a remote serializer when pattern 
generator is disabled.

I may refactor this code later when we will have the internal pad, to 
declare
properly this stream.
.

>>>> +
>>>> +		sink_streams =
>>>> +			v4l2_subdev_state_xlate_streams(state,
>>>> +							MAX96714_PAD_SOURCE,
>>>> +							MAX96714_PAD_SINK,
>>>> +							&streams_mask);
>>>> +
>>>> +		ret = v4l2_subdev_enable_streams(priv->rxport.source.sd,
>>>> +						 priv->rxport.source.pad,
>>>> +						 sink_streams);
>>>> +		if (ret)
>>>> +			goto err;
>>>> +	}
>>>> +
>>>> +	priv->enabled_source_streams |= streams_mask;
>>>> +
>>>> +	return 0;
>>>> +
>>>> +err:
>>>> +	if (!priv->enabled_source_streams)
>>>> +		max96714_disable_tx_port(priv);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int max96714_disable_streams(struct v4l2_subdev *sd,
>>>> +				    struct v4l2_subdev_state *state,
>>>> +				    u32 source_pad, u64 streams_mask)
>>>> +{
>>>> +	struct max96714_priv *priv = sd_to_max96714(sd);
>>>> +	u64 sink_streams;
>>>> +	int ret;
>>>> +
>>>> +	if (!priv->pattern && priv->rxport.source.sd) {
>>>
>>> When will priv->rxport.source.sd be NULL here?
>>
>> Indeed it should not, the priv->rxport.source.sd can only be null if:
>> - There is no serializer
>> - The stream has been started with pattern generator and the pattern
>> generator
>> has been disabled while streaming.
> 
> It seems priv->rxport.source.sd is also accessed in
> max96714_enable_streams() without such a check.
Please see my reply above :)

> 
>>
>> In V7 I will drop this check and add another one to prevent disabling the
>> pattern
>> generator while streaming.
> 
> Sounds good.
> 
>>>> +static void max96714_v4l2_notifier_unregister(struct max96714_priv *priv)
>>>> +{
>>>> +	v4l2_async_nf_unregister(&priv->notifier);
>>>> +	v4l2_async_nf_cleanup(&priv->notifier);
>>>
>>> It'd be nicer to call these directly IMO. Maybe we could introduce
>>> v4l2_async_nf_unregister_cleanup()? Feel free to post a patch. :-)
>> Ok, I will call these directly, and I will do the same for the MAX96717
>> serializer.
>>
>> I will post a patchset later introducing the
>> `v4l2_async_nf_unregister_cleanup`
>> and converting all the drivers calling these two functions.
> 
> That would be nice. :-) It should be easy to do that with Coccinelle.
> 
> ...
> 
>>>> +	ret = max96714_enable_core_hw(priv);
>>>
>>> Please switch to runtime PM.
>>
>> Ok, the v7 will use runtime PM and I will use the powerdown gpio
>> to poweroff the device. However it implies to move some functions arround
>> e.g initialize the tx or the pattern generator ..
>> So it it will be done as separate patches.
>>
>> Playing with the pm_runtime operation also showed up that the connection
>> doesn't always resume properly, I will extra patches to fix that.
> 
> Ack.
> 

-- 
Julien Massot
Senior Software Engineer
Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718