[{"id":11808,"web_url":"https://patchwork.libcamera.org/comment/11808/","msgid":"<20200804074911.h4552ignajgnej7f@uno.localdomain>","date":"2020-08-04T07:49:11","subject":"Re: [libcamera-devel] [PATCH v6 4/9] libcamera: utils: Add method\n\tto lookup firmware ID in sysfs","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Mon, Aug 03, 2020 at 11:17:28PM +0200, Niklas Söderlund wrote:\n> A devices firmware description is recorded differently in sysfs\n> depending on if the devices uses OF or ACPI. Add a helper to abstract\n\ns/on if/if/\ns/devices/device (and that's more likely a system decision, not a\ndevice one)\n\n> this, allowing users to not care which of the two are used.\n\nusers not to care\n\n>\n> For OF-based systems the ID is the full path of the device in the\n> device tree description. For ACPI-based systems the ID is the ACPI\n> firmware nodes path. Both ID sources are guaranteed to be unique and\n> persistent as long as the firmware of the system is not changed.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/internal/utils.h | 2 +\n> src/libcamera/utils.cpp | 61 ++++++++++++++++++++++++++++++\n> 2 files changed, 63 insertions(+)\n>\n> diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h\n> index 45cd6f120c51586b..69977340e2593db6 100644\n> --- a/include/libcamera/internal/utils.h\n> +++ b/include/libcamera/internal/utils.h\n> @@ -200,6 +200,8 @@ details::StringSplitter split(const std::string &str, const std::string &delim);\n> std::string libcameraBuildPath();\n> std::string libcameraSourcePath();\n>\n> +int tryLookupFirmwareID(const std::string &devPath, std::string *id);\n> +\n> constexpr unsigned int alignDown(unsigned int value, unsigned int alignment)\n> {\n> \treturn value / alignment * alignment;\n> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> index 615df46ac142a2a9..07ebce61f230e5f0 100644\n> --- a/src/libcamera/utils.cpp\n> +++ b/src/libcamera/utils.cpp\n> @@ -9,6 +9,7 @@\n>\n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -444,6 +445,66 @@ std::string libcameraSourcePath()\n> \treturn path + \"/\";\n> }\n>\n> +/**\n> + * \\brief Try to read a device firmware ID from sysfs\n> + * \\param[in] devPath Path in sysfs to search\n> + * \\param[out] id Location to store ID if found\n> + *\n> + * The ID recorded in the devices firmware description is recorded differently\n> + * in sysfs depending on if the devices uses OF or ACPI. This helper abstracts\n> + * this away, allowing callers to not care which of the two are used.\n\nSame comments as for the commit message\n\n> + *\n> + * For OF-based systems the ID is the full path of the device in the device tree\n> + * description. For ACPI-based systems the ID is the ACPI firmware nodes path.\n> + * Both ID sources are guaranteed to be unique and persistent as long as the\n> + * firmware of the system is not changed.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -ENOMEM \\a id is nullptr\n> + * \\retval -EINVAL Error when looking up firmware ID\n> + * \\retval -ENODEV No firmware ID aviable for device\n\navailable\n\n> + */\n> +int tryLookupFirmwareID(const std::string &devPath, std::string *id)\n\nI'm going to be picky on names, I know, but why 'try' ? Beause this\nfunction can fail ? Can't the other ones ? I would just 'firwareId()'\n\n> +{\n> +\tstruct stat statbuf;\n> +\tstd::string path;\n> +\n> +\tif (!id)\n> +\t\treturn -EINVAL;\n\nHow can you pass a null string pointer ? By calling this method with\nnullptr explicitely only, isn't it ? Is this worth this check ?\n\n> +\n> +\t/* Try to lookup ID as if the system where OF-based. */\n\nI don't get what this comment means. Maybe 'as is the system was\nOF-based' ? Why not 'ID lookup for OF-based systems' ?\n\n> +\tpath = devPath + \"/of_node\";\n> +\tif (stat(path.c_str(), &statbuf) == 0) {\n\nCan stat fail for other reasons and not just because the file is not\nthere ? Should be check the return value and propagate errors which\nare not caused by the file not being there ? Looking at the stat man\npage it's not trivial though as I see both ENOENT and ENODIR could be\nreturned..\n\n> +\t\tchar *ofPath = realpath(path.c_str(), nullptr);\n> +\t\tif (!ofPath)\n> +\t\t\treturn -EINVAL;\n> +\n> +\t\t*id = ofPath;\n> +\t\tfree(ofPath);\n> +\n> +\t\tstatic const std::string dropStr = \"/sys/firmware/devicetree/\";\n> +\t\tif (id->find(dropStr) == 0)\n> +\t\t\tid->erase(0, dropStr.length());\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +\t/* Try to lookup ID as if the system where ACPI-based. */\n\nSame\n\n> +\tpath = devPath + \"/firmware_node/path\";\n> +\tif (stat(path.c_str(), &statbuf) == 0) {\n\nAnd same here as well :)\n\n> +\t\tstd::ifstream file(path.c_str());\n> +\t\tif (!file.is_open())\n> +\t\t\treturn -EINVAL;\n> +\n> +\t\tstd::getline(file, *id);\n> +\t\tfile.close();\n> +\n> +\t\treturn 0;\n> +\t}\n> +\n> +\treturn -ENODEV;\n> +}\n> +\n> /**\n> * \\fn alignDown(unsigned int value, unsigned int alignment)\n> * \\brief Align \\a value down to \\a alignment\n> --\n> 2.28.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B4218BD86F\n\tfor ;\n\tTue, 4 Aug 2020 07:45:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3821C61B02;\n\tTue, 4 Aug 2020 09:45:37 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EE400609B9\n\tfor ;\n\tTue, 4 Aug 2020 09:45:35 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 3C6241BF203;\n\tTue, 4 Aug 2020 07:45:34 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Tue, 4 Aug 2020 09:49:11 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200804074911.h4552ignajgnej7f@uno.localdomain>","References":"<20200803211733.1037019-1-niklas.soderlund@ragnatech.se>\n\t<20200803211733.1037019-5-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200803211733.1037019-5-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v6 4/9] libcamera: utils: Add method\n\tto lookup firmware ID in sysfs","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11835,"web_url":"https://patchwork.libcamera.org/comment/11835/","msgid":"<20200804140538.GD2566810@oden.dyn.berto.se>","date":"2020-08-04T14:05:38","subject":"Re: [libcamera-devel] [PATCH v6 4/9] libcamera: utils: Add method\n\tto lookup firmware ID in sysfs","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your feedback.\n\nOn 2020-08-04 09:49:11 +0200, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Mon, Aug 03, 2020 at 11:17:28PM +0200, Niklas Söderlund wrote:\n> > A devices firmware description is recorded differently in sysfs\n> > depending on if the devices uses OF or ACPI. Add a helper to abstract\n> \n> s/on if/if/\n> s/devices/device (and that's more likely a system decision, not a\n> device one)\n> \n> > this, allowing users to not care which of the two are used.\n> \n> users not to care\n> \n> >\n> > For OF-based systems the ID is the full path of the device in the\n> > device tree description. For ACPI-based systems the ID is the ACPI\n> > firmware nodes path. Both ID sources are guaranteed to be unique and\n> > persistent as long as the firmware of the system is not changed.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > include/libcamera/internal/utils.h | 2 +\n> > src/libcamera/utils.cpp | 61 ++++++++++++++++++++++++++++++\n> > 2 files changed, 63 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h\n> > index 45cd6f120c51586b..69977340e2593db6 100644\n> > --- a/include/libcamera/internal/utils.h\n> > +++ b/include/libcamera/internal/utils.h\n> > @@ -200,6 +200,8 @@ details::StringSplitter split(const std::string &str, const std::string &delim);\n> > std::string libcameraBuildPath();\n> > std::string libcameraSourcePath();\n> >\n> > +int tryLookupFirmwareID(const std::string &devPath, std::string *id);\n> > +\n> > constexpr unsigned int alignDown(unsigned int value, unsigned int alignment)\n> > {\n> > \treturn value / alignment * alignment;\n> > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> > index 615df46ac142a2a9..07ebce61f230e5f0 100644\n> > --- a/src/libcamera/utils.cpp\n> > +++ b/src/libcamera/utils.cpp\n> > @@ -9,6 +9,7 @@\n> >\n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -444,6 +445,66 @@ std::string libcameraSourcePath()\n> > \treturn path + \"/\";\n> > }\n> >\n> > +/**\n> > + * \\brief Try to read a device firmware ID from sysfs\n> > + * \\param[in] devPath Path in sysfs to search\n> > + * \\param[out] id Location to store ID if found\n> > + *\n> > + * The ID recorded in the devices firmware description is recorded differently\n> > + * in sysfs depending on if the devices uses OF or ACPI. This helper abstracts\n> > + * this away, allowing callers to not care which of the two are used.\n> \n> Same comments as for the commit message\n> \n> > + *\n> > + * For OF-based systems the ID is the full path of the device in the device tree\n> > + * description. For ACPI-based systems the ID is the ACPI firmware nodes path.\n> > + * Both ID sources are guaranteed to be unique and persistent as long as the\n> > + * firmware of the system is not changed.\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + * \\retval -ENOMEM \\a id is nullptr\n> > + * \\retval -EINVAL Error when looking up firmware ID\n> > + * \\retval -ENODEV No firmware ID aviable for device\n> \n> available\n> \n> > + */\n> > +int tryLookupFirmwareID(const std::string &devPath, std::string *id)\n> \n> I'm going to be picky on names, I know, but why 'try' ? Beause this\n> function can fail ? Can't the other ones ? I would just 'firwareId()'\n> \n> > +{\n> > +\tstruct stat statbuf;\n> > +\tstd::string path;\n> > +\n> > +\tif (!id)\n> > +\t\treturn -EINVAL;\n> \n> How can you pass a null string pointer ? By calling this method with\n> nullptr explicitely only, isn't it ? Is this worth this check ?\n\nIt's cheap and prevents us from crashing, but maybe it can be more \nhelpful as an ASSERT(). I will do so for next version.\n\n> \n> > +\n> > +\t/* Try to lookup ID as if the system where OF-based. */\n> \n> I don't get what this comment means. Maybe 'as is the system was\n> OF-based' ? Why not 'ID lookup for OF-based systems' ?\n> \n> > +\tpath = devPath + \"/of_node\";\n> > +\tif (stat(path.c_str(), &statbuf) == 0) {\n> \n> Can stat fail for other reasons and not just because the file is not\n> there ? Should be check the return value and propagate errors which\n> are not caused by the file not being there ? Looking at the stat man\n> page it's not trivial though as I see both ENOENT and ENODIR could be\n> returned..\n\nI thought about that but then I thought what would the caller do \ndifferent if it knew the file existed but for some reason it can't be \nread? This is nothing we can recover from and we will be unable to \nretrieve the ID in any case. I see lot's of work for no gain doing that.\n\n> \n> > +\t\tchar *ofPath = realpath(path.c_str(), nullptr);\n> > +\t\tif (!ofPath)\n> > +\t\t\treturn -EINVAL;\n> > +\n> > +\t\t*id = ofPath;\n> > +\t\tfree(ofPath);\n> > +\n> > +\t\tstatic const std::string dropStr = \"/sys/firmware/devicetree/\";\n> > +\t\tif (id->find(dropStr) == 0)\n> > +\t\t\tid->erase(0, dropStr.length());\n> > +\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > +\t/* Try to lookup ID as if the system where ACPI-based. */\n> \n> Same\n> \n> > +\tpath = devPath + \"/firmware_node/path\";\n> > +\tif (stat(path.c_str(), &statbuf) == 0) {\n> \n> And same here as well :)\n> \n> > +\t\tstd::ifstream file(path.c_str());\n> > +\t\tif (!file.is_open())\n> > +\t\t\treturn -EINVAL;\n> > +\n> > +\t\tstd::getline(file, *id);\n> > +\t\tfile.close();\n> > +\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > +\treturn -ENODEV;\n> > +}\n> > +\n> > /**\n> > * \\fn alignDown(unsigned int value, unsigned int alignment)\n> > * \\brief Align \\a value down to \\a alignment\n> > --\n> > 2.28.0\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2E6DABD87A\n\tfor ;\n\tTue, 4 Aug 2020 14:05:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B2F4F6054A;\n\tTue, 4 Aug 2020 16:05:42 +0200 (CEST)","from mail-lf1-x143.google.com (mail-lf1-x143.google.com\n\t[IPv6:2a00:1450:4864:20::143])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1ACA360545\n\tfor ;\n\tTue, 4 Aug 2020 16:05:41 +0200 (CEST)","by mail-lf1-x143.google.com with SMTP id m15so21755321lfp.7\n\tfor ;\n\tTue, 04 Aug 2020 07:05:41 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\te23sm6316255lfj.80.2020.08.04.07.05.39\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 04 Aug 2020 07:05:39 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"Yu91nKgF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=pzwPwWEzO55TCAd3IttIMkzX5Sg5rKRv3EJhU/i8Heo=;\n\tb=Yu91nKgFqEwfXZQRbHlWvLTYgfqcv//nNmTNdW3RcVj3pX9iqkQFKyS8RsltP6iWeE\n\tC/0xrNU6j2Pyg7uKBcKR8FaD++SYJPNHeBs5r7hlxetbEGsE/GnpKGDvNzdDv7MafPvK\n\tGR11uIk8SS250t3ZNJP8wyzI4TpS2l2Fkpqsy1vIl7F/3CslAyXAJHhsM9x4R/eG1E8a\n\txxmLaBmoPe/6LR7tHFPkG1Rmts7XydyNzNHeZXiaILcQgLMxs10pbKIf1ntbWFyxpb+z\n\toshZv6GdZxIChkB9I5fX0qzFpFkgl1n8102k0CRVhlrgzdWjxDOn8bxQeqJTZ+d2GfAH\n\t2lPQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=pzwPwWEzO55TCAd3IttIMkzX5Sg5rKRv3EJhU/i8Heo=;\n\tb=tVE9tENP5j8Fuai3ilG+C/YjvORIv9tK1rEFHulK/INsgsPrS787Dd6eOxQXeBISoE\n\tqasiRViHzMBopte81P7T3GKkB2zWYhJOQ20JhqUDToI5bLOer2FSrP6Dtjq/leHUR8J/\n\tL96uywSelI6LJmVJQPgcnGu0/SNki2jLLQvi2j8JpaYhc/7TC8UjAeEBHgyyX8qSEI+w\n\t7sTEClXJtTZ9X8RviixUt+c+NC+9wbaKJda/vaV8uE0Qx2RZ4Ll0MlzLPoMKBZ1PEvbf\n\tSZTUAIDcLGET7E+3Z4UfxlKxQ6nWxx5NmZJXcpuDhgljaJYxPt3L/fDurupTmfvp4HoB\n\toPpA==","X-Gm-Message-State":"AOAM533ozFyfJH+fctCKHw7RMNo6ofQUCjlYUK6L28HA7hNtIc12lD2J\n\tfvqTVTmxk+VOKq+fQJWvqbGS+GSqOUA=","X-Google-Smtp-Source":"ABdhPJw/pozIeZSOT8yCbS363h/NsLN3lU+bCOwuPNkpjwfST4CqDtL3sy50bqgaYO2fKxd/pgoBsg==","X-Received":"by 2002:a05:6512:3156:: with SMTP id\n\ts22mr11056680lfi.140.1596549940332; \n\tTue, 04 Aug 2020 07:05:40 -0700 (PDT)","Date":"Tue, 4 Aug 2020 16:05:38 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20200804140538.GD2566810@oden.dyn.berto.se>","References":"<20200803211733.1037019-1-niklas.soderlund@ragnatech.se>\n\t<20200803211733.1037019-5-niklas.soderlund@ragnatech.se>\n\t<20200804074911.h4552ignajgnej7f@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200804074911.h4552ignajgnej7f@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v6 4/9] libcamera: utils: Add method\n\tto lookup firmware ID in sysfs","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11836,"web_url":"https://patchwork.libcamera.org/comment/11836/","msgid":"<20200804142515.ihoawgouxfmjwvxa@uno.localdomain>","date":"2020-08-04T14:25:15","subject":"Re: [libcamera-devel] [PATCH v6 4/9] libcamera: utils: Add method\n\tto lookup firmware ID in sysfs","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Tue, Aug 04, 2020 at 04:05:38PM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your feedback.\n>\n> On 2020-08-04 09:49:11 +0200, Jacopo Mondi wrote:\n> > Hi Niklas,\n> >\n> > On Mon, Aug 03, 2020 at 11:17:28PM +0200, Niklas Söderlund wrote:\n> > > A devices firmware description is recorded differently in sysfs\n> > > depending on if the devices uses OF or ACPI. Add a helper to abstract\n> >\n> > s/on if/if/\n> > s/devices/device (and that's more likely a system decision, not a\n> > device one)\n> >\n> > > this, allowing users to not care which of the two are used.\n> >\n> > users not to care\n> >\n> > >\n> > > For OF-based systems the ID is the full path of the device in the\n> > > device tree description. For ACPI-based systems the ID is the ACPI\n> > > firmware nodes path. Both ID sources are guaranteed to be unique and\n> > > persistent as long as the firmware of the system is not changed.\n> > >\n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > include/libcamera/internal/utils.h | 2 +\n> > > src/libcamera/utils.cpp | 61 ++++++++++++++++++++++++++++++\n> > > 2 files changed, 63 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h\n> > > index 45cd6f120c51586b..69977340e2593db6 100644\n> > > --- a/include/libcamera/internal/utils.h\n> > > +++ b/include/libcamera/internal/utils.h\n> > > @@ -200,6 +200,8 @@ details::StringSplitter split(const std::string &str, const std::string &delim);\n> > > std::string libcameraBuildPath();\n> > > std::string libcameraSourcePath();\n> > >\n> > > +int tryLookupFirmwareID(const std::string &devPath, std::string *id);\n> > > +\n> > > constexpr unsigned int alignDown(unsigned int value, unsigned int alignment)\n> > > {\n> > > \treturn value / alignment * alignment;\n> > > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> > > index 615df46ac142a2a9..07ebce61f230e5f0 100644\n> > > --- a/src/libcamera/utils.cpp\n> > > +++ b/src/libcamera/utils.cpp\n> > > @@ -9,6 +9,7 @@\n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -444,6 +445,66 @@ std::string libcameraSourcePath()\n> > > \treturn path + \"/\";\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Try to read a device firmware ID from sysfs\n> > > + * \\param[in] devPath Path in sysfs to search\n> > > + * \\param[out] id Location to store ID if found\n> > > + *\n> > > + * The ID recorded in the devices firmware description is recorded differently\n> > > + * in sysfs depending on if the devices uses OF or ACPI. This helper abstracts\n> > > + * this away, allowing callers to not care which of the two are used.\n> >\n> > Same comments as for the commit message\n> >\n> > > + *\n> > > + * For OF-based systems the ID is the full path of the device in the device tree\n> > > + * description. For ACPI-based systems the ID is the ACPI firmware nodes path.\n> > > + * Both ID sources are guaranteed to be unique and persistent as long as the\n> > > + * firmware of the system is not changed.\n> > > + *\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + * \\retval -ENOMEM \\a id is nullptr\n> > > + * \\retval -EINVAL Error when looking up firmware ID\n> > > + * \\retval -ENODEV No firmware ID aviable for device\n> >\n> > available\n> >\n> > > + */\n> > > +int tryLookupFirmwareID(const std::string &devPath, std::string *id)\n> >\n> > I'm going to be picky on names, I know, but why 'try' ? Beause this\n> > function can fail ? Can't the other ones ? I would just 'firwareId()'\n> >\n> > > +{\n> > > +\tstruct stat statbuf;\n> > > +\tstd::string path;\n> > > +\n> > > +\tif (!id)\n> > > +\t\treturn -EINVAL;\n> >\n> > How can you pass a null string pointer ? By calling this method with\n> > nullptr explicitely only, isn't it ? Is this worth this check ?\n>\n> It's cheap and prevents us from crashing, but maybe it can be more\n> helpful as an ASSERT(). I will do so for next version.\n>\n> >\n> > > +\n> > > +\t/* Try to lookup ID as if the system where OF-based. */\n> >\n> > I don't get what this comment means. Maybe 'as is the system was\n> > OF-based' ? Why not 'ID lookup for OF-based systems' ?\n> >\n> > > +\tpath = devPath + \"/of_node\";\n> > > +\tif (stat(path.c_str(), &statbuf) == 0) {\n> >\n> > Can stat fail for other reasons and not just because the file is not\n> > there ? Should be check the return value and propagate errors which\n> > are not caused by the file not being there ? Looking at the stat man\n> > page it's not trivial though as I see both ENOENT and ENODIR could be\n> > returned..\n>\n> I thought about that but then I thought what would the caller do\n> different if it knew the file existed but for some reason it can't be\n> read? This is nothing we can recover from and we will be unable to\n> retrieve the ID in any case. I see lot's of work for no gain doing that.\n>\n\nIn case of potential permission issues, it would get an strerror()\nthat would help debug the problem.\n\nBut I agree it's a minor issue indeed.\n\n> >\n> > > +\t\tchar *ofPath = realpath(path.c_str(), nullptr);\n> > > +\t\tif (!ofPath)\n> > > +\t\t\treturn -EINVAL;\n> > > +\n> > > +\t\t*id = ofPath;\n> > > +\t\tfree(ofPath);\n> > > +\n> > > +\t\tstatic const std::string dropStr = \"/sys/firmware/devicetree/\";\n> > > +\t\tif (id->find(dropStr) == 0)\n> > > +\t\t\tid->erase(0, dropStr.length());\n> > > +\n> > > +\t\treturn 0;\n> > > +\t}\n> > > +\n> > > +\t/* Try to lookup ID as if the system where ACPI-based. */\n> >\n> > Same\n> >\n> > > +\tpath = devPath + \"/firmware_node/path\";\n> > > +\tif (stat(path.c_str(), &statbuf) == 0) {\n> >\n> > And same here as well :)\n> >\n> > > +\t\tstd::ifstream file(path.c_str());\n> > > +\t\tif (!file.is_open())\n> > > +\t\t\treturn -EINVAL;\n> > > +\n> > > +\t\tstd::getline(file, *id);\n> > > +\t\tfile.close();\n> > > +\n> > > +\t\treturn 0;\n> > > +\t}\n> > > +\n> > > +\treturn -ENODEV;\n> > > +}\n> > > +\n> > > /**\n> > > * \\fn alignDown(unsigned int value, unsigned int alignment)\n> > > * \\brief Align \\a value down to \\a alignment\n> > > --\n> > > 2.28.0\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DA4D1BD86F\n\tfor ;\n\tTue, 4 Aug 2020 14:21:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5D0CF6054A;\n\tTue, 4 Aug 2020 16:21:35 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8A76B60545\n\tfor ;\n\tTue, 4 Aug 2020 16:21:34 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id E01231BF207;\n\tTue, 4 Aug 2020 14:21:33 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Tue, 4 Aug 2020 16:25:15 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200804142515.ihoawgouxfmjwvxa@uno.localdomain>","References":"<20200803211733.1037019-1-niklas.soderlund@ragnatech.se>\n\t<20200803211733.1037019-5-niklas.soderlund@ragnatech.se>\n\t<20200804074911.h4552ignajgnej7f@uno.localdomain>\n\t<20200804140538.GD2566810@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200804140538.GD2566810@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH v6 4/9] libcamera: utils: Add method\n\tto lookup firmware ID in sysfs","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]