| Message ID | 1244121977-24927-1-git-send-email-ameya.palande@nokia.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
>From: Ameya Palande [mailto:ameya.palande@nokia.com] >Subject: [PATCH] DSPBRIDGE: Compiler wanrning fix for unlocked_ioctl > >From: Ameya Palande <ameya.palande@nokia.com> > >Signed-off-by: Ameya Palande <ameya.palande@nokia.com> >--- > drivers/dsp/bridge/rmgr/drv_interface.c | 2 +- > drivers/dsp/bridge/rmgr/drv_interface.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > >diff --git a/drivers/dsp/bridge/rmgr/drv_interface.c b/drivers/dsp/bridge/rmgr/drv_interface.c >index c3d8a02..f41e153 100644 >--- a/drivers/dsp/bridge/rmgr/drv_interface.c >+++ b/drivers/dsp/bridge/rmgr/drv_interface.c >@@ -665,7 +665,7 @@ static int bridge_release(struct inode *ip, struct file *filp) > } > > /* This function provides IO interface to the bridge driver. */ >-static int bridge_ioctl(struct file *filp, unsigned int code, >+static long bridge_ioctl(struct file *filp, unsigned int code, > unsigned long args) > { > int status; >diff --git a/drivers/dsp/bridge/rmgr/drv_interface.h b/drivers/dsp/bridge/rmgr/drv_interface.h >index 3e77ed0..dc49210 100644 >--- a/drivers/dsp/bridge/rmgr/drv_interface.h >+++ b/drivers/dsp/bridge/rmgr/drv_interface.h >@@ -34,7 +34,7 @@ static int __init bridge_init(void); /* Initialize bridge */ > static void __exit bridge_exit(void); /* Opposite of initialize */ > static int bridge_open(struct inode *, struct file *); /* Open */ > static int bridge_release(struct inode *, struct file *); /* Release */ >-static int bridge_ioctl(struct file *, unsigned int, >+static long bridge_ioctl(struct file *, unsigned int, > unsigned long); > static int bridge_mmap(struct file *filp, struct vm_area_struct *vma); > #endif /* ifndef _DRV_INTERFACE_H_ */ >-- >1.6.2.4 > Pushed to bridge tree on dev.omapzoom.org - omar -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap- > owner@vger.kernel.org] On Behalf Of Hiroshi DOYU > Sent: Monday, June 08, 2009 6:34 AM > To: Ramirez Luna, Omar > Cc: ameya.palande@nokia.com; linux-omap@vger.kernel.org; > felipe.contreras@gmail.com > Subject: Re: [PATCH] DSPBRIDGE: Compiler wanrning fix for unlocked_ioctl > http://marc.info/?l=linux-omap&m=124201773423892&w=2 > > for the second one about 128 byte alignment. let me know your > thought/plan on it. How about enabling this[1] under a configurable option? Though this is a strong check, currently components such as ti-openmax IL does a padded allocation to ignore overwrite of buffer region which might be at risk. The check is desirable, but should have an option to disable it, if so desired. Regards, Nishanth Menon Ref: [1] http://marc.info/?l=linux-omap&m=124201773423892&w=2 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Doyu-san, Regarding > http://marc.info/?l=linux-omap&m=124201773423892&w=2 > I think that the first one should be merged into d.o-z.org now, but > for the second one about 128 byte alignment. let me know your > thought/plan on it. -- I think you sent this patch as a probable fix for the slab corruption that was observed in Bridge driver, but then we found that slab corruption was due to some other issue in Bridge driver and not due to the cache alignment. Irrespective of above point, I think it is good to enforce the cache alignment check, but I think the check should be in Proc Map function and to start with the check should be under a flag so as not to affect some MM applications that use padding to get over the alignment issue. Thank you, Best regards, Hari > -----Original Message----- > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap- > owner@vger.kernel.org] On Behalf Of Hiroshi DOYU > Sent: Monday, June 08, 2009 6:34 AM > To: Ramirez Luna, Omar > Cc: ameya.palande@nokia.com; linux-omap@vger.kernel.org; > felipe.contreras@gmail.com > Subject: Re: [PATCH] DSPBRIDGE: Compiler wanrning fix for unlocked_ioctl > > From: "ext Ramirez Luna, Omar" <omar.ramirez@ti.com> > Subject: RE: [PATCH] DSPBRIDGE: Compiler wanrning fix for unlocked_ioctl > Date: Mon, 8 Jun 2009 08:49:03 +0200 > > > >From: Ameya Palande [mailto:ameya.palande@nokia.com] > > >Subject: [PATCH] DSPBRIDGE: Compiler wanrning fix for unlocked_ioctl > > > > > >From: Ameya Palande <ameya.palande@nokia.com> > > > > > >Signed-off-by: Ameya Palande <ameya.palande@nokia.com> > > >--- > > > drivers/dsp/bridge/rmgr/drv_interface.c | 2 +- > > > drivers/dsp/bridge/rmgr/drv_interface.h | 2 +- > > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > > >diff --git a/drivers/dsp/bridge/rmgr/drv_interface.c > b/drivers/dsp/bridge/rmgr/drv_interface.c > > >index c3d8a02..f41e153 100644 > > >--- a/drivers/dsp/bridge/rmgr/drv_interface.c > > >+++ b/drivers/dsp/bridge/rmgr/drv_interface.c > > >@@ -665,7 +665,7 @@ static int bridge_release(struct inode *ip, struct > file *filp) > > > } > > > > > > /* This function provides IO interface to the bridge driver. */ > > >-static int bridge_ioctl(struct file *filp, unsigned int code, > > >+static long bridge_ioctl(struct file *filp, unsigned int code, > > > unsigned long args) > > > { > > > int status; > > >diff --git a/drivers/dsp/bridge/rmgr/drv_interface.h > b/drivers/dsp/bridge/rmgr/drv_interface.h > > >index 3e77ed0..dc49210 100644 > > >--- a/drivers/dsp/bridge/rmgr/drv_interface.h > > >+++ b/drivers/dsp/bridge/rmgr/drv_interface.h > > >@@ -34,7 +34,7 @@ static int __init bridge_init(void); /* Initialize > bridge */ > > > static void __exit bridge_exit(void); /* Opposite of initialize */ > > > static int bridge_open(struct inode *, struct file *); /* Open */ > > > static int bridge_release(struct inode *, struct file *); /* > Release */ > > >-static int bridge_ioctl(struct file *, unsigned int, > > >+static long bridge_ioctl(struct file *, unsigned int, > > > unsigned long); > > > static int bridge_mmap(struct file *filp, struct vm_area_struct *vma); > > > #endif /* ifndef _DRV_INTERFACE_H_ */ > > >-- > > >1.6.2.4 > > > > > > > Pushed to bridge tree on dev.omapzoom.org > > Thanks. Also there still seems to remain 2 patches: > > http://marc.info/?l=linux-omap&m=124201773423895&w=2 > http://marc.info/?l=linux-omap&m=124201773423892&w=2 > > I think that the first one should be merged into d.o-z.org now, but > for the second one about 128 byte alignment. let me know your > thought/plan on it. > > > > > - omar > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 8, 2009 at 11:49 PM, Kanigeri, Hari<h-kanigeri2@ti.com> wrote: > Hi Doyu-san, > > Regarding > >> http://marc.info/?l=linux-omap&m=124201773423892&w=2 >> I think that the first one should be merged into d.o-z.org now, but >> for the second one about 128 byte alignment. let me know your >> thought/plan on it. > > -- I think you sent this patch as a probable fix for the slab corruption that was observed in Bridge driver, but then we found that slab corruption was due to some other issue in Bridge driver and not due to the cache alignment. > > Irrespective of above point, I think it is good to enforce the cache alignment check, but I think the check should be in Proc Map function and to start with the check should be under a flag so as not to affect some MM applications that use padding to get over the alignment issue. I agree, the check should be in proc map, and should be optional. However, I would prefer it to be an all-or-nothing option, how about in kconfig?
> I agree, the check should be in proc map, and should be optional. > However, I would prefer it to be an all-or-nothing option, > how about in kconfig? > -- One other way is we can use a bit mask in map attributes argument in DSP Proc Map function to enforce the check on the buffer. What are the reasons as why you want to make it all-or-nothing option ? Thank you, Best regards, Hari > -----Original Message----- > From: Felipe Contreras [mailto:felipe.contreras@gmail.com] > Sent: Monday, June 08, 2009 4:19 PM > To: Kanigeri, Hari > Cc: Hiroshi DOYU; Ramirez Luna, Omar; > ameya.palande@nokia.com; linux-omap@vger.kernel.org > Subject: Re: [PATCH] DSPBRIDGE: Compiler wanrning fix for > unlocked_ioctl > > On Mon, Jun 8, 2009 at 11:49 PM, Kanigeri, > Hari<h-kanigeri2@ti.com> wrote: > > Hi Doyu-san, > > > > Regarding > > > >> http://marc.info/?l=linux-omap&m=124201773423892&w=2 > >> I think that the first one should be merged into d.o-z.org > now, but > >> for the second one about 128 byte alignment. let me know your > >> thought/plan on it. > > > > -- I think you sent this patch as a probable fix for the > slab corruption that was observed in Bridge driver, but then > we found that slab corruption was due to some other issue in > Bridge driver and not due to the cache alignment. > > > > Irrespective of above point, I think it is good to enforce > the cache alignment check, but I think the check should be in > Proc Map function and to start with the check should be under > a flag so as not to affect some MM applications that use > padding to get over the alignment issue. > > I agree, the check should be in proc map, and should be optional. > However, I would prefer it to be an all-or-nothing option, > how about in kconfig? > > -- > Felipe Contreras > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 9, 2009 at 4:18 AM, Kanigeri, Hari<h-kanigeri2@ti.com> wrote: >> I agree, the check should be in proc map, and should be optional. >> However, I would prefer it to be an all-or-nothing option, >> how about in kconfig? >> > > -- One other way is we can use a bit mask in map attributes argument in DSP Proc Map function to enforce the check on the buffer. > > What are the reasons as why you want to make it all-or-nothing option ? The objective is to be more strict to give less power to user-space to screw up the system, an argument in proc map still lets user-space to screw up.
On Tue, Jun 9, 2009 at 10:02 AM, Hiroshi DOYU<Hiroshi.DOYU@nokia.com> wrote: > From: "ext Kanigeri, Hari" <h-kanigeri2@ti.com> > Subject: RE: [PATCH] DSPBRIDGE: Compiler wanrning fix for unlocked_ioctl > Date: Mon, 8 Jun 2009 22:49:21 +0200 > >> Hi Doyu-san, >> >> Regarding >> >> > http://marc.info/?l=linux-omap&m=124201773423892&w=2 >> > I think that the first one should be merged into d.o-z.org now, but >> > for the second one about 128 byte alignment. let me know your >> > thought/plan on it. >> >> -- I think you sent this patch as a probable fix for the slab >> corruption that was observed in Bridge driver, but then we found >> that slab corruption was due to some other issue in Bridge driver >> and not due to the cache alignment. >> >> Irrespective of above point, I think it is good to enforce the cache >> alignment check, but I think the check should be in Proc Map >> function and to start with the check should be under a flag so as >> not to affect some MM applications that use padding to get over the >> alignment issue. > > I think that having configurable option may be reasonable practically, > at the moment. > > But how about the longer term solution? Do you have any plan on how to > deal with this? (ex: TI's OMX layer and some other userland client) Do > you continue the userland buffer padding solution for the futer > release? I don't know about TI's OMX layer, but I'm working on some direct GStreamer wrappers that already do the proper alignment. My plan currently is to keep working on gst-dsp until we have all the elements we need. After that's done we will be able to turn on the check in the kernel. Then, if I have time I might port the changes to TI's omx il. Cheers.
On Tue, Jun 9, 2009 at 12:29 PM, Hiroshi DOYU<Hiroshi.DOYU@nokia.com> wrote: > From: ext Felipe Contreras <felipe.contreras@gmail.com> > Subject: Re: [PATCH] DSPBRIDGE: Compiler wanrning fix for unlocked_ioctl > Date: Tue, 9 Jun 2009 11:16:52 +0200 > >> On Tue, Jun 9, 2009 at 10:02 AM, Hiroshi DOYU<Hiroshi.DOYU@nokia.com> wrote: >> > From: "ext Kanigeri, Hari" <h-kanigeri2@ti.com> >> > Subject: RE: [PATCH] DSPBRIDGE: Compiler wanrning fix for unlocked_ioctl >> > Date: Mon, 8 Jun 2009 22:49:21 +0200 >> > >> >> Hi Doyu-san, >> >> >> >> Regarding >> >> >> >> > http://marc.info/?l=linux-omap&m=124201773423892&w=2 >> >> > I think that the first one should be merged into d.o-z.org now, but >> >> > for the second one about 128 byte alignment. let me know your >> >> > thought/plan on it. >> >> >> >> -- I think you sent this patch as a probable fix for the slab >> >> corruption that was observed in Bridge driver, but then we found >> >> that slab corruption was due to some other issue in Bridge driver >> >> and not due to the cache alignment. >> >> >> >> Irrespective of above point, I think it is good to enforce the cache >> >> alignment check, but I think the check should be in Proc Map >> >> function and to start with the check should be under a flag so as >> >> not to affect some MM applications that use padding to get over the >> >> alignment issue. >> > >> > I think that having configurable option may be reasonable practically, >> > at the moment. >> > >> > But how about the longer term solution? Do you have any plan on how to >> > deal with this? (ex: TI's OMX layer and some other userland client) Do >> > you continue the userland buffer padding solution for the futer >> > release? >> >> I don't know about TI's OMX layer, but I'm working on some direct >> GStreamer wrappers that already do the proper alignment. > > I expect that it will bring huge performance benfit. You mean because of the alignment or because of the implementation? In any case, you are correct :) >> My plan currently is to keep working on gst-dsp until we have all the >> elements we need. After that's done we will be able to turn on the >> check in the kernel. >> >> Then, if I have time I might port the changes to TI's omx il. > > Both would be necessary for real products. IMHO much more would be necessary for real products. Right now the DSP SW can read/write any location of memory (security risk?), I think all the memory should be protected and then the bridgedriver should give the DSP permissions on certain memory areas.
diff --git a/drivers/dsp/bridge/rmgr/drv_interface.c b/drivers/dsp/bridge/rmgr/drv_interface.c index c3d8a02..f41e153 100644 --- a/drivers/dsp/bridge/rmgr/drv_interface.c +++ b/drivers/dsp/bridge/rmgr/drv_interface.c @@ -665,7 +665,7 @@ static int bridge_release(struct inode *ip, struct file *filp) } /* This function provides IO interface to the bridge driver. */ -static int bridge_ioctl(struct file *filp, unsigned int code, +static long bridge_ioctl(struct file *filp, unsigned int code, unsigned long args) { int status; diff --git a/drivers/dsp/bridge/rmgr/drv_interface.h b/drivers/dsp/bridge/rmgr/drv_interface.h index 3e77ed0..dc49210 100644 --- a/drivers/dsp/bridge/rmgr/drv_interface.h +++ b/drivers/dsp/bridge/rmgr/drv_interface.h @@ -34,7 +34,7 @@ static int __init bridge_init(void); /* Initialize bridge */ static void __exit bridge_exit(void); /* Opposite of initialize */ static int bridge_open(struct inode *, struct file *); /* Open */ static int bridge_release(struct inode *, struct file *); /* Release */ -static int bridge_ioctl(struct file *, unsigned int, +static long bridge_ioctl(struct file *, unsigned int, unsigned long); static int bridge_mmap(struct file *filp, struct vm_area_struct *vma); #endif /* ifndef _DRV_INTERFACE_H_ */