* [PATCH v3 2/4] docs: remove :c:func: annotations from xarray.rst
From: Jonathan Corbet @ 2019-06-26 17:28 UTC (permalink / raw)
To: linux-doc
Cc: Mauro Carvalho Chehab, Jani Nikula, linux-kernel, Jonathan Corbet,
Matthew Wilcox
In-Reply-To: <20190626172859.16113-1-corbet@lwn.net>
Now that the build system automatically marks up function references, we
don't have to clutter the source files, so take it out.
[Some paragraphs could now benefit from refilling, but that was left out to
avoid obscuring the real changes.]
Acked-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Jonathan Corbet <corbet@lwn.net>
---
Documentation/core-api/xarray.rst | 270 +++++++++++++++---------------
1 file changed, 135 insertions(+), 135 deletions(-)
diff --git a/Documentation/core-api/xarray.rst b/Documentation/core-api/xarray.rst
index ef6f9f98f595..fcedc5349ace 100644
--- a/Documentation/core-api/xarray.rst
+++ b/Documentation/core-api/xarray.rst
@@ -30,27 +30,27 @@ it called marks. Each mark may be set or cleared independently of
the others. You can iterate over entries which are marked.
Normal pointers may be stored in the XArray directly. They must be 4-byte
-aligned, which is true for any pointer returned from :c:func:`kmalloc` and
-:c:func:`alloc_page`. It isn't true for arbitrary user-space pointers,
+aligned, which is true for any pointer returned from kmalloc() and
+alloc_page(). It isn't true for arbitrary user-space pointers,
nor for function pointers. You can store pointers to statically allocated
objects, as long as those objects have an alignment of at least 4.
You can also store integers between 0 and ``LONG_MAX`` in the XArray.
-You must first convert it into an entry using :c:func:`xa_mk_value`.
+You must first convert it into an entry using xa_mk_value().
When you retrieve an entry from the XArray, you can check whether it is
-a value entry by calling :c:func:`xa_is_value`, and convert it back to
-an integer by calling :c:func:`xa_to_value`.
+a value entry by calling xa_is_value(), and convert it back to
+an integer by calling xa_to_value().
Some users want to store tagged pointers instead of using the marks
-described above. They can call :c:func:`xa_tag_pointer` to create an
-entry with a tag, :c:func:`xa_untag_pointer` to turn a tagged entry
-back into an untagged pointer and :c:func:`xa_pointer_tag` to retrieve
+described above. They can call xa_tag_pointer() to create an
+entry with a tag, xa_untag_pointer() to turn a tagged entry
+back into an untagged pointer and xa_pointer_tag() to retrieve
the tag of an entry. Tagged pointers use the same bits that are used
to distinguish value entries from normal pointers, so each user must
decide whether they want to store value entries or tagged pointers in
any particular XArray.
-The XArray does not support storing :c:func:`IS_ERR` pointers as some
+The XArray does not support storing IS_ERR() pointers as some
conflict with value entries or internal entries.
An unusual feature of the XArray is the ability to create entries which
@@ -64,89 +64,89 @@ entry will cause the XArray to forget about the range.
Normal API
==========
-Start by initialising an XArray, either with :c:func:`DEFINE_XARRAY`
-for statically allocated XArrays or :c:func:`xa_init` for dynamically
+Start by initialising an XArray, either with DEFINE_XARRAY()
+for statically allocated XArrays or xa_init() for dynamically
allocated ones. A freshly-initialised XArray contains a ``NULL``
pointer at every index.
-You can then set entries using :c:func:`xa_store` and get entries
-using :c:func:`xa_load`. xa_store will overwrite any entry with the
+You can then set entries using xa_store() and get entries
+using xa_load(). xa_store will overwrite any entry with the
new entry and return the previous entry stored at that index. You can
-use :c:func:`xa_erase` instead of calling :c:func:`xa_store` with a
+use xa_erase() instead of calling xa_store() with a
``NULL`` entry. There is no difference between an entry that has never
been stored to, one that has been erased and one that has most recently
had ``NULL`` stored to it.
You can conditionally replace an entry at an index by using
-:c:func:`xa_cmpxchg`. Like :c:func:`cmpxchg`, it will only succeed if
+xa_cmpxchg(). Like cmpxchg(), it will only succeed if
the entry at that index has the 'old' value. It also returns the entry
which was at that index; if it returns the same entry which was passed as
-'old', then :c:func:`xa_cmpxchg` succeeded.
+'old', then xa_cmpxchg() succeeded.
If you want to only store a new entry to an index if the current entry
-at that index is ``NULL``, you can use :c:func:`xa_insert` which
+at that index is ``NULL``, you can use xa_insert() which
returns ``-EBUSY`` if the entry is not empty.
You can enquire whether a mark is set on an entry by using
-:c:func:`xa_get_mark`. If the entry is not ``NULL``, you can set a mark
-on it by using :c:func:`xa_set_mark` and remove the mark from an entry by
-calling :c:func:`xa_clear_mark`. You can ask whether any entry in the
-XArray has a particular mark set by calling :c:func:`xa_marked`.
+xa_get_mark(). If the entry is not ``NULL``, you can set a mark
+on it by using xa_set_mark() and remove the mark from an entry by
+calling xa_clear_mark(). You can ask whether any entry in the
+XArray has a particular mark set by calling xa_marked().
You can copy entries out of the XArray into a plain array by calling
-:c:func:`xa_extract`. Or you can iterate over the present entries in
-the XArray by calling :c:func:`xa_for_each`. You may prefer to use
-:c:func:`xa_find` or :c:func:`xa_find_after` to move to the next present
+xa_extract(). Or you can iterate over the present entries in
+the XArray by calling xa_for_each(). You may prefer to use
+xa_find() or xa_find_after() to move to the next present
entry in the XArray.
-Calling :c:func:`xa_store_range` stores the same entry in a range
+Calling xa_store_range() stores the same entry in a range
of indices. If you do this, some of the other operations will behave
in a slightly odd way. For example, marking the entry at one index
may result in the entry being marked at some, but not all of the other
indices. Storing into one index may result in the entry retrieved by
some, but not all of the other indices changing.
-Sometimes you need to ensure that a subsequent call to :c:func:`xa_store`
-will not need to allocate memory. The :c:func:`xa_reserve` function
+Sometimes you need to ensure that a subsequent call to xa_store()
+will not need to allocate memory. The xa_reserve() function
will store a reserved entry at the indicated index. Users of the
normal API will see this entry as containing ``NULL``. If you do
-not need to use the reserved entry, you can call :c:func:`xa_release`
+not need to use the reserved entry, you can call xa_release()
to remove the unused entry. If another user has stored to the entry
-in the meantime, :c:func:`xa_release` will do nothing; if instead you
-want the entry to become ``NULL``, you should use :c:func:`xa_erase`.
-Using :c:func:`xa_insert` on a reserved entry will fail.
+in the meantime, xa_release() will do nothing; if instead you
+want the entry to become ``NULL``, you should use xa_erase().
+Using xa_insert() on a reserved entry will fail.
-If all entries in the array are ``NULL``, the :c:func:`xa_empty` function
+If all entries in the array are ``NULL``, the xa_empty() function
will return ``true``.
Finally, you can remove all entries from an XArray by calling
-:c:func:`xa_destroy`. If the XArray entries are pointers, you may wish
+xa_destroy(). If the XArray entries are pointers, you may wish
to free the entries first. You can do this by iterating over all present
-entries in the XArray using the :c:func:`xa_for_each` iterator.
+entries in the XArray using the xa_for_each() iterator.
Allocating XArrays
------------------
-If you use :c:func:`DEFINE_XARRAY_ALLOC` to define the XArray, or
-initialise it by passing ``XA_FLAGS_ALLOC`` to :c:func:`xa_init_flags`,
+If you use DEFINE_XARRAY_ALLOC() to define the XArray, or
+initialise it by passing ``XA_FLAGS_ALLOC`` to xa_init_flags(),
the XArray changes to track whether entries are in use or not.
-You can call :c:func:`xa_alloc` to store the entry at an unused index
+You can call xa_alloc() to store the entry at an unused index
in the XArray. If you need to modify the array from interrupt context,
-you can use :c:func:`xa_alloc_bh` or :c:func:`xa_alloc_irq` to disable
+you can use xa_alloc_bh() or xa_alloc_irq() to disable
interrupts while allocating the ID.
-Using :c:func:`xa_store`, :c:func:`xa_cmpxchg` or :c:func:`xa_insert` will
+Using xa_store(), xa_cmpxchg() or xa_insert() will
also mark the entry as being allocated. Unlike a normal XArray, storing
-``NULL`` will mark the entry as being in use, like :c:func:`xa_reserve`.
-To free an entry, use :c:func:`xa_erase` (or :c:func:`xa_release` if
+``NULL`` will mark the entry as being in use, like xa_reserve().
+To free an entry, use xa_erase() (or xa_release() if
you only want to free the entry if it's ``NULL``).
By default, the lowest free entry is allocated starting from 0. If you
want to allocate entries starting at 1, it is more efficient to use
-:c:func:`DEFINE_XARRAY_ALLOC1` or ``XA_FLAGS_ALLOC1``. If you want to
+DEFINE_XARRAY_ALLOC1() or ``XA_FLAGS_ALLOC1``. If you want to
allocate IDs up to a maximum, then wrap back around to the lowest free
-ID, you can use :c:func:`xa_alloc_cyclic`.
+ID, you can use xa_alloc_cyclic().
You cannot use ``XA_MARK_0`` with an allocating XArray as this mark
is used to track whether an entry is free or not. The other marks are
@@ -155,17 +155,17 @@ available for your use.
Memory allocation
-----------------
-The :c:func:`xa_store`, :c:func:`xa_cmpxchg`, :c:func:`xa_alloc`,
-:c:func:`xa_reserve` and :c:func:`xa_insert` functions take a gfp_t
+The xa_store(), xa_cmpxchg(), xa_alloc(),
+xa_reserve() and xa_insert() functions take a gfp_t
parameter in case the XArray needs to allocate memory to store this entry.
If the entry is being deleted, no memory allocation needs to be performed,
and the GFP flags specified will be ignored.
It is possible for no memory to be allocatable, particularly if you pass
a restrictive set of GFP flags. In that case, the functions return a
-special value which can be turned into an errno using :c:func:`xa_err`.
+special value which can be turned into an errno using xa_err().
If you don't need to know exactly which error occurred, using
-:c:func:`xa_is_err` is slightly more efficient.
+xa_is_err() is slightly more efficient.
Locking
-------
@@ -174,54 +174,54 @@ When using the Normal API, you do not have to worry about locking.
The XArray uses RCU and an internal spinlock to synchronise access:
No lock needed:
- * :c:func:`xa_empty`
- * :c:func:`xa_marked`
+ * xa_empty()
+ * xa_marked()
Takes RCU read lock:
- * :c:func:`xa_load`
- * :c:func:`xa_for_each`
- * :c:func:`xa_find`
- * :c:func:`xa_find_after`
- * :c:func:`xa_extract`
- * :c:func:`xa_get_mark`
+ * xa_load()
+ * xa_for_each()
+ * xa_find()
+ * xa_find_after()
+ * xa_extract()
+ * xa_get_mark()
Takes xa_lock internally:
- * :c:func:`xa_store`
- * :c:func:`xa_store_bh`
- * :c:func:`xa_store_irq`
- * :c:func:`xa_insert`
- * :c:func:`xa_insert_bh`
- * :c:func:`xa_insert_irq`
- * :c:func:`xa_erase`
- * :c:func:`xa_erase_bh`
- * :c:func:`xa_erase_irq`
- * :c:func:`xa_cmpxchg`
- * :c:func:`xa_cmpxchg_bh`
- * :c:func:`xa_cmpxchg_irq`
- * :c:func:`xa_store_range`
- * :c:func:`xa_alloc`
- * :c:func:`xa_alloc_bh`
- * :c:func:`xa_alloc_irq`
- * :c:func:`xa_reserve`
- * :c:func:`xa_reserve_bh`
- * :c:func:`xa_reserve_irq`
- * :c:func:`xa_destroy`
- * :c:func:`xa_set_mark`
- * :c:func:`xa_clear_mark`
+ * xa_store()
+ * xa_store_bh()
+ * xa_store_irq()
+ * xa_insert()
+ * xa_insert_bh()
+ * xa_insert_irq()
+ * xa_erase()
+ * xa_erase_bh()
+ * xa_erase_irq()
+ * xa_cmpxchg()
+ * xa_cmpxchg_bh()
+ * xa_cmpxchg_irq()
+ * xa_store_range()
+ * xa_alloc()
+ * xa_alloc_bh()
+ * xa_alloc_irq()
+ * xa_reserve()
+ * xa_reserve_bh()
+ * xa_reserve_irq()
+ * xa_destroy()
+ * xa_set_mark()
+ * xa_clear_mark()
Assumes xa_lock held on entry:
- * :c:func:`__xa_store`
- * :c:func:`__xa_insert`
- * :c:func:`__xa_erase`
- * :c:func:`__xa_cmpxchg`
- * :c:func:`__xa_alloc`
- * :c:func:`__xa_set_mark`
- * :c:func:`__xa_clear_mark`
+ * __xa_store()
+ * __xa_insert()
+ * __xa_erase()
+ * __xa_cmpxchg()
+ * __xa_alloc()
+ * __xa_set_mark()
+ * __xa_clear_mark()
If you want to take advantage of the lock to protect the data structures
-that you are storing in the XArray, you can call :c:func:`xa_lock`
-before calling :c:func:`xa_load`, then take a reference count on the
-object you have found before calling :c:func:`xa_unlock`. This will
+that you are storing in the XArray, you can call xa_lock()
+before calling xa_load(), then take a reference count on the
+object you have found before calling xa_unlock(). This will
prevent stores from removing the object from the array between looking
up the object and incrementing the refcount. You can also use RCU to
avoid dereferencing freed memory, but an explanation of that is beyond
@@ -261,7 +261,7 @@ context and then erase them in softirq context, you can do that this way::
}
If you are going to modify the XArray from interrupt or softirq context,
-you need to initialise the array using :c:func:`xa_init_flags`, passing
+you need to initialise the array using xa_init_flags(), passing
``XA_FLAGS_LOCK_IRQ`` or ``XA_FLAGS_LOCK_BH``.
The above example also shows a common pattern of wanting to extend the
@@ -269,20 +269,20 @@ coverage of the xa_lock on the store side to protect some statistics
associated with the array.
Sharing the XArray with interrupt context is also possible, either
-using :c:func:`xa_lock_irqsave` in both the interrupt handler and process
-context, or :c:func:`xa_lock_irq` in process context and :c:func:`xa_lock`
+using xa_lock_irqsave() in both the interrupt handler and process
+context, or xa_lock_irq() in process context and xa_lock()
in the interrupt handler. Some of the more common patterns have helper
-functions such as :c:func:`xa_store_bh`, :c:func:`xa_store_irq`,
-:c:func:`xa_erase_bh`, :c:func:`xa_erase_irq`, :c:func:`xa_cmpxchg_bh`
-and :c:func:`xa_cmpxchg_irq`.
+functions such as xa_store_bh(), xa_store_irq(),
+xa_erase_bh(), xa_erase_irq(), xa_cmpxchg_bh()
+and xa_cmpxchg_irq().
Sometimes you need to protect access to the XArray with a mutex because
that lock sits above another mutex in the locking hierarchy. That does
-not entitle you to use functions like :c:func:`__xa_erase` without taking
+not entitle you to use functions like __xa_erase() without taking
the xa_lock; the xa_lock is used for lockdep validation and will be used
for other purposes in the future.
-The :c:func:`__xa_set_mark` and :c:func:`__xa_clear_mark` functions are also
+The __xa_set_mark() and __xa_clear_mark() functions are also
available for situations where you look up an entry and want to atomically
set or clear a mark. It may be more efficient to use the advanced API
in this case, as it will save you from walking the tree twice.
@@ -300,27 +300,27 @@ indeed the normal API is implemented in terms of the advanced API. The
advanced API is only available to modules with a GPL-compatible license.
The advanced API is based around the xa_state. This is an opaque data
-structure which you declare on the stack using the :c:func:`XA_STATE`
+structure which you declare on the stack using the XA_STATE()
macro. This macro initialises the xa_state ready to start walking
around the XArray. It is used as a cursor to maintain the position
in the XArray and let you compose various operations together without
having to restart from the top every time.
The xa_state is also used to store errors. You can call
-:c:func:`xas_error` to retrieve the error. All operations check whether
+xas_error() to retrieve the error. All operations check whether
the xa_state is in an error state before proceeding, so there's no need
for you to check for an error after each call; you can make multiple
calls in succession and only check at a convenient point. The only
errors currently generated by the XArray code itself are ``ENOMEM`` and
``EINVAL``, but it supports arbitrary errors in case you want to call
-:c:func:`xas_set_err` yourself.
+xas_set_err() yourself.
-If the xa_state is holding an ``ENOMEM`` error, calling :c:func:`xas_nomem`
+If the xa_state is holding an ``ENOMEM`` error, calling xas_nomem()
will attempt to allocate more memory using the specified gfp flags and
cache it in the xa_state for the next attempt. The idea is that you take
the xa_lock, attempt the operation and drop the lock. The operation
attempts to allocate memory while holding the lock, but it is more
-likely to fail. Once you have dropped the lock, :c:func:`xas_nomem`
+likely to fail. Once you have dropped the lock, xas_nomem()
can try harder to allocate more memory. It will return ``true`` if it
is worth retrying the operation (i.e. that there was a memory error *and*
more memory was allocated). If it has previously allocated memory, and
@@ -333,7 +333,7 @@ Internal Entries
The XArray reserves some entries for its own purposes. These are never
exposed through the normal API, but when using the advanced API, it's
possible to see them. Usually the best way to handle them is to pass them
-to :c:func:`xas_retry`, and retry the operation if it returns ``true``.
+to xas_retry(), and retry the operation if it returns ``true``.
.. flat-table::
:widths: 1 1 6
@@ -343,89 +343,89 @@ to :c:func:`xas_retry`, and retry the operation if it returns ``true``.
- Usage
* - Node
- - :c:func:`xa_is_node`
+ - xa_is_node()
- An XArray node. May be visible when using a multi-index xa_state.
* - Sibling
- - :c:func:`xa_is_sibling`
+ - xa_is_sibling()
- A non-canonical entry for a multi-index entry. The value indicates
which slot in this node has the canonical entry.
* - Retry
- - :c:func:`xa_is_retry`
+ - xa_is_retry()
- This entry is currently being modified by a thread which has the
xa_lock. The node containing this entry may be freed at the end
of this RCU period. You should restart the lookup from the head
of the array.
* - Zero
- - :c:func:`xa_is_zero`
+ - xa_is_zero()
- Zero entries appear as ``NULL`` through the Normal API, but occupy
an entry in the XArray which can be used to reserve the index for
future use. This is used by allocating XArrays for allocated entries
which are ``NULL``.
Other internal entries may be added in the future. As far as possible, they
-will be handled by :c:func:`xas_retry`.
+will be handled by xas_retry().
Additional functionality
------------------------
-The :c:func:`xas_create_range` function allocates all the necessary memory
+The xas_create_range() function allocates all the necessary memory
to store every entry in a range. It will set ENOMEM in the xa_state if
it cannot allocate memory.
-You can use :c:func:`xas_init_marks` to reset the marks on an entry
+You can use xas_init_marks() to reset the marks on an entry
to their default state. This is usually all marks clear, unless the
XArray is marked with ``XA_FLAGS_TRACK_FREE``, in which case mark 0 is set
and all other marks are clear. Replacing one entry with another using
-:c:func:`xas_store` will not reset the marks on that entry; if you want
+xas_store() will not reset the marks on that entry; if you want
the marks reset, you should do that explicitly.
-The :c:func:`xas_load` will walk the xa_state as close to the entry
+The xas_load() will walk the xa_state as close to the entry
as it can. If you know the xa_state has already been walked to the
entry and need to check that the entry hasn't changed, you can use
-:c:func:`xas_reload` to save a function call.
+xas_reload() to save a function call.
If you need to move to a different index in the XArray, call
-:c:func:`xas_set`. This resets the cursor to the top of the tree, which
+xas_set(). This resets the cursor to the top of the tree, which
will generally make the next operation walk the cursor to the desired
spot in the tree. If you want to move to the next or previous index,
-call :c:func:`xas_next` or :c:func:`xas_prev`. Setting the index does
+call xas_next() or xas_prev(). Setting the index does
not walk the cursor around the array so does not require a lock to be
held, while moving to the next or previous index does.
-You can search for the next present entry using :c:func:`xas_find`. This
-is the equivalent of both :c:func:`xa_find` and :c:func:`xa_find_after`;
+You can search for the next present entry using xas_find(). This
+is the equivalent of both xa_find() and xa_find_after();
if the cursor has been walked to an entry, then it will find the next
entry after the one currently referenced. If not, it will return the
-entry at the index of the xa_state. Using :c:func:`xas_next_entry` to
-move to the next present entry instead of :c:func:`xas_find` will save
+entry at the index of the xa_state. Using xas_next_entry() to
+move to the next present entry instead of xas_find() will save
a function call in the majority of cases at the expense of emitting more
inline code.
-The :c:func:`xas_find_marked` function is similar. If the xa_state has
+The xas_find_marked() function is similar. If the xa_state has
not been walked, it will return the entry at the index of the xa_state,
if it is marked. Otherwise, it will return the first marked entry after
-the entry referenced by the xa_state. The :c:func:`xas_next_marked`
-function is the equivalent of :c:func:`xas_next_entry`.
+the entry referenced by the xa_state. The xas_next_marked()
+function is the equivalent of xas_next_entry().
-When iterating over a range of the XArray using :c:func:`xas_for_each`
-or :c:func:`xas_for_each_marked`, it may be necessary to temporarily stop
-the iteration. The :c:func:`xas_pause` function exists for this purpose.
+When iterating over a range of the XArray using xas_for_each()
+or xas_for_each_marked(), it may be necessary to temporarily stop
+the iteration. The xas_pause() function exists for this purpose.
After you have done the necessary work and wish to resume, the xa_state
is in an appropriate state to continue the iteration after the entry
you last processed. If you have interrupts disabled while iterating,
then it is good manners to pause the iteration and reenable interrupts
every ``XA_CHECK_SCHED`` entries.
-The :c:func:`xas_get_mark`, :c:func:`xas_set_mark` and
-:c:func:`xas_clear_mark` functions require the xa_state cursor to have
+The xas_get_mark(), xas_set_mark() and
+xas_clear_mark() functions require the xa_state cursor to have
been moved to the appropriate location in the xarray; they will do
-nothing if you have called :c:func:`xas_pause` or :c:func:`xas_set`
+nothing if you have called xas_pause() or xas_set()
immediately before.
-You can call :c:func:`xas_set_update` to have a callback function
+You can call xas_set_update() to have a callback function
called each time the XArray updates a node. This is used by the page
cache workingset code to maintain its list of nodes which contain only
shadow entries.
@@ -443,25 +443,25 @@ eg indices 64-127 may be tied together, but 2-6 may not be. This may
save substantial quantities of memory; for example tying 512 entries
together will save over 4kB.
-You can create a multi-index entry by using :c:func:`XA_STATE_ORDER`
-or :c:func:`xas_set_order` followed by a call to :c:func:`xas_store`.
-Calling :c:func:`xas_load` with a multi-index xa_state will walk the
+You can create a multi-index entry by using XA_STATE_ORDER()
+or xas_set_order() followed by a call to xas_store().
+Calling xas_load() with a multi-index xa_state will walk the
xa_state to the right location in the tree, but the return value is not
meaningful, potentially being an internal entry or ``NULL`` even when there
-is an entry stored within the range. Calling :c:func:`xas_find_conflict`
+is an entry stored within the range. Calling xas_find_conflict()
will return the first entry within the range or ``NULL`` if there are no
-entries in the range. The :c:func:`xas_for_each_conflict` iterator will
+entries in the range. The xas_for_each_conflict() iterator will
iterate over every entry which overlaps the specified range.
-If :c:func:`xas_load` encounters a multi-index entry, the xa_index
+If xas_load() encounters a multi-index entry, the xa_index
in the xa_state will not be changed. When iterating over an XArray
-or calling :c:func:`xas_find`, if the initial index is in the middle
+or calling xas_find(), if the initial index is in the middle
of a multi-index entry, it will not be altered. Subsequent calls
or iterations will move the index to the first index in the range.
Each entry will only be returned once, no matter how many indices it
occupies.
-Using :c:func:`xas_next` or :c:func:`xas_prev` with a multi-index xa_state
+Using xas_next() or xas_prev() with a multi-index xa_state
is not supported. Using either of these functions on a multi-index entry
will reveal sibling entries; these should be skipped over by the caller.
--
2.21.0
^ permalink raw reply related
* [PATCH v3 1/4] Docs: An initial automarkup extension for sphinx
From: Jonathan Corbet @ 2019-06-26 17:28 UTC (permalink / raw)
To: linux-doc
Cc: Mauro Carvalho Chehab, Jani Nikula, linux-kernel, Jonathan Corbet
In-Reply-To: <20190626172859.16113-1-corbet@lwn.net>
Rather than fill our text files with :c:func:`function()` syntax, just do
the markup via a hook into the sphinx build process.
Signed-off-by: Jonathan Corbet <corbet@lwn.net>
---
Documentation/conf.py | 3 +-
Documentation/sphinx/automarkup.py | 93 ++++++++++++++++++++++++++++++
2 files changed, 95 insertions(+), 1 deletion(-)
create mode 100644 Documentation/sphinx/automarkup.py
diff --git a/Documentation/conf.py b/Documentation/conf.py
index 7ace3f8852bd..a502baecbb85 100644
--- a/Documentation/conf.py
+++ b/Documentation/conf.py
@@ -34,7 +34,8 @@ needs_sphinx = '1.3'
# Add any Sphinx extension module names here, as strings. They can be
# extensions coming with Sphinx (named 'sphinx.ext.*') or your custom
# ones.
-extensions = ['kerneldoc', 'rstFlatTable', 'kernel_include', 'cdomain', 'kfigure', 'sphinx.ext.ifconfig']
+extensions = ['kerneldoc', 'rstFlatTable', 'kernel_include', 'cdomain',
+ 'kfigure', 'sphinx.ext.ifconfig', 'automarkup']
# The name of the math extension changed on Sphinx 1.4
if (major == 1 and minor > 3) or (major > 1):
diff --git a/Documentation/sphinx/automarkup.py b/Documentation/sphinx/automarkup.py
new file mode 100644
index 000000000000..b300cf129869
--- /dev/null
+++ b/Documentation/sphinx/automarkup.py
@@ -0,0 +1,93 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright 2019 Jonathan Corbet <corbet@lwn.net>
+#
+# Apply kernel-specific tweaks after the initial document processing
+# has been done.
+#
+from docutils import nodes
+from sphinx import addnodes
+import re
+
+#
+# Regex nastiness. Of course.
+# Try to identify "function()" that's not already marked up some
+# other way. Sphinx doesn't like a lot of stuff right after a
+# :c:func: block (i.e. ":c:func:`mmap()`s" flakes out), so the last
+# bit tries to restrict matches to things that won't create trouble.
+#
+RE_function = re.compile(r'([\w_][\w\d_]+\(\))')
+
+#
+# Many places in the docs refer to common system calls. It is
+# pointless to try to cross-reference them and, as has been known
+# to happen, somebody defining a function by these names can lead
+# to the creation of incorrect and confusing cross references. So
+# just don't even try with these names.
+#
+Skipfuncs = [ 'open', 'close', 'read', 'write', 'fcntl', 'mmap'
+ 'select', 'poll', 'fork', 'execve', 'clone', 'ioctl']
+
+#
+# Find all occurrences of function() and try to replace them with
+# appropriate cross references.
+#
+def markup_funcs(docname, app, node):
+ cdom = app.env.domains['c']
+ t = node.astext()
+ done = 0
+ repl = [ ]
+ for m in RE_function.finditer(t):
+ #
+ # Include any text prior to function() as a normal text node.
+ #
+ if m.start() > done:
+ repl.append(nodes.Text(t[done:m.start()]))
+ #
+ # Go through the dance of getting an xref out of the C domain
+ #
+ target = m.group(1)[:-2]
+ target_text = nodes.Text(target + '()')
+ xref = None
+ if target not in Skipfuncs:
+ lit_text = nodes.literal(classes=['xref', 'c', 'c-func'])
+ lit_text += target_text
+ pxref = addnodes.pending_xref('', refdomain = 'c',
+ reftype = 'function',
+ reftarget = target, modname = None,
+ classname = None)
+ xref = cdom.resolve_xref(app.env, docname, app.builder,
+ 'function', target, pxref, lit_text)
+ #
+ # Toss the xref into the list if we got it; otherwise just put
+ # the function text.
+ #
+ if xref:
+ repl.append(xref)
+ else:
+ repl.append(target_text)
+ done = m.end()
+ if done < len(t):
+ repl.append(nodes.Text(t[done:]))
+ return repl
+
+def auto_markup(app, doctree, name):
+ #
+ # This loop could eventually be improved on. Someday maybe we
+ # want a proper tree traversal with a lot of awareness of which
+ # kinds of nodes to prune. But this works well for now.
+ #
+ # The nodes.literal test catches ``literal text``, its purpose is to
+ # avoid adding cross-references to functions that have been explicitly
+ # marked with cc:func:.
+ #
+ for para in doctree.traverse(nodes.paragraph):
+ for node in para.traverse(nodes.Text):
+ if not isinstance(node.parent, nodes.literal):
+ node.parent.replace(node, markup_funcs(name, app, node))
+
+def setup(app):
+ app.connect('doctree-resolved', auto_markup)
+ return {
+ 'parallel_read_safe': True,
+ 'parallel_write_safe': True,
+ }
--
2.21.0
^ permalink raw reply related
* [PATCH v3 0/4] docs: Automatically mark up function references
From: Jonathan Corbet @ 2019-06-26 17:28 UTC (permalink / raw)
To: linux-doc
Cc: Mauro Carvalho Chehab, Jani Nikula, linux-kernel, Jonathan Corbet
For the record, I hope that this is the version I will actually merge. It
adds an extension to automatically recognize references to functions and
create cross references for them, eliminating the need to use the unsightly
:c:func:``function`` notation.
Since v2 little has happened:
- Expand the skip list of system-call names that we shouldn't even
try to mark up.
- Improve the comments in the extension code slightly
- Add a paragraph to the doc-guide discouraging use of :c:func:
Jonathan Corbet (4):
Docs: An initial automarkup extension for sphinx
docs: remove :c:func: annotations from xarray.rst
kernel-doc: Don't try to mark up function names
docs: Note that :c:func: should no longer be used
Documentation/conf.py | 3 +-
Documentation/core-api/xarray.rst | 270 ++++++++++++++---------------
Documentation/doc-guide/sphinx.rst | 13 +-
Documentation/sphinx/automarkup.py | 93 ++++++++++
scripts/kernel-doc | 2 +-
5 files changed, 239 insertions(+), 142 deletions(-)
create mode 100644 Documentation/sphinx/automarkup.py
--
2.21.0
^ permalink raw reply
* Re: [PATCH] binfmt_elf: Extract .note.gnu.property from an ELF file
From: Andy Lutomirski @ 2019-06-26 17:14 UTC (permalink / raw)
To: Dave Martin
Cc: Yu-cheng Yu, X86 ML, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
LKML, open list:DOCUMENTATION, Linux-MM, linux-arch, Linux API,
Arnd Bergmann, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Eugene Syromiatnikov, Florian Weimer, H.J. Lu, Jann Horn,
Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Peter Zijlstra, Randy Dunlap,
Ravi V. Shankar, Vedvyas Shanbhogue, Szabolcs Nagy, libc-alpha
In-Reply-To: <20190502111003.GO3567@e103592.cambridge.arm.com>
On Thu, May 2, 2019 at 4:10 AM Dave Martin <Dave.Martin@arm.com> wrote:
>
> On Wed, May 01, 2019 at 02:12:17PM -0700, Yu-cheng Yu wrote:
> > An ELF file's .note.gnu.property indicates features the executable file
> > can support. For example, the property GNU_PROPERTY_X86_FEATURE_1_AND
> > indicates the file supports GNU_PROPERTY_X86_FEATURE_1_IBT and/or
> > GNU_PROPERTY_X86_FEATURE_1_SHSTK.
> >
> > This patch was part of the Control-flow Enforcement series; the original
> > patch is here: https://lkml.org/lkml/2018/11/20/205. Dave Martin responded
> > that ARM recently introduced new features to NT_GNU_PROPERTY_TYPE_0 with
> > properties closely modelled on GNU_PROPERTY_X86_FEATURE_1_AND, and it is
> > logical to split out the generic part. Here it is.
> >
> > With this patch, if an arch needs to setup features from ELF properties,
> > it needs CONFIG_ARCH_USE_GNU_PROPERTY to be set, and a specific
> > arch_setup_property().
> >
> > For example, for X86_64:
> >
> > int arch_setup_property(void *ehdr, void *phdr, struct file *f, bool inter)
> > {
> > int r;
> > uint32_t property;
> >
> > r = get_gnu_property(ehdr, phdr, f, GNU_PROPERTY_X86_FEATURE_1_AND,
> > &property);
> > ...
> > }
>
> Thanks, this is timely for me. I should be able to build the needed
> arm64 support pretty quickly around this now.
>
> [Cc'ing libc-alpha for the elf.h question -- see (2)]
>
>
> A couple of questions before I look in more detail:
>
> 1) Can we rely on PT_GNU_PROPERTY being present in the phdrs to describe
> the NT_GNU_PROPERTY_TYPE_0 note? If so, we can avoid trying to parse
> irrelevant PT_NOTE segments.
>
>
> 2) Are there standard types for things like the program property header?
> If not, can we add something in elf.h? We should try to coordinate with
> libc on that. Something like
>
Where did PT_GNU_PROPERTY come from? Are there actual docs for it?
Can someone here tell us what the actual semantics of this new ELF
thingy are? From some searching, it seems like it's kind of an ELF
note but kind of not. An actual description would be fantastic.
Also, I don't think there's any actual requirement that the upstream
kernel recognize existing CET-enabled RHEL 8 binaries as being
CET-enabled. I tend to think that RHEL 8 jumped the gun here. While
the upstream kernel should make some reasonble effort to make sure
that RHEL 8 binaries will continue to run, I don't see why we need to
go out of our way to keep the full set of mitigations available for
binaries that were developed against a non-upstream kernel.
In fact, if we handle the legacy bitmap differently from RHEL 8, we
may *have* to make sure that we don't recognize existing RHEL 8
binaries as CET-enabled.
Sigh.
^ permalink raw reply
* Re: [PATCH] scsi: convert to rst for documenation
From: Phong Tran @ 2019-06-26 16:53 UTC (permalink / raw)
To: linux-scsi, Jonathan Corbet
Cc: Shuah Khan, martin.petersen, axboe, avri.altman, beanhuo, evgreen,
Henrik Austad, jpittman, linux-doc, linux-kernel,
linux-kernel-mentees
In-Reply-To: <20190625153658.53ad0e18@lwn.net>
On Wed, Jun 26, 2019 at 4:37 AM Jonathan Corbet <corbet@lwn.net> wrote:
>
> On Sat, 22 Jun 2019 22:19:47 +0700
> Phong Tran <tranmanphong@gmail.com> wrote:
>
> > - Update to the link in documenation
> > - Remove trailing white space
> > - Adaptation the sphinx doc syntax
> >
> > Signed-off-by: Phong Tran <tranmanphong@gmail.com>
>
> Thanks for working to improve the documentation! That said, I think this
> patch needs a fair amount of work before we are ready to accept it. I'll
> only get partway in, but it should be enough to start with.
>
> The first overall thing I would like to point out (and hopefully the SCSI
> folks won't fight me too much on this) is that Documentation/scsi is the
> wrong place for much of this stuff. We are doing our best to organize the
> documentation with the audience in mind. So, for example, documents that
> are of interest to system administrators should go into
> Documentation/admin-guide. Information for driver developers should go in
> Documentation/driver-api. And so on.
>
in my understanding,
link_power_management_policy.txt and scsi-parameters.txt should be in
Documentation/admin-guide correct?
scsi_mid_low_api.txt should be moved to Documentation/driver-api?
> [...]
>
> > diff --git a/Documentation/scsi/link_power_management_policy.rst b/Documentation/scsi/link_power_management_policy.rst
> > new file mode 100644
> > index 000000000000..170f58c94cac
> > --- /dev/null
> > +++ b/Documentation/scsi/link_power_management_policy.rst
> > @@ -0,0 +1,22 @@
> > +SCSI Power Management Policy
> > +============================
> > +
> > +This parameter allows the user to set the link (interface) power management.
> > +There are 3 possible options:
>
> This isn't your fault, but...*which* parameter allows this? The document
> describes the values, but not where they can be set. That makes it less
> than fully useful.
>
> > ++-------------------+------------------------------------------------------+
> > +| Value | Effect |
> > ++===================+======================================================+
> > +| min_power | Tell the controller to try to make the link use the |
> > +| | least possible power when possible. This may |
> > +| | sacrifice some performance due to increased latency |
> > +| | when coming out of lower power states. |
> > ++-------------------+------------------------------------------------------+
> > +| max_performance | Generally, this means no power management. Tell |
> > +| | the controller to have performance be a priority |
> > +| | over power management. |
> > ++-------------------+------------------------------------------------------+
> > +| medium_power | Tell the controller to enter a lower power state |
> > +| | when possible, but do not enter the lowest power |
> > +| | state, thus improving latency over min_power setting.|
> > ++-------------------+------------------------------------------------------+
>
> [...]
>
> > diff --git a/Documentation/scsi/scsi-changer.txt b/Documentation/scsi/scsi-changer.rst
> > similarity index 71%
> > rename from Documentation/scsi/scsi-changer.txt
> > rename to Documentation/scsi/scsi-changer.rst
> > index ade046ea7c17..a4923873c77b 100644
> > --- a/Documentation/scsi/scsi-changer.txt
> > +++ b/Documentation/scsi/scsi-changer.rst
> > @@ -1,4 +1,3 @@
> > -
> > README for the SCSI media changer driver
> > ========================================
> >
> > @@ -10,7 +9,7 @@ common small CD-ROM changers, neither one-lun-per-slot SCSI changers
> > nor IDE drives.
> >
> > Userland tools available from here:
> > - http://linux.bytesex.org/misc/changer.html
> > + http://linux.bytesex.org/misc/changer.html
> >
> >
> > General Information
> > @@ -28,15 +27,17 @@ The SCSI changer model is complex, compared to - for example - IDE-CD
> > changers. But it allows to handle nearly all possible cases. It knows
> > 4 different types of changer elements:
> >
> > +::
>
> Two notes:
>
> - You can put the double colon on the line above ("...elements::") and
> don't need to make a separate line for it.
>
> - But, more to the point, please avoid the temptation to use a literal
> block for something that doesn't actually require that treatment. This
> should be reworked as an RST definition list.
>
> > media transport - this one shuffles around the media, i.e. the
> > transport arm. Also known as "picker".
> > storage - a slot which can hold a media.
> > import/export - the same as above, but is accessible from outside,
> > i.e. there the operator (you !) can use this to
> > fill in and remove media from the changer.
> > - Sometimes named "mailslot".
> > + Sometimes named "mailslot".
> > data transfer - this is the device which reads/writes, i.e. the
> > - CD-ROM / Tape / whatever drive.
> > + CD-ROM / Tape / whatever drive.
>
> [...]
>
> > diff --git a/Documentation/scsi/scsi-generic.txt b/Documentation/scsi/scsi-generic.rst
> > similarity index 70%
> > rename from Documentation/scsi/scsi-generic.txt
> > rename to Documentation/scsi/scsi-generic.rst
> > index 51be20a6a14d..8356810160f0 100644
> > --- a/Documentation/scsi/scsi-generic.txt
> > +++ b/Documentation/scsi/scsi-generic.rst
> > @@ -1,8 +1,10 @@
> > - Notes on Linux SCSI Generic (sg) driver
> > - ---------------------------------------
> > - 20020126
> > +=======================================
> > +Notes on Linux SCSI Generic (sg) driver
> > +=======================================
> > +20020126
> > +
> > Introduction
> > -============
> > +------------
> > The SCSI Generic driver (sg) is one of the four "high level" SCSI device
> > drivers along with sd, st and sr (disk, tape and CDROM respectively). Sg
> > is more generalized (but lower level) than its siblings and tends to be
> > @@ -16,20 +18,20 @@ and examples.
> >
> >
> > Major versions of the sg driver
> > -===============================
> > +-------------------------------
> > There are three major versions of sg found in the linux kernel (lk):
> > - - sg version 1 (original) from 1992 to early 1999 (lk 2.2.5) .
> > - It is based in the sg_header interface structure.
> > + - sg version 1 (original) from 1992 to early 1999 (lk 2.2.5) .
> > + It is based in the sg_header interface structure.
> > - sg version 2 from lk 2.2.6 in the 2.2 series. It is based on
> > - an extended version of the sg_header interface structure.
> > + an extended version of the sg_header interface structure.
> > - sg version 3 found in the lk 2.4 series (and the lk 2.5 series).
> > - It adds the sg_io_hdr interface structure.
> > + It adds the sg_io_hdr interface structure.
>
> Perhaps we don't *really* need to preserve information about what versions
> were around in the 1990's?
>
> > Sg driver documentation
> > -=======================
> > +-----------------------
> > The most recent documentation of the sg driver is kept at the Linux
> > -Documentation Project's (LDP) site:
> > +Documentation Project's (LDP) site:
> > http://www.tldp.org/HOWTO/SCSI-Generic-HOWTO
>
> That document claims to have been last updated in 2002. Is there really
> nothing more recent than that?
>
> > This describes the sg version 3 driver found in the lk 2.4 series.
>
> ...and it's unclear to me that users of the 5.x kernel are much concerned
> with what was found in 2.4.
>
> That is the problem with this document in general. I suspect that about
> the only useful information left in it is the location of the sg3_utils
> source. I honestly don't think that it helps the documentation that much
> to carry forward ancient information to the RST format.
>
> Of course, doing this right by deleting obsolete information and updating
> the documents to reflect current reality is a *lot* more work. Probably
> far more than you were thinking of signing up for. If you were willing to
> work on this, there may be somebody from the SCSI community who would be in
> a position to help you with it.
>
yes! I would greatly appreciate
> Unfortunately, the SCSI community probably did not see this patch because
> you didn't copy the linux-scsi list. I'll fix that now, but they will not
> have seen your original patch. You should be sure to include them on
> future postings.
>
Note in next post.
> I would like to make a suggestion, in addition to all of the above: rather
> than trying to do a mass conversion in a single 4000-line patch, start with
> a single file and post a patch doing just that one, being sure to include
yes will send a new patch for each file.
> the linux-scsi list. That will give everybody something more workable to
> start with.
>
> Thanks,
>
> jon
^ permalink raw reply
* Re: [PATCH v2 2/2] crypto: doc - Fix formatting of new crypto engine content
From: Gary R Hook @ 2019-06-26 16:50 UTC (permalink / raw)
To: Joe Perches, Hook, Gary, herbert@gondor.apana.org.au,
corbet@lwn.net, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-crypto@vger.kernel.org,
davem@davemloft.net
In-Reply-To: <983486e9b2daaa34f84f99a890fcedfeae22b24f.camel@perches.com>
On 6/25/19 7:13 PM, Joe Perches wrote:
> On Tue, 2019-06-25 at 23:43 +0000, Hook, Gary wrote:
>> Tidy up the formatting/grammar in crypto_engine.rst. Use bulleted lists
>> where appropriate.
>
> Hi again Gary.
Howdy!
>
>> diff --git a/Documentation/crypto/crypto_engine.rst b/Documentation/crypto/crypto_engine.rst
> []
>> +Before transferring any request, you have to fill the context enginectx by
>> +providing functions for the following:
>> +
>> +* ``prepare_crypt_hardware``: Called once before any prepare functions are
>> + called.
>> +
>> +* ``unprepare_crypt_hardware``: Called once after all unprepare functions have
>> + been called.
>> +
>> +* ``prepare_cipher_request``/``prepare_hash_request``: Called before each
>> + corresponding request is performed. If some processing or other preparatory
>> + work is required, do it here.
>> +
>> +* ``unprepare_cipher_request``/``unprepare_hash_request``: Called after each
>> + request is handled. Clean up / undo what was done in the prepare function.
>> +
>> +* ``cipher_one_request``/``hash_one_request``: Handle the current request by
>> + performing the operation.
>
> I again suggest not using ``<func>`` but instead use <func>()
> and remove unnecessary blank lines.
with all due respect, those aren't functions, they're function pointers
(as structure members). Therefore, if anything, they should be notated
as (*func)(). But I tried that (with the new patches), and they weren't
detected and emboldened (that's not a word, I know) in the html.
So I left them as-is.
I don't pretend to be a guru on this markup, but if there's a way to
make symbol names fixed-width and bold, I'll gladly do it. But I
disagree on turning these into functions, because that's not what they are.
> i.e.:
>
> * prepare_crypt_hardware(): Called once before any prepare functions are
> called.
> * unprepare_crypt_hardware(): Called once after all unprepare functions
> have been called.
> * prepare_cipher_request()/prepare_hash_request(): Called before each
> corresponding request is performed. If some processing or other preparatory
> work is required, do it here.
> * unprepare_cipher_request()/unprepare_hash_request(): Called after each
> request is handled. Clean up / undo what was done in the prepare function.
> * cipher_one_request()/hash_one_request(): Handle the current request by
> performing the operation.
>
> []
>> +When your driver receives a crypto_request, you must to transfer it to
>> +the crypto engine via one of:
>> +
>> +* crypto_transfer_ablkcipher_request_to_engine()
>
> And removing the unnecessary blank lines below
>
>> +
>> +* crypto_transfer_aead_request_to_engine()
>> +
>> +* crypto_transfer_akcipher_request_to_engine()
>> +
>> +* crypto_transfer_hash_request_to_engine()
>> +
>> +* crypto_transfer_skcipher_request_to_engine()
>> +
>> +At the end of the request process, a call to one of the following functions is needed:
>> +
>> +* crypto_finalize_ablkcipher_request()
>> +
>> +* crypto_finalize_aead_request()
>> +
>> +* crypto_finalize_akcipher_request()
>> +
>> +* crypto_finalize_hash_request()
>> +
>> +* crypto_finalize_skcipher_request()
>
>
The lines between the bulleted items will go, yes. Not the ones around
the leading text of each list (which are necessary to delineate the lists).
^ permalink raw reply
* [PATCH] drm: fix a reference for a renamed file: fb/modedb.rst
From: Mauro Carvalho Chehab @ 2019-06-26 16:14 UTC (permalink / raw)
To: Linux Doc Mailing List
Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, linux-kernel,
Jonathan Corbet, Maarten Lankhorst, Maxime Ripard, Sean Paul,
David Airlie, Daniel Vetter, dri-devel
Due to two patches being applied about the same time, the
reference for modedb.rst file got wrong:
Documentation/fb/modedb.txt is now Documentation/fb/modedb.rst.
Fixes: 1bf4e09227c3 ("drm/modes: Allow to specify rotation and reflection on the commandline")
Fixes: ab42b818954c ("docs: fb: convert docs to ReST and rename to *.rst")
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
drivers/gpu/drm/drm_modes.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 57e6408288c8..4645af681ef8 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1680,7 +1680,7 @@ static int drm_mode_parse_cmdline_options(char *str, size_t len,
*
* Additionals options can be provided following the mode, using a comma to
* separate each option. Valid options can be found in
- * Documentation/fb/modedb.txt.
+ * Documentation/fb/modedb.rst.
*
* The intermediate drm_cmdline_mode structure is required to store additional
* options from the command line modline like the force-enable/disable flag.
--
2.21.0
^ permalink raw reply related
* [PATCH v2] docs: gpu: add msm-crash-dump.rst to the index.rst file
From: Mauro Carvalho Chehab @ 2019-06-26 16:09 UTC (permalink / raw)
To: Linux Doc Mailing List
Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, linux-kernel,
Jonathan Corbet, David Airlie, Daniel Vetter, Maarten Lankhorst,
Maxime Ripard, Sean Paul, dri-devel
This file is currently orphaned.
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
Documentation/gpu/drivers.rst | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/gpu/drivers.rst b/Documentation/gpu/drivers.rst
index 4bfb7068e9f7..6c88c57b90cf 100644
--- a/Documentation/gpu/drivers.rst
+++ b/Documentation/gpu/drivers.rst
@@ -20,6 +20,7 @@ GPU Driver Documentation
xen-front
afbc
komeda-kms
+ msm-crash-dump
.. only:: subproject and html
--
2.21.0
^ permalink raw reply related
* Re: On Nitrokey Pro's ECC support
From: Jonathan Corbet @ 2019-06-26 15:28 UTC (permalink / raw)
To: Konstantin Ryabitsev; +Cc: Jarkko Sakkinen, linux-doc, linux-kernel
In-Reply-To: <20190626152138.GA28688@chatter.i7.local>
On Wed, 26 Jun 2019 11:21:38 -0400
Konstantin Ryabitsev <konstantin@linuxfoundation.org> wrote:
> >Maybe Konstantin (copied) might be willing to supply an update to the
> >document to reflect this?
>
> Hello:
>
> I just sent a patch with updates that reflect ECC capabilities in newer
> devices.
Hey, man, that took you just under an hour to get done. We can't all just
wait around while you twiddle your thumbs... :)
Seriously, though, thanks for doing this,
jon
^ permalink raw reply
* Re: On Nitrokey Pro's ECC support
From: Konstantin Ryabitsev @ 2019-06-26 15:21 UTC (permalink / raw)
To: Jonathan Corbet; +Cc: Jarkko Sakkinen, linux-doc, linux-kernel
In-Reply-To: <20190626082541.2cd5897c@lwn.net>
On Wed, Jun 26, 2019 at 08:25:41AM -0600, Jonathan Corbet wrote:
>On Wed, 26 Jun 2019 17:11:46 +0300
>Jarkko Sakkinen <jarkko.sakkinen@iki.fi> wrote:
>
>> I was getting myself a smartcard stick and looked for options from [1].
>> The documentation says that Nitrokey Pro does not support ECC while it
>> actually does [2]. I was already canceling my order when Jan Suhr, the
>> CEO of that company, kindly pointed out to me this.
>>
>> [1] https://www.kernel.org/doc/html/latest/process/maintainer-pgp-guide.html
>> [2] https://shop.nitrokey.com/shop/product/nitrokey-pro-2-3
>
>Maybe Konstantin (copied) might be willing to supply an update to the
>document to reflect this?
Hello:
I just sent a patch with updates that reflect ECC capabilities in newer
devices.
Best,
-K
^ permalink raw reply
* [PATCH] Documentation: PGP: update for newer HW devices
From: Konstantin Ryabitsev @ 2019-06-26 15:20 UTC (permalink / raw)
To: linux-doc; +Cc: corbet
Newer devices like Yubikey 5 and Nitrokey Pro 2 have added support for
NISTP's implementation of ECC cryptography, so update the guide
accordingly and add a note on when to use nistp256 and when to use
ed25519 for generating S keys.
Signed-off-by: Konstantin Ryabitsev <konstantin@linuxfoundation.org>
---
.../process/maintainer-pgp-guide.rst | 31 ++++++++++---------
1 file changed, 17 insertions(+), 14 deletions(-)
diff --git a/Documentation/process/maintainer-pgp-guide.rst b/Documentation/process/maintainer-pgp-guide.rst
index 4bab7464ff8c..17db11b7ed48 100644
--- a/Documentation/process/maintainer-pgp-guide.rst
+++ b/Documentation/process/maintainer-pgp-guide.rst
@@ -238,7 +238,10 @@ your new subkey::
work.
If for some reason you prefer to stay with RSA subkeys, just replace
- "ed25519" with "rsa2048" in the above command.
+ "ed25519" with "rsa2048" in the above command. Additionally, if you
+ plan to use a hardware device that does not support ED25519 ECC
+ keys, like Nitrokey Pro or a Yubikey, then you should use
+ "nistp256" instead or "ed25519."
Back up your master key for disaster recovery
@@ -432,23 +435,23 @@ Available smartcard devices
Unless all your laptops and workstations have smartcard readers, the
easiest is to get a specialized USB device that implements smartcard
-functionality. There are several options available:
+functionality. There are several options available:
- `Nitrokey Start`_: Open hardware and Free Software, based on FSI
- Japan's `Gnuk`_. Offers support for ECC keys, but fewest security
- features (such as resistance to tampering or some side-channel
- attacks).
-- `Nitrokey Pro`_: Similar to the Nitrokey Start, but more
- tamper-resistant and offers more security features, but no ECC
- support.
-- `Yubikey 4`_: proprietary hardware and software, but cheaper than
+ Japan's `Gnuk`_. One of the few available commercial devices that
+ support ED25519 ECC keys, but offer fewest security features (such as
+ resistance to tampering or some side-channel attacks).
+- `Nitrokey Pro 2`_: Similar to the Nitrokey Start, but more
+ tamper-resistant and offers more security features. Pro 2 supports ECC
+ cryptography (NISTP).
+- `Yubikey 5`_: proprietary hardware and software, but cheaper than
Nitrokey Pro and comes available in the USB-C form that is more useful
with newer laptops. Offers additional security features such as FIDO
- U2F, but no ECC.
+ U2F, among others, and now finally supports ECC keys (NISTP).
`LWN has a good review`_ of some of the above models, as well as several
-others. If you want to use ECC keys, your best bet among commercially
-available devices is the Nitrokey Start.
+others. Your choice will depend on cost, shipping availability in your
+geographical region, and open/proprietary hardware considerations.
.. note::
@@ -457,8 +460,8 @@ available devices is the Nitrokey Start.
Foundation.
.. _`Nitrokey Start`: https://shop.nitrokey.com/shop/product/nitrokey-start-6
-.. _`Nitrokey Pro`: https://shop.nitrokey.com/shop/product/nitrokey-pro-3
-.. _`Yubikey 4`: https://www.yubico.com/product/yubikey-4-series/
+.. _`Nitrokey Pro 2`: https://shop.nitrokey.com/shop/product/nitrokey-pro-2-3
+.. _`Yubikey 5`: https://www.yubico.com/products/yubikey-5-overview/
.. _Gnuk: http://www.fsij.org/doc-gnuk/
.. _`LWN has a good review`: https://lwn.net/Articles/736231/
.. _`qualify for a free Nitrokey Start`: https://www.kernel.org/nitrokey-digital-tokens-for-kernel-developers.html
--
2.21.0
^ permalink raw reply related
* On Nitrokey Pro's ECC support
From: Jarkko Sakkinen @ 2019-06-26 14:11 UTC (permalink / raw)
To: corbet; +Cc: linux-doc, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 428 bytes --]
Hi
I was getting myself a smartcard stick and looked for options from [1].
The documentation says that Nitrokey Pro does not support ECC while it
actually does [2]. I was already canceling my order when Jan Suhr, the
CEO of that company, kindly pointed out to me this.
[1] https://www.kernel.org/doc/html/latest/process/maintainer-pgp-guide.html
[2] https://shop.nitrokey.com/shop/product/nitrokey-pro-2-3
/Jarkko
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] docs: gpu: add msm-crash-dump.rst to the index.rst file
From: Daniel Vetter @ 2019-06-26 14:28 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Linux Doc Mailing List, Mauro Carvalho Chehab, linux-kernel,
Jonathan Corbet, David Airlie, Daniel Vetter, Maarten Lankhorst,
Maxime Ripard, Sean Paul, dri-devel
In-Reply-To: <e22a340cf94240094cfb38f8c62f6916ea99394a.1561556169.git.mchehab+samsung@kernel.org>
On Wed, Jun 26, 2019 at 10:36:11AM -0300, Mauro Carvalho Chehab wrote:
> This file is currently orphaned.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> ---
> Documentation/gpu/index.rst | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/gpu/index.rst b/Documentation/gpu/index.rst
> index 1fcf8e851e15..55f3f4294686 100644
> --- a/Documentation/gpu/index.rst
> +++ b/Documentation/gpu/index.rst
> @@ -12,6 +12,7 @@ Linux GPU Driver Developer's Guide
> drm-uapi
> drm-client
> drivers
> + msm-crash-dump
Should be added to drivers.rst I think, since it's driver-specific
documentation.
-Daniel
> vga-switcheroo
> vgaarbiter
> todo
> --
> 2.21.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply
* Re: On Nitrokey Pro's ECC support
From: Jonathan Corbet @ 2019-06-26 14:25 UTC (permalink / raw)
To: Jarkko Sakkinen; +Cc: linux-doc, linux-kernel, Konstantin Ryabitsev
In-Reply-To: <c9c1e7f83a55bc5fb621e2e4e1dab90c5b3aac01.camel@iki.fi>
On Wed, 26 Jun 2019 17:11:46 +0300
Jarkko Sakkinen <jarkko.sakkinen@iki.fi> wrote:
> I was getting myself a smartcard stick and looked for options from [1].
> The documentation says that Nitrokey Pro does not support ECC while it
> actually does [2]. I was already canceling my order when Jan Suhr, the
> CEO of that company, kindly pointed out to me this.
>
> [1] https://www.kernel.org/doc/html/latest/process/maintainer-pgp-guide.html
> [2] https://shop.nitrokey.com/shop/product/nitrokey-pro-2-3
Maybe Konstantin (copied) might be willing to supply an update to the
document to reflect this?
Thanks,
jon
^ permalink raw reply
* Re: [PATCH v12 04/13] mfd: Add Ingenic TCU driver
From: Paul Cercueil @ 2019-06-26 13:55 UTC (permalink / raw)
To: Lee Jones
Cc: Rob Herring, Mark Rutland, Ralf Baechle, Paul Burton, James Hogan,
Jonathan Corbet, Daniel Lezcano, Thomas Gleixner,
Michael Turquette, Stephen Boyd, Jason Cooper, Marc Zyngier,
Mathieu Malaterre, linux-kernel, devicetree, linux-mips,
linux-doc, linux-clk, od
In-Reply-To: <20190626131850.GW21119@dell>
Hi Lee,
Le mer. 26 juin 2019 à 15:18, Lee Jones <lee.jones@linaro.org> a
écrit :
> On Tue, 21 May 2019, Paul Cercueil wrote:
>
>> This driver will provide a regmap that can be retrieved very early
>> in
>> the boot process through the API function ingenic_tcu_get_regmap().
>>
>> Additionally, it will call devm_of_platform_populate() so that all
>> the
>> children devices will be probed.
>>
>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>> ---
>>
>> Notes:
>> v12: New patch
>>
>> drivers/mfd/Kconfig | 8 +++
>> drivers/mfd/Makefile | 1 +
>> drivers/mfd/ingenic-tcu.c | 113
>> ++++++++++++++++++++++++++++++++
>> include/linux/mfd/ingenic-tcu.h | 8 +++
>> 4 files changed, 130 insertions(+)
>> create mode 100644 drivers/mfd/ingenic-tcu.c
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 294d9567cc71..a13544474e05 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -494,6 +494,14 @@ config HTC_I2CPLD
>> This device provides input and output GPIOs through an I2C
>> interface to one or more sub-chips.
>>
>> +config INGENIC_TCU
>> + bool "Ingenic Timer/Counter Unit (TCU) support"
>> + depends on MIPS || COMPILE_TEST
>> + select REGMAP_MMIO
>> + help
>> + Say yes here to support the Timer/Counter Unit (TCU) IP present
>> + in the JZ47xx SoCs from Ingenic.
>> +
>> config MFD_INTEL_QUARK_I2C_GPIO
>> tristate "Intel Quark MFD I2C GPIO"
>> depends on PCI
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index 52b1a90ff515..fb89e131ae98 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -180,6 +180,7 @@ obj-$(CONFIG_AB8500_CORE) += ab8500-core.o
>> ab8500-sysctrl.o
>> obj-$(CONFIG_MFD_TIMBERDALE) += timberdale.o
>> obj-$(CONFIG_PMIC_ADP5520) += adp5520.o
>> obj-$(CONFIG_MFD_KEMPLD) += kempld-core.o
>> +obj-$(CONFIG_INGENIC_TCU) += ingenic-tcu.o
>> obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO) += intel_quark_i2c_gpio.o
>> obj-$(CONFIG_LPC_SCH) += lpc_sch.o
>> obj-$(CONFIG_LPC_ICH) += lpc_ich.o
>> diff --git a/drivers/mfd/ingenic-tcu.c b/drivers/mfd/ingenic-tcu.c
>> new file mode 100644
>> index 000000000000..6c1d5e4310c1
>> --- /dev/null
>> +++ b/drivers/mfd/ingenic-tcu.c
>> @@ -0,0 +1,113 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * JZ47xx SoCs TCU MFD driver
>
> Nit: Another line here please.
>
>> + * Copyright (C) 2019 Paul Cercueil <paul@crapouillou.net>
>> + */
>> +
>> +#include <linux/mfd/ingenic-tcu.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +struct ingenic_soc_info {
>> + unsigned int num_channels;
>> +};
>> +
>> +static struct regmap *tcu_regmap __initdata;
>> +
>> +static const struct regmap_config ingenic_tcu_regmap_config = {
>> + .reg_bits = 32,
>> + .val_bits = 32,
>> + .reg_stride = 4,
>> + .max_register = TCU_REG_OST_CNTHBUF,
>> +};
>> +
>> +static const struct ingenic_soc_info jz4740_soc_info = {
>> + .num_channels = 8,
>> +};
>> +
>> +static const struct ingenic_soc_info jz4725b_soc_info = {
>> + .num_channels = 6,
>> +};
>> +
>> +static const struct of_device_id ingenic_tcu_of_match[] = {
>> + { .compatible = "ingenic,jz4740-tcu", .data = &jz4740_soc_info, },
>> + { .compatible = "ingenic,jz4725b-tcu", .data = &jz4725b_soc_info,
>> },
>> + { .compatible = "ingenic,jz4770-tcu", .data = &jz4740_soc_info, },
>> + { }
>> +};
>> +
>> +static struct regmap * __init ingenic_tcu_create_regmap(struct
>> device_node *np)
>> +{
>> + struct resource res;
>> + void __iomem *base;
>> + struct regmap *map;
>> +
>> + if (!of_match_node(ingenic_tcu_of_match, np))
>> + return ERR_PTR(-EINVAL);
>> +
>> + base = of_io_request_and_map(np, 0, "TCU");
>> + if (IS_ERR(base))
>> + return ERR_PTR(PTR_ERR(base));
>> +
>> + map = regmap_init_mmio(NULL, base, &ingenic_tcu_regmap_config);
>> + if (IS_ERR(map))
>> + goto err_iounmap;
>> +
>> + return map;
>> +
>> +err_iounmap:
>> + iounmap(base);
>> + of_address_to_resource(np, 0, &res);
>> + release_mem_region(res.start, resource_size(&res));
>> +
>> + return map;
>> +}
>
> Why does this need to be set-up earlier than probe()?
See the explanation below.
>> +static int __init ingenic_tcu_probe(struct platform_device *pdev)
>> +{
>> + struct regmap *map = ingenic_tcu_get_regmap(pdev->dev.of_node);
>> +
>> + platform_set_drvdata(pdev, map);
>> +
>> + regmap_attach_dev(&pdev->dev, map, &ingenic_tcu_regmap_config);
>> +
>> + return devm_of_platform_populate(&pdev->dev);
>> +}
>> +
>> +static struct platform_driver ingenic_tcu_driver = {
>> + .driver = {
>> + .name = "ingenic-tcu",
>> + .of_match_table = ingenic_tcu_of_match,
>> + },
>> +};
>> +
>> +static int __init ingenic_tcu_platform_init(void)
>> +{
>> + return platform_driver_probe(&ingenic_tcu_driver,
>> + ingenic_tcu_probe);
>
> What? Why?
The device driver probed here will populate the children devices,
which will be able to retrieve the pointer to the regmap through
device_get_regmap(dev->parent).
The children devices are normal platform drivers that can be probed
the normal way. These are the PWM driver, the watchdog driver, and the
OST (OS Timer) clocksource driver, all part of the same hardware block
(the Timer/Counter Unit or TCU).
>> +}
>> +subsys_initcall(ingenic_tcu_platform_init);
>> +
>> +struct regmap * __init ingenic_tcu_get_regmap(struct device_node
>> *np)
>> +{
>> + if (!tcu_regmap)
>> + tcu_regmap = ingenic_tcu_create_regmap(np);
>> +
>> + return tcu_regmap;
>> +}
>
> This makes me pretty uncomfortable.
>
> What calls it?
The TCU IRQ driver (patch [06/13]), clocks driver (patch [05/13]), and
the
non-OST clocksource driver (patch [07/13]) all probe very early in the
boot
process, and share the same devicetree node. They call this function to
get
a pointer to the regmap.
>> +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int
>> channel)
>> +{
>> + const struct ingenic_soc_info *soc =
>> device_get_match_data(dev->parent);
>> +
>> + /* Enable all TCU channels for PWM use by default except channels
>> 0/1 */
>> + u32 pwm_channels_mask = GENMASK(soc->num_channels - 1, 2);
>> +
>> + device_property_read_u32(dev->parent, "ingenic,pwm-channels-mask",
>> + &pwm_channels_mask);
>> +
>> + return !!(pwm_channels_mask & BIT(channel));
>> +}
>> +EXPORT_SYMBOL_GPL(ingenic_tcu_pwm_can_use_chn);
>> diff --git a/include/linux/mfd/ingenic-tcu.h
>> b/include/linux/mfd/ingenic-tcu.h
>> index 2083fa20821d..21df23916cd2 100644
>> --- a/include/linux/mfd/ingenic-tcu.h
>> +++ b/include/linux/mfd/ingenic-tcu.h
>> @@ -6,6 +6,11 @@
>> #define __LINUX_MFD_INGENIC_TCU_H_
>>
>> #include <linux/bitops.h>
>> +#include <linux/init.h>
>> +
>> +struct device;
>> +struct device_node;
>> +struct regmap;
>>
>> #define TCU_REG_WDT_TDR 0x00
>> #define TCU_REG_WDT_TCER 0x04
>> @@ -53,4 +58,7 @@
>> #define TCU_REG_TCNTc(c) (TCU_REG_TCNT0 + ((c) *
>> TCU_CHANNEL_STRIDE))
>> #define TCU_REG_TCSRc(c) (TCU_REG_TCSR0 + ((c) *
>> TCU_CHANNEL_STRIDE))
>>
>> +struct regmap * __init ingenic_tcu_get_regmap(struct device_node
>> *np);
>> +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int
>> channel);
>> +
>> #endif /* __LINUX_MFD_INGENIC_TCU_H_ */
>
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH] docs: move gcc_plugins.txt to core-api and rename to .rst
From: Mauro Carvalho Chehab @ 2019-06-26 13:47 UTC (permalink / raw)
To: Kees Cook
Cc: Mauro Carvalho Chehab, Linux Doc Mailing List,
Mauro Carvalho Chehab, linux-kernel, Jonathan Corbet, Emese Revfy
The gcc_plugins.txt file is already a ReST file. Move it
to the core-api book while renaming it.
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
Documentation/{gcc-plugins.txt => core-api/gcc-plugins.rst} | 0
Documentation/core-api/index.rst | 2 +-
2 files changed, 1 insertion(+), 1 deletion(-)
rename Documentation/{gcc-plugins.txt => core-api/gcc-plugins.rst} (100%)
diff --git a/Documentation/gcc-plugins.txt b/Documentation/core-api/gcc-plugins.rst
similarity index 100%
rename from Documentation/gcc-plugins.txt
rename to Documentation/core-api/gcc-plugins.rst
diff --git a/Documentation/core-api/index.rst b/Documentation/core-api/index.rst
index 2466a4c51031..d1e5b95bf86d 100644
--- a/Documentation/core-api/index.rst
+++ b/Documentation/core-api/index.rst
@@ -35,7 +35,7 @@ Core utilities
boot-time-mm
memory-hotplug
protection-keys
-
+ gcc-plugins
Interfaces for kernel debugging
===============================
--
2.21.0
^ permalink raw reply related
* [PATCH] docs: gpu: add msm-crash-dump.rst to the index.rst file
From: Mauro Carvalho Chehab @ 2019-06-26 13:36 UTC (permalink / raw)
To: Linux Doc Mailing List
Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, linux-kernel,
Jonathan Corbet, David Airlie, Daniel Vetter, Maarten Lankhorst,
Maxime Ripard, Sean Paul, dri-devel
This file is currently orphaned.
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
Documentation/gpu/index.rst | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/gpu/index.rst b/Documentation/gpu/index.rst
index 1fcf8e851e15..55f3f4294686 100644
--- a/Documentation/gpu/index.rst
+++ b/Documentation/gpu/index.rst
@@ -12,6 +12,7 @@ Linux GPU Driver Developer's Guide
drm-uapi
drm-client
drivers
+ msm-crash-dump
vga-switcheroo
vgaarbiter
todo
--
2.21.0
^ permalink raw reply related
* [PATCH] docs: filesystems: Remove uneeded .rst extension on toctables
From: Mauro Carvalho Chehab @ 2019-06-26 13:35 UTC (permalink / raw)
To: Linux Doc Mailing List
Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, linux-kernel,
Jonathan Corbet, Theodore Ts'o, Andreas Dilger, NeilBrown,
Darrick J. Wong, Matthew Wilcox, Christian Brauner, linux-ext4
There's no need to use a .rst on Sphinx toc tables. As most of
the Documentation don't use, remove the remaing occurrences.
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
Documentation/filesystems/ext4/index.rst | 8 ++++----
Documentation/filesystems/index.rst | 4 ++--
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/Documentation/filesystems/ext4/index.rst b/Documentation/filesystems/ext4/index.rst
index 3be3e54d480d..705d813d558f 100644
--- a/Documentation/filesystems/ext4/index.rst
+++ b/Documentation/filesystems/ext4/index.rst
@@ -8,7 +8,7 @@ ext4 Data Structures and Algorithms
:maxdepth: 6
:numbered:
- about.rst
- overview.rst
- globals.rst
- dynamic.rst
+ about
+ overview
+ globals
+ dynamic
diff --git a/Documentation/filesystems/index.rst b/Documentation/filesystems/index.rst
index 35644840a690..1651173f1118 100644
--- a/Documentation/filesystems/index.rst
+++ b/Documentation/filesystems/index.rst
@@ -17,7 +17,7 @@ algorithms work.
:maxdepth: 2
vfs
- path-lookup.rst
+ path-lookup
api-summary
splice
@@ -41,4 +41,4 @@ Documentation for individual filesystem types can be found here.
.. toctree::
:maxdepth: 2
- binderfs.rst
+ binderfs
--
2.21.0
^ permalink raw reply related
* Re: [PATCH v12 04/13] mfd: Add Ingenic TCU driver
From: Lee Jones @ 2019-06-26 13:18 UTC (permalink / raw)
To: Paul Cercueil
Cc: Rob Herring, Mark Rutland, Ralf Baechle, Paul Burton, James Hogan,
Jonathan Corbet, Daniel Lezcano, Thomas Gleixner,
Michael Turquette, Stephen Boyd, Jason Cooper, Marc Zyngier,
Mathieu Malaterre, linux-kernel, devicetree, linux-mips,
linux-doc, linux-clk, od
In-Reply-To: <20190521145141.9813-5-paul@crapouillou.net>
On Tue, 21 May 2019, Paul Cercueil wrote:
> This driver will provide a regmap that can be retrieved very early in
> the boot process through the API function ingenic_tcu_get_regmap().
>
> Additionally, it will call devm_of_platform_populate() so that all the
> children devices will be probed.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>
> Notes:
> v12: New patch
>
> drivers/mfd/Kconfig | 8 +++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/ingenic-tcu.c | 113 ++++++++++++++++++++++++++++++++
> include/linux/mfd/ingenic-tcu.h | 8 +++
> 4 files changed, 130 insertions(+)
> create mode 100644 drivers/mfd/ingenic-tcu.c
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 294d9567cc71..a13544474e05 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -494,6 +494,14 @@ config HTC_I2CPLD
> This device provides input and output GPIOs through an I2C
> interface to one or more sub-chips.
>
> +config INGENIC_TCU
> + bool "Ingenic Timer/Counter Unit (TCU) support"
> + depends on MIPS || COMPILE_TEST
> + select REGMAP_MMIO
> + help
> + Say yes here to support the Timer/Counter Unit (TCU) IP present
> + in the JZ47xx SoCs from Ingenic.
> +
> config MFD_INTEL_QUARK_I2C_GPIO
> tristate "Intel Quark MFD I2C GPIO"
> depends on PCI
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 52b1a90ff515..fb89e131ae98 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -180,6 +180,7 @@ obj-$(CONFIG_AB8500_CORE) += ab8500-core.o ab8500-sysctrl.o
> obj-$(CONFIG_MFD_TIMBERDALE) += timberdale.o
> obj-$(CONFIG_PMIC_ADP5520) += adp5520.o
> obj-$(CONFIG_MFD_KEMPLD) += kempld-core.o
> +obj-$(CONFIG_INGENIC_TCU) += ingenic-tcu.o
> obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO) += intel_quark_i2c_gpio.o
> obj-$(CONFIG_LPC_SCH) += lpc_sch.o
> obj-$(CONFIG_LPC_ICH) += lpc_ich.o
> diff --git a/drivers/mfd/ingenic-tcu.c b/drivers/mfd/ingenic-tcu.c
> new file mode 100644
> index 000000000000..6c1d5e4310c1
> --- /dev/null
> +++ b/drivers/mfd/ingenic-tcu.c
> @@ -0,0 +1,113 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * JZ47xx SoCs TCU MFD driver
Nit: Another line here please.
> + * Copyright (C) 2019 Paul Cercueil <paul@crapouillou.net>
> + */
> +
> +#include <linux/mfd/ingenic-tcu.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +struct ingenic_soc_info {
> + unsigned int num_channels;
> +};
> +
> +static struct regmap *tcu_regmap __initdata;
> +
> +static const struct regmap_config ingenic_tcu_regmap_config = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 4,
> + .max_register = TCU_REG_OST_CNTHBUF,
> +};
> +
> +static const struct ingenic_soc_info jz4740_soc_info = {
> + .num_channels = 8,
> +};
> +
> +static const struct ingenic_soc_info jz4725b_soc_info = {
> + .num_channels = 6,
> +};
> +
> +static const struct of_device_id ingenic_tcu_of_match[] = {
> + { .compatible = "ingenic,jz4740-tcu", .data = &jz4740_soc_info, },
> + { .compatible = "ingenic,jz4725b-tcu", .data = &jz4725b_soc_info, },
> + { .compatible = "ingenic,jz4770-tcu", .data = &jz4740_soc_info, },
> + { }
> +};
> +
> +static struct regmap * __init ingenic_tcu_create_regmap(struct device_node *np)
> +{
> + struct resource res;
> + void __iomem *base;
> + struct regmap *map;
> +
> + if (!of_match_node(ingenic_tcu_of_match, np))
> + return ERR_PTR(-EINVAL);
> +
> + base = of_io_request_and_map(np, 0, "TCU");
> + if (IS_ERR(base))
> + return ERR_PTR(PTR_ERR(base));
> +
> + map = regmap_init_mmio(NULL, base, &ingenic_tcu_regmap_config);
> + if (IS_ERR(map))
> + goto err_iounmap;
> +
> + return map;
> +
> +err_iounmap:
> + iounmap(base);
> + of_address_to_resource(np, 0, &res);
> + release_mem_region(res.start, resource_size(&res));
> +
> + return map;
> +}
Why does this need to be set-up earlier than probe()?
> +static int __init ingenic_tcu_probe(struct platform_device *pdev)
> +{
> + struct regmap *map = ingenic_tcu_get_regmap(pdev->dev.of_node);
> +
> + platform_set_drvdata(pdev, map);
> +
> + regmap_attach_dev(&pdev->dev, map, &ingenic_tcu_regmap_config);
> +
> + return devm_of_platform_populate(&pdev->dev);
> +}
> +
> +static struct platform_driver ingenic_tcu_driver = {
> + .driver = {
> + .name = "ingenic-tcu",
> + .of_match_table = ingenic_tcu_of_match,
> + },
> +};
> +
> +static int __init ingenic_tcu_platform_init(void)
> +{
> + return platform_driver_probe(&ingenic_tcu_driver,
> + ingenic_tcu_probe);
What? Why?
> +}
> +subsys_initcall(ingenic_tcu_platform_init);
> +
> +struct regmap * __init ingenic_tcu_get_regmap(struct device_node *np)
> +{
> + if (!tcu_regmap)
> + tcu_regmap = ingenic_tcu_create_regmap(np);
> +
> + return tcu_regmap;
> +}
This makes me pretty uncomfortable.
What calls it?
> +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int channel)
> +{
> + const struct ingenic_soc_info *soc = device_get_match_data(dev->parent);
> +
> + /* Enable all TCU channels for PWM use by default except channels 0/1 */
> + u32 pwm_channels_mask = GENMASK(soc->num_channels - 1, 2);
> +
> + device_property_read_u32(dev->parent, "ingenic,pwm-channels-mask",
> + &pwm_channels_mask);
> +
> + return !!(pwm_channels_mask & BIT(channel));
> +}
> +EXPORT_SYMBOL_GPL(ingenic_tcu_pwm_can_use_chn);
> diff --git a/include/linux/mfd/ingenic-tcu.h b/include/linux/mfd/ingenic-tcu.h
> index 2083fa20821d..21df23916cd2 100644
> --- a/include/linux/mfd/ingenic-tcu.h
> +++ b/include/linux/mfd/ingenic-tcu.h
> @@ -6,6 +6,11 @@
> #define __LINUX_MFD_INGENIC_TCU_H_
>
> #include <linux/bitops.h>
> +#include <linux/init.h>
> +
> +struct device;
> +struct device_node;
> +struct regmap;
>
> #define TCU_REG_WDT_TDR 0x00
> #define TCU_REG_WDT_TCER 0x04
> @@ -53,4 +58,7 @@
> #define TCU_REG_TCNTc(c) (TCU_REG_TCNT0 + ((c) * TCU_CHANNEL_STRIDE))
> #define TCU_REG_TCSRc(c) (TCU_REG_TCSR0 + ((c) * TCU_CHANNEL_STRIDE))
>
> +struct regmap * __init ingenic_tcu_get_regmap(struct device_node *np);
> +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int channel);
> +
> #endif /* __LINUX_MFD_INGENIC_TCU_H_ */
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v2 3/5] perf: stm32: ddrperfm driver creation
From: Mark Rutland @ 2019-06-26 12:25 UTC (permalink / raw)
To: Gerald BAEZA
Cc: Will Deacon, robh+dt@kernel.org, mcoquelin.stm32@gmail.com,
Alexandre TORGUE, corbet@lwn.net, linux@armlinux.org.uk,
olof@lixom.net, horms+renesas@verge.net.au, arnd@arndb.de,
linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
linux-stm32@st-md-mailman.stormreply.com,
linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org
In-Reply-To: <20190626122228.GB20313@lakrids.cambridge.arm.com>
Hi,
Sorry for the (quite horrific) mangling my mailserver has inflicted via
my reply. Obviously, the confidentiality disclaimer is bogus, too.
I'll make sure I use the right SMTP server in future.
Mark.
On Wed, Jun 26, 2019 at 12:22:31PM +0000, Mark Rutland wrote:
> On Mon, May 20, 2019 at 03:27:17PM +0000, Gerald BAEZA wrote:
> > The DDRPERFM is the DDR Performance Monitor embedded in STM32MP1 SOC.
> >
> > This perf drivers supports the read, write, activate, idle and total
> > time counters, described in the reference manual RM0436.
>
> Is this document publicly accessible anywhere?
>
> If so, could you please provide a link?
>
> > A 'bandwidth' attribute is added in the 'ddrperfm' event_source in order
> > to directly get the read and write bandwidths (in MB/s), from the last
> > read, write and total time counters reading.
> > This attribute is aside the 'events' attributes group because it is not
> > a counter, as seen by perf tool.
>
> This should be removed. This is unusually stateful, and this can be
> calculated in userspace by using the events. I'm also not keen on
> creating a precedent for weird sysfs bits for PMUs.
>
> > Signed-off-by: Gerald Baeza <gerald.baeza@st.com>
> > ---
> > drivers/perf/Kconfig | 6 +
> > drivers/perf/Makefile | 1 +
> > drivers/perf/stm32_ddr_pmu.c | 512 +++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 519 insertions(+)
> > create mode 100644 drivers/perf/stm32_ddr_pmu.c
> >
> > diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> > index a94e586..9add8a7 100644
> > --- a/drivers/perf/Kconfig
> > +++ b/drivers/perf/Kconfig
> > @@ -105,6 +105,12 @@ config THUNDERX2_PMU
> > The SoC has PMU support in its L3 cache controller (L3C) and
> > in the DDR4 Memory Controller (DMC).
> >
> > +config STM32_DDR_PMU
> > + tristate "STM32 DDR PMU"
> > + depends on MACH_STM32MP157
> > + help
> > + Support for STM32 DDR performance monitor (DDRPERFM).
> > +
> > config XGENE_PMU
> > depends on ARCH_XGENE
> > bool "APM X-Gene SoC PMU"
> > diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
> > index 3048994..fa64719 100644
> > --- a/drivers/perf/Makefile
> > +++ b/drivers/perf/Makefile
> > @@ -8,6 +8,7 @@ obj-$(CONFIG_ARM_SMMU_V3_PMU) += arm_smmuv3_pmu.o
> > obj-$(CONFIG_HISI_PMU) += hisilicon/
> > obj-$(CONFIG_QCOM_L2_PMU)+= qcom_l2_pmu.o
> > obj-$(CONFIG_QCOM_L3_PMU) += qcom_l3_pmu.o
> > +obj-$(CONFIG_STM32_DDR_PMU) += stm32_ddr_pmu.o
> > obj-$(CONFIG_THUNDERX2_PMU) += thunderx2_pmu.o
> > obj-$(CONFIG_XGENE_PMU) += xgene_pmu.o
> > obj-$(CONFIG_ARM_SPE_PMU) += arm_spe_pmu.o
> > diff --git a/drivers/perf/stm32_ddr_pmu.c b/drivers/perf/stm32_ddr_pmu.c
> > new file mode 100644
> > index 0000000..ae4a813
> > --- /dev/null
> > +++ b/drivers/perf/stm32_ddr_pmu.c
> > @@ -0,0 +1,512 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * This file is the STM32 DDR performance monitor (DDRPERFM) driver
> > + *
> > + * Copyright (C) 2019, STMicroelectronics - All Rights Reserved
> > + * Author: Gerald Baeza <gerald.baeza@st.com>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/hrtimer.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/perf_event.h>
> > +#include <linux/reset.h>
> > +#include <linux/slab.h>
> > +#include <linux/types.h>
> > +
> > +#define POLL_MS4000/* The counter roll over after ~8s @533MHz */
>
> I take it this is because there's no IRQ? If so, please expand the
> comment to call that out, e.g.
>
> /*
> * The PMU has no overflow IRQ, so we must poll to avoid losing events.
> * The fastest counter can overflow in ~8s @533MHz, so we poll in 4s
> * intervals to ensure we don't miss a rollover.
> */
> #define POLL_MS4000
>
> Which clock specifically is operating at 533MHz, and is this something
> that may vary? Is it possible that this may go higher in future?
>
> > +#define WORD_LENGTH4/* Bytes */
> > +#define BURST_LENGTH8/* Words */
> > +
> > +#define DDRPERFM_CTL0x000
> > +#define DDRPERFM_CFG0x004
> > +#define DDRPERFM_STATUS 0x008
> > +#define DDRPERFM_CCR0x00C
> > +#define DDRPERFM_IER0x010
> > +#define DDRPERFM_ISR0x014
> > +#define DDRPERFM_ICR0x018
> > +#define DDRPERFM_TCNT0x020
> > +#define DDRPERFM_CNT(X)(0x030 + 8 * (X))
> > +#define DDRPERFM_HWCFG0x3F0
> > +#define DDRPERFM_VER0x3F4
> > +#define DDRPERFM_ID0x3F8
> > +#define DDRPERFM_SID0x3FC
> > +
> > +#define CTL_START0x00000001
> > +#define CTL_STOP0x00000002
> > +#define CCR_CLEAR_ALL0x8000000F
> > +#define SID_MAGIC_ID0xA3C5DD01
> > +
> > +#define STRING "Read = %llu, Write = %llu, Read & Write = %llu (MB/s)\n"
>
> If you need this, please expand it in-place. As-is, this is needless
> obfuscation.
>
> > +
> > +enum {
> > +READ_CNT,
> > +WRITE_CNT,
> > +ACTIVATE_CNT,
> > +IDLE_CNT,
> > +TIME_CNT,
> > +PMU_NR_COUNTERS
> > +};
>
> I take it these are fixed-purpose counters in the hardware?
>
> > +
> > +struct stm32_ddr_pmu {
> > +struct pmu pmu;
> > +void __iomem *membase;
> > +struct clk *clk;
> > +struct clk *clk_ddr;
> > +unsigned long clk_ddr_rate;
> > +struct hrtimer hrtimer;
> > +ktime_t poll_period;
> > +spinlock_t lock; /* for shared registers access */
>
> All accesses to a PMU instance should be serialized on the same CPU,
> so you shouldn't need a lock (though you will need to disable IRQs to
> serialize with the interrupt handler).
>
> The perf subsystem cannot sanely access the PMU across multiple CPUs.
>
> > +struct perf_event *events[PMU_NR_COUNTERS];
> > +u64 events_cnt[PMU_NR_COUNTERS];
> > +};
> > +
> > +static inline struct stm32_ddr_pmu *pmu_to_stm32_ddr_pmu(struct pmu *p)
> > +{
> > +return container_of(p, struct stm32_ddr_pmu, pmu);
> > +}
> > +
> > +static inline struct stm32_ddr_pmu *hrtimer_to_stm32_ddr_pmu(struct hrtimer *h)
> > +{
> > +return container_of(h, struct stm32_ddr_pmu, hrtimer);
> > +}
> > +
> > +static u64 stm32_ddr_pmu_compute_bw(struct stm32_ddr_pmu *stm32_ddr_pmu,
> > + int counter)
> > +{
> > +u64 bw = stm32_ddr_pmu->events_cnt[counter], tmp;
> > +u64 div = stm32_ddr_pmu->events_cnt[TIME_CNT];
> > +u32 prediv = 1, premul = 1;
> > +
> > +if (bw && div) {
> > +/* Maximize the dividend into 64 bits */
> > +while ((bw < 0x8000000000000000ULL) &&
> > + (premul < 0x40000000UL)) {
> > +bw = bw << 1;
> > +premul *= 2;
> > +}
> > +/* Minimize the dividor to fit in 32 bits */
> > +while ((div > 0xffffffffUL) && (prediv < 0x40000000UL)) {
> > +div = div >> 1;
> > +prediv *= 2;
> > +}
> > +/* Divide counter per time and multiply per DDR settings */
> > +do_div(bw, div);
> > +tmp = bw * BURST_LENGTH * WORD_LENGTH;
> > +tmp *= stm32_ddr_pmu->clk_ddr_rate;
> > +if (tmp < bw)
> > +goto error;
> > +bw = tmp;
> > +/* Cancel the prediv and premul factors */
> > +while (prediv > 1) {
> > +bw = bw >> 1;
> > +prediv /= 2;
> > +}
> > +while (premul > 1) {
> > +bw = bw >> 1;
> > +premul /= 2;
> > +}
> > +/* Convert MHz to Hz and B to MB, to finally get MB/s */
> > +tmp = bw * 1000000;
> > +if (tmp < bw)
> > +goto error;
> > +bw = tmp;
> > +premul = 1024 * 1024;
> > +while (premul > 1) {
> > +bw = bw >> 1;
> > +premul /= 2;
> > +}
> > +}
> > +return bw;
> > +
> > +error:
> > +pr_warn("stm32-ddr-pmu: overflow detected\n");
> > +return 0;
> > +}
>
> IIUC this is for the stateful sysfs stuff, which should be removed.
>
> > +
> > +static void stm32_ddr_pmu_event_configure(struct perf_event *event)
> > +{
> > +struct stm32_ddr_pmu *stm32_ddr_pmu = pmu_to_stm32_ddr_pmu(event->pmu);
> > +unsigned long lock_flags, config_base = event->hw.config_base;
> > +u32 val;
> > +
> > +spin_lock_irqsave(&stm32_ddr_pmu->lock, lock_flags);
> > +writel_relaxed(CTL_STOP, stm32_ddr_pmu->membase + DDRPERFM_CTL);
> > +
> > +if (config_base < TIME_CNT) {
> > +val = readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_CFG);
> > +val |= (1 << config_base);
> > +writel_relaxed(val, stm32_ddr_pmu->membase + DDRPERFM_CFG);
> > +}
> > +spin_unlock_irqrestore(&stm32_ddr_pmu->lock, lock_flags);
> > +}
>
> You don't need the lock here. This is called from your pmu::{start,add}
> callbacks, and the perf core already ensures those are serialized (via
> the context lock), and called with IRQs disabled.
>
> > +
> > +static void stm32_ddr_pmu_event_read(struct perf_event *event)
> > +{
> > +struct stm32_ddr_pmu *stm32_ddr_pmu = pmu_to_stm32_ddr_pmu(event->pmu);
> > +unsigned long flags, config_base = event->hw.config_base;
> > +struct hw_perf_event *hw = &event->hw;
> > +u64 prev_count, new_count, mask;
> > +u32 val, offset, bit;
> > +
> > +spin_lock_irqsave(&stm32_ddr_pmu->lock, flags);
> > +
> > +writel_relaxed(CTL_STOP, stm32_ddr_pmu->membase + DDRPERFM_CTL);
> > +
> > +if (config_base == TIME_CNT) {
> > +offset = DDRPERFM_TCNT;
> > +bit = 1 << 31;
> > +} else {
> > +offset = DDRPERFM_CNT(config_base);
> > +bit = 1 << config_base;
> > +}
> > +val = readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_STATUS);
> > +if (val & bit)
> > +pr_warn("stm32_ddr_pmu hardware overflow\n");
> > +val = readl_relaxed(stm32_ddr_pmu->membase + offset);
> > +writel_relaxed(bit, stm32_ddr_pmu->membase + DDRPERFM_CCR);
> > +writel_relaxed(CTL_START, stm32_ddr_pmu->membase + DDRPERFM_CTL);
> > +
> > +do {
> > +prev_count = local64_read(&hw->prev_count);
> > +new_count = prev_count + val;
> > +} while (local64_xchg(&hw->prev_count, new_count) != prev_count);
> > +
> > +mask = GENMASK_ULL(31, 0);
> > +local64_add(val & mask, &event->count);
> > +
> > +if (new_count < prev_count)
> > +pr_warn("STM32 DDR PMU counter saturated\n");
>
> Do the counter saturate, or do they roll-over?
>
> I think that the message is misleading here, but I just want to check.
>
> > +
> > +spin_unlock_irqrestore(&stm32_ddr_pmu->lock, flags);
> > +}
> > +
> > +static void stm32_ddr_pmu_event_start(struct perf_event *event, int flags)
> > +{
> > +struct stm32_ddr_pmu *stm32_ddr_pmu = pmu_to_stm32_ddr_pmu(event->pmu);
> > +struct hw_perf_event *hw = &event->hw;
> > +unsigned long lock_flags;
> > +
> > +if (WARN_ON_ONCE(!(hw->state & PERF_HES_STOPPED)))
> > +return;
> > +
> > +if (flags & PERF_EF_RELOAD)
> > +WARN_ON_ONCE(!(hw->state & PERF_HES_UPTODATE));
> > +
> > +stm32_ddr_pmu_event_configure(event);
> > +
> > +/* Clear all counters to synchronize them, then start */
> > +spin_lock_irqsave(&stm32_ddr_pmu->lock, lock_flags);
> > +writel_relaxed(CCR_CLEAR_ALL, stm32_ddr_pmu->membase + DDRPERFM_CCR);
> > +writel_relaxed(CTL_START, stm32_ddr_pmu->membase + DDRPERFM_CTL);
> > +spin_unlock_irqrestore(&stm32_ddr_pmu->lock, lock_flags);
>
> By 'clear' do you mean set the counters to zero?
>
> Or is there a control bit that determine whether they count?
>
> If we're updating the counter, we could update prev_count at the same
> instant.
>
> > +
> > +hw->state = 0;
> > +}
> > +
> > +static void stm32_ddr_pmu_event_stop(struct perf_event *event, int flags)
> > +{
> > +struct stm32_ddr_pmu *stm32_ddr_pmu = pmu_to_stm32_ddr_pmu(event->pmu);
> > +unsigned long lock_flags, config_base = event->hw.config_base;
> > +struct hw_perf_event *hw = &event->hw;
> > +u32 val, bit;
> > +
> > +if (WARN_ON_ONCE(hw->state & PERF_HES_STOPPED))
> > +return;
> > +
> > +spin_lock_irqsave(&stm32_ddr_pmu->lock, lock_flags);
> > +writel_relaxed(CTL_STOP, stm32_ddr_pmu->membase + DDRPERFM_CTL);
> > +if (config_base == TIME_CNT)
> > +bit = 1 << 31;
> > +else
> > +bit = 1 << config_base;
> > +writel_relaxed(bit, stm32_ddr_pmu->membase + DDRPERFM_CCR);
> > +if (config_base < TIME_CNT) {
> > +val = readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_CFG);
> > +val &= ~bit;
> > +writel_relaxed(val, stm32_ddr_pmu->membase + DDRPERFM_CFG);
> > +}
> > +spin_unlock_irqrestore(&stm32_ddr_pmu->lock, lock_flags);
> > +
> > +hw->state |= PERF_HES_STOPPED;
> > +
> > +if (flags & PERF_EF_UPDATE) {
> > +stm32_ddr_pmu_event_read(event);
> > +hw->state |= PERF_HES_UPTODATE;
> > +}
> > +}
> > +
> > +static int stm32_ddr_pmu_event_add(struct perf_event *event, int flags)
> > +{
> > +struct stm32_ddr_pmu *stm32_ddr_pmu = pmu_to_stm32_ddr_pmu(event->pmu);
> > +unsigned long config_base = event->hw.config_base;
> > +struct hw_perf_event *hw = &event->hw;
> > +
> > +stm32_ddr_pmu->events_cnt[config_base] = 0;
> > +stm32_ddr_pmu->events[config_base] = event;
> > +
> > +clk_enable(stm32_ddr_pmu->clk);
> > +hrtimer_start(&stm32_ddr_pmu->hrtimer, stm32_ddr_pmu->poll_period,
> > + HRTIMER_MODE_REL);
> > +
> > +stm32_ddr_pmu_event_configure(event);
> > +
> > +hw->state = PERF_HES_STOPPED | PERF_HES_UPTODATE;
> > +
> > +if (flags & PERF_EF_START)
> > +stm32_ddr_pmu_event_start(event, 0);
> > +
> > +return 0;
> > +}
> > +
> > +static void stm32_ddr_pmu_event_del(struct perf_event *event, int flags)
> > +{
> > +struct stm32_ddr_pmu *stm32_ddr_pmu = pmu_to_stm32_ddr_pmu(event->pmu);
> > +unsigned long config_base = event->hw.config_base;
> > +bool stop = true;
> > +int i;
> > +
> > +stm32_ddr_pmu_event_stop(event, PERF_EF_UPDATE);
> > +
> > +stm32_ddr_pmu->events_cnt[config_base] += local64_read(&event->count);
> > +stm32_ddr_pmu->events[config_base] = NULL;
> > +
> > +for (i = 0; i < PMU_NR_COUNTERS; i++)
> > +if (stm32_ddr_pmu->events[i])
> > +stop = false;
> > +if (stop)
> > +hrtimer_cancel(&stm32_ddr_pmu->hrtimer);
> > +
> > +clk_disable(stm32_ddr_pmu->clk);
>
> This doesn't look right. If I add two events A and B, then delete event
> A, surely we want the clock on for event B?
>
> Does the clock only affect whether the counters count, or does it also
> affect whether the register file is usable?
>
> > +}
> > +
> > +static int stm32_ddr_pmu_event_init(struct perf_event *event)
> > +{
> > +struct hw_perf_event *hw = &event->hw;
> > +
> > +if (is_sampling_event(event))
> > +return -EINVAL;
> > +
> > +if (event->attach_state & PERF_ATTACH_TASK)
> > +return -EINVAL;
> > +
> > +if (event->attr.exclude_user ||
> > + event->attr.exclude_kernel ||
> > + event->attr.exclude_hv ||
> > + event->attr.exclude_idle ||
> > + event->attr.exclude_host ||
> > + event->attr.exclude_guest)
> > +return -EINVAL;
> > +
> > +if (event->cpu < 0)
> > +return -EINVAL;
>
> For a system PMU like this, you must ensure that all events are assigned
> to the same CPU.
>
> You'll need to designate some arbitrary CPU to mange the PMU, expose
> that under sysfs, and upon hotplug events you must choose a new CPU and
> migrate existing events.
>
> For a simple example, see arch/arm/mm/cache-l2x0-pmu.c.
>
> > +
> > +hw->config_base = event->attr.config;
> > +
> > +return 0;
> > +}
> > +
> > +static enum hrtimer_restart stm32_ddr_pmu_poll(struct hrtimer *hrtimer)
> > +{
> > +struct stm32_ddr_pmu *stm32_ddr_pmu = hrtimer_to_stm32_ddr_pmu(hrtimer);
> > +int i;
> > +
> > +for (i = 0; i < PMU_NR_COUNTERS; i++)
> > +if (stm32_ddr_pmu->events[i])
> > +stm32_ddr_pmu_event_read(stm32_ddr_pmu->events[i]);
> > +
> > +hrtimer_forward_now(hrtimer, stm32_ddr_pmu->poll_period);
> > +
> > +return HRTIMER_RESTART;
> > +}
> > +
> > +static ssize_t stm32_ddr_pmu_sysfs_show(struct device *dev,
> > +struct device_attribute *attr,
> > +char *buf)
> > +{
> > +struct dev_ext_attribute *eattr;
> > +
> > +eattr = container_of(attr, struct dev_ext_attribute, attr);
> > +
> > +return sprintf(buf, "config=0x%lx\n", (unsigned long)eattr->var);
> > +}
> > +
> > +static ssize_t bandwidth_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > +struct stm32_ddr_pmu *stm32_ddr_pmu = dev_get_drvdata(dev);
> > +u64 r_bw, w_bw;
> > +int ret;
> > +
> > +if (stm32_ddr_pmu->events_cnt[TIME_CNT]) {
> > +r_bw = stm32_ddr_pmu_compute_bw(stm32_ddr_pmu, READ_CNT);
> > +w_bw = stm32_ddr_pmu_compute_bw(stm32_ddr_pmu, WRITE_CNT);
> > +
> > +ret = snprintf(buf, PAGE_SIZE, STRING,
> > + r_bw, w_bw, (r_bw + w_bw));
> > +} else {
> > +ret = snprintf(buf, PAGE_SIZE, "No data available\n");
> > +}
> > +
> > +return ret;
> > +}
>
> As commented above, this should not be exposed under sysfs. It doesn't
> follow the usual ABI rules, it's weirdly stateful, and it's redundant.
>
> > +
> > +#define STM32_DDR_PMU_ATTR(_name, _func, _config)\
> > +(&((struct dev_ext_attribute[]) {\
> > +{ __ATTR(_name, 0444, _func, NULL), (void *)_config } \
> > +})[0].attr.attr)
> > +
> > +#define STM32_DDR_PMU_EVENT_ATTR(_name, _config)\
> > +STM32_DDR_PMU_ATTR(_name, stm32_ddr_pmu_sysfs_show,\
> > + (unsigned long)_config)
> > +
> > +static struct attribute *stm32_ddr_pmu_event_attrs[] = {
> > +STM32_DDR_PMU_EVENT_ATTR(read_cnt, READ_CNT),
> > +STM32_DDR_PMU_EVENT_ATTR(write_cnt, WRITE_CNT),
> > +STM32_DDR_PMU_EVENT_ATTR(activate_cnt, ACTIVATE_CNT),
> > +STM32_DDR_PMU_EVENT_ATTR(idle_cnt, IDLE_CNT),
> > +STM32_DDR_PMU_EVENT_ATTR(time_cnt, TIME_CNT),
> > +NULL
> > +};
> > +
> > +static DEVICE_ATTR_RO(bandwidth);
> > +static struct attribute *stm32_ddr_pmu_bandwidth_attrs[] = {
> > +&dev_attr_bandwidth.attr,
> > +NULL,
> > +};
> > +
> > +static struct attribute_group stm32_ddr_pmu_event_attrs_group = {
> > +.name = "events",
> > +.attrs = stm32_ddr_pmu_event_attrs,
> > +};
> > +
> > +static struct attribute_group stm32_ddr_pmu_bandwidth_attrs_group = {
> > +.attrs = stm32_ddr_pmu_bandwidth_attrs,
> > +};
> > +
> > +static const struct attribute_group *stm32_ddr_pmu_attr_groups[] = {
> > +&stm32_ddr_pmu_event_attrs_group,
> > +&stm32_ddr_pmu_bandwidth_attrs_group,
> > +NULL,
> > +};
> > +
> > +static int stm32_ddr_pmu_device_probe(struct platform_device *pdev)
> > +{
> > +struct stm32_ddr_pmu *stm32_ddr_pmu;
> > +struct reset_control *rst;
> > +struct resource *res;
> > +int i, ret;
> > +u32 val;
> > +
> > +stm32_ddr_pmu = devm_kzalloc(&pdev->dev, sizeof(struct stm32_ddr_pmu),
> > + GFP_KERNEL);
> > +if (!stm32_ddr_pmu)
> > +return -ENOMEM;
> > +platform_set_drvdata(pdev, stm32_ddr_pmu);
> > +
> > +res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +stm32_ddr_pmu->membase = devm_ioremap_resource(&pdev->dev, res);
> > +if (IS_ERR(stm32_ddr_pmu->membase)) {
> > +pr_warn("Unable to get STM32 DDR PMU membase\n");
> > +return PTR_ERR(stm32_ddr_pmu->membase);
> > +}
> > +
> > +stm32_ddr_pmu->clk = devm_clk_get(&pdev->dev, "bus");
> > +if (IS_ERR(stm32_ddr_pmu->clk)) {
> > +pr_warn("Unable to get STM32 DDR PMU clock\n");
> > +return PTR_ERR(stm32_ddr_pmu->clk);
> > +}
> > +
> > +ret = clk_prepare_enable(stm32_ddr_pmu->clk);
> > +if (ret) {
> > +pr_warn("Unable to prepare STM32 DDR PMU clock\n");
> > +return ret;
> > +}
> > +
> > +stm32_ddr_pmu->clk_ddr = devm_clk_get(&pdev->dev, "ddr");
> > +if (IS_ERR(stm32_ddr_pmu->clk_ddr)) {
> > +pr_warn("Unable to get STM32 DDR clock\n");
> > +return PTR_ERR(stm32_ddr_pmu->clk_ddr);
> > +}
> > +stm32_ddr_pmu->clk_ddr_rate = clk_get_rate(stm32_ddr_pmu->clk_ddr);
> > +stm32_ddr_pmu->clk_ddr_rate /= 1000000;
> > +
> > +stm32_ddr_pmu->poll_period = ms_to_ktime(POLL_MS);
> > +hrtimer_init(&stm32_ddr_pmu->hrtimer, CLOCK_MONOTONIC,
> > + HRTIMER_MODE_REL);
> > +stm32_ddr_pmu->hrtimer.function = stm32_ddr_pmu_poll;
> > +spin_lock_init(&stm32_ddr_pmu->lock);
> > +
> > +for (i = 0; i < PMU_NR_COUNTERS; i++) {
> > +stm32_ddr_pmu->events[i] = NULL;
> > +stm32_ddr_pmu->events_cnt[i] = 0;
> > +}
> > +
> > +val = readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_SID);
> > +if (val != SID_MAGIC_ID)
> > +return -EINVAL;
> > +
> > +stm32_ddr_pmu->pmu = (struct pmu) {
> > +.task_ctx_nr = perf_invalid_context,
> > +.start = stm32_ddr_pmu_event_start,
> > +.stop = stm32_ddr_pmu_event_stop,
> > +.add = stm32_ddr_pmu_event_add,
> > +.del = stm32_ddr_pmu_event_del,
> > +.event_init = stm32_ddr_pmu_event_init,
> > +.attr_groups = stm32_ddr_pmu_attr_groups,
> > +};
> > +ret = perf_pmu_register(&stm32_ddr_pmu->pmu, "ddrperfm", -1);
>
> Please give this a better name. The usual convention is to use "_pmu" as
> the suffix, so we should use that rather than "perfm".
>
> To be unambiguous, something like "stm32_ddr_pmu" would be good.
>
> Thanks,
> Mark.
>
> > +if (ret) {
> > +pr_warn("Unable to register STM32 DDR PMU\n");
> > +return ret;
> > +}
> > +
> > +rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> > +if (!IS_ERR(rst)) {
> > +reset_control_assert(rst);
> > +udelay(2);
> > +reset_control_deassert(rst);
> > +}
> > +
> > +pr_info("stm32-ddr-pmu: probed (ID=0x%08x VER=0x%08x), DDR@%luMHz\n",
> > +readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_ID),
> > +readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_VER),
> > +stm32_ddr_pmu->clk_ddr_rate);
> > +
> > +clk_disable(stm32_ddr_pmu->clk);
> > +
> > +return 0;
> > +}
> > +
> > +static int stm32_ddr_pmu_device_remove(struct platform_device *pdev)
> > +{
> > +struct stm32_ddr_pmu *stm32_ddr_pmu = platform_get_drvdata(pdev);
> > +
> > +perf_pmu_unregister(&stm32_ddr_pmu->pmu);
> > +
> > +return 0;
> > +}
> > +
> > +static const struct of_device_id stm32_ddr_pmu_of_match[] = {
> > +{ .compatible = "st,stm32-ddr-pmu" },
> > +{ },
> > +};
> > +
> > +static struct platform_driver stm32_ddr_pmu_driver = {
> > +.driver = {
> > +.name= "stm32-ddr-pmu",
> > +.of_match_table = of_match_ptr(stm32_ddr_pmu_of_match),
> > +},
> > +.probe = stm32_ddr_pmu_device_probe,
> > +.remove = stm32_ddr_pmu_device_remove,
> > +};
> > +
> > +module_platform_driver(stm32_ddr_pmu_driver);
> > +
> > +MODULE_DESCRIPTION("Perf driver for STM32 DDR performance monitor");
> > +MODULE_AUTHOR("Gerald Baeza <gerald.baeza@st.com>");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.7.4
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply
* Re: [PATCH v2 3/5] perf: stm32: ddrperfm driver creation
From: Mark Rutland @ 2019-06-26 12:22 UTC (permalink / raw)
To: Gerald BAEZA
Cc: Will Deacon, robh+dt@kernel.org, mcoquelin.stm32@gmail.com,
Alexandre TORGUE, corbet@lwn.net, linux@armlinux.org.uk,
olof@lixom.net, horms+renesas@verge.net.au, arnd@arndb.de,
linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
linux-stm32@st-md-mailman.stormreply.com,
linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org
In-Reply-To: <1558366019-24214-4-git-send-email-gerald.baeza@st.com>
On Mon, May 20, 2019 at 03:27:17PM +0000, Gerald BAEZA wrote:
> The DDRPERFM is the DDR Performance Monitor embedded in STM32MP1 SOC.
>
> This perf drivers supports the read, write, activate, idle and total
> time counters, described in the reference manual RM0436.
Is this document publicly accessible anywhere?
If so, could you please provide a link?
> A 'bandwidth' attribute is added in the 'ddrperfm' event_source in order
> to directly get the read and write bandwidths (in MB/s), from the last
> read, write and total time counters reading.
> This attribute is aside the 'events' attributes group because it is not
> a counter, as seen by perf tool.
This should be removed. This is unusually stateful, and this can be
calculated in userspace by using the events. I'm also not keen on
creating a precedent for weird sysfs bits for PMUs.
> Signed-off-by: Gerald Baeza <gerald.baeza@st.com>
> ---
> drivers/perf/Kconfig | 6 +
> drivers/perf/Makefile | 1 +
> drivers/perf/stm32_ddr_pmu.c | 512 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 519 insertions(+)
> create mode 100644 drivers/perf/stm32_ddr_pmu.c
>
> diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> index a94e586..9add8a7 100644
> --- a/drivers/perf/Kconfig
> +++ b/drivers/perf/Kconfig
> @@ -105,6 +105,12 @@ config THUNDERX2_PMU
> The SoC has PMU support in its L3 cache controller (L3C) and
> in the DDR4 Memory Controller (DMC).
>
> +config STM32_DDR_PMU
> + tristate "STM32 DDR PMU"
> + depends on MACH_STM32MP157
> + help
> + Support for STM32 DDR performance monitor (DDRPERFM).
> +
> config XGENE_PMU
> depends on ARCH_XGENE
> bool "APM X-Gene SoC PMU"
> diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
> index 3048994..fa64719 100644
> --- a/drivers/perf/Makefile
> +++ b/drivers/perf/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_ARM_SMMU_V3_PMU) += arm_smmuv3_pmu.o
> obj-$(CONFIG_HISI_PMU) += hisilicon/
> obj-$(CONFIG_QCOM_L2_PMU)+= qcom_l2_pmu.o
> obj-$(CONFIG_QCOM_L3_PMU) += qcom_l3_pmu.o
> +obj-$(CONFIG_STM32_DDR_PMU) += stm32_ddr_pmu.o
> obj-$(CONFIG_THUNDERX2_PMU) += thunderx2_pmu.o
> obj-$(CONFIG_XGENE_PMU) += xgene_pmu.o
> obj-$(CONFIG_ARM_SPE_PMU) += arm_spe_pmu.o
> diff --git a/drivers/perf/stm32_ddr_pmu.c b/drivers/perf/stm32_ddr_pmu.c
> new file mode 100644
> index 0000000..ae4a813
> --- /dev/null
> +++ b/drivers/perf/stm32_ddr_pmu.c
> @@ -0,0 +1,512 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * This file is the STM32 DDR performance monitor (DDRPERFM) driver
> + *
> + * Copyright (C) 2019, STMicroelectronics - All Rights Reserved
> + * Author: Gerald Baeza <gerald.baeza@st.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/hrtimer.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/perf_event.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +#define POLL_MS4000/* The counter roll over after ~8s @533MHz */
I take it this is because there's no IRQ? If so, please expand the
comment to call that out, e.g.
/*
* The PMU has no overflow IRQ, so we must poll to avoid losing events.
* The fastest counter can overflow in ~8s @533MHz, so we poll in 4s
* intervals to ensure we don't miss a rollover.
*/
#define POLL_MS4000
Which clock specifically is operating at 533MHz, and is this something
that may vary? Is it possible that this may go higher in future?
> +#define WORD_LENGTH4/* Bytes */
> +#define BURST_LENGTH8/* Words */
> +
> +#define DDRPERFM_CTL0x000
> +#define DDRPERFM_CFG0x004
> +#define DDRPERFM_STATUS 0x008
> +#define DDRPERFM_CCR0x00C
> +#define DDRPERFM_IER0x010
> +#define DDRPERFM_ISR0x014
> +#define DDRPERFM_ICR0x018
> +#define DDRPERFM_TCNT0x020
> +#define DDRPERFM_CNT(X)(0x030 + 8 * (X))
> +#define DDRPERFM_HWCFG0x3F0
> +#define DDRPERFM_VER0x3F4
> +#define DDRPERFM_ID0x3F8
> +#define DDRPERFM_SID0x3FC
> +
> +#define CTL_START0x00000001
> +#define CTL_STOP0x00000002
> +#define CCR_CLEAR_ALL0x8000000F
> +#define SID_MAGIC_ID0xA3C5DD01
> +
> +#define STRING "Read = %llu, Write = %llu, Read & Write = %llu (MB/s)\n"
If you need this, please expand it in-place. As-is, this is needless
obfuscation.
> +
> +enum {
> +READ_CNT,
> +WRITE_CNT,
> +ACTIVATE_CNT,
> +IDLE_CNT,
> +TIME_CNT,
> +PMU_NR_COUNTERS
> +};
I take it these are fixed-purpose counters in the hardware?
> +
> +struct stm32_ddr_pmu {
> +struct pmu pmu;
> +void __iomem *membase;
> +struct clk *clk;
> +struct clk *clk_ddr;
> +unsigned long clk_ddr_rate;
> +struct hrtimer hrtimer;
> +ktime_t poll_period;
> +spinlock_t lock; /* for shared registers access */
All accesses to a PMU instance should be serialized on the same CPU,
so you shouldn't need a lock (though you will need to disable IRQs to
serialize with the interrupt handler).
The perf subsystem cannot sanely access the PMU across multiple CPUs.
> +struct perf_event *events[PMU_NR_COUNTERS];
> +u64 events_cnt[PMU_NR_COUNTERS];
> +};
> +
> +static inline struct stm32_ddr_pmu *pmu_to_stm32_ddr_pmu(struct pmu *p)
> +{
> +return container_of(p, struct stm32_ddr_pmu, pmu);
> +}
> +
> +static inline struct stm32_ddr_pmu *hrtimer_to_stm32_ddr_pmu(struct hrtimer *h)
> +{
> +return container_of(h, struct stm32_ddr_pmu, hrtimer);
> +}
> +
> +static u64 stm32_ddr_pmu_compute_bw(struct stm32_ddr_pmu *stm32_ddr_pmu,
> + int counter)
> +{
> +u64 bw = stm32_ddr_pmu->events_cnt[counter], tmp;
> +u64 div = stm32_ddr_pmu->events_cnt[TIME_CNT];
> +u32 prediv = 1, premul = 1;
> +
> +if (bw && div) {
> +/* Maximize the dividend into 64 bits */
> +while ((bw < 0x8000000000000000ULL) &&
> + (premul < 0x40000000UL)) {
> +bw = bw << 1;
> +premul *= 2;
> +}
> +/* Minimize the dividor to fit in 32 bits */
> +while ((div > 0xffffffffUL) && (prediv < 0x40000000UL)) {
> +div = div >> 1;
> +prediv *= 2;
> +}
> +/* Divide counter per time and multiply per DDR settings */
> +do_div(bw, div);
> +tmp = bw * BURST_LENGTH * WORD_LENGTH;
> +tmp *= stm32_ddr_pmu->clk_ddr_rate;
> +if (tmp < bw)
> +goto error;
> +bw = tmp;
> +/* Cancel the prediv and premul factors */
> +while (prediv > 1) {
> +bw = bw >> 1;
> +prediv /= 2;
> +}
> +while (premul > 1) {
> +bw = bw >> 1;
> +premul /= 2;
> +}
> +/* Convert MHz to Hz and B to MB, to finally get MB/s */
> +tmp = bw * 1000000;
> +if (tmp < bw)
> +goto error;
> +bw = tmp;
> +premul = 1024 * 1024;
> +while (premul > 1) {
> +bw = bw >> 1;
> +premul /= 2;
> +}
> +}
> +return bw;
> +
> +error:
> +pr_warn("stm32-ddr-pmu: overflow detected\n");
> +return 0;
> +}
IIUC this is for the stateful sysfs stuff, which should be removed.
> +
> +static void stm32_ddr_pmu_event_configure(struct perf_event *event)
> +{
> +struct stm32_ddr_pmu *stm32_ddr_pmu = pmu_to_stm32_ddr_pmu(event->pmu);
> +unsigned long lock_flags, config_base = event->hw.config_base;
> +u32 val;
> +
> +spin_lock_irqsave(&stm32_ddr_pmu->lock, lock_flags);
> +writel_relaxed(CTL_STOP, stm32_ddr_pmu->membase + DDRPERFM_CTL);
> +
> +if (config_base < TIME_CNT) {
> +val = readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_CFG);
> +val |= (1 << config_base);
> +writel_relaxed(val, stm32_ddr_pmu->membase + DDRPERFM_CFG);
> +}
> +spin_unlock_irqrestore(&stm32_ddr_pmu->lock, lock_flags);
> +}
You don't need the lock here. This is called from your pmu::{start,add}
callbacks, and the perf core already ensures those are serialized (via
the context lock), and called with IRQs disabled.
> +
> +static void stm32_ddr_pmu_event_read(struct perf_event *event)
> +{
> +struct stm32_ddr_pmu *stm32_ddr_pmu = pmu_to_stm32_ddr_pmu(event->pmu);
> +unsigned long flags, config_base = event->hw.config_base;
> +struct hw_perf_event *hw = &event->hw;
> +u64 prev_count, new_count, mask;
> +u32 val, offset, bit;
> +
> +spin_lock_irqsave(&stm32_ddr_pmu->lock, flags);
> +
> +writel_relaxed(CTL_STOP, stm32_ddr_pmu->membase + DDRPERFM_CTL);
> +
> +if (config_base == TIME_CNT) {
> +offset = DDRPERFM_TCNT;
> +bit = 1 << 31;
> +} else {
> +offset = DDRPERFM_CNT(config_base);
> +bit = 1 << config_base;
> +}
> +val = readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_STATUS);
> +if (val & bit)
> +pr_warn("stm32_ddr_pmu hardware overflow\n");
> +val = readl_relaxed(stm32_ddr_pmu->membase + offset);
> +writel_relaxed(bit, stm32_ddr_pmu->membase + DDRPERFM_CCR);
> +writel_relaxed(CTL_START, stm32_ddr_pmu->membase + DDRPERFM_CTL);
> +
> +do {
> +prev_count = local64_read(&hw->prev_count);
> +new_count = prev_count + val;
> +} while (local64_xchg(&hw->prev_count, new_count) != prev_count);
> +
> +mask = GENMASK_ULL(31, 0);
> +local64_add(val & mask, &event->count);
> +
> +if (new_count < prev_count)
> +pr_warn("STM32 DDR PMU counter saturated\n");
Do the counter saturate, or do they roll-over?
I think that the message is misleading here, but I just want to check.
> +
> +spin_unlock_irqrestore(&stm32_ddr_pmu->lock, flags);
> +}
> +
> +static void stm32_ddr_pmu_event_start(struct perf_event *event, int flags)
> +{
> +struct stm32_ddr_pmu *stm32_ddr_pmu = pmu_to_stm32_ddr_pmu(event->pmu);
> +struct hw_perf_event *hw = &event->hw;
> +unsigned long lock_flags;
> +
> +if (WARN_ON_ONCE(!(hw->state & PERF_HES_STOPPED)))
> +return;
> +
> +if (flags & PERF_EF_RELOAD)
> +WARN_ON_ONCE(!(hw->state & PERF_HES_UPTODATE));
> +
> +stm32_ddr_pmu_event_configure(event);
> +
> +/* Clear all counters to synchronize them, then start */
> +spin_lock_irqsave(&stm32_ddr_pmu->lock, lock_flags);
> +writel_relaxed(CCR_CLEAR_ALL, stm32_ddr_pmu->membase + DDRPERFM_CCR);
> +writel_relaxed(CTL_START, stm32_ddr_pmu->membase + DDRPERFM_CTL);
> +spin_unlock_irqrestore(&stm32_ddr_pmu->lock, lock_flags);
By 'clear' do you mean set the counters to zero?
Or is there a control bit that determine whether they count?
If we're updating the counter, we could update prev_count at the same
instant.
> +
> +hw->state = 0;
> +}
> +
> +static void stm32_ddr_pmu_event_stop(struct perf_event *event, int flags)
> +{
> +struct stm32_ddr_pmu *stm32_ddr_pmu = pmu_to_stm32_ddr_pmu(event->pmu);
> +unsigned long lock_flags, config_base = event->hw.config_base;
> +struct hw_perf_event *hw = &event->hw;
> +u32 val, bit;
> +
> +if (WARN_ON_ONCE(hw->state & PERF_HES_STOPPED))
> +return;
> +
> +spin_lock_irqsave(&stm32_ddr_pmu->lock, lock_flags);
> +writel_relaxed(CTL_STOP, stm32_ddr_pmu->membase + DDRPERFM_CTL);
> +if (config_base == TIME_CNT)
> +bit = 1 << 31;
> +else
> +bit = 1 << config_base;
> +writel_relaxed(bit, stm32_ddr_pmu->membase + DDRPERFM_CCR);
> +if (config_base < TIME_CNT) {
> +val = readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_CFG);
> +val &= ~bit;
> +writel_relaxed(val, stm32_ddr_pmu->membase + DDRPERFM_CFG);
> +}
> +spin_unlock_irqrestore(&stm32_ddr_pmu->lock, lock_flags);
> +
> +hw->state |= PERF_HES_STOPPED;
> +
> +if (flags & PERF_EF_UPDATE) {
> +stm32_ddr_pmu_event_read(event);
> +hw->state |= PERF_HES_UPTODATE;
> +}
> +}
> +
> +static int stm32_ddr_pmu_event_add(struct perf_event *event, int flags)
> +{
> +struct stm32_ddr_pmu *stm32_ddr_pmu = pmu_to_stm32_ddr_pmu(event->pmu);
> +unsigned long config_base = event->hw.config_base;
> +struct hw_perf_event *hw = &event->hw;
> +
> +stm32_ddr_pmu->events_cnt[config_base] = 0;
> +stm32_ddr_pmu->events[config_base] = event;
> +
> +clk_enable(stm32_ddr_pmu->clk);
> +hrtimer_start(&stm32_ddr_pmu->hrtimer, stm32_ddr_pmu->poll_period,
> + HRTIMER_MODE_REL);
> +
> +stm32_ddr_pmu_event_configure(event);
> +
> +hw->state = PERF_HES_STOPPED | PERF_HES_UPTODATE;
> +
> +if (flags & PERF_EF_START)
> +stm32_ddr_pmu_event_start(event, 0);
> +
> +return 0;
> +}
> +
> +static void stm32_ddr_pmu_event_del(struct perf_event *event, int flags)
> +{
> +struct stm32_ddr_pmu *stm32_ddr_pmu = pmu_to_stm32_ddr_pmu(event->pmu);
> +unsigned long config_base = event->hw.config_base;
> +bool stop = true;
> +int i;
> +
> +stm32_ddr_pmu_event_stop(event, PERF_EF_UPDATE);
> +
> +stm32_ddr_pmu->events_cnt[config_base] += local64_read(&event->count);
> +stm32_ddr_pmu->events[config_base] = NULL;
> +
> +for (i = 0; i < PMU_NR_COUNTERS; i++)
> +if (stm32_ddr_pmu->events[i])
> +stop = false;
> +if (stop)
> +hrtimer_cancel(&stm32_ddr_pmu->hrtimer);
> +
> +clk_disable(stm32_ddr_pmu->clk);
This doesn't look right. If I add two events A and B, then delete event
A, surely we want the clock on for event B?
Does the clock only affect whether the counters count, or does it also
affect whether the register file is usable?
> +}
> +
> +static int stm32_ddr_pmu_event_init(struct perf_event *event)
> +{
> +struct hw_perf_event *hw = &event->hw;
> +
> +if (is_sampling_event(event))
> +return -EINVAL;
> +
> +if (event->attach_state & PERF_ATTACH_TASK)
> +return -EINVAL;
> +
> +if (event->attr.exclude_user ||
> + event->attr.exclude_kernel ||
> + event->attr.exclude_hv ||
> + event->attr.exclude_idle ||
> + event->attr.exclude_host ||
> + event->attr.exclude_guest)
> +return -EINVAL;
> +
> +if (event->cpu < 0)
> +return -EINVAL;
For a system PMU like this, you must ensure that all events are assigned
to the same CPU.
You'll need to designate some arbitrary CPU to mange the PMU, expose
that under sysfs, and upon hotplug events you must choose a new CPU and
migrate existing events.
For a simple example, see arch/arm/mm/cache-l2x0-pmu.c.
> +
> +hw->config_base = event->attr.config;
> +
> +return 0;
> +}
> +
> +static enum hrtimer_restart stm32_ddr_pmu_poll(struct hrtimer *hrtimer)
> +{
> +struct stm32_ddr_pmu *stm32_ddr_pmu = hrtimer_to_stm32_ddr_pmu(hrtimer);
> +int i;
> +
> +for (i = 0; i < PMU_NR_COUNTERS; i++)
> +if (stm32_ddr_pmu->events[i])
> +stm32_ddr_pmu_event_read(stm32_ddr_pmu->events[i]);
> +
> +hrtimer_forward_now(hrtimer, stm32_ddr_pmu->poll_period);
> +
> +return HRTIMER_RESTART;
> +}
> +
> +static ssize_t stm32_ddr_pmu_sysfs_show(struct device *dev,
> +struct device_attribute *attr,
> +char *buf)
> +{
> +struct dev_ext_attribute *eattr;
> +
> +eattr = container_of(attr, struct dev_ext_attribute, attr);
> +
> +return sprintf(buf, "config=0x%lx\n", (unsigned long)eattr->var);
> +}
> +
> +static ssize_t bandwidth_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> +struct stm32_ddr_pmu *stm32_ddr_pmu = dev_get_drvdata(dev);
> +u64 r_bw, w_bw;
> +int ret;
> +
> +if (stm32_ddr_pmu->events_cnt[TIME_CNT]) {
> +r_bw = stm32_ddr_pmu_compute_bw(stm32_ddr_pmu, READ_CNT);
> +w_bw = stm32_ddr_pmu_compute_bw(stm32_ddr_pmu, WRITE_CNT);
> +
> +ret = snprintf(buf, PAGE_SIZE, STRING,
> + r_bw, w_bw, (r_bw + w_bw));
> +} else {
> +ret = snprintf(buf, PAGE_SIZE, "No data available\n");
> +}
> +
> +return ret;
> +}
As commented above, this should not be exposed under sysfs. It doesn't
follow the usual ABI rules, it's weirdly stateful, and it's redundant.
> +
> +#define STM32_DDR_PMU_ATTR(_name, _func, _config)\
> +(&((struct dev_ext_attribute[]) {\
> +{ __ATTR(_name, 0444, _func, NULL), (void *)_config } \
> +})[0].attr.attr)
> +
> +#define STM32_DDR_PMU_EVENT_ATTR(_name, _config)\
> +STM32_DDR_PMU_ATTR(_name, stm32_ddr_pmu_sysfs_show,\
> + (unsigned long)_config)
> +
> +static struct attribute *stm32_ddr_pmu_event_attrs[] = {
> +STM32_DDR_PMU_EVENT_ATTR(read_cnt, READ_CNT),
> +STM32_DDR_PMU_EVENT_ATTR(write_cnt, WRITE_CNT),
> +STM32_DDR_PMU_EVENT_ATTR(activate_cnt, ACTIVATE_CNT),
> +STM32_DDR_PMU_EVENT_ATTR(idle_cnt, IDLE_CNT),
> +STM32_DDR_PMU_EVENT_ATTR(time_cnt, TIME_CNT),
> +NULL
> +};
> +
> +static DEVICE_ATTR_RO(bandwidth);
> +static struct attribute *stm32_ddr_pmu_bandwidth_attrs[] = {
> +&dev_attr_bandwidth.attr,
> +NULL,
> +};
> +
> +static struct attribute_group stm32_ddr_pmu_event_attrs_group = {
> +.name = "events",
> +.attrs = stm32_ddr_pmu_event_attrs,
> +};
> +
> +static struct attribute_group stm32_ddr_pmu_bandwidth_attrs_group = {
> +.attrs = stm32_ddr_pmu_bandwidth_attrs,
> +};
> +
> +static const struct attribute_group *stm32_ddr_pmu_attr_groups[] = {
> +&stm32_ddr_pmu_event_attrs_group,
> +&stm32_ddr_pmu_bandwidth_attrs_group,
> +NULL,
> +};
> +
> +static int stm32_ddr_pmu_device_probe(struct platform_device *pdev)
> +{
> +struct stm32_ddr_pmu *stm32_ddr_pmu;
> +struct reset_control *rst;
> +struct resource *res;
> +int i, ret;
> +u32 val;
> +
> +stm32_ddr_pmu = devm_kzalloc(&pdev->dev, sizeof(struct stm32_ddr_pmu),
> + GFP_KERNEL);
> +if (!stm32_ddr_pmu)
> +return -ENOMEM;
> +platform_set_drvdata(pdev, stm32_ddr_pmu);
> +
> +res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +stm32_ddr_pmu->membase = devm_ioremap_resource(&pdev->dev, res);
> +if (IS_ERR(stm32_ddr_pmu->membase)) {
> +pr_warn("Unable to get STM32 DDR PMU membase\n");
> +return PTR_ERR(stm32_ddr_pmu->membase);
> +}
> +
> +stm32_ddr_pmu->clk = devm_clk_get(&pdev->dev, "bus");
> +if (IS_ERR(stm32_ddr_pmu->clk)) {
> +pr_warn("Unable to get STM32 DDR PMU clock\n");
> +return PTR_ERR(stm32_ddr_pmu->clk);
> +}
> +
> +ret = clk_prepare_enable(stm32_ddr_pmu->clk);
> +if (ret) {
> +pr_warn("Unable to prepare STM32 DDR PMU clock\n");
> +return ret;
> +}
> +
> +stm32_ddr_pmu->clk_ddr = devm_clk_get(&pdev->dev, "ddr");
> +if (IS_ERR(stm32_ddr_pmu->clk_ddr)) {
> +pr_warn("Unable to get STM32 DDR clock\n");
> +return PTR_ERR(stm32_ddr_pmu->clk_ddr);
> +}
> +stm32_ddr_pmu->clk_ddr_rate = clk_get_rate(stm32_ddr_pmu->clk_ddr);
> +stm32_ddr_pmu->clk_ddr_rate /= 1000000;
> +
> +stm32_ddr_pmu->poll_period = ms_to_ktime(POLL_MS);
> +hrtimer_init(&stm32_ddr_pmu->hrtimer, CLOCK_MONOTONIC,
> + HRTIMER_MODE_REL);
> +stm32_ddr_pmu->hrtimer.function = stm32_ddr_pmu_poll;
> +spin_lock_init(&stm32_ddr_pmu->lock);
> +
> +for (i = 0; i < PMU_NR_COUNTERS; i++) {
> +stm32_ddr_pmu->events[i] = NULL;
> +stm32_ddr_pmu->events_cnt[i] = 0;
> +}
> +
> +val = readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_SID);
> +if (val != SID_MAGIC_ID)
> +return -EINVAL;
> +
> +stm32_ddr_pmu->pmu = (struct pmu) {
> +.task_ctx_nr = perf_invalid_context,
> +.start = stm32_ddr_pmu_event_start,
> +.stop = stm32_ddr_pmu_event_stop,
> +.add = stm32_ddr_pmu_event_add,
> +.del = stm32_ddr_pmu_event_del,
> +.event_init = stm32_ddr_pmu_event_init,
> +.attr_groups = stm32_ddr_pmu_attr_groups,
> +};
> +ret = perf_pmu_register(&stm32_ddr_pmu->pmu, "ddrperfm", -1);
Please give this a better name. The usual convention is to use "_pmu" as
the suffix, so we should use that rather than "perfm".
To be unambiguous, something like "stm32_ddr_pmu" would be good.
Thanks,
Mark.
> +if (ret) {
> +pr_warn("Unable to register STM32 DDR PMU\n");
> +return ret;
> +}
> +
> +rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> +if (!IS_ERR(rst)) {
> +reset_control_assert(rst);
> +udelay(2);
> +reset_control_deassert(rst);
> +}
> +
> +pr_info("stm32-ddr-pmu: probed (ID=0x%08x VER=0x%08x), DDR@%luMHz\n",
> +readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_ID),
> +readl_relaxed(stm32_ddr_pmu->membase + DDRPERFM_VER),
> +stm32_ddr_pmu->clk_ddr_rate);
> +
> +clk_disable(stm32_ddr_pmu->clk);
> +
> +return 0;
> +}
> +
> +static int stm32_ddr_pmu_device_remove(struct platform_device *pdev)
> +{
> +struct stm32_ddr_pmu *stm32_ddr_pmu = platform_get_drvdata(pdev);
> +
> +perf_pmu_unregister(&stm32_ddr_pmu->pmu);
> +
> +return 0;
> +}
> +
> +static const struct of_device_id stm32_ddr_pmu_of_match[] = {
> +{ .compatible = "st,stm32-ddr-pmu" },
> +{ },
> +};
> +
> +static struct platform_driver stm32_ddr_pmu_driver = {
> +.driver = {
> +.name= "stm32-ddr-pmu",
> +.of_match_table = of_match_ptr(stm32_ddr_pmu_of_match),
> +},
> +.probe = stm32_ddr_pmu_device_probe,
> +.remove = stm32_ddr_pmu_device_remove,
> +};
> +
> +module_platform_driver(stm32_ddr_pmu_driver);
> +
> +MODULE_DESCRIPTION("Perf driver for STM32 DDR performance monitor");
> +MODULE_AUTHOR("Gerald Baeza <gerald.baeza@st.com>");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply
* Re: [PATCH v4 00/14] ima: introduce IMA Digest Lists extension
From: Roberto Sassu @ 2019-06-26 11:38 UTC (permalink / raw)
To: Mimi Zohar, dmitry.kasatkin, mjg59, Rob Landley
Cc: linux-integrity, linux-security-module, linux-fsdevel, linux-doc,
linux-kernel, silviu.vlasceanu
In-Reply-To: <1561484133.4066.16.camel@linux.ibm.com>
On 6/25/2019 7:35 PM, Mimi Zohar wrote:
> [Cc'ing Rob Landley]
>
> On Tue, 2019-06-25 at 14:57 +0200, Roberto Sassu wrote:
>> Mimi, do you have any thoughts on this version?
>
> I need to look closer, but when I first looked these changes seemed to
> be really invasive. Let's first work on getting the CPIO xattr
If you can provide early comments, that would be great. I'll have a look
at the problems and when the xattr support for the ram disk is
upstreamed I will be ready to send a new version.
Thanks
Roberto
--
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI
^ permalink raw reply
* Re: [PATCH v5 13/18] kunit: tool: add Python wrappers for running KUnit tests
From: Brendan Higgins @ 2019-06-26 8:02 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Peter Zijlstra, Rob Herring, Stephen Boyd, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg,
Felix Guo
In-Reply-To: <20190626000150.GT19023@42.do-not-panic.com>
On Tue, Jun 25, 2019 at 5:01 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Mon, Jun 17, 2019 at 01:26:08AM -0700, Brendan Higgins wrote:
> > create mode 100644 tools/testing/kunit/test_data/test_is_test_passed-all_passed.log
> > create mode 100644 tools/testing/kunit/test_data/test_is_test_passed-crash.log
> > create mode 100644 tools/testing/kunit/test_data/test_is_test_passed-failure.log
> > create mode 100644 tools/testing/kunit/test_data/test_is_test_passed-no_tests_run.log
> > create mode 100644 tools/testing/kunit/test_data/test_output_isolated_correctly.log
> > create mode 100644 tools/testing/kunit/test_data/test_read_from_file.kconfig
>
> Why are these being added upstream? The commit log does not explain
> this.
Oh sorry, those are for testing purposes. I thought that was clear
from being in the test_data directory. I will reference it in the
commit log in the next revision.
^ permalink raw reply
* Re: [PATCH v5 07/18] kunit: test: add initial tests
From: Brendan Higgins @ 2019-06-26 7:53 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
Peter Zijlstra, Rob Herring, Stephen Boyd, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <20190625232249.GS19023@42.do-not-panic.com>
On Tue, Jun 25, 2019 at 4:22 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Mon, Jun 17, 2019 at 01:26:02AM -0700, Brendan Higgins wrote:
> > diff --git a/kunit/example-test.c b/kunit/example-test.c
> > new file mode 100644
> > index 0000000000000..f44b8ece488bb
> > --- /dev/null
> > +++ b/kunit/example-test.c
>
> <-- snip -->
>
> > +/*
> > + * This defines a suite or grouping of tests.
> > + *
> > + * Test cases are defined as belonging to the suite by adding them to
> > + * `kunit_cases`.
> > + *
> > + * Often it is desirable to run some function which will set up things which
> > + * will be used by every test; this is accomplished with an `init` function
> > + * which runs before each test case is invoked. Similarly, an `exit` function
> > + * may be specified which runs after every test case and can be used to for
> > + * cleanup. For clarity, running tests in a test module would behave as follows:
> > + *
>
> To be clear this is not the kernel module init, but rather the kunit
> module init. I think using kmodule would make this clearer to a reader.
Seems reasonable. Will fix in next revision.
> > + * module.init(test);
> > + * module.test_case[0](test);
> > + * module.exit(test);
> > + * module.init(test);
> > + * module.test_case[1](test);
> > + * module.exit(test);
> > + * ...;
> > + */
>
> Luis
^ permalink raw reply
* Re: [PATCH v5 01/18] kunit: test: add KUnit test runner core
From: Brendan Higgins @ 2019-06-26 6:41 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Stephen Boyd, Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook,
Kieran Bingham, Peter Zijlstra, Rob Herring, shuah,
Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
Linux Kernel Mailing List, open list:KERNEL SELFTEST FRAMEWORK,
linux-nvdimm, linux-um, Sasha Levin, Bird, Timothy,
Amir Goldstein, Dan Carpenter, Daniel Vetter, Jeff Dike,
Joel Stanley, Julia Lawall, Kevin Hilman, Knut Omang,
Logan Gunthorpe, Michael Ellerman, Petr Mladek, Randy Dunlap,
Richard Weinberger, David Rientjes, Steven Rostedt, wfg
In-Reply-To: <20190625230253.GQ19023@42.do-not-panic.com>
On Tue, Jun 25, 2019 at 4:02 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Tue, Jun 25, 2019 at 03:14:45PM -0700, Brendan Higgins wrote:
> > On Tue, Jun 25, 2019 at 2:44 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> > > Since its a new architecture and since you seem to imply most tests
> > > don't require locking or even IRQs disabled, I think its worth to
> > > consider the impact of adding such extreme locking requirements for
> > > an initial ramp up.
> >
> > Fair enough, I can see the point of not wanting to use irq disabled
> > until we get someone complaining about it, but I think making it
> > thread safe is reasonable. It means there is one less thing to confuse
> > a KUnit user and the only penalty paid is some very minor performance.
>
> One reason I'm really excited about kunit is speed... so by all means I
> think we're at a good point to analyze performance optimizationsm if
> they do make sense.
Yeah, but I think there are much lower hanging fruit than this (as you
point out below). I am all for making/keeping KUnit super fast, but I
also don't want to waste time with premature optimizations and I think
having thread safe expectations and non-thread safe expectations hurts
usability.
Still, I am on board with making this a mutex instead of a spinlock for now.
> While on the topic of parallization, what about support for running
> different test cases in parallel? Or at the very least different kunit
> modules in parallel. Few questions come up based on this prospect:
>
> * Why not support parallelism from the start?
Just because it is more work and there isn't much to gain from it right now.
Some numbers:
I currently have a collection of 86 test cases in the branch that this
patchset is from. I turned on PRINTK_TIME and looked at the first
KUnit output and the last. On UML, start time was 0.090000, and end
time was 0.090000. Looks like sched_clock is not very good on UML.
Still it seems quite likely that all of these tests run around 0.01
seconds or less on UML: I ran KUnit with only 2 test cases enabled
three times and got an average runtime of 1.55867 seconds with a
standard deviation of 0.0346747. I then ran it another three times
with all test cases enabled and got an average runtime of 1.535
seconds with a standard deviation of 0.0150997. The second average is
less, but that doesn't really mean anything because it is well within
one standard deviation with a very small sample size. Nevertheless, we
can conclude that the actual runtime of those 84 test cases is most
likely within one standard deviation, so on the order of 0.01 seconds.
On x86 running on QEMU, first message from KUnit was printed at
0.194251 and the last KUnit message was printed at 0.340915, meaning
that all 86 test cases ran in about 0.146664 seconds.
In any case, running KUnit tests in parallel is definitely something I
plan on adding it eventually, but it just doesn't really seem worth it
right now. I find the incremental build time of the kernel to
typically be between 3 and 30 seconds, and a clean build to be between
30 seconds to several minutes, depending on the number of available
cores, so I don't think most users would even notice the amount of
runtime contributed by the actual unit tests until we start getting
into the 1000s of test cases. I don't suspect it will become an issue
until we get into the 10,000s of test cases. I think we are a pretty
long way off from that.
> * Are you opposed to eventually having this added? For instance, there is
> enough code on lib/test_kmod.c for batching tons of kthreads each
> one running its own thing for testing purposes which could be used
> as template.
I am not opposed to adding it eventually at all. I actually plan on
doing so, just not in this patchset. There are a lot of additional
features, improvements, and sugar that I really want to add, so much
so that most of it doesn't belong in this patchset; I just think this
is one of those things that belongs in a follow up. I tried to boil
down this patchset to as small as I could while still being useful;
this is basically an MVP. Maybe after this patchset gets merged I
should post a list of things I have ready for review, or would like to
work on, and people can comment on what things they want to see next.
> * If we eventually *did* support it:
> - Would logs be skewed?
Probably, before I went with the TAP approach, I was tagging each
message with the test case it came from and I could have parsed it and
assembled a coherent view of the logs using that; now that I am using
TAP conforming output, that won't work. I haven't really thought too
hard about how to address it, but there are ways. For the UML users, I
am planning on adding a feature to guarantee hermeticity between tests
running in different modules by adding a feature that allows a single
kernel to be built with all tests included, and then determine which
tests get run by passing in command line arguments or something. This
way you can get the isolation from running tests in separate
environments without increasing the build cost. We could also use this
method to achieve parallelism by dispatching multiple kernels at once.
That only works for UML, but I imagine you could do something similar
for users running tests under qemu.
> - Could we have a way to query: give me log for only kunit module
> named "foo"?
Yeah, I think that would make sense as part of the hermeticity thing I
mentioned above.
Hope that seems reasonable!
^ permalink raw reply
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