| Message ID | 0ace6202bddb495ae0e632ae2fd0346f99fcdab4.1530169759.git-series.quentin.schulz@bootlin.com |
|---|---|
| State | Accepted |
| Delegated to: | Richard Weinberger |
| Headers | show |
| Series | ubi: add possibility to skip CRC check for static UBI volumes | expand |
Quentin, Am Donnerstag, 28. Juni 2018, 09:40:53 CEST schrieb Quentin Schulz: > Now that we have the logic for skipping CRC check for static UBI volumes > in the core, let's expose it to users. > > This makes use of a padding byte in the volume description data > structure as a flag. This flag only tell for now whether we should skip > the CRC check of a volume. > > This checks the UBI volume for which we are trying to skip the CRC check > is static. > > Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com> > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> > Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com> > --- > drivers/mtd/ubi/cdev.c | 4 ++++ > drivers/mtd/ubi/vmt.c | 3 +++ > include/uapi/mtd/ubi-user.h | 16 ++++++++++++++-- > 3 files changed, 21 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c > index 45c3296..3eea1df 100644 > --- a/drivers/mtd/ubi/cdev.c > +++ b/drivers/mtd/ubi/cdev.c > @@ -622,6 +622,10 @@ static int verify_mkvol_req(const struct ubi_device *ubi, > req->vol_type != UBI_STATIC_VOLUME) > goto bad; > > + if (req->flags & UBI_VOL_SKIP_CRC_CHECK_FLG && > + req->vol_type != UBI_STATIC_VOLUME) > + goto bad; We should also reject unknown flags here. Thanks, //richard
On Sun, 01 Jul 2018 21:35:57 +0200 Richard Weinberger <richard@nod.at> wrote: > Quentin, > > Am Donnerstag, 28. Juni 2018, 09:40:53 CEST schrieb Quentin Schulz: > > Now that we have the logic for skipping CRC check for static UBI volumes > > in the core, let's expose it to users. > > > > This makes use of a padding byte in the volume description data > > structure as a flag. This flag only tell for now whether we should skip > > the CRC check of a volume. > > > > This checks the UBI volume for which we are trying to skip the CRC check > > is static. > > > > Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com> > > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> > > Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com> > > --- > > drivers/mtd/ubi/cdev.c | 4 ++++ > > drivers/mtd/ubi/vmt.c | 3 +++ > > include/uapi/mtd/ubi-user.h | 16 ++++++++++++++-- > > 3 files changed, 21 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c > > index 45c3296..3eea1df 100644 > > --- a/drivers/mtd/ubi/cdev.c > > +++ b/drivers/mtd/ubi/cdev.c > > @@ -622,6 +622,10 @@ static int verify_mkvol_req(const struct ubi_device *ubi, > > req->vol_type != UBI_STATIC_VOLUME) > > goto bad; > > > > + if (req->flags & UBI_VOL_SKIP_CRC_CHECK_FLG && Oops, missed that req->flags & UBI_VOL_SKIP_CRC_CHECK_FLG check was missing parens (checkpatch --strict should complain about that). > > + req->vol_type != UBI_STATIC_VOLUME) > > + goto bad; > > We should also reject unknown flags here. I agree. Talking about missing checks, it seems that none of the padding sections are checked (I hope all mkvol users are zero-ing the struct as requested in ubi-user.h). And we should probably also check that vtbl->flags does not contain unknown flags.
Am Sonntag, 1. Juli 2018, 22:33:47 CEST schrieb Boris Brezillon: > On Sun, 01 Jul 2018 21:35:57 +0200 > Richard Weinberger <richard@nod.at> wrote: > > > Quentin, > > > > Am Donnerstag, 28. Juni 2018, 09:40:53 CEST schrieb Quentin Schulz: > > > Now that we have the logic for skipping CRC check for static UBI volumes > > > in the core, let's expose it to users. > > > > > > This makes use of a padding byte in the volume description data > > > structure as a flag. This flag only tell for now whether we should skip > > > the CRC check of a volume. > > > > > > This checks the UBI volume for which we are trying to skip the CRC check > > > is static. > > > > > > Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com> > > > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> > > > Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com> > > > --- > > > drivers/mtd/ubi/cdev.c | 4 ++++ > > > drivers/mtd/ubi/vmt.c | 3 +++ > > > include/uapi/mtd/ubi-user.h | 16 ++++++++++++++-- > > > 3 files changed, 21 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c > > > index 45c3296..3eea1df 100644 > > > --- a/drivers/mtd/ubi/cdev.c > > > +++ b/drivers/mtd/ubi/cdev.c > > > @@ -622,6 +622,10 @@ static int verify_mkvol_req(const struct ubi_device *ubi, > > > req->vol_type != UBI_STATIC_VOLUME) > > > goto bad; > > > > > > + if (req->flags & UBI_VOL_SKIP_CRC_CHECK_FLG && > > Oops, missed that req->flags & UBI_VOL_SKIP_CRC_CHECK_FLG check was > missing parens (checkpatch --strict should complain about that). Latest when building my local branch or in linux-next we had noticed. No need to worry. > > > + req->vol_type != UBI_STATIC_VOLUME) > > > + goto bad; > > > > We should also reject unknown flags here. > > I agree. Talking about missing checks, it seems that none of the > padding sections are checked (I hope all mkvol users are zero-ing the > struct as requested in ubi-user.h). And we should probably also > check that vtbl->flags does not contain unknown flags. At least mtd-utils does zeros the request struct before using it. So it does the right thing. Adding checks for non-zero padding and more checking for vtbl->flags is a good idea. Thanks, //richard
On Sun, 2018-07-01 at 22:33 +0200, Boris Brezillon wrote: > On Sun, 01 Jul 2018 21:35:57 +0200 Richard Weinberger <richard@nod.at> wrote: > > Am Donnerstag, 28. Juni 2018, 09:40:53 CEST schrieb Quentin Schulz: > > > Now that we have the logic for skipping CRC check for static UBI volumes > > > in the core, let's expose it to users. [] > > > diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c [] > > > @@ -622,6 +622,10 @@ static int verify_mkvol_req(const struct ubi_device *ubi, > > > req->vol_type != UBI_STATIC_VOLUME) > > > goto bad; > > > > > > + if (req->flags & UBI_VOL_SKIP_CRC_CHECK_FLG && > > Oops, missed that req->flags & UBI_VOL_SKIP_CRC_CHECK_FLG check was > missing parens (checkpatch --strict should complain about that). Why should checkpatch complain? & has higher precedence than &&.
Am Sonntag, 1. Juli 2018, 22:54:32 CEST schrieb Joe Perches: > On Sun, 2018-07-01 at 22:33 +0200, Boris Brezillon wrote: > > On Sun, 01 Jul 2018 21:35:57 +0200 Richard Weinberger <richard@nod.at> wrote: > > > Am Donnerstag, 28. Juni 2018, 09:40:53 CEST schrieb Quentin Schulz: > > > > Now that we have the logic for skipping CRC check for static UBI volumes > > > > in the core, let's expose it to users. > [] > > > > diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c > [] > > > > @@ -622,6 +622,10 @@ static int verify_mkvol_req(const struct ubi_device *ubi, > > > > req->vol_type != UBI_STATIC_VOLUME) > > > > goto bad; > > > > > > > > + if (req->flags & UBI_VOL_SKIP_CRC_CHECK_FLG && > > > > Oops, missed that req->flags & UBI_VOL_SKIP_CRC_CHECK_FLG check was > > missing parens (checkpatch --strict should complain about that). > > Why should checkpatch complain? > & has higher precedence than &&. The code is more readable. Thanks, //richard
On Sun, 2018-07-01 at 23:01 +0200, Richard Weinberger wrote: > Am Sonntag, 1. Juli 2018, 22:54:32 CEST schrieb Joe Perches: > > On Sun, 2018-07-01 at 22:33 +0200, Boris Brezillon wrote: > > > On Sun, 01 Jul 2018 21:35:57 +0200 Richard Weinberger <richard@nod.at> wrote: > > > > Am Donnerstag, 28. Juni 2018, 09:40:53 CEST schrieb Quentin Schulz: > > > > > Now that we have the logic for skipping CRC check for static UBI volumes > > > > > in the core, let's expose it to users. > > [] > > > > > diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c > > [] > > > > > @@ -622,6 +622,10 @@ static int verify_mkvol_req(const struct ubi_device *ubi, > > > > > req->vol_type != UBI_STATIC_VOLUME) > > > > > goto bad; > > > > > > > > > > + if (req->flags & UBI_VOL_SKIP_CRC_CHECK_FLG && > > > > > > Oops, missed that req->flags & UBI_VOL_SKIP_CRC_CHECK_FLG check was > > > missing parens (checkpatch --strict should complain about that). > > > > Why should checkpatch complain? > > & has higher precedence than &&. > > The code is more readable. IYO. checkpatch doesn't care and I think it's unnecessary. Just fyi: checkpatch does suggest parenthesis removal with --strict when using == or != with && or || e.g.: $ cat -n foo.c 1 bool function(void) 2 { 3 if (foo & 1 && bar & 2) 4 return true; 5 if ((foo & 1) && (bar && 2)) 6 return true; 7 if (foo == 1 && bar != 2) 8 return true; 9 if ((foo == 1) && (bar != 2)) 10 return true; 11 return false; 12 } $ ./scripts/checkpatch.pl -f --strict foo.c WARNING: Missing or malformed SPDX-License-Identifier tag in line 1 #1: FILE: foo.c:1: +bool function(void) CHECK: Unnecessary parentheses around 'foo == 1' #9: FILE: foo.c:9: + if ((foo == 1) && (bar != 2)) CHECK: Unnecessary parentheses around 'bar != 2' #9: FILE: foo.c:9: + if ((foo == 1) && (bar != 2)) total: 0 errors, 1 warnings, 2 checks, 12 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. foo.c has style problems, please review. NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS.
Hi Richard, Boris, On Sun, Jul 01, 2018 at 10:50:41PM +0200, Richard Weinberger wrote: > Am Sonntag, 1. Juli 2018, 22:33:47 CEST schrieb Boris Brezillon: > > On Sun, 01 Jul 2018 21:35:57 +0200 > > Richard Weinberger <richard@nod.at> wrote: > > > > > Quentin, > > > > > > Am Donnerstag, 28. Juni 2018, 09:40:53 CEST schrieb Quentin Schulz: > > > > Now that we have the logic for skipping CRC check for static UBI volumes > > > > in the core, let's expose it to users. > > > > > > > > This makes use of a padding byte in the volume description data > > > > structure as a flag. This flag only tell for now whether we should skip > > > > the CRC check of a volume. > > > > > > > > This checks the UBI volume for which we are trying to skip the CRC check > > > > is static. > > > > > > > > Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com> > > > > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> > > > > Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com> > > > > --- > > > > drivers/mtd/ubi/cdev.c | 4 ++++ > > > > drivers/mtd/ubi/vmt.c | 3 +++ > > > > include/uapi/mtd/ubi-user.h | 16 ++++++++++++++-- > > > > 3 files changed, 21 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c > > > > index 45c3296..3eea1df 100644 > > > > --- a/drivers/mtd/ubi/cdev.c > > > > +++ b/drivers/mtd/ubi/cdev.c > > > > @@ -622,6 +622,10 @@ static int verify_mkvol_req(const struct ubi_device *ubi, > > > > req->vol_type != UBI_STATIC_VOLUME) > > > > goto bad; > > > > > > > > + if (req->flags & UBI_VOL_SKIP_CRC_CHECK_FLG && > > > > Oops, missed that req->flags & UBI_VOL_SKIP_CRC_CHECK_FLG check was > > missing parens (checkpatch --strict should complain about that). > > Latest when building my local branch or in linux-next we had noticed. > No need to worry. > > > > > + req->vol_type != UBI_STATIC_VOLUME) > > > > + goto bad; > > > > > > We should also reject unknown flags here. > > > > I agree. Should I send another version of my patches for it? Same for parenthesis around the flags masking above? Quentin
On Sun, 01 Jul 2018 13:54:32 -0700 Joe Perches <joe@perches.com> wrote: > On Sun, 2018-07-01 at 22:33 +0200, Boris Brezillon wrote: > > On Sun, 01 Jul 2018 21:35:57 +0200 Richard Weinberger <richard@nod.at> wrote: > > > Am Donnerstag, 28. Juni 2018, 09:40:53 CEST schrieb Quentin Schulz: > > > > Now that we have the logic for skipping CRC check for static UBI volumes > > > > in the core, let's expose it to users. > [] > > > > diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c > [] > > > > @@ -622,6 +622,10 @@ static int verify_mkvol_req(const struct ubi_device *ubi, > > > > req->vol_type != UBI_STATIC_VOLUME) > > > > goto bad; > > > > > > > > + if (req->flags & UBI_VOL_SKIP_CRC_CHECK_FLG && > > > > Oops, missed that req->flags & UBI_VOL_SKIP_CRC_CHECK_FLG check was > > missing parens (checkpatch --strict should complain about that). > > Why should checkpatch complain? > & has higher precedence than &&. > Yes, I know, but I remember checkpatch complaining about that in one of my patch (maybe it was a slightly different case though). Anyway, I double checked and, as you report, checkpatch does not complain, so please ignore this comment (sorry for the noise).
On Mon, 2 Jul 2018 08:44:33 +0200 Quentin Schulz <quentin.schulz@bootlin.com> wrote: > Hi Richard, Boris, > > On Sun, Jul 01, 2018 at 10:50:41PM +0200, Richard Weinberger wrote: > > Am Sonntag, 1. Juli 2018, 22:33:47 CEST schrieb Boris Brezillon: > > > On Sun, 01 Jul 2018 21:35:57 +0200 > > > Richard Weinberger <richard@nod.at> wrote: > > > > > > > Quentin, > > > > > > > > Am Donnerstag, 28. Juni 2018, 09:40:53 CEST schrieb Quentin Schulz: > > > > > Now that we have the logic for skipping CRC check for static UBI volumes > > > > > in the core, let's expose it to users. > > > > > > > > > > This makes use of a padding byte in the volume description data > > > > > structure as a flag. This flag only tell for now whether we should skip > > > > > the CRC check of a volume. > > > > > > > > > > This checks the UBI volume for which we are trying to skip the CRC check > > > > > is static. > > > > > > > > > > Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com> > > > > > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> > > > > > Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com> > > > > > --- > > > > > drivers/mtd/ubi/cdev.c | 4 ++++ > > > > > drivers/mtd/ubi/vmt.c | 3 +++ > > > > > include/uapi/mtd/ubi-user.h | 16 ++++++++++++++-- > > > > > 3 files changed, 21 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c > > > > > index 45c3296..3eea1df 100644 > > > > > --- a/drivers/mtd/ubi/cdev.c > > > > > +++ b/drivers/mtd/ubi/cdev.c > > > > > @@ -622,6 +622,10 @@ static int verify_mkvol_req(const struct ubi_device *ubi, > > > > > req->vol_type != UBI_STATIC_VOLUME) > > > > > goto bad; > > > > > > > > > > + if (req->flags & UBI_VOL_SKIP_CRC_CHECK_FLG && > > > > > > Oops, missed that req->flags & UBI_VOL_SKIP_CRC_CHECK_FLG check was > > > missing parens (checkpatch --strict should complain about that). > > > > Latest when building my local branch or in linux-next we had noticed. > > No need to worry. > > > > > > > + req->vol_type != UBI_STATIC_VOLUME) > > > > > + goto bad; > > > > > > > > We should also reject unknown flags here. > > > > > > I agree. > > Should I send another version of my patches for it? Yes please, respin your series with this additional check. Just define #define UBI_VOL_VALID_FLGS (UBI_VOL_SKIP_CRC_CHECK_FLG) and then, in verify_mkvol_req() add if (req->flags & ~UBI_VOL_VALID_FLGS) goto bad; > Same for > parenthesis around the flags masking above? No need to fix that one (unless Richard cares), as it seems I had it wrong.
Am Montag, 2. Juli 2018, 08:52:27 CEST schrieb Boris Brezillon: > On Mon, 2 Jul 2018 08:44:33 +0200 > Quentin Schulz <quentin.schulz@bootlin.com> wrote: > > > Hi Richard, Boris, > > > > On Sun, Jul 01, 2018 at 10:50:41PM +0200, Richard Weinberger wrote: > > > Am Sonntag, 1. Juli 2018, 22:33:47 CEST schrieb Boris Brezillon: > > > > On Sun, 01 Jul 2018 21:35:57 +0200 > > > > Richard Weinberger <richard@nod.at> wrote: > > > > > > > > > Quentin, > > > > > > > > > > Am Donnerstag, 28. Juni 2018, 09:40:53 CEST schrieb Quentin Schulz: > > > > > > Now that we have the logic for skipping CRC check for static UBI volumes > > > > > > in the core, let's expose it to users. > > > > > > > > > > > > This makes use of a padding byte in the volume description data > > > > > > structure as a flag. This flag only tell for now whether we should skip > > > > > > the CRC check of a volume. > > > > > > > > > > > > This checks the UBI volume for which we are trying to skip the CRC check > > > > > > is static. > > > > > > > > > > > > Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com> > > > > > > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> > > > > > > Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com> > > > > > > --- > > > > > > drivers/mtd/ubi/cdev.c | 4 ++++ > > > > > > drivers/mtd/ubi/vmt.c | 3 +++ > > > > > > include/uapi/mtd/ubi-user.h | 16 ++++++++++++++-- > > > > > > 3 files changed, 21 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c > > > > > > index 45c3296..3eea1df 100644 > > > > > > --- a/drivers/mtd/ubi/cdev.c > > > > > > +++ b/drivers/mtd/ubi/cdev.c > > > > > > @@ -622,6 +622,10 @@ static int verify_mkvol_req(const struct ubi_device *ubi, > > > > > > req->vol_type != UBI_STATIC_VOLUME) > > > > > > goto bad; > > > > > > > > > > > > + if (req->flags & UBI_VOL_SKIP_CRC_CHECK_FLG && > > > > > > > > Oops, missed that req->flags & UBI_VOL_SKIP_CRC_CHECK_FLG check was > > > > missing parens (checkpatch --strict should complain about that). > > > > > > Latest when building my local branch or in linux-next we had noticed. > > > No need to worry. > > > > > > > > > + req->vol_type != UBI_STATIC_VOLUME) > > > > > > + goto bad; > > > > > > > > > > We should also reject unknown flags here. > > > > > > > > I agree. > > > > Should I send another version of my patches for it? Yes. Please. > Yes please, respin your series with this additional check. Just define > > #define UBI_VOL_VALID_FLGS (UBI_VOL_SKIP_CRC_CHECK_FLG) > > and then, in verify_mkvol_req() add > > if (req->flags & ~UBI_VOL_VALID_FLGS) > goto bad; Yep. > > Same for > > parenthesis around the flags masking above? > > No need to fix that one (unless Richard cares), as it seems I had it > wrong. Nah. If both gcc and checkpatch don't complain, let's keep it as-is. Thanks, //richard
diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c index 45c3296..3eea1df 100644 --- a/drivers/mtd/ubi/cdev.c +++ b/drivers/mtd/ubi/cdev.c @@ -622,6 +622,10 @@ static int verify_mkvol_req(const struct ubi_device *ubi, req->vol_type != UBI_STATIC_VOLUME) goto bad; + if (req->flags & UBI_VOL_SKIP_CRC_CHECK_FLG && + req->vol_type != UBI_STATIC_VOLUME) + goto bad; + if (req->alignment > ubi->leb_size) goto bad; diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c index e2606a4..729588b 100644 --- a/drivers/mtd/ubi/vmt.c +++ b/drivers/mtd/ubi/vmt.c @@ -174,6 +174,9 @@ int ubi_create_volume(struct ubi_device *ubi, struct ubi_mkvol_req *req) vol->dev.class = &ubi_class; vol->dev.groups = volume_dev_groups; + if (req->flags & UBI_VOL_SKIP_CRC_CHECK_FLG) + vol->skip_check = 1; + spin_lock(&ubi->volumes_lock); if (vol_id == UBI_VOL_NUM_AUTO) { /* Find unused volume ID */ diff --git a/include/uapi/mtd/ubi-user.h b/include/uapi/mtd/ubi-user.h index 5b04a49..6e5ded5 100644 --- a/include/uapi/mtd/ubi-user.h +++ b/include/uapi/mtd/ubi-user.h @@ -285,6 +285,18 @@ struct ubi_attach_req { __s8 padding[10]; }; +/* + * UBI volume flags. + * + * @UBI_VOL_SKIP_CRC_CHECK_FLG: skip the CRC check done on a static volume at + * open time. Only valid for static volumes and + * should only be used if the volume user has a + * way to verify data integrity + */ +enum { + UBI_VOL_SKIP_CRC_CHECK_FLG = 0x1, +}; + /** * struct ubi_mkvol_req - volume description data structure used in * volume creation requests. @@ -292,7 +304,7 @@ struct ubi_attach_req { * @alignment: volume alignment * @bytes: volume size in bytes * @vol_type: volume type (%UBI_DYNAMIC_VOLUME or %UBI_STATIC_VOLUME) - * @padding1: reserved for future, not used, has to be zeroed + * @flags: volume flags (%UBI_VOL_SKIP_CRC_CHECK_FLG) * @name_len: volume name length * @padding2: reserved for future, not used, has to be zeroed * @name: volume name @@ -321,7 +333,7 @@ struct ubi_mkvol_req { __s32 alignment; __s64 bytes; __s8 vol_type; - __s8 padding1; + __u8 flags; __s16 name_len; __s8 padding2[4]; char name[UBI_MAX_VOLUME_NAME + 1];