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

[Sean Anderson <seanga2@gmail.com>]

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);
>   }
>