2019-08-21 00:20:04

by Caitlyn Finn

Subject: [PATCH 0/2] Submitting my first patch series (Checkpatch fixes)

Hello!

This patch series cleans up some checkpatch fixes in erofs. The patches
include balancing conditional braces and fixing some indentation. No testing
done, all patches build and checkpath cleanly.

Caitlyn (2):
staging/erofs/xattr.h: Fixed misaligned function arguments.
staging/erofs: Balanced braces around a few conditional statements.

drivers/staging/erofs/inode.c | 4 ++--
drivers/staging/erofs/unzip_vle.c | 12 ++++++------
drivers/staging/erofs/xattr.h | 6 +++---
3 files changed, 11 insertions(+), 11 deletions(-)

--
2.7.4

2019-08-21 00:20:45

by Caitlyn Finn

Subject: [PATCH 2/2] staging/erofs: Balanced braces around a few conditional statements.

Balanced braces to fix some checkpath warnings in inode.c and
unzip_vle.c

Signed-off-by: Caitlyn <[email protected]>
---
drivers/staging/erofs/inode.c | 4 ++--
drivers/staging/erofs/unzip_vle.c | 12 ++++++------
2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/erofs/inode.c b/drivers/staging/erofs/inode.c
index 4c3d8bf..8de6fcd 100644
--- a/drivers/staging/erofs/inode.c
+++ b/drivers/staging/erofs/inode.c
@@ -278,9 +278,9 @@ struct inode *erofs_iget(struct super_block *sb,
vi->nid = nid;

err = fill_inode(inode, isdir);
- if (likely(!err))
+ if (likely(!err)) {
unlock_new_inode(inode);
- else {
+ } else {
iget_failed(inode);
inode = ERR_PTR(err);
}
diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
index f0dab81..f431614 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -915,21 +915,21 @@ static int z_erofs_vle_unzip(struct super_block *sb,
mutex_lock(&work->lock);
nr_pages = work->nr_pages;

- if (likely(nr_pages <= Z_EROFS_VLE_VMAP_ONSTACK_PAGES))
+ if (likely(nr_pages <= Z_EROFS_VLE_VMAP_ONSTACK_PAGES)) {
pages = pages_onstack;
- else if (nr_pages <= Z_EROFS_VLE_VMAP_GLOBAL_PAGES &&
- mutex_trylock(&z_pagemap_global_lock))
+ } else if (nr_pages <= Z_EROFS_VLE_VMAP_GLOBAL_PAGES &&
+ mutex_trylock(&z_pagemap_global_lock)) {
pages = z_pagemap_global;
- else {
+ } else {
repeat:
pages = kvmalloc_array(nr_pages, sizeof(struct page *),
GFP_KERNEL);

/* fallback to global pagemap for the lowmem scenario */
if (unlikely(!pages)) {
- if (nr_pages > Z_EROFS_VLE_VMAP_GLOBAL_PAGES)
+ if (nr_pages > Z_EROFS_VLE_VMAP_GLOBAL_PAGES) {
goto repeat;
- else {
+ } else {
mutex_lock(&z_pagemap_global_lock);
pages = z_pagemap_global;
}
--
2.7.4

2019-08-21 00:43:34

by Gao Xiang

Subject: Re: [PATCH 2/2] staging/erofs: Balanced braces around a few conditional statements.

On Tue, Aug 20, 2019 at 08:18:20PM -0400, Caitlyn wrote:
> Balanced braces to fix some checkpath warnings in inode.c and
> unzip_vle.c
>
> Signed-off-by: Caitlyn <[email protected]>
> ---
> drivers/staging/erofs/inode.c | 4 ++--
> drivers/staging/erofs/unzip_vle.c | 12 ++++++------
> 2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/erofs/inode.c b/drivers/staging/erofs/inode.c
> index 4c3d8bf..8de6fcd 100644
> --- a/drivers/staging/erofs/inode.c
> +++ b/drivers/staging/erofs/inode.c
> @@ -278,9 +278,9 @@ struct inode *erofs_iget(struct super_block *sb,
> vi->nid = nid;
>
> err = fill_inode(inode, isdir);
> - if (likely(!err))
> + if (likely(!err)) {
> unlock_new_inode(inode);

The only valid place is here.

Thanks,
Gao Xiang

> - else {
> + } else {
> iget_failed(inode);
> inode = ERR_PTR(err);
> }
> diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
> index f0dab81..f431614 100644
> --- a/drivers/staging/erofs/unzip_vle.c
> +++ b/drivers/staging/erofs/unzip_vle.c
> @@ -915,21 +915,21 @@ static int z_erofs_vle_unzip(struct super_block *sb,
> mutex_lock(&work->lock);
> nr_pages = work->nr_pages;
>
> - if (likely(nr_pages <= Z_EROFS_VLE_VMAP_ONSTACK_PAGES))
> + if (likely(nr_pages <= Z_EROFS_VLE_VMAP_ONSTACK_PAGES)) {
> pages = pages_onstack;
> - else if (nr_pages <= Z_EROFS_VLE_VMAP_GLOBAL_PAGES &&
> - mutex_trylock(&z_pagemap_global_lock))
> + } else if (nr_pages <= Z_EROFS_VLE_VMAP_GLOBAL_PAGES &&
> + mutex_trylock(&z_pagemap_global_lock)) {
> pages = z_pagemap_global;
> - else {
> + } else {
> repeat:
> pages = kvmalloc_array(nr_pages, sizeof(struct page *),
> GFP_KERNEL);
>
> /* fallback to global pagemap for the lowmem scenario */
> if (unlikely(!pages)) {
> - if (nr_pages > Z_EROFS_VLE_VMAP_GLOBAL_PAGES)
> + if (nr_pages > Z_EROFS_VLE_VMAP_GLOBAL_PAGES) {
> goto repeat;
> - else {
> + } else {
> mutex_lock(&z_pagemap_global_lock);
> pages = z_pagemap_global;
> }
> --
> 2.7.4
>
> _______________________________________________
> devel mailing list
> [email protected]
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

2019-08-21 00:53:14

by Gao Xiang

Subject: Re: [PATCH 0/2] Submitting my first patch series (Checkpatch fixes)

Hi Caitlyn,

On Tue, Aug 20, 2019 at 08:18:18PM -0400, Caitlyn wrote:
> Hello!
>
> This patch series cleans up some checkpatch fixes in erofs. The patches
> include balancing conditional braces and fixing some indentation. No testing
> done, all patches build and checkpath cleanly.

I think you need to work on the latest staging tree or linux-next.
This patchset cannot be applied (there is the only valid place in inode.c,
I will reply in the following patch.)

Thanks,
Gao Xiang

>
> Caitlyn (2):
> staging/erofs/xattr.h: Fixed misaligned function arguments.
> staging/erofs: Balanced braces around a few conditional statements.
>
> drivers/staging/erofs/inode.c | 4 ++--
> drivers/staging/erofs/unzip_vle.c | 12 ++++++------
> drivers/staging/erofs/xattr.h | 6 +++---
> 3 files changed, 11 insertions(+), 11 deletions(-)
>
> --
> 2.7.4
>
> _______________________________________________
> devel mailing list
> [email protected]
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

2019-08-21 02:29:50

by Joe Perches

Subject: Re: [PATCH 2/2] staging/erofs: Balanced braces around a few conditional statements.

On Tue, 2019-08-20 at 20:18 -0400, Caitlyn wrote:
> Balanced braces to fix some checkpath warnings in inode.c and
> unzip_vle.c
[]
> diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
[]
> @@ -915,21 +915,21 @@ static int z_erofs_vle_unzip(struct super_block *sb,
> mutex_lock(&work->lock);
> nr_pages = work->nr_pages;
>
> - if (likely(nr_pages <= Z_EROFS_VLE_VMAP_ONSTACK_PAGES))
> + if (likely(nr_pages <= Z_EROFS_VLE_VMAP_ONSTACK_PAGES)) {
> pages = pages_onstack;
> - else if (nr_pages <= Z_EROFS_VLE_VMAP_GLOBAL_PAGES &&
> - mutex_trylock(&z_pagemap_global_lock))
> + } else if (nr_pages <= Z_EROFS_VLE_VMAP_GLOBAL_PAGES &&
> + mutex_trylock(&z_pagemap_global_lock)) {

Extra space after tab

> pages = z_pagemap_global;
> - else {
> + } else {
> repeat:
> pages = kvmalloc_array(nr_pages, sizeof(struct page *),
> GFP_KERNEL);
>
> /* fallback to global pagemap for the lowmem scenario */
> if (unlikely(!pages)) {
> - if (nr_pages > Z_EROFS_VLE_VMAP_GLOBAL_PAGES)
> + if (nr_pages > Z_EROFS_VLE_VMAP_GLOBAL_PAGES) {
> goto repeat;
> - else {
> + } else {

Unnecessary else

> mutex_lock(&z_pagemap_global_lock);
> pages = z_pagemap_global;
> }

2019-08-21 02:58:17

by Gao Xiang

Subject: Re: [PATCH 2/2] staging/erofs: Balanced braces around a few conditional statements.

On Tue, Aug 20, 2019 at 07:26:46PM -0700, Joe Perches wrote:
> On Tue, 2019-08-20 at 20:18 -0400, Caitlyn wrote:
> > Balanced braces to fix some checkpath warnings in inode.c and
> > unzip_vle.c
> []
> > diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
> []
> > @@ -915,21 +915,21 @@ static int z_erofs_vle_unzip(struct super_block *sb,
> > mutex_lock(&work->lock);
> > nr_pages = work->nr_pages;
> >
> > - if (likely(nr_pages <= Z_EROFS_VLE_VMAP_ONSTACK_PAGES))
> > + if (likely(nr_pages <= Z_EROFS_VLE_VMAP_ONSTACK_PAGES)) {
> > pages = pages_onstack;
> > - else if (nr_pages <= Z_EROFS_VLE_VMAP_GLOBAL_PAGES &&
> > - mutex_trylock(&z_pagemap_global_lock))
> > + } else if (nr_pages <= Z_EROFS_VLE_VMAP_GLOBAL_PAGES &&
> > + mutex_trylock(&z_pagemap_global_lock)) {
>
> Extra space after tab

There is actually balanced braces in linux-next.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/staging/erofs/zdata.c#n762

>
> > pages = z_pagemap_global;
> > - else {
> > + } else {
> > repeat:
> > pages = kvmalloc_array(nr_pages, sizeof(struct page *),
> > GFP_KERNEL);
> >
> > /* fallback to global pagemap for the lowmem scenario */
> > if (unlikely(!pages)) {
> > - if (nr_pages > Z_EROFS_VLE_VMAP_GLOBAL_PAGES)
> > + if (nr_pages > Z_EROFS_VLE_VMAP_GLOBAL_PAGES) {
> > goto repeat;
> > - else {
> > + } else {
>
> Unnecessary else

There is not the "goto repeat" in linux-next anymore.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/staging/erofs/zdata.c#n765

Thanks,
Gao Xiang

>
> > mutex_lock(&z_pagemap_global_lock);
> > pages = z_pagemap_global;
> > }
>
>

2019-08-21 15:44:48

by Tobin C. Harding

Subject: Re: [PATCH 2/2] staging/erofs: Balanced braces around a few conditional statements.

On Wed, Aug 21, 2019 at 10:31:22AM +0800, Gao Xiang wrote:
> On Tue, Aug 20, 2019 at 07:26:46PM -0700, Joe Perches wrote:
> > On Tue, 2019-08-20 at 20:18 -0400, Caitlyn wrote:
> > > Balanced braces to fix some checkpath warnings in inode.c and
> > > unzip_vle.c
> > []
> > > diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
> > []
> > > @@ -915,21 +915,21 @@ static int z_erofs_vle_unzip(struct super_block *sb,
> > > mutex_lock(&work->lock);
> > > nr_pages = work->nr_pages;
> > >
> > > - if (likely(nr_pages <= Z_EROFS_VLE_VMAP_ONSTACK_PAGES))
> > > + if (likely(nr_pages <= Z_EROFS_VLE_VMAP_ONSTACK_PAGES)) {
> > > pages = pages_onstack;
> > > - else if (nr_pages <= Z_EROFS_VLE_VMAP_GLOBAL_PAGES &&
> > > - mutex_trylock(&z_pagemap_global_lock))
> > > + } else if (nr_pages <= Z_EROFS_VLE_VMAP_GLOBAL_PAGES &&
> > > + mutex_trylock(&z_pagemap_global_lock)) {
> >
> > Extra space after tab
>
> There is actually balanced braces in linux-next.
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/staging/erofs/zdata.c#n762

Which tree did these changes go in through please Gao? I believe
Caitlyn was working off of the staging-next branch of Greg's staging
tree.

thanks,
Tobin.

2019-08-21 15:55:56

by Gao Xiang

Subject: Re: [PATCH 2/2] staging/erofs: Balanced braces around a few conditional statements.

Hi Tobin,

On Wed, Aug 21, 2019 at 08:13:35AM -0700, Tobin C. Harding wrote:
> On Wed, Aug 21, 2019 at 10:31:22AM +0800, Gao Xiang wrote:
> > On Tue, Aug 20, 2019 at 07:26:46PM -0700, Joe Perches wrote:
> > > On Tue, 2019-08-20 at 20:18 -0400, Caitlyn wrote:
> > > > Balanced braces to fix some checkpath warnings in inode.c and
> > > > unzip_vle.c
> > > []
> > > > diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
> > > []
> > > > @@ -915,21 +915,21 @@ static int z_erofs_vle_unzip(struct super_block *sb,
> > > > mutex_lock(&work->lock);
> > > > nr_pages = work->nr_pages;
> > > >
> > > > - if (likely(nr_pages <= Z_EROFS_VLE_VMAP_ONSTACK_PAGES))
> > > > + if (likely(nr_pages <= Z_EROFS_VLE_VMAP_ONSTACK_PAGES)) {
> > > > pages = pages_onstack;
> > > > - else if (nr_pages <= Z_EROFS_VLE_VMAP_GLOBAL_PAGES &&
> > > > - mutex_trylock(&z_pagemap_global_lock))
> > > > + } else if (nr_pages <= Z_EROFS_VLE_VMAP_GLOBAL_PAGES &&
> > > > + mutex_trylock(&z_pagemap_global_lock)) {
> > >
> > > Extra space after tab
> >
> > There is actually balanced braces in linux-next.
> > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/staging/erofs/zdata.c#n762
>
> Which tree did these changes go in through please Gao? I believe
> Caitlyn was working off of the staging-next branch of Greg's staging
> tree.

I don't think so, the reason is that unzip_vle.c was renamed to zdata.c
months ago, see:
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/tree/drivers/staging/erofs?h=staging-next

so I think the patch is outdated when I first look at it.

Thanks,
Gao Xiang

>
> thanks,
> Tobin.

2019-08-21 20:14:42

by Caitlyn Finn

Subject: Re: [PATCH 2/2] staging/erofs: Balanced braces around a few conditional statements.

On Wed, Aug 21, 2019 at 11:52 AM Gao Xiang <[email protected]> wrote:
>
> Hi Tobin,
>
> On Wed, Aug 21, 2019 at 08:13:35AM -0700, Tobin C. Harding wrote:
> > On Wed, Aug 21, 2019 at 10:31:22AM +0800, Gao Xiang wrote:
> > > On Tue, Aug 20, 2019 at 07:26:46PM -0700, Joe Perches wrote:
> > > > On Tue, 2019-08-20 at 20:18 -0400, Caitlyn wrote:
> > > > > Balanced braces to fix some checkpath warnings in inode.c and
> > > > > unzip_vle.c
> > > > []
> > > > > diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
> > > > []
> > > > > @@ -915,21 +915,21 @@ static int z_erofs_vle_unzip(struct super_block *sb,
> > > > > mutex_lock(&work->lock);
> > > > > nr_pages = work->nr_pages;
> > > > >
> > > > > - if (likely(nr_pages <= Z_EROFS_VLE_VMAP_ONSTACK_PAGES))
> > > > > + if (likely(nr_pages <= Z_EROFS_VLE_VMAP_ONSTACK_PAGES)) {
> > > > > pages = pages_onstack;
> > > > > - else if (nr_pages <= Z_EROFS_VLE_VMAP_GLOBAL_PAGES &&
> > > > > - mutex_trylock(&z_pagemap_global_lock))
> > > > > + } else if (nr_pages <= Z_EROFS_VLE_VMAP_GLOBAL_PAGES &&
> > > > > + mutex_trylock(&z_pagemap_global_lock)) {
> > > >
> > > > Extra space after tab
> > >
> > > There is actually balanced braces in linux-next.
> > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/staging/erofs/zdata.c#n762
> >
> > Which tree did these changes go in through please Gao? I believe
> > Caitlyn was working off of the staging-next branch of Greg's staging
> > tree.
>
> I don't think so, the reason is that unzip_vle.c was renamed to zdata.c
> months ago, see:
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/tree/drivers/staging/erofs?h=staging-next
>
> so I think the patch is outdated when I first look at it.
>
> Thanks,
> Gao Xiang

Gao,

I see now that I was on an outdated revision (Linux 5.3-rc4) of the
staging-next branch of Greg's staging tree, from
before that change was merged. I'll be certain that I'm fully
up-to-date before submitting future patches.

Thanks all for your time and assistance, and Gao and Joe for the
review comments as well, I'll review and submit
an appropriate patch series at a later time.

Thanks,
Caitlyn Finn