| Message ID | 1392216053-10344-1-git-send-email-hector.palacios@digi.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
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
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;
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; > > >
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,
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,
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
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
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,
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
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,
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,
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,
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,
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>
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,
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!
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,
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,
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. >
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,
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;
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(-)