[Buildroot] [PATCH v2] package/iqvlinux: new package
Romain Naour
romain.naour at openwide.fr
Fri Oct 9 13:31:34 UTC 2015
Hello Arnout,
Le 05/10/2015 20:22, Arnout Vandecappelle a écrit :
> On 05-10-15 17:48, Romain Naour wrote:
>> The PCI support needs to be checked since this driver is based
>> on it. Otherwise the build fail with:
>> #error "This driver requires PCI support to be available"
>>
>> But this message is concealed by several occurrence of this
>> one:
>> error: implicit declaration of function 'pci_find_bus' [-Werror=implicit-function-declaration]
>>
>> Signed-off-by: Romain Naour <romain.naour at openwide.fr>
>> Cc: Yann E. MORIN <yann.morin.1998 at free.fr>
>> ---
>> v2: - rename the package simply to iqvlinux (ThomasP)
>> - move it to "Hardware Handling" menu (ThomasP)
>> - Cc Yann for the kernel-module infra
>> - Add a check for CONFIG_PCI even if it's redundant with
>> the message from the Makefile.
>> (Do we really need this check ?)
>
> Yes, I think that's a good idea.
>
> Normally in such a case we would add a KCONFIG_ENABLE_OPT in linux.mk (cfr.
> xtables-addons). However, for PCI, it's just stupid to blindly enable PCI if it
> wasn't enabled by your config, because it wouldn't enable any actual root
> complex driver so you wouldn't be able to access the bus anyway.
Indeed, some architecture or CPU doesn't have a PCI bus, that why I didn't use
KCONFIG_ENABLE_OPT here.
>
> So the best alternative is indeed to add a check like you do. Actually, I think
> it would be a good idea if we would do that for xtables-addons and the like as well.
>
> It would be even better if the check was done as a LINUX_POST_CONFIGURE_HOOK,
> so it is caught as early as possible. It would be even betterer if it would be
> done immediately after menuconfig, but that's a bit more tricky (would require
> infra update).
Ok, I'll move the check to LINUX_POST_CONFIGURE_HOOK at first and see later if
we can do something better in the Buildroot infra (in a followup path).
>
>
>> ---
>> package/Config.in | 1 +
>> package/iqvlinux/0001-don-t-use-host-gcc.patch | 32 ++++++++++++++++++++++++++
>> package/iqvlinux/Config.in | 17 ++++++++++++++
>> package/iqvlinux/iqvlinux.hash | 5 ++++
>> package/iqvlinux/iqvlinux.mk | 31 +++++++++++++++++++++++++
>> 5 files changed, 86 insertions(+)
>> create mode 100644 package/iqvlinux/0001-don-t-use-host-gcc.patch
>> create mode 100644 package/iqvlinux/Config.in
>> create mode 100644 package/iqvlinux/iqvlinux.hash
>> create mode 100644 package/iqvlinux/iqvlinux.mk
>>
>> diff --git a/package/Config.in b/package/Config.in
>> index 3794f44..5e2ac80 100644
>> --- a/package/Config.in
>> +++ b/package/Config.in
>> @@ -364,6 +364,7 @@ endif
>> source "package/iostat/Config.in"
>> source "package/ipmitool/Config.in"
>> source "package/ipmiutil/Config.in"
>> + source "package/iqvlinux/Config.in"
>> source "package/irda-utils/Config.in"
>> source "package/iucode-tool/Config.in"
>> source "package/kbd/Config.in"
>> diff --git a/package/iqvlinux/0001-don-t-use-host-gcc.patch b/package/iqvlinux/0001-don-t-use-host-gcc.patch
>> new file mode 100644
>> index 0000000..66820ee
>> --- /dev/null
>> +++ b/package/iqvlinux/0001-don-t-use-host-gcc.patch
>> @@ -0,0 +1,32 @@
>> +From e768f2a9f618d2b5ff0f4be452eaf1dfffdae844 Mon Sep 17 00:00:00 2001
>> +From: Romain Naour <romain.naour at openwide.fr>
>> +Date: Fri, 2 Oct 2015 11:50:08 +0200
>> +Subject: [PATCH] don't use host gcc
>> +
>> +Signed-off-by: Romain Naour <romain.naour at openwide.fr>
>> +---
>> + src/linux/driver/Makefile | 8 --------
>> + 1 file changed, 8 deletions(-)
>> +
>> +diff --git a/src/linux/driver/Makefile b/src/linux/driver/Makefile
>> +index c1dee29..3470d58 100644
>> +--- a/src/linux/driver/Makefile
>> ++++ b/src/linux/driver/Makefile
>> +@@ -100,14 +100,6 @@ ifeq (,$(wildcard $(CONFIG_FILE)))
>> + $(error Linux kernel source not configured - missing autoconf.h)
>> + endif
>> +
>> +-ifneq (,$(findstring egcs-2.91.66, $(shell cat /proc/version)))
>> +- CC := kgcc gcc cc
>> +-else
>> +- CC := gcc cc
>> +-endif
>> +-test_cc = $(shell $(cc) --version > /dev/null 2>&1 && echo $(cc))
>> +-CC := $(foreach cc, $(CC), $(test_cc))
>> +-CC := $(firstword $(CC))
>
> Instead of this patch, can' you just
>
> IQVLINUX_MODULE_MAKE_OPTS += CC=$(TARGET_CC)
>
> ? Except of course if the patch is upstreamable.
I thought that CC was already set by pkg-infra... but in fact it's set by
TARGET_CONFIGURE_OPTS which is not used by kernel-module infra.
It build fine without the patch and with your line added.
>
>> + ifeq (,$(CC))
>> + $(error Compiler not found)
>> + endif
>> +--
>> +1.7.10.4
>> +
>> diff --git a/package/iqvlinux/Config.in b/package/iqvlinux/Config.in
>> new file mode 100644
>> index 0000000..d0394f5
>> --- /dev/null
>> +++ b/package/iqvlinux/Config.in
>> @@ -0,0 +1,17 @@
>> +config BR2_PACKAGE_IQVLINUX
>> + bool "iqvlinux"
>> + depends on BR2_LINUX_KERNEL
>> + help
>> + Intel Ethernet Adapter Debug Driver for Linux (iqvlinux), which
>> + supports kernel versions 2.6.x up through 4.0.x.
>> +
>> + This debug driver supports all Intel's networking Tools based on
>> + the SDK version 2.19.36.0 or higher.
>
> This sounds like mumbojumbo to me. Can't you state something like 'including
> e1000, e1000e, xxx'?
There is no explicit chip reference in the source, I'm using this driver with an
I210 Ethernet chip.
>
>> +
>> + Note: This driver require PCI support is enabled in the kernel
> requires that PCI support is enabled
> or requires PCI support to be enabled
>> + (ie. CONFIG_PCI).
> i.e.
>
Typos fixed.
>> +
>> + http://sourceforge.net/projects/e1000/files/iqvlinux/
>> +
>> +comment "iqvlinux needs a Linux kernel to be built"
>> + depends on !BR2_LINUX_KERNEL
>> diff --git a/package/iqvlinux/iqvlinux.hash b/package/iqvlinux/iqvlinux.hash
>> new file mode 100644
>> index 0000000..ddf57b7
>> --- /dev/null
>> +++ b/package/iqvlinux/iqvlinux.hash
>> @@ -0,0 +1,5 @@
>> +# From http://sourceforge.net/projects/e1000/files/iqvlinux/1.1.5.3/
>> +sha1 bd94416e4364015dbbd78a22e51080bf7ea81fac iqvlinux.tar.gz
>> +md5 fb6a2a4dc122d39070fcb06985c97a05 iqvlinux.tar.gz
>> +# locally computed
>> +sha256 8cb19f3bfe040100a13bb2d05cb2b54f2b259e55cef23f8cc5aa6f2f31e98bec iqvlinux.tar.gz
>> diff --git a/package/iqvlinux/iqvlinux.mk b/package/iqvlinux/iqvlinux.mk
>> new file mode 100644
>> index 0000000..b84cfa3
>> --- /dev/null
>> +++ b/package/iqvlinux/iqvlinux.mk
>> @@ -0,0 +1,31 @@
>> +################################################################################
>> +#
>> +# iqvlinux
>> +#
>> +################################################################################
>> +
>> +IQVLINUX_VERSION = 1.1.5.3
>> +
>> +IQVLINUX_SITE = \
>> + http://sourceforge.net/projects/e1000/files/iqvlinux/$(IQVLINUX_VERSION)
>> +IQVLINUX_SOURCE = iqvlinux.tar.gz
>> +
>> +IQVLINUX_LICENSE = GPLv2 BSD-3c
>
> Is that AND or OR? Or is it some parts one and other parts the other? If it's
> AND, leave it like it is; if it's OR, add an 'or'.
It seems it's an OR, but I haven't checked in each files, at least in cardbus_t.h:
"This file is provided under a dual BSD/GPLv2 license. When using or
redistributing this file, you may do so under either license."
I'll add an 'or'.
>
>> +IQVLINUX_LICENSE_FILES = COPYING
>> +
>> +IQVLINUX_MODULE_MAKE_OPTS = NALDIR=$(@D) \
>> + KSRC=$(LINUX_DIR)
>
> I don't like this way of indenting. All in one line should be OK, no?
Ok, I'll fixes that.
>
>> +
>> +IQVLINUX_MODULE_SUBDIRS = src/linux/driver
>> +
>> +define IQVLINUX_PCI_CHECK
>> + @if ! grep -Fqx 'CONFIG_PCI=y' $(LINUX_DIR)/.config; then \
>> + echo "ERROR: Kernel does not support PCI bus."; \
>
> There should be something like 'Enable CONFIG_PCI in the linux kernel config'.
>
> Also I'd send it to stderr 1>&2
Ok, Also this check should be done in the linux configure step only if this
package is enabled.
>
>> + exit 1; \
>> + fi
>> +endef
>> +
>> +IQVLINUX_PRE_BUILD_HOOKS += IQVLINUX_PCI_CHECK
>
> Fits more as a PRE_CONFIGURE_HOOK IMHO. Or, as I said above, as a
> LINUX_POST_CONFIGURE_HOOK.
Thanks for your review !
Best regards,
Romain
>
>
> Regards,
> Arnout
>
>> +
>> +$(eval $(kernel-module))
>> +$(eval $(generic-package))
>>
>
>
More information about the buildroot
mailing list