On 01/30/2015 05:40 PM, Greg KH wrote:
> On Fri, Jan 30, 2015 at 09:55:30AM +0100, Jacek Anaszewski wrote:
>> Hi Pavel,
>>
>> On 01/29/2015 10:14 PM, Pavel Machek wrote:
>>> Hi!
>>>
>>>>>> + - flash_fault - list of flash faults that may have occurred:
>>>>>> + * led-over-voltage - flash controller voltage to the flash LED
>>>>>> + has exceededthe limit specific to the flash controller
>>>>>> + * flash-timeout-exceeded - the flash strobe was still on when
>>>>>> + the timeout set by the user has expired; not all flash
>>>>>> + controllers may set this in all such conditions
>>>>>> + * controller-over-temperature - the flash controller has
>>>>>> + overheated
>>>>>> + * controller-short-circuit - the short circuit protection
>>>>>> + of the flash controller has been triggered
>>>>>> + * led-power-supply-over-current - current in the LED power
>>>>>> + supply has exceeded the limit specific to the flash
>>>>>> + controller
>>>>>> + * indicator-led-fault - the flash controller has detected
>>>>>> + a short or open circuit condition on the indicator LED
>>>>>> + * led-under-voltage - flash controller voltage to the flash
>>>>>> + LED has been below the minimum limit specific to
>>>>>> + the flash
>>>>>> + * controller-under-voltage - the input voltage of the flash
>>>>>> + controller is below the limit under which strobing the
>>>>>> + flash at full current will not be possible. The condition
>>>>>> + persists until this flag is no longer set
>>>>>> + * led-over-temperature - the temperature of the LED has exceeded
>>>>>> + its allowed upper limit
>>>>>> +
>>>>>> + Flash faults are cleared, if possible, by reading the attribute.
>>>>>
>>>>> That's bad. Now you can no longer present flash_fault file as readable
>>>>> to non-root users, and grep -ri foo /sys will interfere with your
>>>>> camera application.
>>>>>
>>>>> Bad interface, just fix it.
>>>>
>>>> In my opinion it isn't crucial for the user to be aware of the
>>>> fact that some non-persistent fault happened right after strobing the
>>>> flash (e.g. over temperature).
>>>>
>>>> I cannot see anything harmful in the situation when someone does grep
>>>> on /sys and clears non-persistent fault on a flash LED device.
>>>
>>> So why export the faults at all?
>>
>> Faults may prevent strobing the flash in case of some devices.
>> The example of such a device is ADP1663 (drivers/media/i2c/adp1653.c).
>> This driver reads the faults before strobing the flash and if a
>> fault preventing strobing has occurred it returns -EBUSY.
>>
>> If this driver was made a LED Flash class driver, then it would
>> expose flash_faults attribute. The driver would probably need
>> redesigning - checking the faults before strobing would have to be
>> avoided and it should be left to the userspace.
>
> That's fine, but Pavel's point is that you shouldn't "clear a fault" by
> reading a sysfs file as you don't control who reads all sysfs files
> (hint, libudev might cache all attributes when they are found / change,
> which could prevent anyone else from seeing that fault.)
>
> So please fix this, make a write to clear a fault or some other such
> explicit action, not a simple read. That's not an acceptable api.
I am aware what Pavel'a point was, I just presented the arguments
justifying existence of the flash_faults attribute at all.
In my opinion flash_faults attribute should report the current state of
the device. For the devices which clear the faults on I2C readout the
faults read would have to be cached in the driver, until they are
explicitly cleared, to keep the sysfs interface consistent.
Nonetheless, there can be also devices which don't require clearing the
faults - they are reported only when the actual condition occurs,
e.g. over temperature or under voltage. When the related value gets
back to the acceptable level the fault is no longer reported by the
device.
In this case some faults will remain unnoticed by the user space. This
is the argument in favour of my statement that caching the faults does
not make a sense and is not crucial. The user's vital interest is to
know whether the flash LED is operational right before strobing.
Since we cannot guarantee reporting all the faults that occurred for
all possible flash LED devices, the only sensible solution is to report
only the currently valid fault.
--
Best Regards,
Jacek Anaszewski
On Mon 2015-02-02 10:07:02, Jacek Anaszewski wrote:
> On 01/30/2015 05:40 PM, Greg KH wrote:
> >On Fri, Jan 30, 2015 at 09:55:30AM +0100, Jacek Anaszewski wrote:
> >>Hi Pavel,
> >>
> >>On 01/29/2015 10:14 PM, Pavel Machek wrote:
> >>>Hi!
> >>>
> >>>>>>+ - flash_fault - list of flash faults that may have occurred:
> >>>>>>+ * led-over-voltage - flash controller voltage to the flash LED
> >>>>>>+ has exceededthe limit specific to the flash controller
> >>>>>>+ * flash-timeout-exceeded - the flash strobe was still on when
> >>>>>>+ the timeout set by the user has expired; not all flash
> >>>>>>+ controllers may set this in all such conditions
> >>>>>>+ * controller-over-temperature - the flash controller has
> >>>>>>+ overheated
> >>>>>>+ * controller-short-circuit - the short circuit protection
> >>>>>>+ of the flash controller has been triggered
> >>>>>>+ * led-power-supply-over-current - current in the LED power
> >>>>>>+ supply has exceeded the limit specific to the flash
> >>>>>>+ controller
> >>>>>>+ * indicator-led-fault - the flash controller has detected
> >>>>>>+ a short or open circuit condition on the indicator LED
> >>>>>>+ * led-under-voltage - flash controller voltage to the flash
> >>>>>>+ LED has been below the minimum limit specific to
> >>>>>>+ the flash
> >>>>>>+ * controller-under-voltage - the input voltage of the flash
> >>>>>>+ controller is below the limit under which strobing the
> >>>>>>+ flash at full current will not be possible. The condition
> >>>>>>+ persists until this flag is no longer set
> >>>>>>+ * led-over-temperature - the temperature of the LED has exceeded
> >>>>>>+ its allowed upper limit
> >>>>>>+
> >>>>>>+ Flash faults are cleared, if possible, by reading the attribute.
> >>>>>
> >>>>>That's bad. Now you can no longer present flash_fault file as readable
> >>>>>to non-root users, and grep -ri foo /sys will interfere with your
> >>>>>camera application.
> >>>>>
> >>>>>Bad interface, just fix it.
> >>>>
> >>>>In my opinion it isn't crucial for the user to be aware of the
> >>>>fact that some non-persistent fault happened right after strobing the
> >>>>flash (e.g. over temperature).
> >>>>
> >>>>I cannot see anything harmful in the situation when someone does grep
> >>>>on /sys and clears non-persistent fault on a flash LED device.
> >>>
> >>>So why export the faults at all?
> >>
> >>Faults may prevent strobing the flash in case of some devices.
> >>The example of such a device is ADP1663 (drivers/media/i2c/adp1653.c).
> >>This driver reads the faults before strobing the flash and if a
> >>fault preventing strobing has occurred it returns -EBUSY.
> >>
> >>If this driver was made a LED Flash class driver, then it would
> >>expose flash_faults attribute. The driver would probably need
> >>redesigning - checking the faults before strobing would have to be
> >>avoided and it should be left to the userspace.
> >
> >That's fine, but Pavel's point is that you shouldn't "clear a fault" by
> >reading a sysfs file as you don't control who reads all sysfs files
> >(hint, libudev might cache all attributes when they are found / change,
> >which could prevent anyone else from seeing that fault.)
> >
> >So please fix this, make a write to clear a fault or some other such
> >explicit action, not a simple read. That's not an acceptable api.
>
> I am aware what Pavel'a point was, I just presented the arguments
> justifying existence of the flash_faults attribute at all.
Fine. Then, you should understand what you need to fix at this point.
> In my opinion flash_faults attribute should report the current state of
> the device. For the devices which clear the faults on I2C readout the
> faults read would have to be cached in the driver, until they are
> explicitly cleared, to keep the sysfs interface consistent.
Yes, just do the caching.
> Nonetheless, there can be also devices which don't require clearing the
> faults - they are reported only when the actual condition occurs,
> e.g. over temperature or under voltage. When the related value gets
> back to the acceptable level the fault is no longer reported by the
> device.
>
> In this case some faults will remain unnoticed by the user space. This
> is the argument in favour of my statement that caching the faults does
> not make a sense and is not crucial. The user's vital interest is to
> know whether the flash LED is operational right before strobing.
You can still do caching, exactly the same way. If you want current
and previous faults, do the read. If you want currently active faults,
you do write then read.
> Since we cannot guarantee reporting all the faults that occurred for
> all possible flash LED devices, the only sensible solution is to report
> only the currently valid fault.
How do you propose to do that on devices that clear on read?
[Actually, you could _always_ do two reads on those devices, discard
first result, and return the second. But I'm not sure how hardware
will like that.]
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On 02/02/2015 10:44 AM, Pavel Machek wrote:
> On Mon 2015-02-02 10:07:02, Jacek Anaszewski wrote:
>> On 01/30/2015 05:40 PM, Greg KH wrote:
>>> On Fri, Jan 30, 2015 at 09:55:30AM +0100, Jacek Anaszewski wrote:
>>>> Hi Pavel,
>>>>
>>>> On 01/29/2015 10:14 PM, Pavel Machek wrote:
>>>>> Hi!
>>>>>
>>>>>>>> + - flash_fault - list of flash faults that may have occurred:
>>>>>>>> + * led-over-voltage - flash controller voltage to the flash LED
>>>>>>>> + has exceededthe limit specific to the flash controller
>>>>>>>> + * flash-timeout-exceeded - the flash strobe was still on when
>>>>>>>> + the timeout set by the user has expired; not all flash
>>>>>>>> + controllers may set this in all such conditions
>>>>>>>> + * controller-over-temperature - the flash controller has
>>>>>>>> + overheated
>>>>>>>> + * controller-short-circuit - the short circuit protection
>>>>>>>> + of the flash controller has been triggered
>>>>>>>> + * led-power-supply-over-current - current in the LED power
>>>>>>>> + supply has exceeded the limit specific to the flash
>>>>>>>> + controller
>>>>>>>> + * indicator-led-fault - the flash controller has detected
>>>>>>>> + a short or open circuit condition on the indicator LED
>>>>>>>> + * led-under-voltage - flash controller voltage to the flash
>>>>>>>> + LED has been below the minimum limit specific to
>>>>>>>> + the flash
>>>>>>>> + * controller-under-voltage - the input voltage of the flash
>>>>>>>> + controller is below the limit under which strobing the
>>>>>>>> + flash at full current will not be possible. The condition
>>>>>>>> + persists until this flag is no longer set
>>>>>>>> + * led-over-temperature - the temperature of the LED has exceeded
>>>>>>>> + its allowed upper limit
>>>>>>>> +
>>>>>>>> + Flash faults are cleared, if possible, by reading the attribute.
>>>>>>>
>>>>>>> That's bad. Now you can no longer present flash_fault file as readable
>>>>>>> to non-root users, and grep -ri foo /sys will interfere with your
>>>>>>> camera application.
>>>>>>>
>>>>>>> Bad interface, just fix it.
>>>>>>
>>>>>> In my opinion it isn't crucial for the user to be aware of the
>>>>>> fact that some non-persistent fault happened right after strobing the
>>>>>> flash (e.g. over temperature).
>>>>>>
>>>>>> I cannot see anything harmful in the situation when someone does grep
>>>>>> on /sys and clears non-persistent fault on a flash LED device.
>>>>>
>>>>> So why export the faults at all?
>>>>
>>>> Faults may prevent strobing the flash in case of some devices.
>>>> The example of such a device is ADP1663 (drivers/media/i2c/adp1653.c).
>>>> This driver reads the faults before strobing the flash and if a
>>>> fault preventing strobing has occurred it returns -EBUSY.
>>>>
>>>> If this driver was made a LED Flash class driver, then it would
>>>> expose flash_faults attribute. The driver would probably need
>>>> redesigning - checking the faults before strobing would have to be
>>>> avoided and it should be left to the userspace.
>>>
>>> That's fine, but Pavel's point is that you shouldn't "clear a fault" by
>>> reading a sysfs file as you don't control who reads all sysfs files
>>> (hint, libudev might cache all attributes when they are found / change,
>>> which could prevent anyone else from seeing that fault.)
>>>
>>> So please fix this, make a write to clear a fault or some other such
>>> explicit action, not a simple read. That's not an acceptable api.
>>
>> I am aware what Pavel'a point was, I just presented the arguments
>> justifying existence of the flash_faults attribute at all.
>
> Fine. Then, you should understand what you need to fix at this point.
>
>> In my opinion flash_faults attribute should report the current state of
>> the device. For the devices which clear the faults on I2C readout the
>> faults read would have to be cached in the driver, until they are
>> explicitly cleared, to keep the sysfs interface consistent.
>
> Yes, just do the caching.
>
>> Nonetheless, there can be also devices which don't require clearing the
>> faults - they are reported only when the actual condition occurs,
>> e.g. over temperature or under voltage. When the related value gets
>> back to the acceptable level the fault is no longer reported by the
>> device.
>>
>> In this case some faults will remain unnoticed by the user space. This
>> is the argument in favour of my statement that caching the faults does
>> not make a sense and is not crucial. The user's vital interest is to
>> know whether the flash LED is operational right before strobing.
>
> You can still do caching, exactly the same way. If you want current
> and previous faults, do the read. If you want currently active faults,
> you do write then read.
>
>> Since we cannot guarantee reporting all the faults that occurred for
>> all possible flash LED devices, the only sensible solution is to report
>> only the currently valid fault.
>
> How do you propose to do that on devices that clear on read?
>
> [Actually, you could _always_ do two reads on those devices, discard
> first result, and return the second. But I'm not sure how hardware
> will like that.]
This would be the most sensible option.
However, let's analyze the typical use cases for flash strobing:
-------------------------------------------------------------
>>>>>>>> Version without faults caching:
============
Driver side:
============
read_faults()
faults = read_i2c(); //read faults
if faults
write_i2c(); //clear faults, only for some devices
faults = read_i2c(); //read faults
return faults
================
User space side:
================
1. faults = `cat flash_faults` //read_faults()
2. if faults then
print "Unable to strobe the flash LED due to faults"
else
echo 1 > flash_strobe
>>>>>>>> Version with faults caching:
============
Driver side:
============
read_faults()
faults |= read_i2c(); //read faults
clear_faults()
write_i2c(); //clear faults
faults = 0;
================
User space side:
================
1. faults = `cat flash_faults` //read_faults()
2. if faults then
echo 0 > flash_faults //clear_faults()
faults = `cat flash_faults` //read_faults()
3, if !faults
echo 1 > flash_strobe
else
print "Unable to strobe the flash LED due to faults"
-------------------------------------------------------------
From the above it seems that version with clearing faults on read
results in the simpler flash strobing procedure on userspace side,
by the cost of additional bus access on the driver side.
If the more complicated flash strobing procedure is acceptable
for everyone I will change the interface. BTW, I think, that
we don't need additional attribute, just writing the flash_faults
attribute can do the clearing.
--
Best Regards,
Jacek Anaszewski
Hi!
> >[Actually, you could _always_ do two reads on those devices, discard
> >first result, and return the second. But I'm not sure how hardware
> >will like that.]
>
> This would be the most sensible option.
>
>
> However, let's analyze the typical use cases for flash strobing:
>
>
> -------------------------------------------------------------
>
>
> >>>>>>>> Version without faults caching:
>
> ============
> Driver side:
> ============
>
> read_faults()
> faults = read_i2c(); //read faults
> if faults
> write_i2c(); //clear faults, only for some devices
> faults = read_i2c(); //read faults
> return faults
>
> ================
> User space side:
> ================
>
> 1. faults = `cat flash_faults` //read_faults()
> 2. if faults then
> print "Unable to strobe the flash LED due to faults"
> else
> echo 1 > flash_strobe
>
>
> >>>>>>>> Version with faults caching:
>
> ============
> Driver side:
> ============
>
> read_faults()
> faults |= read_i2c(); //read faults
>
> clear_faults()
> write_i2c(); //clear faults
> faults = 0;
>
>
> ================
> User space side:
> ================
>
> 1. faults = `cat flash_faults` //read_faults()
> 2. if faults then
> echo 0 > flash_faults //clear_faults()
> faults = `cat flash_faults` //read_faults()
> 3, if !faults
> echo 1 > flash_strobe
> else
> print "Unable to strobe the flash LED due to faults"
>
>
> -------------------------------------------------------------
>
> From the above it seems that version with clearing faults on read
> results in the simpler flash strobing procedure on userspace side,
> by the cost of additional bus access on the driver side.
I like caching version more (as it will allow by-hand debugging of
"why did not flash fire? Aha, lets see in the file, there was fault),
but both should be acceptable.
> we don't need additional attribute, just writing the flash_faults
> attribute can do the clearing.
Yes, writing flash_faults to clear is acceptable.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi Pavel,
On 02/02/2015 02:51 PM, Pavel Machek wrote:
> Hi!
>
>>> [Actually, you could _always_ do two reads on those devices, discard
>>> first result, and return the second. But I'm not sure how hardware
>>> will like that.]
>>
>> This would be the most sensible option.
>>
>>
>> However, let's analyze the typical use cases for flash strobing:
>>
>>
>> -------------------------------------------------------------
>>
>>
>>>>>>>>>> Version without faults caching:
>>
>> ============
>> Driver side:
>> ============
>>
>> read_faults()
>> faults = read_i2c(); //read faults
>> if faults
>> write_i2c(); //clear faults, only for some devices
>> faults = read_i2c(); //read faults
>> return faults
>>
>> ================
>> User space side:
>> ================
>>
>> 1. faults = `cat flash_faults` //read_faults()
>> 2. if faults then
>> print "Unable to strobe the flash LED due to faults"
>> else
>> echo 1 > flash_strobe
>>
>>
>>>>>>>>>> Version with faults caching:
>>
>> ============
>> Driver side:
>> ============
>>
>> read_faults()
>> faults |= read_i2c(); //read faults
>>
>> clear_faults()
>> write_i2c(); //clear faults
>> faults = 0;
>>
>>
>> ================
>> User space side:
>> ================
>>
>> 1. faults = `cat flash_faults` //read_faults()
>> 2. if faults then
>> echo 0 > flash_faults //clear_faults()
>> faults = `cat flash_faults` //read_faults()
>> 3, if !faults
>> echo 1 > flash_strobe
>> else
>> print "Unable to strobe the flash LED due to faults"
>>
>>
>> -------------------------------------------------------------
>>
>> From the above it seems that version with clearing faults on read
>> results in the simpler flash strobing procedure on userspace side,
>> by the cost of additional bus access on the driver side.
>
> I like caching version more (as it will allow by-hand debugging of
> "why did not flash fire? Aha, lets see in the file, there was fault),
> but both should be acceptable.
>
>> we don't need additional attribute, just writing the flash_faults
>> attribute can do the clearing.
>
> Yes, writing flash_faults to clear is acceptable.
I've been just inspired with another approach:
Faults register is read in the strobe_set callback, right after sending
flash strobe command to the device. The userspace can read the cached
faults through flash_faults attribute.
This way, we avoid reading sysfs attribute with side effect and
gain the possibility of giving immediate feedback to the user.
Exemplary use case:
1. echo 1 > flash_strobe
write_i2c(); //strobe flash
faults = read_i2c(); //read faults
if faults
return -EINVAL;
return 0;
2. cat flash_faults
return faults;
--
Best Regards,
Jacek Anaszewski