On Fri, 2021-04-23 at 13:03 +0200, Stefan Wahren wrote:
> Am 23.04.21 um 11:59 schrieb nicolas saenz julienne:
> > On Thu, 2021-04-22 at 22:07 +0200, Stefan Wahren wrote:
> > > Replace the custom set of return values with proper Linux error codes for
> > > vchiq_set_service_option(). Now we can use the result directly as return
> > > value for vchiq_ioctl().
> > >
> > > Signed-off-by: Stefan Wahren <stefan.wahren@xxxxxxxx>
> > > ---
> > > .../vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 ++--
> > > .../vc04_services/interface/vchiq_arm/vchiq_core.c | 18 +++++++++---------
> > > .../vc04_services/interface/vchiq_arm/vchiq_core.h | 2 +-
> > > 3 files changed, 12 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > index 3395201..ab2ce07 100644
> > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > @@ -1517,8 +1517,8 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > > break;
> > > }
> > >
> > >
> > > - status = vchiq_set_service_option(
> > > - args.handle, args.option, args.value);
> > > + ret = vchiq_set_service_option(args.handle, args.option,
> > > + args.value);
> > > } break;
> > >
> > >
> > > case VCHIQ_IOC_LIB_VERSION: {
> > > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> > > index 9f9677a..31c1c1a 100644
> > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
> > > @@ -3371,21 +3371,21 @@ void vchiq_get_config(struct vchiq_config *config)
> > > config->version_min = VCHIQ_VERSION_MIN;
> > > }
> > >
> > >
> > > -enum vchiq_status
> > > +int
> > > vchiq_set_service_option(unsigned int handle,
> > > enum vchiq_service_option option, int value)
> > > {
> > > struct vchiq_service *service = find_service_by_handle(handle);
> > > - enum vchiq_status status = VCHIQ_ERROR;
> > > struct vchiq_service_quota *quota;
> > > + int ret = -EIO;
> > Due to the nature of the driver it's hard to make a clear distinction, but is
> > -EIO really the best error for the function? I don't see error paths that
> > really depend on communication with VC4. I see it more aligned with -EINVAL.
>
> In general i totally agree with you. I just wanted to the keep the
> o(l)dd behavior, because vchiq_ioctl() has a catch all for VCHIQ_ERROR
> -> -EIO
>
> I'm not sure if any user application relies on the error code of
> vchiq_ioctl(). Otherwise this isn't argument for all the other patches ;-)
This is something that bugged me the last time I tried to do a vchiq cleanup.
At some point we'll start affecting the IOCTL interface in subtle ways. It's a
PITA as we don't really know if there are users out there that wrote their own
VCHIQ applications. RPi ones we could fix.
I remember thinking that the best course of action would be to completely
decouple the core part from the char device. As in, for it to live under a
completely different directory, and registered the way we do it with the camera
interface. We'd get a way more concise VCHIQ core. But IIRC there are lots of
quirks and hacks to accomodate to the IOCTl behavior we'd have to iron out.
Regards,
Nicolas
