Re: cio2 ipu3 module to automatically connect sensors via swnodes

[Dan Scally <djrscally@gmail.com>]

Hi Heikki

On 09/09/2020 14:40, Heikki Krogerus wrote:
> Hi,
>
> On Sat, Sep 05, 2020 at 09:19:51AM +0100, Daniel Scally wrote:
>> Hello
>>
>> Following on from this thread:
>> https://www.spinics.net/lists/linux-driver-devel/msg135122.html -
>> apologies, can't see a way to reply to it directly.
>>
>> Myself and others [1] have been trying to get cameras working on
>> Microsoft Surface and similar platforms, currently I'm working on
>> expanding Jordan's module connecting cameras to the cio2
>> infrastructure (which works - we can use it to take images), aiming to
>> discover connection information from SSDB in the DSDT, as well as
>> connect as many supported sensors as are found on the device. I'm just
>> struggling with a problem I've encountered using the swnode patch that
>> Heikki linked in that thread; the module's working ok when I only
>> attempt to connect a single one of my sensors (by preventing the
>> driver for the other sensor from loading, in which case this new
>> module ignores the sensor), but when I attempt to connect both cameras
>> at the same time I get a kernel oops as part of
>> software_node_get_next_child. The module is creating software_nodes
>> like this...
>>
>> /sys/kernel/software_node/INT343E/port0/endpoint0
>> /sys/kernel/software_node/INT343E/port1/endpoint0
>> /sys/kernel/software_node/OVTI2680/port0/endpoint0
>> /sys/kernel/software_node/OVTI5648/port0/endpoint0
>>
>> And that's the structure that I expect to see, but it seems like the
>> call to list_next_entry in that function is returning something that
>> isn't quite a valid swnode. Printing the address of c->fwnode after
>> that point returns "3", which isn't a valid address of course, and
>> that's causing the oops when it's either returned (in the version of
>> the function without the patches) or when the program flow tries to
>> call the "get" op against that fwnode (in the patched version). I've
>> been trying to track it down for a while now without success, so I
>> posted the problem on SO[2] and it was suggested that I mail these
>> addressees for advice - hope that that is ok.
>>
>> My copy of Jordan's module is parked in my git repo [3] for now, and
>> requires a batch of patches from Jordan's repo [4] (one made by Heikki
>> to fill in the missing swnode graph pieces, and some further tweaks) -
>> I've been applying those against 5.8.0-rc7. Any other criticism more
>> than welcome - I'm new to both c and kernel programming so I'm happy
>> to take all the advice people have the time to give.
> I'm almost certain that my graph patch is broken. My tests did not
> cover nearly as much that is needed to properly test the patch. I
> think at least the refcount handling is totally broken in
> software_node_graph_get_next_endpoint(), so that function at least
> needs to be rewritten.
>
> Unfortunately I do not have time to work on that patch right now.
>
> thanks,

Alright no problem; I shall persevere with what we have and see if I can 
figure out why the linking isn't working. I think I actually found the 
problem with refcount handling in software_node_graph_get_endpoint(); 
see 
https://lore.kernel.org/linux-media/2d4f1abb-c617-476a-1005-0ed91906a5f5@gmail.com/T/#mf0dcf7dd78ae0cd40af998bc25a5a775c7e3f93d 
- that function seems to behave as expected after a small tweak (at 
least as I expect it to...it doesn't get and hold 4x references to the 
port fwnode_handle now), but there might be one more refcount handling 
issue somewhere (I see one more reference held than I expect to see at 
the moment...but it could be from my code too of course).


If I get to the bottom of the problems I'll let you know.


Regards

Dan