Re: [PATCH RFC 4/4] mm, page_alloc: add static key for should_fail_alloc_page()
On Fri, May 31, 2024 at 11:33:35AM +0200, Vlastimil Babka wrote: > Similarly to should_failslab(), remove the overhead of calling the > noinline function should_fail_alloc_page() with a static key that guards > the allocation hotpath callsite and is controlled by the fault and error > injection frameworks. > > Signed-off-by: Vlastimil Babka Reviewed-by: Roman Gushchin Thanks!
Re: [PATCH RFC 3/4] mm, slab: add static key for should_failslab()
On Fri, May 31, 2024 at 11:33:34AM +0200, Vlastimil Babka wrote:
> Since commit 4f6923fbb352 ("mm: make should_failslab always available for
> fault injection") should_failslab() is unconditionally a noinline
> function. This adds visible overhead to the slab allocation hotpath,
> even if the function is empty. With CONFIG_FAILSLAB=y there's additional
> overhead when the functionality is not enabled by a boot parameter or
> debugfs.
>
> The overhead can be eliminated with a static key around the callsite.
> Fault injection and error injection frameworks can now be told that the
> this function has a static key associated, and are able to enable and
> disable it accordingly.
>
> Signed-off-by: Vlastimil Babka
Reviewed-by: Roman Gushchin
Re: [PATCH RFC 0/4] static key support for error injection functions
On Fri, May 31, 2024 at 11:33:31AM +0200, Vlastimil Babka wrote:
> Incomplete, help needed from ftrace/kprobe and bpf folks.
>
> As previously mentioned by myself [1] and others [2] the functions
> designed for error injection can bring visible overhead in fastpaths
> such as slab or page allocation, because even if nothing hooks into them
> at a given moment, they are noninline function calls regardless of
> CONFIG_ options since commits 4f6923fbb352 ("mm: make should_failslab
> always available for fault injection") and af3b854492f3
> ("mm/page_alloc.c: allow error injection").
>
> Live patching their callsites has been also suggested in both [1] and
> [2] threads, and this is an attempt to do that with static keys that
> guard the call sites. When disabled, the error injection functions still
> exist and are noinline, but are not being called. Any of the existing
> mechanisms that can inject errors should make sure to enable the
> respective static key. I have added that support to some of them but
> need help with the others.
I think it's a clever idea and makes total sense!
>
> - the legacy fault injection, i.e. CONFIG_FAILSLAB and
> CONFIG_FAIL_PAGE_ALLOC is handled in Patch 1, and can be passed the
> address of the static key if it exists. The key will be activated if the
> fault injection probability becomes non-zero, and deactivated in the
> opposite transition. This also removes the overhead of the evaluation
> (on top of the noninline function call) when these mechanisms are
> configured in the kernel but unused at the moment.
>
> - the generic error injection using kretprobes with
> override_function_with_return is handled in patch 2. The
> ALLOW_ERROR_INJECTION() annotation is extended so that static key
> address can be passed, and the framework controls it when error
> injection is enabled or disabled in debugfs for the function.
>
> There are two more users I know of but am not familiar enough to fix up
> myself. I hope people that are more familiar can help me here.
>
> - ftrace seems to be using override_function_with_return from
> #define ftrace_override_function_with_return but I found no place
> where the latter is used. I assume it might be hidden behind more
> macro magic? But the point is if ftrace can be instructed to act like
> an error injection, it would also have to use some form of metadata
> (from patch 2 presumably?) to get to the static key and control it.
>
> If ftrace can only observe the function being called, maybe it
> wouldn't be wrong to just observe nothing if the static key isn't
> enabled because nobody is doing the fault injection?
>
> - bpftrace, as can be seen from the example in commit 4f6923fbb352
> description. I suppose bpf is already aware what functions the
> currently loaded bpf programs hook into, so that it could look up the
> static key and control it. Maybe using again the metadata from patch 2,
> or extending its own, as I've noticed there's e.g. BTF_ID(func,
> should_failslab)
>
> Now I realize maybe handling this at the k(ret)probe level would be
> sufficient for all cases except the legacy fault injection from Patch 1?
> Also wanted to note that by AFAIU by using the static_key_slow_dec/inc
> API (as done in patches 1/2) should allow all mechanisms to coexist
> naturally without fighting each other on the static key state, and also
> handle the reference count for e.g. active probes or bpf programs if
> there's no similar internal mechanism.
>
> Patches 3 and 4 implement the static keys for the two mm fault injection
> sites in slab and page allocators. For a quick demonstration I've run a
> VM and the simple test from [1] that stresses the slab allocator and got
> this time before the series:
>
> real0m8.349s
> user0m0.694s
> sys 0m7.648s
>
> with perf showing
>
>0.61% nonexistent [kernel.kallsyms] [k] should_failslab.constprop.0
>0.00% nonexistent [kernel.kallsyms] [k] should_fail_alloc_page
>
>
> ▒
>
> And after the series
>
> real0m7.924s
> user0m0.727s
> sys 0m7.191s
Is "user" increase a measurement error or it's real?
Otherwise, nice savings!
Re: [PATCH 10/20] function_graph: Have the instances use their own ftrace_ops for filtering
On Fri, 31 May 2024 23:50:23 +0900
Masami Hiramatsu (Google) wrote:
> So is it similar to the fprobe/kprobe, use shared signle ftrace_ops,
> but keep each fgraph has own hash table?
Sort of.
I created helper functions in ftrace that lets you have a "manager
ftrace_ops" that will be used to assign to ftrace (with the function
that will demultiplex), and then you can have "subops" that can be
assigned that is per user. Here's a glimpse of the code:
/**
* ftrace_startup_subops - enable tracing for subops of an ops
* @ops: Manager ops (used to pick all the functions of its subops)
* @subops: A new ops to add to @ops
* @command: Extra commands to use to enable tracing
*
* The @ops is a manager @ops that has the filter that includes all the
functions
* that its list of subops are tracing. Adding a new @subops will add the
* functions of @subops to @ops.
*/
int ftrace_startup_subops(struct ftrace_ops *ops, struct ftrace_ops *subops,
int command)
{
struct ftrace_hash *filter_hash;
struct ftrace_hash *notrace_hash;
struct ftrace_hash *save_filter_hash;
struct ftrace_hash *save_notrace_hash;
int size_bits;
int ret;
if (unlikely(ftrace_disabled))
return -ENODEV;
ftrace_ops_init(ops);
ftrace_ops_init(subops);
/* Make everything canonical (Just in case!) */
if (!ops->func_hash->filter_hash)
ops->func_hash->filter_hash = EMPTY_HASH;
if (!ops->func_hash->notrace_hash)
ops->func_hash->notrace_hash = EMPTY_HASH;
if (!subops->func_hash->filter_hash)
subops->func_hash->filter_hash = EMPTY_HASH;
if (!subops->func_hash->notrace_hash)
subops->func_hash->notrace_hash = EMPTY_HASH;
/* For the first subops to ops just enable it normally */
if (list_empty(>subop_list)) {
/* Just use the subops hashes */
filter_hash = copy_hash(subops->func_hash->filter_hash);
notrace_hash = copy_hash(subops->func_hash->notrace_hash);
if (!filter_hash || !notrace_hash) {
free_ftrace_hash(filter_hash);
free_ftrace_hash(notrace_hash);
return -ENOMEM;
}
save_filter_hash = ops->func_hash->filter_hash;
save_notrace_hash = ops->func_hash->notrace_hash;
ops->func_hash->filter_hash = filter_hash;
ops->func_hash->notrace_hash = notrace_hash;
list_add(>list, >subop_list);
ret = ftrace_startup(ops, command);
if (ret < 0) {
list_del(>list);
ops->func_hash->filter_hash = save_filter_hash;
ops->func_hash->notrace_hash = save_notrace_hash;
free_ftrace_hash(filter_hash);
free_ftrace_hash(notrace_hash);
} else {
free_ftrace_hash(save_filter_hash);
free_ftrace_hash(save_notrace_hash);
subops->flags |= FTRACE_OPS_FL_ENABLED;
}
return ret;
}
/*
* Here there's already something attached. Here are the rules:
* o If either filter_hash is empty then the final stays empty
* o Otherwise, the final is a superset of both hashes
* o If either notrace_hash is empty then the final stays empty
* o Otherwise, the final is an intersection between the hashes
*/
if (ops->func_hash->filter_hash == EMPTY_HASH ||
subops->func_hash->filter_hash == EMPTY_HASH) {
filter_hash = EMPTY_HASH;
} else {
size_bits = max(ops->func_hash->filter_hash->size_bits,
subops->func_hash->filter_hash->size_bits);
filter_hash = alloc_and_copy_ftrace_hash(size_bits,
ops->func_hash->filter_hash);
if (!filter_hash)
return -ENOMEM;
ret = append_hash(_hash, subops->func_hash->filter_hash);
if (ret < 0) {
free_ftrace_hash(filter_hash);
return ret;
}
}
if (ops->func_hash->notrace_hash == EMPTY_HASH ||
subops->func_hash->notrace_hash == EMPTY_HASH) {
notrace_hash = EMPTY_HASH;
} else {
size_bits = max(ops->func_hash->filter_hash->size_bits,
subops->func_hash->filter_hash->size_bits);
notrace_hash = alloc_ftrace_hash(size_bits);
if (!notrace_hash) {
free_ftrace_hash(filter_hash);
return -ENOMEM;
}
ret = intersect_hash(_hash, ops->func_hash->filter_hash,
Re: [PATCH] mctp i2c: Add rx trace
Hi Tal, kernel test robot noticed the following build errors: [auto build test ERROR on linus/master] [also build test ERROR on horms-ipvs/master v6.10-rc1 next-20240531] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Tal-Yacobi/mctp-i2c-Add-rx-trace/20240528-223555 base: linus/master patch link: https://lore.kernel.org/r/20240528143420.742611-1-talycb8%40gmail.com patch subject: [PATCH] mctp i2c: Add rx trace config: hexagon-randconfig-r052-20240531 (https://download.01.org/0day-ci/archive/20240601/202406010530.skassbs4-...@intel.com/config) compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project bafda89a0944d947fc4b3b5663185e07a397ac30) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240601/202406010530.skassbs4-...@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-kbuild-all/202406010530.skassbs4-...@intel.com/ All errors (new ones prefixed by >>, old ones prefixed by <<): WARNING: modpost: missing MODULE_DESCRIPTION() in vmlinux.o WARNING: modpost: missing MODULE_DESCRIPTION() in kernel/locking/locktorture.o WARNING: modpost: missing MODULE_DESCRIPTION() in kernel/rcu/rcutorture.o WARNING: modpost: missing MODULE_DESCRIPTION() in kernel/rcu/rcuscale.o WARNING: modpost: missing MODULE_DESCRIPTION() in kernel/rcu/refscale.o WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/nls_cp855.o WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/nls_cp857.o WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/nls_cp863.o WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/nls_cp864.o WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/nls_cp865.o WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/nls_cp869.o WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/nls_cp936.o WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/nls_ascii.o WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/nls_iso8859-5.o WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/nls_iso8859-7.o WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/nls_iso8859-9.o WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/nls_iso8859-15.o WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/mac-celtic.o WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/mac-inuit.o WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/mac-romanian.o WARNING: modpost: missing MODULE_DESCRIPTION() in fs/jbd2/jbd2.o WARNING: modpost: missing MODULE_DESCRIPTION() in fs/fat/fat.o WARNING: modpost: missing MODULE_DESCRIPTION() in crypto/xor.o WARNING: modpost: missing MODULE_DESCRIPTION() in lib/math/rational.o WARNING: modpost: missing MODULE_DESCRIPTION() in lib/crypto/libarc4.o WARNING: modpost: missing MODULE_DESCRIPTION() in lib/zlib_inflate/zlib_inflate.o WARNING: modpost: missing MODULE_DESCRIPTION() in lib/zlib_deflate/zlib_deflate.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/video/backlight/rt4831-backlight.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/video/fbdev/vfb.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/regulator/rt4831-regulator.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/reset/hisilicon/hi6220_reset.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/tty/goldfish.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/char/lp.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/char/ppdev.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/gpu/drm/gud/gud.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/gpu/drm/drm_panel_orientation_quirks.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/gpu/drm/udl/udl.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/base/regmap/regmap-spmi.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/base/regmap/regmap-w1.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mfd/pcf50633-gpio.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mfd/rt4831.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/dax/dax.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/host/ohci-exynos.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/host/xhci-pci-renesas.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/serial/usb_debug.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/serial/navman.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/input/matrix-keymap.o WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/i2c/busses/i2c-qup.o WARNING: modpost: missing MODULE_DESCRIPTION() in
Re: [PATCH 0/2] livepatch: Add compiler optimization disclaimer/docs
On 5/31/24 07:23, Miroslav Benes wrote: > Hi, > > On Tue, 21 Jul 2020, Joe Lawrence wrote: > >> In light of [PATCH] Revert "kbuild: use -flive-patching when >> CONFIG_LIVEPATCH is enabled" [1], we should add some loud disclaimers >> and explanation of the impact compiler optimizations have on >> livepatching. >> >> The first commit provides detailed explanations and examples. The list >> was taken mostly from Miroslav's LPC talk a few years back. This is a >> bit rough, so corrections and additional suggestions welcome. Expanding >> upon the source-based patching approach would be helpful, too. >> >> The second commit adds a small README.rst file in the livepatch samples >> directory pointing the reader to the doc introduced in the first commit. >> >> I didn't touch the livepatch kselftests yet as I'm still unsure about >> how to best account for IPA here. We could add the same README.rst >> disclaimer here, too, but perhaps we have a chance to do something more. >> Possibilities range from checking for renamed functions as part of their >> build, or the selftest scripts, or even adding something to the kernel >> API. I think we'll have a better idea after reviewing the compiler >> considerations doc. > > thanks to Marcos for resurrecting this. > > Joe, do you have an updated version by any chance? Some things have > changed since July 2020 so it calls for a new review. If there was an > improved version, it would be easier. If not, no problem at all. > Yea, it's been a little while :) I don't have any newer version than this one. I can rebase, apply all of the v1 suggestions, and see where it stands. LMK if you can think of any specifics that could be added. For example, CONFIG_KERNEL_IBT will be driving some changes soon, whether it be klp-convert for source-based patches or vmlinux.o binary comparison for kpatch-build. I can push a v2 with a few changes, but IIRC, last time we reviewed this, it kinda begged the question of how someone is creating the livepatch in the first place. As long as we're fine holding that thought for a while longer, this doc may still be useful by itself. -- Joe
Re: [PATCH] livepatch: introduce klp_func called interface
On Tue, May 21, 2024 at 08:34:46AM +0200, Miroslav Benes wrote: > Hello, > > On Mon, 20 May 2024, zhang warden wrote: > > > > > > > > On May 20, 2024, at 14:46, Miroslav Benes wrote: > > > > > > Hi, > > > > > > On Mon, 20 May 2024, Wardenjohn wrote: > > > > > >> Livepatch module usually used to modify kernel functions. > > >> If the patched function have bug, it may cause serious result > > >> such as kernel crash. > > >> > > >> This is a kobject attribute of klp_func. Sysfs interface named > > >> "called" is introduced to livepatch which will be set as true > > >> if the patched function is called. > > >> > > >> /sys/kernel/livepatchcalled > > >> > > >> This value "called" is quite necessary for kernel stability > > >> assurance for livepatching module of a running system. > > >> Testing process is important before a livepatch module apply to > > >> a production system. With this interface, testing process can > > >> easily find out which function is successfully called. > > >> Any testing process can make sure they have successfully cover > > >> all the patched function that changed with the help of this interface. > > > > > > Even easier is to use the existing tracing infrastructure in the kernel > > > (ftrace for example) to track the new function. You can obtain much more > > > information with that than the new attribute provides. > > > > > > Regards, > > > Miroslav > > Hi Miroslav > > > > First, in most cases, testing process is should be automated, which make > > using existing tracing infrastructure inconvenient. > > could you elaborate, please? We use ftrace exactly for this purpose and > our testing process is also more or less automated. > > > Second, livepatch is > > already use ftrace for functional replacement, I don’t think it is a > > good choice of using kernel tracing tool to trace a patched function. > > Why? > > > At last, this attribute can be thought of as a state of a livepatch > > function. It is a state, like the "patched" "transition" state of a > > klp_patch. Adding this state will not break the state consistency of > > livepatch. > > Yes, but the information you get is limited compared to what is available > now. You would obtain the information that a patched function was called > but ftrace could also give you the context and more. > > It would not hurt to add a new attribute per se but I am wondering about > the reason and its usage. Once we have it, the commit message should be > improved based on that. > I agree with Miroslav about using ftrace, or Dan in the other thread about using gcov if even more granular coverage is needed. Admittedly, I would have to go find documentation to remember how to do either, but at least I'd be using well-tested facilities designed for this exact purpose. Adding these attributes to livepatch sysfs would be expedient and probably easier for us to use, but imposes a recurring burden on us to maintain and test (where is the documentation and kselftest for this new interface?). Or, we could let the other tools handle all of that for us. Perhaps if someone already has an off-the-shelf script that is using ftrace to monitor livepatched code, it could be donated to Documentation/livepatch/? I can ask our QE folks if they have something like this. Regards, -- Joe
Re: [PATCH 02/12] wifi: wcn36xx: make use of QCOM_FW_HELPER
Dmitry Baryshkov writes: > Make the driver use qcom_fw_helper to autodetect the path to the > calibration data file. > > Signed-off-by: Dmitry Baryshkov Not a fan of one sentence commit messages. It would be nice to explain a bit more in the commit message, for instance answering to the question 'why?' and maybe provide a short example how this is supposed to work? -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH 01/12] soc: qcom: add firmware name helper
Bjorn Andersson writes: > On Mon, May 27, 2024 at 02:42:44PM GMT, Dmitry Baryshkov wrote: > >> On Thu, 23 May 2024 at 01:48, Bjorn Andersson >> wrote: >> > >> > On Tue, May 21, 2024 at 03:08:31PM +0200, Dmitry Baryshkov wrote: >> > > On Tue, 21 May 2024 at 13:20, Kalle Valo wrote: >> > > > >> > > > Dmitry Baryshkov writes: >> > > > >> > > > > On Tue, 21 May 2024 at 12:52, wrote: >> > > > >> >> > > > >> On 21/05/2024 11:45, Dmitry Baryshkov wrote: >> > > > >> > Qualcomm platforms have different sets of the firmware files, >> > > > >> > which >> > > > >> > differ from platform to platform (and from board to board, due to >> > > > >> > the >> > > > >> > embedded signatures). Rather than listing all the firmware files, >> > > > >> > including full paths, in the DT, provide a way to determine >> > > > >> > firmware >> > > > >> > path based on the root DT node compatible. >> > > > >> >> > > > >> Ok this looks quite over-engineered but necessary to handle the >> > > > >> legacy, >> > > > >> but I really think we should add a way to look for a board-specific >> > > > >> path >> > > > >> first and fallback to those SoC specific paths. >> > > > > >> > > > > Again, CONFIG_FW_LOADER_USER_HELPER => delays. >> > > > >> > > > To me this also looks like very over-engineered, can you elaborate more >> > > > why this is needed? Concrete examples would help to understand better. >> > > >> > > Sure. During the meeting last week Arnd suggested evaluating if we can >> > > drop firmware-name from the board DT files. Several reasons for that: >> > > - DT should describe the hardware, not the Linux-firmware locations >> > > - having firmware name in DT complicates updating the tree to use >> > > different firmware API (think of mbn vs mdt vs any other format) >> > > - If the DT gets supplied by the vendor (e.g. for >> > > SystemReady-certified devices), there should be a sync between the >> > > vendor's DT, linux kernel and the rootfs. Dropping firmware names from >> > > DT solves that by removing one piece of the equation >> > > >> > > Now for the complexity of the solution. Each SoC family has their own >> > > firmware set. This includes firmware for the DSPs, for modem, WiFi >> > > bits, GPU shader, etc. >> > > For the development boards these devices are signed by the testing key >> > > and the actual signature is not validated against the root of trust >> > > certificate. >> > > For the end-user devices the signature is actually validated against >> > > the bits fused to the SoC during manufacturing process. CA certificate >> > > (and thus the fuses) differ from vendor to vendor (and from the device >> > > to device) >> > > >> > > Not all of the firmware files are a part of the public linux-firmware >> > > tree. However we need to support the rootfs bundled with the firmware >> > > for different platforms (both public and vendor). The non-signed files >> > > come from the Adreno GPU and can be shared between platforms. All >> > > other files are SoC-specific and in some cases device-specific. >> > > >> > > So for example the SDM845 db845c (open device) loads following firmware >> > > files: >> > > Not signed: >> > > - qcom/a630_sqe.fw >> > > - qcom/a630_gmu.bin >> > > >> > > Signed, will work for any non-secured sdm845 device: >> > > - qcom/sdm845/a630_zap.mbn >> > > - qcom/sdm845/adsp.mbn >> > > - qcom/sdm845/cdsp.mbn >> > > - qcom/sdm485/mba.mbn >> > > - qcom/sdm845/modem.mbn >> > > - qcom/sdm845/wlanmdsp.mbn (loaded via TQFTP) >> > > - qcom/venus-5.2/venus.mbn >> > > >> > > Signed, works only for DB845c. >> > > - qcom/sdm845/Thundercomm/db845c/slpi.mbn >> > > >> > > In comparison, the SDM845 Pixel-3 phone (aka blueline) should load the >> > > following firmware files: >> > > - qcom/a630_sqe.fw (the same, non-signed file) >> > > - qcom/a630_gmu.bin (the same, non-signed file) >> > > - qcom/sdm845/Google/blueline/a630_zap.mbn >> > >> > How do you get from "a630_zap.mbn" to this? By extending the lookup >> > table for every target, or what am I missing? >> >> More or less so. Matching the root OF node gives us the firmware >> location, then it gets prepended to all firmware targets. Not an ideal >> solution, as there is no fallback support, but at least it gives us >> some points to discuss (and to decide whether to move to some >> particular direction or to abandon the idea completely, making Arnd >> unhappy again). >> > > I understand the desire to not put linux-firmware-specific paths in the > DeviceTree Me too. > but I think I'm less keen on having a big lookup table in the > kernel... Yeah, also for me this feels wrong. But on the other hand I don't have anything better to suggest either... -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCHv7 bpf-next 0/9] uprobe: uretprobe speed up
On Thu, May 23, 2024 at 5:11 AM Jiri Olsa wrote: > > hi, > as part of the effort on speeding up the uprobes [0] coming with > return uprobe optimization by using syscall instead of the trap > on the uretprobe trampoline. > > The speed up depends on instruction type that uprobe is installed > and depends on specific HW type, please check patch 1 for details. > > Patches 1-8 are based on bpf-next/master, but patch 2 and 3 are > apply-able on linux-trace.git tree probes/for-next branch. > Patch 9 is based on man-pages master. > > v7 changes: > - fixes in man page [Alejandro Colomar] > - fixed patch #1 fixes tag [Oleg] > > Also available at: > https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git > uretprobe_syscall > > thanks, > jirka > > > Notes to check list items in Documentation/process/adding-syscalls.rst: > > - System Call Alternatives > New syscall seems like the best way in here, because we need > just to quickly enter kernel with no extra arguments processing, > which we'd need to do if we decided to use another syscall. > > - Designing the API: Planning for Extension > The uretprobe syscall is very specific and most likely won't be > extended in the future. > > At the moment it does not take any arguments and even if it does > in future, it's allowed to be called only from trampoline prepared > by kernel, so there'll be no broken user. > > - Designing the API: Other Considerations > N/A because uretprobe syscall does not return reference to kernel > object. > > - Proposing the API > Wiring up of the uretprobe system call is in separate change, > selftests and man page changes are part of the patchset. > > - Generic System Call Implementation > There's no CONFIG option for the new functionality because it > keeps the same behaviour from the user POV. > > - x86 System Call Implementation > It's 64-bit syscall only. > > - Compatibility System Calls (Generic) > N/A uretprobe syscall has no arguments and is not supported > for compat processes. > > - Compatibility System Calls (x86) > N/A uretprobe syscall is not supported for compat processes. > > - System Calls Returning Elsewhere > N/A. > > - Other Details > N/A. > > - Testing > Adding new bpf selftests and ran ltp on top of this change. > > - Man Page > Attached. > > - Do not call System Calls in the Kernel > N/A. > > > [0] https://lore.kernel.org/bpf/ZeCXHKJ--iYYbmLj@krava/ > --- > Jiri Olsa (8): > x86/shstk: Make return uprobe work with shadow stack > uprobe: Wire up uretprobe system call > uprobe: Add uretprobe syscall to speed up return probe > selftests/x86: Add return uprobe shadow stack test > selftests/bpf: Add uretprobe syscall test for regs integrity > selftests/bpf: Add uretprobe syscall test for regs changes > selftests/bpf: Add uretprobe syscall call from user space test > selftests/bpf: Add uretprobe shadow stack test > Masami, Steven, It seems like the series is ready to go in. Are you planning to take the first 4 patches through your linux-trace tree? > arch/x86/entry/syscalls/syscall_64.tbl | 1 + > arch/x86/include/asm/shstk.h| 4 + > arch/x86/kernel/shstk.c | 16 > arch/x86/kernel/uprobes.c | 124 > - > include/linux/syscalls.h| 2 + > include/linux/uprobes.h | 3 + > include/uapi/asm-generic/unistd.h | 5 +- > kernel/events/uprobes.c | 24 -- > kernel/sys_ni.c | 2 + > tools/include/linux/compiler.h | 4 + > tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c | 123 > - > tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c | 385 > +++ > tools/testing/selftests/bpf/progs/uprobe_syscall.c | 15 > tools/testing/selftests/bpf/progs/uprobe_syscall_executed.c | 17 > tools/testing/selftests/x86/test_shadow_stack.c | 145 > ++ > 15 files changed, 860 insertions(+), 10 deletions(-) > create mode 100644 tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c > create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall.c > create mode 100644 > tools/testing/selftests/bpf/progs/uprobe_syscall_executed.c > > Jiri Olsa (1): > man2: Add uretprobe syscall page > > man/man2/uretprobe.2 | 56 > > 1 file changed, 56 insertions(+) > create mode 100644 man/man2/uretprobe.2
Re: [PATCH v5 5/7] remoteproc: core: support of the tee interface
On Thu, May 30, 2024 at 09:42:26AM +0200, Arnaud POULIQUEN wrote:
> Hello Mathieu,
>
> On 5/29/24 22:35, Mathieu Poirier wrote:
> > On Wed, May 29, 2024 at 09:13:26AM +0200, Arnaud POULIQUEN wrote:
> >> Hello Mathieu,
> >>
> >> On 5/28/24 23:30, Mathieu Poirier wrote:
> >>> On Tue, May 21, 2024 at 10:09:59AM +0200, Arnaud Pouliquen wrote:
> 1) on start:
> - Using the TEE loader, the resource table is loaded by an external
> entity.
> In such case the resource table address is not find from the firmware but
> provided by the TEE remoteproc framework.
> Use the rproc_get_loaded_rsc_table instead of rproc_find_loaded_rsc_table
> - test that rproc->cached_table is not null before performing the memcpy
>
> 2)on stop
> The use of the cached_table seems mandatory:
> - during recovery sequence to have a snapshot of the resource table
> resources used,
> - on stop to allow for the deinitialization of resources after the
> the remote processor has been shutdown.
> However if the TEE interface is being used, we first need to unmap the
> table_ptr before setting it to rproc->cached_table.
> The update of rproc->table_ptr to rproc->cached_table is performed in
> tee_remoteproc.
>
> Signed-off-by: Arnaud Pouliquen
> ---
> drivers/remoteproc/remoteproc_core.c | 31 +---
> 1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> index 42bca01f3bde..3a642151c983 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1267,6 +1267,7 @@ EXPORT_SYMBOL(rproc_resource_cleanup);
> static int rproc_set_rsc_table_on_start(struct rproc *rproc, const
> struct firmware *fw)
> {
> struct resource_table *loaded_table;
> +struct device *dev = >dev;
>
> /*
> * The starting device has been given the rproc->cached_table
> as the
> @@ -1276,12 +1277,21 @@ static int rproc_set_rsc_table_on_start(struct
> rproc *rproc, const struct firmwa
> * this information to device memory. We also update the
> table_ptr so
> * that any subsequent changes will be applied to the loaded
> version.
> */
> -loaded_table = rproc_find_loaded_rsc_table(rproc, fw);
> -if (loaded_table) {
> -memcpy(loaded_table, rproc->cached_table,
> rproc->table_sz);
> -rproc->table_ptr = loaded_table;
> +if (rproc->tee_interface) {
> +loaded_table = rproc_get_loaded_rsc_table(rproc,
> >table_sz);
> +if (IS_ERR(loaded_table)) {
> +dev_err(dev, "can't get resource table\n");
> +return PTR_ERR(loaded_table);
> +}
> +} else {
> +loaded_table = rproc_find_loaded_rsc_table(rproc, fw);
> }
>
> +if (loaded_table && rproc->cached_table)
> +memcpy(loaded_table, rproc->cached_table,
> rproc->table_sz);
> +
> >>>
> >>> Why is this not part of the else {} above as it was the case before this
> >>> patch?
> >>> And why was an extra check for ->cached_table added?
> >>
> >> Here we have to cover 2 use cases if rproc->tee_interface is set.
> >> 1) The remote processor is in stop state
> >> - loaded_table points to the resource table in the remote memory and
> >> - rproc->cached_table is null
> >> => no memcopy
> >> 2) crash recovery
> >> - loaded_table points to the resource table in the remote memory
> >> - rproc-cached_table point to a copy of the resource table
> >
> > A cached_table exists because it was created in
> > rproc_reset_rsc_table_on_stop().
> > But as the comment says [1], that part of the code was meant to be used for
> > the
> > attach()/detach() use case. Mixing both will become extremely confusing and
> > impossible to maintain.
>
> i am not sure to understand your point here... the cached_table table was
> already existing for the "normal" case[2]. Seems to me that the cache table is
> needed on stop in all scenarios.
>
> [2]
> https://elixir.bootlin.com/linux/v4.20.17/source/drivers/remoteproc/remoteproc_core.c#L1402
>
> >
> > I think the TEE scenario should be as similar as the "normal" one where TEE
> > is
> > not involved. To that end, I suggest to create a cached_table in
> > tee_rproc_parse_fw(), exactly the same way it is done in
> > rproc_elf_load_rsc_table(). That way the code path in
> > rproc_set_rsc_table_on_start() become very similar and we have a
> > cached_table to
> > work with when the remote processor is recovered. In fact we may not need
> >
Re: [PATCH RFC 3/4] mm, slab: add static key for should_failslab()
On Fri, May 31, 2024 at 9:44 AM Alexei Starovoitov
wrote:
>
> On Fri, May 31, 2024 at 2:33 AM Vlastimil Babka wrote:
> >
> > Since commit 4f6923fbb352 ("mm: make should_failslab always available for
> > fault injection") should_failslab() is unconditionally a noinline
> > function. This adds visible overhead to the slab allocation hotpath,
> > even if the function is empty. With CONFIG_FAILSLAB=y there's additional
> > overhead when the functionality is not enabled by a boot parameter or
> > debugfs.
> >
> > The overhead can be eliminated with a static key around the callsite.
> > Fault injection and error injection frameworks can now be told that the
> > this function has a static key associated, and are able to enable and
> > disable it accordingly.
> >
> > Signed-off-by: Vlastimil Babka
> > ---
> > mm/failslab.c | 2 +-
> > mm/slab.h | 3 +++
> > mm/slub.c | 10 +++---
> > 3 files changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/mm/failslab.c b/mm/failslab.c
> > index ffc420c0e767..878fd08e5dac 100644
> > --- a/mm/failslab.c
> > +++ b/mm/failslab.c
> > @@ -9,7 +9,7 @@ static struct {
> > bool ignore_gfp_reclaim;
> > bool cache_filter;
> > } failslab = {
> > - .attr = FAULT_ATTR_INITIALIZER,
> > + .attr = FAULT_ATTR_INITIALIZER_KEY(_failslab_active.key),
> > .ignore_gfp_reclaim = true,
> > .cache_filter = false,
> > };
> > diff --git a/mm/slab.h b/mm/slab.h
> > index 5f8f47c5bee0..792e19cb37b8 100644
> > --- a/mm/slab.h
> > +++ b/mm/slab.h
> > @@ -11,6 +11,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > /*
> > * Internal slab definitions
> > @@ -160,6 +161,8 @@ static_assert(IS_ALIGNED(offsetof(struct slab,
> > freelist), sizeof(freelist_aba_t)
> > */
> > #define slab_page(s) folio_page(slab_folio(s), 0)
> >
> > +DECLARE_STATIC_KEY_FALSE(should_failslab_active);
> > +
> > /*
> > * If network-based swap is enabled, sl*b must keep track of whether pages
> > * were allocated from pfmemalloc reserves.
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 0809760cf789..3bb579760a37 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -3874,13 +3874,15 @@ static __always_inline void
> > maybe_wipe_obj_freeptr(struct kmem_cache *s,
> > 0, sizeof(void *));
> > }
> >
> > +DEFINE_STATIC_KEY_FALSE(should_failslab_active);
> > +
> > noinline int should_failslab(struct kmem_cache *s, gfp_t gfpflags)
> > {
> > if (__should_failslab(s, gfpflags))
> > return -ENOMEM;
> > return 0;
> > }
> > -ALLOW_ERROR_INJECTION(should_failslab, ERRNO);
> > +ALLOW_ERROR_INJECTION_KEY(should_failslab, ERRNO, _failslab_active);
> >
> > static __fastpath_inline
> > struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s, gfp_t flags)
> > @@ -3889,8 +3891,10 @@ struct kmem_cache *slab_pre_alloc_hook(struct
> > kmem_cache *s, gfp_t flags)
> >
> > might_alloc(flags);
> >
> > - if (unlikely(should_failslab(s, flags)))
> > - return NULL;
> > + if (static_branch_unlikely(_failslab_active)) {
> > + if (should_failslab(s, flags))
> > + return NULL;
> > + }
>
> makes sense.
> Acked-by: Alexei Starovoitov
>
> Do you have any microbenchmark numbers before/after this optimization?
There are numbers in the cover letter for the entire series:
https://lore.kernel.org/lkml/20240531-fault-injection-statickeys-v1-0-a513fd0a9...@suse.cz/
Re: [PATCH] tracing/fprobe: Support raw tracepoint events on modules
Hi Masami,
kernel test robot noticed the following build errors:
[auto build test ERROR on linus/master]
[also build test ERROR on v6.10-rc1 next-20240531]
[cannot apply to rostedt-trace/for-next rostedt-trace/for-next-urgent]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Masami-Hiramatsu-Google/tracing-fprobe-Support-raw-tracepoint-events-on-modules/20240531-175013
base: linus/master
patch link:
https://lore.kernel.org/r/171714888633.198965.13093663631481169611.stgit%40devnote2
patch subject: [PATCH] tracing/fprobe: Support raw tracepoint events on modules
config: s390-defconfig
(https://download.01.org/0day-ci/archive/20240601/202406010034.fsnp9rsq-...@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project
bafda89a0944d947fc4b3b5663185e07a397ac30)
reproduce (this is a W=1 build):
(https://download.01.org/0day-ci/archive/20240601/202406010034.fsnp9rsq-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Closes:
https://lore.kernel.org/oe-kbuild-all/202406010034.fsnp9rsq-...@intel.com/
All errors (new ones prefixed by >>):
In file included from kernel/tracepoint.c:5:
In file included from include/linux/module.h:19:
In file included from include/linux/elf.h:6:
In file included from arch/s390/include/asm/elf.h:173:
In file included from arch/s390/include/asm/mmu_context.h:11:
In file included from arch/s390/include/asm/pgalloc.h:18:
In file included from include/linux/mm.h:2253:
include/linux/vmstat.h:500:43: warning: arithmetic between different
enumeration types ('enum zone_stat_item' and 'enum numa_stat_item')
[-Wenum-enum-conversion]
500 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
|~ ^
501 |item];
|
include/linux/vmstat.h:507:43: warning: arithmetic between different
enumeration types ('enum zone_stat_item' and 'enum numa_stat_item')
[-Wenum-enum-conversion]
507 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
|~ ^
508 |NR_VM_NUMA_EVENT_ITEMS +
|~~
include/linux/vmstat.h:514:36: warning: arithmetic between different
enumeration types ('enum node_stat_item' and 'enum lru_list')
[-Wenum-enum-conversion]
514 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~ ^ ~~~
include/linux/vmstat.h:519:43: warning: arithmetic between different
enumeration types ('enum zone_stat_item' and 'enum numa_stat_item')
[-Wenum-enum-conversion]
519 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
|~ ^
520 |NR_VM_NUMA_EVENT_ITEMS +
|~~
include/linux/vmstat.h:528:43: warning: arithmetic between different
enumeration types ('enum zone_stat_item' and 'enum numa_stat_item')
[-Wenum-enum-conversion]
528 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
|~ ^
529 |NR_VM_NUMA_EVENT_ITEMS +
|~~
>> kernel/tracepoint.c:751:34: error: no member named
>> '__start___tracepoints_ptrs' in 'struct module'
751 |
for_each_tracepoint_range(mod->__start___tracepoints_ptrs,
| ~~~ ^
5 warnings and 1 error generated.
vim +751 kernel/tracepoint.c
738
739 void for_each_module_tracepoint(void (*fct)(struct tracepoint *tp, void
*priv),
740 void *priv)
741 {
742 struct tp_module *tp_mod;
743 struct module *mod;
744
745 if (!mod->num_tracepoints)
746 return;
747
748 mutex_lock(_module_list_mutex);
749 list_for_each_entry(tp_mod, _module_list, list) {
750 mod = tp_mod->mod;
> 751
> for_each_tracepoint_range(mod->__start___tracepoints_ptrs,
752 mod->tracepoints_ptrs + mod->num_tracepoints,
753 fct, priv);
754 }
755 mutex_unlock(_module_list_mutex);
756 }
757 #endif /* CONFIG_MODULES */
758
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH RFC 3/4] mm, slab: add static key for should_failslab()
On Fri, May 31, 2024 at 2:33 AM Vlastimil Babka wrote:
>
> Since commit 4f6923fbb352 ("mm: make should_failslab always available for
> fault injection") should_failslab() is unconditionally a noinline
> function. This adds visible overhead to the slab allocation hotpath,
> even if the function is empty. With CONFIG_FAILSLAB=y there's additional
> overhead when the functionality is not enabled by a boot parameter or
> debugfs.
>
> The overhead can be eliminated with a static key around the callsite.
> Fault injection and error injection frameworks can now be told that the
> this function has a static key associated, and are able to enable and
> disable it accordingly.
>
> Signed-off-by: Vlastimil Babka
> ---
> mm/failslab.c | 2 +-
> mm/slab.h | 3 +++
> mm/slub.c | 10 +++---
> 3 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/mm/failslab.c b/mm/failslab.c
> index ffc420c0e767..878fd08e5dac 100644
> --- a/mm/failslab.c
> +++ b/mm/failslab.c
> @@ -9,7 +9,7 @@ static struct {
> bool ignore_gfp_reclaim;
> bool cache_filter;
> } failslab = {
> - .attr = FAULT_ATTR_INITIALIZER,
> + .attr = FAULT_ATTR_INITIALIZER_KEY(_failslab_active.key),
> .ignore_gfp_reclaim = true,
> .cache_filter = false,
> };
> diff --git a/mm/slab.h b/mm/slab.h
> index 5f8f47c5bee0..792e19cb37b8 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -11,6 +11,7 @@
> #include
> #include
> #include
> +#include
>
> /*
> * Internal slab definitions
> @@ -160,6 +161,8 @@ static_assert(IS_ALIGNED(offsetof(struct slab, freelist),
> sizeof(freelist_aba_t)
> */
> #define slab_page(s) folio_page(slab_folio(s), 0)
>
> +DECLARE_STATIC_KEY_FALSE(should_failslab_active);
> +
> /*
> * If network-based swap is enabled, sl*b must keep track of whether pages
> * were allocated from pfmemalloc reserves.
> diff --git a/mm/slub.c b/mm/slub.c
> index 0809760cf789..3bb579760a37 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -3874,13 +3874,15 @@ static __always_inline void
> maybe_wipe_obj_freeptr(struct kmem_cache *s,
> 0, sizeof(void *));
> }
>
> +DEFINE_STATIC_KEY_FALSE(should_failslab_active);
> +
> noinline int should_failslab(struct kmem_cache *s, gfp_t gfpflags)
> {
> if (__should_failslab(s, gfpflags))
> return -ENOMEM;
> return 0;
> }
> -ALLOW_ERROR_INJECTION(should_failslab, ERRNO);
> +ALLOW_ERROR_INJECTION_KEY(should_failslab, ERRNO, _failslab_active);
>
> static __fastpath_inline
> struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s, gfp_t flags)
> @@ -3889,8 +3891,10 @@ struct kmem_cache *slab_pre_alloc_hook(struct
> kmem_cache *s, gfp_t flags)
>
> might_alloc(flags);
>
> - if (unlikely(should_failslab(s, flags)))
> - return NULL;
> + if (static_branch_unlikely(_failslab_active)) {
> + if (should_failslab(s, flags))
> + return NULL;
> + }
makes sense.
Acked-by: Alexei Starovoitov
Do you have any microbenchmark numbers before/after this optimization?
Re: [PATCH RFC 0/4] static key support for error injection functions
Hi,
On Fri, May 31, 2024 at 11:33:31AM +0200, Vlastimil Babka wrote:
> Incomplete, help needed from ftrace/kprobe and bpf folks.
> - the generic error injection using kretprobes with
> override_function_with_return is handled in patch 2. The
> ALLOW_ERROR_INJECTION() annotation is extended so that static key
> address can be passed, and the framework controls it when error
> injection is enabled or disabled in debugfs for the function.
>
> There are two more users I know of but am not familiar enough to fix up
> myself. I hope people that are more familiar can help me here.
>
> - ftrace seems to be using override_function_with_return from
> #define ftrace_override_function_with_return but I found no place
> where the latter is used. I assume it might be hidden behind more
> macro magic? But the point is if ftrace can be instructed to act like
> an error injection, it would also have to use some form of metadata
> (from patch 2 presumably?) to get to the static key and control it.
I don't think you've missed anything; nothing currently uses
ftrace_override_function_with_return(). I added that in commit:
94d095ffa0e16bb7 ("ftrace: abstract DYNAMIC_FTRACE_WITH_ARGS accesses")
... so that it was possible to do anything that was possible with
FTRACE_WITH_REGS and/or kprobes, under the expectation that we might
want to move fault injection and BPF probes over to fprobes in future,
as ftrace/fprobes is generally faster than kprobes (e.g. for
architectures that can't do KPROBES_ON_FTRACE or OPTPROBES).
That's just the mechanism for the handler to use; I'd expect whatever
registered the handler to be responsible for flipping the static key,
and I don't think anything needs to change within ftrace itself.
> If ftrace can only observe the function being called, maybe it
> wouldn't be wrong to just observe nothing if the static key isn't
> enabled because nobody is doing the fault injection?
Yep, that sounds right to me.
Mark.
Re: [PATCH 10/20] function_graph: Have the instances use their own ftrace_ops for filtering
On Fri, 31 May 2024 02:03:46 -0400
Steven Rostedt wrote:
> On Fri, 31 May 2024 12:12:41 +0900
> Masami Hiramatsu (Google) wrote:
>
> > On Thu, 30 May 2024 22:30:57 -0400
> > Steven Rostedt wrote:
> >
> > > On Fri, 24 May 2024 22:37:02 -0400
> > > Steven Rostedt wrote:
> > >
> > > > From: "Steven Rostedt (VMware)"
> > > >
> > > > Allow for instances to have their own ftrace_ops part of the fgraph_ops
> > > > that makes the funtion_graph tracer filter on the set_ftrace_filter file
> > > > of the instance and not the top instance.
> > > >
> > > > Note that this also requires to update ftrace_graph_func() to call new
> > > > function_graph_enter_ops() instead of function_graph_enter() so that
> > > > it avoid pushing on shadow stack multiple times on the same function.
> > >
> > > So I found a major design flaw in this patch.
> > >
> > > >
> > > > Co-developed with Masami Hiramatsu:
> > > > Link:
> > > > https://lore.kernel.org/linux-trace-kernel/171509102088.162236.15758883237657317789.stgit@devnote2
> > > >
> > > > Signed-off-by: Steven Rostedt (VMware)
> > > > Signed-off-by: Masami Hiramatsu (Google)
> > > > Signed-off-by: Steven Rostedt (Google)
> > > > ---
> > >
> > > > diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> > > > index 8da0e66ca22d..998558cb8f15 100644
> > > > --- a/arch/x86/kernel/ftrace.c
> > > > +++ b/arch/x86/kernel/ftrace.c
> > > > @@ -648,9 +648,24 @@ void ftrace_graph_func(unsigned long ip, unsigned
> > > > long parent_ip,
> > > >struct ftrace_ops *op, struct ftrace_regs *fregs)
> > > > {
> > > > struct pt_regs *regs = >regs;
> > > > - unsigned long *stack = (unsigned long
> > > > *)kernel_stack_pointer(regs);
> > > > + unsigned long *parent = (unsigned long
> > > > *)kernel_stack_pointer(regs);
> > > > + struct fgraph_ops *gops = container_of(op, struct fgraph_ops,
> > > > ops);
> > > > + int bit;
> > > > +
> > > > + if (unlikely(ftrace_graph_is_dead()))
> > > > + return;
> > > > +
> > > > + if (unlikely(atomic_read(>tracing_graph_pause)))
> > > > + return;
> > > >
> > > > - prepare_ftrace_return(ip, (unsigned long *)stack, 0);
> > > > + bit = ftrace_test_recursion_trylock(ip, *parent);
> > > > + if (bit < 0)
> > > > + return;
> > > > +
> > > > + if (!function_graph_enter_ops(*parent, ip, 0, parent, gops))
> > >
> > > So each registered graph ops has its own ftrace_ops which gets
> > > registered with ftrace, so this function does get called in a loop (by
> > > the ftrace iterator function). This means that we would need that code
> > > to detect the function_graph_enter_ops() getting called multiple times
> > > for the same function. This means each fgraph_ops gits its own retstack
> > > on the shadow stack.
> >
> > Ah, that is my concern and the reason why I added bitmap and stack reuse
> > code in the ftrace_push_return_trace().
> >
> > >
> > > I find this a waste of shadow stack resources, and also complicates the
> > > code with having to deal with tail calls and all that.
> > >
> > > BUT! There's good news! I also thought about another way of handling
> > > this. I have something working, but requires a bit of rewriting the
> > > code. I should have something out in a day or two.
> >
> > Hmm, I just wonder why you don't reocver my bitmap check and stack
> > reusing code. Are there any problem on it? (Too complicated?)
> >
>
> I actually dislike the use of ftrace itself to do the loop. I rather
> have fgraph be in control of it.
(actually, I agreed with you, looping in ftrace may cause trouble)
>
> I've come up with a new "subops" assignment, where you can have one
> ftrace_ops represent multiple sub ftrace_ops. Basically, each fgraph
> ops can register its own ftrace_ops under a single graph_ops
> ftrace_ops. The graph_ops will be used to decide what functions call
> the callback, and then the callback does the multiplexing.
So is it similar to the fprobe/kprobe, use shared signle ftrace_ops,
but keep each fgraph has own hash table?
> This removes the need to touch the architecture code. It can also be
> used by fprobes to handle the attachments to functions for several
> different sets of callbacks.
>
> I'll send out patches soon.
OK, I'll wait for that.
Thank you!
>
> -- Steve
>
--
Masami Hiramatsu (Google)
Re: [PATCH 0/3] tracing: Fix some selftest issues
On Fri, 31 May 2024 03:24:25 -0400 Steven Rostedt wrote: > On Fri, 31 May 2024 11:37:21 +0900 > Masami Hiramatsu (Google) wrote: > > > So, in summary, it is designed to be a module. Steve, I think these tests > > should be kept as modules. There are many reason to do so. > > > > - This test is designed to be used as module. > > - This can conflict with other boot time selftest if it is embedded. > > - We can make these tests and boot time selftest mutable exclusive but > >if we make these tests as modules, we can build and run both tests > >safely. > > - Embedding these tests leave new events when the kernel boot, which > >user must be cleaned up by manual. > > > > What would you think? > > I was mostly following what Ingo told me long ago, where having it > built in is just one more way to test it ;-) Ah, would you mean embedding the module is also a part of tests? > > But that said, from your first patch, you show the stack dump and > mention: > > > Since the kprobes and synth event generation tests adds and enable > > generated events in init_module() and delete it in exit_module(), > > if we make it as built-in, those events are left in kernel and cause > > kprobe event self-test failure. > > But you don't explain what exactly the conflict is. What about those > events causes kprobe selftests to fail? The major conflict happens when the boot-time test cleans up the kprobe events by dyn_events_release_all(_kprobe_ops); And I removed it by [3/3] patch in this series :) because it does not needed and not confirmed there is no other kprobe events when the test starts. Also the warning message are redundant so I removed it by [2/3]. So without this [1/3], if we apply [2/3] and [3/3], the problem will be mitigated, but I think the root cause is that these modules are built-in. Thank you, > > > -- Steve -- Masami Hiramatsu (Google)
Re: [PATCH] livepatch: introduce klp_func called interface
> And for the unlikely branch, isn’t the complier will compile this branch > into a cold branch that will do no harm to the function performance? The test (cmp insn or something like that) still needs to be there. Since there is only a simple assignment in the branch, the compiler may just choose not to have a cold branch in this case. The only difference is in which case you would jump here. You can see for yourself (and prove me wrong if it comes to it). Miroslav
Re: [PATCH] livepatch: introduce klp_func called interface
> On May 31, 2024, at 15:21, Miroslav Benes wrote: > > Hi, > > On Fri, 31 May 2024, zhang warden wrote: > > you have not replied to my questions/feedback yet. > > Also, I do not think that unlikely changes anything here. It is a simple > branch after all. > > Miroslav Hi Miroslav, Sorry for my carelessness. I apologise for my ignorance. > Second, livepatch is > already use ftrace for functional replacement, I don’t think it is a > good choice of using kernel tracing tool to trace a patched function. I admit that ftrace can use for tracing the new patched function. But for some cases, user who what to know the state of this function can easily cat the 'called' interface. It is more convenient than using ftrace to trace the state. And for the unlikely branch, isn’t the complier will compile this branch into a cold branch that will do no harm to the function performance? Regards, Wardenjohn
Re: [PATCH v6 2/5] mfd: add driver for Marvell 88PM886 PMIC
Lee Jones, 2024-05-31T11:24:52+01:00: > Are you planning on seeing to Mark's review comments? Indeed, I'm hoping that I will be able to send it over the weekend. K. B.
Re: [PATCH v3 1/3] arm64: dts: qcom: pm7250b: Add node for PMIC VBUS booster
On 30.05.2024 5:05 PM, Luca Weiss wrote: > Add the required DTS node for the USB VBUS output regulator, which is > available on PM7250B. This will provide the VBUS source to connected > peripherals. > > Reviewed-by: Bryan O'Donoghue > Signed-off-by: Luca Weiss > --- Reviewed-by: Konrad Dybcio Konrad
Re: [PATCH] arm64: dts: qcom: sm8550-samsung-q5q: fix typo
On 31/05/2024 14:05, David Wronek wrote:
> It looks like "cdsp_mem" was pasted in the license header by accident.
> Fix the typo by removing it.
>
> Signed-off-by: David Wronek
> ---
> arch/arm64/boot/dts/qcom/sm8550-samsung-q5q.dts | 2 +-
Fixes: ba2c082a401f ("arm64: dts: qcom: sm8550: Add support for Samsung Galaxy
Z Fold5")
Reviewed-by: Krzysztof Kozlowski
Best regards,
Krzysztof
Re: [PATCH 0/2] livepatch: Add compiler optimization disclaimer/docs
Hi, On Tue, 21 Jul 2020, Joe Lawrence wrote: > In light of [PATCH] Revert "kbuild: use -flive-patching when > CONFIG_LIVEPATCH is enabled" [1], we should add some loud disclaimers > and explanation of the impact compiler optimizations have on > livepatching. > > The first commit provides detailed explanations and examples. The list > was taken mostly from Miroslav's LPC talk a few years back. This is a > bit rough, so corrections and additional suggestions welcome. Expanding > upon the source-based patching approach would be helpful, too. > > The second commit adds a small README.rst file in the livepatch samples > directory pointing the reader to the doc introduced in the first commit. > > I didn't touch the livepatch kselftests yet as I'm still unsure about > how to best account for IPA here. We could add the same README.rst > disclaimer here, too, but perhaps we have a chance to do something more. > Possibilities range from checking for renamed functions as part of their > build, or the selftest scripts, or even adding something to the kernel > API. I think we'll have a better idea after reviewing the compiler > considerations doc. thanks to Marcos for resurrecting this. Joe, do you have an updated version by any chance? Some things have changed since July 2020 so it calls for a new review. If there was an improved version, it would be easier. If not, no problem at all. Miroslav
Re: [PATCH v6 2/5] mfd: add driver for Marvell 88PM886 PMIC
On Sat, 04 May 2024, Karel Balej wrote: > Marvell 88PM886 is a PMIC which provides various functions such as > onkey, battery, charger and regulators. It is found for instance in the > samsung,coreprimevelte smartphone with which this was tested. Implement > basic support to allow for the use of regulators and onkey. > > Signed-off-by: Karel Balej > --- > > Notes: > v6: > - Address Lee's comments: > - Don't break long line in the power off handler. > - Set PLATFORM_DEVID_NONE. This should be safe with respect to > collisions since there are no known devices with more than one of > these PMICs, plus given their general purpose nature it is unlikely > that there would ever be. Also include the corresponding header. > - Move all defines to the header. > - Give the base page's maximum register its real name. > - Set irq_base to 0. > v5: > - Address Mark's feedback: > - Move regmap config back out of the header and rename it. Also lower > its maximum register based on what's actually used in the downstream > code. > RFC v4: > - Use MFD_CELL_* macros. > - Address Lee's feedback: > - Do not define regmap_config.val_bits and .reg_bits. > - Drop everything regulator related except mfd_cell (regmap > initialization, IDs enum etc.). Drop pm886_initialize_subregmaps. > - Do not store regmap pointers as an array as there is now only one > regmap. Also drop the corresponding enum. > - Move regmap_config to the header as it is needed in the regulators > driver. > - pm886_chip.whoami -> chip_id > - Reword chip ID mismatch error message and print the ID as > hexadecimal. > - Fix includes in include/linux/88pm886.h. > - Drop the pm886_irq_number enum and define the (for the moment) only > IRQ explicitly. > - Have only one MFD cell for all regulators as they are now registered > all at once in the regulators driver. > - Reword commit message. > - Make device table static and remove comma after the sentinel to signal > that nothing should come after it. > RFC v3: > - Drop onkey cell .of_compatible. > - Rename LDO page offset and regmap to REGULATORS. > RFC v2: > - Remove some abstraction. > - Sort includes alphabetically and add linux/of.h. > - Depend on OF, remove of_match_ptr and add MODULE_DEVICE_TABLE. > - Use more temporaries and break long lines. > - Do not initialize ret in probe. > - Use the wakeup-source DT property. > - Rename ret to err. > - Address Lee's comments: > - Drop patched in presets for base regmap and related defines. > - Use full sentences in comments. > - Remove IRQ comment. > - Define regmap_config member values. > - Rename data to sys_off_data. > - Add _PMIC suffix to Kconfig. > - Use dev_err_probe. > - Do not store irq_data. > - s/WHOAMI/CHIP_ID > - Drop LINUX part of include guard name. > - Merge in the regulator series modifications in order to have more > devices and modify the commit message accordingly. Changes with > respect to the original regulator series patches: > - ret -> err > - Add temporary for dev in pm88x_initialize_subregmaps. > - Drop of_compatible for the regulators. > - Do not duplicate LDO regmap for bucks. > - Rewrite commit message. > > drivers/mfd/88pm886.c | 148 > drivers/mfd/Kconfig | 12 +++ > drivers/mfd/Makefile| 1 + > include/linux/mfd/88pm886.h | 69 + > 4 files changed, 230 insertions(+) > create mode 100644 drivers/mfd/88pm886.c > create mode 100644 include/linux/mfd/88pm886.h I don't see any more issues. Are you planning on seeing to Mark's review comments? -- Lee Jones [李琼斯]
Re: [PATCH v2] dt-bindings: remoteproc: k3-dsp: correct optional sram properties for AM62A SoCs
On 30/05/2024 18:48, Hari Nagalla wrote: > The C7xv-dsp on AM62A have 32KB L1 I-cache and a 64KB L1 D-cache. It > does not have an addressable l1dram . So, remove this optional sram > property from the bindings to fix device tree build warnings. > > Signed-off-by: Hari Nagalla > --- > Changes from v1 to v2: > *) Kept back memory-regions property, as it is unrelated to this patch >correcting the sram property for AM62A C7xv-dsp nodes. > > DT binding check log: > https://paste.sr.ht/~hnagalla/cb26237560a572a17c0874b687353e00b400285a > > v1: https://lore.kernel.org/all/20230810110545.11644-1-hnaga...@ti.com/ > > .../bindings/remoteproc/ti,k3-dsp-rproc.yaml | 15 ++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git > a/Documentation/devicetree/bindings/remoteproc/ti,k3-dsp-rproc.yaml > b/Documentation/devicetree/bindings/remoteproc/ti,k3-dsp-rproc.yaml > index 9768db8663eb..771cfceb5458 100644 > --- a/Documentation/devicetree/bindings/remoteproc/ti,k3-dsp-rproc.yaml > +++ b/Documentation/devicetree/bindings/remoteproc/ti,k3-dsp-rproc.yaml > @@ -111,7 +111,6 @@ else: > properties: >compatible: > enum: > - - ti,am62a-c7xv-dsp >- ti,j721e-c71-dsp >- ti,j721s2-c71-dsp >then: > @@ -124,6 +123,20 @@ else: > items: >- const: l2sram >- const: l1dram > + else: > +if: Please use allOf (move top one to bottom) and define separate ifs for each variant instead of nesting them. Best regards, Krzysztof
Re: [PATCH 0/3] tracing: Fix some selftest issues
On Fri, 31 May 2024 11:37:21 +0900 Masami Hiramatsu (Google) wrote: > So, in summary, it is designed to be a module. Steve, I think these tests > should be kept as modules. There are many reason to do so. > > - This test is designed to be used as module. > - This can conflict with other boot time selftest if it is embedded. > - We can make these tests and boot time selftest mutable exclusive but >if we make these tests as modules, we can build and run both tests >safely. > - Embedding these tests leave new events when the kernel boot, which >user must be cleaned up by manual. > > What would you think? I was mostly following what Ingo told me long ago, where having it built in is just one more way to test it ;-) But that said, from your first patch, you show the stack dump and mention: > Since the kprobes and synth event generation tests adds and enable > generated events in init_module() and delete it in exit_module(), > if we make it as built-in, those events are left in kernel and cause > kprobe event self-test failure. But you don't explain what exactly the conflict is. What about those events causes kprobe selftests to fail? -- Steve
Re: [PATCH] livepatch: introduce klp_func called interface
Hi, On Fri, 31 May 2024, zhang warden wrote: > > Hi Bros, > > How about my patch? I do think it is a viable feature to show the state > of the patched function. If we add an unlikely branch test before we set > the 'called' state, once this function is called, there maybe no > negative effect to the performance. you have not replied to my questions/feedback yet. Also, I do not think that unlikely changes anything here. It is a simple branch after all. Miroslav
Re: [PATCH v2] sched/rt: Clean up usage of rt_task()
On 2024-05-30 12:10:44 [+0100], Qais Yousef wrote: > > This is not consistent because IMHO the clock setup & slack should be > > handled equally. So I am asking the sched folks for a policy and I am > > leaning towards looking at task-policy in this case instead of prio > > because you shouldn't do anything that can delay. > > Can't we do that based on is_soft/is_hard flag in hrtimer struct when we apply > the slack in hrtimer_set_expires_range_ns() instead? We need to decide on a policy first. You don't want to add overhead on each invocation plus some in-kernel ask for delta. ->is_soft is not a good criteria. Sebastian
Re: [PATCH 10/20] function_graph: Have the instances use their own ftrace_ops for filtering
On Fri, 31 May 2024 12:12:41 +0900
Masami Hiramatsu (Google) wrote:
> On Thu, 30 May 2024 22:30:57 -0400
> Steven Rostedt wrote:
>
> > On Fri, 24 May 2024 22:37:02 -0400
> > Steven Rostedt wrote:
> >
> > > From: "Steven Rostedt (VMware)"
> > >
> > > Allow for instances to have their own ftrace_ops part of the fgraph_ops
> > > that makes the funtion_graph tracer filter on the set_ftrace_filter file
> > > of the instance and not the top instance.
> > >
> > > Note that this also requires to update ftrace_graph_func() to call new
> > > function_graph_enter_ops() instead of function_graph_enter() so that
> > > it avoid pushing on shadow stack multiple times on the same function.
> >
> > So I found a major design flaw in this patch.
> >
> > >
> > > Co-developed with Masami Hiramatsu:
> > > Link:
> > > https://lore.kernel.org/linux-trace-kernel/171509102088.162236.15758883237657317789.stgit@devnote2
> > >
> > > Signed-off-by: Steven Rostedt (VMware)
> > > Signed-off-by: Masami Hiramatsu (Google)
> > > Signed-off-by: Steven Rostedt (Google)
> > > ---
> >
> > > diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> > > index 8da0e66ca22d..998558cb8f15 100644
> > > --- a/arch/x86/kernel/ftrace.c
> > > +++ b/arch/x86/kernel/ftrace.c
> > > @@ -648,9 +648,24 @@ void ftrace_graph_func(unsigned long ip, unsigned
> > > long parent_ip,
> > > struct ftrace_ops *op, struct ftrace_regs *fregs)
> > > {
> > > struct pt_regs *regs = >regs;
> > > - unsigned long *stack = (unsigned long *)kernel_stack_pointer(regs);
> > > + unsigned long *parent = (unsigned long *)kernel_stack_pointer(regs);
> > > + struct fgraph_ops *gops = container_of(op, struct fgraph_ops, ops);
> > > + int bit;
> > > +
> > > + if (unlikely(ftrace_graph_is_dead()))
> > > + return;
> > > +
> > > + if (unlikely(atomic_read(>tracing_graph_pause)))
> > > + return;
> > >
> > > - prepare_ftrace_return(ip, (unsigned long *)stack, 0);
> > > + bit = ftrace_test_recursion_trylock(ip, *parent);
> > > + if (bit < 0)
> > > + return;
> > > +
> > > + if (!function_graph_enter_ops(*parent, ip, 0, parent, gops))
> >
> > So each registered graph ops has its own ftrace_ops which gets
> > registered with ftrace, so this function does get called in a loop (by
> > the ftrace iterator function). This means that we would need that code
> > to detect the function_graph_enter_ops() getting called multiple times
> > for the same function. This means each fgraph_ops gits its own retstack
> > on the shadow stack.
>
> Ah, that is my concern and the reason why I added bitmap and stack reuse
> code in the ftrace_push_return_trace().
>
> >
> > I find this a waste of shadow stack resources, and also complicates the
> > code with having to deal with tail calls and all that.
> >
> > BUT! There's good news! I also thought about another way of handling
> > this. I have something working, but requires a bit of rewriting the
> > code. I should have something out in a day or two.
>
> Hmm, I just wonder why you don't reocver my bitmap check and stack
> reusing code. Are there any problem on it? (Too complicated?)
>
I actually dislike the use of ftrace itself to do the loop. I rather
have fgraph be in control of it.
I've come up with a new "subops" assignment, where you can have one
ftrace_ops represent multiple sub ftrace_ops. Basically, each fgraph
ops can register its own ftrace_ops under a single graph_ops
ftrace_ops. The graph_ops will be used to decide what functions call
the callback, and then the callback does the multiplexing.
This removes the need to touch the architecture code. It can also be
used by fprobes to handle the attachments to functions for several
different sets of callbacks.
I'll send out patches soon.
-- Steve
Re: [PATCH 10/20] function_graph: Have the instances use their own ftrace_ops for filtering
On Thu, 30 May 2024 22:30:57 -0400
Steven Rostedt wrote:
> On Fri, 24 May 2024 22:37:02 -0400
> Steven Rostedt wrote:
>
> > From: "Steven Rostedt (VMware)"
> >
> > Allow for instances to have their own ftrace_ops part of the fgraph_ops
> > that makes the funtion_graph tracer filter on the set_ftrace_filter file
> > of the instance and not the top instance.
> >
> > Note that this also requires to update ftrace_graph_func() to call new
> > function_graph_enter_ops() instead of function_graph_enter() so that
> > it avoid pushing on shadow stack multiple times on the same function.
>
> So I found a major design flaw in this patch.
>
> >
> > Co-developed with Masami Hiramatsu:
> > Link:
> > https://lore.kernel.org/linux-trace-kernel/171509102088.162236.15758883237657317789.stgit@devnote2
> >
> > Signed-off-by: Steven Rostedt (VMware)
> > Signed-off-by: Masami Hiramatsu (Google)
> > Signed-off-by: Steven Rostedt (Google)
> > ---
>
> > diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> > index 8da0e66ca22d..998558cb8f15 100644
> > --- a/arch/x86/kernel/ftrace.c
> > +++ b/arch/x86/kernel/ftrace.c
> > @@ -648,9 +648,24 @@ void ftrace_graph_func(unsigned long ip, unsigned long
> > parent_ip,
> >struct ftrace_ops *op, struct ftrace_regs *fregs)
> > {
> > struct pt_regs *regs = >regs;
> > - unsigned long *stack = (unsigned long *)kernel_stack_pointer(regs);
> > + unsigned long *parent = (unsigned long *)kernel_stack_pointer(regs);
> > + struct fgraph_ops *gops = container_of(op, struct fgraph_ops, ops);
> > + int bit;
> > +
> > + if (unlikely(ftrace_graph_is_dead()))
> > + return;
> > +
> > + if (unlikely(atomic_read(>tracing_graph_pause)))
> > + return;
> >
> > - prepare_ftrace_return(ip, (unsigned long *)stack, 0);
> > + bit = ftrace_test_recursion_trylock(ip, *parent);
> > + if (bit < 0)
> > + return;
> > +
> > + if (!function_graph_enter_ops(*parent, ip, 0, parent, gops))
>
> So each registered graph ops has its own ftrace_ops which gets
> registered with ftrace, so this function does get called in a loop (by
> the ftrace iterator function). This means that we would need that code
> to detect the function_graph_enter_ops() getting called multiple times
> for the same function. This means each fgraph_ops gits its own retstack
> on the shadow stack.
Ah, that is my concern and the reason why I added bitmap and stack reuse
code in the ftrace_push_return_trace().
>
> I find this a waste of shadow stack resources, and also complicates the
> code with having to deal with tail calls and all that.
>
> BUT! There's good news! I also thought about another way of handling
> this. I have something working, but requires a bit of rewriting the
> code. I should have something out in a day or two.
Hmm, I just wonder why you don't reocver my bitmap check and stack
reusing code. Are there any problem on it? (Too complicated?)
Thanks,
>
> -- Steve
>
>
> > + *parent = (unsigned long)_to_handler;
> > +
> > + ftrace_test_recursion_unlock(bit);
> > }
> > #endif
> >
>
--
Masami Hiramatsu (Google)
Re: [PATCH 0/3] tracing: Fix some selftest issues
On Wed, 29 May 2024 11:01:43 -0500 Tom Zanussi wrote: > Hi Masami, > > On Wed, 2024-05-29 at 08:38 +0900, Masami Hiramatsu wrote: > > On Wed, 29 May 2024 01:46:40 +0900 > > Masami Hiramatsu (Google) wrote: > > > > > On Mon, 27 May 2024 19:29:07 -0400 > > > Steven Rostedt wrote: > > > > > > > On Sun, 26 May 2024 19:10:57 +0900 > > > > "Masami Hiramatsu (Google)" wrote: > > > > > > > > > Hi, > > > > > > > > > > Here is a series of some fixes/improvements for the test > > > > > modules and boot > > > > > time selftest of kprobe events. I found a WARNING message with > > > > > some boot > > > > > time selftest configuration, which came from the combination of > > > > > embedded > > > > > kprobe generate API tests module and ftrace boot-time selftest. > > > > > So the main > > > > > problem is that the test module should not be built-in. But I > > > > > also think > > > > > this WARNING message is useless (because there are warning > > > > > messages already) > > > > > and the cleanup code is redundant. This series fixes those > > > > > issues. > > > > > > > > Note, when I enable trace tests as builtin instead of modules, I > > > > just > > > > disable the bootup self tests when it detects this. This helps > > > > with > > > > doing tests via config options than having to add user space code > > > > that > > > > loads modules. > > > > > > > > Could you do something similar? > > > > > > OK, in that case, I would like to move the test cleanup code in > > > module_exit function into the end of module_init function. > > > It looks there is no reason to split those into 2 parts. > > > > Wait, I would like to hear Tom's opinion. I found following usage > > comments > > in the code. > > > > * Following that are a few examples using the created events to test > > * various ways of tracing a synthetic event. > > * > > * To test, select CONFIG_SYNTH_EVENT_GEN_TEST and build the module. > > * Then: > > * > > * # insmod kernel/trace/synth_event_gen_test.ko > > * # cat /sys/kernel/tracing/trace > > * > > * You should see several events in the trace buffer - > > * "create_synth_test", "empty_synth_test", and several instances of > > * "gen_synth_test". > > * > > * To remove the events, remove the module: > > * > > * # rmmod synth_event_gen_test > > > > Tom, is that intended behavior ? and are you expected to reuse these > > events outside of the module? e.g. load the test module and run some > > test script in user space which uses those events? > > > > Yeah, this module was meant as a sample module showing how to create > and generate synthetic events in-kernel. > > So the interested user insmods the module, looks at the trace stream > and sees, ok the events are there as expected, so it does work, great, > let's remove the module to get rid of them and go write our own. > > Having both the creation and cleanup in module_init() wouldn't allow > the user the opportunity to do that i.e. verify the results by reading > the trace file. So, in summary, it is designed to be a module. Steve, I think these tests should be kept as modules. There are many reason to do so. - This test is designed to be used as module. - This can conflict with other boot time selftest if it is embedded. - We can make these tests and boot time selftest mutable exclusive but if we make these tests as modules, we can build and run both tests safely. - Embedding these tests leave new events when the kernel boot, which user must be cleaned up by manual. What would you think? Thank you, > > Tom > > > As far as I can see, those tests are not intended to be embedded in > > the > > kernel because those are expected to be removed. > > > > Thank you, > > > > > > > > Thank you, > > > > > > > > > > > -- Steve > > > > > > > > > > > > > > > > > > Thank you, > > > > > > > > > > --- > > > > > > > > > > Masami Hiramatsu (Google) (3): > > > > > tracing: Build event generation tests only as modules > > > > > tracing/kprobe: Remove unneeded WARN_ON_ONCE() in > > > > > selftests > > > > > tracing/kprobe: Remove cleanup code unrelated to selftest > > > > > > > > > > > > > > -- > > > Masami Hiramatsu (Google) > > > > > > -- Masami Hiramatsu (Google)
Re: [PATCH 10/20] function_graph: Have the instances use their own ftrace_ops for filtering
On Fri, 24 May 2024 22:37:02 -0400
Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)"
>
> Allow for instances to have their own ftrace_ops part of the fgraph_ops
> that makes the funtion_graph tracer filter on the set_ftrace_filter file
> of the instance and not the top instance.
>
> Note that this also requires to update ftrace_graph_func() to call new
> function_graph_enter_ops() instead of function_graph_enter() so that
> it avoid pushing on shadow stack multiple times on the same function.
So I found a major design flaw in this patch.
>
> Co-developed with Masami Hiramatsu:
> Link:
> https://lore.kernel.org/linux-trace-kernel/171509102088.162236.15758883237657317789.stgit@devnote2
>
> Signed-off-by: Steven Rostedt (VMware)
> Signed-off-by: Masami Hiramatsu (Google)
> Signed-off-by: Steven Rostedt (Google)
> ---
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index 8da0e66ca22d..998558cb8f15 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -648,9 +648,24 @@ void ftrace_graph_func(unsigned long ip, unsigned long
> parent_ip,
> struct ftrace_ops *op, struct ftrace_regs *fregs)
> {
> struct pt_regs *regs = >regs;
> - unsigned long *stack = (unsigned long *)kernel_stack_pointer(regs);
> + unsigned long *parent = (unsigned long *)kernel_stack_pointer(regs);
> + struct fgraph_ops *gops = container_of(op, struct fgraph_ops, ops);
> + int bit;
> +
> + if (unlikely(ftrace_graph_is_dead()))
> + return;
> +
> + if (unlikely(atomic_read(>tracing_graph_pause)))
> + return;
>
> - prepare_ftrace_return(ip, (unsigned long *)stack, 0);
> + bit = ftrace_test_recursion_trylock(ip, *parent);
> + if (bit < 0)
> + return;
> +
> + if (!function_graph_enter_ops(*parent, ip, 0, parent, gops))
So each registered graph ops has its own ftrace_ops which gets
registered with ftrace, so this function does get called in a loop (by
the ftrace iterator function). This means that we would need that code
to detect the function_graph_enter_ops() getting called multiple times
for the same function. This means each fgraph_ops gits its own retstack
on the shadow stack.
I find this a waste of shadow stack resources, and also complicates the
code with having to deal with tail calls and all that.
BUT! There's good news! I also thought about another way of handling
this. I have something working, but requires a bit of rewriting the
code. I should have something out in a day or two.
-- Steve
> + *parent = (unsigned long)_to_handler;
> +
> + ftrace_test_recursion_unlock(bit);
> }
> #endif
>
Re: [PATCH] livepatch: introduce klp_func called interface
Hi Bros, How about my patch? I do think it is a viable feature to show the state of the patched function. If we add an unlikely branch test before we set the 'called' state, once this function is called, there maybe no negative effect to the performance. Please give me some advice. Regards, Wardenjohn
Re: [PATCH net-next V2] virtio-net: synchronize operstate with admin state on up/down
On Thu, May 30, 2024 at 9:09 PM Michael S. Tsirkin wrote:
>
> On Thu, May 30, 2024 at 06:29:51PM +0800, Jason Wang wrote:
> > On Thu, May 30, 2024 at 2:10 PM Michael S. Tsirkin wrote:
> > >
> > > On Thu, May 30, 2024 at 11:20:55AM +0800, Jason Wang wrote:
> > > > This patch synchronize operstate with admin state per RFC2863.
> > > >
> > > > This is done by trying to toggle the carrier upon open/close and
> > > > synchronize with the config change work. This allows propagate status
> > > > correctly to stacked devices like:
> > > >
> > > > ip link add link enp0s3 macvlan0 type macvlan
> > > > ip link set link enp0s3 down
> > > > ip link show
> > > >
> > > > Before this patch:
> > > >
> > > > 3: enp0s3: mtu 1500 qdisc pfifo_fast state DOWN
> > > > mode DEFAULT group default qlen 1000
> > > > link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff
> > > > ..
> > > > 5: macvlan0@enp0s3: mtu 1500
> > > > qdisc noqueue state UP mode DEFAULT group default qlen 1000
> > > > link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff
> > > >
> > > > After this patch:
> > > >
> > > > 3: enp0s3: mtu 1500 qdisc pfifo_fast state DOWN
> > > > mode DEFAULT group default qlen 1000
> > > > link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff
> > > > ...
> > > > 5: macvlan0@enp0s3: mtu 1500
> > > > qdisc noqueue state LOWERLAYERDOWN mode DEFAULT group default qlen 1000
> > > > link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff
> > > >
> > > > Cc: Venkat Venkatsubra
> > > > Cc: Gia-Khanh Nguyen
> > > > Reviewed-by: Xuan Zhuo
> > > > Acked-by: Michael S. Tsirkin
> > > > Signed-off-by: Jason Wang
> > > > ---
> > > > Changes since V1:
> > > > - rebase
> > > > - add ack/review tags
> > >
> > >
> > >
> > >
> > >
> > > > ---
> > > > drivers/net/virtio_net.c | 94 +++-
> > > > 1 file changed, 63 insertions(+), 31 deletions(-)
> > > >
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index 4a802c0ea2cb..69e4ae353c51 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -433,6 +433,12 @@ struct virtnet_info {
> > > > /* The lock to synchronize the access to refill_enabled */
> > > > spinlock_t refill_lock;
> > > >
> > > > + /* Is config change enabled? */
> > > > + bool config_change_enabled;
> > > > +
> > > > + /* The lock to synchronize the access to config_change_enabled */
> > > > + spinlock_t config_change_lock;
> > > > +
> > > > /* Work struct for config space updates */
> > > > struct work_struct config_work;
> > > >
> > >
> > >
> > > But we already have dev->config_lock and dev->config_enabled.
> > >
> > > And it actually works better - instead of discarding config
> > > change events it defers them until enabled.
> > >
> >
> > Yes but then both virtio-net driver and virtio core can ask to enable
> > and disable and then we need some kind of synchronization which is
> > non-trivial.
>
> Well for core it happens on bring up path before driver works
> and later on tear down after it is gone.
> So I do not think they ever do it at the same time.
For example, there could be a suspend/resume when the admin state is down.
>
>
> > And device enabling on the core is different from bringing the device
> > up in the networking subsystem. Here we just delay to deal with the
> > config change interrupt on ndo_open(). (E.g try to ack announce is
> > meaningless when the device is down).
> >
> > Thanks
>
> another thing is that it is better not to re-read all config
> on link up if there was no config interrupt - less vm exits.
Yes, but it should not matter much as it's done in the ndo_open().
Thanks
>
> --
> MST
>
Re: [PATCHv6 bpf-next 1/9] x86/shstk: Make return uprobe work with shadow stack
On Tue, 2024-05-21 at 12:48 +0200, Jiri Olsa wrote:
> Currently the application with enabled shadow stack will crash
> if it sets up return uprobe. The reason is the uretprobe kernel
> code changes the user space task's stack, but does not update
> shadow stack accordingly.
>
> Adding new functions to update values on shadow stack and using
> them in uprobe code to keep shadow stack in sync with uretprobe
> changes to user stack.
>
> Fixes: 8b1c23543436 ("x86/shstk: Add return uprobe support")
> Signed-off-by: Jiri Olsa
> ---
Acked-by: Rick Edgecombe
Re: [PATCH v8 0/5] soc: qcom: add in-kernel pd-mapper implementation
I've tested this applied on top of kernel 6.8.11 on an X13s over the past week and it's been working well. -- classabbyamp
Re: [PATCH v2 2/6] livepatch: Add klp-convert tool
On Thu, May 16, 2024 at 03:30:05PM +0200, Lukas Hruska wrote: > Livepatches need to access external symbols which can't be handled > by the normal relocation mechanism. It is needed for two types > of symbols: > > + Symbols which can be local for the original livepatched function. > The alternative implementation in the livepatch sees them > as external symbols. > > + Symbols in modules which are exported via EXPORT_SYMBOL*(). They > must be handled special way otherwise the livepatch module would > depend on the livepatched one. Loading such livepatch would cause > loading the other module as well. > > The address of these symbols can be found via kallsyms. Or they can > be relocated using livepatch specific relocation sections as specified > in Documentation/livepatch/module-elf-format.txt. > > Currently, there is no trivial way to embed the required information as > requested in the final livepatch elf object. klp-convert solves this > problem by using annotations in the elf object to convert the relocation > accordingly to the specification, enabling it to be handled by the > livepatch loader. > > Given the above, create scripts/livepatch to hold tools developed for > livepatches and add source files for klp-convert there. > > Allow to annotate such external symbols in the livepatch by a macro > KLP_RELOC_SYMBOL(). It will create symbol with all needed > metadata. For example: > > extern char *saved_command_line \ > KLP_RELOC_SYMBOL(vmlinux, vmlinux, saved_command_line, 0); > > would create symbol > > $>readelf -r -W : > Relocation section '.rela.text' at offset 0x32e60 contains 10 entries: > Offset Info Type Symbol's Value > Symbol's Name + Addend > [...] > 0068 003c0002 R_X86_64_PC32 > .klp.sym.rela.vmlinux.vmlinux.saved_command_line,0 - 4 > [...] > > Also add scripts/livepatch/klp-convert. The tool transforms symbols > created by KLP_RELOC_SYMBOL() to object specific rela sections > and rela entries which would later be proceed when the livepatch > or the livepatched object is loaded. > > For example, klp-convert would replace the above symbol with: > > $> readelf -r -W > Relocation section '.klp.rela.vmlinux.text' at offset 0x5cb60 contains 1 > entry: > Offset Info Type Symbol's Value > Symbol's Name + Addend > 0068 003c0002 R_X86_64_PC32 > .klp.sym.vmlinux.saved_command_line,0 - 4 > > klp-convert relies on libelf and on a list implementation. Add files > scripts/livepatch/elf.c and scripts/livepatch/elf.h, which are a libelf > interfacing layer and scripts/livepatch/list.h, which is a list > implementation. > > Update Makefiles to correctly support the compilation of the new tool, > update MAINTAINERS file and add a .gitignore file. > > [jpoim...@redhat.com: initial version] > Signed-off-by: Josh Poimboeuf > [joe.lawre...@redhat.com: clean-up and fixes] > Signed-off-by: Joe Lawrence > [lhru...@suse.cz: klp-convert code, minimal approach] > Signed-off-by: Lukas Hruska > Reviewed-by: Marcos Paulo de Souza > --- > MAINTAINERS | 1 + > include/linux/livepatch.h | 19 + > scripts/Makefile| 1 + > scripts/livepatch/.gitignore| 1 + > scripts/livepatch/Makefile | 5 + > scripts/livepatch/elf.c | 817 > scripts/livepatch/elf.h | 73 +++ > scripts/livepatch/klp-convert.c | 284 +++ > scripts/livepatch/klp-convert.h | 23 + > scripts/livepatch/list.h| 391 +++ > 10 files changed, 1615 insertions(+) > create mode 100644 scripts/livepatch/.gitignore > create mode 100644 scripts/livepatch/Makefile > create mode 100644 scripts/livepatch/elf.c > create mode 100644 scripts/livepatch/elf.h > create mode 100644 scripts/livepatch/klp-convert.c > create mode 100644 scripts/livepatch/klp-convert.h > create mode 100644 scripts/livepatch/list.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 130b8b0bd4f7..d2facc1f4e15 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -12618,6 +12618,7 @@ F:include/uapi/linux/livepatch.h > F: kernel/livepatch/ > F: kernel/module/livepatch.c > F: samples/livepatch/ > +F: scripts/livepatch/ > F: tools/testing/selftests/livepatch/ > > LLC (802.2) > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h > index 9b9b38e89563..83bbcd1c43fd 100644 > --- a/include/linux/livepatch.h > +++ b/include/linux/livepatch.h > @@ -235,6 +235,25 @@ int klp_apply_section_relocs(struct module *pmod, > Elf_Shdr *sechdrs, >unsigned int symindex, unsigned int secindex, >const char *objname); > > +/** > + * KLP_RELOC_SYMBOL_POS - define relocation for external symbols > + * > + * @LP_OBJ_NAME: name of the livepatched object
Re: [PATCH v3 3/3] arm64: dts: qcom: sm7225-fairphone-fp4: Enable USB role switching
On Thu, May 30, 2024 at 05:05:49PM +0200, Luca Weiss wrote: > Configure the Type-C and VBUS regulator on PM7250B and wire it up to the > USB PHY, so that USB role and orientation switching works. > > For now USB Power Delivery properties are skipped / disabled, so that > the (presumably) bootloader-configured charger doesn't get messed with >From my understanding the no-pd, typec-power-opmode disable PD negotiation over the USB-C. If a device gets connected to the power supply, it will still negotiate something like 5V / 1.5A. > and we can charge the phone with at least some amount of power. > > Reviewed-by: Konrad Dybcio > Signed-off-by: Luca Weiss > --- > .../dts/qcom/sm6350-sony-xperia-lena-pdx213.dts| 1 + > arch/arm64/boot/dts/qcom/sm6350.dtsi | 50 +++ > arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts | 58 > +- > 3 files changed, 108 insertions(+), 1 deletion(-) > Usually the SoC changes come in a separate patch, but I won't insist on that. Reviewed-by: Dmitry Baryshkov -- With best wishes Dmitry
Re: [PATCH v3 2/3] arm64: dts: qcom: pm7250b: Add a TCPM description
On Thu, May 30, 2024 at 05:05:48PM +0200, Luca Weiss wrote: > Type-C port management functionality lives inside of the PMIC block on > pm7250b. > > The Type-C port management logic controls orientation detection, > vbus/vconn sense and to send/receive Type-C Power Domain messages. > > Reviewed-by: Bryan O'Donoghue > Reviewed-by: Konrad Dybcio > Signed-off-by: Luca Weiss > --- > arch/arm64/boot/dts/qcom/pm7250b.dtsi | 40 > +++ > 1 file changed, 40 insertions(+) > Reviewed-by: Dmitry Baryshkov -- With best wishes Dmitry
Re: [PATCH v3 1/3] arm64: dts: qcom: pm7250b: Add node for PMIC VBUS booster
On Thu, May 30, 2024 at 05:05:47PM +0200, Luca Weiss wrote: > Add the required DTS node for the USB VBUS output regulator, which is > available on PM7250B. This will provide the VBUS source to connected > peripherals. > > Reviewed-by: Bryan O'Donoghue > Signed-off-by: Luca Weiss > --- > arch/arm64/boot/dts/qcom/pm7250b.dtsi | 6 ++ > 1 file changed, 6 insertions(+) > Reviewed-by: Dmitry Baryshkov -- With best wishes Dmitry
Re: [PATCH v5 5/7] remoteproc: core: support of the tee interface
On Thu, May 30, 2024 at 09:42:26AM +0200, Arnaud POULIQUEN wrote:
> Hello Mathieu,
>
> On 5/29/24 22:35, Mathieu Poirier wrote:
> > On Wed, May 29, 2024 at 09:13:26AM +0200, Arnaud POULIQUEN wrote:
> >> Hello Mathieu,
> >>
> >> On 5/28/24 23:30, Mathieu Poirier wrote:
> >>> On Tue, May 21, 2024 at 10:09:59AM +0200, Arnaud Pouliquen wrote:
> 1) on start:
> - Using the TEE loader, the resource table is loaded by an external
> entity.
> In such case the resource table address is not find from the firmware but
> provided by the TEE remoteproc framework.
> Use the rproc_get_loaded_rsc_table instead of rproc_find_loaded_rsc_table
> - test that rproc->cached_table is not null before performing the memcpy
>
> 2)on stop
> The use of the cached_table seems mandatory:
> - during recovery sequence to have a snapshot of the resource table
> resources used,
> - on stop to allow for the deinitialization of resources after the
> the remote processor has been shutdown.
> However if the TEE interface is being used, we first need to unmap the
> table_ptr before setting it to rproc->cached_table.
> The update of rproc->table_ptr to rproc->cached_table is performed in
> tee_remoteproc.
>
> Signed-off-by: Arnaud Pouliquen
> ---
> drivers/remoteproc/remoteproc_core.c | 31 +---
> 1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> index 42bca01f3bde..3a642151c983 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1267,6 +1267,7 @@ EXPORT_SYMBOL(rproc_resource_cleanup);
> static int rproc_set_rsc_table_on_start(struct rproc *rproc, const
> struct firmware *fw)
> {
> struct resource_table *loaded_table;
> +struct device *dev = >dev;
>
> /*
> * The starting device has been given the rproc->cached_table
> as the
> @@ -1276,12 +1277,21 @@ static int rproc_set_rsc_table_on_start(struct
> rproc *rproc, const struct firmwa
> * this information to device memory. We also update the
> table_ptr so
> * that any subsequent changes will be applied to the loaded
> version.
> */
> -loaded_table = rproc_find_loaded_rsc_table(rproc, fw);
> -if (loaded_table) {
> -memcpy(loaded_table, rproc->cached_table,
> rproc->table_sz);
> -rproc->table_ptr = loaded_table;
> +if (rproc->tee_interface) {
> +loaded_table = rproc_get_loaded_rsc_table(rproc,
> >table_sz);
> +if (IS_ERR(loaded_table)) {
> +dev_err(dev, "can't get resource table\n");
> +return PTR_ERR(loaded_table);
> +}
> +} else {
> +loaded_table = rproc_find_loaded_rsc_table(rproc, fw);
> }
>
> +if (loaded_table && rproc->cached_table)
> +memcpy(loaded_table, rproc->cached_table,
> rproc->table_sz);
> +
> >>>
> >>> Why is this not part of the else {} above as it was the case before this
> >>> patch?
> >>> And why was an extra check for ->cached_table added?
> >>
> >> Here we have to cover 2 use cases if rproc->tee_interface is set.
> >> 1) The remote processor is in stop state
> >> - loaded_table points to the resource table in the remote memory and
> >> - rproc->cached_table is null
> >> => no memcopy
> >> 2) crash recovery
> >> - loaded_table points to the resource table in the remote memory
> >> - rproc-cached_table point to a copy of the resource table
> >
> > A cached_table exists because it was created in
> > rproc_reset_rsc_table_on_stop().
> > But as the comment says [1], that part of the code was meant to be used for
> > the
> > attach()/detach() use case. Mixing both will become extremely confusing and
> > impossible to maintain.
>
> i am not sure to understand your point here... the cached_table table was
> already existing for the "normal" case[2]. Seems to me that the cache table is
> needed on stop in all scenarios.
>
> [2]
> https://elixir.bootlin.com/linux/v4.20.17/source/drivers/remoteproc/remoteproc_core.c#L1402
>
> >
> > I think the TEE scenario should be as similar as the "normal" one where TEE
> > is
> > not involved. To that end, I suggest to create a cached_table in
> > tee_rproc_parse_fw(), exactly the same way it is done in
> > rproc_elf_load_rsc_table(). That way the code path in
> > rproc_set_rsc_table_on_start() become very similar and we have a
> > cached_table to
> > work with when the remote processor is recovered. In fact we may not need
> >
Re: [PATCH 2/3] remoteproc: k3-r5: Acquire mailbox handle during probe
On 5/30/24 4:07 AM, Beleswar Padhi wrote:
Acquire the mailbox handle during device probe and do not release handle
in stop/detach routine or error paths. This removes the redundant
requests for mbox handle later during rproc start/attach. This also
allows to defer remoteproc driver's probe if mailbox is not probed yet.
Signed-off-by: Beleswar Padhi
---
drivers/remoteproc/ti_k3_r5_remoteproc.c | 66
1 file changed, 21 insertions(+), 45 deletions(-)
diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c
b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index 26362a509ae3..157e8fd57665 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -391,6 +391,7 @@ static int k3_r5_rproc_request_mbox(struct rproc *rproc)
struct mbox_client *client = >client;
struct device *dev = kproc->dev;
int ret;
+ long err;
client->dev = dev;
client->tx_done = NULL;
@@ -400,10 +401,9 @@ static int k3_r5_rproc_request_mbox(struct rproc *rproc)
kproc->mbox = mbox_request_channel(client, 0);
if (IS_ERR(kproc->mbox)) {
- ret = -EBUSY;
- dev_err(dev, "mbox_request_channel failed: %ld\n",
- PTR_ERR(kproc->mbox));
- return ret;
+ err = PTR_ERR(kproc->mbox);
+ dev_err(dev, "mbox_request_channel failed: %ld\n", err);
+ return (err == -EPROBE_DEFER) ? -EPROBE_DEFER : -EBUSY;
Why turn all other errors into EBUSY? If you just return the error as-is you
can simply make these 3 lines just:
return dev_err_probe(dev, PTR_ERR(kproc->mbox), "mbox_request_channel
failed\n");
}
/*
@@ -552,10 +552,6 @@ static int k3_r5_rproc_start(struct rproc *rproc)
u32 boot_addr;
int ret;
- ret = k3_r5_rproc_request_mbox(rproc);
- if (ret)
- return ret;
-
boot_addr = rproc->bootaddr;
/* TODO: add boot_addr sanity checking */
dev_dbg(dev, "booting R5F core using boot addr = 0x%x\n", boot_addr);
@@ -564,7 +560,7 @@ static int k3_r5_rproc_start(struct rproc *rproc)
core = kproc->core;
ret = ti_sci_proc_set_config(core->tsp, boot_addr, 0, 0);
if (ret)
- goto put_mbox;
+ goto out;
The label "out" doesn't do anything, just directly `return ret;` here and
in the other cases below.
/* unhalt/run all applicable cores */
if (cluster->mode == CLUSTER_MODE_LOCKSTEP) {
@@ -581,12 +577,12 @@ static int k3_r5_rproc_start(struct rproc *rproc)
dev_err(dev, "%s: can not start core 1 before core 0\n",
__func__);
ret = -EPERM;
- goto put_mbox;
+ goto out;
}
ret = k3_r5_core_run(core);
if (ret)
- goto put_mbox;
+ goto out;
}
return 0;
@@ -596,8 +592,7 @@ static int k3_r5_rproc_start(struct rproc *rproc)
if (k3_r5_core_halt(core))
dev_warn(core->dev, "core halt back failed\n");
}
-put_mbox:
- mbox_free_channel(kproc->mbox);
+out:
return ret;
}
@@ -658,8 +653,6 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
goto out;
}
- mbox_free_channel(kproc->mbox);
-
return 0;
unroll_core_halt:
@@ -674,42 +667,21 @@ static int k3_r5_rproc_stop(struct rproc *rproc)
/*
* Attach to a running R5F remote processor (IPC-only mode)
*
- * The R5F attach callback only needs to request the mailbox, the remote
- * processor is already booted, so there is no need to issue any TI-SCI
- * commands to boot the R5F cores in IPC-only mode. This callback is invoked
- * only in IPC-only mode.
+ * The R5F attach callback is a NOP. The remote processor is already booted,
and
+ * all required resources have been acquired during probe routine, so there is
+ * no need to issue any TI-SCI commands to boot the R5F cores in IPC-only mode.
+ * This callback is invoked only in IPC-only mode and exists for sanity sake.
*/
-static int k3_r5_rproc_attach(struct rproc *rproc)
-{
- struct k3_r5_rproc *kproc = rproc->priv;
- struct device *dev = kproc->dev;
- int ret;
-
- ret = k3_r5_rproc_request_mbox(rproc);
- if (ret)
- return ret;
-
- dev_info(dev, "R5F core initialized in IPC-only mode\n");
- return 0;
-}
+static int k3_r5_rproc_attach(struct rproc *rproc) { return 0; }
/*
* Detach from a running R5F remote processor (IPC-only mode)
*
- * The R5F detach callback performs the opposite operation to attach callback
- * and only needs to release the mailbox, the R5F cores are not stopped and
- * will be left in booted state in IPC-only mode. This callback is invoked
- * only in IPC-only mode.
+ * The R5F detach callback is a
Re: Bug in Kernel 6.8.x, 6.9.x Causing Trace/Panic During Shutdown/Reboot
On Thu, 30 May 2024 16:02:37 +0300
Ilkka Naulapää wrote:
> applied your patch and here's the output.
>
Unfortunately, it doesn't give me any new information. I added one more
BUG on, want to try this? Otherwise, I'm pretty much at a lost. :-/
-- Steve
diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
index de5b72216b1a..a090495e78c9 100644
--- a/fs/tracefs/inode.c
+++ b/fs/tracefs/inode.c
@@ -39,13 +39,17 @@ static struct inode *tracefs_alloc_inode(struct super_block
*sb)
return NULL;
ti->flags = 0;
+ ti->magic = 20240823;
return >vfs_inode;
}
static void tracefs_free_inode(struct inode *inode)
{
- kmem_cache_free(tracefs_inode_cachep, get_tracefs(inode));
+ struct tracefs_inode *ti = get_tracefs(inode);
+
+ BUG_ON(ti->magic != 20240823);
+ kmem_cache_free(tracefs_inode_cachep, ti);
}
static ssize_t default_read_file(struct file *file, char __user *buf,
@@ -147,16 +151,6 @@ static const struct inode_operations
tracefs_dir_inode_operations = {
.rmdir = tracefs_syscall_rmdir,
};
-struct inode *tracefs_get_inode(struct super_block *sb)
-{
- struct inode *inode = new_inode(sb);
- if (inode) {
- inode->i_ino = get_next_ino();
- inode->i_atime = inode->i_mtime =
inode_set_ctime_current(inode);
- }
- return inode;
-}
-
struct tracefs_mount_opts {
kuid_t uid;
kgid_t gid;
@@ -384,6 +378,7 @@ static void tracefs_dentry_iput(struct dentry *dentry,
struct inode *inode)
return;
ti = get_tracefs(inode);
+ BUG_ON(ti->magic != 20240823);
if (ti && ti->flags & TRACEFS_EVENT_INODE)
eventfs_set_ef_status_free(dentry);
iput(inode);
@@ -568,6 +563,18 @@ struct dentry *eventfs_end_creating(struct dentry *dentry)
return dentry;
}
+struct inode *tracefs_get_inode(struct super_block *sb)
+{
+ struct inode *inode = new_inode(sb);
+
+ BUG_ON(sb->s_op != _super_operations);
+ if (inode) {
+ inode->i_ino = get_next_ino();
+ inode->i_atime = inode->i_mtime =
inode_set_ctime_current(inode);
+ }
+ return inode;
+}
+
/**
* tracefs_create_file - create a file in the tracefs filesystem
* @name: a pointer to a string containing the name of the file to create.
diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h
index 69c2b1d87c46..9059b8b11bb6 100644
--- a/fs/tracefs/internal.h
+++ b/fs/tracefs/internal.h
@@ -9,12 +9,15 @@ enum {
struct tracefs_inode {
unsigned long flags;
void*private;
+ unsigned long magic;
struct inodevfs_inode;
};
static inline struct tracefs_inode *get_tracefs(const struct inode *inode)
{
- return container_of(inode, struct tracefs_inode, vfs_inode);
+ struct tracefs_inode *ti = container_of(inode, struct tracefs_inode,
vfs_inode);
+ BUG_ON(ti->magic != 20240823);
+ return ti;
}
struct dentry *tracefs_start_creating(const char *name, struct dentry *parent);
Re: [PATCH net-next V2] virtio-net: synchronize operstate with admin state on up/down
On Thu, May 30, 2024 at 06:29:51PM +0800, Jason Wang wrote:
> On Thu, May 30, 2024 at 2:10 PM Michael S. Tsirkin wrote:
> >
> > On Thu, May 30, 2024 at 11:20:55AM +0800, Jason Wang wrote:
> > > This patch synchronize operstate with admin state per RFC2863.
> > >
> > > This is done by trying to toggle the carrier upon open/close and
> > > synchronize with the config change work. This allows propagate status
> > > correctly to stacked devices like:
> > >
> > > ip link add link enp0s3 macvlan0 type macvlan
> > > ip link set link enp0s3 down
> > > ip link show
> > >
> > > Before this patch:
> > >
> > > 3: enp0s3: mtu 1500 qdisc pfifo_fast state DOWN
> > > mode DEFAULT group default qlen 1000
> > > link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff
> > > ..
> > > 5: macvlan0@enp0s3: mtu 1500
> > > qdisc noqueue state UP mode DEFAULT group default qlen 1000
> > > link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff
> > >
> > > After this patch:
> > >
> > > 3: enp0s3: mtu 1500 qdisc pfifo_fast state DOWN
> > > mode DEFAULT group default qlen 1000
> > > link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff
> > > ...
> > > 5: macvlan0@enp0s3: mtu 1500
> > > qdisc noqueue state LOWERLAYERDOWN mode DEFAULT group default qlen 1000
> > > link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff
> > >
> > > Cc: Venkat Venkatsubra
> > > Cc: Gia-Khanh Nguyen
> > > Reviewed-by: Xuan Zhuo
> > > Acked-by: Michael S. Tsirkin
> > > Signed-off-by: Jason Wang
> > > ---
> > > Changes since V1:
> > > - rebase
> > > - add ack/review tags
> >
> >
> >
> >
> >
> > > ---
> > > drivers/net/virtio_net.c | 94 +++-
> > > 1 file changed, 63 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 4a802c0ea2cb..69e4ae353c51 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -433,6 +433,12 @@ struct virtnet_info {
> > > /* The lock to synchronize the access to refill_enabled */
> > > spinlock_t refill_lock;
> > >
> > > + /* Is config change enabled? */
> > > + bool config_change_enabled;
> > > +
> > > + /* The lock to synchronize the access to config_change_enabled */
> > > + spinlock_t config_change_lock;
> > > +
> > > /* Work struct for config space updates */
> > > struct work_struct config_work;
> > >
> >
> >
> > But we already have dev->config_lock and dev->config_enabled.
> >
> > And it actually works better - instead of discarding config
> > change events it defers them until enabled.
> >
>
> Yes but then both virtio-net driver and virtio core can ask to enable
> and disable and then we need some kind of synchronization which is
> non-trivial.
Well for core it happens on bring up path before driver works
and later on tear down after it is gone.
So I do not think they ever do it at the same time.
> And device enabling on the core is different from bringing the device
> up in the networking subsystem. Here we just delay to deal with the
> config change interrupt on ndo_open(). (E.g try to ack announce is
> meaningless when the device is down).
>
> Thanks
another thing is that it is better not to re-read all config
on link up if there was no config interrupt - less vm exits.
--
MST
Re: How to trace kvm:kvm_exit using fprobe?
Hi don, Thanks for reporting! I confirmed that the raw tracepoint probe event does not work on the tracepoint in the module. (not only kvm but also other modules) Let me fix the issue. Thank you, On Tue, 21 May 2024 17:28:48 -0700 don wrote: > Hi, Mr Masami Hiramatsu， > I see your presentation "Function Parameters with BTF Function tracing with > parameters", > and I started using fprobe and dynamic_events to trace functions. > > I have a question regarding tracepoints using fprobe/dynamic_events > I can trace the kvm:kvm_exit event using trace-cmd: > > *trace-cmd stream -e kvm:kvm_exit* > But if I echo to dynamic_event will get "Invalid argument" error: > # cd /sys/kernel/debug/tracing > > *# echo 't:kvm kvm_exit' >> dynamic_events*-bash: echo: write error: > Invalid argument > > How to solve this problem? > > Thanks, > don -- Masami Hiramatsu (Google)
Re: [PATCH net-next V2] virtio-net: synchronize operstate with admin state on up/down
On Thu, May 30, 2024 at 6:29 PM Jason Wang wrote:
>
> On Thu, May 30, 2024 at 2:10 PM Michael S. Tsirkin wrote:
> >
> > On Thu, May 30, 2024 at 11:20:55AM +0800, Jason Wang wrote:
> > > This patch synchronize operstate with admin state per RFC2863.
> > >
> > > This is done by trying to toggle the carrier upon open/close and
> > > synchronize with the config change work. This allows propagate status
> > > correctly to stacked devices like:
> > >
> > > ip link add link enp0s3 macvlan0 type macvlan
> > > ip link set link enp0s3 down
> > > ip link show
> > >
> > > Before this patch:
> > >
> > > 3: enp0s3: mtu 1500 qdisc pfifo_fast state DOWN
> > > mode DEFAULT group default qlen 1000
> > > link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff
> > > ..
> > > 5: macvlan0@enp0s3: mtu 1500
> > > qdisc noqueue state UP mode DEFAULT group default qlen 1000
> > > link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff
> > >
> > > After this patch:
> > >
> > > 3: enp0s3: mtu 1500 qdisc pfifo_fast state DOWN
> > > mode DEFAULT group default qlen 1000
> > > link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff
> > > ...
> > > 5: macvlan0@enp0s3: mtu 1500
> > > qdisc noqueue state LOWERLAYERDOWN mode DEFAULT group default qlen 1000
> > > link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff
> > >
> > > Cc: Venkat Venkatsubra
> > > Cc: Gia-Khanh Nguyen
> > > Reviewed-by: Xuan Zhuo
> > > Acked-by: Michael S. Tsirkin
> > > Signed-off-by: Jason Wang
> > > ---
> > > Changes since V1:
> > > - rebase
> > > - add ack/review tags
> >
> >
> >
> >
> >
> > > ---
> > > drivers/net/virtio_net.c | 94 +++-
> > > 1 file changed, 63 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 4a802c0ea2cb..69e4ae353c51 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -433,6 +433,12 @@ struct virtnet_info {
> > > /* The lock to synchronize the access to refill_enabled */
> > > spinlock_t refill_lock;
> > >
> > > + /* Is config change enabled? */
> > > + bool config_change_enabled;
> > > +
> > > + /* The lock to synchronize the access to config_change_enabled */
> > > + spinlock_t config_change_lock;
> > > +
> > > /* Work struct for config space updates */
> > > struct work_struct config_work;
> > >
> >
> >
> > But we already have dev->config_lock and dev->config_enabled.
> >
> > And it actually works better - instead of discarding config
> > change events it defers them until enabled.
> >
>
> Yes but then both virtio-net driver and virtio core can ask to enable
> and disable and then we need some kind of synchronization which is
> non-trivial.
>
> And device enabling on the core is different from bringing the device
> up in the networking subsystem. Here we just delay to deal with the
> config change interrupt on ndo_open(). (E.g try to ack announce is
> meaningless when the device is down).
Or maybe you meant to make config_enabled a nest counter?
Thanks
>
> Thanks
Re: [PATCH v2] sched/rt: Clean up usage of rt_task()
On 05/29/24 12:55, Sebastian Andrzej Siewior wrote:
> On 2024-05-29 11:34:09 [+0100], Qais Yousef wrote:
> > > behaviour. But then it is insistent which matters only in the RT case.
> > > Puh. Any sched folks regarding policy?
> >
> > I am not sure I understood you here. Could you rephrase please?
>
> Right now a SCHED_OTHER task boosted to a realtime priority gets
> slack=0. In the !RT scenario everything is fine.
> For RT the slack=0 also happens but the init of the timer looks at the
> policy instead at the possible boosted priority and uses a different
> clock attribute. This can lead to a delayed wake up (so avoiding the
> slack does not solve the problem).
>
> This is not consistent because IMHO the clock setup & slack should be
> handled equally. So I am asking the sched folks for a policy and I am
> leaning towards looking at task-policy in this case instead of prio
> because you shouldn't do anything that can delay.
Can't we do that based on is_soft/is_hard flag in hrtimer struct when we apply
the slack in hrtimer_set_expires_range_ns() instead?
(not compile tested even)
diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h
index aa1e65ccb615..e001f20bbea9 100644
--- a/include/linux/hrtimer.h
+++ b/include/linux/hrtimer.h
@@ -102,12 +102,16 @@ static inline void hrtimer_set_expires(struct hrtimer
*timer, ktime_t time)
static inline void hrtimer_set_expires_range(struct hrtimer *timer, ktime_t
time, ktime_t delta)
{
+ if (timer->is_soft || timer->is_hard)
+ delta = 0;
timer->_softexpires = time;
timer->node.expires = ktime_add_safe(time, delta);
}
static inline void hrtimer_set_expires_range_ns(struct hrtimer *timer, ktime_t
time, u64 delta)
{
+ if (timer->is_soft || timer->is_hard)
+ delta = 0;
timer->_softexpires = time;
timer->node.expires = ktime_add_safe(time, ns_to_ktime(delta));
}
Re: [PATCH net-next V2] virtio-net: synchronize operstate with admin state on up/down
On Thu, May 30, 2024 at 2:10 PM Michael S. Tsirkin wrote:
>
> On Thu, May 30, 2024 at 11:20:55AM +0800, Jason Wang wrote:
> > This patch synchronize operstate with admin state per RFC2863.
> >
> > This is done by trying to toggle the carrier upon open/close and
> > synchronize with the config change work. This allows propagate status
> > correctly to stacked devices like:
> >
> > ip link add link enp0s3 macvlan0 type macvlan
> > ip link set link enp0s3 down
> > ip link show
> >
> > Before this patch:
> >
> > 3: enp0s3: mtu 1500 qdisc pfifo_fast state DOWN mode
> > DEFAULT group default qlen 1000
> > link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff
> > ..
> > 5: macvlan0@enp0s3: mtu 1500 qdisc
> > noqueue state UP mode DEFAULT group default qlen 1000
> > link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff
> >
> > After this patch:
> >
> > 3: enp0s3: mtu 1500 qdisc pfifo_fast state DOWN mode
> > DEFAULT group default qlen 1000
> > link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff
> > ...
> > 5: macvlan0@enp0s3: mtu 1500
> > qdisc noqueue state LOWERLAYERDOWN mode DEFAULT group default qlen 1000
> > link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff
> >
> > Cc: Venkat Venkatsubra
> > Cc: Gia-Khanh Nguyen
> > Reviewed-by: Xuan Zhuo
> > Acked-by: Michael S. Tsirkin
> > Signed-off-by: Jason Wang
> > ---
> > Changes since V1:
> > - rebase
> > - add ack/review tags
>
>
>
>
>
> > ---
> > drivers/net/virtio_net.c | 94 +++-
> > 1 file changed, 63 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 4a802c0ea2cb..69e4ae353c51 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -433,6 +433,12 @@ struct virtnet_info {
> > /* The lock to synchronize the access to refill_enabled */
> > spinlock_t refill_lock;
> >
> > + /* Is config change enabled? */
> > + bool config_change_enabled;
> > +
> > + /* The lock to synchronize the access to config_change_enabled */
> > + spinlock_t config_change_lock;
> > +
> > /* Work struct for config space updates */
> > struct work_struct config_work;
> >
>
>
> But we already have dev->config_lock and dev->config_enabled.
>
> And it actually works better - instead of discarding config
> change events it defers them until enabled.
>
Yes but then both virtio-net driver and virtio core can ask to enable
and disable and then we need some kind of synchronization which is
non-trivial.
And device enabling on the core is different from bringing the device
up in the networking subsystem. Here we just delay to deal with the
config change interrupt on ndo_open(). (E.g try to ack announce is
meaningless when the device is down).
Thanks
Re: [PATCH] mctp i2c: Add rx trace
Hi Tal, > > > mctp-i2c rx implementation doesn't call > > > __i2c_transfer which calls the i2c reply trace function. > > > > No, but we can trace the i2c rx path through the trace_i2c_slave > > tracepoint. It is a little messier than tracing trace_i2c_write, > > but > > has been sufficient with the debugging I've needed in the past. > > Oh, I missed that. > I had to test it with an older kernel without i2c_slave tracing > so I looked only at the regular i2c and mctp trace paths. OK! That tracepoint was (coincidentally) added in 5.18, same as the MCTP-over-i2c transport. So we should have coverage for both features on upstream kernels, at least. > > > Add an mctp_reply trace function that will be used instead. > > > > Can you elaborate a little on what you were/are looking to inspect > > here? (mainly: which packet fields are you interested in?) That > > will > > help to determine the best approach here. > > Sure, I basically wanted to trace the i2c packet buffer in a simple > way. OK - did you specifically need the i2c transport headers? Since the MCTP interfaces are regular net devices, the easiest way to trace generic MCTP transfers is generally via a packet capture (tcpdump, wireshark, etc). Cheers, Jeremy
Re: [PATCH v5 5/7] remoteproc: core: support of the tee interface
Hello Mathieu,
On 5/29/24 22:35, Mathieu Poirier wrote:
> On Wed, May 29, 2024 at 09:13:26AM +0200, Arnaud POULIQUEN wrote:
>> Hello Mathieu,
>>
>> On 5/28/24 23:30, Mathieu Poirier wrote:
>>> On Tue, May 21, 2024 at 10:09:59AM +0200, Arnaud Pouliquen wrote:
1) on start:
- Using the TEE loader, the resource table is loaded by an external entity.
In such case the resource table address is not find from the firmware but
provided by the TEE remoteproc framework.
Use the rproc_get_loaded_rsc_table instead of rproc_find_loaded_rsc_table
- test that rproc->cached_table is not null before performing the memcpy
2)on stop
The use of the cached_table seems mandatory:
- during recovery sequence to have a snapshot of the resource table
resources used,
- on stop to allow for the deinitialization of resources after the
the remote processor has been shutdown.
However if the TEE interface is being used, we first need to unmap the
table_ptr before setting it to rproc->cached_table.
The update of rproc->table_ptr to rproc->cached_table is performed in
tee_remoteproc.
Signed-off-by: Arnaud Pouliquen
---
drivers/remoteproc/remoteproc_core.c | 31 +---
1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_core.c
b/drivers/remoteproc/remoteproc_core.c
index 42bca01f3bde..3a642151c983 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1267,6 +1267,7 @@ EXPORT_SYMBOL(rproc_resource_cleanup);
static int rproc_set_rsc_table_on_start(struct rproc *rproc, const struct
firmware *fw)
{
struct resource_table *loaded_table;
+ struct device *dev = >dev;
/*
* The starting device has been given the rproc->cached_table as the
@@ -1276,12 +1277,21 @@ static int rproc_set_rsc_table_on_start(struct
rproc *rproc, const struct firmwa
* this information to device memory. We also update the table_ptr so
* that any subsequent changes will be applied to the loaded version.
*/
- loaded_table = rproc_find_loaded_rsc_table(rproc, fw);
- if (loaded_table) {
- memcpy(loaded_table, rproc->cached_table, rproc->table_sz);
- rproc->table_ptr = loaded_table;
+ if (rproc->tee_interface) {
+ loaded_table = rproc_get_loaded_rsc_table(rproc,
>table_sz);
+ if (IS_ERR(loaded_table)) {
+ dev_err(dev, "can't get resource table\n");
+ return PTR_ERR(loaded_table);
+ }
+ } else {
+ loaded_table = rproc_find_loaded_rsc_table(rproc, fw);
}
+ if (loaded_table && rproc->cached_table)
+ memcpy(loaded_table, rproc->cached_table, rproc->table_sz);
+
>>>
>>> Why is this not part of the else {} above as it was the case before this
>>> patch?
>>> And why was an extra check for ->cached_table added?
>>
>> Here we have to cover 2 use cases if rproc->tee_interface is set.
>> 1) The remote processor is in stop state
>> - loaded_table points to the resource table in the remote memory and
>> - rproc->cached_table is null
>> => no memcopy
>> 2) crash recovery
>> - loaded_table points to the resource table in the remote memory
>> - rproc-cached_table point to a copy of the resource table
>
> A cached_table exists because it was created in
> rproc_reset_rsc_table_on_stop().
> But as the comment says [1], that part of the code was meant to be used for
> the
> attach()/detach() use case. Mixing both will become extremely confusing and
> impossible to maintain.
i am not sure to understand your point here... the cached_table table was
already existing for the "normal" case[2]. Seems to me that the cache table is
needed on stop in all scenarios.
[2]
https://elixir.bootlin.com/linux/v4.20.17/source/drivers/remoteproc/remoteproc_core.c#L1402
>
> I think the TEE scenario should be as similar as the "normal" one where TEE is
> not involved. To that end, I suggest to create a cached_table in
> tee_rproc_parse_fw(), exactly the same way it is done in
> rproc_elf_load_rsc_table(). That way the code path in
> rproc_set_rsc_table_on_start() become very similar and we have a cached_table
> to
> work with when the remote processor is recovered. In fact we may not need
> rproc_set_rsc_table_on_start() at all but that needs to be asserted.
This is was I proposed in my V4 [3]. Could you please confirm that this aligns
with what you have in mind?
In such a case, should I keep the updates below in
rproc_reset_rsc_table_on_stop(), or should I revert to using rproc->rsc_table to
store the pointer to the resource table in tee_remoteproc for the associated
memory map/unmap?"
[3]
Re: [PATCH net-next V2] virtio-net: synchronize operstate with admin state on up/down
On Thu, May 30, 2024 at 11:20:55AM +0800, Jason Wang wrote:
> This patch synchronize operstate with admin state per RFC2863.
>
> This is done by trying to toggle the carrier upon open/close and
> synchronize with the config change work. This allows propagate status
> correctly to stacked devices like:
>
> ip link add link enp0s3 macvlan0 type macvlan
> ip link set link enp0s3 down
> ip link show
>
> Before this patch:
>
> 3: enp0s3: mtu 1500 qdisc pfifo_fast state DOWN mode
> DEFAULT group default qlen 1000
> link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff
> ..
> 5: macvlan0@enp0s3: mtu 1500 qdisc
> noqueue state UP mode DEFAULT group default qlen 1000
> link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff
>
> After this patch:
>
> 3: enp0s3: mtu 1500 qdisc pfifo_fast state DOWN mode
> DEFAULT group default qlen 1000
> link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff
> ...
> 5: macvlan0@enp0s3: mtu 1500 qdisc
> noqueue state LOWERLAYERDOWN mode DEFAULT group default qlen 1000
> link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff
>
> Cc: Venkat Venkatsubra
> Cc: Gia-Khanh Nguyen
> Reviewed-by: Xuan Zhuo
> Acked-by: Michael S. Tsirkin
> Signed-off-by: Jason Wang
> ---
> Changes since V1:
> - rebase
> - add ack/review tags
> ---
> drivers/net/virtio_net.c | 94 +++-
> 1 file changed, 63 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 4a802c0ea2cb..69e4ae353c51 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -433,6 +433,12 @@ struct virtnet_info {
> /* The lock to synchronize the access to refill_enabled */
> spinlock_t refill_lock;
>
> + /* Is config change enabled? */
> + bool config_change_enabled;
> +
> + /* The lock to synchronize the access to config_change_enabled */
> + spinlock_t config_change_lock;
> +
> /* Work struct for config space updates */
> struct work_struct config_work;
>
But we already have dev->config_lock and dev->config_enabled.
And it actually works better - instead of discarding config
change events it defers them until enabled.
> @@ -623,6 +629,20 @@ static void disable_delayed_refill(struct virtnet_info
> *vi)
> spin_unlock_bh(>refill_lock);
> }
>
> +static void enable_config_change(struct virtnet_info *vi)
> +{
> + spin_lock_irq(>config_change_lock);
> + vi->config_change_enabled = true;
> + spin_unlock_irq(>config_change_lock);
> +}
> +
> +static void disable_config_change(struct virtnet_info *vi)
> +{
> + spin_lock_irq(>config_change_lock);
> + vi->config_change_enabled = false;
> + spin_unlock_irq(>config_change_lock);
> +}
> +
> static void enable_rx_mode_work(struct virtnet_info *vi)
> {
> rtnl_lock();
> @@ -2421,6 +2441,25 @@ static int virtnet_enable_queue_pair(struct
> virtnet_info *vi, int qp_index)
> return err;
> }
>
> +static void virtnet_update_settings(struct virtnet_info *vi)
> +{
> + u32 speed;
> + u8 duplex;
> +
> + if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_SPEED_DUPLEX))
> + return;
> +
> + virtio_cread_le(vi->vdev, struct virtio_net_config, speed, );
> +
> + if (ethtool_validate_speed(speed))
> + vi->speed = speed;
> +
> + virtio_cread_le(vi->vdev, struct virtio_net_config, duplex, );
> +
> + if (ethtool_validate_duplex(duplex))
> + vi->duplex = duplex;
> +}
> +
> static int virtnet_open(struct net_device *dev)
> {
> struct virtnet_info *vi = netdev_priv(dev);
> @@ -2439,6 +2478,18 @@ static int virtnet_open(struct net_device *dev)
> goto err_enable_qp;
> }
>
> + /* Assume link up if device can't report link status,
> +otherwise get link status from config. */
> + netif_carrier_off(dev);
> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) {
> + enable_config_change(vi);
> + schedule_work(>config_work);
> + } else {
> + vi->status = VIRTIO_NET_S_LINK_UP;
> + virtnet_update_settings(vi);
> + netif_carrier_on(dev);
> + }
> +
> return 0;
>
> err_enable_qp:
> @@ -2875,12 +2926,19 @@ static int virtnet_close(struct net_device *dev)
> disable_delayed_refill(vi);
> /* Make sure refill_work doesn't re-enable napi! */
> cancel_delayed_work_sync(>refill);
> + /* Make sure config notification doesn't schedule config
Re: [PATCH v2 2/6] livepatch: Add klp-convert tool
Hi Lukas,
As mentioned offlist, reviewing and testing this is on my TODO list, but
here are some early notes ...
On Thu, May 16, 2024 at 03:30:05PM +0200, Lukas Hruska wrote:
> Livepatches need to access external symbols which can't be handled
> by the normal relocation mechanism. It is needed for two types
> of symbols:
>
> + Symbols which can be local for the original livepatched function.
> The alternative implementation in the livepatch sees them
> as external symbols.
>
> + Symbols in modules which are exported via EXPORT_SYMBOL*(). They
> must be handled special way otherwise the livepatch module would
> depend on the livepatched one. Loading such livepatch would cause
> loading the other module as well.
>
> The address of these symbols can be found via kallsyms. Or they can
> be relocated using livepatch specific relocation sections as specified
> in Documentation/livepatch/module-elf-format.txt.
>
> Currently, there is no trivial way to embed the required information as
> requested in the final livepatch elf object. klp-convert solves this
> problem by using annotations in the elf object to convert the relocation
> accordingly to the specification, enabling it to be handled by the
> livepatch loader.
>
> Given the above, create scripts/livepatch to hold tools developed for
> livepatches and add source files for klp-convert there.
>
> Allow to annotate such external symbols in the livepatch by a macro
> KLP_RELOC_SYMBOL(). It will create symbol with all needed
> metadata. For example:
>
> extern char *saved_command_line \
> KLP_RELOC_SYMBOL(vmlinux, vmlinux, saved_command_line, 0);
Nit: should this be KLP_RELOC_SYMBOL_POS if it including the 0 position?
(Or KLP_RELOC_SYMBOL and omit the implied 0-th position.)
> diff --git a/scripts/livepatch/elf.c b/scripts/livepatch/elf.c
> --- /dev/null
> +++ b/scripts/livepatch/elf.c
> +static int update_shstrtab(struct elf *elf)
> +{
> + struct section *shstrtab, *sec;
> + size_t orig_size, new_size = 0, offset, len;
> + char *buf;
> +
> + shstrtab = find_section_by_name(elf, ".shstrtab");
> + if (!shstrtab) {
> + WARN("can't find .shstrtab");
> + return -1;
> + }
> +
> + orig_size = new_size = shstrtab->size;
> +
> + list_for_each_entry(sec, >sections, list) {
> + if (sec->sh.sh_name != ~0U)
> + continue;
> + new_size += strlen(sec->name) + 1;
> + }
> +
> + if (new_size == orig_size)
> + return 0;
> +
> + buf = malloc(new_size);
> + if (!buf) {
> + WARN("malloc failed");
> + return -1;
> + }
> + memcpy(buf, (void *)shstrtab->data, orig_size);
While reviewing klp-convert v7 [1], Alexey Dobriyan notes here,
"This code is called realloc(). :-)"
[1]
https://lore.kernel.org/live-patching/4ce29654-4e1e-4680-9c25-715823ff5e02@p183/
> +static int update_strtab(struct elf *elf)
> +{
> + struct section *strtab;
> + struct symbol *sym;
> + size_t orig_size, new_size = 0, offset, len;
> + char *buf;
> +
> + strtab = find_section_by_name(elf, ".strtab");
> + if (!strtab) {
> + WARN("can't find .strtab");
> + return -1;
> + }
> +
> + orig_size = new_size = strtab->size;
> +
> + list_for_each_entry(sym, >symbols, list) {
> + if (sym->sym.st_name != ~0U)
> + continue;
> + new_size += strlen(sym->name) + 1;
> + }
> +
> + if (new_size == orig_size)
> + return 0;
> +
> + buf = malloc(new_size);
> + if (!buf) {
> + WARN("malloc failed");
> + return -1;
> + }
> + memcpy(buf, (void *)strtab->data, orig_size);
I think Alexey's realloc suggestion would apply here, too.
> +static int write_file(struct elf *elf, const char *file)
> +{
> + int fd;
> + Elf *e;
> + GElf_Ehdr eh, ehout;
> + Elf_Scn *scn;
> + Elf_Data *data;
> + GElf_Shdr sh;
> + struct section *sec;
> +
> + fd = creat(file, 0664);
> + if (fd == -1) {
> + WARN("couldn't create %s", file);
> + return -1;
> + }
> +
> + e = elf_begin(fd, ELF_C_WRITE, NULL);
Alexy also found an ELF coding bug:
"elf_end() doesn't close descriptor, so there is potentially corrupted
data. There is no unlink() call if writes fail as well."
> +void elf_close(struct elf *elf)
> +{
> + struct section *sec, *tmpsec;
> + struct symbol *sym, *tmpsym;
> + struct rela *rela, *tmprela;
> +
> + list_for_each_entry_safe(sym, tmpsym, >symbols, list) {
> + list_del(>list);
> + free(sym);
> + }
> + list_for_each_entry_safe(sec, tmpsec, >sections, list) {
> + list_for_each_entry_safe(rela, tmprela, >relas, list) {
> + list_del(>list);
> + free(rela);
> + }
> + list_del(>list);
> +
Re: [PATCH v5 5/7] remoteproc: core: support of the tee interface
On Wed, May 29, 2024 at 09:13:26AM +0200, Arnaud POULIQUEN wrote:
> Hello Mathieu,
>
> On 5/28/24 23:30, Mathieu Poirier wrote:
> > On Tue, May 21, 2024 at 10:09:59AM +0200, Arnaud Pouliquen wrote:
> >> 1) on start:
> >> - Using the TEE loader, the resource table is loaded by an external entity.
> >> In such case the resource table address is not find from the firmware but
> >> provided by the TEE remoteproc framework.
> >> Use the rproc_get_loaded_rsc_table instead of rproc_find_loaded_rsc_table
> >> - test that rproc->cached_table is not null before performing the memcpy
> >>
> >> 2)on stop
> >> The use of the cached_table seems mandatory:
> >> - during recovery sequence to have a snapshot of the resource table
> >> resources used,
> >> - on stop to allow for the deinitialization of resources after the
> >> the remote processor has been shutdown.
> >> However if the TEE interface is being used, we first need to unmap the
> >> table_ptr before setting it to rproc->cached_table.
> >> The update of rproc->table_ptr to rproc->cached_table is performed in
> >> tee_remoteproc.
> >>
> >> Signed-off-by: Arnaud Pouliquen
> >> ---
> >> drivers/remoteproc/remoteproc_core.c | 31 +---
> >> 1 file changed, 23 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/remoteproc/remoteproc_core.c
> >> b/drivers/remoteproc/remoteproc_core.c
> >> index 42bca01f3bde..3a642151c983 100644
> >> --- a/drivers/remoteproc/remoteproc_core.c
> >> +++ b/drivers/remoteproc/remoteproc_core.c
> >> @@ -1267,6 +1267,7 @@ EXPORT_SYMBOL(rproc_resource_cleanup);
> >> static int rproc_set_rsc_table_on_start(struct rproc *rproc, const struct
> >> firmware *fw)
> >> {
> >>struct resource_table *loaded_table;
> >> + struct device *dev = >dev;
> >>
> >>/*
> >> * The starting device has been given the rproc->cached_table as the
> >> @@ -1276,12 +1277,21 @@ static int rproc_set_rsc_table_on_start(struct
> >> rproc *rproc, const struct firmwa
> >> * this information to device memory. We also update the table_ptr so
> >> * that any subsequent changes will be applied to the loaded version.
> >> */
> >> - loaded_table = rproc_find_loaded_rsc_table(rproc, fw);
> >> - if (loaded_table) {
> >> - memcpy(loaded_table, rproc->cached_table, rproc->table_sz);
> >> - rproc->table_ptr = loaded_table;
> >> + if (rproc->tee_interface) {
> >> + loaded_table = rproc_get_loaded_rsc_table(rproc,
> >> >table_sz);
> >> + if (IS_ERR(loaded_table)) {
> >> + dev_err(dev, "can't get resource table\n");
> >> + return PTR_ERR(loaded_table);
> >> + }
> >> + } else {
> >> + loaded_table = rproc_find_loaded_rsc_table(rproc, fw);
> >>}
> >>
> >> + if (loaded_table && rproc->cached_table)
> >> + memcpy(loaded_table, rproc->cached_table, rproc->table_sz);
> >> +
> >
> > Why is this not part of the else {} above as it was the case before this
> > patch?
> > And why was an extra check for ->cached_table added?
>
> Here we have to cover 2 use cases if rproc->tee_interface is set.
> 1) The remote processor is in stop state
> - loaded_table points to the resource table in the remote memory and
> - rproc->cached_table is null
> => no memcopy
> 2) crash recovery
> - loaded_table points to the resource table in the remote memory
> - rproc-cached_table point to a copy of the resource table
A cached_table exists because it was created in rproc_reset_rsc_table_on_stop().
But as the comment says [1], that part of the code was meant to be used for the
attach()/detach() use case. Mixing both will become extremely confusing and
impossible to maintain.
I think the TEE scenario should be as similar as the "normal" one where TEE is
not involved. To that end, I suggest to create a cached_table in
tee_rproc_parse_fw(), exactly the same way it is done in
rproc_elf_load_rsc_table(). That way the code path in
rproc_set_rsc_table_on_start() become very similar and we have a cached_table to
work with when the remote processor is recovered. In fact we may not need
rproc_set_rsc_table_on_start() at all but that needs to be asserted.
[1].
https://elixir.bootlin.com/linux/v6.10-rc1/source/drivers/remoteproc/remoteproc_core.c#L1565
> => need to perform the memcpy to reapply settings in the resource table
>
> I can duplicate the memcpy in if{} and else{} but this will be similar code
> as needed in both case.
> Adding rproc->cached_table test if proc->tee_interface=NULL seems also
> reasonable as a memcpy from 0 should not be performed.
>
>
> >
> > This should be a simple change, i.e introduce an if {} else {} block to take
> > care of the two scenarios. Plus the comment is misplaced now.
>
> What about split it in 2 patches?
> - one adding the test on rproc->cached_table for the memcpy
> - one adding the if {} else {}?
>
> Thanks,
> Arnaud
>
>
> >
> > More comments tomorrow.
> >
>
Re: Bug in Kernel 6.8.x, 6.9.x Causing Trace/Panic During Shutdown/Reboot
On Wed, 29 May 2024 14:47:57 -0400
Steven Rostedt wrote:
> Let me make a debug patch (that crashes on this issue) for that kernel,
> and perhaps you could bisect it?
Can you try this on 6.6-rc1 and see if it gives you any other splats?
Hmm, you can switch it to WARN_ON and that way it may not crash the
machine, and you can use dmesg to get the output.
Thanks,
-- Steve
diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
index de5b72216b1a..a090495e78c9 100644
--- a/fs/tracefs/inode.c
+++ b/fs/tracefs/inode.c
@@ -39,13 +39,17 @@ static struct inode *tracefs_alloc_inode(struct super_block
*sb)
return NULL;
ti->flags = 0;
+ ti->magic = 20240823;
return >vfs_inode;
}
static void tracefs_free_inode(struct inode *inode)
{
- kmem_cache_free(tracefs_inode_cachep, get_tracefs(inode));
+ struct tracefs_inode *ti = get_tracefs(inode);
+
+ BUG_ON(ti->magic != 20240823);
+ kmem_cache_free(tracefs_inode_cachep, ti);
}
static ssize_t default_read_file(struct file *file, char __user *buf,
@@ -147,16 +151,6 @@ static const struct inode_operations
tracefs_dir_inode_operations = {
.rmdir = tracefs_syscall_rmdir,
};
-struct inode *tracefs_get_inode(struct super_block *sb)
-{
- struct inode *inode = new_inode(sb);
- if (inode) {
- inode->i_ino = get_next_ino();
- inode->i_atime = inode->i_mtime =
inode_set_ctime_current(inode);
- }
- return inode;
-}
-
struct tracefs_mount_opts {
kuid_t uid;
kgid_t gid;
@@ -384,6 +378,7 @@ static void tracefs_dentry_iput(struct dentry *dentry,
struct inode *inode)
return;
ti = get_tracefs(inode);
+ BUG_ON(ti->magic != 20240823);
if (ti && ti->flags & TRACEFS_EVENT_INODE)
eventfs_set_ef_status_free(dentry);
iput(inode);
@@ -568,6 +563,18 @@ struct dentry *eventfs_end_creating(struct dentry *dentry)
return dentry;
}
+struct inode *tracefs_get_inode(struct super_block *sb)
+{
+ struct inode *inode = new_inode(sb);
+
+ BUG_ON(sb->s_op != _super_operations);
+ if (inode) {
+ inode->i_ino = get_next_ino();
+ inode->i_atime = inode->i_mtime =
inode_set_ctime_current(inode);
+ }
+ return inode;
+}
+
/**
* tracefs_create_file - create a file in the tracefs filesystem
* @name: a pointer to a string containing the name of the file to create.
diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h
index 69c2b1d87c46..9f6f303a9e58 100644
--- a/fs/tracefs/internal.h
+++ b/fs/tracefs/internal.h
@@ -9,6 +9,7 @@ enum {
struct tracefs_inode {
unsigned long flags;
void*private;
+ unsigned long magic;
struct inodevfs_inode;
};
Re: Bug in Kernel 6.8.x, 6.9.x Causing Trace/Panic During Shutdown/Reboot
On Wed, 29 May 2024 21:36:08 +0300 Ilkka Naulapää wrote: > applied your patch without others, so trace and panic there. > Screenshot attached. Also tested kernels backward and found out that Bah, it's still in an RCU callback, which doesn't tell us why a normal inode is being sent to the trace inode free list. > this trace bug first triggered on 6.6-rc1. Hmm, that's when eventfs was added. > > Let me know if you need more assistance with this. Let me make a debug patch (that crashes on this issue) for that kernel, and perhaps you could bisect it? Thanks! -- Steve
Re: [PATCH] mctp i2c: Add rx trace
Hi Jeremy, Thanks for the reply. > > mctp-i2c rx implementation doesn't call > > __i2c_transfer which calls the i2c reply trace function. > > No, but we can trace the i2c rx path through the trace_i2c_slave > tracepoint. It is a little messier than tracing trace_i2c_write, but > has been sufficient with the debugging I've needed in the past. Oh, I missed that. I had to test it with an older kernel without i2c_slave tracing so I looked only at the regular i2c and mctp trace paths. > > Add an mctp_reply trace function that will be used instead. > > Can you elaborate a little on what you were/are looking to inspect > here? (mainly: which packet fields are you interested in?) That will > help to determine the best approach here. Sure, I basically wanted to trace the i2c packet buffer in a simple way. Although, it seems we already have that in the trace_i2c_slave since 5.18 so it could be redundant, unless you see any other potential use for it. Thanks, Tal.
Re: [PATCH 0/3] tracing: Fix some selftest issues
Hi Masami, On Wed, 2024-05-29 at 08:38 +0900, Masami Hiramatsu wrote: > On Wed, 29 May 2024 01:46:40 +0900 > Masami Hiramatsu (Google) wrote: > > > On Mon, 27 May 2024 19:29:07 -0400 > > Steven Rostedt wrote: > > > > > On Sun, 26 May 2024 19:10:57 +0900 > > > "Masami Hiramatsu (Google)" wrote: > > > > > > > Hi, > > > > > > > > Here is a series of some fixes/improvements for the test > > > > modules and boot > > > > time selftest of kprobe events. I found a WARNING message with > > > > some boot > > > > time selftest configuration, which came from the combination of > > > > embedded > > > > kprobe generate API tests module and ftrace boot-time selftest. > > > > So the main > > > > problem is that the test module should not be built-in. But I > > > > also think > > > > this WARNING message is useless (because there are warning > > > > messages already) > > > > and the cleanup code is redundant. This series fixes those > > > > issues. > > > > > > Note, when I enable trace tests as builtin instead of modules, I > > > just > > > disable the bootup self tests when it detects this. This helps > > > with > > > doing tests via config options than having to add user space code > > > that > > > loads modules. > > > > > > Could you do something similar? > > > > OK, in that case, I would like to move the test cleanup code in > > module_exit function into the end of module_init function. > > It looks there is no reason to split those into 2 parts. > > Wait, I would like to hear Tom's opinion. I found following usage > comments > in the code. > > * Following that are a few examples using the created events to test > * various ways of tracing a synthetic event. > * > * To test, select CONFIG_SYNTH_EVENT_GEN_TEST and build the module. > * Then: > * > * # insmod kernel/trace/synth_event_gen_test.ko > * # cat /sys/kernel/tracing/trace > * > * You should see several events in the trace buffer - > * "create_synth_test", "empty_synth_test", and several instances of > * "gen_synth_test". > * > * To remove the events, remove the module: > * > * # rmmod synth_event_gen_test > > Tom, is that intended behavior ? and are you expected to reuse these > events outside of the module? e.g. load the test module and run some > test script in user space which uses those events? > Yeah, this module was meant as a sample module showing how to create and generate synthetic events in-kernel. So the interested user insmods the module, looks at the trace stream and sees, ok the events are there as expected, so it does work, great, let's remove the module to get rid of them and go write our own. Having both the creation and cleanup in module_init() wouldn't allow the user the opportunity to do that i.e. verify the results by reading the trace file. Tom > As far as I can see, those tests are not intended to be embedded in > the > kernel because those are expected to be removed. > > Thank you, > > > > > Thank you, > > > > > > > > -- Steve > > > > > > > > > > > > > > Thank you, > > > > > > > > --- > > > > > > > > Masami Hiramatsu (Google) (3): > > > > tracing: Build event generation tests only as modules > > > > tracing/kprobe: Remove unneeded WARN_ON_ONCE() in > > > > selftests > > > > tracing/kprobe: Remove cleanup code unrelated to selftest > > > > > > > > > > -- > > Masami Hiramatsu (Google) > >
Re: [PATCH RFC 1/2] dt-bindings: soc: qcom,smsm: Allow specifying mboxes instead of qcom,ipc
On Samstag, 25. Mai 2024 18:47:08 MESZ Krzysztof Kozlowski wrote:
> On 24/05/2024 19:55, Luca Weiss wrote:
> > On Donnerstag, 23. Mai 2024 08:19:11 MESZ Krzysztof Kozlowski wrote:
> >> On 23/05/2024 08:16, Luca Weiss wrote:
> >>> On Donnerstag, 23. Mai 2024 08:02:13 MESZ Krzysztof Kozlowski wrote:
> On 22/05/2024 19:34, Luca Weiss wrote:
> > On Mittwoch, 22. Mai 2024 08:49:43 MESZ Krzysztof Kozlowski wrote:
> >> On 21/05/2024 22:35, Luca Weiss wrote:
> >>> On Dienstag, 21. Mai 2024 10:58:07 MESZ Krzysztof Kozlowski wrote:
> On 20/05/2024 17:11, Luca Weiss wrote:
> > Hi Krzysztof
> >
> > Ack, sounds good.
> >
> > Maybe also from you, any opinion between these two binding styles?
> >
> > So first using index of mboxes for the numbering, where for the
> > known
> > usages the first element (and sometimes the 3rd - ipc-2) are empty
> > <>.
> >
> > The second variant is using mbox-names to get the correct
> > channel-mbox
> > mapping.
> >
> > - qcom,ipc-1 = < 8 13>;
> > - qcom,ipc-2 = < 8 9>;
> > - qcom,ipc-3 = < 8 19>;
> > + mboxes = <0>, < 13>, < 9>, < 19>;
> >
> > vs.
> >
> > - qcom,ipc-1 = < 8 13>;
> > - qcom,ipc-2 = < 8 9>;
> > - qcom,ipc-3 = < 8 19>;
> > + mboxes = < 13>, < 9>, < 19>;
> > + mbox-names = "ipc-1", "ipc-2", "ipc-3";
>
> Sorry, don't get, ipc-1 is the first mailbox, so why would there be
> <0>
> in first case?
> >>>
> >>> Actually not, ipc-0 would be permissible by the driver, used for the
> >>> 0th host
> >>>
> >>> e.g. from:
> >>>
> >>> /* Iterate over all hosts to check whom wants a kick */
> >>> for (host = 0; host < smsm->num_hosts; host++) {
> >>> hostp = >hosts[host];
> >>>
> >>> Even though no mailbox is specified in any upstream dts for this 0th
> >>> host I
> >>> didn't want the bindings to restrict that, that's why in the first
> >>> example
> >>> there's an empty element (<0>) for the 0th smsm host
> >>>
> Anyway, the question is if you need to know that some
> mailbox is missing. But then it is weird to name them "ipc-1" etc.
> >>>
> >>> In either case we'd just query the mbox (either by name or index) and
> >>> then
> >>> see if it's there? Not quite sure I understand the sentence..
> >>> Pretty sure either binding would work the same way.
> >>
> >> The question is: does the driver care only about having some mailboxes
> >> or the driver cares about each specific mailbox? IOW, is skipping ipc-0
> >> important for the driver?
> >
> > There's nothing special from driver side about any mailbox. Some SoCs
> > have
> > a mailbox for e.g. hosts 1&2&3, some have only 1&3, and apq8064 even has
> > 1&2&3&4.
> >
> > And if the driver doesn't find a mailbox for a host, it just ignores it
> > but then of course it can't 'ring' the mailbox for that host when
> > necessary.
> >
> > Not sure how much more I can add here, to be fair I barely understand
> > what
> > this driver is doing myself apart from the obvious.
>
> From what you said, it looks like it is enough to just list mailboxes,
> e.g. for ipc-1, ipc-2 and ipc-4 (so no ipc-0 and ipc-3):
> >>>
> >>> No, for sure we need also the possibility to list ipc-3.
> >>
> >> ? You can list it, what's the problem>
> >
> > Maybe we're talking past each other...
> >
> > You asked why this wouldn't work:
> >
> > e.g. for ipc-1, ipc-2 and ipc-4 (so no ipc-0 and ipc-3):
> > mboxes = < 13>, < 9>, < 19>;
> >
> > How would we know that the 3rd mailbox ( 19) is for the 4th host
> > (previous ipc-4)?
> >
> > 1. If we use mboxes with indexes we'd need to have <0> values for
> > "smsm hosts" where we don't have a mailbox for - this is at least
> > for the 2nd smsm host (qcom,ipc-2) for a bunch of SoCs.
> >
> > 2. If we use mboxes with mbox-names then we could skip that since we
> > can directly specify which "smsm host" a given mailbox is for.
> >
> > My only question really is whether 1. or 2. is a better idea.
> >
> > Is this clearer now or still not?
>
>
> So again, does the driver care about missing entry? If so, why?
What do you mean with "care"?
I didn't change any behavior to what's happening now, if e.g. qcom,ipc-3
is not set right now then the driver is okay with that and just won't
ring the mailbox for that smsm host.
The behavior will be the same with mbox, if a mbox for e.g. the 3rd smsm
host is not set, the driver is okay with that but then of course won't do
anything for that host.
See the driver patch for
Re: [PATCH 1/5] dt-bindings: remoteproc: qcom,sm8550-pas: Document the SA8775p ADSP, CDSP and GPDSP
On 22.05.2024 3:08 PM, Bartosz Golaszewski wrote: > On Wed, May 22, 2024 at 3:06 PM wrote: >> >> On 22/05/2024 15:04, Bartosz Golaszewski wrote: >>> On Wed, May 22, 2024 at 2:42 PM wrote: On 22/05/2024 14:08, Bartosz Golaszewski wrote: > From: Tengfei Fan > > Document the compatibles for the components used to boot the ADSP, CDSP0, > CDSP1, GPDSP0 and GPDSP1 on the SA8775p SoC. > > Signed-off-by: Tengfei Fan > Co-developed-by: Bartosz Golaszewski > Signed-off-by: Bartosz Golaszewski > --- >.../bindings/remoteproc/qcom,sm8550-pas.yaml | 76 > +- >1 file changed, 75 insertions(+), 1 deletion(-) > > diff --git > a/Documentation/devicetree/bindings/remoteproc/qcom,sm8550-pas.yaml > b/Documentation/devicetree/bindings/remoteproc/qcom,sm8550-pas.yaml > index 73fda7565cd1..9d3a862c39e1 100644 > --- a/Documentation/devicetree/bindings/remoteproc/qcom,sm8550-pas.yaml > +++ b/Documentation/devicetree/bindings/remoteproc/qcom,sm8550-pas.yaml > @@ -16,6 +16,11 @@ description: >properties: > compatible: >enum: > + - qcom,sa8775p-adsp-pas > + - qcom,sa8775p-cdsp0-pas > + - qcom,sa8775p-cdsp1-pas > + - qcom,sa8775p-gpdsp0-pas > + - qcom,sa8775p-gpdsp1-pas > - qcom,sm8550-adsp-pas > - qcom,sm8550-cdsp-pas > - qcom,sm8550-mpss-pas > @@ -44,12 +49,13 @@ properties: > > firmware-name: >$ref: /schemas/types.yaml#/definitions/string-array > +minItems: 1 This will allow a single firmware name for all compatible, which is wrong >>> >>> So increasing the limit from the default under allOf doesn't seem to >>> work, should I instead keep this and make the lower limit stricter for >>> all other models? >> >> Yes add minItems in all the allOf:if: and add the missing allOf:if: for >> the new compatibles to set the minItems, same for memory-region. >> >> Or you may simply spin off a new yaml, this one is getting quite large. >> > > Yeah, maybe that's a better idea. + if you get rid of the 0/1 in "nsp0/nsp1" you save a couple more lines Konrad
Re: [PATCH 1/2] docs/livepatch: Add new compiler considerations doc
From: mpdeso...@suse.com
On Wed, 2 Sep 2020 15:45:33 +0200 (CEST) Miroslav Benes
wrote:
> Hi,
>
> first, I'm sorry for the late reply. Thanks, Josh, for the reminder.
>
> CCing Nicolai. Nicolai, could you take a look at the proposed
> documentation too, please? You have more up-to-date experience.
>
> On Tue, 21 Jul 2020, Joe Lawrence wrote:
>
> > +Examples
> > +
> > +
> > +Interprocedural optimization (IPA)
> > +--
> > +
> > +Function inlining is probably the most common compiler optimization that
> > +affects livepatching. In a simple example, inlining transforms the
> > original
> > +code::
> > +
> > + foo() { ... [ foo implementation ] ... }
> > +
> > + bar() { ... foo() ... }
> > +
> > +to::
> > +
> > + bar() { ... [ foo implementation ] ... }
> > +
> > +Inlining is comparable to macro expansion, however the compiler may inline
> > +cases which it determines worthwhile (while preserving original call/return
> > +semantics in others) or even partially inline pieces of functions (see cold
> > +functions in GCC function suffixes section below).
> > +
> > +To safely livepatch ``foo()`` from the previous example, all of its callers
> > +need to be taken into consideration. For those callers that the compiler
> > had
> > +inlined ``foo()``, a livepatch should include a new version of the calling
> > +function such that it:
> > +
> > + 1. Calls a new, patched version of the inlined function, or
> > + 2. Provides an updated version of the caller that contains its own
> > inlined
> > + and updated version of the inlined function
>
> I'm afraid the above could cause a confusion...
>
> "1. Calls a new, patched version of the inlined function, or". The
> function is not inlined in this case. Would it be more understandable to
> use function names?
>
> 1. Calls a new, patched version of function foo(), or
> 2. Provides an updated version of bar() that contains its own inlined and
>updated version of foo() (as seen in the example above).
>
> Not to say that it is again a call of the compiler to decide that, so one
> usually prepares an updated version of foo() and updated version of bar()
> calling to it. Updated foo() has to be there for non-inlined cases anyway.
>
> > +
> > +Other interesting IPA examples include:
> > +
> > +- *IPA-SRA*: removal of unused parameters, replace parameters passed by
> > + referenced by parameters passed by value. This optimization basically
>
> s/referenced/reference/
>
> > + violates ABI.
> > +
> > + .. note::
> > + GCC changes the name of function. See GCC function suffixes
> > + section below.
> > +
> > +- *IPA-CP*: find values passed to functions are constants and then
> > optimizes
> > + accordingly Several clones of a function are possible if a set is
> > limited.
>
> "...accordingly. Several..."
>
> [...]
>
> > + void cdev_put(struct cdev *p)
> > + {
> > + if (p) {
> > + struct module *owner = p->owner;
> > + kobject_put(>kobj);
> > + module_put(owner);
> > + }
> > + }
>
> git am complained here about whitespace damage.
>
> [...]
>
> > +kgraft-analysis-tool
> > +
> > +
> > +With the -fdump-ipa-clones flag, GCC will dump IPA clones that were created
> > +by all inter-procedural optimizations in ``.000i.ipa-clones``
> > files.
> > +
> > +kgraft-analysis-tool pretty-prints those IPA cloning decisions. The full
> > +list of affected functions provides additional updates that the
> > source-based
> > +livepatch author may need to consider. For example, for the function
> > +``scatterwalk_unmap()``:
> > +
> > +::
> > +
> > + $ ./kgraft-ipa-analysis.py --symbol=scatterwalk_unmap
> > aesni-intel_glue.i.000i.ipa-clones
> > + Function: scatterwalk_unmap/2930 (include/crypto/scatterwalk.h:81:60)
> > +isra: scatterwalk_unmap.isra.2/3142
> > (include/crypto/scatterwalk.h:81:60)
> > + inlining to: helper_rfc4106_decrypt/3007
> > (arch/x86/crypto/aesni-intel_glue.c:1016:12)
> > + inlining to: helper_rfc4106_decrypt/3007
> > (arch/x86/crypto/aesni-intel_glue.c:1016:12)
> > + inlining to: helper_rfc4106_encrypt/3006
> > (arch/x86/crypto/aesni-intel_glue.c:939:12)
> > +
> > +Affected functions: 3
> > + scatterwalk_unmap.isra.2/3142 (include/crypto/scatterwalk.h:81:60)
> > + helper_rfc4106_decrypt/3007
> > (arch/x86/crypto/aesni-intel_glue.c:1016:12)
> > + helper_rfc4106_encrypt/3006
> > (arch/x86/crypto/aesni-intel_glue.c:939:12)
>
> The example for the github is not up-to-date. The tool now expects
> file_list with *.ipa-clones files and the output is a bit different for
> the recent kernel.
>
> $ echo arch/x86/crypto/aesni-intel_glue.c.000i.ipa-clones |
> kgraft-ipa-analysis.py --symbol=scatterwalk_unmap /dev/stdin
> Parsing file (1/1): arch/x86/crypto/aesni-intel_glue.c.000i.ipa-clones
> Function: scatterwalk_unmap/3935
Re: [PATCH v2 0/5] livepatch: klp-convert tool - Minimal version
From: mpdeso...@suse.com On Thu, 16 May 2024 15:30:03 +0200 Lukas Hruska wrote: > Summary > --- > > This is a significantly simplified version of the original klp-convert tool. > The klp-convert code has never got a proper review and also clean ups > were not easy. The last version was v7, see > https://lore.kernel.org/r/20230306140824.3858543-1-joe.lawre...@redhat.com > > The main change is that the tool does not longer search for the > symbols which would need the livepatch specific relocation entry. > Also klp.symbols file is not longer needed. > > Instead, the needed information is appended to the symbol declaration > via a new macro KLP_RELOC_SYMBOL(). It creates symbol with all needed > metadata. For example: > > extern char *saved_command_line \ > KLP_RELOC_SYMBOL(vmlinux, vmlinux, saved_command_line, 0); > > would create symbol > > $>readelf -r -W : > Relocation section '.rela.text' at offset 0x32e60 contains 10 entries: > Offset Info Type Symbol's Value > Symbol's Name + Addend > [...] > 0068 003c0002 R_X86_64_PC32 > .klp.sym.rela.vmlinux.vmlinux.saved_command_line,0 - 4 > [...] > > > The simplified klp-convert tool just transforms symbols > created by KLP_RELOC_SYMBOL() to object specific rela sections > and rela entries which would later be proceed when the livepatch > or the livepatched object is loaded. > > For example, klp-convert would replace the above symbols with: > > $> readelf -r -W > Relocation section '.klp.rela.vmlinux.text' at offset 0x5cb60 contains 1 > entry: > Offset Info Type Symbol's Value > Symbol's Name + Addend > 0068 003c0002 R_X86_64_PC32 > .klp.sym.vmlinux.saved_command_line,0 - 4 > > > Note that similar macro was needed also in the original version > to handle more symbols of the same name (sympos). > > Given the above, add klp-convert tool; integrate klp-convert tool into > kbuild; add data-structure and macros to enable users to annotate > livepatch source code; make modpost stage compatible with livepatches; > update livepatch-sample and update documentation. > > > Testing > --- > > The patchset selftests build and execute on x86_64, s390x, and ppc64le > for both default config (with added livepatch dependencies) and a larger > SLE-15-ish config. > > > Summary of changes in this minimal version v2 > > > - rebase for v6.9 > - cleaned-up SoB chains (suggested by pmladek) > - klp-convert: remove the symbol map auto-resolving solution > - klp-convert: add macro for flagging variables inside a LP src to be > resolved by this tool > - klp-convert: code simplification > - selftests: add selftest livepatching function using an external symbol Thanks for sending this new version Lukas! It currently fails to apply on current Linux master, but the conflict is very simple to address (attached patch). Joe, Josh, other people, can you also take a look in the patchset? It would be good we can move this forward. Thanks, Marcos commit 1a1cf8b0967c26857b17e8ceb02f6a1bd854667d Author: Marcos Paulo de Souza Date: Wed May 29 10:18:38 2024 -0300 Solve merge problem Signed-off-by: Marcos Paulo de Souza diff --git a/Makefile b/Makefile index f975b6396328..579dfb46e691 100644 --- a/Makefile +++ b/Makefile @@ -1491,7 +1491,7 @@ endif # CONFIG_MODULES # Directories & files removed with 'make clean' CLEAN_FILES += vmlinux.symvers modules-only.symvers \ modules.builtin modules.builtin.modinfo modules.nsdeps \ - compile_commands.json rust/test \ + compile_commands.json .thinlto-cache rust/test \ rust-project.json .vmlinux.objs .vmlinux.export.c # Directories & files removed with 'make mrproper' > > Previous versions > - > > RFC: > https://lore.kernel.org/r/cover.1477578530.git.jpoim...@redhat.com/ > v2: > https://lore.kernel.org/r/f52d29f7-7d1b-ad3d-050b-a9fa8878f...@redhat.com/ > v3: > https://lore.kernel.org/r/20190410155058.9437-1-joe.lawre...@redhat.com/ > v4: > https://lore.kernel.org/r/20190509143859.9050-1-joe.lawre...@redhat.com/ > v5: > (not posted) > https://github.com/joe-lawrence/klp-convert-tree/tree/klp-convert-v5-devel > v6: > https://lore.kernel.org/r/20220216163940.228309-1-joe.lawre...@redhat.com/ > v7: > https://lore.kernel.org/r/20230306140824.3858543-1-joe.lawre...@redhat.com/ > v1 minimal: > https://lore.kernel.org/r/20231106162513.17556-1-lhru...@suse.cz/
Re: [PATCH 2/2] samples/livepatch: Add README.rst disclaimer
From: mpdeso...@suse.com On Tue, 21 Jul 2020 12:14:07 -0400 Joe Lawrence wrote: > The livepatch samples aren't very careful with respect to compiler > IPA-optimization of target kernel functions. > > Add a quick disclaimer and pointer to the compiler-considerations.rst > file to warn readers. > > Suggested-by: Josh Poimboeuf > Signed-off-by: Joe Lawrence > --- > samples/livepatch/README.rst | 15 +++ > 1 file changed, 15 insertions(+) > create mode 100644 samples/livepatch/README.rst Acked-by: Marcos Paulo de Souza > > diff --git a/samples/livepatch/README.rst b/samples/livepatch/README.rst > new file mode 100644 > index ..2f8ef945f2ac > --- /dev/null > +++ b/samples/livepatch/README.rst > @@ -0,0 +1,15 @@ > +.. SPDX-License-Identifier: GPL-2.0+ > + > +== > +Disclaimer > +== > + > +The livepatch sample programs were written with idealized compiler > +output in mind. That is to say that they do not consider ways in which > +optimization may transform target kernel functions. > + > +The samples present only a simple API demonstration and should not be > +considered completely safe. > + > +See the Documentation/livepatching/compiler-considerations.rst file for > +more details on compiler optimizations and how they affect livepatching. > -- > 2.21.3
Re: [PATCH v2] sched/rt: Clean up usage of rt_task()
On 2024-05-29 11:34:09 [+0100], Qais Yousef wrote: > > behaviour. But then it is insistent which matters only in the RT case. > > Puh. Any sched folks regarding policy? > > I am not sure I understood you here. Could you rephrase please? Right now a SCHED_OTHER task boosted to a realtime priority gets slack=0. In the !RT scenario everything is fine. For RT the slack=0 also happens but the init of the timer looks at the policy instead at the possible boosted priority and uses a different clock attribute. This can lead to a delayed wake up (so avoiding the slack does not solve the problem). This is not consistent because IMHO the clock setup & slack should be handled equally. So I am asking the sched folks for a policy and I am leaning towards looking at task-policy in this case instead of prio because you shouldn't do anything that can delay. Sebastian
Re: [PATCH v2] sched/rt: Clean up usage of rt_task()
On 05/29/24 10:29, Sebastian Andrzej Siewior wrote: > On 2024-05-27 18:26:50 [+0100], Qais Yousef wrote: > > > In order to be PI-boosted you need to acquire a lock and the only lock > > > you can sleep while acquired without generating a warning is a mutex_t > > > (or equivalent sleeping lock) on PREEMPT_RT. > > > > Note we care about the behavior for !PREEMPT_RT. PI issues are important > > there > > too. I assume the fact the PREEMPT_RT changes the locks behavior is what > > you're > > referring to here and not applicable to normal case. > > So for !PREEMPT_RT you need a rtmutex for PI. RCU and i2c is using it > within the kernel and this shouldn't go via the `slack' API. > > The FUTEX API on the other hand is a different story and it might > matter. So you have one task running SCHED_OTHER and acquiring a lock in > userspace (pthread_mutex_t, PTHREAD_PRIO_INHERIT). Another task running > at SCHED_FIFO/ RR/ DL would also acquire that lock, block on it and > then inherit its priority. > This is the point where the former task has a different policy vs > priority considering PI-boosting. You could argue that the task > shouldn't sleep or invoke anything possible sleeping with a timeout > 0 > because it is using an important lock. > But then it is userland and has the freedom to do whatever it wants you > know… Yes.. > > So it might be better to forget what I said and keeping the current Okay I'll drop the patch then in next posting. > behaviour. But then it is insistent which matters only in the RT case. > Puh. Any sched folks regarding policy? I am not sure I understood you here. Could you rephrase please?
Re: [PATCH v3 3/3] sched/rt, dl: Convert functions to return bool
On 05/29/24 09:34, Sebastian Andrzej Siewior wrote:
> On 2024-05-28 00:45:08 [+0100], Qais Yousef wrote:
> > diff --git a/include/linux/sched/deadline.h b/include/linux/sched/deadline.h
> > index 5cb88b748ad6..87d2370dd3db 100644
> > --- a/include/linux/sched/deadline.h
> > +++ b/include/linux/sched/deadline.h
> > @@ -10,7 +10,7 @@
> >
> > #include
> >
> > -static inline int dl_prio(int prio)
> > +static inline bool dl_prio(int prio)
> > {
> > if (unlikely(prio < MAX_DL_PRIO))
> > return 1;
>
> if we return a bool we should return true/ false.
Right. Fixed.
Thanks!
--
Qais Yousef
Re: [PATCH v2] sched/rt: Clean up usage of rt_task()
On 2024-05-27 18:26:50 [+0100], Qais Yousef wrote: > > In order to be PI-boosted you need to acquire a lock and the only lock > > you can sleep while acquired without generating a warning is a mutex_t > > (or equivalent sleeping lock) on PREEMPT_RT. > > Note we care about the behavior for !PREEMPT_RT. PI issues are important there > too. I assume the fact the PREEMPT_RT changes the locks behavior is what > you're > referring to here and not applicable to normal case. So for !PREEMPT_RT you need a rtmutex for PI. RCU and i2c is using it within the kernel and this shouldn't go via the `slack' API. The FUTEX API on the other hand is a different story and it might matter. So you have one task running SCHED_OTHER and acquiring a lock in userspace (pthread_mutex_t, PTHREAD_PRIO_INHERIT). Another task running at SCHED_FIFO/ RR/ DL would also acquire that lock, block on it and then inherit its priority. This is the point where the former task has a different policy vs priority considering PI-boosting. You could argue that the task shouldn't sleep or invoke anything possible sleeping with a timeout > 0 because it is using an important lock. But then it is userland and has the freedom to do whatever it wants you know… So it might be better to forget what I said and keeping the current behaviour. But then it is insistent which matters only in the RT case. Puh. Any sched folks regarding policy? > Thanks! Sebastian
Re: [PATCH v3 3/3] sched/rt, dl: Convert functions to return bool
On 2024-05-28 00:45:08 [+0100], Qais Yousef wrote:
> diff --git a/include/linux/sched/deadline.h b/include/linux/sched/deadline.h
> index 5cb88b748ad6..87d2370dd3db 100644
> --- a/include/linux/sched/deadline.h
> +++ b/include/linux/sched/deadline.h
> @@ -10,7 +10,7 @@
>
> #include
>
> -static inline int dl_prio(int prio)
> +static inline bool dl_prio(int prio)
> {
> if (unlikely(prio < MAX_DL_PRIO))
> return 1;
if we return a bool we should return true/ false.
Sebastian
Re: Re: [RFC PATCH 0/5] vsock/virtio: Add support for multi-devices
Hi, MST! > > include/linux/virtio_vsock.h| 2 +- > > include/net/af_vsock.h | 25 ++- > > include/uapi/linux/virtio_vsock.h | 1 + > > include/uapi/linux/vm_sockets.h | 14 ++ > > net/vmw_vsock/af_vsock.c| 116 +-- > > net/vmw_vsock/virtio_transport.c| 255 ++-- > > net/vmw_vsock/virtio_transport_common.c | 16 +- > > net/vmw_vsock/vsock_loopback.c | 4 +- > > 8 files changed, 352 insertions(+), 81 deletions(-) > > As any change to virtio device/driver interface, this has to > go through the virtio TC. Please subscribe at > virtio-comment+subscr...@lists.linux.dev and then > contact the TC at virtio-comm...@lists.linux.dev > > You will likely eventually need to write a spec draft document, too. Thanks for your reply. I've sent a series of RFC documents for the spec changes to virtio TC [1]. [1]: https://lore.kernel.org/virtio-comment/20240528054725.268173-1-niuxuewei@antgroup.com/ Thanks Xuewei
Re: [PATCH v5 5/7] remoteproc: core: support of the tee interface
Hello Mathieu,
On 5/28/24 23:30, Mathieu Poirier wrote:
> On Tue, May 21, 2024 at 10:09:59AM +0200, Arnaud Pouliquen wrote:
>> 1) on start:
>> - Using the TEE loader, the resource table is loaded by an external entity.
>> In such case the resource table address is not find from the firmware but
>> provided by the TEE remoteproc framework.
>> Use the rproc_get_loaded_rsc_table instead of rproc_find_loaded_rsc_table
>> - test that rproc->cached_table is not null before performing the memcpy
>>
>> 2)on stop
>> The use of the cached_table seems mandatory:
>> - during recovery sequence to have a snapshot of the resource table
>> resources used,
>> - on stop to allow for the deinitialization of resources after the
>> the remote processor has been shutdown.
>> However if the TEE interface is being used, we first need to unmap the
>> table_ptr before setting it to rproc->cached_table.
>> The update of rproc->table_ptr to rproc->cached_table is performed in
>> tee_remoteproc.
>>
>> Signed-off-by: Arnaud Pouliquen
>> ---
>> drivers/remoteproc/remoteproc_core.c | 31 +---
>> 1 file changed, 23 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c
>> b/drivers/remoteproc/remoteproc_core.c
>> index 42bca01f3bde..3a642151c983 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1267,6 +1267,7 @@ EXPORT_SYMBOL(rproc_resource_cleanup);
>> static int rproc_set_rsc_table_on_start(struct rproc *rproc, const struct
>> firmware *fw)
>> {
>> struct resource_table *loaded_table;
>> +struct device *dev = >dev;
>>
>> /*
>> * The starting device has been given the rproc->cached_table as the
>> @@ -1276,12 +1277,21 @@ static int rproc_set_rsc_table_on_start(struct rproc
>> *rproc, const struct firmwa
>> * this information to device memory. We also update the table_ptr so
>> * that any subsequent changes will be applied to the loaded version.
>> */
>> -loaded_table = rproc_find_loaded_rsc_table(rproc, fw);
>> -if (loaded_table) {
>> -memcpy(loaded_table, rproc->cached_table, rproc->table_sz);
>> -rproc->table_ptr = loaded_table;
>> +if (rproc->tee_interface) {
>> +loaded_table = rproc_get_loaded_rsc_table(rproc,
>> >table_sz);
>> +if (IS_ERR(loaded_table)) {
>> +dev_err(dev, "can't get resource table\n");
>> +return PTR_ERR(loaded_table);
>> +}
>> +} else {
>> +loaded_table = rproc_find_loaded_rsc_table(rproc, fw);
>> }
>>
>> +if (loaded_table && rproc->cached_table)
>> +memcpy(loaded_table, rproc->cached_table, rproc->table_sz);
>> +
>
> Why is this not part of the else {} above as it was the case before this
> patch?
> And why was an extra check for ->cached_table added?
Here we have to cover 2 use cases if rproc->tee_interface is set.
1) The remote processor is in stop state
- loaded_table points to the resource table in the remote memory and
- rproc->cached_table is null
=> no memcopy
2) crash recovery
- loaded_table points to the resource table in the remote memory
- rproc-cached_table point to a copy of the resource table
=> need to perform the memcpy to reapply settings in the resource table
I can duplicate the memcpy in if{} and else{} but this will be similar code
as needed in both case.
Adding rproc->cached_table test if proc->tee_interface=NULL seems also
reasonable as a memcpy from 0 should not be performed.
>
> This should be a simple change, i.e introduce an if {} else {} block to take
> care of the two scenarios. Plus the comment is misplaced now.
What about split it in 2 patches?
- one adding the test on rproc->cached_table for the memcpy
- one adding the if {} else {}?
Thanks,
Arnaud
>
> More comments tomorrow.
>
> Thanks,
> Mathieu
>
>> +rproc->table_ptr = loaded_table;
>> +
>> return 0;
>> }
>>
>> @@ -1318,11 +1328,16 @@ static int rproc_reset_rsc_table_on_stop(struct
>> rproc *rproc)
>> kfree(rproc->clean_table);
>>
>> out:
>> -/*
>> - * Use a copy of the resource table for the remainder of the
>> - * shutdown process.
>> +/* If the remoteproc_tee interface is used, then we have first to unmap
>> the resource table
>> + * before updating the proc->table_ptr reference.
>> */
>> -rproc->table_ptr = rproc->cached_table;
>> +if (!rproc->tee_interface) {
>> +/*
>> + * Use a copy of the resource table for the remainder of the
>> + * shutdown process.
>> + */
>> +rproc->table_ptr = rproc->cached_table;
>> +}
>> return 0;
>> }
>>
>> --
>> 2.25.1
>>
Re: [PATCH DNM 2/2] arm64: dts: qcom: qcm6490-fairphone-fp5: Add DisplayPort sound support
On Tue May 28, 2024 at 11:35 PM CEST, Bjorn Andersson wrote:
> On Fri, May 10, 2024 at 02:27:09PM GMT, Luca Weiss wrote:
> > Add the required nodes for sound playback via a connected external
> > display (DisplayPort over USB-C).
> >
> > Signed-off-by: Luca Weiss
> > ---
> > Depends on a bunch of patches upstream doing bringup of Display (DSI),
> > DisplayPort, GPU, and then finally audio could land. But we're blocked
> > on DPU 1:1:1 topology for all of that unfortunately.
> >
> > And also machine driver for sound just exists a bit hackily.
>
> Thanks for sharing this, Luca. Can you please resubmit this once it's
> ready to be merged, so that I don't need to keep track of it?
Definitely, though I don't expect it to be soon unfortunately, maybe you
can ask your colleagues though to fix that DPU 1:1:1 topology ;)
>
> Regards,
> Bjorn
>
> > ---
> > arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts | 36
> > ++
> > 1 file changed, 36 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts
> > b/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts
> > index 05bbf1da5cb8..2bbbcaeff95e 100644
> > --- a/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts
> > +++ b/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts
> > @@ -14,6 +14,8 @@
> > #include
> > #include
> > #include
> > +#include
> > +#include
> > #include "sc7280.dtsi"
> > #include "pm7250b.dtsi"
> > #include "pm7325.dtsi"
> > @@ -774,6 +776,12 @@ _resin {
> > status = "okay";
> > };
> >
> > + {
> > + dai@104 {
> > + reg = ;
> > + };
> > +};
> > +
> > _spi13_cs {
> > drive-strength = <6>;
> > bias-disable;
> > @@ -847,6 +855,34 @@ _2 {
> > status = "okay";
> > };
> >
> > + {
> > + compatible = "fairphone,fp5-sndcard";
> > + model = "Fairphone 5";
> > +
> > + mm1-dai-link {
> > + link-name = "MultiMedia1";
> > + cpu {
> > + sound-dai = < MSM_FRONTEND_DAI_MULTIMEDIA1>;
> > + };
> > + };
> > +
> > + displayport-rx-dai-link {
> > + link-name = "DisplayPort Playback";
> > +
> > + cpu {
> > + sound-dai = < DISPLAY_PORT_RX>;
> > + };
> > +
> > + platform {
> > + sound-dai = <>;
> > + };
> > +
> > + codec {
> > + sound-dai = <_dp>;
> > + };
> > + };
> > +};
> > +
> > {
> > status = "okay";
> >
> >
> > --
> > 2.45.0
> >
Re: [PATCH net-next 0/5] net: constify ctl_table arguments of utility functions
Hello: This series was applied to netdev/net-next.git (main) by Jakub Kicinski : On Mon, 27 May 2024 19:04:18 +0200 you wrote: > The sysctl core is preparing to only expose instances of > struct ctl_table as "const". > This will also affect the ctl_table argument of sysctl handlers. > > As the function prototype of all sysctl handlers throughout the tree > needs to stay consistent that change will be done in one commit. > > [...] Here is the summary with links: - [net-next,1/5] net/neighbour: constify ctl_table arguments of utility function https://git.kernel.org/netdev/net-next/c/874aa96d78c7 - [net-next,2/5] net/ipv4/sysctl: constify ctl_table arguments of utility functions https://git.kernel.org/netdev/net-next/c/551814313f11 - [net-next,3/5] net/ipv6/addrconf: constify ctl_table arguments of utility functions https://git.kernel.org/netdev/net-next/c/c55eb03765f4 - [net-next,4/5] net/ipv6/ndisc: constify ctl_table arguments of utility function https://git.kernel.org/netdev/net-next/c/7a20cd1e71d8 - [net-next,5/5] ipvs: constify ctl_table arguments of utility functions https://git.kernel.org/netdev/net-next/c/0a9f788fdde4 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
Re: [PATCH 01/12] soc: qcom: add firmware name helper
On Mon, May 27, 2024 at 02:42:44PM GMT, Dmitry Baryshkov wrote: > On Thu, 23 May 2024 at 01:48, Bjorn Andersson > wrote: > > > > On Tue, May 21, 2024 at 03:08:31PM +0200, Dmitry Baryshkov wrote: > > > On Tue, 21 May 2024 at 13:20, Kalle Valo wrote: > > > > > > > > Dmitry Baryshkov writes: > > > > > > > > > On Tue, 21 May 2024 at 12:52, wrote: > > > > >> > > > > >> On 21/05/2024 11:45, Dmitry Baryshkov wrote: > > > > >> > Qualcomm platforms have different sets of the firmware files, which > > > > >> > differ from platform to platform (and from board to board, due to > > > > >> > the > > > > >> > embedded signatures). Rather than listing all the firmware files, > > > > >> > including full paths, in the DT, provide a way to determine > > > > >> > firmware > > > > >> > path based on the root DT node compatible. > > > > >> > > > > >> Ok this looks quite over-engineered but necessary to handle the > > > > >> legacy, > > > > >> but I really think we should add a way to look for a board-specific > > > > >> path > > > > >> first and fallback to those SoC specific paths. > > > > > > > > > > Again, CONFIG_FW_LOADER_USER_HELPER => delays. > > > > > > > > To me this also looks like very over-engineered, can you elaborate more > > > > why this is needed? Concrete examples would help to understand better. > > > > > > Sure. During the meeting last week Arnd suggested evaluating if we can > > > drop firmware-name from the board DT files. Several reasons for that: > > > - DT should describe the hardware, not the Linux-firmware locations > > > - having firmware name in DT complicates updating the tree to use > > > different firmware API (think of mbn vs mdt vs any other format) > > > - If the DT gets supplied by the vendor (e.g. for > > > SystemReady-certified devices), there should be a sync between the > > > vendor's DT, linux kernel and the rootfs. Dropping firmware names from > > > DT solves that by removing one piece of the equation > > > > > > Now for the complexity of the solution. Each SoC family has their own > > > firmware set. This includes firmware for the DSPs, for modem, WiFi > > > bits, GPU shader, etc. > > > For the development boards these devices are signed by the testing key > > > and the actual signature is not validated against the root of trust > > > certificate. > > > For the end-user devices the signature is actually validated against > > > the bits fused to the SoC during manufacturing process. CA certificate > > > (and thus the fuses) differ from vendor to vendor (and from the device > > > to device) > > > > > > Not all of the firmware files are a part of the public linux-firmware > > > tree. However we need to support the rootfs bundled with the firmware > > > for different platforms (both public and vendor). The non-signed files > > > come from the Adreno GPU and can be shared between platforms. All > > > other files are SoC-specific and in some cases device-specific. > > > > > > So for example the SDM845 db845c (open device) loads following firmware > > > files: > > > Not signed: > > > - qcom/a630_sqe.fw > > > - qcom/a630_gmu.bin > > > > > > Signed, will work for any non-secured sdm845 device: > > > - qcom/sdm845/a630_zap.mbn > > > - qcom/sdm845/adsp.mbn > > > - qcom/sdm845/cdsp.mbn > > > - qcom/sdm485/mba.mbn > > > - qcom/sdm845/modem.mbn > > > - qcom/sdm845/wlanmdsp.mbn (loaded via TQFTP) > > > - qcom/venus-5.2/venus.mbn > > > > > > Signed, works only for DB845c. > > > - qcom/sdm845/Thundercomm/db845c/slpi.mbn > > > > > > In comparison, the SDM845 Pixel-3 phone (aka blueline) should load the > > > following firmware files: > > > - qcom/a630_sqe.fw (the same, non-signed file) > > > - qcom/a630_gmu.bin (the same, non-signed file) > > > - qcom/sdm845/Google/blueline/a630_zap.mbn > > > > How do you get from "a630_zap.mbn" to this? By extending the lookup > > table for every target, or what am I missing? > > More or less so. Matching the root OF node gives us the firmware > location, then it gets prepended to all firmware targets. Not an ideal > solution, as there is no fallback support, but at least it gives us > some points to discuss (and to decide whether to move to some > particular direction or to abandon the idea completely, making Arnd > unhappy again). > I understand the desire to not put linux-firmware-specific paths in the DeviceTree, but I think I'm less keen on having a big lookup table in the kernel... Regards, Bjorn
Re: (subset) [PATCH v2 0/3] Convert qcom,hfpll documentation to yaml + related changes
On Sun, 18 Feb 2024 21:57:24 +0100, Luca Weiss wrote: > Finally touch the hfpll doc and convert it to yaml, and do some related > changes along the way. > > Applied, thanks! [3/3] arm64: dts: qcom: qcs404: Use qcs404-hfpll compatible for hfpll commit: 839936d9676bdc2e4dde63631131feb8870fa4d2 Best regards, -- Bjorn Andersson
Re: (subset) [PATCH 0/2] Mark qcom,ipc as deprecated in two schemas
On Thu, 25 Apr 2024 21:14:29 +0200, Luca Weiss wrote: > The mboxes property has been supported in those bindings since a while > and was always meant to the replace qcom,ipc properties, so let's mark > qcom,ipc as deprecated - and update the examples to use mboxes. > > Related: > https://lore.kernel.org/linux-arm-msm/20240424-apcs-mboxes-v1-0-6556c47cb...@z3ntu.xyz/ > > [...] Applied, thanks! [2/2] dt-bindings: soc: qcom,smp2p: Mark qcom,ipc as deprecated commit: 7ce966eb6f1288eb92bc2eb5df8933acee1ae6ed Best regards, -- Bjorn Andersson
Re: [PATCH] clk: qcom: gcc-sm6350: Fix gpll6* & gpll7 parents
On Wed, 08 May 2024 10:12:53 +0200, Luca Weiss wrote: > Both gpll6 and gpll7 are parented to CXO at 19.2 MHz and not to GPLL0 > which runs at 600 MHz. Also gpll6_out_even should have the parent gpll6 > and not gpll0. > > Adjust the parents of these clocks to make Linux report the correct rate > and not absurd numbers like gpll7 at ~25 GHz or gpll6 at 24 GHz. > > [...] Applied, thanks! [1/1] clk: qcom: gcc-sm6350: Fix gpll6* & gpll7 parents commit: 3414f41a13eb41db15c558fbc695466203dca4fa Best regards, -- Bjorn Andersson
Re: [PATCH 0/2] Enable vibrator on PMI632 + Fairphone 3
On Thu, 18 Apr 2024 08:36:53 +0200, Luca Weiss wrote: > With the patches to add vibration support for PMI632 finally applied, > let's enable this for the PMI632 PMIC and Fairphone 3 smartphone. > > https://lore.kernel.org/linux-arm-msm/20240416-pm8xxx-vibrator-new-design-v11-0-7b1c951e1...@quicinc.com/ > > Patches have landed in the input tree: > https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/ > > [...] Applied, thanks! [1/2] arm64: dts: qcom: pmi632: Add vibrator commit: bbb1dd6402f9c67ea00bc6bf0e2a01d71db4c7fd [2/2] arm64: dts: qcom: sdm632-fairphone-fp3: Enable vibrator commit: ffaa4b5d5d07aed600d82929d8862263ce341a71 Best regards, -- Bjorn Andersson
Re: [PATCH] mctp i2c: Add rx trace
Hi Tal, Thanks for the contribution! Some comments: > mctp-i2c rx implementation doesn't call > __i2c_transfer which calls the i2c reply trace function. No, but we can trace the i2c rx path through the trace_i2c_slave tracepoint. It is a little messier than tracing trace_i2c_write, but has been sufficient with the debugging I've needed in the past. > Add an mctp_reply trace function that will be used instead. Can you elaborate a little on what you were/are looking to inspect here? (mainly: which packet fields are you interested in?) That will help to determine the best approach here. Cheers, Jeremy
Re: [PATCH 0/3] tracing: Fix some selftest issues
On Wed, 29 May 2024 01:46:40 +0900 Masami Hiramatsu (Google) wrote: > On Mon, 27 May 2024 19:29:07 -0400 > Steven Rostedt wrote: > > > On Sun, 26 May 2024 19:10:57 +0900 > > "Masami Hiramatsu (Google)" wrote: > > > > > Hi, > > > > > > Here is a series of some fixes/improvements for the test modules and boot > > > time selftest of kprobe events. I found a WARNING message with some boot > > > time selftest configuration, which came from the combination of embedded > > > kprobe generate API tests module and ftrace boot-time selftest. So the > > > main > > > problem is that the test module should not be built-in. But I also think > > > this WARNING message is useless (because there are warning messages > > > already) > > > and the cleanup code is redundant. This series fixes those issues. > > > > Note, when I enable trace tests as builtin instead of modules, I just > > disable the bootup self tests when it detects this. This helps with > > doing tests via config options than having to add user space code that > > loads modules. > > > > Could you do something similar? > > OK, in that case, I would like to move the test cleanup code in > module_exit function into the end of module_init function. > It looks there is no reason to split those into 2 parts. Wait, I would like to hear Tom's opinion. I found following usage comments in the code. * Following that are a few examples using the created events to test * various ways of tracing a synthetic event. * * To test, select CONFIG_SYNTH_EVENT_GEN_TEST and build the module. * Then: * * # insmod kernel/trace/synth_event_gen_test.ko * # cat /sys/kernel/tracing/trace * * You should see several events in the trace buffer - * "create_synth_test", "empty_synth_test", and several instances of * "gen_synth_test". * * To remove the events, remove the module: * * # rmmod synth_event_gen_test Tom, is that intended behavior ? and are you expected to reuse these events outside of the module? e.g. load the test module and run some test script in user space which uses those events? As far as I can see, those tests are not intended to be embedded in the kernel because those are expected to be removed. Thank you, > > Thank you, > > > > > -- Steve > > > > > > > > > > Thank you, > > > > > > --- > > > > > > Masami Hiramatsu (Google) (3): > > > tracing: Build event generation tests only as modules > > > tracing/kprobe: Remove unneeded WARN_ON_ONCE() in selftests > > > tracing/kprobe: Remove cleanup code unrelated to selftest > > > > > > -- > Masami Hiramatsu (Google) -- Masami Hiramatsu (Google)
Re: [PATCH DNM 2/2] arm64: dts: qcom: qcm6490-fairphone-fp5: Add DisplayPort sound support
On Fri, May 10, 2024 at 02:27:09PM GMT, Luca Weiss wrote:
> Add the required nodes for sound playback via a connected external
> display (DisplayPort over USB-C).
>
> Signed-off-by: Luca Weiss
> ---
> Depends on a bunch of patches upstream doing bringup of Display (DSI),
> DisplayPort, GPU, and then finally audio could land. But we're blocked
> on DPU 1:1:1 topology for all of that unfortunately.
>
> And also machine driver for sound just exists a bit hackily.
Thanks for sharing this, Luca. Can you please resubmit this once it's
ready to be merged, so that I don't need to keep track of it?
Regards,
Bjorn
> ---
> arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts | 36
> ++
> 1 file changed, 36 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts
> b/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts
> index 05bbf1da5cb8..2bbbcaeff95e 100644
> --- a/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts
> +++ b/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts
> @@ -14,6 +14,8 @@
> #include
> #include
> #include
> +#include
> +#include
> #include "sc7280.dtsi"
> #include "pm7250b.dtsi"
> #include "pm7325.dtsi"
> @@ -774,6 +776,12 @@ _resin {
> status = "okay";
> };
>
> + {
> + dai@104 {
> + reg = ;
> + };
> +};
> +
> _spi13_cs {
> drive-strength = <6>;
> bias-disable;
> @@ -847,6 +855,34 @@ _2 {
> status = "okay";
> };
>
> + {
> + compatible = "fairphone,fp5-sndcard";
> + model = "Fairphone 5";
> +
> + mm1-dai-link {
> + link-name = "MultiMedia1";
> + cpu {
> + sound-dai = < MSM_FRONTEND_DAI_MULTIMEDIA1>;
> + };
> + };
> +
> + displayport-rx-dai-link {
> + link-name = "DisplayPort Playback";
> +
> + cpu {
> + sound-dai = < DISPLAY_PORT_RX>;
> + };
> +
> + platform {
> + sound-dai = <>;
> + };
> +
> + codec {
> + sound-dai = <_dp>;
> + };
> + };
> +};
> +
> {
> status = "okay";
>
>
> --
> 2.45.0
>
Re: [PATCH v5 5/7] remoteproc: core: support of the tee interface
On Tue, May 21, 2024 at 10:09:59AM +0200, Arnaud Pouliquen wrote:
> 1) on start:
> - Using the TEE loader, the resource table is loaded by an external entity.
> In such case the resource table address is not find from the firmware but
> provided by the TEE remoteproc framework.
> Use the rproc_get_loaded_rsc_table instead of rproc_find_loaded_rsc_table
> - test that rproc->cached_table is not null before performing the memcpy
>
> 2)on stop
> The use of the cached_table seems mandatory:
> - during recovery sequence to have a snapshot of the resource table
> resources used,
> - on stop to allow for the deinitialization of resources after the
> the remote processor has been shutdown.
> However if the TEE interface is being used, we first need to unmap the
> table_ptr before setting it to rproc->cached_table.
> The update of rproc->table_ptr to rproc->cached_table is performed in
> tee_remoteproc.
>
> Signed-off-by: Arnaud Pouliquen
> ---
> drivers/remoteproc/remoteproc_core.c | 31 +---
> 1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> index 42bca01f3bde..3a642151c983 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1267,6 +1267,7 @@ EXPORT_SYMBOL(rproc_resource_cleanup);
> static int rproc_set_rsc_table_on_start(struct rproc *rproc, const struct
> firmware *fw)
> {
> struct resource_table *loaded_table;
> + struct device *dev = >dev;
>
> /*
>* The starting device has been given the rproc->cached_table as the
> @@ -1276,12 +1277,21 @@ static int rproc_set_rsc_table_on_start(struct rproc
> *rproc, const struct firmwa
>* this information to device memory. We also update the table_ptr so
>* that any subsequent changes will be applied to the loaded version.
>*/
> - loaded_table = rproc_find_loaded_rsc_table(rproc, fw);
> - if (loaded_table) {
> - memcpy(loaded_table, rproc->cached_table, rproc->table_sz);
> - rproc->table_ptr = loaded_table;
> + if (rproc->tee_interface) {
> + loaded_table = rproc_get_loaded_rsc_table(rproc,
> >table_sz);
> + if (IS_ERR(loaded_table)) {
> + dev_err(dev, "can't get resource table\n");
> + return PTR_ERR(loaded_table);
> + }
> + } else {
> + loaded_table = rproc_find_loaded_rsc_table(rproc, fw);
> }
>
> + if (loaded_table && rproc->cached_table)
> + memcpy(loaded_table, rproc->cached_table, rproc->table_sz);
> +
Why is this not part of the else {} above as it was the case before this patch?
And why was an extra check for ->cached_table added?
This should be a simple change, i.e introduce an if {} else {} block to take
care of the two scenarios. Plus the comment is misplaced now.
More comments tomorrow.
Thanks,
Mathieu
> + rproc->table_ptr = loaded_table;
> +
> return 0;
> }
>
> @@ -1318,11 +1328,16 @@ static int rproc_reset_rsc_table_on_stop(struct rproc
> *rproc)
> kfree(rproc->clean_table);
>
> out:
> - /*
> - * Use a copy of the resource table for the remainder of the
> - * shutdown process.
> + /* If the remoteproc_tee interface is used, then we have first to unmap
> the resource table
> + * before updating the proc->table_ptr reference.
>*/
> - rproc->table_ptr = rproc->cached_table;
> + if (!rproc->tee_interface) {
> + /*
> + * Use a copy of the resource table for the remainder of the
> + * shutdown process.
> + */
> + rproc->table_ptr = rproc->cached_table;
> + }
> return 0;
> }
>
> --
> 2.25.1
>
Re: [PATCH v10] remoteproc: qcom: Move minidump related layout and API to soc/qcom directory
On Fri, May 03, 2024 at 01:48:07PM GMT, Mukesh Ojha wrote:
> Currently, Qualcomm Minidump is being used to collect mini version of
> remoteproc coredump with the help of boot firmware however, Minidump
> as a feature is not limited to be used only for remote processor and
> can also be used for Application processors. So, in preparation of
> using it move the Minidump related data structure and its function
> to its own file under drivers/soc/qcom with qcom_rproc_minidump.c
> which clearly says it is only for remoteproc minidump.
>
> Extra changes made apart from the movement is,
> 1. Adds new config, kernel headers and module macros to get this
>module compiled.
> 2. Guards the qcom_minidump() with CONFIG_QCOM_RPROC_MINIDUMP.
> 3. Selects this QCOM_RPROC_MINIDUMP config when QCOM_RPROC_COMMON
>enabled.
> 4. Added new header qcom_minidump.h .
>
> Signed-off-by: Mukesh Ojha
I wouldn't be able to merge this without anything depending on it...
[..]
> diff --git a/drivers/soc/qcom/qcom_rproc_minidump.c
> b/drivers/soc/qcom/qcom_rproc_minidump.c
[..]
> +void qcom_minidump(struct rproc *rproc, unsigned int minidump_id,
> + void (*rproc_dumpfn_t)(struct rproc *rproc,
> + struct rproc_dump_segment *segment, void *dest, size_t offset,
> + size_t size))
> +{
> + int ret;
> + struct minidump_subsystem *subsystem;
> + struct minidump_global_toc *toc;
> +
> + /* Get Global minidump ToC*/
> + toc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, NULL);
> +
> + /* check if global table pointer exists and init is set */
> + if (IS_ERR(toc) || !toc->status) {
> + dev_err(>dev, "Minidump TOC not found in SMEM\n");
> + return;
> + }
> +
> + /* Get subsystem table of contents using the minidump id */
> + subsystem = >subsystems[minidump_id];
> +
> + /**
> + * Collect minidump if SS ToC is valid and segment table
> + * is initialized in memory and encryption status is set.
> + */
> + if (subsystem->regions_baseptr == 0 ||
> + le32_to_cpu(subsystem->status) != 1 ||
> + le32_to_cpu(subsystem->enabled) != MINIDUMP_SS_ENABLED) {
> + return rproc_coredump(rproc);
> + }
> +
> + if (le32_to_cpu(subsystem->encryption_status) != MINIDUMP_SS_ENCR_DONE)
> {
> + dev_err(>dev, "Minidump not ready, skipping\n");
> + return;
> + }
> +
> + /**
> + * Clear out the dump segments populated by parse_fw before
> + * re-populating them with minidump segments.
> + */
> + rproc_coredump_cleanup(rproc);
I don't think this should be invoked outside drivers/remoteproc, and the
comment talks about a remoteproc-internal concern...
> +
> + ret = qcom_add_minidump_segments(rproc, subsystem, rproc_dumpfn_t);
This function changes the internal state of the remoteproc and relies on
other operations to clean things up.
I think we could come up with a better design of this, and I don't think
we should spread this outside of the remoteproc framework.
Regards,
Bjorn
> + if (ret) {
> + dev_err(>dev, "Failed with error: %d while adding
> minidump entries\n", ret);
> + goto clean_minidump;
> + }
> + rproc_coredump_using_sections(rproc);
> +clean_minidump:
> + qcom_minidump_cleanup(rproc);
> +}
> +EXPORT_SYMBOL_GPL(qcom_minidump);
> +
> +MODULE_DESCRIPTION("Qualcomm Remoteproc Minidump helper module");
> +MODULE_LICENSE("GPL");
> diff --git a/include/soc/qcom/qcom_minidump.h
> b/include/soc/qcom/qcom_minidump.h
> new file mode 100644
> index ..0fe156066bc0
> --- /dev/null
> +++ b/include/soc/qcom/qcom_minidump.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#ifndef _QCOM_MINIDUMP_H_
> +#define _QCOM_MINIDUMP_H_
> +
> +struct rproc;
> +struct rproc_dump_segment;
> +
> +#if IS_ENABLED(CONFIG_QCOM_RPROC_MINIDUMP)
> +void qcom_minidump(struct rproc *rproc, unsigned int minidump_id,
> +void (*rproc_dumpfn_t)(struct rproc *rproc,
> +struct rproc_dump_segment *segment, void *dest, size_t
> offset,
> +size_t size));
> +#else
> +static inline void qcom_minidump(struct rproc *rproc, unsigned int
> minidump_id,
> +void (*rproc_dumpfn_t)(struct rproc *rproc,
> +struct rproc_dump_segment *segment, void *dest, size_t
> offset,
> +size_t size)) { }
> +#endif /* CONFIG_QCOM_RPROC_MINIDUMP */
> +#endif /* _QCOM_MINIDUMP_H_ */
> --
> 2.7.4
>
Re: [PATCH v5 4/7] remoteproc: core introduce rproc_set_rsc_table_on_start function
On Tue, May 21, 2024 at 10:09:58AM +0200, Arnaud Pouliquen wrote:
> Split rproc_start()to prepare the update of the management of
I don't see any "splitting" for rproc_start() in this patch. Please consider
rewording or removing.
> the cache table on start, for the support of the firmware loading
> by the TEE interface.
> - create rproc_set_rsc_table_on_start() to address the management of
> the cache table in a specific function, as done in
> rproc_reset_rsc_table_on_stop().
> - rename rproc_set_rsc_table in rproc_set_rsc_table_on_attach()
> - move rproc_reset_rsc_table_on_stop() to be close to the
> rproc_set_rsc_table_on_start() function
This patch is really hard to read due to all 3 operations happening at the same
time. Please split in 3 smaller patches.
>
> Suggested-by: Mathieu Poirier
> Signed-off-by: Arnaud Pouliquen
> ---
> drivers/remoteproc/remoteproc_core.c | 116 ++-
> 1 file changed, 62 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> index f276956f2c5c..42bca01f3bde 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1264,18 +1264,9 @@ void rproc_resource_cleanup(struct rproc *rproc)
> }
> EXPORT_SYMBOL(rproc_resource_cleanup);
>
> -static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> +static int rproc_set_rsc_table_on_start(struct rproc *rproc, const struct
> firmware *fw)
> {
> struct resource_table *loaded_table;
> - struct device *dev = >dev;
> - int ret;
> -
> - /* load the ELF segments to memory */
> - ret = rproc_load_segments(rproc, fw);
> - if (ret) {
> - dev_err(dev, "Failed to load program segments: %d\n", ret);
> - return ret;
> - }
>
> /*
>* The starting device has been given the rproc->cached_table as the
> @@ -1291,6 +1282,64 @@ static int rproc_start(struct rproc *rproc, const
> struct firmware *fw)
> rproc->table_ptr = loaded_table;
> }
>
> + return 0;
> +}
> +
> +static int rproc_reset_rsc_table_on_stop(struct rproc *rproc)
> +{
> + /* A resource table was never retrieved, nothing to do here */
> + if (!rproc->table_ptr)
> + return 0;
> +
> + /*
> + * If a cache table exists the remote processor was started by
> + * the remoteproc core. That cache table should be used for
> + * the rest of the shutdown process.
> + */
> + if (rproc->cached_table)
> + goto out;
> +
> + /*
> + * If we made it here the remote processor was started by another
> + * entity and a cache table doesn't exist. As such make a copy of
> + * the resource table currently used by the remote processor and
> + * use that for the rest of the shutdown process. The memory
> + * allocated here is free'd in rproc_shutdown().
> + */
> + rproc->cached_table = kmemdup(rproc->table_ptr,
> + rproc->table_sz, GFP_KERNEL);
> + if (!rproc->cached_table)
> + return -ENOMEM;
> +
> + /*
> + * Since the remote processor is being switched off the clean table
> + * won't be needed. Allocated in rproc_set_rsc_table_on_start().
> + */
> + kfree(rproc->clean_table);
> +
> +out:
> + /*
> + * Use a copy of the resource table for the remainder of the
> + * shutdown process.
> + */
> + rproc->table_ptr = rproc->cached_table;
> + return 0;
> +}
> +
> +static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> +{
> + struct device *dev = >dev;
> + int ret;
> +
> + /* load the ELF segments to memory */
> + ret = rproc_load_segments(rproc, fw);
> + if (ret) {
> + dev_err(dev, "Failed to load program segments: %d\n", ret);
> + return ret;
> + }
> +
> + rproc_set_rsc_table_on_start(rproc, fw);
> +
> ret = rproc_prepare_subdevices(rproc);
> if (ret) {
> dev_err(dev, "failed to prepare subdevices for %s: %d\n",
> @@ -1450,7 +1499,7 @@ static int rproc_fw_boot(struct rproc *rproc, const
> struct firmware *fw)
> return ret;
> }
>
> -static int rproc_set_rsc_table(struct rproc *rproc)
> +static int rproc_set_rsc_table_on_attach(struct rproc *rproc)
> {
> struct resource_table *table_ptr;
> struct device *dev = >dev;
> @@ -1540,54 +1589,13 @@ static int rproc_reset_rsc_table_on_detach(struct
> rproc *rproc)
>
> /*
>* The clean resource table is no longer needed. Allocated in
> - * rproc_set_rsc_table().
> + * rproc_set_rsc_table_on_attach().
>*/
> kfree(rproc->clean_table);
>
> return 0;
> }
>
> -static int rproc_reset_rsc_table_on_stop(struct rproc *rproc)
> -{
> - /* A resource table was never retrieved, nothing to do here */
> - if (!rproc->table_ptr)
> - return 0;
>
Re: [PATCH 2/2] objpool: cache nr_possible_cpus() and avoid caching nr_cpu_ids
On Wed, Apr 24, 2024 at 02:52:14PM -0700, Andrii Nakryiko wrote:
> Profiling shows that calling nr_possible_cpus() in objpool_pop() takes
> a noticeable amount of CPU (when profiled on 80-core machine), as we
> need to recalculate number of set bits in a CPU bit mask. This number
> can't change, so there is no point in paying the price for recalculating
> it. As such, cache this value in struct objpool_head and use it in
> objpool_pop().
>
> On the other hand, cached pool->nr_cpus isn't necessary, as it's not
> used in hot path and is also a pretty trivial value to retrieve. So drop
> pool->nr_cpus in favor of using nr_cpu_ids everywhere. This way the size
> of struct objpool_head remains the same, which is a nice bonus.
>
> Same BPF selftests benchmarks were used to evaluate the effect. Using
> changes in previous patch (inlining of objpool_pop/objpool_push) as
> baseline, here are the differences:
>
> BASELINE
>
> kretprobe :9.937 ± 0.174M/s
> kretprobe-multi: 10.440 ± 0.108M/s
>
> AFTER
> =
> kretprobe : 10.106 ± 0.120M/s (+1.7%)
> kretprobe-multi: 10.515 ± 0.180M/s (+0.7%)
nice, overall lgtm
jirka
>
> Cc: Matt (Qiang) Wu
> Signed-off-by: Andrii Nakryiko
> ---
> include/linux/objpool.h | 6 +++---
> lib/objpool.c | 12 ++--
> 2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/objpool.h b/include/linux/objpool.h
> index d8b1f7b91128..cb1758eaa2d3 100644
> --- a/include/linux/objpool.h
> +++ b/include/linux/objpool.h
> @@ -73,7 +73,7 @@ typedef int (*objpool_fini_cb)(struct objpool_head *head,
> void *context);
> * struct objpool_head - object pooling metadata
> * @obj_size: object size, aligned to sizeof(void *)
> * @nr_objs:total objs (to be pre-allocated with objpool)
> - * @nr_cpus:local copy of nr_cpu_ids
> + * @nr_possible_cpus: cached value of num_possible_cpus()
> * @capacity: max objs can be managed by one objpool_slot
> * @gfp:gfp flags for kmalloc & vmalloc
> * @ref:refcount of objpool
> @@ -85,7 +85,7 @@ typedef int (*objpool_fini_cb)(struct objpool_head *head,
> void *context);
> struct objpool_head {
> int obj_size;
> int nr_objs;
> - int nr_cpus;
> + int nr_possible_cpus;
> int capacity;
> gfp_t gfp;
> refcount_t ref;
> @@ -176,7 +176,7 @@ static inline void *objpool_pop(struct objpool_head *pool)
> raw_local_irq_save(flags);
>
> cpu = raw_smp_processor_id();
> - for (i = 0; i < num_possible_cpus(); i++) {
> + for (i = 0; i < pool->nr_possible_cpus; i++) {
> obj = __objpool_try_get_slot(pool, cpu);
> if (obj)
> break;
> diff --git a/lib/objpool.c b/lib/objpool.c
> index f696308fc026..234f9d0bd081 100644
> --- a/lib/objpool.c
> +++ b/lib/objpool.c
> @@ -50,7 +50,7 @@ objpool_init_percpu_slots(struct objpool_head *pool, int
> nr_objs,
> {
> int i, cpu_count = 0;
>
> - for (i = 0; i < pool->nr_cpus; i++) {
> + for (i = 0; i < nr_cpu_ids; i++) {
>
> struct objpool_slot *slot;
> int nodes, size, rc;
> @@ -60,8 +60,8 @@ objpool_init_percpu_slots(struct objpool_head *pool, int
> nr_objs,
> continue;
>
> /* compute how many objects to be allocated with this slot */
> - nodes = nr_objs / num_possible_cpus();
> - if (cpu_count < (nr_objs % num_possible_cpus()))
> + nodes = nr_objs / pool->nr_possible_cpus;
> + if (cpu_count < (nr_objs % pool->nr_possible_cpus))
> nodes++;
> cpu_count++;
>
> @@ -103,7 +103,7 @@ static void objpool_fini_percpu_slots(struct objpool_head
> *pool)
> if (!pool->cpu_slots)
> return;
>
> - for (i = 0; i < pool->nr_cpus; i++)
> + for (i = 0; i < nr_cpu_ids; i++)
> kvfree(pool->cpu_slots[i]);
> kfree(pool->cpu_slots);
> }
> @@ -130,13 +130,13 @@ int objpool_init(struct objpool_head *pool, int
> nr_objs, int object_size,
>
> /* initialize objpool pool */
> memset(pool, 0, sizeof(struct objpool_head));
> - pool->nr_cpus = nr_cpu_ids;
> + pool->nr_possible_cpus = num_possible_cpus();
> pool->obj_size = object_size;
> pool->capacity = capacity;
> pool->gfp = gfp & ~__GFP_ZERO;
> pool->context = context;
> pool->release = release;
> - slot_size = pool->nr_cpus * sizeof(struct objpool_slot);
> + slot_size = nr_cpu_ids * sizeof(struct objpool_slot);
> pool->cpu_slots = kzalloc(slot_size, pool->gfp);
> if (!pool->cpu_slots)
> return -ENOMEM;
> --
> 2.43.0
>
>
Re: [PATCH v5 2/7] dt-bindings: remoteproc: Add compatibility for TEE support
On Tue, May 21, 2024 at 10:09:56AM +0200, Arnaud Pouliquen wrote:
> The "st,stm32mp1-m4-tee" compatible is utilized in a system configuration
> where the Cortex-M4 firmware is loaded by the Trusted execution Environment
> (TEE).
> For instance, this compatible is used in both the Linux and OP-TEE
> device-tree:
> - In OP-TEE, a node is defined in the device tree with the
> st,stm32mp1-m4-tee to support signed remoteproc firmware.
> Based on DT properties, OP-TEE authenticates, loads, starts, and stops
> the firmware.
I don't see how firmware can be started and stopped. Please rework.
> - On Linux, when the compatibility is set, the Cortex-M resets should not
> be declared in the device tree.
This is a description of "what" is happening and not "why".
More comments to come shortly.
Thanks,
Mathieu
>
> Signed-off-by: Arnaud Pouliquen
> Reviewed-by: Rob Herring
> ---
> .../bindings/remoteproc/st,stm32-rproc.yaml | 51 ---
> 1 file changed, 43 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml
> b/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml
> index 370af61d8f28..36ea54016b76 100644
> --- a/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml
> +++ b/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml
> @@ -16,7 +16,12 @@ maintainers:
>
> properties:
>compatible:
> -const: st,stm32mp1-m4
> +enum:
> + - st,stm32mp1-m4
> + - st,stm32mp1-m4-tee
> +description:
> + Use "st,stm32mp1-m4" for the Cortex-M4 coprocessor management by
> non-secure context
> + Use "st,stm32mp1-m4-tee" for the Cortex-M4 coprocessor management by
> secure context
>
>reg:
> description:
> @@ -142,21 +147,41 @@ properties:
> required:
>- compatible
>- reg
> - - resets
>
> allOf:
>- if:
>properties:
> -reset-names:
> - not:
> -contains:
> - const: hold_boot
> +compatible:
> + contains:
> +const: st,stm32mp1-m4
> then:
> + if:
> +properties:
> + reset-names:
> +not:
> + contains:
> +const: hold_boot
> + then:
> +required:
> + - st,syscfg-holdboot
> + else:
> +properties:
> + st,syscfg-holdboot: false
> +required:
> + - reset-names
>required:
> -- st,syscfg-holdboot
> -else:
> +- resets
> +
> + - if:
> + properties:
> +compatible:
> + contains:
> +const: st,stm32mp1-m4-tee
> +then:
>properties:
> st,syscfg-holdboot: false
> +reset-names: false
> +resets: false
>
> additionalProperties: false
>
> @@ -188,5 +213,15 @@ examples:
>st,syscfg-rsc-tbl = < 0x144 0x>;
>st,syscfg-m4-state = < 0x148 0x>;
> };
> + - |
> +#include
> +m4@1000 {
> + compatible = "st,stm32mp1-m4-tee";
> + reg = <0x1000 0x4>,
> +<0x3000 0x4>,
> +<0x3800 0x1>;
> + st,syscfg-rsc-tbl = < 0x144 0x>;
> + st,syscfg-m4-state = < 0x148 0x>;
> +};
>
> ...
> --
> 2.25.1
>
Re: [PATCH] remoteproc: stm32_rproc: Fix mailbox interrupts queuing
On Tue, May 21, 2024 at 06:23:16PM +0200, Gwenael Treuveur wrote:
> Manage interrupt coming from coprocessor also when state is
> ATTACHED.
>
> Fixes: 35bdafda40cc ("remoteproc: stm32_rproc: Add mutex protection for
> workqueue")
> Signed-off-by: Gwenael Treuveur
> Acked-by: Arnaud Pouliquen
I will pickup this patch but this time only - next time all reviews and tagging
need to happend on the mailing list.
Mathieu
> ---
> drivers/remoteproc/stm32_rproc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/remoteproc/stm32_rproc.c
> b/drivers/remoteproc/stm32_rproc.c
> index 88623df7d0c3..8c7f7950b80e 100644
> --- a/drivers/remoteproc/stm32_rproc.c
> +++ b/drivers/remoteproc/stm32_rproc.c
> @@ -294,7 +294,7 @@ static void stm32_rproc_mb_vq_work(struct work_struct
> *work)
>
> mutex_lock(>lock);
>
> - if (rproc->state != RPROC_RUNNING)
> + if (rproc->state != RPROC_RUNNING && rproc->state != RPROC_ATTACHED)
> goto unlock_mutex;
>
> if (rproc_vq_interrupt(rproc, mb->vq_id) == IRQ_NONE)
>
> base-commit: 4d5ba6ead1dc9fa298d727e92db40cd98564d1ac
> --
> 2.25.1
>
Re: Bug in Kernel 6.8.x, 6.9.x Causing Trace/Panic During Shutdown/Reboot
On Tue, 28 May 2024 07:51:30 +0300
Ilkka Naulapää wrote:
> yeah, the cache_from_obj tracing bug (without panic) has been
> displayed quite some time now - maybe even since 6.7.x or so. I could
> try checking a few versions back for this and try bisecting it if I
> can find when this started.
>
OK, so I don't think the commit your last bisect hit is the cause of
the bug. It added a delay (via RCU) and is causing the real bug to blow
up more.
Can you add this patch to v6.9.2 and hopefully it crashes in a better
location that we can find where the mixup happened.
You may need to add the other commit (too if this doesn't trigger.
Thanks,
-- Steve
diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
index 417c840e6403..7af3f696696d 100644
--- a/fs/tracefs/inode.c
+++ b/fs/tracefs/inode.c
@@ -50,6 +50,7 @@ static struct inode *tracefs_alloc_inode(struct super_block
*sb)
list_add_rcu(>list, _inodes);
spin_unlock_irqrestore(_inode_lock, flags);
+ ti->magic = 20240823;
return >vfs_inode;
}
@@ -66,6 +67,7 @@ static void tracefs_free_inode(struct inode *inode)
struct tracefs_inode *ti = get_tracefs(inode);
unsigned long flags;
+ BUG_ON(ti->magic != 20240823);
spin_lock_irqsave(_inode_lock, flags);
list_del_rcu(>list);
spin_unlock_irqrestore(_inode_lock, flags);
@@ -271,16 +273,6 @@ static const struct inode_operations
tracefs_file_inode_operations = {
.setattr= tracefs_setattr,
};
-struct inode *tracefs_get_inode(struct super_block *sb)
-{
- struct inode *inode = new_inode(sb);
- if (inode) {
- inode->i_ino = get_next_ino();
- simple_inode_init_ts(inode);
- }
- return inode;
-}
-
struct tracefs_mount_opts {
kuid_t uid;
kgid_t gid;
@@ -448,6 +440,17 @@ static const struct super_operations
tracefs_super_operations = {
.show_options = tracefs_show_options,
};
+struct inode *tracefs_get_inode(struct super_block *sb)
+{
+ struct inode *inode = new_inode(sb);
+ BUG_ON(sb->s_op != _super_operations);
+ if (inode) {
+ inode->i_ino = get_next_ino();
+ simple_inode_init_ts(inode);
+ }
+ return inode;
+}
+
/*
* It would be cleaner if eventfs had its own dentry ops.
*
diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h
index f704d8348357..dda7d2708e30 100644
--- a/fs/tracefs/internal.h
+++ b/fs/tracefs/internal.h
@@ -16,6 +16,7 @@ struct tracefs_inode {
};
/* The below gets initialized with memset_after(ti, 0, vfs_inode) */
struct list_headlist;
+ unsigned long magic;
unsigned long flags;
void*private;
};
Re: (subset) [PATCH v2 0/2] Allow gpio-hog nodes in qcom,pmic-gpio bindings (& dt fixup)
On Tue, 09 Apr 2024 20:36:35 +0200, Luca Weiss wrote: > Resolve the dt validation failure on Nexus 5. > > Applied, thanks! [2/2] ARM: dts: qcom: msm8974-hammerhead: Update gpio hog node name commit: 92b9ce5b11d7ba281f5bf0029185d5c891b29344 Best regards, -- Bjorn Andersson
Re: (subset) [PATCH 0/2] Fix msm8974 apcs syscon compatible
On Mon, 08 Apr 2024 21:32:02 +0200, Luca Weiss wrote: > Finally fix a warning about the apcs-global syscon used on msm8974 that > has been around forever. > > Applied, thanks! [2/2] ARM: dts: qcom: msm8974: Use proper compatible for APCS syscon commit: c133cfc12cd717b72ce534477415446e1c33de47 Best regards, -- Bjorn Andersson
Re: [PATCH 0/3] tracing: Fix some selftest issues
On Mon, 27 May 2024 19:29:07 -0400 Steven Rostedt wrote: > On Sun, 26 May 2024 19:10:57 +0900 > "Masami Hiramatsu (Google)" wrote: > > > Hi, > > > > Here is a series of some fixes/improvements for the test modules and boot > > time selftest of kprobe events. I found a WARNING message with some boot > > time selftest configuration, which came from the combination of embedded > > kprobe generate API tests module and ftrace boot-time selftest. So the main > > problem is that the test module should not be built-in. But I also think > > this WARNING message is useless (because there are warning messages already) > > and the cleanup code is redundant. This series fixes those issues. > > Note, when I enable trace tests as builtin instead of modules, I just > disable the bootup self tests when it detects this. This helps with > doing tests via config options than having to add user space code that > loads modules. > > Could you do something similar? OK, in that case, I would like to move the test cleanup code in module_exit function into the end of module_init function. It looks there is no reason to split those into 2 parts. Thank you, > > -- Steve > > > > > > Thank you, > > > > --- > > > > Masami Hiramatsu (Google) (3): > > tracing: Build event generation tests only as modules > > tracing/kprobe: Remove unneeded WARN_ON_ONCE() in selftests > > tracing/kprobe: Remove cleanup code unrelated to selftest > > -- Masami Hiramatsu (Google)
Re: [PATCH 1/2] objpool: enable inlining objpool_push() and objpool_pop() operations
Hi,
Sorry for late reply.
On Fri, 10 May 2024 10:20:56 +0200
Vlastimil Babka wrote:
> On 5/10/24 9:59 AM, wuqiang.matt wrote:
> > On 2024/5/7 21:55, Vlastimil Babka wrote:
> >>
> >>> + } while (!try_cmpxchg_acquire(>tail, , tail + 1));
> >>> +
> >>> + /* now the tail position is reserved for the given obj */
> >>> + WRITE_ONCE(slot->entries[tail & slot->mask], obj);
> >>> + /* update sequence to make this obj available for pop() */
> >>> + smp_store_release(>last, tail + 1);
> >>> +
> >>> + return 0;
> >>> +}
> >>>
> >>> /**
> >>>* objpool_push() - reclaim the object and return back to objpool
> >>> @@ -134,7 +219,19 @@ void *objpool_pop(struct objpool_head *pool);
> >>>* return: 0 or error code (it fails only when user tries to push
> >>>* the same object multiple times or wrong "objects" into objpool)
> >>>*/
> >>> -int objpool_push(void *obj, struct objpool_head *pool);
> >>> +static inline int objpool_push(void *obj, struct objpool_head *pool)
> >>> +{
> >>> + unsigned long flags;
> >>> + int rc;
> >>> +
> >>> + /* disable local irq to avoid preemption & interruption */
> >>> + raw_local_irq_save(flags);
> >>> + rc = __objpool_try_add_slot(obj, pool, raw_smp_processor_id());
> >>
> >> And IIUC, we could in theory objpool_pop() on one cpu, then later another
> >> cpu might do objpool_push() and cause the latter cpu's pool to go over
> >> capacity? Is there some implicit requirements of objpool users to take care
> >> of having matched cpu for pop and push? Are the current objpool users
> >> obeying this requirement? (I can see the selftests do, not sure about the
> >> actual users).
> >> Or am I missing something? Thanks.
> >
> > The objects are all pre-allocated along with creation of the new objpool
> > and the total number of objects never exceeds the capacity on local node.
>
> Aha, I see, the capacity of entries is enough to hold objects from all nodes
> in the most unfortunate case they all end up freed from a single cpu.
>
> > So objpool_push() would always find an available slot from the ring-array
> > for the given object to insert back. objpool_pop() would try looping all
> > the percpu slots until an object is found or whole objpool is empty.
>
> So it's correct, but seems rather wasteful to have the whole capacity for
> entries replicated on every cpu? It does make objpool_push() simple and
> fast, but as you say, objpool_pop() still has to search potentially all
> non-local percpu slots, with disabled irqs, which is far from ideal.
For the kretprobe/fprobe use-case, it is important to push (return) object
fast. We can reservce enough number of objects when registering but push
operation will happen always on random CPU.
>
> And the "abort if the slot was already full" comment for
> objpool_try_add_slot() seems still misleading? Maybe that was your initial
> idea but changed later?
Ah, it should not happen...
>
> > Currently kretprobe is the only actual usecase of objpool.
Note that fprobe is also using this objpool, but currently I'm working on
integrating fprobe on function-graph tracer[1] which will make fprobe not
using objpool. And also I'm planning to replace kretprobe with the new
fprobe eventually. So if SLUB will use objpool for frontend caching, it
sounds good to me. (Maybe it can speed up the object allocation/free)
> >
> > I'm testing an updated objpool in our HIDS project for critical pathes,
> > which is widely deployed on servers inside my company. The new version
> > eliminates the raw_local_irq_save and raw_local_irq_restore pair of
> > objpool_push and gains up to 5% of performance boost.
>
> Mind Ccing me and linux-mm once you are posting that?
Can you add me too?
Thank you,
>
> Thanks,
> Vlastimil
>
--
Masami Hiramatsu (Google)
Re: [PATCH v3 2/2] x86/sgx: Resolve EREMOVE page vs EAUG page data race
On 5/17/24 04:06, Dmitrii Kuvaiskii wrote:
...
First, why is SGX so special here? How is the SGX problem different
than what the core mm code does?
> --- a/arch/x86/kernel/cpu/sgx/encl.h
> +++ b/arch/x86/kernel/cpu/sgx/encl.h
> @@ -25,6 +25,9 @@
> /* 'desc' bit marking that the page is being reclaimed. */
> #define SGX_ENCL_PAGE_BEING_RECLAIMEDBIT(3)
>
> +/* 'desc' bit marking that the page is being removed. */
> +#define SGX_ENCL_PAGE_BEING_REMOVED BIT(2)
Second, convince me that this _needs_ a new bit. Why can't we just have
a bit that effectively means "return EBUSY if you see this bit when
handling a fault".
> struct sgx_encl_page {
> unsigned long desc;
> unsigned long vm_max_prot_bits:8;
> diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> index 5d390df21440..de59219ae794 100644
> --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> @@ -1142,6 +1142,7 @@ static long sgx_encl_remove_pages(struct sgx_encl *encl,
>* Do not keep encl->lock because of dependency on
>* mmap_lock acquired in sgx_zap_enclave_ptes().
>*/
> + entry->desc |= SGX_ENCL_PAGE_BEING_REMOVED;
This also needs a comment, no matter what.
Re: [PATCH v3 0/2] x86/sgx: Fix two data races in EAUG/EREMOVE flows
On 5/17/24 04:06, Dmitrii Kuvaiskii wrote: > We wrote a trivial stress test to reproduce the hangs observed in > real-world applications. The test stresses #PF-based page allocation and > SGX_IOC_ENCLAVE_REMOVE_PAGES flows in the SGX driver: This seems like something we'd want in the kernel SGX selftests.
Re: (subset) [PATCH v2 0/3] arm64: dts: qcom: msm8996: enable fastrpc and glink-edge
On Thu, 18 Apr 2024 09:44:19 +0300, Dmitry Baryshkov wrote: > Enable the FastRPC and glink-edge nodes on MSM8996 platform. Tested on > APQ8096 Dragonboard820c. > > Applied, thanks! [2/3] arm64: dts: qcom: msm8996: add glink-edge nodes commit: 56ae780a4387d71dd709895acd95112d01f37fb4 [3/3] arm64: dts: msm8996: add fastrpc nodes commit: 1b80b83f893dd69efe3c3bf84cd9f661218ccfc0 Best regards, -- Bjorn Andersson
Re: [PATCH] rv: Update rv_en(dis)able_monitor doc to match kernel-doc
On 5/20/24 07:42, Yang Li wrote: > The patch updates the function documentation comment for > rv_en(dis)able_monitor to adhere to the kernel-doc specification. > > Signed-off-by: Yang Li Acked-by: Daniel Bristot de Oliveira Thanks -- Daniel
Re: [PATCH] [v5] kallsyms: rework symbol lookup return codes
Hi Arnd,
On Thu, Apr 4, 2024 at 4:52 PM Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> Building with W=1 in some configurations produces a false positive
> warning for kallsyms:
>
> kernel/kallsyms.c: In function '__sprint_symbol.isra':
> kernel/kallsyms.c:503:17: error: 'strcpy' source argument is the same as
> destination [-Werror=restrict]
> 503 | strcpy(buffer, name);
> | ^~~~
>
> This originally showed up while building with -O3, but later started
> happening in other configurations as well, depending on inlining
> decisions. The underlying issue is that the local 'name' variable is
> always initialized to the be the same as 'buffer' in the called functions
> that fill the buffer, which gcc notices while inlining, though it could
> see that the address check always skips the copy.
>
> The calling conventions here are rather unusual, as all of the internal
> lookup functions (bpf_address_lookup, ftrace_mod_address_lookup,
> ftrace_func_address_lookup, module_address_lookup and
> kallsyms_lookup_buildid) already use the provided buffer and either return
> the address of that buffer to indicate success, or NULL for failure,
> but the callers are written to also expect an arbitrary other buffer
> to be returned.
>
> Rework the calling conventions to return the length of the filled buffer
> instead of its address, which is simpler and easier to follow as well
> as avoiding the warning. Leave only the kallsyms_lookup() calling conventions
> unchanged, since that is called from 16 different functions and
> adapting this would be a much bigger change.
>
> Link: https://lore.kernel.org/all/20200107214042.855757-1-a...@arndb.de/
> Link: https://lore.kernel.org/lkml/20240326130647.7bfb1...@gandalf.local.home/
> Reviewed-by: Luis Chamberlain
> Acked-by: Steven Rostedt (Google)
> Signed-off-by: Arnd Bergmann
> ---
> v5: fix ftrace_mod_address_lookup return value,
> rebased on top of 2e114248e086 ("bpf: Replace deprecated strncpy with
> strscpy")
> v4: fix string length
> v3: use strscpy() instead of strlcpy()
> v2: complete rewrite after the first patch was rejected (in 2020). This
> is now one of only two warnings that are in the way of enabling
> -Wextra/-Wrestrict by default.
Aha, commit 06bb7fc0feee32d9 ("kbuild: turn on -Wrestrict by default")
still made v6.10-rc1, without this one...
> Signed-off-by: Arnd Bergmann
Thanks, this fixes
kernel/kallsyms.c: In function ‘__sprint_symbol.constprop’:
kernel/kallsyms.c:492:17: warning: ‘strcpy’ source argument is the
same as destination [-Werror=restrict]
492 | strcpy(buffer, name);
| ^~~~
I am seeing with shmobile_defconfig and gcc version 11.4.0 (Ubuntu
11.4.0-1ubuntu1~22.04).
Tested-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [linus:master] [mm] d99e3140a4: BUG:KCSAN:data-race_in_folio_remove_rmap_ptes/print_report
On 2024/5/28 15:43, David Hildenbrand wrote:
> Am 28.05.24 um 09:11 schrieb kernel test robot:
>>
>>
>> Hello,
>>
>> kernel test robot noticed
>> "BUG:KCSAN:data-race_in_folio_remove_rmap_ptes/print_report" on:
>>
>> commit: d99e3140a4d33e26066183ff727d8f02f56bec64 ("mm: turn
>> folio_test_hugetlb into a PageType")
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
>>
>> [test failed on linus/master c760b3725e52403dc1b28644fb09c47a83cacea6]
>> [test failed on linux-next/master 3689b0ef08b70e4e03b82ebd37730a03a672853a]
>>
>> in testcase: trinity
>> version: trinity-i386-abe9de86-1_20230429
>> with following parameters:
>>
>> runtime: 300s
>> group: group-04
>> nr_groups: 5
>>
>>
>>
>> compiler: gcc-13
>> test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
>>
>> (please refer to attached dmesg/kmsg for entire log/backtrace)
>>
>>
>> we noticed this issue does not always happen. we also noticed there are
>> different random KCSAN issues for both this commit and its parent. but below
>> 4 only happen on this commit with not small rate and keep clean on parent.
>>
>
> Likely that's just a page_type check racing against concurrent
> mapcount changes.
>
> In __folio_rmap_sanity_checks() we check
> VM_WARN_ON_FOLIO(folio_test_hugetlb(folio), folio);
>
> To make sure we don't get hugetlb folios in the wrong rmap code path. That
> can easily race with concurrent mapcount changes, just like any other
> page_type checks that end up in folio_test_type/page_has_type e.g., from
> PFN walkers.
>
> Load tearing in these functions shouldn't really result in false positives
> (what we care about), but READ_ONCE shouldn't hurt or make a difference.
>
>
> From b03dc9bf27571442d886d8da624a4e4f737433f2 Mon Sep 17 00:00:00 2001
> From: David Hildenbrand
> Date: Tue, 28 May 2024 09:37:20 +0200
> Subject: [PATCH] mm: read page_type using READ_ONCE
>
> KCSAN complains about possible data races: while we check for a
> page_type -- for example for sanity checks -- we might concurrently
> modify the mapcount that overlays page_type.
>
> Let's use READ_ONCE to avoid laod tearing (shouldn't make a difference)
> and to make KCSAN happy.
>
> Note: nothing should really be broken besides wrong KCSAN complaints.
>
> Reported-by: kernel test robot
> Closes: https://lore.kernel.org/oe-lkp/202405281431.c46a3be9-...@intel.com
> Signed-off-by: David Hildenbrand
LGTM. Thanks for fixing.
Reviewed-by: Miaohe Lin
Thanks.
.
Re: [PATCH v2 1/5] dt-bindings: remoteproc: qcom,sa8775p-pas: Document the SA8775p ADSP, CDSP and GPDSP
On 28/05/2024 09:26, Bartosz Golaszewski wrote: > On Mon, May 27, 2024 at 10:56 AM Krzysztof Kozlowski wrote: >> >> On 27/05/2024 10:43, Bartosz Golaszewski wrote: >>> From: Bartosz Golaszewski >>> >>> Document the components used to boot the ADSP, CDSP0, CDSP1, GPDSP0 and >>> GPDSP1 on the SA8775p SoC. >>> >>> Co-developed-by: Tengfei Fan >> >> Missing SoB. >> > > Does it though? The patch never passed through Tengfei's hands, I just > wanted to give him credit for the work modifying the sm8550-pas > bindings. Then what was co-developed by Tengfei? If nothing, then indeed no SoB but also no Co-developed-by. Docs are clear here: every Co-developed-by *must* be followed by SoB. Best regards, Krzysztof
Re: [linus:master] [mm] d99e3140a4: BUG:KCSAN:data-race_in_folio_remove_rmap_ptes/print_report
On Tue, May 28, 2024 at 09:43:39AM +0200, David Hildenbrand wrote: > Likely that's just a page_type check racing against concurrent > mapcount changes. > > In __folio_rmap_sanity_checks() we check > VM_WARN_ON_FOLIO(folio_test_hugetlb(folio), folio); Yeah, and that "collides" with last = atomic_add_negative(-1, >_mapcount) from __folio_remove_rmap. > To make sure we don't get hugetlb folios in the wrong rmap code path. That > can easily race with concurrent mapcount changes, just like any other > page_type checks that end up in folio_test_type/page_has_type e.g., from > PFN walkers. > > Load tearing in these functions shouldn't really result in false positives > (what we care about), but READ_ONCE shouldn't hurt or make a difference. > > > From b03dc9bf27571442d886d8da624a4e4f737433f2 Mon Sep 17 00:00:00 2001 > From: David Hildenbrand > Date: Tue, 28 May 2024 09:37:20 +0200 > Subject: [PATCH] mm: read page_type using READ_ONCE > > KCSAN complains about possible data races: while we check for a > page_type -- for example for sanity checks -- we might concurrently > modify the mapcount that overlays page_type. > > Let's use READ_ONCE to avoid laod tearing (shouldn't make a difference) > and to make KCSAN happy. > > Note: nothing should really be broken besides wrong KCSAN complaints. > > Reported-by: kernel test robot > Closes: https://lore.kernel.org/oe-lkp/202405281431.c46a3be9-...@intel.com > Signed-off-by: David Hildenbrand Reviewed-by: Oscar Salvador Thanks! -- Oscar Salvador SUSE Labs
Re: [linus:master] [mm] d99e3140a4: BUG:KCSAN:data-race_in_folio_remove_rmap_ptes/print_report
Am 28.05.24 um 09:11 schrieb kernel test robot:
Hello,
kernel test robot noticed
"BUG:KCSAN:data-race_in_folio_remove_rmap_ptes/print_report" on:
commit: d99e3140a4d33e26066183ff727d8f02f56bec64 ("mm: turn folio_test_hugetlb into
a PageType")
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
[test failed on linus/master c760b3725e52403dc1b28644fb09c47a83cacea6]
[test failed on linux-next/master 3689b0ef08b70e4e03b82ebd37730a03a672853a]
in testcase: trinity
version: trinity-i386-abe9de86-1_20230429
with following parameters:
runtime: 300s
group: group-04
nr_groups: 5
compiler: gcc-13
test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
(please refer to attached dmesg/kmsg for entire log/backtrace)
we noticed this issue does not always happen. we also noticed there are
different random KCSAN issues for both this commit and its parent. but below
4 only happen on this commit with not small rate and keep clean on parent.
Likely that's just a page_type check racing against concurrent
mapcount changes.
In __folio_rmap_sanity_checks() we check
VM_WARN_ON_FOLIO(folio_test_hugetlb(folio), folio);
To make sure we don't get hugetlb folios in the wrong rmap code path. That
can easily race with concurrent mapcount changes, just like any other
page_type checks that end up in folio_test_type/page_has_type e.g., from
PFN walkers.
Load tearing in these functions shouldn't really result in false positives
(what we care about), but READ_ONCE shouldn't hurt or make a difference.
From b03dc9bf27571442d886d8da624a4e4f737433f2 Mon Sep 17 00:00:00 2001
From: David Hildenbrand
Date: Tue, 28 May 2024 09:37:20 +0200
Subject: [PATCH] mm: read page_type using READ_ONCE
KCSAN complains about possible data races: while we check for a
page_type -- for example for sanity checks -- we might concurrently
modify the mapcount that overlays page_type.
Let's use READ_ONCE to avoid laod tearing (shouldn't make a difference)
and to make KCSAN happy.
Note: nothing should really be broken besides wrong KCSAN complaints.
Reported-by: kernel test robot
Closes: https://lore.kernel.org/oe-lkp/202405281431.c46a3be9-...@intel.com
Signed-off-by: David Hildenbrand
---
include/linux/page-flags.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 104078afe0b1..e46ccbb9aa58 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -955,9 +955,9 @@ PAGEFLAG_FALSE(HasHWPoisoned, has_hwpoisoned)
#define PG_slab0x1000
#define PageType(page, flag) \
- ((page->page_type & (PAGE_TYPE_BASE | flag)) == PAGE_TYPE_BASE)
+ ((READ_ONCE(page->page_type) & (PAGE_TYPE_BASE | flag)) ==
PAGE_TYPE_BASE)
#define folio_test_type(folio, flag) \
- ((folio->page.page_type & (PAGE_TYPE_BASE | flag)) == PAGE_TYPE_BASE)
+ ((READ_ONCE(folio->page.page_type) & (PAGE_TYPE_BASE | flag)) ==
PAGE_TYPE_BASE)
static inline int page_type_has_type(unsigned int page_type)
{
@@ -966,7 +966,7 @@ static inline int page_type_has_type(unsigned int page_type)
static inline int page_has_type(const struct page *page)
{
- return page_type_has_type(page->page_type);
+ return page_type_has_type(READ_ONCE(page->page_type));
}
#define FOLIO_TYPE_OPS(lname, fname) \
--
2.45.1
--
Thanks,
David / dhildenb
Re: [PATCH v2 1/5] dt-bindings: remoteproc: qcom,sa8775p-pas: Document the SA8775p ADSP, CDSP and GPDSP
On Mon, May 27, 2024 at 10:56 AM Krzysztof Kozlowski wrote: > > On 27/05/2024 10:43, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski > > > > Document the components used to boot the ADSP, CDSP0, CDSP1, GPDSP0 and > > GPDSP1 on the SA8775p SoC. > > > > Co-developed-by: Tengfei Fan > > Missing SoB. > Does it though? The patch never passed through Tengfei's hands, I just wanted to give him credit for the work modifying the sm8550-pas bindings. Bart
