[U-Boot] [PATCH] part_efi: fix protective_mbr struct allocation

[U-Boot] [PATCH] part_efi: fix protective_mbr struct allocation

[Hector Palacios]

The calloc() call was allocating space for the sizeof the struct
pointer rather than for the struct contents.

Signed-off-by: Hector Palacios <hector.palacios@digi.com>
---
 disk/part_efi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/disk/part_efi.c b/disk/part_efi.c
index 5dfaf490c89a..7fabec059d7a 100644
--- a/disk/part_efi.c
+++ b/disk/part_efi.c
@@ -232,7 +232,7 @@ static int set_protective_mbr(block_dev_desc_t *dev_desc)
 	legacy_mbr *p_mbr;
 
 	/* Setup the Protective MBR */
-	p_mbr = calloc(1, sizeof(p_mbr));
+	p_mbr = calloc(1, sizeof(legacy_mbr));
 	if (p_mbr == NULL) {
 		printf("%s: calloc failed!\n", __func__);
 		return -1;

[U-Boot] [PATCH] part_efi: fix protective_mbr struct allocation

[Fabio Estevam]

On Wed, Feb 12, 2014 at 12:40 PM, Hector Palacios
<hector.palacios@digi.com> wrote:
> The calloc() call was allocating space for the sizeof the struct
> pointer rather than for the struct contents.
>
> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
> ---
>  disk/part_efi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/disk/part_efi.c b/disk/part_efi.c
> index 5dfaf490c89a..7fabec059d7a 100644
> --- a/disk/part_efi.c
> +++ b/disk/part_efi.c
> @@ -232,7 +232,7 @@ static int set_protective_mbr(block_dev_desc_t *dev_desc)
>         legacy_mbr *p_mbr;
>
>         /* Setup the Protective MBR */
> -       p_mbr = calloc(1, sizeof(p_mbr));
> +       p_mbr = calloc(1, sizeof(legacy_mbr));

What about:

p_mbr = calloc(1, sizeof(*p_mbr)) ?


Regards,

Fabio Estevam

[U-Boot] [PATCH] part_efi: fix protective_mbr struct allocation

[Albert ARIBAUD]

Hi Fabio,

On Wed, 12 Feb 2014 12:43:02 -0200, Fabio Estevam <festevam@gmail.com>
wrote:

> On Wed, Feb 12, 2014 at 12:40 PM, Hector Palacios
> <hector.palacios@digi.com> wrote:
> > The calloc() call was allocating space for the sizeof the struct
> > pointer rather than for the struct contents.
> >
> > Signed-off-by: Hector Palacios <hector.palacios@digi.com>
> > ---
> >  disk/part_efi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/disk/part_efi.c b/disk/part_efi.c
> > index 5dfaf490c89a..7fabec059d7a 100644
> > --- a/disk/part_efi.c
> > +++ b/disk/part_efi.c
> > @@ -232,7 +232,7 @@ static int set_protective_mbr(block_dev_desc_t *dev_desc)
> >         legacy_mbr *p_mbr;
> >
> >         /* Setup the Protective MBR */
> > -       p_mbr = calloc(1, sizeof(p_mbr));
> > +       p_mbr = calloc(1, sizeof(legacy_mbr));
> 
> What about:
> 
> p_mbr = calloc(1, sizeof(*p_mbr)) ?

I don't like the idea of setting p_mbr based on *p_mbr at a time where
p_mbr is still undefined. I know that from a C standard perspective
this is ok, but I'd rather simply not run any risk and pass sizeof
the struct type, not a (non-existent) dereferenced 'value'.

> Regards,
> 
> Fabio Estevam

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] part_efi: fix protective_mbr struct allocation

[Fabio Estevam]

Hi Albert,

On Wed, Feb 12, 2014 at 2:33 PM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:

>> What about:
>>
>> p_mbr = calloc(1, sizeof(*p_mbr)) ?
>
> I don't like the idea of setting p_mbr based on *p_mbr at a time where
> p_mbr is still undefined. I know that from a C standard perspective
> this is ok, but I'd rather simply not run any risk and pass sizeof
> the struct type, not a (non-existent) dereferenced 'value'.

At least in kernel this is the preferred way.

According to Documentation/CodingStyle:

"Chapter 14: Allocating memory

The preferred form for passing a size of a struct is the following:

    p = kmalloc(sizeof(*p), ...);

The alternative form where struct name is spelled out hurts readability and
introduces an opportunity for a bug when the pointer variable type is changed
but the corresponding sizeof that is passed to a memory allocator is not."

Regards,

Fabio Estevam

[U-Boot] [PATCH] part_efi: fix protective_mbr struct allocation

[Albert ARIBAUD]

Hi Fabio,

On Wed, 12 Feb 2014 15:33:17 -0200, Fabio Estevam <festevam@gmail.com>
wrote:

