* -EFAULT error code at firedtv driver @ 2011-07-06 16:04 Mauro Carvalho Chehab 2011-07-06 17:33 ` [PATCH] [media] firedtv: change some -EFAULT returns to more fitting error codes Stefan Richter 0 siblings, 1 reply; 4+ messages in thread From: Mauro Carvalho Chehab @ 2011-07-06 16:04 UTC (permalink / raw) To: Stefan Richter, Linux Media Mailing List Hi Stefan, I'm validating if all drivers are behaving equally with respect to the error codes returned to userspace, and double-checking with the API. On almost all places, -EFAULT code is used only to indicate when copy_from_user/copy_to_user fails. However, firedtv uses a lot of -EFAULT, where it seems to me that other error codes should be used instead (like -EIO for bus transfer errors and -EINVAL/-ERANGE for invalid/out of range parameters). I'll be posting soon a series of patches fixing the error codes on the other places, but, as I don't know how do you use -EFAULT inside the firewire core, I prefer to not touch at firedtv. Could you please take a look on it? Thanks! Mauro ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] [media] firedtv: change some -EFAULT returns to more fitting error codes 2011-07-06 16:04 -EFAULT error code at firedtv driver Mauro Carvalho Chehab @ 2011-07-06 17:33 ` Stefan Richter 2011-07-06 18:45 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 4+ messages in thread From: Stefan Richter @ 2011-07-06 17:33 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: linux-media, Henrik Kurelid Mauro Carvalho Chehab wrote: > I'm validating if all drivers are behaving equally with respect to the > error codes returned to userspace, and double-checking with the API. > > On almost all places, -EFAULT code is used only to indicate when > copy_from_user/copy_to_user fails. However, firedtv uses a lot of > -EFAULT, where it seems to me that other error codes should be used > instead (like -EIO for bus transfer errors and -EINVAL/-ERANGE for > invalid/out of range parameters). This concerns only the CI (CAM) related code of firedtv of which I know little. Let's just pass through the error returns of lower level I/O code where applicable, and -EACCES (permission denied) when a seemingly valid but negative FCP response or an unknown-to-firedtv CA message is received. Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de> Cc: Henrik Kurelid <henrik@kurelid.se> --- Only tested on FireDTV-{C,T}/CI without CAM. drivers/media/dvb/firewire/firedtv-avc.c | 2 drivers/media/dvb/firewire/firedtv-ci.c | 34 +++++++++++------------ 2 files changed, 17 insertions(+), 19 deletions(-) Index: b/drivers/media/dvb/firewire/firedtv-avc.c =================================================================== --- a/drivers/media/dvb/firewire/firedtv-avc.c +++ b/drivers/media/dvb/firewire/firedtv-avc.c @@ -1208,7 +1208,7 @@ int avc_ca_pmt(struct firedtv *fdtv, cha if (r->response != AVC_RESPONSE_ACCEPTED) { dev_err(fdtv->device, "CA PMT failed with response 0x%x\n", r->response); - ret = -EFAULT; + ret = -EACCES; } out: mutex_unlock(&fdtv->avc_mutex); Index: b/drivers/media/dvb/firewire/firedtv-ci.c =================================================================== --- a/drivers/media/dvb/firewire/firedtv-ci.c +++ b/drivers/media/dvb/firewire/firedtv-ci.c @@ -45,11 +45,6 @@ static int fdtv_get_ca_flags(struct fire return flags; } -static int fdtv_ca_reset(struct firedtv *fdtv) -{ - return avc_ca_reset(fdtv) ? -EFAULT : 0; -} - static int fdtv_ca_get_caps(void *arg) { struct ca_caps *cap = arg; @@ -65,12 +60,14 @@ static int fdtv_ca_get_slot_info(struct { struct firedtv_tuner_status stat; struct ca_slot_info *slot = arg; + int err; - if (avc_tuner_status(fdtv, &stat)) - return -EFAULT; + err = avc_tuner_status(fdtv, &stat); + if (err) + return err; if (slot->num != 0) - return -EFAULT; + return -EACCES; slot->type = CA_CI; slot->flags = fdtv_get_ca_flags(&stat); @@ -81,21 +78,21 @@ static int fdtv_ca_app_info(struct fired { struct ca_msg *reply = arg; - return avc_ca_app_info(fdtv, reply->msg, &reply->length) ? -EFAULT : 0; + return avc_ca_app_info(fdtv, reply->msg, &reply->length); } static int fdtv_ca_info(struct firedtv *fdtv, void *arg) { struct ca_msg *reply = arg; - return avc_ca_info(fdtv, reply->msg, &reply->length) ? -EFAULT : 0; + return avc_ca_info(fdtv, reply->msg, &reply->length); } static int fdtv_ca_get_mmi(struct firedtv *fdtv, void *arg) { struct ca_msg *reply = arg; - return avc_ca_get_mmi(fdtv, reply->msg, &reply->length) ? -EFAULT : 0; + return avc_ca_get_mmi(fdtv, reply->msg, &reply->length); } static int fdtv_ca_get_msg(struct firedtv *fdtv, void *arg) @@ -111,14 +108,15 @@ static int fdtv_ca_get_msg(struct firedt err = fdtv_ca_info(fdtv, arg); break; default: - if (avc_tuner_status(fdtv, &stat)) - err = -EFAULT; - else if (stat.ca_mmi == 1) + err = avc_tuner_status(fdtv, &stat); + if (err) + break; + if (stat.ca_mmi == 1) err = fdtv_ca_get_mmi(fdtv, arg); else { dev_info(fdtv->device, "unhandled CA message 0x%08x\n", fdtv->ca_last_command); - err = -EFAULT; + err = -EACCES; } } fdtv->ca_last_command = 0; @@ -141,7 +139,7 @@ static int fdtv_ca_pmt(struct firedtv *f data_length = msg->msg[3]; } - return avc_ca_pmt(fdtv, &msg->msg[data_pos], data_length) ? -EFAULT : 0; + return avc_ca_pmt(fdtv, &msg->msg[data_pos], data_length); } static int fdtv_ca_send_msg(struct firedtv *fdtv, void *arg) @@ -170,7 +168,7 @@ static int fdtv_ca_send_msg(struct fired default: dev_err(fdtv->device, "unhandled CA message 0x%08x\n", fdtv->ca_last_command); - err = -EFAULT; + err = -EACCES; } return err; } @@ -184,7 +182,7 @@ static int fdtv_ca_ioctl(struct file *fi switch (cmd) { case CA_RESET: - err = fdtv_ca_reset(fdtv); + err = avc_ca_reset(fdtv); break; case CA_GET_CAP: err = fdtv_ca_get_caps(arg); -- Stefan Richter -=====-==-== -=== --==- http://arcgraph.de/sr/ ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] [media] firedtv: change some -EFAULT returns to more fitting error codes 2011-07-06 17:33 ` [PATCH] [media] firedtv: change some -EFAULT returns to more fitting error codes Stefan Richter @ 2011-07-06 18:45 ` Mauro Carvalho Chehab 2011-07-06 18:54 ` [PATCH resend] " Stefan Richter 0 siblings, 1 reply; 4+ messages in thread From: Mauro Carvalho Chehab @ 2011-07-06 18:45 UTC (permalink / raw) To: Stefan Richter; +Cc: linux-media, Henrik Kurelid Hi Stefan, Em 06-07-2011 14:33, Stefan Richter escreveu: > Mauro Carvalho Chehab wrote: >> I'm validating if all drivers are behaving equally with respect to the >> error codes returned to userspace, and double-checking with the API. >> >> On almost all places, -EFAULT code is used only to indicate when >> copy_from_user/copy_to_user fails. However, firedtv uses a lot of >> -EFAULT, where it seems to me that other error codes should be used >> instead (like -EIO for bus transfer errors and -EINVAL/-ERANGE for >> invalid/out of range parameters). Thanks for the patch! Unfortunately, it didn't arrived fine here (it was whitespace mangled). Could you please re-send it with the proper whitespacing (or attached)? > > This concerns only the CI (CAM) related code of firedtv of which I know > little. Let's just pass through the error returns of lower level I/O > code where applicable, and -EACCES (permission denied) when a seemingly > valid but negative FCP response or an unknown-to-firedtv CA message is > received. Works for me. > > Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de> > Cc: Henrik Kurelid <henrik@kurelid.se> > --- > Only tested on FireDTV-{C,T}/CI without CAM. > > drivers/media/dvb/firewire/firedtv-avc.c | 2 > drivers/media/dvb/firewire/firedtv-ci.c | 34 +++++++++++------------ > 2 files changed, 17 insertions(+), 19 deletions(-) > > Index: b/drivers/media/dvb/firewire/firedtv-avc.c > =================================================================== > --- a/drivers/media/dvb/firewire/firedtv-avc.c > +++ b/drivers/media/dvb/firewire/firedtv-avc.c > @@ -1208,7 +1208,7 @@ int avc_ca_pmt(struct firedtv *fdtv, cha > if (r->response != AVC_RESPONSE_ACCEPTED) { > dev_err(fdtv->device, > "CA PMT failed with response 0x%x\n", > r->response); > - ret = -EFAULT; > + ret = -EACCES; > } > out: > mutex_unlock(&fdtv->avc_mutex); > Index: b/drivers/media/dvb/firewire/firedtv-ci.c > =================================================================== > --- a/drivers/media/dvb/firewire/firedtv-ci.c > +++ b/drivers/media/dvb/firewire/firedtv-ci.c > @@ -45,11 +45,6 @@ static int fdtv_get_ca_flags(struct fire > return flags; > } > > -static int fdtv_ca_reset(struct firedtv *fdtv) > -{ > - return avc_ca_reset(fdtv) ? -EFAULT : 0; > -} > - > static int fdtv_ca_get_caps(void *arg) > { > struct ca_caps *cap = arg; > @@ -65,12 +60,14 @@ static int fdtv_ca_get_slot_info(struct > { > struct firedtv_tuner_status stat; > struct ca_slot_info *slot = arg; > + int err; > > - if (avc_tuner_status(fdtv, &stat)) > - return -EFAULT; > + err = avc_tuner_status(fdtv, &stat); > + if (err) > + return err; > > if (slot->num != 0) > - return -EFAULT; > + return -EACCES; > > slot->type = CA_CI; > slot->flags = fdtv_get_ca_flags(&stat); > @@ -81,21 +78,21 @@ static int fdtv_ca_app_info(struct fired > { > struct ca_msg *reply = arg; > > - return avc_ca_app_info(fdtv, reply->msg, &reply->length) ? > -EFAULT : 0; > + return avc_ca_app_info(fdtv, reply->msg, &reply->length); > } > > static int fdtv_ca_info(struct firedtv *fdtv, void *arg) > { > struct ca_msg *reply = arg; > > - return avc_ca_info(fdtv, reply->msg, &reply->length) ? -EFAULT : > 0; > + return avc_ca_info(fdtv, reply->msg, &reply->length); > } > > static int fdtv_ca_get_mmi(struct firedtv *fdtv, void *arg) > { > struct ca_msg *reply = arg; > > - return avc_ca_get_mmi(fdtv, reply->msg, &reply->length) ? > -EFAULT : 0; > + return avc_ca_get_mmi(fdtv, reply->msg, &reply->length); > } > > static int fdtv_ca_get_msg(struct firedtv *fdtv, void *arg) > @@ -111,14 +108,15 @@ static int fdtv_ca_get_msg(struct firedt > err = fdtv_ca_info(fdtv, arg); > break; > default: > - if (avc_tuner_status(fdtv, &stat)) > - err = -EFAULT; > - else if (stat.ca_mmi == 1) > + err = avc_tuner_status(fdtv, &stat); > + if (err) > + break; > + if (stat.ca_mmi == 1) > err = fdtv_ca_get_mmi(fdtv, arg); > else { > dev_info(fdtv->device, "unhandled CA message > 0x%08x\n", fdtv->ca_last_command); > - err = -EFAULT; > + err = -EACCES; > } > } > fdtv->ca_last_command = 0; > @@ -141,7 +139,7 @@ static int fdtv_ca_pmt(struct firedtv *f > data_length = msg->msg[3]; > } > > - return avc_ca_pmt(fdtv, &msg->msg[data_pos], data_length) ? > -EFAULT : 0; > + return avc_ca_pmt(fdtv, &msg->msg[data_pos], data_length); > } > > static int fdtv_ca_send_msg(struct firedtv *fdtv, void *arg) > @@ -170,7 +168,7 @@ static int fdtv_ca_send_msg(struct fired > default: > dev_err(fdtv->device, "unhandled CA message 0x%08x\n", > fdtv->ca_last_command); > - err = -EFAULT; > + err = -EACCES; > } > return err; > } > @@ -184,7 +182,7 @@ static int fdtv_ca_ioctl(struct file *fi > > switch (cmd) { > case CA_RESET: > - err = fdtv_ca_reset(fdtv); > + err = avc_ca_reset(fdtv); > break; > case CA_GET_CAP: > err = fdtv_ca_get_caps(arg); > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH resend] [media] firedtv: change some -EFAULT returns to more fitting error codes 2011-07-06 18:45 ` Mauro Carvalho Chehab @ 2011-07-06 18:54 ` Stefan Richter 0 siblings, 0 replies; 4+ messages in thread From: Stefan Richter @ 2011-07-06 18:54 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: linux-media, Henrik Kurelid Mauro Carvalho Chehab wrote: > I'm validating if all drivers are behaving equally with respect to the > error codes returned to userspace, and double-checking with the API. > > On almost all places, -EFAULT code is used only to indicate when > copy_from_user/copy_to_user fails. However, firedtv uses a lot of > -EFAULT, where it seems to me that other error codes should be used > instead (like -EIO for bus transfer errors and -EINVAL/-ERANGE for > invalid/out of range parameters). This concerns only the CI (CAM) related code of firedtv of which I know little. Let's just pass through the error returns of lower level I/O code where applicable, and -EACCES (permission denied) when a seemingly valid but negative FCP response or an unknown-to-firedtv CA message is received. Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de> Cc: Henrik Kurelid <henrik@kurelid.se> --- Resent, hopefully without linewrap breakage this time. drivers/media/dvb/firewire/firedtv-avc.c | 2 drivers/media/dvb/firewire/firedtv-ci.c | 34 +++++++++++------------ 2 files changed, 17 insertions(+), 19 deletions(-) Index: b/drivers/media/dvb/firewire/firedtv-avc.c =================================================================== --- a/drivers/media/dvb/firewire/firedtv-avc.c +++ b/drivers/media/dvb/firewire/firedtv-avc.c @@ -1208,7 +1208,7 @@ int avc_ca_pmt(struct firedtv *fdtv, cha if (r->response != AVC_RESPONSE_ACCEPTED) { dev_err(fdtv->device, "CA PMT failed with response 0x%x\n", r->response); - ret = -EFAULT; + ret = -EACCES; } out: mutex_unlock(&fdtv->avc_mutex); Index: b/drivers/media/dvb/firewire/firedtv-ci.c =================================================================== --- a/drivers/media/dvb/firewire/firedtv-ci.c +++ b/drivers/media/dvb/firewire/firedtv-ci.c @@ -45,11 +45,6 @@ static int fdtv_get_ca_flags(struct fire return flags; } -static int fdtv_ca_reset(struct firedtv *fdtv) -{ - return avc_ca_reset(fdtv) ? -EFAULT : 0; -} - static int fdtv_ca_get_caps(void *arg) { struct ca_caps *cap = arg; @@ -65,12 +60,14 @@ static int fdtv_ca_get_slot_info(struct { struct firedtv_tuner_status stat; struct ca_slot_info *slot = arg; + int err; - if (avc_tuner_status(fdtv, &stat)) - return -EFAULT; + err = avc_tuner_status(fdtv, &stat); + if (err) + return err; if (slot->num != 0) - return -EFAULT; + return -EACCES; slot->type = CA_CI; slot->flags = fdtv_get_ca_flags(&stat); @@ -81,21 +78,21 @@ static int fdtv_ca_app_info(struct fired { struct ca_msg *reply = arg; - return avc_ca_app_info(fdtv, reply->msg, &reply->length) ? -EFAULT : 0; + return avc_ca_app_info(fdtv, reply->msg, &reply->length); } static int fdtv_ca_info(struct firedtv *fdtv, void *arg) { struct ca_msg *reply = arg; - return avc_ca_info(fdtv, reply->msg, &reply->length) ? -EFAULT : 0; + return avc_ca_info(fdtv, reply->msg, &reply->length); } static int fdtv_ca_get_mmi(struct firedtv *fdtv, void *arg) { struct ca_msg *reply = arg; - return avc_ca_get_mmi(fdtv, reply->msg, &reply->length) ? -EFAULT : 0; + return avc_ca_get_mmi(fdtv, reply->msg, &reply->length); } static int fdtv_ca_get_msg(struct firedtv *fdtv, void *arg) @@ -111,14 +108,15 @@ static int fdtv_ca_get_msg(struct firedt err = fdtv_ca_info(fdtv, arg); break; default: - if (avc_tuner_status(fdtv, &stat)) - err = -EFAULT; - else if (stat.ca_mmi == 1) + err = avc_tuner_status(fdtv, &stat); + if (err) + break; + if (stat.ca_mmi == 1) err = fdtv_ca_get_mmi(fdtv, arg); else { dev_info(fdtv->device, "unhandled CA message 0x%08x\n", fdtv->ca_last_command); - err = -EFAULT; + err = -EACCES; } } fdtv->ca_last_command = 0; @@ -141,7 +139,7 @@ static int fdtv_ca_pmt(struct firedtv *f data_length = msg->msg[3]; } - return avc_ca_pmt(fdtv, &msg->msg[data_pos], data_length) ? -EFAULT : 0; + return avc_ca_pmt(fdtv, &msg->msg[data_pos], data_length); } static int fdtv_ca_send_msg(struct firedtv *fdtv, void *arg) @@ -170,7 +168,7 @@ static int fdtv_ca_send_msg(struct fired default: dev_err(fdtv->device, "unhandled CA message 0x%08x\n", fdtv->ca_last_command); - err = -EFAULT; + err = -EACCES; } return err; } @@ -184,7 +182,7 @@ static int fdtv_ca_ioctl(struct file *fi switch (cmd) { case CA_RESET: - err = fdtv_ca_reset(fdtv); + err = avc_ca_reset(fdtv); break; case CA_GET_CAP: err = fdtv_ca_get_caps(arg); -- Stefan Richter -=====-==-== -=== --==- http://arcgraph.de/sr/ ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-07-06 18:54 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-07-06 16:04 -EFAULT error code at firedtv driver Mauro Carvalho Chehab 2011-07-06 17:33 ` [PATCH] [media] firedtv: change some -EFAULT returns to more fitting error codes Stefan Richter 2011-07-06 18:45 ` Mauro Carvalho Chehab 2011-07-06 18:54 ` [PATCH resend] " Stefan Richter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox