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

[Jacopo Mondi]

Hi Niklas,

On Tue, Aug 04, 2020 at 04:05:38PM +0200, Niklas Söderlund wrote:
> 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.
>

In case of potential permission issues, it would get an strerror()
that would help debug the problem.

But I agree it's a minor issue indeed.

> >
> > > +		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