From: Artem Bityutskiy <dedekind1@gmail.com>
To: Maxim Levitsky <maximlevitsky@gmail.com>
Cc: linux-mtd@lists.infradead.org,
Andrew Morton <akpm@linux-foundation.org>,
David Woodhouse <dwmw2@infradead.org>,
Maciej Rutecki <maciej.rutecki@gmail.com>
Subject: Re: [PATCH 1/3] MTD: blktrans: Final version of hotplug support
Date: Sun, 19 Sep 2010 21:28:28 +0300 [thread overview]
Message-ID: <1284920908.1704.5.camel@brekeke> (raw)
In-Reply-To: <1284851558-1154-2-git-send-email-maximlevitsky@gmail.com>
On Sun, 2010-09-19 at 01:12 +0200, Maxim Levitsky wrote:
> Or at least this patch claims it to be so...
>
> Now it once again possible to remove mtdtrans and mtd drivers even
> if underlying mtd device is mounted.
> This time in addition to code review, I also made the code
> pass some torture tests like module reload in a loop + read in a loop +
> card insert/removal all at same time.
>
> The blktrans_open/blktrans_release don't take the mtd table lock because:
>
> While device is added (that includes execution of add_mtd_blktrans_dev)
> the lock is already taken
>
> Now suppose the device will never be removed. In this case even if we have changes
> in mtd table, the entry that we need will stay exactly the same. (Note that we don't
> look at table at all, just following private pointer of block device).
>
> Now suppose that someone tries to remove the mtd device.
> This will be propagated to trans driver which _ought_ to call del_mtd_blktrans_dev
> which will take the per device lock, release the mtd device and set trans->mtd = NULL.
> From this point on, following opens won't even be able to know anything about that mtd device
> (which at that point is likely not to exist)
> Also the same care is taken not to trip over NULL mtd pointer in blktrans_dev_release.
>
> In addition to that I moved the blk_cleanup_queue back to del_mtd_blktrans_dev
> because I now know that is ok, and I removed the BUG from deregister_mtd_blktrans
> because the current mtd trans driver can still have 'dead' users upon removal.
> These won't touch it again, so its safe to remove the module.
>
> Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
Too many changes in one patch - please, split it on smaller patches. Do
one change per patch not many changes in one. This patch is not really
reviewable. And since you are fixing regression, make sure you have
first N _minimal_ patches which just fix the regression and nothing
else, and to the rest in the following M patches.
--
Best Regards,
Artem Bityutskiy (Битюцкий Артём)
next prev parent reply other threads:[~2010-09-19 18:28 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-09-18 23:12 [MTD] My updates Maxim Levitsky
2010-09-18 23:12 ` [PATCH 1/3] MTD: blktrans: Final version of hotplug support Maxim Levitsky
2010-09-19 2:03 ` [PATCH V2] " maximlevitsky
2010-09-19 18:28 ` Artem Bityutskiy [this message]
2010-09-19 20:35 ` [PATCH 1/3] " Maxim Levitsky
2010-09-20 1:08 ` Maxim Levitsky
2010-09-20 8:28 ` Artem Bityutskiy
2010-09-18 23:12 ` [PATCH 2/3] MTD: r852: remove useless pci powerup/down from suspend/resume routines Maxim Levitsky
2010-09-18 23:12 ` [PATCH 3/3] mtd: sm_ftl: cosmetic, use bool when possible Maxim Levitsky
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=1284920908.1704.5.camel@brekeke \
--to=dedekind1@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=dwmw2@infradead.org \
--cc=linux-mtd@lists.infradead.org \
--cc=maciej.rutecki@gmail.com \
--cc=maximlevitsky@gmail.com \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).