[Buildroot] [PATCH] pcm-tools: new package

Carlos Santos casantos at datacom.ind.br
Tue Apr 3 02:02:41 UTC 2018


> From: "Thomas Petazzoni" <thomas.petazzoni at bootlin.com>
> To: "Carlos Santos" <casantos at datacom.ind.br>
> Cc: "buildroot" <buildroot at buildroot.org>
> Sent: Sunday, April 1, 2018 11:16:54 AM
> Subject: Re: [Buildroot] [PATCH] pcm-tools: new package

> Hello,
> 
> Here are some additional comments, on top of the ones made by Romain.
> 
> On Tue, 23 Jan 2018 12:03:24 -0200, Carlos Santos wrote:
> 
>> diff --git
>> a/package/pcm-tools/0001-Appease-GCC-snprintf-outut-size-larger-than-destinat.patch
>> b/package/pcm-tools/0001-Appease-GCC-snprintf-outut-size-larger-than-destinat.patch
>> new file mode 100644
>> index 0000000000..5d77ebfcb6
>> --- /dev/null
>> +++
>> b/package/pcm-tools/0001-Appease-GCC-snprintf-outut-size-larger-than-destinat.patch
>> @@ -0,0 +1,40 @@
>> +From d07d6602ac2d4c90f7e448bdc5b780e8aa9faf7c Mon Sep 17 00:00:00 2001
>> +From: Carlos Santos <casantos at datacom.ind.br>
>> +Date: Sun, 21 Jan 2018 23:44:46 -0200
>> +Subject: [PATCH 1/3] Appease GCC: snprintf outut size larger than destination
>> + size
>> +
>> +GCC ignores that PCI function numbers range from 0 to 7, generations are
>> +up to 4 (5, by 2019 Q2) and link speeds are up to 16x. So it warns that
>> +snprintf output may be truncated in build_pci_header (in pcm-iio.cpp).
>> +
>> +Solve this by increasing the buffer sizes, since the additional three
>> +bytes are harmless. We could satisfy GCC by masking p.bdf.funcno and
>> +p.link_speed to 0x07, thus limitting the output to one decimal digit,
>> +and p.link_width to 0x1f (two decimal digits). This approach, however,
>> +would hide invalid arguments, preventing the user from noticing the
>> +error.
>> +
>> +Signed-off-by: Carlos Santos <casantos at datacom.ind.br>
> 
> Have the patches been submitted upstream ?

The first patch was already accepted upstream.

>> diff --git a/package/pcm-tools/0002-Look-for-pci.ids-at-usr-share-hwdata.patch
>> b/package/pcm-tools/0002-Look-for-pci.ids-at-usr-share-hwdata.patch
>> new file mode 100644
>> index 0000000000..4c04589867
>> --- /dev/null
>> +++ b/package/pcm-tools/0002-Look-for-pci.ids-at-usr-share-hwdata.patch
>> @@ -0,0 +1,34 @@
>> +From 9213e92ec5aaee0e319b94f9501458e44e7873ba Mon Sep 17 00:00:00 2001
>> +From: Carlos Santos <casantos at datacom.ind.br>
>> +Date: Tue, 23 Jan 2018 08:27:49 -0200
>> +Subject: [PATCH 2/3] Look for pci.ids at /usr/share/hwdata
>> +
>> +For Buildroot, on which pci.ids is provided by the "hwdata" package.
> 
> It's not really nice to have Buildroot specific patches. Can you find a
> solution that is acceptable upstream ? Config option perhaps ?

They are necessary to deal with specific path where files are installed
in Buildroot builds. Anyway, I will try to find a way to make them more
generic.

