diff mbox series

[10/11] sandbox: mmc: Support a backing file

Message ID 20210818214022.10.I42087e8088f8d9fb704821d73f17d7ae246a2e69@changeid
State Changes Requested
Delegated to: Tom Rini
Headers show
Series sandbox: Minor fixes and improvements | expand

Commit Message

Simon Glass Aug. 19, 2021, 3:40 a.m. UTC 
Provide a way for sandbox MMC to present data from a backing file. This
allows a filesystem to be created on the host and easily served via an
emulated mmc device.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 doc/device-tree-bindings/mmc/sandbox,mmc.txt | 18 ++++++++
 drivers/mmc/sandbox_mmc.c                    | 46 ++++++++++++++++----
 2 files changed, 55 insertions(+), 9 deletions(-)
 create mode 100644 doc/device-tree-bindings/mmc/sandbox,mmc.txt

Comments

Jaehoon Chung Aug. 28, 2021, 5:15 a.m. UTC | #1 
On 8/19/21 12:40 PM, Simon Glass wrote:
> Provide a way for sandbox MMC to present data from a backing file. This
> allows a filesystem to be created on the host and easily served via an
> emulated mmc device.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung

> ---
> 
>  doc/device-tree-bindings/mmc/sandbox,mmc.txt | 18 ++++++++
>  drivers/mmc/sandbox_mmc.c                    | 46 ++++++++++++++++----
>  2 files changed, 55 insertions(+), 9 deletions(-)
>  create mode 100644 doc/device-tree-bindings/mmc/sandbox,mmc.txt
> 
> diff --git a/doc/device-tree-bindings/mmc/sandbox,mmc.txt b/doc/device-tree-bindings/mmc/sandbox,mmc.txt
> new file mode 100644
> index 00000000000..1170bcd6a00
> --- /dev/null
> +++ b/doc/device-tree-bindings/mmc/sandbox,mmc.txt
> @@ -0,0 +1,18 @@
> +Sandbox MMC
> +===========
> +
> +Required properties:
> +- compatible : "sandbox,mmc"
> +
> +Optional properties:
> +- filename : Name of backing file, if any. This is mapped into the MMC device
> +    so can be used to provide a filesystem or other test data
> +
> +
> +Example
> +-------
> +
> +mmc2 {
> +	compatible = "sandbox,mmc";
> +	non-removable;
> +};
> diff --git a/drivers/mmc/sandbox_mmc.c b/drivers/mmc/sandbox_mmc.c
> index 895fbffecfc..1139951c626 100644
> --- a/drivers/mmc/sandbox_mmc.c
> +++ b/drivers/mmc/sandbox_mmc.c
> @@ -9,23 +9,26 @@
>  #include <errno.h>
>  #include <fdtdec.h>
>  #include <log.h>
> +#include <malloc.h>
>  #include <mmc.h>
> +#include <os.h>
>  #include <asm/test.h>
>  
>  struct sandbox_mmc_plat {
>  	struct mmc_config cfg;
>  	struct mmc mmc;
> +	const char *fname;
>  };
>  
> -#define MMC_CSIZE 0
> -#define MMC_CMULT 8 /* 8 because the card is high-capacity */
> -#define MMC_BL_LEN_SHIFT 10
> -#define MMC_BL_LEN BIT(MMC_BL_LEN_SHIFT)
> -#define MMC_CAPACITY (((MMC_CSIZE + 1) << (MMC_CMULT + 2)) \
> -		      * MMC_BL_LEN) /* 1 MiB */
> +#define MMC_CMULT		8 /* 8 because the card is high-capacity */
> +#define MMC_BL_LEN_SHIFT	10
> +#define MMC_BL_LEN		BIT(MMC_BL_LEN_SHIFT)
> +#define SIZE_MULTIPLE		((1 << (MMC_CMULT + 2)) * MMC_BL_LEN)
>  
>  struct sandbox_mmc_priv {
> -	u8 buf[MMC_CAPACITY];
> +	int csize;	/* CSIZE value to report */
> +	char *buf;
> +	int size;
>  };
>  
>  /**
> @@ -60,8 +63,8 @@ static int sandbox_mmc_send_cmd(struct udevice *dev, struct mmc_cmd *cmd,
>  	case MMC_CMD_SEND_CSD:
>  		cmd->response[0] = 0;
>  		cmd->response[1] = (MMC_BL_LEN_SHIFT << 16) |
> -				   ((MMC_CSIZE >> 16) & 0x3f);
> -		cmd->response[2] = (MMC_CSIZE & 0xffff) << 16;
> +				   ((priv->csize >> 16) & 0x3f);
> +		cmd->response[2] = (priv->csize & 0xffff) << 16;
>  		cmd->response[3] = 0;
>  		break;
>  	case SD_CMD_SWITCH_FUNC: {
> @@ -143,6 +146,8 @@ static int sandbox_mmc_of_to_plat(struct udevice *dev)
>  	struct blk_desc *blk;
>  	int ret;
>  
> +	plat->fname = dev_read_string(dev, "filename");
> +
>  	ret = mmc_of_parse(dev, cfg);
>  	if (ret)
>  		return ret;
> @@ -156,6 +161,29 @@ static int sandbox_mmc_of_to_plat(struct udevice *dev)
>  static int sandbox_mmc_probe(struct udevice *dev)
>  {
>  	struct sandbox_mmc_plat *plat = dev_get_plat(dev);
> +	struct sandbox_mmc_priv *priv = dev_get_priv(dev);
> +	int ret;
> +
> +	if (plat->fname) {
> +		ret = os_map_file(plat->fname, OS_O_RDWR | OS_O_CREAT,
> +				  (void **)&priv->buf, &priv->size);
> +		if (ret) {
> +			log_err("%s: Unable to map file '%s'\n", dev->name,
> +				plat->fname);
> +			return ret;
> +		}
> +		priv->csize = priv->size / SIZE_MULTIPLE - 1;
> +	} else {
> +		priv->csize = 0;
> +		priv->size = (priv->csize + 1) * SIZE_MULTIPLE; /* 1 MiB */
> +
> +		priv->buf = malloc(priv->size);
> +		if (!priv->buf) {
> +			log_err("%s: Not enough memory (%x bytes)\n",
> +				dev->name, priv->size);
> +			return -ENOMEM;
> +		}
> +	}
>  
>  	return mmc_init(&plat->mmc);
>  }
>
Sean Anderson Aug. 28, 2021, 6:39 a.m. UTC | #2 
On 8/18/21 11:40 PM, Simon Glass wrote:
> Provide a way for sandbox MMC to present data from a backing file. This
> allows a filesystem to be created on the host and easily served via an
> emulated mmc device.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>   doc/device-tree-bindings/mmc/sandbox,mmc.txt | 18 ++++++++
>   drivers/mmc/sandbox_mmc.c                    | 46 ++++++++++++++++----
>   2 files changed, 55 insertions(+), 9 deletions(-)
>   create mode 100644 doc/device-tree-bindings/mmc/sandbox,mmc.txt
> 
> diff --git a/doc/device-tree-bindings/mmc/sandbox,mmc.txt b/doc/device-tree-bindings/mmc/sandbox,mmc.txt
> new file mode 100644
> index 00000000000..1170bcd6a00
> --- /dev/null
> +++ b/doc/device-tree-bindings/mmc/sandbox,mmc.txt
> @@ -0,0 +1,18 @@
> +Sandbox MMC
> +===========
> +
> +Required properties:
> +- compatible : "sandbox,mmc"
> +
> +Optional properties:
> +- filename : Name of backing file, if any. This is mapped into the MMC device
> +    so can be used to provide a filesystem or other test data

Can we stick this in the command line arguments somehow? For testing
with different images, I think that editing the device tree is not the
best way to do things. It also makes it trickier to add automated tests.

> +
> +
> +Example
> +-------
> +
> +mmc2 {
> +	compatible = "sandbox,mmc";
> +	non-removable;
> +};
> diff --git a/drivers/mmc/sandbox_mmc.c b/drivers/mmc/sandbox_mmc.c
> index 895fbffecfc..1139951c626 100644
> --- a/drivers/mmc/sandbox_mmc.c
> +++ b/drivers/mmc/sandbox_mmc.c
> @@ -9,23 +9,26 @@
>   #include <errno.h>
>   #include <fdtdec.h>
>   #include <log.h>
> +#include <malloc.h>
>   #include <mmc.h>
> +#include <os.h>
>   #include <asm/test.h>
>   
>   struct sandbox_mmc_plat {
>   	struct mmc_config cfg;
>   	struct mmc mmc;
> +	const char *fname;
>   };
>   
> -#define MMC_CSIZE 0
> -#define MMC_CMULT 8 /* 8 because the card is high-capacity */
> -#define MMC_BL_LEN_SHIFT 10
> -#define MMC_BL_LEN BIT(MMC_BL_LEN_SHIFT)
> -#define MMC_CAPACITY (((MMC_CSIZE + 1) << (MMC_CMULT + 2)) \
> -		      * MMC_BL_LEN) /* 1 MiB */
> +#define MMC_CMULT		8 /* 8 because the card is high-capacity */
> +#define MMC_BL_LEN_SHIFT	10
> +#define MMC_BL_LEN		BIT(MMC_BL_LEN_SHIFT)
> +#define SIZE_MULTIPLE		((1 << (MMC_CMULT + 2)) * MMC_BL_LEN)
>   
>   struct sandbox_mmc_priv {
> -	u8 buf[MMC_CAPACITY];
> +	int csize;	/* CSIZE value to report */
> +	char *buf;
> +	int size;

nit: to save a whole 2 bytes on 64-bit you can reorder this like

char *
int
int

>   };
>   
>   /**
> @@ -60,8 +63,8 @@ static int sandbox_mmc_send_cmd(struct udevice *dev, struct mmc_cmd *cmd,
>   	case MMC_CMD_SEND_CSD:
>   		cmd->response[0] = 0;
>   		cmd->response[1] = (MMC_BL_LEN_SHIFT << 16) |
> -				   ((MMC_CSIZE >> 16) & 0x3f);
> -		cmd->response[2] = (MMC_CSIZE & 0xffff) << 16;
> +				   ((priv->csize >> 16) & 0x3f);
> +		cmd->response[2] = (priv->csize & 0xffff) << 16;
>   		cmd->response[3] = 0;
>   		break;
>   	case SD_CMD_SWITCH_FUNC: {
> @@ -143,6 +146,8 @@ static int sandbox_mmc_of_to_plat(struct udevice *dev)
>   	struct blk_desc *blk;
>   	int ret;
>   
> +	plat->fname = dev_read_string(dev, "filename");
> +
>   	ret = mmc_of_parse(dev, cfg);
>   	if (ret)
>   		return ret;
> @@ -156,6 +161,29 @@ static int sandbox_mmc_of_to_plat(struct udevice *dev)
>   static int sandbox_mmc_probe(struct udevice *dev)
>   {
>   	struct sandbox_mmc_plat *plat = dev_get_plat(dev);
> +	struct sandbox_mmc_priv *priv = dev_get_priv(dev);
> +	int ret;
> +
> +	if (plat->fname) {
> +		ret = os_map_file(plat->fname, OS_O_RDWR | OS_O_CREAT,
> +				  (void **)&priv->buf, &priv->size);
> +		if (ret) {
> +			log_err("%s: Unable to map file '%s'\n", dev->name,
> +				plat->fname);
> +			return ret;
> +		}
> +		priv->csize = priv->size / SIZE_MULTIPLE - 1;

Won't this cut off the end of the file? If we can't avoid this, we
should at least warn the user that we are truncating it.

--Sean

> +	} else {
> +		priv->csize = 0;
> +		priv->size = (priv->csize + 1) * SIZE_MULTIPLE; /* 1 MiB */
> +
> +		priv->buf = malloc(priv->size);
> +		if (!priv->buf) {
> +			log_err("%s: Not enough memory (%x bytes)\n",
> +				dev->name, priv->size);
> +			return -ENOMEM;
> +		}
> +	}
>   
>   	return mmc_init(&plat->mmc);
>   }
>
Tom Rini Sept. 16, 2021, 6:40 p.m. UTC | #3 
On Wed, Aug 18, 2021 at 09:40:32PM -0600, Simon Glass wrote:

