| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 09 Jun 2015 08:23:59 +0200
From: Hans Verkuil <hverkuil@...all.nl>
To: Mauro Carvalho Chehab <mchehab@....samsung.com>,
"Luis R. Rodriguez" <mcgrof@...not-panic.com>
CC: bp@...e.de, tomi.valkeinen@...com, bhelgaas@...gle.com,
linux-media@...r.kernel.org, linux-rdma@...r.kernel.org,
linux-kernel@...r.kernel.org,
"Luis R. Rodriguez" <mcgrof@...e.com>,
Andy Walls <awalls@...metrocast.net>,
Doug Ledford <dledford@...hat.com>,
Andy Lutomirski <luto@...capital.net>,
Suresh Siddha <sbsiddha@...il.com>,
Ingo Molnar <mingo@...e.hu>,
Thomas Gleixner <tglx@...utronix.de>,
Juergen Gross <jgross@...e.com>,
Daniel Vetter <daniel.vetter@...ll.ch>,
Dave Airlie <airlied@...hat.com>,
Antonino Daplas <adaplas@...il.com>,
Jean-Christophe Plagniol-Villard <plagnioj@...osoft.com>,
Dave Hansen <dave.hansen@...ux.intel.com>,
Arnd Bergmann <arnd@...db.de>,
"Michael S. Tsirkin" <mst@...hat.com>,
Stefan Bader <stefan.bader@...onical.com>,
Ville Syrjälä <syrjala@....fi>,
Mel Gorman <mgorman@...e.de>, Vlastimil Babka <vbabka@...e.cz>,
Davidlohr Bueso <dbueso@...e.de>, konrad.wilk@...cle.com,
ville.syrjala@...ux.intel.com, david.vrabel@...rix.com,
jbeulich@...e.com, toshi.kani@...com,
Roger Pau Monné <roger.pau@...rix.com>,
linux-fbdev@...r.kernel.org, ivtv-devel@...vdriver.org,
xen-devel@...ts.xensource.com
Subject: Re: [PATCH v6 1/3] ivtv: use arch_phys_wc_add() and require PAT disabled
On 06/09/2015 02:56 AM, Mauro Carvalho Chehab wrote:
> Em Mon, 08 Jun 2015 17:20:20 -0700
> "Luis R. Rodriguez" <mcgrof@...not-panic.com> escreveu:
>
>> From: "Luis R. Rodriguez" <mcgrof@...e.com>
>>
>> We are burrying direct access to MTRR code support on
>> x86 in order to take advantage of PAT. In the future we
>> also want to make the default behaviour of ioremap_nocache()
>> to use strong UC, use of mtrr_add() on those systems
>> would make write-combining void.
>>
>> In order to help both enable us to later make strong
>> UC default and in order to phase out direct MTRR access
>> code port the driver over to arch_phys_wc_add() and
>> annotate that the device driver requires systems to
>> boot with PAT disabled, with the nopat kernel parameter.
>>
>> This is a worthy comprmise given that the hardware is
>> really rare these days, and perhaps only some lost souls
>> in some third world country are expected to be using this
>> feature of the device driver.
>>
>> Acked-by: Andy Walls <awalls@...metrocast.net>
>> Cc: Andy Walls <awalls@...metrocast.net>
>> Cc: Doug Ledford <dledford@...hat.com>
>> Cc: Mauro Carvalho Chehab <mchehab@....samsung.com>
>
> Provided that you fix the issues below:
> Acked-by: Mauro Carvalho Chehab <mchehab@....samsung.com>
>
>> Cc: Andy Lutomirski <luto@...capital.net>
>> Cc: Suresh Siddha <sbsiddha@...il.com>
>> Cc: Ingo Molnar <mingo@...e.hu>
>> Cc: Thomas Gleixner <tglx@...utronix.de>
>> Cc: Juergen Gross <jgross@...e.com>
>> Cc: Daniel Vetter <daniel.vetter@...ll.ch>
>> Cc: Dave Airlie <airlied@...hat.com>
>> Cc: Bjorn Helgaas <bhelgaas@...gle.com>
>> Cc: Antonino Daplas <adaplas@...il.com>
>> Cc: Jean-Christophe Plagniol-Villard <plagnioj@...osoft.com>
>> Cc: Tomi Valkeinen <tomi.valkeinen@...com>
>> Cc: Dave Hansen <dave.hansen@...ux.intel.com>
>> Cc: Arnd Bergmann <arnd@...db.de>
>> Cc: Michael S. Tsirkin <mst@...hat.com>
>> Cc: Stefan Bader <stefan.bader@...onical.com>
>> Cc: Ville Syrjälä <syrjala@....fi>
>> Cc: Mel Gorman <mgorman@...e.de>
>> Cc: Vlastimil Babka <vbabka@...e.cz>
>> Cc: Borislav Petkov <bp@...e.de>
>> Cc: Davidlohr Bueso <dbueso@...e.de>
>> Cc: konrad.wilk@...cle.com
>> Cc: ville.syrjala@...ux.intel.com
>> Cc: david.vrabel@...rix.com
>> Cc: jbeulich@...e.com
>> Cc: toshi.kani@...com
>> Cc: Roger Pau Monné <roger.pau@...rix.com>
>> Cc: linux-fbdev@...r.kernel.org
>> Cc: linux-kernel@...r.kernel.org
>> Cc: ivtv-devel@...vdriver.org
>> Cc: linux-media@...r.kernel.org
>> Cc: xen-devel@...ts.xensource.com
>> Signed-off-by: Luis R. Rodriguez <mcgrof@...e.com>
>> ---
>> drivers/media/pci/ivtv/Kconfig | 3 +++
>> drivers/media/pci/ivtv/ivtvfb.c | 58 ++++++++++++++++-------------------------
>> 2 files changed, 26 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/media/pci/ivtv/Kconfig b/drivers/media/pci/ivtv/Kconfig
>> index dd6ee57e..b2a7f88 100644
>> --- a/drivers/media/pci/ivtv/Kconfig
>> +++ b/drivers/media/pci/ivtv/Kconfig
>> @@ -57,5 +57,8 @@ config VIDEO_FB_IVTV
>> This is used in the Hauppauge PVR-350 card. There is a driver
>> homepage at <http://www.ivtvdriver.org>.
>>
>> + If you have this hardware you will need to boot with PAT disabled
>> + on your x86 systems, use the nopat kernel parameter.
>> +
>
> Hmm... FB_IVTV is not hardware... it is framebuffer support for IVTV.
> It is optional to use FB API for the video output port of this board,
> instead of using V4L2 API.
That's not true. It is hardware: it drives the video output OSD which overlays
the video output. The reason it is optional is that it is hard to unload a
framebuffer module and most people don't need it.
So V4L2 drives the video output and ivtvfb drives the OSD overlay. So it is
not a case of 'instead of'.
>
> I would say, instead, something like:
>
> "In order to use this module, you will need to boot with PAT disabled
> on x86 systems, using the nopat kernel parameter."
I do agree with this change, but that's because this module is optional and
not for the reasons you mentioned above.
Regards,
Hans
>
>> To compile this driver as a module, choose M here: the
>> module will be called ivtvfb.
>> diff --git a/drivers/media/pci/ivtv/ivtvfb.c b/drivers/media/pci/ivtv/ivtvfb.c
>> index 9ff1230..7685ae3 100644
>> --- a/drivers/media/pci/ivtv/ivtvfb.c
>> +++ b/drivers/media/pci/ivtv/ivtvfb.c
>> @@ -44,8 +44,8 @@
>> #include <linux/ivtvfb.h>
>> #include <linux/slab.h>
>>
>> -#ifdef CONFIG_MTRR
>> -#include <asm/mtrr.h>
>> +#ifdef CONFIG_X86_64
>> +#include <asm/pat.h>
>> #endif
>>
>> #include "ivtv-driver.h"
>> @@ -155,12 +155,11 @@ struct osd_info {
>> /* Buffer size */
>> u32 video_buffer_size;
>>
>> -#ifdef CONFIG_MTRR
>> /* video_base rounded down as required by hardware MTRRs */
>> unsigned long fb_start_aligned_physaddr;
>> /* video_base rounded up as required by hardware MTRRs */
>> unsigned long fb_end_aligned_physaddr;
>> -#endif
>> + int wc_cookie;
>>
>> /* Store the buffer offset */
>> int set_osd_coords_x;
>> @@ -1099,6 +1098,8 @@ static int ivtvfb_init_vidmode(struct ivtv *itv)
>> static int ivtvfb_init_io(struct ivtv *itv)
>> {
>> struct osd_info *oi = itv->osd_info;
>> + /* Find the largest power of two that maps the whole buffer */
>> + int size_shift = 31;
>>
>> mutex_lock(&itv->serialize_lock);
>> if (ivtv_init_on_first_open(itv)) {
>> @@ -1132,29 +1133,16 @@ static int ivtvfb_init_io(struct ivtv *itv)
>> oi->video_pbase, oi->video_vbase,
>> oi->video_buffer_size / 1024);
>>
>> -#ifdef CONFIG_MTRR
>> - {
>> - /* Find the largest power of two that maps the whole buffer */
>> - int size_shift = 31;
>> -
>> - while (!(oi->video_buffer_size & (1 << size_shift))) {
>> - size_shift--;
>> - }
>> - size_shift++;
>> - oi->fb_start_aligned_physaddr = oi->video_pbase & ~((1 << size_shift) - 1);
>> - oi->fb_end_aligned_physaddr = oi->video_pbase + oi->video_buffer_size;
>> - oi->fb_end_aligned_physaddr += (1 << size_shift) - 1;
>> - oi->fb_end_aligned_physaddr &= ~((1 << size_shift) - 1);
>> - if (mtrr_add(oi->fb_start_aligned_physaddr,
>> - oi->fb_end_aligned_physaddr - oi->fb_start_aligned_physaddr,
>> - MTRR_TYPE_WRCOMB, 1) < 0) {
>> - IVTVFB_INFO("disabled mttr\n");
>> - oi->fb_start_aligned_physaddr = 0;
>> - oi->fb_end_aligned_physaddr = 0;
>> - }
>> - }
>> -#endif
>> -
>> + while (!(oi->video_buffer_size & (1 << size_shift)))
>> + size_shift--;
>> + size_shift++;
>> + oi->fb_start_aligned_physaddr = oi->video_pbase & ~((1 << size_shift) - 1);
>> + oi->fb_end_aligned_physaddr = oi->video_pbase + oi->video_buffer_size;
>> + oi->fb_end_aligned_physaddr += (1 << size_shift) - 1;
>> + oi->fb_end_aligned_physaddr &= ~((1 << size_shift) - 1);
>> + oi->wc_cookie = arch_phys_wc_add(oi->fb_start_aligned_physaddr,
>> + oi->fb_end_aligned_physaddr -
>> + oi->fb_start_aligned_physaddr);
>> /* Blank the entire osd. */
>> memset_io(oi->video_vbase, 0, oi->video_buffer_size);
>>
>> @@ -1172,14 +1160,7 @@ static void ivtvfb_release_buffers (struct ivtv *itv)
>>
>> /* Release pseudo palette */
>> kfree(oi->ivtvfb_info.pseudo_palette);
>> -
>> -#ifdef CONFIG_MTRR
>> - if (oi->fb_end_aligned_physaddr) {
>> - mtrr_del(-1, oi->fb_start_aligned_physaddr,
>> - oi->fb_end_aligned_physaddr - oi->fb_start_aligned_physaddr);
>> - }
>> -#endif
>> -
>> + arch_phys_wc_del(oi->wc_cookie);
>> kfree(oi);
>> itv->osd_info = NULL;
>> }
>> @@ -1284,6 +1265,13 @@ static int __init ivtvfb_init(void)
>> int registered = 0;
>> int err;
>>
>> +#ifdef CONFIG_X86_64
>> + if (WARN(pat_enabled(),
>> + "ivtvfb needs PAT disabled, boot with nopat kernel parameter\n")) {
>> + return EINVAL;
>
> Errors are always negative. So:
> return -EINVAL
>
> Or, perhaps, -ENODEV.
>
>> + }
>> +#endif
>> +
>> if (ivtvfb_card_id < -1 || ivtvfb_card_id >= IVTV_MAX_CARDS) {
>> printk(KERN_ERR "ivtvfb: ivtvfb_card_id parameter is out of range (valid range: -1 - %d)\n",
>> IVTV_MAX_CARDS - 1);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists