[1/5] mtd: fix bdev exclusive open bugs in block2mtd::add_device()

Submitted by Tejun Heo on Nov. 1, 2010, 4:15 p.m.

Details

Message ID 1288628129-12811-2-git-send-email-tj@kernel.org
State New, archived
Headers show

Commit Message

Tejun Heo Nov. 1, 2010, 4:15 p.m.
There are two bdev exclusive open bugs.

* open_bdev_exclusive() must not be called with NULL holder.  Use dev
  as the holder.

* open_by_devnum() doesn't open the bdev exclusively but
  block2mtd_free_device() always assumes it.  Explicitly claim the
  bdev.

The latter is rather clumsy but will be simplified with future
blkdev_get/put() cleanups.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: linux-mtd@lists.infradead.org
---
 drivers/mtd/devices/block2mtd.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

Comments

Artem Bityutskiy Nov. 13, 2010, 10:38 a.m.
Hi,

On Mon, 2010-11-01 at 17:15 +0100, Tejun Heo wrote:
> +			if (!IS_ERR(bdev)) {
> +				int ret;
> +				ret = bd_claim(bdev, dev);
> +				if (ret) {
> +					blkdev_put(bdev,
> +						   FMODE_READ | FMODE_WRITE);

Would be a bit cleaner to define ea temporary variable:

fmode_t mode = FMODE_READ | FMODE_WRITE;

Would you to re-send with this little change please?

And ideally, 2 independent patches would be nicer because you fix 2
independent issues.
Tejun Heo Nov. 13, 2010, 10:42 a.m.
Hello,

On 11/13/2010 11:38 AM, Artem Bityutskiy wrote:
> On Mon, 2010-11-01 at 17:15 +0100, Tejun Heo wrote:
>> +			if (!IS_ERR(bdev)) {
>> +				int ret;
>> +				ret = bd_claim(bdev, dev);
>> +				if (ret) {
>> +					blkdev_put(bdev,
>> +						   FMODE_READ | FMODE_WRITE);
> 
> Would be a bit cleaner to define ea temporary variable:
> 
> fmode_t mode = FMODE_READ | FMODE_WRITE;
> 
> Would you to re-send with this little change please?

Yeap, sure.

> And ideally, 2 independent patches would be nicer because you fix 2
> independent issues.

Hmmm... not really.  The patch is small enough and splitting it won't
really buy as any better bisectability.  Splitting patches into
logical changes is a good thing but it's no religious dogma.  Let's
apply it to the point it actually helps.

Thank you.
Artem Bityutskiy Nov. 13, 2010, 11:10 a.m.
On Sat, 2010-11-13 at 11:42 +0100, Tejun Heo wrote:
> Hmmm... not really.  The patch is small enough and splitting it won't
> really buy as any better bisectability.  Splitting patches into
> logical changes is a good thing but it's no religious dogma.  Let's
> apply it to the point it actually helps.

Well, it is more about if your second fix is buggy, we could revert it
without reverting the first fix. But anyway, this is not a big deal, so
let's forget about this indeed.

Patch hide | download patch | download mbox

diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
index 2cf0cc6..806a9ed 100644
--- a/drivers/mtd/devices/block2mtd.c
+++ b/drivers/mtd/devices/block2mtd.c
@@ -246,7 +246,7 @@  static struct block2mtd_dev *add_device(char *devname, int erase_size)
 		return NULL;
 
 	/* Get a handle on the device */
-	bdev = open_bdev_exclusive(devname, FMODE_READ|FMODE_WRITE, NULL);
+	bdev = open_bdev_exclusive(devname, FMODE_READ|FMODE_WRITE, dev);
 #ifndef MODULE
 	if (IS_ERR(bdev)) {
 
@@ -256,6 +256,15 @@  static struct block2mtd_dev *add_device(char *devname, int erase_size)
 		dev_t devt = name_to_dev_t(devname);
 		if (devt) {
 			bdev = open_by_devnum(devt, FMODE_WRITE | FMODE_READ);
+			if (!IS_ERR(bdev)) {
+				int ret;
+				ret = bd_claim(bdev, dev);
+				if (ret) {
+					blkdev_put(bdev,
+						   FMODE_READ | FMODE_WRITE);
+					bdev = ERR_PTR(ret);
+				}
+			}
 		}
 	}
 #endif