> Provide a way for sandbox MMC to present data from a backing file. This
> allows a filesystem to be created on the host and easily served via an
> emulated mmc device.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
> ---
> 
>  doc/device-tree-bindings/mmc/sandbox,mmc.txt | 18 ++++++++
>  drivers/mmc/sandbox_mmc.c                    | 46 ++++++++++++++++----
>  2 files changed, 55 insertions(+), 9 deletions(-)
>  create mode 100644 doc/device-tree-bindings/mmc/sandbox,mmc.txt

As is, this breaks how I've always run pytests on sandbox.
Simon Glass Sept. 18, 2021, 9:34 a.m. UTC | #4 
Hi Sean,

On Sat, 28 Aug 2021 at 00:39, Sean Anderson <seanga2@gmail.com> wrote:
>
> On 8/18/21 11:40 PM, Simon Glass wrote:
> > Provide a way for sandbox MMC to present data from a backing file. This
> > allows a filesystem to be created on the host and easily served via an
> > emulated mmc device.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >   doc/device-tree-bindings/mmc/sandbox,mmc.txt | 18 ++++++++
> >   drivers/mmc/sandbox_mmc.c                    | 46 ++++++++++++++++----
> >   2 files changed, 55 insertions(+), 9 deletions(-)
> >   create mode 100644 doc/device-tree-bindings/mmc/sandbox,mmc.txt
> >
> > diff --git a/doc/device-tree-bindings/mmc/sandbox,mmc.txt b/doc/device-tree-bindings/mmc/sandbox,mmc.txt
> > new file mode 100644
> > index 00000000000..1170bcd6a00
> > --- /dev/null
> > +++ b/doc/device-tree-bindings/mmc/sandbox,mmc.txt
> > @@ -0,0 +1,18 @@
> > +Sandbox MMC
> > +===========
> > +
> > +Required properties:
> > +- compatible : "sandbox,mmc"
> > +
> > +Optional properties:
> > +- filename : Name of backing file, if any. This is mapped into the MMC device
> > +    so can be used to provide a filesystem or other test data
>
> Can we stick this in the command line arguments somehow? For testing
> with different images, I think that editing the device tree is not the
> best way to do things. It also makes it trickier to add automated tests.

We can, of course. We used to have that with SPI flash but we ended up
dropping it as it was unwieldy and disconnected from the devicetree .
The automated tests can work by setting up a file in advance. In fact
we do this with SPI flash and I2C eeprom.

So I think this works much better. See dm_test_spi_flash() for how the
file is set up in a unit test.

>
> > +
> > +
> > +Example
> > +-------
> > +
> > +mmc2 {
> > +     compatible = "sandbox,mmc";
> > +     non-removable;
> > +};
> > diff --git a/drivers/mmc/sandbox_mmc.c b/drivers/mmc/sandbox_mmc.c
> > index 895fbffecfc..1139951c626 100644
> > --- a/drivers/mmc/sandbox_mmc.c
> > +++ b/drivers/mmc/sandbox_mmc.c
> > @@ -9,23 +9,26 @@
> >   #include <errno.h>
> >   #include <fdtdec.h>
> >   #include <log.h>
> > +#include <malloc.h>
> >   #include <mmc.h>
> > +#include <os.h>
> >   #include <asm/test.h>
> >
> >   struct sandbox_mmc_plat {
> >       struct mmc_config cfg;
> >       struct mmc mmc;
> > +     const char *fname;
> >   };
> >
> > -#define MMC_CSIZE 0
> > -#define MMC_CMULT 8 /* 8 because the card is high-capacity */
> > -#define MMC_BL_LEN_SHIFT 10
> > -#define MMC_BL_LEN BIT(MMC_BL_LEN_SHIFT)
> > -#define MMC_CAPACITY (((MMC_CSIZE + 1) << (MMC_CMULT + 2)) \
> > -                   * MMC_BL_LEN) /* 1 MiB */
> > +#define MMC_CMULT            8 /* 8 because the card is high-capacity */
> > +#define MMC_BL_LEN_SHIFT     10
> > +#define MMC_BL_LEN           BIT(MMC_BL_LEN_SHIFT)
> > +#define SIZE_MULTIPLE                ((1 << (MMC_CMULT + 2)) * MMC_BL_LEN)
> >
> >   struct sandbox_mmc_priv {
> > -     u8 buf[MMC_CAPACITY];
> > +     int csize;      /* CSIZE value to report */
> > +     char *buf;
> > +     int size;
>
> nit: to save a whole 2 bytes on 64-bit you can reorder this like
>
> char *
> int
> int

OK will do.


>
> >   };
> >
> >   /**
> > @@ -60,8 +63,8 @@ static int sandbox_mmc_send_cmd(struct udevice *dev, struct mmc_cmd *cmd,
> >       case MMC_CMD_SEND_CSD:
> >               cmd->response[0] = 0;
> >               cmd->response[1] = (MMC_BL_LEN_SHIFT << 16) |
> > -                                ((MMC_CSIZE >> 16) & 0x3f);
> > -             cmd->response[2] = (MMC_CSIZE & 0xffff) << 16;
> > +                                ((priv->csize >> 16) & 0x3f);
> > +             cmd->response[2] = (priv->csize & 0xffff) << 16;
> >               cmd->response[3] = 0;
> >               break;
> >       case SD_CMD_SWITCH_FUNC: {
> > @@ -143,6 +146,8 @@ static int sandbox_mmc_of_to_plat(struct udevice *dev)
> >       struct blk_desc *blk;
> >       int ret;
> >
> > +     plat->fname = dev_read_string(dev, "filename");
> > +
> >       ret = mmc_of_parse(dev, cfg);
> >       if (ret)
> >               return ret;
> > @@ -156,6 +161,29 @@ static int sandbox_mmc_of_to_plat(struct udevice *dev)
> >   static int sandbox_mmc_probe(struct udevice *dev)
> >   {
> >       struct sandbox_mmc_plat *plat = dev_get_plat(dev);
> > +     struct sandbox_mmc_priv *priv = dev_get_priv(dev);
> > +     int ret;
> > +
> > +     if (plat->fname) {
> > +             ret = os_map_file(plat->fname, OS_O_RDWR | OS_O_CREAT,
> > +                               (void **)&priv->buf, &priv->size);
> > +             if (ret) {
> > +                     log_err("%s: Unable to map file '%s'\n", dev->name,
> > +                             plat->fname);
> > +                     return ret;
> > +             }
> > +             priv->csize = priv->size / SIZE_MULTIPLE - 1;
>
> Won't this cut off the end of the file? If we can't avoid this, we
> should at least warn the user that we are truncating it.

Yes it does and in fact the granularity is 1MB, I think. I can add a warning.

>
> --Sean
>
> > +     } else {
> > +             priv->csize = 0;
> > +             priv->size = (priv->csize + 1) * SIZE_MULTIPLE; /* 1 MiB */
> > +
> > +             priv->buf = malloc(priv->size);
> > +             if (!priv->buf) {
> > +                     log_err("%s: Not enough memory (%x bytes)\n",
> > +                             dev->name, priv->size);
> > +                     return -ENOMEM;
> > +             }
> > +     }
> >
> >       return mmc_init(&plat->mmc);
> >   }
> >
>

Regards,
Simon
Simon Glass Sept. 18, 2021, 9:34 a.m. UTC | #5 
Hi Tom,

On Thu, 16 Sept 2021 at 12:40, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Aug 18, 2021 at 09:40:32PM -0600, Simon Glass wrote:
>
> > Provide a way for sandbox MMC to present data from a backing file. This
> > allows a filesystem to be created on the host and easily served via an
> > emulated mmc device.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
> > ---
> >
> >  doc/device-tree-bindings/mmc/sandbox,mmc.txt | 18 ++++++++
> >  drivers/mmc/sandbox_mmc.c                    | 46 ++++++++++++++++----
> >  2 files changed, 55 insertions(+), 9 deletions(-)
> >  create mode 100644 doc/device-tree-bindings/mmc/sandbox,mmc.txt
>
> As is, this breaks how I've always run pytests on sandbox.

How does it break it? Do you get an error? The feature is supposed to
be optional.

Regards,
Simon
Tom Rini Sept. 18, 2021, 1:16 p.m. UTC | #6 
On Sat, Sep 18, 2021 at 03:34:54AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 16 Sept 2021 at 12:40, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Aug 18, 2021 at 09:40:32PM -0600, Simon Glass wrote:
> >
> > > Provide a way for sandbox MMC to present data from a backing file. This
> > > allows a filesystem to be created on the host and easily served via an
> > > emulated mmc device.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
> > > ---
> > >
> > >  doc/device-tree-bindings/mmc/sandbox,mmc.txt | 18 ++++++++
> > >  drivers/mmc/sandbox_mmc.c                    | 46 ++++++++++++++++----
> > >  2 files changed, 55 insertions(+), 9 deletions(-)
> > >  create mode 100644 doc/device-tree-bindings/mmc/sandbox,mmc.txt
> >
> > As is, this breaks how I've always run pytests on sandbox.
> 
> How does it break it? Do you get an error? The feature is supposed to
> be optional.