>> diff --git a/package/pcm-tools/0003-Look-for-pcm-core-at-usr-bin.patch
>> b/package/pcm-tools/0003-Look-for-pcm-core-at-usr-bin.patch
>> new file mode 100644
>> index 0000000000..92dc06c197
>> --- /dev/null
>> +++ b/package/pcm-tools/0003-Look-for-pcm-core-at-usr-bin.patch
>> @@ -0,0 +1,28 @@
>> +From bef36c901e7f2cca3c21ee057b75f73065830774 Mon Sep 17 00:00:00 2001
>> +From: Carlos Santos <casantos at datacom.ind.br>
>> +Date: Tue, 23 Jan 2018 10:56:55 -0200
>> +Subject: [PATCH 3/3] Look for pcm-core at /usr/bin
>> +
>> +For Buildroot, on which pcm-core.x ins installed as /usr/bin/pcm-core.
>> +
>> +Signed-off-by: Carlos Santos <casantos at datacom.ind.br>
>> +---
>> + pmu-query.py | 2 +-
>> + 1 file changed, 1 insertion(+), 1 deletion(-)
>> +
>> +diff --git a/pmu-query.py b/pmu-query.py
>> +index 4c596c7..71aa1b1 100755
>> +--- a/pmu-query.py
>> ++++ b/pmu-query.py
>> +@@ -43,7 +43,7 @@ if filename == None:
>> +     elif platform.system() == 'Windows':
>> +         p = subprocess.Popen(['pcm-core.exe
>> -c'],stdout=subprocess.PIPE,shell=True)
>> +     else:
>> +-        p = subprocess.Popen(['./pcm-core.x
>> -c'],stdout=subprocess.PIPE,shell=True)
>> ++        p = subprocess.Popen(['/usr/bin/pcm-core
>> -c'],stdout=subprocess.PIPE,shell=True)
> 
> Same here, can we find an upstreamable solution, ideally ?
> 
>> +if BR2_PACKAGE_PCM_TOOLS
>> +
>> +# The pmu-query script is not compatible with Python 3
>> +config BR2_PACKAGE_PCM_TOOLS_PMU_QUERY_REQS
>> +	bool
>> +	default y
>> +	depends on BR2_PACKAGE_PYTHON && BR2_PACKAGE_PYTHON_SSL &&
>> BR2_PACKAGE_PYTHON_HASHLIB # urllib2
>> +	depends on BR2_PACKAGE_CA_CERTIFICATES # https
> 
> There's really no need for a separate
> BR2_PACKAGE_PCM_TOOLS_PMU_QUERY_REQS option, just put the
> selects/depends on inside BR2_PACKAGE_PCM_TOOLS_PMU_QUERY.
> 
> As Romain said, only "depends on BR2_PACKAGE_PYTHON" should remain a
> "depends on", everything else should be a "select".

OK

>> +
>> +config BR2_PACKAGE_PCM_TOOLS_PMU_QUERY
>> +	bool "install the pmu-query script"
>> +	default y
>> +	depends on BR2_PACKAGE_PCM_TOOLS_PMU_QUERY_REQS
>> +
>> +comment "pmu-query needs Python 2.x w/ ssl + hashlib modules, ca-certificates"
>> +	depends on !BR2_PACKAGE_PCM_TOOLS_PMU_QUERY_REQS
> 
> Which simplifies that quite a lot.
> 
> 
>> +PCM_TOOLS_VERSION = 201710
>> +PCM_TOOLS_SITE = $(call github,opcm,pcm,$(PCM_TOOLS_VERSION))
>> +PCM_TOOLS_LICENSE = BSD-3-Clause
>> +PCM_TOOLS_LICENSE_FILES = LICENSE
>> +
>> +PCM_TOOLS_EXE_FILES = \
>> +	pcm-core pcm-iio pcm-lspci pcm-memory pcm-msr pcm-numa \
>> +	pcm-pcie pcm-power pcm-sensor pcm-tsx pcm
>> +
>> +define PCM_TOOLS_BUILD_CMDS
>> +	$(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) -C $(@D) \
>> +		UNAME=Linux HOST=_LINUX CC="$(TARGET_CC)"
> 
> TARGET_CONFIGURE_OPTS already contains CC="$(TARGET_CC)"
> 
>> +define PCM_TOOLS_INSTALL_TARGET_CMDS
>> +	$(Q) set -x -e; \
>> +	for f in $(PCM_TOOLS_EXE_FILES); do \
>> +		$(INSTALL) -D -m 755 $(@D)/$$f.x $(TARGET_DIR)/usr/bin/$$f; \
>> +	done;
>> +	if test "$(BR2_PACKAGE_PCM_TOOLS_PMU_QUERY)" = "y"; then \
>> +		$(INSTALL) -D -m 755 $(@D)/pmu-query.py $(TARGET_DIR)/usr/bin/pmu-query; \
>> +	fi
>> +endef
> 
> That's not a very Buildroot-ish way of doing this. Here is a more
> Buildroot-ish solution:
> 
> ifeq ($(BR2_PACKAGE_PCM_TOOLS_PMU_QUERY),y)
> define PCM_TOOLS_INSTALL_PMU_QUERY
>	$(INSTALL) -D -m 755 $(@D)/pmu-query.py $(TARGET_DIR)/usr/bin/pmu-query
> endef
> endif
> 
> define PCM_TOOLS_INSTALL_TARGET_CMDS
>	$(foreach f,$(PCM_TOOLS_EXE_FILES),\
>		$(INSTALL) -D -m 755 $(@D)/$(f).x $(TARGET_DIR)/usr/bin/$(f)
>	)
>	$(PCM_TOOLS_INSTALL_PMU_QUERY)
> endef
> 
> Could you rework your patch to take into Romain's comments and the
> above comments ?

I will submit an updated version of the series. Thanks for your review.

-- 
Carlos Santos (Casantos) - DATACOM, P&D
“The greatest triumph that modern PR can offer is the transcendent 
success of having your words and actions judged by your reputation, 
rather than the other way about.” — Christopher Hitchens


More information about the buildroot mailing list