[PATCH v3] Task2: checkpatch.pl documentation mentorship: Remove semicolon warnings

Lukas Bulwahn lukas.bulwahn at gmail.com
Tue Aug 10 14:10:43 UTC 2021


On Tue, Aug 10, 2021 at 3:44 PM Rahul Balaji <rahulbalaji78 at gmail.com> wrote:
>
> Signed-off-by: Rahul Balaji <rahulbalaji78 at gmail.com>
>
> no functional changes.
>
> Here are explanations for the changes I made.
> The changelog is presented down below after the explanation.
>
> drivers/mtd/ubi/block.c:408: WARNING:ONE_SEMICOLON: Statements
> terminations use 1 semicolon
>
> removed the extra semicolon that caused the warning.
> No functional change.
> true positive.
> file showed no errors on make.
>
> checkpatch.pl on this file after change :
>
> WARNING: quoted string split across lines
> #354: FILE: drivers/mtd/ubi/block.c:354:
> + pr_warn("UBI: block: volume size is not a multiple of 512, "
> + "last %llu bytes are ignored!\n",
>
> WARNING: braces {} are not necessary for single statement blocks
> #374: FILE: drivers/mtd/ubi/block.c:374:
> + if (ret) {
> + return ret;
> + }
>
> total: 0 errors, 2 warnings, 697 lines checked
>
> drivers/gpu/drm/nouveau/nouveau_gem.c:342: WARNING:ONE_SEMICOLON:
> Statements terminations use 1 semicolon
>
> removed the extra semicolon that caused the warning.
> no functional change.
> true positive.
> file showed no errors on make.
>
> checkpatch.pl on this file after change :
>
> WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
> #1: FILE: drivers/gpu/drm/nouveau/nouveau_gem.c:1:
> +/*
>
> ERROR: do not use assignment in if condition
> #165: FILE: drivers/gpu/drm/nouveau/nouveau_gem.c:165:
> + if (!(work = kmalloc(sizeof(*work), GFP_KERNEL))) {
>
> ERROR: space prohibited after that '&' (ctx:WxW)
> #183: FILE: drivers/gpu/drm/nouveau/nouveau_gem.c:183:
> + struct nouveau_vmm *vmm = cli->svm.cli ? &cli->svm : & cli->vmm;
>                                                       ^
>
> WARNING: Block comments use a trailing */ on a separate line
> #241: FILE: drivers/gpu/drm/nouveau/nouveau_gem.c:241:
> + * to the caller, instead of a normal nouveau_bo ttm reference. */
>
> WARNING: suspect code indent for conditional statements (8, 8)
> #298: FILE: drivers/gpu/drm/nouveau/nouveau_gem.c:298:
> + else
> + if (cli->device.info.family >= NV_DEVICE_INFO_V0_TESLA)
>
> WARNING: quoted string split across lines
> #458: FILE: drivers/gpu/drm/nouveau/nouveau_gem.c:458:
> + NV_PRINTK(err, cli, "multiple instances of buffer %d on "
> +      "validation list\n", b->handle);
>
> WARNING: Missing a blank line after declarations
> #486: FILE: drivers/gpu/drm/nouveau/nouveau_gem.c:486:
> + struct nouveau_vma *vma = nouveau_vma_find(nvbo, vmm);
> + if (!vma) {
>
> WARNING: suspect code indent for conditional statements (16, 16)
> #502: FILE: drivers/gpu/drm/nouveau/nouveau_gem.c:502:
> + else
> + if (b->valid_domains & NOUVEAU_GEM_DOMAIN_VRAM)
>
> WARNING: suspect code indent for conditional statements (16, 16)
> #505: FILE: drivers/gpu/drm/nouveau/nouveau_gem.c:505:
> + else
> + if (b->valid_domains & NOUVEAU_GEM_DOMAIN_GART)
>
> WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
> #625: FILE: drivers/gpu/drm/nouveau/nouveau_gem.c:625:
> +u_memcpya(uint64_t user, unsigned nmemb, unsigned size)
>
> WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
> #625: FILE: drivers/gpu/drm/nouveau/nouveau_gem.c:625:
> +u_memcpya(uint64_t user, unsigned nmemb, unsigned size)
>
> WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
> #651: FILE: drivers/gpu/drm/nouveau/nouveau_gem.c:651:
> + unsigned i;
>
> WARNING: suspect code indent for conditional statements (16, 16)
> #695: FILE: drivers/gpu/drm/nouveau/nouveau_gem.c:695:
> + else
> + if (r->flags & NOUVEAU_GEM_RELOC_HIGH)
>
> WARNING: suspect code indent for conditional statements (8, 8)
> #841: FILE: drivers/gpu/drm/nouveau/nouveau_gem.c:841:
> + } else
> + if (drm->client.device.info.chipset >= 0x25) {
>
> WARNING: Avoid multiple line dereference - prefer 'nvbo->bo.resource->num_pages'
> #874: FILE: drivers/gpu/drm/nouveau/nouveau_gem.c:874:
> +  nvbo->bo.resource->
> +  num_pages,
>
> ERROR: do not use assignment in if condition
> #902: FILE: drivers/gpu/drm/nouveau/nouveau_gem.c:902:
> + if (!(ret = nouveau_fence_wait(fence, false, false))) {
>
> ERROR: do not use assignment in if condition
> #903: FILE: drivers/gpu/drm/nouveau/nouveau_gem.c:903:
> + if ((ret = dma_fence_get_status(&fence->base)) == 1)
>
> WARNING: suspect code indent for conditional statements (8, 8)
> #937: FILE: drivers/gpu/drm/nouveau/nouveau_gem.c:937:
> + } else
> + if (drm->client.device.info.chipset >= 0x25) {
>
> total: 4 errors, 14 warnings, 1016 lines checked
>
> fs/block_dev.c:1271: WARNING:ONE_SEMICOLON: Statements terminations
> use 1 semicolon
>
> removed the semicolon that caused the warning.
> no functional changes.
> true positive.
> file showed no errors on make.
>
> checkpatch.pl on this file after change:
>
> WARNING: Missing a blank line after declarations
> #68: FILE: fs/block_dev.c:68:
> + char name[BDEVNAME_SIZE];
> + pr_warn_ratelimited("VFS: Dirty inode writeback failed "
>
> WARNING: quoted string split across lines
> #69: FILE: fs/block_dev.c:69:
> + pr_warn_ratelimited("VFS: Dirty inode writeback failed "
> +    "for block device %s (err=%d).\n",
>
> WARNING: Missing a blank line after declarations
> #120: FILE: fs/block_dev.c:120:
> + int err = bd_prepare_to_claim(bdev, truncate_bdev_range);
> + if (err)
>
> WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> #171: FILE: fs/block_dev.c:171:
> +EXPORT_SYMBOL(set_blocksize);
>
> WARNING: Block comments use a trailing */ on a separate line
> #178: FILE: fs/block_dev.c:178:
> + * and it's value is between 512 and PAGE_SIZE */
>
> WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> #184: FILE: fs/block_dev.c:184:
> +EXPORT_SYMBOL(sb_set_blocksize);
>
> WARNING: Missing a blank line after declarations
> #189: FILE: fs/block_dev.c:189:
> + int minsize = bdev_logical_block_size(sb->s_bdev);
> + if (size < minsize)
>
> WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> #194: FILE: fs/block_dev.c:194:
> +EXPORT_SYMBOL(sb_min_blocksize);
>
> WARNING: Missing a blank line after declarations
> #547: FILE: fs/block_dev.c:547:
> + struct super_block *sb = get_super(bdev);
> + if (sb) {
>
> WARNING: Missing a blank line after declarations
> #549: FILE: fs/block_dev.c:549:
> + int res = sync_filesystem(sb);
> + drop_super(sb);
>
> ERROR: "foo * bar" should be "foo *bar"
> #642: FILE: fs/block_dev.c:642:
> +static int blkdev_readpage(struct file * file, struct page * page)
>
> ERROR: "foo * bar" should be "foo *bar"
> #642: FILE: fs/block_dev.c:642:
> +static int blkdev_readpage(struct file * file, struct page * page)
>
> WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
> #653: FILE: fs/block_dev.c:653:
> + loff_t pos, unsigned len, unsigned flags,
>
> WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
> #653: FILE: fs/block_dev.c:653:
> + loff_t pos, unsigned len, unsigned flags,
>
> WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
> #661: FILE: fs/block_dev.c:661:
> + loff_t pos, unsigned len, unsigned copied,
>
> WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
> #661: FILE: fs/block_dev.c:661:
> + loff_t pos, unsigned len, unsigned copied,
>
> WARNING: Missing a blank line after declarations
> #665: FILE: fs/block_dev.c:665:
> + int ret;
> + ret = block_write_end(file, mapping, pos, len, copied, page, fsdata);
>
> ERROR: trailing whitespace
> #688: FILE: fs/block_dev.c:688:
> +^I$
>
> ERROR: trailing whitespace
> #694: FILE: fs/block_dev.c:694:
> +^I$
>
> ERROR: "foo * bar" should be "foo *bar"
> #795: FILE: fs/block_dev.c:795:
> +static struct kmem_cache * bdev_cachep __read_mostly;
>
> WARNING: Missing a blank line after declarations
> #830: FILE: fs/block_dev.c:830:
> + struct block_device *bdev = &BDEV_I(inode)->bdev;
> + truncate_inode_pages_final(&inode->i_data);
>
> WARNING: Missing a blank line after declarations
> #852: FILE: fs/block_dev.c:852:
> + struct pseudo_fs_context *ctx = init_pseudo(fc, BDEVFS_MAGIC);
> + if (!ctx)
>
> ERROR: trailing whitespace
> #967: FILE: fs/block_dev.c:967:
> + $
>
> WARNING: please, no spaces at the start of a line
> #967: FILE: fs/block_dev.c:967:
> + $
>
> WARNING: please, no space before tabs
> #988: FILE: fs/block_dev.c:988:
> +^I^Ireturn false; ^I /* held by someone else */$
>
> WARNING: please, no space before tabs
> #990: FILE: fs/block_dev.c:990:
> +^I^Ireturn true;  ^I /* is a whole device which isn't held */$
>
> WARNING: please, no space before tabs
> #993: FILE: fs/block_dev.c:993:
> +^I^Ireturn true; ^I /* is a partition of a device that is being partitioned */$
>
> ERROR: "foo * bar" should be "foo *bar"
> #1491: FILE: fs/block_dev.c:1491:
> +static int blkdev_open(struct inode * inode, struct file * filp)
>
> ERROR: "foo * bar" should be "foo *bar"
> #1491: FILE: fs/block_dev.c:1491:
> +static int blkdev_open(struct inode * inode, struct file * filp)
>
> ERROR: do not use assignment in if condition
> #1549: FILE: fs/block_dev.c:1549:
> + if ((bdev_free = !bdev->bd_holders))
>
> ERROR: "foo * bar" should be "foo *bar"
> #1583: FILE: fs/block_dev.c:1583:
> +static int blkdev_close(struct inode * inode, struct file * filp)
>
> ERROR: "foo * bar" should be "foo *bar"
> #1583: FILE: fs/block_dev.c:1583:
> +static int blkdev_close(struct inode * inode, struct file * filp)
>
> WARNING: Missing a blank line after declarations
> #1586: FILE: fs/block_dev.c:1586:
> + struct block_device *bdev = I_BDEV(bdev_file_inode(filp));
> + blkdev_put(bdev, filp->f_mode);
>
> WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
> #1590: FILE: fs/block_dev.c:1590:
> +static long block_ioctl(struct file *file, unsigned cmd, unsigned long arg)
>
> total: 11 errors, 23 warnings, 1878 lines checked
>
> include/trace/events/iocost.h:163: WARNING:ONE_SEMICOLON: Statements
> terminations use 1 semicolon
>
> removed the extra semicolon that caused the warning.
> no functional change.
> true positive.
> showed no errors on "make -j8 all".
>
> checkpatch.pl after changes :
>
> WARNING: space prohibited between function name and open parenthesis '('
> #21: FILE: include/trace/events/iocost.h:21:
> + TP_STRUCT__entry (
>
> WARNING: quoted string split across lines
> #52: FILE: include/trace/events/iocost.h:52:
> + TP_printk("[%s:%s] now=%llu:%llu vrate=%llu "
> +  "period=%llu->%llu vtime=%llu "
>
> WARNING: quoted string split across lines
> #53: FILE: include/trace/events/iocost.h:53:
> +  "period=%llu->%llu vtime=%llu "
> +  "weight=%u/%u hweight=%llu/%llu",
>
> WARNING: space prohibited between function name and open parenthesis '('
> #85: FILE: include/trace/events/iocost.h:85:
> + TP_STRUCT__entry (
>
> WARNING: space prohibited between function name and open parenthesis '('
> #149: FILE: include/trace/events/iocost.h:149:
> + TP_STRUCT__entry (
>
> WARNING: space prohibited between function name and open parenthesis '('
> #190: FILE: include/trace/events/iocost.h:190:
> + TP_STRUCT__entry (
>
> total: 0 errors, 6 warnings, 225 lines checked
>
> net/sched/cls_rsvp.h:231: WARNING:ONE_SEMICOLON: Statements
> terminations use 1 semicolon
>
> false positive.
> no changes made.
> a for loop will require 2 semicolons as per the rules of C.
> we can maybe have a check for this rule where it can be ignored when
> it is in a for loop ? or have a check for the number of consecutive semicolons be less than 2.
>
> net/sched/cls_u32.c:782: WARNING:ONE_SEMICOLON: Statements
> terminations use 1 semicolon
>
> false positive.
> no changes made.
> a for loop will require 2 semicolons as per the rules of C.
>
> drivers/net/ethernet/mediatek/mtk_star_emac.c:1089:
> WARNING:ONE_SEMICOLON: Statements terminations use 1 semicolon
>
> false positive.
> no changes made.
> a for loop will require 2 semicolons as per the rules of C.
>
> arch/ia64/include/asm/mca_asm.h:139: WARNING:ONE_SEMICOLON: Statements
> terminations use 1 semicolon
>
> arch/ia64/include/asm/mca_asm.h:213: WARNING:ONE_SEMICOLON: Statements
> terminations use 1 semicolon
>
> arch/ia64/include/asm/native/inst.h:79: WARNING:ONE_SEMICOLON:
> Statements terminations use 1 semicolon
>
> arch/ia64/kernel/minstate.h:14: WARNING:ONE_SEMICOLON: Statements
> terminations use 1 semicolon
>
> arch/ia64/kernel/minstate.h:151: WARNING:ONE_SEMICOLON: Statements
> terminations use 1 semicolon
>
> arch/ia64/kernel/minstate.h:213: WARNING:ONE_SEMICOLON: Statements
> terminations use 1 semicolon
>
> false positive.
> i am not entirely sure of this rule,
> but what i noticed is that the place where ";;"
> is used is all under #define. I am not entirely sure of it's
> function in c under .h files. Can you help me out on that one ?
> but other than that, i am positive it shouldn't be removed,
> or else the function might get altered or it might result in an error.
>
> change log :
>
>  drivers/gpu/drm/nouveau/nouveau_gem.c | 2 +-
>  drivers/mtd/ubi/block.c               | 2 +-
>  fs/block_dev.c                        | 2 +-
>  include/trace/events/iocost.h         | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
>

Okay, now you at least provide a commit message.

Your commit message still looks like an unstructured set of notes with
various personal comments, but not a properly summarizing technical
description of the motivation of your changes and an argument why it
improves the code (fixes an issue the kernel community should care
about to pick this patch).

Further, I suggest you identify which person you would need to send a
change for each file.

If the group of people to send the change to is different, split the
change into separate patches and only mention the relevant information
for each separate patch.

Good luck.

Lukas

> diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
> index 5b27845075a1..d476940ee97c 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> @@ -339,7 +339,7 @@ nouveau_gem_set_domain(struct drm_gem_object *gem, uint32_t read_domains,
>         struct ttm_buffer_object *bo = &nvbo->bo;
>         uint32_t domains = valid_domains & nvbo->valid_domains &
>                 (write_domains ? write_domains : read_domains);
> -       uint32_t pref_domains = 0;;
> +       uint32_t pref_domains = 0;
>
>         if (!domains)
>                 return -EINVAL;
> diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
> index e003b4b44ffa..e0402c2f8959 100644
> --- a/drivers/mtd/ubi/block.c
> +++ b/drivers/mtd/ubi/block.c
> @@ -405,7 +405,7 @@ int ubiblock_create(struct ubi_volume_info *vi)
>         ret = blk_mq_alloc_tag_set(&dev->tag_set);
>         if (ret) {
>                 dev_err(disk_to_dev(dev->gd), "blk_mq_alloc_tag_set failed");
> -               goto out_free_dev;;
> +               goto out_free_dev;
>         }
>
>
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 9ef4f1fc2cb0..61b938fa5d5b 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1268,7 +1268,7 @@ static int blkdev_get_whole(struct block_device *bdev, fmode_t mode)
>         if (test_bit(GD_NEED_PART_SCAN, &disk->state))
>                 bdev_disk_changed(disk, false);
>         bdev->bd_openers++;
> -       return 0;;
> +       return 0;
>  }
>
>  static void blkdev_put_whole(struct block_device *bdev, fmode_t mode)
> diff --git a/include/trace/events/iocost.h b/include/trace/events/iocost.h
> index e282ce02fa2d..6d1626e7a4ce 100644
> --- a/include/trace/events/iocost.h
> +++ b/include/trace/events/iocost.h
> @@ -160,7 +160,7 @@ TRACE_EVENT(iocost_ioc_vrate_adj,
>
>         TP_fast_assign(
>                 __assign_str(devname, ioc_name(ioc));
> -               __entry->old_vrate = atomic64_read(&ioc->vtime_rate);;
> +               __entry->old_vrate = atomic64_read(&ioc->vtime_rate);
>                 __entry->new_vrate = new_vrate;
>                 __entry->busy_level = ioc->busy_level;
>                 __entry->read_missed_ppm = missed_ppm[READ];
> --
> 2.25.1


More information about the Linux-kernel-mentees mailing list