Without doing anything to enable it, a few of the mmc unit tests failed.
I don't have the logs handy right now (I made that sometimes bad
decision to test 2 series at once, and now I'm waiting a bit more on
final feedback on the changes the timestamp cleanup needed before I push
that + the rest of this series).
Simon Glass Sept. 18, 2021, 4:31 p.m. UTC | #7 
Hi Tom,

On Sat, 18 Sept 2021 at 07:16, Tom Rini <trini@konsulko.com> wrote:
>
> On Sat, Sep 18, 2021 at 03:34:54AM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 16 Sept 2021 at 12:40, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Aug 18, 2021 at 09:40:32PM -0600, Simon Glass wrote:
> > >
> > > > Provide a way for sandbox MMC to present data from a backing file. This
> > > > allows a filesystem to be created on the host and easily served via an
> > > > emulated mmc device.
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
> > > > ---
> > > >
> > > >  doc/device-tree-bindings/mmc/sandbox,mmc.txt | 18 ++++++++
> > > >  drivers/mmc/sandbox_mmc.c                    | 46 ++++++++++++++++----
> > > >  2 files changed, 55 insertions(+), 9 deletions(-)
> > > >  create mode 100644 doc/device-tree-bindings/mmc/sandbox,mmc.txt
> > >
> > > As is, this breaks how I've always run pytests on sandbox.
> >
> > How does it break it? Do you get an error? The feature is supposed to
> > be optional.
>
> Without doing anything to enable it, a few of the mmc unit tests failed.
> I don't have the logs handy right now (I made that sometimes bad
> decision to test 2 series at once, and now I'm waiting a bit more on
> final feedback on the changes the timestamp cleanup needed before I push
> that + the rest of this series).

Oh dear, I will take a look and see what is going on there.

Regards,
Simon
Simon Glass Sept. 20, 2021, 3:18 a.m. UTC | #8 
Hi Tom,

