Re: powerpc/nvram: Fix an incorrect partition merge

[Michael Ellerman]

On Thu, 2015-10-12 at 07:30:02 UTC, xinhui wrote:
> From: Pan Xinhui 
> 
> When we merge two contiguous partitions whose signatures are marked
> NVRAM_SIG_FREE, We need update prev's length and checksum, then write it
> to nvram, not cur's. So lets fix this mistake now.
> 
> Also use memset instead of strncpy to set the partition's name. It's
> more readable if we want to fill up with duplicate chars .
> 
> Signed-off-by: Pan Xinhui 

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/11b7e154b132232535befe51c5

cheers

Re: powerpc/nvram: Fix an incorrect partition merge

[Michael Ellerman]

On Thu, 2015-10-12 at 07:30:02 UTC, xinhui wrote:
> From: Pan Xinhui 
> 
> When we merge two contiguous partitions whose signatures are marked
> NVRAM_SIG_FREE, We need update prev's length and checksum, then write it
> to nvram, not cur's. So lets fix this mistake now.
> 
> Also use memset instead of strncpy to set the partition's name. It's
> more readable if we want to fill up with duplicate chars .
> 
> Signed-off-by: Pan Xinhui 

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/11b7e154b132232535befe51c5

cheers

Re: powerpc/nvram: Fix an incorrect partition merge

[xinhui]

On 2016年06月07日 19:39, Michael Ellerman wrote:

On Mon, 2016-06-06 at 13:31 +0800, xinhui wrote:

On 2016年06月03日 19:47, Michael Ellerman wrote:

On Thu, 2015-10-12 at 07:30:02 UTC, xinhui wrote:

From: Pan Xinhui 

When we merge two contiguous partitions whose signatures are marked
NVRAM_SIG_FREE, We need update prev's length and checksum, then write it
to nvram, not cur's. So lets fix this mistake now.

Also use memset instead of strncpy to set the partition's name. It's
more readable if we want to fill up with duplicate chars .


Does this ever happen in practice? ie. should we backport the fix to stable
kernels?


I did not see that nvram warning in practice. BUT I suggest to backport it to 
stable kernel. :)

Let me recall the story. :)
In past days, I was using pstore to keep some kernel logs. and sometimes I 
found my own logs and the panic logs did not show.
pstore use a fixed-address reserved memory In x86 while nvram instead in ppc.

Then I spent some days to review the nvram codes.
And worked out three patches to fix all issues that I found in nvram. BUT looks 
like I only sent out two of them. :)
I lost the third patch maybe...


OK.


Has it always been broken?


no. after nvram partition corruption hit, all nvram partitions will be erased 
and re-alloc after the second machine reboot.
I don't know who does it but i guess it is the firmware. :)


Actually I meant has the code always contained the bug, or was it added 
recently.


fix fa2b4e54d41f3c9f1bee6a7d63ecd4f0ab161e89 ("powerpc/nvram: Improve partition 
removal")
and it is from Ben.

So cc Ben :)

cheers

Re: powerpc/nvram: Fix an incorrect partition merge

[xinhui]

On 2016年06月07日 19:39, Michael Ellerman wrote:

On Mon, 2016-06-06 at 13:31 +0800, xinhui wrote:

On 2016年06月03日 19:47, Michael Ellerman wrote:

On Thu, 2015-10-12 at 07:30:02 UTC, xinhui wrote:

From: Pan Xinhui 

When we merge two contiguous partitions whose signatures are marked
NVRAM_SIG_FREE, We need update prev's length and checksum, then write it
to nvram, not cur's. So lets fix this mistake now.

Also use memset instead of strncpy to set the partition's name. It's
more readable if we want to fill up with duplicate chars .


Does this ever happen in practice? ie. should we backport the fix to stable
kernels?


I did not see that nvram warning in practice. BUT I suggest to backport it to 
stable kernel. :)

Let me recall the story. :)
In past days, I was using pstore to keep some kernel logs. and sometimes I 
found my own logs and the panic logs did not show.
pstore use a fixed-address reserved memory In x86 while nvram instead in ppc.

Then I spent some days to review the nvram codes.
And worked out three patches to fix all issues that I found in nvram. BUT looks 
like I only sent out two of them. :)
I lost the third patch maybe...


OK.


Has it always been broken?


no. after nvram partition corruption hit, all nvram partitions will be erased 
and re-alloc after the second machine reboot.
I don't know who does it but i guess it is the firmware. :)


Actually I meant has the code always contained the bug, or was it added 
recently.


fix fa2b4e54d41f3c9f1bee6a7d63ecd4f0ab161e89 ("powerpc/nvram: Improve partition 
removal")
and it is from Ben.

So cc Ben :)

cheers

Re: powerpc/nvram: Fix an incorrect partition merge

[Michael Ellerman]

On Mon, 2016-06-06 at 13:31 +0800, xinhui wrote:
> On 2016年06月03日 19:47, Michael Ellerman wrote:
> > On Thu, 2015-10-12 at 07:30:02 UTC, xinhui wrote:
> > > From: Pan Xinhui 
> > > 
> > > When we merge two contiguous partitions whose signatures are marked
> > > NVRAM_SIG_FREE, We need update prev's length and checksum, then write it
> > > to nvram, not cur's. So lets fix this mistake now.
> > > 
> > > Also use memset instead of strncpy to set the partition's name. It's
> > > more readable if we want to fill up with duplicate chars .
> > 
> > Does this ever happen in practice? ie. should we backport the fix to stable
> > kernels?
> 
> I did not see that nvram warning in practice. BUT I suggest to backport it to 
> stable kernel. :)
> 
> Let me recall the story. :)
> In past days, I was using pstore to keep some kernel logs. and sometimes I 
> found my own logs and the panic logs did not show.
> pstore use a fixed-address reserved memory In x86 while nvram instead in ppc.
> 
> Then I spent some days to review the nvram codes.
> And worked out three patches to fix all issues that I found in nvram. BUT 
> looks like I only sent out two of them. :)
> I lost the third patch maybe...

OK.

> > Has it always been broken?
> 
> no. after nvram partition corruption hit, all nvram partitions will be erased 
> and re-alloc after the second machine reboot.
> I don't know who does it but i guess it is the firmware. :)

Actually I meant has the code always contained the bug, or was it added 
recently.

cheers

Re: powerpc/nvram: Fix an incorrect partition merge

[Michael Ellerman]

On Mon, 2016-06-06 at 13:31 +0800, xinhui wrote:
> On 2016年06月03日 19:47, Michael Ellerman wrote:
> > On Thu, 2015-10-12 at 07:30:02 UTC, xinhui wrote:
> > > From: Pan Xinhui 
> > > 
> > > When we merge two contiguous partitions whose signatures are marked
> > > NVRAM_SIG_FREE, We need update prev's length and checksum, then write it
> > > to nvram, not cur's. So lets fix this mistake now.
> > > 
> > > Also use memset instead of strncpy to set the partition's name. It's
> > > more readable if we want to fill up with duplicate chars .
> > 
> > Does this ever happen in practice? ie. should we backport the fix to stable
> > kernels?
> 
> I did not see that nvram warning in practice. BUT I suggest to backport it to 
> stable kernel. :)
> 
> Let me recall the story. :)
> In past days, I was using pstore to keep some kernel logs. and sometimes I 
> found my own logs and the panic logs did not show.
> pstore use a fixed-address reserved memory In x86 while nvram instead in ppc.
> 
> Then I spent some days to review the nvram codes.
> And worked out three patches to fix all issues that I found in nvram. BUT 
> looks like I only sent out two of them. :)
> I lost the third patch maybe...

OK.

> > Has it always been broken?
> 
> no. after nvram partition corruption hit, all nvram partitions will be erased 
> and re-alloc after the second machine reboot.
> I don't know who does it but i guess it is the firmware. :)

Actually I meant has the code always contained the bug, or was it added 
recently.

cheers

Re: powerpc/nvram: Fix an incorrect partition merge

[Stewart Smith]

xinhui  writes:
>> Has it always been broken?
>>
>
> no. after nvram partition corruption hit, all nvram partitions will be
> erased and re-alloc after the second machine reboot.
> I don't know who does it but i guess it is the firmware. :)

It is. PAPR says we format NVRAM when it's corrupted. This is also true
for NVRAM for PowerNV (not just pseries guest).

-- 
Stewart Smith
OPAL Architect, IBM.

Re: powerpc/nvram: Fix an incorrect partition merge

[Stewart Smith]

