| 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
| ||
|
Message-ID: <386072610803060850i2e0b6e35q7e05448d5fd39bca@mail.gmail.com>
Date: Thu, 6 Mar 2008 08:50:32 -0800
From: "Bryan Wu" <cooloney@...nel.org>
To: "Andrew Morton" <akpm@...ux-foundation.org>
Cc: bryan.wu@...log.com, linux-fbdev-devel@...ts.sourceforge.net,
"Randy Dunlap" <randy.dunlap@...cle.com>,
"Michael Hennerich" <michael.hennerich@...log.com>,
linux-kernel@...r.kernel.org, adaplas@...il.com
Subject: Re: [Linux-fbdev-devel] [PATCH 1/1 try#2] [VIDEO/FRAMEBUFFER]: add BF52x EZkit Display driver
On Wed, Mar 5, 2008 at 11:02 PM, Andrew Morton
<akpm@...ux-foundation.org> wrote:
> On Thu, 31 Jan 2008 00:49:43 +0800 Bryan Wu <bryan.wu@...log.com> wrote:
> >
>
> bah, January.
>
> I'm trolling the fbdev list only now - I guess I'll slip this one into
> 2.6.25.
>
You finally caught this, -;)
>
> > +static int bfin_t350mcqb_request_ports(int action)
> > +{
> > + u16 ppi0_req_8[] = {P_PPI0_CLK, P_PPI0_FS1, P_PPI0_FS2,
> > + P_PPI0_D0, P_PPI0_D1, P_PPI0_D2,
> > + P_PPI0_D3, P_PPI0_D4, P_PPI0_D5,
> > + P_PPI0_D6, P_PPI0_D7, 0};
>
> This array will be assembled on the stack at runtime. If
> peripheral_request_list() doesn't alter it then it would be better to
> create it at compile-time by adding `static'.
>
Exactly, I will do it.
>
> > + if (action) {
> > + if (peripheral_request_list(ppi0_req_8, DRIVER_NAME)) {
> > + printk(KERN_ERR "Requesting Peripherals faild\n");
> > + return -EFAULT;
> > + }
> > + } else
> > + peripheral_free_list(ppi0_req_8);
> > +
> > + return 0;
> > +}
> > +
>
> > +static int bfin_t350mcqb_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
> > +{
> > + struct bfin_t350mcqbfb_info *fbi = info->par;
> > +
> > + if (fbi->lq043_mmap)
> > + return -1;
> > +
> > + spin_lock(&fbi->lock);
> > + fbi->lq043_mmap = 1;
> > + spin_unlock(&fbi->lock);
>
> the locking here probably doesn't do a lot.
>
Actually, this locking is necessary here for trying to mmap by two
application, IMO.
>
> > + vma->vm_start = (unsigned long)(fbi->fb_buffer + ACTIVE_VIDEO_MEM_OFFSET);
> > +
> > + vma->vm_end = vma->vm_start + info->fix.smem_len;
> > + /* For those who don't understand how mmap works, go read
> > + * Documentation/nommu-mmap.txt.
> > + * For those that do, you will know that the VM_MAYSHARE flag
> > + * must be set in the vma->vm_flags structure on noMMU
> > + * Other flags can be set, and are documented in
> > + * include/linux/mm.h
> > + */
> > + vma->vm_flags |= VM_MAYSHARE;
> > +
> > + return 0;
> > +}
> > +
> > +
>
>
> > +static int __init bfin_t350mcqb_probe(struct platform_device *pdev)
> > +{
> > + struct bfin_t350mcqbfb_info *info;
> > + struct fb_info *fbinfo;
> > + int ret;
> > +
> > + printk(KERN_INFO DRIVER_NAME ": %dx%d %d-bit RGB FrameBuffer initializing...\n",
> > + LCD_X_RES, LCD_Y_RES, LCD_BPP);
> > +
> > + if (request_dma(CH_PPI, "CH_PPI") < 0) {
> > + printk(KERN_ERR DRIVER_NAME
> > + ": couldn't request CH_PPI DMA\n");
> > + ret = -EFAULT;
> > + goto out1;
> > + }
> > +
> > + fbinfo =
> > + framebuffer_alloc(sizeof(struct bfin_t350mcqbfb_info), &pdev->dev);
> > + if (!fbinfo) {
> > + ret = -ENOMEM;
> > + goto out2;
> > + }
> > +
> > + info = fbinfo->par;
> > + info->fb = fbinfo;
> > + info->dev = &pdev->dev;
> > +
> > + platform_set_drvdata(pdev, fbinfo);
> > +
> > + strcpy(fbinfo->fix.id, driver_name);
> > +
> > + fbinfo->fix.type = FB_TYPE_PACKED_PIXELS;
> > + fbinfo->fix.type_aux = 0;
> > + fbinfo->fix.xpanstep = 0;
> > + fbinfo->fix.ypanstep = 0;
> > + fbinfo->fix.ywrapstep = 0;
> > + fbinfo->fix.accel = FB_ACCEL_NONE;
> > + fbinfo->fix.visual = FB_VISUAL_TRUECOLOR;
> > +
> > + fbinfo->var.nonstd = 0;
> > + fbinfo->var.activate = FB_ACTIVATE_NOW;
> > + fbinfo->var.height = -1;
> > + fbinfo->var.width = -1;
> > + fbinfo->var.accel_flags = 0;
> > + fbinfo->var.vmode = FB_VMODE_NONINTERLACED;
> > +
> > + fbinfo->var.xres = LCD_X_RES;
> > + fbinfo->var.xres_virtual = LCD_X_RES;
> > + fbinfo->var.yres = LCD_Y_RES;
> > + fbinfo->var.yres_virtual = LCD_Y_RES;
> > + fbinfo->var.bits_per_pixel = LCD_BPP;
> > +
> > + fbinfo->var.red.offset = 0;
> > + fbinfo->var.green.offset = 8;
> > + fbinfo->var.blue.offset = 16;
> > + fbinfo->var.transp.offset = 0;
> > + fbinfo->var.red.length = 8;
> > + fbinfo->var.green.length = 8;
> > + fbinfo->var.blue.length = 8;
> > + fbinfo->var.transp.length = 0;
> > + fbinfo->fix.smem_len = LCD_X_RES * LCD_Y_RES * LCD_BPP / 8;
> > +
> > + fbinfo->fix.line_length = fbinfo->var.xres_virtual *
> > + fbinfo->var.bits_per_pixel / 8;
> > +
> > +
> > + fbinfo->fbops = &bfin_t350mcqb_fb_ops;
> > + fbinfo->flags = FBINFO_FLAG_DEFAULT;
> > +
> > + info->fb_buffer =
> > + dma_alloc_coherent(NULL, fbinfo->fix.smem_len, &info->dma_handle,
> > + GFP_KERNEL);
> > +
> > + if (NULL == info->fb_buffer) {
> > + printk(KERN_ERR DRIVER_NAME
> > + ": couldn't allocate dma buffer.\n");
> > + ret = -ENOMEM;
> > + goto out3;
> > + }
> > +
> > + memset(info->fb_buffer, 0, fbinfo->fix.smem_len);
> > +
> > + fbinfo->screen_base = (void *)info->fb_buffer + ACTIVE_VIDEO_MEM_OFFSET;
> > + fbinfo->fix.smem_start = (int)info->fb_buffer + ACTIVE_VIDEO_MEM_OFFSET;
> > +
> > + fbinfo->fbops = &bfin_t350mcqb_fb_ops;
> > +
> > + fbinfo->pseudo_palette = kmalloc(sizeof(u32) * 16, GFP_KERNEL);
> > + if (!fbinfo->pseudo_palette) {
> > + printk(KERN_ERR DRIVER_NAME
> > + "Fail to allocate pseudo_palette\n");
> > +
> > + ret = -ENOMEM;
> > + goto out4;
> > + }
> > +
> > + memset(fbinfo->pseudo_palette, 0, sizeof(u32) * 16);
>
> You just invented kzalloc!
>
Right
>
>
> > + if (fb_alloc_cmap(&fbinfo->cmap, BFIN_LCD_NBR_PALETTE_ENTRIES, 0)
> > + < 0) {
> > + printk(KERN_ERR DRIVER_NAME
> > + "Fail to allocate colormap (%d entries)\n",
> > + BFIN_LCD_NBR_PALETTE_ENTRIES);
> > + ret = -EFAULT;
> > + goto out5;
> > + }
> > +
> > + if (bfin_t350mcqb_request_ports(1)) {
> > + printk(KERN_ERR DRIVER_NAME ": couldn't request gpio port.\n");
> > + ret = -EFAULT;
> > + goto out6;
> > + }
> > +
> > + info->irq = platform_get_irq(pdev, 0);
> > + if (info->irq < 0) {
> > + ret = -EINVAL;
> > + goto out7;
> > + }
> > +
> > + if (request_irq(info->irq, (void *)bfin_t350mcqb_irq_error, IRQF_DISABLED,
> > + "PPI ERROR", info) < 0) {
> > + printk(KERN_ERR DRIVER_NAME
> > + ": unable to request PPI ERROR IRQ\n");
> > + ret = -EFAULT;
>
> EFAULT?
>
It should return the request_irq return value and other similar
function calls should be checked like this,
right? Michael.
-Bryan
--
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