On Sat, 18 Sept 2021 at 10:31, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Tom,
>
> On Sat, 18 Sept 2021 at 07:16, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sat, Sep 18, 2021 at 03:34:54AM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Thu, 16 Sept 2021 at 12:40, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Wed, Aug 18, 2021 at 09:40:32PM -0600, Simon Glass wrote:
> > > >
> > > > > Provide a way for sandbox MMC to present data from a backing file. This
> > > > > allows a filesystem to be created on the host and easily served via an
> > > > > emulated mmc device.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
> > > > > ---
> > > > >
> > > > >  doc/device-tree-bindings/mmc/sandbox,mmc.txt | 18 ++++++++
> > > > >  drivers/mmc/sandbox_mmc.c                    | 46 ++++++++++++++++----
> > > > >  2 files changed, 55 insertions(+), 9 deletions(-)
> > > > >  create mode 100644 doc/device-tree-bindings/mmc/sandbox,mmc.txt
> > > >
> > > > As is, this breaks how I've always run pytests on sandbox.
> > >
> > > How does it break it? Do you get an error? The feature is supposed to
> > > be optional.
> >
> > Without doing anything to enable it, a few of the mmc unit tests failed.
> > I don't have the logs handy right now (I made that sometimes bad
> > decision to test 2 series at once, and now I'm waiting a bit more on
> > final feedback on the changes the timestamp cleanup needed before I push
> > that + the rest of this series).
>
> Oh dear, I will take a look and see what is going on there.

The problem seems to me something going wrong with malloc(). I'm not
really sure what but I have seen it before. Basically the mallinfo
struct becomes corrupted somehow and from there everything goes
haywire. It actually happens today in normal operation, but somehow it
doesn't cause problems. I looked at it a while back and did not make
progress.

Anyway I will see if I can dig into it again.

Regards,
Simon
Simon Glass Oct. 21, 2021, 8:17 p.m. UTC | #9 
Hi Tom,

On Sun, 19 Sept 2021 at 21:18, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Tom,
>
> On Sat, 18 Sept 2021 at 10:31, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Tom,
> >
> > On Sat, 18 Sept 2021 at 07:16, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Sat, Sep 18, 2021 at 03:34:54AM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Thu, 16 Sept 2021 at 12:40, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Wed, Aug 18, 2021 at 09:40:32PM -0600, Simon Glass wrote:
> > > > >
> > > > > > Provide a way for sandbox MMC to present data from a backing file. This
> > > > > > allows a filesystem to be created on the host and easily served via an
> > > > > > emulated mmc device.
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
> > > > > > ---
> > > > > >
> > > > > >  doc/device-tree-bindings/mmc/sandbox,mmc.txt | 18 ++++++++
> > > > > >  drivers/mmc/sandbox_mmc.c                    | 46 ++++++++++++++++----
> > > > > >  2 files changed, 55 insertions(+), 9 deletions(-)
> > > > > >  create mode 100644 doc/device-tree-bindings/mmc/sandbox,mmc.txt
> > > > >
> > > > > As is, this breaks how I've always run pytests on sandbox.
> > > >
> > > > How does it break it? Do you get an error? The feature is supposed to
> > > > be optional.
> > >
> > > Without doing anything to enable it, a few of the mmc unit tests failed.
> > > I don't have the logs handy right now (I made that sometimes bad
> > > decision to test 2 series at once, and now I'm waiting a bit more on
> > > final feedback on the changes the timestamp cleanup needed before I push
> > > that + the rest of this series).
> >
> > Oh dear, I will take a look and see what is going on there.
>
> The problem seems to me something going wrong with malloc(). I'm not
> really sure what but I have seen it before. Basically the mallinfo
> struct becomes corrupted somehow and from there everything goes
> haywire. It actually happens today in normal operation, but somehow it
> doesn't cause problems. I looked at it a while back and did not make
> progress.
>
> Anyway I will see if I can dig into it again.

Actually i did figure this out. Partly is was a calculation issue but
mostly it was not unmapping the memory after running, which causes
problems when it is 25MB each time.

Regards,
Simon
diff mbox series

Patch

