[libcamera-devel] [PATCH v6 4/9] libcamera: utils: Add method to lookup firmware ID in sysfs

[Niklas Söderlund]

Hi Jacopo,

Thanks for your feedback.

On 2020-08-04 09:49:11 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Mon, Aug 03, 2020 at 11:17:28PM +0200, Niklas Söderlund wrote:
> > A devices firmware description is recorded differently in sysfs
> > depending on if the devices uses OF or ACPI. Add a helper to abstract
> 
> s/on if/if/
> s/devices/device (and that's more likely a system decision, not a
> device one)
> 
> > this, allowing users to not care which of the two are used.
> 
> users not to care
> 
> >
> > For OF-based systems the ID is the full path of the device in the
> > device tree description. For ACPI-based systems the ID is the ACPI
> > firmware nodes path. Both ID sources are guaranteed to be unique and
> > persistent as long as the firmware of the system is not changed.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  include/libcamera/internal/utils.h |  2 +
> >  src/libcamera/utils.cpp            | 61 ++++++++++++++++++++++++++++++
> >  2 files changed, 63 insertions(+)
> >
> > diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h
> > index 45cd6f120c51586b..69977340e2593db6 100644
> > --- a/include/libcamera/internal/utils.h
> > +++ b/include/libcamera/internal/utils.h
> > @@ -200,6 +200,8 @@ details::StringSplitter split(const std::string &str, const std::string &delim);
> >  std::string libcameraBuildPath();
> >  std::string libcameraSourcePath();
> >
> > +int tryLookupFirmwareID(const std::string &devPath, std::string *id);
> > +
> >  constexpr unsigned int alignDown(unsigned int value, unsigned int alignment)
> >  {
> >  	return value / alignment * alignment;
> > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> > index 615df46ac142a2a9..07ebce61f230e5f0 100644
> > --- a/src/libcamera/utils.cpp
> > +++ b/src/libcamera/utils.cpp
> > @@ -9,6 +9,7 @@
> >
> >  #include <dlfcn.h>
> >  #include <elf.h>
> > +#include <fstream>
> >  #include <iomanip>
> >  #include <limits.h>
> >  #include <link.h>
> > @@ -444,6 +445,66 @@ std::string libcameraSourcePath()
> >  	return path + "/";
> >  }
> >
> > +/**
> > + * \brief Try to read a device firmware ID from sysfs
> > + * \param[in] devPath Path in sysfs to search
> > + * \param[out] id Location to store ID if found
> > + *
> > + * The ID recorded in the devices firmware description is recorded differently
> > + * in sysfs depending on if the devices uses OF or ACPI. This helper abstracts
> > + * this away, allowing callers to not care which of the two are used.
> 
> Same comments as for the commit message
> 
> > + *
> > + * For OF-based systems the ID is the full path of the device in the device tree
> > + * description. For ACPI-based systems the ID is the ACPI firmware nodes path.
> > + * Both ID sources are guaranteed to be unique and persistent as long as the
> > + * firmware of the system is not changed.
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + * \retval -ENOMEM \a id is nullptr
> > + * \retval -EINVAL Error when looking up firmware ID
> > + * \retval -ENODEV No firmware ID aviable for device
> 
> available
> 
> > + */
> > +int tryLookupFirmwareID(const std::string &devPath, std::string *id)
> 
> I'm going to be picky on names, I know, but why 'try' ? Beause this
> function can fail ? Can't the other ones ? I would just 'firwareId()'
> 
> > +{
> > +	struct stat statbuf;
> > +	std::string path;
> > +
> > +	if (!id)
> > +		return -EINVAL;
> 
> How can you pass a null string pointer ? By calling this method with
> nullptr explicitely only, isn't it ? Is this worth this check ?

It's cheap and prevents us from crashing, but maybe it can be more 
helpful as an ASSERT(). I will do so for next version.

> 
> > +
> > +	/* Try to lookup ID as if the system where OF-based. */
> 
> I don't get what this comment means. Maybe 'as is the system was
> OF-based' ? Why not 'ID lookup for OF-based systems' ?
> 
> > +	path = devPath + "/of_node";
> > +	if (stat(path.c_str(), &statbuf) == 0) {
> 
> Can stat fail for other reasons and not just because the file is not
> there ? Should be check the return value and propagate errors which
> are not caused by the file not being there ? Looking at the stat man
> page it's not trivial though as I see both ENOENT and ENODIR could be
> returned..

I thought about that but then I thought what would the caller do 
different if it knew the file existed but for some reason it can't be 
read? This is nothing we can recover from and we will be unable to 
retrieve the ID in any case. I see lot's of work for no gain doing that.

> 
> > +		char *ofPath = realpath(path.c_str(), nullptr);
> > +		if (!ofPath)
> > +			return -EINVAL;
> > +
> > +		*id = ofPath;
> > +		free(ofPath);
> > +
> > +		static const std::string dropStr = "/sys/firmware/devicetree/";
> > +		if (id->find(dropStr) == 0)
> > +			id->erase(0, dropStr.length());
> > +
> > +		return 0;
> > +	}
> > +
> > +	/* Try to lookup ID as if the system where ACPI-based. */
> 
> Same
> 
> > +	path = devPath + "/firmware_node/path";
> > +	if (stat(path.c_str(), &statbuf) == 0) {
> 
> And same here as well :)
> 
> > +		std::ifstream file(path.c_str());
> > +		if (!file.is_open())
> > +			return -EINVAL;
> > +
> > +		std::getline(file, *id);
> > +		file.close();
> > +
> > +		return 0;
> > +	}
> > +
> > +	return -ENODEV;
> > +}
> > +
> >  /**
> >   * \fn alignDown(unsigned int value, unsigned int alignment)
> >   * \brief Align \a value down to \a alignment
> > --
> > 2.28.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund