From: hch@infradead.org (Christoph Hellwig)
Subject: [PATCH v5 1/5] lightnvm: Support for Open-Channel SSDs
Date: Thu, 23 Jul 2015 02:53:29 -0700 [thread overview]
Message-ID: <20150723095329.GA26420@infradead.org> (raw)
In-Reply-To: <1437587464-7964-2-git-send-email-mb@lightnvm.io>
Hi Matias,
I like this new architecture.
Minor nitpicks below:
> @@ -0,0 +1,598 @@
> +/*
> + * core.c - Open-channel SSD integration core
No need to state the file name in the top of the file comments, it
doesn't add value and get stale easily.
> +static LIST_HEAD(_targets);
> +static LIST_HEAD(_bms);
> +static LIST_HEAD(_devices);
> +static DECLARE_RWSEM(_lock);
Please add a nvm_ prefix to these.
> +static int nvm_ioctl(struct block_device *bdev, fmode_t mode, unsigned int cmd,
> + unsigned long arg)
> +{
> + return 0;
> +}
> +
> +static int nvm_open(struct block_device *bdev, fmode_t mode)
> +{
> + return 0;
> +}
> +
> +static void nvm_release(struct gendisk *disk, fmode_t mode)
> +{
> +}
No need to implement these empty methods.
> +/* _lock must be taken */
Turn this into an lockdep_assert_held in the code, please.
> +int nvm_register(struct request_queue *q, struct gendisk *disk,
> + struct nvm_dev_ops *ops)
> +{
No need to pass a gendisk here, in fact LighNVM low level driver
shouldn't even need to allocate one. The only thing you seem to be
using here is the name anyway.
> +void nvm_unregister(struct gendisk *disk)
Same here.
WARNING: multiple messages have this Message-ID (diff)
From: Christoph Hellwig <hch@infradead.org>
To: Matias Bj??rling <mb@lightnvm.io>
Cc: hch@infradead.org, axboe@fb.com, linux-fsdevel@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-nvme@lists.infradead.org,
Stephen.Bates@pmcs.com, keith.busch@intel.com
Subject: Re: [PATCH v5 1/5] lightnvm: Support for Open-Channel SSDs
Date: Thu, 23 Jul 2015 02:53:29 -0700 [thread overview]
Message-ID: <20150723095329.GA26420@infradead.org> (raw)
In-Reply-To: <1437587464-7964-2-git-send-email-mb@lightnvm.io>
Hi Matias,
I like this new architecture.
Minor nitpicks below:
> @@ -0,0 +1,598 @@
> +/*
> + * core.c - Open-channel SSD integration core
No need to state the file name in the top of the file comments, it
doesn't add value and get stale easily.
> +static LIST_HEAD(_targets);
> +static LIST_HEAD(_bms);
> +static LIST_HEAD(_devices);
> +static DECLARE_RWSEM(_lock);
Please add a nvm_ prefix to these.
> +static int nvm_ioctl(struct block_device *bdev, fmode_t mode, unsigned int cmd,
> + unsigned long arg)
> +{
> + return 0;
> +}
> +
> +static int nvm_open(struct block_device *bdev, fmode_t mode)
> +{
> + return 0;
> +}
> +
> +static void nvm_release(struct gendisk *disk, fmode_t mode)
> +{
> +}
No need to implement these empty methods.
> +/* _lock must be taken */
Turn this into an lockdep_assert_held in the code, please.
> +int nvm_register(struct request_queue *q, struct gendisk *disk,
> + struct nvm_dev_ops *ops)
> +{
No need to pass a gendisk here, in fact LighNVM low level driver
shouldn't even need to allocate one. The only thing you seem to be
using here is the name anyway.
> +void nvm_unregister(struct gendisk *disk)
Same here.
next prev parent reply other threads:[~2015-07-23 9:53 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-22 17:50 [PATCH v5 0/5] Support for Open-Channel SSDs Matias Bjørling
2015-07-22 17:50 ` Matias Bjørling
2015-07-22 17:51 ` [PATCH v5 1/5] lightnvm: " Matias Bjørling
2015-07-22 17:51 ` Matias Bjørling
2015-07-22 17:51 ` Matias Bjørling
2015-07-23 9:53 ` Christoph Hellwig [this message]
2015-07-23 9:53 ` Christoph Hellwig
2015-07-23 10:38 ` Matias Bjørling
2015-07-23 10:38 ` Matias Bjørling
2015-07-22 17:51 ` [PATCH v5 2/5] rrpc: Hybrid Open-Channel SSD RRPC target Matias Bjørling
2015-07-22 17:51 ` Matias Bjørling
2015-07-22 17:51 ` [PATCH v5 3/5] bm_hb: Hybrid Open-Channel SSD block manager Matias Bjørling
2015-07-22 17:51 ` Matias Bjørling
2015-07-22 17:51 ` [PATCH v5 4/5] null_blk: LightNVM support Matias Bjørling
2015-07-22 17:51 ` Matias Bjørling
2015-07-23 9:53 ` Christoph Hellwig
2015-07-23 9:53 ` Christoph Hellwig
2015-07-23 10:48 ` Matias Bjørling
2015-07-23 10:48 ` Matias Bjørling
2015-07-25 6:25 ` Christoph Hellwig
2015-07-25 6:25 ` Christoph Hellwig
2015-07-22 17:51 ` [PATCH v5 5/5] nvme: " Matias Bjørling
2015-07-22 17:51 ` Matias Bjørling
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150723095329.GA26420@infradead.org \
--to=hch@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.