Re: [PATCH 3/3] rust: upgrade to Rust 1.73.0
> The vast majority of changes are due to our `alloc` fork being upgraded > at once. Ah this is annoying sometimes :/ > > There are two kinds of changes to be aware of: the ones coming from > upstream, which we should follow as closely as possible, and the updates > needed in our added fallible APIs to keep them matching the newer > infallible APIs coming from upstream. > > Instead of taking a look at the diff of this patch, an alternative > approach is reviewing a diff of the changes between upstream `alloc` and > the kernel's. This allows to easily inspect the kernel additions only, > especially to check if the fallible methods we already have still match > the infallible ones in the new version coming from upstream. > > Another approach is reviewing the changes introduced in the additions in > the kernel fork between the two versions. This is useful to spot > potentially unintended changes to our additions. > > To apply these approaches, one may follow steps similar to the following > to generate a pair of patches that show the differences between upstream > Rust and the kernel (for the subset of `alloc` we use) before and after > applying this patch: > > # Get the difference with respect to the old version. > git -C rust checkout $(linux/scripts/min-tool-version.sh rustc) > git -C linux ls-tree -r --name-only HEAD -- rust/alloc | > cut -d/ -f3- | > grep -Fv README.md | > xargs -IPATH cp rust/library/alloc/src/PATH linux/rust/alloc/PATH > git -C linux diff --patch-with-stat --summary -R > old.patch > git -C linux restore rust/alloc > > # Apply this patch. > git -C linux am rust-upgrade.patch > > # Get the difference with respect to the new version. > git -C rust checkout $(linux/scripts/min-tool-version.sh rustc) > git -C linux ls-tree -r --name-only HEAD -- rust/alloc | > cut -d/ -f3- | > grep -Fv README.md | > xargs -IPATH cp rust/library/alloc/src/PATH linux/rust/alloc/PATH > git -C linux diff --patch-with-stat --summary -R > new.patch > git -C linux restore rust/alloc > > Now one may check the `new.patch` to take a look at the additions (first > approach) or at the difference between those two patches (second > approach). For the latter, a side-by-side tool is recommended. > > Link: > https://github.com/rust-lang/rust/blob/stable/RELEASES.md#version-1730-2023-10-05 > [1] > Link: https://rust-for-linux.com/rust-version-policy [2] > Link: https://github.com/Rust-for-Linux/linux/issues/2 [3] > Link: https://github.com/rust-lang/compiler-team/issues/596 [4] > Signed-off-by: Miguel Ojeda > --- > Documentation/process/changes.rst | 2 +- > rust/alloc/alloc.rs | 22 -- > rust/alloc/boxed.rs | 48 --- > rust/alloc/lib.rs | 5 ++-- > rust/alloc/raw_vec.rs | 30 --- > rust/alloc/vec/mod.rs | 4 +-- > rust/alloc/vec/spec_extend.rs | 8 +++--- > rust/compiler_builtins.rs | 1 + > scripts/min-tool-version.sh | 2 +- > 9 files changed, 63 insertions(+), 59 deletions(-) > > diff --git a/Documentation/process/changes.rst > b/Documentation/process/changes.rst > index 731cc104c56f..bb96ca0f774b 100644 > --- a/Documentation/process/changes.rst Reviewed-by: Vincenzo Palazzo
Re: [PATCH] docs: submitting-patches: Introduce Test: tag
On 8.10.2023 19:18, Geert Uytterhoeven wrote: > On Sat, Oct 7, 2023 at 2:57 PM Jonathan Corbet wrote: >> Konrad Dybcio writes: >> >>> Currently, we blindly trust the submitters that they both compiled their >>> code at all, tested it on a relevant device, and have done so in a manner >>> that made sense for a given changeset. >>> >>> If at least two of these three things were always true, the review >>> workflow would be much more exciting. >>> >>> Introduce a new Test: tag to help submitters express the way the patch >>> was tested, making it easier to understand for reviewers and maintainers >>> whether it was tested, and if so, whether that test was sufficient. >>> >>> I originally found something like this on Google's Android kernel repos >>> and loved the concept. >>> >>> Test: make htmldocs and manual examination >>> Signed-off-by: Konrad Dybcio >>> --- >>> Documentation/process/submitting-patches.rst | 18 +- >>> 1 file changed, 17 insertions(+), 1 deletion(-) >> >> Do we really want to do this? To me, it almost seems like it codifies >> the idea that sending *untested* patches is OK as long as you leave out >> the tag. > > Exactly. We are already receiving too many untested patches. > >> Others may disagree, but I don't think we need yet another tag for this. >> Testing of patches before sending them should be the norm; if special > > +1 > >> notes about testing are needed, they can go in or below the changelog, >> as appropriate. > > +1 > Okay, I see your points, let's forget about this.. Konrad
Re: [REBASE PATCH v5 10/17] pstore: Add pstore_region_defined() helper and export it
Thanks for the review.
On 9/14/2023 4:54 AM, Kees Cook wrote:
On Mon, Sep 11, 2023 at 04:23:52PM +0530, Mukesh Ojha wrote:
There are users like Qualcomm minidump which is interested in
knowing the pstore frontend addresses and sizes from the backend
(ram) to be able to register it with firmware to finally collect
them during crash for debugging.
Signed-off-by: Mukesh Ojha
---
fs/pstore/platform.c | 15 +++
fs/pstore/ram.c| 42 ++
include/linux/pstore.h | 6 ++
3 files changed, 63 insertions(+)
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index e5bca9a004cc..fdac951059c1 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -139,6 +139,21 @@ enum pstore_type_id pstore_name_to_type(const char *name)
}
EXPORT_SYMBOL_GPL(pstore_name_to_type);
+int pstore_region_defined(struct pstore_record *record,
+ void **virt, phys_addr_t *phys,
+ size_t *size, unsigned int *max_dump_cnt)
+{
+ if (!psinfo)
+ return -EINVAL;
+
+ record->psi = psinfo;
Uh, this makes no sense to me. If this is a real pstore_record, you
cannot just assign psi here.
Ok.
+
+ return psinfo->region_info ?
+ psinfo->region_info(record, virt, phys, size, max_dump_cnt) :
+ -EINVAL;
Common code style for this kind of thing is usually like this:
if (!psinfo->region_info)
return -EINVAL;
return psinfo->region_info(...)
Thanks.
-Mukesh
+}
+EXPORT_SYMBOL_GPL(pstore_region_defined);
+
static void pstore_timer_kick(void)
{
if (pstore_update_ms < 0)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index ab551caa1d2a..62202f3ddf63 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -437,6 +437,47 @@ static int ramoops_pstore_erase(struct pstore_record
*record)
return 0;
}
+static int ramoops_region_info(struct pstore_record *record,
+ void **virt, phys_addr_t *phys,
+ size_t *size, unsigned int *max_dump_cnt)
But there's a larger problem here -- "virt", "phys" and likely
"max_dump_cnt" are aspects _specific to the ram backend_. This can't be
a generic pstore interface.
I'm not opposed to it being exposed only from ramoops, though.
But I think you'll want a pstore generic way to iterate over the
records..
[PATCH net-next] docs: try to encourage (netdev?) reviewers
Add a section to netdev maintainer doc encouraging reviewers to chime in on the mailing list. The questions about "when is it okay to share feedback" keep coming up (most recently at netconf) and the answer is "pretty much always". Extend the section of 7.AdvancedTopics.rst which deals with reviews a little bit to add stuff we had been recommending locally. Signed-off-by: Jakub Kicinski -- RFC -> v1: - spelling (compliment) - move to common docs: - ask for more opinions - use of tags - compliments - ask less experienced reviewers to avoid style comments (using Florian's wording) CC: and...@lunn.ch CC: jesse.brandeb...@intel.com CC: s...@queasysnail.net CC: ho...@verge.net.au CC: przemyslaw.kits...@intel.com CC: f.faine...@gmail.com CC: j...@resnulli.us CC: ecree.xil...@gmail.com --- Documentation/process/7.AdvancedTopics.rst | 18 ++ Documentation/process/maintainer-netdev.rst | 15 +++ 2 files changed, 33 insertions(+) diff --git a/Documentation/process/7.AdvancedTopics.rst b/Documentation/process/7.AdvancedTopics.rst index bf7cbfb4caa5..415749feed17 100644 --- a/Documentation/process/7.AdvancedTopics.rst +++ b/Documentation/process/7.AdvancedTopics.rst @@ -146,6 +146,7 @@ pull. The git request-pull command can be helpful in this regard; it will format the request as other developers expect, and will also check to be sure that you have remembered to push those changes to the public server. +.. _development_advancedtopics_reviews: Reviewing patches - @@ -167,6 +168,12 @@ comments as questions rather than criticisms. Asking "how does the lock get released in this path?" will always work better than stating "the locking here is wrong." +Another technique useful in case of a disagreement is to ask for others +to chime in. If a discussion reaches a stalemate after a few exchanges, +calling for opinions of other reviewers or maintainers. Often those in +agreement with a reviewer remain silent unless called upon. +Opinion of multiple people carries exponentially more weight. + Different developers will review code from different points of view. Some are mostly concerned with coding style and whether code lines have trailing white space. Others will focus primarily on whether the change implemented @@ -176,3 +183,14 @@ security issues, duplication of code found elsewhere, adequate documentation, adverse effects on performance, user-space ABI changes, etc. All types of review, if they lead to better code going into the kernel, are welcome and worthwhile. + +There is no strict requirement to use specific tags like ``Reviewed-by``. +In fact reviews in plain English are more informative and encouraged +even when a tag is provided (e.g. "I looked at aspects A, B and C of this +submission and it looks good to me.") +Some form of a review message / reply is obviously necessary otherwise +maintainers will not know that the reviewer has looked at the patch at all! + +Last but not least patch review may become a negative process, focused +on pointing out problems. Please throw in a compliment once in a while, +particularly for newbies! diff --git a/Documentation/process/maintainer-netdev.rst b/Documentation/process/maintainer-netdev.rst index 09dcf6377c27..a0cb00e7f579 100644 --- a/Documentation/process/maintainer-netdev.rst +++ b/Documentation/process/maintainer-netdev.rst @@ -441,6 +441,21 @@ in a way which would break what would normally be considered uAPI. new ``netdevsim`` features must be accompanied by selftests under ``tools/testing/selftests/``. +Reviewer guidance +- + +Reviewing other people's patches on the list is highly encouraged, +regardless of the level of expertise. For general guidance and +helpful tips please see :ref:`development_advancedtopics_reviews`. + +It's safe to assume that netdev maintainers know the community and the level +of expertise of the reviewers. The reviewers should not be concerned about +their comments impeding or derailing the patch flow. + +Less experienced reviewers are highly encouraged to do more in-depth +review of submissions and not focus exclusively on trivial / subject +matters like code formatting, tags etc. + Testimonials / feedback --- -- 2.41.0