diff --git a/doc/device-tree-bindings/mmc/sandbox,mmc.txt b/doc/device-tree-bindings/mmc/sandbox,mmc.txt
new file mode 100644
index 00000000000..1170bcd6a00
--- /dev/null
+++ b/doc/device-tree-bindings/mmc/sandbox,mmc.txt
@@ -0,0 +1,18 @@ 
+Sandbox MMC
+===========
+
+Required properties:
+- compatible : "sandbox,mmc"
+
+Optional properties:
+- filename : Name of backing file, if any. This is mapped into the MMC device
+    so can be used to provide a filesystem or other test data
+
+
+Example
+-------
+
+mmc2 {
+	compatible = "sandbox,mmc";
+	non-removable;
+};
diff --git a/drivers/mmc/sandbox_mmc.c b/drivers/mmc/sandbox_mmc.c
index 895fbffecfc..1139951c626 100644
--- a/drivers/mmc/sandbox_mmc.c
+++ b/drivers/mmc/sandbox_mmc.c
@@ -9,23 +9,26 @@ 
 #include <errno.h>
 #include <fdtdec.h>
 #include <log.h>
+#include <malloc.h>
 #include <mmc.h>
+#include <os.h>
 #include <asm/test.h>
 
 struct sandbox_mmc_plat {
 	struct mmc_config cfg;
 	struct mmc mmc;
+	const char *fname;
 };
 
-#define MMC_CSIZE 0
-#define MMC_CMULT 8 /* 8 because the card is high-capacity */
-#define MMC_BL_LEN_SHIFT 10
-#define MMC_BL_LEN BIT(MMC_BL_LEN_SHIFT)
-#define MMC_CAPACITY (((MMC_CSIZE + 1) << (MMC_CMULT + 2)) \
-		      * MMC_BL_LEN) /* 1 MiB */
+#define MMC_CMULT		8 /* 8 because the card is high-capacity */
+#define MMC_BL_LEN_SHIFT	10
+#define MMC_BL_LEN		BIT(MMC_BL_LEN_SHIFT)
+#define SIZE_MULTIPLE		((1 << (MMC_CMULT + 2)) * MMC_BL_LEN)
 
 struct sandbox_mmc_priv {
-	u8 buf[MMC_CAPACITY];
+	int csize;	/* CSIZE value to report */
+	char *buf;
+	int size;
 };
 
 /**
@@ -60,8 +63,8 @@  static int sandbox_mmc_send_cmd(struct udevice *dev, struct mmc_cmd *cmd,
 	case MMC_CMD_SEND_CSD:
 		cmd->response[0] = 0;
 		cmd->response[1] = (MMC_BL_LEN_SHIFT << 16) |
-				   ((MMC_CSIZE >> 16) & 0x3f);
-		cmd->response[2] = (MMC_CSIZE & 0xffff) << 16;
+				   ((priv->csize >> 16) & 0x3f);
+		cmd->response[2] = (priv->csize & 0xffff) << 16;
 		cmd->response[3] = 0;
 		break;
 	case SD_CMD_SWITCH_FUNC: {
@@ -143,6 +146,8 @@  static int sandbox_mmc_of_to_plat(struct udevice *dev)
 	struct blk_desc *blk;
 	int ret;
 
+	plat->fname = dev_read_string(dev, "filename");
+
 	ret = mmc_of_parse(dev, cfg);
 	if (ret)
 		return ret;
@@ -156,6 +161,29 @@  static int sandbox_mmc_of_to_plat(struct udevice *dev)
 static int sandbox_mmc_probe(struct udevice *dev)
 {
 	struct sandbox_mmc_plat *plat = dev_get_plat(dev);
+	struct sandbox_mmc_priv *priv = dev_get_priv(dev);
+	int ret;
+
+	if (plat->fname) {
+		ret = os_map_file(plat->fname, OS_O_RDWR | OS_O_CREAT,
+				  (void **)&priv->buf, &priv->size);
+		if (ret) {
+			log_err("%s: Unable to map file '%s'\n", dev->name,
+				plat->fname);
+			return ret;
+		}
+		priv->csize = priv->size / SIZE_MULTIPLE - 1;
+	} else {
+		priv->csize = 0;
+		priv->size = (priv->csize + 1) * SIZE_MULTIPLE; /* 1 MiB */
+
+		priv->buf = malloc(priv->size);
+		if (!priv->buf) {
+			log_err("%s: Not enough memory (%x bytes)\n",
+				dev->name, priv->size);
+			return -ENOMEM;
+		}
+	}
 
 	return mmc_init(&plat->mmc);
 }