Re: [PATCH] [UBI] Volume table update fix

[Artem Bityutskiy <dedekind@infradead.org>]

Hi,

thanks for the patch. Below are my minor/stylistic notes.

> diff --git a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c
> index 1afc61e..c776037 100644
> --- a/drivers/mtd/ubi/vtbl.c
> +++ b/drivers/mtd/ubi/vtbl.c
> @@ -84,7 +84,7 @@ static struct ubi_vtbl_record empty_vtbl_record;
>  int ubi_change_vtbl_record(struct ubi_device *ubi, int idx,
>  			   struct ubi_vtbl_record *vtbl_rec)
>  {
> -	int i, err;
> +	int copy, err, err1;
>  	uint32_t crc;
>  	struct ubi_volume *layout_vol;
> 
> @@ -99,19 +99,41 @@ int ubi_change_vtbl_record(struct ubi_device *ubi, int idx,
>  	}
> 
>  	memcpy(&ubi->vtbl[idx], vtbl_rec, sizeof(struct ubi_vtbl_record));
> -	for (i = 0; i < UBI_LAYOUT_VOLUME_EBS; i++) {
> -		err = ubi_eba_unmap_leb(ubi, layout_vol, i);
> +	for (copy = 0; copy < UBI_LAYOUT_VOLUME_EBS; copy++) {
> +		err = ubi_eba_unmap_leb(ubi, layout_vol, copy);
>  		if (err)
> -			return err;
> +			goto out_error;
> 
> -		err = ubi_eba_write_leb(ubi, layout_vol, i, ubi->vtbl, 0,
> +		err = ubi_eba_write_leb(ubi, layout_vol, copy, ubi->vtbl, 0,
>  					ubi->vtbl_size, UBI_LONGTERM);
>  		if (err)
> -			return err;
> +			goto out_error;
>  	}
> 
>  	paranoid_vtbl_check(ubi);
>  	return 0;
> +
> +out_error:
> +	/* If first copy was written,volume creation is successful.
> +	* But switch to read only mode as we have only one copy.
> +	* If first copy itself was not written, older version is in copy 2.
> +	* Unmap first copy and call wl_flush.
> +	* Volume creating is unsuccessful.
> +	*/

Please, clean-up the comment. Split lines nicer - you have 79 characters
per line, use the same starting '/*' as other UBI comments do. Just
glance at the other UBI comments. BTW, the commit message has somewhat
unclean line splitting as well.

> +	ubi_err("Error writing volume table copy #%d", copy+1);

UBI prints should not start with capital letters, because the printing
macros add prefixes. Take a look at other UBI prints.

> +	err1 = ubi_eba_unmap_leb(ubi, layout_vol, copy);
> +	if (!err1) {
> +		ubi_wl_flush(ubi);
> +		/* Don't bother about error in flush
> +		 * We are going read only any ways
> +		 */
Please. clean up this comment a little. You might as well just kill it. 

> +	}
> +	ubi_ro_mode(ubi);
> +	ubi_msg("Try detaching and attaching UBI again");

Please, remove this message. The kernel messages should not be used for
suggestions like this. They are not FAQ.

> +	if (copy > 0)
> +		return 0;
> +	else
> +		return err;
This is a tricky place, IMO, and deserves a comment. Could we have
something like:

/*
 * If the first volume table copy has been changed then overall the
 * operation has succeeded, because the change would be there if we now
 * re-attached the UBI device. Thus, return success in this case.
 */

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)