> Hi Albert,
> 
> On Wed, Feb 12, 2014 at 2:33 PM, Albert ARIBAUD
> <albert.u.boot@aribaud.net> wrote:
> 
> >> What about:
> >>
> >> p_mbr = calloc(1, sizeof(*p_mbr)) ?
> >
> > I don't like the idea of setting p_mbr based on *p_mbr at a time where
> > p_mbr is still undefined. I know that from a C standard perspective
> > this is ok, but I'd rather simply not run any risk and pass sizeof
> > the struct type, not a (non-existent) dereferenced 'value'.
> 
> At least in kernel this is the preferred way.
> 
> According to Documentation/CodingStyle:
> 
> "Chapter 14: Allocating memory
> 
> The preferred form for passing a size of a struct is the following:
> 
>     p = kmalloc(sizeof(*p), ...);
> 
> The alternative form where struct name is spelled out hurts readability and
> introduces an opportunity for a bug when the pointer variable type is changed
> but the corresponding sizeof that is passed to a memory allocator is not."

I still don't like it, but it makes sense indeed.

> Regards,
> 
> Fabio Estevam

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] part_efi: fix protective_mbr struct allocation

[Lukasz Majewski]

Hi Hector,

> The calloc() call was allocating space for the sizeof the struct
> pointer rather than for the struct contents.
> 
> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
> ---
>  disk/part_efi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/disk/part_efi.c b/disk/part_efi.c
> index 5dfaf490c89a..7fabec059d7a 100644
> --- a/disk/part_efi.c
> +++ b/disk/part_efi.c
> @@ -232,7 +232,7 @@ static int set_protective_mbr(block_dev_desc_t
> *dev_desc) legacy_mbr *p_mbr;
>  
>  	/* Setup the Protective MBR */
> -	p_mbr = calloc(1, sizeof(p_mbr));
> +	p_mbr = calloc(1, sizeof(legacy_mbr));

Thanks for spotting the error. _Damn_

However, this is not enough. 

Since this buffer is passed to mmc for writing (and some targets may
use cache) the legacy_mbr shall be defined as:

ALLOC_CACHE_ALIGN_BUFFER(legacy_mbr, p_mbr, sizeof(legacy_mbr));
memset(p_mbr, 0, sizeof(legacy_mbr));

Would you like to prepare v2 of this patch or shall I prepare the fix?

>  	if (p_mbr == NULL) {
>  		printf("%s: calloc failed!\n", __func__);
>  		return -1;



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH] part_efi: fix protective_mbr struct allocation

[Palacios, Hector]

Hi Lukasz,

On 02/12/2014 04:56 PM, Lukasz Majewski wrote:
> Hi Hector,
>
>> The calloc() call was allocating space for the sizeof the struct
>> pointer rather than for the struct contents.
>>
>> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
>> ---
>>   disk/part_efi.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/disk/part_efi.c b/disk/part_efi.c
>> index 5dfaf490c89a..7fabec059d7a 100644
>> --- a/disk/part_efi.c
>> +++ b/disk/part_efi.c
>> @@ -232,7 +232,7 @@ static int set_protective_mbr(block_dev_desc_t
>> *dev_desc) legacy_mbr *p_mbr;
>>
>>   	/* Setup the Protective MBR */
>> -	p_mbr = calloc(1, sizeof(p_mbr));
>> +	p_mbr = calloc(1, sizeof(legacy_mbr));
>
> Thanks for spotting the error. _Damn_
>
> However, this is not enough.
>
> Since this buffer is passed to mmc for writing (and some targets may
> use cache) the legacy_mbr shall be defined as:
>
> ALLOC_CACHE_ALIGN_BUFFER(legacy_mbr, p_mbr, sizeof(legacy_mbr));
> memset(p_mbr, 0, sizeof(legacy_mbr));

Unfortunately this is causing unaligned access in my i.MX6.
I'm specifically passing the -mno-unaligned-access when building this file so I guess 
it has to do with the macro and the packed structure.

>
> Would you like to prepare v2 of this patch or shall I prepare the fix?
>
>>   	if (p_mbr == NULL) {
>>   		printf("%s: calloc failed!\n", __func__);
>>   		return -1;
>
>
>

[U-Boot] [PATCH] part_efi: fix protective_mbr struct allocation

[Albert ARIBAUD]

Hi Hector,

On Wed, 12 Feb 2014 17:24:26 +0100, "Palacios, Hector"
<Hector.Palacios@digi.com> wrote:

> Unfortunately this is causing unaligned access in my i.MX6.
> I'm specifically passing the -mno-unaligned-access when building this file so I guess 
> it has to do with the macro and the packed structure.

I don't think this is due to packed structures, because
-mno-unaligned-access tells the compiler that unaligned fields in
packed structures should be accesses by breaking the unaligned access
into smaller aligned ones, so this would not cause an alignment abort.

What can cause an alignment abort despite -mno-unaligned-access is
dereferencing a badly aligned pointer, and this is probably what
happens here.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] part_efi: fix protective_mbr struct allocation

