Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752593AbbGWJxj (ORCPT ); Thu, 23 Jul 2015 05:53:39 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:37083 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752254AbbGWJxc (ORCPT ); Thu, 23 Jul 2015 05:53:32 -0400 Date: Thu, 23 Jul 2015 02:53:29 -0700 From: Christoph Hellwig To: Matias Bj??rling 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 Message-ID: <20150723095329.GA26420@infradead.org> References: <1437587464-7964-1-git-send-email-mb@lightnvm.io> <1437587464-7964-2-git-send-email-mb@lightnvm.io> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1437587464-7964-2-git-send-email-mb@lightnvm.io> User-Agent: Mutt/1.5.23 (2014-03-12) X-SRS-Rewrite: SMTP reverse-path rewritten from by bombadil.infradead.org See http://www.infradead.org/rpr.html Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1423 Lines: 58 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/