lkml.org 
[lkml]   [2020]   [Jul]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH] scsi: virtio_scsi: Remove unnecessary condition checks
    From
    Date
    On 10/07/20 08:32, Markus Elfring wrote:
    >>>> + mempool_destroy(virtscsi_cmd_pool);
    >>>> + virtscsi_cmd_pool = NULL;
    >>>> + kmem_cache_destroy(virtscsi_cmd_cache);
    >>>> + virtscsi_cmd_cache = NULL;
    >>>> return ret;
    >>>> }
    >>>
    >>> How do you think about to add a jump target so that the execution
    >>> of a few statements can be avoided according to a previous
    >>> null pointer check?
    >>
    >> The point of the patch is precisely to simplify the code,
    >
    > I suggest to reconsider also Linux coding style aspects
    > for the implementation of the function “init”.
    > https://elixir.bootlin.com/linux/v5.8-rc4/source/drivers/scsi/virtio_scsi.c#L980
    > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/virtio_scsi.c?id=42f82040ee66db13525dc6f14b8559890b2f4c1c#n980
    >
    > if (!virtscsi_cmd_cache) {
    > pr_err("kmem_cache_create() for virtscsi_cmd_cache failed\n");
    > - goto error;
    > + return -ENOMEM;
    > }

    Could be doable, but I don't see a particular benefit. Having a single
    error loop is an advantage by itself.

    The coding style is a suggestion. Note the difference between

    kfree(foo->bar);
    kfree(foo);

    and

    kfree(bar);
    kfree(foo);

    > See also:
    > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=42f82040ee66db13525dc6f14b8559890b2f4c1c#n461
    >
    >
    >> executing a couple more instruction is not an issue.
    >
    > With which update steps would like to achieve such a code variant?
    >
    > destroy_pool:
    > mempool_destroy(virtscsi_cmd_pool);
    > virtscsi_cmd_pool = NULL;
    > destroy_cache:
    > kmem_cache_destroy(virtscsi_cmd_cache);
    > virtscsi_cmd_cache = NULL;
    > return ret;

    ... while there's no advantage in this.

    Paolo

    \
     
     \ /
      Last update: 2020-07-10 09:18    [W:2.581 / U:0.160 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site