[v2] libflash/file: Only use 64bit MTD erase ioctl() when needed

Message ID 20170420053944.16127-1-cyril.bur@au1.ibm.com
State Accepted
Headers show

Commit Message

Cyril Bur April 20, 2017, 5:39 a.m.
We recently made MTD 64 bit safe in e5720d3fe94 which now requires the
64 bit MTD erase ioctl. Unfortunately this ioctl is not present in
older kernels used by some BMC vendors that use pflash.

This patch addresses this by only using the 64bit version of the erase
ioctl() if the parameters exceed 32bit in size.

If an erase requires the 64bit ioctl() on a kernel which does not
support it, the code will still attempt it. There is no way of knowing
beforehand if the kernel supports it. The ioctl() will fail and an error
will be returned from from the function.

Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
---
V2: Reword this commit message
    Dropped the test, on reflection it isn't a great test. In that it
    doesn't really test much.

 libflash/file.c | 44 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 38 insertions(+), 6 deletions(-)

Comments

Alistair Popple April 20, 2017, 5:52 a.m. | #1
This looks reasonable to me. I'm guessing there is no workaround for
doing a 64-bit MTD erase on kernels that don't support it so just
returning an error seems ok to me.

Acked-By: Alistair Popple <alistair@popple.id.au>

On Thu, 20 Apr 2017 03:39:44 PM Cyril Bur wrote:
> We recently made MTD 64 bit safe in e5720d3fe94 which now requires the
> 64 bit MTD erase ioctl. Unfortunately this ioctl is not present in
> older kernels used by some BMC vendors that use pflash.
>
> This patch addresses this by only using the 64bit version of the erase
> ioctl() if the parameters exceed 32bit in size.
>
> If an erase requires the 64bit ioctl() on a kernel which does not
> support it, the code will still attempt it. There is no way of knowing
> beforehand if the kernel supports it. The ioctl() will fail and an error
> will be returned from from the function.
>
> Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
> ---
> V2: Reword this commit message
>     Dropped the test, on reflection it isn't a great test. In that it
>     doesn't really test much.
>
>  libflash/file.c | 44 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 38 insertions(+), 6 deletions(-)
>
> diff --git a/libflash/file.c b/libflash/file.c
> index 5f074cf2..8d1ed02a 100644
> --- a/libflash/file.c
> +++ b/libflash/file.c
> @@ -15,6 +15,7 @@
>   */
>  #define _GNU_SOURCE
>  #include <errno.h>
> +#include <inttypes.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -130,13 +131,44 @@ static int file_erase(struct blocklevel_device *bl, uint64_t dst, uint64_t len)
>  static int mtd_erase(struct blocklevel_device *bl, uint64_t dst, uint64_t len)
>  {
>  	struct file_data *file_data = container_of(bl, struct file_data, bl);
> -	struct erase_info_user64 erase_info = {
> -		.start = dst,
> -		.length = len
> -	};
> +	int err;
>
> -	if (ioctl(file_data->fd, MEMERASE64, &erase_info) == -1)
> -		return FLASH_ERR_PARM_ERROR;
> +	FL_DBG("%s: dst: 0x%" PRIx64 ", len: 0x%" PRIx64 "\n", __func__, dst, len);
> +
> +	/*
> +	 * Some kernels that pflash supports do not know about the 64bit
> +	 * version of the ioctl() therefore we'll just use the 32bit (which
> +	 * should always be supported...) unless we MUST use the 64bit and
> +	 * then lets just hope the kernel knows how to deal with it. If it
> +	 * is unsupported the ioctl() will fail and we'll report that -
> +	 * there is no other option.
> +	 */
> +	if (dst > UINT_MAX || len > UINT_MAX) {
> +		struct erase_info_user64 erase_info = {
> +			.start = dst,
> +			.length = len
> +		};
> +
> +		if (ioctl(file_data->fd, MEMERASE64, &erase_info) == -1) {
> +			err = errno;
> +			if (err == 25) /* Kernel doesn't do 64bit MTD erase ioctl() */
> +				FL_DBG("Attempted a 64bit erase on a kernel which doesn't support it\n");
> +			FL_ERR("%s: IOCTL to kernel failed! %s\n", __func__, strerror(err));
> +			errno = err;
> +			return FLASH_ERR_PARM_ERROR;
> +		}
> +	} else {
> +		struct erase_info_user erase_info = {
> +			.start = dst,
> +			.length = len
> +		};
> +		if (ioctl(file_data->fd, MEMERASE, &erase_info) == -1) {
> +			err = errno;
> +			FL_ERR("%s: IOCTL to kernel failed! %s\n", __func__, strerror(err));
> +			errno = err;
> +			return FLASH_ERR_PARM_ERROR;
> +		}
> +	}
>
>  	return 0;
>  }
>
Michael Neuling April 20, 2017, 5:57 a.m. | #2
This has been merged into skiboot upstream as 86d4bcbdcb.



