Patches for arch/arm/mm/cache-l2x0.c

Patches for arch/arm/mm/cache-l2x0.c

[Mason]

Hello,

Over the past few months, I wrote board-support code based on kernel v4.2
and things worked as expected.

Then I had to rebase to a LTS kernel (I used v4.1.13) and something broke
in the L2 cache setup. Apparently, Russell King did a lot of fixing and
cleaning up between v4.1 and v4.2

I cherry-picked the following 5 patches, and my 4.1 kernel works again:

346248a2d1e3a815297125c1347d90dafcc51990 ARM: l2c: avoid passing auxiliary control register through enable method
 arch/arm/mm/cache-l2x0.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)
dc63c0733050996143a82f2b095fc378a04274f0 ARM: l2c: only unlock caches if NS_LOCKDOWN bit is set
 arch/arm/mm/cache-l2x0.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)
7787be2a74dc618bf32348a0f588eebf7ebe0a06 ARM: l2c: clean up l2c_configure()
 arch/arm/mm/cache-l2x0.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)
8f80afa16002e9b4784dc1d51c48f95f52838cfb ARM: l2c: write auxiliary control register first
 arch/arm/mm/cache-l2x0.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
9749167eec6a057122b7a1ab2193abd079645aba ARM: l2c: restore the behaviour documented above l2c_enable()
 arch/arm/mm/cache-l2x0.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)


The problem is that my system runs Linux in NS (non-secure) mode, and there
are a bunch of instructions that Linux can't use in that mode.

I suspect the crash comes from trying to unconditionally unlock the caches,
thus patch dc63c0733050.

But 9749167eec6a and 8f80afa16002 also look like good candidates for
back-porting to v4.1

What do you think?

Regards.

Re: Patches for arch/arm/mm/cache-l2x0.c

[Mason]

[ Back-porting v4.2 patches to v4.1 LTS ]

On 26/11/2015 09:54, Mason wrote:

> Over the past few months, I wrote board-support code based on kernel v4.2
> and things worked as expected.
> 
> Then I had to rebase to a LTS kernel (I used v4.1.13) and something broke
> in the L2 cache setup. Apparently, Russell King did a lot of fixing and
> cleaning up between v4.1 and v4.2
> 
> I cherry-picked the following 5 patches, and my 4.1 kernel works again:
> 
> 346248a2d1e3a815297125c1347d90dafcc51990 ARM: l2c: avoid passing auxiliary control register through enable method
>  arch/arm/mm/cache-l2x0.c | 32 +++++++++++++++++---------------
>  1 file changed, 17 insertions(+), 15 deletions(-)
> dc63c0733050996143a82f2b095fc378a04274f0 ARM: l2c: only unlock caches if NS_LOCKDOWN bit is set
>  arch/arm/mm/cache-l2x0.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 7787be2a74dc618bf32348a0f588eebf7ebe0a06 ARM: l2c: clean up l2c_configure()
>  arch/arm/mm/cache-l2x0.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 8f80afa16002e9b4784dc1d51c48f95f52838cfb ARM: l2c: write auxiliary control register first
>  arch/arm/mm/cache-l2x0.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 9749167eec6a057122b7a1ab2193abd079645aba ARM: l2c: restore the behaviour documented above l2c_enable()
>  arch/arm/mm/cache-l2x0.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> 
> The problem is that my system runs Linux in NS (non-secure) mode, and there
> are a bunch of instructions that Linux can't use in that mode.
> 
> I suspect the crash comes from trying to unconditionally unlock the caches,
> thus patch dc63c0733050.
> 
> But 9749167eec6a and 8f80afa16002 also look like good candidates for
> back-porting to v4.1
> 
> What do you think?

Any opinion on this topic?

Regards.

Re: Patches for arch/arm/mm/cache-l2x0.c

[Russell King - ARM Linux]

