From: Brian Norris <computersforpeace@gmail.com>
To: Alexander Holler <holler@ahsoftware.de>
Cc: linux-kernel@vger.kernel.org, linux-mtd@lists.infradead.org,
David Woodhouse <dwmw2@infradead.org>
Subject: Re: [PATCH 01/27] mtd: nand: introduce function to fix a common bug in most nand-drivers not showing a device in sysfs
Date: Wed, 28 May 2014 01:43:44 -0700 [thread overview]
Message-ID: <20140528084344.GB4285@norris-Latitude-E6410> (raw)
In-Reply-To: <1401142372-14148-2-git-send-email-holler@ahsoftware.de>
On Tue, May 27, 2014 at 12:12:26AM +0200, Alexander Holler wrote:
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -23,7 +23,7 @@
> #include <linux/types.h>
> #include <linux/uio.h>
> #include <linux/notifier.h>
> -#include <linux/device.h>
> +#include <linux/platform_device.h>
>
> #include <mtd/mtd-abi.h>
>
> @@ -366,6 +366,15 @@ static inline int mtd_can_have_bb(const struct mtd_info *mtd)
> struct mtd_partition;
> struct mtd_part_parser_data;
>
> +static inline void mtd_setup_common_members(struct mtd_info *mtd, void *priv,
> + struct platform_device *pdev)
Thanks for the diligence on catching these issues, but I'm not sure this
helper function is fully the correct approach here.
> +{
> + mtd->priv = priv;
I don't think you should hide this one here. It will be quite obvious if
a driver didn't stash its private data but tries to access it later. Are
there any drivers that missed this?
> + mtd->owner = pdev->dev.driver->owner;
> + mtd->dev.parent = &pdev->dev;
> + mtd->name = pdev->dev.driver->name;
I think this is a little dangerous. You're potentially clobbering the
name that a driver already chose here. And why did you pick to use the
driver name? This gives non-unique names if there is more than one
device instantiated for a driver. That's why some drivers already use
the device name, not the driver name:
mtd->name = dev_name(&pev->dev);
And in fact, if any drivers are missing mtd->name, perhaps it's best to
just modify the MTD registration to give them a default:
if (!mtd->name)
mtd->name = dev_name(&pdev->dev);
> +}
BTW, nothing in this function actually makes sense to require a
platform_device, does it? And it's possible to have non-platform drivers
that want to do basic MTD initialization. So (if we still keep this
helper function at all), I'd recommend just a 'struct device *dev'
parameter.
> +
> extern int mtd_device_parse_register(struct mtd_info *mtd,
> const char * const *part_probe_types,
> struct mtd_part_parser_data *parser_data,
How about we rethink the "helper" approach, and instead just do
validation in the core code? This would cover most of the important
parts of your helper, I think:
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index d201feeb3ca6..39ba5812a5a3 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -397,6 +397,11 @@ int add_mtd_device(struct mtd_info *mtd)
if (device_register(&mtd->dev) != 0)
goto fail_added;
+ if (mtd->dev.parent)
+ mtd->owner = mtd->dev.parent->driver->owner;
+ else
+ WARN_ON(1);
+
if (MTD_DEVT(i))
device_create(&mtd_class, mtd->dev.parent,
MTD_DEVT(i) + 1,
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 1ca9aec141ff..9869bbef50cf 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -370,7 +370,6 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
slave->mtd.subpage_sft = master->subpage_sft;
slave->mtd.name = name;
- slave->mtd.owner = master->owner;
slave->mtd.backing_dev_info = master->backing_dev_info;
/* NOTE: we don't arrange MTDs as a tree; it'd be error-prone
--
Brian
next prev parent reply other threads:[~2014-05-28 8:43 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-05-26 22:12 [PATCH 00/27] Fix common bug in most nand drivers not showing a device in sysfs Alexander Holler
2014-05-26 22:12 ` [PATCH 01/27] mtd: nand: introduce function to fix a common bug in most nand-drivers " Alexander Holler
2014-05-28 8:43 ` Brian Norris [this message]
2014-05-28 18:52 ` Alexander Holler
2014-05-28 20:10 ` Brian Norris
2014-05-28 21:09 ` Alexander Holler
2014-05-28 21:49 ` Brian Norris
2014-05-29 3:53 ` Alexander Holler
2014-05-29 6:17 ` Artem Bityutskiy
2014-05-30 9:33 ` Alexander Holler
2014-11-02 21:03 ` Frans Klaver
2014-11-05 9:34 ` Brian Norris
2014-11-05 9:48 ` Frans Klaver
2014-05-26 22:12 ` [PATCH 02/27] mtd: nand: orion_nand: show device structure " Alexander Holler
2014-05-26 22:12 ` [PATCH 03/27] mtd: nand: omap2: " Alexander Holler
2014-05-26 22:12 ` [PATCH 04/27] mtd: nand: atmel_nand: " Alexander Holler
2014-05-26 22:12 ` [PATCH 05/27] mtd: nand: gpio: " Alexander Holler
2014-05-26 22:12 ` [PATCH 06/27] mtd: nand: fsmc: " Alexander Holler
2014-05-26 22:12 ` [PATCH 07/27] mtd: nand: gpmi: " Alexander Holler
2014-05-26 22:12 ` [PATCH 08/27] mtd: nand: plat: " Alexander Holler
2014-05-26 22:12 ` [PATCH 09/27] mtd: nand: pxa3xx: " Alexander Holler
2014-05-27 3:12 ` Alexander Shiyan
2014-05-27 6:01 ` Alexander Holler
2014-05-28 7:25 ` Brian Norris
2014-08-08 17:04 ` Ezequiel Garcia
2014-08-08 17:16 ` Brian Norris
2014-08-08 18:11 ` Ezequiel Garcia
2014-08-08 20:31 ` Alexander Holler
2014-05-26 22:12 ` [PATCH 10/27] mtd: nand: s3c2410: " Alexander Holler
2014-05-26 22:12 ` [PATCH 11/27] mtd: nand: sh_flctl: " Alexander Holler
2014-05-26 22:12 ` [PATCH 12/27] mtd: nand: sharpsl: " Alexander Holler
2014-05-26 22:12 ` [PATCH 13/27] mtd: nand: tmio: " Alexander Holler
2014-05-26 22:12 ` [PATCH 14/27] mtd: nand: docg4: " Alexander Holler
2014-05-26 22:12 ` [PATCH 15/27] mtd: nand: davinci: use mtd_setup_common_members() Alexander Holler
2014-05-26 22:12 ` [PATCH 16/27] mtd: nand: lpc32xx_mlc: " Alexander Holler
2014-05-26 22:12 ` [PATCH 17/27] mtd: nand: lpc32xx_slc: " Alexander Holler
2014-05-26 22:12 ` [PATCH 18/27] mtd: nand: mxc: " Alexander Holler
2014-05-26 22:12 ` [PATCH 19/27] mtd: nand: bcm47: show device structure in sysfs Alexander Holler
2014-05-26 22:12 ` [PATCH 20/27] mtd: nand: fsl_elbc: " Alexander Holler
2014-05-26 22:12 ` [PATCH 21/27] mtd: nand: fsl_upm: " Alexander Holler
2014-05-26 22:12 ` [PATCH 22/27] mtd: nand: fsl_ifc: " Alexander Holler
2014-05-26 22:12 ` [PATCH 23/27] mtd: nand: jz4740: " Alexander Holler
2014-05-26 22:12 ` [PATCH 24/27] mtd: nand: mpc5121_nfc: " Alexander Holler
2014-05-26 22:12 ` [PATCH 25/27] mtd: nand: ndfc: " Alexander Holler
2014-05-26 22:12 ` [PATCH 26/27] mtd: nand: txx9ndfmc: " Alexander Holler
2014-05-26 22:12 ` [PATCH 27/27] mtd: nand: socrates: use mtd_setup_common_members() Alexander Holler
2014-05-27 6:26 ` [PATCH 07/27 v2] mtd: nand: gpmi: show device structure in sysfs Alexander Holler
2014-05-27 6:28 ` [PATCH 09/27 v2] mtd: nand: pxa3xx: " Alexander Holler
2014-10-16 6:37 ` [PATCH 00/27] Fix common bug in most nand drivers not showing a device " Alexander Holler
2014-10-17 9:14 ` Alexander Holler
2014-10-17 10:53 ` Frans Klaver
2014-10-17 15:54 ` Alexander Holler
2014-10-19 21:24 ` Frans Klaver
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=20140528084344.GB4285@norris-Latitude-E6410 \
--to=computersforpeace@gmail.com \
--cc=dwmw2@infradead.org \
--cc=holler@ahsoftware.de \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.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 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).