linux-kernel - Re: [PATCH 6/6 v2] ARM: Add support for the display controllers in VT8500 and WM8505

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 8 Nov 2010 12:56:42 +0000
From:	Alexey Charkov <alchark@...il.com>
To:	Paul Mundt <lethal@...ux-sh.org>
Cc:	linux-arm-kernel@...ts.infradead.org,
	vt8500-wm8505-linux-kernel@...glegroups.com,
	Andrew Morton <akpm@...ux-foundation.org>,
	Guennadi Liakhovetski <g.liakhovetski@....de>,
	Florian Tobias Schandinat <FlorianSchandinat@....de>,
	Ralf Baechle <ralf@...ux-mips.org>,
	"David S. Miller" <davem@...emloft.net>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/6 v2] ARM: Add support for the display controllers in
 VT8500 and WM8505

2010/11/8 Paul Mundt <lethal@...ux-sh.org>:
> On Sun, Nov 07, 2010 at 07:28:57PM +0300, Alexey Charkov wrote:
>> +static int __devinit vt8500lcd_probe(struct platform_device *pdev)
>> +{
>
> ...
>
>> +     addr = fbi;
>> +     addr = addr + sizeof(struct vt8500lcd_info);
>> +     fbi->fb.pseudo_palette  = addr;
>> +
> ...
>
>> +     fbi->palette_size       = PAGE_ALIGN(512);
>> +     fbi->palette_cpu        = dma_alloc_coherent(&pdev->dev,
>> +                                                  fbi->palette_size,
>> +                                                  &fbi->palette_phys,
>> +                                                  GFP_KERNEL);
>> +     if (fbi->fb.pseudo_palette == NULL) {
>> +             dev_err(&pdev->dev, "Failed to allocate palette buffer\n");
>> +             ret = -ENOMEM;
>> +             goto failed_free_io;
>> +     }
>> +
> This looks like a bogus test, you've already allocated enough space for
> the pseudo_palette and will have bailed out on the kmalloc() failing well
> before this. You also don't have any error handling for fbi->palette_cpu,
> which is presumably what you intended to do here.
>

True, this has to be corrected (old copy-paste error left unnoticed
somehow). It's also deallocated wrongly in the error path, which I
have just noticed.

>> +static int __devexit vt8500lcd_remove(struct platform_device *pdev)
>> +{
>> +     struct vt8500lcd_info *fbi = platform_get_drvdata(pdev);
>> +     struct resource *res;
>> +     int irq;
>> +
>> +     if (!fbi)
>> +             return 0;
>> +
>> +     unregister_framebuffer(&fbi->fb);
>> +
>> +     writel(0, fbi->regbase);
>> +
>> +     if (fbi->fb.cmap.len)
>> +             fb_dealloc_cmap(&fbi->fb.cmap);
>> +
>> +     irq = platform_get_irq(pdev, 0);
>> +     free_irq(irq, fbi);
>> +
>> +     iounmap(fbi->regbase);
>> +
>
> You're also missing a dma_free_coherent() here.
>

True, will be fixed.

>> +static int __devinit wm8505fb_probe(struct platform_device *pdev)
>> +{
>> +     fbi->fb.screen_base     = pdata->video_mem_virt;
>> +     fbi->fb.screen_size     = pdata->video_mem_len;
>> +
> ...
>
>> +failed_free_mem:
>> +     free_pages_exact(fbi->fb.screen_base, fbi->fb.screen_size);
>
> ...
>
> What in the name of all that is holy are you doing here? If you need to
> have your platform deal with virtual address allocation and freeing then
> you should pass in callbacks for that and hide the instrumentation
> details there. Presently this is tying you down to an alloc_pages_exact()
> interface buried in the board setup, which isn't going to mesh well with
> other platforms that may wish to go about this an alternate way (like
> memblock reservations).
>

Actually, this is a leftover from a previous implementation with
alloc_pages_exact(), which could not work for larger screen sizes (due
to the framebuffer growing over 4MB). Now memory is reserved via
memblock, so this should have probably been just dropped.

>> diff --git a/drivers/video/wmt_ge_rops.c b/drivers/video/wmt_ge_rops.c
>> new file mode 100644
>> index 0000000..c71f97e
>> --- /dev/null
>> +++ b/drivers/video/wmt_ge_rops.c
>> +void __iomem *regbase;
>> +
> Uhm, no. If this is only used in this driver then just make it static.
> Given that you are using the driver model here though and could
> theoretically support multiple rop engines, you're much better off making
> this private data and burying it under the appropriate per-device data
> structures.
>

Is that platform_{set,get}_drvdata() what you are talking about? Using
multiple engines seems extremely unlikely, though :)

>> +int wmt_ge_sync(struct fb_info *p)
>> +{
>> +     while (readl(regbase + GE_STATUS_OFF) & 4)
>> +             /* busy wait */;
>> +
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL(wmt_ge_sync);
>> +
> While I admire your optimism in your hardware, experience suggests you
> really want a timeout here. You may also wish to insert a cpu_relax()
> here.
>

I wonder if this callback is allowed to sleep? In principle, the
hardware seems to support interrupt generation on operation
completion, so maybe wait_event_interruptible_timeout() could be used
here to remove the busy wait altogether?

Thank you for the comments, Paul!

Best regards,
Alexey
--
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/