* [PATCH] clkdev: Remove duplicated negative index check from __of_clk_get()
From: Geert Uytterhoeven @ 2018-06-01 19:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <152788083877.225090.16757573144833344500@swboyd.mtv.corp.google.com>
Hi Stephen,
On Fri, Jun 1, 2018 at 9:20 PM, Stephen Boyd <sboyd@kernel.org> wrote:
> Quoting Geert Uytterhoeven (2018-05-18 03:58:40)
>> __of_clk_get() calls of_parse_phandle_with_args(), which rejects
>> negative indices since commit bd69f73f2c81eed9 ("of: Create function for
>> counting number of phandles in a property").
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> Commit bd69f73f2c81eed9 is in v3.9.
>
> Did you send this to Russell's patch tracker? Otherwise I can pick it up
Not yet. The patch tracker is for reviewed patches, AFAIK.
> to clk-next.
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH v2 0/5] crypto: Speck support
From: Tomer Ashur @ 2018-06-01 19:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <8c9dc804-1f59-a245-57ba-51db3c234621@esat.kuleuven.be>
[Resending because the email bounced back from all 3 mailing lists.
Sorry if you get this email twice]
Hi Eric et al.,
I know that this thread is already stale, and I'm sorry I couldn't join
earlier but maybe late is better than never. Allow me to first introduce
myself: my name is Tomer Ashur and I'm a post-doctoral fellow in KU
Leuven. I am part of symmetric-key group led by Vincent Rijmen where I'm
mostly involved in cryptanalysis. I am also part of ISO/IEC JTC 1/SC
27/WG 2, the group which decided to reject Simon and Speck from ISO. If
it's okay with you, I'd like to give my perspective on what happened in
ISO and what is Speck's real standing with the academic community.
First, I'd like to say that the NSA has done quite extensive work in
muddying the waters, arguing that Simon & Speck are secure and that all
objections are political. This is not true, as I will now show with
examples. The bottom line is that there are still many open questions
about their security, questions that the NSA has, on multiple occasions,
refused to answer.
> It seems to me justified about as well as one would hope for a new cipher -
> "Notes on the design and analysis of Simon and Speck" seems to me to give ... detail on the reasoning
This is actually an optical illusion. First you need to understand the
context for this document. The NSA (in particular, the exact same person
who previously promoted DUAL_EC in ISO) proposed to include Simon &
Speck in ISO/IEC 29192-2 back in 2015. For obvious reasons they were met
with skepticism. A main concern was the lack of any design rationale and
internal cryptanalytic results. The NSA people fought tooth and nail for
a year and a half simultaneously arguing two almost mutually-exclusive
points: (i) they employ the most talented cryptographers and hence, we
should trust them when they say that an algorithm is secure; and (ii)
they are average cryptographers and hence they would not be able to
insert a backdoor into the algorithm.
More than once they argued in a meeting that the cryptanalysis for the
ciphers has been stabilized (i.e., that attacks will not improve) just
to be proved wrong in the next meeting (their answer: "well, _now_ it
has fully stabilized", which was again proven wrong in the next
meeting). One of them even had a bet with Tanja Lange that no attack on
either Simon or Speck would be extended by 3 rounds or more in the
upcoming year. He lost this bet. They were very uncooperative, and made
it a point to let us know that they will not be providing more
information about the algorithms.
So, in this climate, you can imagine how surprised we all were when in
one of the meetings (after not getting the votes they needed in order to
proceed to the next stage) they announced that they will provide a
design rationale. At first they distributed it to us in ISO, but per my
suggestion they then uploaded it to ePrint (see ePrint 2017/560).
But our joy was short-lived. Once you read this so-called design
rationale you can immediately notice two things. Firstly, that they
explain in length all decisions affecting performance (in particular,
rotation amounts - which in one of the meetings they described as
"most-efficient; secure-enough"). The second thing is that when it comes
to cryptanalysis this document is merely a literature review. There is
literally nothing new there - all they do is to cite published works by
academics, something wrongly.
Now, there is no nice way to say that, but this document includes
omissions, falsehoods, half-truths and outright lies. I will not go into
the full analysis of the document, but here are some examples:
1. Omissions - I already said that this document does not provide any
new information. This becomes apparent when you try to find out how
they chose the number of rounds. The document remains quite vague on
this question. There is a lot of hand waving about "Matsui-like
techniques", "multipath effect", etc. but nowhere you can find (in
the old version, they recently uploaded a new version which I didn't
have time to read yet) a place where they say: "this is how we set
the number of rounds".
Another omission is about the key schedule - you won't find any
useful information about the design decisions leading to these
particular key schedules. Simon uses 3 matrices U,V, and W which are
not explained, not does the constant c. Speck's key schedule is more
straightforward but a discussion about the symmetries that may arise
from using the round function for the key schedule would still be
appropriate here. Not discussing the combined security of the cipher
with its key schedule goes against the current trend in linear
cryptanalysis (see e.g., [2] and many follow up papers).
2. Half-truths -? take a look at page 16 where they explain how they
avoided rotation/slide attacks. They give the standard explanation
that using round-constants would thwart these attacks. This could
have been fine if the last sentence wasn't "/Also see [AL16]/". From
the text it seems as if /AL16/ supports the claims made in this
paragraph. However, /AL16/ is a paper I co-authored which is how I
know that not only that it doesn't support the claim, it actually
shows how to adapt rotational cryptanalysis to algorithms using
round constants.
As a side note, the goal of /AL16/ was to present a novel way to use
rotational cryptanalysis in the presence of round constants. This
paper was published in FSE'17 and we followed up on it with a paper
in FSE'18 using this attack against Speck{32,48,64} [1]. The reason
we focused on these versions and not the larger one is not, as was
suggested in this thread, that they are somehow more secure. The
actual reason is much less prosaic: these are the resources we had
at our disposal. This is also the reason the weak-key classes are so
small. But the fact that my publicly funded university cannot afford
a better number cruncher doesn't mean that someone with access to
such won't be able to find better results. In fact, I am quite
convinced that if you give our tool the resources it needs, it would
penetrate way more than the currently best known distinguisher of 19
rounds for Speck128 (translating to better key recovery attacks).
What is important to understand here is in the same way you do
"real-world crypto", academics often do "proofs of concept". After
publishing the attack technique and the attack on (reduced-)Speck, I
moved to my next project because the scientific marginal benefit is
small. There is of course the personal gain of being known as the
guy who broke Speck, but I'm not particularly interested in such
fame. All of that being said, if anyone has the firepower to run
this tool and to improve the existing attacks for Speck128, feel
free to drop me an email.
3. Falsehoods - with this word I refer to claims in the so-called
design rationale that are wrong. We can argue whether they were
included on purpose or if they are simply mistakes. But in either
case, they are exist and they are worrisome. I would only give one
example: "/the design team?s early analytic efforts led us to
believe that the limiting cryptanalytic features for Simon and
Speck-type block ciphers would be of the linear and differential
sort"/ (see Page 4). Believing that differential and linear attacks
would be the most dangerous attacks is reasonable, but as we can see
from [1], it is wrong.
4. Lies - this is the most troubling part. The NSA lies to the public
(including the American people) on official documents. I already
wrote that the choice for the exact number of rounds is only
motivated through some hand waving. This makes it hard to tell what
the real security margin is. But even if you interpret the hand
waving conservatively, the math results in much smaller security
margins than what is claimed. I gave a rump session talk about this
in Crypto 2017 which you can view here [3]. The talk focuses on
Simon but the story for Speck is similar and results in security
margins of 15.6%, 15.6%, and 14.7% for Speck128 with key sizes 128,
192, and 256, respectively. According to the NSA, that is, and only
if you accept the claim that attacks have stabilized.
the choice for the number of rounds was heavily discussed in the ISO
meeting in Berlin about 6 months ago. When confronted with this
question, the NSA answered (again) that they will not be providing
further information, added that anyone with a decent level of
English would immediately understand what they meant, and called me
an incompetent cryptographer. Nevertheless, a few months after the
meeting they updated the so-called design rationale and added a
footnote that reads:
> "The original version of this paper said 50% here, but noted that
> this was ?very conser-
> vative.? This led to confusion by some, who interpreted 50% as an
> exact value, rather than
> the very conservative upper bound we intended it to be. This is
> supported by the literature
> (see, e.g., [CW15]) and by our internal analysis. Indeed 50% is a
> significant overestimate;
> 25% appears to be a more accurate estimate. We apologize for the
> lack of clarity here, and
> note that even if future advances increased the 25% to 50% Simon
> would still be secure." (Page 11)
This is a fine clarification except that it is an outrageous lie.
For example, for Simon32 the so-called design rationale reports that
the best linear trail can penetrate at most 12 rounds. As part of my
research I found an 18-round linear hull which _was confirmed, in
writing,_ by the NSA (I should have the email somewhere and can find
it if anyone is interested). The difference between 12 and 18 rounds
is indeed 50% and not 25% as they argue in the updated document.
These are only part of the problems I and others found with the
so-called design rationale. Having so many problems in a document meant
to convince people that you're not doing anything sinister is either an
indication for some serious incompetence, or an indication that
something sinister is actually happening. Either way, it is clear that
this document is meant for PR and has no scientific value. It surely
does not inspire confidence in the algorithms.
All of this was known to the people in the room when ISO made its
decision to reject Simon and Speck (after deliberating about this for
more than 3 years. Not because there were disagreements but because we
wanted to give the NSA a fair chance). These people also got a first
hand impression of how poorly the people the NSA sent fare with
_technical_ questions, basically refusing to answer all, and throwing
tantrums instead. And then, the ISO people also saw another thing.
During the discussions I asked the NSA two non-technical questions (from
a crypto point of view. These are technical questions from a
standardization point of view):?
??? - Q: You claim that third party analysis is indicative of the
algorithm's real security. Were you aware of all these results when you
published the algorithms, or are any of them better than what you knew of?
??? - A: I refuse to answer that
??? -Q: Are you aware of any cryptanalytic results better than those
already found by academia?
??? -A: I refuse to answer that either.
Now, there seem to be some notion that the people in ISO are bureaucrats
with limited understanding in cryptography. The truth is that WG 2 (the
cryptography experts) includes people like Kan Yasuda, Shiho Moriai, Dan
Berenstein, Pascal Paillier, Tanja Lange, Orr Dunkelman and Jian Guo
(partial list). You can't say that they don't know what they're doing.
Which is why, having all this information, we decided that including
these algorithms in one of our standards would undermine the trust
people have in ISO and the work it is doing.
Note that in parallel to the Simon and Speck process, people from the
NSA (different from those involved in Simon and Speck) are successfully
promoting at least two other projects. So you can't say that there
really is a significant anti-NSA bias either. No, these algorithms seem
insecure, attacks against them keep improving, their designers either
refuse to answer basic questions about their security or lie... What
other conclusion could we have reached except that there might be a
security problem with these algorithms?
This of course brings us back to the question asked early in this thread:
> support for SM4 was just added too, which is a Chinese government standard. Are you going to send a patch to remove that
> too, or is it just NSA designed algorithms that are not okay?
This seems pretty obvious to me. If you don't feel comfortable with SM4,
don't add it either. There are at least that many reasons to distrust
the Chinese government as there are to distrust the NSA.
However, the answer to the question
> Could you say a little more about what it is that separates Speck from SM4
> for you?
is a bit different. There are two main things that separate Speck from
SM4. Firstly, it seems more secure. This is either because it actually
is more secure, or because the Chinese did a better job in hiding their
backdoors; but at least it doesn't scream "something strange is going on
here!!!". Second, SM4 is also being standardized in ISO these days and
the Chinese are very cooperative with the process. Whatever question you
have about this algorithm, I can get you an answer from the person
promoting SM4. This inspires confidence in the algorithm and the
process. Is this enough? I don't think so. But being a member of ISO I'm
bound by certain rules that don't allow me to reject algorithms based on
my intuition, so it seems that SM4 (as well as LEA and Kuznyechik) would
probably find their way into the respective standards.
That being said, if you ask for my opinion, just don't include SM4.
Which bring us to the million dollar question:
> So, what do you propose replacing it with?
Nothing. I am usually not one to argue for maintaining the status quo
and I sure am in favor of encryption-for-all but this case is the text
book example for employing the Precautionary Principle. You yourself are
not fully convinced that Speck is secure and does not contain any
backdoors. If it was really secure, it could have been used in all cases
and not only on low-end devices where AES is too slow. AES is slower
than Speck on most platforms.
Now, I'm a sort of a mathematician which doesn't know much about
processor generations and implementation efficiency. Things like 134833
KB/s are Chinese to me. But the way I understand it, these devices that
are to weak to support AES would not be around in 2-5 years which would
make the problem go away. In the foreseeable future, even if the
crypto-extension isn't added to low-end processors, they would still
improve to a degree they can run some of the efficient-but-not-enough
algorithms of today, no?
I would also like to point out that including an algorithm because "it's
better than nothing" result in something that is not
better-than-nothing, but stands in the way of good solutions. Since
there is no acute problem, why do we need to solve it? This is from the
cryptographers' point of view. From the end-user point of view when they
get something bundled into Android, they don't know that it was included
there as something that is "better than nothing". They think of it as
"good enough; endorsed by Android/Google/Linux". What you give them is a
false sense of security because they don't know of all the question
marks surrounding Speck (both technical and political).
So I think that as a first step, no-encryption is better than using
Speck. Then we can move for a longer term solution. Since this is an
important enough issue I asked around and people are happily willing to
help. For example, Dan Berenstein seems to believe that a solution can
be built using a generic construction along the lines of your discussion
with Samuel (with or without a variant of ChaCha). Even if a generic
construction cannot be used Berenstein told me he's willing to help
design a solution. I also asked Vincent Rijmen and Orr Dunkelman and
they both told me they'd be willing to work in a team to find (or
design) a solution. This is already an impressive cadre and I'm sure it
would not be too much of a problem to solicit other notable
cryptographer because basically, no one in this community thinks it's a
good idea to use Speck.
Sorry for the long post and Shabbat Shalom,
Tomer Ashur, PhD
Senior Researcher
COSIC, KU Leuven
[1] https://eprint.iacr.org/2017/1036
[2] https://eprint.iacr.org/2012/303
[3] https://www.youtube.com/watch?v=3d-xruyR89g&t=2s
On 05/08/2018 01:20 AM, Eric Biggers wrote:
> Hi Samuel,
>
> On Thu, Apr 26, 2018 at 03:05:44AM +0100, Samuel Neves wrote:
>> On Wed, Apr 25, 2018 at 8:49 PM, Eric Biggers <ebiggers@google.com> wrote:
>>> I agree that my explanation should have been better, and should have considered
>>> more crypto algorithms. The main difficulty is that we have extreme performance
>>> requirements -- it needs to be 50 MB/s at the very least on even low-end ARM
>>> devices like smartwatches. And even with the NEON-accelerated Speck128-XTS
>>> performance exceeding that after much optimization, we've been getting a lot of
>>> pushback as people want closer to 100 MB/s.
>>>
>> I couldn't find any NEON-capable ARMv7 chip below 800 MHz, so this
>> would put the performance upper bound around 15 cycles per byte, with
>> the comfortable number being ~7. That's indeed tough, though not
>> impossible.
>>
>>> That's why I also included Speck64-XTS in the patches, since it was
>>> straightforward to include, and some devices may really need that last 20-30% of
>>> performance for encryption to be feasible at all. (And when the choice is
>>> between unencrypted and a 64-bit block cipher, used in a context where the
>>> weakest points in the cryptosystem are actually elsewhere such as the user's
>>> low-entropy PIN and the flash storage doing wear-leveling, I'd certainly take
>>> the 64-bit block cipher.) So far we haven't had to use Speck64 though, and if
>>> that continues to be the case I'd be fine with Speck64 being removed, leaving
>>> just Speck128.
>>>
>> I would very much prefer that to be the case. As many of us know,
>> "it's better than nothing" has been often used to justify other bad
>> choices, like RC4, that end up preventing better ones from being
>> adopted. At a time where we're trying to get rid of 64-bit ciphers in
>> TLS, where data volumes per session are comparatively low, it would be
>> unfortunate if the opposite starts happening on encryption at rest.
>>
>>> Note that in practice, to have any chance at meeting the performance requirement
>>> the cipher needed to be NEON accelerated. That made benchmarking really hard
>>> and time-consuming, since to definitely know how an algorithm performs it can
>>> take upwards of a week to implement a NEON version. It needs to be very well
>>> optimized too, to compare the algorithms fairly -- e.g. with Speck I got a 20%
>>> performance improvement on some CPUs just by changing the NEON instructions used
>>> to implement the 8-bit rotates, an optimization that is not possible with
>>> ciphers that don't use rotate amounts that are multiples of 8. (This was an
>>> intentional design choice by the Speck designers; they do know what they're
>>> doing, actually.)
>>>
>>> Thus, we had to be pretty aggressive about dropping algorithms from
>>> consideration if there were preliminary indications that they wouldn't perform
>>> well, or had too little cryptanalysis, or had other issues such as an unclear
>>> patent situation. Threefish for example I did test the C implementation at
>>> https://github.com/wernerd/Skein3Fish, but on ARM32 it was over 4 times slower
>>> than my NEON implementation of Speck128/256-XTS. And I did not see a clear way
>>> that it could be improved over 4x with NEON, if at all, so I did not take the
>>> long time it would have taken to write an optimized NEON implementation to
>>> benchmark it properly. Perhaps that was a mistake. But, time is not unlimited.
>>>
>> In my limited experience with NEON and 64-bit ARX, there's usually a
>> ~2x speedup solely from NEON's native 64-bit operations on ARMv7-A.
>> The extra speedup from encrypting 2 block in parallel is then
>> somewhere between 1x and 2x, depending on various details. Getting
>> near 4x might be feasible, but it is indeed time-consuming to get
>> there.
>>
>>> As for the wide-block mode using ChaCha20 and Poly1305, you'd have to ask Paul
>>> Crowley to explain it properly, but briefly it's actually a pseudorandom
>>> permutation over an arbitrarily-sized message. So with dm-crypt for example, it
>>> would operate on a whole 512-byte sector, and if any bit of the 512-byte
>>> plaintext is changed, then every bit in the 512-byte ciphertext would change
>>> with 50% probability. To make this possible, the construction uses a polynomial
>>> evalution in GF(2^130-5) as a universal hash function, similar to the Poly1305
>>> mode.
>>>
>> Oh, OK, that sounds like something resembling Naor-Reingold or its
>> relatives. That would work, but with 3 or 4 passes I guess it wouldn't
>> be very fast.
>>
>>> Using ChaCha20's underlying 512-bit permutation to build a tweakable block
>>> cipher is an interesting idea. But maybe in my crypto-naivety, it is not
>>> obvious to me how to do so. Do you have references to any relevant papers?
>>> Remember that we strongly prefer a published cipher to a custom one -- even if
>>> the core is reused, a mistake may be made in the way it is used. Thus,
>>> similarly to Paul's wide-block mode, I'd be concerned that we'd have to
>>> self-publish a new construction, then use it with no outside crypto review.
>>> *Maybe* it would be straightforward enough to be okay, but to know I'd need to
>>> see the details of how it would actually work.
>>>
>> This would be the 'tweakable Even-Mansour' construction and its
>> variants. The variant I'm most familiar with would be MEM [1],
>> focusing on software friendliness, but there is other provable
>> security work in the same vein, including [3, 4, 5]. It's very similar
>> to how the XEX mode turns a block cipher into a tweakable block
>> cipher.
>>
>> In [1, 2] we used a 1024-bit permutation out of BLAKE2 instead of
>> ChaCha20's, but everything translates easily from one to the other. We
>> also included cheap masks for 512-bit permutations, just in case.
>>
>> [1] https://eprint.iacr.org/2015/999
>> [2] https://github.com/MEM-AEAD/mem-aead
>> [3] https://eprint.iacr.org/2015/539
>> [4] https://eprint.iacr.org/2015/476
>> [5] https://competitions.cr.yp.to/round2/minalpherv11.pdf
>>
>>> But in the end, Speck seemed like the clear choice because it had multiple NEON
>>> implementations available already which showed it could be implemented very
>>> efficiently in NEON; it has over 70 cryptanalysis papers (far more than most
>>> ciphers) yet the security margin is still similar to AES; it has no intellectual
>>> property concerns; there is a paper clearly explaining the design decisions; it
>>> is naturally resistant to timing attacks; it supports a 128-bit block size, so
>>> it can be easily used in XTS mode; it supports the same key sizes as AES; and it
>>> has a simple and understandable design with no "magic numbers" besides 8 and 3
>>> (compare to an actual backdoored algorithm like Dual_EC_DRGB, which basically
>>> had a public key embedded in the algorithm). Also as Paul mentioned he is
>>> confident in the construction, and he has published cryptanalysis on Salsa20, so
>>> his opinion is probably more significant than mine :-)
>>>
>>> But I will definitely take a closer look at SPARX and some of the other ciphers
>>> you mentioned in case I missed something. I really do appreciate the
>>> suggestions, by the way, and in any case we do need to be very well prepared to
>>> justify our choices. I just hope that people can understand that we are
>>> implementing real-world crypto which must operate under *very* tight performance
>>> constraints on ARM processors, and it must be compatible with dm-crypt and
>>> fscrypt with no room for ciphertext expansion. Thus, many algorithms which may
>>> at first seem reasonable choices had to (unfortunately) be excluded.
>>>
>> I understand it is a tough choice, and it's unfortunate that many of
>> the algorithms we have cater mostly to either the
>> high-hardware-accelerated-end or the extremely low-end, without a lot
>> of good options at the middle-end.
>>
> First, we're planning a publication which explains our choices in more detail,
> so please treat this as some more preliminary notes.
>
> To make sure we've exhausted as many alternatives as possible, I wrote NEON
> implementations of all the block ciphers you suggested with the exception of
> SKINNY (which looked very hardware-oriented and not efficient in software), as
> well as some that others have suggested. (It was tough, but after doing a
> couple, it got much easier...) The following shows the decryption performance
> I'm getting on an ARMv7 platform. Encryption speeds were usually similar, but
> in our use case we care much more about decryption, as that affects the most
> critical metrics such as the time to launch applications.
>
> ChaCha8-MEM: 183256 KB/s
> ChaCha12-MEM: 134833 KB/s
> Chaskey-LTS-XTS: 99097 KB/s
> ChaCha20-MEM: 87875 KB/s
> Speck64/128-XTS: 85332 KB/s
> Speck128/128-XTS: 73404 KB/s
> RC5-128/12/256-XTS: 69887 KB/s
> Speck128/256-XTS: 69597 KB/s
> RC5-64/12/128-XTS: 69267 KB/s
> LEA-128-XTS: 67986 KB/s
> CHAM128/128-XTS: 52982 KB/s
> LEA-256-XTS: 50429 KB/s
> Threefish-256: 48349 KB/s
> RC6-XTS: 46855 KB/s
> RC5-128/20/256-XTS: 44291 KB/s
> RC5-64/20/128-XTS: 43924 KB/s
> NOEKEON-XTS: 40705 KB/s
> Sparx128/128-XTS: 39191 KB/s
> XTEA-XTS: 38239 KB/s
> AES-128-XTS: 25549 KB/s
> AES-256-XTS: 18640 KB/s
>
> Remember that for dm-crypt or fscrypt over flash storage and/or f2fs, a stream
> cipher is insecure. Moreover, on these (low-end) devices the status quo is no
> encryption, and we need every bit of performance available. Anything below
> 50 MB/s is definitely unacceptable. But even at that speed we get many
> complaints, so in practice we need something faster. That means that the
> algorithms close to 50 MB/s, such as Threefish, still aren't fast enough.
>
> ChaCha-MEM (based roughly on your paper: https://eprint.iacr.org/2015/999), has
> the best performance, especially if we allow for the 12 or 8-round variants. My
> code for it is based roughly on the existing
> arch/arm/crypto/chacha20-neon-core.S, but updated to support the inverse
> permutation (on 4 blocks at a time, using all 16 NEON registers) and do the
> masking required by MEM. However, ChaCha-MEM would be a pretty bleeding-edge
> and customized construction, and Paul Crowley and I have concerns about its
> security. The problem is that the MEM security proof assumes that the
> underlying permutation has no more detectable structural properties than a
> randomly selected permutation. However, the ChaCha permutation is known to have
> certain symmetries, e.g. if the sixteen 32-bit words are (a, a, a, a, b, b, b,
> b, c, c, c, c, d, d, d, d), then they always map to some (e, e, e, e, f, f, f,
> f, g, g, g, g, h, h, h, h).
>
> For the MEM mask generation, we can use the "expand 32-byte k" constant to break
> the symmetry, like is done in the ChaCha stream cipher. However, that's not
> possible for the inner application of the permutation. So, we'd be using the
> ChaCha permutation in a manner in which it wasn't intended, and the security of
> the ChaCha stream cipher wouldn't directly carry over. Granted, it's not
> impossible that it would be secure, but at the present time it doesn't seem like
> a good choice to actually field.
>
> Chaskey-LTS is faster than Speck, but unfortunately it's not really a viable
> option because it has only a 64-bit security level, due to its use of the
> Even-Mansour construction with a 128-bit key. Of course, it would still be
> better than nothing, but we prefer a cipher that has a security level in line
> with what is accepted for modern crypto.
>
> RC5 with the traditional 12 rounds is about as fast as Speck, but there is a
> known differential attack on that number of rounds. So if we choose RC5 we'd
> almost certainly have to use the 20-round variant, which is much slower.
>
> That leaves LEA-128-XTS as the only other algorithm that might meet the
> performance requirement, as it is only slightly slower than Speck128-XTS. It
> may be the most viable alternative, but beyond the slight performance loss it
> still has some disadvantages compared to Speck:
>
> - Importantly, the LEA authors forgot to include test vectors, so I'm not yet
> 100% sure I implemented it correctly. (The Speck authors unfortunately didn't
> make the endianness of their test vectors clear in their initial publication,
> but at least they actually provided test vectors!)
> - LEA has received some cryptanalysis, but not nearly as much as Speck.
> - It took some very heavy optimization to get good LEA performance, much more
> than I had to do for Speck. My final LEA code has separate code paths for
> 128-bit and 256-bit keys, and has reordered and preprocessed the round keys,
> and reordered the operations. As a result, it's harder to see how it maps to
> the original paper. In contrast, my Speck code is more straightforward and
> maintainable.
> - LEA-256 (256-bit key) is much slower than LEA-128 (128-bit key), as it has
> 33% more rounds. LEA-256 would not be fast enough, so we would have to use
> LEA-128. In contrast, with Speck we can use Speck128/256 (256-bit key).
> We're willing to accept a 128-bit security level, but 256-bit is preferable.
> (I think the Speck designers took a more informed approach to setting
> appropriate security margins for a lightweight cipher; it seems that other
> designers often choose too few or too many rounds, especially as the key
> length is varied.)
> - LEA encryption is also a bit slower than decryption, while with Speck
> encryption and decryption are almost exactly the same speed.
>
> Note that like Speck, LEA doesn't appear to be approved by a standards
> organization either; it's just specified in a research paper.
>
> Thus, from a technical perspective, and given the current state of the art in
> lightweight cryptography, currently Speck128-XTS seems to be the best choice for
> the problem domain. It's unfortunate that there are so few good options and
> that the field is so politicized, but it is what it is.
>
> Still, we don't want to abandon HPolyC (Paul's new ChaCha and Poly1305-based
> wide-block mode), and eventually we hope to offer it as an option as well. But
> it's not yet published, and it's a more complex algorithm that is harder to
> implement so I haven't yet had a chance to implement and benchmark it. And we
> don't want to continue to leave users unprotected while we spend a long time
> coming up with the perfect algorithm, or for hardware AES support to arrive to
> all low-end CPUs when it's unclear if/when that will happen.
>
> Again, we're planning a publication which will explain all this in more detail.
>
> Thanks!
>
> Eric
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180601/c1da80f3/attachment-0001.sig>
^ permalink raw reply
* [RFC PATCH 2/8] coresight: Fix remote endpoint parsing
From: Mathieu Poirier @ 2018-06-01 19:38 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527858967-16047-3-git-send-email-suzuki.poulose@arm.com>
On Fri, Jun 01, 2018 at 02:16:01PM +0100, Suzuki K Poulose wrote:
> When parsing the remote endpoint of an output port, we do :
> rport = of_graph_get_remote_port(ep);
> rparent = of_graph_get_remote_port_parent(ep);
>
> and then parse the "remote_port" as if it was the remote endpoint,
> which is wrong. The code worked fine because we used endpoint number
> as the port number. Let us fix it and optimise a bit as:
>
> remote_ep = of_graph_get_remote_endpoint(ep);
> if (remote_ep)
> remote_parent = of_graph_get_port_parent(remote_ep);
>
> and then, parse the remote_ep for the port/endpoint details.
>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
> drivers/hwtracing/coresight/of_coresight.c | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/of_coresight.c b/drivers/hwtracing/coresight/of_coresight.c
> index 7c37544..e0deab0 100644
> --- a/drivers/hwtracing/coresight/of_coresight.c
> +++ b/drivers/hwtracing/coresight/of_coresight.c
> @@ -128,7 +128,7 @@ of_get_coresight_platform_data(struct device *dev,
> struct device *rdev;
> struct device_node *ep = NULL;
> struct device_node *rparent = NULL;
> - struct device_node *rport = NULL;
> + struct device_node *rep = NULL;
>
> pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> if (!pdata)
> @@ -169,16 +169,17 @@ of_get_coresight_platform_data(struct device *dev,
> pdata->outports[i] = endpoint.port;
>
> /*
> - * Get a handle on the remote port and parent
> - * attached to it.
> + * Get a handle on the remote endpoint and the device
> + * it is attached to.
> */
> - rparent = of_graph_get_remote_port_parent(ep);
> - rport = of_graph_get_remote_port(ep);
> -
> - if (!rparent || !rport)
> + rep = of_graph_get_remote_endpoint(ep);
> + if (!rep)
> + continue;
> + rparent = of_graph_get_port_parent(rep);
> + if (!rparent)
> continue;
>
> - if (of_graph_parse_endpoint(rport, &rendpoint))
> + if (of_graph_parse_endpoint(rep, &rendpoint))
> continue;
You are correct and I'm out to lunch.
>
> rdev = of_coresight_get_endpoint_device(rparent);
> @@ -186,7 +187,7 @@ of_get_coresight_platform_data(struct device *dev,
> return ERR_PTR(-EPROBE_DEFER);
>
> pdata->child_names[i] = dev_name(rdev);
> - pdata->child_ports[i] = rendpoint.id;
> + pdata->child_ports[i] = rendpoint.port;
You need to do a of_node_put() here for both rep and rparent.
>
> i++;
> } while (ep);
> --
> 2.7.4
>
^ permalink raw reply
* [RFC PATCH 2/8] coresight: Fix remote endpoint parsing
From: Mathieu Poirier @ 2018-06-01 19:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180601193837.GB9838@xps15>
On 1 June 2018 at 13:38, Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
> On Fri, Jun 01, 2018 at 02:16:01PM +0100, Suzuki K Poulose wrote:
>> When parsing the remote endpoint of an output port, we do :
>> rport = of_graph_get_remote_port(ep);
>> rparent = of_graph_get_remote_port_parent(ep);
>>
>> and then parse the "remote_port" as if it was the remote endpoint,
>> which is wrong. The code worked fine because we used endpoint number
>> as the port number. Let us fix it and optimise a bit as:
>>
>> remote_ep = of_graph_get_remote_endpoint(ep);
>> if (remote_ep)
>> remote_parent = of_graph_get_port_parent(remote_ep);
>>
>> and then, parse the remote_ep for the port/endpoint details.
>>
>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>> drivers/hwtracing/coresight/of_coresight.c | 19 ++++++++++---------
>> 1 file changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/of_coresight.c b/drivers/hwtracing/coresight/of_coresight.c
>> index 7c37544..e0deab0 100644
>> --- a/drivers/hwtracing/coresight/of_coresight.c
>> +++ b/drivers/hwtracing/coresight/of_coresight.c
>> @@ -128,7 +128,7 @@ of_get_coresight_platform_data(struct device *dev,
>> struct device *rdev;
>> struct device_node *ep = NULL;
>> struct device_node *rparent = NULL;
>> - struct device_node *rport = NULL;
>> + struct device_node *rep = NULL;
>>
>> pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> if (!pdata)
>> @@ -169,16 +169,17 @@ of_get_coresight_platform_data(struct device *dev,
>> pdata->outports[i] = endpoint.port;
>>
>> /*
>> - * Get a handle on the remote port and parent
>> - * attached to it.
>> + * Get a handle on the remote endpoint and the device
>> + * it is attached to.
>> */
>> - rparent = of_graph_get_remote_port_parent(ep);
>> - rport = of_graph_get_remote_port(ep);
>> -
>> - if (!rparent || !rport)
>> + rep = of_graph_get_remote_endpoint(ep);
>> + if (!rep)
>> + continue;
>> + rparent = of_graph_get_port_parent(rep);
>> + if (!rparent)
>> continue;
>>
>> - if (of_graph_parse_endpoint(rport, &rendpoint))
>> + if (of_graph_parse_endpoint(rep, &rendpoint))
>> continue;
>
> You are correct and I'm out to lunch.
>
>>
>> rdev = of_coresight_get_endpoint_device(rparent);
>> @@ -186,7 +187,7 @@ of_get_coresight_platform_data(struct device *dev,
>> return ERR_PTR(-EPROBE_DEFER);
>>
>> pdata->child_names[i] = dev_name(rdev);
>> - pdata->child_ports[i] = rendpoint.id;
>> + pdata->child_ports[i] = rendpoint.port;
>
> You need to do a of_node_put() here for both rep and rparent.
Same thing for the "continue" and error condition above.
>
>>
>> i++;
>> } while (ep);
>> --
>> 2.7.4
>>
^ permalink raw reply
* [PATCH] arm64: alternative:flush cache with unpatched code
From: Rohit Khanna @ 2018-06-01 19:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180601090321.sy3t64rtps7qn2nx@salmiak>
[RK] - Thanks for the comments Mark. Reply inlined.
Thanks
Rohit
________________________________________
From: Mark Rutland <mark.rutland@arm.com>
Sent: Friday, June 1, 2018 2:03 AM
To: Rohit Khanna
Cc: catalin.marinas at arm.com; robin.murphy at arm.com; Suzuki.Poulose at arm.com; linux-arm-kernel at lists.infradead.org; Alexander Van Brunt; Bo Yan; will.deacon at arm.com
Subject: Re: [PATCH] arm64: alternative:flush cache with unpatched code
Hi,
As a general thing, could you please add a version number to patches in future?
i.e. this should be PATCHv4.
It really helps keeping track of patches, distinguishing versions, etc.
On Thu, May 31, 2018 at 01:37:34PM -0700, Rohit Khanna wrote:
> In the current implementation, __apply_alternatives patches
> flush_icache_range and then executes it without invalidating the icache.
> Thus, icache can contain some of the old instructions for
> flush_icache_range. This can cause unpredictable behavior as during
> execution we can get a mix of old and new instructions for
> flush_icache_range.
>
> This patch :
>
> 1. Adds a new function clean_dcache_range_nopatch for flushing kernel
> memory range. This function uses non hot-patched code and can be
> safely used to flush cache during code patching.
>
> 2. Modifies __apply_alternatives so that it uses
> clean_dcache_range_nopatch to flush the cache range after patching code.
>
> Signed-off-by: Rohit Khanna <rokhanna@nvidia.com>
> ---
> arch/arm64/include/asm/sysreg.h | 3 +++
> arch/arm64/kernel/alternative.c | 37 ++++++++++++++++++++++++++++++++++++-
> 2 files changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 6171178075dc..9d1aee7c9aba 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -617,6 +617,9 @@
> #define MVFR1_FPDNAN_SHIFT 4
> #define MVFR1_FPFTZ_SHIFT 0
>
> +/* SYS_CTR_EL0 */
> +#define SYS_CTR_ISIZE_SHIFT 0
> +#define SYS_CTR_DSIZE_SHIFT 16
We already have CTR_DMINLINE_SHIFT in <asm/cache.h>
Can we please add CTR_IMINLIN_SHIFT there too?
Maybe those should be moved into sysreg.h, but that can be a separate cleanup.
[RK] - <asm/cache.h> doesnt contain CTR_DMINLINE_SHIFT.
> #define ID_AA64MMFR0_TGRAN4_SHIFT 28
> #define ID_AA64MMFR0_TGRAN64_SHIFT 24
> diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
> index 5c4bce4ac381..6b8c5438b37b 100644
> --- a/arch/arm64/kernel/alternative.c
> +++ b/arch/arm64/kernel/alternative.c
> @@ -122,6 +122,41 @@ static void patch_alternative(struct alt_instr *alt,
> }
> }
>
> +/* This is used for flushing kernel memory range after
> + * __apply_alternatives has patched kernel code
> + */
> +static void clean_dcache_range_nopatch(void *start, void *end)
> +{
> + u64 d_start, i_start, d_size, i_size, ctr_el0;
I don't think we need separate i_start and d_start variables. A 'start' or
'cur' variable could be used for both.
[RK] - ok.
> +
> + /* use sanitised value of ctr_el0 rather than raw value from CPU */
> + ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
> + /* size in bytes */
> + d_size = cpuid_feature_extract_unsigned_field(ctr_el0,
> + SYS_CTR_DSIZE_SHIFT);
> + i_size = cpuid_feature_extract_unsigned_field(ctr_el0,
> + SYS_CTR_ISIZE_SHIFT);
This isn't the size in bytes. Each is log2 the number of (4-byte) words.
i.e. the size in bytes is (xMinLine << 2).
[RK] - This doesnt seem right. For eg if IMinLine = 4 or 0b100
then with above formula ICacheSize in Bytes = 4 << 2 = 16
The correct formula should be (4 << xMinLine).
So in case IMinLine = 4 or 0b100,
ICacheSizeBytes = 4 << 4 = 64B
> +
> + d_start = (u64)start & ~(d_size - 1);
> + while (d_start <= (u64)end) {
> + /* Use civac instead of cvau. This is required
> + * due to ARM errata 826319, 827319, 824069,
> + * 819472 on A53
> + */
> + asm volatile("dc civac, %0" : : "r" (d_start));
Either this needs a memory clobber, or we need barrier() first, to ensure that
the compiler doesn't re-order this against some of the patching code, however
unlikely that may be.
[RK] - So add barrier() before calling clean_dcache_range_nopatch() ?
> + d_start += d_size;
> + }
The loop can be simplified to:
do {
asm ( ... );
} while (d_start += d_size, d_start < (u64)end)
[RK] - ok
> + dsb(ish);
> +
> + i_start = (u64)start & ~(i_size - 1);
> + while (i_start <= (u64)end) {
> + asm volatile("ic ivau, %0" : : "r" (i_start));
> + i_start += i_size;
> + }
Likewise here.
[RK] - ok
Thanks,
Mark.
> + dsb(ish);
> + isb();
> +}
> +
> static void __apply_alternatives(void *alt_region, bool use_linear_alias)
> {
> struct alt_instr *alt;
> @@ -155,7 +190,7 @@ static void __apply_alternatives(void *alt_region, bool use_linear_alias)
>
> alt_cb(alt, origptr, updptr, nr_inst);
>
> - flush_icache_range((uintptr_t)origptr,
> + clean_dcache_range_nopatch((uintptr_t)origptr,
> (uintptr_t)(origptr + nr_inst));
> }
> }
> --
> 2.1.4
>
^ permalink raw reply
* linux-next-20180601: build error in arch/arm64/kvm/hyp/hyp-entry.S
From: Stefan Wahren @ 2018-06-01 20:17 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
i can't build todays linux-next-20180601 and get the following error message:
arch/arm64/kvm/hyp/hyp-entry.S: Assembler messages:
arch/arm64/kvm/hyp/hyp-entry.S:128: Error: constant expression required at operand 3 -- `bfi x0,x1,#VCPU_WORKAROUND_2_FLAG_SHIFT,#1'
Related commit:
arm64: KVM: Handle guest's ARCH_WORKAROUND_2 requests
Toolchain: gcc-linaro-7.2.1-2017.11-x86_64_aarch64-linux-gnu
Kernel config: arm64/defconfig
Regards
Stefan
^ permalink raw reply
* [RFC PATCH 6/8] dts: coresight: Clean up the device tree graph bindings
From: Mathieu Poirier @ 2018-06-01 20:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527858967-16047-7-git-send-email-suzuki.poulose@arm.com>
On Fri, Jun 01, 2018 at 02:16:05PM +0100, Suzuki K Poulose wrote:
> The coresight drivers relied on default bindings for graph
> in DT, while reusing the "reg" field of the "ports" to indicate
> the actual hardware port number for the connections. However,
> with the rules getting stricter w.r.t to the address mismatch
> with the label, it is no longer possible to use the port address
> field for the hardware port number. Hence, we add an explicit
> property to denote the hardware port number, "coresight,hwid"
> which must be specified for each "endpoint".
>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Rob Herring <robh@kernel.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
> .../devicetree/bindings/arm/coresight.txt | 26 +++++++++---
> drivers/hwtracing/coresight/of_coresight.c | 46 ++++++++++++++++------
> 2 files changed, 54 insertions(+), 18 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
> index bd36e40..385581a 100644
> --- a/Documentation/devicetree/bindings/arm/coresight.txt
> +++ b/Documentation/devicetree/bindings/arm/coresight.txt
> @@ -104,7 +104,11 @@ properties to uniquely identify the connection details.
> "slave-mode"
>
> * Hardware Port number at the component:
> - - The hardware port number is assumed to be the address of the "port" component.
> + - (Obsolete) The hardware port number is assumed to be the address of the "port" component.
> + - Each "endpoint" must define the hardware port of the local end of the
> + connection using the following property:
> + "coresight,hwid" - 32bit integer, hardware port number at the local end.
> +
>
>
> Example:
> @@ -120,6 +124,7 @@ Example:
> etb_in_port: endpoint at 0 {
> slave-mode;
> remote-endpoint = <&replicator_out_port0>;
> + coresight,hwid = <0>;
> };
> };
> };
> @@ -134,6 +139,7 @@ Example:
> tpiu_in_port: endpoint at 0 {
> slave-mode;
> remote-endpoint = <&replicator_out_port1>;
> + coresight,hwid = <0>;
> };
> };
> };
> @@ -154,6 +160,7 @@ Example:
> reg = <0>;
> replicator_out_port0: endpoint {
> remote-endpoint = <&etb_in_port>;
> + coresight,hwid = <0>;
> };
> };
>
> @@ -161,15 +168,17 @@ Example:
> reg = <1>;
> replicator_out_port1: endpoint {
> remote-endpoint = <&tpiu_in_port>;
> + coresight,hwid = <1>;
> };
> };
>
> /* replicator input port */
> port at 2 {
> - reg = <0>;
> + reg = <1>;
> replicator_in_port0: endpoint {
> slave-mode;
> remote-endpoint = <&funnel_out_port0>;
> + coresight,hwid = <0>;
> };
> };
> };
> @@ -191,31 +200,35 @@ Example:
> funnel_out_port0: endpoint {
> remote-endpoint =
> <&replicator_in_port0>;
> + coresight,hwid = <0>;
> };
> };
>
> /* funnel input ports */
> port at 1 {
> - reg = <0>;
> + reg = <1>;
> funnel_in_port0: endpoint {
> slave-mode;
> remote-endpoint = <&ptm0_out_port>;
> + coresight,hwid = <0>;
> };
> };
>
> port at 2 {
> - reg = <1>;
> + reg = <2>;
> funnel_in_port1: endpoint {
> slave-mode;
> remote-endpoint = <&ptm1_out_port>;
> + coresight,hwid = <1>;
> };
> };
>
> port at 3 {
> - reg = <2>;
> + reg = <3>;
> funnel_in_port2: endpoint {
> slave-mode;
> remote-endpoint = <&etm0_out_port>;
> + coresight,hwid = <2>;
> };
> };
>
> @@ -233,6 +246,7 @@ Example:
> port {
> ptm0_out_port: endpoint {
> remote-endpoint = <&funnel_in_port0>;
> + coresight,hwid = <0>;
> };
> };
> };
> @@ -247,6 +261,7 @@ Example:
> port {
> ptm1_out_port: endpoint {
> remote-endpoint = <&funnel_in_port1>;
> + coresight,hwid = <0>;
> };
> };
> };
> @@ -263,6 +278,7 @@ Example:
> port {
> stm_out_port: endpoint {
> remote-endpoint = <&main_funnel_in_port2>;
> + coresight,hwid = <0>;
> };
> };
> };
> diff --git a/drivers/hwtracing/coresight/of_coresight.c b/drivers/hwtracing/coresight/of_coresight.c
> index a3f3416..99d7a9c 100644
> --- a/drivers/hwtracing/coresight/of_coresight.c
> +++ b/drivers/hwtracing/coresight/of_coresight.c
> @@ -105,14 +105,37 @@ int of_coresight_get_cpu(const struct device_node *node)
> }
> EXPORT_SYMBOL_GPL(of_coresight_get_cpu);
>
> +/*
> + * of_graph_ep_coresight_get_port_id : Get the hardware port number for the
> + * given endpoint device node. Prefer the explicit "coresight,hwid" property
> + * over the endpoint register id (obsolete bindings).
> + */
> +static int of_graph_ep_coresight_get_port_id(struct device *dev,
> + struct device_node *ep_node)
static int of_coresight_endpoint_get_port_id(struct device *dev,
struct device_node *ep_node)
I think that makes more sense since this function is only visible in this file.
> +{
> + struct of_endpoint ep;
> + int rc, port_id;
> +
> +
> + if (!of_property_read_u32(ep_node, "coresight,hwid", &port_id))
> + return port_id;
> +
> + rc = of_graph_parse_endpoint(ep_node, &ep);
> + if (rc)
> + return rc;
> + dev_warn_once(dev,
> + "ep%d: Mandatory \"coresight,hwid\" property missing."
> + " DT uses obsolete coresight bindings\n", ep.port);
> + return ep.port;
> +}
> +
> struct coresight_platform_data *
> of_get_coresight_platform_data(struct device *dev,
> const struct device_node *node)
> {
> - int ret = 0;
> + int ret = 0, outport, inport;
> struct coresight_platform_data *pdata;
> struct coresight_connection *conn;
> - struct of_endpoint endpoint, rendpoint;
> struct device *rdev;
> struct device_node *ep = NULL;
> struct device_node *rparent = NULL;
> @@ -148,14 +171,10 @@ of_get_coresight_platform_data(struct device *dev,
> if (of_find_property(ep, "slave-mode", NULL))
> continue;
>
> - /* Get a handle on the local endpoint */
> - ret = of_graph_parse_endpoint(ep, &endpoint);
> -
> - if (ret)
> + outport = of_graph_ep_coresight_get_port_id(dev, ep);
> + if (outport < 0)
> continue;
> -
> - /* The local out port number */
> - conn->outport = endpoint.port;
> + conn->outport = outport;
>
> /*
> * Get a handle on the remote endpoint and the device
> @@ -168,15 +187,16 @@ of_get_coresight_platform_data(struct device *dev,
> if (!rparent)
> continue;
>
> - if (of_graph_parse_endpoint(rep, &rendpoint))
> - continue;
> -
> rdev = of_coresight_get_endpoint_device(rparent);
> if (!rdev)
> return ERR_PTR(-EPROBE_DEFER);
>
> + inport = of_graph_ep_coresight_get_port_id(rdev, rep);
> + if (inport < 0)
> + continue;
> +
> conn->child_name = dev_name(rdev);
> - conn->child_port = rendpoint.port;
> + conn->child_port = inport;
> conn++;
> } while (ep);
> }
> --
> 2.7.4
>
^ permalink raw reply
* [RFC PATCH 7/8] dts: coresight: Define new bindings for direction of data flow
From: Mathieu Poirier @ 2018-06-01 20:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527858967-16047-8-git-send-email-suzuki.poulose@arm.com>
On Fri, Jun 01, 2018 at 02:16:06PM +0100, Suzuki K Poulose wrote:
> So far we have relied on an undocumented property "slave-mode",
> to indicate if the given port is input or not. Since we are
> redefining the coresight bindings, define new property for the
> "direction" of data flow for a given connection endpoint in the
> device.
>
> Each endpoint must define the following property.
>
> - "direction" : 0 => Port is input
> 1 => Port is output
>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
> drivers/hwtracing/coresight/of_coresight.c | 20 ++++++++++++++++----
You haven't documented the binding in bindings/arm/coresight.txt the same way
you did with "coresight,hwid". I'm guessing you simply forgot to do a "git add"
on the file when preparing the patchset.
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/of_coresight.c b/drivers/hwtracing/coresight/of_coresight.c
> index 99d7a9c..63c1668 100644
> --- a/drivers/hwtracing/coresight/of_coresight.c
> +++ b/drivers/hwtracing/coresight/of_coresight.c
> @@ -52,7 +52,19 @@ of_coresight_get_endpoint_device(struct device_node *endpoint)
> endpoint, of_dev_node_match);
> }
>
> -static void of_coresight_get_ports(const struct device_node *node,
> +static bool of_coresight_ep_is_input(struct device *dev, struct device_node *ep_node)
I suggested of_coresight_endpoint_get_port_id() in my review of 6/8. I'm good
with either "ep" or "endpoint", as long as the names are consistent.
> +{
> + u32 dir;
> +
> + if (!of_property_read_u32(ep_node, "direction", &dir))
> + return dir == 0;
> +
> + dev_warn_once(dev, "Missing mandatory \"direction\" property!\n");
> + return of_property_read_bool(ep_node, "slave-mode");
> +}
> +
> +static void of_coresight_get_ports(struct device *dev,
> + const struct device_node *node,
> int *nr_inport, int *nr_outport)
> {
> struct device_node *ep = NULL;
> @@ -63,7 +75,7 @@ static void of_coresight_get_ports(const struct device_node *node,
> if (!ep)
> break;
>
> - if (of_property_read_bool(ep, "slave-mode"))
> + if (of_coresight_ep_is_input(dev, ep))
> in++;
> else
> out++;
> @@ -149,7 +161,7 @@ of_get_coresight_platform_data(struct device *dev,
> pdata->name = dev_name(dev);
>
> /* Get the number of input and output port for this component */
> - of_coresight_get_ports(node, &pdata->nr_inport, &pdata->nr_outport);
> + of_coresight_get_ports(dev, node, &pdata->nr_inport, &pdata->nr_outport);
>
> if (pdata->nr_outport) {
> ret = of_coresight_alloc_memory(dev, pdata);
> @@ -168,7 +180,7 @@ of_get_coresight_platform_data(struct device *dev,
> * No need to deal with input ports, processing for as
> * processing for output ports will deal with them.
> */
> - if (of_find_property(ep, "slave-mode", NULL))
> + if (of_coresight_ep_is_input(dev, ep))
> continue;
>
> outport = of_graph_ep_coresight_get_port_id(dev, ep);
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH 0/3] Add R8A77980/Condor/V3HSK GPIO support
From: Sergei Shtylyov @ 2018-06-01 20:41 UTC (permalink / raw)
To: linux-arm-kernel
Hello!
Here's the set of 3 patches against Simon Horman's 'renesas.git' repo's
'renesas-devel-20180529-v4.17-rc7' tag plus the I2C patches reposted yesterday.
We're adding the R8A77980 GPIO nodes and then describing the PHY IRQ for the
GEther/EtherAVB devices declared earlier.
[1/3] arm64: dts: renesas: r8a77980: add GPIO support
[2/3] arm64: dts: renesas: condor: specify EtherAVB PHY IRQ
[3/3] arm64: dts: renesas: v3hsk: specify GEther PHY IRQ
WBR, Sergei
^ permalink raw reply
* [PATCH 1/3] arm64: dts: renesas: r8a77980: add GPIO support
From: Sergei Shtylyov @ 2018-06-01 20:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <4acc208e-c593-1e8a-00ca-fc9a5574074e@cogentembedded.com>
Describe all 6 GPIO controllers in the R8A77980 device tree.
Based on the original (and large) patch by Vladimir Barinov.
Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
---
arch/arm64/boot/dts/renesas/r8a77980.dtsi | 90 ++++++++++++++++++++++++++++++
1 file changed, 90 insertions(+)
Index: renesas/arch/arm64/boot/dts/renesas/r8a77980.dtsi
===================================================================
--- renesas.orig/arch/arm64/boot/dts/renesas/r8a77980.dtsi
+++ renesas/arch/arm64/boot/dts/renesas/r8a77980.dtsi
@@ -118,6 +118,96 @@
#size-cells = <2>;
ranges;
+ gpio0: gpio at e6050000 {
+ compatible = "renesas,gpio-r8a77980",
+ "renesas,rcar-gen3-gpio";
+ reg = <0 0xe6050000 0 0x50>;
+ interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
+ #gpio-cells = <2>;
+ gpio-controller;
+ gpio-ranges = <&pfc 0 0 22>;
+ #interrupt-cells = <2>;
+ interrupt-controller;
+ clocks = <&cpg CPG_MOD 912>;
+ power-domains = <&sysc R8A77980_PD_ALWAYS_ON>;
+ resets = <&cpg 912>;
+ };
+
+ gpio1: gpio at e6051000 {
+ compatible = "renesas,gpio-r8a77980",
+ "renesas,rcar-gen3-gpio";
+ reg = <0 0xe6051000 0 0x50>;
+ interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
+ #gpio-cells = <2>;
+ gpio-controller;
+ gpio-ranges = <&pfc 0 32 28>;
+ #interrupt-cells = <2>;
+ interrupt-controller;
+ clocks = <&cpg CPG_MOD 911>;
+ power-domains = <&sysc R8A77980_PD_ALWAYS_ON>;
+ resets = <&cpg 911>;
+ };
+
+ gpio2: gpio at e6052000 {
+ compatible = "renesas,gpio-r8a77980",
+ "renesas,rcar-gen3-gpio";
+ reg = <0 0xe6052000 0 0x50>;
+ interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>;
+ #gpio-cells = <2>;
+ gpio-controller;
+ gpio-ranges = <&pfc 0 64 30>;
+ #interrupt-cells = <2>;
+ interrupt-controller;
+ clocks = <&cpg CPG_MOD 910>;
+ power-domains = <&sysc R8A77980_PD_ALWAYS_ON>;
+ resets = <&cpg 910>;
+ };
+
+ gpio3: gpio at e6053000 {
+ compatible = "renesas,gpio-r8a77980",
+ "renesas,rcar-gen3-gpio";
+ reg = <0 0xe6053000 0 0x50>;
+ interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
+ #gpio-cells = <2>;
+ gpio-controller;
+ gpio-ranges = <&pfc 0 96 17>;
+ #interrupt-cells = <2>;
+ interrupt-controller;
+ clocks = <&cpg CPG_MOD 909>;
+ power-domains = <&sysc R8A77980_PD_ALWAYS_ON>;
+ resets = <&cpg 909>;
+ };
+
+ gpio4: gpio at e6054000 {
+ compatible = "renesas,gpio-r8a77980",
+ "renesas,rcar-gen3-gpio";
+ reg = <0 0xe6054000 0 0x50>;
+ interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
+ #gpio-cells = <2>;
+ gpio-controller;
+ gpio-ranges = <&pfc 0 128 25>;
+ #interrupt-cells = <2>;
+ interrupt-controller;
+ clocks = <&cpg CPG_MOD 908>;
+ power-domains = <&sysc R8A77980_PD_ALWAYS_ON>;
+ resets = <&cpg 908>;
+ };
+
+ gpio5: gpio at e6055000 {
+ compatible = "renesas,gpio-r8a77980",
+ "renesas,rcar-gen3-gpio";
+ reg = <0 0xe6055000 0 0x50>;
+ interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
+ #gpio-cells = <2>;
+ gpio-controller;
+ gpio-ranges = <&pfc 0 160 15>;
+ #interrupt-cells = <2>;
+ interrupt-controller;
+ clocks = <&cpg CPG_MOD 907>;
+ power-domains = <&sysc R8A77980_PD_ALWAYS_ON>;
+ resets = <&cpg 907>;
+ };
+
pfc: pin-controller at e6060000 {
compatible = "renesas,pfc-r8a77980";
reg = <0 0xe6060000 0 0x50c>;
^ permalink raw reply
* [PATCH 2/3] arm64: dts: renesas: condor: specify EtherAVB PHY IRQ
From: Sergei Shtylyov @ 2018-06-01 20:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <4acc208e-c593-1e8a-00ca-fc9a5574074e@cogentembedded.com>
Specify EtherAVB PHY IRQ in the Condor board's device tree, now that
we have the GPIO support (previously phylib had to resort to polling).
Based on the original (and large) patch by Vladimir Barinov.
Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
---
arch/arm64/boot/dts/renesas/r8a77980-condor.dts | 2 ++
1 file changed, 2 insertions(+)
Index: renesas/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
===================================================================
--- renesas.orig/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
+++ renesas/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
@@ -59,6 +59,8 @@
phy0: ethernet-phy at 0 {
rxc-skew-ps = <1500>;
reg = <0>;
+ interrupt-parent = <&gpio1>;
+ interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
};
};
^ permalink raw reply
* [PATCH 3/3] arm64: dts: renesas: v3hsk: specify GEther PHY IRQ
From: Sergei Shtylyov @ 2018-06-01 20:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <4acc208e-c593-1e8a-00ca-fc9a5574074e@cogentembedded.com>
Specify GEther PHY IRQ in the V3H Starter Kit board's device tree, now
that we have the GPIO support (previously phylib had to resort to polling).
Based on the original (and large) patch by Vladimir Barinov.
Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
---
arch/arm64/boot/dts/renesas/r8a77980-v3hsk.dts | 2 ++
1 file changed, 2 insertions(+)
Index: renesas/arch/arm64/boot/dts/renesas/r8a77980-v3hsk.dts
===================================================================
--- renesas.orig/arch/arm64/boot/dts/renesas/r8a77980-v3hsk.dts
+++ renesas/arch/arm64/boot/dts/renesas/r8a77980-v3hsk.dts
@@ -48,6 +48,8 @@
phy0: ethernet-phy at 0 {
reg = <0>;
+ interrupt-parent = <&gpio4>;
+ interrupts = <23 IRQ_TYPE_LEVEL_LOW>;
};
};
^ permalink raw reply
* [PATCH V4] scsi: hpsa: drop shutdown callback
From: Don Brace @ 2018-06-01 20:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527860768-11367-1-git-send-email-okaya@codeaurora.org>
> -----Original Message-----
> From: Sinan Kaya [mailto:okaya at codeaurora.org]
> Sent: Friday, June 01, 2018 8:46 AM
> To: linux-pci at vger.kernel.org; ryan at finnie.org; timur at codeaurora.org
> Cc: linux-arm-msm at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> Sinan Kaya <okaya@codeaurora.org>; stable at vger.kernel.org; Don Brace
> <don.brace@microsemi.com>; James E.J. Bottomley
> <jejb@linux.vnet.ibm.com>; Martin K. Petersen <martin.petersen@oracle.com>;
> esc.storagedev <esc.storagedev@microsemi.com>; open list:HEWLETT-
> PACKARD SMART ARRAY RAID DRIVER (hpsa) <linux-scsi@vger.kernel.org>; open
> list <linux-kernel@vger.kernel.org>
> Subject: [PATCH V4] scsi: hpsa: drop shutdown callback
>
> EXTERNAL EMAIL
>
>
> 'Commit cc27b735ad3a ("PCI/portdrv: Turn off PCIe services during
> shutdown")' has been added to kernel to shutdown pending PCIe port
> service interrupts during reboot so that a newly started kexec kernel
> wouldn't observe pending interrupts.
>
> pcie_port_device_remove() is disabling the root port and switches by
> calling pci_disable_device() after all PCIe service drivers are shutdown.
>
> This has been found to cause crashes on HP DL360 Gen9 machines during
> reboot due to hpsa driver not clearing the bus master bit during the
> shutdown procedure by calling pci_disable_device().
>
> Disable device as part of the shutdown sequence.
>
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
Looks good. Thanks for your patch
Thanks for changing the patch name also.
Tested-by: Don Brace <don.brace@microsemi.com>
Acked-by: Don Brace <don.brace@microsemi.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=199779
> Fixes: cc27b735ad3a ("PCI/portdrv: Turn off PCIe services during shutdown")
> Cc: stable at vger.kernel.org
> Reported-by: Ryan Finnie <ryan@finnie.org>
> ---
> drivers/scsi/hpsa.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index 3a9eca1..b92f86a 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -8869,7 +8869,7 @@ static void hpsa_disable_rld_caching(struct ctlr_info
> *h)
> kfree(options);
> }
>
> -static void hpsa_shutdown(struct pci_dev *pdev)
> +static void __hpsa_shutdown(struct pci_dev *pdev)
> {
> struct ctlr_info *h;
>
> @@ -8884,6 +8884,12 @@ static void hpsa_shutdown(struct pci_dev *pdev)
> hpsa_disable_interrupt_mode(h); /* pci_init 2 */
> }
>
> +static void hpsa_shutdown(struct pci_dev *pdev)
> +{
> + __hpsa_shutdown(pdev);
> + pci_disable_device(pdev);
> +}
> +
> static void hpsa_free_device_info(struct ctlr_info *h)
> {
> int i;
> @@ -8927,7 +8933,7 @@ static void hpsa_remove_one(struct pci_dev *pdev)
> scsi_remove_host(h->scsi_host); /* init_one 8 */
> /* includes hpsa_free_irqs - init_one 4 */
> /* includes hpsa_disable_interrupt_mode - pci_init 2 */
> - hpsa_shutdown(pdev);
> + __hpsa_shutdown(pdev);
>
> hpsa_free_device_info(h); /* scan */
>
> --
> 2.7.4
^ permalink raw reply
* [RFC PATCH 8/8] dts: juno: Update coresight bindings for hw port
From: Mathieu Poirier @ 2018-06-01 20:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527858967-16047-9-git-send-email-suzuki.poulose@arm.com>
On Fri, Jun 01, 2018 at 02:16:07PM +0100, Suzuki K Poulose wrote:
> Switch to updated coresight bindings for hw ports.
>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Reviewed-and-tested-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
> arch/arm64/boot/dts/arm/juno-base.dtsi | 82 +++++++++++++++++++++++++---------
> arch/arm64/boot/dts/arm/juno.dts | 5 ++-
> 2 files changed, 63 insertions(+), 24 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/arm/juno-base.dtsi b/arch/arm64/boot/dts/arm/juno-base.dtsi
> index eb749c5..33b41ba 100644
> --- a/arch/arm64/boot/dts/arm/juno-base.dtsi
> +++ b/arch/arm64/boot/dts/arm/juno-base.dtsi
> @@ -122,15 +122,18 @@
> port at 0 {
> reg = <0>;
> etf0_in_port: endpoint {
> - slave-mode;
> + direction = <0>;
> remote-endpoint = <&main_funnel_out_port>;
> + coresight,hwid = <0>;
> };
> };
>
> /* output port */
> port at 1 {
> - reg = <0>;
> + reg = <1>;
> etf0_out_port: endpoint {
> + coresight,hwid = <0>;
> + direction = <1>;
> };
> };
> };
> @@ -145,8 +148,9 @@
> power-domains = <&scpi_devpd 0>;
> port {
> tpiu_in_port: endpoint {
> - slave-mode;
> + direction = <0>;
> remote-endpoint = <&replicator_out_port0>;
> + coresight,hwid = <0>;
> };
> };
> };
> @@ -168,23 +172,27 @@
> reg = <0>;
> main_funnel_out_port: endpoint {
> remote-endpoint = <&etf0_in_port>;
> + coresight,hwid = <0>;
> + direction = <1>;
> };
> };
>
> /* input ports */
> port at 1 {
> - reg = <0>;
> + reg = <1>;
> main_funnel_in_port0: endpoint {
> - slave-mode;
> + direction = <0>;
> remote-endpoint = <&cluster0_funnel_out_port>;
> + coresight,hwid = <0>;
> };
> };
>
> port at 2 {
> - reg = <1>;
> + reg = <2>;
> main_funnel_in_port1: endpoint {
> - slave-mode;
> + direction = <0>;
> remote-endpoint = <&cluster1_funnel_out_port>;
> + coresight,hwid = <1>;
> };
> };
> };
> @@ -200,8 +208,9 @@
> power-domains = <&scpi_devpd 0>;
> port {
> etr_in_port: endpoint {
> - slave-mode;
> + direction = <0>;
> remote-endpoint = <&replicator_out_port1>;
> + coresight,hwid = <0>;
> };
> };
> };
> @@ -217,6 +226,8 @@
> power-domains = <&scpi_devpd 0>;
> port {
> stm_out_port: endpoint {
> + coresight,hwid = <0>;
> + direction = <1>;
> };
> };
> };
> @@ -240,6 +251,8 @@
> port {
> cluster0_etm0_out_port: endpoint {
> remote-endpoint = <&cluster0_funnel_in_port0>;
> + coresight,hwid = <0>;
> + direction = <1>;
> };
> };
> };
> @@ -259,22 +272,26 @@
> reg = <0>;
> cluster0_funnel_out_port: endpoint {
> remote-endpoint = <&main_funnel_in_port0>;
> + coresight,hwid = <0>;
> + direction = <1>;
> };
> };
>
> port at 1 {
> - reg = <0>;
> + reg = <1>;
> cluster0_funnel_in_port0: endpoint {
> - slave-mode;
> + direction = <0>;
> remote-endpoint = <&cluster0_etm0_out_port>;
> + coresight,hwid = <0>;
> };
> };
>
> port at 2 {
> - reg = <1>;
> + reg = <2>;
> cluster0_funnel_in_port1: endpoint {
> - slave-mode;
> + direction = <0>;
> remote-endpoint = <&cluster0_etm1_out_port>;
> + coresight,hwid = <1>;
> };
> };
> };
> @@ -299,6 +316,8 @@
> port {
> cluster0_etm1_out_port: endpoint {
> remote-endpoint = <&cluster0_funnel_in_port1>;
> + coresight,hwid = <0>;
> + direction = <1>;
> };
> };
> };
> @@ -322,6 +341,8 @@
> port {
> cluster1_etm0_out_port: endpoint {
> remote-endpoint = <&cluster1_funnel_in_port0>;
> + coresight,hwid = <0>;
> + direction = <1>;
> };
> };
> };
> @@ -341,36 +362,42 @@
> reg = <0>;
> cluster1_funnel_out_port: endpoint {
> remote-endpoint = <&main_funnel_in_port1>;
> + coresight,hwid = <0>;
> + direction = <1>;
> };
> };
>
> port at 1 {
> - reg = <0>;
> + reg = <1>;
> cluster1_funnel_in_port0: endpoint {
> - slave-mode;
> + direction = <0>;
> remote-endpoint = <&cluster1_etm0_out_port>;
> + coresight,hwid = <0>;
> };
> };
>
> port at 2 {
> - reg = <1>;
> + reg = <2>;
> cluster1_funnel_in_port1: endpoint {
> - slave-mode;
> + direction = <0>;
> remote-endpoint = <&cluster1_etm1_out_port>;
> + coresight,hwid = <1>;
> };
> };
> port at 3 {
> - reg = <2>;
> + reg = <3>;
> cluster1_funnel_in_port2: endpoint {
> - slave-mode;
> + direction = <0>;
> remote-endpoint = <&cluster1_etm2_out_port>;
> + coresight,hwid = <2>;
> };
> };
> port at 4 {
> - reg = <3>;
> + reg = <4>;
> cluster1_funnel_in_port3: endpoint {
> - slave-mode;
> + direction = <0>;
> remote-endpoint = <&cluster1_etm3_out_port>;
> + coresight,hwid = <3>;
> };
> };
> };
> @@ -395,6 +422,8 @@
> port {
> cluster1_etm1_out_port: endpoint {
> remote-endpoint = <&cluster1_funnel_in_port1>;
> + coresight,hwid = <0>;
> + direction = <1>;
> };
> };
> };
> @@ -418,6 +447,8 @@
> port {
> cluster1_etm2_out_port: endpoint {
> remote-endpoint = <&cluster1_funnel_in_port2>;
> + coresight,hwid = <0>;
> + direction = <1>;
> };
> };
> };
> @@ -441,6 +472,8 @@
> port {
> cluster1_etm3_out_port: endpoint {
> remote-endpoint = <&cluster1_funnel_in_port3>;
> + coresight,hwid = <0>;
> + direction = <1>;
> };
> };
> };
> @@ -462,6 +495,8 @@
> reg = <0>;
> replicator_out_port0: endpoint {
> remote-endpoint = <&tpiu_in_port>;
> + coresight,hwid = <0>;
> + direction = <1>;
> };
> };
>
> @@ -469,14 +504,17 @@
> reg = <1>;
> replicator_out_port1: endpoint {
> remote-endpoint = <&etr_in_port>;
> + coresight,hwid = <1>;
> + direction = <1>;
> };
> };
>
> /* replicator input port */
> port at 2 {
> - reg = <0>;
> + reg = <2>;
> replicator_in_port0: endpoint {
> - slave-mode;
> + direction = <0>;
> + coresight,hwid = <0>;
> };
> };
> };
> diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
> index c9236c4..27b8036 100644
> --- a/arch/arm64/boot/dts/arm/juno.dts
> +++ b/arch/arm64/boot/dts/arm/juno.dts
> @@ -260,10 +260,11 @@
> &main_funnel {
> ports {
> port at 3 {
> - reg = <2>;
> + reg = <3>;
> main_funnel_in_port2: endpoint {
> - slave-mode;
> + direction = <0>;
> remote-endpoint = <&stm_out_port>;
> + coresight,hwid = <2>;
> };
> };
> };
> --
> 2.7.4
>
^ permalink raw reply
* [RFC PATCH 0/8] coresight: Update device tree bindings
From: Mathieu Poirier @ 2018-06-01 21:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527858967-16047-1-git-send-email-suzuki.poulose@arm.com>
On Fri, Jun 01, 2018 at 02:15:59PM +0100, Suzuki K Poulose wrote:
> Coresight uses DT graph bindings to describe the connections of the
> components. However we have some undocumented usage of the bindings
> to describe some of the properties of the connections.
>
> The coresight driver needs to know the hardware ports invovled
> in the connection and the direction of data flow to effectively
> manage the trace sessions. So far we have relied on the "port"
> address (as described by the generic graph bindings) to represent
> the hardware port of the component for a connection.
>
> The hardware uses separate numbering scheme for input and output
> ports, which implies, we could have two different (input and output)
> ports with the same port number. This could create problems in the
> graph bindings where the label of the port wouldn't match the address.
>
> e.g, with the existing bindings we get :
>
> port at 0{ // Output port 0
> reg = <0>;
> ...
> };
>
> port at 1{
> reg = <0>; // Input port 0
> endpoint {
> slave-mode;
> ...
> };
> };
>
> With the new enforcement in the DT rules, mismatches in label and address
> are not allowed (as see in the case for port at 1). So, we need a new mechanism
> to describe the hardware port number reliably.
>
> Also, we relied on an undocumented "slave-mode" property (see the above
> example) to indicate if the port is an input port. Let us formalise and
> switch to a new property to describe the direction of data flow.
>
> There were three options considered for the hardware port number scheme:
>
> 1) Use natural ordering in the DT to infer the hardware port number.
> i.e, Mandate that the all ports are listed in the DT and in the ascending
> order for each class (input and output respectively).
> Pros :
> - We don't need new properties and if the existing DTS list them in
> order (which most of them do), they work out of the box.
> Cons :
> - We must list all the ports even if the system cannot/shouldn't use
> it.
> - It is prone to human errors (if the order is not kept).
>
> 2) Use an explicit property to list both the direction and the hw port
> number and direction. Define "coresight,hwid" as 2 member array of u32,
> where the members are port number and the direction respectively.
> e.g
>
> port at 0{
> reg = <0>;
> endpoint {
> coresight,hwid = <0 1>; // Port # 0, Output
> }
> };
>
> port at 1{
> reg = <1>;
> endpoint {
> coresight,hwid = <0 0>; // Port # 0, Input
> };
> };
>
> Pros:
> - The bindings are formal but not so reader friendly and could potentially
> lead to human errors.
> Cons:
> - Backward compatiblity is lost.
> 3) Use explicit properties (implemented in the series) for the hardware
> port id and direction. We define a new property "coresight,hwid" for
> each endpoint in coresight devices to specify the hardware port number
> explicitly. Also use a separate property "direction" to specify the
> direction of the data flow.
>
> e.g,
>
> port at 0{
> reg = <0>;
> endpoint {
> direction = <1>; // Output
> coresight,hwid = <0>; // Port # 0
> }
> };
>
> port at 1{
> reg = <1>;
> endpoint {
> direction = <0>; // Input
> coresight,hwid = <0>; // Port # 0
> };
> };
>
> Pros:
> - The bindings are formal and reader friendly, and less prone to errors.
> Cons:
> - Backward compatibility is lost.
>
>
> This series achieves implements Option (3) listed above while still retaining
> the backward compatibility. The driver now issues a warning (once) when it
> encounters the old bindings.
> It also cleans up the platform parsing code to reduce the memory usage by
> reusing the platform description. The series also includes the
> changes for Juno platform as an example. If there are no objections
> to the approach, I could post the series, converting all the
> in-kernel DTS to the new binding.
>
> Suzuki K Poulose (8):
> dts: binding: coresight: Document graph bindings
> coresight: Fix remote endpoint parsing
> coresight: Cleanup platform description data
> coresight: platform: Cleanup coresight connection handling
> coresight: Handle errors in finding input/output ports
> dts: coresight: Clean up the device tree graph bindings
> dts: coresight: Define new bindings for direction of data flow
> dts: juno: Update coresight bindings for hw port
>
> .../devicetree/bindings/arm/coresight.txt | 52 ++++++++--
> arch/arm64/boot/dts/arm/juno-base.dtsi | 82 +++++++++++----
> arch/arm64/boot/dts/arm/juno.dts | 5 +-
> drivers/hwtracing/coresight/coresight.c | 28 ++----
> drivers/hwtracing/coresight/of_coresight.c | 111 ++++++++++++---------
> include/linux/coresight.h | 11 +-
> 6 files changed, 181 insertions(+), 108 deletions(-)
Aside from the comments I've already posted I'm pretty much good with this set.
Please rebase the next revision on my "next" branch and run checkpatch.pl on the
set. Patch 6/8 and 7/8 are generating warnings.
Thanks,
Mathieu
>
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH v1 2/2] arm/arm64: KVM: Add KVM_GET/SET_VCPU_EVENTS
From: Eric Northup @ 2018-06-01 21:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527772139-19665-3-git-send-email-gengdongjiu@huawei.com>
On Wed, May 30, 2018 at 10:04 PM Dongjiu Geng <gengdongjiu@huawei.com> wrote:
>
> For the migrating VMs, user space may need to know the exception
> state. For example, in the machine A, KVM make an SError pending,
> when migrate to B, KVM also needs to pend an SError.
>
> This new IOCTL exports user-invisible states related to SError.
> Together with appropriate user space changes, user space can get/set
> the SError exception state to do migrate/snapshot/suspend.
>
> Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
> --
> this series patch is separated from https://www.spinics.net/lists/kvm/msg168917.html
> change since V12:
> 1. change (vcpu->arch.hcr_el2 & HCR_VSE) to !!(vcpu->arch.hcr_el2 & HCR_VSE) in kvm_arm_vcpu_get_events()
>
> Change since V11:
> Address James's comments, thanks James
> 1. Align the struct of kvm_vcpu_events to 64 bytes
> 2. Avoid exposing the stale ESR value in the kvm_arm_vcpu_get_events()
> 3. Change variables 'injected' name to 'serror_pending' in the kvm_arm_vcpu_set_events()
> 4. Change to sizeof(events) from sizeof(struct kvm_vcpu_events) in kvm_arch_vcpu_ioctl()
>
> Change since V10:
> Address James's comments, thanks James
> 1. Merge the helper function with the user.
> 2. Move the ISS_MASK into pend_guest_serror() to clear top bits
> 3. Make kvm_vcpu_events struct align to 4 bytes
> 4. Add something check in the kvm_arm_vcpu_set_events()
> 5. Check kvm_arm_vcpu_get/set_events()'s return value.
> 6. Initialise kvm_vcpu_events to 0 so that padding transferred to user-space doesn't
> contain kernel stack.
> ---
> Documentation/virtual/kvm/api.txt | 31 ++++++++++++++++++++++++++++---
> arch/arm/include/asm/kvm_host.h | 6 ++++++
> arch/arm/kvm/guest.c | 12 ++++++++++++
> arch/arm64/include/asm/kvm_emulate.h | 5 +++++
> arch/arm64/include/asm/kvm_host.h | 7 +++++++
> arch/arm64/include/uapi/asm/kvm.h | 13 +++++++++++++
> arch/arm64/kvm/guest.c | 36 ++++++++++++++++++++++++++++++++++++
> arch/arm64/kvm/inject_fault.c | 7 ++++++-
> arch/arm64/kvm/reset.c | 1 +
> virt/kvm/arm/arm.c | 21 +++++++++++++++++++++
> 10 files changed, 135 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index fdac969..8896737 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -835,11 +835,13 @@ struct kvm_clock_data {
>
> Capability: KVM_CAP_VCPU_EVENTS
> Extended by: KVM_CAP_INTR_SHADOW
> -Architectures: x86
> +Architectures: x86, arm, arm64
> Type: vm ioctl
> Parameters: struct kvm_vcpu_event (out)
> Returns: 0 on success, -1 on error
>
> +X86:
> +
> Gets currently pending exceptions, interrupts, and NMIs as well as related
> states of the vcpu.
>
> @@ -881,15 +883,32 @@ Only two fields are defined in the flags field:
> - KVM_VCPUEVENT_VALID_SMM may be set in the flags field to signal that
> smi contains a valid state.
>
> +ARM, ARM64:
> +
> +Gets currently pending SError exceptions as well as related states of the vcpu.
> +
> +struct kvm_vcpu_events {
> + struct {
> + __u8 serror_pending;
> + __u8 serror_has_esr;
> + /* Align it to 8 bytes */
> + __u8 pad[6];
> + __u64 serror_esr;
> + } exception;
> + __u32 reserved[12];
>
> +};
> +
> 4.32 KVM_SET_VCPU_EVENTS
>
> -Capability: KVM_CAP_VCPU_EVENTS
> +Capebility: KVM_CAP_VCPU_EVENTS
> Extended by: KVM_CAP_INTR_SHADOW
> -Architectures: x86
> +Architectures: x86, arm, arm64
> Type: vm ioctl
> Parameters: struct kvm_vcpu_event (in)
> Returns: 0 on success, -1 on error
>
> +X86:
> +
> Set pending exceptions, interrupts, and NMIs as well as related states of the
> vcpu.
>
> @@ -910,6 +929,12 @@ shall be written into the VCPU.
>
> KVM_VCPUEVENT_VALID_SMM can only be set if KVM_CAP_X86_SMM is available.
>
> +ARM, ARM64:
> +
> +Set pending SError exceptions as well as related states of the vcpu.
> +
> +See KVM_GET_VCPU_EVENTS for the data structure.
> +
>
> 4.33 KVM_GET_DEBUGREGS
>
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index c7c28c8..39f9901 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -213,6 +213,12 @@ unsigned long kvm_arm_num_regs(struct kvm_vcpu *vcpu);
> int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *indices);
> int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
> int kvm_arm_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
> +int kvm_arm_vcpu_get_events(struct kvm_vcpu *vcpu,
> + struct kvm_vcpu_events *events);
> +
> +int kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
> + struct kvm_vcpu_events *events);
> +
> unsigned long kvm_call_hyp(void *hypfn, ...);
> void force_vm_exit(const cpumask_t *mask);
>
> diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c
> index a18f33e..c685f0e 100644
> --- a/arch/arm/kvm/guest.c
> +++ b/arch/arm/kvm/guest.c
> @@ -261,6 +261,18 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
> return -EINVAL;
> }
>
> +int kvm_arm_vcpu_get_events(struct kvm_vcpu *vcpu,
> + struct kvm_vcpu_events *events)
> +{
> + return -EINVAL;
> +}
> +
> +int kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
> + struct kvm_vcpu_events *events)
> +{
> + return -EINVAL;
> +}
> +
> int __attribute_const__ kvm_target_cpu(void)
> {
> switch (read_cpuid_part()) {
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index 1dab3a9..18f61ff 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -81,6 +81,11 @@ static inline unsigned long *vcpu_hcr(struct kvm_vcpu *vcpu)
> return (unsigned long *)&vcpu->arch.hcr_el2;
> }
>
> +static inline unsigned long vcpu_get_vsesr(struct kvm_vcpu *vcpu)
> +{
> + return vcpu->arch.vsesr_el2;
> +}
> +
> static inline void vcpu_set_vsesr(struct kvm_vcpu *vcpu, u64 vsesr)
> {
> vcpu->arch.vsesr_el2 = vsesr;
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 469de8a..357304a 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -335,6 +335,11 @@ unsigned long kvm_arm_num_regs(struct kvm_vcpu *vcpu);
> int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *indices);
> int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
> int kvm_arm_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
> +int kvm_arm_vcpu_get_events(struct kvm_vcpu *vcpu,
> + struct kvm_vcpu_events *events);
> +
> +int kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
> + struct kvm_vcpu_events *events);
>
> #define KVM_ARCH_WANT_MMU_NOTIFIER
> int kvm_unmap_hva(struct kvm *kvm, unsigned long hva);
> @@ -363,6 +368,8 @@ void handle_exit_early(struct kvm_vcpu *vcpu, struct kvm_run *run,
> int kvm_perf_init(void);
> int kvm_perf_teardown(void);
>
> +void kvm_set_sei_esr(struct kvm_vcpu *vcpu, u64 syndrome);
> +
> struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
>
> void __kvm_set_tpidr_el2(u64 tpidr_el2);
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 04b3256..df4faee 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -39,6 +39,7 @@
> #define __KVM_HAVE_GUEST_DEBUG
> #define __KVM_HAVE_IRQ_LINE
> #define __KVM_HAVE_READONLY_MEM
> +#define __KVM_HAVE_VCPU_EVENTS
>
> #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
>
> @@ -153,6 +154,18 @@ struct kvm_sync_regs {
> struct kvm_arch_memory_slot {
> };
>
> +/* for KVM_GET/SET_VCPU_EVENTS */
> +struct kvm_vcpu_events {
> + struct {
> + __u8 serror_pending;
> + __u8 serror_has_esr;
> + /* Align it to 8 bytes */
> + __u8 pad[6];
> + __u64 serror_esr;
> + } exception;
> + __u32 reserved[12];
It will be easier to re-purpose this in the future if the field is
reserved and is checked that it must be zero. SET_VCPU_EVENTS would
return EINVAL if reserved fields get used until a later meaning is
defined for them.
> +};
> +
> /* If you need to interpret the index values, here is the key: */
> #define KVM_REG_ARM_COPROC_MASK 0x000000000FFF0000
> #define KVM_REG_ARM_COPROC_SHIFT 16
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index 56a0260..71d3841 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -289,6 +289,42 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
> return -EINVAL;
> }
>
> +int kvm_arm_vcpu_get_events(struct kvm_vcpu *vcpu,
> + struct kvm_vcpu_events *events)
> +{
> + events->exception.serror_pending = !!(vcpu->arch.hcr_el2 & HCR_VSE);
> + events->exception.serror_has_esr =
> + cpus_have_const_cap(ARM64_HAS_RAS_EXTN) &&
> + (!!vcpu_get_vsesr(vcpu));
> +
> + if (events->exception.serror_pending &&
> + events->exception.serror_has_esr)
> + events->exception.serror_esr = vcpu_get_vsesr(vcpu);
> + else
> + events->exception.serror_esr = 0;
> +
> + return 0;
> +}
> +
> +int kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
> + struct kvm_vcpu_events *events)
> +{
> + bool serror_pending = events->exception.serror_pending;
> + bool has_esr = events->exception.serror_has_esr;
> +
> + if (serror_pending && has_esr) {
> + if (!cpus_have_const_cap(ARM64_HAS_RAS_EXTN))
> + return -EINVAL;
> +
> + kvm_set_sei_esr(vcpu, events->exception.serror_esr);
> +
> + } else if (serror_pending) {
> + kvm_inject_vabt(vcpu);
> + }
> +
> + return 0;
> +}
> +
> int __attribute_const__ kvm_target_cpu(void)
> {
> unsigned long implementor = read_cpuid_implementor();
> diff --git a/arch/arm64/kvm/inject_fault.c b/arch/arm64/kvm/inject_fault.c
> index d8e7165..9e0ca56 100644
> --- a/arch/arm64/kvm/inject_fault.c
> +++ b/arch/arm64/kvm/inject_fault.c
> @@ -166,7 +166,7 @@ void kvm_inject_undefined(struct kvm_vcpu *vcpu)
>
> static void pend_guest_serror(struct kvm_vcpu *vcpu, u64 esr)
> {
> - vcpu_set_vsesr(vcpu, esr);
> + vcpu_set_vsesr(vcpu, esr & ESR_ELx_ISS_MASK);
> *vcpu_hcr(vcpu) |= HCR_VSE;
> }
>
> @@ -186,3 +186,8 @@ void kvm_inject_vabt(struct kvm_vcpu *vcpu)
> {
> pend_guest_serror(vcpu, ESR_ELx_ISV);
> }
> +
> +void kvm_set_sei_esr(struct kvm_vcpu *vcpu, u64 syndrome)
> +{
> + pend_guest_serror(vcpu, syndrome);
> +}
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index 38c8a64..20e919a 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -82,6 +82,7 @@ int kvm_arch_dev_ioctl_check_extension(struct kvm *kvm, long ext)
> break;
> case KVM_CAP_SET_GUEST_DEBUG:
> case KVM_CAP_VCPU_ATTRIBUTES:
> + case KVM_CAP_VCPU_EVENTS:
> r = 1;
> break;
> default:
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index a4c1b76..8b43968 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -1107,6 +1107,27 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> r = kvm_arm_vcpu_has_attr(vcpu, &attr);
> break;
> }
> + case KVM_GET_VCPU_EVENTS: {
> + struct kvm_vcpu_events events;
> +
> + memset(&events, 0, sizeof(events));
> + if (kvm_arm_vcpu_get_events(vcpu, &events))
> + return -EINVAL;
> +
> + if (copy_to_user(argp, &events, sizeof(events)))
> + return -EFAULT;
> +
> + return 0;
> + }
> + case KVM_SET_VCPU_EVENTS: {
> + struct kvm_vcpu_events events;
> +
> + if (copy_from_user(&events, argp,
> + sizeof(struct kvm_vcpu_events)))
> + return -EFAULT;
> +
> + return kvm_arm_vcpu_set_events(vcpu, &events);
> + }
> default:
> r = -EINVAL;
> }
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH v4 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings
From: David Collins @ 2018-06-01 21:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180531114816.GC13319@sirena.org.uk>
Hello Mark,
On 05/31/2018 04:48 AM, Mark Brown wrote:
> On Wed, May 30, 2018 at 04:39:10PM -0700, David Collins wrote:
>> The DRMS modes to use and max allowed current per mode need to be
>> specified at the board level in device tree instead of hard-coded per
>> regulator type in the driver. There are at least two use cases driving
>> this need: LDOs shared between RPMh client processors and SMPSes requiring
>> PWM mode in certain circumstances.
>
> Is there really no way to improve the RPM firmware?
This aggregation takes place in a discrete hardware block, not in a
general purpose processor. Theoretically, future chips could have
redesigned VRM hardware; however, there is no plan to make such a change.
>> Consider the case of a regulator with physical 10 mA LPM max current. Say
>> that modem and application processors each have a load on the regulator
>> that draws 9 mA. If they each respect the 10 mA limit, then they'd each
>> vote for LPM. The VRM block in RPMh hardware will aggregate these requests
>
> This is, of course, why the regulator API aggregates this stuff based on
> the current not based on having every individual user select a mode.
>
>> together using a max function which will result in the regulator being set
>> to LPM, even though the total load is 18 mA (which would require high
>> power mode (HPM)). To get around this corner case, a LPM max current of 1
>> uA can be used for all LDO supplies that have non-application processor
>> consumers. Thus, any non-zero regulator_set_load() current request will
>> result in setting the regulator to HPM (which is always safe).
>
> That's obviously just never going to work well, the best you can do here
> is just pretend that the other components are always operating at full
> power (which I assume all the other components are doing too...) or not
> try to use load based mode switching in the first place.
I will remove the DT-based mode and max load current mapping support. In
its place, I'll implement hard coded LPM current limits for LDOs of 10 mA
or 30 mA (depending upon subtype) like is done in other regulator drivers.
If we actually encounter an issue caused by the APPS processor and another
RPMh client both voting for LPM when HPM is needed for the combination,
then we can disable load-based mode control for the affected regulator in
DT and configure its initial mode as HPM.
>> The second situation that needs board-level DRMS mode and current limit
>> specification is SMPS regulator AUTO mode to PWM (HPM) mode switching.
>> SMPS regulators should theoretically always be able to operate in AUTO
>> mode as it switches automatically between PWM mode (which can provide the
>> maximum current) and PFM mode (which supports lower current but has higher
>> efficiency). However, there may be board/system issues that require
>> switching to PWM mode for certain use cases as it has better load
>> regulation (i.e. no PFM ripple for lower loads) and supports more
>> aggressive load current steps (i.e. greater A/ns).
>
>> If a Linux consumer requires the ability to force a given SMPS regulator
>> from AUTO mode into PWM mode and that SMPS is shared by other Linux
>> consumers (which may be the case, but at least must be guaranteed to work
>> architecturally), then regulator_set_load() is the only option since it
>> provides aggregation, where as regulator_set_mode() does not.
>
> That's obviously broken though, what you're describing is just clearly
> nothing to do with load so trying to configure it using load is just
> going to lead to problems later on. Honestly it sounds like you just
> want to force the regulator into forced PWM mode all the time, otherwise
> you need to look into implementing support for describing the thing
> you're actually trying to do and add a mechanism for per consumer mode
> configuration.
>
> This has been a frequent pattern with these RPM drivers, trying to find
> some way to shoehorn something that happens to work right now into the
> code. That's going to make things fragile and hard to maintain, we need
> code that does the thing it's saying it does so that it's easier to
> understand and work with - getting things running isn't enough, they
> need to be clear.
I agree that using regulator_set_load() to handle SMPS AUTO mode to PWM
mode switching is hacky. Since there is no natural way to pick SMPS modes
based on load current, I will remove the functionality completely. In its
place, we can configure the SMPSes to have an initial mode of AUTO. If a
use case requiring PWM mode arises, then the consumer driver responsible
for it can call regulator_set_mode() to switch between AUTO and PWM modes
explicitly.
Since regulator_set_mode() does not support aggregation currently, this
technique would work only if there is exactly one consumer per regulator
that needs explicit mode control. Should we hit a situation that needs
multiple consumers to have such mode control, then we could simply default
the SMPS to PWM mode.
I'll also start looking into per-consumer mode configuration and
regulator_set_mode() aggregation within the regulator framework. I think
that we should be able to function without it for now; however, it would
be good to have going forward.
Take care,
David
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* [Query] PAGE_OFFSET on KASLR enabled ARM64 kernel
From: Bhupesh Sharma @ 2018-06-01 21:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACi5LpN=fSGhgNFOQysJWWzFarchFa1q2R_-J05uQs3wxrANRA@mail.gmail.com>
On 05/31/2018 10:21 AM, Bhupesh Sharma wrote:
> Hi Ard,
>
> Sorry I was out for most of the day yesterday. Please see my responses inline.
>
> On Mon, May 28, 2018 at 12:16 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> On 27 May 2018 at 23:03, Bhupesh Sharma <bhsharma@redhat.com> wrote:
>>> Hi ARM64 maintainers,
>>>
>>> I am confused about the PAGE_OFFSET value (or the start of the linear
>>> map) on a KASLR enabled ARM64 kernel that I am seeing on a board which
>>> supports a compatible EFI firmware (with EFI_RNG_PROTOCOL support).
>>>
>>> 1. 'arch/arm64/include/asm/memory.h' defines PAGE_OFFSET as:
>>>
>>> /*
>>> * PAGE_OFFSET - the virtual address of the start of the linear map (top
>>> * (VA_BITS - 1))
>>> */
>>> #define PAGE_OFFSET (UL(0xffffffffffffffff) - \
>>> (UL(1) << (VA_BITS - 1)) + 1)
>>>
>>> So for example on a platform with VA_BITS=48, we have:
>>> PAGE_OFFSET = 0xffff800000000000
>>>
>>> 2. However, for the KASLR case, we set the 'memstart_offset_seed ' to
>>> use the 16-bits of the 'kaslr-seed' to randomize the linear region in
>>> 'arch/arm64/kernel/kaslr.c' :
>>>
>>> u64 __init kaslr_early_init(u64 dt_phys)
>>> {
>>> <snip..>
>>> /* use the top 16 bits to randomize the linear region */
>>> memstart_offset_seed = seed >> 48;
>>> <snip..>
>>> }
>>>
>>> 3. Now, we use the 'memstart_offset_seed' value to randomize the
>>> 'memstart_addr' value in 'arch/arm64/mm/init.c':
>>>
>>> void __init arm64_memblock_init(void)
>>> {
>>> <snip..>
>>>
>>> if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {
>>> extern u16 memstart_offset_seed;
>>> u64 range = linear_region_size -
>>> (memblock_end_of_DRAM() - memblock_start_of_DRAM());
>>>
>>> /*
>>> * If the size of the linear region exceeds, by a sufficient
>>> * margin, the size of the region that the available physical
>>> * memory spans, randomize the linear region as well.
>>> */
>>> if (memstart_offset_seed > 0 && range >= ARM64_MEMSTART_ALIGN) {
>>> range = range / ARM64_MEMSTART_ALIGN + 1;
>>> memstart_addr -= ARM64_MEMSTART_ALIGN *
>>> ((range * memstart_offset_seed) >> 16);
>>> }
>>> }
>>> <snip..>
>>> }
>>>
>>> 4. Since 'memstart_addr' indicates the start of physical RAM, we
>>> randomize the same on basis of 'memstart_offset_seed' value above.
>>> Also the 'memstart_addr' value is available in '/proc/kallsyms' and
>>> hence can be accessed by user-space applications to read the
>>> 'memstart_addr' value.
>>>
>>> 5. Now since the PAGE_OFFSET value is also used by several user space
>>> tools (for e.g. makedumpfile tool uses the same to determine the start
>>> of linear region and hence to read PT_NOTE fields from /proc/kcore), I
>>> am not sure how to read the randomized value of the same in the KASLR
>>> enabled case.
>>>
>>> 6. Reading the code further and adding some debug prints, it seems the
>>> 'memblock_start_of_DRAM()' value is more closer to the actual start of
>>> linear region rather than 'memstart_addr' and 'PAGE_OFFSET" in case of
>>> KASLR enabled kernel:
>>>
>>> [root at qualcomm-amberwing] # dmesg | grep -i "arm64_memblock_init" -A 5
>>>
>>> [ 0.000000] inside arm64_memblock_init, memstart_addr = ffff976a00000000,
>>> linearstart_addr = ffffe89600200000, memblock_start_of_DRAM = ffffe89600200000,
>>> PHYS_OFFSET = ffff976a00000000, PAGE_OFFSET = ffff800000000000,
>>> KIMAGE_VADDR = ffff000008000000, kimage_vaddr = ffff20c2d7800000
>>>
>>> [root at qualcomm-amberwing] # dmesg | grep -i "Virtual kernel memory layout" -A 15
>>> [ 0.000000] Virtual kernel memory layout:
>>> [ 0.000000] modules : 0xffff000000000000 - 0xffff000008000000
>>> ( 128 MB)
>>> [ 0.000000] vmalloc : 0xffff000008000000 - 0xffff7bdfffff0000
>>> (126847 GB)
>>> [ 0.000000] .text : 0xffff20c2d7880000 - 0xffff20c2d8040000
>>> ( 7936 KB)
>>> [ 0.000000] .rodata : 0xffff20c2d8040000 - 0xffff20c2d83a0000
>>> ( 3456 KB)
>>> [ 0.000000] .init : 0xffff20c2d83a0000 - 0xffff20c2d8750000
>>> ( 3776 KB)
>>> [ 0.000000] .data : 0xffff20c2d8750000 - 0xffff20c2d891b200
>>> ( 1837 KB)
>>> [ 0.000000] .bss : 0xffff20c2d891b200 - 0xffff20c2d90a5198
>>> ( 7720 KB)
>>> [ 0.000000] fixed : 0xffff7fdffe790000 - 0xffff7fdffec00000
>>> ( 4544 KB)
>>> [ 0.000000] PCI I/O : 0xffff7fdffee00000 - 0xffff7fdfffe00000
>>> ( 16 MB)
>>> [ 0.000000] vmemmap : 0xffff7fe000000000 - 0xffff800000000000
>>> ( 128 GB maximum)
>>> [ 0.000000] 0xffff7ffa25800800 - 0xffff7ffa2b800000
>>> ( 95 MB actual)
>>> [ 0.000000] memory : 0xffffe89600200000 - 0xffffe8ae00000000
>>> ( 98302 MB)
>>>
>>> As one can see above, the 'memblock_start_of_DRAM()' value of
>>> 0xffffe89600200000 represents the start of linear region:
>>>
>>> [ 0.000000] memory : 0xffffe89600200000 - 0xffffe8ae00000000
>>> ( 98302 MB)
>>>
>>> So, my question is to access the start of linear region (which was
>>> earlier determinable via PAGE_OFFSET macro), whether I should:
>>>
>>> - do some back-computation for the start of linear region from the
>>> 'memstart_addr' in user-space, or
>>> - use a new global variable in kernel which is assigned the value of
>>> memblock_start_of_DRAM()' and assign it to '/proc/kallsyms', so that
>>> it can be read by user-space tools, or
>>> - whether we should rather look at removing the PAGE_OFFSET usage from
>>> the kernel and replace it with a global variable instead which is
>>> properly updated for KASLR case as well.
>>>
>>> Kindly share your opinions on what can be a suitable solution in this case.
>>>
>>> Thanks for your help.
>>>
>>
>> Hello Bhupesh,
>>
>> Could you explain what the relevance is of PAGE_OFFSET to userland?
>> The only thing that should matter is where the actual linear mapping
>> of DRAM is, and I am not sure I understand why we care about where it
>> resides relative to the base of the linear region.
>
> Actually certain user-space tools like makedumpfile (which is used to
> generate and compress the vmcore) and crash-utility (which is used to
> debug the vmcore), rely on the PAGE_OFFSET value (which denotes the
> base of the linear map region) to determine virtual to physical
> mapping of the addresses lying in the linear region .
>
> One specific use case that I am working on at the moment is the
> makedumpfile '--mem-usage', which allows one to see the page numbers
> of current system (1st kernel) in different use (please see
> MAKEDUMPFILE(8) for more details).
>
> Using this we can know how many pages are dumpable when different
> dump_level is specified when invoking the makedumpfile.
>
> Normally, makedumpfile analyses the contents of '/proc/kcore' (while
> excluding the crashkernel range), and then calculates the page number
> of different kind per vmcoreinfo.
>
> For e.g. here is an output from my arm64 board (a non KASLR boot):
>
> TYPE PAGES EXCLUDABLE DESCRIPTION
> ----------------------------------------------------------------------
> ZERO 49524 yes Pages
> filled with zero
> NON_PRI_CACHE 15143 yes Cache
> pages without private flag
> PRI_CACHE 29147 yes Cache
> pages with private flag
> USER 3684 yes User process pages
> FREE 1450569 yes Free pages
> KERN_DATA 14243 no Dumpable kernel data
>
> page size: 65536
> Total pages on system: 1562310
> Total size on system: 102387548160 Byte
>
> This use case requires directly reading the '/proc/kcore' and the
> hence the PAGE_OFFSET value is used to determine the base address of
> the linear region, whose value is not static in case of KASLR boot.
>
> Another use-case is where the crash-utility uses the PAGE_OFFSET value
> to perform a virtual-to-physical conversion for the address lying in
> the linear region:
>
> ulong
> arm64_VTOP(ulong addr)
> {
> if (machdep->flags & NEW_VMEMMAP) {
> if (addr >= machdep->machspec->page_offset)
> return machdep->machspec->phys_offset
> + (addr - machdep->machspec->page_offset);
>
> <..snip..>
> }
>
Another confusing concept is the rounded-up value of 'memstart_addr' in
'arch/arm64/mm/init.c' when booting a non-KASLR_ kernel and when the
value of memblock_start_of_DRAM() < ARM64_MEMSTART_ALIGN:
void __init arm64_memblock_init(void)
{
<..snip..>
/*
* Select a suitable value for the base of physical memory.
*/
memstart_addr = round_down(memblock_start_of_DRAM(),
ARM64_MEMSTART_ALIGN);
<..snip..>
}
For example, let's consider a case (which I see on my qualcomm board)
where memblock_start_of_DRAM() = 0x200000 and ARM64_MEMSTART_ALIGN =
0x40000000 (I am using VA_BITS = 48 and a 64K page size), in this case
memstart_addr is calculated at 0, as the round_down results in a value of 0.
This is in contrast with the definition of the 'memblock_start_of_DRAM':
/* lowest address */
phys_addr_t __init_memblock memblock_start_of_DRAM(void)
{
return memblock.memory.regions[0].base;
}
As indicated by logs below, the first memblock region base starts from
0x200000 rather than the 'memstart_addr' value (which is 0)
# dmesg | grep -i "Processing" -A 5
[ 0.000000] efi: Processing EFI memory map:
[ 0.000000] efi: 0x000000200000-0x00000021ffff [Runtime Data
|RUN| | | | | | | |WB|WT|WC|UC]
[ 0.000000] efi: 0x000000400000-0x0000005fffff [ACPI Memory NVS
| | | | | | | | | | | |UC]
# head -1 /proc/iomem
00200000-0021ffff : reserved
Since we define 'PHYS_OFFSET' as the physical address of the start of
memory it would be 0 in this case:
/* PHYS_OFFSET - the physical address of the start of memory. */
#define PHYS_OFFSET ({ VM_BUG_ON(memstart_addr & 1); memstart_addr; })
On the other hand, the first memblock starts from 0x200000, so my
question is whether we should update the user-space tools which use the
memblocks listed in '/proc/iomem' to obtain the value of PHY_OFFSET (by
reading the base of the 1st memblock) and read the value of
'memstart_addr' somehow in user-space to get the PHY_OFFSET, or should
the change be done at the kernel end to calculate 'memstart_addr' as:
/*
* Select a suitable value for the base of physical memory.
*/
memstart_addr = round_down(memblock_start_of_DRAM(),
ARM64_MEMSTART_ALIGN);
if (memstart_addr)
memstart_addr = memblock_start_of_DRAM();
Please share your views.
Thanks,
Bhupesh
^ permalink raw reply
* [PATCH] arm64: alternative:flush cache with unpatched code
From: Rohit Khanna @ 2018-06-01 21:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527882765869.38555@nvidia.com>
1 more comment inline.
Thanks
Rohit
________________________________________
From: linux-arm-kernel <linux-arm-kernel-bounces@lists.infradead.org> on behalf of Rohit Khanna <rokhanna@nvidia.com>
Sent: Friday, June 1, 2018 12:52 PM
To: Mark Rutland
Cc: Suzuki.Poulose at arm.com; catalin.marinas at arm.com; will.deacon at arm.com; Bo Yan; robin.murphy at arm.com; Alexander Van Brunt; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH] arm64: alternative:flush cache with unpatched code
[RK] - Thanks for the comments Mark. Reply inlined.
Thanks
Rohit
________________________________________
From: Mark Rutland <mark.rutland@arm.com>
Sent: Friday, June 1, 2018 2:03 AM
To: Rohit Khanna
Cc: catalin.marinas at arm.com; robin.murphy at arm.com; Suzuki.Poulose at arm.com; linux-arm-kernel at lists.infradead.org; Alexander Van Brunt; Bo Yan; will.deacon at arm.com
Subject: Re: [PATCH] arm64: alternative:flush cache with unpatched code
Hi,
As a general thing, could you please add a version number to patches in future?
i.e. this should be PATCHv4.
It really helps keeping track of patches, distinguishing versions, etc.
On Thu, May 31, 2018 at 01:37:34PM -0700, Rohit Khanna wrote:
> In the current implementation, __apply_alternatives patches
> flush_icache_range and then executes it without invalidating the icache.
> Thus, icache can contain some of the old instructions for
> flush_icache_range. This can cause unpredictable behavior as during
> execution we can get a mix of old and new instructions for
> flush_icache_range.
>
> This patch :
>
> 1. Adds a new function clean_dcache_range_nopatch for flushing kernel
> memory range. This function uses non hot-patched code and can be
> safely used to flush cache during code patching.
>
> 2. Modifies __apply_alternatives so that it uses
> clean_dcache_range_nopatch to flush the cache range after patching code.
>
> Signed-off-by: Rohit Khanna <rokhanna@nvidia.com>
> ---
> arch/arm64/include/asm/sysreg.h | 3 +++
> arch/arm64/kernel/alternative.c | 37 ++++++++++++++++++++++++++++++++++++-
> 2 files changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 6171178075dc..9d1aee7c9aba 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -617,6 +617,9 @@
> #define MVFR1_FPDNAN_SHIFT 4
> #define MVFR1_FPFTZ_SHIFT 0
>
> +/* SYS_CTR_EL0 */
> +#define SYS_CTR_ISIZE_SHIFT 0
> +#define SYS_CTR_DSIZE_SHIFT 16
We already have CTR_DMINLINE_SHIFT in <asm/cache.h>
Can we please add CTR_IMINLIN_SHIFT there too?
Maybe those should be moved into sysreg.h, but that can be a separate cleanup.
[RK] - <asm/cache.h> doesnt contain CTR_DMINLINE_SHIFT.
[RK ] - Sorry, I was looking at kernel-4.14 and couldnt find it there. I can find it in linux-next repo.
> #define ID_AA64MMFR0_TGRAN4_SHIFT 28
> #define ID_AA64MMFR0_TGRAN64_SHIFT 24
> diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
> index 5c4bce4ac381..6b8c5438b37b 100644
> --- a/arch/arm64/kernel/alternative.c
> +++ b/arch/arm64/kernel/alternative.c
> @@ -122,6 +122,41 @@ static void patch_alternative(struct alt_instr *alt,
> }
> }
>
> +/* This is used for flushing kernel memory range after
> + * __apply_alternatives has patched kernel code
> + */
> +static void clean_dcache_range_nopatch(void *start, void *end)
> +{
> + u64 d_start, i_start, d_size, i_size, ctr_el0;
I don't think we need separate i_start and d_start variables. A 'start' or
'cur' variable could be used for both.
[RK] - ok.
> +
> + /* use sanitised value of ctr_el0 rather than raw value from CPU */
> + ctr_el0 = read_sanitised_ftr_reg(SYS_CTR_EL0);
> + /* size in bytes */
> + d_size = cpuid_feature_extract_unsigned_field(ctr_el0,
> + SYS_CTR_DSIZE_SHIFT);
> + i_size = cpuid_feature_extract_unsigned_field(ctr_el0,
> + SYS_CTR_ISIZE_SHIFT);
This isn't the size in bytes. Each is log2 the number of (4-byte) words.
i.e. the size in bytes is (xMinLine << 2).
[RK] - This doesnt seem right. For eg if IMinLine = 4 or 0b100
then with above formula ICacheSize in Bytes = 4 << 2 = 16
The correct formula should be (4 << xMinLine).
So in case IMinLine = 4 or 0b100,
ICacheSizeBytes = 4 << 4 = 64B
> +
> + d_start = (u64)start & ~(d_size - 1);
> + while (d_start <= (u64)end) {
> + /* Use civac instead of cvau. This is required
> + * due to ARM errata 826319, 827319, 824069,
> + * 819472 on A53
> + */
> + asm volatile("dc civac, %0" : : "r" (d_start));
Either this needs a memory clobber, or we need barrier() first, to ensure that
the compiler doesn't re-order this against some of the patching code, however
unlikely that may be.
[RK] - So add barrier() before calling clean_dcache_range_nopatch() ?
> + d_start += d_size;
> + }
The loop can be simplified to:
do {
asm ( ... );
} while (d_start += d_size, d_start < (u64)end)
[RK] - ok
> + dsb(ish);
> +
> + i_start = (u64)start & ~(i_size - 1);
> + while (i_start <= (u64)end) {
> + asm volatile("ic ivau, %0" : : "r" (i_start));
> + i_start += i_size;
> + }
Likewise here.
[RK] - ok
Thanks,
Mark.
> + dsb(ish);
> + isb();
> +}
> +
> static void __apply_alternatives(void *alt_region, bool use_linear_alias)
> {
> struct alt_instr *alt;
> @@ -155,7 +190,7 @@ static void __apply_alternatives(void *alt_region, bool use_linear_alias)
>
> alt_cb(alt, origptr, updptr, nr_inst);
>
> - flush_icache_range((uintptr_t)origptr,
> + clean_dcache_range_nopatch((uintptr_t)origptr,
> (uintptr_t)(origptr + nr_inst));
> }
> }
> --
> 2.1.4
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [Query] PAGE_OFFSET on KASLR enabled ARM64 kernel
From: Bhupesh Sharma @ 2018-06-01 21:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5c78224a-528d-7aa5-58ba-858681f03b6a@redhat.com>
On Sat, Jun 2, 2018 at 3:11 AM, Bhupesh Sharma <bhsharma@redhat.com> wrote:
> On 05/31/2018 10:21 AM, Bhupesh Sharma wrote:
>>
>> Hi Ard,
>>
>> Sorry I was out for most of the day yesterday. Please see my responses
>> inline.
>>
>> On Mon, May 28, 2018 at 12:16 PM, Ard Biesheuvel
>> <ard.biesheuvel@linaro.org> wrote:
>>>
>>> On 27 May 2018 at 23:03, Bhupesh Sharma <bhsharma@redhat.com> wrote:
>>>>
>>>> Hi ARM64 maintainers,
>>>>
>>>> I am confused about the PAGE_OFFSET value (or the start of the linear
>>>> map) on a KASLR enabled ARM64 kernel that I am seeing on a board which
>>>> supports a compatible EFI firmware (with EFI_RNG_PROTOCOL support).
>>>>
>>>> 1. 'arch/arm64/include/asm/memory.h' defines PAGE_OFFSET as:
>>>>
>>>> /*
>>>> * PAGE_OFFSET - the virtual address of the start of the linear map
>>>> (top
>>>> * (VA_BITS - 1))
>>>> */
>>>> #define PAGE_OFFSET (UL(0xffffffffffffffff) - \
>>>> (UL(1) << (VA_BITS - 1)) + 1)
>>>>
>>>> So for example on a platform with VA_BITS=48, we have:
>>>> PAGE_OFFSET = 0xffff800000000000
>>>>
>>>> 2. However, for the KASLR case, we set the 'memstart_offset_seed ' to
>>>> use the 16-bits of the 'kaslr-seed' to randomize the linear region in
>>>> 'arch/arm64/kernel/kaslr.c' :
>>>>
>>>> u64 __init kaslr_early_init(u64 dt_phys)
>>>> {
>>>> <snip..>
>>>> /* use the top 16 bits to randomize the linear region */
>>>> memstart_offset_seed = seed >> 48;
>>>> <snip..>
>>>> }
>>>>
>>>> 3. Now, we use the 'memstart_offset_seed' value to randomize the
>>>> 'memstart_addr' value in 'arch/arm64/mm/init.c':
>>>>
>>>> void __init arm64_memblock_init(void)
>>>> {
>>>> <snip..>
>>>>
>>>> if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {
>>>> extern u16 memstart_offset_seed;
>>>> u64 range = linear_region_size -
>>>> (memblock_end_of_DRAM() - memblock_start_of_DRAM());
>>>>
>>>> /*
>>>> * If the size of the linear region exceeds, by a sufficient
>>>> * margin, the size of the region that the available physical
>>>> * memory spans, randomize the linear region as well.
>>>> */
>>>> if (memstart_offset_seed > 0 && range >= ARM64_MEMSTART_ALIGN)
>>>> {
>>>> range = range / ARM64_MEMSTART_ALIGN + 1;
>>>> memstart_addr -= ARM64_MEMSTART_ALIGN *
>>>> ((range * memstart_offset_seed) >> 16);
>>>> }
>>>> }
>>>> <snip..>
>>>> }
>>>>
>>>> 4. Since 'memstart_addr' indicates the start of physical RAM, we
>>>> randomize the same on basis of 'memstart_offset_seed' value above.
>>>> Also the 'memstart_addr' value is available in '/proc/kallsyms' and
>>>> hence can be accessed by user-space applications to read the
>>>> 'memstart_addr' value.
>>>>
>>>> 5. Now since the PAGE_OFFSET value is also used by several user space
>>>> tools (for e.g. makedumpfile tool uses the same to determine the start
>>>> of linear region and hence to read PT_NOTE fields from /proc/kcore), I
>>>> am not sure how to read the randomized value of the same in the KASLR
>>>> enabled case.
>>>>
>>>> 6. Reading the code further and adding some debug prints, it seems the
>>>> 'memblock_start_of_DRAM()' value is more closer to the actual start of
>>>> linear region rather than 'memstart_addr' and 'PAGE_OFFSET" in case of
>>>> KASLR enabled kernel:
>>>>
>>>> [root at qualcomm-amberwing] # dmesg | grep -i "arm64_memblock_init" -A 5
>>>>
>>>> [ 0.000000] inside arm64_memblock_init, memstart_addr =
>>>> ffff976a00000000,
>>>> linearstart_addr = ffffe89600200000, memblock_start_of_DRAM =
>>>> ffffe89600200000,
>>>> PHYS_OFFSET = ffff976a00000000, PAGE_OFFSET = ffff800000000000,
>>>> KIMAGE_VADDR = ffff000008000000, kimage_vaddr = ffff20c2d7800000
>>>>
>>>> [root at qualcomm-amberwing] # dmesg | grep -i "Virtual kernel memory
>>>> layout" -A 15
>>>> [ 0.000000] Virtual kernel memory layout:
>>>> [ 0.000000] modules : 0xffff000000000000 - 0xffff000008000000
>>>> ( 128 MB)
>>>> [ 0.000000] vmalloc : 0xffff000008000000 - 0xffff7bdfffff0000
>>>> (126847 GB)
>>>> [ 0.000000] .text : 0xffff20c2d7880000 - 0xffff20c2d8040000
>>>> ( 7936 KB)
>>>> [ 0.000000] .rodata : 0xffff20c2d8040000 - 0xffff20c2d83a0000
>>>> ( 3456 KB)
>>>> [ 0.000000] .init : 0xffff20c2d83a0000 - 0xffff20c2d8750000
>>>> ( 3776 KB)
>>>> [ 0.000000] .data : 0xffff20c2d8750000 - 0xffff20c2d891b200
>>>> ( 1837 KB)
>>>> [ 0.000000] .bss : 0xffff20c2d891b200 - 0xffff20c2d90a5198
>>>> ( 7720 KB)
>>>> [ 0.000000] fixed : 0xffff7fdffe790000 - 0xffff7fdffec00000
>>>> ( 4544 KB)
>>>> [ 0.000000] PCI I/O : 0xffff7fdffee00000 - 0xffff7fdfffe00000
>>>> ( 16 MB)
>>>> [ 0.000000] vmemmap : 0xffff7fe000000000 - 0xffff800000000000
>>>> ( 128 GB maximum)
>>>> [ 0.000000] 0xffff7ffa25800800 - 0xffff7ffa2b800000
>>>> ( 95 MB actual)
>>>> [ 0.000000] memory : 0xffffe89600200000 - 0xffffe8ae00000000
>>>> ( 98302 MB)
>>>>
>>>> As one can see above, the 'memblock_start_of_DRAM()' value of
>>>> 0xffffe89600200000 represents the start of linear region:
>>>>
>>>> [ 0.000000] memory : 0xffffe89600200000 - 0xffffe8ae00000000
>>>> ( 98302 MB)
>>>>
>>>> So, my question is to access the start of linear region (which was
>>>> earlier determinable via PAGE_OFFSET macro), whether I should:
>>>>
>>>> - do some back-computation for the start of linear region from the
>>>> 'memstart_addr' in user-space, or
>>>> - use a new global variable in kernel which is assigned the value of
>>>> memblock_start_of_DRAM()' and assign it to '/proc/kallsyms', so that
>>>> it can be read by user-space tools, or
>>>> - whether we should rather look at removing the PAGE_OFFSET usage from
>>>> the kernel and replace it with a global variable instead which is
>>>> properly updated for KASLR case as well.
>>>>
>>>> Kindly share your opinions on what can be a suitable solution in this
>>>> case.
>>>>
>>>> Thanks for your help.
>>>>
>>>
>>> Hello Bhupesh,
>>>
>>> Could you explain what the relevance is of PAGE_OFFSET to userland?
>>> The only thing that should matter is where the actual linear mapping
>>> of DRAM is, and I am not sure I understand why we care about where it
>>> resides relative to the base of the linear region.
>>
>>
>> Actually certain user-space tools like makedumpfile (which is used to
>> generate and compress the vmcore) and crash-utility (which is used to
>> debug the vmcore), rely on the PAGE_OFFSET value (which denotes the
>> base of the linear map region) to determine virtual to physical
>> mapping of the addresses lying in the linear region .
>>
>> One specific use case that I am working on at the moment is the
>> makedumpfile '--mem-usage', which allows one to see the page numbers
>> of current system (1st kernel) in different use (please see
>> MAKEDUMPFILE(8) for more details).
>>
>> Using this we can know how many pages are dumpable when different
>> dump_level is specified when invoking the makedumpfile.
>>
>> Normally, makedumpfile analyses the contents of '/proc/kcore' (while
>> excluding the crashkernel range), and then calculates the page number
>> of different kind per vmcoreinfo.
>>
>> For e.g. here is an output from my arm64 board (a non KASLR boot):
>>
>> TYPE PAGES EXCLUDABLE DESCRIPTION
>>
>> ----------------------------------------------------------------------
>> ZERO 49524 yes Pages
>> filled with zero
>> NON_PRI_CACHE 15143 yes Cache
>> pages without private flag
>> PRI_CACHE 29147 yes Cache
>> pages with private flag
>> USER 3684 yes User process
>> pages
>> FREE 1450569 yes Free pages
>> KERN_DATA 14243 no Dumpable
>> kernel data
>>
>> page size: 65536
>> Total pages on system: 1562310
>> Total size on system: 102387548160 Byte
>>
>> This use case requires directly reading the '/proc/kcore' and the
>> hence the PAGE_OFFSET value is used to determine the base address of
>> the linear region, whose value is not static in case of KASLR boot.
>>
>> Another use-case is where the crash-utility uses the PAGE_OFFSET value
>> to perform a virtual-to-physical conversion for the address lying in
>> the linear region:
>>
>> ulong
>> arm64_VTOP(ulong addr)
>> {
>> if (machdep->flags & NEW_VMEMMAP) {
>> if (addr >= machdep->machspec->page_offset)
>> return machdep->machspec->phys_offset
>> + (addr - machdep->machspec->page_offset);
>>
>> <..snip..>
>> }
>>
>
> Another confusing concept is the rounded-up value of 'memstart_addr' in
> 'arch/arm64/mm/init.c' when booting a non-KASLR_ kernel and when the value
> of memblock_start_of_DRAM() < ARM64_MEMSTART_ALIGN:
>
> void __init arm64_memblock_init(void)
> {
>
> <..snip..>
> /*
> * Select a suitable value for the base of physical memory.
> */
> memstart_addr = round_down(memblock_start_of_DRAM(),
> ARM64_MEMSTART_ALIGN);
> <..snip..>
> }
>
> For example, let's consider a case (which I see on my qualcomm board) where
> memblock_start_of_DRAM() = 0x200000 and ARM64_MEMSTART_ALIGN = 0x40000000 (I
> am using VA_BITS = 48 and a 64K page size), in this case
> memstart_addr is calculated at 0, as the round_down results in a value of 0.
>
> This is in contrast with the definition of the 'memblock_start_of_DRAM':
>
> /* lowest address */
> phys_addr_t __init_memblock memblock_start_of_DRAM(void)
> {
> return memblock.memory.regions[0].base;
> }
>
> As indicated by logs below, the first memblock region base starts from
> 0x200000 rather than the 'memstart_addr' value (which is 0)
>
> # dmesg | grep -i "Processing" -A 5
> [ 0.000000] efi: Processing EFI memory map:
> [ 0.000000] efi: 0x000000200000-0x00000021ffff [Runtime Data |RUN| |
> | | | | | |WB|WT|WC|UC]
> [ 0.000000] efi: 0x000000400000-0x0000005fffff [ACPI Memory NVS | |
> | | | | | | | | | |UC]
>
> # head -1 /proc/iomem
> 00200000-0021ffff : reserved
>
> Since we define 'PHYS_OFFSET' as the physical address of the start of memory
> it would be 0 in this case:
>
> /* PHYS_OFFSET - the physical address of the start of memory. */
> #define PHYS_OFFSET ({ VM_BUG_ON(memstart_addr & 1);
> memstart_addr; })
>
> On the other hand, the first memblock starts from 0x200000, so my question
> is whether we should update the user-space tools which use the memblocks
> listed in '/proc/iomem' to obtain the value of PHY_OFFSET (by reading the
> base of the 1st memblock) and read the value of 'memstart_addr' somehow in
> user-space to get the PHY_OFFSET, or should the change be done at the kernel
> end to calculate 'memstart_addr' as:
>
>
> /*
> * Select a suitable value for the base of physical memory.
> */
> memstart_addr = round_down(memblock_start_of_DRAM(),
> ARM64_MEMSTART_ALIGN);
> if (memstart_addr)
Sorry for the typo: I meant if (!memstart_addr) above
Regards,
Bhupesh
> memstart_addr = memblock_start_of_DRAM();
>
> Please share your views.
>
> Thanks,
> Bhupesh
^ permalink raw reply
* [PATCH v3 2/8] dt-bindings: media: Document data-enable-active property
From: Sakari Ailus @ 2018-06-01 22:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180601145827.GG10472@w540>
On Fri, Jun 01, 2018 at 04:58:27PM +0200, jacopo mondi wrote:
> Hi Sakari,
>
> On Fri, Jun 01, 2018 at 01:29:10PM +0300, Sakari Ailus wrote:
> > Hi Jacopo,
> >
> > Thanks for the patch.
> >
> > On Tue, May 29, 2018 at 05:05:53PM +0200, Jacopo Mondi wrote:
> > > Add 'data-enable-active' property to endpoint node properties list.
> > >
> > > The property allows to specify the polarity of the data-enable signal, which
> > > when in active state determinates when data lanes have to sampled for valid
> > > pixel data.
> >
> > Lanes or lines? I understand this is forthe parallel interface.
> >
>
> Now I'm confused. Are the parallel data 'lines' and the CSI-2 one
> 'lanes'? I thought 'lanes' applies to both.. am I wrong?
"Lane" is conventionally refer to a differential pair of wires on a serial
bus such as CSI-2. "Line" is used of a wire on a parallel bus.
--
Sakari Ailus
sakari.ailus at linux.intel.com
^ permalink raw reply
* [PATCH] ASoC: dapm: delete dapm_kcontrol_data paths entry before freeing
From: Srinivas Kandagatla @ 2018-06-01 22:53 UTC (permalink / raw)
To: linux-arm-kernel
dapm_kcontrol_data is freed as part of dapm_kcontrol_free(), leaving the
paths list pointer dangling in the list.
This leads to system crash when we try to unload and reload sound card.
I hit this bug during ADSP crash/reboot test case on Dragon board DB410c.
Below is the kernel BUG with SLAB Poisoning
=============================================================================
BUG kmalloc-128 (Tainted: G W ): Poison overwritten
-----------------------------------------------------------------------------
Disabling lock debugging due to kernel taint
INFO: 0xffff80003cf1c310-0xffff80003cf1c31f. First byte 0x10 instead of 0x6b
INFO: Allocated in dapm_kcontrol_data_alloc.isra.37+0x34/0x2a8 age=6929 cpu=0 pid=50
__slab_alloc.isra.24+0x24/0x38
kmem_cache_alloc+0x190/0x1d8
dapm_kcontrol_data_alloc.isra.37+0x34/0x2a8
dapm_create_or_share_kcontrol+0x1d4/0x290
snd_soc_dapm_new_widgets+0x410/0x568
snd_soc_register_card+0xa58/0xcd0
apq8016_sbc_bind+0x31c/0x458
try_to_bring_up_master+0x204/0x2e8
component_add+0x94/0x178
q6pcm_routing_probe+0x38/0x48
platform_drv_probe+0x58/0xb8
driver_probe_device+0x324/0x478
__device_attach_driver+0xa8/0x160
bus_for_each_drv+0x48/0x98
__device_attach+0xc0/0x158
device_initial_probe+0x10/0x18
INFO: Freed in dapm_kcontrol_free+0x40/0x50 age=3135 cpu=1 pid=1792
kfree+0x1bc/0x1d0
dapm_kcontrol_free+0x40/0x50
snd_ctl_free_one+0x20/0x38
snd_ctl_remove+0xf0/0x108
snd_ctl_dev_free+0x3c/0x70
__snd_device_free+0x50/0x88
snd_device_free_all+0x2c/0x50
release_card_device+0x1c/0x78
device_release+0x34/0x98
kobject_put+0x90/0x1f0
put_device+0x14/0x20
snd_card_free+0x54/0x70
snd_soc_unregister_card+0x84/0x138
snd_soc_unregister_component+0xa4/0xd0
q6routing_dai_unbind+0x44/0x78
component_unbind.isra.4+0x28/0x50
INFO: Slab 0xffff7e0000f3c700 objects=25 used=0 fp=0xffff80003cf1fc80 flags=0xfffc00000008100
INFO: Object 0x (ptrval) @offset=768 fp=0x (ptrval)
Redzone (ptrval): bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb ................
Redzone (ptrval): bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb ................
Redzone (ptrval): bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb ................
Redzone (ptrval): bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb ................
Redzone (ptrval): bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb ................
Redzone (ptrval): bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb ................
Redzone (ptrval): bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb ................
Redzone (ptrval): bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb ................
Object (ptrval): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
Object (ptrval): 10 c3 f1 3c 00 80 ff ff 10 c3 f1 3c 00 80 ff ff ...<.......<....
Object (ptrval): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
Object (ptrval): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
Object (ptrval): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
Object (ptrval): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
Object (ptrval): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
Object (ptrval): 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5 kkkkkkkkkkkkkkk.
Redzone (ptrval): bb bb bb bb bb bb bb bb ........
Padding (ptrval): 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZZZZZZZZZ
Padding (ptrval): 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZZZZZZZZZ
Padding (ptrval): 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZZZZZZZZZ
Padding (ptrval): 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZZZZZZZZZ
CPU: 1 PID: 1792 Comm: sh Tainted: G B W 4.17.0-rc7-02229-gb429ee402d16-dirty #202
Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
Call trace:
dump_backtrace+0x0/0x1b0
show_stack+0x14/0x20
dump_stack+0x9c/0xbc
print_trailer+0x124/0x1d8
check_bytes_and_report+0xe8/0x120
check_object+0x24c/0x288
__free_slab+0x9c/0x2f0
discard_slab+0x60/0x88
__slab_free+0x35c/0x3e8
kfree+0x1bc/0x1d0
snd_soc_dapm_free_widget+0xac/0xd0
snd_soc_dapm_free+0x64/0xb8
soc_remove_component+0x50/0x80
soc_remove_dai_links+0x110/0x208
snd_soc_unregister_card+0x9c/0x138
snd_soc_unregister_component+0xa4/0xd0
q6routing_dai_unbind+0x44/0x78
component_unbind.isra.4+0x28/0x50
component_unbind_all+0xc0/0xe8
apq8016_sbc_unbind+0x50/0xa0
take_down_master+0x24/0x48
component_del+0x90/0x130
q6afe_dai_dev_remove+0x40/0x68
platform_drv_remove+0x24/0x50
device_release_driver_internal+0x170/0x208
device_release_driver+0x14/0x20
bus_remove_device+0xcc/0x150
device_del+0x10c/0x310
platform_device_del.part.3+0x24/0x90
platform_device_unregister+0x18/0x30
of_platform_device_destroy+0x94/0x98
q6afe_remove+0x20/0x38
apr_device_remove+0x30/0x70
device_release_driver_internal+0x170/0x208
device_release_driver+0x14/0x20
bus_remove_device+0xcc/0x150
device_del+0x10c/0x310
device_unregister+0x1c/0x70
apr_remove_device+0xc/0x18
device_for_each_child+0x50/0x80
apr_remove+0x18/0x20
rpmsg_dev_remove+0x38/0x68
device_release_driver_internal+0x170/0x208
device_release_driver+0x14/0x20
bus_remove_device+0xcc/0x150
device_del+0x10c/0x310
device_unregister+0x1c/0x70
qcom_smd_remove_device+0xc/0x18
device_for_each_child+0x50/0x80
qcom_smd_unregister_edge+0x3c/0x70
smd_subdev_remove+0x18/0x28
rproc_stop+0x48/0xd8
rproc_shutdown+0x60/0xe8
state_store+0xbc/0xf8
dev_attr_store+0x18/0x28
sysfs_kf_write+0x3c/0x50
kernfs_fop_write+0x118/0x1e0
__vfs_write+0x18/0x110
vfs_write+0xa4/0x1a8
ksys_write+0x48/0xb0
sys_write+0xc/0x18
el0_svc_naked+0x30/0x34
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
sound/soc/soc-dapm.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 1e9a36389667..36a39ba30226 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -433,6 +433,8 @@ static int dapm_kcontrol_data_alloc(struct snd_soc_dapm_widget *widget,
static void dapm_kcontrol_free(struct snd_kcontrol *kctl)
{
struct dapm_kcontrol_data *data = snd_kcontrol_chip(kctl);
+
+ list_del(&data->paths);
kfree(data->wlist);
kfree(data);
}
--
2.16.2
^ permalink raw reply related
* [PATCH] of: platform: stop accessing invalid dev in of_platform_device_destroy
From: Srinivas Kandagatla @ 2018-06-01 22:58 UTC (permalink / raw)
To: linux-arm-kernel
Immediately after the platform_device_unregister() the device will be cleaned up.
Accessing the freed pointer immediately after that will crash the system.
Found this bug when kernel is built with CONFIG_PAGE_POISONING and testing
loading/unloading audio drivers in a loop on Qcom platforms.
Fix this by removing accessing the dev pointer.
Below is the carsh trace:
Unable to handle kernel paging request at virtual address 6b6b6b6b6b6c03
Mem abort info:
ESR = 0x96000021
Exception class = DABT (current EL), IL = 32 bits
SET = 0, FnV = 0
EA = 0, S1PTW = 0
Data abort info:
ISV = 0, ISS = 0x00000021
CM = 0, WnR = 0
[006b6b6b6b6b6c03] address between user and kernel address ranges
Internal error: Oops: 96000021 [#1] PREEMPT SMP
Modules linked in:
CPU: 2 PID: 1784 Comm: sh Tainted: G W 4.17.0-rc7-02230-ge3a63a7ef641-dirty #204
Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
pstate: 80000005 (Nzcv daif -PAN -UAO)
pc : clear_bit+0x18/0x2c
lr : of_platform_device_destroy+0x64/0xb8
sp : ffff00000c9c3930
x29: ffff00000c9c3930 x28: ffff80003d39b200
x27: ffff000008bb1000 x26: 0000000000000040
x25: 0000000000000124 x24: ffff80003a9a3080
x23: 0000000000000060 x22: ffff00000939f518
x21: ffff80003aa79e98 x20: ffff80003aa3dae0
x19: ffff80003aa3c890 x18: ffff800009feb794
x17: 0000000000000000 x16: 0000000000000000
x15: ffff800009feb790 x14: 0000000000000000
x13: ffff80003a058778 x12: ffff80003a058728
x11: ffff80003a058750 x10: 0000000000000000
x9 : 0000000000000006 x8 : ffff80003a825988
x7 : bbbbbbbbbbbbbbbb x6 : 0000000000000001
x5 : 0000000000000000 x4 : 0000000000000001
x3 : 0000000000000008 x2 : 0000000000000001
x1 : 6b6b6b6b6b6b6c03 x0 : 0000000000000000
Process sh (pid: 1784, stack limit = 0x (ptrval))
Call trace:
clear_bit+0x18/0x2c
q6afe_remove+0x20/0x38
apr_device_remove+0x30/0x70
device_release_driver_internal+0x170/0x208
device_release_driver+0x14/0x20
bus_remove_device+0xcc/0x150
device_del+0x10c/0x310
device_unregister+0x1c/0x70
apr_remove_device+0xc/0x18
device_for_each_child+0x50/0x80
apr_remove+0x18/0x20
rpmsg_dev_remove+0x38/0x68
device_release_driver_internal+0x170/0x208
device_release_driver+0x14/0x20
bus_remove_device+0xcc/0x150
device_del+0x10c/0x310
device_unregister+0x1c/0x70
qcom_smd_remove_device+0xc/0x18
device_for_each_child+0x50/0x80
qcom_smd_unregister_edge+0x3c/0x70
smd_subdev_remove+0x18/0x28
rproc_stop+0x48/0xd8
rproc_shutdown+0x60/0xe8
state_store+0xbc/0xf8
dev_attr_store+0x18/0x28
sysfs_kf_write+0x3c/0x50
kernfs_fop_write+0x118/0x1e0
__vfs_write+0x18/0x110
vfs_write+0xa4/0x1a8
ksys_write+0x48/0xb0
sys_write+0xc/0x18
el0_svc_naked+0x30/0x34
Code: d2800022 8b400c21 f9800031 9ac32043 (c85f7c22)
---[ end trace 32020935775616a2 ]---
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
drivers/of/platform.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index c00d81dfac0b..78c32b93c0e7 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -544,8 +544,6 @@ int of_platform_device_destroy(struct device *dev, void *data)
amba_device_unregister(to_amba_device(dev));
#endif
- of_node_clear_flag(dev->of_node, OF_POPULATED);
- of_node_clear_flag(dev->of_node, OF_POPULATED_BUS);
return 0;
}
EXPORT_SYMBOL_GPL(of_platform_device_destroy);
--
2.16.2
^ permalink raw reply related
* [PATCH] of: platform: stop accessing invalid dev in of_platform_device_destroy
From: Srinivas Kandagatla @ 2018-06-01 23:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180601225822.23439-1-srinivas.kandagatla@linaro.org>
On 01/06/18 23:58, Srinivas Kandagatla wrote:
>
> - of_node_clear_flag(dev->of_node, OF_POPULATED);
> - of_node_clear_flag(dev->of_node, OF_POPULATED_BUS);
This change seems to have a side effect during re-probing. I will dig in
bit more to see how best it can be fixed.
thanks,
srini
^ permalink raw reply
* [PATCH v2] of: platform: stop accessing invalid dev in of_platform_device_destroy
From: Srinivas Kandagatla @ 2018-06-02 0:03 UTC (permalink / raw)
To: linux-arm-kernel
Immediately after the platform_device_unregister() the device will be cleaned up.
Accessing the freed pointer immediately after that will crash the system.
Found this bug when kernel is built with CONFIG_PAGE_POISONING and testing
loading/unloading audio drivers in a loop on Qcom platforms.
Fix this by removing accessing the dev pointer.
Below is the carsh trace:
Unable to handle kernel paging request at virtual address 6b6b6b6b6b6c03
Mem abort info:
ESR = 0x96000021
Exception class = DABT (current EL), IL = 32 bits
SET = 0, FnV = 0
EA = 0, S1PTW = 0
Data abort info:
ISV = 0, ISS = 0x00000021
CM = 0, WnR = 0
[006b6b6b6b6b6c03] address between user and kernel address ranges
Internal error: Oops: 96000021 [#1] PREEMPT SMP
Modules linked in:
CPU: 2 PID: 1784 Comm: sh Tainted: G W 4.17.0-rc7-02230-ge3a63a7ef641-dirty #204
Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
pstate: 80000005 (Nzcv daif -PAN -UAO)
pc : clear_bit+0x18/0x2c
lr : of_platform_device_destroy+0x64/0xb8
sp : ffff00000c9c3930
x29: ffff00000c9c3930 x28: ffff80003d39b200
x27: ffff000008bb1000 x26: 0000000000000040
x25: 0000000000000124 x24: ffff80003a9a3080
x23: 0000000000000060 x22: ffff00000939f518
x21: ffff80003aa79e98 x20: ffff80003aa3dae0
x19: ffff80003aa3c890 x18: ffff800009feb794
x17: 0000000000000000 x16: 0000000000000000
x15: ffff800009feb790 x14: 0000000000000000
x13: ffff80003a058778 x12: ffff80003a058728
x11: ffff80003a058750 x10: 0000000000000000
x9 : 0000000000000006 x8 : ffff80003a825988
x7 : bbbbbbbbbbbbbbbb x6 : 0000000000000001
x5 : 0000000000000000 x4 : 0000000000000001
x3 : 0000000000000008 x2 : 0000000000000001
x1 : 6b6b6b6b6b6b6c03 x0 : 0000000000000000
Process sh (pid: 1784, stack limit = 0x (ptrval))
Call trace:
clear_bit+0x18/0x2c
q6afe_remove+0x20/0x38
apr_device_remove+0x30/0x70
device_release_driver_internal+0x170/0x208
device_release_driver+0x14/0x20
bus_remove_device+0xcc/0x150
device_del+0x10c/0x310
device_unregister+0x1c/0x70
apr_remove_device+0xc/0x18
device_for_each_child+0x50/0x80
apr_remove+0x18/0x20
rpmsg_dev_remove+0x38/0x68
device_release_driver_internal+0x170/0x208
device_release_driver+0x14/0x20
bus_remove_device+0xcc/0x150
device_del+0x10c/0x310
device_unregister+0x1c/0x70
qcom_smd_remove_device+0xc/0x18
device_for_each_child+0x50/0x80
qcom_smd_unregister_edge+0x3c/0x70
smd_subdev_remove+0x18/0x28
rproc_stop+0x48/0xd8
rproc_shutdown+0x60/0xe8
state_store+0xbc/0xf8
dev_attr_store+0x18/0x28
sysfs_kf_write+0x3c/0x50
kernfs_fop_write+0x118/0x1e0
__vfs_write+0x18/0x110
vfs_write+0xa4/0x1a8
ksys_write+0x48/0xb0
sys_write+0xc/0x18
el0_svc_naked+0x30/0x34
Code: d2800022 8b400c21 f9800031 9ac32043 (c85f7c22)
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
Changes since v1:
- fixed issue while reprobing.
drivers/of/platform.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index c00d81dfac0b..84c5c899187b 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -529,10 +529,13 @@ arch_initcall_sync(of_platform_default_populate_init);
int of_platform_device_destroy(struct device *dev, void *data)
{
+ struct device_node *np;
+
/* Do not touch devices not populated from the device tree */
if (!dev->of_node || !of_node_check_flag(dev->of_node, OF_POPULATED))
return 0;
+ np = dev->of_node;
/* Recurse for any nodes that were treated as busses */
if (of_node_check_flag(dev->of_node, OF_POPULATED_BUS))
device_for_each_child(dev, NULL, of_platform_device_destroy);
@@ -544,8 +547,8 @@ int of_platform_device_destroy(struct device *dev, void *data)
amba_device_unregister(to_amba_device(dev));
#endif
- of_node_clear_flag(dev->of_node, OF_POPULATED);
- of_node_clear_flag(dev->of_node, OF_POPULATED_BUS);
+ of_node_clear_flag(np, OF_POPULATED);
+ of_node_clear_flag(np, OF_POPULATED_BUS);
return 0;
}
EXPORT_SYMBOL_GPL(of_platform_device_destroy);
--
2.16.2
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox