[libcamera-devel] [PATCH] libcamera: sysfs: Fix directory exists check

[Sebastian Fricke]

On 09.01.2021 07:35, Niklas Söderlund wrote:
>Hi Sebastian,
>
>Thanks for testing patch.

It is my pleasure :)

>
>On 2021-01-09 07:12:23 +0100, Sebastian Fricke wrote:
>> Hey Niklas,
>>
>> Thank you for the patch.
>> I have tested your patch on my FriendlyElec NanoPC-T4 development board
>> with the OV13850.
>> *Before* I applied your patch the output looked like this:
>> ```
>> basti at nanopct4:~$ cam -l
>> Available cameras:
>> 1: Internal front camera (platform/ff110000.i2c/i2c-1/1-0010 ov13850)
>> ```
>> And *after* applying your patch it looks like this:
>> ```
>> basti at nanopct4:~$ cam -l
>> Available cameras:
>> 1: Internal front camera (/base/i2c at ff110000/ov13850 at 10)
>> ```
>
>It looks like the patch solves the persistent naming regression on your
>platform as well. If you test a camera enumeration before
>8f4e16f014c820a0 dose it match the later case
>(/base/i2c at ff110000/ov13850 at 10) ?

Here the output exactly one patch before '8f4e16f014c820a0':

basti at nanopct4:~/libcamera$ cam -l
Available cameras:
1: Internal front camera (/base/i2c at ff110000/ov13850 at 10)
basti at nanopct4:~/libcamera$ git log
commit 0ed053245702d193b9c0ad7d8bda9e975ddfb6ed (HEAD -> tmp)
Author: Jacopo Mondi <jacopo at jmondi.org>
Date:   Wed Dec 23 18:34:34 2020 +0100

     android: camera_device: Simplify properties.get()

And here the output when I build with '8f4e16f014c820a0':

basti at nanopct4:~/libcamera$ cam -l
Available cameras:
1: Internal front camera (/base/i2c at ff110000/ov13850 at 10)
basti at nanopct4:~/libcamera$ git log
commit 8f4e16f014c820a0ecb85e28453b88c277bc859f (HEAD -> tmp)
Author: Kieran Bingham <kieran.bingham at ideasonboard.com>
Date:   Tue Dec 22 13:50:20 2020 +0000

     test: file: Check that directories are not treated as files

The commit you mention does only touch a unit test, so it seems to be
impossible that this is the patch that you fix.

I looked it up and the patch that you actually fix is:
0a823785fad27e1eb9a900e80923bc9bde106de9
"libcamera: file: Check file exist()"

Here is the output with '0a823785fad27e1eb9a900e80923bc9bde106de9'
applied:

basti at nanopct4:~/libcamera$ cam -l
Available cameras:
1: Internal front camera (platform/ff110000.i2c/i2c-1/1-0010 ov13850)
basti at nanopct4:~/libcamera$ git log
commit 0a823785fad27e1eb9a900e80923bc9bde106de9 (HEAD -> tmp)
Author: Kieran Bingham <kieran.bingham at ideasonboard.com>
Date:   Mon Nov 30 23:25:40 2020 +0000

     libcamera: file: Check files exist()

The patch "8f4e16f014c820a0ecb85e28453b88c277bc859f" comes right before
"0a823785fad27e1eb9a900e80923bc9bde106de9". So yes, before '..859f' the
output was correct.

>
>>
>> I also tested this patch on my laptop:
>> *Before*:
>> ```
>> basti at basti-TUXEDO-Book-XA1510:~$ cam -l
>> Available cameras:
>> 1: External camera 'Chicony USB2.0 Camera: Chicony ' (\_SB_.PCI0.GP18.XHC0.RHUB.PRT4-4:1.0-04f2:b685)
>> ```
>> *After*:
>> ```
>> basti at basti-TUXEDO-Book-XA1510:~$ cam -l
>> Available cameras:
>> 1: External camera 'Chicony USB2.0 Camera: Chicony ' (\_SB_.PCI0.GP18.XHC0.RHUB.PRT4-4:1.0-04f2:b685)
>> ```
>>
>> The output on my laptop looks a little odd, but that is another issue.
>
>What is odd about your laptop output?

I mainly think it looks odd because of the starting backslash and the
total different structure. But it is no big issue.

>
>>
>> Greetings
>>
>> Sebastian
>>
>> On 08.01.2021 18:00, Niklas Söderlund wrote:
>> > The scope of File::exists() was changed to only validate that a file
>> > exists and is therefore not usable to check if a directory exists. This
>> > breaks the persistent name generation for DT based systems as it uses
>> > File::exists() to check for directories, fix this by using stat()
>> > directly.
>> >
>> > On Scarlet the persistent names of the cameras are impacted by this and
>> > where incorrectly reported as:
>> >
>> >    $ cam -l
>> >    Available cameras:
>> >    1: Internal front camera (platform/ff160000.i2c/i2c-7/7-003c ov2685)
>> >    2: Internal front camera (platform/ff160000.i2c/i2c-7/7-0036 ov5695
>> >
>> > While the expected ones are restored with this fix:
>> >
>> >    $ cam -l
>> >    Available cameras:
>> >    1: Internal front camera (/base/i2c at ff160000/camera at 3c)
>> >    2: Internal front camera (/base/i2c at ff160000/camera at 36)
>> >
>> > Fixes: 8f4e16f014c820a0 ("test: file: Check that directories are not treated as files")
>> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>> > ---
>> > src/libcamera/sysfs.cpp | 3 ++-
>> > 1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/src/libcamera/sysfs.cpp b/src/libcamera/sysfs.cpp
>> > index 3ebe66f8d69b61d4..e9004b2b59c8638d 100644
>> > --- a/src/libcamera/sysfs.cpp
>> > +++ b/src/libcamera/sysfs.cpp
>> > @@ -70,10 +70,11 @@ std::string charDevPath(const std::string &deviceNode)
>> > std::string firmwareNodePath(const std::string &device)
>> > {
>> > 	std::string fwPath, node;
>> > +	struct stat st;
>> >
>> > 	/* Lookup for DT-based systems */
>> > 	node = device + "/of_node";
>> > -	if (File::exists(node)) {
>> > +	if (!stat(node.c_str(), &st)) {
>> > 		char *ofPath = realpath(node.c_str(), nullptr);
>> > 		if (!ofPath)
>> > 			return {};
>> > --
>> > 2.30.0
>> >
>> > _______________________________________________
>> > libcamera-devel mailing list
>> > libcamera-devel at lists.libcamera.org
>> > https://lists.libcamera.org/listinfo/libcamera-devel
>
>-- 
>Regards,
>Niklas Söderlund