Re: [PATCH V2] tty: hvc: hvc_opal: eliminate uses of of_node_put()
On Fri, May 03, 2024 at 04:52:15PM +0300, Lu Dai wrote:
> Make use of the __free() cleanup handler to automatically free nodes
> when they get out of scope.
>
> Remove the need for a 'goto' as an effect.
>
> Signed-off-by: Lu Dai
> ---
> Changes since v1:
> Move the assignment of 'opal' to its declaration
> Seperate the declaration of 'np'
>
> drivers/tty/hvc/hvc_opal.c | 13 +
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c
> index 095c33ad10f8..c17e8343ea60 100644
> --- a/drivers/tty/hvc/hvc_opal.c
> +++ b/drivers/tty/hvc/hvc_opal.c
> @@ -327,19 +327,18 @@ static void udbg_init_opal_common(void)
>
> void __init hvc_opal_init_early(void)
> {
> - struct device_node *stdout_node = of_node_get(of_stdout);
> + struct device_node *stdout_node __free(device_node) =
> of_node_get(of_stdout);
> const __be32 *termno;
> const struct hv_ops *ops;
> u32 index;
>
> /* If the console wasn't in /chosen, try /ibm,opal */
> if (!stdout_node) {
> - struct device_node *opal, *np;
> -
> /* Current OPAL takeover doesn't provide the stdout
>* path, so we hard wire it
>*/
> - opal = of_find_node_by_path("/ibm,opal/consoles");
> + struct device_node *opal __free(device_node) =
> + of_find_node_by_path("/ibm,opal/consoles");
> if (opal) {
No blank line?
> pr_devel("hvc_opal: Found consoles in new location\n");
> } else {
> @@ -350,13 +349,13 @@ void __init hvc_opal_init_early(void)
> }
> if (!opal)
> return;
> + struct device_node *np;
> for_each_child_of_node(opal, np) {
Ick, no, don't do that please. Take some time and become more familiar
with kernel coding style and issues, perhaps work in drivers/staging/
first, before attempting to do stuff like this that is not correct.
thanks,
greg k-h
Re: [PATCH] tty: hvc: hvc_opal: eliminate uses of of_node_put()
On Fri, May 03, 2024 at 02:43:30PM +0300, Lu Dai wrote:
> Make use of the __free() cleanup handler to automatically free nodes
> when they get out of scope.
>
> Removes the need for a 'goto' as an effect.
>
> Signed-off-by: Lu Dai
> ---
> drivers/tty/hvc/hvc_opal.c | 9 +++--
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c
> index 095c33ad10f8..67e90fa993a3 100644
> --- a/drivers/tty/hvc/hvc_opal.c
> +++ b/drivers/tty/hvc/hvc_opal.c
> @@ -327,14 +327,14 @@ static void udbg_init_opal_common(void)
>
> void __init hvc_opal_init_early(void)
> {
> - struct device_node *stdout_node = of_node_get(of_stdout);
> + struct device_node *stdout_node __free(device_node) =
> of_node_get(of_stdout);
> const __be32 *termno;
> const struct hv_ops *ops;
> u32 index;
>
> /* If the console wasn't in /chosen, try /ibm,opal */
> if (!stdout_node) {
> - struct device_node *opal, *np;
> + struct device_node *opal __free(device_node), *np;
*np needs to be on a separate line, right?
thanks,
greg k-h
Re: [PATCH v2] tty: hvc: wakeup hvc console immediately when needed
On Mon, Apr 15, 2024 at 03:26:17PM +0800, li.ha...@zte.com.cn wrote: > From: Li Hao > > Cancel the do_wakeup flag in hvc_struct, and change it to immediately > wake up tty when hp->n_outbuf is 0 in hvc_push(). > > When we receive a key input character, the interrupt handling function > hvc_handle_interrupt() will be executed, and the echo thread > flush_to_ldisc() will be added to the queue. > > If the user is currently using tcsetattr(), a hang may occur. tcsetattr() > enters kernel and waits for hp->n_outbuf to become 0 via > tty_wait_until_sent(). If the echo thread finishes executing before > reaching tty_wait_until_sent (for example, put_chars() takes too long), > it will cause while meeting the wakeup condition (hp->do_wakeup = 1), > tty_wait_until_sent() cannot be woken up (missed the tty_wakeup() of > this round's tty_poll). Unless the next key input character comes, > hvc_poll will be executed, and tty_wakeup() will be performed through > the do_wakeup flag. > > v1->v2: > Some fixes according to: > https://lore.kernel.org/all/75dff5cd-7b0e-4039-9157-8bf10cf7b...@kernel.org/ > use tty_port_tty_wakeup() instead of tty_wakeup() to wake up tty As per the documentation, the v1... stuff needs to go below the --- line so git will strip it away. Please fix up and send a v3. thanks, greg k-h
Re: linux-next: duplicate patch in the driver-core.current tree
On Fri, Apr 12, 2024 at 02:36:25PM +1000, Michael Ellerman wrote:
> Stephen Rothwell writes:
> > Hi all,
> >
> > The following commit is also in the powerpc-fixes tree as a different
> > commit (but the same patch):
> >
> > 156539fd6501 ("Documentation: embargoed-hardware-issues.rst: Add myself
> > for Power")
> >
> > This is commit
> >
> > 36627111b568 ("Documentation: embargoed-hardware-issues.rst: Add myself
> > for Power")
> >
> > in the powerpc-fixes tree.
>
> I can drop my version easily enough.
Either is fine, or both, it doesn't really matter :)
thanks,
greg k-h
Re: [PATCH] tty: hvc: wakeup hvc console immediately when needed
On Fri, Apr 12, 2024 at 11:38:48AM +0800, li.ha...@zte.com.cn wrote:
> From: Li Hao
>
> Cancel the do_wakeup flag in hvc_struct, and change it to immediately
> wake up tty when hp->n_outbuf is 0 in hvc_push().
>
> When we receive a key input character, the interrupt handling function
> hvc_handle_interrupt() will be executed, and the echo thread
> flush_to_ldisc() will be added to the queue.
>
> If the user is currently using tcsetattr(), a hang may occur. tcsetattr()
> enters kernel and waits for hp->n_outbuf to become 0 via
> tty_wait_until_sent(). If the echo thread finishes executing before
> reaching tty_wait_until_sent (for example, put_chars() takes too long),
> it will cause while meeting the wakeup condition (hp->do_wakeup = 1),
> tty_wait_until_sent() cannot be woken up (missed the tty_wakeup() of
> this round's tty_poll). Unless the next key input character comes,
> hvc_poll will be executed, and tty_wakeup() will be performed through
> the do_wakeup flag.
>
> Signed-off-by: Li Hao
> ---
> drivers/tty/hvc/hvc_console.c | 12 +---
> drivers/tty/hvc/hvc_console.h | 1 -
> 2 files changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
> index cd1f657f7..2fa90d938 100644
> --- a/drivers/tty/hvc/hvc_console.c
> +++ b/drivers/tty/hvc/hvc_console.c
> @@ -476,11 +476,13 @@ static void hvc_hangup(struct tty_struct *tty)
> static int hvc_push(struct hvc_struct *hp)
> {
> int n;
> + struct tty_struct *tty;
>
> n = hp->ops->put_chars(hp->vtermno, hp->outbuf, hp->n_outbuf);
> + tty = tty_port_tty_get(>port);
> if (n <= 0) {
> if (n == 0 || n == -EAGAIN) {
> - hp->do_wakeup = 1;
> + tty_wakeup(tty);
> return 0;
> }
> /* throw away output on error; this happens when
> @@ -491,7 +493,7 @@ static int hvc_push(struct hvc_struct *hp)
> if (hp->n_outbuf > 0)
> memmove(hp->outbuf, hp->outbuf + n, hp->n_outbuf);
> else
> - hp->do_wakeup = 1;
> + tty_wakeup(tty);
>
> return n;
> }
> @@ -739,11 +741,7 @@ static int __hvc_poll(struct hvc_struct *hp, bool
> may_sleep)
> poll_mask |= HVC_POLL_READ;
>
> out:
> - /* Wakeup write queue if necessary */
> - if (hp->do_wakeup) {
> - hp->do_wakeup = 0;
> - tty_wakeup(tty);
> - }
> + /* Wakeup in hvc_push */
> bail:
> spin_unlock_irqrestore(>lock, flags);
>
> diff --git a/drivers/tty/hvc/hvc_console.h b/drivers/tty/hvc/hvc_console.h
> index cf4c1af08..6622f71ba 100644
> --- a/drivers/tty/hvc/hvc_console.h
> +++ b/drivers/tty/hvc/hvc_console.h
> @@ -36,7 +36,6 @@ struct hvc_struct {
> struct tty_port port;
> spinlock_t lock;
> int index;
> - int do_wakeup;
> int outbuf_size;
> int n_outbuf;
> uint32_t vtermno;
> --
> 2.25.1
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.
You are receiving this message because of the following common error(s)
as indicated below:
- This looks like a new version of a previously submitted patch, but you
did not list below the --- line any changes from the previous version.
Please read the section entitled "The canonical patch format" in the
kernel file, Documentation/process/submitting-patches.rst for what
needs to be done here to properly describe this.
If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.
thanks,
greg k-h's patch email bot
Re: [PATCH] tty: hvc: wakeup hvc console immediately when needed
On Thu, Apr 11, 2024 at 09:50:17PM +0800, li.ha...@zte.com.cn wrote: > From: Li Hao > > Cancel the do_wakeup flag in hvc_struct, and change it to immediately > wake up tty when hp->n_outbuf is 0 in hvc_push(). > > When we receive a key input character, the interrupt handling function > hvc_handle_interrupt() will be executed, and the echo thread > flush_to_ldisc() will be added to the queue. > > If the user is currently using tcsetattr(), a hang may occur. tcsetattr() > enters kernel and waits for hp->n_outbuf to become 0 via > tty_wait_until_sent(). If the echo thread finishes executing before > reaching tty_wait_until_sent (for example, put_chars() takes too long), > it will cause while meeting the wakeup condition (hp->do_wakeup = 1), > tty_wait_until_sent() cannot be woken up (missed the tty_wakeup() of > this rounds tty_poll). Unless the next key input character comes, > hvc_poll will be executed, and tty_wakeup() will be performed through > the do_wakeup flag. > > Signed-off-by: Li Hao Did checkpatch say this was ok?
Re: FAILED: Patch "powerpc: xor_vmx: Add '-mhard-float' to CFLAGS" failed to apply to 5.10-stable tree
On Wed, Mar 27, 2024 at 08:16:13AM -0700, Nathan Chancellor wrote: > On Wed, Mar 27, 2024 at 08:20:07AM -0400, Sasha Levin wrote: > > The patch below does not apply to the 5.10-stable tree. > > If someone wants it applied there, or to any other stable or longterm > > tree, then please email the backport, including the original git commit > > id to . > ... > > -- original commit in Linus's tree -- > > > > From 35f20786c481d5ced9283ff42de5c69b65e5ed13 Mon Sep 17 00:00:00 2001 > > From: Nathan Chancellor > > Date: Sat, 27 Jan 2024 11:07:43 -0700 > > Subject: [PATCH] powerpc: xor_vmx: Add '-mhard-float' to CFLAGS > > I have attached a backport that will work for 5.15 and earlier. I think > you worked around this conflict in 5.15 by taking 04e85bbf71c9 but I am > not sure that is a smart idea. I think it might just be better to drop > that dependency and apply this version in 5.15. I'll go drop it and take this version, thanks! greg k-h
Re: [PATCH 4/9] USB: Convert from tasklet to BH workqueue
On Wed, Mar 27, 2024 at 04:03:09PM +, Allen Pais wrote:
> The only generic interface to execute asynchronously in the BH context is
> tasklet; however, it's marked deprecated and has some design flaws. To
> replace tasklets, BH workqueue support was recently added. A BH workqueue
> behaves similarly to regular workqueues except that the queued work items
> are executed in the BH context.
>
> This patch converts drivers/infiniband/* from tasklet to BH workqueue.
No it does not, I think your changelog is wrong :(
>
> Based on the work done by Tejun Heo
> Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-6.10
>
> Signed-off-by: Allen Pais
> ---
> drivers/usb/atm/usbatm.c| 55 +++--
> drivers/usb/atm/usbatm.h| 3 +-
> drivers/usb/core/hcd.c | 22 ++--
> drivers/usb/gadget/udc/fsl_qe_udc.c | 21 +--
> drivers/usb/gadget/udc/fsl_qe_udc.h | 4 +--
> drivers/usb/host/ehci-sched.c | 2 +-
> drivers/usb/host/fhci-hcd.c | 3 +-
> drivers/usb/host/fhci-sched.c | 10 +++---
> drivers/usb/host/fhci.h | 5 +--
> drivers/usb/host/xhci-dbgcap.h | 3 +-
> drivers/usb/host/xhci-dbgtty.c | 15
> include/linux/usb/cdc_ncm.h | 2 +-
> include/linux/usb/usbnet.h | 2 +-
> 13 files changed, 76 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/usb/atm/usbatm.c b/drivers/usb/atm/usbatm.c
> index 2da6615fbb6f..74849f24e52e 100644
> --- a/drivers/usb/atm/usbatm.c
> +++ b/drivers/usb/atm/usbatm.c
> @@ -17,7 +17,7 @@
> * - Removed the limit on the number of devices
> * - Module now autoloads on device plugin
> * - Merged relevant parts of sarlib
> - * - Replaced the kernel thread with a tasklet
> + * - Replaced the kernel thread with a work
a "work"?
> * - New packet transmission code
> * - Changed proc file contents
> * - Fixed all known SMP races
> @@ -68,6 +68,7 @@
> #include
> #include
> #include
> +#include
>
> #ifdef VERBOSE_DEBUG
> static int usbatm_print_packet(struct usbatm_data *instance, const unsigned
> char *data, int len);
> @@ -249,7 +250,7 @@ static void usbatm_complete(struct urb *urb)
> /* vdbg("%s: urb 0x%p, status %d, actual_length %d",
>__func__, urb, status, urb->actual_length); */
>
> - /* Can be invoked from task context, protect against interrupts */
> + /* Can be invoked from work context, protect against interrupts */
"workqueue"? This too seems wrong.
Same for other comment changes in this patch.
thanks,
greg k-h
Re: [PATCH 4.19] powerpc: Use always instead of always-y in for crtsavres.o
On Fri, Jan 26, 2024 at 10:36:31AM -0700, Nathan Chancellor wrote:
> This commit is for linux-4.19.y only, it has no direct upstream
> equivalent.
>
> Prior to commit 5f2fb52fac15 ("kbuild: rename hostprogs-y/always to
> hostprogs/always-y"), always-y did not exist, making the backport of
> mainline commit 1b1e38002648 ("powerpc: add crtsavres.o to always-y
> instead of extra-y") to linux-4.19.y as commit b7b85ec5ec15 ("powerpc:
> add crtsavres.o to always-y instead of extra-y") incorrect, breaking the
> build with linkers that need crtsavres.o:
>
> ld.lld: error: cannot open arch/powerpc/lib/crtsavres.o: No such file or
> directory
>
> Backporting the aforementioned kbuild commit is not suitable for stable
> due to its size and number of conflicts, so transform the always-y usage
> to an equivalent form using always, which resolves the build issues.
>
> Fixes: b7b85ec5ec15 ("powerpc: add crtsavres.o to always-y instead of
> extra-y")
> Signed-off-by: Nathan Chancellor
> ---
Both now queued up, thanks!
greg k-h
Re: [PATCH] NUMA: Early use of cpu_to_node() returns 0 instead of the correct node id
On Fri, Jan 19, 2024 at 11:32:27AM +0800, Huang Shijie wrote:
> During the kernel booting, the generic cpu_to_node() is called too early in
> arm64, powerpc and riscv when CONFIG_NUMA is enabled.
>
> There are at least four places in the common code where
> the generic cpu_to_node() is called before it is initialized:
> 1.) early_trace_init() in kernel/trace/trace.c
> 2.) sched_init() in kernel/sched/core.c
> 3.) init_sched_fair_class()in kernel/sched/fair.c
> 4.) workqueue_init_early() in kernel/workqueue.c
>
> In order to fix the bug, the patch changes generic cpu_to_node to
> function pointer, and export it for kernel modules.
> Introduce smp_prepare_boot_cpu_start() to wrap the original
> smp_prepare_boot_cpu(), and set cpu_to_node with early_cpu_to_node.
> Introduce smp_prepare_cpus_done() to wrap the original smp_prepare_cpus(),
> and set the cpu_to_node to formal _cpu_to_node().
>
> Signed-off-by: Huang Shijie
> ---
> drivers/base/arch_numa.c | 11 +++
> include/linux/topology.h | 6 ++
> init/main.c | 29 +++--
> 3 files changed, 40 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/arch_numa.c b/drivers/base/arch_numa.c
> index 5b59d133b6af..867a477fa975 100644
> --- a/drivers/base/arch_numa.c
> +++ b/drivers/base/arch_numa.c
> @@ -61,6 +61,17 @@ EXPORT_SYMBOL(cpumask_of_node);
>
> #endif
>
> +#ifdef CONFIG_USE_PERCPU_NUMA_NODE_ID
> +#ifndef cpu_to_node
> +int _cpu_to_node(int cpu)
> +{
> + return per_cpu(numa_node, cpu);
> +}
> +int (*cpu_to_node)(int cpu);
> +EXPORT_SYMBOL(cpu_to_node);
> +#endif
> +#endif
> +
> static void numa_update_cpu(unsigned int cpu, bool remove)
> {
> int nid = cpu_to_node(cpu);
> diff --git a/include/linux/topology.h b/include/linux/topology.h
> index 52f5850730b3..e7ce2bae11dd 100644
> --- a/include/linux/topology.h
> +++ b/include/linux/topology.h
> @@ -91,10 +91,8 @@ static inline int numa_node_id(void)
> #endif
>
> #ifndef cpu_to_node
> -static inline int cpu_to_node(int cpu)
> -{
> - return per_cpu(numa_node, cpu);
> -}
> +extern int (*cpu_to_node)(int cpu);
> +extern int _cpu_to_node(int cpu);
> #endif
>
> #ifndef set_numa_node
> diff --git a/init/main.c b/init/main.c
> index e24b0780fdff..b142e9c51161 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -870,6 +870,18 @@ static void __init print_unknown_bootoptions(void)
> memblock_free(unknown_options, len);
> }
>
> +static void __init smp_prepare_boot_cpu_start(void)
> +{
> + smp_prepare_boot_cpu(); /* arch-specific boot-cpu hooks */
> +
> +#ifdef CONFIG_USE_PERCPU_NUMA_NODE_ID
> +#ifndef cpu_to_node
> + /* The early_cpu_to_node should be ready now. */
> + cpu_to_node = early_cpu_to_node;
> +#endif
> +#endif
> +}
> +
> asmlinkage __visible __init __no_sanitize_address __noreturn
> __no_stack_protector
> void start_kernel(void)
> {
> @@ -899,7 +911,7 @@ void start_kernel(void)
> setup_command_line(command_line);
> setup_nr_cpu_ids();
> setup_per_cpu_areas();
> - smp_prepare_boot_cpu(); /* arch-specific boot-cpu hooks */
> + smp_prepare_boot_cpu_start();
> boot_cpu_hotplug_init();
>
> pr_notice("Kernel command line: %s\n", saved_command_line);
> @@ -1519,6 +1531,19 @@ void __init console_on_rootfs(void)
> fput(file);
> }
>
> +static void __init smp_prepare_cpus_done(unsigned int setup_max_cpus)
> +{
> + /* Different ARCHs may override smp_prepare_cpus() */
> + smp_prepare_cpus(setup_max_cpus);
> +
> +#ifdef CONFIG_USE_PERCPU_NUMA_NODE_ID
> +#ifndef cpu_to_node
> + /* Change to the formal function. */
> + cpu_to_node = _cpu_to_node;
> +#endif
> +#endif
> +}
> +
> static noinline void __init kernel_init_freeable(void)
> {
> /* Now the scheduler is fully set up and can do blocking allocations */
> @@ -1531,7 +1556,7 @@ static noinline void __init kernel_init_freeable(void)
>
> cad_pid = get_pid(task_pid(current));
>
> - smp_prepare_cpus(setup_max_cpus);
> + smp_prepare_cpus_done(setup_max_cpus);
>
> workqueue_init();
>
> --
> 2.40.1
>
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.
You are receiving this message because of the following common error(s)
as indicated below:
- This looks like a new version of a previously submitted patch, but you
did not list below the --- line any changes from the previous version.
Please read the section entitled "The canonical patch format" in the
kernel file, Documentation/process/submitting-patches.rst
Re: [PATCH] init: refactor the generic cpu_to_node for NUMA
On Thu, Jan 18, 2024 at 11:14:12AM +0800, Huang Shijie wrote: > (0) We list the ARCHs which support the NUMA: >arm64, loongarch, powerpc, riscv, >sparc, mips, s390, x86, I do not understand this format, what are you saying here? Have you read the kernel documentation for how to write changelog texts? It doesn't say "list a bunch of things", it's a bit more descriptive. > > (1) Some ARCHs in (0) override the generic cpu_to_node(), such as: >sparc, mips, s390, x86. > > Since these ARCHs have their own cpu_to_node(), we do not care > about them. > > (2) The ARCHs enable NUMA and use the generic cpu_to_node. > From (0) and (1), we can know that four ARCHs support NUMA and > use the generic cpu_to_node: > arm64, loongarch, powerpc, riscv, > > The generic cpu_to_node depends on percpu "numa_node". > > (2.1) The loongarch sets "numa_node" in: > start_kernel --> smp_prepare_boot_cpu() > > (2.2) The arm64, powerpc, riscv set "numa_node" in: > start_kernel --> arch_call_rest_init() --> rest_init() > --> kernel_init() --> kernel_init_freeable() >--> smp_prepare_cpus() > > (2.3) The first place calling the cpu_to_node() is early_trace_init(): > start_kernel --> early_trace_init()--> __ring_buffer_alloc() > --> rb_allocate_cpu_buffer() > > (2.4) So it safe for loongarch. But for arm64, powerpc and riscv, > there are at least four places in the common code where > the cpu_to_node() is called before it is initialized: > a.) early_trace_init() in kernel/trace/trace.c > b.) sched_init() in kernel/sched/core.c > c.) init_sched_fair_class()in kernel/sched/fair.c > d.) workqueue_init_early() in kernel/workqueue.c > > (3) In order to fix the issue, the patch refactors the generic cpu_to_node: > (3.1) change cpu_to_node to function pointer, > and export it for kernel modules. > > (3.2) introduce _cpu_to_node() which is the original cpu_to_node(). > > (3.3) introduce smp_prepare_boot_cpu_start() to wrap the original > smp_prepare_boot_cpu(), and set cpu_to_node with > early_cpu_to_node which works fine for arm64, powerpc, > riscv and loongarch. > > (3.4) introduce smp_prepare_cpus_done() to wrap the original > smp_prepare_cpus(). > The "numa_node" is ready after smp_prepare_cpus(), > then set cpu_to_node with _cpu_to_node(). When you start listing different things in a changelog, that's a hint to the reviewer to say "please break this up" as patches need to do only one thing at a time. As I can't follow the above text at all, that's all the review comments I'm able to give here, sorry. But as-is, this isn't acceptable :( thanks, greg k-h
Re: linux-next: duplicate patches in the char-misc tree
On Wed, Nov 29, 2023 at 12:24:05PM +1100, Stephen Rothwell wrote:
> Hi all,
>
> The following commits are also in the powerpc tree as different commits
> (but the same patches):
>
> bc1183a63057 ("misc: ocxl: main: Remove unnecessary ‘0’ values from rc")
> 29eb0dc7bd1e ("misc: ocxl: link: Remove unnecessary (void*) conversions")
> 0e425d703c30 ("misc: ocxl: afu_irq: Remove unnecessary (void*) conversions")
> 62df29a542f9 ("misc: ocxl: context: Remove unnecessary (void*) conversions")
>
> These are commits
>
> 29685ea5754f ("misc: ocxl: main: Remove unnecessary ‘0’ values from rc")
> 220f3ced8e42 ("misc: ocxl: link: Remove unnecessary (void*) conversions")
> 84ba5d3675e2 ("misc: ocxl: afu_irq: Remove unnecessary (void*) conversions")
> 82d30723d58f ("misc: ocxl: context: Remove unnecessary (void*) conversions")
>
> in the powerpc tree.
Thanks, that should be fine, I didn't realize these ended up in the
powerpc tree already.
greg k-h
Re: [PATCH v2 0/5] MODVERSIONS + RUST Redux
On Tue, Nov 28, 2023 at 08:05:26AM +, Greg KH wrote:
> On Mon, Nov 27, 2023 at 11:27:07AM -0800, Matthew Maurer wrote:
> > > > >
> > > > > > With regards to future directions that likely won't work for
> > > > > > loosening it:
> > > > > > Unfortunately, the .rmeta format itself is not stable, so I
> > > > > > wouldn't want to
> > > > > > teach genksyms to open it up and split out the pieces for specific
> > > > > > functions.
> > > > > > Extending genksyms to parse Rust would also not solve the situation
> > > > > > -
> > > > > > layouts are allowed to differ across compiler versions or even (in
> > > > > > rare
> > > > > > cases) seemingly unrelated code changes.
> > > > >
> > > > > What do you mean by "layout" here? Yes, the crcs can be different
> > > > > across compiler versions and seemingly unrelated code changes
> > > > > (genksyms
> > > > > is VERY fragile) but that's ok, that's not what you are checking here.
> > > > > You want to know if the rust function signature changes or not from
> > > > > the
> > > > > last time you built the code, with the same compiler and options,
> > > > > that's
> > > > > all you are verifying.
> > What I mean by layout here is that if you write in Rust:
> > struct Foo {
> > x: i32,
> > y: i32,
> > }
> > it is not guaranteed to have the same layout across different compilations,
> > even
> > within the same compiler. See
> > https://doc.rust-lang.org/reference/type-layout.html#the-rust-representation
>
> Then you are going to have big problems, sorry.
>
> > Specifically, the compiler is allowed to arbitrarily insert padding,
> > reorder fields, etc.
> > on the same code as long as the overall alignment of the struct and
> > individual
> > alignment of the fields remains correct and non-overlapping.
> >
> > This means the compiler is *explicitly* allowed to, for example, permute x
> > and y
> > as an optimization. In the above example this is unlikely, but if you
> > instead consider
> > struct Bar {
> > x: i8,
> > y: i64,
> > z: i8,
> > }
> > It's easy to see why the compiler might decide to structure this as
> > y,x,z to reduce the
> > size of the struct. Those optimization decisions may be affected by
> > any other part of
> > the code, PGO, etc.
>
> Then you all need to figure out some way to determine how the compiler
> layed out the structure after it compiled/optimized it and be able to
> compare it to previous builds (or just generate a crc based on the
> layout it chose.)
>
> > > > > > Future directions that might work for loosening it:
> > > > > > * Generating crcs from debuginfo + compiler + flags
> > > > > > * Adding a feature to the rust compiler to dump this information.
> > > > > > This
> > > > > > is likely to
> > > > > > get pushback because Rust's current stance is that there is no
> > > > > > ability to load
> > > > > > object code built against a different library.
> > > > >
> > > > > Why not parse the function signature like we do for C?
> > Because the function signature is insufficient to check the ABI, see above.
> > > > >
> > > > > > Would setting up Rust symbols so that they have a crc built out of
> > > > > > .rmeta be
> > > > > > sufficient for you to consider this useful? If not, can you help me
> > > > > > understand
> > > > > > what level of precision would be required?
> > > > >
> > > > > What exactly does .rmeta have to do with the function signature?
> > > > > That's
> > > > > all you care about here.
> > The .rmeta file contains the decisions the compiler made about layout
> > in the crate
> > you're interfacing with. For example, the choice to encode Bar
> > with a yxz field order would be written into the .rmeta file.
>
> Ok, then yes, can you parse the .rmeta file to get that information?
>
> > > > rmeta is generated per crate.
> > > >
> > > > CRC is computed per symbol.
> > > >
> > > > They have different granularity.
> > > > It is weird to refuse a module for incompatibility
>
Re: [PATCH v2 0/5] MODVERSIONS + RUST Redux
On Mon, Nov 27, 2023 at 11:27:07AM -0800, Matthew Maurer wrote:
> > > >
> > > > > With regards to future directions that likely won't work for
> > > > > loosening it:
> > > > > Unfortunately, the .rmeta format itself is not stable, so I wouldn't
> > > > > want to
> > > > > teach genksyms to open it up and split out the pieces for specific
> > > > > functions.
> > > > > Extending genksyms to parse Rust would also not solve the situation -
> > > > > layouts are allowed to differ across compiler versions or even (in
> > > > > rare
> > > > > cases) seemingly unrelated code changes.
> > > >
> > > > What do you mean by "layout" here? Yes, the crcs can be different
> > > > across compiler versions and seemingly unrelated code changes (genksyms
> > > > is VERY fragile) but that's ok, that's not what you are checking here.
> > > > You want to know if the rust function signature changes or not from the
> > > > last time you built the code, with the same compiler and options, that's
> > > > all you are verifying.
> What I mean by layout here is that if you write in Rust:
> struct Foo {
> x: i32,
> y: i32,
> }
> it is not guaranteed to have the same layout across different compilations,
> even
> within the same compiler. See
> https://doc.rust-lang.org/reference/type-layout.html#the-rust-representation
Then you are going to have big problems, sorry.
> Specifically, the compiler is allowed to arbitrarily insert padding,
> reorder fields, etc.
> on the same code as long as the overall alignment of the struct and individual
> alignment of the fields remains correct and non-overlapping.
>
> This means the compiler is *explicitly* allowed to, for example, permute x
> and y
> as an optimization. In the above example this is unlikely, but if you
> instead consider
> struct Bar {
> x: i8,
> y: i64,
> z: i8,
> }
> It's easy to see why the compiler might decide to structure this as
> y,x,z to reduce the
> size of the struct. Those optimization decisions may be affected by
> any other part of
> the code, PGO, etc.
Then you all need to figure out some way to determine how the compiler
layed out the structure after it compiled/optimized it and be able to
compare it to previous builds (or just generate a crc based on the
layout it chose.)
> > > > > Future directions that might work for loosening it:
> > > > > * Generating crcs from debuginfo + compiler + flags
> > > > > * Adding a feature to the rust compiler to dump this information. This
> > > > > is likely to
> > > > > get pushback because Rust's current stance is that there is no
> > > > > ability to load
> > > > > object code built against a different library.
> > > >
> > > > Why not parse the function signature like we do for C?
> Because the function signature is insufficient to check the ABI, see above.
> > > >
> > > > > Would setting up Rust symbols so that they have a crc built out of
> > > > > .rmeta be
> > > > > sufficient for you to consider this useful? If not, can you help me
> > > > > understand
> > > > > what level of precision would be required?
> > > >
> > > > What exactly does .rmeta have to do with the function signature? That's
> > > > all you care about here.
> The .rmeta file contains the decisions the compiler made about layout
> in the crate
> you're interfacing with. For example, the choice to encode Bar
> with a yxz field order would be written into the .rmeta file.
Ok, then yes, can you parse the .rmeta file to get that information?
> > > rmeta is generated per crate.
> > >
> > > CRC is computed per symbol.
> > >
> > > They have different granularity.
> > > It is weird to refuse a module for incompatibility
> > > of a symbol that it is not using at all.
> >
> > I agree, this should be on a per-symbol basis, so the Rust
> > infrastructure in the kernel needs to be fixed up to support this
> > properly, not just ignored like this patchset does.
> I agree there is a divergence here, I tried to point it out so that it
> wouldn't be
> a surprise later. The .rmeta file itself (which is the only way we
> could know that
> the ABI actually matches, because layout decisions are in there) is an
> unstable
> format, which is why I would be reluctant to try to parse it and find only the
> relevant portions to hash. This isn't just a "technically unstable"
> format, but one
> in which the compiler essentially just serializes out relevant internal data
> structures, so any parser for it will involve significant alterations
> on compiler
> updates, which doesn't seem like a good plan.
> >
> > thanks,
> >
> > greg k-h
> Given the above additional information, would you be interested in a patchset
> which either:
>
> A. Computes the CRC off the Rust type signature, knowing the compiler is
> allowed to change the ABI based on information not contained in the CRC.
No.
> B. Uses the CRC of the .rmeta file, knowing, as was pointed out, that this
> effectively contains the ABI of every symbol in the compilation unit, as well
> as inline
Re: linux-next: duplicate patch in the tty tree
On Mon, Nov 27, 2023 at 11:57:18AM +1100, Stephen Rothwell wrote:
> Hi all,
>
> The following commit is also in the powerpc tree as a different commit
> (but the same patch):
>
> aa46b225ebbf ("tty: hvc: hvc_opal: Convert to platform remove callback
> returning void")
>
> This is commit
>
> f99c0e0d0859 ("tty: hvc: hvc_opal: Convert to platform remove callback
> returning void")
>
> in the powerpc tree.
Should be fine, thanks for the notice.
greg k-h
Re: Please backport feea65a338e5 ("powerpc/powernv: Fix fortify source warnings in opal-prd.c")
On Mon, Nov 20, 2023 at 10:20:13AM +1100, Michael Ellerman wrote:
> Hi,
>
> Please backport feea65a338e5 ("powerpc/powernv: Fix fortify source
> warnings in opal-prd.c") to the 6.5, 6.1, 5.15, 5.10 stable trees.
Now queued up, thanks.
greg k-h
Re: [PATCH 00/17] tty: small cleanups and fixes
On Tue, Nov 21, 2023 at 10:22:41AM +0100, Jiri Slaby (SUSE) wrote: > This is a series to fix/clean up some obvious issues I revealed during > u8+size_t conversions (to be posted later). I applied most of these except the last few, as I think you were going to reorder them, right? thanks, greg k-h
Re: [PATCH v2 0/5] MODVERSIONS + RUST Redux
On Thu, Nov 23, 2023 at 08:38:45PM +0900, Masahiro Yamada wrote: > On Thu, Nov 23, 2023 at 6:05 PM Greg KH wrote: > > > > On Wed, Nov 22, 2023 at 01:04:09PM -0800, Matthew Maurer wrote: > > > > So, even if you enable CONFIG_MODVERSIONS, > > > > nothing is checked for Rust. > > > > Genksyms computes a CRC from "int foo", and > > > > the module subsystem confirms it is a "int" > > > > variable. > > > > > > > > We know this check always succeeds. > > > > > > > > Why is this useful? > > > The reason this is immediately useful is that it allows us to have Rust > > > in use with a kernel where C modules are able to benefit from MODVERSIONS > > > checking. The check would effectively be a no-op for now, as you have > > > correctly > > > determined, but we could refine it to make it more restrictive later. > > > Since the > > > existing C approach errs on the side of "it could work" rather than "it > > > will > > > work", I thought being more permissive was the correct initial solution. > > > > But it's just providing "fake" information to the CRC checker, which > > means that the guarantee of a ABI check is not true at all. > > > > So the ask for the user of "ensure that the ABI checking is correct" is > > being circumvented here, and any change in the rust side can not be > > detected at all. > > > > The kernel is a "whole", either an option works for it, or it doesn't, > > and you are splitting that guarantee here by saying "modversions will > > only work for a portion of the kernel, not the whole thing" which is > > going to cause problems for when people expect it to actually work > > properly. > > > > So, I'd strongly recommend fixing this for the rust code if you wish to > > allow modversions to be enabled at all. > > > > > With regards to future directions that likely won't work for loosening it: > > > Unfortunately, the .rmeta format itself is not stable, so I wouldn't want > > > to > > > teach genksyms to open it up and split out the pieces for specific > > > functions. > > > Extending genksyms to parse Rust would also not solve the situation - > > > layouts are allowed to differ across compiler versions or even (in rare > > > cases) seemingly unrelated code changes. > > > > What do you mean by "layout" here? Yes, the crcs can be different > > across compiler versions and seemingly unrelated code changes (genksyms > > is VERY fragile) but that's ok, that's not what you are checking here. > > You want to know if the rust function signature changes or not from the > > last time you built the code, with the same compiler and options, that's > > all you are verifying. > > > > > Future directions that might work for loosening it: > > > * Generating crcs from debuginfo + compiler + flags > > > * Adding a feature to the rust compiler to dump this information. This > > > is likely to > > > get pushback because Rust's current stance is that there is no ability > > > to load > > > object code built against a different library. > > > > Why not parse the function signature like we do for C? > > > > > Would setting up Rust symbols so that they have a crc built out of .rmeta > > > be > > > sufficient for you to consider this useful? If not, can you help me > > > understand > > > what level of precision would be required? > > > > What exactly does .rmeta have to do with the function signature? That's > > all you care about here. > > > > > rmeta is generated per crate. > > CRC is computed per symbol. > > They have different granularity. > It is weird to refuse a module for incompatibility > of a symbol that it is not using at all. I agree, this should be on a per-symbol basis, so the Rust infrastructure in the kernel needs to be fixed up to support this properly, not just ignored like this patchset does. thanks, greg k-h
Re: [PATCH v1] powerpc: Add PVN support for HeXin C2000 processor
On Thu, Nov 23, 2023 at 05:36:11PM +0800, Zhao Ke wrote: > HeXin Tech Co. has applied for a new PVN from the OpenPower Community > for its new processor C2000. The OpenPower has assigned a new PVN > and this newly assigned PVN is 0x0066, add pvr register related > support for this PVN. > > Signed-off-by: Zhao Ke > Link: > https://discuss.openpower.foundation/t/how-to-get-a-new-pvr-for-processors-follow-power-isa/477/10 > --- > v0 -> v1: > - Fix .cpu_name with the correct description > --- > --- > arch/powerpc/include/asm/reg.h| 1 + > arch/powerpc/kernel/cpu_specs_book3s_64.h | 15 +++ > arch/powerpc/kvm/book3s_pr.c | 1 + > arch/powerpc/mm/book3s64/pkeys.c | 3 ++- > arch/powerpc/platforms/powernv/subcore.c | 3 ++- > drivers/misc/cxl/cxl.h| 3 ++- > 6 files changed, 23 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h > index 4ae4ab9090a2..7fd09f25452d 100644 > --- a/arch/powerpc/include/asm/reg.h > +++ b/arch/powerpc/include/asm/reg.h > @@ -1361,6 +1361,7 @@ > #define PVR_POWER8E 0x004B > #define PVR_POWER8NVL0x004C > #define PVR_POWER8 0x004D > +#define PVR_HX_C2000 0x0066 > #define PVR_POWER9 0x004E > #define PVR_POWER10 0x0080 > #define PVR_BE 0x0070 Why is this not in sorted order? thanks, greg k-h
Re: [PATCH v2 0/5] MODVERSIONS + RUST Redux
On Wed, Nov 22, 2023 at 01:04:09PM -0800, Matthew Maurer wrote: > > So, even if you enable CONFIG_MODVERSIONS, > > nothing is checked for Rust. > > Genksyms computes a CRC from "int foo", and > > the module subsystem confirms it is a "int" > > variable. > > > > We know this check always succeeds. > > > > Why is this useful? > The reason this is immediately useful is that it allows us to have Rust > in use with a kernel where C modules are able to benefit from MODVERSIONS > checking. The check would effectively be a no-op for now, as you have > correctly > determined, but we could refine it to make it more restrictive later. > Since the > existing C approach errs on the side of "it could work" rather than "it will > work", I thought being more permissive was the correct initial solution. But it's just providing "fake" information to the CRC checker, which means that the guarantee of a ABI check is not true at all. So the ask for the user of "ensure that the ABI checking is correct" is being circumvented here, and any change in the rust side can not be detected at all. The kernel is a "whole", either an option works for it, or it doesn't, and you are splitting that guarantee here by saying "modversions will only work for a portion of the kernel, not the whole thing" which is going to cause problems for when people expect it to actually work properly. So, I'd strongly recommend fixing this for the rust code if you wish to allow modversions to be enabled at all. > With regards to future directions that likely won't work for loosening it: > Unfortunately, the .rmeta format itself is not stable, so I wouldn't want to > teach genksyms to open it up and split out the pieces for specific functions. > Extending genksyms to parse Rust would also not solve the situation - > layouts are allowed to differ across compiler versions or even (in rare > cases) seemingly unrelated code changes. What do you mean by "layout" here? Yes, the crcs can be different across compiler versions and seemingly unrelated code changes (genksyms is VERY fragile) but that's ok, that's not what you are checking here. You want to know if the rust function signature changes or not from the last time you built the code, with the same compiler and options, that's all you are verifying. > Future directions that might work for loosening it: > * Generating crcs from debuginfo + compiler + flags > * Adding a feature to the rust compiler to dump this information. This > is likely to > get pushback because Rust's current stance is that there is no ability to > load > object code built against a different library. Why not parse the function signature like we do for C? > Would setting up Rust symbols so that they have a crc built out of .rmeta be > sufficient for you to consider this useful? If not, can you help me understand > what level of precision would be required? What exactly does .rmeta have to do with the function signature? That's all you care about here. thanks, greg k-h
Re: [PATCH v2 2/5] modules: Refactor + kdoc elf_validity_cached_copy
On Sat, Nov 18, 2023 at 02:54:43AM +, Matthew Maurer wrote: > Functionality is almost identical, just structured for better > documentation and readability. Changes: > > * Section names are checked for *all* non-SHT_NULL sections, not just > SHF_ALLOC sections. We have code that accesses section names of > non-SHF_ALLOC sections (see find_any_sec for example) > * The section name check occurs *before* strcmping on section names. > Previously, it was possible to use an out-of-bounds offset to strcmp > against ".modinfo" or ".gnu.linkonce.this_module" > * strtab is checked for NUL lead+termination and nonzero size > * The symbol table is swept to ensure offsets are inbounds of strtab > > While some of these oversights would normally be worrying, all of the > potentially unverified accesses occur after signature check, and only in > response to a user who can load a kernel module. > > Signed-off-by: Matthew Maurer > --- > kernel/module/internal.h | 7 +- > kernel/module/main.c | 585 +-- > 2 files changed, 444 insertions(+), 148 deletions(-) Again, this needs to be broken into much smaller pieces before we can even review it. Would you want to review this? thanks, greg "think of the reviewers" k-h
Re: [PATCH v2 1/5] export_report: Rehabilitate script
On Sat, Nov 18, 2023 at 02:54:42AM +, Matthew Maurer wrote: > * modules.order has .o files when in a build dir, support this > * .mod.c source layout has changed, update regexes to match > * Add a stage 3, to be more robust against additional .mod.c content When you have to list different things you do in a patch, that is a huge hint that you need to break up your patch into smaller pieces. Remember, each patch can only do one logical thing. I know it feels odd, but it makes it easier to review. This patch, as-is, is nothing that I would be able to take, please make it a series. thanks, greg k-h
Re: [PATCH 10/10] [RFC] wifi: remove ipw2100/ipw2200 drivers
On Thu, Oct 26, 2023 at 12:41:27PM +0300, Kalle Valo wrote: > For example, I see lots of dead code under '#ifdef NOT_YET' and '#if 0', > removing those is a good a start. Also converting the ugly debug_level > procfs file to something more modern would be nice, maybe using just > dev_dbg() throught the driver is a good option? Or maybe use a module > parameter instead? Ick, no new module parameters, this isn't the 1990's, please just use the netdev debug lines instead :) thanks, greg k-h
Re: linux-next: manual merge of the tty tree with the powerpc tree
On Fri, Aug 18, 2023 at 02:58:26PM +1000, Stephen Rothwell wrote:
> Hi all,
>
> Today's linux-next merge of the tty tree got a conflict in:
>
> arch/powerpc/include/asm/fs_pd.h
>
> between commits:
>
> e6e077cb2aa4 ("powerpc/include: Declare mpc8xx_immr in 8xx_immap.h")
> fecc436a97af ("powerpc/include: Remove mpc8260.h and m82xx_pci.h")
> fbbf4280dae4 ("powerpc/8xx: Remove immr_map() and immr_unmap()")
> 7768716d2f19 ("powerpc/cpm2: Remove cpm2_map() and cpm2_unmap()")
>
> from the powerpc tree and commit:
>
> c2d6c1b4f034 ("serial: cpm_uart: Use get_baudrate() instead of
> uart_baudrate()")
>
> from the tty tree.
>
> I fixed it up (see below) and can carry the fix as necessary. This
> is now fixed as far as linux-next is concerned, but any non trivial
> conflicts should be mentioned to your upstream maintainer when your tree
> is submitted for merging. You may also want to consider cooperating
> with the maintainer of the conflicting tree to minimise any particularly
> complex conflicts.
>
> Note that after all the above are applied, it looks like this file can
> be removed completely as nothing in the tree includes it any more.
Thanks for the notice, I'll let the ppc developers remove it as it's in
their tree.
thanks,
greg k-h
Re: Fwd: Memory corruption in multithreaded user space program while calling fork
On Wed, Jul 05, 2023 at 10:51:57AM +0200, Linux regression tracking (Thorsten Leemhuis) wrote: > On 05.07.23 09:08, Greg KH wrote: > > On Tue, Jul 04, 2023 at 01:22:54PM -0700, Suren Baghdasaryan wrote: > >> On Tue, Jul 4, 2023 at 9:18 AM Andrew Morton > >> wrote: > >>> On Tue, 4 Jul 2023 09:00:19 +0100 Greg KH > >>> wrote: > >>>>>>>> Thanks! I'll investigate this later today. After discussing with > >>>>>>>> Andrew, we would like to disable CONFIG_PER_VMA_LOCK by default until > >>>>>>>> the issue is fixed. I'll post a patch shortly. > >>>>>>> > >>>>>>> Posted at: > >>>>>>> https://lore.kernel.org/all/20230703182150.2193578-1-sur...@google.com/ > >>>>>> > >>>>>> As that change fixes something in 6.4, why not cc: stable on it as > >>>>>> well? > >>>>> > >>>>> Sorry, I thought since per-VMA locks were introduced in 6.4 and this > >>>>> patch is fixing 6.4 I didn't need to send it to stable for older > >>>>> versions. Did I miss something? > >>>> > >>>> 6.4.y is a stable kernel tree right now, so yes, it needs to be included > >>>> there :) > >>> > >>> I'm in wait-a-few-days-mode on this. To see if we have a backportable > >>> fix rather than disabling the feature in -stable. > > Andrew, how long will you remain in "wait-a-few-days-mode"? Given what > Greg said below and that we already had three reports I know of I'd > prefer if we could fix this rather sooner than later in mainline -- > especially as Arch Linux and openSUSE Tumbleweed likely have switched to > 6.4.y already or will do so soon. Ick, yeah, and Fedora should be switching soon too, and I want to drop support for 6.3.y "any day now". Is there just a revert we can do now first to resolve the regression and then work on fixing this up "better" for 6.6-rc1? thanks, greg k-h
Re: Fwd: Memory corruption in multithreaded user space program while calling fork
On Tue, Jul 04, 2023 at 01:22:54PM -0700, Suren Baghdasaryan wrote: > On Tue, Jul 4, 2023 at 9:18 AM Andrew Morton > wrote: > > > > On Tue, 4 Jul 2023 09:00:19 +0100 Greg KH > > wrote: > > > > > > > > > Thanks! I'll investigate this later today. After discussing with > > > > > > > Andrew, we would like to disable CONFIG_PER_VMA_LOCK by default > > > > > > > until > > > > > > > the issue is fixed. I'll post a patch shortly. > > > > > > > > > > > > Posted at: > > > > > > https://lore.kernel.org/all/20230703182150.2193578-1-sur...@google.com/ > > > > > > > > > > As that change fixes something in 6.4, why not cc: stable on it as > > > > > well? > > > > > > > > Sorry, I thought since per-VMA locks were introduced in 6.4 and this > > > > patch is fixing 6.4 I didn't need to send it to stable for older > > > > versions. Did I miss something? > > > > > > 6.4.y is a stable kernel tree right now, so yes, it needs to be included > > > there :) > > > > I'm in wait-a-few-days-mode on this. To see if we have a backportable > > fix rather than disabling the feature in -stable. > > Ok, I think we have a fix posted at [2] and it's cleanly applies to > 6.4.y stable branch as well. However fork() performance might slightly > regress, therefore disabling per-VMA locks by default for now seems to > be preferable even with this fix (see discussion at > https://lore.kernel.org/all/54cd9ffb-8f4b-003f-c2d6-3b6b0d2cb...@google.com/). > IOW, both [1] and [2] should be applied to 6.4.y stable. Both apply > cleanly and I CC'ed stable on [2]. Greg, should I send [1] separately > to stable@vger? We can't do anything for stable until it lands in Linus's tree, so if you didn't happen to have the stable@ tag in the patch, just email us the git SHA1 and I can pick it up that way. thanks, greg k-h
Re: Fwd: Memory corruption in multithreaded user space program while calling fork
On Tue, Jul 04, 2023 at 12:45:39AM -0700, Suren Baghdasaryan wrote:
> On Mon, Jul 3, 2023 at 11:44 AM Greg KH wrote:
> >
> > On Mon, Jul 03, 2023 at 11:27:19AM -0700, Suren Baghdasaryan wrote:
> > > On Mon, Jul 3, 2023 at 11:08 AM Suren Baghdasaryan
> > > wrote:
> > > >
> > > > On Mon, Jul 3, 2023 at 2:53 AM Linux regression tracking (Thorsten
> > > > Leemhuis) wrote:
> > > > >
> > > > > On 02.07.23 14:27, Bagas Sanjaya wrote:
> > > > > > I notice a regression report on Bugzilla [1]. Quoting from it:
> > > > > >
> > > > > >> After upgrading to kernel version 6.4.0 from 6.3.9, I noticed
> > > > > >> frequent but random crashes in a user space program. After a lot
> > > > > >> of reduction, I have come up with the following reproducer program:
> > > > > > [...]
> > > > > >> After tuning the various parameters for my computer, exit code 2,
> > > > > >> which indicates that memory corruption was detected, occurs
> > > > > >> approximately 99% of the time. Exit code 1, which occurs
> > > > > >> approximately 1% of the time, means it ran out of
> > > > > >> statically-allocated memory before reproducing the issue, and
> > > > > >> increasing the memory usage any more only leads to diminishing
> > > > > >> returns. There is also something like a 0.1% chance that it
> > > > > >> segfaults due to memory corruption elsewhere than in the
> > > > > >> statically-allocated buffer.
> > > > > >>
> > > > > >> With this reproducer in hand, I was able to perform the following
> > > > > >> bisection:
> > > > > > [...]
> > > > > >
> > > > > > See Bugzilla for the full thread.
> > > > >
> > > > > Additional details from
> > > > > https://bugzilla.kernel.org/show_bug.cgi?id=217624#c5 :
> > > > >
> > > > > ```
> > > > > I can confirm that v6.4 with 0bff0aaea03e2a3ed6bfa302155cca8a432a1829
> > > > > reverted no longer causes any memory corruption with either my
> > > > > reproducer or the original program.
> > > > > ```
> > > > >
> > > > > FWIW: 0bff0aaea03 ("x86/mm: try VMA lock-based page fault handling
> > > > > first") [merged for v6.4-rc1, authored by Suren Baghdasaryan [already
> > > > > CCed]]
> > > > >
> > > > > That's the same commit that causes build problems with go:
> > > > >
> > > > > https://lore.kernel.org/all/dbdef34c-3a07-5951-e1ae-e9c6e3cdf...@kernel.org/
> > > >
> > > > Thanks! I'll investigate this later today. After discussing with
> > > > Andrew, we would like to disable CONFIG_PER_VMA_LOCK by default until
> > > > the issue is fixed. I'll post a patch shortly.
> > >
> > > Posted at:
> > > https://lore.kernel.org/all/20230703182150.2193578-1-sur...@google.com/
> >
> > As that change fixes something in 6.4, why not cc: stable on it as well?
>
> Sorry, I thought since per-VMA locks were introduced in 6.4 and this
> patch is fixing 6.4 I didn't need to send it to stable for older
> versions. Did I miss something?
6.4.y is a stable kernel tree right now, so yes, it needs to be included
there :)
Re: Fwd: Memory corruption in multithreaded user space program while calling fork
On Mon, Jul 03, 2023 at 11:27:19AM -0700, Suren Baghdasaryan wrote:
> On Mon, Jul 3, 2023 at 11:08 AM Suren Baghdasaryan wrote:
> >
> > On Mon, Jul 3, 2023 at 2:53 AM Linux regression tracking (Thorsten
> > Leemhuis) wrote:
> > >
> > > On 02.07.23 14:27, Bagas Sanjaya wrote:
> > > > I notice a regression report on Bugzilla [1]. Quoting from it:
> > > >
> > > >> After upgrading to kernel version 6.4.0 from 6.3.9, I noticed frequent
> > > >> but random crashes in a user space program. After a lot of reduction,
> > > >> I have come up with the following reproducer program:
> > > > [...]
> > > >> After tuning the various parameters for my computer, exit code 2,
> > > >> which indicates that memory corruption was detected, occurs
> > > >> approximately 99% of the time. Exit code 1, which occurs
> > > >> approximately 1% of the time, means it ran out of statically-allocated
> > > >> memory before reproducing the issue, and increasing the memory usage
> > > >> any more only leads to diminishing returns. There is also something
> > > >> like a 0.1% chance that it segfaults due to memory corruption
> > > >> elsewhere than in the statically-allocated buffer.
> > > >>
> > > >> With this reproducer in hand, I was able to perform the following
> > > >> bisection:
> > > > [...]
> > > >
> > > > See Bugzilla for the full thread.
> > >
> > > Additional details from
> > > https://bugzilla.kernel.org/show_bug.cgi?id=217624#c5 :
> > >
> > > ```
> > > I can confirm that v6.4 with 0bff0aaea03e2a3ed6bfa302155cca8a432a1829
> > > reverted no longer causes any memory corruption with either my
> > > reproducer or the original program.
> > > ```
> > >
> > > FWIW: 0bff0aaea03 ("x86/mm: try VMA lock-based page fault handling
> > > first") [merged for v6.4-rc1, authored by Suren Baghdasaryan [already
> > > CCed]]
> > >
> > > That's the same commit that causes build problems with go:
> > >
> > > https://lore.kernel.org/all/dbdef34c-3a07-5951-e1ae-e9c6e3cdf...@kernel.org/
> >
> > Thanks! I'll investigate this later today. After discussing with
> > Andrew, we would like to disable CONFIG_PER_VMA_LOCK by default until
> > the issue is fixed. I'll post a patch shortly.
>
> Posted at:
> https://lore.kernel.org/all/20230703182150.2193578-1-sur...@google.com/
As that change fixes something in 6.4, why not cc: stable on it as well?
thanks,
greg k-h
Re: [PATCH] powerpc: fix debugfs_create_dir error checking
On Sun, May 28, 2023 at 01:16:44PM +0530, mirim...@outlook.com wrote:
> From: Immad Mir
>
> The debugfs_create_dir returns ERR_PTR incase of an error and the
> correct way of checking it by using the IS_ERR inline function, and
> not the simple null comparision. This patch fixes this.
>
> Suggested-By: Ivan Orlov
> Signed-off-by: Immad Mir
> ---
> arch/powerpc/platforms/powernv/opal-xscom.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/opal-xscom.c
> b/arch/powerpc/platforms/powernv/opal-xscom.c
> index 6b4eed2ef..262cd6fac 100644
> --- a/arch/powerpc/platforms/powernv/opal-xscom.c
> +++ b/arch/powerpc/platforms/powernv/opal-xscom.c
> @@ -168,7 +168,7 @@ static int scom_debug_init_one(struct dentry *root,
> struct device_node *dn,
> ent->path.size = strlen((char *)ent->path.data);
>
> dir = debugfs_create_dir(ent->name, root);
> - if (!dir) {
> + if (IS_ERR(dir)) {
> kfree(ent->path.data);
> kfree(ent);
> return -1;
Why is this driver caring if debugfs is working or not at all? It
should just ignore the error and keep moving forward.
And -1 is not a valid error number :(
Have you hit this error on this driver?
thanks,
greg k-h
Re: Symbol cpu_feature_keys should be exported to all modules on powerpc
On Tue, Feb 28, 2023 at 04:18:12PM +0800, 王昊然 wrote: > Just like symbol 'mmu_feature_keys'[1], 'cpu_feature_keys' was referenced > indirectly by many inline functions; any GPL-incompatible modules using such > a function will be potentially broken due to 'cpu_feature_keys' being > exported as GPL-only. > > For example it still breaks ZFS, see > https://github.com/openzfs/zfs/issues/14545 > > [1]: > https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20220329085709.4132729-1-haoke...@gmail.com/ External modules are always on their own, sorry. Especially ones that are not released under the GPL. good luck! greg k-h
Re: [PATCH v2] usb: fix some spelling mistakes in comment of gadget
On Wed, Feb 15, 2023 at 05:35:35PM -0800, Zhou nan wrote: > usb: Fix spelling mistake in comments of gadget. > > Signed-off-by: Zhou nan > --- Hi, This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree. You are receiving this message because of the following common error(s) as indicated below: - You did not specify a description of why the patch is needed, or possibly, any description at all, in the email body. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/process/submitting-patches.rst for what is needed in order to properly describe the change. - You did not write a descriptive Subject: for the patch, allowing Greg, and everyone else, to know what this patch is all about. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/process/submitting-patches.rst for what a proper Subject: line should look like. If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers. thanks, greg k-h's patch email bot
Re: [PATCH] usb: fix some spelling mistakes in comment
On Wed, Feb 15, 2023 at 12:43:24AM -0800, Zhou nan wrote:
> Fix typos in comment.
>
> Signed-off-by: Zhou nan
> ---
> drivers/usb/gadget/udc/fsl_udc_core.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/fsl_udc_core.c
> b/drivers/usb/gadget/udc/fsl_udc_core.c
> index a67873a074b7..da876d09fc01 100644
> --- a/drivers/usb/gadget/udc/fsl_udc_core.c
> +++ b/drivers/usb/gadget/udc/fsl_udc_core.c
> @@ -471,7 +471,7 @@ static int dr_ep_get_stall(unsigned char ep_num, unsigned
> char dir)
> /
>
> /*--
> -* struct_ep_qh_setup(): set the Endpoint Capabilites field of QH
> +* struct_ep_qh_setup(): set the Endpoint Capabilities field of QH
> * @zlt: Zero Length Termination Select (1: disable; 0: enable)
> * @mult: Mult field
> --*/
> @@ -483,7 +483,7 @@ static void struct_ep_qh_setup(struct fsl_udc *udc,
> unsigned char ep_num,
> struct ep_queue_head *p_QH = >ep_qh[2 * ep_num + dir];
> unsigned int tmp = 0;
>
> - /* set the Endpoint Capabilites in QH */
> + /* set the Endpoint Capabilities in QH */
> switch (ep_type) {
> case USB_ENDPOINT_XFER_CONTROL:
> /* Interrupt On Setup (IOS). for control ep */
> @@ -593,7 +593,7 @@ static int fsl_ep_enable(struct usb_ep *_ep,
> ep->stopped = 0;
>
> /* Controller related setup */
> - /* Init EPx Queue Head (Ep Capabilites field in QH
> + /* Init EPx Queue Head (Ep Capabilities field in QH
>* according to max, zlt, mult) */
> struct_ep_qh_setup(udc, (unsigned char) ep_index(ep),
> (unsigned char) ((desc->bEndpointAddress & USB_DIR_IN)
> @@ -1361,7 +1361,7 @@ static void ch9getstatus(struct fsl_udc *udc, u8
> request_type, u16 value,
> udc->ep0_dir = USB_DIR_IN;
> /* Borrow the per device status_req */
> req = udc->status_req;
> - /* Fill in the reqest structure */
> + /* Fill in the request structure */
> *((u16 *) req->req.buf) = cpu_to_le16(tmp);
>
> req->ep = ep;
> --
> 2.27.0
>
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.
You are receiving this message because of the following common error(s)
as indicated below:
- Your patch did many different things all at once, making it difficult
to review. All Linux kernel patches need to only do one thing at a
time. If you need to do multiple things (such as clean up all coding
style issues in a file/driver), do it in a sequence of patches, each
one doing only one thing. This will make it easier to review the
patches to ensure that they are correct, and to help alleviate any
merge issues that larger patches can cause.
- You did not specify a description of why the patch is needed, or
possibly, any description at all, in the email body. Please read the
section entitled "The canonical patch format" in the kernel file,
Documentation/process/submitting-patches.rst for what is needed in
order to properly describe the change.
- You did not write a descriptive Subject: for the patch, allowing Greg,
and everyone else, to know what this patch is all about. Please read
the section entitled "The canonical patch format" in the kernel file,
Documentation/process/submitting-patches.rst for what a proper
Subject: line should look like.
If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.
thanks,
greg k-h's patch email bot
Re: [PATCH v2 3/6] hvcs: Use driver groups to manage driver attributes
On Thu, Feb 02, 2023 at 04:28:01PM -0600, Brian King wrote:
> Rather than manually creating attributes for the hvcs driver,
> let the driver core do this for us. This also fixes some hotplug
> remove issues and ensures that cleanup of these attributes
> is done in the right order.
>
> Signed-off-by: Brian King
> ---
> drivers/tty/hvc/hvcs.c | 24 +++-
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
> index 7f79444b4d89..5de7ad40 100644
> --- a/drivers/tty/hvc/hvcs.c
> +++ b/drivers/tty/hvc/hvcs.c
> @@ -473,6 +473,20 @@ static ssize_t rescan_store(struct device_driver *ddp,
> const char * buf,
>
> static DRIVER_ATTR_RW(rescan);
>
> +static struct attribute *hvcs_attrs[] = {
> + _attr_rescan.attr,
> + NULL,
> +};
> +
> +static struct attribute_group hvcs_attr_group = {
> + .attrs = hvcs_attrs,
> +};
> +
> +const static struct attribute_group *hvcs_attr_groups[] = {
> + _attr_group,
> + NULL,
> +};
Again, ATTRIBUTE_GROUPS()?
thanks,
greg k-h
Re: [PATCH v2 1/6] hvcs: Fix hvcs port reference counting
On Thu, Feb 02, 2023 at 04:27:59PM -0600, Brian King wrote: > The hvcs driver only ever gets two references to the port. One > at initialization time, and one at install time. Remove the code > that was trying to do multiple port puts for each open, which > would result in more puts than gets. > > Signed-off-by: Brian King > --- > drivers/tty/hvc/hvcs.c | 18 -- > 1 file changed, 18 deletions(-) I already took this patch (you got an email about it), no need to send it again. thanks, greg k-h
Re: [PATCH v2 2/6] hvcs: Use dev_groups to manage hvcs device attributes
On Thu, Feb 02, 2023 at 04:28:00PM -0600, Brian King wrote:
> Use the dev_groups functionality to manage the attribute groups
> for hvcs devices. This simplifies the code and also eliminates
> errors coming from kernfs when attempting to remove a console
> device that is in use.
>
> Signed-off-by: Brian King
> ---
> drivers/tty/hvc/hvcs.c | 24 +++-
> 1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
> index faf5ccfc561e..7f79444b4d89 100644
> --- a/drivers/tty/hvc/hvcs.c
> +++ b/drivers/tty/hvc/hvcs.c
> @@ -432,7 +432,7 @@ static ssize_t hvcs_index_show(struct device *dev, struct
> device_attribute *attr
>
> static DEVICE_ATTR(index, S_IRUGO, hvcs_index_show, NULL);
>
> -static struct attribute *hvcs_attrs[] = {
> +static struct attribute *hvcs_dev_attrs[] = {
> _attr_partner_vtys.attr,
> _attr_partner_clcs.attr,
> _attr_current_vty.attr,
> @@ -441,8 +441,13 @@ static struct attribute *hvcs_attrs[] = {
> NULL,
> };
>
> -static struct attribute_group hvcs_attr_group = {
> - .attrs = hvcs_attrs,
> +static struct attribute_group hvcs_attr_dev_group = {
> + .attrs = hvcs_dev_attrs,
> +};
> +
> +const static struct attribute_group *hvcs_attr_dev_groups[] = {
> + _attr_dev_group,
> + NULL,
> };
Why not just use the ATTRIBUTE_GROUPS() macro here?
thanks,
greg k-h
Re: [PATCH 2/6] hvcs: Remove sysfs file prior to vio unregister
On Wed, Feb 01, 2023 at 01:57:39PM -0600, Brian King wrote:
> This moves the removal of the rescan sysfs attribute to occur
> before the call to unregister the vio to ensure the removal
> does not fail due to the vio driver already being freed.
>
> Signed-off-by: Brian King
> ---
> drivers/tty/hvc/hvcs.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
> index faf5ccfc561e..9131dcb2e8d8 100644
> --- a/drivers/tty/hvc/hvcs.c
> +++ b/drivers/tty/hvc/hvcs.c
> @@ -1519,6 +1519,8 @@ static int __init hvcs_module_init(void)
>
> static void __exit hvcs_module_exit(void)
> {
> + driver_remove_file(_vio_driver.driver, _attr_rescan);
Again, set the default group for the driver and then you don't have to
do any of this at all please.
thanks,
greg k-h
Re: [PATCH 3/6] hvcs: Remove sysfs group earlier
On Wed, Feb 01, 2023 at 01:57:40PM -0600, Brian King wrote: > Cleanup the sysfs group earlier in remove. This eliminates > errors coming from kernfs when attempting to remove a console > device that is in use. > > Signed-off-by: Brian King > --- > drivers/tty/hvc/hvcs.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c > index 9131dcb2e8d8..9c5887d0c882 100644 > --- a/drivers/tty/hvc/hvcs.c > +++ b/drivers/tty/hvc/hvcs.c > @@ -688,8 +688,6 @@ static void hvcs_destruct_port(struct tty_port *p) > spin_unlock_irqrestore(>lock, flags); > spin_unlock(_structs_lock); > > - sysfs_remove_group(>dev.kobj, _attr_group); > - > kfree(hvcsd); > } > > @@ -814,6 +812,8 @@ static void hvcs_remove(struct vio_dev *dev) >*/ > tty_port_put(>port); > > + sysfs_remove_group(>dev.kobj, _attr_group); > + Why is this needed at all? The files should be auto-removed when the device is removed, right? And calling sysfs_*() functions from a driver is a huge hint that something is wrong here. Worst case, this should be calling device_remove_group(), but really, the default groups pointer should be set and then you don't have to add/remove anything, it will all happen automatically for you by the driver core at the properly place and time. Can you do that instead of this change? That should fix it all up properly. thanks, greg k-h
Re: [PATCH 6/6] powerpc/pseries: Implement secvars for dynamic secure boot
On Wed, Dec 28, 2022 at 06:29:43PM +1100, Russell Currey wrote: > The pseries platform can support dynamic secure boot (i.e. secure boot > using user-defined keys) using variables contained with the PowerVM LPAR > Platform KeyStore (PLPKS). Using the powerpc secvar API, expose the > relevant variables for pseries dynamic secure boot through the existing > secvar filesystem layout. > > The relevant variables for dynamic secure boot are signed in the > keystore, and can only be modified using the H_PKS_SIGNED_UPDATE hcall. > Object labels in the keystore are encoded using ucs2 format. With our > fixed variable names we don't have to care about encoding outside of the > necessary byte padding. > > When a user writes to a variable, the first 8 bytes of data must contain > the signed update flags as defined by the hypervisor. > > When a user reads a variable, the first 4 bytes of data contain the > policies defined for the object. > > Limitations exist due to the underlying implementation of sysfs binary > attributes, as is the case for the OPAL secvar implementation - > partial writes are unsupported and writes cannot be larger than PAGE_SIZE. > > Co-developed-by: Nayna Jain > Signed-off-by: Nayna Jain > Co-developed-by: Andrew Donnellan > Signed-off-by: Andrew Donnellan > Signed-off-by: Russell Currey > --- > Documentation/ABI/testing/sysfs-secvar| 8 + > arch/powerpc/platforms/pseries/Kconfig| 13 + > arch/powerpc/platforms/pseries/Makefile | 4 +- > arch/powerpc/platforms/pseries/plpks-secvar.c | 250 ++ > 4 files changed, 273 insertions(+), 2 deletions(-) > create mode 100644 arch/powerpc/platforms/pseries/plpks-secvar.c > > diff --git a/Documentation/ABI/testing/sysfs-secvar > b/Documentation/ABI/testing/sysfs-secvar > index feebb8c57294..e6fef664c9c8 100644 > --- a/Documentation/ABI/testing/sysfs-secvar > +++ b/Documentation/ABI/testing/sysfs-secvar > @@ -44,3 +44,11 @@ Contact: Nayna Jain > Description: A write-only file that is used to submit the new value for the > variable. The size of the file represents the maximum size of > the variable data that can be written. > + > +What:/sys/firmware/secvar/config > +Date:December 2022 > +Contact: Nayna Jain > +Description: This optional directory contains read-only config attributes as > + defined by the secure variable implementation. All data is in > + ASCII format. The directory is only created if the backing > + implementation provides variables to populate it. That is _WAY_ too vague, sorry. You have to define the files that are in here and what the contents of them are. If you do not, you will get warnings from the tools that we have that you can run on a system that wants to verify all sysfs files are properly documented. > diff --git a/arch/powerpc/platforms/pseries/Kconfig > b/arch/powerpc/platforms/pseries/Kconfig > index a3b4d99567cb..94e08c405d50 100644 > --- a/arch/powerpc/platforms/pseries/Kconfig > +++ b/arch/powerpc/platforms/pseries/Kconfig > @@ -162,6 +162,19 @@ config PSERIES_PLPKS > > If unsure, select N. > > +config PSERIES_PLPKS_SECVAR > + depends on PSERIES_PLPKS > + depends on PPC_SECURE_BOOT > + bool "Support for the PLPKS secvar interface" > + help > + PowerVM can support dynamic secure boot with user-defined keys > + through the PLPKS. Keystore objects used in dynamic secure boot > + can be exposed to the kernel and userspace through the powerpc > + secvar infrastructure. Select this to enable the PLPKS backend > + for secvars for use in pseries dynamic secure boot. > + > + If unsure, select N. > + > config PAPR_SCM > depends on PPC_PSERIES && MEMORY_HOTPLUG && LIBNVDIMM > tristate "Support for the PAPR Storage Class Memory interface" > diff --git a/arch/powerpc/platforms/pseries/Makefile > b/arch/powerpc/platforms/pseries/Makefile > index 92310202bdd7..807756991f9d 100644 > --- a/arch/powerpc/platforms/pseries/Makefile > +++ b/arch/powerpc/platforms/pseries/Makefile > @@ -27,8 +27,8 @@ obj-$(CONFIG_PAPR_SCM) += papr_scm.o > obj-$(CONFIG_PPC_SPLPAR) += vphn.o > obj-$(CONFIG_PPC_SVM)+= svm.o > obj-$(CONFIG_FA_DUMP)+= rtas-fadump.o > -obj-$(CONFIG_PSERIES_PLPKS) += plpks.o > - > +obj-$(CONFIG_PSERIES_PLPKS) += plpks.o > +obj-$(CONFIG_PSERIES_PLPKS_SECVAR) += plpks-secvar.o > obj-$(CONFIG_SUSPEND)+= suspend.o > obj-$(CONFIG_PPC_VAS)+= vas.o vas-sysfs.o > > diff --git a/arch/powerpc/platforms/pseries/plpks-secvar.c > b/arch/powerpc/platforms/pseries/plpks-secvar.c > new file mode 100644 > index ..3f9ff16c03c8 > --- /dev/null > +++ b/arch/powerpc/platforms/pseries/plpks-secvar.c > @@ -0,0 +1,250 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Secure variable implementation using
Re: [PATCH] [Backport for 4.14] perf script python: Remove explicit shebang from tests/attr.c
On Mon, Dec 19, 2022 at 12:21:47PM +, Christophe Leroy wrote: > > > Le 19/12/2022 à 13:18, Greg KH a écrit : > > On Fri, Dec 16, 2022 at 12:38:12PM +0100, Christophe Leroy wrote: > >> From: Tony Jones > >> > >> [Upstream commit d72eadbc1d2866fc047edd4535ffb0298fe240be] > >> > >> tests/attr.c invokes attr.py via an explicit invocation of Python > >> ($PYTHON) so there is therefore no need for an explicit shebang. > >> > >> Also most distros follow pep-0394 which recommends that /usr/bin/python > >> refer only to v2 and so may not exist on the system (if PYTHON=python3). > >> > >> Signed-off-by: Tony Jones > >> Acked-by: Jiri Olsa > >> Cc: Jonathan Corbet > >> Cc: Ravi Bangoria > >> Cc: Seeteena Thoufeek > >> Link: http://lkml.kernel.org/r/20190124005229.16146-5-to...@suse.de > >> Signed-off-by: Arnaldo Carvalho de Melo > >> Signed-off-by: Christophe Leroy > >> --- > >> tools/perf/tests/attr.py | 1 - > >> 1 file changed, 1 deletion(-) > > > > Why only 4.14? What about 4.19? > > > > > I submitted that backport because I encountered the problem while > building perf for 4.14. > > I didn't look at other kernel versions. In the future, remember that we can not add fixes to an older version and not a newer one, otherwise you would have a regression moving to a newer kernel. thanks, greg k-h
Re: [PATCH] [Backport for 4.14] perf script python: Remove explicit shebang from tests/attr.c
On Fri, Dec 16, 2022 at 12:38:12PM +0100, Christophe Leroy wrote: > From: Tony Jones > > [Upstream commit d72eadbc1d2866fc047edd4535ffb0298fe240be] > > tests/attr.c invokes attr.py via an explicit invocation of Python > ($PYTHON) so there is therefore no need for an explicit shebang. > > Also most distros follow pep-0394 which recommends that /usr/bin/python > refer only to v2 and so may not exist on the system (if PYTHON=python3). > > Signed-off-by: Tony Jones > Acked-by: Jiri Olsa > Cc: Jonathan Corbet > Cc: Ravi Bangoria > Cc: Seeteena Thoufeek > Link: http://lkml.kernel.org/r/20190124005229.16146-5-to...@suse.de > Signed-off-by: Arnaldo Carvalho de Melo > Signed-off-by: Christophe Leroy > --- > tools/perf/tests/attr.py | 1 - > 1 file changed, 1 deletion(-) Why only 4.14? What about 4.19? thanks, greg k-h
Re: [PATCH] [REBASED for 4.14] once: add DO_ONCE_SLOW() for sleepable contexts
On Tue, Dec 13, 2022 at 01:22:40PM +0100, Christophe Leroy wrote:
> From: Eric Dumazet
>
> [ Upstream commit 62c07983bef9d3e78e71189441e1a470f0d1e653 ]
>
> Christophe Leroy reported a ~80ms latency spike
> happening at first TCP connect() time.
>
> This is because __inet_hash_connect() uses get_random_once()
> to populate a perturbation table which became quite big
> after commit 4c2c8f03a5ab ("tcp: increase source port perturb table to 2^16")
>
> get_random_once() uses DO_ONCE(), which block hard irqs for the duration
> of the operation.
>
> This patch adds DO_ONCE_SLOW() which uses a mutex instead of a spinlock
> for operations where we prefer to stay in process context.
>
> Then __inet_hash_connect() can use get_random_slow_once()
> to populate its perturbation table.
>
> Fixes: 4c2c8f03a5ab ("tcp: increase source port perturb table to 2^16")
> Fixes: 190cc82489f4 ("tcp: change source port randomizarion at connect()
> time")
> Reported-by: Christophe Leroy
> Link:
> https://lore.kernel.org/netdev/cann89ilaeybaoyajy0y9umgfff5gpxduog-ervb2jddrnq5...@mail.gmail.com/T/#t
> Signed-off-by: Eric Dumazet
> Cc: Willy Tarreau
> Tested-by: Christophe Leroy
> Signed-off-by: David S. Miller
> Signed-off-by: Sasha Levin
> Signed-off-by: Christophe Leroy
> ---
> include/linux/once.h | 28
> lib/once.c | 30 ++
> net/ipv4/inet_hashtables.c | 4 ++--
> 3 files changed, 60 insertions(+), 2 deletions(-)
Now queued up, thanks.
greg k-h
Re: [PATCH 2/6] macio: Make remove callback of macio driver void returned
On Mon, Dec 05, 2022 at 11:36:40PM +0800, Dawei Li wrote:
> Commit fc7a6209d571 ("bus: Make remove callback return
> void") forces bus_type::remove be void-returned, it doesn't
> make much sense for any bus based driver implementing remove
> callbalk to return non-void to its caller.
>
> This change is for macio bus based drivers.
>
> Signed-off-by: Dawei Li
> ---
> arch/powerpc/include/asm/macio.h| 12 ++--
> drivers/ata/pata_macio.c| 4 +---
> drivers/macintosh/rack-meter.c | 4 +---
> drivers/net/ethernet/apple/bmac.c | 4 +---
> drivers/net/ethernet/apple/mace.c | 4 +---
> drivers/net/wireless/intersil/orinoco/airport.c | 4 +---
> drivers/scsi/mac53c94.c | 5 +
> drivers/scsi/mesh.c | 5 +
> drivers/tty/serial/pmac_zilog.c | 7 ++-
> sound/aoa/soundbus/i2sbus/core.c| 4 +---
> 10 files changed, 16 insertions(+), 37 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/macio.h
> b/arch/powerpc/include/asm/macio.h
> index ff5fd82d9ff0..f641c730c3b7 100644
> --- a/arch/powerpc/include/asm/macio.h
> +++ b/arch/powerpc/include/asm/macio.h
> @@ -124,15 +124,15 @@ static inline struct pci_dev *macio_get_pci_dev(struct
> macio_dev *mdev)
> */
> struct macio_driver
> {
> - int (*probe)(struct macio_dev* dev, const struct of_device_id
> *match);
> - int (*remove)(struct macio_dev* dev);
> + int (*probe)(struct macio_dev *dev, const struct of_device_id
> *match);
> + void(*remove)(struct macio_dev *dev);
Again, you are changing lines you do not need to here.
thanks,
greg k-h
Re: [PATCH 6/6] soundbus: make remove callback of soundbus driver void returned
On Mon, Dec 05, 2022 at 11:36:44PM +0800, Dawei Li wrote:
> Since commit fc7a6209d571 ("bus: Make remove callback return
> void") forces bus_type::remove be void-returned, it doesn't
> make much sense for any bus based driver implementing remove
> callbalk to return non-void to its caller.
>
> This change is for soundbus based drivers.
>
> Signed-off-by: Dawei Li
> ---
> sound/aoa/fabrics/layout.c| 3 +--
> sound/aoa/soundbus/soundbus.h | 6 +++---
> 2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/sound/aoa/fabrics/layout.c b/sound/aoa/fabrics/layout.c
> index ec4ef18555bc..850dc8c53e9b 100644
> --- a/sound/aoa/fabrics/layout.c
> +++ b/sound/aoa/fabrics/layout.c
> @@ -1094,7 +1094,7 @@ static int aoa_fabric_layout_probe(struct soundbus_dev
> *sdev)
> return -ENODEV;
> }
>
> -static int aoa_fabric_layout_remove(struct soundbus_dev *sdev)
> +static void aoa_fabric_layout_remove(struct soundbus_dev *sdev)
> {
> struct layout_dev *ldev = dev_get_drvdata(>ofdev.dev);
> int i;
> @@ -1123,7 +1123,6 @@ static int aoa_fabric_layout_remove(struct soundbus_dev
> *sdev)
> kfree(ldev);
> sdev->pcmid = -1;
> sdev->pcmname = NULL;
> - return 0;
> }
>
> #ifdef CONFIG_PM_SLEEP
> diff --git a/sound/aoa/soundbus/soundbus.h b/sound/aoa/soundbus/soundbus.h
> index 3a99c1f1a3ca..230dfa1ba270 100644
> --- a/sound/aoa/soundbus/soundbus.h
> +++ b/sound/aoa/soundbus/soundbus.h
> @@ -184,10 +184,10 @@ struct soundbus_driver {
>
> /* we don't implement any matching at all */
>
> - int (*probe)(struct soundbus_dev* dev);
> - int (*remove)(struct soundbus_dev* dev);
> + int (*probe)(struct soundbus_dev *dev);
Why change this line too?
thanks,
greg k-h
Re: [PATCH 5/6] ac97: make remove callback of ac97 driver void returned
On Mon, Dec 05, 2022 at 11:36:43PM +0800, Dawei Li wrote:
> Since commit fc7a6209d571 ("bus: Make remove callback return
> void") forces bus_type::remove be void-returned, it doesn't
> make much sense for any bus based driver implementing remove
> callbalk to return non-void to its caller.
>
> This change is for ac97 bus based drivers.
>
> Signed-off-by: Dawei Li
> ---
> drivers/mfd/wm97xx-core.c | 4 +---
> include/sound/ac97/codec.h | 6 +++---
> sound/ac97/bus.c | 5 ++---
> 3 files changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mfd/wm97xx-core.c b/drivers/mfd/wm97xx-core.c
> index 9a2331eb1bfa..663acbb1854c 100644
> --- a/drivers/mfd/wm97xx-core.c
> +++ b/drivers/mfd/wm97xx-core.c
> @@ -319,13 +319,11 @@ static int wm97xx_ac97_probe(struct ac97_codec_device
> *adev)
> return ret;
> }
>
> -static int wm97xx_ac97_remove(struct ac97_codec_device *adev)
> +static void wm97xx_ac97_remove(struct ac97_codec_device *adev)
> {
> struct wm97xx_priv *wm97xx = ac97_get_drvdata(adev);
>
> snd_ac97_compat_release(wm97xx->ac97);
> -
> - return 0;
> }
>
> static const struct ac97_id wm97xx_ac97_ids[] = {
> diff --git a/include/sound/ac97/codec.h b/include/sound/ac97/codec.h
> index 9792d25fa369..a26e9e0082f6 100644
> --- a/include/sound/ac97/codec.h
> +++ b/include/sound/ac97/codec.h
> @@ -62,9 +62,9 @@ struct ac97_codec_device {
> */
> struct ac97_codec_driver {
> struct device_driverdriver;
> - int (*probe)(struct ac97_codec_device *);
> - int (*remove)(struct ac97_codec_device *);
> - void(*shutdown)(struct ac97_codec_device *);
> + int (*probe)(struct ac97_codec_device *dev);
Why did you change this line?
> + void(*remove)(struct ac97_codec_device *dev);
> + void(*shutdown)(struct ac97_codec_device *dev);
And this line?
Don't change things that you don't describe in your changelog and that
are not needed for your change.
thanks,
greg k-h
Re: [PATCH 4/6] xen: make remove callback of xen driver void returned
On Mon, Dec 05, 2022 at 11:36:42PM +0800, Dawei Li wrote:
> Since commit fc7a6209d571 ("bus: Make remove callback return
> void") forces bus_type::remove be void-returned, it doesn't
> make much sense for any bus based driver implementing remove
> callbalk to return non-void to its caller.
Please wrap changelogs at 72 columns.
And this should go through the maintainers of the Xen bus code, not me,
right?
And why wasn't this attached to the 0/6 email properly? Did you use
different tools? If so, our tools can't find the link to keep them in
sync either :(
thanks,
greg k-h
Re: [PATCH 0/6] Make remove() of any bus based driver void returned
On Mon, Dec 05, 2022 at 11:36:38PM +0800, Dawei Li wrote:
> For bus-based driver, device removal is implemented as:
> device_remove() => bus->remove() => driver->remove()
>
> Driver core needs no feedback from bus driver about the result of
> remove callback. In which case, commit fc7a6209d571 ("bus: Make
> remove callback return void") forces bus_type::remove be void-returned.
>
> Now we have the situation that both 1st & 2nd part of calling chain
> are void returned, so it does not make much sense for the last one
> (driver->remove) to return non-void to its caller.
>
> So the basic idea behind this patchset is making remove() callback of
> any bus-based driver to be void returned.
>
> This patchset includes changes for drivers below:
> 1. hyperv
> 2. macio
> 3. apr
> 4. xen
> 5. ac87
> 6. soundbus
Then that should be 6 different patchsets going to 6 different
subsystems. No need to make this seems like a unified set of patches at
all.
> Q: Why not platform drivers?
> A: Too many of them.(maybe 4K+)
That will have to be done eventually, right?
thanks,
greg k-h
Re: [PATCH] powerpc/boot: Convert more files to use SPDX tags
On Fri, Aug 19, 2022 at 09:04:30PM +1000, Michael Ellerman wrote:
> These files are all plain GPL 2.0, with a second sentence about being
> licensed as-is.
>
> Similar to the rule in commit 577b61cee5b2 ("treewide: Replace GPLv2
> boilerplate/reference with SPDX - gpl-2.0_398.RULE").
>
> Signed-off-by: Michael Ellerman
> ---
> arch/powerpc/boot/44x.h | 5 +
> arch/powerpc/boot/4xx.h | 5 +
> arch/powerpc/boot/ops.h | 6 ++
> arch/powerpc/boot/serial.c | 6 ++
> arch/powerpc/boot/simple_alloc.c | 6 ++
> 5 files changed, 8 insertions(+), 20 deletions(-)
Reviewed-by: Greg Kroah-Hartman
Do you want this to go through the SPDX tree, or will you route it
through the normal ppc tree? Either is fine with me, just let me know
if you want me to take it in the SPDX tree.
thanks,
greg k-h
Re: [PATCH] [backport for 4.14] powerpc/ptdump: Fix display of RW pages on FSL_BOOK3E
On Tue, Aug 16, 2022 at 10:45:29AM +0200, Christophe Leroy wrote:
> [ Upstream commit dd8de84b57b02ba9c1fe530a6d916c0853f136bd ]
>
> On FSL_BOOK3E, _PAGE_RW is defined with two bits, one for user and one
> for supervisor. As soon as one of the two bits is set, the page has
> to be display as RW. But the way it is implemented today requires both
> bits to be set in order to display it as RW.
>
> Instead of display RW when _PAGE_RW bits are set and R otherwise,
> reverse the logic and display R when _PAGE_RW bits are all 0 and
> RW otherwise.
>
> This change has no impact on other platforms as _PAGE_RW is a single
> bit on all of them.
>
> Fixes: 8eb07b187000 ("powerpc/mm: Dump linux pagetables")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Christophe Leroy
> Signed-off-by: Michael Ellerman
> Link:
> https://lore.kernel.org/r/0c33b96317811edf691e81698aaee8fa45ec3449.1656427391.git.christophe.le...@csgroup.eu
> ---
> arch/powerpc/mm/dump_linuxpagetables.c | 13 -
> 1 file changed, 4 insertions(+), 9 deletions(-)
>
Now queued up.
greg k-h
Re: [PATCH] macintosh:fix oob read in do_adb_query function
On Wed, Jul 13, 2022 at 11:37:34PM +0800, Ning Qiang wrote: > In do_adb_query function of drivers/macintosh/adb.c, req->data is copy > form userland. the parameter "req->data[2]" is Missing check, the > array size of adb_handler[] is 16, so "adb_handler[ > req->data[2]].original_address" and "adb_handler[ > req->data[2]].handler_id" will lead to oob read. > > Signed-off-by: Ning Qiang Cc: stable Reviewed-by: Greg Kroah-Hartman
Re: [PATCH] macintosh:fix oob read in do_adb_query function
On Wed, Jul 13, 2022 at 09:40:37PM +0800, NAME wrote:
> From: sohu0106
For obvious reasons, we need a real name here, and in the signed-off-by
line.
> In do_adb_query function of drivers/macintosh/adb.c,
> req->data is copy form userland. The parameter
> "req->data[2]" is Missing check, the array size of
> adb_handler[] is 16, so "adb_handler[req->data[2]].
> original_address" and "adb_handler[req->data[2]].
> handler_id" will lead to oob read.
You can use all 72 columns, if you want to re-wrap these lines when you
resend.
>
> Signed-off-by: sohu0106
> ---
> drivers/macintosh/adb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/macintosh/adb.c b/drivers/macintosh/adb.c
> index 439fab4eaa85..1bbb9ca08d40 100644
> --- a/drivers/macintosh/adb.c
> +++ b/drivers/macintosh/adb.c
> @@ -647,7 +647,7 @@ do_adb_query(struct adb_request *req)
>
> switch(req->data[1]) {
> case ADB_QUERY_GETDEVINFO:
> - if (req->nbytes < 3)
> + if (req->nbytes < 3 || req->data[2] >= 16)
Shouldn't 16 be the array size instead of having this hard coded to a
magic number?
Something like "sizeof(adb_handler) / sizeof(struct adb_handler)"?
Maybe not, that's messy, your choice.
thanks,
greg k-h
Re: [linux-next:master] BUILD REGRESSION 088b9c375534d905a4d337c78db3b3bfbb52c4a0
org, net...@vger.kernel.org, k...@vger.kernel.org, da...@lists.linux.dev, linux...@kvack.org, accessrunner-gene...@lists.sourceforge.net, linux1394-de...@lists.sourceforge.net, linux-l...@vger.kernel.org, rds-de...@oss.oracle.com, linux-...@vger.kernel.org, d...@vger.kernel.org, intel-wired-...@lists.osuosl.org, linux-ser...@vger.kernel.org, devicet...@vger.kernel.org, linux-...@lists.01.org, osmocom-net-g...@lists.osmocom.org, appar...@lists.ubuntu.com, linux-r...@vger.kernel.org, linux-bca...@vger.kernel.org, linux-media...@lists.infradead.org, linux-te...@vger.kernel.org, linux-s...@vger.kernel.org, patc...@opensource.cirrus.com, linux-unio...@vger.kernel.org, linux-blueto...@vger.kernel.org, n...@lists.linux.dev, tipc-discuss...@lists.sourceforge.net, linuxppc-dev@lists.ozlabs.org, linux-bt...@vger.kernel.org Errors-To: linuxppc-dev-bounces+archive=mail-archive@lists.ozlabs.org Sender: "Linuxppc-dev" On Thu, Jul 07, 2022 at 02:56:34PM +0800, kernel test robot wrote: > tree/branch: > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master > branch HEAD: 088b9c375534d905a4d337c78db3b3bfbb52c4a0 Add linux-next > specific files for 20220706 > > Error/Warning reports: > > https://lore.kernel.org/linux-doc/202207070644.x48xoovs-...@intel.com > > Error/Warning: (recently discovered and may have been fixed) > > Documentation/arm/google/chromebook-boot-flow.rst: WARNING: document isn't > included in any toctree > arm-linux-gnueabi-ld: dc_dmub_srv.c:(.text+0x1108): undefined reference to > `__aeabi_ddiv' > arm-linux-gnueabi-ld: dc_dmub_srv.c:(.text+0x1124): undefined reference to > `__aeabi_ui2d' > arm-linux-gnueabi-ld: dc_dmub_srv.c:(.text+0x1164): undefined reference to > `__aeabi_dmul' > arm-linux-gnueabi-ld: dc_dmub_srv.c:(.text+0x1170): undefined reference to > `__aeabi_dadd' > arm-linux-gnueabi-ld: dc_dmub_srv.c:(.text+0x1180): undefined reference to > `__aeabi_dsub' > arm-linux-gnueabi-ld: dc_dmub_srv.c:(.text+0x1190): undefined reference to > `__aeabi_d2uiz' > arm-linux-gnueabi-ld: dc_dmub_srv.c:(.text+0x162c): undefined reference to > `__aeabi_d2iz' > arm-linux-gnueabi-ld: dc_dmub_srv.c:(.text+0x16b0): undefined reference to > `__aeabi_i2d' > dc_dmub_srv.c:(.text+0x10f8): undefined reference to `__aeabi_ui2d' > dc_dmub_srv.c:(.text+0x464): undefined reference to `__floatunsidf' > dc_dmub_srv.c:(.text.dc_dmub_setup_subvp_dmub_command+0x33c): undefined > reference to `__floatunsidf' > drivers/pci/endpoint/functions/pci-epf-vntb.c:975:5: warning: no previous > prototype for 'pci_read' [-Wmissing-prototypes] > drivers/pci/endpoint/functions/pci-epf-vntb.c:984:5: warning: no previous > prototype for 'pci_write' [-Wmissing-prototypes] > drivers/vfio/vfio_iommu_type1.c:2141:35: warning: cast to smaller integer > type 'enum iommu_cap' from 'void *' [-Wvoid-pointer-to-enum-cast] > mips-linux-ld: dc_dmub_srv.c:(.text.dc_dmub_setup_subvp_dmub_command+0x34c): > undefined reference to `__floatunsidf' > mips-linux-ld: dc_dmub_srv.c:(.text.dc_dmub_setup_subvp_dmub_command+0x378): > undefined reference to `__divdf3' > mips-linux-ld: dc_dmub_srv.c:(.text.dc_dmub_setup_subvp_dmub_command+0x38c): > undefined reference to `__muldf3' > mips-linux-ld: dc_dmub_srv.c:(.text.dc_dmub_setup_subvp_dmub_command+0x3a0): > undefined reference to `__adddf3' > mips-linux-ld: dc_dmub_srv.c:(.text.dc_dmub_setup_subvp_dmub_command+0x3b4): > undefined reference to `__subdf3' > mips-linux-ld: dc_dmub_srv.c:(.text.dc_dmub_setup_subvp_dmub_command+0x3d4): > undefined reference to `__fixunsdfsi' > mips-linux-ld: dc_dmub_srv.c:(.text.dc_dmub_setup_subvp_dmub_command+0x750): > undefined reference to `__fixdfsi' > mips-linux-ld: dc_dmub_srv.c:(.text.dc_dmub_setup_subvp_dmub_command+0x7c0): > undefined reference to `__floatsidf' > powerpc-linux-ld: drivers/pci/endpoint/functions/pci-epf-vntb.c:174: > undefined reference to `ntb_link_event' > xtensa-linux-ld: dc_dmub_srv.c:(.text+0x468): undefined reference to > `__divdf3' > xtensa-linux-ld: dc_dmub_srv.c:(.text+0x46c): undefined reference to > `__muldf3' > xtensa-linux-ld: dc_dmub_srv.c:(.text+0x470): undefined reference to > `__adddf3' > xtensa-linux-ld: dc_dmub_srv.c:(.text+0x474): undefined reference to > `__subdf3' > xtensa-linux-ld: dc_dmub_srv.c:(.text+0x478): undefined reference to > `__fixunsdfsi' > xtensa-linux-ld: dc_dmub_srv.c:(.text+0x47c): undefined reference to > `__fixdfsi' > xtensa-linux-ld: dc_dmub_srv.c:(.text+0x480): undefined reference to > `__floatsidf' > xtensa-linux-ld: dc_dmub_srv.c:(.text+0x60c): undefined reference to > `__floatunsidf' > > Unverified Error/Warning (likely false positive, please contact us if > interested): > > arch/x86/events/core.c:2114 init_hw_perf_events() warn: missing error code > 'err' > drivers/android/binder.c:1481:19-23: ERROR: from is NULL but dereferenced. > drivers/android/binder.c:2920:29-33: ERROR: target_thread is NULL but > dereferenced. >
Re: [PATCH] cxl: drop unexpected word "the" in the comments
On Thu, Jun 30, 2022 at 11:10:13AM +1000, Andrew Donnellan wrote: > On Wed, 2022-06-29 at 22:15 +1000, Michael Ellerman wrote: > > On Tue, 21 Jun 2022 20:53:21 +0800, Jiang Jian wrote: > > > there is an unexpected word "the" in the comments that need to be > > > dropped > > > > > > file: drivers/misc/cxl/cxl.h > > > line: 1107 > > > +/* check if the given pci_dev is on the the cxl vphb bus */ > > > changed to > > > +/* check if the given pci_dev is on the cxl vphb bus */ > > > > > > [...] > > > > Applied to powerpc/next. > > > > [1/1] cxl: drop unexpected word "the" in the comments > > > > https://git.kernel.org/powerpc/c/882c835b71e22ca82361dab3b60b85b557abd72f > > I believe Greg's already merged this in char-misc... git can handle merges like this without any problems :)
Re: [PATCH v4.9] kexec_file: drop weak attribute from arch_kexec_apply_relocations[_add]
On Tue, Jun 28, 2022 at 09:12:48PM +0530, Naveen N. Rao wrote:
> commit 3e35142ef99fe6b4fe5d834ad43ee13cca10a2dc upstream.
>
> Since commit d1bcae833b32f1 ("ELF: Don't generate unused section
> symbols") [1], binutils (v2.36+) started dropping section symbols that
> it thought were unused. This isn't an issue in general, but with
> kexec_file.c, gcc is placing kexec_arch_apply_relocations[_add] into a
> separate .text.unlikely section and the section symbol ".text.unlikely"
> is being dropped. Due to this, recordmcount is unable to find a non-weak
> symbol in .text.unlikely to generate a relocation record against.
>
> Address this by dropping the weak attribute from these functions.
> Instead, follow the existing pattern of having architectures #define the
> name of the function they want to override in their headers.
>
> [1] https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=d1bcae833b32f1
>
> [a...@linux-foundation.org: arch/s390/include/asm/kexec.h needs
> linux/module.h]
> Link:
> https://lkml.kernel.org/r/20220519091237.676736-1-naveen.n@linux.vnet.ibm.com
> Signed-off-by: Michael Ellerman
> Signed-off-by: Naveen N. Rao
> Cc: "Eric W. Biederman"
> Cc:
> Signed-off-by: Andrew Morton
> ---
> arch/x86/include/asm/kexec.h | 7 +++
> include/linux/kexec.h| 26 ++
> kernel/kexec_file.c | 18 --
> 3 files changed, 29 insertions(+), 22 deletions(-)
>
All now queued up, thanks.
greg k-h
Re: [PATCH v5.18] powerpc/ftrace: Remove ftrace init tramp once kernel init is complete
On Mon, Jun 27, 2022 at 11:09:29PM +0530, Naveen N. Rao wrote:
> commit 84ade0a6655bee803d176525ef457175cbf4df22 upstream.
>
> Stop using the ftrace trampoline for init section once kernel init is
> complete.
>
> Fixes: 67361cf8071286 ("powerpc/ftrace: Handle large kernel configs")
> Cc: sta...@vger.kernel.org # v4.20+
> Signed-off-by: Naveen N. Rao
> Signed-off-by: Michael Ellerman
> Link:
> https://lore.kernel.org/r/20220516071422.463738-1-naveen.n@linux.vnet.ibm.com
> ---
> arch/powerpc/include/asm/ftrace.h | 4 +++-
> arch/powerpc/kernel/trace/ftrace.c | 15 ---
> arch/powerpc/mm/mem.c | 2 ++
> 3 files changed, 17 insertions(+), 4 deletions(-)
All now queued up, thanks.
greg k-h
Re: [PATCH RFC] drivers/usb/ehci-fsl: Fix interrupt setup in host mode.
On Tue, Jun 21, 2022 at 11:09:41PM +0100, Darren Stevens wrote:
> In patch a1a2b7125e1079 (Drop static setup of IRQ resource from DT
> core) we stopped platform_get_resource() from returning the IRQ, as all
> drivers were supposed to have switched to platform_get_irq()
> Unfortunately the Freescale EHCI driver in host mode got missed. Fix
> it. Also fix allocation of resources to work with current kernel.
>
> Fixes:a1a2b7125e1079 (Drop static setup of IRQ resource from DT core)
Nit, please put a space after the :.
Also not that many characters are needed, as you can see in our
documentation, this is the proper format:
Fixes: a1a2b7125e10 ("of/platform: Drop static setup of IRQ resource from DT
core")
> Reported-by Christian Zigotzky
> Signed-off-by Darren Stevens
> ---
> Tested on AmigaOne X5000/20 and X5000/40 not sure if this is entirely
> correct fix though. Contains code by Rob Herring (in fsl-mph-dr-of.c)
>
> diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
> index 385be30..d0bf7fb 100644
> --- a/drivers/usb/host/ehci-fsl.c
> +++ b/drivers/usb/host/ehci-fsl.c
> @@ -23,6 +23,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include "ehci.h"
> @@ -46,9 +47,10 @@ static struct hc_driver __read_mostly
> fsl_ehci_hc_driver; */
> static int fsl_ehci_drv_probe(struct platform_device *pdev)
> {
> + struct device_node *dn = pdev->dev.of_node;
> struct fsl_usb2_platform_data *pdata;
> struct usb_hcd *hcd;
> - struct resource *res;
> + struct resource res;
> int irq;
> int retval;
> u32 tmp;
> @@ -76,14 +78,10 @@ static int fsl_ehci_drv_probe(struct
> platform_device *pdev) return -ENODEV;
> }
>
> - res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> - if (!res) {
> - dev_err(>dev,
> - "Found HC with no IRQ. Check %s setup!\n",
> - dev_name(>dev));
> - return -ENODEV;
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + return irq;
> }
Did you run checkpatch on this? Coding style is not correct :(
thanks,
greg k-h
Re: [PATCH 2/2] uio:powerpc:mpc85xx: l2-cache-sram uio driver implementation
On Tue, Jun 14, 2022 at 07:53:46AM +, Wenhu Wang wrote:
> >> >> +
> >> >> +struct mpc85xx_l2ctlr {
> >> >> + u32 ctl;/* 0x000 - L2 control */
> >> >
> >> >What is the endian of these u32 values? You map them directly to
> >> >memory, so they must be specified some way, right? Please make it
> >> >obvious what they are.
> >> >
> >>
> >> Surely, the values should be u32 here, modified in v2
> >> The controller info could be found in
> >> "QorIQ�� P2020 Integrated Processor Reference Manual"
> >> "Chapter 6 L2 Look-Aside Cache/SRAM"
> >> See: http://m4udit.dinauz.org/P2020RM_rev0.pdf
> >
> >That's not the answer to my question :)
> >
> >These are big-endian, right? Please mark them as such and access them
> >properly with the correct functions.
>
> Yes, they are big-edian.
> Does it work to add comments(about order and access functions) for the
> structure ahead of it��
> And appending like "_be", "_access_be" or "_big_endian"? (struct
> mpc85xx_l2ctlr_be {};
No, not comments, these should be of the type __be32, right?
thanks,
greg k-h
Re: 回复: [PATCH 2/2] uio:powerpc:mpc85xx: l2-cache-sram uio driver implementation
On Tue, Jun 14, 2022 at 06:09:35AM +, Wenhu Wang wrote: > Hi Greg, thanks for the comments. > The questions are replied specifically below. > I have figured out and tested the patch v2, which is to be posted later. > >发件人: Greg KH > >发送时间: 2022年6月9日 21:17 > >收件人: Wang Wenhu > >抄送: christophe.le...@csgroup.eu ; > >m...@ellerman.id.au ; linuxppc-dev@lists.ozlabs.org > >; linux-ker...@vger.kernel.org > > > >主题: Re: [PATCH 2/2] uio:powerpc:mpc85xx: l2-cache-sram uio driver > >implementation > > > >On Thu, Jun 09, 2022 at 03:28:55AM -0700, Wang Wenhu wrote: > >> The l2-cache could be optionally configured as SRAM partly or fully. > >> Users can make use of it as a block of independent memory that offers > >> special usage, such as for debuging or other cratical status info > >> storage which keeps consistently even when the whole system crashed. > >> > >> The hardware related configuration process utilized the work of the > >> earlier implementation, which has been removed now. > >> See: > >> https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git/commit/?id=dc21ed2aef4150fc2fcf58227a4ff24502015c03 > >> > >> Cc: Christophe Leroy > >> Signed-off-by: Wang Wenhu > >> --- > >> drivers/uio/Kconfig | 10 + > >> drivers/uio/Makefile | 1 + > >> drivers/uio/uio_fsl_85xx_cache_sram.c | 286 ++ > >> 3 files changed, 297 insertions(+) > >> create mode 100644 drivers/uio/uio_fsl_85xx_cache_sram.c > >> > >> diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig > >> index 2e16c5338e5b..9199ced03880 100644 > >> --- a/drivers/uio/Kconfig > >> +++ b/drivers/uio/Kconfig > >> @@ -105,6 +105,16 @@ config UIO_NETX > >> To compile this driver as a module, choose M here; the module > >> will be called uio_netx. > >> > >> +config UIO_FSL_85XX_CACHE_SRAM > >> + tristate "Freescale 85xx Cache-Sram driver" > >> + depends on FSL_SOC_BOOKE && PPC32 > >> + help > >> + Generic driver for accessing the Cache-Sram form user level. This > >> + is extremely helpful for some user-space applications that require > >> + high performance memory accesses. > >> + > >> + If you don't know what to do here, say N. > > > >Module name information? > > > > More detailed and clearer info in v2 > > >> + > >> config UIO_FSL_ELBC_GPCM > >> tristate "eLBC/GPCM driver" > >> depends on FSL_LBC > >> diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile > >> index f2f416a14228..1ba07d92a1b1 100644 > >> --- a/drivers/uio/Makefile > >> +++ b/drivers/uio/Makefile > >> @@ -12,3 +12,4 @@ obj-$(CONFIG_UIO_MF624) += uio_mf624.o > >> obj-$(CONFIG_UIO_FSL_ELBC_GPCM) += uio_fsl_elbc_gpcm.o > >> obj-$(CONFIG_UIO_HV_GENERIC) += uio_hv_generic.o > >> obj-$(CONFIG_UIO_DFL) += uio_dfl.o > >> +obj-$(CONFIG_UIO_FSL_85XX_CACHE_SRAM) += uio_fsl_85xx_cache_sram.o > >> diff --git a/drivers/uio/uio_fsl_85xx_cache_sram.c > >> b/drivers/uio/uio_fsl_85xx_cache_sram.c > >> new file mode 100644 > >> index ..d363f9d2b179 > >> --- /dev/null > >> +++ b/drivers/uio/uio_fsl_85xx_cache_sram.c > >> @@ -0,0 +1,286 @@ > >> +// SPDX-License-Identifier: GPL-2.0 > >> +/* > >> + * Copyright (C) 2022 Wang Wenhu > >> + * All rights reserved. > >> + */ > >> + > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> + > >> +#define DRIVER_NAME "uio_mpc85xx_cache_sram" > >> +#define UIO_INFO_VER "0.0.1" > >> +#define UIO_NAME "uio_cache_sram" > >> + > >> +#define L2CR_L2FI 0x4000 /* L2 flash > >> invalidate */ > >> +#define L2CR_L2IO 0x0020 /* L2 > >> instruction only */ > >> +#define L2CR_SRAM_ZERO 0x /* L2SRAM > >> zero size */ > >> +#define L2CR_SRAM_FULL 0x0001 /* L2SRAM > >> full size */ > >> +#define L2CR_SRAM_HALF 0x0002 /* L2SRAM > >> half siz
Re: [PATCH 2/2] uio:powerpc:mpc85xx: l2-cache-sram uio driver implementation
On Thu, Jun 09, 2022 at 03:28:55AM -0700, Wang Wenhu wrote:
> The l2-cache could be optionally configured as SRAM partly or fully.
> Users can make use of it as a block of independent memory that offers
> special usage, such as for debuging or other cratical status info
> storage which keeps consistently even when the whole system crashed.
>
> The hardware related configuration process utilized the work of the
> earlier implementation, which has been removed now.
> See:
> https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git/commit/?id=dc21ed2aef4150fc2fcf58227a4ff24502015c03
>
> Cc: Christophe Leroy
> Signed-off-by: Wang Wenhu
> ---
> drivers/uio/Kconfig | 10 +
> drivers/uio/Makefile | 1 +
> drivers/uio/uio_fsl_85xx_cache_sram.c | 286 ++
> 3 files changed, 297 insertions(+)
> create mode 100644 drivers/uio/uio_fsl_85xx_cache_sram.c
>
> diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
> index 2e16c5338e5b..9199ced03880 100644
> --- a/drivers/uio/Kconfig
> +++ b/drivers/uio/Kconfig
> @@ -105,6 +105,16 @@ config UIO_NETX
> To compile this driver as a module, choose M here; the module
> will be called uio_netx.
>
> +config UIO_FSL_85XX_CACHE_SRAM
> + tristate "Freescale 85xx Cache-Sram driver"
> + depends on FSL_SOC_BOOKE && PPC32
> + help
> + Generic driver for accessing the Cache-Sram form user level. This
> + is extremely helpful for some user-space applications that require
> + high performance memory accesses.
> +
> + If you don't know what to do here, say N.
Module name information?
> +
> config UIO_FSL_ELBC_GPCM
> tristate "eLBC/GPCM driver"
> depends on FSL_LBC
> diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
> index f2f416a14228..1ba07d92a1b1 100644
> --- a/drivers/uio/Makefile
> +++ b/drivers/uio/Makefile
> @@ -12,3 +12,4 @@ obj-$(CONFIG_UIO_MF624) += uio_mf624.o
> obj-$(CONFIG_UIO_FSL_ELBC_GPCM) += uio_fsl_elbc_gpcm.o
> obj-$(CONFIG_UIO_HV_GENERIC) += uio_hv_generic.o
> obj-$(CONFIG_UIO_DFL)+= uio_dfl.o
> +obj-$(CONFIG_UIO_FSL_85XX_CACHE_SRAM)+= uio_fsl_85xx_cache_sram.o
> diff --git a/drivers/uio/uio_fsl_85xx_cache_sram.c
> b/drivers/uio/uio_fsl_85xx_cache_sram.c
> new file mode 100644
> index ..d363f9d2b179
> --- /dev/null
> +++ b/drivers/uio/uio_fsl_85xx_cache_sram.c
> @@ -0,0 +1,286 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2022 Wang Wenhu
> + * All rights reserved.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define DRIVER_NAME "uio_mpc85xx_cache_sram"
> +#define UIO_INFO_VER "0.0.1"
> +#define UIO_NAME "uio_cache_sram"
> +
> +#define L2CR_L2FI0x4000 /* L2 flash
> invalidate */
> +#define L2CR_L2IO0x0020 /* L2
> instruction only */
> +#define L2CR_SRAM_ZERO 0x /* L2SRAM zero
> size */
> +#define L2CR_SRAM_FULL 0x0001 /* L2SRAM full
> size */
> +#define L2CR_SRAM_HALF 0x0002 /* L2SRAM half
> size */
> +#define L2CR_SRAM_TWO_HALFS 0x0003 /* L2SRAM two half
> sizes */
> +#define L2CR_SRAM_QUART 0x0004 /* L2SRAM one
> quarter size */
> +#define L2CR_SRAM_TWO_QUARTS 0x0005 /* L2SRAM two quarter size */
> +#define L2CR_SRAM_EIGHTH 0x0006 /* L2SRAM one eighth
> size */
> +#define L2CR_SRAM_TWO_EIGHTH 0x0007 /* L2SRAM two eighth size */
> +
> +#define L2SRAM_OPTIMAL_SZ_SHIFT 0x0003 /* Optimum size for
> L2SRAM */
> +
> +#define L2SRAM_BAR_MSK_LO18 0xC000 /* Lower 18 bits */
> +#define L2SRAM_BARE_MSK_HI4 0x000F /* Upper 4 bits */
> +
> +enum cache_sram_lock_ways {
> + LOCK_WAYS_ZERO,
> + LOCK_WAYS_EIGHTH,
> + LOCK_WAYS_TWO_EIGHTH,
Why not have values for these?
> + LOCK_WAYS_HALF = 4,
> + LOCK_WAYS_FULL = 8,
> +};
> +
> +struct mpc85xx_l2ctlr {
> + u32 ctl;/* 0x000 - L2 control */
What is the endian of these u32 values? You map them directly to
memory, so they must be specified some way, right? Please make it
obvious what they are.
> + u8 res1[0xC];
> + u32 ewar0; /* 0x010 - External write address 0 */
> + u32 ewarea0;/* 0x014 - External write address extended 0 */
> + u32 ewcr0; /* 0x018 - External write ctrl */
> + u8 res2[4];
> + u32 ewar1; /* 0x020 - External write address 1 */
> + u32 ewarea1;/* 0x024 - External write address extended 1 */
> + u32 ewcr1; /* 0x028 - External write ctrl 1 */
> + u8 res3[4];
> + u32 ewar2; /* 0x030 - External write address 2 */
> + u32
Re: [PATCH for v5.15] perf symbol: Remove arch__symbols__fixup_end()
On Mon, May 02, 2022 at 05:37:36PM -0700, Namhyung Kim wrote: > Now the generic code can handle kallsyms fixup properly so no need to > keep the arch-functions anymore. > > Signed-off-by: Namhyung Kim > Acked-by: Ian Rogers > Cc: Heiko Carstens > Cc: Ingo Molnar > Cc: Jiri Olsa > Cc: John Garry > Cc: Leo Yan > Cc: Mark Rutland > Cc: Masami Hiramatsu > Cc: Mathieu Poirier > Cc: Michael Ellerman > Cc: Michael Petlan > Cc: Peter Zijlstra > Cc: Song Liu > Cc: Will Deacon > Cc: linux-s...@vger.kernel.org > Cc: linuxppc-dev@lists.ozlabs.org > Link: https://lore.kernel.org/r/20220416004048.1514900-4-namhy...@kernel.org > Signed-off-by: Arnaldo Carvalho de Melo > --- > The original commit id is a5d20d42a2f2dc2b2f9e9361912062732414090d Both now queued up, thanks. greg k-h
Re: [PATCH v4.19 0/2] Custom backports for powerpc SLB issues
On Thu, Apr 28, 2022 at 10:41:48PM +1000, Michael Ellerman wrote: > Hi Greg, > > Here are two custom backports to v4.19 for some powerpc issues we've > discovered. > Both were fixed upstream as part of a large non-backportable rewrite. Other > stable > kernel versions are not affected. > > cheers > > Michael Ellerman (1): > powerpc/64s: Unmerge EX_LR and EX_DAR > > Nicholas Piggin (1): > powerpc/64/interrupt: Temporarily save PPR on stack to fix register > corruption due to SLB miss > > arch/powerpc/include/asm/exception-64s.h | 37 ++-- > 1 file changed, 22 insertions(+), 15 deletions(-) > > -- > 2.35.1 > Both now queued up, thanks. greg k-h
Re: [PATCH v5 05/10] serial: termbits: ADDRB to indicate 9th bit addressing mode
On Tue, Apr 26, 2022 at 05:01:31PM +0300, Ilpo Järvinen wrote:
> One additional thing below I missed on the first read.
>
> On Tue, 26 Apr 2022, Ilpo Järvinen wrote:
> > On Tue, 26 Apr 2022, Greg KH wrote:
> >
> > > On Tue, Apr 26, 2022 at 03:24:43PM +0300, Ilpo Järvinen wrote:
> > > > Add ADDRB to termbits to indicate 9th bit addressing mode. This change
> > > > is necessary for supporting devices with RS485 multipoint addressing
> > > > [*]. A later patch in the patch series adds support for Synopsys
> > > > Designware UART capable for 9th bit addressing mode. In this mode, 9th
> > > > bit is used to indicate an address (byte) within the communication
> > > > line. The 9th bit addressing mode is selected using ADDRB introduced by
> > > > an earlier patch.
> > > >
> > > > [*] Technically, RS485 is just an electronic spec and does not itself
> > > > specify the 9th bit addressing mode but 9th bit seems at least
> > > > "semi-standard" way to do addressing with RS485.
> > > >
> > > > Cc: linux-...@vger.kernel.org
> > > > Cc: Ivan Kokshaysky
> > > > Cc: Matt Turner
> > > > Cc: linux-al...@vger.kernel.org
> > > > Cc: Thomas Bogendoerfer
> > > > Cc: linux-m...@vger.kernel.org
> > > > Cc: "James E.J. Bottomley"
> > > > Cc: Helge Deller
> > > > Cc: linux-par...@vger.kernel.org
> > > > Cc: Michael Ellerman
> > > > Cc: Benjamin Herrenschmidt
> > > > Cc: Paul Mackerras
> > > > Cc: linuxppc-dev@lists.ozlabs.org
> > > > Cc: "David S. Miller"
> > > > Cc: sparcli...@vger.kernel.org
> > > > Cc: Arnd Bergmann
> > > > Cc: linux-a...@vger.kernel.org
> > > > Cc: linux-...@vger.kernel.org
> > > > Signed-off-by: Ilpo Järvinen
> > > > ---
>
> > > > #define CMSPAR0100 /* mark or space (stick)
> > > > parity */
> > > > #define CRTSCTS 0200 /* flow control */
> > > >
> > > > diff --git a/arch/powerpc/include/uapi/asm/termbits.h
> > > > b/arch/powerpc/include/uapi/asm/termbits.h
> > > > index ed18bc61f63d..c6a033732f39 100644
> > > > --- a/arch/powerpc/include/uapi/asm/termbits.h
> > > > +++ b/arch/powerpc/include/uapi/asm/termbits.h
> > > > @@ -171,6 +171,7 @@ struct ktermios {
> > > > #define HUPCL 0004
> > > >
> > > > #define CLOCAL 0010
> > > > +#define ADDRB 0040/* address bit */
> > > > #define CMSPAR 0100 /* mark or space (stick) parity
> > > > */
> > > > #define CRTSCTS 0200 /* flow control */
> > > >
> > > > diff --git a/arch/sparc/include/uapi/asm/termbits.h
> > > > b/arch/sparc/include/uapi/asm/termbits.h
> > > > index ce5ad5d0f105..5eb1d547b5c4 100644
> > > > --- a/arch/sparc/include/uapi/asm/termbits.h
> > > > +++ b/arch/sparc/include/uapi/asm/termbits.h
> > > > @@ -201,6 +201,7 @@ struct ktermios {
> > > > #define B350 0x1012
> > > > #define B400 0x1013 */
> > > > #define CIBAUD 0x100f /* input baud rate (not used) */
> > > > +#define ADDRB0x2000 /* address bit */
> > > > #define CMSPAR 0x4000 /* mark or space (stick) parity */
> > > > #define CRTSCTS 0x8000 /* flow control */
> > >
> > > Why all the different values? Can't we pick one and use it for all
> > > arches? Having these be different in different arches and userspace
> > > should not be a thing for new fields, right?
>
> ADDRB value is the same for all archs (it's just this octal vs hex
> notation I've followed as per the nearby defines within the same file
> which makes them look different).
>
> Should I perhaps add to my cleanup list conversion of all those octal ones
> to hex?
Argh, yes, please, let's do that now, I totally missed that. Will let
us see how to unify them as well.
thanks,
greg k-h
Re: [PATCH v5 06/10] serial: General support for multipoint addresses
On Tue, Apr 26, 2022 at 04:36:49PM +0300, Ilpo Järvinen wrote:
> On Tue, 26 Apr 2022, Greg KH wrote:
>
> > On Tue, Apr 26, 2022 at 03:24:44PM +0300, Ilpo Järvinen wrote:
> > > Add generic support for serial multipoint addressing. Two new ioctls
> > > are added. TIOCSADDR is used to indicate the destination/receive
> > > address. TIOCGADDR returns the current address in use. The driver
> > > should implement set_addr and get_addr to support addressing mode.
> > >
> > > Adjust ADDRB clearing to happen only if driver does not provide
> > > set_addr (=the driver doesn't support address mode).
> > >
> > > This change is necessary for supporting devices with RS485 multipoint
> > > addressing [*]. A following patch in the patch series adds support for
> > > Synopsys Designware UART capable for 9th bit addressing mode. In this
> > > mode, 9th bit is used to indicate an address (byte) within the
> > > communication line. The 9th bit addressing mode is selected using ADDRB
> > > introduced by the previous patch.
> > >
> > > Transmit addresses / receiver filter are specified by setting the flags
> > > SER_ADDR_DEST and/or SER_ADDR_RECV. When the user supplies the transmit
> > > address, in the 9bit addressing mode it is sent out immediately with
> > > the 9th bit set to 1. After that, the subsequent normal data bytes are
> > > sent with 9th bit as 0 and they are intended to the device with the
> > > given address. It is up to receiver to enforce the filter using
> > > SER_ADDR_RECV. When userspace has supplied the receive address, the
> > > driver is expected to handle the matching of the address and only data
> > > with that address is forwarded to the user. Both SER_ADDR_DEST and
> > > SER_ADDR_RECV can be given at the same time in a single call if the
> > > addresses are the same.
> > >
> > > The user can clear the receive filter with SER_ADDR_RECV_CLEAR.
> > >
> > > [*] Technically, RS485 is just an electronic spec and does not itself
> > > specify the 9th bit addressing mode but 9th bit seems at least
> > > "semi-standard" way to do addressing with RS485.
> > >
> > > Cc: linux-...@vger.kernel.org
> > > Cc: Ivan Kokshaysky
> > > Cc: Matt Turner
> > > Cc: linux-al...@vger.kernel.org
> > > Cc: Thomas Bogendoerfer
> > > Cc: linux-m...@vger.kernel.org
> > > Cc: "James E.J. Bottomley"
> > > Cc: Helge Deller
> > > Cc: linux-par...@vger.kernel.org
> > > Cc: Michael Ellerman
> > > Cc: Benjamin Herrenschmidt
> > > Cc: Paul Mackerras
> > > Cc: linuxppc-dev@lists.ozlabs.org
> > > Cc: Yoshinori Sato
> > > Cc: Rich Felker
> > > Cc: linux...@vger.kernel.org
> > > Cc: "David S. Miller"
> > > Cc: sparcli...@vger.kernel.org
> > > Cc: Chris Zankel
> > > Cc: Max Filippov
> > > Cc: linux-xte...@linux-xtensa.org
> > > Cc: Arnd Bergmann
> > > Cc: linux-a...@vger.kernel.org
> > > Cc: linux-...@vger.kernel.org
> > > Signed-off-by: Ilpo Järvinen
> > > ---
>
> > > diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
> > > index fa6b16e5fdd8..8cb785ea7087 100644
> > > --- a/include/uapi/linux/serial.h
> > > +++ b/include/uapi/linux/serial.h
> > > @@ -149,4 +149,12 @@ struct serial_iso7816 {
> > > __u32 reserved[5];
> > > };
> > >
> > > +struct serial_addr {
> > > + __u32 flags;
> > > +#define SER_ADDR_RECV(1 << 0)
> > > +#define SER_ADDR_RECV_CLEAR (1 << 1)
> > > +#define SER_ADDR_DEST(1 << 2)
> >
> > You never check for invalid flags being sent to the kernel, which means
> > this api can never change in the future to add new flags :(
>
> Ok, so you mean the general level should to check
> if (...->flags & ~(SER_ADDR_FLAGS_ALL))
> return -EINVAL;
> ?
For any new kernel api you always have to ensure that no "extra" flags
or bits are set and reject it otherwise you can never add any more bits
or flags in the future. This should be in the Documentation/ directory
for how to add new ioctls somewhere.
> There's some code in the driver that detects invalid flag combinations
> (in 10/10) but I guess it doesn't satisfies what you're after. It is
> similar to how serial_rs485 flags is handled, that is, clearing flags it
> didn't handle (when it can) and returning -EINVA
Re: [PATCH v5 06/10] serial: General support for multipoint addresses
On Tue, Apr 26, 2022 at 03:24:44PM +0300, Ilpo Järvinen wrote: > Add generic support for serial multipoint addressing. Two new ioctls > are added. TIOCSADDR is used to indicate the destination/receive > address. TIOCGADDR returns the current address in use. The driver > should implement set_addr and get_addr to support addressing mode. > > Adjust ADDRB clearing to happen only if driver does not provide > set_addr (=the driver doesn't support address mode). > > This change is necessary for supporting devices with RS485 multipoint > addressing [*]. A following patch in the patch series adds support for > Synopsys Designware UART capable for 9th bit addressing mode. In this > mode, 9th bit is used to indicate an address (byte) within the > communication line. The 9th bit addressing mode is selected using ADDRB > introduced by the previous patch. > > Transmit addresses / receiver filter are specified by setting the flags > SER_ADDR_DEST and/or SER_ADDR_RECV. When the user supplies the transmit > address, in the 9bit addressing mode it is sent out immediately with > the 9th bit set to 1. After that, the subsequent normal data bytes are > sent with 9th bit as 0 and they are intended to the device with the > given address. It is up to receiver to enforce the filter using > SER_ADDR_RECV. When userspace has supplied the receive address, the > driver is expected to handle the matching of the address and only data > with that address is forwarded to the user. Both SER_ADDR_DEST and > SER_ADDR_RECV can be given at the same time in a single call if the > addresses are the same. > > The user can clear the receive filter with SER_ADDR_RECV_CLEAR. > > [*] Technically, RS485 is just an electronic spec and does not itself > specify the 9th bit addressing mode but 9th bit seems at least > "semi-standard" way to do addressing with RS485. > > Cc: linux-...@vger.kernel.org > Cc: Ivan Kokshaysky > Cc: Matt Turner > Cc: linux-al...@vger.kernel.org > Cc: Thomas Bogendoerfer > Cc: linux-m...@vger.kernel.org > Cc: "James E.J. Bottomley" > Cc: Helge Deller > Cc: linux-par...@vger.kernel.org > Cc: Michael Ellerman > Cc: Benjamin Herrenschmidt > Cc: Paul Mackerras > Cc: linuxppc-dev@lists.ozlabs.org > Cc: Yoshinori Sato > Cc: Rich Felker > Cc: linux...@vger.kernel.org > Cc: "David S. Miller" > Cc: sparcli...@vger.kernel.org > Cc: Chris Zankel > Cc: Max Filippov > Cc: linux-xte...@linux-xtensa.org > Cc: Arnd Bergmann > Cc: linux-a...@vger.kernel.org > Cc: linux-...@vger.kernel.org > Signed-off-by: Ilpo Järvinen > --- > .../driver-api/serial/serial-rs485.rst| 23 ++- > arch/alpha/include/uapi/asm/ioctls.h | 3 + > arch/mips/include/uapi/asm/ioctls.h | 3 + > arch/parisc/include/uapi/asm/ioctls.h | 3 + > arch/powerpc/include/uapi/asm/ioctls.h| 3 + > arch/sh/include/uapi/asm/ioctls.h | 3 + > arch/sparc/include/uapi/asm/ioctls.h | 3 + > arch/xtensa/include/uapi/asm/ioctls.h | 3 + > drivers/tty/serial/8250/8250_core.c | 2 + > drivers/tty/serial/serial_core.c | 62 ++- > drivers/tty/tty_io.c | 2 + > include/linux/serial_core.h | 6 ++ > include/uapi/asm-generic/ioctls.h | 3 + > include/uapi/linux/serial.h | 8 +++ > 14 files changed, 125 insertions(+), 2 deletions(-) > > diff --git a/Documentation/driver-api/serial/serial-rs485.rst > b/Documentation/driver-api/serial/serial-rs485.rst > index 6bc824f948f9..2f45f007fa5b 100644 > --- a/Documentation/driver-api/serial/serial-rs485.rst > +++ b/Documentation/driver-api/serial/serial-rs485.rst > @@ -95,7 +95,28 @@ RS485 Serial Communications > /* Error handling. See errno. */ > } > > -5. References > +5. Multipoint Addressing > + > + > + The Linux kernel provides serial_addr structure to handle addressing > within > + multipoint serial communications line such as RS485. 9th bit addressiong > mode > + is enabled by adding ADDRB flag in termios c_cflag. > + > + Serial core calls device specific set/get_addr in response to TIOCSADDR > and > + TIOCGADDR ioctls with a pointer to serial_addr. Destination and receive > + address can be specified using serial_addr flags field. Receive address > may > + also be cleared using flags. Once an address is set, the communication > + can occur only with the particular device and other peers are filtered > out. > + It is left up to the receiver side to enforce the filtering. > + > + Address flags: > + - SER_ADDR_RECV: Receive (filter) address. > + - SER_ADDR_RECV_CLEAR: Clear receive filter (only for TIOCSADDR). > + - SER_ADDR_DEST: Destination address. > + > + Note: not all devices supporting RS485 support multipoint addressing. > + > +6. References > = > > [1] include/uapi/linux/serial.h > diff --git
Re: [PATCH v5 05/10] serial: termbits: ADDRB to indicate 9th bit addressing mode
On Tue, Apr 26, 2022 at 03:24:43PM +0300, Ilpo Järvinen wrote:
> Add ADDRB to termbits to indicate 9th bit addressing mode. This change
> is necessary for supporting devices with RS485 multipoint addressing
> [*]. A later patch in the patch series adds support for Synopsys
> Designware UART capable for 9th bit addressing mode. In this mode, 9th
> bit is used to indicate an address (byte) within the communication
> line. The 9th bit addressing mode is selected using ADDRB introduced by
> an earlier patch.
>
> [*] Technically, RS485 is just an electronic spec and does not itself
> specify the 9th bit addressing mode but 9th bit seems at least
> "semi-standard" way to do addressing with RS485.
>
> Cc: linux-...@vger.kernel.org
> Cc: Ivan Kokshaysky
> Cc: Matt Turner
> Cc: linux-al...@vger.kernel.org
> Cc: Thomas Bogendoerfer
> Cc: linux-m...@vger.kernel.org
> Cc: "James E.J. Bottomley"
> Cc: Helge Deller
> Cc: linux-par...@vger.kernel.org
> Cc: Michael Ellerman
> Cc: Benjamin Herrenschmidt
> Cc: Paul Mackerras
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: "David S. Miller"
> Cc: sparcli...@vger.kernel.org
> Cc: Arnd Bergmann
> Cc: linux-a...@vger.kernel.org
> Cc: linux-...@vger.kernel.org
> Signed-off-by: Ilpo Järvinen
> ---
> arch/alpha/include/uapi/asm/termbits.h | 1 +
> arch/mips/include/uapi/asm/termbits.h| 1 +
> arch/parisc/include/uapi/asm/termbits.h | 1 +
> arch/powerpc/include/uapi/asm/termbits.h | 1 +
> arch/sparc/include/uapi/asm/termbits.h | 1 +
> drivers/char/pcmcia/synclink_cs.c| 2 ++
> drivers/ipack/devices/ipoctal.c | 2 ++
> drivers/mmc/core/sdio_uart.c | 2 ++
> drivers/net/usb/hso.c| 3 ++-
> drivers/s390/char/tty3270.c | 3 +++
> drivers/staging/greybus/uart.c | 2 ++
> drivers/tty/amiserial.c | 6 +-
> drivers/tty/moxa.c | 1 +
> drivers/tty/mxser.c | 1 +
> drivers/tty/serial/serial_core.c | 2 ++
> drivers/tty/synclink_gt.c| 2 ++
> drivers/tty/tty_ioctl.c | 2 ++
> drivers/usb/class/cdc-acm.c | 2 ++
> drivers/usb/serial/usb-serial.c | 6 --
> include/uapi/asm-generic/termbits.h | 1 +
> net/bluetooth/rfcomm/tty.c | 2 ++
> 21 files changed, 40 insertions(+), 4 deletions(-)
>
> diff --git a/arch/alpha/include/uapi/asm/termbits.h
> b/arch/alpha/include/uapi/asm/termbits.h
> index 4575ba34a0ea..0c123e715486 100644
> --- a/arch/alpha/include/uapi/asm/termbits.h
> +++ b/arch/alpha/include/uapi/asm/termbits.h
> @@ -180,6 +180,7 @@ struct ktermios {
> #define HUPCL0004
>
> #define CLOCAL 0010
> +#define ADDRB0040/* address bit */
> #define CMSPAR 0100 /* mark or space (stick) parity
> */
> #define CRTSCTS0200 /* flow control */
>
> diff --git a/arch/mips/include/uapi/asm/termbits.h
> b/arch/mips/include/uapi/asm/termbits.h
> index dfeffba729b7..4732d31b0e4e 100644
> --- a/arch/mips/include/uapi/asm/termbits.h
> +++ b/arch/mips/include/uapi/asm/termbits.h
> @@ -182,6 +182,7 @@ struct ktermios {
> #define B350 0010016
> #define B400 0010017
> #define CIBAUD 00200360 /* input baud rate */
> +#define ADDRB 0040 /* address bit */
> #define CMSPAR 0100 /* mark or space (stick) parity */
> #define CRTSCTS0200 /* flow control */
>
> diff --git a/arch/parisc/include/uapi/asm/termbits.h
> b/arch/parisc/include/uapi/asm/termbits.h
> index 40e920f8d683..d6bbd10d92ba 100644
> --- a/arch/parisc/include/uapi/asm/termbits.h
> +++ b/arch/parisc/include/uapi/asm/termbits.h
> @@ -159,6 +159,7 @@ struct ktermios {
> #define B350 0010016
> #define B400 0010017
> #define CIBAUD00200360 /* input baud rate */
> +#define ADDRB 0040 /* address bit */
tabs where the rest were not?
> #define CMSPAR0100 /* mark or space (stick) parity */
> #define CRTSCTS 0200 /* flow control */
>
> diff --git a/arch/powerpc/include/uapi/asm/termbits.h
> b/arch/powerpc/include/uapi/asm/termbits.h
> index ed18bc61f63d..c6a033732f39 100644
> --- a/arch/powerpc/include/uapi/asm/termbits.h
> +++ b/arch/powerpc/include/uapi/asm/termbits.h
> @@ -171,6 +171,7 @@ struct ktermios {
> #define HUPCL0004
>
> #define CLOCAL 0010
> +#define ADDRB0040/* address bit */
> #define CMSPAR 0100 /* mark or space (stick) parity
> */
> #define CRTSCTS0200 /* flow control */
>
> diff --git a/arch/sparc/include/uapi/asm/termbits.h
> b/arch/sparc/include/uapi/asm/termbits.h
> index ce5ad5d0f105..5eb1d547b5c4 100644
> --- a/arch/sparc/include/uapi/asm/termbits.h
> +++
Re: [PATCH] [Rebased for 5.4] powerpc/kasan: Fix early region not updated correctly
On Sun, Apr 03, 2022 at 03:49:43PM +0200, Christophe Leroy wrote: > From: Chen Jingwen > > This is backport for 5.4 > > Upstream commit dd75080aa8409ce10d50fb58981c6b59bf8707d3 Now queued up, thanks. greg k-h
Re: [PATCH] [Rebased for 5.4] powerpc/kasan: Fix early region not updated correctly
On Sun, Apr 03, 2022 at 11:54:55AM +, Christophe Leroy wrote: > > > Le 03/04/2022 à 12:25, Greg KH a écrit : > > On Sat, Apr 02, 2022 at 06:13:31PM +0200, Christophe Leroy wrote: > >> From: Chen Jingwen > >> > >> This is backport for 5.4 > >> > >> Upstream commit 5647a94a26e352beed61788b46e035d9d12664cd > > > > This is not a commit in Linus's tree :( > > > > Oops. Don't know what I did, that's indeed the commit after I > cherry-picked it on top of 5.4.188 and before I modified it. > > According to the mail you sent me yesterday to tell it FAILED to apply > on 5.4, the upstream commit is dd75080aa8409ce10d50fb58981c6b59bf8707d3 Can you resend with this fixed up please? thanks, greg k-h
Re: [PATCH] [Rebased for 5.4] powerpc/kasan: Fix early region not updated correctly
On Sat, Apr 02, 2022 at 06:13:31PM +0200, Christophe Leroy wrote: > From: Chen Jingwen > > This is backport for 5.4 > > Upstream commit 5647a94a26e352beed61788b46e035d9d12664cd This is not a commit in Linus's tree :(
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Tue, Mar 01, 2022 at 06:40:04PM +0100, Jakob Koschel wrote: > > > > On 1. Mar 2022, at 18:36, Greg KH wrote: > > > > On Tue, Mar 01, 2022 at 12:28:15PM +0100, Jakob Koschel wrote: > >> > >> > >>> On 1. Mar 2022, at 01:41, Linus Torvalds > >>> wrote: > >>> > >>> On Mon, Feb 28, 2022 at 1:47 PM Jakob Koschel > >>> wrote: > >>>> > >>>> The goal of this is to get compiler warnings right? This would indeed be > >>>> great. > >>> > >>> Yes, so I don't mind having a one-time patch that has been gathered > >>> using some automated checker tool, but I don't think that works from a > >>> long-term maintenance perspective. > >>> > >>> So if we have the basic rule being "don't use the loop iterator after > >>> the loop has finished, because it can cause all kinds of subtle > >>> issues", then in _addition_ to fixing the existing code paths that > >>> have this issue, I really would want to (a) get a compiler warning for > >>> future cases and (b) make it not actually _work_ for future cases. > >>> > >>> Because otherwise it will just happen again. > >>> > >>>> Changing the list_for_each_entry() macro first will break all of those > >>>> cases > >>>> (e.g. the ones using 'list_entry_is_head()). > >>> > >>> So I have no problems with breaking cases that we basically already > >>> have a patch for due to your automated tool. There were certainly > >>> more than a handful, but it didn't look _too_ bad to just make the > >>> rule be "don't use the iterator after the loop". > >>> > >>> Of course, that's just based on that patch of yours. Maybe there are a > >>> ton of other cases that your patch didn't change, because they didn't > >>> match your trigger case, so I may just be overly optimistic here. > >> > >> Based on the coccinelle script there are ~480 cases that need fixing > >> in total. I'll now finish all of them and then split them by > >> submodules as Greg suggested and repost a patch set per submodule. > >> Sounds good? > > > > Sounds good to me! > > > > If you need help carving these up and maintaining them over time as > > different subsystem maintainers accept/ignore them, just let me know. > > Doing large patchsets like this can be tough without a lot of > > experience. > > Very much appreciated! > > There will probably be some cases that do not match one of the pattern > we already discussed and need separate attention. > > I was planning to start with one subsystem and adjust the coming ones > according to the feedback gather there instead of posting all of them > in one go. That seems wise. Feel free to use USB as a testing ground for this if you want to :) thanks, greg k-h
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Tue, Mar 01, 2022 at 12:28:15PM +0100, Jakob Koschel wrote: > > > > On 1. Mar 2022, at 01:41, Linus Torvalds > > wrote: > > > > On Mon, Feb 28, 2022 at 1:47 PM Jakob Koschel > > wrote: > >> > >> The goal of this is to get compiler warnings right? This would indeed be > >> great. > > > > Yes, so I don't mind having a one-time patch that has been gathered > > using some automated checker tool, but I don't think that works from a > > long-term maintenance perspective. > > > > So if we have the basic rule being "don't use the loop iterator after > > the loop has finished, because it can cause all kinds of subtle > > issues", then in _addition_ to fixing the existing code paths that > > have this issue, I really would want to (a) get a compiler warning for > > future cases and (b) make it not actually _work_ for future cases. > > > > Because otherwise it will just happen again. > > > >> Changing the list_for_each_entry() macro first will break all of those > >> cases > >> (e.g. the ones using 'list_entry_is_head()). > > > > So I have no problems with breaking cases that we basically already > > have a patch for due to your automated tool. There were certainly > > more than a handful, but it didn't look _too_ bad to just make the > > rule be "don't use the iterator after the loop". > > > > Of course, that's just based on that patch of yours. Maybe there are a > > ton of other cases that your patch didn't change, because they didn't > > match your trigger case, so I may just be overly optimistic here. > > Based on the coccinelle script there are ~480 cases that need fixing > in total. I'll now finish all of them and then split them by > submodules as Greg suggested and repost a patch set per submodule. > Sounds good? Sounds good to me! If you need help carving these up and maintaining them over time as different subsystem maintainers accept/ignore them, just let me know. Doing large patchsets like this can be tough without a lot of experience. thanks, greg k-h
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 01:06:57PM +0100, Jakob Koschel wrote: > > > > On 28. Feb 2022, at 12:20, Greg KH wrote: > > > > On Mon, Feb 28, 2022 at 12:08:18PM +0100, Jakob Koschel wrote: > >> If the list does not contain the expected element, the value of > >> list_for_each_entry() iterator will not point to a valid structure. > >> To avoid type confusion in such case, the list iterator > >> scope will be limited to list_for_each_entry() loop. > >> > >> In preparation to limiting scope of a list iterator to the list traversal > >> loop, use a dedicated pointer to point to the found element. > >> Determining if an element was found is then simply checking if > >> the pointer is != NULL. > >> > >> Signed-off-by: Jakob Koschel > >> --- > >> arch/x86/kernel/cpu/sgx/encl.c | 6 +++-- > >> drivers/scsi/scsi_transport_sas.c| 17 - > >> drivers/thermal/thermal_core.c | 38 ++-- > >> drivers/usb/gadget/configfs.c| 22 ++-- > >> drivers/usb/gadget/udc/max3420_udc.c | 11 +--- > >> drivers/usb/gadget/udc/tegra-xudc.c | 11 +--- > >> drivers/usb/mtu3/mtu3_gadget.c | 11 +--- > >> drivers/usb/musb/musb_gadget.c | 11 +--- > >> drivers/vfio/mdev/mdev_core.c| 11 +--- > >> 9 files changed, 88 insertions(+), 50 deletions(-) > > > > The drivers/usb/ portion of this patch should be in patch 1/X, right? > > I kept them separate since it's a slightly different case. > Using 'ptr' instead of '>other_member'. Regardless, I'll split > this commit up as you mentioned. > > > > > Also, you will have to split these up per-subsystem so that the > > different subsystem maintainers can take these in their trees. > > Thanks for the feedback. > To clarify I understand you correctly: > I should do one patch set per-subsystem with every instance/(file?) > fixed as a separate commit? Yes, each file needs a different commit as they are usually all written or maintained by different people. As I said in my other response, if you need any help with this, just let me know, I'm glad to help. thanks, greg k-h
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 12:08:18PM +0100, Jakob Koschel wrote: > If the list does not contain the expected element, the value of > list_for_each_entry() iterator will not point to a valid structure. > To avoid type confusion in such case, the list iterator > scope will be limited to list_for_each_entry() loop. > > In preparation to limiting scope of a list iterator to the list traversal > loop, use a dedicated pointer to point to the found element. > Determining if an element was found is then simply checking if > the pointer is != NULL. > > Signed-off-by: Jakob Koschel > --- > arch/x86/kernel/cpu/sgx/encl.c | 6 +++-- > drivers/scsi/scsi_transport_sas.c| 17 - > drivers/thermal/thermal_core.c | 38 ++-- > drivers/usb/gadget/configfs.c| 22 ++-- > drivers/usb/gadget/udc/max3420_udc.c | 11 +--- > drivers/usb/gadget/udc/tegra-xudc.c | 11 +--- > drivers/usb/mtu3/mtu3_gadget.c | 11 +--- > drivers/usb/musb/musb_gadget.c | 11 +--- > drivers/vfio/mdev/mdev_core.c| 11 +--- > 9 files changed, 88 insertions(+), 50 deletions(-) The drivers/usb/ portion of this patch should be in patch 1/X, right? Also, you will have to split these up per-subsystem so that the different subsystem maintainers can take these in their trees. thanks, greg k-h
Re: [PATCH 03/14] nds32: fix access_ok() checks in get/put_user
On Tue, Feb 15, 2022 at 10:18:15AM +0100, Arnd Bergmann wrote:
> On Mon, Feb 14, 2022 at 6:01 PM Christoph Hellwig wrote:
> >
> > On Mon, Feb 14, 2022 at 05:34:41PM +0100, Arnd Bergmann wrote:
> > > From: Arnd Bergmann
> > >
> > > The get_user()/put_user() functions are meant to check for
> > > access_ok(), while the __get_user()/__put_user() functions
> > > don't.
> > >
> > > This broke in 4.19 for nds32, when it gained an extraneous
> > > check in __get_user(), but lost the check it needs in
> > > __put_user().
> >
> > Can we follow the lead of MIPS (which this was originally copied
> > from I think) and kill the pointless __get/put_user_check wrapper
> > that just obsfucate the code?
>
> I had another look, but I think that would be a bigger change than
> I want to have in a fix for stable backports, as nds32 also uses
> the _check versions in __{get,put}_user_error.
Don't worry about stable backports first, get it correct and merged and
then worry about them if you really have to.
If someone cares about nds32 for stable kernels, they can do the
backport work :)
thanks,
greg k-h
Re: [PATCH] Styleguide fix: Removed un-needed whitespaces and formatting errors in drivers/tty
On Mon, Feb 07, 2022 at 12:33:07AM +0530, Ankit Kumar Pandey wrote:
> There were lot of styleguide errors raised by checkpatch.pl against
> drivers/tty/* which I have fixed. There is zero code change apart from
> changes related to styleguide. checkpatch.pl returns 0 error for
> style guide now.
>
> ---
> drivers/tty/amiserial.c| 285 ++---
> drivers/tty/ehv_bytechan.c | 5 +-
> drivers/tty/goldfish.c | 2 +
> 3 files changed, 142 insertions(+), 150 deletions(-)
>
> diff --git a/drivers/tty/amiserial.c b/drivers/tty/amiserial.c
> index 1e60dbef6..15917254e 100644
> --- a/drivers/tty/amiserial.c
> +++ b/drivers/tty/amiserial.c
> @@ -12,13 +12,13 @@
> * (non hardware specific) changes to serial.c.
> *
> * The port is registered with the tty driver as minor device 64, and
> - * therefore other ports should should only use 65 upwards.
> + * therefore other ports should only use 65 upwards.
> *
> * Richard Lucock 28/12/99
> *
> * Copyright (C) 1991, 1992 Linus Torvalds
> - * Copyright (C) 1992, 1993, 1994, 1995, 1996, 1997,
> - * 1998, 1999 Theodore Ts'o
> + * Copyright (C) 1992, 1993, 1994, 1995, 1996, 1997,
> + * 1998, 1999 Theodore Ts'o
> *
> */
>
> @@ -78,8 +78,8 @@ struct serial_state {
> int ignore_status_mask;
> int timeout;
> int quot;
> - int IER;/* Interrupt Enable Register */
> - int MCR;/* Modem control register */
> + int IER;/* Interrupt Enable Register */
> + int MCR;/* Modem control register */
> int x_char; /* xon/xoff character */
> };
>
> @@ -116,9 +116,9 @@ static struct serial_state serial_state;
> #define SER_CTS (1<<4)
> #define SER_DSR (1<<3)
>
> -static __inline__ void rtsdtr_ctrl(int bits)
> +static inline void rtsdtr_ctrl(int bits)
> {
> -ciab.pra = ((bits & (SER_RTS | SER_DTR)) ^ (SER_RTS | SER_DTR)) |
> (ciab.pra & ~(SER_RTS | SER_DTR));
> + ciab.pra = ((bits & (SER_RTS | SER_DTR)) ^ (SER_RTS | SER_DTR)) |
> (ciab.pra & ~(SER_RTS | SER_DTR));
> }
>
> /*
> @@ -175,7 +175,7 @@ static void rs_start(struct tty_struct *tty)
>
> static void receive_chars(struct serial_state *info)
> {
> -int status;
> + int status;
> int serdatr;
> unsigned char ch, flag;
> struct async_icount *icount;
> @@ -189,10 +189,10 @@ static void receive_chars(struct serial_state *info)
> amiga_custom.intreq = IF_RBF;
> mb();
>
> - if((serdatr & 0x1ff) == 0)
> - status |= UART_LSR_BI;
> - if(serdatr & SDR_OVRUN)
> - status |= UART_LSR_OE;
> + if ((serdatr & 0x1ff) == 0)
> + status |= UART_LSR_BI;
> + if (serdatr & SDR_OVRUN)
> + status |= UART_LSR_OE;
>
> ch = serdatr & 0xff;
> icount->rx++;
> @@ -213,45 +213,44 @@ static void receive_chars(struct serial_state *info)
> /*
> * For statistics only
> */
> - if (status & UART_LSR_BI) {
> - status &= ~(UART_LSR_FE | UART_LSR_PE);
> - icount->brk++;
> - } else if (status & UART_LSR_PE)
> - icount->parity++;
> - else if (status & UART_LSR_FE)
> - icount->frame++;
> - if (status & UART_LSR_OE)
> - icount->overrun++;
> + if (status & UART_LSR_BI) {
> + status &= ~(UART_LSR_FE | UART_LSR_PE);
> + icount->brk++;
> + } else if (status & UART_LSR_PE)
> + icount->parity++;
> + else if (status & UART_LSR_FE)
> + icount->frame++;
> + if (status & UART_LSR_OE)
> + icount->overrun++;
>
> /*
> * Now check to see if character should be
> * ignored, and mask off conditions which
> * should be ignored.
> */
> - if (status & info->ignore_status_mask)
> - goto out;
> + if (status & info->ignore_status_mask)
> + goto out;
>
> - status &= info->read_status_mask;
> -
> - if (status & (UART_LSR_BI)) {
> + status &= info->read_status_mask;
> + if (status & (UART_LSR_BI)) {
> #ifdef SERIAL_DEBUG_INTR
> - printk("handling break");
> + printk("handling break");
> #endif
> - flag = TTY_BREAK;
> - if (info->tport.flags & ASYNC_SAK)
> - do_SAK(info->tport.tty);
> - } else if (status & UART_LSR_PE)
> - flag = TTY_PARITY;
> - else if (status & UART_LSR_FE)
> - flag = TTY_FRAME;
> - if (status & UART_LSR_OE) {
> - /*
> - * Overrun is special, since it's
> - * reported immediately, and doesn't
> - * affect the current character
> - */
> - oe
Re: [PATCH v2 3/3] vstatus: Display an informational message when the VSTATUS character is pressed or TIOCSTAT ioctl is called.
On Sun, Feb 06, 2022 at 07:48:54AM -0800, Walt Drummond wrote:
> When triggered by pressing the VSTATUS key or calling the TIOCSTAT
> ioctl, the n_tty line discipline will display a message on the user's
> tty that provides basic information about the system and an
> 'interesting' process in the current foreground process group, eg:
>
> load: 0.58 cmd: sleep 744474 [sleeping] 0.36r 0.00u 0.00s 0% 772k
>
> The status message provides:
> - System load average
> - Command name and process id (from the perspective of the session)
> - Scheduler state
> - Total wall-clock run time
> - User space run time
> - System space run time
> - Percentage of on-cpu time
> - Resident set size
This should be documented somewhere, and not buried in a changelog text
like this. Can you also add this information somewhere in the
Documentation/ directory so that people have a hint as to what is going
on here?
> The message is only displayed when the tty has the VSTATUS character
> set, the local flags ICANON and IEXTEN are enabled and NOKERNINFO is
> disabled; it is always displayed when TIOCSTAT is called regardless of
> tty settings.
>
> Signed-off-by: Walt Drummond
> ---
> drivers/tty/Makefile | 2 +-
> drivers/tty/n_tty.c| 34 +++
> drivers/tty/n_tty_status.c | 181 +
> drivers/tty/tty_io.c | 2 +-
> include/linux/tty.h| 5 +
> 5 files changed, 222 insertions(+), 2 deletions(-)
> create mode 100644 drivers/tty/n_tty_status.c
Also, any chance for a test to be added so that we can ensure that this
doesn't change over time in ways that confuse/break people?
Is this now a new user/kernel api format that we must preserve for
forever? Can we add/remove items over time that make sense or are
programs (not just people), going to parse this?
>
> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
> index a2bd75fbaaa4..3539d7ab77e5 100644
> --- a/drivers/tty/Makefile
> +++ b/drivers/tty/Makefile
> @@ -2,7 +2,7 @@
> obj-$(CONFIG_TTY)+= tty_io.o n_tty.o tty_ioctl.o tty_ldisc.o \
> tty_buffer.o tty_port.o tty_mutex.o \
> tty_ldsem.o tty_baudrate.o tty_jobctrl.o \
> -n_null.o
> +n_null.o n_tty_status.o
> obj-$(CONFIG_LEGACY_PTYS)+= pty.o
> obj-$(CONFIG_UNIX98_PTYS)+= pty.o
> obj-$(CONFIG_AUDIT) += tty_audit.o
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index 64a058a4c63b..fd70efc333d7 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -80,6 +80,7 @@
> #define ECHO_BLOCK 256
> #define ECHO_DISCARD_WATERMARK N_TTY_BUF_SIZE - (ECHO_BLOCK + 32)
>
> +#define STATUS_LINE_LEN 160 /* tty status line will truncate at this
> length */
Tabs please.
>
> #undef N_TTY_TRACE
> #ifdef N_TTY_TRACE
> @@ -127,6 +128,8 @@ struct n_tty_data {
> struct mutex output_lock;
> };
>
> +static void n_tty_status(struct tty_struct *tty);
> +
> #define MASK(x) ((x) & (N_TTY_BUF_SIZE - 1))
>
> static inline size_t read_cnt(struct n_tty_data *ldata)
> @@ -1334,6 +1337,11 @@ static void n_tty_receive_char_special(struct
> tty_struct *tty, unsigned char c)
> commit_echoes(tty);
> return;
> }
> + if (c == STATUS_CHAR(tty) && L_IEXTEN(tty)) {
> + if (!L_NOKERNINFO(tty))
> + n_tty_status(tty);
> + return;
> + }
> if (c == '\n') {
> if (L_ECHO(tty) || L_ECHONL(tty)) {
> echo_char_raw('\n', ldata);
> @@ -1763,6 +1771,7 @@ static void n_tty_set_termios(struct tty_struct *tty,
> struct ktermios *old)
> set_bit(EOF_CHAR(tty), ldata->char_map);
> set_bit('\n', ldata->char_map);
> set_bit(EOL_CHAR(tty), ldata->char_map);
> + set_bit(STATUS_CHAR(tty), ldata->char_map);
> if (L_IEXTEN(tty)) {
> set_bit(WERASE_CHAR(tty), ldata->char_map);
> set_bit(LNEXT_CHAR(tty), ldata->char_map);
> @@ -2413,6 +2422,26 @@ static unsigned long inq_canon(struct n_tty_data
> *ldata)
> return nr;
> }
>
> +static void n_tty_status(struct tty_struct *tty)
> +{
> + struct n_tty_data *ldata = tty->disc_data;
> + char *msg;
> + size_t len;
> +
> + msg = kzalloc(STATUS_LINE_LEN, GFP_KERNEL);
Please check for memory failures.
> +
> + if (ldata->column != 0) {
> + *msg = '\n';
> + len = n_tty_get_status(tty, msg + 1, STATUS_LINE_LEN - 1);
> + } else {
> + len = n_tty_get_status(tty, msg, STATUS_LINE_LEN);
> + }
> +
> + do_n_tty_write(tty, NULL, msg, len);
> +
> + kfree(msg);
> +}
> +
> static int
Re: [PATCH v2 2/3] status: Add user space API definitions for VSTATUS, NOKERNINFO and TIOCSTAT
On Sun, Feb 06, 2022 at 07:48:53AM -0800, Walt Drummond wrote:
> Add definitions for the VSTATUS control character, and the NOKERNINFO
> local control flag in the termios struct, and add an ioctl number for
> the ioctl TIOCSTAT. Also add a default VSTATUS character (Ctrl-T)
> default valuses in termios.c_cc. Do this for all architectures.
>
> Signed-off-by: Walt Drummond
> ---
> arch/alpha/include/asm/termios.h | 4 ++--
> arch/alpha/include/uapi/asm/ioctls.h | 1 +
> arch/alpha/include/uapi/asm/termbits.h | 2 ++
> arch/ia64/include/asm/termios.h | 4 ++--
> arch/ia64/include/uapi/asm/termbits.h| 2 ++
> arch/mips/include/asm/termios.h | 4 ++--
> arch/mips/include/uapi/asm/ioctls.h | 1 +
> arch/mips/include/uapi/asm/termbits.h| 2 ++
> arch/parisc/include/asm/termios.h| 4 ++--
> arch/parisc/include/uapi/asm/ioctls.h| 1 +
> arch/parisc/include/uapi/asm/termbits.h | 2 ++
> arch/powerpc/include/asm/termios.h | 4 ++--
> arch/powerpc/include/uapi/asm/ioctls.h | 2 ++
> arch/powerpc/include/uapi/asm/termbits.h | 2 ++
> arch/s390/include/asm/termios.h | 4 ++--
> arch/sh/include/uapi/asm/ioctls.h| 1 +
> arch/sparc/include/uapi/asm/ioctls.h | 1 +
> arch/sparc/include/uapi/asm/termbits.h | 2 ++
> arch/xtensa/include/uapi/asm/ioctls.h| 1 +
> include/asm-generic/termios.h| 4 ++--
> include/uapi/asm-generic/ioctls.h| 1 +
> include/uapi/asm-generic/termbits.h | 2 ++
> 22 files changed, 37 insertions(+), 14 deletions(-)
>
> diff --git a/arch/alpha/include/asm/termios.h
> b/arch/alpha/include/asm/termios.h
> index b7c77bb1bfd2..d28ddc649286 100644
> --- a/arch/alpha/include/asm/termios.h
> +++ b/arch/alpha/include/asm/termios.h
> @@ -8,9 +8,9 @@
> werase=^W kill=^U reprint=^R sxtc=\0
> intr=^C quit=^\ susp=^Z
> start=^Qstop=^S lnext=^Vdiscard=^U
> - vmin=\1 vtime=\0
> + vmin=\1 vtime=\0status=^T
> */
> -#define INIT_C_CC
> "\004\000\000\177\027\025\022\000\003\034\032\000\021\023\026\025\001\000"
> +#define INIT_C_CC
> "\004\000\000\177\027\025\022\000\003\034\032\000\021\023\026\025\001\000\024"
>
> /*
> * Translate a "termio" structure into a "termios". Ugh.
> diff --git a/arch/alpha/include/uapi/asm/ioctls.h
> b/arch/alpha/include/uapi/asm/ioctls.h
> index 971311605288..70fdeab2b5f2 100644
> --- a/arch/alpha/include/uapi/asm/ioctls.h
> +++ b/arch/alpha/include/uapi/asm/ioctls.h
> @@ -124,5 +124,6 @@
>
> #define TIOCMIWAIT 0x545C /* wait for a change on serial input line(s) */
> #define TIOCGICOUNT 0x545D /* read serial port inline interrupt counts */
> +#define TIOCSTAT _IO('T', 0x5E) /* display process group stats on tty */
>
> #endif /* _ASM_ALPHA_IOCTLS_H */
> diff --git a/arch/alpha/include/uapi/asm/termbits.h
> b/arch/alpha/include/uapi/asm/termbits.h
> index 4575ba34a0ea..9a1b9aa92d29 100644
> --- a/arch/alpha/include/uapi/asm/termbits.h
> +++ b/arch/alpha/include/uapi/asm/termbits.h
> @@ -70,6 +70,7 @@ struct ktermios {
> #define VDISCARD 15
> #define VMIN 16
> #define VTIME 17
> +#define VSTATUS 18
>
> /* c_iflag bits */
> #define IGNBRK 001
> @@ -203,6 +204,7 @@ struct ktermios {
> #define PENDIN 0x2000
> #define IEXTEN 0x0400
> #define EXTPROC 0x1000
> +#define NOKERNINFO 0x4000
Here, and elsewhere, you seem to mix tabs and spaces. Please use what
is in the original file (tabs here.)
> /* Values for the ACTION argument to `tcflow'. */
> #define TCOOFF 0
> diff --git a/arch/ia64/include/asm/termios.h b/arch/ia64/include/asm/termios.h
> index 589c026444cc..40e83f9b6ead 100644
> --- a/arch/ia64/include/asm/termios.h
> +++ b/arch/ia64/include/asm/termios.h
> @@ -15,9 +15,9 @@
> eof=^D vtime=\0vmin=\1 sxtc=\0
> start=^Qstop=^S susp=^Z eol=\0
> reprint=^R discard=^U werase=^W lnext=^V
> - eol2=\0
> + eol2=\0 status=^T
Same here. And for the other files in this patch. Let's keep them
unified please.
thanks,
greg k-h
Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area
On Wed, Feb 02, 2022 at 08:04:01AM +, Matthew Garrett wrote: > On Wed, Feb 02, 2022 at 08:22:03AM +0100, Ard Biesheuvel wrote: > > On Wed, 2 Feb 2022 at 08:10, Matthew Garrett wrote: > > > Which other examples are you thinking of? I think this conversation may > > > have accidentally become conflated with a different prior one and now > > > we're talking at cross purposes. > > > > This came up a while ago during review of one of the earlier revisions > > of this patch set. > > > > https://lore.kernel.org/linux-efi/yrzuiivizmfgj...@google.com/ > > > > which describes another two variations on the theme, for pKVM guests > > as well as Android bare metal. > > Oh, I see! That makes much more sense - sorry, I wasn't Cc:ed on that, > so thought this was related to the efivars/Power secure boot. My > apologies, sorry for the noise. In that case, given the apparent > agreement between the patch owners that a consistent interface would > work for them, I think I agree with Greg that we should strive for that. > Given the described behaviour of the Google implementation, it feels > like the semantics in this implementation would be sufficient for them > as well, but having confirmation of that would be helpful. > > On the other hand, I also agree that a new filesystem for this is > overkill. I did that for efivarfs and I think the primary lesson from > that is that people who aren't familiar with the vfs shouldn't be > writing filesystems. Securityfs seems entirely reasonable, and it's > consistent with other cases where we expose firmware-provided data > that's security relevant. > > The only thing I personally struggle with here is whether "coco" is the > best name for it, and whether there are reasonable use cases that > wouldn't be directly related to confidential computing (eg, if the > firmware on a bare-metal platform had a mechanism for exposing secrets > to the OS based on some specific platform security state, it would seem > reasonable to expose it via this mechanism but it may not be what we'd > normally think of as Confidential Computing). > > But I'd also say that while we only have one implementation currently > sending patches, it's fine for the code to live in that implementation > and then be abstracted out once we have another. Well right now the Android code looks the cleanest and should be about ready to be merged into my tree. But I can almost guarantee that that interface is not what anyone else wants to use, so if you think somehow that everyone else is going to want to deal with a char device node and a simple mmap, with a DT description of the thing, hey, I'm all for it :) Seriously, people need to come up with something sane or this is going to be a total mess. thanks, greg k-h
Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area
On Wed, Feb 02, 2022 at 06:54:43AM +, Matthew Garrett wrote: > On Wed, Feb 02, 2022 at 07:10:02AM +0100, Greg KH wrote: > > On Wed, Feb 02, 2022 at 04:01:57AM +, Matthew Garrett wrote: > > > We're talking about things that have massively different semantics. > > > > I see lots of different platforms trying to provide access to their > > "secure" firmware data to userspace in different ways. That feels to me > > like they are the same thing that userspace would care about in a > > unified way. > > EFI variables are largely for the OS to provide information to the > firmware, while this patchset is to provide information from the > firmware to the OS. I don't see why we'd expect to use the same userland > tooling for both. I totally agree, I'm not worried about EFI variables here, I don't know why that came up. > In the broader case - I don't think we *can* use the same userland > tooling for everything. For example, the patches to add support for > manipulating the Power secure boot keys originally attempted to make it > look like efivars, but the underlying firmware semantics are > sufficiently different that even exposing the same kernel interface > wouldn't be a sufficient abstraction and userland would still need to > behave differently. Exposing an interface that looks consistent but > isn't is arguably worse for userland than exposing explicitly distinct > interfaces. So what does userspace really need here? Just the ability to find if the platform has blobs that it cares about, and how to read/write them. I see different platform patches trying to stick these blobs in different locations and ways to access (securityfs, sysfs, char device node), which seems crazy to me. Why can't we at least pick one way to access these to start with, and then have the filesystem layout be platform-specific as needed, which will give the correct hints to userspace as to what it needs to do here? thanks, greg k-h
Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area
On Wed, Feb 02, 2022 at 04:01:57AM +, Matthew Garrett wrote: > On Tue, Feb 01, 2022 at 09:24:50AM -0500, James Bottomley wrote: > > On Tue, 2022-02-01 at 14:50 +0100, Greg KH wrote: > > > You all need to work together to come up with a unified place for > > > this and stop making it platform-specific. > > We're talking about things that have massively different semantics. I see lots of different platforms trying to provide access to their "secure" firmware data to userspace in different ways. That feels to me like they are the same thing that userspace would care about in a unified way. Unless we expeect userspace tools to have to be platform-specific for all of this? That does not seem wise. what am I missing here? thanks, greg k-h
Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area
On Tue, Feb 01, 2022 at 09:24:50AM -0500, James Bottomley wrote: > [cc's added] > On Tue, 2022-02-01 at 14:50 +0100, Greg KH wrote: > > On Tue, Feb 01, 2022 at 12:44:08PM +, Dov Murik wrote: > [...] > > > # ls -la /sys/kernel/security/coco/efi_secret > > > total 0 > > > drwxr-xr-x 2 root root 0 Jun 28 11:55 . > > > drwxr-xr-x 3 root root 0 Jun 28 11:54 .. > > > -r--r- 1 root root 0 Jun 28 11:54 736870e5-84f0-4973-92ec- > > > 06879ce3da0b > > > -r--r- 1 root root 0 Jun 28 11:54 83c83f7f-1356-4975-8b7e- > > > d3a0b54312c6 > > > -r--r- 1 root root 0 Jun 28 11:54 9553f55d-3da2-43ee-ab5d- > > > ff17f78864d2 > > > > Please see my comments on the powerpc version of this type of thing: > > > > https://lore.kernel.org/r/20220122005637.28199-1-na...@linux.ibm.com > > If you want a debate, actually cc'ing the people on the other thread > would have been a good start ... > > For those added, this patch series is at: > > https://lore.kernel.org/all/20220201124413.1093099-1-dovmu...@linux.ibm.com/ Thanks for adding everyone. > > You all need to work together to come up with a unified place for > > this and stop making it platform-specific. > > I'm not entirely sure of that. If you look at the differences between > EFI variables and the COCO proposal: the former has an update API > which, in the case of signed variables, is rather complex and a UC16 > content requirement. The latter is binary data with read only/delete. > Plus each variable in EFI is described by a GUID, so having a directory > of random guids, some of which behave like COCO secrets and some of > which are EFI variables is going to be incredibly confusing (and also > break all our current listing tools which seems somewhat undesirable). > > So we could end up with > > /efivar > /coco The powerpc stuff is not efi. But yes, that is messy here. But why doesn't the powerpc follow the coco standard? > To achieve the separation, but I really don't see what this buys us. > Both filesystems would likely end up with different backends because of > the semantic differences and we can easily start now in different > places (effectively we've already done this for efi variables) and > unify later if that is the chosen direction, so it doesn't look like a > blocker. > > > Until then, we can't take this. > > I don't believe anyone was asking you to take it. I was on the review list...
Re: [RFC PATCH 0/2] powerpc/pseries: add support for local secure storage called Platform Keystore(PKS)
On Mon, Jan 24, 2022 at 11:25:17AM +1100, Daniel Axtens wrote:
> Hi Greg,
>
> > Ok, this is like the 3rd or 4th different platform-specific proposal for
> > this type of functionality. I think we need to give up on
> > platform-specific user/kernel apis on this (random sysfs/securityfs
> > files scattered around the tree), and come up with a standard place for
> > all of this.
>
> I agree that we do have a number of platforms exposing superficially
> similar functionality. Indeed, back in 2019 I had a crack at a unified
> approach: [1] [2].
>
> Looking back at it now, I am not sure it ever would have worked because
> the semantics of the underlying firmware stores are quite
> different. Here are the ones I know about:
>
> - OpenPower/PowerNV Secure Variables:
>
>* Firmware semantics:
> - flat variable space
> - variables are fixed in firmware, can neither be created nor
> destroyed
> - variable names are ASCII
> - no concept of policy/attributes
>
>* Current kernel interface semantics:
> - names are case sensitive
> - directory per variable
>
>
> - (U)EFI variables:
>
>* Firmware semantics:
> - flat variable space
> - variables can be created/destroyed but the semantics are fiddly
> [3]
> - variable names are UTF-16 + UUID
> - variables have 32-bit attributes
>
>* efivarfs interface semantics:
> - file per variable
> - attributes are the first 4 bytes of the file
> - names are partially case-insensitive (UUID part) and partially
> case-sensitive ('name' part)
>
>* sysfs interface semantics (as used by CONFIG_GOOGLE_SMI)
> - directory per variable
> - attributes are a separate sysfs file
> - to create a variable you write a serialised structure to
> `/sys/firmware/efi/vars/new_var`, to delete a var you write
> to `.../del_var`
> - names are case-sensitive including the UUID
>
>
> - PowerVM Partition Key Store Variables:
>
>* Firmware semantics:
> - _not_ a flat space, there are 3 domains ("consumers"): firmware,
> bootloader and OS (not yet supported by the patch set)
> - variables can be created and destroyed but the semantics are
> fiddly and fiddly in different ways to UEFI [4]
> - variable names are arbitrary byte strings: the hypervisor permits
> names to contain nul and /.
> - variables have 32-bit attributes ("policy") that don't align with
> UEFI attributes
>
>* No stable kernel interface yet
>
> Even if we could come up with some stable kernel interface features
> (e.g. decide if we want file per variable vs directory per variable), I
> don't know how easy it would be to deal with the underlying semantic
> differences - I think userspace would still need substantial
> per-platform knowledge.
>
> Or have I misunderstood what you're asking for? (If you want them all to
> live under /sys/firmware, these ones all already do...)
I want them to be unified in some way, right now there are lots of
proposals for the same type of thing, but in different places (sysfs,
securityfs, somewhere else), and with different names.
Please work together.
thanks,
greg k-h
Re: [PATCH 0/3] status: TTY status message request
On Mon, Jan 17, 2022 at 08:42:57PM -0800, Walt Drummond wrote: > This patchset adds TTY status message request feature to the n_tty > line dicipline. This feature prints a brief message containing basic > system and process group information to a user's TTY in response to a > new control character in the line dicipline (default Ctrl-T) or the > TIOCSTAT ioctl. The message contains the current system load, the > name and PID of an interesting process in the forground process group, > it's run time, percent CPU usage and RSS. An example of this message > is: > > load: 0.31 cmd: sleep 3616843 [sleeping] 0.36r 0.00u 0.00s 0% 696k > > User API visible changes are limited to: > - The addition of VSTATUS in termios.c_cc[] > - The addition of NOKERNINFO bit in termios.l_cflags > - The addition of the TIOCSTAT ioctl number > > None of these changes break the existing kernel api as the termios > structure on all architectures has enough space in the control > character array (.c_cc) for the new character, and the other changes > are space agnostic. > > This feature is in many other Unix-like systems, both current and > historical. In other implementations, this feature would also send > SIGINFO to the process group; this implementation does not. > > Walt Drummond (3): > vstatus: Allow the n_tty line dicipline to write to a user tty > vstatus: Add user space API definitions for VSTATUS, NOKERNINFO and > TIOCSTAT > status: Display an informational message when the VSTATUS character is > pressed or TIOCSTAT ioctl is called. > > arch/alpha/include/asm/termios.h | 4 +- > arch/alpha/include/uapi/asm/ioctls.h | 1 + > arch/alpha/include/uapi/asm/termbits.h | 34 ++--- > arch/ia64/include/asm/termios.h | 4 +- > arch/ia64/include/uapi/asm/termbits.h| 34 ++--- > arch/mips/include/asm/termios.h | 4 +- > arch/mips/include/uapi/asm/ioctls.h | 1 + > arch/mips/include/uapi/asm/termbits.h| 36 ++--- > arch/parisc/include/asm/termios.h| 4 +- > arch/parisc/include/uapi/asm/ioctls.h| 1 + > arch/parisc/include/uapi/asm/termbits.h | 34 ++--- > arch/powerpc/include/asm/termios.h | 4 +- > arch/powerpc/include/uapi/asm/ioctls.h | 2 + > arch/powerpc/include/uapi/asm/termbits.h | 34 ++--- > arch/s390/include/asm/termios.h | 4 +- > arch/sh/include/uapi/asm/ioctls.h| 1 + > arch/sparc/include/uapi/asm/ioctls.h | 1 + > arch/sparc/include/uapi/asm/termbits.h | 38 +++--- > arch/xtensa/include/uapi/asm/ioctls.h| 1 + > drivers/tty/Makefile | 2 +- > drivers/tty/n_tty.c | 113 +++- > drivers/tty/n_tty_status.c | 162 +++ > drivers/tty/tty_io.c | 2 +- > include/asm-generic/termios.h| 4 +- > include/linux/tty.h | 123 - > include/uapi/asm-generic/ioctls.h| 1 + > include/uapi/asm-generic/termbits.h | 34 ++--- > 27 files changed, 461 insertions(+), 222 deletions(-) > create mode 100644 drivers/tty/n_tty_status.c > > -- > 2.30.2 > You forgot to cc: me on patch 2/3, which would be needed if I was to take them all. Please fix up patch 2 and resend the whole series. thanks, greg k-h
Re: [RFC PATCH 0/2] powerpc/pseries: add support for local secure storage called Platform Keystore(PKS)
On Fri, Jan 21, 2022 at 07:56:35PM -0500, Nayna Jain wrote: > PowerVM provides an isolated Platform Keystore(PKS) storage allocation > for each partition with individually managed access controls to store > sensitive information securely. Linux Kernel can access this storage by > interfacing with hypervisor using a new set of hypervisor calls. > > PowerVM guest secure boot intend to use Platform Keystore for the > purpose of storing public keys. Secure boot requires public keys to > be able to verify the grub and boot kernel. To allow authenticated > manipulation of keys, it supports variables to store key authorities > - PK/KEK and code signing keys - db. It also supports denied list to > disallow booting even if signed with valid key. This is done via > denied list database - dbx or sbat. These variables would be stored in > PKS, and are managed and controlled by firmware. > > The purpose of this patchset is to add support for users to > read/write/add/delete variables required for secure boot on PowerVM. Ok, this is like the 3rd or 4th different platform-specific proposal for this type of functionality. I think we need to give up on platform-specific user/kernel apis on this (random sysfs/securityfs files scattered around the tree), and come up with a standard place for all of this. Please work with the other developers of the other drivers for this to make this unified so that userspace has a chance to use this in a sane manner. thanks, greg k-h
Re: [PATCH stable 4.19 1/1] arch: pgtable: define MAX_POSSIBLE_PHYSMEM_BITS where needed
On Wed, Nov 03, 2021 at 01:56:56PM -0700, Florian Fainelli wrote:
> From: Arnd Bergmann
>
> [ Upstream commit cef397038167ac15d085914493d6c86385773709 ]
>
> Stefan Agner reported a bug when using zsram on 32-bit Arm machines
> with RAM above the 4GB address boundary:
>
> Unable to handle kernel NULL pointer dereference at virtual address
> pgd = a27bd01c
> [] *pgd=236a0003, *pmd=1ffa64003
> Internal error: Oops: 207 [#1] SMP ARM
> Modules linked in: mdio_bcm_unimac(+) brcmfmac cfg80211 brcmutil
> raspberrypi_hwmon hci_uart crc32_arm_ce bcm2711_thermal phy_generic genet
> CPU: 0 PID: 123 Comm: mkfs.ext4 Not tainted 5.9.6 #1
> Hardware name: BCM2711
> PC is at zs_map_object+0x94/0x338
> LR is at zram_bvec_rw.constprop.0+0x330/0xa64
> pc : []lr : []psr: 6013
> sp : e376bbe0 ip : fp : c1e2921c
> r10: 0002 r9 : c1dda730 r8 :
> r7 : e8ff7a00 r6 : r5 : 02f9ffa0 r4 : e371
> r3 : 000fdffe r2 : c1e0ce80 r1 : ebf979a0 r0 :
> Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
> Control: 30c5383d Table: 235c2a80 DAC: fffd
> Process mkfs.ext4 (pid: 123, stack limit = 0x495a22e6)
> Stack: (0xe376bbe0 to 0xe376c000)
>
> As it turns out, zsram needs to know the maximum memory size, which
> is defined in MAX_PHYSMEM_BITS when CONFIG_SPARSEMEM is set, or in
> MAX_POSSIBLE_PHYSMEM_BITS on the x86 architecture.
>
> The same problem will be hit on all 32-bit architectures that have a
> physical address space larger than 4GB and happen to not enable sparsemem
> and include asm/sparsemem.h from asm/pgtable.h.
>
> After the initial discussion, I suggested just always defining
> MAX_POSSIBLE_PHYSMEM_BITS whenever CONFIG_PHYS_ADDR_T_64BIT is
> set, or provoking a build error otherwise. This addresses all
> configurations that can currently have this runtime bug, but
> leaves all other configurations unchanged.
>
> I looked up the possible number of bits in source code and
> datasheets, here is what I found:
>
> - on ARC, CONFIG_ARC_HAS_PAE40 controls whether 32 or 40 bits are used
> - on ARM, CONFIG_LPAE enables 40 bit addressing, without it we never
>support more than 32 bits, even though supersections in theory allow
>up to 40 bits as well.
> - on MIPS, some MIPS32r1 or later chips support 36 bits, and MIPS32r5
>XPA supports up to 60 bits in theory, but 40 bits are more than
>anyone will ever ship
> - On PowerPC, there are three different implementations of 36 bit
>addressing, but 32-bit is used without CONFIG_PTE_64BIT
> - On RISC-V, the normal page table format can support 34 bit
>addressing. There is no highmem support on RISC-V, so anything
>above 2GB is unused, but it might be useful to eventually support
>CONFIG_ZRAM for high pages.
>
> Fixes: 61989a80fb3a ("staging: zsmalloc: zsmalloc memory allocation library")
> Fixes: 02390b87a945 ("mm/zsmalloc: Prepare to variable MAX_PHYSMEM_BITS")
> Acked-by: Thomas Bogendoerfer
> Reviewed-by: Stefan Agner
> Tested-by: Stefan Agner
> Acked-by: Mike Rapoport
> Link:
> https://lore.kernel.org/linux-mm/bdfa44bf1c570b05d6c70898e2bbb0acf234ecdf.1604762181.git.ste...@agner.ch/
> Signed-off-by: Arnd Bergmann
> Signed-off-by: Sasha Levin
> [florian: patch arch/powerpc/include/asm/pte-common.h for 4.19.y]
> Signed-off-by: Florian Fainelli
All now queued up, thanks.
greg k-h
Re: [PATCH v11 2/3] tty: hvc: pass DMA capable memory to put_chars()
On Tue, Oct 26, 2021 at 02:11:51PM +0800, Xianting Tian wrote:
>
> 在 2021/10/26 下午2:10, Greg KH 写道:
> > On Tue, Oct 26, 2021 at 02:02:21PM +0800, Xianting Tian wrote:
> > > 在 2021/10/26 下午1:10, Jiri Slaby 写道:
> > > > On 15. 10. 21, 4:46, Xianting Tian wrote:
> > > > > @@ -151,9 +142,11 @@ static uint32_t vtermnos[MAX_NR_HVC_CONSOLES] =
> > > > > static void hvc_console_print(struct console *co, const char *b,
> > > > > unsigned count)
> > > > > {
> > > > > - char c[N_OUTBUF] __ALIGNED__;
> > > > > + char *c;
> > > > > unsigned i = 0, n = 0;
> > > > > int r, donecr = 0, index = co->index;
> > > > > + unsigned long flags;
> > > > > + struct hvc_struct *hp;
> > > > > /* Console access attempt outside of acceptable console
> > > > > range. */
> > > > > if (index >= MAX_NR_HVC_CONSOLES)
> > > > > @@ -163,6 +156,13 @@ static void hvc_console_print(struct console
> > > > > *co, const char *b,
> > > > > if (vtermnos[index] == -1)
> > > > > return;
> > > > > + hp = cons_hvcs[index];
> > > > > + if (!hp)
> > > > > + return;
> > > > You effectively make the console unusable until someone calls
> > > > hvc_alloc() for this device, correct? This doesn't look right. Neither
> > > > you describe this change of behaviour in the commit log.
> > > I mentioned such info in the commit log:
> > > 'Introduce another array(cons_hvcs[]) for hvc pointers next to the
> > > cons_ops[] and vtermnos[] arrays. With the array, we can easily find
> > > hvc's cons_outbuf and its lock.'
> > >
> > > After you pointed it out, I just found what you said make sense, I
> > > checked the code hvc_console_print() can support print before hvc_alloc()
> > > is called when someone use hvc_instantiate() for an early console
> > > discovery method.
> > > I send a patch to fix the issue? or these serial pathches reverted
> > > fisrtly then I resend new version patches? thanks
> > Let me revert these now and you can send an updated version.
> OK, thanks.
I have now reverted patches 2/3 and 3/3 in this series from my tree.
The first patch was just fine.
thanks,
greg k-h
Re: [PATCH v11 2/3] tty: hvc: pass DMA capable memory to put_chars()
On Tue, Oct 26, 2021 at 02:02:21PM +0800, Xianting Tian wrote:
> 在 2021/10/26 下午1:10, Jiri Slaby 写道:
> > On 15. 10. 21, 4:46, Xianting Tian wrote:
> > > @@ -151,9 +142,11 @@ static uint32_t vtermnos[MAX_NR_HVC_CONSOLES] =
> > > static void hvc_console_print(struct console *co, const char *b,
> > > unsigned count)
> > > {
> > > - char c[N_OUTBUF] __ALIGNED__;
> > > + char *c;
> > > unsigned i = 0, n = 0;
> > > int r, donecr = 0, index = co->index;
> > > + unsigned long flags;
> > > + struct hvc_struct *hp;
> > > /* Console access attempt outside of acceptable console
> > > range. */
> > > if (index >= MAX_NR_HVC_CONSOLES)
> > > @@ -163,6 +156,13 @@ static void hvc_console_print(struct console
> > > *co, const char *b,
> > > if (vtermnos[index] == -1)
> > > return;
> > > + hp = cons_hvcs[index];
> > > + if (!hp)
> > > + return;
> >
> > You effectively make the console unusable until someone calls
> > hvc_alloc() for this device, correct? This doesn't look right. Neither
> > you describe this change of behaviour in the commit log.
>
> I mentioned such info in the commit log:
> 'Introduce another array(cons_hvcs[]) for hvc pointers next to the
> cons_ops[] and vtermnos[] arrays. With the array, we can easily find
> hvc's cons_outbuf and its lock.'
>
> After you pointed it out, I just found what you said make sense, I checked
> the code hvc_console_print() can support print before hvc_alloc() is called
> when someone use hvc_instantiate() for an early console discovery method.
> I send a patch to fix the issue? or these serial pathches reverted fisrtly
> then I resend new version patches? thanks
Let me revert these now and you can send an updated version.
thanks,
greg k-h
Re: Stable backport request
On Sun, Oct 24, 2021 at 09:21:09AM +1100, Michael Ellerman wrote:
> Hi Greg,
>
> Please backport the following commit to v5.4 and v5.10:
>
> 73287caa9210ded6066833195f4335f7f688a46b
> ("powerpc64/idle: Fix SP offsets when saving GPRs")
>
>
> And please backport the following commits to v5.4, v5.10 and v5.14:
>
> 9b4416c5095c20e110c82ae602c254099b83b72f
> ("KVM: PPC: Book3S HV: Fix stack handling in idle_kvm_start_guest()")
>
> cdeb5d7d890e14f3b70e8087e745c4a6a7d9f337
> ("KVM: PPC: Book3S HV: Make idle_kvm_start_guest() return 0 if it went to
> guest")
>
> 496c5fe25c377ddb7815c4ce8ecfb676f051e9b6
> ("powerpc/idle: Don't corrupt back chain when going idle")
All now queued up, thanks!
greg k-h
Re: [PATCH v11 0/3] make hvc pass dma capable memory to its backend
On Fri, Oct 15, 2021 at 10:46:55AM +0800, Xianting Tian wrote:
> Dear all,
>
> This patch series make hvc framework pass DMA capable memory to
> put_chars() of hvc backend(eg, virtio-console), and revert commit
> c4baad5029 ("virtio-console: avoid DMA from stack”)
Thanks for sticking with this, looks much better now, all now queued up.
greg k-h
Re: [PATCH v11 2/3] tty: hvc: pass DMA capable memory to put_chars()
A: http://en.wikipedia.org/wiki/Top_post Q: Were do I find info about this thing called top-posting? A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? A: Top-posting. Q: What is the most annoying thing in e-mail? A: No. Q: Should I include quotations after my reply? http://daringfireball.net/2007/07/on_top On Wed, Oct 20, 2021 at 04:47:23PM +0800, Xianting Tian wrote: > hi Greg， > > Could I get your comments of this new version patches？ thanks It has been less than 5 days. Please relax, and only worry after 2 weeks have gone by. We have lots of patches to review. To help maintainers out, why don't you review other patches on the mailing lists as well while you wait? thanks, greg k-h
Re: [PATCH v10 2/3] tty: hvc: pass DMA capable memory to put_chars()
On Thu, Oct 14, 2021 at 04:34:59PM +0800, Xianting Tian wrote:
>
> 在 2021/10/10 下午1:33, Greg KH 写道:
> > On Sat, Oct 09, 2021 at 11:45:23PM +0800, Xianting Tian wrote:
> > > 在 2021/10/9 下午7:58, Greg KH 写道:
> > > > Did you look at the placement using pahole as to how this structure now
> > > > looks?
> > > thanks for all your commnts. for this one, do you mean I need to remove
> > > the
> > > blank line? thanks
> > >
> > No, I mean to use the tool 'pahole' to see the structure layout that you
> > just created and determine if it really is the best way to add these new
> > fields, especially as you are adding huge buffers with odd alignment.
>
> thanks,
>
> Based on your comments, I removed 'char outchar', remian the position of
> 'int outbuf_size' unchanged to keep outbuf_size and lock in the same cache
> line. Now hvc_struct change as below,
>
> struct hvc_struct {
> struct tty_port port;
> spinlock_t lock;
> int index;
> int do_wakeup;
> - char *outbuf;
> int outbuf_size;
> int n_outbuf;
> uint32_t vtermno;
> @@ -48,6 +57,16 @@ struct hvc_struct {
> struct work_struct tty_resize;
> struct list_head next;
> unsigned long flags;
> +
> + /*
> + * the buf is used in hvc console api for putting chars,
> + * and also used in hvc_poll_put_char() for putting single char.
> + */
> + char cons_outbuf[N_OUTBUF] __ALIGNED__;
> + spinlock_t cons_outbuf_lock;
> +
> + /* the buf is used for putting chars to tty */
> + char outbuf[] __ALIGNED__;
> };
>
> pahole for above hvc_struct as below, is it ok for you? do we need to pack
> the hole? thanks
>
> struct hvc_struct {
> struct tty_port port; /* 0 352 */
> /* --- cacheline 5 boundary (320 bytes) was 32 bytes ago --- */
> spinlock_t lock; /* 352 4 */
> int index; /* 356 4 */
> int do_wakeup; /* 360 4 */
> int outbuf_size; /* 364 4 */
> int n_outbuf; /* 368 4 */
> uint32_t vtermno; /* 372 4 */
> const struct hv_ops * ops; /* 376 8 */
> /* --- cacheline 6 boundary (384 bytes) --- */
> int irq_requested; /* 384 4 */
> int data; /* 388 4 */
> struct winsize ws; /* 392 8 */
> struct work_struct tty_resize; /* 400 32 */
> struct list_head next; /* 432 16 */
> /* --- cacheline 7 boundary (448 bytes) --- */
> long unsigned int flags; /* 448 8 */
>
> /* XXX 56 bytes hole, try to pack */
>
> /* --- cacheline 8 boundary (512 bytes) --- */
> char cons_outbuf[16]; /* 512 16 */
> spinlock_t cons_outbuf_lock; /* 528 4 */
>
> /* XXX 44 bytes hole, try to pack */
Why not move the spinlock up above the cons_outbuf? Will that not be a
bit better?
Anyway, this is all fine, and much better than before, thanks.
greg k-h
Re: [PATCH v10 2/3] tty: hvc: pass DMA capable memory to put_chars()
On Sat, Oct 09, 2021 at 11:45:23PM +0800, Xianting Tian wrote: > > 在 2021/10/9 下午7:58, Greg KH 写道: > > Did you look at the placement using pahole as to how this structure now > > looks? > > thanks for all your commnts. for this one, do you mean I need to remove the > blank line? thanks > No, I mean to use the tool 'pahole' to see the structure layout that you just created and determine if it really is the best way to add these new fields, especially as you are adding huge buffers with odd alignment. thanks, greg k-h
Re: [PATCH v10 2/3] tty: hvc: pass DMA capable memory to put_chars()
On Sat, Oct 09, 2021 at 07:48:28PM +0800, Xianting Tian wrote:
> --- a/drivers/tty/hvc/hvc_console.h
> +++ b/drivers/tty/hvc/hvc_console.h
> @@ -32,13 +32,21 @@
> */
> #define HVC_ALLOC_TTY_ADAPTERS 8
>
> +/*
> + * These sizes are most efficient for vio, because they are the
> + * native transfer size. We could make them selectable in the
> + * future to better deal with backends that want other buffer sizes.
> + */
> +#define N_OUTBUF 16
> +#define N_INBUF 16
> +
> +#define __ALIGNED__ __attribute__((__aligned__(sizeof(long
Does this conflict with what is in hvcs.c?
> +
> struct hvc_struct {
> struct tty_port port;
> spinlock_t lock;
> int index;
> int do_wakeup;
> - char *outbuf;
> - int outbuf_size;
> int n_outbuf;
> uint32_t vtermno;
> const struct hv_ops *ops;
> @@ -48,6 +56,18 @@ struct hvc_struct {
> struct work_struct tty_resize;
> struct list_head next;
> unsigned long flags;
> +
> + /* the buf is used in hvc console api for putting chars */
> + char cons_outbuf[N_OUTBUF] __ALIGNED__;
> + spinlock_t cons_outbuf_lock;
Did you look at the placement using pahole as to how this structure now
looks?
> +
> + /* the buf is for putting single char to tty */
> + char outchar;
> + spinlock_t outchar_lock;
So you have a lock for a character and a different one for a longer
string? Why can they not just use the same lock? Why are 2 needed at
all, can't you just use the first character of cons_outbuf[] instead?
Surely you do not have 2 sends happening at the same time, right?
> +
> + /* the buf is for putting chars to tty */
> + int outbuf_size;
> + char outbuf[0] __ALIGNED__;
I thought we were not allowing [0] anymore in kernel structures?
thanks,
greg k-h
Re: [PATCH v10 2/3] tty: hvc: pass DMA capable memory to put_chars()
On Sat, Oct 09, 2021 at 07:48:28PM +0800, Xianting Tian wrote:
> As well known, hvc backend can register its opertions to hvc backend.
> the operations contain put_chars(), get_chars() and so on.
>
> Some hvc backend may do dma in its operations. eg, put_chars() of
> virtio-console. But in the code of hvc framework, it may pass DMA
> incapable memory to put_chars() under a specific configuration, which
> is explained in commit c4baad5029(virtio-console: avoid DMA from stack):
> 1, c[] is on stack,
>hvc_console_print():
> char c[N_OUTBUF] __ALIGNED__;
> cons_ops[index]->put_chars(vtermnos[index], c, i);
> 2, ch is on stack,
>static void hvc_poll_put_char(,,char ch)
>{
> struct tty_struct *tty = driver->ttys[0];
> struct hvc_struct *hp = tty->driver_data;
> int n;
>
> do {
> n = hp->ops->put_chars(hp->vtermno, , 1);
> } while (n <= 0);
>}
>
> Commit c4baad5029 is just the fix to avoid DMA from stack memory, which
> is passed to virtio-console by hvc framework in above code. But I think
> the fix is aggressive, it directly uses kmemdup() to alloc new buffer
> from kmalloc area and do memcpy no matter the memory is in kmalloc area
> or not. But most importantly, it should better be fixed in the hvc
> framework, by changing it to never pass stack memory to the put_chars()
> function in the first place. Otherwise, we still face the same issue if
> a new hvc backend using dma added in the furture.
>
> In this patch, add 'char cons_outbuf[]' as part of 'struct hvc_struct',
> so hp->cons_outbuf is no longer the stack memory, we can use it in above
> case 1. Add 'char outchar' as part of 'struct hvc_struct', we can use it
> in above case 2. We also add lock for each above buf to protect them
> separately instead of using the global lock of hvc.
>
> Introduce another array(cons_hvcs[]) for hvc pointers next to the
> cons_ops[] and vtermnos[] arrays. With the array, we can easily find
> hvc's cons_outbuf and its lock.
>
> With the patch, we can revert the fix c4baad5029.
>
> Signed-off-by: Xianting Tian
> Signed-off-by: Shile Zhang
> ---
> drivers/tty/hvc/hvc_console.c | 37 +--
> drivers/tty/hvc/hvc_console.h | 24 +--
> 2 files changed, 44 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
> index 5bb8c4e44..4d8f112f2 100644
> --- a/drivers/tty/hvc/hvc_console.c
> +++ b/drivers/tty/hvc/hvc_console.c
> @@ -41,16 +41,6 @@
> */
> #define HVC_CLOSE_WAIT (HZ/100) /* 1/10 of a second */
>
> -/*
> - * These sizes are most efficient for vio, because they are the
> - * native transfer size. We could make them selectable in the
> - * future to better deal with backends that want other buffer sizes.
> - */
> -#define N_OUTBUF 16
> -#define N_INBUF 16
> -
> -#define __ALIGNED__ __attribute__((__aligned__(sizeof(long
> -
Are you sure this applies on top of patch 1/3 here?
> +/*
> + * These sizes are most efficient for vio, because they are the
> + * native transfer size. We could make them selectable in the
> + * future to better deal with backends that want other buffer sizes.
> + */
> +#define N_OUTBUF 16
> +#define N_INBUF 16
> +
> +#define __ALIGNED__ __attribute__((__aligned__(sizeof(long
Again, are you sure this is correct?
thanks,
greg k-h
Re: [PATCH v8 1/2] powerpc/pseries: Interface to represent PAPR firmware attributes
On Tue, Sep 28, 2021 at 06:13:18PM +0530, Pratik Sampat wrote: > Hello Greg, > > Thank you for your review. > > On 28/09/21 5:38 pm, Greg KH wrote: > > On Tue, Sep 28, 2021 at 05:21:01PM +0530, Pratik R. Sampat wrote: > > > Adds a generic interface to represent the energy and frequency related > > > PAPR attributes on the system using the new H_CALL > > > "H_GET_ENERGY_SCALE_INFO". > > > > > > H_GET_EM_PARMS H_CALL was previously responsible for exporting this > > > information in the lparcfg, however the H_GET_EM_PARMS H_CALL > > > will be deprecated P10 onwards. > > > > > > The H_GET_ENERGY_SCALE_INFO H_CALL is of the following call format: > > > hcall( > > >uint64 H_GET_ENERGY_SCALE_INFO, // Get energy scale info > > >uint64 flags, // Per the flag request > > >uint64 firstAttributeId,// The attribute id > > >uint64 bufferAddress, // Guest physical address of the output buffer > > >uint64 bufferSize // The size in bytes of the output buffer > > > ); > > > > > > This H_CALL can query either all the attributes at once with > > > firstAttributeId = 0, flags = 0 as well as query only one attribute > > > at a time with firstAttributeId = id, flags = 1. > > > > > > The output buffer consists of the following > > > 1. number of attributes - 8 bytes > > > 2. array offset to the data location - 8 bytes > > > 3. version info - 1 byte > > > 4. A data array of size num attributes, which contains the following: > > >a. attribute ID - 8 bytes > > >b. attribute value in number - 8 bytes > > >c. attribute name in string - 64 bytes > > >d. attribute value in string - 64 bytes > > > > > > The new H_CALL exports information in direct string value format, hence > > > a new interface has been introduced in > > > /sys/firmware/papr/energy_scale_info to export this information to > > > userspace in an extensible pass-through format. > > > > > > The H_CALL returns the name, numeric value and string value (if exists) > > > > > > The format of exposing the sysfs information is as follows: > > > /sys/firmware/papr/energy_scale_info/ > > > |-- / > > > |-- desc > > > |-- value > > > |-- value_desc (if exists) > > > |-- / > > > |-- desc > > > |-- value > > > |-- value_desc (if exists) > > > ... > > > > > > The energy information that is exported is useful for userspace tools > > > such as powerpc-utils. Currently these tools infer the > > > "power_mode_data" value in the lparcfg, which in turn is obtained from > > > the to be deprecated H_GET_EM_PARMS H_CALL. > > > On future platforms, such userspace utilities will have to look at the > > > data returned from the new H_CALL being populated in this new sysfs > > > interface and report this information directly without the need of > > > interpretation. > > > > > > Signed-off-by: Pratik R. Sampat > > > Reviewed-by: Gautham R. Shenoy > > > Reviewed-by: Fabiano Rosas > > > Reviewed-by: Kajol Jain > > > --- > > > .../sysfs-firmware-papr-energy-scale-info | 26 ++ > > > arch/powerpc/include/asm/hvcall.h | 24 +- > > > arch/powerpc/kvm/trace_hv.h | 1 + > > > arch/powerpc/platforms/pseries/Makefile | 3 +- > > > .../pseries/papr_platform_attributes.c| 312 ++ > > > 5 files changed, 364 insertions(+), 2 deletions(-) > > > create mode 100644 > > > Documentation/ABI/testing/sysfs-firmware-papr-energy-scale-info > > > create mode 100644 > > > arch/powerpc/platforms/pseries/papr_platform_attributes.c > > > > > > diff --git > > > a/Documentation/ABI/testing/sysfs-firmware-papr-energy-scale-info > > > b/Documentation/ABI/testing/sysfs-firmware-papr-energy-scale-info > > > new file mode 100644 > > > index ..139a576c7c9d > > > --- /dev/null > > > +++ b/Documentation/ABI/testing/sysfs-firmware-papr-energy-scale-info > > > @@ -0,0 +1,26 @@ > > > +What:/sys/firmware/papr/energy_scale_info > > > +Date:June 2021 > > > +Contact: Linux for PowerPC mailing list > > > +Description:
Re: [PATCH v8 1/2] powerpc/pseries: Interface to represent PAPR firmware attributes
On Tue, Sep 28, 2021 at 05:21:01PM +0530, Pratik R. Sampat wrote: > Adds a generic interface to represent the energy and frequency related > PAPR attributes on the system using the new H_CALL > "H_GET_ENERGY_SCALE_INFO". > > H_GET_EM_PARMS H_CALL was previously responsible for exporting this > information in the lparcfg, however the H_GET_EM_PARMS H_CALL > will be deprecated P10 onwards. > > The H_GET_ENERGY_SCALE_INFO H_CALL is of the following call format: > hcall( > uint64 H_GET_ENERGY_SCALE_INFO, // Get energy scale info > uint64 flags, // Per the flag request > uint64 firstAttributeId,// The attribute id > uint64 bufferAddress, // Guest physical address of the output buffer > uint64 bufferSize // The size in bytes of the output buffer > ); > > This H_CALL can query either all the attributes at once with > firstAttributeId = 0, flags = 0 as well as query only one attribute > at a time with firstAttributeId = id, flags = 1. > > The output buffer consists of the following > 1. number of attributes - 8 bytes > 2. array offset to the data location - 8 bytes > 3. version info - 1 byte > 4. A data array of size num attributes, which contains the following: > a. attribute ID - 8 bytes > b. attribute value in number - 8 bytes > c. attribute name in string - 64 bytes > d. attribute value in string - 64 bytes > > The new H_CALL exports information in direct string value format, hence > a new interface has been introduced in > /sys/firmware/papr/energy_scale_info to export this information to > userspace in an extensible pass-through format. > > The H_CALL returns the name, numeric value and string value (if exists) > > The format of exposing the sysfs information is as follows: > /sys/firmware/papr/energy_scale_info/ >|-- / > |-- desc > |-- value > |-- value_desc (if exists) >|-- / > |-- desc > |-- value > |-- value_desc (if exists) > ... > > The energy information that is exported is useful for userspace tools > such as powerpc-utils. Currently these tools infer the > "power_mode_data" value in the lparcfg, which in turn is obtained from > the to be deprecated H_GET_EM_PARMS H_CALL. > On future platforms, such userspace utilities will have to look at the > data returned from the new H_CALL being populated in this new sysfs > interface and report this information directly without the need of > interpretation. > > Signed-off-by: Pratik R. Sampat > Reviewed-by: Gautham R. Shenoy > Reviewed-by: Fabiano Rosas > Reviewed-by: Kajol Jain > --- > .../sysfs-firmware-papr-energy-scale-info | 26 ++ > arch/powerpc/include/asm/hvcall.h | 24 +- > arch/powerpc/kvm/trace_hv.h | 1 + > arch/powerpc/platforms/pseries/Makefile | 3 +- > .../pseries/papr_platform_attributes.c| 312 ++ > 5 files changed, 364 insertions(+), 2 deletions(-) > create mode 100644 > Documentation/ABI/testing/sysfs-firmware-papr-energy-scale-info > create mode 100644 arch/powerpc/platforms/pseries/papr_platform_attributes.c > > diff --git a/Documentation/ABI/testing/sysfs-firmware-papr-energy-scale-info > b/Documentation/ABI/testing/sysfs-firmware-papr-energy-scale-info > new file mode 100644 > index ..139a576c7c9d > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-firmware-papr-energy-scale-info > @@ -0,0 +1,26 @@ > +What:/sys/firmware/papr/energy_scale_info > +Date:June 2021 > +Contact: Linux for PowerPC mailing list > +Description: Directory hosting a set of platform attributes like > + energy/frequency on Linux running as a PAPR guest. > + > + Each file in a directory contains a platform > + attribute hierarchy pertaining to performance/ > + energy-savings mode and processor frequency. > + > +What:/sys/firmware/papr/energy_scale_info/ > + /sys/firmware/papr/energy_scale_info//desc > + /sys/firmware/papr/energy_scale_info//value > + /sys/firmware/papr/energy_scale_info//value_desc > +Date:June 2021 > +Contact: Linux for PowerPC mailing list > +Description: Energy, frequency attributes directory for POWERVM servers > + > + This directory provides energy, frequency, folding information. > It > + contains below sysfs attributes: > + > + - desc: String description of the attribute > + > + - value: Numeric value of attribute > + > + - value_desc: String value of attribute Can you just make 4 different entries in this file, making it easier to parse and extend over time? > diff --git a/arch/powerpc/include/asm/hvcall.h > b/arch/powerpc/include/asm/hvcall.h > index 9bcf345cb208..38980fef7a3d 100644 > --- a/arch/powerpc/include/asm/hvcall.h > +++ b/arch/powerpc/include/asm/hvcall.h > @@ -323,7 +323,8 @@ >
Re: [PATCH v8 2/3] tty: hvc: pass DMA capable memory to put_chars()
On Sat, Sep 18, 2021 at 08:32:01PM +0800, Xianting Tian wrote: > hi > > Will you consider to continue the disscussion of this patch? thanks I do not see a newer version of this series. thanks, greg k-h
Re: [PATCH v7 1/2] tty: hvc: pass DMA capable memory to put_chars()
On Tue, Aug 17, 2021 at 09:22:59PM +0800, Xianting Tian wrote: > We tested the patch, it worked normally. Who is "we"? > Signed-off-by: Xianting Tian Like I said before, I need another developer from your company to review and sign-off on this patch (and the other one), before I am willing to look at it, based on the previous mistakes that have happened here. thanks, greg k-h
Re: [PATCH v6 1/2] tty: hvc: pass DMA capable memory to put_chars()
On Thu, Aug 12, 2021 at 05:45:31PM +0800, Xianting Tian wrote:
> As well known, hvc backend can register its opertions to hvc backend.
> the opertions contain put_chars(), get_chars() and so on.
>
> Some hvc backend may do dma in its opertions. eg, put_chars() of
> virtio-console. But in the code of hvc framework, it may pass DMA
> incapable memory to put_chars() under a specific configuration, which
> is explained in commit c4baad5029(virtio-console: avoid DMA from stack):
> 1, c[] is on stack,
>hvc_console_print():
> char c[N_OUTBUF] __ALIGNED__;
> cons_ops[index]->put_chars(vtermnos[index], c, i);
> 2, ch is on stack,
>static void hvc_poll_put_char(,,char ch)
>{
> struct tty_struct *tty = driver->ttys[0];
> struct hvc_struct *hp = tty->driver_data;
> int n;
>
> do {
> n = hp->ops->put_chars(hp->vtermno, , 1);
> } while (n <= 0);
>}
>
> Commit c4baad5029 is just the fix to avoid DMA from stack memory, which
> is passed to virtio-console by hvc framework in above code. But I think
> the fix is aggressive, it directly uses kmemdup() to alloc new buffer
> from kmalloc area and do memcpy no matter the memory is in kmalloc area
> or not. But most importantly, it should better be fixed in the hvc
> framework, by changing it to never pass stack memory to the put_chars()
> function in the first place. Otherwise, we still face the same issue if
> a new hvc backend using dma added in the furture.
>
> We make 'char c[N_OUTBUF]' part of 'struct hvc_struct', so hp->c is no
> longer the stack memory. we can use it in above two cases.
>
> Other fix is use L1_CACHE_BYTES as the alignment, use 'sizeof(long)' as
> dma alignment is wrong. And use struct_size() to calculate size of
> hvc_struct.
>
> Introduce another array(cons_outbuf[]) for the hp->c pointers next to
> the cons_ops[] and vtermnos[] arrays.
>
> With the patch, we can remove the fix c4baad5029.
>
> Signed-off-by: Xianting Tian
> Tested-by: Xianting Tian
As the build shows, you obviously did not test this code :(
Also, no need to add a tested-by line as that should be implicit if you
wrote and signed off on it.
I am going to ask you to get some help from some other developers at
your company, and get them to test and sign off on this series before
sending it out again, as there seems to be a bit of a disconnect as to
what is actually needed to do when sending a patch for us to review.
That is now a requirement for us to be able to take your changes here.
thanks,
greg k-h
Re: [PATCH v3 1/2] tty: hvc: pass DMA capable memory to put_chars()
On Thu, Aug 05, 2021 at 04:08:46PM +0800, Xianting Tian wrote: > > 在 2021/8/5 下午3:58, Jiri Slaby 写道: > > Hi, > > > > On 04. 08. 21, 4:54, Xianting Tian wrote: > > > @@ -933,6 +949,16 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, > > > int data, > > > hp->outbuf_size = outbuf_size; > > > hp->outbuf = &((char *)hp)[ALIGN(sizeof(*hp), sizeof(long))]; > > > + /* > > > + * hvc_con_outbuf is guaranteed to be aligned at least to the > > > + * size(N_OUTBUF) by kmalloc(). > > > + */ > > > + hp->hvc_con_outbuf = kzalloc(N_OUTBUF, GFP_KERNEL); > > > + if (!hp->hvc_con_outbuf) > > > + return ERR_PTR(-ENOMEM); > > > > This leaks hp, right? > > > > BTW your 2 patches are still not threaded, that is hard to follow. > > yes, thanks, I found the bug, I am preparing to do this in v4. > > It is the first time I send series patches(number >1), I checked the method > for sending series patch on LKML.org, I should send '0/2' which is the > history info for series patches. Please use 'git send-email' to send the full series all at once, otherwise it is hard to make the emails threaded "by hand" if you do not do so. thanks, greg k-h
Re: [PATCH] fpga: dfl: fme: Fix cpu hotplug issue in performance reporting
On Tue, Jun 29, 2021 at 12:45:20PM +0530, kajoljain wrote: > > > On 6/28/21 3:47 PM, Kajol Jain wrote: > > The performance reporting driver added cpu hotplug > > feature but it didn't add pmu migration call in cpu > > offline function. > > This can create an issue incase the current designated > > cpu being used to collect fme pmu data got offline, > > as based on current code we are not migrating fme pmu to > > new target cpu. Because of that perf will still try to > > fetch data from that offline cpu and hence we will not > > get counter data. > > > > Patch fixed this issue by adding pmu_migrate_context call > > in fme_perf_offline_cpu function. > > > > Adding sta...@vger.kernel.org in cc list as suggested by Moritz Fischer. This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly.
Re: [PATCH] powerpc/udbg_hvc: retry putc on -EAGAIN
On Sun, May 23, 2021 at 08:51:09PM +1000, Michael Ellerman wrote: > Greg KH writes: > > On Fri, May 14, 2021 at 04:44:22PM -0500, Nathan Lynch wrote: > >> hvterm_raw_put_chars() calls hvc_put_chars(), which may return -EAGAIN > >> when the underlying hcall returns a "busy" status, but udbg_hvc_putc() > >> doesn't handle this. When using xmon on a PowerVM guest, this can > >> result in incomplete or garbled output when printing relatively large > >> amounts of data quickly, such as when dumping the kernel log buffer. > >> > >> Call again on -EAGAIN. > >> > >> Signed-off-by: Nathan Lynch > >> --- > >> drivers/tty/hvc/hvc_vio.c | 2 +- > > > > Subject line does not match up with this file name. > > > > Don't you want "tty" and "hvc" in there somewhere? > > It's a powerpc only driver, but I guess the subject should still be > "tty: hvc: ..." to match convention. > > I was planning to take this via the powerpc tree, but I can drop it if > you'd rather take it. No problem, feel free to take it yourself! greg k-h
