Change in flashrom[master]: linux_spi: add missing include

[David Hendricks (Code Review)]

David Hendricks has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/35830 )

Change subject: linux_spi: add missing include
......................................................................

linux_spi: add missing include

Some defines (e.g. _IOC_SIZEBITS) are defined in linux/ioctl.h,
so it must be included before it is used, by SPI_IOC_MESSAGE
from linux/spi/spidev.h

Fixes build errors with the musl C library, as seen in these
Buildroot autobuilder failures:

http://autobuild.buildroot.org/results/2a3/2a3744007c630c267575a638ebcd83a4b...
    http://autobuild.buildroot.org/results/3de/3de936d9be79e151e66af15193084d82a...

Signed-off-by: "Yann E. MORIN" 
[Retrieved from:
https: //git.buildroot.net/buildroot/tree/package/flashrom/0001-spi.patch]
Signed-off-by: Fabrice Fontaine 
Change-Id: Ieab60f59bc63aca0dc4867f31699dab4167da05b
---
M linux_spi.c
1 file changed, 1 insertion(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/30/35830/1

diff --git a/linux_spi.c b/linux_spi.c
index de09e60..30b6288 100644
--- a/linux_spi.c
+++ b/linux_spi.c
@@ -26,6 +26,7 @@
 #include <unistd.h>
 #include <sys/ioctl.h>
 #include <linux/types.h>
+#include <linux/ioctl.h>
 #include <linux/spi/spidev.h>
 #include <linux/ioctl.h>
 #include "flash.h"

[David Hendricks (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/35830

to look at the new patch set (#2).

Change subject: linux_spi: add missing include
......................................................................

linux_spi: add missing include

Some defines (e.g. _IOC_SIZEBITS) are defined in linux/ioctl.h,
so it must be included before it is used, by SPI_IOC_MESSAGE
from linux/spi/spidev.h

Fixes build errors with the musl C library.

Signed-off-by: "Yann E. MORIN" 
Retrieved from:
https://git.buildroot.net/buildroot/tree/package/flashrom/0001-spi.patch
Signed-off-by: Fabrice Fontaine 
Change-Id: Ieab60f59bc63aca0dc4867f31699dab4167da05b
---
M linux_spi.c
1 file changed, 1 insertion(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/30/35830/2

[David Hendricks (Code Review)]

David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35830 )

Change subject: linux_spi: add missing include
......................................................................

Patch Set 2:

(2 comments)

From https://github.com/flashrom/flashrom/pull/90

PS2 edits the commit message to remove some long URLs that were in the original.

https://review.coreboot.org/c/flashrom/+/35830/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/35830/1//COMMIT_MSG@17 
PS1, Line 17:     http://autobuild.buildroot.org/results/3de/3de936d9be79e151e66af15193084d82a...
Removed these long lines

https://review.coreboot.org/c/flashrom/+/35830/1//COMMIT_MSG@21 
PS1, Line 21: https: //git.buildroot.net/buildroot/tree/package/flashrom/0001-spi.patch]
Removed the brackets so that the URL fits on one line.

However, since that patch will be made obsolete, perhaps the URL should point to the directory rather than the file itself.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35830 )

Change subject: linux_spi: add missing include
......................................................................

Patch Set 3: Code-Review+2

(3 comments)

I'm ok with the change, just the reasoning seems completely
wrong. It also deserves a comment above the includes that
we keep the order because we expect broken header files.

https://review.coreboot.org/c/flashrom/+/35830/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/35830/3//COMMIT_MSG@7 
PS3, Line 7: missing
This looks more like a workaround for broken kernel header files
to me.

https://review.coreboot.org/c/flashrom/+/35830/3//COMMIT_MSG@11 
PS3, Line 11: from linux/spi/spidev.h
Really walks like a bug, talks like a bug in `linux/spi/spidev.h`.

https://review.coreboot.org/c/flashrom/+/35830/3//COMMIT_MSG@13 
PS3, Line 13: Fixes build errors with the musl C library.
IIRC, Alpine Linux uses musl libc and that always built fine...

Change in flashrom[master]: linux_spi: reorder includes for linux <4.14

[David Hendricks (Code Review)]

Hello Angel Pons, build bot (Jenkins), Nico Huber,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/35830

to look at the new patch set (#4).

Change subject: linux_spi: reorder includes for linux <4.14
......................................................................

linux_spi: reorder includes for linux <4.14

This works around a missing header in spidev.h present in older
versions of Linux. Patch is ported from:
https://git.buildroot.net/buildroot/tree/package/flashrom/0001-spi.patch

Signed-off-by: "Yann E. MORIN" 
Signed-off-by: Fabrice Fontaine 
Signed-off-by: David Hendricks 
Change-Id: Ieab60f59bc63aca0dc4867f31699dab4167da05b
---
M linux_spi.c
1 file changed, 7 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/30/35830/4

Change in flashrom[master]: linux_spi: reorder includes for linux <4.14

[David Hendricks (Code Review)]

David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35830 )

Change subject: linux_spi: reorder includes for linux <4.14
......................................................................

Patch Set 3:

(1 comment)
> Patch Set 3: Code-Review+2
>
> (3 comments)
>
> I'm ok with the change, just the reasoning seems completely
> wrong. It also deserves a comment above the includes that
> we keep the order because we expect broken header files.

Yeah, your comment prompted me to look into this further and the commit message seems misleading.

I can go either way with this patch... On one hand, it's obsolete with newer versions of Linux. On the other hand, flashrom often runs on older systems.

I modified the commit message to be more accurate, at least, and added a comment in linux_spi.c about the reason for the ordering. I think we have some extra includes in there, but figure we should eventually run include-what-you-use or something to clean that all up at once.

https://review.coreboot.org/c/flashrom/+/35830/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/35830/3//COMMIT_MSG@7 
PS3, Line 7: missing
> This looks more like a workaround for broken kernel header files […]

Indeed, it's a bug in Linux that was fixed in 4.14:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/in...

Change in flashrom[master]: linux_spi: reorder includes for linux <4.14

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/35830 )

Change subject: linux_spi: reorder includes for linux <4.14
......................................................................

Patch Set 4: Code-Review+2

(3 comments)

https://review.coreboot.org/c/flashrom/+/35830/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/35830/1//COMMIT_MSG@21 
PS1, Line 21: https: //git.buildroot.net/buildroot/tree/package/flashrom/0001-spi.patch]
> Removed the brackets so that the URL fits on one line. […]

Done

https://review.coreboot.org/c/flashrom/+/35830/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/35830/3//COMMIT_MSG@11 
PS3, Line 11: from linux/spi/spidev.h
> Really walks like a bug, talks like a bug in `linux/spi/spidev.h`.

Done

https://review.coreboot.org/c/flashrom/+/35830/3//COMMIT_MSG@13 
PS3, Line 13: Fixes build errors with the musl C library.
> IIRC, Alpine Linux uses musl libc and that always built fine...

Done

Change in flashrom[master]: linux_spi: reorder includes for linux <4.14

[Nico Huber (Code Review)]

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/35830 )

Change subject: linux_spi: reorder includes for linux <4.14
......................................................................

linux_spi: reorder includes for linux <4.14

This works around a missing header in spidev.h present in older
versions of Linux. Patch is ported from:
https://git.buildroot.net/buildroot/tree/package/flashrom/0001-spi.patch

Signed-off-by: "Yann E. MORIN" 
Signed-off-by: Fabrice Fontaine 
Signed-off-by: David Hendricks 
Change-Id: Ieab60f59bc63aca0dc4867f31699dab4167da05b
Reviewed-on: https://review.coreboot.org/c/flashrom/+/35830
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
---
M linux_spi.c
1 file changed, 7 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved

diff --git a/linux_spi.c b/linux_spi.c
index e3d7486..aa73c18 100644
--- a/linux_spi.c
+++ b/linux_spi.c
@@ -26,12 +26,17 @@
 #include <unistd.h>
 #include <sys/ioctl.h>
 #include <linux/types.h>
-#include <linux/spi/spidev.h>
-#include <linux/ioctl.h>
 #include "flash.h"
 #include "chipdrivers.h"
 #include "programmer.h"
 #include "spi.h"
+/*
+ * Linux versions prior to v4.14-rc7 may need linux/ioctl.h included here due
+ * to missing from linux/spi/spidev.h. This was fixed in the following commit:
+ * a2b4a79b88b2 spi: uapi: spidev: add missing ioctl header
+ */
+#include <linux/ioctl.h>
+#include <linux/spi/spidev.h>

/* Devices known to work with this module (FIXME: export as struct dev_entry):
  * Beagle Bone Black