[Palacios, Hector]

Hi Albert,

On 02/12/2014 05:31 PM, Albert ARIBAUD wrote:
> Hi Hector,
>
> On Wed, 12 Feb 2014 17:24:26 +0100, "Palacios, Hector"
> <Hector.Palacios@digi.com> wrote:
>
>> Unfortunately this is causing unaligned access in my i.MX6.
>> I'm specifically passing the -mno-unaligned-access when building this file so I guess
>> it has to do with the macro and the packed structure.
>
> I don't think this is due to packed structures, because
> -mno-unaligned-access tells the compiler that unaligned fields in
> packed structures should be accesses by breaking the unaligned access
> into smaller aligned ones, so this would not cause an alignment abort.
>
> What can cause an alignment abort despite -mno-unaligned-access is
> dereferencing a badly aligned pointer, and this is probably what
> happens here.

Oh, the macro ALLOC_CACHE_ALIGN_BUFFER is returning a correctly aligned value for the 
pointer: 0x4FD33BA0
The problem is we don't have to free() the pointer anymore at the end of the function.

I'll repost the patch
-- 
H?ctor Palacios

[U-Boot] [PATCH] part_efi: fix protective_mbr struct allocation

[Lukasz Majewski]

Hi Hector,

> Hi Albert,
> 
> On 02/12/2014 05:31 PM, Albert ARIBAUD wrote:
> > Hi Hector,
> >
> > On Wed, 12 Feb 2014 17:24:26 +0100, "Palacios, Hector"
> > <Hector.Palacios@digi.com> wrote:
> >
> >> Unfortunately this is causing unaligned access in my i.MX6.
> >> I'm specifically passing the -mno-unaligned-access when building
> >> this file so I guess it has to do with the macro and the packed
> >> structure.
> >
> > I don't think this is due to packed structures, because
> > -mno-unaligned-access tells the compiler that unaligned fields in
> > packed structures should be accesses by breaking the unaligned
> > access into smaller aligned ones, so this would not cause an
> > alignment abort.
> >
> > What can cause an alignment abort despite -mno-unaligned-access is
> > dereferencing a badly aligned pointer, and this is probably what
> > happens here.
> 
> Oh, the macro ALLOC_CACHE_ALIGN_BUFFER is returning a correctly
> aligned value for the pointer: 0x4FD33BA0
> The problem is we don't have to free() the pointer anymore at the end
> of the function.
> 
> I'll repost the patch

I think, that the patch for fixing the unaligned access in this
function has already been posted by Piotr Wilczek. We have experienced
similar issues with Samsung's Exynos4 based targets.

[PATCH V2] disk:efi: avoid unaligned access on efi partition

Despite its disappearance from patchwork it shall be available at
mailing list archive.

The v1 can be found at the following link:
http://patchwork.ozlabs.org/patch/282753/


Inclusion of v2 has been postponed since there was a discussion if we
shall allow unaligned access (-mno-unaligned-access flag) at armv7
(after patches posted by Tom).

As fair as I can tell, we will keep the current approach so, I think
that Tom will be willing to pull this patch (v2) now.

Best regards,
Lukasz Majewski
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140212/3b2b8e7f/attachment.pgp>

[U-Boot] [PATCH] part_efi: fix protective_mbr struct allocation

[Albert ARIBAUD]

Hi Lukasz,

On Wed, 12 Feb 2014 21:45:41 +0100, Lukasz Majewski
<l.majewski@majess.pl> wrote:

> I think, that the patch for fixing the unaligned access in this
> function has already been posted by Piotr Wilczek. We have experienced
> similar issues with Samsung's Exynos4 based targets.
> 
> [PATCH V2] disk:efi: avoid unaligned access on efi partition
> 
> Despite its disappearance from patchwork it shall be available at
> mailing list archive.
> 
> The v1 can be found at the following link:
> http://patchwork.ozlabs.org/patch/282753/

Neither V1 nor V2 have disappeared from patchwork. A "filter" on the
subject, selecting "any" state and "both" archived and non-archived
patches will return both:

V1: http://patchwork.ozlabs.org/patch/282753/ (New)
V2: http://patchwork.ozlabs.org/patch/314717/ (Deferred)

(I think that the patchwork web interface does not make it possible at
all to remove a patch anyway)

> Inclusion of v2 has been postponed since there was a discussion if we
> shall allow unaligned access (-mno-unaligned-access flag) at armv7
> (after patches posted by Tom).
> 
> As fair as I can tell, we will keep the current approach so, I think
> that Tom will be willing to pull this patch (v2) now.

