| Message ID | 1337054928-16228-1-git-send-email-gerg@snapgear.com |
|---|---|
| State | New, archived |
| Headers | show |
On 15/05/12 14:08, gerg@snapgear.com wrote: [snip] > /****************************************************************************/ > @@ -65,22 +62,22 @@ static int __init uclinux_mtd_init(void) > mapp->size = PAGE_ALIGN(ntohl(*((unsigned long *)(mapp->phys + 8)))); > mapp->bankwidth = 4; > > - printk("uclinux[mtd]: RAM probe address=0x%x size=0x%x\n", > + printk(KERN_NOTICE "uclinux[mtd]: RAM probe address=0x%x size=0x%x\n", > (int) mapp->phys, (int) mapp->size); > > - mapp->virt = phys_to_virt(mapp->phys); > + mapp->virt = (void __iomem *) (unsigned long) phys_to_virt(mapp->phys); I am a little un-easy with this casting. It would seem to indicate some type of abuse of the virt field... But this same thing has been done in other mtd mapping drivers, for example drivers/mtd/maps/amd76xrom.c. Regards Greg ------------------------------------------------------------------------ Greg Ungerer -- Principal Engineer EMAIL: gerg@snapgear.com SnapGear Group, McAfee PHONE: +61 7 3435 2888 8 Gardner Close FAX: +61 7 3217 5323 Milton, QLD, 4064, Australia WEB: http://www.SnapGear.com
On Tue, 2012-05-15 at 14:08 +1000, gerg@snapgear.com wrote: > From: Greg Ungerer <gerg@uclinux.org> > > Perform a number of cleanups on the uclinux.c map driver. > No structural or semantic changes, only minor cleanups. > > . insert appropriate prefix into printk() calls > . remove redundant "if" checks in the module exit code > . remove unnecessary includes > . make the struct uclinux_ram_map static > . cast the virtual address calculations to keep them sparse clean > > Signed-off-by: Greg Ungerer <gerg@uclinux.org> Thanks, pushed all 3 patches to l2-mtd.git. Side note: why do you Cc uclinux-dev@uclinux.org if it is closed mailing list. Why do you need to make people replying you see rejects from the mailing list?
On Tue, May 15, 2012 at 12:08 AM, <gerg@snapgear.com> wrote:
> . make the struct uclinux_ram_map static
NAK: this breaks Blackfin systems. we specifically don't want this to
be static.
it should probably get a comment added above it saying as much.
-mike
On 15/05/12 23:12, Artem Bityutskiy wrote: > On Tue, 2012-05-15 at 14:08 +1000, gerg@snapgear.com wrote: >> From: Greg Ungerer<gerg@uclinux.org> >> >> Perform a number of cleanups on the uclinux.c map driver. >> No structural or semantic changes, only minor cleanups. >> >> . insert appropriate prefix into printk() calls >> . remove redundant "if" checks in the module exit code >> . remove unnecessary includes >> . make the struct uclinux_ram_map static >> . cast the virtual address calculations to keep them sparse clean >> >> Signed-off-by: Greg Ungerer<gerg@uclinux.org> > > Thanks, pushed all 3 patches to l2-mtd.git. Thanks. > Side note: why do you Cc uclinux-dev@uclinux.org if it is closed mailing > list. Why do you need to make people replying you see rejects from the > mailing list? Most of the users of the uclinux.c map driver will be on the uclinux mailing list. It is very annoying that it is a closed list, but at least there is a chance they know changes are being made to some code they use. Regards Greg ------------------------------------------------------------------------ Greg Ungerer -- Principal Engineer EMAIL: gerg@snapgear.com SnapGear Group, McAfee PHONE: +61 7 3435 2888 8 Gardner Close FAX: +61 7 3217 5323 Milton, QLD, 4064, Australia WEB: http://www.SnapGear.com
On 16/05/12 01:57, Mike Frysinger wrote: > On Tue, May 15, 2012 at 12:08 AM,<gerg@snapgear.com> wrote: >> . make the struct uclinux_ram_map static > > NAK: this breaks Blackfin systems. we specifically don't want this to > be static. > > it should probably get a comment added above it saying as much. A comment won't fix the sparse warning. You need a proper declaration. Regards Greg ------------------------------------------------------------------------ Greg Ungerer -- Principal Engineer EMAIL: gerg@snapgear.com SnapGear Group, McAfee PHONE: +61 7 3435 2888 8 Gardner Close FAX: +61 7 3217 5323 Milton, QLD, 4064, Australia WEB: http://www.SnapGear.com
On Tue, May 15, 2012 at 8:45 PM, Greg Ungerer <gerg@snapgear.com> wrote: > On 16/05/12 01:57, Mike Frysinger wrote: >> On Tue, May 15, 2012 at 12:08 AM,<gerg@snapgear.com> wrote: >>> >>> . make the struct uclinux_ram_map static >> >> >> NAK: this breaks Blackfin systems. we specifically don't want this to >> be static. >> >> it should probably get a comment added above it saying as much. > > A comment won't fix the sparse warning. You need a proper declaration. perhaps, but marking it static to fix a warning that people rarely see whilst simultaneously knowingly breaking an arch doesn't sound like the correct trade off. -mike
On 16/05/12 12:42, Mike Frysinger wrote: > On Tue, May 15, 2012 at 8:45 PM, Greg Ungerer<gerg@snapgear.com> wrote: >> On 16/05/12 01:57, Mike Frysinger wrote: >>> On Tue, May 15, 2012 at 12:08 AM,<gerg@snapgear.com> áwrote: >>>> >>>> . make the struct uclinux_ram_map static >>> >>> >>> NAK: this breaks Blackfin systems. áwe specifically don't want this to >>> be static. >>> >>> it should probably get a comment added above it saying as much. >> >> A comment won't fix the sparse warning. You need a proper declaration. > > perhaps, but marking it static to fix a warning that people rarely see > whilst simultaneously knowingly breaking an arch doesn't sound like > the correct trade off. I agree, of course. It wasn't done to knowingly break an arch. But the sparse warning can be fixed with a proper declaration, that would avoid you having a local extern for it in arch/blackfin/kernel/setup.c as well. Cleaner all round. Regards Greg ------------------------------------------------------------------------ Greg Ungerer -- Principal Engineer EMAIL: gerg@snapgear.com SnapGear Group, McAfee PHONE: +61 7 3435 2888 8 Gardner Close FAX: +61 7 3217 5323 Milton, QLD, 4064, Australia WEB: http://www.SnapGear.com
On Tue, May 15, 2012 at 10:55 PM, Greg Ungerer <gerg@snapgear.com> wrote: > On 16/05/12 12:42, Mike Frysinger wrote: >> On Tue, May 15, 2012 at 8:45 PM, Greg Ungerer<gerg@snapgear.com> wrote: >>> On 16/05/12 01:57, Mike Frysinger wrote: >>>> On Tue, May 15, 2012 at 12:08 AM,<gerg@snapgear.com> áwrote: >>>>> . make the struct uclinux_ram_map static >>>> >>>> NAK: this breaks Blackfin systems. áwe specifically don't want this to >>>> be static. >>>> it should probably get a comment added above it saying as much. >>> >>> A comment won't fix the sparse warning. You need a proper declaration. >> >> perhaps, but marking it static to fix a warning that people rarely see >> whilst simultaneously knowingly breaking an arch doesn't sound like >> the correct trade off. > > I agree, of course. It wasn't done to knowingly break an arch. But > the sparse warning can be fixed with a proper declaration, that > would avoid you having a local extern for it in > arch/blackfin/kernel/setup.c as well. Cleaner all round. i thought you were going for merging anyways. where would you suggest adding such a decl ? there isn't an existing one i can see that this would fit into. might have to create a new one just for this ? -mike
On 05/16/2012 03:02 PM, Mike Frysinger wrote: > On Tue, May 15, 2012 at 10:55 PM, Greg Ungerer<gerg@snapgear.com> wrote: >> On 16/05/12 12:42, Mike Frysinger wrote: >>> On Tue, May 15, 2012 at 8:45 PM, Greg Ungerer<gerg@snapgear.com> áwrote: >>>> On 16/05/12 01:57, Mike Frysinger wrote: >>>>> On Tue, May 15, 2012 at 12:08 AM,<gerg@snapgear.com> á├Ãwrote: >>>>>> . make the struct uclinux_ram_map static >>>>> >>>>> NAK: this breaks Blackfin systems. ├Ãwe specifically don't want this to >>>>> be static. >>>>> it should probably get a comment added above it saying as much. >>>> >>>> A comment won't fix the sparse warning. You need a proper declaration. >>> >>> perhaps, but marking it static to fix a warning that people rarely see >>> whilst simultaneously knowingly breaking an arch doesn't sound like >>> the correct trade off. >> >> I agree, of course. It wasn't done to knowingly break an arch. But >> the sparse warning can be fixed with a proper declaration, that >> would avoid you having a local extern for it in >> arch/blackfin/kernel/setup.c as well. Cleaner all round. > > i thought you were going for merging anyways. > > where would you suggest adding such a decl ? there isn't an existing > one i can see that this would fit into. might have to create a new > one just for this ? No I don't see any existing place that makes any sense. I guess it could be something like a new file include/linux/mtd/uclinux.h. But it looks like Artem is ok with just reverting it to not be static. I am happy to leave it that way if you are. Regards Greg ------------------------------------------------------------------------ Greg Ungerer -- Principal Engineer EMAIL: gerg@snapgear.com SnapGear Group, McAfee PHONE: +61 7 3435 2888 8 Gardner Close, FAX: +61 7 3891 3630 Milton, QLD, 4064, Australia WEB: http://www.SnapGear.com
On Wed, May 16, 2012 at 7:49 AM, Greg Ungerer <gerg@snapgear.com> wrote: > On 05/16/2012 03:02 PM, Mike Frysinger wrote: >> >> On Tue, May 15, 2012 at 10:55 PM, Greg Ungerer<gerg@snapgear.com> wrote: >>> >>> On 16/05/12 12:42, Mike Frysinger wrote: >>>> >>>> On Tue, May 15, 2012 at 8:45 PM, Greg Ungerer<gerg@snapgear.com> >>>> áwrote: >>>>> >>>>> On 16/05/12 01:57, Mike Frysinger wrote: >>>>>> >>>>>> On Tue, May 15, 2012 at 12:08 AM,<gerg@snapgear.com> á├Ãwrote: >>>>>> >>>>>>> . make the struct uclinux_ram_map static >>>>>> >>>>>> >>>>>> NAK: this breaks Blackfin systems. ├Ãwe specifically don't want >>>>>> this to >>>>>> >>>>>> be static. >>>>>> it should probably get a comment added above it saying as much. >>>>> >>>>> >>>>> A comment won't fix the sparse warning. You need a proper declaration. >>>> >>>> >>>> perhaps, but marking it static to fix a warning that people rarely see >>>> whilst simultaneously knowingly breaking an arch doesn't sound like >>>> the correct trade off. >>> >>> >>> I agree, of course. It wasn't done to knowingly break an arch. But >>> the sparse warning can be fixed with a proper declaration, that >>> would avoid you having a local extern for it in >>> arch/blackfin/kernel/setup.c as well. Cleaner all round. >> >> >> i thought you were going for merging anyways. >> >> where would you suggest adding such a decl ? there isn't an existing >> one i can see that this would fit into. might have to create a new >> one just for this ? > > > No I don't see any existing place that makes any sense. I guess it > could be something like a new file include/linux/mtd/uclinux.h. > > But it looks like Artem is ok with just reverting it to not be static. > I am happy to leave it that way if you are. would be good to add a comment so someone doesn't clean this up again. i can post a patch for that though. i know the current struct behavior is ugly, but it's cleaner than previous iterations ;) -mike
diff --git a/drivers/mtd/maps/uclinux.c b/drivers/mtd/maps/uclinux.c index 6d43c75..d91b5b4 100644 --- a/drivers/mtd/maps/uclinux.c +++ b/drivers/mtd/maps/uclinux.c @@ -12,19 +12,16 @@ #include <linux/types.h> #include <linux/init.h> #include <linux/kernel.h> -#include <linux/fs.h> #include <linux/mm.h> -#include <linux/major.h> #include <linux/mtd/mtd.h> #include <linux/mtd/map.h> #include <linux/mtd/partitions.h> -#include <asm/io.h> /****************************************************************************/ extern char _ebss; -struct map_info uclinux_ram_map = { +static struct map_info uclinux_ram_map = { .name = "RAM", .phys = (unsigned long)&_ebss, .size = 0, @@ -46,11 +43,11 @@ static int uclinux_point(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, void **virt, resource_size_t *phys) { struct map_info *map = mtd->priv; - *virt = map->virt + from; + *virt = (void *) (unsigned long) map->virt + from; if (phys) *phys = map->phys + from; *retlen = len; - return(0); + return 0; } /****************************************************************************/ @@ -65,22 +62,22 @@ static int __init uclinux_mtd_init(void) mapp->size = PAGE_ALIGN(ntohl(*((unsigned long *)(mapp->phys + 8)))); mapp->bankwidth = 4; - printk("uclinux[mtd]: RAM probe address=0x%x size=0x%x\n", + printk(KERN_NOTICE "uclinux[mtd]: RAM probe address=0x%x size=0x%x\n", (int) mapp->phys, (int) mapp->size); - mapp->virt = phys_to_virt(mapp->phys); + mapp->virt = (void __iomem *) (unsigned long) phys_to_virt(mapp->phys); - if (mapp->virt == 0) { - printk("uclinux[mtd]: no virtual mapping?\n"); - return(-EIO); + if (mapp->virt == NULL) { + printk(KERN_ERR "uclinux[mtd]: no virtual mapping?\n"); + return -EIO; } simple_map_init(mapp); mtd = do_map_probe("map_ram", mapp); if (!mtd) { - printk("uclinux[mtd]: failed to find a mapping?\n"); - return(-ENXIO); + printk(KERN_ERR "uclinux[mtd]: failed to find a mapping?\n"); + return -ENXIO; } mtd->owner = THIS_MODULE; @@ -90,20 +87,15 @@ static int __init uclinux_mtd_init(void) uclinux_ram_mtdinfo = mtd; mtd_device_register(mtd, uclinux_romfs, NUM_PARTITIONS); - return(0); + return 0; } /****************************************************************************/ static void __exit uclinux_mtd_cleanup(void) { - if (uclinux_ram_mtdinfo) { - mtd_device_unregister(uclinux_ram_mtdinfo); - map_destroy(uclinux_ram_mtdinfo); - uclinux_ram_mtdinfo = NULL; - } - if (uclinux_ram_map.virt) - uclinux_ram_map.virt = 0; + mtd_device_unregister(uclinux_ram_mtdinfo); + map_destroy(uclinux_ram_mtdinfo); } /****************************************************************************/