| Message ID | 1418564120-19523-1-git-send-email-oded.gabbay@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 14.12.2014 um 14:35 schrieb Oded Gabbay: > When an ioctl function returns -EAGAIN, don't print error in kfd_ioctl() You most likely want to handle -ERESTARTSYS the same way. Christian. > > Signed-off-by: Oded Gabbay <oded.gabbay@amd.com> > --- > drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > index 7d4974b..69c5fe7 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > @@ -571,7 +571,7 @@ static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) > break; > } > > - if (err < 0) > + if ((err < 0) && (err != -EAGAIN)) > dev_err(kfd_device, > "ioctl error %ld for ioctl cmd 0x%x (#%d)\n", > err, cmd, _IOC_NR(cmd));
On 12/14/2014 04:10 PM, Christian König wrote: > Am 14.12.2014 um 14:35 schrieb Oded Gabbay: >> When an ioctl function returns -EAGAIN, don't print error in kfd_ioctl() > > You most likely want to handle -ERESTARTSYS the same way. > > Christian. Thanks, will fix and resend. Oded > >> >> Signed-off-by: Oded Gabbay <oded.gabbay@amd.com> >> --- >> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >> index 7d4974b..69c5fe7 100644 >> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >> @@ -571,7 +571,7 @@ static long kfd_ioctl(struct file *filep, unsigned int >> cmd, unsigned long arg) >> break; >> } >> - if (err < 0) >> + if ((err < 0) && (err != -EAGAIN)) >> dev_err(kfd_device, >> "ioctl error %ld for ioctl cmd 0x%x (#%d)\n", >> err, cmd, _IOC_NR(cmd)); >
On Sun, Dec 14, 2014 at 03:10:17PM +0100, Christian König wrote: > Am 14.12.2014 um 14:35 schrieb Oded Gabbay: > >When an ioctl function returns -EAGAIN, don't print error in kfd_ioctl() > > You most likely want to handle -ERESTARTSYS the same way. Please just reuse drmIoctl or at least copy it perfectly. We've had too many tears about ioctl restarting going badly wrong. Also make sure you never do a raw ioctl call anywhere for amdkfd. Adding Dave. Thanks, Daniel
On 15 December 2014 at 17:59, Daniel Vetter <daniel@ffwll.ch> wrote: > On Sun, Dec 14, 2014 at 03:10:17PM +0100, Christian König wrote: >> Am 14.12.2014 um 14:35 schrieb Oded Gabbay: >> >When an ioctl function returns -EAGAIN, don't print error in kfd_ioctl() >> >> You most likely want to handle -ERESTARTSYS the same way. > > Please just reuse drmIoctl or at least copy it perfectly. We've had too > many tears about ioctl restarting going badly wrong. Also make sure you > never do a raw ioctl call anywhere for amdkfd. Adding Dave. Also please don't make a user triggerable printk. If the user can throw crap at the ioctl and get msgs in dmesg, then its annoying as hell. Copy the drm.debug stuff and code as well, and for userspace, yes do what Daniel says and use drmIoctl wrapper or something like that, though Daniel I believe one of the main consumers on i915 insists on opencoding his ioctls. Dave.
On 12/15/2014 10:32 AM, Dave Airlie wrote: > On 15 December 2014 at 17:59, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Sun, Dec 14, 2014 at 03:10:17PM +0100, Christian König wrote: >>> Am 14.12.2014 um 14:35 schrieb Oded Gabbay: >>>> When an ioctl function returns -EAGAIN, don't print error in kfd_ioctl() >>> >>> You most likely want to handle -ERESTARTSYS the same way. >> >> Please just reuse drmIoctl or at least copy it perfectly. We've had too >> many tears about ioctl restarting going badly wrong. Also make sure you >> never do a raw ioctl call anywhere for amdkfd. Adding Dave. > > Also please don't make a user triggerable printk. > > If the user can throw crap at the ioctl and get msgs in dmesg, > then its annoying as hell. > > Copy the drm.debug stuff and code as well, and for userspace, > yes do what Daniel says and use drmIoctl wrapper or something like > that, though Daniel I believe one of the main consumers on i915 > insists on opencoding his ioctls. > > Dave. > Dave & Daniel, Thanks for the guidance & comments. I will take a look at drmioctl & drm.debug stuff and copy/reuse what's relevant. After that, I'll resubmit this patch, and probably this will generate some more patches for me to submit for review. Oded
On 12/15/2014 10:32 AM, Dave Airlie wrote: > On 15 December 2014 at 17:59, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Sun, Dec 14, 2014 at 03:10:17PM +0100, Christian König wrote: >>> Am 14.12.2014 um 14:35 schrieb Oded Gabbay: >>>> When an ioctl function returns -EAGAIN, don't print error in kfd_ioctl() >>> >>> You most likely want to handle -ERESTARTSYS the same way. >> >> Please just reuse drmIoctl or at least copy it perfectly. We've had too >> many tears about ioctl restarting going badly wrong. Also make sure you >> never do a raw ioctl call anywhere for amdkfd. Adding Dave. > > Also please don't make a user triggerable printk. > > If the user can throw crap at the ioctl and get msgs in dmesg, > then its annoying as hell. > > Copy the drm.debug stuff and code as well, and for userspace, > yes do what Daniel says and use drmIoctl wrapper or something like > that, though Daniel I believe one of the main consumers on i915 > insists on opencoding his ioctls. > > Dave. > Hi Dave, Daniel I just sent a patch-set that copies the drm_ioctl() handling to kfd_ioctl(), as you requested. All error prints have been converted to debug prints. This is the first part and I'm now going to change the userspace as well. Oded
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c index 7d4974b..69c5fe7 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c @@ -571,7 +571,7 @@ static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg) break; } - if (err < 0) + if ((err < 0) && (err != -EAGAIN)) dev_err(kfd_device, "ioctl error %ld for ioctl cmd 0x%x (#%d)\n", err, cmd, _IOC_NR(cmd));
When an ioctl function returns -EAGAIN, don't print error in kfd_ioctl() Signed-off-by: Oded Gabbay <oded.gabbay@amd.com> --- drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)