Agreed, but then we should make sure no one has comments on V2 that
they might have withheld due to the initial rejection of V2.

> Best regards,
> Lukasz Majewski

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] part_efi: fix protective_mbr struct allocation

[Lukasz Majewski]

Hi Albert, Tom

> Hi Lukasz,
> 
> On Wed, 12 Feb 2014 21:45:41 +0100, Lukasz Majewski
> <l.majewski@majess.pl> wrote:
> 
> > I think, that the patch for fixing the unaligned access in this
> > function has already been posted by Piotr Wilczek. We have
> > experienced similar issues with Samsung's Exynos4 based targets.
> > 
> > [PATCH V2] disk:efi: avoid unaligned access on efi partition
> > 
> > Despite its disappearance from patchwork it shall be available at
> > mailing list archive.
> > 
> > The v1 can be found at the following link:
> > http://patchwork.ozlabs.org/patch/282753/
> 
> Neither V1 nor V2 have disappeared from patchwork. A "filter" on the
> subject, selecting "any" state and "both" archived and non-archived
> patches will return both:
> 
> V1: http://patchwork.ozlabs.org/patch/282753/ (New)
> V2: http://patchwork.ozlabs.org/patch/314717/ (Deferred)
> 
> (I think that the patchwork web interface does not make it possible at
> all to remove a patch anyway)

Thanks for pointing out. Now it is perfectly visible :-)

> 
> > Inclusion of v2 has been postponed since there was a discussion if
> > we shall allow unaligned access (-mno-unaligned-access flag) at
> > armv7 (after patches posted by Tom).
> > 
> > As fair as I can tell, we will keep the current approach so, I think
> > that Tom will be willing to pull this patch (v2) now.
> 
> Agreed, but then we should make sure no one has comments on V2 that
> they might have withheld due to the initial rejection of V2.

Any comments? 

This patch do fix unaligned access problem on Trats2 (Exynos4412), when
we restore/create GPT, so I would like to know if there are any new
inquires regarding this patch.


> 
> > Best regards,
> > Lukasz Majewski
> 
> Amicalement,


-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH] part_efi: fix protective_mbr struct allocation

[Albert ARIBAUD]

Hi Lukasz,

On Wed, 19 Feb 2014 09:19:04 +0100, Lukasz Majewski
<l.majewski@samsung.com> wrote:

> Hi Albert, Tom
> 
> > Hi Lukasz,
> > 
> > On Wed, 12 Feb 2014 21:45:41 +0100, Lukasz Majewski
> > <l.majewski@majess.pl> wrote:
> > 
> > > I think, that the patch for fixing the unaligned access in this
> > > function has already been posted by Piotr Wilczek. We have
> > > experienced similar issues with Samsung's Exynos4 based targets.
> > > 
> > > [PATCH V2] disk:efi: avoid unaligned access on efi partition
> > > 
> > > Despite its disappearance from patchwork it shall be available at
> > > mailing list archive.
> > > 
> > > The v1 can be found at the following link:
> > > http://patchwork.ozlabs.org/patch/282753/
> > 
> > Neither V1 nor V2 have disappeared from patchwork. A "filter" on the
> > subject, selecting "any" state and "both" archived and non-archived
> > patches will return both:
> > 
> > V1: http://patchwork.ozlabs.org/patch/282753/ (New)
> > V2: http://patchwork.ozlabs.org/patch/314717/ (Deferred)
> > 
> > (I think that the patchwork web interface does not make it possible at
> > all to remove a patch anyway)
> 
> Thanks for pointing out. Now it is perfectly visible :-)
> 
> > 
> > > Inclusion of v2 has been postponed since there was a discussion if
> > > we shall allow unaligned access (-mno-unaligned-access flag) at
> > > armv7 (after patches posted by Tom).
> > > 
> > > As fair as I can tell, we will keep the current approach so, I think
> > > that Tom will be willing to pull this patch (v2) now.
> > 
> > Agreed, but then we should make sure no one has comments on V2 that
> > they might have withheld due to the initial rejection of V2.
> 
> Any comments? 
> 
> This patch do fix unaligned access problem on Trats2 (Exynos4412), when
> we restore/create GPT, so I would like to know if there are any new
> inquires regarding this patch.

Does not seem to be, so I will apply V2.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] part_efi: fix protective_mbr struct allocation

[Albert ARIBAUD]

On Wed, 19 Feb 2014 11:08:03 +0100, Albert ARIBAUD

