[PATCH] mmc: card: modify mmc_getgeo function

[PATCH] mmc: card: modify mmc_getgeo function

[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 <girish.shivananjappa@linaro.org>
---
 drivers/mmc/card/block.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 1ff5486..bebb13b 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -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;
 }
 
-- 
1.7.1

Re: [PATCH] mmc: card: modify mmc_getgeo function

[Chris Ball]

Hi,

On Tue, Sep 20 2011, Girish K S wrote:
> 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 <girish.shivananjappa@linaro.org>
> ---
>  drivers/mmc/card/block.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index 1ff5486..bebb13b 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -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 <girish.shivananjappa@linaro.org>
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.

    Signed-off-by: Girish K S <girish.shivananjappa@linaro.org>
    Signed-off-by: Chris Ball <cjb@laptop.org>                    

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] mmc: card: modify mmc_getgeo function

[Uwe Kleine-König]

Hello,

On Wed, Sep 21, 2011 at 02:52:08PM -0400, Chris Ball wrote:
> Hi,
> 
> On Tue, Sep 20 2011, Girish K S wrote:
> > 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 <girish.shivananjappa@linaro.org>
> > ---
> >  drivers/mmc/card/block.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> > index 1ff5486..bebb13b 100644
> > --- a/drivers/mmc/card/block.c
> > +++ b/drivers/mmc/card/block.c
> > @@ -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:
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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] mmc: card: modify mmc_getgeo function

[Uwe Kleine-König]

Hello,

On Wed, Sep 21, 2011 at 02:52:08PM -0400, Chris Ball wrote:
> Hi,
> 
> On Tue, Sep 20 2011, Girish K S wrote:
> > 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 <girish.shivananjappa@linaro.org>
> > ---
> >  drivers/mmc/card/block.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> > index 1ff5486..bebb13b 100644
> > --- a/drivers/mmc/card/block.c
> > +++ b/drivers/mmc/card/block.c
> > @@ -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:
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

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] mmc: card: modify mmc_getgeo function

[Chris Ball]

Hi,

On Mon, Sep 26 2011, Uwe Kleine-König wrote:
>> 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

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.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

[PATCH] mmc: card: modify mmc_getgeo function

[Chris Ball]

Hi,

On Mon, Sep 26 2011, Uwe Kleine-K?nig wrote:
>> 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

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.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] mmc: card: modify mmc_getgeo function

[Russell King - ARM Linux]

On Mon, Sep 26, 2011 at 08:41:13AM -0400, Chris Ball wrote:
> Hi,
> 
> On Mon, Sep 26 2011, Uwe Kleine-König wrote:
> >> 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
> 
> 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] mmc: card: modify mmc_getgeo function

[Russell King - ARM Linux]

On Mon, Sep 26, 2011 at 08:41:13AM -0400, Chris Ball wrote:
> Hi,
> 
> On Mon, Sep 26 2011, Uwe Kleine-K?nig wrote:
> >> 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
> 
> 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.

Re: [PATCH] mmc: card: modify mmc_getgeo function

[Uwe Kleine-König]

On Mon, Sep 26, 2011 at 08:41:13AM -0400, Chris Ball wrote:
> Hi,
> 
> On Mon, Sep 26 2011, Uwe Kleine-König wrote:
> >> 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
> 
> 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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] mmc: card: modify mmc_getgeo function

[Uwe Kleine-König]

On Mon, Sep 26, 2011 at 08:41:13AM -0400, Chris Ball wrote:
> Hi,
> 
> On Mon, Sep 26 2011, Uwe Kleine-K?nig wrote:
> >> 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
> 
> 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

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] mmc: card: modify mmc_getgeo function

[Russell King - ARM Linux]

On Mon, Sep 26, 2011 at 03:13:33PM +0200, Uwe Kleine-König wrote:
> 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.

[PATCH] mmc: card: modify mmc_getgeo function

[Russell King - ARM Linux]

On Mon, Sep 26, 2011 at 03:13:33PM +0200, Uwe Kleine-K?nig wrote:
> 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.