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

Submitted by Cyril Bur on April 20, 2017, 1:56 a.m.

Details

Message ID 20170420015639.4274-2-cyril.bur@au1.ibm.com
State Superseded
Headers show

Commit Message

Cyril Bur April 20, 2017, 1:56 a.m.
Commit e5720d3fe94f5b85c374520b89fb8fbb2143df94 introduced using the
64bit MTD erase ioctl() after a rework of blocklevel to make it fully
64bit safe. It turns out that not all kernels on which pflash might run
actually have the 64bit MTD ioctl(), this is probably due to the kernel
being based off 2.6.28.10.

This bug is partially exposed by making the blocklevel_erase() path
better from d6a5b53f878bb422cfd2a960fff9dce5e3bb4117 which added
blocklevel_smart_erase().

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

Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
---
 libflash/file.c | 42 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

Comments

Michael Neuling April 20, 2017, 4:30 a.m.
On Thu, 2017-04-20 at 11:56 +1000, Cyril Bur wrote:
> Commit e5720d3fe94f5b85c374520b89fb8fbb2143df94 introduced using the
> 64bit MTD erase ioctl() after a rework of blocklevel to make it fully
> 64bit safe. It turns out that not all kernels on which pflash might run
> actually have the 64bit MTD ioctl(), this is probably due to the kernel
> being based off 2.6.28.10.

How about this:
--
We recently made MTD 64 bit safe in e5720d3fe94 which now requires the 64 bit
MTD erase ioctl.  Unfortantley his ioctl is not present in older kernel (pre
????) which is 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.
--

BTW What happens if we can't fall back on these systems?

> > Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
> ---
>  libflash/file.c | 42 ++++++++++++++++++++++++++++++++++++------
>  1 file changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/libflash/file.c b/libflash/file.c
> index 5f074cf2..ac4ceeaf 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,42 @@ 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

hope?

> +	 */
> > +	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;
>  }
Cyril Bur April 20, 2017, 4:45 a.m.
On Thu, 2017-04-20 at 14:30 +1000, Michael Neuling wrote:
> On Thu, 2017-04-20 at 11:56 +1000, Cyril Bur wrote:
> > Commit e5720d3fe94f5b85c374520b89fb8fbb2143df94 introduced using the
> > 64bit MTD erase ioctl() after a rework of blocklevel to make it fully
> > 64bit safe. It turns out that not all kernels on which pflash might run
> > actually have the 64bit MTD ioctl(), this is probably due to the kernel
> > being based off 2.6.28.10.
> 
> How about this:
> --
> We recently made MTD 64 bit safe in e5720d3fe94 which now requires the 64 bit
> MTD erase ioctl.  Unfortantley his ioctl is not present in older kernel (pre
> ????) which is 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.

Sure

> --
> 
> BTW What happens if we can't fall back on these systems?
> 

We'll try the 64bit ioctl() and it will fail. I don't expect this
scenario will happen since the BMC systems would need to have more than
32bits of addressable flash. Currently the biggest amount of flash I
know of on a BMC system is 128MB so we're a way off from the problem.

Addressing more than 32bits could happen but this is typically in test
on HOST kernels. Any host kernels which don't have the 64bit MTD ioctls
would be far too old to even think about running on BMC systems.

> > > Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
> > 
> > ---
> >  libflash/file.c | 42 ++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 36 insertions(+), 6 deletions(-)
> > 
> > diff --git a/libflash/file.c b/libflash/file.c
> > index 5f074cf2..ac4ceeaf 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,42 @@ 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
> 
> hope?
> 
> > +	 */
> > > +	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 hide | download patch | download mbox

diff --git a/libflash/file.c b/libflash/file.c
index 5f074cf2..ac4ceeaf 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,42 @@  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 (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;
 }