> > Thanks for pointing out. Now it is perfectly visible :-)
> > 
> > > 
> > > > Inclusion of v2 has been postponed since there was a discussion if
> > > > we shall allow unaligned access (-mno-unaligned-access flag) at
> > > > armv7 (after patches posted by Tom).
> > > > 
> > > > As fair as I can tell, we will keep the current approach so, I think
> > > > that Tom will be willing to pull this patch (v2) now.
> > > 
> > > Agreed, but then we should make sure no one has comments on V2 that
> > > they might have withheld due to the initial rejection of V2.
> > 
> > Any comments? 
> > 
> > This patch do fix unaligned access problem on Trats2 (Exynos4412), when
> > we restore/create GPT, so I would like to know if there are any new
> > inquires regarding this patch.
> 
> Does not seem to be, so I will apply V2.

Correction: I would like it to be applied as per current ARM alignment
policy, but this patch is not ARM per se and is in Tom's hands.

Tom, can you apply http://patchwork.ozlabs.org/patch/314717/ ? This
would by no means close the discussion I opened, and in the event
of a policy change, the patch could always be reverted; meanwhile, it
matches our current policy.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] part_efi: fix protective_mbr struct allocation

[Palacios, Hector]

On 02/19/2014 11:16 AM, Albert ARIBAUD wrote:
> On Wed, 19 Feb 2014 11:08:03 +0100, Albert ARIBAUD
>
>>> Thanks for pointing out. Now it is perfectly visible :-)
>>>
>>>>
>>>>> Inclusion of v2 has been postponed since there was a discussion if
>>>>> we shall allow unaligned access (-mno-unaligned-access flag) at
>>>>> armv7 (after patches posted by Tom).
>>>>>
>>>>> As fair as I can tell, we will keep the current approach so, I think
>>>>> that Tom will be willing to pull this patch (v2) now.
>>>>
>>>> Agreed, but then we should make sure no one has comments on V2 that
>>>> they might have withheld due to the initial rejection of V2.
>>>
>>> Any comments?
>>>
>>> This patch do fix unaligned access problem on Trats2 (Exynos4412), when
>>> we restore/create GPT, so I would like to know if there are any new
>>> inquires regarding this patch.
>>
>> Does not seem to be, so I will apply V2.
>
> Correction: I would like it to be applied as per current ARM alignment
> policy, but this patch is not ARM per se and is in Tom's hands.
>
> Tom, can you apply http://patchwork.ozlabs.org/patch/314717/ ? This
> would by no means close the discussion I opened, and in the event
> of a policy change, the patch could always be reverted; meanwhile, it
> matches our current policy.

I tested Piotr's patch on i.MX6 (armv7) custom board and it is working fine without 
the -mno-unaligned-access flag.

Tested-by: Hector Palacios <hector.palacios@digi.com>

-- 
H?ctor Palacios

[U-Boot] [PATCH] part_efi: fix protective_mbr struct allocation

[Albert ARIBAUD]

Hi Hector,

On Wed, 19 Feb 2014 13:52:07 +0100, "Palacios, Hector"
<Hector.Palacios@digi.com> wrote:

> On 02/19/2014 11:16 AM, Albert ARIBAUD wrote:
> > On Wed, 19 Feb 2014 11:08:03 +0100, Albert ARIBAUD
> >
> >>> Thanks for pointing out. Now it is perfectly visible :-)
> >>>
> >>>>
> >>>>> Inclusion of v2 has been postponed since there was a discussion if
> >>>>> we shall allow unaligned access (-mno-unaligned-access flag) at
> >>>>> armv7 (after patches posted by Tom).
> >>>>>
> >>>>> As fair as I can tell, we will keep the current approach so, I think
> >>>>> that Tom will be willing to pull this patch (v2) now.
> >>>>
> >>>> Agreed, but then we should make sure no one has comments on V2 that
> >>>> they might have withheld due to the initial rejection of V2.
> >>>
> >>> Any comments?
> >>>
> >>> This patch do fix unaligned access problem on Trats2 (Exynos4412), when
> >>> we restore/create GPT, so I would like to know if there are any new
> >>> inquires regarding this patch.
> >>
> >> Does not seem to be, so I will apply V2.
> >
> > Correction: I would like it to be applied as per current ARM alignment
> > policy, but this patch is not ARM per se and is in Tom's hands.
> >
> > Tom, can you apply http://patchwork.ozlabs.org/patch/314717/ ? This
> > would by no means close the discussion I opened, and in the event
> > of a policy change, the patch could always be reverted; meanwhile, it
> > matches our current policy.
> 
> I tested Piotr's patch on i.MX6 (armv7) custom board and it is working fine without 
> the -mno-unaligned-access flag.
> 
> Tested-by: Hector Palacios <hector.palacios@digi.com>

You've just Tested-By-ed your own patch, I think.

... but I am the one to blame, and should not have discussed Piotr's
patch in this thread.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] part_efi: fix protective_mbr struct allocation

[Lukasz Majewski]

Hi Albert,

