diff mbox

package/pflash: new package

Message ID 1469426487-19558-1-git-send-email-joel@jms.id.au
State Superseded
Headers show

Commit Message

Joel Stanley July 25, 2016, 6:01 a.m. UTC 
pflash is a tool for reading and writing to the PNOR on OpenPower
servers, for firmware upgrades, and extracting diagnostic information.

This includes a fix so that pflash can build with musl, which has been submitted
upstream[1].

[1] http://patchwork.ozlabs.org/patch/652168/

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 package/Config.in                                  |  1 +
 ...-pflash-use-atexit-for-musl-compatibility.patch | 42 ++++++++++++++++++++++
 package/pflash/Config.in                           |  6 ++++
 package/pflash/pflash.mk                           | 32 +++++++++++++++++
 4 files changed, 81 insertions(+)
 create mode 100644 package/pflash/0001-pflash-use-atexit-for-musl-compatibility.patch
 create mode 100644 package/pflash/Config.in
 create mode 100644 package/pflash/pflash.mk

Comments

Bernd Kuhls July 25, 2016, 6:14 a.m. UTC | #1 
Hi,

Am Mon, 25 Jul 2016 15:31:27 +0930 schrieb Joel Stanley:

> diff --git a/package/pflash/Config.in b/package/pflash/Config.in
> new file mode 100644
> index 000000000000..315989724088
> --- /dev/null
> +++ b/package/pflash/Config.in
> @@ -0,0 +1,6 @@
> +config BR2_PACKAGE_PFLASH
> +	bool "pflash"
> +	depends on BR2_arm || BR2_powerpc64 || BR2_powerpc64le || BR2_x86_64
> +	help
> +	  PNOR Flash reading and writing tool for OpenPower servers. Used for
> +	  firmware upgrades and extracting diagnostic infromation from flash.

please add 

         https://github.com/open-power/skiboot

to the help text.

> diff --git a/package/pflash/pflash.mk b/package/pflash/pflash.mk
> new file mode 100644
> index 000000000000..ab19ad699866
> --- /dev/null
> +++ b/package/pflash/pflash.mk
> @@ -0,0 +1,32 @@
> +################################################################################
> +#
> +# pflash
> +#
> +################################################################################
> +
> +PFLASH_VERSION = skiboot-5.2.4
> +

no newline here.

> +PFLASH_SITE = $(call github,open-power,skiboot,$(PFLASH_VERSION))
> +PFLASH_INSTALL_STAGING = YES

Do other packages need header files from pflash? If not, it is
not necessary to install the package to staging.

Regards, Bernd
Joel Stanley July 25, 2016, 6:20 a.m. UTC | #2 
On Mon, Jul 25, 2016 at 3:44 PM, Bernd Kuhls <bernd.kuhls@t-online.de> wrote:
> Hi,
>
> Am Mon, 25 Jul 2016 15:31:27 +0930 schrieb Joel Stanley:
>
>> diff --git a/package/pflash/Config.in b/package/pflash/Config.in
>> new file mode 100644
>> index 000000000000..315989724088
>> --- /dev/null
>> +++ b/package/pflash/Config.in
>> @@ -0,0 +1,6 @@
>> +config BR2_PACKAGE_PFLASH
>> +     bool "pflash"
>> +     depends on BR2_arm || BR2_powerpc64 || BR2_powerpc64le || BR2_x86_64
>> +     help
>> +       PNOR Flash reading and writing tool for OpenPower servers. Used for
>> +       firmware upgrades and extracting diagnostic infromation from flash.
>
> please add
>
>          https://github.com/open-power/skiboot
>
> to the help text.

Good idea.

>
>> diff --git a/package/pflash/pflash.mk b/package/pflash/pflash.mk
>> new file mode 100644
>> index 000000000000..ab19ad699866
>> --- /dev/null
>> +++ b/package/pflash/pflash.mk
>> @@ -0,0 +1,32 @@
>> +################################################################################
>> +#
>> +# pflash
>> +#
>> +################################################################################
>> +
>> +PFLASH_VERSION = skiboot-5.2.4
>> +
>
> no newline here.
>
>> +PFLASH_SITE = $(call github,open-power,skiboot,$(PFLASH_VERSION))
>> +PFLASH_INSTALL_STAGING = YES
>
> Do other packages need header files from pflash? If not, it is
> not necessary to install the package to staging.

Thanks for the explanation. We don't install any headers so I can drop this.

Thanks for the review!

Cheers,

Joel

> Regards, Bernd
>
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
Thomas Petazzoni July 25, 2016, 10:07 p.m. UTC | #3 
Hello,

On Mon, 25 Jul 2016 15:31:27 +0930, Joel Stanley wrote:

> diff --git a/package/pflash/Config.in b/package/pflash/Config.in
> new file mode 100644
> index 000000000000..315989724088
> --- /dev/null
> +++ b/package/pflash/Config.in
> @@ -0,0 +1,6 @@
> +config BR2_PACKAGE_PFLASH
> +	bool "pflash"
> +	depends on BR2_arm || BR2_powerpc64 || BR2_powerpc64le || BR2_x86_64

Is there something that makes it inherently usable only on those
architectures?

> diff --git a/package/pflash/pflash.mk b/package/pflash/pflash.mk
> new file mode 100644
> index 000000000000..ab19ad699866
> --- /dev/null
> +++ b/package/pflash/pflash.mk
> @@ -0,0 +1,32 @@
> +################################################################################
> +#
> +# pflash
> +#
> +################################################################################
> +
> +PFLASH_VERSION = skiboot-5.2.4
> +
> +PFLASH_SITE = $(call github,open-power,skiboot,$(PFLASH_VERSION))

So skiboot is a much much larger repository, with lots of other things,
and you're building just the external/pflash subdirectory of it.

Are you sure we'll never want to have a package for other things in the
skiboot repository?

> +PFLASH_INSTALL_STAGING = YES

You're not specifying any PFLASH_INSTALL_STAGING_CMDS, so this is
useless.

> +PFLASH_LICENSE = Apache 2.0
> +PFLASH_LICENSE_FILE = LICENCE
> +
> +PFLASH_MAKE_OPTS += CROSS_COMPILE="$(TARGET_CROSS)" \
> +		    PFLASH_VERSION=$(PFLASH_VERSION) \
> +		    DESTDIR=$(TARGET_DIR) \

DESTDIR should only be passed at install time.

> +		    -C $(@D)/external/pflash \

Unneeded trailing \

Also, we prefer the -C option and its argument to be part of the build
and install commands themselves.

Thanks!

Thomas
Joel Stanley July 26, 2016, 2:27 a.m. UTC | #4 
On Tue, Jul 26, 2016 at 7:37 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Hello,
>
> On Mon, 25 Jul 2016 15:31:27 +0930, Joel Stanley wrote:
>
>> diff --git a/package/pflash/Config.in b/package/pflash/Config.in
>> new file mode 100644
>> index 000000000000..315989724088
>> --- /dev/null
>> +++ b/package/pflash/Config.in
>> @@ -0,0 +1,6 @@
>> +config BR2_PACKAGE_PFLASH
>> +     bool "pflash"
>> +     depends on BR2_arm || BR2_powerpc64 || BR2_powerpc64le || BR2_x86_64
>
> Is there something that makes it inherently usable only on those
> architectures?

Yeah, the 'libflash' library selects different backends depending on it's

>
>> diff --git a/package/pflash/pflash.mk b/package/pflash/pflash.mk
>> new file mode 100644
>> index 000000000000..ab19ad699866
>> --- /dev/null
>> +++ b/package/pflash/pflash.mk
>> @@ -0,0 +1,32 @@
>> +################################################################################
>> +#
>> +# pflash
>> +#
>> +################################################################################
>> +
>> +PFLASH_VERSION = skiboot-5.2.4
>> +
>> +PFLASH_SITE = $(call github,open-power,skiboot,$(PFLASH_VERSION))
>
> So skiboot is a much much larger repository, with lots of other things,
> and you're building just the external/pflash subdirectory of it.
>
> Are you sure we'll never want to have a package for other things in the
> skiboot repository?

