mmc: card: modify mmc_getgeo function
Subsystems (2)
PATCH 1/1 mmc: card: modify mmc_getgeo function
In the earlier code the cylinder, sector and head are assigned independently. Current patch generates the cylinder number with the values of sector and head.
This patch only makes they cylinder value to be dependent on the sector and head.
--- drivers/mmc/card/block.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
drivers/mmc/card/block.c +2 -1
@@ -226,9 +226,10 @@ static int mmc_blk_release(struct gendisk *disk, fmode_t mode)static int mmc_blk_getgeo (struct block_device *bdev,struct hd_geometry *geo) { - geo->cylinders =get_capacity (bdev->bd_disk) / (4 *16 ); geo->heads =4 ; geo->sectors =16 ; + geo->cylinders =get_capacity (bdev->bd_disk) / + (geo->heads * geo->sectors);return 0 ; }
Signature · 1 line
1.7.1
Hi,
Quoted 16 lines · Girish K S
In the earlier code the cylinder, sector and head are assigned independently. Current patch generates the cylinder number with the values of sector and head.
This patch only makes they cylinder value to be dependent on the sector and head.Signed-off-by Girish K S <[email protected]>--- drivers/mmc/card/block.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)drivers/mmc/card/block.c +0
-0
drivers/mmc/card/block.c static int mmc_blk_release(struct gendisk *disk, fmode_t mode) · line 234
Quoted 10 lines
drivers/mmc/card/block.c +2
-1@@ -226,9 +226,10 @@ static int mmc_blk_release(struct gendisk *disk, fmode_t mode)static int mmc_blk_getgeo (struct block_device *bdev,struct hd_geometry *geo) { - geo->cylinders =get_capacity (bdev->bd_disk) / (4 *16 ); geo->heads =4 ; geo->sectors =16 ; + geo->cylinders =get_capacity (bdev->bd_disk) / + (geo->heads * geo->sectors);return 0 ; }
Thanks, pushed to mmc-next for 3.2 with a reworded commit message:
Author: Girish K S <[email protected]> Date: Tue Sep 20 16:38:49 2011 +0530
mmc: card: Remove duplicated constants
Reuse heads/sectors instead of duplicating them in the cylinders calculation.
- Chris.
Signature · 2 lines
Chris Ball <[email protected]> <http://printf.net/> One Laptop Per Child
PATCH 1/1 mmc: card: modify mmc_getgeo function
Hello,
Quoted 23 lines · Chris Ball
Hi,
Quoted 20 lines · Girish K S
In the earlier code the cylinder, sector and head are assigned independently. Current patch generates the cylinder number with the values of sector and head.
This patch only makes they cylinder value to be dependent on the sector and head.Signed-off-by Girish K S <[email protected]>--- drivers/mmc/card/block.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)drivers/mmc/card/block.c +0
-1@@ -226,9 +226,10 @@ static int mmc_blk_release(struct gendisk *disk, fmode_t mode)static int mmc_blk_getgeo (struct block_device *bdev,struct hd_geometry *geo) { - geo->cylinders =get_capacity (bdev->bd_disk) / (4 *16 );
Quoted 8 lines
drivers/mmc/card/block.c static int mmc_blk_release(struct gendisk *disk, fmode_t mode) · line 234
Quoted 6 lines
drivers/mmc/card/block.c +2
-0geo->heads =4 ; geo->sectors =16 ; + geo->cylinders =get_capacity (bdev->bd_disk) / + (geo->heads * geo->sectors);return 0 ; }Thanks, pushed to mmc-next for 3.2 with a reworded commit message:
This (i.e. ee9e0e0 (mmc: card: Remove duplicated constants) in next) makes gcc emit a reference to __aeabi_uldivmod in one of my nightly builds which isn't defined.
The final linking stage fails with:
LD .tmp_vmlinux1 drivers/built-in.o: In function `mmc_blk_getgeo': clkdev.c:(.text+0xd1528): undefined reference to `__aeabi_uldivmod' make[2]: *** [.tmp_vmlinux1] Error 1 make[1]: *** [sub-make] Error 2 make: *** [all] Error 2
(I don't know why clkdev.c is referenced here. I'm not using ccache.)
It seems gcc isn't smart enough to notice that it can just use the same generated code ...
Having said that AFAIK the code used before wasn't ok, too. (I.e. an u64 division that was just noticed to be a shift by luck.)
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
PATCH 1/1 mmc: card: modify mmc_getgeo function
Hi,
Quoted 5 lines · Uwe Kleine-K?nig
Quoted 1 line
Thanks, pushed to mmc-next for 3.2 with a reworded commit message:
This (i.e. ee9e0e0 (mmc: card: Remove duplicated constants) in next) makes gcc emit a reference to
__aeabi_uldivmodin one of my nightly builds which isn't defined.
Quoted 8 lines
The final linking stage fails with:
LD .tmp_vmlinux1 drivers/built-in.o: In function `mmc_blk_getgeo': clkdev.c:(.text+0xd1528): undefined reference to `__aeabi_uldivmod' make[2]: *** [.tmp_vmlinux1] Error 1 make[1]: *** [sub-make] Error 2 make: *** [all] Error 2
Interesting, thanks. It builds fine here on my (gcc-4.6) ARM toolchains. Looking online, I think you're hitting an old gcc-4.3 bug?
- Chris.
Signature · 2 lines
Chris Ball <[email protected]> <http://printf.net/> One Laptop Per Child
PATCH 1/1 mmc: card: modify mmc_getgeo function
Quoted 17 lines · Chris Ball
Hi,
Quoted 5 lines · Uwe Kleine-K?nig
Quoted 1 line
Thanks, pushed to mmc-next for 3.2 with a reworded commit message:
This (i.e. ee9e0e0 (mmc: card: Remove duplicated constants) in next) makes gcc emit a reference to
__aeabi_uldivmodin one of my nightly builds which isn't defined.Quoted 8 lines
The final linking stage fails with:
LD .tmp_vmlinux1 drivers/built-in.o: In function `mmc_blk_getgeo': clkdev.c:(.text+0xd1528): undefined reference to `__aeabi_uldivmod' make[2]: *** [.tmp_vmlinux1] Error 1 make[1]: *** [sub-make] Error 2 make: *** [all] Error 2
Quoted 2 lines
Interesting, thanks. It builds fine here on my (gcc-4.6) ARM toolchains. Looking online, I think you're hitting an old gcc-4.3 bug?
Check your setting of CONFIG_LBDAF - the return type from get_capacity depends on this (which may be either unsigned long or u64).
Now, the thing about a constant division by (16*4) is that its relatively easy for gcc to spot that this is the same as a shift - and use a shift instead of a divide for both the unsigned long and u64 cases.
However, the change may result in gcc no longer realizing that it's a constant division by a power-of-2, and that optimization can be applied.
If you want to eliminate these constants, I'd suggest two definitions
MMC_GEO_SECTORS
MMC_GEO_HEADS
and just subsituting the '4' and '16' in the function with the appropriate symbolic constants. That'd avoid causing this regression.
PATCH 1/1 mmc: card: modify mmc_getgeo function
In reply to Chris BallQuoted 17 lines · Chris Ball
Hi,
Quoted 5 lines · Uwe Kleine-K?nig
Quoted 1 line
Thanks, pushed to mmc-next for 3.2 with a reworded commit message:
This (i.e. ee9e0e0 (mmc: card: Remove duplicated constants) in next) makes gcc emit a reference to
__aeabi_uldivmodin one of my nightly builds which isn't defined.Quoted 8 lines
The final linking stage fails with:
LD .tmp_vmlinux1 drivers/built-in.o: In function `mmc_blk_getgeo': clkdev.c:(.text+0xd1528): undefined reference to `__aeabi_uldivmod' make[2]: *** [.tmp_vmlinux1] Error 1 make[1]: *** [sub-make] Error 2 make: *** [all] Error 2
Quoted 2 lines
Interesting, thanks. It builds fine here on my (gcc-4.6) ARM toolchains. Looking online, I think you're hitting an old gcc-4.3 bug?
Yeah, this is gcc 4.3.2. Is it too old to be supported?
Do you think of http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48783? Then no, that is not the problem. The function is actually used:
00000028 <mmc_blk_getgeo>:
28: e3a03004 mov r3, #4
2c: e3a02010 mov r2, #16
30: e92d4010 push {r4, lr}
34: e1a04001 mov r4, r1
38: e5c13000 strb r3, [r1]
3c: e5c12001 strb r2, [r1, #1]
40: e590c050 ldr ip, [r0, #80] ; 0x50
44: e3a02040 mov r2, #64 ; 0x40
48: e3a03000 mov r3, #0
4c: e1cc04d8 ldrd r0, [ip, #72] ; 0x48
50: ebfffffe bl 0 <__aeabi_uldivmod>
54: e1c400b2 strh r0, [r4, #2]
58: e3a00000 mov r0, #0
5c: e8bd8010 pop {r4, pc}I thought that the problem is more:
> The kernel doesn't support 64-bit by 64-bit division at all then? Nope. 64-bit by 64-bit divides are grossly inefficient and should be avoided.
(taken from http://www.spinics.net/lists/arm/msg13532.html)
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
PATCH 1/1 mmc: card: modify mmc_getgeo function
Quoted 13 lines · Uwe Kleine-K?nig
00000028 <
mmc_blk_getgeo>:28: e3a03004 mov r3, #4 2c: e3a02010 mov r2, #16 30: e92d4010 push {r4, lr} 34: e1a04001 mov r4, r1 38: e5c13000 strb r3, [r1] 3c: e5c12001 strb r2, [r1, #1] 40: e590c050 ldr ip, [r0, #80] ; 0x50 44: e3a02040 mov r2, #64 ; 0x40 48: e3a03000 mov r3, #0 4c: e1cc04d8 ldrd r0, [ip, #72] ; 0x48 50: ebfffffe bl 0 <__aeabi_uldivmod> 54: e1c400b2 strh r0, [r4, #2]
Yes, this is just silly. A 64-bit by 64-bit division by a power-of-2 value to ultimately store a 16-bit value.
Even truncating the output from get_capacity() would be better and no less (in)correct (it may look weird but what the assembly shows is that it really doesn't matter). u64 / 64 will give the same 16-bit result as u64-truncated-to-u32 / 64.