> Hi Hector,
> 
> On Wed, 19 Feb 2014 13:52:07 +0100, "Palacios, Hector"
> <Hector.Palacios@digi.com> wrote:
> 
> > On 02/19/2014 11:16 AM, Albert ARIBAUD wrote:
> > > On Wed, 19 Feb 2014 11:08:03 +0100, Albert ARIBAUD
> > >
> > >>> Thanks for pointing out. Now it is perfectly visible :-)
> > >>>
> > >>>>
> > >>>>> Inclusion of v2 has been postponed since there was a
> > >>>>> discussion if we shall allow unaligned access
> > >>>>> (-mno-unaligned-access flag) at armv7 (after patches posted
> > >>>>> by Tom).
> > >>>>>
> > >>>>> As fair as I can tell, we will keep the current approach so,
> > >>>>> I think that Tom will be willing to pull this patch (v2) now.
> > >>>>
> > >>>> Agreed, but then we should make sure no one has comments on V2
> > >>>> that they might have withheld due to the initial rejection of
> > >>>> V2.
> > >>>
> > >>> Any comments?
> > >>>
> > >>> This patch do fix unaligned access problem on Trats2
> > >>> (Exynos4412), when we restore/create GPT, so I would like to
> > >>> know if there are any new inquires regarding this patch.
> > >>
> > >> Does not seem to be, so I will apply V2.
> > >
> > > Correction: I would like it to be applied as per current ARM
> > > alignment policy, but this patch is not ARM per se and is in
> > > Tom's hands.
> > >
> > > Tom, can you apply http://patchwork.ozlabs.org/patch/314717/ ?
> > > This would by no means close the discussion I opened, and in the
> > > event of a policy change, the patch could always be reverted;
> > > meanwhile, it matches our current policy.
> > 
> > I tested Piotr's patch on i.MX6 (armv7) custom board and it is
> > working fine without the -mno-unaligned-access flag.
> > 
> > Tested-by: Hector Palacios <hector.palacios@digi.com>
> 
> You've just Tested-By-ed your own patch, I think.

Nope. 

Patch prepared by Piotr is orthogonal to the one prepared by Hector.

Hector has spotted other mistake at GPT code (made by me).
Fix for it has been posted a few days ago:

http://patchwork.ozlabs.org/patch/319914/

> 
> ... but I am the one to blame, and should not have discussed Piotr's
> patch in this thread.
> 
> Amicalement,



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH] part_efi: fix protective_mbr struct allocation

[Albert ARIBAUD]

Hi Lukasz,

On Wed, 19 Feb 2014 15:25:37 +0100, Lukasz Majewski
<l.majewski@samsung.com> wrote:

> Hi Albert,
> 
> > Hi Hector,
> > 
> > On Wed, 19 Feb 2014 13:52:07 +0100, "Palacios, Hector"
> > <Hector.Palacios@digi.com> wrote:
> > 
> > > On 02/19/2014 11:16 AM, Albert ARIBAUD wrote:
> > > > On Wed, 19 Feb 2014 11:08:03 +0100, Albert ARIBAUD
> > > >
> > > >>> Thanks for pointing out. Now it is perfectly visible :-)
> > > >>>
> > > >>>>
> > > >>>>> Inclusion of v2 has been postponed since there was a
> > > >>>>> discussion if we shall allow unaligned access
> > > >>>>> (-mno-unaligned-access flag) at armv7 (after patches posted
> > > >>>>> by Tom).
> > > >>>>>
> > > >>>>> As fair as I can tell, we will keep the current approach so,
> > > >>>>> I think that Tom will be willing to pull this patch (v2) now.
> > > >>>>
> > > >>>> Agreed, but then we should make sure no one has comments on V2
> > > >>>> that they might have withheld due to the initial rejection of
> > > >>>> V2.
> > > >>>
> > > >>> Any comments?
> > > >>>
> > > >>> This patch do fix unaligned access problem on Trats2
> > > >>> (Exynos4412), when we restore/create GPT, so I would like to
> > > >>> know if there are any new inquires regarding this patch.
> > > >>
> > > >> Does not seem to be, so I will apply V2.
> > > >
> > > > Correction: I would like it to be applied as per current ARM
> > > > alignment policy, but this patch is not ARM per se and is in
> > > > Tom's hands.
> > > >
> > > > Tom, can you apply http://patchwork.ozlabs.org/patch/314717/ ?
> > > > This would by no means close the discussion I opened, and in the
> > > > event of a policy change, the patch could always be reverted;
> > > > meanwhile, it matches our current policy.
> > > 
> > > I tested Piotr's patch on i.MX6 (armv7) custom board and it is
> > > working fine without the -mno-unaligned-access flag.
> > > 
> > > Tested-by: Hector Palacios <hector.palacios@digi.com>
> > 
> > You've just Tested-By-ed your own patch, I think.
> 
> Nope. 
> 
> Patch prepared by Piotr is orthogonal to the one prepared by Hector.
> 
> Hector has spotted other mistake at GPT code (made by me).
> Fix for it has been posted a few days ago:
> 
> http://patchwork.ozlabs.org/patch/319914/

