| Message ID | 1440592108-3740-16-git-send-email-holler@ahsoftware.de |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi Alexander, No judgment here for the rest of this series, but for this patch: On Wed, Aug 26, 2015 at 02:28:27PM +0200, Alexander Holler wrote: > The mtd-core has to be initialized before other dependent mtd-drivers, > otherwise a crash might occur. > > Currently mtd_init() is called in the initcall-level device, which is the > same level where most mtd-drivers will end up. By luck this seemed to have > been called most of the time before other mtd-drivers without having been > explicitly enforced. I can't really speak for the original authors, but it does not appear to be entirely "by luck." Link order was one of the de facto ways to get this ordering (though it's not really a great one), and mtdcore was always linked first within the drivers/mtd/ directory structure. But that's just background, I think this is worth fixing anyway. It could, for instance, become a problem if drivers are located outside drivers/mtd/; I see random board files in arch/ that register with MTD, and I'm actually not sure how they have never tripped on this. > But if mtd_init() is not called before a dependent > driver, a null-pointer exception might occur (e.g. because the mtd device > class isn't registered). > > To fix this, mtd-init() is moved to the initcall-level fs (right before > the standard initcall level device). Is there a good reason we shouldn't just make this a subsys_initcall()? That would match the naming better, and it mirrors what, e.g., block/genhd uses. It would also allow flexibility if there are other current/future use cases that might need to sit between the subsystem and the drivers. > Signed-off-by: Alexander Holler <holler@ahsoftware.de> > --- > drivers/mtd/mtdcore.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index 8bbbb75..fa8e6452 100644 > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -1303,7 +1303,7 @@ static void __exit cleanup_mtd(void) > bdi_destroy(&mtd_bdi); > } > > -module_init(init_mtd); > +fs_initcall_sync(init_mtd); Why the *_sync() version? init_mtd() is very simple and doesn't have any multithreading issues to handle. > module_exit(cleanup_mtd); > > MODULE_LICENSE("GPL"); > -- > 2.1.0 > Regards, Brian
Am 01.09.2015 um 23:19 schrieb Brian Norris: > Hi Alexander, > > No judgment here for the rest of this series, but for this patch: > > On Wed, Aug 26, 2015 at 02:28:27PM +0200, Alexander Holler wrote: >> The mtd-core has to be initialized before other dependent mtd-drivers, >> otherwise a crash might occur. >> >> Currently mtd_init() is called in the initcall-level device, which is the >> same level where most mtd-drivers will end up. By luck this seemed to have >> been called most of the time before other mtd-drivers without having been >> explicitly enforced. > > I can't really speak for the original authors, but it does not appear to > be entirely "by luck." Link order was one of the de facto ways to get > this ordering (though it's not really a great one), and mtdcore was > always linked first within the drivers/mtd/ directory structure. > > But that's just background, I think this is worth fixing anyway. It > could, for instance, become a problem if drivers are located outside > drivers/mtd/; I see random board files in arch/ that register with MTD, > and I'm actually not sure how they have never tripped on this. I've already found at least a half a dozen other drivers with the same problem through my shuffling of the drivers which all end up in the standard initcall level device. I'm aware that this is based on the link order, which itself is based on linker behaviour (and maybe other things like make or other build tools). I'm just calling it luck, because this is nowhere explicitly stated, at least I've never seen such a statement, neither in any source, nor somewhere in Documentation. So I've choosen the term "by luck" in order to provoke a bit (or to stimulate a discussion about that widespread problem). > >> But if mtd_init() is not called before a dependent >> driver, a null-pointer exception might occur (e.g. because the mtd device >> class isn't registered). >> >> To fix this, mtd-init() is moved to the initcall-level fs (right before >> the standard initcall level device). > > Is there a good reason we shouldn't just make this a subsys_initcall()? > That would match the naming better, and it mirrors what, e.g., > block/genhd uses. It would also allow flexibility if there are other > current/future use cases that might need to sit between the subsystem > and the drivers. No real reason here. The names for the initcall levels seem to be outdated since a long time anyway, and I think just speaking about the numbers 1-7 (or 0-14) would be better anyways. The only reason why I've used the fs (sync) level is because I was too lazy to check out if mtdcore might depend on something else in any other level. Therefor I've used the one most close to were it was before. Also I've an idea about how to fix that in general for all drivers (by using the same algorithm I've now introduced just for DT-based drivers with a device description). Wouldn't be that hard to use that for all drivers, but as I've written in a follow up to the introductory mail, one step after another. Regards, Alexander Holler
Am 02.09.2015 um 07:34 schrieb Alexander Holler: > Am 01.09.2015 um 23:19 schrieb Brian Norris: >> Hi Alexander, >> >> No judgment here for the rest of this series, but for this patch: >> >> On Wed, Aug 26, 2015 at 02:28:27PM +0200, Alexander Holler wrote: >>> The mtd-core has to be initialized before other dependent mtd-drivers, >>> otherwise a crash might occur. >>> >>> Currently mtd_init() is called in the initcall-level device, which is >>> the >>> same level where most mtd-drivers will end up. By luck this seemed to >>> have >>> been called most of the time before other mtd-drivers without having >>> been >>> explicitly enforced. >> >> I can't really speak for the original authors, but it does not appear to >> be entirely "by luck." Link order was one of the de facto ways to get >> this ordering (though it's not really a great one), and mtdcore was >> always linked first within the drivers/mtd/ directory structure. >> >> But that's just background, I think this is worth fixing anyway. It >> could, for instance, become a problem if drivers are located outside >> drivers/mtd/; I see random board files in arch/ that register with MTD, >> and I'm actually not sure how they have never tripped on this. > > I've already found at least a half a dozen other drivers with the same > problem through my shuffling of the drivers which all end up in the > standard initcall level device. I'm aware that this is based on the link > order, which itself is based on linker behaviour (and maybe other things > like make or other build tools). I'm just calling it luck, because this > is nowhere explicitly stated, at least I've never seen such a statement, > neither in any source, nor somewhere in Documentation. So I've choosen > the term "by luck" in order to provoke a bit (or to stimulate a > discussion about that widespread problem). A good example why "luck" might not be far away from the truth is what happens when a drivers moves e.g. from staging to it's real position. Also the position will stay inside the same initcall level, the move of the driver into another directory (maybe together with a rename) will likely change its position in the initcall-sequence, without anyone really expecting this. >>> But if mtd_init() is not called before a dependent >>> driver, a null-pointer exception might occur (e.g. because the mtd >>> device >>> class isn't registered). >>> >>> To fix this, mtd-init() is moved to the initcall-level fs (right before >>> the standard initcall level device). >> >> Is there a good reason we shouldn't just make this a subsys_initcall()? >> That would match the naming better, and it mirrors what, e.g., >> block/genhd uses. It would also allow flexibility if there are other >> current/future use cases that might need to sit between the subsystem >> and the drivers. > > No real reason here. The names for the initcall levels seem to be > outdated since a long time anyway, and I think just speaking about the > numbers 1-7 (or 0-14) would be better anyways. The only reason why I've > used the fs (sync) level is because I was too lazy to check out if > mtdcore might depend on something else in any other level. Therefor I've > used the one most close to were it was before. Lazy was the wrong term. It is time consuming, cumbersome and therefor error-prone to check on what other stuff a driver depends. One reason why choosing the right place in the initcall sequence isn't that easy and why some automation make sense. > Also I've an idea about how to fix that in general for all drivers (by > using the same algorithm I've now introduced just for DT-based drivers > with a device description). Wouldn't be that hard to use that for all > drivers, but as I've written in a follow up to the introductory mail, > one step after another. > > Regards, > > Alexander Holler
Am 04.09.2015 um 06:00 schrieb Alexander Holler: > Am 02.09.2015 um 07:34 schrieb Alexander Holler: >> Am 01.09.2015 um 23:19 schrieb Brian Norris: >>> Hi Alexander, >>> >>> No judgment here for the rest of this series, but for this patch: >>> >>> On Wed, Aug 26, 2015 at 02:28:27PM +0200, Alexander Holler wrote: >>>> The mtd-core has to be initialized before other dependent mtd-drivers, >>>> otherwise a crash might occur. >>>> >>>> Currently mtd_init() is called in the initcall-level device, which is >>>> the >>>> same level where most mtd-drivers will end up. By luck this seemed to >>>> have >>>> been called most of the time before other mtd-drivers without having >>>> been >>>> explicitly enforced. >>> >>> I can't really speak for the original authors, but it does not appear to >>> be entirely "by luck." Link order was one of the de facto ways to get >>> this ordering (though it's not really a great one), and mtdcore was >>> always linked first within the drivers/mtd/ directory structure. >>> >>> But that's just background, I think this is worth fixing anyway. It >>> could, for instance, become a problem if drivers are located outside >>> drivers/mtd/; I see random board files in arch/ that register with MTD, >>> and I'm actually not sure how they have never tripped on this. As I've just had a look at my patches in order to clean up the patch for parallel initialization (to post it here too): drivers/mtd/ofparts.c has the same problem. In order to let the NAND-driver see the partitions defined in the DT I had to move this into another initcall level (fs sync) too. Regards, Alexander Holler
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index 8bbbb75..fa8e6452 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -1303,7 +1303,7 @@ static void __exit cleanup_mtd(void) bdi_destroy(&mtd_bdi); } -module_init(init_mtd); +fs_initcall_sync(init_mtd); module_exit(cleanup_mtd); MODULE_LICENSE("GPL");
The mtd-core has to be initialized before other dependent mtd-drivers, otherwise a crash might occur. Currently mtd_init() is called in the initcall-level device, which is the same level where most mtd-drivers will end up. By luck this seemed to have been called most of the time before other mtd-drivers without having been explicitly enforced. But if mtd_init() is not called before a dependent driver, a null-pointer exception might occur (e.g. because the mtd device class isn't registered). To fix this, mtd-init() is moved to the initcall-level fs (right before the standard initcall level device). Signed-off-by: Alexander Holler <holler@ahsoftware.de> --- drivers/mtd/mtdcore.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)