xinhui  writes:
>> Has it always been broken?
>>
>
> no. after nvram partition corruption hit, all nvram partitions will be
> erased and re-alloc after the second machine reboot.
> I don't know who does it but i guess it is the firmware. :)

It is. PAPR says we format NVRAM when it's corrupted. This is also true
for NVRAM for PowerNV (not just pseries guest).

-- 
Stewart Smith
OPAL Architect, IBM.

Re: powerpc/nvram: Fix an incorrect partition merge

[xinhui]

On 2016年06月03日 19:47, Michael Ellerman wrote:

On Thu, 2015-10-12 at 07:30:02 UTC, xinhui wrote:

From: Pan Xinhui 

When we merge two contiguous partitions whose signatures are marked
NVRAM_SIG_FREE, We need update prev's length and checksum, then write it
to nvram, not cur's. So lets fix this mistake now.

Also use memset instead of strncpy to set the partition's name. It's
more readable if we want to fill up with duplicate chars .


Does this ever happen in practice? ie. should we backport the fix to stable
kernels?



I did not see that nvram warning in practice. BUT I suggest to backport it to 
stable kernel. :)

Let me recall the story. :)
In past days, I was using pstore to keep some kernel logs. and sometimes I 
found my own logs and the panic logs did not show.
pstore use a fixed-address reserved memory In x86 while nvram instead in ppc.

Then I spent some days to review the nvram codes.
And worked out three patches to fix all issues that I found in nvram. BUT looks 
like I only sent out two of them. :)
I lost the third patch maybe...


Has it always been broken?



no. after nvram partition corruption hit, all nvram partitions will be erased 
and re-alloc after the second machine reboot.
I don't know who does it but i guess it is the firmware. :)

thanks
xinhui

cheers

Re: powerpc/nvram: Fix an incorrect partition merge

[xinhui]

On 2016年06月03日 19:47, Michael Ellerman wrote:

On Thu, 2015-10-12 at 07:30:02 UTC, xinhui wrote:

From: Pan Xinhui 

When we merge two contiguous partitions whose signatures are marked
NVRAM_SIG_FREE, We need update prev's length and checksum, then write it
to nvram, not cur's. So lets fix this mistake now.

Also use memset instead of strncpy to set the partition's name. It's
more readable if we want to fill up with duplicate chars .


Does this ever happen in practice? ie. should we backport the fix to stable
kernels?



I did not see that nvram warning in practice. BUT I suggest to backport it to 
stable kernel. :)

Let me recall the story. :)
In past days, I was using pstore to keep some kernel logs. and sometimes I 
found my own logs and the panic logs did not show.
pstore use a fixed-address reserved memory In x86 while nvram instead in ppc.

Then I spent some days to review the nvram codes.
And worked out three patches to fix all issues that I found in nvram. BUT looks 
like I only sent out two of them. :)
I lost the third patch maybe...


Has it always been broken?



no. after nvram partition corruption hit, all nvram partitions will be erased 
and re-alloc after the second machine reboot.
I don't know who does it but i guess it is the firmware. :)

thanks
xinhui

cheers

Re: powerpc/nvram: Fix an incorrect partition merge

[Michael Ellerman]

On Thu, 2015-10-12 at 07:30:02 UTC, xinhui wrote:
> From: Pan Xinhui 
> 
> When we merge two contiguous partitions whose signatures are marked
> NVRAM_SIG_FREE, We need update prev's length and checksum, then write it
> to nvram, not cur's. So lets fix this mistake now.
> 
> Also use memset instead of strncpy to set the partition's name. It's
> more readable if we want to fill up with duplicate chars .

Does this ever happen in practice? ie. should we backport the fix to stable
kernels?

Has it always been broken?

cheers

Re: powerpc/nvram: Fix an incorrect partition merge

[Michael Ellerman]

On Thu, 2015-10-12 at 07:30:02 UTC, xinhui wrote:
> From: Pan Xinhui 
> 
> When we merge two contiguous partitions whose signatures are marked
> NVRAM_SIG_FREE, We need update prev's length and checksum, then write it
> to nvram, not cur's. So lets fix this mistake now.
> 
> Also use memset instead of strncpy to set the partition's name. It's
> more readable if we want to fill up with duplicate chars .

Does this ever happen in practice? ie. should we backport the fix to stable
kernels?

Has it always been broken?

cheers