I did not comment on the relationship between patches, I only
commented on the fact that Hector said he has tested Piotr's patch but
sent his Tested-by on his own patch thread, not on Piotr's. To verify
this, look up

http://patchwork.ozlabs.org/patch/319649/

... which is Hector's patchwork entry and has his own Tested-by, and

http://patchwork.ozlabs.org/patch/314717/

... which is Piotr's patch and does not have Hector's (or
anyone's) Tested-by.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] part_efi: fix protective_mbr struct allocation

[Lukasz Majewski]

Hi Albert,

> Hi Lukasz,
> 
> On Wed, 19 Feb 2014 15:25:37 +0100, Lukasz Majewski
> <l.majewski@samsung.com> wrote:
> 
> > Hi Albert,
> > 
> > > Hi Hector,
> > > 
> > > On Wed, 19 Feb 2014 13:52:07 +0100, "Palacios, Hector"
> > > <Hector.Palacios@digi.com> wrote:
> > > 
> > > > On 02/19/2014 11:16 AM, Albert ARIBAUD wrote:
> > > > > On Wed, 19 Feb 2014 11:08:03 +0100, Albert ARIBAUD
> > > > >
> > > > >>> Thanks for pointing out. Now it is perfectly visible :-)
> > > > >>>
> > > > >>>>
> > > > >>>>> Inclusion of v2 has been postponed since there was a
> > > > >>>>> discussion if we shall allow unaligned access
> > > > >>>>> (-mno-unaligned-access flag) at armv7 (after patches
> > > > >>>>> posted by Tom).
> > > > >>>>>
> > > > >>>>> As fair as I can tell, we will keep the current approach
> > > > >>>>> so, I think that Tom will be willing to pull this patch
> > > > >>>>> (v2) now.
> > > > >>>>
> > > > >>>> Agreed, but then we should make sure no one has comments
> > > > >>>> on V2 that they might have withheld due to the initial
> > > > >>>> rejection of V2.
> > > > >>>
> > > > >>> Any comments?
> > > > >>>
> > > > >>> This patch do fix unaligned access problem on Trats2
> > > > >>> (Exynos4412), when we restore/create GPT, so I would like to
> > > > >>> know if there are any new inquires regarding this patch.
> > > > >>
> > > > >> Does not seem to be, so I will apply V2.
> > > > >
> > > > > Correction: I would like it to be applied as per current ARM
> > > > > alignment policy, but this patch is not ARM per se and is in
> > > > > Tom's hands.
> > > > >
> > > > > Tom, can you apply http://patchwork.ozlabs.org/patch/314717/ ?
> > > > > This would by no means close the discussion I opened, and in
> > > > > the event of a policy change, the patch could always be
> > > > > reverted; meanwhile, it matches our current policy.
> > > > 
> > > > I tested Piotr's patch on i.MX6 (armv7) custom board and it is
> > > > working fine without the -mno-unaligned-access flag.
> > > > 
> > > > Tested-by: Hector Palacios <hector.palacios@digi.com>
> > > 
> > > You've just Tested-By-ed your own patch, I think.
> > 
> > Nope. 
> > 
> > Patch prepared by Piotr is orthogonal to the one prepared by Hector.
> > 
> > Hector has spotted other mistake at GPT code (made by me).
> > Fix for it has been posted a few days ago:
> > 
> > http://patchwork.ozlabs.org/patch/319914/
> 
> I did not comment on the relationship between patches, I only
> commented on the fact that Hector said he has tested Piotr's patch but
> sent his Tested-by on his own patch thread, not on Piotr's. To verify
> this, look up
> 
> http://patchwork.ozlabs.org/patch/319649/
> 
> ... which is Hector's patchwork entry and has his own Tested-by, and
> 
> http://patchwork.ozlabs.org/patch/314717/
> 
> ... which is Piotr's patch and does not have Hector's (or
> anyone's) Tested-by.

Hmm. I've misunderstood you a bit. 

Anyway thanks for clarification :-).

> 
> Amicalement,



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH] part_efi: fix protective_mbr struct allocation

[Tom Rini]