We might want to package other tools in the future.

How should I change the packaging of pflash?

>
>> +PFLASH_INSTALL_STAGING = YES
>
> You're not specifying any PFLASH_INSTALL_STAGING_CMDS, so this is
> useless.

Thanks for clearing that up. I've fixed it in v2.

>
>> +PFLASH_LICENSE = Apache 2.0
>> +PFLASH_LICENSE_FILE = LICENCE
>> +
>> +PFLASH_MAKE_OPTS += CROSS_COMPILE="$(TARGET_CROSS)" \
>> +                 PFLASH_VERSION=$(PFLASH_VERSION) \
>> +                 DESTDIR=$(TARGET_DIR) \
>
> DESTDIR should only be passed at install time.

Okay.

>
>> +                 -C $(@D)/external/pflash \
>
> Unneeded trailing \

Okay.

>
> Also, we prefer the -C option and its argument to be part of the build
> and install commands themselves.

Even though we would be replicating them for every invocation of make?
I think it's better placed in a variable so changes can be made in one
place.

Cheers,

Joel
Thomas Petazzoni July 26, 2016, 7:32 a.m. UTC | #5 
Hello,

On Tue, 26 Jul 2016 11:57:17 +0930, Joel Stanley wrote:

> >> diff --git a/package/pflash/Config.in b/package/pflash/Config.in
> >> new file mode 100644
> >> index 000000000000..315989724088
> >> --- /dev/null
> >> +++ b/package/pflash/Config.in
> >> @@ -0,0 +1,6 @@
> >> +config BR2_PACKAGE_PFLASH
> >> +     bool "pflash"
> >> +     depends on BR2_arm || BR2_powerpc64 || BR2_powerpc64le || BR2_x86_64  
> >
> > Is there something that makes it inherently usable only on those
> > architectures?  
> 
> Yeah, the 'libflash' library selects different backends depending on it's

OK. But then it makes sense only for pflash itself, and not skitool as
the whole I suppose?

> > So skiboot is a much much larger repository, with lots of other things,
> > and you're building just the external/pflash subdirectory of it.
> >
> > Are you sure we'll never want to have a package for other things in the
> > skiboot repository?  
> 
> We might want to package other tools in the future.
> 
> How should I change the packaging of pflash?

A top-level BR2_PACKAGE_SKITOOL option, with a package called skitool.
And then a sub-option BR2_PACKAGE_SKITOOL_PFLASH, which enables the
build/installation of pflash.

BR2_PACKAGE_SKITOOL can "select" BR2_PACKAGE_SKITOOL_PFLASH to make
sure the package at least installs one thing.

> > Also, we prefer the -C option and its argument to be part of the build
> > and install commands themselves.  
> 
> Even though we would be replicating them for every invocation of make?
> I think it's better placed in a variable so changes can be made in one
> place.

We generally repeat the -C $(@D) in the different invocations, yes. Not
sure it's ideal, but that's how we typically do it in most packages.

Thomas
diff mbox

Patch

diff --git a/package/Config.in b/package/Config.in
index 814141f18784..9f873c31fa58 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -426,6 +426,7 @@  endmenu
 	source "package/owl-linux/Config.in"
 	source "package/parted/Config.in"
 	source "package/pciutils/Config.in"
+	source "package/pflash/Config.in"
 	source "package/picocom/Config.in"
 	source "package/pifmrds/Config.in"
 	source "package/powertop/Config.in"
