Re: [PATCH 1/3] commands: Add MMC ext. CSD register tool

[Sascha Hauer <s.hauer@pengutronix.de>]

Hi Daniel,

Nice command. Several comments inline, there is still some way to go to
get this into shape.

On Mon, Aug 24, 2015 at 01:32:13PM +0200, Daniel Schultz wrote:
> +
> +static int print_field(u8 *reg, int index)
> +{
> +	int rev;
> +	u32 val;
> +	u32 tmp;
> +	u64 tmp64;
> +
> +	rev = reg[EXT_CSD_REV];
> +
> +	if (rev >= 7)

Additional print_field_v7/v6/v5() functions will reduce the indentation
level by one and help violating the 80 character limit less often.

> +		switch (index) {
> +		case EXT_CSD_CMDQ_MODE_EN:
> +			print_field_caption(CMDQ_MODE_EN, RWE_P);
> +			val = get_field_val(CMDQ_MODE_EN, 0, 0x1);
> +			if (val)
> +				printf("\tCommand queuing is enabled\n");
> +			else
> +				printf("\tCommand queuing is disabled\n");

Your printfs are very inefficient. You should use something like:

	if (val)
		str = "en";
	else
		str = "dis";
	printf("Command queuing is %sabled\n", str);

Same goes for many other printfs. This will result in much less similar
strings in the binary.

> +			return 1;
> +
> +		case EXT_CSD_SECURE_REMOVAL_TYPE:
> +			print_field_caption(SECURE_REMOVAL_TYPE, RWaR);
> +			val = get_field_val(SECURE_REMOVAL_TYPE, 0, 0xF);
> +			switch (val) {
> +			case 0x0:
> +				printf("\t[3-0] Supported Secure Removal Type: information removed by an erase of the physical memory\n");
> +				break;
> +			case 0x1:
> +				printf("\t[3-0] Supported Secure Removal Type: information removed by an overwriting the addressed locations with a character followed by an erase\n");
> +				break;
> +			case 0x2:
> +				printf("\t[3-0] Supported Secure Removal Type: information removed by an overwriting the addressed locations with a character, its complement, then a random character\n");
> +				break;
> +			case 0x3:
> +				printf("\t[3-0] Supported Secure Removal Type: information removed using a vendor defined\n");
> +				break;

This is *very* verbose. Could you write this a bit more compact, maybe

	case 0x0:
		str = "erase";
		break;
	case 0x1:
		str = "overwrite, then erase";
		break;
	case 0x2:
		str = "overwrite, complement, then random";
		break;
	case 0x3:
		str = "vendor defined";
		break;

	printf("[3-0] Supported Secure Removal Type: %s\n", str);

Background is that this information only makes sense when being somewhat
familiar with the spec. Without knowing the spec at all even the more
verbose printfs do not help, but when knowing the spec a more compact
writing is enough to remember what is meant.

> +
> +static void print_register_raw(u8 *reg, int index)
> +{
> +	int i;
> +
> +	if (index == 0)
> +		for (i = 0; i < EXT_CSD_BLOCKSIZE; i++) {
> +			if (i % 8 == 0)
> +				printf("%u:", i);
> +			printf(" %#02x", reg[i]);
> +			if (i % 8 == 7)
> +				printf("\n");
> +		}

	memory_display(reg, 0, EXT_CSD_BLOCKSIZE, 1, 0);

Should do it here.

> +static int do_extcsd(int argc, char *argv[])
> +{
> +	struct device_d		*dev;
> +	struct mci		*mci;
> +	struct mci_host		*host;
> +	u8			*dst;
> +	int			retval = 0;
> +	int			opt;
> +	char			*devname;
> +	int			index = 0;
> +	int			value = 0;
> +	int			write_operation = 0;
> +	int			always_write = 0;
> +	int			print_as_raw = 0;
> +
> +	if (argv[1] == NULL)
> +		return COMMAND_ERROR_USAGE;

argc contains the number of arguments. When argc is 1 then argv[1] is
undefined. It may or may not be NULL in this case. You want to use if
(argc < 2) here.

> +
> +	devname = argv[1];

You should use argv[optind] after parsing the arguments for the devname.
With this not only "extcsd <devname> [OPTIONS]" works but also "extcsd
[OPTIONS] <devname>". Don't forget to check if there actually is
argv[optind] by

	if (optind == argc)
		return COMMAND_ERROR_USAGE;

> +	if (!strncmp(devname, "/dev/", 5))
> +		devname += 5;
> +
> +	while ((opt = getopt(argc, argv, "i:v:yr")) > 0)
> +		switch (opt) {
> +		case 'i':
> +			index = simple_strtoul(optarg, NULL, 0);
> +			break;
> +		case 'v':
> +			value = simple_strtoul(optarg, NULL, 0);
> +			write_operation = 1;
> +			break;
> +		case 'y':
> +			always_write = 1;
> +			break;
> +		case 'r':
> +			print_as_raw = 1;
> +			break;
> +		}
> +
> +	dev = get_device_by_name(devname);
> +	if (dev == NULL) {
> +		retval = -ENODEV;
> +		goto error;
> +	}
> +	mci = container_of(dev, struct mci, dev);
> +	if (mci == NULL) {
> +		retval = -ENOENT;
> +		goto error;
> +	}

Unfortunately this doesn't work properly. It works when the argument
really is a mmc devices, but it doesn't detect when the argument is some
other type of device. This code will happily use container_of on
something that is not a MMC device and probably crash barebox. What you
have to do is:

- add a struct list_head member to struct mci and put all mci devices
  to a list during registration
- create a struct mci *mci_get_device_by_name(const char *name)
  function and use it here.

> +	host = mci->host;
> +	if (host == NULL) {
> +		retval = -ENOENT;
> +		goto error;
> +	}
> +	dst = xmalloc(sizeof(*dst) * EXT_CSD_BLOCKSIZE);

xmalloc(EXT_CSD_BLOCKSIZE) is enough here.

> +	if (dst == NULL) {
> +		retval = -ENOMEM;
> +		goto error;
> +	}

xmalloc never fails. You don't need to check the return value.

> +
> +	retval = mci_send_ext_csd(mci, dst);
> +	if (retval != 0)
> +		goto error_with_mem;
> +
> +	if (dst[EXT_CSD_REV] < 3) {
> +		printf("MMC version 4.2 or lower are not supported");
> +		retval = 1;
> +		goto error_with_mem;
> +	}
> +
> +	if (write_operation)
> +		if (!print_field(dst, index)) {
> +			printf("No field with this index found. Abort write operation!\n");
> +		} else {
> +			write_field(mci, dst, index, value, always_write);
> +			printf("\nValue written!\n\n");
> +			retval = mci_send_ext_csd(mci, dst);
> +			if (retval != 0)
> +				goto error_with_mem;
> +			print_field(dst, index);
> +		}
> +	else
> +		if (print_as_raw)
> +			print_register_raw(dst, index);
> +		else
> +			print_register_readable(dst, index);
> +
> +error_with_mem:
> +	free(dst);
> +error:
> +	return retval;
> +}
> +
> +BAREBOX_CMD_HELP_START(extcsd)

Better rename this to mmc-extcsd so that the context of this command is
clear.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox