Patchwork [06/15] ps3flash: Fix region align checks

login
register
mail settings
Submitter Andre Heider
Date Aug. 1, 2011, 8:02 p.m.
Message ID <1312228986-32307-7-git-send-email-a.heider@gmail.com>
Download mbox | patch
Permalink /patch/107802/
State Superseded
Headers show

Comments

Andre Heider - Aug. 1, 2011, 8:02 p.m.
The region fields used by the align checks are set in
ps3stor_setup(), so move those after that call.

Signed-off-by: Andre Heider <a.heider@gmail.com>
---
 drivers/char/ps3flash.c |   30 +++++++++++++++---------------
 1 files changed, 15 insertions(+), 15 deletions(-)
Geert Uytterhoeven - Aug. 1, 2011, 8:29 p.m.
On Mon, Aug 1, 2011 at 22:02, Andre Heider <a.heider@gmail.com> wrote:
> The region fields used by the align checks are set in
> ps3stor_setup(), so move those after that call.

Are you sure?
Aren't they set in
arch/powerpc/platforms/ps3/device-init.c:ps3_setup_storage_dev()?

> Signed-off-by: Andre Heider <a.heider@gmail.com>
> ---
>  drivers/char/ps3flash.c |   30 +++++++++++++++---------------
>  1 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/char/ps3flash.c b/drivers/char/ps3flash.c
> index 85c004a..69c734a 100644
> --- a/drivers/char/ps3flash.c
> +++ b/drivers/char/ps3flash.c
> @@ -360,21 +360,6 @@ static int __devinit ps3flash_probe(struct ps3_system_bus_device *_dev)
>        int error;
>        unsigned long tmp;
>
> -       tmp = dev->regions[dev->region_idx].start*dev->blk_size;
> -       if (tmp % FLASH_BLOCK_SIZE) {
> -               dev_err(&dev->sbd.core,
> -                       "%s:%u region start %lu is not aligned\n", __func__,
> -                       __LINE__, tmp);
> -               return -EINVAL;
> -       }
> -       tmp = dev->regions[dev->region_idx].size*dev->blk_size;
> -       if (tmp % FLASH_BLOCK_SIZE) {
> -               dev_err(&dev->sbd.core,
> -                       "%s:%u region size %lu is not aligned\n", __func__,
> -                       __LINE__, tmp);
> -               return -EINVAL;
> -       }
> -
>        /* use static buffer, kmalloc cannot allocate 256 KiB */
>        if (!ps3flash_bounce_buffer.address)
>                return -ENODEV;
> @@ -405,6 +390,21 @@ static int __devinit ps3flash_probe(struct ps3_system_bus_device *_dev)
>        if (error)
>                goto fail_free_priv;
>
> +       tmp = dev->regions[dev->region_idx].start*dev->blk_size;
> +       if (tmp % FLASH_BLOCK_SIZE) {
> +               dev_err(&dev->sbd.core,
> +                       "%s:%u region start %lu is not aligned\n", __func__,
> +                       __LINE__, tmp);
> +               return -EINVAL;
> +       }
> +       tmp = dev->regions[dev->region_idx].size*dev->blk_size;
> +       if (tmp % FLASH_BLOCK_SIZE) {
> +               dev_err(&dev->sbd.core,
> +                       "%s:%u region size %lu is not aligned\n", __func__,
> +                       __LINE__, tmp);
> +               return -EINVAL;
> +       }
> +
>        ps3flash_misc.parent = &dev->sbd.core;
>        error = misc_register(&ps3flash_misc);
>        if (error) {

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Andre Heider - Aug. 1, 2011, 8:56 p.m.
On Mon, Aug 1, 2011 at 10:29 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Mon, Aug 1, 2011 at 22:02, Andre Heider <a.heider@gmail.com> wrote:
>> The region fields used by the align checks are set in
>> ps3stor_setup(), so move those after that call.
>
> Are you sure?
> Aren't they set in
> arch/powerpc/platforms/ps3/device-init.c:ps3_setup_storage_dev()?

Hm right, unfortunate commit message... :)
dev->region_idx is set in ps3stor_probe_access(), which is called from
ps3stor_setup().
So the code always checked the first region, where it should check the
one beeing used.

Will fix the commit message.

Thanks
Geert Uytterhoeven - Aug. 1, 2011, 9 p.m.
On Mon, Aug 1, 2011 at 22:56, Andre Heider <a.heider@gmail.com> wrote:
> On Mon, Aug 1, 2011 at 10:29 PM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Mon, Aug 1, 2011 at 22:02, Andre Heider <a.heider@gmail.com> wrote:
>>> The region fields used by the align checks are set in
>>> ps3stor_setup(), so move those after that call.
>>
>> Are you sure?
>> Aren't they set in
>> arch/powerpc/platforms/ps3/device-init.c:ps3_setup_storage_dev()?
>
> Hm right, unfortunate commit message... :)
> dev->region_idx is set in ps3stor_probe_access(), which is called from
> ps3stor_setup().
> So the code always checked the first region, where it should check the
> one beeing used.

IC. You're right. That's indeed a bug.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Patch

diff --git a/drivers/char/ps3flash.c b/drivers/char/ps3flash.c
index 85c004a..69c734a 100644
--- a/drivers/char/ps3flash.c
+++ b/drivers/char/ps3flash.c
@@ -360,21 +360,6 @@  static int __devinit ps3flash_probe(struct ps3_system_bus_device *_dev)
 	int error;
 	unsigned long tmp;
 
-	tmp = dev->regions[dev->region_idx].start*dev->blk_size;
-	if (tmp % FLASH_BLOCK_SIZE) {
-		dev_err(&dev->sbd.core,
-			"%s:%u region start %lu is not aligned\n", __func__,
-			__LINE__, tmp);
-		return -EINVAL;
-	}
-	tmp = dev->regions[dev->region_idx].size*dev->blk_size;
-	if (tmp % FLASH_BLOCK_SIZE) {
-		dev_err(&dev->sbd.core,
-			"%s:%u region size %lu is not aligned\n", __func__,
-			__LINE__, tmp);
-		return -EINVAL;
-	}
-
 	/* use static buffer, kmalloc cannot allocate 256 KiB */
 	if (!ps3flash_bounce_buffer.address)
 		return -ENODEV;
@@ -405,6 +390,21 @@  static int __devinit ps3flash_probe(struct ps3_system_bus_device *_dev)
 	if (error)
 		goto fail_free_priv;
 
+	tmp = dev->regions[dev->region_idx].start*dev->blk_size;
+	if (tmp % FLASH_BLOCK_SIZE) {
+		dev_err(&dev->sbd.core,
+			"%s:%u region start %lu is not aligned\n", __func__,
+			__LINE__, tmp);
+		return -EINVAL;
+	}
+	tmp = dev->regions[dev->region_idx].size*dev->blk_size;
+	if (tmp % FLASH_BLOCK_SIZE) {
+		dev_err(&dev->sbd.core,
+			"%s:%u region size %lu is not aligned\n", __func__,
+			__LINE__, tmp);
+		return -EINVAL;
+	}
+
 	ps3flash_misc.parent = &dev->sbd.core;
 	error = misc_register(&ps3flash_misc);
 	if (error) {