diff --git a/package/pflash/0001-pflash-use-atexit-for-musl-compatibility.patch b/package/pflash/0001-pflash-use-atexit-for-musl-compatibility.patch
new file mode 100644
index 000000000000..f8ac58eba329
--- /dev/null
+++ b/package/pflash/0001-pflash-use-atexit-for-musl-compatibility.patch
@@ -0,0 +1,42 @@ 
+From 7f6b017b8dc3b5f276b30353e0a7186e4e433e7a Mon Sep 17 00:00:00 2001
+From: Joel Stanley <joel@jms.id.au>
+Date: Mon, 25 Jul 2016 13:24:37 +0930
+Subject: [PATCH] pflash: use atexit for musl compatibility
+To: skiboot@lists.ozlabs.org
+
+I accidentally built myself a cross-toolchain with the musl libc. It does
+not support on_exit which we use to clean up in pflash.
+
+Instead use atexit with is supported by both uclibc, musl and glibc.
+
+Signed-off-by: Joel Stanley <joel@jms.id.au>
+---
+ external/pflash/pflash.c | 5 ++---
+ 1 file changed, 2 insertions(+), 3 deletions(-)
+
+diff --git a/external/pflash/pflash.c b/external/pflash/pflash.c
+index c124356fd3d9..27000469052e 100644
+--- a/external/pflash/pflash.c
++++ b/external/pflash/pflash.c
+@@ -509,7 +509,7 @@ static void print_help(const char *pname)
+ 	printf("\t\tThis message.\n\n");
+ }
+ 
+-void exiting(int d, void *p)
++void exiting(void)
+ {
+ 	if (need_relock)
+ 		arch_flash_set_wrprotect(bl, 1);
+@@ -775,8 +775,7 @@ int main(int argc, char *argv[])
+ 		exit(1);
+ 	}
+ 
+-	on_exit(exiting, NULL);
+-
++	atexit(exiting);
+ 
+ 	rc = blocklevel_get_info(bl, &fl_name,
+ 			    &fl_total_size, &fl_erase_granule);
+-- 
+2.8.1
+
diff --git a/package/pflash/Config.in b/package/pflash/Config.in
new file mode 100644
index 000000000000..315989724088
--- /dev/null
+++ b/package/pflash/Config.in
@@ -0,0 +1,6 @@ 
+config BR2_PACKAGE_PFLASH
+	bool "pflash"
+	depends on BR2_arm || BR2_powerpc64 || BR2_powerpc64le || BR2_x86_64
+	help
+	  PNOR Flash reading and writing tool for OpenPower servers. Used for
+	  firmware upgrades and extracting diagnostic infromation from flash.
diff --git a/package/pflash/pflash.mk b/package/pflash/pflash.mk
new file mode 100644
index 000000000000..ab19ad699866
--- /dev/null
+++ b/package/pflash/pflash.mk
@@ -0,0 +1,32 @@ 
+################################################################################
+#
+# pflash
+#
+################################################################################
+
+PFLASH_VERSION = skiboot-5.2.4
+
+PFLASH_SITE = $(call github,open-power,skiboot,$(PFLASH_VERSION))
+PFLASH_INSTALL_STAGING = YES
+PFLASH_LICENSE = Apache 2.0
+PFLASH_LICENSE_FILE = LICENCE
+
+PFLASH_MAKE_OPTS += CROSS_COMPILE="$(TARGET_CROSS)" \
+		    PFLASH_VERSION=$(PFLASH_VERSION) \
+		    DESTDIR=$(TARGET_DIR) \
+		    -C $(@D)/external/pflash \
+
+# A makefile bug causes recent versions of pflash to fail setting CC and LD
+# based on CROSS_COMPILE. Set CC and LD to remain compatible with those
+# versions.
+PFLASH_MAKE_OPTS += CC=$(TARGET_CC) LD=$(TARGET_LD)
+
+define PFLASH_BUILD_CMDS
+	$(TARGET_MAKE_ENV) $(MAKE) $(PFLASH_MAKE_OPTS) all
+endef
+
+define PFLASH_INSTALL_TARGET_CMDS
+	$(TARGET_MAKE_ENV) $(MAKE) $(PFLASH_MAKE_OPTS) install
+endef
+
+$(eval $(generic-package))