On Wed, Feb 19, 2014 at 09:19:04AM +0100, Lukasz Majewski wrote:
> Hi Albert, Tom
> 
> > Hi Lukasz,
> > 
> > On Wed, 12 Feb 2014 21:45:41 +0100, Lukasz Majewski
> > <l.majewski@majess.pl> wrote:
> > 
> > > I think, that the patch for fixing the unaligned access in this
> > > function has already been posted by Piotr Wilczek. We have
> > > experienced similar issues with Samsung's Exynos4 based targets.
> > > 
> > > [PATCH V2] disk:efi: avoid unaligned access on efi partition
> > > 
> > > Despite its disappearance from patchwork it shall be available at
> > > mailing list archive.
> > > 
> > > The v1 can be found at the following link:
> > > http://patchwork.ozlabs.org/patch/282753/
> > 
> > Neither V1 nor V2 have disappeared from patchwork. A "filter" on the
> > subject, selecting "any" state and "both" archived and non-archived
> > patches will return both:
> > 
> > V1: http://patchwork.ozlabs.org/patch/282753/ (New)
> > V2: http://patchwork.ozlabs.org/patch/314717/ (Deferred)
> > 
> > (I think that the patchwork web interface does not make it possible at
> > all to remove a patch anyway)
> 
> Thanks for pointing out. Now it is perfectly visible :-)

I've also un-deferred, for now, the patch as well.

> > > Inclusion of v2 has been postponed since there was a discussion if
> > > we shall allow unaligned access (-mno-unaligned-access flag) at
> > > armv7 (after patches posted by Tom).
> > > 
> > > As fair as I can tell, we will keep the current approach so, I think
> > > that Tom will be willing to pull this patch (v2) now.
> > 
> > Agreed, but then we should make sure no one has comments on V2 that
> > they might have withheld due to the initial rejection of V2.
> 
> Any comments? 
> 
> This patch do fix unaligned access problem on Trats2 (Exynos4412), when
> we restore/create GPT, so I would like to know if there are any new
> inquires regarding this patch.

Can you give me some steps on how to hit this bug?  I believe it's a bug
and I believe we need to fix it, I just want to investigate a few things
while we've got a trigger case right now.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140219/bc247027/attachment.pgp>

[U-Boot] [PATCH] part_efi: fix protective_mbr struct allocation

[Lukasz Majewski]

Hi Tom,

> On Wed, Feb 19, 2014 at 09:19:04AM +0100, Lukasz Majewski wrote:
> > Hi Albert, Tom
> > 
> > > Hi Lukasz,
> > > 
> > > On Wed, 12 Feb 2014 21:45:41 +0100, Lukasz Majewski
> > > <l.majewski@majess.pl> wrote:
> > > 
> > > > I think, that the patch for fixing the unaligned access in this
> > > > function has already been posted by Piotr Wilczek. We have
> > > > experienced similar issues with Samsung's Exynos4 based targets.
> > > > 
> > > > [PATCH V2] disk:efi: avoid unaligned access on efi partition
> > > > 
> > > > Despite its disappearance from patchwork it shall be available
> > > > at mailing list archive.
> > > > 
> > > > The v1 can be found at the following link:
> > > > http://patchwork.ozlabs.org/patch/282753/
> > > 
> > > Neither V1 nor V2 have disappeared from patchwork. A "filter" on
> > > the subject, selecting "any" state and "both" archived and
> > > non-archived patches will return both:
> > > 
> > > V1: http://patchwork.ozlabs.org/patch/282753/ (New)
> > > V2: http://patchwork.ozlabs.org/patch/314717/ (Deferred)
> > > 
> > > (I think that the patchwork web interface does not make it
> > > possible at all to remove a patch anyway)
> > 
> > Thanks for pointing out. Now it is perfectly visible :-)
> 
> I've also un-deferred, for now, the patch as well.
> 
> > > > Inclusion of v2 has been postponed since there was a discussion
> > > > if we shall allow unaligned access (-mno-unaligned-access flag)
> > > > at armv7 (after patches posted by Tom).
> > > > 
> > > > As fair as I can tell, we will keep the current approach so, I
> > > > think that Tom will be willing to pull this patch (v2) now.
> > > 
> > > Agreed, but then we should make sure no one has comments on V2
> > > that they might have withheld due to the initial rejection of V2.
> > 
> > Any comments? 
> > 
> > This patch do fix unaligned access problem on Trats2 (Exynos4412),
> > when we restore/create GPT, so I would like to know if there are
> > any new inquires regarding this patch.
> 
> Can you give me some steps on how to hit this bug?  I believe it's a
> bug and I believe we need to fix it, I just want to investigate a few
> things while we've got a trigger case right now.  Thanks!

The easiest way is to type:

gpt write mmc 0 $partitions

before that you shall define several uuid's.

For reference please look into the Trats2/Trats code
and ./doc/README.gpt

As a side note - I'm using following toolchain:
CROSS_COMPILE=/opt/eldk-5.4/armv7a/sysroots/i686-eldk-linux/usr/bin/armv7a-vfp-neon-linux-gnueabi/arm-linux-gnueabi-

It is 4.7.2 from denx.de.

> 



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group