Re: [PATCH v3] backlight: report error on failure

[Daniel Thompson]

On 09/05/17 15:56, Sudip Mukherjee wrote:
> On Mon, May 08, 2017 at 04:45:17PM +0100, Daniel Thompson wrote:
> > On 06/05/17 19:00, Sudip Mukherjee wrote:
> > > It is possible to update the backlight power and the brightness using
> > > the sysfs and on writing it either returns the count or if the callback
> > > function does not exist then returns the error code 'ENXIO'.
> > >
> > > We have a situation where the userspace client is writing to the sysfs
> > > to update the power and since the callback function exists the client
> > > receives the return value as count and considers the operation to be
> > > successful. That is correct as the write to the sysfs was successful.
> > > But there is no way to know if the actual operation was done or not.
> > >
> > > backlight_update_status() returns the error code if it fails. Pass that
> > > to the userspace client who is trying to update the power so that the
> > > client knows that the operation failed.
> > >
> > > This is not a change of ABI as the userspace expects an error of ENXIO,
> > > after this patch the range of errors that are returned to the userspace
> > > will increase.
> >
> > This comment is wrong, no code path through
> > backlight_device_set_brightness() can possibly return ENXIO.
>
> I am seeing backlight_device_set_brightness() can return ENXIO
> if bd->ops is NULL. ofcourse I have not tried to test by passing NULL as
> backlight_ops in backlight_device_register().
>
> > My review comment to v1 was:
> > > Strictly speaking this is an ABI change. Its probably a harmless one
> > > making it ok to change but I'm interested what testing or code review
> > > you've done to be sure the userspace doesn't do odd things if the
> > > kernel starts to pass through errors.
> >
> > I find myself somewhat surprised to find the above review comment addressed
> > by adding text to the patch header denying that there is a change of ABI...
>
> Yes, sorry about this. I got confused between API and ABI. :(
>
> So, this is an ABI change (not API change, as I misunderstood) as now
> the userspace might get some more error codes as return which it was not
> expecting.
> How will you want me to test and review it? I can make a list of the
> other drivers which are registering the backlight and review what they
> are doing if there is an error in the backlight or brightness. And then
> we can have a statistics how many of the drivers will be returning extra
> error codes. I have been seeing few drivers and i noticed all of them
> are just returning 0 at the end.

I really did just wonder what you had already done!

Since yesterday I've done a quick code review of Weston, KDE and 
gnome-settings-daemon and saw no particular grounds to worry. Mostly the 
userspace sets sysfs from layers of IPC mechanisms so there's loads of 
ways it can report failure... I doubt one more will hurt ;-)

Let's just get this comment removed and that's probably enough!


Daniel.