[PATCH] [UBI] Volume table update fix
Artem Bityutskiy
dedekind at infradead.org
Mon Jun 22 03:29:56 EDT 2009
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 (Битюцкий Артём)
More information about the linux-mtd
mailing list