On Tue, Dec 01, 2015 at 09:56:23AM +0100, Mason wrote:
> [ Back-porting v4.2 patches to v4.1 LTS ]
> 
> On 26/11/2015 09:54, Mason wrote:
> 
> > Over the past few months, I wrote board-support code based on kernel v4.2
> > and things worked as expected.
> > 
> > Then I had to rebase to a LTS kernel (I used v4.1.13) and something broke
> > in the L2 cache setup. Apparently, Russell King did a lot of fixing and
> > cleaning up between v4.1 and v4.2
> > 
> > I cherry-picked the following 5 patches, and my 4.1 kernel works again:
> > 
> > 346248a2d1e3a815297125c1347d90dafcc51990 ARM: l2c: avoid passing auxiliary control register through enable method
> >  arch/arm/mm/cache-l2x0.c | 32 +++++++++++++++++---------------
> >  1 file changed, 17 insertions(+), 15 deletions(-)
> > dc63c0733050996143a82f2b095fc378a04274f0 ARM: l2c: only unlock caches if NS_LOCKDOWN bit is set
> >  arch/arm/mm/cache-l2x0.c | 26 +++++++++++++++++++++++++-
> >  1 file changed, 25 insertions(+), 1 deletion(-)
> > 7787be2a74dc618bf32348a0f588eebf7ebe0a06 ARM: l2c: clean up l2c_configure()
> >  arch/arm/mm/cache-l2x0.c | 23 ++++++++++++++---------
> >  1 file changed, 14 insertions(+), 9 deletions(-)
> > 8f80afa16002e9b4784dc1d51c48f95f52838cfb ARM: l2c: write auxiliary control register first
> >  arch/arm/mm/cache-l2x0.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 9749167eec6a057122b7a1ab2193abd079645aba ARM: l2c: restore the behaviour documented above l2c_enable()
> >  arch/arm/mm/cache-l2x0.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > 
> > The problem is that my system runs Linux in NS (non-secure) mode, and there
> > are a bunch of instructions that Linux can't use in that mode.
> > 
> > I suspect the crash comes from trying to unconditionally unlock the caches,
> > thus patch dc63c0733050.
> > 
> > But 9749167eec6a and 8f80afa16002 also look like good candidates for
> > back-porting to v4.1
> > 
> > What do you think?
> 
> Any opinion on this topic?

You can find the answer to this question yourself if you read
Documentation/stable_kernel_rules.txt, which gives the rules for what's
suitable to be backported to stable trees.  Let's go through them one
by one:

 - It must be obviously correct and tested.
Tested yes. Obviously correct - maybe (it's been some time since I wrote
the patches.)

 - It cannot be bigger than 100 lines, with context.
Doubtful, though for individual patches this may be true.

 - It must fix only one thing.
True of each individual patch.

 - It must fix a real bug that bothers people (not a, "This could be a
   problem..." type thing).
Maybe for your situation, but you're an out of tree user, and the
problem doesn't exist with mainline kernels.

 - It must fix a problem that causes a build error (but not for things
   marked CONFIG_BROKEN), an oops, a hang, data corruption, a real
   security issue, or some "oh, that's not good" issue.  In short, something
   critical.
Maybe causes an oops for you, but not for any already merged platform,
so it isn't a problem that is seen with mainline kernels.

 - Serious issues as reported by a user of a distribution kernel may also
   be considered if they fix a notable performance or interactivity issue.
Not applicable - you're not a distribution.

 - New device IDs and quirks are also accepted.
Not applicable - not a new device ID or quirk.

 - No "theoretical race condition" issues, unless an explanation of how the
   race can be exploited is also provided.
Not applicable - not a theoretical race.

 - It cannot contain any "trivial" fixes in it (spelling changes,
   whitespace cleanups, etc).
Yes (it isn't a "trivial" spelling/whitespace patch).

 - It must follow the Documentation/SubmittingPatches rules.
Yes.

 - It or an equivalent fix must already exist in Linus' tree (upstream).
Yes.

So, it doesn't fall into the criteria for stable backporting.  If you did
have Tango merged into 4.1, then there would be more of a justification.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: Patches for arch/arm/mm/cache-l2x0.c

[Mason]

On 01/12/2015 13:59, Russell King - ARM Linux wrote:
> On Tue, Dec 01, 2015 at 09:56:23AM +0100, Mason wrote:
>> [ Back-porting v4.2 patches to v4.1 LTS ]
>>
>> On 26/11/2015 09:54, Mason wrote:
>>
>>> Over the past few months, I wrote board-support code based on kernel v4.2
>>> and things worked as expected.
>>>
>>> Then I had to rebase to a LTS kernel (I used v4.1.13) and something broke
>>> in the L2 cache setup. Apparently, Russell King did a lot of fixing and
>>> cleaning up between v4.1 and v4.2
>>>
>>> I cherry-picked the following 5 patches, and my 4.1 kernel works again:
>>>
>>> 346248a2d1e3a815297125c1347d90dafcc51990 ARM: l2c: avoid passing auxiliary control register through enable method
>>>  arch/arm/mm/cache-l2x0.c | 32 +++++++++++++++++---------------
>>>  1 file changed, 17 insertions(+), 15 deletions(-)
>>> dc63c0733050996143a82f2b095fc378a04274f0 ARM: l2c: only unlock caches if NS_LOCKDOWN bit is set
>>>  arch/arm/mm/cache-l2x0.c | 26 +++++++++++++++++++++++++-
>>>  1 file changed, 25 insertions(+), 1 deletion(-)
>>> 7787be2a74dc618bf32348a0f588eebf7ebe0a06 ARM: l2c: clean up l2c_configure()
>>>  arch/arm/mm/cache-l2x0.c | 23 ++++++++++++++---------
>>>  1 file changed, 14 insertions(+), 9 deletions(-)
>>> 8f80afa16002e9b4784dc1d51c48f95f52838cfb ARM: l2c: write auxiliary control register first
>>>  arch/arm/mm/cache-l2x0.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>> 9749167eec6a057122b7a1ab2193abd079645aba ARM: l2c: restore the behaviour documented above l2c_enable()
>>>  arch/arm/mm/cache-l2x0.c | 10 +++++-----
>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>>
>>> The problem is that my system runs Linux in NS (non-secure) mode, and there
>>> are a bunch of instructions that Linux can't use in that mode.
>>>
>>> I suspect the crash comes from trying to unconditionally unlock the caches,
>>> thus patch dc63c0733050.
>>>
>>> But 9749167eec6a and 8f80afa16002 also look like good candidates for
>>> back-porting to v4.1
>>>
>>> What do you think?
>>
>> Any opinion on this topic?
> 
> You can find the answer to this question yourself if you read
> Documentation/stable_kernel_rules.txt, which gives the rules for what's
> suitable to be backported to stable trees.  Let's go through them one
> by one:

I get the feeling you've also brought tar and feathers :-)

>  - It must be obviously correct and tested.
> Tested yes. Obviously correct - maybe (it's been some time since I wrote
> the patches.)
> 
>  - It cannot be bigger than 100 lines, with context.
> Doubtful, though for individual patches this may be true.

FWIW, I provided the diffstat in my original message. (32, 26, 23, 4, 10)

>  - It must fix only one thing.
> True of each individual patch.
> 
>  - It must fix a real bug that bothers people (not a, "This could be a
>    problem..." type thing).
> Maybe for your situation, but you're an out of tree user, and the
> problem doesn't exist with mainline kernels.

<confused>
Problem would hit anyone running Linux in non-secure mode. The kernel
will crash when it executes l2c_unlock() which unconditionally writes
to a secure register.

>  - It must fix a problem that causes a build error (but not for things
>    marked CONFIG_BROKEN), an oops, a hang, data corruption, a real
>    security issue, or some "oh, that's not good" issue.  In short, something
>    critical.
> Maybe causes an oops for you, but not for any already merged platform,
> so it isn't a problem that is seen with mainline kernels.

<confused>
Why would this problem not affect e.g. arch/arm/mach-highbank ?
(CCing Rob)

(BTW, thanks for the wait_event_interruptible_timeout code outline.)

Regards.

Re: Patches for arch/arm/mm/cache-l2x0.c

[Rob Herring]

On Tue, Dec 1, 2015 at 8:11 AM, Mason <slash.tmp@free.fr> wrote:
> On 01/12/2015 13:59, Russell King - ARM Linux wrote:
>> On Tue, Dec 01, 2015 at 09:56:23AM +0100, Mason wrote:
>>> [ Back-porting v4.2 patches to v4.1 LTS ]
>>>
>>> On 26/11/2015 09:54, Mason wrote:
>>>
>>>> Over the past few months, I wrote board-support code based on kernel v4.2
>>>> and things worked as expected.
>>>>
>>>> Then I had to rebase to a LTS kernel (I used v4.1.13) and something broke
>>>> in the L2 cache setup. Apparently, Russell King did a lot of fixing and
>>>> cleaning up between v4.1 and v4.2
>>>>
>>>> I cherry-picked the following 5 patches, and my 4.1 kernel works again:
>>>>
>>>> 346248a2d1e3a815297125c1347d90dafcc51990 ARM: l2c: avoid passing auxiliary control register through enable method
>>>>  arch/arm/mm/cache-l2x0.c | 32 +++++++++++++++++---------------
>>>>  1 file changed, 17 insertions(+), 15 deletions(-)
>>>> dc63c0733050996143a82f2b095fc378a04274f0 ARM: l2c: only unlock caches if NS_LOCKDOWN bit is set
>>>>  arch/arm/mm/cache-l2x0.c | 26 +++++++++++++++++++++++++-
>>>>  1 file changed, 25 insertions(+), 1 deletion(-)
>>>> 7787be2a74dc618bf32348a0f588eebf7ebe0a06 ARM: l2c: clean up l2c_configure()
>>>>  arch/arm/mm/cache-l2x0.c | 23 ++++++++++++++---------
>>>>  1 file changed, 14 insertions(+), 9 deletions(-)
>>>> 8f80afa16002e9b4784dc1d51c48f95f52838cfb ARM: l2c: write auxiliary control register first
>>>>  arch/arm/mm/cache-l2x0.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>> 9749167eec6a057122b7a1ab2193abd079645aba ARM: l2c: restore the behaviour documented above l2c_enable()
>>>>  arch/arm/mm/cache-l2x0.c | 10 +++++-----
>>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>>
>>>> The problem is that my system runs Linux in NS (non-secure) mode, and there
>>>> are a bunch of instructions that Linux can't use in that mode.
>>>>
>>>> I suspect the crash comes from trying to unconditionally unlock the caches,
>>>> thus patch dc63c0733050.
>>>>
>>>> But 9749167eec6a and 8f80afa16002 also look like good candidates for
>>>> back-porting to v4.1
>>>>
>>>> What do you think?
>>>
>>> Any opinion on this topic?
>>
>> You can find the answer to this question yourself if you read
>> Documentation/stable_kernel_rules.txt, which gives the rules for what's
>> suitable to be backported to stable trees.  Let's go through them one
>> by one:
>
> I get the feeling you've also brought tar and feathers :-)
>
>>  - It must be obviously correct and tested.
>> Tested yes. Obviously correct - maybe (it's been some time since I wrote
>> the patches.)
>>
>>  - It cannot be bigger than 100 lines, with context.
>> Doubtful, though for individual patches this may be true.
>
> FWIW, I provided the diffstat in my original message. (32, 26, 23, 4, 10)
>
>>  - It must fix only one thing.
>> True of each individual patch.
>>
>>  - It must fix a real bug that bothers people (not a, "This could be a
>>    problem..." type thing).
>> Maybe for your situation, but you're an out of tree user, and the
>> problem doesn't exist with mainline kernels.
>
> <confused>
> Problem would hit anyone running Linux in non-secure mode. The kernel
> will crash when it executes l2c_unlock() which unconditionally writes
> to a secure register.
>
>>  - It must fix a problem that causes a build error (but not for things
>>    marked CONFIG_BROKEN), an oops, a hang, data corruption, a real
>>    security issue, or some "oh, that's not good" issue.  In short, something
>>    critical.
>> Maybe causes an oops for you, but not for any already merged platform,
>> so it isn't a problem that is seen with mainline kernels.
>
> <confused>
> Why would this problem not affect e.g. arch/arm/mach-highbank ?
> (CCing Rob)

It could be broken as I don't have h/w. Some people do, but they may
have not updated to 4.1 yet.

However, the lockdown function is configurable to be enabled for
non-secure mode or not, and on highbank it is enabled for non-secure
mode.

Rob

Re: Patches for arch/arm/mm/cache-l2x0.c

[Russell King - ARM Linux]

On Tue, Dec 01, 2015 at 08:36:40AM -0600, Rob Herring wrote:
> On Tue, Dec 1, 2015 at 8:11 AM, Mason <slash.tmp@free.fr> wrote:
> > Why would this problem not affect e.g. arch/arm/mach-highbank ?
> > (CCing Rob)
> 
> It could be broken as I don't have h/w. Some people do, but they may
> have not updated to 4.1 yet.
> 
> However, the lockdown function is configurable to be enabled for
> non-secure mode or not, and on highbank it is enabled for non-secure
> mode.

... which is how most SoCs (if not all that were in mainline around
that time) configure the unlock NS access bit.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.