From: Alexander Holler <holler@ahsoftware.de> To: Brian Norris <computersforpeace@gmail.com> 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 20:52:06 +0200 [thread overview] Message-ID: <53863056.2070405@ahsoftware.de> (raw) In-Reply-To: <20140528084344.GB4285@norris-Latitude-E6410> Am 28.05.2014 10:43, schrieb Brian Norris: > 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? No, it just saves a line of source in all drivers and I think it fits there. I don't understand why do you think it is hidden. > >> + 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); > >> +} I don't clobber any name. The name is set as default before drivers might do this. And the common pattern I've seen wasn't dev_name(foo) but the drivers name. And those drivers which do use dev_name(), still do so by overwriting the default I put into that function. But feel free to change this. I will not go again and again through the 26 drivers until all maintainers and other people are happy. > > 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. > Feel free to chgange it. >> + >> 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: Feel free to change all drivers. I like my approach with fixing 21 bugs by reducing code by 44 lines. And it offers a common function where future similiarities can be put into too. Of course you can just add 21 lines, but that is not how I do such stuff. And I did the patches. If you don't like them, feel free to ignore them. I'm not playing remote keyboard but I do patches like I would do them, not like some arbitrary maintainer would do them. Sorry for the harsh words. Regards, Alexander Holler
WARNING: multiple messages have this Message-ID (diff)
From: Alexander Holler <holler@ahsoftware.de> To: Brian Norris <computersforpeace@gmail.com> Cc: David Woodhouse <dwmw2@infradead.org>, linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.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 20:52:06 +0200 [thread overview] Message-ID: <53863056.2070405@ahsoftware.de> (raw) In-Reply-To: <20140528084344.GB4285@norris-Latitude-E6410> Am 28.05.2014 10:43, schrieb Brian Norris: > 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? No, it just saves a line of source in all drivers and I think it fits there. I don't understand why do you think it is hidden. > >> + 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); > >> +} I don't clobber any name. The name is set as default before drivers might do this. And the common pattern I've seen wasn't dev_name(foo) but the drivers name. And those drivers which do use dev_name(), still do so by overwriting the default I put into that function. But feel free to change this. I will not go again and again through the 26 drivers until all maintainers and other people are happy. > > 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. > Feel free to chgange it. >> + >> 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: Feel free to change all drivers. I like my approach with fixing 21 bugs by reducing code by 44 lines. And it offers a common function where future similiarities can be put into too. Of course you can just add 21 lines, but that is not how I do such stuff. And I did the patches. If you don't like them, feel free to ignore them. I'm not playing remote keyboard but I do patches like I would do them, not like some arbitrary maintainer would do them. Sorry for the harsh words. Regards, Alexander Holler
next prev parent reply other threads:[~2014-05-28 18:52 UTC|newest] Thread overview: 106+ 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 ` 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-26 22:12 ` Alexander Holler 2014-05-28 8:43 ` Brian Norris 2014-05-28 8:43 ` Brian Norris 2014-05-28 18:52 ` Alexander Holler [this message] 2014-05-28 18:52 ` Alexander Holler 2014-05-28 20:10 ` Brian Norris 2014-05-28 20:10 ` Brian Norris 2014-05-28 21:09 ` Alexander Holler 2014-05-28 21:09 ` Alexander Holler 2014-05-28 21:49 ` Brian Norris 2014-05-28 21:49 ` Brian Norris 2014-05-29 3:53 ` Alexander Holler 2014-05-29 3:53 ` Alexander Holler 2014-05-29 6:17 ` Artem Bityutskiy 2014-05-29 6:17 ` Artem Bityutskiy 2014-05-30 9:33 ` Alexander Holler 2014-05-30 9:33 ` Alexander Holler 2014-11-02 21:03 ` Frans Klaver 2014-11-02 21:03 ` Frans Klaver 2014-11-05 9:34 ` Brian Norris 2014-11-05 9:34 ` Brian Norris 2014-11-05 9:48 ` Frans Klaver 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 ` Alexander Holler 2014-05-26 22:12 ` [PATCH 03/27] mtd: nand: omap2: " Alexander Holler 2014-05-26 22:12 ` Alexander Holler 2014-05-26 22:12 ` [PATCH 04/27] mtd: nand: atmel_nand: " Alexander Holler 2014-05-26 22:12 ` Alexander Holler 2014-05-26 22:12 ` [PATCH 05/27] mtd: nand: gpio: " Alexander Holler 2014-05-26 22:12 ` Alexander Holler 2014-05-26 22:12 ` [PATCH 06/27] mtd: nand: fsmc: " Alexander Holler 2014-05-26 22:12 ` Alexander Holler 2014-05-26 22:12 ` [PATCH 07/27] mtd: nand: gpmi: " Alexander Holler 2014-05-26 22:12 ` Alexander Holler 2014-05-26 22:12 ` [PATCH 08/27] mtd: nand: plat: " Alexander Holler 2014-05-26 22:12 ` Alexander Holler 2014-05-26 22:12 ` [PATCH 09/27] mtd: nand: pxa3xx: " Alexander Holler 2014-05-26 22:12 ` Alexander Holler 2014-05-27 3:12 ` Alexander Shiyan 2014-05-27 3:12 ` Alexander Shiyan 2014-05-27 6:01 ` Alexander Holler 2014-05-27 6:01 ` Alexander Holler 2014-05-28 7:25 ` Brian Norris 2014-05-28 7:25 ` Brian Norris 2014-08-08 17:04 ` Ezequiel Garcia 2014-08-08 17:04 ` Ezequiel Garcia 2014-08-08 17:16 ` Brian Norris 2014-08-08 17:16 ` Brian Norris 2014-08-08 18:11 ` Ezequiel Garcia 2014-08-08 18:11 ` Ezequiel Garcia 2014-08-08 20:31 ` Alexander Holler 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 ` Alexander Holler 2014-05-26 22:12 ` [PATCH 11/27] mtd: nand: sh_flctl: " Alexander Holler 2014-05-26 22:12 ` Alexander Holler 2014-05-26 22:12 ` [PATCH 12/27] mtd: nand: sharpsl: " Alexander Holler 2014-05-26 22:12 ` Alexander Holler 2014-05-26 22:12 ` [PATCH 13/27] mtd: nand: tmio: " Alexander Holler 2014-05-26 22:12 ` Alexander Holler 2014-05-26 22:12 ` [PATCH 14/27] mtd: nand: docg4: " Alexander Holler 2014-05-26 22:12 ` 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 ` Alexander Holler 2014-05-26 22:12 ` [PATCH 16/27] mtd: nand: lpc32xx_mlc: " Alexander Holler 2014-05-26 22:12 ` Alexander Holler 2014-05-26 22:12 ` [PATCH 17/27] mtd: nand: lpc32xx_slc: " Alexander Holler 2014-05-26 22:12 ` Alexander Holler 2014-05-26 22:12 ` [PATCH 18/27] mtd: nand: mxc: " Alexander Holler 2014-05-26 22:12 ` 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 ` Alexander Holler 2014-05-26 22:12 ` [PATCH 20/27] mtd: nand: fsl_elbc: " Alexander Holler 2014-05-26 22:12 ` Alexander Holler 2014-05-26 22:12 ` [PATCH 21/27] mtd: nand: fsl_upm: " Alexander Holler 2014-05-26 22:12 ` Alexander Holler 2014-05-26 22:12 ` [PATCH 22/27] mtd: nand: fsl_ifc: " Alexander Holler 2014-05-26 22:12 ` Alexander Holler 2014-05-26 22:12 ` [PATCH 23/27] mtd: nand: jz4740: " Alexander Holler 2014-05-26 22:12 ` Alexander Holler 2014-05-26 22:12 ` [PATCH 24/27] mtd: nand: mpc5121_nfc: " Alexander Holler 2014-05-26 22:12 ` Alexander Holler 2014-05-26 22:12 ` [PATCH 25/27] mtd: nand: ndfc: " Alexander Holler 2014-05-26 22:12 ` Alexander Holler 2014-05-26 22:12 ` [PATCH 26/27] mtd: nand: txx9ndfmc: " Alexander Holler 2014-05-26 22:12 ` Alexander Holler 2014-05-26 22:12 ` [PATCH 27/27] mtd: nand: socrates: use mtd_setup_common_members() Alexander Holler 2014-05-26 22:12 ` 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:26 ` Alexander Holler 2014-05-27 6:28 ` [PATCH 09/27 v2] mtd: nand: pxa3xx: " Alexander Holler 2014-05-27 6:28 ` 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-16 6:37 ` Alexander Holler 2014-10-17 9:14 ` Alexander Holler 2014-10-17 9:14 ` Alexander Holler 2014-10-17 10:53 ` Frans Klaver 2014-10-17 10:53 ` Frans Klaver 2014-10-17 15:54 ` Alexander Holler 2014-10-17 15:54 ` Alexander Holler 2014-10-19 21:24 ` Frans Klaver 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=53863056.2070405@ahsoftware.de \ --to=holler@ahsoftware.de \ --cc=computersforpeace@gmail.com \ --cc=dwmw2@infradead.org \ --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: linkBe 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.