Re: [PATCH v3 18/18] media: atomisp: csi2-bridge: Add support for VCM I2C-client instantiation

Hans de Goede
2023-07-06T12:59:42Z

Hi,

On 7/6/23 14:56, Andy Shevchenko wrote:
> On Thu, Jul 06, 2023 at 02:47:36PM +0200, Hans de Goede wrote:
>> On 7/6/23 14:42, Andy Shevchenko wrote:
>>> On Thu, Jul 06, 2023 at 02:31:14PM +0200, Hans de Goede wrote:
>>>> On 7/6/23 12:15, Andy Shevchenko wrote:
>>>>> On Wed, Jul 05, 2023 at 11:30:10PM +0200, Hans de Goede wrote:
>>>
>>>>>> + vcm_type = kstrdup(obj->string.pointer, GFP_KERNEL);
>>>>>
>>>>> Where is the counterpart kfree()?
>>>>
>>>> The vcm-type is stored in one of the generated sw-nodes and the ipu-bridge
>>>> code only creates those once and them leaves them in memory, even on
>>>> a rmmod. So this is deliberately leaked just like that the ipu_bridge
>>>> struct which contains all the swnode-s is deliberately leaked by
>>>> ipu-bridge.c
>>>
>>> Should we worry about those leakages?
>>
>> No this is by design because removing the swnodes while e.g. a sensor
>> driver might still be bound to the i2c-client is trouble-some and
>> the callers of ipu_bridge_init check if it has already run and then
>> skip calling it.
>>
>> So after a rmmod + modprobe of the atomisp / ipu3-cio2 driver
>> ipu_bridge_init() will not get called a second time. Instead
>> the old swnodes (1) which are already set as secondary fwnodes for
>> the sensor and bridge devices are re-used.
>
> But this will be actual leak if we hot unplug/plug back the device, right?
> (I think we can do that in some [debug?] cases).

No, the kstrdup() is called from the parse_sensor_fwnode() callback passed
to ipu_bridge_init(), so it will only happen once even on unbind + re-bind
or rmmod + modprobe.

Regards,

Hans