On Thu, 2017-04-20 at 15:39 +1000, Cyril Bur wrote:
> We recently made MTD 64 bit safe in e5720d3fe94 which now requires the
> 64 bit MTD erase ioctl. Unfortunately this ioctl is not present in
> older kernels used by some BMC vendors that use pflash.
> 
> This patch addresses this by only using the 64bit version of the erase
> ioctl() if the parameters exceed 32bit in size.
> 
> If an erase requires the 64bit ioctl() on a kernel which does not
> support it, the code will still attempt it. There is no way of knowing
> beforehand if the kernel supports it. The ioctl() will fail and an error
> will be returned from from the function.
> 
> Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
> ---
> V2: Reword this commit message
>     Dropped the test, on reflection it isn't a great test. In that it
>     doesn't really test much.
> 
>  libflash/file.c | 44 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/libflash/file.c b/libflash/file.c
> index 5f074cf2..8d1ed02a 100644
> --- a/libflash/file.c
> +++ b/libflash/file.c
> @@ -15,6 +15,7 @@
>   */
>  #define _GNU_SOURCE
>  #include <errno.h>
> +#include <inttypes.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -130,13 +131,44 @@ static int file_erase(struct blocklevel_device *bl,
> uint64_t dst, uint64_t len)
>  static int mtd_erase(struct blocklevel_device *bl, uint64_t dst, uint64_t
> len)
>  {
>  	struct file_data *file_data = container_of(bl, struct file_data, bl);
> -	struct erase_info_user64 erase_info = {
> -		.start = dst,
> -		.length = len
> -	};
> +	int err;
>  
> -	if (ioctl(file_data->fd, MEMERASE64, &erase_info) == -1)
> -		return FLASH_ERR_PARM_ERROR;
> +	FL_DBG("%s: dst: 0x%" PRIx64 ", len: 0x%" PRIx64 "\n", __func__, dst,
> len);
> +
> +	/*
> +	 * Some kernels that pflash supports do not know about the 64bit
> +	 * version of the ioctl() therefore we'll just use the 32bit (which
> +	 * should always be supported...) unless we MUST use the 64bit and
> +	 * then lets just hope the kernel knows how to deal with it. If it
> +	 * is unsupported the ioctl() will fail and we'll report that -
> +	 * there is no other option.
> +	 */
> +	if (dst > UINT_MAX || len > UINT_MAX) {
> +		struct erase_info_user64 erase_info = {
> +			.start = dst,
> +			.length = len
> +		};
> +
> +		if (ioctl(file_data->fd, MEMERASE64, &erase_info) == -1) {
> +			err = errno;
> +			if (err == 25) /* Kernel doesn't do 64bit MTD erase
> ioctl() */
> +				FL_DBG("Attempted a 64bit erase on a kernel
> which doesn't support it\n");
> +			FL_ERR("%s: IOCTL to kernel failed! %s\n", __func__,
> strerror(err));
> +			errno = err;
> +			return FLASH_ERR_PARM_ERROR;
> +		}
> +	} else {
> +		struct erase_info_user erase_info = {
> +			.start = dst,
> +			.length = len
> +		};
> +		if (ioctl(file_data->fd, MEMERASE, &erase_info) == -1) {
> +			err = errno;
> +			FL_ERR("%s: IOCTL to kernel failed! %s\n", __func__,
> strerror(err));
> +			errno = err;
> +			return FLASH_ERR_PARM_ERROR;
> +		}
> +	}
>  
>  	return 0;
>  }

Patch

diff --git a/libflash/file.c b/libflash/file.c
index 5f074cf2..8d1ed02a 100644
--- a/libflash/file.c
+++ b/libflash/file.c
@@ -15,6 +15,7 @@ 
  */
 #define _GNU_SOURCE
 #include <errno.h>
+#include <inttypes.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -130,13 +131,44 @@  static int file_erase(struct blocklevel_device *bl, uint64_t dst, uint64_t len)
 static int mtd_erase(struct blocklevel_device *bl, uint64_t dst, uint64_t len)
 {
 	struct file_data *file_data = container_of(bl, struct file_data, bl);
-	struct erase_info_user64 erase_info = {
-		.start = dst,
-		.length = len
-	};
+	int err;
 
-	if (ioctl(file_data->fd, MEMERASE64, &erase_info) == -1)
-		return FLASH_ERR_PARM_ERROR;
+	FL_DBG("%s: dst: 0x%" PRIx64 ", len: 0x%" PRIx64 "\n", __func__, dst, len);
+
+	/*
+	 * Some kernels that pflash supports do not know about the 64bit
+	 * version of the ioctl() therefore we'll just use the 32bit (which
+	 * should always be supported...) unless we MUST use the 64bit and
+	 * then lets just hope the kernel knows how to deal with it. If it
+	 * is unsupported the ioctl() will fail and we'll report that -
+	 * there is no other option.
+	 */
+	if (dst > UINT_MAX || len > UINT_MAX) {
+		struct erase_info_user64 erase_info = {
+			.start = dst,
+			.length = len
+		};
+
+		if (ioctl(file_data->fd, MEMERASE64, &erase_info) == -1) {
+			err = errno;
+			if (err == 25) /* Kernel doesn't do 64bit MTD erase ioctl() */
+				FL_DBG("Attempted a 64bit erase on a kernel which doesn't support it\n");
+			FL_ERR("%s: IOCTL to kernel failed! %s\n", __func__, strerror(err));
+			errno = err;
+			return FLASH_ERR_PARM_ERROR;
+		}
+	} else {
+		struct erase_info_user erase_info = {
+			.start = dst,
+			.length = len
+		};
+		if (ioctl(file_data->fd, MEMERASE, &erase_info) == -1) {
+			err = errno;
+			FL_ERR("%s: IOCTL to kernel failed! %s\n", __func__, strerror(err));
+			errno = err;
+			return FLASH_ERR_PARM_ERROR;
+		}
+	}
 
 	return 0;
 }