* Re: [Qemu-devel] BUG: KASAN: use-after-free in free_old_xmit_skbs
From: Jean-Philippe Menil @ 2017-06-27 12:35 UTC (permalink / raw)
To: Jason Wang, Cong Wang
Cc: Linux Kernel Network Developers, Michael S. Tsirkin,
John Fastabend, qemu-devel Developers, virtualization
In-Reply-To: <56868f1a-b4d9-50ca-873b-8fe4f70846d6@redhat.com>
On 06/27/2017 04:13 AM, Jason Wang wrote:
>
>
> On 2017年06月26日 15:35, Jean-Philippe Menil wrote:
>> On 06/26/2017 04:50 AM, Jason Wang wrote:
>>>
>>>
>>> On 2017年06月24日 06:32, Cong Wang wrote:
>>>> On Fri, Jun 23, 2017 at 1:43 AM, Jason Wang <jasowang@redhat.com>
>>>> wrote:
>>>>>
>>>>> On 2017年06月23日 02:53, Michael S. Tsirkin wrote:
>>>>>> On Thu, Jun 22, 2017 at 08:15:58AM +0200, jean-philippe menil wrote:
>>>>>>> Hi Michael,
>>>>>>>
>>>>>>> from what i see, the race appear when we hit virtnet_reset in
>>>>>>> virtnet_xdp_set.
>>>>>>> virtnet_reset
>>>>>>> _remove_vq_common
>>>>>>> virtnet_del_vqs
>>>>>>> virtnet_free_queues
>>>>>>> kfree(vi->sq)
>>>>>>> when the xdp program (with two instances of the program to
>>>>>>> trigger it
>>>>>>> faster)
>>>>>>> is added or removed.
>>>>>>>
>>>>>>> It's easily repeatable, with 2 cpus and 4 queues on the qemu command
>>>>>>> line,
>>>>>>> running the xdp_ttl tool from Jesper.
>>>>>>>
>>>>>>> For now, i'm able to continue my qualification, testing if xdp_qp
>>>>>>> is not
>>>>>>> null,
>>>>>>> but do not seem to be a sustainable trick.
>>>>>>> if (xdp_qp && vi->xdp_queues_pairs != xdp_qp)
>>>>>>>
>>>>>>> Maybe it will be more clear to you with theses informations.
>>>>>>>
>>>>>>> Best regards.
>>>>>>>
>>>>>>> Jean-Philippe
>>>>>>
>>>>>> I'm pretty clear about the issue here, I was trying to figure out
>>>>>> a fix.
>>>>>> Jason, any thoughts?
>>>>>>
>>>>>>
>>>>> Hi Jean:
>>>>>
>>>>> Does the following fix this issue? (I can't reproduce it locally
>>>>> through
>>>>> xdp_ttl)
>>>> It is tricky here.
>>>>
>>>> From my understanding of the code base, the tx_lock is not sufficient
>>>> here, because in virtnet_del_vqs() all vqs are deleted and one vp
>>>> maps to one txq.
>>>>
>>>> I am afraid you have to add a spinlock somewhere to serialized
>>>> free_old_xmit_skbs() vs. vring_del_virtqueue(). As you can see
>>>> they are in different layers, so it is hard to figure out where to add
>>>> it...
>>>>
>>>> Also, make sure we don't sleep inside the spinlock, I see a
>>>> synchronize_net().
>>>
>>> Looks like I miss something. I thought free_old_xmit_skbs() were
>>> serialized in this case since we disable all tx queues after
>>> netif_tx_unlock_bh()?
>>>
>>> Jean:
>>>
>>> I thought this could be easily reproduced by e.g produce some traffic
>>> and in the same time try to attach an xdp program. But looks not. How
>>> do you trigger this? What's your qemu command line for this?
>>>
>>> Thanks
>>
>> Hi Jason,
>>
>> this is how i trigger the bug:
>> - on the guest, tcpdump on on the interface
>> - on the guest, run iperf against the host
>> - on the guest, cat /sys/kernel/debug/tracing/trace_pipe
>> - on the guest, run one or two instances of xdp_ttl compiled with
>> DEBUG uncommented, that i start stop, until i trigger the bug.
>>
>> qemu command line is as follow:
>>
>> qemu-system-x86_64 -name ubuntu --enable-kvm -machine pc,accel=kvm
>> -smp 2 -drive file=/dev/LocalDisk/ubuntu,if=virtio,format=raw -m 2048
>> -rtc base=localtime,clock=host -usbdevice tablet --balloon virtio
>> -netdev
>> tap,id=ubuntu-0,ifname=ubuntu-0,script=/home/jenfi/WORK/jp/qemu/if-up,downscript=/home/jenfi/WORK/jp/qemu/if-down,vhost=on,queues=4
>> -device
>> virtio-net-pci,netdev=ubuntu-0,mac=de:ad:be:ef:01:03,mq=on,guest_tso4=off,guest_tso6=off,guest_ecn=off,guest_ufo=off,vectors=2
>> -vnc 127.0.0.1:3 -nographic -serial
>> file:/home/jenfi/WORK/jp/qemu/ubuntu.out -monitor
>> unix:/home/jenfi/WORK/jp/qemu/ubuntu.sock,server,nowait
>>
>> Notice, the smp 2, queues to 4 and vectors to 2.
>> Seem that if fogot to mention that in the beginning of this thread,
>> sorry for that.
>>
>> Best regards.
>>
>> Jean-Philippe
>>
>
> Thanks Jean, I manage to reproduce the issue.
>
> I thought netif_tx_unlock_bh() will do tx lock but looks not, that's why
> previous patch doesn't work.
>
> Could you please this this patch? (At least it can't trigger the warning
> after more than 20 times of xdp start/stop).
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 1f8c15c..a18f859 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1802,6 +1802,7 @@ static void virtnet_freeze_down(struct
> virtio_device *vdev)
> flush_work(&vi->config_work);
>
> netif_device_detach(vi->dev);
> + netif_tx_disable(vi->dev);
> cancel_delayed_work_sync(&vi->refill);
>
> if (netif_running(vi->dev)) {
>
>
Hi Jason,
Seem to do the trick !
with your patch, i'm unable to repeat the problem anymore (running more
than 2h without any issue).
Best regards.
Jean-Philippe
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [Nouveau] [PATCH v2 00/14] improve the fb_setcmap helper
From: Daniel Vetter @ 2017-06-27 7:19 UTC (permalink / raw)
To: Dave Airlie
Cc: Boris Brezillon, dri-devel, Christian König, nouveau,
intel-gfx@lists.freedesktop.org, Philippe Cornu,
amd-gfx mailing list, Russell King, Yannick Fertre, Daniel Vetter,
Alex Deucher, Dave Airlie, open list:VIRTIO CORE, NET...,
Vincent Abriou, Peter Rosin, LKML, Ben Skeggs
In-Reply-To: <CAPM=9tzr_B9cWG4gEU_+PgkmVnpKtfBFGfQ85gwWpFs+3WQMjg@mail.gmail.com>
On Tue, Jun 27, 2017 at 05:50:38AM +1000, Dave Airlie wrote:
> >
> > I'm traveling and cannot make progress this week. The merge window is
> > also real close so this series will therefore probably miss it unless
> > something unexpected happens...
>
> Don't worry you missed the merge window for drm already, we don't merge
> things after -rc6. Please remember the Linus merge window is for maintainers
> to merge stuff to Linus, things need to be ready long before it, and for drm
> you shouldn't really tie yourself to the merge cycle.
Yeah, drm-misc is open for refactorings like this all the time, and
maintainers make sure the code will get into upstream asap, without
interferring with the upstream merge window schedules. Like Dave said,
don't worry.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply
* Re: [Qemu-devel] BUG: KASAN: use-after-free in free_old_xmit_skbs
From: Jason Wang @ 2017-06-27 2:13 UTC (permalink / raw)
To: jpmenil, Cong Wang
Cc: Linux Kernel Network Developers, Michael S. Tsirkin,
John Fastabend, qemu-devel Developers, virtualization
In-Reply-To: <f91048e7-5567-cdf8-fd98-3077ca7ac9a3@gmail.com>
On 2017年06月26日 15:35, Jean-Philippe Menil wrote:
> On 06/26/2017 04:50 AM, Jason Wang wrote:
>>
>>
>> On 2017年06月24日 06:32, Cong Wang wrote:
>>> On Fri, Jun 23, 2017 at 1:43 AM, Jason Wang <jasowang@redhat.com>
>>> wrote:
>>>>
>>>> On 2017年06月23日 02:53, Michael S. Tsirkin wrote:
>>>>> On Thu, Jun 22, 2017 at 08:15:58AM +0200, jean-philippe menil wrote:
>>>>>> Hi Michael,
>>>>>>
>>>>>> from what i see, the race appear when we hit virtnet_reset in
>>>>>> virtnet_xdp_set.
>>>>>> virtnet_reset
>>>>>> _remove_vq_common
>>>>>> virtnet_del_vqs
>>>>>> virtnet_free_queues
>>>>>> kfree(vi->sq)
>>>>>> when the xdp program (with two instances of the program to
>>>>>> trigger it
>>>>>> faster)
>>>>>> is added or removed.
>>>>>>
>>>>>> It's easily repeatable, with 2 cpus and 4 queues on the qemu command
>>>>>> line,
>>>>>> running the xdp_ttl tool from Jesper.
>>>>>>
>>>>>> For now, i'm able to continue my qualification, testing if xdp_qp
>>>>>> is not
>>>>>> null,
>>>>>> but do not seem to be a sustainable trick.
>>>>>> if (xdp_qp && vi->xdp_queues_pairs != xdp_qp)
>>>>>>
>>>>>> Maybe it will be more clear to you with theses informations.
>>>>>>
>>>>>> Best regards.
>>>>>>
>>>>>> Jean-Philippe
>>>>>
>>>>> I'm pretty clear about the issue here, I was trying to figure out
>>>>> a fix.
>>>>> Jason, any thoughts?
>>>>>
>>>>>
>>>> Hi Jean:
>>>>
>>>> Does the following fix this issue? (I can't reproduce it locally
>>>> through
>>>> xdp_ttl)
>>> It is tricky here.
>>>
>>> From my understanding of the code base, the tx_lock is not sufficient
>>> here, because in virtnet_del_vqs() all vqs are deleted and one vp
>>> maps to one txq.
>>>
>>> I am afraid you have to add a spinlock somewhere to serialized
>>> free_old_xmit_skbs() vs. vring_del_virtqueue(). As you can see
>>> they are in different layers, so it is hard to figure out where to add
>>> it...
>>>
>>> Also, make sure we don't sleep inside the spinlock, I see a
>>> synchronize_net().
>>
>> Looks like I miss something. I thought free_old_xmit_skbs() were
>> serialized in this case since we disable all tx queues after
>> netif_tx_unlock_bh()?
>>
>> Jean:
>>
>> I thought this could be easily reproduced by e.g produce some traffic
>> and in the same time try to attach an xdp program. But looks not. How
>> do you trigger this? What's your qemu command line for this?
>>
>> Thanks
>
> Hi Jason,
>
> this is how i trigger the bug:
> - on the guest, tcpdump on on the interface
> - on the guest, run iperf against the host
> - on the guest, cat /sys/kernel/debug/tracing/trace_pipe
> - on the guest, run one or two instances of xdp_ttl compiled with
> DEBUG uncommented, that i start stop, until i trigger the bug.
>
> qemu command line is as follow:
>
> qemu-system-x86_64 -name ubuntu --enable-kvm -machine pc,accel=kvm
> -smp 2 -drive file=/dev/LocalDisk/ubuntu,if=virtio,format=raw -m 2048
> -rtc base=localtime,clock=host -usbdevice tablet --balloon virtio
> -netdev
> tap,id=ubuntu-0,ifname=ubuntu-0,script=/home/jenfi/WORK/jp/qemu/if-up,downscript=/home/jenfi/WORK/jp/qemu/if-down,vhost=on,queues=4
> -device
> virtio-net-pci,netdev=ubuntu-0,mac=de:ad:be:ef:01:03,mq=on,guest_tso4=off,guest_tso6=off,guest_ecn=off,guest_ufo=off,vectors=2
> -vnc 127.0.0.1:3 -nographic -serial
> file:/home/jenfi/WORK/jp/qemu/ubuntu.out -monitor
> unix:/home/jenfi/WORK/jp/qemu/ubuntu.sock,server,nowait
>
> Notice, the smp 2, queues to 4 and vectors to 2.
> Seem that if fogot to mention that in the beginning of this thread,
> sorry for that.
>
> Best regards.
>
> Jean-Philippe
>
Thanks Jean, I manage to reproduce the issue.
I thought netif_tx_unlock_bh() will do tx lock but looks not, that's why
previous patch doesn't work.
Could you please this this patch? (At least it can't trigger the warning
after more than 20 times of xdp start/stop).
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 1f8c15c..a18f859 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1802,6 +1802,7 @@ static void virtnet_freeze_down(struct
virtio_device *vdev)
flush_work(&vi->config_work);
netif_device_detach(vi->dev);
+ netif_tx_disable(vi->dev);
cancel_delayed_work_sync(&vi->refill);
if (netif_running(vi->dev)) {
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* Re: [PATCH v2 00/14] improve the fb_setcmap helper
From: Dave Airlie @ 2017-06-26 19:50 UTC (permalink / raw)
To: Peter Rosin
Cc: Boris Brezillon, Philippe Cornu, Russell King, nouveau,
intel-gfx@lists.freedesktop.org, LKML, amd-gfx mailing list,
open list:VIRTIO CORE, NET..., Yannick Fertre, dri-devel,
Dave Airlie, Alex Deucher, Daniel Vetter, Vincent Abriou,
Christian König, Ben Skeggs
In-Reply-To: <59515CCF.8060406@axentia.se>
>
> I'm traveling and cannot make progress this week. The merge window is
> also real close so this series will therefore probably miss it unless
> something unexpected happens...
Don't worry you missed the merge window for drm already, we don't merge
things after -rc6. Please remember the Linus merge window is for maintainers
to merge stuff to Linus, things need to be ready long before it, and for drm
you shouldn't really tie yourself to the merge cycle.
Dave.
^ permalink raw reply
* Re: [PATCH v2 00/14] improve the fb_setcmap helper
From: Peter Rosin @ 2017-06-26 19:13 UTC (permalink / raw)
To: linux-kernel, nouveau, dri-devel, virtualization, amd-gfx,
Daniel Vetter, Boris Brezillon, Russell King, Ben Skeggs,
Dave Airlie, intel-gfx, Vincent Abriou, Philippe Cornu,
Yannick Fertre, Alex Deucher, Christian König, Gerd Hoffmann
In-Reply-To: <20170626093526.7dezn6d74m5ktsok@phenom.ffwll.local>
On 2017-06-26 11:35, Daniel Vetter wrote:
> On Thu, Jun 22, 2017 at 08:06:23AM +0200, Peter Rosin wrote:
>> Hi!
>>
>> While trying to get CLUT support for the atmel_hlcdc driver, and
>> specifically for the emulated fbdev interface, I received some
>> push-back that my feeble in-driver attempts should be solved
>> by the core. This is my attempt to do it right.
>>
>> I have obviously not tested all of this with more than a compile,
>> but patches 1 and 3 are enough to make the atmel-hlcdc driver
>> do what I need (when patched to support CLUT modes). The rest is
>> just lots of removals and cleanup made possible by the improved
>> core.
>>
>> Please test, I would not be surprised if I have fouled up some
>> bit-manipulation somewhere in this mostly mechanical change...
>>
>> Changes since v1:
>>
>> - Rebased to next-20170621
>> - Split 1/11 into a preparatory patch, a cleanup patch and then
>> the meat in 3/14.
>> - Handle pseudo-palette for FB_VISUAL_TRUECOLOR.
>> - Removed the empty .gamma_get/.gamma_set fb helpers from the
>> armada driver that I had somehow managed to ignore but which
>> 0day found real quick.
>> - Be less judgemental on drivers only providing .gamma_get and
>> .gamma_set, but no .load_lut. That's actually a valid thing
>> to do if you only need pseudo-palette for FB_VISUAL_TRUECOLOR.
>> - Add a comment about colliding bitfields in the nouveau driver.
>> - Remove gamma_set/gamma_get declarations from the radeon driver
>> (the definitions were removed in v1).
>
> Ok some nits/questions on the first three, but in principle looks all ok I
> think. The driver patches also look good (but I didn't yet carefully
> review all the conversion). What we might want to do is entirely remove
> driver's reliance on ->gamma_store (mostly amounts to in-lining the
> load_lut functions) and only update ->gamma_store after gamma_set returned
> successfully. But that's a bit more work.
>
> Save/restoring it instead might be simpler to fix that bug, but since it's
> pre-existing also ok as follow-up.
I'm traveling and cannot make progress this week. The merge window is
also real close so this series will therefore probably miss it unless
something unexpected happens...
I'll get back to this for the next cycle, just a heads up.
Cheers,
peda
^ permalink raw reply
* Re: [PATCH] virtio_scsi: let host do exception handling
From: Martin K. Petersen @ 2017-06-26 17:11 UTC (permalink / raw)
To: Paolo Bonzini
Cc: James E.J. Bottomley, linux-scsi, Martin K. Petersen,
linux-kernel, stable, virtualization, Douglas Miller
In-Reply-To: <20170621143546.5826-1-pbonzini@redhat.com>
Paolo,
> virtio_scsi tries to do exception handling after the default 30
> seconds timeout expires. However, it's better to let the host control
> the timeout, otherwise with a heavy I/O load it is likely that an
> abort will also timeout. This leads to fatal errors like filesystems
> going offline.
>
> Disable the 'sd' timeout and allow the host to do exception handling,
> following the precedent of the storvsc driver.
Applied to 4.13/scsi-queue. Thank you!
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* Re: [PATCH v2 00/14] improve the fb_setcmap helper
From: Daniel Vetter @ 2017-06-26 9:35 UTC (permalink / raw)
To: Peter Rosin
Cc: Boris Brezillon, Philippe Cornu, Russell King, nouveau, intel-gfx,
linux-kernel, amd-gfx, virtualization, Yannick Fertre, dri-devel,
Dave Airlie, Alex Deucher, Daniel Vetter, Vincent Abriou,
Christian König, Ben Skeggs
In-Reply-To: <1498111597-10714-1-git-send-email-peda@axentia.se>
On Thu, Jun 22, 2017 at 08:06:23AM +0200, Peter Rosin wrote:
> Hi!
>
> While trying to get CLUT support for the atmel_hlcdc driver, and
> specifically for the emulated fbdev interface, I received some
> push-back that my feeble in-driver attempts should be solved
> by the core. This is my attempt to do it right.
>
> I have obviously not tested all of this with more than a compile,
> but patches 1 and 3 are enough to make the atmel-hlcdc driver
> do what I need (when patched to support CLUT modes). The rest is
> just lots of removals and cleanup made possible by the improved
> core.
>
> Please test, I would not be surprised if I have fouled up some
> bit-manipulation somewhere in this mostly mechanical change...
>
> Changes since v1:
>
> - Rebased to next-20170621
> - Split 1/11 into a preparatory patch, a cleanup patch and then
> the meat in 3/14.
> - Handle pseudo-palette for FB_VISUAL_TRUECOLOR.
> - Removed the empty .gamma_get/.gamma_set fb helpers from the
> armada driver that I had somehow managed to ignore but which
> 0day found real quick.
> - Be less judgemental on drivers only providing .gamma_get and
> .gamma_set, but no .load_lut. That's actually a valid thing
> to do if you only need pseudo-palette for FB_VISUAL_TRUECOLOR.
> - Add a comment about colliding bitfields in the nouveau driver.
> - Remove gamma_set/gamma_get declarations from the radeon driver
> (the definitions were removed in v1).
Ok some nits/questions on the first three, but in principle looks all ok I
think. The driver patches also look good (but I didn't yet carefully
review all the conversion). What we might want to do is entirely remove
driver's reliance on ->gamma_store (mostly amounts to in-lining the
load_lut functions) and only update ->gamma_store after gamma_set returned
successfully. But that's a bit more work.
Save/restoring it instead might be simpler to fix that bug, but since it's
pre-existing also ok as follow-up.
Cheers, Daniel
>
> Cheers,
> peda
>
> Peter Rosin (14):
> drm/fb-helper: keep the .gamma_store updated in drm_fb_helper_setcmap
> drm/fb-helper: remove drm_fb_helper_save_lut_atomic
> drm/fb-helper: do a generic fb_setcmap helper in terms of crtc
> .gamma_set
> drm: amd: remove dead code and pointless local lut storage
> drm: armada: remove dead empty functions
> drm: ast: remove dead code and pointless local lut storage
> drm: cirrus: remove dead code and pointless local lut storage
> drm: gma500: remove dead code and pointless local lut storage
> drm: i915: remove dead code and pointless local lut storage
> drm: mgag200: remove dead code and pointless local lut storage
> drm: nouveau: remove dead code and pointless local lut storage
> drm: radeon: remove dead code and pointless local lut storage
> drm: stm: remove dead code and pointless local lut storage
> drm: remove unused and redundant callbacks
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c | 24 ----
> drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 1 -
> drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 27 ++---
> drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 27 ++---
> drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 27 ++---
> drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 27 ++---
> drivers/gpu/drm/amd/amdgpu/dce_virtual.c | 23 ----
> drivers/gpu/drm/armada/armada_crtc.c | 10 --
> drivers/gpu/drm/armada/armada_crtc.h | 2 -
> drivers/gpu/drm/armada/armada_fbdev.c | 2 -
> drivers/gpu/drm/ast/ast_drv.h | 1 -
> drivers/gpu/drm/ast/ast_fb.c | 20 ----
> drivers/gpu/drm/ast/ast_mode.c | 26 +----
> drivers/gpu/drm/cirrus/cirrus_drv.h | 8 --
> drivers/gpu/drm/cirrus/cirrus_fbdev.c | 2 -
> drivers/gpu/drm/cirrus/cirrus_mode.c | 71 +++---------
> drivers/gpu/drm/drm_fb_helper.c | 164 +++++++++++++---------------
> drivers/gpu/drm/gma500/framebuffer.c | 22 ----
> drivers/gpu/drm/gma500/gma_display.c | 32 ++----
> drivers/gpu/drm/gma500/psb_intel_display.c | 7 +-
> drivers/gpu/drm/gma500/psb_intel_drv.h | 1 -
> drivers/gpu/drm/i915/intel_drv.h | 1 -
> drivers/gpu/drm/i915/intel_fbdev.c | 31 ------
> drivers/gpu/drm/mgag200/mgag200_drv.h | 5 -
> drivers/gpu/drm/mgag200/mgag200_fb.c | 2 -
> drivers/gpu/drm/mgag200/mgag200_mode.c | 62 +++--------
> drivers/gpu/drm/nouveau/dispnv04/crtc.c | 26 ++---
> drivers/gpu/drm/nouveau/nouveau_crtc.h | 3 -
> drivers/gpu/drm/nouveau/nouveau_fbcon.c | 22 ----
> drivers/gpu/drm/nouveau/nv50_display.c | 40 +++----
> drivers/gpu/drm/radeon/atombios_crtc.c | 1 -
> drivers/gpu/drm/radeon/radeon_connectors.c | 7 +-
> drivers/gpu/drm/radeon/radeon_display.c | 71 +++++-------
> drivers/gpu/drm/radeon/radeon_fb.c | 2 -
> drivers/gpu/drm/radeon/radeon_legacy_crtc.c | 1 -
> drivers/gpu/drm/radeon/radeon_mode.h | 4 -
> drivers/gpu/drm/stm/ltdc.c | 12 --
> drivers/gpu/drm/stm/ltdc.h | 1 -
> include/drm/drm_fb_helper.h | 32 ------
> include/drm/drm_modeset_helper_vtables.h | 16 ---
> 40 files changed, 205 insertions(+), 658 deletions(-)
>
> --
> 2.1.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply
* Re: [PATCH v2 03/14] drm/fb-helper: do a generic fb_setcmap helper in terms of crtc .gamma_set
From: Daniel Vetter @ 2017-06-26 9:30 UTC (permalink / raw)
To: Peter Rosin
Cc: Boris Brezillon, Dave Airlie, nouveau, intel-gfx, linux-kernel,
amd-gfx, Philippe Cornu, Yannick Fertre, Ben Skeggs, dri-devel,
Daniel Vetter, Alex Deucher, Russell King, virtualization,
Vincent Abriou, Christian König
In-Reply-To: <fd73b01e-3327-faf1-c3eb-80663b739ff6@axentia.se>
On Thu, Jun 22, 2017 at 12:22:10PM +0200, Peter Rosin wrote:
> This makes the redundant fb helpers .load_lut, .gamma_set and .gamma_get
> totally obsolete.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
> drivers/gpu/drm/drm_fb_helper.c | 151 +++++++++++++++++-----------------------
> 1 file changed, 63 insertions(+), 88 deletions(-)
>
> This is an alternative version rebased on top of Daniel's "fbdev helper
> locking rework and deferred setup" series.
>
> And as noted by Daniel, .gamma_set does an atomic commit. Thus, the locks
> needs to be dropped and reacquired for each crtc. So, that is fixed here
> too. Doing it like this with a couple of individual alternative patches
> instead of sending a whole new series since the dependency on Daniel's
> series makes life somewhat difficult...
>
> Cheers,
> peda
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 4aceb59..aa025f1 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1257,50 +1257,6 @@ void drm_fb_helper_set_suspend_unlocked(struct drm_fb_helper *fb_helper,
> }
> EXPORT_SYMBOL(drm_fb_helper_set_suspend_unlocked);
>
> -static int setcolreg(struct drm_crtc *crtc, u16 red, u16 green,
> - u16 blue, u16 regno, struct fb_info *info)
> -{
> - struct drm_fb_helper *fb_helper = info->par;
> - struct drm_framebuffer *fb = fb_helper->fb;
> -
> - if (info->fix.visual == FB_VISUAL_TRUECOLOR) {
> - u32 *palette;
> - u32 value;
> - /* place color in psuedopalette */
> - if (regno > 16)
> - return -EINVAL;
> - palette = (u32 *)info->pseudo_palette;
> - red >>= (16 - info->var.red.length);
> - green >>= (16 - info->var.green.length);
> - blue >>= (16 - info->var.blue.length);
> - value = (red << info->var.red.offset) |
> - (green << info->var.green.offset) |
> - (blue << info->var.blue.offset);
> - if (info->var.transp.length > 0) {
> - u32 mask = (1 << info->var.transp.length) - 1;
> -
> - mask <<= info->var.transp.offset;
> - value |= mask;
> - }
> - palette[regno] = value;
> - return 0;
> - }
> -
> - /*
> - * The driver really shouldn't advertise pseudo/directcolor
> - * visuals if it can't deal with the palette.
> - */
> - if (WARN_ON(!fb_helper->funcs->gamma_set ||
> - !fb_helper->funcs->gamma_get))
> - return -EINVAL;
> -
> - WARN_ON(fb->format->cpp[0] != 1);
> -
> - fb_helper->funcs->gamma_set(crtc, red, green, blue, regno);
> -
> - return 0;
> -}
> -
> /**
> * drm_fb_helper_setcmap - implementation for &fb_ops.fb_setcmap
> * @cmap: cmap to set
> @@ -1310,12 +1266,10 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
> {
> struct drm_fb_helper *fb_helper = info->par;
> struct drm_device *dev = fb_helper->dev;
> - const struct drm_crtc_helper_funcs *crtc_funcs;
> - u16 *red, *green, *blue, *transp;
> + struct drm_modeset_acquire_ctx ctx;
> struct drm_crtc *crtc;
> u16 *r, *g, *b;
> - int i, j, rc = 0;
> - int start;
> + int i, ret = 0;
>
> if (oops_in_progress)
> return -EBUSY;
> @@ -1329,61 +1283,82 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
> return -EBUSY;
> }
>
> - drm_modeset_lock_all(dev);
> - for (i = 0; i < fb_helper->crtc_count; i++) {
> - crtc = fb_helper->crtc_info[i].mode_set.crtc;
> - crtc_funcs = crtc->helper_private;
> + drm_modeset_acquire_init(&ctx, 0);
>
> - red = cmap->red;
> - green = cmap->green;
> - blue = cmap->blue;
> - transp = cmap->transp;
> - start = cmap->start;
> + for (i = 0; i < fb_helper->crtc_count; i++) {
> + if (info->fix.visual == FB_VISUAL_TRUECOLOR) {
> + u32 *palette;
> + int j;
>
> - if (info->fix.visual != FB_VISUAL_TRUECOLOR) {
> - if (!crtc->gamma_size) {
> - rc = -EINVAL;
> - goto out;
> + if (cmap->start + cmap->len > 16) {
> + ret = -EINVAL;
> + break;
> }
>
> - if (cmap->start + cmap->len > crtc->gamma_size) {
> - rc = -EINVAL;
> - goto out;
> + palette = (u32 *)info->pseudo_palette;
> + for (j = 0; j < cmap->len; ++j) {
> + u16 red = cmap->red[j];
> + u16 green = cmap->green[j];
> + u16 blue = cmap->blue[j];
> + u32 value;
> +
> + red >>= 16 - info->var.red.length;
> + green >>= 16 - info->var.green.length;
> + blue >>= 16 - info->var.blue.length;
> + value = (red << info->var.red.offset) |
> + (green << info->var.green.offset) |
> + (blue << info->var.blue.offset);
> + if (info->var.transp.length > 0) {
> + u32 mask = (1 << info->var.transp.length) - 1;
> +
> + mask <<= info->var.transp.offset;
> + value |= mask;
> + }
> + palette[cmap->start + j] = value;
> }
> + continue;
> + }
>
> - r = crtc->gamma_store;
> - g = r + crtc->gamma_size;
> - b = g + crtc->gamma_size;
> +retry:
> + ret = drm_modeset_lock_all_ctx(dev, &ctx);
> + if (ret)
> + break;
>
> - memcpy(r + cmap->start, cmap->red,
> - cmap->len * sizeof(u16));
> - memcpy(g + cmap->start, cmap->green,
> - cmap->len * sizeof(u16));
> - memcpy(b + cmap->start, cmap->blue,
> - cmap->len * sizeof(u16));
> + crtc = fb_helper->crtc_info[i].mode_set.crtc;
> + if (!crtc->funcs->gamma_set || !crtc->gamma_size) {
> + ret = -EINVAL;
> + goto drop_locks;
> }
>
> - for (j = 0; j < cmap->len; j++) {
> - u16 hred, hgreen, hblue, htransp = 0xffff;
> + if (cmap->start + cmap->len > crtc->gamma_size) {
> + ret = -EINVAL;
> + goto drop_locks;
> + }
>
> - hred = *red++;
> - hgreen = *green++;
> - hblue = *blue++;
> + r = crtc->gamma_store;
> + g = r + crtc->gamma_size;
> + b = g + crtc->gamma_size;
>
> - if (transp)
> - htransp = *transp++;
> + memcpy(r + cmap->start, cmap->red, cmap->len * sizeof(u16));
> + memcpy(g + cmap->start, cmap->green, cmap->len * sizeof(u16));
> + memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(u16));
>
> - rc = setcolreg(crtc, hred, hgreen, hblue, start++, info);
> - if (rc)
> - goto out;
> + ret = crtc->funcs->gamma_set(crtc, r, g, b,
> + crtc->gamma_size, &ctx);
> + if (ret == -EDEADLK) {
> + drm_modeset_backoff(&ctx);
> + goto retry;
> }
> - if (crtc_funcs->load_lut)
> - crtc_funcs->load_lut(crtc);
> +drop_locks:
> + drm_modeset_drop_locks(&ctx);
This doesn't work, you can't reuse the acquire_ctx (that's the bit where
the locking machinery gets mad at you). Simplest way really is 2 paths,
one legacy (using drm_modeset_lock_all), the other atomic (with the
gamma_set helper logic inlined, but as one global atomic commit).
I guess updating the gamma_store could be done with common code again, on
success only (another reason for atomic, that way you get all or nothing,
makes error handling easier).
-Daniel
> + if (ret)
> + break;
> }
> - out:
> - drm_modeset_unlock_all(dev);
> +
> + drm_modeset_acquire_fini(&ctx);
> mutex_unlock(&fb_helper->lock);
> - return rc;
> +
> + return ret;
> }
> EXPORT_SYMBOL(drm_fb_helper_setcmap);
>
> --
> 2.1.4
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply
* Re: [PATCH v2 01/14] drm/fb-helper: keep the .gamma_store updated in drm_fb_helper_setcmap
From: Daniel Vetter @ 2017-06-26 9:27 UTC (permalink / raw)
To: Peter Rosin
Cc: Boris Brezillon, Philippe Cornu, Russell King, nouveau, intel-gfx,
linux-kernel, amd-gfx, virtualization, Yannick Fertre, dri-devel,
Dave Airlie, Alex Deucher, Daniel Vetter, Vincent Abriou,
Christian König, Ben Skeggs
In-Reply-To: <1498111597-10714-2-git-send-email-peda@axentia.se>
On Thu, Jun 22, 2017 at 08:06:24AM +0200, Peter Rosin wrote:
> I think the gamma_store can end up invalid on error. But the way I read
> it, that can happen in drm_mode_gamma_set_ioctl as well, so why should
> this pesky legacy fbdev stuff be any better?
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
> drivers/gpu/drm/drm_fb_helper.c | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 574af01..25315fb 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1223,12 +1223,16 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
> const struct drm_crtc_helper_funcs *crtc_funcs;
> u16 *red, *green, *blue, *transp;
> struct drm_crtc *crtc;
> + u16 *r, *g, *b;
> int i, j, rc = 0;
> int start;
>
> if (oops_in_progress)
> return -EBUSY;
>
> + if (cmap->start + cmap->len < cmap->start)
> + return -EINVAL;
Doesn't the fbdev core check this for us?
> +
> drm_modeset_lock_all(dev);
> if (!drm_fb_helper_is_bound(fb_helper)) {
> drm_modeset_unlock_all(dev);
> @@ -1245,6 +1249,29 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
> transp = cmap->transp;
> start = cmap->start;
>
> + if (info->fix.visual != FB_VISUAL_TRUECOLOR) {
I think switching to a positive check of visual == PSEUDOCOLOR is a bit
clearer. Also this makes more sense once we move over to the main
gamma_set function, since as-is it's unclear that setcolreg only does
something for PSEUDOCOLOR. But interim I guess that's ok.
-Daniel
> + if (!crtc->gamma_size) {
> + rc = -EINVAL;
> + goto out;
> + }
> +
> + if (cmap->start + cmap->len > crtc->gamma_size) {
> + rc = -EINVAL;
> + goto out;
> + }
> +
> + r = crtc->gamma_store;
> + g = r + crtc->gamma_size;
> + b = g + crtc->gamma_size;
> +
> + memcpy(r + cmap->start, cmap->red,
> + cmap->len * sizeof(u16));
> + memcpy(g + cmap->start, cmap->green,
> + cmap->len * sizeof(u16));
> + memcpy(b + cmap->start, cmap->blue,
> + cmap->len * sizeof(u16));
> + }
> +
> for (j = 0; j < cmap->len; j++) {
> u16 hred, hgreen, hblue, htransp = 0xffff;
>
> --
> 2.1.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply
* Re: [PATCH v2 03/14] drm/fb-helper: do a generic fb_setcmap helper in terms of crtc .gamma_set
From: Daniel Vetter @ 2017-06-26 9:23 UTC (permalink / raw)
To: Peter Rosin
Cc: Boris Brezillon, Philippe Cornu, Russell King, nouveau, intel-gfx,
linux-kernel, amd-gfx, virtualization, Yannick Fertre, dri-devel,
Dave Airlie, Alex Deucher, Daniel Vetter, Vincent Abriou,
Christian König, Ben Skeggs
In-Reply-To: <1498111597-10714-4-git-send-email-peda@axentia.se>
On Thu, Jun 22, 2017 at 08:06:26AM +0200, Peter Rosin wrote:
> This makes the redundant fb helpers .load_lut, .gamma_set and .gamma_get
> totally obsolete.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
> drivers/gpu/drm/drm_fb_helper.c | 154 ++++++++++++++++------------------------
> 1 file changed, 63 insertions(+), 91 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 7ade384..58eb045 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1150,50 +1150,6 @@ void drm_fb_helper_set_suspend_unlocked(struct drm_fb_helper *fb_helper,
> }
> EXPORT_SYMBOL(drm_fb_helper_set_suspend_unlocked);
>
> -static int setcolreg(struct drm_crtc *crtc, u16 red, u16 green,
> - u16 blue, u16 regno, struct fb_info *info)
> -{
> - struct drm_fb_helper *fb_helper = info->par;
> - struct drm_framebuffer *fb = fb_helper->fb;
> -
> - if (info->fix.visual == FB_VISUAL_TRUECOLOR) {
> - u32 *palette;
> - u32 value;
> - /* place color in psuedopalette */
> - if (regno > 16)
> - return -EINVAL;
> - palette = (u32 *)info->pseudo_palette;
> - red >>= (16 - info->var.red.length);
> - green >>= (16 - info->var.green.length);
> - blue >>= (16 - info->var.blue.length);
> - value = (red << info->var.red.offset) |
> - (green << info->var.green.offset) |
> - (blue << info->var.blue.offset);
> - if (info->var.transp.length > 0) {
> - u32 mask = (1 << info->var.transp.length) - 1;
> -
> - mask <<= info->var.transp.offset;
> - value |= mask;
> - }
> - palette[regno] = value;
> - return 0;
> - }
> -
> - /*
> - * The driver really shouldn't advertise pseudo/directcolor
> - * visuals if it can't deal with the palette.
> - */
> - if (WARN_ON(!fb_helper->funcs->gamma_set ||
> - !fb_helper->funcs->gamma_get))
> - return -EINVAL;
> -
> - WARN_ON(fb->format->cpp[0] != 1);
> -
> - fb_helper->funcs->gamma_set(crtc, red, green, blue, regno);
> -
> - return 0;
> -}
> -
> /**
> * drm_fb_helper_setcmap - implementation for &fb_ops.fb_setcmap
> * @cmap: cmap to set
> @@ -1203,12 +1159,10 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
> {
> struct drm_fb_helper *fb_helper = info->par;
> struct drm_device *dev = fb_helper->dev;
> - const struct drm_crtc_helper_funcs *crtc_funcs;
> - u16 *red, *green, *blue, *transp;
> + struct drm_modeset_acquire_ctx ctx;
> struct drm_crtc *crtc;
> u16 *r, *g, *b;
> - int i, j, rc = 0;
> - int start;
> + int i, ret;
>
> if (oops_in_progress)
> return -EBUSY;
> @@ -1216,65 +1170,83 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
> if (cmap->start + cmap->len < cmap->start)
> return -EINVAL;
>
> - drm_modeset_lock_all(dev);
> + drm_modeset_acquire_init(&ctx, 0);
> +retry:
> + ret = drm_modeset_lock_all_ctx(dev, &ctx);
> + if (ret)
> + goto out;
> if (!drm_fb_helper_is_bound(fb_helper)) {
> - drm_modeset_unlock_all(dev);
> - return -EBUSY;
> + ret = -EBUSY;
> + goto out;
> }
>
> for (i = 0; i < fb_helper->crtc_count; i++) {
> - crtc = fb_helper->crtc_info[i].mode_set.crtc;
> - crtc_funcs = crtc->helper_private;
> -
> - red = cmap->red;
> - green = cmap->green;
> - blue = cmap->blue;
> - transp = cmap->transp;
> - start = cmap->start;
> + if (info->fix.visual == FB_VISUAL_TRUECOLOR) {
> + u32 *palette;
> + int j;
>
> - if (info->fix.visual != FB_VISUAL_TRUECOLOR) {
> - if (!crtc->gamma_size) {
> - rc = -EINVAL;
> + if (cmap->start + cmap->len > 16) {
> + ret = -EINVAL;
> goto out;
> }
>
> - if (cmap->start + cmap->len > crtc->gamma_size) {
> - rc = -EINVAL;
> - goto out;
> + palette = (u32 *)info->pseudo_palette;
> + for (j = 0; j < cmap->len; ++j) {
> + u16 red = cmap->red[j];
> + u16 green = cmap->green[j];
> + u16 blue = cmap->blue[j];
> + u32 value;
> +
> + red >>= 16 - info->var.red.length;
> + green >>= 16 - info->var.green.length;
> + blue >>= 16 - info->var.blue.length;
> + value = (red << info->var.red.offset) |
> + (green << info->var.green.offset) |
> + (blue << info->var.blue.offset);
> + if (info->var.transp.length > 0) {
> + u32 mask = (1 << info->var.transp.length) - 1;
> +
> + mask <<= info->var.transp.offset;
> + value |= mask;
> + }
> + palette[cmap->start + j] = value;
> }
> + continue;
I think it'd be much cleaner if we handle the TRUECOLOR case as an early
return, before the crtc loop. Per-crtc is only needed when we have to
update the hw table (i.e. PSEUDOCOLOR stuff).
> + }
>
> - r = crtc->gamma_store;
> - g = r + crtc->gamma_size;
> - b = g + crtc->gamma_size;
> -
> - memcpy(r + cmap->start, cmap->red,
> - cmap->len * sizeof(u16));
> - memcpy(g + cmap->start, cmap->green,
> - cmap->len * sizeof(u16));
> - memcpy(b + cmap->start, cmap->blue,
> - cmap->len * sizeof(u16));
> + crtc = fb_helper->crtc_info[i].mode_set.crtc;
> + if (!crtc->funcs->gamma_set || !crtc->gamma_size) {
> + ret = -EINVAL;
> + goto out;
> }
>
> - for (j = 0; j < cmap->len; j++) {
> - u16 hred, hgreen, hblue, htransp = 0xffff;
> + if (cmap->start + cmap->len > crtc->gamma_size) {
> + ret = -EINVAL;
> + goto out;
> + }
>
> - hred = *red++;
> - hgreen = *green++;
> - hblue = *blue++;
> + r = crtc->gamma_store;
> + g = r + crtc->gamma_size;
> + b = g + crtc->gamma_size;
>
> - if (transp)
> - htransp = *transp++;
> + memcpy(r + cmap->start, cmap->red, cmap->len * sizeof(u16));
> + memcpy(g + cmap->start, cmap->green, cmap->len * sizeof(u16));
> + memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(u16));
>
> - rc = setcolreg(crtc, hred, hgreen, hblue, start++, info);
> - if (rc)
> - goto out;
> - }
> - if (crtc_funcs->load_lut)
> - crtc_funcs->load_lut(crtc);
> + ret = crtc->funcs->gamma_set(crtc, r, g, b,
> + crtc->gamma_size, &ctx);
As discussed on earlier threads, I think the cleanest version here would
be 2 functions, one which does the crtc-loop like in your code here for
legacy drivers, and the 2nd one which does 1 atomic commit over all crtcs,
open-coding the gamma_set helper. For the legacy one you can also keep
using drm_modeset_lock_all if you want too.
And this will be all a bit easier on top of my branch, since then you can
untangle fbdev locking from the kms side better.
-Daniel
> + if (ret)
> + break;
> }
> - out:
> - drm_modeset_unlock_all(dev);
> - return rc;
> +out:
> + if (ret == -EDEADLK) {
> + drm_modeset_backoff(&ctx);
> + goto retry;
> + }
> + drm_modeset_drop_locks(&ctx);
> + drm_modeset_acquire_fini(&ctx);
> +
> + return ret;
> }
> EXPORT_SYMBOL(drm_fb_helper_setcmap);
>
> --
> 2.1.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply
* Re: [PATCH v2 02/14] drm/fb-helper: remove drm_fb_helper_save_lut_atomic
From: Daniel Vetter @ 2017-06-26 9:18 UTC (permalink / raw)
To: Peter Rosin
Cc: Boris Brezillon, Philippe Cornu, Russell King, nouveau, intel-gfx,
linux-kernel, amd-gfx, virtualization, Yannick Fertre, dri-devel,
Dave Airlie, Alex Deucher, Daniel Vetter, Vincent Abriou,
Christian König, Ben Skeggs
In-Reply-To: <1498111597-10714-3-git-send-email-peda@axentia.se>
On Thu, Jun 22, 2017 at 08:06:25AM +0200, Peter Rosin wrote:
> drm_fb_helper_save_lut_atomic is redundant since the .gamma_store is
> now always kept up to date by drm_fb_helper_setcmap.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
Also note that this is for kgdb support only and so likely very buggy
(since no one cried when we started to break kgdb support when switching
drivers to atomic).
-Daniel
> ---
> drivers/gpu/drm/drm_fb_helper.c | 17 -----------------
> 1 file changed, 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 25315fb..7ade384 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -229,22 +229,6 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
> }
> EXPORT_SYMBOL(drm_fb_helper_remove_one_connector);
>
> -static void drm_fb_helper_save_lut_atomic(struct drm_crtc *crtc, struct drm_fb_helper *helper)
> -{
> - uint16_t *r_base, *g_base, *b_base;
> - int i;
> -
> - if (helper->funcs->gamma_get == NULL)
> - return;
> -
> - r_base = crtc->gamma_store;
> - g_base = r_base + crtc->gamma_size;
> - b_base = g_base + crtc->gamma_size;
> -
> - for (i = 0; i < crtc->gamma_size; i++)
> - helper->funcs->gamma_get(crtc, &r_base[i], &g_base[i], &b_base[i], i);
> -}
> -
> static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc)
> {
> uint16_t *r_base, *g_base, *b_base;
> @@ -285,7 +269,6 @@ int drm_fb_helper_debug_enter(struct fb_info *info)
> if (drm_drv_uses_atomic_modeset(mode_set->crtc->dev))
> continue;
>
> - drm_fb_helper_save_lut_atomic(mode_set->crtc, helper);
> funcs->mode_set_base_atomic(mode_set->crtc,
> mode_set->fb,
> mode_set->x,
> --
> 2.1.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply
* Re: BUG: KASAN: use-after-free in free_old_xmit_skbs
From: Jean-Philippe Menil @ 2017-06-26 7:35 UTC (permalink / raw)
To: Jason Wang, Cong Wang
Cc: Linux Kernel Network Developers, virtualization, John Fastabend,
qemu-devel Developers, Michael S. Tsirkin
In-Reply-To: <9327ebf9-59db-f34e-b945-ef381ee95315@redhat.com>
On 06/26/2017 04:50 AM, Jason Wang wrote:
>
>
> On 2017年06月24日 06:32, Cong Wang wrote:
>> On Fri, Jun 23, 2017 at 1:43 AM, Jason Wang <jasowang@redhat.com> wrote:
>>>
>>> On 2017年06月23日 02:53, Michael S. Tsirkin wrote:
>>>> On Thu, Jun 22, 2017 at 08:15:58AM +0200, jean-philippe menil wrote:
>>>>> Hi Michael,
>>>>>
>>>>> from what i see, the race appear when we hit virtnet_reset in
>>>>> virtnet_xdp_set.
>>>>> virtnet_reset
>>>>> _remove_vq_common
>>>>> virtnet_del_vqs
>>>>> virtnet_free_queues
>>>>> kfree(vi->sq)
>>>>> when the xdp program (with two instances of the program to trigger it
>>>>> faster)
>>>>> is added or removed.
>>>>>
>>>>> It's easily repeatable, with 2 cpus and 4 queues on the qemu command
>>>>> line,
>>>>> running the xdp_ttl tool from Jesper.
>>>>>
>>>>> For now, i'm able to continue my qualification, testing if xdp_qp
>>>>> is not
>>>>> null,
>>>>> but do not seem to be a sustainable trick.
>>>>> if (xdp_qp && vi->xdp_queues_pairs != xdp_qp)
>>>>>
>>>>> Maybe it will be more clear to you with theses informations.
>>>>>
>>>>> Best regards.
>>>>>
>>>>> Jean-Philippe
>>>>
>>>> I'm pretty clear about the issue here, I was trying to figure out a
>>>> fix.
>>>> Jason, any thoughts?
>>>>
>>>>
>>> Hi Jean:
>>>
>>> Does the following fix this issue? (I can't reproduce it locally through
>>> xdp_ttl)
>> It is tricky here.
>>
>> From my understanding of the code base, the tx_lock is not sufficient
>> here, because in virtnet_del_vqs() all vqs are deleted and one vp
>> maps to one txq.
>>
>> I am afraid you have to add a spinlock somewhere to serialized
>> free_old_xmit_skbs() vs. vring_del_virtqueue(). As you can see
>> they are in different layers, so it is hard to figure out where to add
>> it...
>>
>> Also, make sure we don't sleep inside the spinlock, I see a
>> synchronize_net().
>
> Looks like I miss something. I thought free_old_xmit_skbs() were
> serialized in this case since we disable all tx queues after
> netif_tx_unlock_bh()?
>
> Jean:
>
> I thought this could be easily reproduced by e.g produce some traffic
> and in the same time try to attach an xdp program. But looks not. How do
> you trigger this? What's your qemu command line for this?
>
> Thanks
Hi Jason,
this is how i trigger the bug:
- on the guest, tcpdump on on the interface
- on the guest, run iperf against the host
- on the guest, cat /sys/kernel/debug/tracing/trace_pipe
- on the guest, run one or two instances of xdp_ttl compiled with DEBUG
uncommented, that i start stop, until i trigger the bug.
qemu command line is as follow:
qemu-system-x86_64 -name ubuntu --enable-kvm -machine pc,accel=kvm -smp
2 -drive file=/dev/LocalDisk/ubuntu,if=virtio,format=raw -m 2048 -rtc
base=localtime,clock=host -usbdevice tablet --balloon virtio -netdev
tap,id=ubuntu-0,ifname=ubuntu-0,script=/home/jenfi/WORK/jp/qemu/if-up,downscript=/home/jenfi/WORK/jp/qemu/if-down,vhost=on,queues=4
-device
virtio-net-pci,netdev=ubuntu-0,mac=de:ad:be:ef:01:03,mq=on,guest_tso4=off,guest_tso6=off,guest_ecn=off,guest_ufo=off,vectors=2
-vnc 127.0.0.1:3 -nographic -serial
file:/home/jenfi/WORK/jp/qemu/ubuntu.out -monitor
unix:/home/jenfi/WORK/jp/qemu/ubuntu.sock,server,nowait
Notice, the smp 2, queues to 4 and vectors to 2.
Seem that if fogot to mention that in the beginning of this thread,
sorry for that.
Best regards.
Jean-Philippe
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: BUG: KASAN: use-after-free in free_old_xmit_skbs
From: Jason Wang @ 2017-06-26 2:50 UTC (permalink / raw)
To: Cong Wang
Cc: Michael S. Tsirkin, Linux Kernel Network Developers,
John Fastabend, qemu-devel Developers, virtualization,
jean-philippe menil
In-Reply-To: <CAM_iQpXhW___-iP3kdv9MJcW=hroyp2SuMzAZdx3KCpc9-W76g@mail.gmail.com>
On 2017年06月24日 06:32, Cong Wang wrote:
> On Fri, Jun 23, 2017 at 1:43 AM, Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2017年06月23日 02:53, Michael S. Tsirkin wrote:
>>> On Thu, Jun 22, 2017 at 08:15:58AM +0200, jean-philippe menil wrote:
>>>> Hi Michael,
>>>>
>>>> from what i see, the race appear when we hit virtnet_reset in
>>>> virtnet_xdp_set.
>>>> virtnet_reset
>>>> _remove_vq_common
>>>> virtnet_del_vqs
>>>> virtnet_free_queues
>>>> kfree(vi->sq)
>>>> when the xdp program (with two instances of the program to trigger it
>>>> faster)
>>>> is added or removed.
>>>>
>>>> It's easily repeatable, with 2 cpus and 4 queues on the qemu command
>>>> line,
>>>> running the xdp_ttl tool from Jesper.
>>>>
>>>> For now, i'm able to continue my qualification, testing if xdp_qp is not
>>>> null,
>>>> but do not seem to be a sustainable trick.
>>>> if (xdp_qp && vi->xdp_queues_pairs != xdp_qp)
>>>>
>>>> Maybe it will be more clear to you with theses informations.
>>>>
>>>> Best regards.
>>>>
>>>> Jean-Philippe
>>>
>>> I'm pretty clear about the issue here, I was trying to figure out a fix.
>>> Jason, any thoughts?
>>>
>>>
>> Hi Jean:
>>
>> Does the following fix this issue? (I can't reproduce it locally through
>> xdp_ttl)
> It is tricky here.
>
> From my understanding of the code base, the tx_lock is not sufficient
> here, because in virtnet_del_vqs() all vqs are deleted and one vp
> maps to one txq.
>
> I am afraid you have to add a spinlock somewhere to serialized
> free_old_xmit_skbs() vs. vring_del_virtqueue(). As you can see
> they are in different layers, so it is hard to figure out where to add
> it...
>
> Also, make sure we don't sleep inside the spinlock, I see a
> synchronize_net().
Looks like I miss something. I thought free_old_xmit_skbs() were
serialized in this case since we disable all tx queues after
netif_tx_unlock_bh()?
Jean:
I thought this could be easily reproduced by e.g produce some traffic
and in the same time try to attach an xdp program. But looks not. How do
you trigger this? What's your qemu command line for this?
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: BUG: KASAN: use-after-free in free_old_xmit_skbs
From: Cong Wang @ 2017-06-23 22:32 UTC (permalink / raw)
To: Jason Wang
Cc: Michael S. Tsirkin, Linux Kernel Network Developers,
John Fastabend, qemu-devel Developers, virtualization,
jean-philippe menil
In-Reply-To: <4f6d4342-8476-882e-e7a8-b3f0ebd20d95@redhat.com>
On Fri, Jun 23, 2017 at 1:43 AM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2017年06月23日 02:53, Michael S. Tsirkin wrote:
>>
>> On Thu, Jun 22, 2017 at 08:15:58AM +0200, jean-philippe menil wrote:
>>>
>>> Hi Michael,
>>>
>>> from what i see, the race appear when we hit virtnet_reset in
>>> virtnet_xdp_set.
>>> virtnet_reset
>>> _remove_vq_common
>>> virtnet_del_vqs
>>> virtnet_free_queues
>>> kfree(vi->sq)
>>> when the xdp program (with two instances of the program to trigger it
>>> faster)
>>> is added or removed.
>>>
>>> It's easily repeatable, with 2 cpus and 4 queues on the qemu command
>>> line,
>>> running the xdp_ttl tool from Jesper.
>>>
>>> For now, i'm able to continue my qualification, testing if xdp_qp is not
>>> null,
>>> but do not seem to be a sustainable trick.
>>> if (xdp_qp && vi->xdp_queues_pairs != xdp_qp)
>>>
>>> Maybe it will be more clear to you with theses informations.
>>>
>>> Best regards.
>>>
>>> Jean-Philippe
>>
>>
>> I'm pretty clear about the issue here, I was trying to figure out a fix.
>> Jason, any thoughts?
>>
>>
>
> Hi Jean:
>
> Does the following fix this issue? (I can't reproduce it locally through
> xdp_ttl)
It is tricky here.
From my understanding of the code base, the tx_lock is not sufficient
here, because in virtnet_del_vqs() all vqs are deleted and one vp
maps to one txq.
I am afraid you have to add a spinlock somewhere to serialized
free_old_xmit_skbs() vs. vring_del_virtqueue(). As you can see
they are in different layers, so it is hard to figure out where to add
it...
Also, make sure we don't sleep inside the spinlock, I see a
synchronize_net().
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH] x86: Remove unnecessary return from void function
From: Anton Vasilyev @ 2017-06-23 16:23 UTC (permalink / raw)
To: Jeremy Fitzhardinge
Cc: ldv-project, x86, Rusty Russell, Alok Kataria, linux-kernel,
virtualization, Chris Wright, Ingo Molnar, Anton Vasilyev,
Thomas Gleixner
The patch removes unnecessary return from void function.
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Anton Vasilyev <vasilyev@ispras.ru>
---
arch/x86/include/asm/paravirt.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index 55fa56f..a3dcf89 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -118,7 +118,7 @@ static inline u64 paravirt_read_msr(unsigned msr)
static inline void paravirt_write_msr(unsigned msr,
unsigned low, unsigned high)
{
- return PVOP_VCALL3(pv_cpu_ops.write_msr, msr, low, high);
+ PVOP_VCALL3(pv_cpu_ops.write_msr, msr, low, high);
}
static inline u64 paravirt_read_msr_safe(unsigned msr, int *err)
--
2.7.4
^ permalink raw reply related
* Re: [RFC] virtio-mem: paravirtualized memory
From: Stefan Hajnoczi @ 2017-06-23 12:45 UTC (permalink / raw)
To: David Hildenbrand
Cc: Andrea Arcangeli, KVM, Michael S. Tsirkin, qemu-devel@nongnu.org,
virtualization@lists.linux-foundation.org, linux-mm@kvack.org
In-Reply-To: <2361e86b-6660-4261-a805-c82c3b3a37c6@redhat.com>
[-- Attachment #1.1: Type: text/plain, Size: 3688 bytes --]
On Wed, Jun 21, 2017 at 02:32:48PM +0200, David Hildenbrand wrote:
> On 21.06.2017 13:08, Stefan Hajnoczi wrote:
> > On Mon, Jun 19, 2017 at 12:26:52PM +0200, David Hildenbrand wrote:
> >> On 19.06.2017 12:08, Stefan Hajnoczi wrote:
> >>> On Fri, Jun 16, 2017 at 04:20:02PM +0200, David Hildenbrand wrote:
> >>>> Important restrictions of this concept:
> >>>> - Guests without a virtio-mem guest driver can't see that memory.
> >>>> - We will always require some boot memory that cannot get unplugged.
> >>>> Also, virtio-mem memory (as all other hotplugged memory) cannot become
> >>>> DMA memory under Linux. So the boot memory also defines the amount of
> >>>> DMA memory.
> >>>
> >>> I didn't know that hotplug memory cannot become DMA memory.
> >>>
> >>> Ouch. Zero-copy disk I/O with O_DIRECT and network I/O with virtio-net
> >>> won't be possible.
> >>>
> >>> When running an application that uses O_DIRECT file I/O this probably
> >>> means we now have 2 copies of pages in memory: 1. in the application and
> >>> 2. in the kernel page cache.
> >>>
> >>> So this increases pressure on the page cache and reduces performance :(.
> >>>
> >>> Stefan
> >>>
> >>
> >> arch/x86/mm/init_64.c:
> >>
> >> /*
> >> * Memory is added always to NORMAL zone. This means you will never get
> >> * additional DMA/DMA32 memory.
> >> */
> >> int arch_add_memory(int nid, u64 start, u64 size, bool for_device)
> >> {
> >>
> >> The is for sure something to work on in the future. Until then, base
> >> memory of 3.X GB should be sufficient, right?
> >
> > I'm not sure that helps because applications typically don't control
> > where their buffers are located?
>
> Okay, let me try to explain what is going on here (no expert, please
> someone correct me if I am wrong).
>
> There is a difference between DMA and DMA memory in Linux. DMA memory is
> simply memory with special addresses. DMA is the general technique of a
> device directly copying data to ram, bypassing the CPU.
>
> ZONE_DMA contains all* memory < 16MB
> ZONE_DMA32 contains all* memory < 4G
> * meaning available on boot via a820 map, not hotplugged.
>
> So memory from these zones can be used by devices that can only deal
> with 24bit/32bit addresses.
>
> Hotplugged memory is never added to the ZONE_DMA/DMA32, but to
> ZONE_NORMAL. That means, kmalloc(.., GFP_DMA will) not be able to use
> hotplugged memory. Say you have 1GB of main storage and hotplug 1G (on
> address 1G). This memory will not be available in the ZONE_DMA, although
> below 4g.
>
> Memory in ZONE_NORMAL is used for ordinary kmalloc(), so all these
> memory can be used to do DMA, but you are not guaranteed to get 32bit
> capable addresses. I pretty much assume that virtio-net can deal with
> 64bit addresses.
>
>
> My understanding of O_DIRECT:
>
> The user space buffers (O_DIRECT) is directly used to do DMA. This will
> work just fine as long as the device can deal with 64bit addresses. I
> guess this is the case for virtio-net, otherwise there would be the
> exact same problem already without virtio-mem.
>
> Summary:
>
> virtio-mem memory can be used for DMA, it will simply not be added to
> ZONE_DMA/DMA32 and therefore won't be available for kmalloc(...,
> GFP_DMA). This should work just fine with O_DIRECT as before.
>
> If necessary, we could try to add memory to the ZONE_DMA later on,
> however for now I would rate this a minor problem. By simply using 3.X
> GB of base memory, basically all memory that could go to ZONE_DMA/DMA32
> already is in these zones without virtio-mem.
Nice, thanks for clearing this up!
Stefan
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [Intel-gfx] [PATCH v2 13/14] drm: stm: remove dead code and pointless local lut storage
From: Daniel Vetter @ 2017-06-23 9:35 UTC (permalink / raw)
To: Philippe CORNU
Cc: Boris Brezillon, Christian König, David Airlie,
nouveau@lists.freedesktop.org, intel-gfx@lists.freedesktop.org,
linux-kernel@vger.kernel.org, amd-gfx@lists.freedesktop.org,
Russell King, Yannick FERTRE, Ben Skeggs,
dri-devel@lists.freedesktop.org, Benjamin Gaignard, Dave Airlie,
Alex Deucher, Daniel Vetter,
virtualization@lists.linux-foundation.org, Vincent ABRIOU
In-Reply-To: <159dad37-7f12-0b4d-5512-d19a80670d5c@st.com>
On Thu, Jun 22, 2017 at 11:49:34AM +0000, Philippe CORNU wrote:
>
>
> On 06/22/2017 08:06 AM, Peter Rosin wrote:
> > The redundant fb helper .load_lut is no longer used, and can not
> > work right without also providing the fb helpers .gamma_set and
> > .gamma_get thus rendering the code in this driver suspect.
> >
>
> Hi Peter,
> STM32 chipsets supports 8-bit CLUT mode but this driver version does not
> support it "yet" (final patch has not been upstreamed because it was a
> too big fbdev patch for simply adding CLUT...).
>
> Regarding your patch below, if it helps you to ease the drm framework
> update then I am agree to "acknowledge it" asap, else if you are not in
> a hurry, I would prefer a better and definitive patch handling 8-bit
> CLUT properly and I am ok to help or/and to do it : )
I'll take your ack, since your 8bit lut support will be massively simpler
with Peter's series here :-)
> Extra questions:
> - any plan to update modetest with the DRM_FORMAT_C8 support + gamma
> get/set?
We have gamma igts, well for the fancy new atomic color manager stuff.
Testing drivers with modetest is kinda not that awesome :-)
-Daniel
> - do you have a simple way to test clut with fbdev, last year we where
> using an old version of the SDL but I am still looking for a small piece
> of code to do it (else I will do it myself but C8 on fbdev is not really
> a priority ;-)
>
> best regards,
> Philippe
>
> > Just remove the dead code.
> >
> > Signed-off-by: Peter Rosin <peda@axentia.se>
> > ---
> > drivers/gpu/drm/stm/ltdc.c | 12 ------------
> > drivers/gpu/drm/stm/ltdc.h | 1 -
> > 2 files changed, 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
> > index 1b9483d..87829b9 100644
> > --- a/drivers/gpu/drm/stm/ltdc.c
> > +++ b/drivers/gpu/drm/stm/ltdc.c
> > @@ -375,17 +375,6 @@ static irqreturn_t ltdc_irq(int irq, void *arg)
> > * DRM_CRTC
> > */
> >
> > -static void ltdc_crtc_load_lut(struct drm_crtc *crtc)
> > -{
> > - struct ltdc_device *ldev = crtc_to_ltdc(crtc);
> > - unsigned int i, lay;
> > -
> > - for (lay = 0; lay < ldev->caps.nb_layers; lay++)
> > - for (i = 0; i < 256; i++)
> > - reg_write(ldev->regs, LTDC_L1CLUTWR + lay * LAY_OFS,
> > - ldev->clut[i]);
> > -}
> > -
> > static void ltdc_crtc_enable(struct drm_crtc *crtc)
> > {
> > struct ltdc_device *ldev = crtc_to_ltdc(crtc);
> > @@ -523,7 +512,6 @@ static void ltdc_crtc_atomic_flush(struct drm_crtc *crtc,
> > }
> >
> > static struct drm_crtc_helper_funcs ltdc_crtc_helper_funcs = {
> > - .load_lut = ltdc_crtc_load_lut,
> > .enable = ltdc_crtc_enable,
> > .disable = ltdc_crtc_disable,
> > .mode_set_nofb = ltdc_crtc_mode_set_nofb,
> > diff --git a/drivers/gpu/drm/stm/ltdc.h b/drivers/gpu/drm/stm/ltdc.h
> > index d7a9c73..620ca55 100644
> > --- a/drivers/gpu/drm/stm/ltdc.h
> > +++ b/drivers/gpu/drm/stm/ltdc.h
> > @@ -27,7 +27,6 @@ struct ltdc_device {
> > struct drm_panel *panel;
> > struct mutex err_lock; /* protecting error_status */
> > struct ltdc_caps caps;
> > - u32 clut[256]; /* color look up table */
> > u32 error_status;
> > u32 irq_status;
> > };
> >
> _______________________________________________
> 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
^ permalink raw reply
* Re: BUG: KASAN: use-after-free in free_old_xmit_skbs
From: Jean-Philippe Menil @ 2017-06-23 9:33 UTC (permalink / raw)
To: Jason Wang
Cc: netdev, virtualization, John Fastabend, qemu-devel,
Michael S. Tsirkin
In-Reply-To: <4f6d4342-8476-882e-e7a8-b3f0ebd20d95@redhat.com>
On 06/23/2017 10:43 AM, Jason Wang wrote:
>
>
> On 2017年06月23日 02:53, Michael S. Tsirkin wrote:
>> On Thu, Jun 22, 2017 at 08:15:58AM +0200, jean-philippe menil wrote:
>>> 2017-06-06 1:52 GMT+02:00 Michael S. Tsirkin <mst@redhat.com>:
>>>
>>> On Mon, Jun 05, 2017 at 05:08:25AM +0300, Michael S. Tsirkin wrote:
>>> > On Mon, Jun 05, 2017 at 12:48:53AM +0200, Jean-Philippe Menil
>>> wrote:
>>> > > Hi,
>>> > >
>>> > > while playing with xdp and ebpf, i'm hitting the following:
>>> > >
>>> > > [ 309.993136]
>>> > >
>>> ==================================================================
>>> > > [ 309.994735] BUG: KASAN: use-after-free in
>>> > > free_old_xmit_skbs.isra.29+0x2b7/0x2e0 [virtio_net]
>>> > > [ 309.998396] Read of size 8 at addr ffff88006aa64220 by
>>> task sshd/323
>>> > > [ 310.000650]
>>> > > [ 310.002305] CPU: 1 PID: 323 Comm: sshd Not tainted
>>> 4.12.0-rc3+ #2
>>> > > [ 310.004018] Hardware name: QEMU Standard PC (i440FX +
>>> PIIX, 1996),
>>> BIOS
>>> > > 1.10.2-20170228_101828-anatol 04/01/2014
>> ...
>>
>>> >
>>> > Since commit 680557cf79f82623e2c4fd42733077d60a843513
>>> > virtio_net: rework mergeable buffer handling
>>> >
>>> > we no longer must do the resets, we now have enough space
>>> > to store a bit saying whether a buffer is xdp one or not.
>>> >
>>> > And that's probably a cleaner way to fix these issues than
>>> > try to find and fix the race condition.
>>> >
>>> > John?
>>> >
>>> > --
>>> > MST
>>>
>>>
>>> I think I see the source of the race. virtio net calls
>>> netif_device_detach and assumes no packets will be sent after
>>> this point. However, all it does is stop all queues so
>>> no new packets will be transmitted.
>>>
>>> Try locking with HARD_TX_LOCK?
>>>
>>> --
>>> MST
>>>
>>>
>>> Hi Michael,
>>>
>>> from what i see, the race appear when we hit virtnet_reset in
>>> virtnet_xdp_set.
>>> virtnet_reset
>>> _remove_vq_common
>>> virtnet_del_vqs
>>> virtnet_free_queues
>>> kfree(vi->sq)
>>> when the xdp program (with two instances of the program to trigger it
>>> faster)
>>> is added or removed.
>>>
>>> It's easily repeatable, with 2 cpus and 4 queues on the qemu command
>>> line,
>>> running the xdp_ttl tool from Jesper.
>>>
>>> For now, i'm able to continue my qualification, testing if xdp_qp is
>>> not null,
>>> but do not seem to be a sustainable trick.
>>> if (xdp_qp && vi->xdp_queues_pairs != xdp_qp)
>>>
>>> Maybe it will be more clear to you with theses informations.
>>>
>>> Best regards.
>>>
>>> Jean-Philippe
>>
>> I'm pretty clear about the issue here, I was trying to figure out a fix.
>> Jason, any thoughts?
>>
>>
>
> Hi Jean:
>
> Does the following fix this issue? (I can't reproduce it locally through
> xdp_ttl)
>
> Thanks
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 1f8c15c..3e65c3f 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1801,7 +1801,9 @@ static void virtnet_freeze_down(struct
> virtio_device *vdev)
> /* Make sure no work handler is accessing the device */
> flush_work(&vi->config_work);
>
> + netif_tx_lock_bh(vi->dev);
> netif_device_detach(vi->dev);
> + netif_tx_unlock_bh(vi->dev);
> cancel_delayed_work_sync(&vi->refill);
>
Hi Jason,
unfortunately, same crash on same place, the lock did not help.
[ 574.522886]
==================================================================
[ 574.527393] BUG: KASAN: use-after-free in
free_old_xmit_skbs.isra.28+0x29b/0x2b0 [virtio_net]
[ 574.531934] Read of size 8 at addr ffff88005d220020 by task iperf/2252
[ 574.536296]
[ 574.539729] CPU: 1 PID: 2252 Comm: iperf Not tainted 4.12.0-rc5+ #5
[ 574.543916] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS 1.10.2-20170228_101828-anatol 04/01/2014
[ 574.552046] Call Trace:
[ 574.555648] dump_stack+0xb3/0x10b
[ 574.559471] ? free_old_xmit_skbs.isra.28+0x29b/0x2b0 [virtio_net]
[ 574.563578] print_address_description+0x6a/0x280
[ 574.567253] ? free_old_xmit_skbs.isra.28+0x29b/0x2b0 [virtio_net]
[ 574.571223] kasan_report+0x22b/0x340
[ 574.574698] __asan_report_load8_noabort+0x14/0x20
[ 574.578490] free_old_xmit_skbs.isra.28+0x29b/0x2b0 [virtio_net]
[ 574.582586] ? dev_queue_xmit_nit+0x5fb/0x850
[ 574.586348] ? virtnet_del_vqs+0xf0/0xf0 [virtio_net]
[ 574.590153] ? __skb_clone+0x24a/0x7d0
[ 574.593835] start_xmit+0x15a/0x1620 [virtio_net]
[ 574.597939] dev_hard_start_xmit+0x17f/0x7e0
[ 574.601832] sch_direct_xmit+0x2a8/0x5d0
[ 574.605665] ? dev_deactivate_queue.constprop.31+0x150/0x150
[ 574.609827] __dev_queue_xmit+0x1124/0x18b0
[ 574.613595] ? selinux_ip_postroute+0x4b2/0xa90
[ 574.617928] ? netdev_pick_tx+0x2d0/0x2d0
[ 574.621852] ? mark_held_locks+0xc8/0x120
[ 574.625673] ? ip_finish_output+0x626/0x9b0
[ 574.631679] ? ip_finish_output2+0xb44/0x1160
[ 574.637642] dev_queue_xmit+0x17/0x20
[ 574.641693] ip_finish_output2+0xcd1/0x1160
[ 574.645621] ? do_add_counters+0x480/0x480
[ 574.649554] ? do_add_counters+0x403/0x480
[ 574.653209] ? ip_copy_metadata+0x630/0x630
[ 574.657066] ip_finish_output+0x626/0x9b0
[ 574.660482] ? ip_finish_output+0x626/0x9b0
[ 574.663905] ip_output+0x1e2/0x580
[ 574.667235] ? ip_mc_output+0xe80/0xe80
[ 574.670574] ? ip_fragment.constprop.57+0x200/0x200
[ 574.673949] ip_local_out+0x95/0x160
[ 574.677249] ? __sk_dst_check+0xa7/0x260
[ 574.680446] ip_queue_xmit+0x889/0x17f0
[ 574.683575] ? __tcp_v4_send_check+0x1b8/0x350
[ 574.686801] tcp_transmit_skb+0x194a/0x2db0
[ 574.689832] ? __tcp_select_window+0x500/0x500
[ 574.693310] ? sched_clock_cpu+0x1b/0x190
[ 574.696371] ? tcp_grow_window.isra.24+0x2a8/0x4b0
[ 574.699509] tcp_send_ack+0x46f/0x710
[ 574.702395] __tcp_ack_snd_check+0x233/0x380
[ 574.705365] tcp_rcv_established+0x14eb/0x2230
[ 574.708332] ? trace_hardirqs_on_caller+0x3f4/0x560
[ 574.711800] ? tcp_data_queue+0x3e70/0x3e70
[ 574.714761] ? sk_wait_data+0x2af/0x400
[ 574.719220] tcp_v4_do_rcv+0x56c/0x820
[ 574.724018] tcp_prequeue_process+0x18f/0x2c0
[ 574.729062] tcp_recvmsg+0xff6/0x26a0
[ 574.734615] ? tcp_tx_timestamp.part.27+0x290/0x290
[ 574.739519] ? _copy_from_user+0x84/0xe0
[ 574.744115] ? rw_copy_check_uvector+0x1f6/0x290
[ 574.748722] ? sock_has_perm+0x1e4/0x270
[ 574.751537] ? selinux_tun_dev_create+0xc0/0xc0
[ 574.754068] inet_recvmsg+0x117/0x530
[ 574.756823] ? memzero_page+0x130/0x130
[ 574.759503] ? inet_sk_rebuild_header+0x1880/0x1880
[ 574.762303] ? selinux_socket_recvmsg+0x36/0x40
[ 574.765114] ? security_socket_recvmsg+0x8f/0xc0
[ 574.768156] ? inet_sk_rebuild_header+0x1880/0x1880
[ 574.771181] sock_recvmsg+0xd7/0x110
[ 574.773972] ? __sock_recv_wifi_status+0x180/0x180
[ 574.777002] ___sys_recvmsg+0x24d/0x560
[ 574.779789] ? ___sys_sendmsg+0x920/0x920
[ 574.782734] ? __fget+0x200/0x380
[ 574.785657] ? lock_downgrade+0x650/0x650
[ 574.788584] ? __fget+0x229/0x380
[ 574.791362] ? __fget_light+0xa1/0x1f0
[ 574.794162] ? __fdget+0x18/0x20
[ 574.796832] __sys_recvmsg+0xce/0x170
[ 574.799572] ? __sys_recvmsg+0xce/0x170
[ 574.802695] ? SyS_sendmmsg+0x60/0x60
[ 574.805461] ? __schedule+0x7cb/0x1a70
[ 574.808211] ? retint_kernel+0x10/0x10
[ 574.810922] ? trace_hardirqs_on_caller+0x3f4/0x560
[ 574.813890] ? trace_hardirqs_on_thunk+0x1a/0x1c
[ 574.816783] ? trace_hardirqs_on_caller+0x3f4/0x560
[ 574.819693] SyS_recvmsg+0x2d/0x50
[ 574.822829] entry_SYSCALL_64_fastpath+0x1f/0xbe
[ 574.825697] RIP: 0033:0x7f7fbd77e3c0
[ 574.828366] RSP: 002b:00007f7fba39ed50 EFLAGS: 00000293 ORIG_RAX:
000000000000002f
[ 574.833588] RAX: ffffffffffffffda RBX: 0000000000000008 RCX:
00007f7fbd77e3c0
[ 574.838882] RDX: 0000000000000000 RSI: 00007f7fba39edb0 RDI:
0000000000000008
[ 574.844377] RBP: 0000000000000046 R08: 0000000000000000 R09:
000000a0ff7159c1
[ 574.849937] R10: 00143f7b62d9620b R11: 0000000000000293 R12:
0000000000000000
[ 574.855391] R13: 0000000004000000 R14: 00007f7fa0000b10 R15:
0000000000000001
[ 574.860146]
[ 574.862738] Allocated by task 2291:
[ 574.865528] save_stack_trace+0x16/0x20
[ 574.868370] save_stack+0x46/0xd0
[ 574.871096] kasan_kmalloc+0xad/0xe0
[ 574.873838] __kmalloc+0x115/0x2d0
[ 574.876524] __vring_new_virtqueue+0x6a/0x790
[ 574.879432] vring_create_virtqueue+0x203/0x380
[ 574.882367] setup_vq+0x159/0x660
[ 574.885115] vp_setup_vq+0xbe/0x390
[ 574.887802] vp_find_vqs_msix+0x568/0xb90
[ 574.890494] vp_find_vqs+0x93/0x460
[ 574.893175] vp_modern_find_vqs+0x44/0x170
[ 574.895932] init_vqs+0x8eb/0x1150 [virtio_net]
[ 574.898778] virtnet_restore_up+0x4c/0x5c0 [virtio_net]
[ 574.901889] virtnet_xdp+0x820/0xd00 [virtio_net]
[ 574.904858] dev_change_xdp_fd+0x1bb/0x340
[ 574.907708] do_setlink+0x23fb/0x2c00
[ 574.910491] rtnl_setlink+0x280/0x340
[ 574.913448] rtnetlink_rcv_msg+0x288/0x680
[ 574.916348] netlink_rcv_skb+0x340/0x470
[ 574.919165] rtnetlink_rcv+0x2a/0x40
[ 574.922027] netlink_unicast+0x58d/0x860
[ 574.924897] netlink_sendmsg+0x8d2/0xca0
[ 574.927815] sock_sendmsg+0xca/0x110
[ 574.930708] SYSC_sendto+0x20d/0x340
[ 574.933562] SyS_sendto+0x40/0x50
[ 574.936380] entry_SYSCALL_64_fastpath+0x1f/0xbe
[ 574.939322]
[ 574.941724] Freed by task 2291:
[ 574.944389] save_stack_trace+0x16/0x20
[ 574.947107] save_stack+0x46/0xd0
[ 574.949893] kasan_slab_free+0x72/0xc0
[ 574.952526] kfree+0xe6/0x2c0
[ 574.955082] vring_del_virtqueue+0xef/0x220
[ 574.957773] del_vq+0x126/0x270
[ 574.960283] vp_del_vqs+0x1f5/0xa30
[ 574.962743] virtnet_del_vqs+0xb7/0xf0 [virtio_net]
[ 574.965930] virtnet_xdp+0x7b8/0xd00 [virtio_net]
[ 574.968762] dev_change_xdp_fd+0x309/0x340
[ 574.971487] do_setlink+0x23fb/0x2c00
[ 574.974041] rtnl_setlink+0x280/0x340
[ 574.976727] rtnetlink_rcv_msg+0x288/0x680
[ 574.979366] netlink_rcv_skb+0x340/0x470
[ 574.981949] rtnetlink_rcv+0x2a/0x40
[ 574.984462] netlink_unicast+0x58d/0x860
[ 574.987151] netlink_sendmsg+0x8d2/0xca0
[ 574.989736] sock_sendmsg+0xca/0x110
[ 574.992351] SYSC_sendto+0x20d/0x340
[ 574.995262] SyS_sendto+0x40/0x50
[ 574.998959] entry_SYSCALL_64_fastpath+0x1f/0xbe
[ 575.001625]
[ 575.003976] The buggy address belongs to the object at ffff88005d220000
[ 575.003976] which belongs to the cache kmalloc-8192 of size 8192
[ 575.010183] The buggy address is located 32 bytes inside of
[ 575.010183] 8192-byte region [ffff88005d220000, ffff88005d222000)
[ 575.016265] The buggy address belongs to the page:
[ 575.019125] page:ffffea0001748800 count:1 mapcount:0 mapping:
(null) index:0x0 compound_mapcount: 0
[ 575.025320] flags: 0x100000000008100(slab|head)
[ 575.028167] raw: 0100000000008100 0000000000000000 0000000000000000
0000000100030003
[ 575.031632] raw: dead000000000100 dead000000000200 ffff88006c802280
0000000000000000
[ 575.035447] page dumped because: kasan: bad access detected
[ 575.039170]
[ 575.041893] Memory state around the buggy address:
[ 575.045408] ffff88005d21ff00: fc fc fc fc fc fc fc fc fc fc fc fc fc
fc fc fc
[ 575.051399] ffff88005d21ff80: fc fc fc fc fc fc fc fc fc fc fc fc fc
fc fc fc
[ 575.057558] >ffff88005d220000: fb fb fb fb fb fb fb fb fb fb fb fb fb
fb fb fb
[ 575.063760] ^
[ 575.069310] ffff88005d220080: fb fb fb fb fb fb fb fb fb fb fb fb fb
fb fb fb
[ 575.075359] ffff88005d220100: fb fb fb fb fb fb fb fb fb fb fb fb fb
fb fb fb
[ 575.081461]
==================================================================
[ 575.086914] Disabling lock debugging due to kernel taint
[ 575.090717] virtio_net virtio1: output.0:id 31 is not a head!
[ 575.096336] net enp0s4: Unexpected TXQ (0) queue failure: -5
[ 575.102000] net enp0s4: Unexpected TXQ (0) queue failure: -5
[ 575.107383] net enp0s4: Unexpected TXQ (0) queue failure: -5
[ 575.112785] net enp0s4: Unexpected TXQ (0) queue failure: -5
[ 575.118228] net enp0s4: Unexpected TXQ (0) queue failure: -5
(gdb) l *(free_old_xmit_skbs+0x29b)
0x20db is in free_old_xmit_skbs (drivers/net/virtio_net.c:1051).
1046
1047 static void free_old_xmit_skbs(struct send_queue *sq)
1048 {
1049 struct sk_buff *skb;
1050 unsigned int len;
1051 struct virtnet_info *vi = sq->vq->vdev->priv;
1052 struct virtnet_stats *stats = this_cpu_ptr(vi->stats);
1053 unsigned int packets = 0;
1054 unsigned int bytes = 0;
1055
Best regards,
Jean-Philippe
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: BUG: KASAN: use-after-free in free_old_xmit_skbs
From: Jason Wang @ 2017-06-23 8:43 UTC (permalink / raw)
To: Michael S. Tsirkin, jean-philippe menil
Cc: netdev, John Fastabend, qemu-devel, virtualization
In-Reply-To: <20170622215125-mutt-send-email-mst@kernel.org>
On 2017年06月23日 02:53, Michael S. Tsirkin wrote:
> On Thu, Jun 22, 2017 at 08:15:58AM +0200, jean-philippe menil wrote:
>> 2017-06-06 1:52 GMT+02:00 Michael S. Tsirkin <mst@redhat.com>:
>>
>> On Mon, Jun 05, 2017 at 05:08:25AM +0300, Michael S. Tsirkin wrote:
>> > On Mon, Jun 05, 2017 at 12:48:53AM +0200, Jean-Philippe Menil wrote:
>> > > Hi,
>> > >
>> > > while playing with xdp and ebpf, i'm hitting the following:
>> > >
>> > > [ 309.993136]
>> > > ==================================================================
>> > > [ 309.994735] BUG: KASAN: use-after-free in
>> > > free_old_xmit_skbs.isra.29+0x2b7/0x2e0 [virtio_net]
>> > > [ 309.998396] Read of size 8 at addr ffff88006aa64220 by task sshd/323
>> > > [ 310.000650]
>> > > [ 310.002305] CPU: 1 PID: 323 Comm: sshd Not tainted 4.12.0-rc3+ #2
>> > > [ 310.004018] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
>> BIOS
>> > > 1.10.2-20170228_101828-anatol 04/01/2014
> ...
>
>> >
>> > Since commit 680557cf79f82623e2c4fd42733077d60a843513
>> > virtio_net: rework mergeable buffer handling
>> >
>> > we no longer must do the resets, we now have enough space
>> > to store a bit saying whether a buffer is xdp one or not.
>> >
>> > And that's probably a cleaner way to fix these issues than
>> > try to find and fix the race condition.
>> >
>> > John?
>> >
>> > --
>> > MST
>>
>>
>> I think I see the source of the race. virtio net calls
>> netif_device_detach and assumes no packets will be sent after
>> this point. However, all it does is stop all queues so
>> no new packets will be transmitted.
>>
>> Try locking with HARD_TX_LOCK?
>>
>>
>> --
>> MST
>>
>>
>> Hi Michael,
>>
>> from what i see, the race appear when we hit virtnet_reset in virtnet_xdp_set.
>> virtnet_reset
>> _remove_vq_common
>> virtnet_del_vqs
>> virtnet_free_queues
>> kfree(vi->sq)
>> when the xdp program (with two instances of the program to trigger it faster)
>> is added or removed.
>>
>> It's easily repeatable, with 2 cpus and 4 queues on the qemu command line,
>> running the xdp_ttl tool from Jesper.
>>
>> For now, i'm able to continue my qualification, testing if xdp_qp is not null,
>> but do not seem to be a sustainable trick.
>> if (xdp_qp && vi->xdp_queues_pairs != xdp_qp)
>>
>> Maybe it will be more clear to you with theses informations.
>>
>> Best regards.
>>
>> Jean-Philippe
>
> I'm pretty clear about the issue here, I was trying to figure out a fix.
> Jason, any thoughts?
>
>
Hi Jean:
Does the following fix this issue? (I can't reproduce it locally through
xdp_ttl)
Thanks
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 1f8c15c..3e65c3f 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1801,7 +1801,9 @@ static void virtnet_freeze_down(struct
virtio_device *vdev)
/* Make sure no work handler is accessing the device */
flush_work(&vi->config_work);
+ netif_tx_lock_bh(vi->dev);
netif_device_detach(vi->dev);
+ netif_tx_unlock_bh(vi->dev);
cancel_delayed_work_sync(&vi->refill);
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* Re: [PATCH 01/11] drm/fb-helper: do a generic fb_setcmap helper in terms of crtc .gamma_set
From: Daniel Vetter @ 2017-06-23 8:40 UTC (permalink / raw)
To: Peter Rosin
Cc: nouveau, intel-gfx, linux-kernel, dri-devel, Philippe Cornu,
Yannick Fertre, Ben Skeggs, amd-gfx, Dave Airlie, Alex Deucher,
Daniel Vetter, virtualization, Vincent Abriou,
Christian König
In-Reply-To: <854a89af-d146-8414-73e7-2460c8f33598@axentia.se>
On Thu, Jun 22, 2017 at 10:48:10AM +0200, Peter Rosin wrote:
> On 2017-06-22 08:36, Daniel Vetter wrote:
> > On Wed, Jun 21, 2017 at 11:40:52AM +0200, Peter Rosin wrote:
> >> On 2017-06-21 09:38, Daniel Vetter wrote:
> >>> On Tue, Jun 20, 2017 at 09:25:25PM +0200, Peter Rosin wrote:
> >>>> This makes the redundant fb helpers .load_lut, .gamma_set and .gamma_get
> >>>> totally obsolete.
> >>>>
> >>>> I think the gamma_store can end up invalid on error. But the way I read
> >>>> it, that can happen in drm_mode_gamma_set_ioctl as well, so why should
> >>>> this pesky legacy fbdev stuff be any better?
> >>>>
> >>>> drm_fb_helper_save_lut_atomic justs saves the gamma lut for later. However,
> >>>> it saves it to the gamma_store which should already be up to date with what
> >>>> .gamma_get would return and is thus a nop. So, zap it.
> >>>
> >>> Removing drm_fb_helper_save_lut_atomic should be a separate patch I
> >>> think.
> >>
> >> Then 3 patches would be needed, first some hybrid thing that does it the
> >> old way, but also stores the lut in .gamma_store, then the split-out that
> >> removes drm_fb_helper_save_lut_atomic, then whatever is needed to get
> >> to the desired code. I can certainly do that, but do you want me to?
> >
> > Explain that in the commit message and it's fine.
>
> I did the split in v2, I assume that's ok too. Better in case anyone ever
> needs to run a bisect on this...
>
> >>> It's a pre-existing bug, but should we also try to restore the fbdev lut
> >>> in drm_fb_helper_restore_fbdev_mode_unlocked()? Would be yet another bug,
> >>> but might be relevant for your use-case. Just try to run both an fbdev
> >>> application and some kms-native thing, and then SIGKILL the native kms
> >>> app.
> >>>
> >>> But since pre-existing not really required, and probably too much effort.
> >>
> >> Good thing too, because I don't really know my way around this code...
> >
> > Btw I cc'ed you on one of my patches in the fbdev locking series, we might
> > need to do the same legacy vs. atomic split for the new lut code as I did
> > for dpms. The rule with atomic is that you can't do multiple commits under
> > drm_modeset_lock_all, you either have to do one overall atomic commit
> > (preferred) or drop&reacquire locks again. This matters for LUT since
> > you're updating the LUT on all CRTCs, which when using the gamma_set
> > atomic helper would be multiple commits :-/
>
> Ahh, ok, I see the problem.
>
> > Using the dpms patch as template it shouldn't be too hard to address that
> > for your patch here too.
>
> Hmm, in that patch you handle the legacy case in a separate function, and
> doing that for the lut case looks difficult when the atomic commit happens
> inside the helper (typically drm_atomic_helper_legacy_gamma_set which
> could perhaps be handled, but a real drag to handle for drivers that have
> a custom crtc .gamma_set).
Yeah, that's essentially why you need 2 functions. Legacy one calls the
legacy interfaces, the atomic one calls the new fancy atomic property
stuff directly (i.e. it open-codes the property setting dance from the
helper, but with one atomic commit across all crtc).
The reason that the legacy callback functions are helpers and not just
default behavior is that I expected some drivers to need special hacks to
keep their existing legacy kms userspace working. Atomic helpers unified
behaviour a lot that beforehand was slightly inconsistent. I somewhat
expect that long-term we'll unexport all the compat helpers and just make
them the default for all atomic drivers.
> So, I'm aiming for the drop&reacquire approach...
Hm, I prefer the open-coding, that's at least what we do everywhere else.
Has the benefit that it's more atomic (one commit for everything).
> However, I don't have all of that series, and I suspect that is why I do
> not have any fb_helper->lock.
Oh sorry, entire series is on dri-devel. I can do a git branch too if you
need one, atm it's in my general pile:
https://cgit.freedesktop.org/~danvet/drm/log/
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply
* Re: [PATCH v2 13/14] drm: stm: remove dead code and pointless local lut storage
From: Peter Rosin @ 2017-06-23 5:29 UTC (permalink / raw)
To: Philippe CORNU, linux-kernel@vger.kernel.org
Cc: Boris Brezillon, amd-gfx@lists.freedesktop.org,
intel-gfx@lists.freedesktop.org, David Airlie,
nouveau@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
Russell King, Jani Nikula, Patrik Jakobsson, Yannick FERTRE,
Sean Paul, Benjamin Gaignard, Daniel Vetter, Alex Deucher,
Dave Airlie, virtualization@lists.linux-foundation.org,
Vincent ABRIOU
In-Reply-To: <159dad37-7f12-0b4d-5512-d19a80670d5c@st.com>
On 2017-06-22 13:49, Philippe CORNU wrote:
> On 06/22/2017 08:06 AM, Peter Rosin wrote:
>> The redundant fb helper .load_lut is no longer used, and can not
>> work right without also providing the fb helpers .gamma_set and
>> .gamma_get thus rendering the code in this driver suspect.
>>
>
> Hi Peter,
> STM32 chipsets supports 8-bit CLUT mode but this driver version does not
> support it "yet" (final patch has not been upstreamed because it was a
> too big fbdev patch for simply adding CLUT...).
>
> Regarding your patch below, if it helps you to ease the drm framework
> update then I am agree to "acknowledge it" asap, else if you are not in
> a hurry, I would prefer a better and definitive patch handling 8-bit
> CLUT properly and I am ok to help or/and to do it : )
Hi!
The thing is, without my series you will have to provide four callbacks.
The crtc .gamma_set and the three redundant fb helpers .gamma_get,
.gamma_set and .load_lut that pretty much does exactly what the crtc
.gamma_set is doing. Well not .gamma_get, but...
With my series, you only have to provide the crtc .gamma_set, which you
have to provide anyway. and ...the core will handle everything that
.gamma_get was used for...
I.e., your work to provide CLUT support should start with drm support,
which means the crtc .gamma_set, and then move on to the fbdev
emulation. And I have just eliminated the second step for you, and
as suger on top, you no longer have to convince the core drm maintainers
that adding a lot of fbdev emulation code is needed.
So, I think you actually want to wait for my series to land before adding
CLUT support.
> Extra questions:
> - any plan to update modetest with the DRM_FORMAT_C8 support + gamma
> get/set?
I don't know that code base at all, but from the glimpse I got when browsing
it, it seemed like it was pretty hardwired to non-palettized modes. I ended
up having no need for it, see below...
> - do you have a simple way to test clut with fbdev, last year we where
> using an old version of the SDL but I am still looking for a small piece
> of code to do it (else I will do it myself but C8 on fbdev is not really
> a priority ;-)
I'm doing pretty much the same thing, I have an application that requires
an old SDL, and I'm using the programs/demos/demo.c program from the very
old libggi as a second test app. But that's just because libggi is what
I'm most familiar with, and it doesn't try to be "nice" and do things
automatically, instead you have to manually insert helpers providing
e.g. palette emulation if the application assumes a palettized mode and
only truecolor modes are available from the HW. SDL tends to add those
things for you, making it less easy to test thing, but I'm not an
"SDL-guy", so there may very well exist some knobs I don't know about.
Oh, you probably didn't see this:
http://marc.info/?l=linux-kernel&m=149786920731175&w=4
It sports modeset-pal.c that sets the C8 mode, and does a 5 second
palette animation, w/o using fbdev. I used it instead of digging
further into modetest.
Cheers,
peda
^ permalink raw reply
* [PATCH] char: virtio_console: constify port_attribute_group
From: Arvind Yadav @ 2017-06-23 4:56 UTC (permalink / raw)
To: arnd, gregkh, amit; +Cc: linux-kernel, virtualization
File size before:
text data bss dec hex filename
13775 656 88 14519 38b7 drivers/char/virtio_console.o
File size After adding 'const':
text data bss dec hex filename
13839 592 88 14519 38b7 drivers/char/virtio_console.o
Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
---
drivers/char/virtio_console.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index ad843eb..06bd635 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1308,7 +1308,7 @@ static ssize_t show_port_name(struct device *dev,
NULL
};
-static struct attribute_group port_attribute_group = {
+static const struct attribute_group port_attribute_group = {
.name = NULL, /* put in device directory */
.attrs = port_sysfs_entries,
};
--
1.9.1
^ permalink raw reply related
* Re: BUG: KASAN: use-after-free in free_old_xmit_skbs
From: Michael S. Tsirkin @ 2017-06-22 18:53 UTC (permalink / raw)
To: jean-philippe menil; +Cc: netdev, John Fastabend, qemu-devel, virtualization
In-Reply-To: <CAOTcU=u_HSMQiwDF3Snv5P8mT-b_Aepeb=Z9gbLd1j=2P+Yy+A@mail.gmail.com>
On Thu, Jun 22, 2017 at 08:15:58AM +0200, jean-philippe menil wrote:
> 2017-06-06 1:52 GMT+02:00 Michael S. Tsirkin <mst@redhat.com>:
>
> On Mon, Jun 05, 2017 at 05:08:25AM +0300, Michael S. Tsirkin wrote:
> > On Mon, Jun 05, 2017 at 12:48:53AM +0200, Jean-Philippe Menil wrote:
> > > Hi,
> > >
> > > while playing with xdp and ebpf, i'm hitting the following:
> > >
> > > [ 309.993136]
> > > ==================================================================
> > > [ 309.994735] BUG: KASAN: use-after-free in
> > > free_old_xmit_skbs.isra.29+0x2b7/0x2e0 [virtio_net]
> > > [ 309.998396] Read of size 8 at addr ffff88006aa64220 by task sshd/323
> > > [ 310.000650]
> > > [ 310.002305] CPU: 1 PID: 323 Comm: sshd Not tainted 4.12.0-rc3+ #2
> > > [ 310.004018] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> BIOS
> > > 1.10.2-20170228_101828-anatol 04/01/2014
...
> >
> > Since commit 680557cf79f82623e2c4fd42733077d60a843513
> > virtio_net: rework mergeable buffer handling
> >
> > we no longer must do the resets, we now have enough space
> > to store a bit saying whether a buffer is xdp one or not.
> >
> > And that's probably a cleaner way to fix these issues than
> > try to find and fix the race condition.
> >
> > John?
> >
> > --
> > MST
>
>
> I think I see the source of the race. virtio net calls
> netif_device_detach and assumes no packets will be sent after
> this point. However, all it does is stop all queues so
> no new packets will be transmitted.
>
> Try locking with HARD_TX_LOCK?
>
>
> --
> MST
>
>
> Hi Michael,
>
> from what i see, the race appear when we hit virtnet_reset in virtnet_xdp_set.
> virtnet_reset
> _remove_vq_common
> virtnet_del_vqs
> virtnet_free_queues
> kfree(vi->sq)
> when the xdp program (with two instances of the program to trigger it faster)
> is added or removed.
>
> It's easily repeatable, with 2 cpus and 4 queues on the qemu command line,
> running the xdp_ttl tool from Jesper.
>
> For now, i'm able to continue my qualification, testing if xdp_qp is not null,
> but do not seem to be a sustainable trick.
> if (xdp_qp && vi->xdp_queues_pairs != xdp_qp)
>
> Maybe it will be more clear to you with theses informations.
>
> Best regards.
>
> Jean-Philippe
I'm pretty clear about the issue here, I was trying to figure out a fix.
Jason, any thoughts?
--
MST
^ permalink raw reply
* Re: [PATCH v2 13/14] drm: stm: remove dead code and pointless local lut storage
From: Philippe CORNU @ 2017-06-22 11:49 UTC (permalink / raw)
To: Peter Rosin, linux-kernel@vger.kernel.org
Cc: Boris Brezillon, amd-gfx@lists.freedesktop.org,
intel-gfx@lists.freedesktop.org, David Airlie,
nouveau@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
Russell King, Jani Nikula, Patrik Jakobsson, Yannick FERTRE,
Sean Paul, Benjamin Gaignard, Daniel Vetter, Alex Deucher,
Dave Airlie, virtualization@lists.linux-foundation.org,
Vincent ABRIOU
In-Reply-To: <1498111597-10714-14-git-send-email-peda@axentia.se>
On 06/22/2017 08:06 AM, Peter Rosin wrote:
> The redundant fb helper .load_lut is no longer used, and can not
> work right without also providing the fb helpers .gamma_set and
> .gamma_get thus rendering the code in this driver suspect.
>
Hi Peter,
STM32 chipsets supports 8-bit CLUT mode but this driver version does not
support it "yet" (final patch has not been upstreamed because it was a
too big fbdev patch for simply adding CLUT...).
Regarding your patch below, if it helps you to ease the drm framework
update then I am agree to "acknowledge it" asap, else if you are not in
a hurry, I would prefer a better and definitive patch handling 8-bit
CLUT properly and I am ok to help or/and to do it : )
Extra questions:
- any plan to update modetest with the DRM_FORMAT_C8 support + gamma
get/set?
- do you have a simple way to test clut with fbdev, last year we where
using an old version of the SDL but I am still looking for a small piece
of code to do it (else I will do it myself but C8 on fbdev is not really
a priority ;-)
best regards,
Philippe
> Just remove the dead code.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
> drivers/gpu/drm/stm/ltdc.c | 12 ------------
> drivers/gpu/drm/stm/ltdc.h | 1 -
> 2 files changed, 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
> index 1b9483d..87829b9 100644
> --- a/drivers/gpu/drm/stm/ltdc.c
> +++ b/drivers/gpu/drm/stm/ltdc.c
> @@ -375,17 +375,6 @@ static irqreturn_t ltdc_irq(int irq, void *arg)
> * DRM_CRTC
> */
>
> -static void ltdc_crtc_load_lut(struct drm_crtc *crtc)
> -{
> - struct ltdc_device *ldev = crtc_to_ltdc(crtc);
> - unsigned int i, lay;
> -
> - for (lay = 0; lay < ldev->caps.nb_layers; lay++)
> - for (i = 0; i < 256; i++)
> - reg_write(ldev->regs, LTDC_L1CLUTWR + lay * LAY_OFS,
> - ldev->clut[i]);
> -}
> -
> static void ltdc_crtc_enable(struct drm_crtc *crtc)
> {
> struct ltdc_device *ldev = crtc_to_ltdc(crtc);
> @@ -523,7 +512,6 @@ static void ltdc_crtc_atomic_flush(struct drm_crtc *crtc,
> }
>
> static struct drm_crtc_helper_funcs ltdc_crtc_helper_funcs = {
> - .load_lut = ltdc_crtc_load_lut,
> .enable = ltdc_crtc_enable,
> .disable = ltdc_crtc_disable,
> .mode_set_nofb = ltdc_crtc_mode_set_nofb,
> diff --git a/drivers/gpu/drm/stm/ltdc.h b/drivers/gpu/drm/stm/ltdc.h
> index d7a9c73..620ca55 100644
> --- a/drivers/gpu/drm/stm/ltdc.h
> +++ b/drivers/gpu/drm/stm/ltdc.h
> @@ -27,7 +27,6 @@ struct ltdc_device {
> struct drm_panel *panel;
> struct mutex err_lock; /* protecting error_status */
> struct ltdc_caps caps;
> - u32 clut[256]; /* color look up table */
> u32 error_status;
> u32 irq_status;
> };
>
^ permalink raw reply
* [PATCH v2 03/14] drm/fb-helper: do a generic fb_setcmap helper in terms of crtc .gamma_set
From: Peter Rosin @ 2017-06-22 10:22 UTC (permalink / raw)
To: linux-kernel
Cc: Boris Brezillon, amd-gfx, intel-gfx, Philippe Cornu, David Airlie,
nouveau, dri-devel, Russell King, Jani Nikula, Patrik Jakobsson,
Yannick Fertre, Sean Paul, Benjamin Gaignard, Daniel Vetter,
Alex Deucher, Dave Airlie, virtualization, Vincent Abriou,
Christian König, Ben Skeggs
In-Reply-To: <1498111597-10714-4-git-send-email-peda@axentia.se>
This makes the redundant fb helpers .load_lut, .gamma_set and .gamma_get
totally obsolete.
Signed-off-by: Peter Rosin <peda@axentia.se>
---
drivers/gpu/drm/drm_fb_helper.c | 151 +++++++++++++++++-----------------------
1 file changed, 63 insertions(+), 88 deletions(-)
This is an alternative version rebased on top of Daniel's "fbdev helper
locking rework and deferred setup" series.
And as noted by Daniel, .gamma_set does an atomic commit. Thus, the locks
needs to be dropped and reacquired for each crtc. So, that is fixed here
too. Doing it like this with a couple of individual alternative patches
instead of sending a whole new series since the dependency on Daniel's
series makes life somewhat difficult...
Cheers,
peda
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 4aceb59..aa025f1 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1257,50 +1257,6 @@ void drm_fb_helper_set_suspend_unlocked(struct drm_fb_helper *fb_helper,
}
EXPORT_SYMBOL(drm_fb_helper_set_suspend_unlocked);
-static int setcolreg(struct drm_crtc *crtc, u16 red, u16 green,
- u16 blue, u16 regno, struct fb_info *info)
-{
- struct drm_fb_helper *fb_helper = info->par;
- struct drm_framebuffer *fb = fb_helper->fb;
-
- if (info->fix.visual == FB_VISUAL_TRUECOLOR) {
- u32 *palette;
- u32 value;
- /* place color in psuedopalette */
- if (regno > 16)
- return -EINVAL;
- palette = (u32 *)info->pseudo_palette;
- red >>= (16 - info->var.red.length);
- green >>= (16 - info->var.green.length);
- blue >>= (16 - info->var.blue.length);
- value = (red << info->var.red.offset) |
- (green << info->var.green.offset) |
- (blue << info->var.blue.offset);
- if (info->var.transp.length > 0) {
- u32 mask = (1 << info->var.transp.length) - 1;
-
- mask <<= info->var.transp.offset;
- value |= mask;
- }
- palette[regno] = value;
- return 0;
- }
-
- /*
- * The driver really shouldn't advertise pseudo/directcolor
- * visuals if it can't deal with the palette.
- */
- if (WARN_ON(!fb_helper->funcs->gamma_set ||
- !fb_helper->funcs->gamma_get))
- return -EINVAL;
-
- WARN_ON(fb->format->cpp[0] != 1);
-
- fb_helper->funcs->gamma_set(crtc, red, green, blue, regno);
-
- return 0;
-}
-
/**
* drm_fb_helper_setcmap - implementation for &fb_ops.fb_setcmap
* @cmap: cmap to set
@@ -1310,12 +1266,10 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
{
struct drm_fb_helper *fb_helper = info->par;
struct drm_device *dev = fb_helper->dev;
- const struct drm_crtc_helper_funcs *crtc_funcs;
- u16 *red, *green, *blue, *transp;
+ struct drm_modeset_acquire_ctx ctx;
struct drm_crtc *crtc;
u16 *r, *g, *b;
- int i, j, rc = 0;
- int start;
+ int i, ret = 0;
if (oops_in_progress)
return -EBUSY;
@@ -1329,61 +1283,82 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
return -EBUSY;
}
- drm_modeset_lock_all(dev);
- for (i = 0; i < fb_helper->crtc_count; i++) {
- crtc = fb_helper->crtc_info[i].mode_set.crtc;
- crtc_funcs = crtc->helper_private;
+ drm_modeset_acquire_init(&ctx, 0);
- red = cmap->red;
- green = cmap->green;
- blue = cmap->blue;
- transp = cmap->transp;
- start = cmap->start;
+ for (i = 0; i < fb_helper->crtc_count; i++) {
+ if (info->fix.visual == FB_VISUAL_TRUECOLOR) {
+ u32 *palette;
+ int j;
- if (info->fix.visual != FB_VISUAL_TRUECOLOR) {
- if (!crtc->gamma_size) {
- rc = -EINVAL;
- goto out;
+ if (cmap->start + cmap->len > 16) {
+ ret = -EINVAL;
+ break;
}
- if (cmap->start + cmap->len > crtc->gamma_size) {
- rc = -EINVAL;
- goto out;
+ palette = (u32 *)info->pseudo_palette;
+ for (j = 0; j < cmap->len; ++j) {
+ u16 red = cmap->red[j];
+ u16 green = cmap->green[j];
+ u16 blue = cmap->blue[j];
+ u32 value;
+
+ red >>= 16 - info->var.red.length;
+ green >>= 16 - info->var.green.length;
+ blue >>= 16 - info->var.blue.length;
+ value = (red << info->var.red.offset) |
+ (green << info->var.green.offset) |
+ (blue << info->var.blue.offset);
+ if (info->var.transp.length > 0) {
+ u32 mask = (1 << info->var.transp.length) - 1;
+
+ mask <<= info->var.transp.offset;
+ value |= mask;
+ }
+ palette[cmap->start + j] = value;
}
+ continue;
+ }
- r = crtc->gamma_store;
- g = r + crtc->gamma_size;
- b = g + crtc->gamma_size;
+retry:
+ ret = drm_modeset_lock_all_ctx(dev, &ctx);
+ if (ret)
+ break;
- memcpy(r + cmap->start, cmap->red,
- cmap->len * sizeof(u16));
- memcpy(g + cmap->start, cmap->green,
- cmap->len * sizeof(u16));
- memcpy(b + cmap->start, cmap->blue,
- cmap->len * sizeof(u16));
+ crtc = fb_helper->crtc_info[i].mode_set.crtc;
+ if (!crtc->funcs->gamma_set || !crtc->gamma_size) {
+ ret = -EINVAL;
+ goto drop_locks;
}
- for (j = 0; j < cmap->len; j++) {
- u16 hred, hgreen, hblue, htransp = 0xffff;
+ if (cmap->start + cmap->len > crtc->gamma_size) {
+ ret = -EINVAL;
+ goto drop_locks;
+ }
- hred = *red++;
- hgreen = *green++;
- hblue = *blue++;
+ r = crtc->gamma_store;
+ g = r + crtc->gamma_size;
+ b = g + crtc->gamma_size;
- if (transp)
- htransp = *transp++;
+ memcpy(r + cmap->start, cmap->red, cmap->len * sizeof(u16));
+ memcpy(g + cmap->start, cmap->green, cmap->len * sizeof(u16));
+ memcpy(b + cmap->start, cmap->blue, cmap->len * sizeof(u16));
- rc = setcolreg(crtc, hred, hgreen, hblue, start++, info);
- if (rc)
- goto out;
+ ret = crtc->funcs->gamma_set(crtc, r, g, b,
+ crtc->gamma_size, &ctx);
+ if (ret == -EDEADLK) {
+ drm_modeset_backoff(&ctx);
+ goto retry;
}
- if (crtc_funcs->load_lut)
- crtc_funcs->load_lut(crtc);
+drop_locks:
+ drm_modeset_drop_locks(&ctx);
+ if (ret)
+ break;
}
- out:
- drm_modeset_unlock_all(dev);
+
+ drm_modeset_acquire_fini(&ctx);
mutex_unlock(&fb_helper->lock);
- return rc;
+
+ return ret;
}
EXPORT_SYMBOL(drm_fb_helper_setcmap);
--
2.1.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox