* Re: [Intel-gfx] [PATCH 09/12] drm: Shrink drm_display_mode timings
From: Ville Syrjälä @ 2020-02-20 18:47 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, dri-devel
In-Reply-To: <20200220181908.GX2363188@phenom.ffwll.local>
On Thu, Feb 20, 2020 at 07:19:08PM +0100, Daniel Vetter wrote:
> On Wed, Feb 19, 2020 at 10:35:41PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Store the timings (apart from the clock) as u16. The uapi mode
> > struct already uses u16 for everything so using something bigger
> > internally doesn't really help us.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Makes sense I guess. This could mean some implicit pointer math is now no
> longer auto-upgraded to big enough integers though ...
u16 promotes to int. So can't really see how this would go wrong. Well,
unless someone is using these to store some larger intermediate values.
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> > ---
> > drivers/gpu/drm/drm_modes.c | 7 ------
> > include/drm/drm_modes.h | 46 ++++++++++++++++++-------------------
> > 2 files changed, 23 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > index 0e7c9ba241c4..cc9fc52f9f7c 100644
> > --- a/drivers/gpu/drm/drm_modes.c
> > +++ b/drivers/gpu/drm/drm_modes.c
> > @@ -1917,13 +1917,6 @@ EXPORT_SYMBOL(drm_mode_create_from_cmdline_mode);
> > void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
> > const struct drm_display_mode *in)
> > {
> > - WARN(in->hdisplay > USHRT_MAX || in->hsync_start > USHRT_MAX ||
> > - in->hsync_end > USHRT_MAX || in->htotal > USHRT_MAX ||
> > - in->hskew > USHRT_MAX || in->vdisplay > USHRT_MAX ||
> > - in->vsync_start > USHRT_MAX || in->vsync_end > USHRT_MAX ||
> > - in->vtotal > USHRT_MAX || in->vscan > USHRT_MAX,
> > - "timing values too large for mode info\n");
> > -
> > out->clock = in->clock;
> > out->hdisplay = in->hdisplay;
> > out->hsync_start = in->hsync_start;
> > diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> > index b28c0234fcd7..b585074945b5 100644
> > --- a/include/drm/drm_modes.h
> > +++ b/include/drm/drm_modes.h
> > @@ -278,16 +278,16 @@ struct drm_display_mode {
> > * Pixel clock in kHz.
> > */
> > int clock; /* in kHz */
> > - int hdisplay;
> > - int hsync_start;
> > - int hsync_end;
> > - int htotal;
> > - int hskew;
> > - int vdisplay;
> > - int vsync_start;
> > - int vsync_end;
> > - int vtotal;
> > - int vscan;
> > + u16 hdisplay;
> > + u16 hsync_start;
> > + u16 hsync_end;
> > + u16 htotal;
> > + u16 hskew;
> > + u16 vdisplay;
> > + u16 vsync_start;
> > + u16 vsync_end;
> > + u16 vtotal;
> > + u16 vscan;
> > /**
> > * @flags:
> > *
> > @@ -356,19 +356,19 @@ struct drm_display_mode {
> > * difference is exactly a factor of 10.
> > */
> > int crtc_clock;
> > - int crtc_hdisplay;
> > - int crtc_hblank_start;
> > - int crtc_hblank_end;
> > - int crtc_hsync_start;
> > - int crtc_hsync_end;
> > - int crtc_htotal;
> > - int crtc_hskew;
> > - int crtc_vdisplay;
> > - int crtc_vblank_start;
> > - int crtc_vblank_end;
> > - int crtc_vsync_start;
> > - int crtc_vsync_end;
> > - int crtc_vtotal;
> > + u16 crtc_hdisplay;
> > + u16 crtc_hblank_start;
> > + u16 crtc_hblank_end;
> > + u16 crtc_hsync_start;
> > + u16 crtc_hsync_end;
> > + u16 crtc_htotal;
> > + u16 crtc_hskew;
> > + u16 crtc_vdisplay;
> > + u16 crtc_vblank_start;
> > + u16 crtc_vblank_end;
> > + u16 crtc_vsync_start;
> > + u16 crtc_vsync_end;
> > + u16 crtc_vtotal;
> >
> > /**
> > * @private_flags:
> > --
> > 2.24.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [Intel-gfx] [PATCH 09/12] drm: Shrink drm_display_mode timings
From: Ville Syrjälä @ 2020-02-20 18:47 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, dri-devel
In-Reply-To: <20200220181908.GX2363188@phenom.ffwll.local>
On Thu, Feb 20, 2020 at 07:19:08PM +0100, Daniel Vetter wrote:
> On Wed, Feb 19, 2020 at 10:35:41PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Store the timings (apart from the clock) as u16. The uapi mode
> > struct already uses u16 for everything so using something bigger
> > internally doesn't really help us.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Makes sense I guess. This could mean some implicit pointer math is now no
> longer auto-upgraded to big enough integers though ...
u16 promotes to int. So can't really see how this would go wrong. Well,
unless someone is using these to store some larger intermediate values.
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> > ---
> > drivers/gpu/drm/drm_modes.c | 7 ------
> > include/drm/drm_modes.h | 46 ++++++++++++++++++-------------------
> > 2 files changed, 23 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > index 0e7c9ba241c4..cc9fc52f9f7c 100644
> > --- a/drivers/gpu/drm/drm_modes.c
> > +++ b/drivers/gpu/drm/drm_modes.c
> > @@ -1917,13 +1917,6 @@ EXPORT_SYMBOL(drm_mode_create_from_cmdline_mode);
> > void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
> > const struct drm_display_mode *in)
> > {
> > - WARN(in->hdisplay > USHRT_MAX || in->hsync_start > USHRT_MAX ||
> > - in->hsync_end > USHRT_MAX || in->htotal > USHRT_MAX ||
> > - in->hskew > USHRT_MAX || in->vdisplay > USHRT_MAX ||
> > - in->vsync_start > USHRT_MAX || in->vsync_end > USHRT_MAX ||
> > - in->vtotal > USHRT_MAX || in->vscan > USHRT_MAX,
> > - "timing values too large for mode info\n");
> > -
> > out->clock = in->clock;
> > out->hdisplay = in->hdisplay;
> > out->hsync_start = in->hsync_start;
> > diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> > index b28c0234fcd7..b585074945b5 100644
> > --- a/include/drm/drm_modes.h
> > +++ b/include/drm/drm_modes.h
> > @@ -278,16 +278,16 @@ struct drm_display_mode {
> > * Pixel clock in kHz.
> > */
> > int clock; /* in kHz */
> > - int hdisplay;
> > - int hsync_start;
> > - int hsync_end;
> > - int htotal;
> > - int hskew;
> > - int vdisplay;
> > - int vsync_start;
> > - int vsync_end;
> > - int vtotal;
> > - int vscan;
> > + u16 hdisplay;
> > + u16 hsync_start;
> > + u16 hsync_end;
> > + u16 htotal;
> > + u16 hskew;
> > + u16 vdisplay;
> > + u16 vsync_start;
> > + u16 vsync_end;
> > + u16 vtotal;
> > + u16 vscan;
> > /**
> > * @flags:
> > *
> > @@ -356,19 +356,19 @@ struct drm_display_mode {
> > * difference is exactly a factor of 10.
> > */
> > int crtc_clock;
> > - int crtc_hdisplay;
> > - int crtc_hblank_start;
> > - int crtc_hblank_end;
> > - int crtc_hsync_start;
> > - int crtc_hsync_end;
> > - int crtc_htotal;
> > - int crtc_hskew;
> > - int crtc_vdisplay;
> > - int crtc_vblank_start;
> > - int crtc_vblank_end;
> > - int crtc_vsync_start;
> > - int crtc_vsync_end;
> > - int crtc_vtotal;
> > + u16 crtc_hdisplay;
> > + u16 crtc_hblank_start;
> > + u16 crtc_hblank_end;
> > + u16 crtc_hsync_start;
> > + u16 crtc_hsync_end;
> > + u16 crtc_htotal;
> > + u16 crtc_hskew;
> > + u16 crtc_vdisplay;
> > + u16 crtc_vblank_start;
> > + u16 crtc_vblank_end;
> > + u16 crtc_vsync_start;
> > + u16 crtc_vsync_end;
> > + u16 crtc_vtotal;
> >
> > /**
> > * @private_flags:
> > --
> > 2.24.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* [RFC PATCH] KVM: Add module for platform IRQ forwarding
From: Micah Morton @ 2020-02-20 18:47 UTC (permalink / raw)
To: kvm, vfio-users; +Cc: Micah Morton
Currently, KVM/VFIO offers no way to forward interrupts into a guest VM
when those interrupts are generated by a platform device that sits
behind a PCI bus controller which is given to the VM. This module allows
for forwarding arbitrary interrupts on the host system into the guest,
supporting this platform-device-behind-PCI-controller scenario.
This code is mostly inspired/taken from the equivalent code in VFIO. I
don't necessarily expect this exact code or code that closely resembles
this to be merged -- just sharing this patch that I've written/tested to
show an example of the kind of functionality I'm looking to gain support
for in KVM. (At the very least some of the code duplication from VFIO
should probably be eliminated).
One obvious question would be why not just add this support to VFIO?
See https://www.redhat.com/archives/vfio-users/2019-December/msg00008.html
and the encompassing thread for a discussion as to why this probably
isn't the way to go.
Forwarding arbitrary IRQs to a guest VM does require the VMM to "tell"
the guest about the interrupt (e.g. through ACPI), since such info is
not discoverable by the guest like it is for PCI devices. So separate
logic is needed in the VMM to set this up -- this isn't something done
by the module shared here.
What do people think? Forwarding platform IRQs can have a big payoff for
getting platform devices to work in a guest, especially when the
platform devices sit behind a PCI bus controller that can be easily
passed through to the guest. On an Intel device I'm using for
development, this module allowed me to get multiple devices (keyboard,
touchscreen, touchpad) working in a VM guest on the device that wouldn't
have worked otherwise -- straight out of the box after passing through
the PCI bus controller with vfio-pci (plus constructing some AML for the
guest in the VMM).
NOTE: This code works for forwarding IRQs to a guest (with the VMM
calling the appropriate ioctls with the appropriate args), although it's
missing some code and testing related to shutdown/irq disable/reboot.
Works well enough to demonstrate the feasibility though.
Signed-off-by: Micah Morton <mortonm@chromium.org>
---
include/linux/miscdevice.h | 1 +
include/linux/plat_irqfd.h | 20 ++
include/uapi/linux/platirqforward.h | 55 ++++++
virt/lib/Kconfig | 3 +
virt/lib/Makefile | 1 +
virt/lib/plat_irqfd.c | 146 ++++++++++++++
virt/lib/platirqforward.c | 289 ++++++++++++++++++++++++++++
7 files changed, 515 insertions(+)
create mode 100644 include/linux/plat_irqfd.h
create mode 100644 include/uapi/linux/platirqforward.h
create mode 100644 virt/lib/plat_irqfd.c
create mode 100644 virt/lib/platirqforward.c
diff --git a/include/linux/miscdevice.h b/include/linux/miscdevice.h
index becde6981a95..a9866dc7e10a 100644
--- a/include/linux/miscdevice.h
+++ b/include/linux/miscdevice.h
@@ -39,6 +39,7 @@
#define IRNET_MINOR 187
#define D7S_MINOR 193
#define VFIO_MINOR 196
+#define PLAT_IRQ_FORWARD_MINOR 197
#define TUN_MINOR 200
#define CUSE_MINOR 203
#define MWAVE_MINOR 219 /* ACP/Mwave Modem */
diff --git a/include/linux/plat_irqfd.h b/include/linux/plat_irqfd.h
new file mode 100644
index 000000000000..936a79cc7f5e
--- /dev/null
+++ b/include/linux/plat_irqfd.h
@@ -0,0 +1,20 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef PLAT_IRQFD_H
+#define PLAT_IRQFD_H
+
+struct plat_irq_forward_irqfd {
+ struct eventfd_ctx *eventfd;
+ int (*handler)(void *, void *);
+ void *data;
+ wait_queue_entry_t wait;
+ poll_table pt;
+ struct work_struct shutdown;
+ struct plat_irq_forward_irqfd **pirqfd;
+};
+
+int plat_irq_forward_irqfd_enable(int (*handler)(void *, void *), void *data, struct plat_irq_forward_irqfd **pirqfd, int fd);
+#endif /* PLAT_IRQFD_H */
diff --git a/include/uapi/linux/platirqforward.h b/include/uapi/linux/platirqforward.h
new file mode 100644
index 000000000000..4b26d4cd8957
--- /dev/null
+++ b/include/uapi/linux/platirqforward.h
@@ -0,0 +1,55 @@
+/*
+ * API definition for Platform IRQ Forwarding to KVM guests
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef _UAPIPLATIRQFORWARD_H
+#define _UAPIPLATIRQFORWARD_H
+
+#include <linux/ioctl.h>
+
+#define PLAT_IRQ_FORWARD_API_VERSION 0
+
+#define PLAT_IRQ_FORWARD_TYPE (';')
+#define PLAT_IRQ_FORWARD_BASE 100
+
+struct plat_irq_forward_edge_triggered {
+ struct eventfd_ctx *trigger;
+ uint32_t irq_num;
+ struct list_head list;
+};
+
+struct plat_irq_forward_level_triggered {
+ struct eventfd_ctx *trigger;
+ struct plat_irq_forward_irqfd *unmask;
+ bool is_masked;
+ spinlock_t spinlock;
+ uint32_t irq_num;
+ struct list_head list;
+};
+
+/**
+ *
+ * Set masking and unmasking of interrupts. Caller provides
+ * struct plat_irq_forward_set with all fields set.
+ *
+ */
+struct plat_irq_forward_set {
+ __u32 argsz;
+ __u32 action_flags;
+#define PLAT_IRQ_FORWARD_SET_LEVEL_TRIGGER_EVENTFD (1 << 0)
+#define PLAT_IRQ_FORWARD_SET_LEVEL_UNMASK_EVENTFD (1 << 1)
+#define PLAT_IRQ_FORWARD_SET_EDGE_TRIGGER (1 << 2)
+ __u32 irq_number_host;
+ __u32 count;
+ __u8 eventfd[];
+};
+
+/* ---- IOCTLs for Platform IRQ Forwarding fd (/dev/plat-irq-forward) ---- */
+#define PLAT_IRQ_FORWARD_SET _IO(PLAT_IRQ_FORWARD_TYPE, PLAT_IRQ_FORWARD_BASE + 0)
+
+/* *********************************************************************** */
+
+#endif /* _UAPIPLATIRQFORWARD_H */
diff --git a/virt/lib/Kconfig b/virt/lib/Kconfig
index 2d9523b7155e..68d911ec54cc 100644
--- a/virt/lib/Kconfig
+++ b/virt/lib/Kconfig
@@ -1,3 +1,6 @@
# SPDX-License-Identifier: GPL-2.0-only
config IRQ_BYPASS_MANAGER
tristate
+
+config PLAT_IRQ_FORWARD
+ tristate "Enable forwarding arbitrary platform IRQs to guest in KVM"
diff --git a/virt/lib/Makefile b/virt/lib/Makefile
index bd7f9a78bb6b..633e793b19a5 100644
--- a/virt/lib/Makefile
+++ b/virt/lib/Makefile
@@ -1,2 +1,3 @@
# SPDX-License-Identifier: GPL-2.0-only
obj-$(CONFIG_IRQ_BYPASS_MANAGER) += irqbypass.o
+obj-$(CONFIG_PLAT_IRQ_FORWARD) += platirqforward.o plat_irqfd.o
diff --git a/virt/lib/plat_irqfd.c b/virt/lib/plat_irqfd.c
new file mode 100644
index 000000000000..1c9637f89db4
--- /dev/null
+++ b/virt/lib/plat_irqfd.c
@@ -0,0 +1,146 @@
+#include <linux/file.h>
+#include <linux/vfio.h>
+#include <linux/eventfd.h>
+#include <linux/slab.h>
+#include <uapi/linux/platirqforward.h>
+#include <linux/plat_irqfd.h>
+
+static struct workqueue_struct *vfio_irqfd_cleanup_wq;
+static DEFINE_SPINLOCK(plat_irqfd_lock);
+
+static void plat_irqfd_deactivate(struct plat_irq_forward_irqfd *plat_irqfd)
+{
+ queue_work(vfio_irqfd_cleanup_wq, &plat_irqfd->shutdown);
+}
+
+static int plat_irqfd_wakeup(wait_queue_entry_t *wait, unsigned mode, int sync, void *key)
+{
+ struct plat_irq_forward_irqfd *plat_irqfd = container_of(wait, struct plat_irq_forward_irqfd, wait);
+ __poll_t flags = key_to_poll(key);
+
+ if (flags & EPOLLIN) {
+ /* An event has been signaled, call function */
+ if (!plat_irqfd->handler ||
+ plat_irqfd->handler(NULL, plat_irqfd->data))
+ printk(KERN_EMERG "handler failed\n");
+ }
+
+ if (flags & EPOLLHUP) {
+ unsigned long flags;
+ spin_lock_irqsave(&plat_irqfd_lock, flags);
+
+ /*
+ * The eventfd is closing, if the plat_irqfd has not yet been
+ * queued for release, as determined by testing whether the
+ * plat_irqfd pointer to it is still valid, queue it now. As
+ * with kvm irqfds, we know we won't race against the plat_irqfd
+ * going away because we hold the lock to get here.
+ */
+ if (*(plat_irqfd->pirqfd) == plat_irqfd) {
+ *(plat_irqfd->pirqfd) = NULL;
+ plat_irqfd_deactivate(plat_irqfd);
+ }
+
+ spin_unlock_irqrestore(&plat_irqfd_lock, flags);
+ }
+
+ return 0;
+}
+
+
+static void plat_irqfd_ptable_queue_proc(struct file *file,
+ wait_queue_head_t *wqh, poll_table *pt)
+{
+ struct plat_irq_forward_irqfd *plat_irqfd = container_of(pt, struct plat_irq_forward_irqfd, pt);
+ add_wait_queue(wqh, &plat_irqfd->wait);
+}
+
+static void plat_irqfd_shutdown(struct work_struct *work)
+{
+ struct plat_irq_forward_irqfd *plat_irqfd = container_of(work, struct plat_irq_forward_irqfd, shutdown);
+ u64 cnt;
+
+ eventfd_ctx_remove_wait_queue(plat_irqfd->eventfd, &plat_irqfd->wait, &cnt);
+ eventfd_ctx_put(plat_irqfd->eventfd);
+
+ kfree(plat_irqfd);
+}
+
+int plat_irq_forward_irqfd_enable(int (*handler)(void *, void *), void *data, struct plat_irq_forward_irqfd **pirqfd, int fd)
+{
+ struct fd irqfd;
+ struct eventfd_ctx *ctx;
+ struct plat_irq_forward_irqfd *plat_irqfd;
+ int ret = 0;
+ unsigned int events;
+
+ plat_irqfd = kzalloc(sizeof(*plat_irqfd), GFP_KERNEL);
+ if (!plat_irqfd)
+ return -ENOMEM;
+
+ plat_irqfd->pirqfd = pirqfd;
+ plat_irqfd->handler = handler;
+ plat_irqfd->data = data;
+
+ // shutdown causes crash
+ INIT_WORK(&plat_irqfd->shutdown, plat_irqfd_shutdown);
+
+ irqfd = fdget(fd);
+ if (!irqfd.file) {
+ ret = -EBADF;
+ goto err_fd;
+ }
+
+ ctx = eventfd_ctx_fileget(irqfd.file);
+ if (IS_ERR(ctx)) {
+ ret = PTR_ERR(ctx);
+ goto err_ctx;
+ }
+
+ plat_irqfd->eventfd = ctx;
+
+ // plat_irqfds can be released by closing the eventfd or directly
+ // through ioctl. These are both done through a workqueue, so
+ // we update the pointer to the plat_irqfd under lock to avoid
+ // pushing multiple jobs to release the same plat_irqfd.
+ spin_lock_irq(&plat_irqfd_lock);
+
+ if (*pirqfd) {
+ printk(KERN_EMERG "pirqfd should be NULL. BUG!\n");
+ spin_unlock_irq(&plat_irqfd_lock);
+ ret = -EBUSY;
+ goto err_busy;
+ }
+ *pirqfd = plat_irqfd;
+
+ spin_unlock_irq(&plat_irqfd_lock);
+
+ // Install our own custom wake-up handling so we are notified via
+ // a callback whenever someone signals the underlying eventfd.
+ init_waitqueue_func_entry(&plat_irqfd->wait, plat_irqfd_wakeup);
+ init_poll_funcptr(&plat_irqfd->pt, plat_irqfd_ptable_queue_proc);
+
+ events = irqfd.file->f_op->poll(irqfd.file, &plat_irqfd->pt);
+
+ // Check if there was an event already pending on the eventfd
+ // before we registered and trigger it as if we didn't miss it.
+ if (events & POLLIN) {
+ if (!handler || handler(NULL, data))
+ printk(KERN_EMERG "handler failed\n");
+ }
+
+ // Do not drop the file until the irqfd is fully initialized,
+ // otherwise we might race against the POLLHUP.
+ fdput(irqfd);
+
+ return 0;
+err_busy:
+ eventfd_ctx_put(ctx);
+err_ctx:
+ fdput(irqfd);
+err_fd:
+ kfree(plat_irqfd);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(plat_irq_forward_plat_irqfd_enable);
diff --git a/virt/lib/platirqforward.c b/virt/lib/platirqforward.c
new file mode 100644
index 000000000000..22ca24a27ad0
--- /dev/null
+++ b/virt/lib/platirqforward.c
@@ -0,0 +1,289 @@
+#include <linux/cdev.h>
+#include <linux/compat.h>
+#include <linux/file.h>
+#include <linux/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/sched.h>
+#include <linux/vfio.h>
+#include <linux/eventfd.h>
+#include <linux/delay.h>
+#include <uapi/linux/platirqforward.h>
+#include <linux/plat_irqfd.h>
+
+#define VERSION "0.1"
+#define AUTHOR "Micah Morton <mortonm@chromium.org>"
+#define DESC "Platform IRQ Forwarding"
+
+MODULE_VERSION(VERSION);
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR(AUTHOR);
+MODULE_DESCRIPTION(DESC);
+MODULE_ALIAS_MISCDEV(PLAT_IRQ_FORWARD_MINOR);
+MODULE_ALIAS("devname:plat-irq-forward");
+
+static LIST_HEAD(level_triggered_irqs);
+static LIST_HEAD(edge_triggered_irqs);
+
+
+static int plat_irq_forward_unmask_handler_level(void *opaque, void *level)
+{
+ unsigned long flags;
+ struct plat_irq_forward_level_triggered *l = (struct plat_irq_forward_level_triggered *) level;
+
+ spin_lock_irqsave(&(l->spinlock), flags);
+ if (l->is_masked) {
+ enable_irq(l->irq_num);
+ l->is_masked = false;
+ }
+ spin_unlock_irqrestore(&(l->spinlock), flags);
+ return 0;
+}
+
+
+static irqreturn_t plat_irq_forward_handler_level(int irq, void *level)
+{
+ unsigned long flags;
+ int ret = IRQ_NONE;
+ struct plat_irq_forward_level_triggered *l = (struct plat_irq_forward_level_triggered *) level;
+ spin_lock_irqsave(&(l->spinlock), flags);
+
+ disable_irq_nosync(irq);
+ l->is_masked = true;
+ ret = IRQ_HANDLED;
+
+ spin_unlock_irqrestore(&(l->spinlock), flags);
+
+ if (ret == IRQ_HANDLED)
+ eventfd_signal(l->trigger, 1);
+
+ return ret;
+}
+
+static irqreturn_t plat_irq_forward_handler_edge(int irq, void *edge)
+{
+ eventfd_signal(((struct plat_irq_forward_edge_triggered*)edge)->trigger, 1);
+
+ return IRQ_HANDLED;
+}
+
+static int plat_irq_forward_set_level_trigger(void *data, uint32_t irq_number_host, struct plat_irq_forward_level_triggered *level)
+{
+ int32_t fd;
+ struct eventfd_ctx *trigger;
+ int ret;
+
+ fd = *(int32_t *)data;
+
+ if (fd < 0) /* Disable only */
+ return 0;
+
+ trigger = eventfd_ctx_fdget(fd);
+ if (IS_ERR(trigger)) {
+ return PTR_ERR(trigger);
+ }
+
+ level->trigger = trigger;
+ spin_lock_init(&(level->spinlock));
+
+ ret = request_irq(irq_number_host, plat_irq_forward_handler_level, 0, "level-triggered-irq", level);
+ if (ret) {
+ level->trigger = NULL;
+ eventfd_ctx_put(trigger);
+ return ret;
+ }
+
+ return 0;
+
+}
+
+static int plat_irq_forward_set_level_unmask(void *data, struct plat_irq_forward_level_triggered *level)
+{
+ int32_t fd;
+ fd = *(int32_t *)data;
+
+ if (fd >= 0)
+ return plat_irq_forward_irqfd_enable(plat_irq_forward_unmask_handler_level, level, &(level->unmask), fd);
+ return -1;
+}
+
+static int plat_irq_forward_set_edge_trigger(void *data, uint32_t irq_number_host, struct plat_irq_forward_edge_triggered *edge)
+{
+ struct eventfd_ctx *trigger;
+ int ret;
+ int32_t fd;
+ fd = *(int32_t *)data;
+
+ if (fd < 0) /* Disable only */
+ return 0;
+
+ trigger = eventfd_ctx_fdget(fd);
+ if (IS_ERR(trigger)) {
+ return PTR_ERR(trigger);
+ }
+
+ edge->trigger = trigger;
+
+ ret = request_irq(irq_number_host, plat_irq_forward_handler_edge, IRQF_SHARED, "edge-triggered-irq", edge);
+ if (ret) {
+ edge->trigger = NULL;
+ eventfd_ctx_put(trigger);
+ return ret;
+ }
+
+ return 0;
+}
+
+
+int platform_set_irqs_ioctl_level_trigger(uint32_t irq_number_host, void *data) {
+
+ struct plat_irq_forward_level_triggered *level_irq = kzalloc(sizeof(struct plat_irq_forward_level_triggered), GFP_KERNEL);
+ if (!level_irq)
+ return -ENOMEM;
+ level_irq->trigger = NULL;
+ level_irq->irq_num = irq_number_host;
+ level_irq->unmask = NULL;
+ level_irq->is_masked = true;
+ list_add(&(level_irq->list), &level_triggered_irqs);
+
+ return plat_irq_forward_set_level_trigger(data, irq_number_host, level_irq);
+}
+
+int platform_set_irqs_ioctl_level_unmask(uint32_t irq_number_host, void *data) {
+
+ struct list_head* position = NULL;
+ struct plat_irq_forward_level_triggered *level_irq = NULL;
+ // We must already have a trigger for the IRQ before we add an unmask
+ list_for_each(position, &level_triggered_irqs) {
+ level_irq = list_entry(position, struct plat_irq_forward_level_triggered, list);
+ if (level_irq->irq_num == irq_number_host)
+ return plat_irq_forward_set_level_unmask(data, level_irq);
+ }
+
+ return -1;
+}
+
+int platform_set_irqs_ioctl_edge_trigger(uint32_t irq_number_host, void *data) {
+
+ struct plat_irq_forward_edge_triggered *edge_irq = kzalloc(sizeof(struct plat_irq_forward_edge_triggered), GFP_KERNEL);
+ if (!edge_irq)
+ return -ENOMEM;
+ edge_irq->trigger = NULL;
+ edge_irq->irq_num = irq_number_host;
+ list_add(&(edge_irq->list), &edge_triggered_irqs);
+
+ return plat_irq_forward_set_edge_trigger(data, irq_number_host, edge_irq);
+}
+
+int plat_irq_forward_ioctl(void *device_data, unsigned long arg)
+{
+ u8 *data = NULL;
+ unsigned long minsz;
+ struct plat_irq_forward_set hdr;
+
+
+ minsz = offsetofend(struct plat_irq_forward_set, count);
+
+ if (copy_from_user(&hdr, (void __user *)arg, minsz))
+ return -EFAULT;
+
+ data = memdup_user((void __user *)(arg + minsz), sizeof(int32_t));
+ if (IS_ERR(data))
+ return PTR_ERR(data);
+
+ switch (hdr.action_flags)
+ {
+ case PLAT_IRQ_FORWARD_SET_LEVEL_TRIGGER_EVENTFD:
+ return platform_set_irqs_ioctl_level_trigger(hdr.irq_number_host, data);
+ case PLAT_IRQ_FORWARD_SET_LEVEL_UNMASK_EVENTFD:
+ return platform_set_irqs_ioctl_level_unmask(hdr.irq_number_host, data);
+ case PLAT_IRQ_FORWARD_SET_EDGE_TRIGGER:
+ return platform_set_irqs_ioctl_edge_trigger(hdr.irq_number_host, data);
+ default:
+ return -EINVAL;
+ }
+
+ kfree(data);
+ return 0;
+}
+
+/**
+ * Platform IRQ Forwarding fd, /dev/plat-irq-forward
+ */
+static long plat_irq_forward_fops_unl_ioctl(struct file *filep,
+ unsigned int cmd, unsigned long arg)
+{
+ long ret = -EINVAL;
+
+ switch (cmd) {
+ case PLAT_IRQ_FORWARD_SET:
+ ret = (long) plat_irq_forward_ioctl(filep, arg);
+ break;
+ default:
+ ret = -EINVAL;
+ }
+
+ return ret;
+}
+
+#ifdef CONFIG_COMPAT
+static long plat_irq_forward_fops_compat_ioctl(struct file *filep,
+ unsigned int cmd, unsigned long arg)
+{
+ arg = (unsigned long)compat_ptr(arg);
+ return plat_irq_forward_fops_unl_ioctl(filep, cmd, arg);
+}
+#endif /* CONFIG_COMPAT */
+
+static int plat_irq_forward_fops_open(struct inode *inode, struct file *filep)
+{
+ return 0;
+}
+
+static int plat_irq_forward_fops_release(struct inode *inode, struct file *filep)
+{
+ return 0;
+}
+
+static const struct file_operations plat_irq_forward_fops = {
+ .owner = THIS_MODULE,
+ .open = plat_irq_forward_fops_open,
+ .release = plat_irq_forward_fops_release,
+ .unlocked_ioctl = plat_irq_forward_fops_unl_ioctl,
+#ifdef CONFIG_COMPAT
+ .compat_ioctl = plat_irq_forward_fops_compat_ioctl,
+#endif
+};
+
+static struct miscdevice plat_irq_forward_dev = {
+ .minor = PLAT_IRQ_FORWARD_MINOR,
+ .name = "plat-irq-forward",
+ .fops = &plat_irq_forward_fops,
+ .nodename = "plat-irq-forward",
+ .mode = S_IRUGO | S_IWUGO,
+};
+
+static int __init plat_irq_forward_init(void)
+{
+ int ret;
+
+ ret = misc_register(&plat_irq_forward_dev);
+ if (ret) {
+ pr_err("plat-irq-forward: misc device register failed\n");
+ return ret;
+ }
+
+ pr_info(DESC " version: " VERSION "\n");
+
+ return 0;
+}
+
+// TODO: cleanup/free/disconnect stuff
+static void __exit plat_irq_forward_cleanup(void)
+{
+ misc_deregister(&plat_irq_forward_dev);
+}
+
+module_init(plat_irq_forward_init);
+module_exit(plat_irq_forward_cleanup);
--
2.25.0.265.gbab2e86ba0-goog
^ permalink raw reply related
* Re: BUG: git clean -d cannot remove files from read-only directories
From: Junio C Hamano @ 2020-02-20 18:47 UTC (permalink / raw)
To: Adam Milazzo; +Cc: git@vger.kernel.org
In-Reply-To: <xmqqo8ttaz3p.fsf@gitster-ct.c.googlers.com>
Junio C Hamano <gitster@pobox.com> writes:
> Adam Milazzo <Adam.Milazzo@microsoft.com> writes:
>
>> Repro steps:
>> 1. git init
>> 2. mkdir d
>> 3. touch d/a
>> 4. chmod -w d
>> 5. git clean -fd
>>
>> Actual result:
>> Git doesn't remove anything, saying "warning: failed to remove d/a".
>>
>> Expected result:
>> Git should remove the subdirectory 'd' along with its
>> contents. Note that git can remove a read-only file (touch b;
>> chmod -w b; git clean -f) with no problem.
>
> It is how UNIX-like filesystem works, isn't it?
>
> As long as a directory D is writable/executable, any regular file
> inside it can be removed regardless of the perm-bits of the file.
>
> mkdir d
> touch d/a
> chmod -w d
> rm d/a
>
> would not let you remove the file d/a from d/, exactly because you
> cannot modify d/ (it is not writable).
I realize that the above is probably a bit too terse for those who
are not familiar with the UNIX/POSIX-like filesystems.
A directory is a record of what files (and subdirectories) are in
it. If you add a file to or remove a file from a directory, you
are updating that record---so you need to be able to write to the
directory. You do not have to be able to write (or even to read for
that matter) to the file you are adding or removing to the directory,
because adding or removing a file from a directory involves only
updating the record kept in the directory about what files and
subdirectories are in that directory (hence you need to be able to
write to the directory---but perm bits on the files you are adding
or removing do not play a role in this operation).
A collorary is that modifying an existing file in a directory does
not have to change what is recorded in the directory---what files
are in the directory does not change. You only need to be able to
write the file, so you can edit a file in a read-only directory.
Note that some editors, however, implement an "edit" as "rename the
original file as a backup, create a new copy of a file, and edit
that new copy". With such an editor, you cannot "edit" a writable
file in a read-only directory and the reason would by now be obvious
to you once you understand the explanation above. Both renaming the
original and creating the new copy would be updating what files are
in the directory, requiring you to be able to write to the directory.
Hope this helps.
^ permalink raw reply
* RE: [RFC PATCH v4 02/25] ice: Create and register virtual bus for RDMA
From: Ertman, David M @ 2020-02-20 18:48 UTC (permalink / raw)
To: Jason Gunthorpe, Kirsher, Jeffrey T
Cc: davem@davemloft.net, gregkh@linuxfoundation.org,
netdev@vger.kernel.org, linux-rdma@vger.kernel.org,
nhorman@redhat.com, sassmann@redhat.com, Nguyen, Anthony L,
Bowers, AndrewX
In-Reply-To: <20200214203932.GY31668@ziepe.ca>
> -----Original Message-----
> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Friday, February 14, 2020 12:40 PM
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> Cc: davem@davemloft.net; gregkh@linuxfoundation.org; Ertman, David M
> <david.m.ertman@intel.com>; netdev@vger.kernel.org; linux-
> rdma@vger.kernel.org; nhorman@redhat.com; sassmann@redhat.com;
> Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Bowers, AndrewX
> <andrewx.bowers@intel.com>
> Subject: Re: [RFC PATCH v4 02/25] ice: Create and register virtual bus for
> RDMA
>
> On Wed, Feb 12, 2020 at 11:14:01AM -0800, Jeff Kirsher wrote:
> > +/**
> > + * ice_init_peer_devices - initializes peer devices
> > + * @pf: ptr to ice_pf
> > + *
> > + * This function initializes peer devices on the virtual bus.
> > + */
> > +int ice_init_peer_devices(struct ice_pf *pf)
> > +{
> > + struct ice_vsi *vsi = pf->vsi[0];
> > + struct pci_dev *pdev = pf->pdev;
> > + struct device *dev = &pdev->dev;
> > + int status = 0;
> > + int i;
> > +
> > + /* Reserve vector resources */
> > + status = ice_reserve_peer_qvector(pf);
> > + if (status < 0) {
> > + dev_err(dev, "failed to reserve vectors for peer drivers\n");
> > + return status;
> > + }
> > + for (i = 0; i < ARRAY_SIZE(ice_peers); i++) {
> > + struct ice_peer_dev_int *peer_dev_int;
> > + struct ice_peer_drv_int *peer_drv_int;
> > + struct iidc_qos_params *qos_info;
> > + struct iidc_virtbus_object *vbo;
> > + struct msix_entry *entry = NULL;
> > + struct iidc_peer_dev *peer_dev;
> > + struct virtbus_device *vdev;
> > + int j;
> > +
> > + /* structure layout needed for container_of's looks like:
> > + * ice_peer_dev_int (internal only ice peer superstruct)
> > + * |--> iidc_peer_dev
> > + * |--> *ice_peer_drv_int
> > + *
> > + * iidc_virtbus_object (container_of parent for vdev)
> > + * |--> virtbus_device
> > + * |--> *iidc_peer_dev (pointer from internal struct)
> > + *
> > + * ice_peer_drv_int (internal only peer_drv struct)
> > + */
> > + peer_dev_int = devm_kzalloc(dev, sizeof(*peer_dev_int),
> > + GFP_KERNEL);
> > + if (!peer_dev_int)
> > + return -ENOMEM;
> > +
> > + vbo = kzalloc(sizeof(*vbo), GFP_KERNEL);
> > + if (!vbo) {
> > + devm_kfree(dev, peer_dev_int);
> > + return -ENOMEM;
> > + }
> > +
> > + peer_drv_int = devm_kzalloc(dev, sizeof(*peer_drv_int),
> > + GFP_KERNEL);
>
> To me, this looks like a lifetime mess. All these devm allocations
> against the parent object are being referenced through the vbo with a
> different kref lifetime. The whole thing has very unclear semantics
> who should be cleaning up on error
Will cover this at the end after addressing your following points =)
In my reply, I am going to refer to the kernel object that is registering the
virtbus_device(s) as KO_device and the kernel object that is registering
the virtbus_driver(s) as KO_driver.
>
> > + if (!peer_drv_int) {
> > + devm_kfree(dev, peer_dev_int);
> > + kfree(vbo);
>
> ie here we free two things
At this point in the init flow for KO_device, there has only been kallocs done,
no device has been registered with virtbus. So, only memory cleanup is
required.
>
> > + return -ENOMEM;
> > + }
> > +
> > + pf->peers[i] = peer_dev_int;
> > + vbo->peer_dev = &peer_dev_int->peer_dev;
> > + peer_dev_int->peer_drv_int = peer_drv_int;
> > + peer_dev_int->peer_dev.vdev = &vbo->vdev;
> > +
> > + /* Initialize driver values */
> > + for (j = 0; j < IIDC_EVENT_NBITS; j++)
> > + bitmap_zero(peer_drv_int->current_events[j].type,
> > + IIDC_EVENT_NBITS);
> > +
> > + mutex_init(&peer_dev_int->peer_dev_state_mutex);
> > +
> > + peer_dev = &peer_dev_int->peer_dev;
> > + peer_dev->peer_ops = NULL;
> > + peer_dev->hw_addr = (u8 __iomem *)pf->hw.hw_addr;
> > + peer_dev->peer_dev_id = ice_peers[i].id;
> > + peer_dev->pf_vsi_num = vsi->vsi_num;
> > + peer_dev->netdev = vsi->netdev;
> > +
> > + peer_dev_int->ice_peer_wq =
> > + alloc_ordered_workqueue("ice_peer_wq_%d",
> WQ_UNBOUND,
> > + i);
> > + if (!peer_dev_int->ice_peer_wq)
> > + return -ENOMEM;
>
> Here we free nothing
This is a miss on my part. At this point we should keep consistent and free the memory
that has been allocated as we unwind.
>
> > +
> > + peer_dev->pdev = pdev;
> > + qos_info = &peer_dev->initial_qos_info;
> > +
> > + /* setup qos_info fields with defaults */
> > + qos_info->num_apps = 0;
> > + qos_info->num_tc = 1;
> > +
> > + for (j = 0; j < IIDC_MAX_USER_PRIORITY; j++)
> > + qos_info->up2tc[j] = 0;
> > +
> > + qos_info->tc_info[0].rel_bw = 100;
> > + for (j = 1; j < IEEE_8021QAZ_MAX_TCS; j++)
> > + qos_info->tc_info[j].rel_bw = 0;
> > +
> > + /* for DCB, override the qos_info defaults. */
> > + ice_setup_dcb_qos_info(pf, qos_info);
> > +
> > + /* make sure peer specific resources such as msix_count and
> > + * msix_entries are initialized
> > + */
> > + switch (ice_peers[i].id) {
> > + case IIDC_PEER_RDMA_ID:
> > + if (test_bit(ICE_FLAG_IWARP_ENA, pf->flags)) {
> > + peer_dev->msix_count = pf-
> >num_rdma_msix;
> > + entry = &pf->msix_entries[pf-
> >rdma_base_vector];
> > + }
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + peer_dev->msix_entries = entry;
> > + ice_peer_state_change(peer_dev_int,
> ICE_PEER_DEV_STATE_INIT,
> > + false);
> > +
> > + vdev = &vbo->vdev;
> > + vdev->name = ice_peers[i].name;
> > + vdev->release = ice_peer_vdev_release;
> > + vdev->dev.parent = &pdev->dev;
> > +
> > + status = virtbus_dev_register(vdev);
> > + if (status) {
> > + virtbus_dev_unregister(vdev);
> > + vdev = NULL;
>
> Here we double unregister and free nothing.
>
> You need to go through all of this really carefully and make some kind
> of sane lifetime model and fix all the error unwinding :(
Thanks for catching this. A failure in virtbus_register_device() does
*not* require a call virtbus_unregister_device. The failure path for the
register function handles this. Also, we need to remain consistent with freeing
on unwind.
>
> Why doesn't the release() function of vbo trigger the free of all this
> peer related stuff?
>
> Use a sane design model of splitting into functions to allocate single
> peices of memory, goto error unwind each function, and build things up
> properly.
>
> Jason
I am going to add this to the documentation to record the following information.
The KO_device is responsible for allocating the memory for the virtbus_device
and keeping it viable for the lifetime of the KO_device. KO_device will call
virtbus_register_device to start using the virtbus_device, and KO_device is
responslble for calling virtbus_unregister_device either on KO_device's exit
path (remove/shutdown) or when it is done using the virtbus subsystem.
The KO_driver is responsible for allocating the memory for the virtbus_driver
and keeping it viable for the lifetime of the KO_driver. KO_driver will call
virtbus_register_driver to start using the virtbus_driver, and KO_driver is
responsible for calling virtbus_unregister_driver either on KO_driver's exit
path (remove/shutdown) or when it is done using the virtbus subsystem.
The premise is that the KO_device and KO_driver can load and unload multiple
times and they can reconnect to each other through the virtbus on each
occurrence of their reloads. So one example of a flow looks like the following:
- KO_device loads (order of KO_device and KO_driver loading is irrelevant)
- KO_device allocates memory for virtbus_device(s) it expects to use and
any backing memory it is going to use to interact with KO_driver.
- KO_device performs virtbus_register_device() which is the *only* place
a device_initialize() is performed for virtbus_device.
- KO_driver loads
- KO_driver allocates memory for virtbus_driver(s) it expects to use and
any backing memory it expects to use to interact with KO_device
- KO_driver performs virtbus_register_driver()
- virtbus matches virtbus_device and virtbus_driver and calls the
virtbus_drivers's probe()
- KO_driver and KO_device interact with each other however they choose to do so.
- KO_device (for example) receives a call to its remove callback
- KO_device's unload path severs any interaction the KO_device and KO_driver
were having - implementation dependant
- KO_device's unload path is required to perform a call to
virtbus_unregister_device(). virtbus_unregister_device() is the *only*
place a put_device() is performed.
- KO_device's unload path frees memory associated with the virtbus_device
- vitbus calls KO_drivers's .remove callback defined for the virtbus_driver
So, the lifespan of the virtbus_device is controlled by KO_device and the
lifespan of virtbus_driver is controlled by KO_driver.
It is required for the KO's to "allocate -> register -> unregister -> free"
virtbus objects.
-DaveE
^ permalink raw reply
* Re: [PATCH v2 08/11] commit-graph: reuse existing Bloom filters during write.
From: Jakub Narebski @ 2020-02-20 18:48 UTC (permalink / raw)
To: Garima Singh via GitGitGadget
Cc: git, Derrick Stolee, SZEDER Gábor, Jonathan Tan,
Jeff Hostetler, Taylor Blau, Jeff King, Garima Singh,
Christian Couder, Emily Shaffer, Junio C Hamano, Garima Singh
In-Reply-To: <b20c8d2b2096bf10fe1a5f37a5181c57873a9676.1580943390.git.gitgitgadget@gmail.com>
"Garima Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Garima Singh <garima.singh@microsoft.com>
>
> Read previously computed Bloom filters from the commit-graph file if
> possible to avoid recomputing during commit-graph write.
All right, what is written makes sense for this point in patch series.
But it my opinion it is more important to state that this commit adds
"parsing" of the Bloom filter data from commit-graph file. This means
that it needs to be calculated only once, then stored in commit-graph,
ready to be re-used.
>
> See Documentation/technical/commit-graph-format for the format in which
> the Bloom filter information is written to the commit graph file.
>
> To read Bloom filter for a given commit with lexicographic position
> 'i' we need to:
> 1. Read BIDX[i] which essentially gives us the starting index in BDAT for
> filter of commit i+1. It is essentially the index past the end
> of the filter of commit i. It is called end_index in the code.
>
> 2. For i>0, read BIDX[i-1] which will give us the starting index in BDAT
> for filter of commit i. It is called the start_index in the code.
> For the first commit, where i = 0, Bloom filter data starts at the
> beginning, just past the header in the BDAT chunk. Hence, start_index
> will be 0.
>
> 3. The length of the filter will be end_index - start_index, because
> BIDX[i] gives the cumulative 8-byte words including the ith
> commit's filter.
>
> We toggle whether Bloom filters should be recomputed based on the
> compute_if_null flag.
Nitpick: the flag (the parameter) is called compute_if_not_present, not
compute_if_null.
All right, this explanation is nice and clear.
>
> Helped-by: Derrick Stolee <dstolee@microsoft.com>
> Signed-off-by: Garima Singh <garima.singh@microsoft.com>
> ---
> bloom.c | 49 ++++++++++++++++++++++++++++++++++++++++++-
> bloom.h | 4 +++-
> commit-graph.c | 7 ++++---
> t/helper/test-bloom.c | 2 +-
> 4 files changed, 56 insertions(+), 6 deletions(-)
>
> diff --git a/bloom.c b/bloom.c
> index 818382c03b..90d84dc713 100644
> --- a/bloom.c
> +++ b/bloom.c
> @@ -1,5 +1,7 @@
> #include "git-compat-util.h"
> #include "bloom.h"
> +#include "commit.h"
> +#include "commit-slab.h"
> #include "commit-graph.h"
> #include "object-store.h"
> #include "diff.h"
> @@ -127,8 +129,39 @@ void add_key_to_filter(struct bloom_key *key,
> }
> }
>
> +static int load_bloom_filter_from_graph(struct commit_graph *g,
> + struct bloom_filter *filter,
> + struct commit *c)
> +{
> + uint32_t lex_pos, start_index, end_index;
> +
> + while (c->graph_pos < g->num_commits_in_base)
> + g = g->base_graph;
> +
> + /* The commit graph commit 'c' lives in doesn't carry bloom filters. */
> + if (!g->chunk_bloom_indexes)
> + return 0;
> +
> + lex_pos = c->graph_pos - g->num_commits_in_base;
All right, this finds lexicographical position of the commit following
the chain of incremental commit-graph files, and also check if the
commit-graph fragment that contains the commit in question has Bloom
filter data included.
> +
> + end_index = get_be32(g->chunk_bloom_indexes + 4 * lex_pos);
> +
> + if (lex_pos)
Wouldn't it be better to be more explicit, and write
+ if (lex_pos > 0)
> + start_index = get_be32(g->chunk_bloom_indexes + 4 * (lex_pos - 1));
> + else
> + start_index = 0;
All right, here we find start_index and end_index.
It might be good idea to at least assert() that start_index <= end_index,
though that should not happen (that is why I propose for this check to
be compiled on only for debug builds).
> +
> + filter->len = end_index - start_index;
> + filter->data = (uint64_t *)(g->chunk_bloom_data +
> + sizeof(uint64_t) * start_index +
> + BLOOMDATA_CHUNK_HEADER_SIZE);
All right, nice use of constant.
> +
> + return 1;
> +}
> +
> struct bloom_filter *get_bloom_filter(struct repository *r,
> - struct commit *c)
> + struct commit *c,
> + int compute_if_not_present)
> {
> struct bloom_filter *filter;
> struct bloom_filter_settings settings = DEFAULT_BLOOM_FILTER_SETTINGS;
> @@ -141,6 +174,20 @@ struct bloom_filter *get_bloom_filter(struct repository *r,
>
> filter = bloom_filter_slab_at(&bloom_filters, c);
>
> + if (!filter->data) {
> + load_commit_graph_info(r, c);
> + if (c->graph_pos != COMMIT_NOT_FROM_GRAPH &&
> + r->objects->commit_graph->chunk_bloom_indexes) {
All right, the limitation that the top layer of incremental commit graph
needs to have Bloom filters enabled for it to be even considered is
reasonable tradeoff, in my opinion.
> + if (load_bloom_filter_from_graph(r->objects->commit_graph, filter, c))
> + return filter;
> + else
> + return NULL;
If it should have filter, return it, otherwise return NULL.
I wonder however when it can return NULL (and whether it should compute
Bloom filters if required instead).
> + }
> + }
> +
> + if (filter->data || !compute_if_not_present)
> + return filter;
If we have filter from slab, return it. All right.
However, according to documentation contained in comments in
commit-slab.h, bloom_filter_slab_at() will allocate the location to
store the data, and return freshly allocated memory... fortunately it
uses xcalloc() so returned bloom_filter would have ->len == 0 and
->data == 0.
> +
> repo_diff_setup(r, &diffopt);
> diffopt.flags.recursive = 1;
> diffopt.max_changes = max_changes;
> diff --git a/bloom.h b/bloom.h
> index 7f40c751f7..76f8a9ad0c 100644
> --- a/bloom.h
> +++ b/bloom.h
> @@ -13,6 +13,7 @@ struct bloom_filter_settings {
>
> #define DEFAULT_BLOOM_FILTER_SETTINGS { 1, 7, 10 }
> #define BITS_PER_WORD 64
> +#define BLOOMDATA_CHUNK_HEADER_SIZE 3*sizeof(uint32_t)
All right.
>
> /*
> * A bloom_filter struct represents a data segment to
> @@ -47,7 +48,8 @@ void add_key_to_filter(struct bloom_key *key,
> struct bloom_filter_settings *settings);
>
> struct bloom_filter *get_bloom_filter(struct repository *r,
> - struct commit *c);
> + struct commit *c,
> + int compute_if_not_present);
>
All right, adding new parameter (changing function signature).
> int bloom_filter_contains(struct bloom_filter *filter,
> struct bloom_key *key,
> diff --git a/commit-graph.c b/commit-graph.c
> index 4585b3b702..c0e9834bf2 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -1094,7 +1094,7 @@ static void write_graph_chunk_bloom_indexes(struct hashfile *f,
> ctx->commits.nr);
>
> while (list < last) {
> - struct bloom_filter *filter = get_bloom_filter(ctx->r, *list);
> + struct bloom_filter *filter = get_bloom_filter(ctx->r, *list, 0);
> cur_pos += filter->len;
> display_progress(progress, ++i);
> hashwrite_be32(f, cur_pos);
> @@ -1123,7 +1123,7 @@ static void write_graph_chunk_bloom_data(struct hashfile *f,
> hashwrite_be32(f, settings->bits_per_entry);
>
> while (list < last) {
> - struct bloom_filter *filter = get_bloom_filter(ctx->r, *list);
> + struct bloom_filter *filter = get_bloom_filter(ctx->r, *list, 0);
> display_progress(progress, ++i);
> hashwrite(f, filter->data, filter->len * sizeof(uint64_t));
> list++;
All right, if needed (that is, if '--changed-path' option from the
future commit is provided to 'git commit-graph write'),
compute_bloom_filters() would be called befor write_commit_graph_file(),
which in turn runs write_graph_chunk_bloom_index() and *_data().
Actually, when writing Bloom data chunks (BIDX and BDAT) we could have
requested recomputing filters if necessary: slab storage works as
memoization, so you would calculate Bloom filter data for each commit in
the commit-graph only once. And write_graph_chunk_bloom_indexes()
and write_graph_chunk_bloom_data() are called only if ctx->changed_paths
is true.
So it would work with
+ struct bloom_filter *filter = get_bloom_filter(ctx->r, *list, 1);
Only in the future we would really need to call with compute_if_not_present
parameter set to falsy value.
> @@ -1304,7 +1304,7 @@ static void compute_bloom_filters(struct write_commit_graph_context *ctx)
>
> for (i = 0; i < ctx->commits.nr; i++) {
> struct commit *c = sorted_by_pos[i];
> - struct bloom_filter *filter = get_bloom_filter(ctx->r, c);
> + struct bloom_filter *filter = get_bloom_filter(ctx->r, c, 1);
> ctx->total_bloom_filter_data_size += sizeof(uint64_t) * filter->len;
> display_progress(progress, i + 1);
> }
> @@ -2314,6 +2314,7 @@ void free_commit_graph(struct commit_graph *g)
> g->data = NULL;
> close(g->graph_fd);
> }
> + free(g->bloom_filter_settings);
> free(g->filename);
> free(g);
Shouldn't this fixup be added to earlier commit?
> }
> diff --git a/t/helper/test-bloom.c b/t/helper/test-bloom.c
> index 331957011b..9b4be97f75 100644
> --- a/t/helper/test-bloom.c
> +++ b/t/helper/test-bloom.c
> @@ -47,7 +47,7 @@ static void get_bloom_filter_for_commit(const struct object_id *commit_oid)
> struct bloom_filter *filter;
> setup_git_directory();
> c = lookup_commit(the_repository, commit_oid);
> - filter = get_bloom_filter(the_repository, c);
> + filter = get_bloom_filter(the_repository, c, 1);
> print_bloom_filter(filter);
> }
I would like to see some tests, but that needs to wait for patch that
adds --changed-paths option to the 'write' subcommand.
Things to be tested:
1. That after reading commit-graph with Bloom filter:
- that commit(s) in commit-graph have Bloom filter
- that commits outside commit-graph do not have Bloom filter
2. That incremental commit-graph feature works:
- for commits in deeper layer that have Bloom filter chunks
- for commits in deeper layer that do not have Bloom filter chunks
Best,
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH v26 10/22] x86/sgx: Linux Enclave Driver
From: Sean Christopherson @ 2020-02-20 18:48 UTC (permalink / raw)
To: Jordan Hand
Cc: Jarkko Sakkinen, linux-kernel, x86, linux-sgx, akpm, dave.hansen,
nhorman, npmccallum, haitao.huang, andriy.shevchenko, tglx,
kai.svahn, bp, josh, luto, kai.huang, rientjes, cedric.xing,
puiterwijk, linux-security-module, Suresh Siddha, Haitao Huang
In-Reply-To: <7738b3cf-fb32-5306-5740-59974444e327@linux.microsoft.com>
On Thu, Feb 20, 2020 at 10:33:36AM -0800, Jordan Hand wrote:
> On 2/20/20 10:13 AM, Sean Christopherson wrote:
> > There are essentially two paths we can take:
> >
> > 1) Exempt EPC pages from RIE during mmap()/mprotect(), i.e. don't add
> > PROT_EXEC for enclaves.
> >
> > 2) Punt the issue to userspace.
> >
> > Option (1) is desirable in some ways:
> >
> > - Enclaves will get an executable stack if and only if the loader/creator
> > intentionally configures it to have an executable stack.
> >
> > - Separates enclaves from the personality of the loader.
> >
> > - Userspace doesn't have to do anything for the common case of not
> > wanting an executable stack for its enclaves.
> >
> > The big down side to (1) is that it'd require an ugly hook in architecture
> > agnostic code. And arguably, it reduces the overall security of the
> > platform (more below).
> >
> > For (2), userspace has a few options:
> >
> > a) Tell the linker the enclave loader doesn't need RIE, either via a .note
> > in assembly files or via the global "-z noexecstack" flag.
> >
> > b) Spawn a separate process to run/map the enclave if the enclave loader
> > needs RIE.
> >
> > c) Require enclaves to allow PROT_EXEC on all pages. Note, this is an
> > absolutely terrible idea and only included for completeness.
> >
> > As shown by the lack of a mmap()/mprotect() hook in this series to squash
> > RIE, we chose option (2). Given that enclave loaders are not legacy code
> > and hopefully following decent coding practices, option (2a) should suffice
> > for all loaders. The security benefit mentioned above is that forcing
> > enclave loaders to squash RIE eliminates an exectuable stack as an attack
> > vector on the loader.
>
> I see your point and I do agree that there are security benefits to (2a)
> and I think we could do that for our loader. That said, it does concern
> me that this breaks perfectly valid userspace behavior. If a userspace
> process decides to use RIE, I don't know that the SGX driver should
> disobey that decision.
>
> So option (3) would be to just honor RIE for enclave pages and when page
> permissions are set to PROT_READ in sgx_encl_page_alloc and RIE is set,
> also add PROT_EXEC.
Ah, right, IIRC that idea also came up in our internal discussions. Note,
SGX would need to implement this option by checking for RIE in
sgx_encl_may_map(), as the process that built the enclave may not be the
same process that is running the enclave.
> I understand your concerns that this using RIE is bad security practice
> and I'm not convinced that (3) is the way to go, but from a philosophy
> perspective I don't know that the kernel should be in the business of
> stopping userspace from doing valid things.
>
> If option (3) can't/shouldn't be done for some reason, option (1) at
> least keeps from breaking expected userspace behavior. But I do agree
> that (1) is ugly to implement.
My biggest concern for allowing PROT_EXEC if RIE is that it would result
in #PF(SGX) (#GP on Skylake) due to an EPCM violation if the enclave
actually tried to execute from such a page. This isn't a problem for the
kernel as the fault will be reported cleanly through the vDSO (or get
delivered as a SIGSEGV if the enclave isn't entered through the vDSO), but
it's a bit weird for userspace as userspace will see the #PF(SGX) and
likely assume the EPC was lost, e.g. silently restart the enclave instead
of logging an error that the enclave is broken.
^ permalink raw reply
* Re: [PATCH v1 1/1] target/riscv: Correctly implement TSR trap
From: Alistair Francis @ 2020-02-20 18:41 UTC (permalink / raw)
To: Alistair Francis
Cc: qemu-devel@nongnu.org Developers, open list:RISC-V,
Palmer Dabbelt
In-Reply-To: <62fcfe747245cf8edcabcbe8f1f0b59be035fad6.1579584948.git.alistair.francis@wdc.com>
On Mon, Jan 20, 2020 at 9:43 PM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> As reported in: https://bugs.launchpad.net/qemu/+bug/1851939 we weren't
> correctly handling illegal instructions based on the value of MSTATUS_TSR
> and the current privledge level.
>
> This patch fixes the issue raised in the bug by raising an illegal
> instruction if TSR is set and we are in S-Mode.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
@Palmer Dabbelt Ping!
Alistair
> ---
> target/riscv/op_helper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index 331cc36232..eed8eea6f2 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -83,7 +83,7 @@ target_ulong helper_sret(CPURISCVState *env, target_ulong cpu_pc_deb)
> }
>
> if (env->priv_ver >= PRIV_VERSION_1_10_0 &&
> - get_field(env->mstatus, MSTATUS_TSR)) {
> + get_field(env->mstatus, MSTATUS_TSR) && !(env->priv >= PRV_M)) {
> riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> }
>
> --
> 2.24.1
>
^ permalink raw reply
* Re: [GSoC][RFC][PATCH 1/2] STRBUF_INIT_CONST: a new way to initialize strbuf
From: René Scharfe @ 2020-02-20 18:49 UTC (permalink / raw)
To: Johannes Sixt, Robear Selwans
Cc: git, brian m. carlson, Jeff King, Junio C Hamano, Derrick Stolee
In-Reply-To: <467c035f-c7cd-01e1-e64c-2c915610de01@kdbg.org>
Am 19.02.20 um 09:13 schrieb Johannes Sixt:
> Am 18.02.20 um 05:18 schrieb Robear Selwans:
>> A new function `STRBUF_INIT_CONST(const_str)` was added to allow for a
>> quick initialization of strbuf.
>>
>> Details:
>> Using `STRBUF_INIT_CONST(str)` creates a new struct of type `strbuf` and
>> initializes its `buf`, `len` and `alloc` as `str`, `strlen(str)` and
>> `0`, respectively.
>>
>> Use Case:
>> This is meant to be used to initialize strbufs with constant values and
>> thus, only allocating memory when needed.
>>
>> Usage Example:
>> ```
>> strbuf env_var = STRBUF_INIT_CONST("dev");
>> ```
>>
>> This was added according to the issue opened at [https://github.com/gitgitgadget/git/issues/398]
>
> I am not a friend of this change at all. Why do so many functions and
> strbuf instances have to pay a price (check for immutable string) for a
> feature that they are not using?
>
> As the macro is just intended for convenience, I suggest to implement it
> using strbuf_addstr() under the hood. That is much less code churn, and
> the price is paid only by the strbufs that actually use the feature.
I was also wondering what the benefits of this change might be. Saving
one line and thus increasing convenience slightly doesn't justify all
this added complexity. Saving an allocation in the following sequence
might be worthwhile:
struct strbuf sb = STRBUF_INIT;
strbuf_addstr(&sb, "foo");
/* Use sb without modifying it. */
strbuf_release(&sb); /* or leak it */
I found two examples of this pattern in the code, one in range-diff.c in
the function show_range_diff(), and below is a patch for getting rid of
the second one. Are there other reasons why we'd want that feature?
Could you perhaps include a patch that makes use of it in this series,
to highlight its benefits?
-- >8 --
Subject: [PATCH] commit-graph: use progress title directly
merge_commit_graphs() copies the (translated) progress message into a
strbuf and passes the copy to start_delayed_progress() at each loop
iteration. The latter function takes a string pointer, so let's avoid
the detour and hand the string to it directly. That's shorter, simpler
and slightly more efficient.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
commit-graph.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 656dd647d5..f013a84e29 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1657,19 +1657,15 @@ static void merge_commit_graphs(struct write_commit_graph_context *ctx)
{
struct commit_graph *g = ctx->r->objects->commit_graph;
uint32_t current_graph_number = ctx->num_commit_graphs_before;
- struct strbuf progress_title = STRBUF_INIT;
while (g && current_graph_number >= ctx->num_commit_graphs_after) {
current_graph_number--;
- if (ctx->report_progress) {
- strbuf_addstr(&progress_title, _("Merging commit-graph"));
- ctx->progress = start_delayed_progress(progress_title.buf, 0);
- }
+ if (ctx->report_progress)
+ ctx->progress = start_delayed_progress(_("Merging commit-graph"), 0);
merge_commit_graph(ctx, g);
stop_progress(&ctx->progress);
- strbuf_release(&progress_title);
g = g->base_graph;
}
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v1 1/1] target/riscv: Correctly implement TSR trap
From: Alistair Francis @ 2020-02-20 18:41 UTC (permalink / raw)
To: Alistair Francis
Cc: Palmer Dabbelt, open list:RISC-V,
qemu-devel@nongnu.org Developers
In-Reply-To: <62fcfe747245cf8edcabcbe8f1f0b59be035fad6.1579584948.git.alistair.francis@wdc.com>
On Mon, Jan 20, 2020 at 9:43 PM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> As reported in: https://bugs.launchpad.net/qemu/+bug/1851939 we weren't
> correctly handling illegal instructions based on the value of MSTATUS_TSR
> and the current privledge level.
>
> This patch fixes the issue raised in the bug by raising an illegal
> instruction if TSR is set and we are in S-Mode.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
@Palmer Dabbelt Ping!
Alistair
> ---
> target/riscv/op_helper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index 331cc36232..eed8eea6f2 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -83,7 +83,7 @@ target_ulong helper_sret(CPURISCVState *env, target_ulong cpu_pc_deb)
> }
>
> if (env->priv_ver >= PRIV_VERSION_1_10_0 &&
> - get_field(env->mstatus, MSTATUS_TSR)) {
> + get_field(env->mstatus, MSTATUS_TSR) && !(env->priv >= PRV_M)) {
> riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> }
>
> --
> 2.24.1
>
^ permalink raw reply
* Re: [PATCH] sound: Replace zero-length array with flexible-array member
From: Gustavo A. R. Silva @ 2020-02-20 18:52 UTC (permalink / raw)
To: Mark Brown
Cc: Jaroslav Kysela, Takashi Iwai, Liam Girdwood, Clemens Ladisch,
Takashi Sakamoto, Cezary Rojewski, Pierre-Louis Bossart, Jie Yang,
alsa-devel, linux-kernel
In-Reply-To: <20200220184544.GG3926@sirena.org.uk>
On 2/20/20 12:45, Mark Brown wrote:
> On Thu, Feb 20, 2020 at 07:24:20AM -0600, Gustavo A. R. Silva wrote:
>> The current codebase makes use of the zero-length array language
>> extension to the C90 standard, but the preferred mechanism to declare
>> variable-length types such as these ones is a flexible array member[1][2],
>> introduced in C99:
>
> Acked-by: Mark Brown <broonie@kernel.org>
>
Thanks, Mark.
--
Gustavo
^ permalink raw reply
* Re: [PATCH] trace-cmd: Add output to show "make test"
From: Tzvetomir Stoyanov @ 2020-02-20 18:50 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Linux Trace Devel
In-Reply-To: <20200220093622.6df7dafc@gandalf.local.home>
On Thu, Feb 20, 2020 at 4:36 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
>
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
>
> The unit tests are made with "make test". In order to let developers know of
> this option, have it displayed when just "make" is used.
>
Looks good to me, thanks Steven!
Acked-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> Makefile | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Makefile b/Makefile
> index 60cd1570..a3facaa9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -364,6 +364,7 @@ $(obj)/lib/traceevent/plugins/trace_python_dir: force
> show_gui_make:
> @echo "Note: to build the gui, type \"make gui\""
> @echo " to build man pages, type \"make doc\""
> + @echo " to build unit tests, type \"make test\""
>
> PHONY += show_gui_make
>
> --
> 2.20.1
>
--
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center
^ permalink raw reply
* Re: [PATCH] sound: Replace zero-length array with flexible-array member
From: Gustavo A. R. Silva @ 2020-02-20 18:52 UTC (permalink / raw)
To: Mark Brown
Cc: Pierre-Louis Bossart, Cezary Rojewski, alsa-devel, Jie Yang,
Takashi Iwai, Clemens Ladisch, Liam Girdwood, linux-kernel
In-Reply-To: <20200220184544.GG3926@sirena.org.uk>
On 2/20/20 12:45, Mark Brown wrote:
> On Thu, Feb 20, 2020 at 07:24:20AM -0600, Gustavo A. R. Silva wrote:
>> The current codebase makes use of the zero-length array language
>> extension to the C90 standard, but the preferred mechanism to declare
>> variable-length types such as these ones is a flexible array member[1][2],
>> introduced in C99:
>
> Acked-by: Mark Brown <broonie@kernel.org>
>
Thanks, Mark.
--
Gustavo
^ permalink raw reply
* Re: [PATCH v2 1/4] drm/simple-kms: Add drm_simple_encoder_{init, create}()
From: Sam Ravnborg @ 2020-02-20 18:50 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: airlied, dri-devel, maarten.lankhorst, mripard, virtualization,
noralf, daniel, alexander.deucher, spice-devel, emil.velikov
In-Reply-To: <20200218084815.2137-2-tzimmermann@suse.de>
Hi Thomas.
On Tue, Feb 18, 2020 at 09:48:12AM +0100, Thomas Zimmermann wrote:
> This patch makes the internal encoder implementation of the simple
> KMS helpers available to drivers.
>
> These simple-encoder helpers initialize an encoder with an empty
> implementation. This covers the requirements of most of the existing
> DRM drivers. A call to drm_simple_encoder_create() allocates and
> initializes an encoder instance, a call to drm_simple_encoder_init()
> initializes a pre-allocated instance.
>
> v2:
> * move simple encoder to KMS helpers
> * remove name argument; simplifies implementation
> * don't allocate with devm_ interfaces; unsafe with DRM
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
> drivers/gpu/drm/drm_simple_kms_helper.c | 83 ++++++++++++++++++++++++-
> include/drm/drm_simple_kms_helper.h | 7 +++
> 2 files changed, 87 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> index 15fb516ae2d8..745c2f34c42b 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -26,12 +26,90 @@
> * entity. Some flexibility for code reuse is provided through a separately
> * allocated &drm_connector object and supporting optional &drm_bridge
> * encoder drivers.
> + *
> + * Many drivers use an encoder with an empty implementation. Such encoders
> + * fulfill the minimum requirements of the display pipeline, but don't add
> + * additional functionality. The simple-encoder functions
> + * drm_simple_encoder_init() and drm_simple_encoder_create() provide an
> + * appropriate implementation.
> */
>
> -static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = {
> +static const struct drm_encoder_funcs drm_simple_encoder_funcs_cleanup = {
> .destroy = drm_encoder_cleanup,
> };
>
> +/**
> + * drm_simple_encoder_init - Initialize a preallocated encoder
> + * @dev: drm device
> + * @funcs: callbacks for this encoder
> + * @encoder_type: user visible type of the encoder
> + *
> + * Initialises a preallocated encoder that has no further functionality. The
> + * encoder will be released automatically.
I got confused here. The comment says the encoder is automatically
released. But in this case we have a preallocated encoder (maybe
embedded in ast_private or something.
So the encoder is - as I understnad it - not released. But it is cleaned
up as it is also documented in the next sentences.
Sorry if I am dense, just returned from some travelling...
Sam
Settings for possible CRTC and
> + * clones are left to their initial values. The encoder will be cleaned up
> + * automatically as part of the mode-setting cleanup.
> + *
> + * Also see drm_simple_encoder_create().
> + *
> + * Returns:
> + * Zero on success, error code on failure.
> + */
> +int drm_simple_encoder_init(struct drm_device *dev,
> + struct drm_encoder *encoder,
> + int encoder_type)
> +{
> + return drm_encoder_init(dev, encoder,
> + &drm_simple_encoder_funcs_cleanup,
> + encoder_type, NULL);
> +}
> +EXPORT_SYMBOL(drm_simple_encoder_init);
> +
> +static void drm_encoder_destroy(struct drm_encoder *encoder)
> +{
> + drm_encoder_cleanup(encoder);
> + kfree(encoder);
> +}
> +
> +static const struct drm_encoder_funcs drm_simple_encoder_funcs_destroy = {
> + .destroy = drm_encoder_destroy,
> +};
> +
> +/**
> + * drm_simple_encoder_create - Allocate and initialize an encoder
> + * @dev: drm device
> + * @encoder_type: user visible type of the encoder
> + *
> + * Allocates and initialises an encoder that has no further functionality. The
> + * encoder will be destroyed automatically as part of the mode-setting cleanup.
> + *
> + * See drm_simple_encoder_init() for more information.
> + *
> + * Returns:
> + * The encoder on success, a pointer-encoder error code on failure.
> + */
> +struct drm_encoder *drm_simple_encoder_create(struct drm_device *dev,
> + int encoder_type)
> +{
> + struct drm_encoder *encoder;
> + int ret;
> +
> + encoder = kzalloc(sizeof(*encoder), GFP_KERNEL);
> + if (!encoder)
> + return ERR_PTR(-ENOMEM);
> + ret = drm_encoder_init(dev, encoder,
> + &drm_simple_encoder_funcs_destroy,
> + encoder_type, NULL);
> + if (ret)
> + goto err_kfree;
> +
> + return encoder;
> +
> +err_kfree:
> + kfree(encoder);
> + return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL(drm_simple_encoder_create);
> +
> static enum drm_mode_status
> drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc,
> const struct drm_display_mode *mode)
> @@ -288,8 +366,7 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
> return ret;
>
> encoder->possible_crtcs = drm_crtc_mask(crtc);
> - ret = drm_encoder_init(dev, encoder, &drm_simple_kms_encoder_funcs,
> - DRM_MODE_ENCODER_NONE, NULL);
> + ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_NONE);
> if (ret || !connector)
> return ret;
>
> diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
> index e253ba7bea9d..54d5066d90c7 100644
> --- a/include/drm/drm_simple_kms_helper.h
> +++ b/include/drm/drm_simple_kms_helper.h
> @@ -181,4 +181,11 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
> const uint64_t *format_modifiers,
> struct drm_connector *connector);
>
> +int drm_simple_encoder_init(struct drm_device *dev,
> + struct drm_encoder *encoder,
> + int encoder_type);
> +
> +struct drm_encoder *drm_simple_encoder_create(struct drm_device *dev,
> + int encoder_type);
> +
> #endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
> --
> 2.25.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [RFC PATCH] security,anon_inodes,kvm: enable security support for anon inodes
From: Daniel Colascione @ 2020-02-20 18:50 UTC (permalink / raw)
To: Casey Schaufler
Cc: Paul Moore, LSM List, Linux FS Devel, Al Viro, SElinux list, kvm,
Nick Kralevich, Stephen Smalley
In-Reply-To: <eb2dbe22-91af-17c6-3dfb-d9ec619a4d7a@schaufler-ca.com>
On Thu, Feb 20, 2020 at 10:11 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 2/17/2020 4:14 PM, Paul Moore wrote:
> > On Thu, Feb 13, 2020 at 2:41 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> >> Add support for labeling and controlling access to files attached to anon
> >> inodes. Introduce extended interfaces for creating such files to permit
> >> passing a related file as an input to decide how to label the anon
> >> inode. Define a security hook for initializing the anon inode security
> >> attributes. Security attributes are either inherited from a related file
> >> or determined based on some combination of the creating task and policy
> >> (in the case of SELinux, using type_transition rules). As an
> >> example user of the inheritance support, convert kvm to use the new
> >> interface for passing the related file so that the anon inode can inherit
> >> the security attributes of /dev/kvm and provide consistent access control
> >> for subsequent ioctl operations. Other users of anon inodes, including
> >> userfaultfd, will default to the transition-based mechanism instead.
> >>
> >> Compared to the series in
> >> https://lore.kernel.org/selinux/20200211225547.235083-1-dancol@google.com/,
> >> this approach differs in that it does not require creation of a separate
> >> anonymous inode for each file (instead storing the per-instance security
> >> information in the file security blob), it applies labeling and control
> >> to all users of anonymous inodes rather than requiring opt-in via a new
> >> flag, it supports labeling based on a related inode if provided,
> >> it relies on type transitions to compute the label of the anon inode
> >> when there is no related inode, and it does not require introducing a new
> >> security class for each user of anonymous inodes.
> >>
> >> On the other hand, the approach in this patch does expose the name passed
> >> by the creator of the anon inode to the policy (an indirect mapping could
> >> be provided within SELinux if these names aren't considered to be stable),
> >> requires the definition of type_transition rules to distinguish userfaultfd
> >> inodes from proc inodes based on type since they share the same class,
> >> doesn't support denying the creation of anonymous inodes (making the hook
> >> added by this patch return something other than void is problematic due to
> >> it being called after the file is already allocated and error handling in
> >> the callers can't presently account for this scenario and end up calling
> >> release methods multiple times), and may be more expensive
> >> (security_transition_sid overhead on each anon inode allocation).
> >>
> >> We are primarily posting this RFC patch now so that the two different
> >> approaches can be concretely compared. We anticipate a hybrid of the
> >> two approaches being the likely outcome in the end. In particular
> >> if support for allocating a separate inode for each of these files
> >> is acceptable, then we would favor storing the security information
> >> in the inode security blob and using it instead of the file security
> >> blob.
> > Bringing this back up in hopes of attracting some attention from the
> > fs-devel crowd and Al. As Stephen already mentioned, from a SELinux
> > perspective we would prefer to attach the security blob to the inode
> > as opposed to the file struct; does anyone have any objections to
> > that?
>
> Sorry for the delay - been sick the past few days.
>
> I agree that the inode is a better place than the file for information
> about the inode. This is especially true for Smack, which uses
> multiple extended attributes in some cases. I don't believe that any
> except the access label will be relevant to anonymous inodes, but
> I can imagine security modules with policies that would.
>
> I am always an advocate of full xattr support. It goes a long
> way in reducing the number and complexity of special case interfaces.
It sounds like we have broad consensus on using the inode to hold
security information, implying that anon_inodes should create new
inodes. Do any of the VFS people want to object?
^ permalink raw reply
* Re: [PATCH v2 1/4] drm/simple-kms: Add drm_simple_encoder_{init, create}()
From: Sam Ravnborg @ 2020-02-20 18:50 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: airlied, dri-devel, virtualization, kraxel, alexander.deucher,
spice-devel, emil.velikov
In-Reply-To: <20200218084815.2137-2-tzimmermann@suse.de>
Hi Thomas.
On Tue, Feb 18, 2020 at 09:48:12AM +0100, Thomas Zimmermann wrote:
> This patch makes the internal encoder implementation of the simple
> KMS helpers available to drivers.
>
> These simple-encoder helpers initialize an encoder with an empty
> implementation. This covers the requirements of most of the existing
> DRM drivers. A call to drm_simple_encoder_create() allocates and
> initializes an encoder instance, a call to drm_simple_encoder_init()
> initializes a pre-allocated instance.
>
> v2:
> * move simple encoder to KMS helpers
> * remove name argument; simplifies implementation
> * don't allocate with devm_ interfaces; unsafe with DRM
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
> drivers/gpu/drm/drm_simple_kms_helper.c | 83 ++++++++++++++++++++++++-
> include/drm/drm_simple_kms_helper.h | 7 +++
> 2 files changed, 87 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> index 15fb516ae2d8..745c2f34c42b 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -26,12 +26,90 @@
> * entity. Some flexibility for code reuse is provided through a separately
> * allocated &drm_connector object and supporting optional &drm_bridge
> * encoder drivers.
> + *
> + * Many drivers use an encoder with an empty implementation. Such encoders
> + * fulfill the minimum requirements of the display pipeline, but don't add
> + * additional functionality. The simple-encoder functions
> + * drm_simple_encoder_init() and drm_simple_encoder_create() provide an
> + * appropriate implementation.
> */
>
> -static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = {
> +static const struct drm_encoder_funcs drm_simple_encoder_funcs_cleanup = {
> .destroy = drm_encoder_cleanup,
> };
>
> +/**
> + * drm_simple_encoder_init - Initialize a preallocated encoder
> + * @dev: drm device
> + * @funcs: callbacks for this encoder
> + * @encoder_type: user visible type of the encoder
> + *
> + * Initialises a preallocated encoder that has no further functionality. The
> + * encoder will be released automatically.
I got confused here. The comment says the encoder is automatically
released. But in this case we have a preallocated encoder (maybe
embedded in ast_private or something.
So the encoder is - as I understnad it - not released. But it is cleaned
up as it is also documented in the next sentences.
Sorry if I am dense, just returned from some travelling...
Sam
Settings for possible CRTC and
> + * clones are left to their initial values. The encoder will be cleaned up
> + * automatically as part of the mode-setting cleanup.
> + *
> + * Also see drm_simple_encoder_create().
> + *
> + * Returns:
> + * Zero on success, error code on failure.
> + */
> +int drm_simple_encoder_init(struct drm_device *dev,
> + struct drm_encoder *encoder,
> + int encoder_type)
> +{
> + return drm_encoder_init(dev, encoder,
> + &drm_simple_encoder_funcs_cleanup,
> + encoder_type, NULL);
> +}
> +EXPORT_SYMBOL(drm_simple_encoder_init);
> +
> +static void drm_encoder_destroy(struct drm_encoder *encoder)
> +{
> + drm_encoder_cleanup(encoder);
> + kfree(encoder);
> +}
> +
> +static const struct drm_encoder_funcs drm_simple_encoder_funcs_destroy = {
> + .destroy = drm_encoder_destroy,
> +};
> +
> +/**
> + * drm_simple_encoder_create - Allocate and initialize an encoder
> + * @dev: drm device
> + * @encoder_type: user visible type of the encoder
> + *
> + * Allocates and initialises an encoder that has no further functionality. The
> + * encoder will be destroyed automatically as part of the mode-setting cleanup.
> + *
> + * See drm_simple_encoder_init() for more information.
> + *
> + * Returns:
> + * The encoder on success, a pointer-encoder error code on failure.
> + */
> +struct drm_encoder *drm_simple_encoder_create(struct drm_device *dev,
> + int encoder_type)
> +{
> + struct drm_encoder *encoder;
> + int ret;
> +
> + encoder = kzalloc(sizeof(*encoder), GFP_KERNEL);
> + if (!encoder)
> + return ERR_PTR(-ENOMEM);
> + ret = drm_encoder_init(dev, encoder,
> + &drm_simple_encoder_funcs_destroy,
> + encoder_type, NULL);
> + if (ret)
> + goto err_kfree;
> +
> + return encoder;
> +
> +err_kfree:
> + kfree(encoder);
> + return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL(drm_simple_encoder_create);
> +
> static enum drm_mode_status
> drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc,
> const struct drm_display_mode *mode)
> @@ -288,8 +366,7 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
> return ret;
>
> encoder->possible_crtcs = drm_crtc_mask(crtc);
> - ret = drm_encoder_init(dev, encoder, &drm_simple_kms_encoder_funcs,
> - DRM_MODE_ENCODER_NONE, NULL);
> + ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_NONE);
> if (ret || !connector)
> return ret;
>
> diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
> index e253ba7bea9d..54d5066d90c7 100644
> --- a/include/drm/drm_simple_kms_helper.h
> +++ b/include/drm/drm_simple_kms_helper.h
> @@ -181,4 +181,11 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
> const uint64_t *format_modifiers,
> struct drm_connector *connector);
>
> +int drm_simple_encoder_init(struct drm_device *dev,
> + struct drm_encoder *encoder,
> + int encoder_type);
> +
> +struct drm_encoder *drm_simple_encoder_create(struct drm_device *dev,
> + int encoder_type);
> +
> #endif /* __LINUX_DRM_SIMPLE_KMS_HELPER_H */
> --
> 2.25.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* [Regression] Docs build broken by commit 51e46c7a4007
From: Rafael J. Wysocki @ 2020-02-20 18:50 UTC (permalink / raw)
To: Kees Cook
Cc: Jonathan Corbet, Linux Kernel Mailing List,
open list:DOCUMENTATION
Hi Kees,
On two of my systems the docs build has been broken by commit
51e46c7a4007 ("docs, parallelism: Rearrange how jobserver reservations
are made").
The symptom is that the build system complains about the "output"
directory not being there and returns with an error.
Reverting the problematic commit makes the problem go away.
Please advise. :-)
Cheers,
Rafael
^ permalink raw reply
* Re: [PATCH v2 1/2] gpio: of: Extract of_gpiochip_add_hog()
From: Frank Rowand @ 2020-02-20 18:50 UTC (permalink / raw)
To: Geert Uytterhoeven, Linus Walleij, Bartosz Golaszewski,
Pantelis Antoniou, Rob Herring, Mark Rutland
Cc: Peter Ujfalusi, Chris Brandt, linux-gpio, devicetree,
linux-renesas-soc, linux-kernel
In-Reply-To: <20200220130149.26283-2-geert+renesas@glider.be>
On 2/20/20 7:01 AM, Geert Uytterhoeven wrote:
> Extract the code to add all GPIO hogs of a gpio-hog node into its own
> function, so it can be reused.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v2:
> - No changes.
> ---
> drivers/gpio/gpiolib-of.c | 49 ++++++++++++++++++++++++++-------------
> 1 file changed, 33 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index c6d30f73df078e0b..2b47f93886075294 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -604,6 +604,35 @@ static struct gpio_desc *of_parse_own_gpio(struct device_node *np,
> return desc;
> }
>
> +/**
> + * of_gpiochip_add_hog - Add all hogs in a hog device node
> + * @chip: gpio chip to act on
> + * @hog: device node describing the hogs
> + *
> + * Returns error if it fails otherwise 0 on success.
> + */
> +static int of_gpiochip_add_hog(struct gpio_chip *chip, struct device_node *hog)
> +{
> + enum gpiod_flags dflags;
> + struct gpio_desc *desc;
> + unsigned long lflags;
> + const char *name;
> + unsigned int i;
> + int ret;
> +
> + for (i = 0;; i++) {
> + desc = of_parse_own_gpio(hog, chip, i, &name, &lflags, &dflags);
> + if (IS_ERR(desc))
> + break;
> +
> + ret = gpiod_hog(desc, name, lflags, dflags);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> /**
> * of_gpiochip_scan_gpios - Scan gpio-controller for gpio definitions
> * @chip: gpio chip to act on
> @@ -614,29 +643,17 @@ static struct gpio_desc *of_parse_own_gpio(struct device_node *np,
> */
> static int of_gpiochip_scan_gpios(struct gpio_chip *chip)
> {
> - struct gpio_desc *desc = NULL;
> struct device_node *np;
> - const char *name;
> - unsigned long lflags;
> - enum gpiod_flags dflags;
> - unsigned int i;
> int ret;
>
> for_each_available_child_of_node(chip->of_node, np) {
> if (!of_property_read_bool(np, "gpio-hog"))
> continue;
>
> - for (i = 0;; i++) {
> - desc = of_parse_own_gpio(np, chip, i, &name, &lflags,
> - &dflags);
> - if (IS_ERR(desc))
> - break;
> -
> - ret = gpiod_hog(desc, name, lflags, dflags);
> - if (ret < 0) {
> - of_node_put(np);
> - return ret;
> - }
> + ret = of_gpiochip_add_hog(chip, np);
> + if (ret < 0) {
> + of_node_put(np);
> + return ret;
> }
> }
>
>
Reviewed-by: Frank Rowand <frank.rowand@sony.com>
^ permalink raw reply
* Re: [PATCH v2 2/2] gpio: of: Add DT overlay support for GPIO hogs
From: Frank Rowand @ 2020-02-20 18:51 UTC (permalink / raw)
To: Geert Uytterhoeven, Linus Walleij, Bartosz Golaszewski,
Pantelis Antoniou, Rob Herring, Mark Rutland
Cc: Peter Ujfalusi, Chris Brandt, linux-gpio, devicetree,
linux-renesas-soc, linux-kernel, Frank Rowand
In-Reply-To: <20200220130149.26283-3-geert+renesas@glider.be>
On 2/20/20 7:01 AM, Geert Uytterhoeven wrote:
> As GPIO hogs are configured at GPIO controller initialization time,
> adding/removing GPIO hogs in DT overlays does not work.
>
> Add support for GPIO hogs described in DT overlays by registering an OF
> reconfiguration notifier, to handle the addition and removal of GPIO hog
> subnodes to/from a GPIO controller device node.
>
> Note that when a GPIO hog device node is being removed, its "gpios"
> properties is no longer available, so we have to keep track of which
> node a hog belongs to, which is done by adding a pointer to the hog's
> device node to struct gpio_desc.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v2:
> - Drop RFC state,
> - Document that modifying existing gpio-hog nodes is not supported.
> ---
> drivers/gpio/gpiolib-of.c | 90 +++++++++++++++++++++++++++++++++++++++
> drivers/gpio/gpiolib-of.h | 2 +
> drivers/gpio/gpiolib.c | 14 ++++--
> drivers/gpio/gpiolib.h | 3 ++
> 4 files changed, 106 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 2b47f93886075294..ccc449df3792ae97 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -628,6 +628,10 @@ static int of_gpiochip_add_hog(struct gpio_chip *chip, struct device_node *hog)
> ret = gpiod_hog(desc, name, lflags, dflags);
> if (ret < 0)
> return ret;
> +
> +#ifdef CONFIG_OF_DYNAMIC
> + desc->hog = hog;
> +#endif
> }
>
> return 0;
> @@ -655,11 +659,97 @@ static int of_gpiochip_scan_gpios(struct gpio_chip *chip)
> of_node_put(np);
> return ret;
> }
> +
> + of_node_set_flag(np, OF_POPULATED);
> }
>
> return 0;
> }
>
> +#ifdef CONFIG_OF_DYNAMIC
> +/**
> + * of_gpiochip_remove_hog - Remove all hogs in a hog device node
> + * @chip: gpio chip to act on
> + * @hog: device node describing the hogs
> + */
> +static void of_gpiochip_remove_hog(struct gpio_chip *chip,
> + struct device_node *hog)
> +{
> + struct gpio_desc *descs = chip->gpiodev->descs;
> + unsigned int i;
> +
> + for (i = 0; i < chip->ngpio; i++) {
> + if (test_bit(FLAG_IS_HOGGED, &descs[i].flags) &&
> + descs[i].hog == hog)
> + gpiochip_free_own_desc(&descs[i]);
> + }
> +}
> +
> +static int of_gpiochip_match_node(struct gpio_chip *chip, void *data)
> +{
> + return chip->gpiodev->dev.of_node == data;
> +}
> +
> +static struct gpio_chip *of_find_gpiochip_by_node(struct device_node *np)
> +{
> + return gpiochip_find(np, of_gpiochip_match_node);
> +}
> +
> +static int of_gpio_notify(struct notifier_block *nb, unsigned long action,
> + void *arg)
> +{
> + struct of_reconfig_data *rd = arg;
> + struct gpio_chip *chip;
> + int ret;
> +
> + /*
> + * This only supports adding and removing complete gpio-hog nodes.
> + * Modifying an existing gpio-hog node is not supported (except for
> + * changing its "status" property, which is treated the same as
> + * addition/removal).
> + */
> + switch (of_reconfig_get_state_change(action, arg)) {
> + case OF_RECONFIG_CHANGE_ADD:
> + if (!of_property_read_bool(rd->dn, "gpio-hog"))
> + return NOTIFY_OK; /* not for us */
> +
> + if (of_node_test_and_set_flag(rd->dn, OF_POPULATED))
> + return NOTIFY_OK;
> +
> + chip = of_find_gpiochip_by_node(rd->dn->parent);
> + if (chip == NULL)
> + return NOTIFY_OK; /* not for us */
> +
> + ret = of_gpiochip_add_hog(chip, rd->dn);
> + if (ret < 0) {
> + pr_err("%s: failed to add hogs for %pOF\n", __func__,
> + rd->dn);
> + of_node_clear_flag(rd->dn, OF_POPULATED);
> + return notifier_from_errno(ret);
> + }
> + break;
> +
> + case OF_RECONFIG_CHANGE_REMOVE:
> + if (!of_node_check_flag(rd->dn, OF_POPULATED))
> + return NOTIFY_OK; /* already depopulated */
> +
> + chip = of_find_gpiochip_by_node(rd->dn->parent);
> + if (chip == NULL)
> + return NOTIFY_OK; /* not for us */
> +
> + of_gpiochip_remove_hog(chip, rd->dn);
> + of_node_clear_flag(rd->dn, OF_POPULATED);
> + break;
> + }
> +
> + return NOTIFY_OK;
> +}
> +
> +struct notifier_block gpio_of_notifier = {
> + .notifier_call = of_gpio_notify,
> +};
> +#endif /* CONFIG_OF_DYNAMIC */
> +
> /**
> * of_gpio_simple_xlate - translate gpiospec to the GPIO number and flags
> * @gc: pointer to the gpio_chip structure
> diff --git a/drivers/gpio/gpiolib-of.h b/drivers/gpio/gpiolib-of.h
> index 9768831b1fe2f25b..ed26664f153782fc 100644
> --- a/drivers/gpio/gpiolib-of.h
> +++ b/drivers/gpio/gpiolib-of.h
> @@ -35,4 +35,6 @@ static inline bool of_gpio_need_valid_mask(const struct gpio_chip *gc)
> }
> #endif /* CONFIG_OF_GPIO */
>
> +extern struct notifier_block gpio_of_notifier;
> +
> #endif /* GPIOLIB_OF_H */
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 56de871060ea211e..6f312220fe80acaf 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2925,6 +2925,9 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
> clear_bit(FLAG_PULL_DOWN, &desc->flags);
> clear_bit(FLAG_BIAS_DISABLE, &desc->flags);
> clear_bit(FLAG_IS_HOGGED, &desc->flags);
> +#ifdef CONFIG_OF_DYNAMIC
> + desc->hog = NULL;
> +#endif
> ret = true;
> }
>
> @@ -5126,10 +5129,15 @@ static int __init gpiolib_dev_init(void)
> if (ret < 0) {
> pr_err("gpiolib: failed to allocate char dev region\n");
> bus_unregister(&gpio_bus_type);
> - } else {
> - gpiolib_initialized = true;
> - gpiochip_setup_devs();
> + return ret;
> }
> +
> + gpiolib_initialized = true;
> + gpiochip_setup_devs();
> +
> + if (IS_ENABLED(CONFIG_OF_DYNAMIC))
> + WARN_ON(of_reconfig_notifier_register(&gpio_of_notifier));
> +
> return ret;
> }
> core_initcall(gpiolib_dev_init);
> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> index 3e0aab2945d82974..18c75e83fd7679ec 100644
> --- a/drivers/gpio/gpiolib.h
> +++ b/drivers/gpio/gpiolib.h
> @@ -119,6 +119,9 @@ struct gpio_desc {
> const char *label;
> /* Name of the GPIO */
> const char *name;
> +#ifdef CONFIG_OF_DYNAMIC
> + struct device_node *hog;
> +#endif
> };
>
> int gpiod_request(struct gpio_desc *desc, const char *label);
>
Reviewed-by: Frank Rowand <frank.rowand@sony.com>
^ permalink raw reply
* Re: [PATCH v26 10/22] x86/sgx: Linux Enclave Driver
From: Andy Lutomirski @ 2020-02-20 18:51 UTC (permalink / raw)
To: Sean Christopherson
Cc: Jordan Hand, Jarkko Sakkinen, LKML, X86 ML, linux-sgx,
Andrew Morton, Dave Hansen, Neil Horman, npmccallum,
Huang, Haitao, Andy Shevchenko, Thomas Gleixner, Svahn, Kai,
Borislav Petkov, Josh Triplett, Andrew Lutomirski, Huang, Kai,
David Rientjes, Xing, Cedric, puiterwijk, LSM List, Suresh Siddha,
Haitao Huang
In-Reply-To: <20200220181345.GD3972@linux.intel.com>
On Thu, Feb 20, 2020 at 10:13 AM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Tue, Feb 18, 2020 at 07:26:31PM -0800, Jordan Hand wrote:
> > I ran our validation tests for the Open Enclave SDK against this patch
> > set and came across a potential issue.
> >
> > On 2/9/20 1:25 PM, Jarkko Sakkinen wrote:
> > > +/**
> > > + * sgx_encl_may_map() - Check if a requested VMA mapping is allowed
> > > + * @encl: an enclave
> > > + * @start: lower bound of the address range, inclusive
> > > + * @end: upper bound of the address range, exclusive
> > > + * @vm_prot_bits: requested protections of the address range
> > > + *
> > > + * Iterate through the enclave pages contained within [@start, @end) to verify
> > > + * the permissions requested by @vm_prot_bits do not exceed that of any enclave
> > > + * page to be mapped. Page addresses that do not have an associated enclave
> > > + * page are interpreted to zero permissions.
> > > + *
> > > + * Return:
> > > + * 0 on success,
> > > + * -EACCES if VMA permissions exceed enclave page permissions
> > > + */
> > > +int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
> > > + unsigned long end, unsigned long vm_prot_bits)
> > > +{
> > > + unsigned long idx, idx_start, idx_end;
> > > + struct sgx_encl_page *page;
> > > +
> > > + /* PROT_NONE always succeeds. */
> > > + if (!vm_prot_bits)
> > > + return 0;
> > > +
> > > + idx_start = PFN_DOWN(start);
> > > + idx_end = PFN_DOWN(end - 1);
> > > +
> > > + for (idx = idx_start; idx <= idx_end; ++idx) {
> > > + mutex_lock(&encl->lock);
> > > + page = radix_tree_lookup(&encl->page_tree, idx);
> > > + mutex_unlock(&encl->lock);
> > > +
> > > + if (!page || (~page->vm_max_prot_bits & vm_prot_bits))
> > > + return -EACCES;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
> > > + unsigned long offset,
> > > + u64 secinfo_flags)
> > > +{
> > > + struct sgx_encl_page *encl_page;
> > > + unsigned long prot;
> > > +
> > > + encl_page = kzalloc(sizeof(*encl_page), GFP_KERNEL);
> > > + if (!encl_page)
> > > + return ERR_PTR(-ENOMEM);
> > > +
> > > + encl_page->desc = encl->base + offset;
> > > + encl_page->encl = encl;
> > > +
> > > + prot = _calc_vm_trans(secinfo_flags, SGX_SECINFO_R, PROT_READ) |
> > > + _calc_vm_trans(secinfo_flags, SGX_SECINFO_W, PROT_WRITE) |
> > > + _calc_vm_trans(secinfo_flags, SGX_SECINFO_X, PROT_EXEC);
> > > +
> > > + /*
> > > + * TCS pages must always RW set for CPU access while the SECINFO
> > > + * permissions are *always* zero - the CPU ignores the user provided
> > > + * values and silently overwrites them with zero permissions.
> > > + */
> > > + if ((secinfo_flags & SGX_SECINFO_PAGE_TYPE_MASK) == SGX_SECINFO_TCS)
> > > + prot |= PROT_READ | PROT_WRITE;
> > > +
> > > + /* Calculate maximum of the VM flags for the page. */
> > > + encl_page->vm_max_prot_bits = calc_vm_prot_bits(prot, 0);
> >
> > During mprotect (in mm/mprotect.c line 525) the following checks if
> > READ_IMPLIES_EXECUTE and a PROT_READ is being requested. If so and
> > VM_MAYEXEC is set, it also adds PROT_EXEC to the request.
> >
> > if (rier && (vma->vm_flags & VM_MAYEXEC))
> > prot |= PROT_EXEC;
> >
> > But if we look at sgx_encl_page_alloc(), we see vm_max_prot_bits is set
> > without taking VM_MAYEXEC into account:
> >
> > encl_page->vm_max_prot_bits = calc_vm_prot_bits(prot, 0);
> >
> > sgx_encl_may_map() checks that the requested protection can be added with:
> >
> > if (!page || (~page->vm_max_prot_bits & vm_prot_bits))
> > return -EACCESS
> >
> > This means that for any process where READ_IMPLIES_EXECUTE is set and
> > page where (vma->vm_flags & VM_MAYEXEC) == true, mmap/mprotect calls to
> > that request PROT_READ on a page that was not added with PROT_EXEC will
> > fail.
>
> I could've sworn this was discussed on the SGX list at one point, but
> apparently we only discussed it internally. Anyways...
>
> More than likely, the READ_IMPLIES_EXECUTE (RIE) crud rears its head
> because part of the enclave loader is written in assembly. Unless
> explicitly told otherwise, the linker assumes that any program with
> assembly code may need an executable stack, which leads to the RIE
> personality being set for the process. Here's a fantastic write up for
> more details: https://www.airs.com/blog/archives/518
>
> There are essentially two paths we can take:
>
> 1) Exempt EPC pages from RIE during mmap()/mprotect(), i.e. don't add
> PROT_EXEC for enclaves.
Seems reasonable.
Honestly, it probably makes sense to try to exempt almost everything
from RIE. I'd be a bit surprised if RIE is actually useful for
anything other than plain anonymous pages and private file mappings.
^ permalink raw reply
* Re: [PATCH 4/8] libxfs: enable tools to check that metadata updates have been committed
From: Brian Foster @ 2020-02-20 18:50 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: sandeen, linux-xfs
In-Reply-To: <20200220182642.GZ9506@magnolia>
On Thu, Feb 20, 2020 at 10:26:42AM -0800, Darrick J. Wong wrote:
> On Thu, Feb 20, 2020 at 12:58:50PM -0500, Brian Foster wrote:
> > On Thu, Feb 20, 2020 at 08:46:38AM -0800, Darrick J. Wong wrote:
> > > On Thu, Feb 20, 2020 at 09:06:12AM -0500, Brian Foster wrote:
> > > > On Wed, Feb 19, 2020 at 05:42:06PM -0800, Darrick J. Wong wrote:
> > > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > >
> > > > > Add a new function that will ensure that everything we changed has
> > > > > landed on stable media, and report the results. Subsequent commits will
> > > > > teach the individual programs to report when things go wrong.
> > > > >
> > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > ---
> > > > > include/xfs_mount.h | 3 +++
> > > > > libxfs/init.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> > > > > libxfs/libxfs_io.h | 2 ++
> > > > > libxfs/rdwr.c | 27 +++++++++++++++++++++++++--
> > > > > 4 files changed, 73 insertions(+), 2 deletions(-)
> > > > >
> > > > >
> > > > > diff --git a/include/xfs_mount.h b/include/xfs_mount.h
> > > > > index 29b3cc1b..c80aaf69 100644
> > > > > --- a/include/xfs_mount.h
> > > > > +++ b/include/xfs_mount.h
> > > > > @@ -187,4 +187,7 @@ extern xfs_mount_t *libxfs_mount (xfs_mount_t *, xfs_sb_t *,
> > > > > extern void libxfs_umount (xfs_mount_t *);
> > > > > extern void libxfs_rtmount_destroy (xfs_mount_t *);
> > > > >
> > > > > +void libxfs_flush_devices(struct xfs_mount *mp, int *datadev, int *logdev,
> > > > > + int *rtdev);
> > > > > +
> > > > > #endif /* __XFS_MOUNT_H__ */
> > > > > diff --git a/libxfs/init.c b/libxfs/init.c
> > > > > index a0d4b7f4..d1d3f4df 100644
> > > > > --- a/libxfs/init.c
> > > > > +++ b/libxfs/init.c
> > > > > @@ -569,6 +569,8 @@ libxfs_buftarg_alloc(
> > > > > }
> > > > > btp->bt_mount = mp;
> > > > > btp->dev = dev;
> > > > > + btp->lost_writes = false;
> > > > > +
> > > > > return btp;
> > > > > }
> > > > >
> > > > > @@ -791,6 +793,47 @@ libxfs_rtmount_destroy(xfs_mount_t *mp)
> > > > > mp->m_rsumip = mp->m_rbmip = NULL;
> > > > > }
> > > > >
> > > > > +static inline int
> > > > > +libxfs_flush_buftarg(
> > > > > + struct xfs_buftarg *btp)
> > > > > +{
> > > > > + if (btp->lost_writes)
> > > > > + return -ENOTRECOVERABLE;
> > > >
> > > > I'm curious why we'd want to skip the flush just because some writes
> > > > happened to fail..? I suppose the fs might be borked, but it seems a
> > > > little strange to at least not try the flush, particularly since we
> > > > might still flush any of the other two possible devices.
> > >
> > > My thinking here was that if the write verifiers (or the pwrite() calls
> > > themselves) failed then there's no point in telling the disk to flush
> > > its write cache since we already know it's not in sync with the buffer
> > > cache.
> > >
> >
> > I suppose, but it seems there is some value in flushing what we did
> > write.. That's effectively historical behavior (since we ignored
> > errors), right?
>
> It's the historical behavior, yes. I don't think it makes much sense,
> but OTOH I'm not opposed to restoring that.
>
The way I think about it is if repair or something happens to rewrite a
bunch of metadata structures, etc. and then a particular I/O happens to
fail, we'll still end up with a corrupted fs in the end, but I don't see
that as a reason not to care about the integrity of the data that might
have been successfully written. We're most likely borked either way,
this just seems a bit less risky (and also less of a wart/landmine given
that the close codepath is still going to do the flush anyways).
> > > > > +
> > > > > + return libxfs_blkdev_issue_flush(btp);
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * Purge the buffer cache to write all dirty buffers to disk and free all
> > > > > + * incore buffers. Buffers that cannot be written will cause the lost_writes
> > > > > + * flag to be set in the buftarg. If there were no lost writes, flush the
> > > > > + * device to make sure the writes made it to stable storage.
> > > > > + *
> > > > > + * For each device, the return code will be set to -ENOTRECOVERABLE if we
> > > > > + * couldn't write something to disk; or the results of the block device flush
> > > > > + * operation.
> > > >
> > > > Why not -EIO?
> > >
> > > Originally I thought it might be useful to be able to distinguish
> > > between "dirty buffers never even made it out of the buffer cache" vs.
> > > "dirty buffers were sent to the disk but the disk sent back media
> > > errors", though in the end the userspace tools don't make any
> > > distinction.
> > >
> > > That said, looking at this again, maybe I should track write verifier
> > > failure separately so that we can return EFSCORRUPTED for that?
> > >
> >
> > It's not clear to me that anything application level would care much
> > about verifier failure vs. I/O failure, but I've no objection to doing
> > something like that either.
>
> Yeah. The single usecase I can think of is where repair trips over a
> write verifier and we should make it really obvious to the sysadmin that
> repair is buggy and needs either (a) an upgrade or (b) a complaint filed
> on linux-xfs.
>
We do have the write verifier failure messages, but yeah, I can see that
being a more accurate distinction between -EIO and -EFSCORRUPTED.
Brian
> --D
>
> > Brian
> >
> > > --D
> > >
> > > >
> > > > Brian
> > > >
> > > > > + */
> > > > > +void
> > > > > +libxfs_flush_devices(
> > > > > + struct xfs_mount *mp,
> > > > > + int *datadev,
> > > > > + int *logdev,
> > > > > + int *rtdev)
> > > > > +{
> > > > > + *datadev = *logdev = *rtdev = 0;
> > > > > +
> > > > > + libxfs_bcache_purge();
> > > > > +
> > > > > + if (mp->m_ddev_targp)
> > > > > + *datadev = libxfs_flush_buftarg(mp->m_ddev_targp);
> > > > > +
> > > > > + if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp)
> > > > > + *logdev = libxfs_flush_buftarg(mp->m_logdev_targp);
> > > > > +
> > > > > + if (mp->m_rtdev_targp)
> > > > > + *rtdev = libxfs_flush_buftarg(mp->m_rtdev_targp);
> > > > > +}
> > > > > +
> > > > > /*
> > > > > * Release any resource obtained during a mount.
> > > > > */
> > > > > diff --git a/libxfs/libxfs_io.h b/libxfs/libxfs_io.h
> > > > > index 579df52b..fc0fd060 100644
> > > > > --- a/libxfs/libxfs_io.h
> > > > > +++ b/libxfs/libxfs_io.h
> > > > > @@ -23,10 +23,12 @@ struct xfs_perag;
> > > > > struct xfs_buftarg {
> > > > > struct xfs_mount *bt_mount;
> > > > > dev_t dev;
> > > > > + bool lost_writes;
> > > > > };
> > > > >
> > > > > extern void libxfs_buftarg_init(struct xfs_mount *mp, dev_t ddev,
> > > > > dev_t logdev, dev_t rtdev);
> > > > > +int libxfs_blkdev_issue_flush(struct xfs_buftarg *btp);
> > > > >
> > > > > #define LIBXFS_BBTOOFF64(bbs) (((xfs_off_t)(bbs)) << BBSHIFT)
> > > > >
> > > > > diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
> > > > > index 8b47d438..92e497f9 100644
> > > > > --- a/libxfs/rdwr.c
> > > > > +++ b/libxfs/rdwr.c
> > > > > @@ -17,6 +17,7 @@
> > > > > #include "xfs_inode_fork.h"
> > > > > #include "xfs_inode.h"
> > > > > #include "xfs_trans.h"
> > > > > +#include "libfrog/platform.h"
> > > > >
> > > > > #include "libxfs.h" /* for LIBXFS_EXIT_ON_FAILURE */
> > > > >
> > > > > @@ -1227,9 +1228,11 @@ libxfs_brelse(
> > > > >
> > > > > if (!bp)
> > > > > return;
> > > > > - if (bp->b_flags & LIBXFS_B_DIRTY)
> > > > > + if (bp->b_flags & LIBXFS_B_DIRTY) {
> > > > > fprintf(stderr,
> > > > > "releasing dirty buffer to free list!\n");
> > > > > + bp->b_target->lost_writes = true;
> > > > > + }
> > > > >
> > > > > pthread_mutex_lock(&xfs_buf_freelist.cm_mutex);
> > > > > list_add(&bp->b_node.cn_mru, &xfs_buf_freelist.cm_list);
> > > > > @@ -1248,9 +1251,11 @@ libxfs_bulkrelse(
> > > > > return 0 ;
> > > > >
> > > > > list_for_each_entry(bp, list, b_node.cn_mru) {
> > > > > - if (bp->b_flags & LIBXFS_B_DIRTY)
> > > > > + if (bp->b_flags & LIBXFS_B_DIRTY) {
> > > > > fprintf(stderr,
> > > > > "releasing dirty buffer (bulk) to free list!\n");
> > > > > + bp->b_target->lost_writes = true;
> > > > > + }
> > > > > count++;
> > > > > }
> > > > >
> > > > > @@ -1479,6 +1484,24 @@ libxfs_irele(
> > > > > kmem_cache_free(xfs_inode_zone, ip);
> > > > > }
> > > > >
> > > > > +/*
> > > > > + * Flush everything dirty in the kernel and disk write caches to stable media.
> > > > > + * Returns 0 for success or a negative error code.
> > > > > + */
> > > > > +int
> > > > > +libxfs_blkdev_issue_flush(
> > > > > + struct xfs_buftarg *btp)
> > > > > +{
> > > > > + int fd, ret;
> > > > > +
> > > > > + if (btp->dev == 0)
> > > > > + return 0;
> > > > > +
> > > > > + fd = libxfs_device_to_fd(btp->dev);
> > > > > + ret = platform_flush_device(fd, btp->dev);
> > > > > + return ret ? -errno : 0;
> > > > > +}
> > > > > +
> > > > > /*
> > > > > * Write out a buffer list synchronously.
> > > > > *
> > > > >
> > > >
> > >
> >
>
^ permalink raw reply
* [PATCH] classes/meson.bbclass: Explicitly mark functions as missing in MinGW
From: Joshua Watt @ 2020-02-20 18:51 UTC (permalink / raw)
To: openembedded-core
meson (with some help from GCC) misdetects certain builtin functions as
being present on MinGW, when in fact they are not. Fix it by explicitly
marking the functions as missing. This requires rearranging the
meson.cross file to put the [properties] section at the end so the
append works properly (meson can't handle duplicate sections).
Signed-off-by: Joshua Watt <JPEWhacker@gmail.com>
---
meta/classes/meson.bbclass | 25 +++++++++++++++++--------
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/meta/classes/meson.bbclass b/meta/classes/meson.bbclass
index 06034e8b47..5c74658d01 100644
--- a/meta/classes/meson.bbclass
+++ b/meta/classes/meson.bbclass
@@ -96,14 +96,6 @@ readelf = ${@meson_array('READELF', d)}
pkgconfig = 'pkg-config'
llvm-config = 'llvm-config${LLVMVERSION}'
-[properties]
-needs_exe_wrapper = true
-c_args = ${@meson_array('CFLAGS', d)}
-c_link_args = ${@meson_array('LDFLAGS', d)}
-cpp_args = ${@meson_array('CXXFLAGS', d)}
-cpp_link_args = ${@meson_array('LDFLAGS', d)}
-gtkdoc_exe_wrapper = '${B}/gtkdoc-qemuwrapper'
-
[host_machine]
system = '${@meson_operating_system('HOST_OS', d)}'
cpu_family = '${@meson_cpu_family('HOST_ARCH', d)}'
@@ -115,6 +107,23 @@ system = '${@meson_operating_system('TARGET_OS', d)}'
cpu_family = '${@meson_cpu_family('TARGET_ARCH', d)}'
cpu = '${TARGET_ARCH}'
endian = '${@meson_endian('TARGET', d)}'
+
+[properties]
+needs_exe_wrapper = true
+c_args = ${@meson_array('CFLAGS', d)}
+c_link_args = ${@meson_array('LDFLAGS', d)}
+cpp_args = ${@meson_array('CXXFLAGS', d)}
+cpp_link_args = ${@meson_array('LDFLAGS', d)}
+gtkdoc_exe_wrapper = '${B}/gtkdoc-qemuwrapper'
+EOF
+}
+
+do_write_config_append_mingw32() {
+ # meson mis-detects the presence of several builtin functions in MinGW
+ # (see https://github.com/mesonbuild/meson/issues/3672).
+ # Explicitly mark these functions as missing in the cross file
+ cat >>${WORKDIR}/meson.cross <<EOF
+has_function_strndup = false
EOF
}
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] ASoC: tas2562: Add support for digital volume control
From: Dan Murphy @ 2020-02-20 18:46 UTC (permalink / raw)
To: Mark Brown; +Cc: lgirdwood, perex, tiwai, alsa-devel, linux-kernel
In-Reply-To: <20200220184507.GF3926@sirena.org.uk>
Mark
On 2/20/20 12:45 PM, Mark Brown wrote:
> On Thu, Feb 20, 2020 at 11:27:21AM -0600, Dan Murphy wrote:
>
>> + /* Set the Digital volume to -110dB */
>> + ret = snd_soc_component_write(component, TAS2562_DVC_CFG4, 0x00);
>> + if (ret)
>> + return ret;
>> + ret = snd_soc_component_write(component, TAS2562_DVC_CFG3, 0x00);
>> + if (ret)
>> + return ret;
>> + ret = snd_soc_component_write(component, TAS2562_DVC_CFG2, 0x0d);
>> + if (ret)
>> + return ret;
>> + ret = snd_soc_component_write(component, TAS2562_DVC_CFG1, 0x43);
>> + if (ret)
>> + return ret;
> Is there a reason not to use the chip default here? Otherwise this
> looks good.
Chip default is set to 0dB full blast+ 0x40400000. This sets the volume
to -110dB.
I have the values backwards. CFG4 should b 0x43 and CFG3 should be 0x0d
and CFG1&2 should be 0.
I will resend v2 with this fixed.
Dan
^ permalink raw reply
* Re: [PATCH] ASoC: tas2562: Add support for digital volume control
From: Dan Murphy @ 2020-02-20 18:46 UTC (permalink / raw)
To: Mark Brown; +Cc: linux-kernel, alsa-devel, tiwai, lgirdwood
In-Reply-To: <20200220184507.GF3926@sirena.org.uk>
Mark
On 2/20/20 12:45 PM, Mark Brown wrote:
> On Thu, Feb 20, 2020 at 11:27:21AM -0600, Dan Murphy wrote:
>
>> + /* Set the Digital volume to -110dB */
>> + ret = snd_soc_component_write(component, TAS2562_DVC_CFG4, 0x00);
>> + if (ret)
>> + return ret;
>> + ret = snd_soc_component_write(component, TAS2562_DVC_CFG3, 0x00);
>> + if (ret)
>> + return ret;
>> + ret = snd_soc_component_write(component, TAS2562_DVC_CFG2, 0x0d);
>> + if (ret)
>> + return ret;
>> + ret = snd_soc_component_write(component, TAS2562_DVC_CFG1, 0x43);
>> + if (ret)
>> + return ret;
> Is there a reason not to use the chip default here? Otherwise this
> looks good.
Chip default is set to 0dB full blast+ 0x40400000. This sets the volume
to -110dB.
I have the values backwards. CFG4 should b 0x43 and CFG3 should be 0x0d
and CFG1&2 should be 0.
I will resend v2 with this fixed.
Dan
^ permalink raw reply
* Re: [PATCH 1/2] of: unittest: add overlay gpio test to catch gpio hog problem
From: Frank Rowand @ 2020-02-20 18:53 UTC (permalink / raw)
To: Rob Herring
Cc: Geert Uytterhoeven, pantelis.antoniou, devicetree, linux-kernel,
Alan Tull
In-Reply-To: <ff65f982-f71e-5bef-1811-fdb94fd7da2f@gmail.com>
On 2/19/20 5:37 PM, Frank Rowand wrote:
> On 2/19/20 3:56 PM, Rob Herring wrote:
>> On Tue, Jan 28, 2020 at 11:46:04PM -0600, frowand.list@gmail.com wrote:
>>> From: Frank Rowand <frank.rowand@sony.com>
>>>
>>> Geert reports that gpio hog nodes are not properly processed when
>>> the gpio hog node is added via an overlay reply and provides an
>>> RFC patch to fix the problem [1].
>>>
>>> Add a unittest that shows the problem. Unittest will report "1 failed"
>>> test before applying Geert's RFC patch and "0 failed" after applying
>>> Geert's RFC patch.
>>
>> What's the status of that? I don't want to leave the tests failing at
>> least outside of a kernel release.
>
> I agree. I would like to see my patches applied, showing the test fail,
> immediately followed by Geert's fix. So my series should not go in
> until Geert's patch is ready.
Geert has sent a v2 patch series.
I have sent a v2 of this patch, tested with v2 of Geert's patch series.
-Frank
>
>>
>>>
>>> [1] https://lore.kernel.org/linux-devicetree/20191230133852.5890-1-geert+renesas@glider.be/
>>>
>>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>>> ---
>>>
>>> There are checkpatch warnings.
>>> - New files are in a directory already covered by MAINTAINERS
>>> - The undocumented compatibles are restricted to use by unittest
>>> and should not be documented under Documentation
>>> - The printk() KERN_<LEVEL> warnings are false positives. The level
>>> is supplied by a define parameter instead of a hard coded constant
>>> - The lines over 80 characters are consistent with unittest.c style
>>>
>>> This unittest was also valuable in that it allowed me to explore
>>> possible issues related to the proposed solution to the gpio hog
>>> problem.
>>>
>>> changes since RFC:
>>> - fixed node names in overlays
>>> - removed unused fields from struct unittest_gpio_dev
>>> - of_unittest_overlay_gpio() cleaned up comments
>>> - of_unittest_overlay_gpio() moved saving global values into
>>> probe_pass_count and chip_request_count more tightly around
>>> test code expected to trigger changes in the global values
>>>
>>> drivers/of/unittest-data/Makefile | 8 +-
>>> drivers/of/unittest-data/overlay_gpio_01.dts | 23 +++
>>> drivers/of/unittest-data/overlay_gpio_02a.dts | 16 ++
>>> drivers/of/unittest-data/overlay_gpio_02b.dts | 16 ++
>>> drivers/of/unittest-data/overlay_gpio_03.dts | 23 +++
>>> drivers/of/unittest-data/overlay_gpio_04a.dts | 16 ++
>>> drivers/of/unittest-data/overlay_gpio_04b.dts | 16 ++
>>> drivers/of/unittest.c | 255 ++++++++++++++++++++++++++
>>> 8 files changed, 372 insertions(+), 1 deletion(-)
>>> create mode 100644 drivers/of/unittest-data/overlay_gpio_01.dts
>>> create mode 100644 drivers/of/unittest-data/overlay_gpio_02a.dts
>>> create mode 100644 drivers/of/unittest-data/overlay_gpio_02b.dts
>>> create mode 100644 drivers/of/unittest-data/overlay_gpio_03.dts
>>> create mode 100644 drivers/of/unittest-data/overlay_gpio_04a.dts
>>> create mode 100644 drivers/of/unittest-data/overlay_gpio_04b.dts
>>>
>>> diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
>>> index 9b6807065827..009f4045c8e4 100644
>>> --- a/drivers/of/unittest-data/Makefile
>>> +++ b/drivers/of/unittest-data/Makefile
>>> @@ -21,7 +21,13 @@ obj-$(CONFIG_OF_OVERLAY) += overlay.dtb.o \
>>> overlay_bad_add_dup_prop.dtb.o \
>>> overlay_bad_phandle.dtb.o \
>>> overlay_bad_symbol.dtb.o \
>>> - overlay_base.dtb.o
>>> + overlay_base.dtb.o \
>>> + overlay_gpio_01.dtb.o \
>>> + overlay_gpio_02a.dtb.o \
>>> + overlay_gpio_02b.dtb.o \
>>> + overlay_gpio_03.dtb.o \
>>> + overlay_gpio_04a.dtb.o \
>>> + overlay_gpio_04b.dtb.o
>>>
>>> # enable creation of __symbols__ node
>>> DTC_FLAGS_overlay += -@
>>> diff --git a/drivers/of/unittest-data/overlay_gpio_01.dts b/drivers/of/unittest-data/overlay_gpio_01.dts
>>> new file mode 100644
>>> index 000000000000..f039e8bce3b6
>>> --- /dev/null
>>> +++ b/drivers/of/unittest-data/overlay_gpio_01.dts
>>> @@ -0,0 +1,23 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/dts-v1/;
>>> +/plugin/;
>>> +
>>> +&unittest_test_bus {
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> + gpio_01 {
>>
>> Missing unit address:
>>
>> gpio@0
>
> But my changelog claimed that I fixed that, isn't that
> good enough? :-)
>
> /me pulls big brown paper bag over head.
>
> And the same for all the issues you point out below, for the
> second patch version in a row.
>
> I'll re-spin on 5.6-rc1 and truly include the fixes.
>
> -Frank
>
>
>>
>>
>>> + compatible = "unittest-gpio";
>>> + reg = <0>;
>>> + gpio-controller;
>>> + #gpio-cells = <2>;
>>> + ngpios = <2>;
>>> + gpio-line-names = "line-A", "line-B";
>>> +
>>> + line_b {
>>
>> Don't use '_'.
>>
>> line-b
>>
>>> + gpio-hog;
>>> + gpios = <2 0>;
>>> + input;
>>> + line-name = "line-B-input";
>>> + };
>>> + };
>>> +};
>>> diff --git a/drivers/of/unittest-data/overlay_gpio_02a.dts b/drivers/of/unittest-data/overlay_gpio_02a.dts
>>> new file mode 100644
>>> index 000000000000..cdafab604793
>>> --- /dev/null
>>> +++ b/drivers/of/unittest-data/overlay_gpio_02a.dts
>>> @@ -0,0 +1,16 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/dts-v1/;
>>> +/plugin/;
>>> +
>>> +&unittest_test_bus {
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> + gpio_02 {
>>
>> gpio@1
>>
>> ...and a few more.
>>
>>> + compatible = "unittest-gpio";
>>> + reg = <1>;
>>> + gpio-controller;
>>> + #gpio-cells = <2>;
>>> + ngpios = <2>;
>>> + gpio-line-names = "line-A", "line-B";
>>> + };
>>> +};
>>
>
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.