Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1438379 > unrolled thread

Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led  driver Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-07-07 10:50 +0200
    Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver "H. Nikolaus Schaller" <hns@goldelico.com> - 2016-07-07 11:00 +0200

#1438379 — Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

Hi Nikolaus,

On 07/06/2016 12:02 PM, H. Nikolaus Schaller wrote:
> Hi,
> finally, I found the time to update the driver according to the many comments
> received a while ago.
>
> Most of them have been worked in, including the regmap idea and
> brightness_set_blocking().
>
> The driver works on our system, so that I will mail [PATCH v2] as a followup.
>
> There is only one aspect of the new solution I am not sure if it is
> really better than our old proposal (see below).
>
>
>> Am 20.04.2016 um 23:04 schrieb Jacek Anaszewski <jacek.anaszewski@gmail.com>:
>>
>> On 04/19/2016 07:21 PM, H. Nikolaus Schaller wrote:
>>>> I believe the LEDS core now handles the workqueues generically for
>>>> blocking operations, so it's no longer needed in the individual drivers.
>>>
>>> We had a lot of trouble with locking and blocking especially if we
>>> want to indicate CPU or (root) disk activity.
>>
>> What kind of troubles you had? Could you share more details?
>> Does it mean that current LED core design doesn't fit for your
>> use cases?
>
> The system started to flicker the LEDs irregularily and sometimes
> the whole kernel stalled.
>
>>
>>> So it is implemented in a way that changes can be requested faster
>>> than the I2C bus can write new values to the chip.
>>>
>>> Only after one sequence of I2C writes is done, another work function
>>> can be scheduled. And each group of writes updates as many LEDs
>>> in parallel if necessary.
>>
>> You can serialize the operations in brightness_set_blocking with
>> a mutex.
>
> Yes, that works fine in our (incomplete) test setup.
>
> But I think it assumes that the i2c bus is never congested by other i2c traffic.
>
> I have not found code that obviously takes care of the situation if led
> trigger events (e.g. mmc or cpu triggers) are coming in faster than the
> i2c (even using regmap) can write out over i2c.
>
> If I understand the led core code correctly, it will just do another schedule_work
> for every single change of led brightness.

Please look at schedule_work documentation in include/linux/workqueue.h:

/**
  * schedule_work - put work task in global workqueue
  * @work: job to be done
  *
  * Returns %false if @work was already on the kernel-global workqueue and
  * %true otherwise.
  *
  * This puts a job in the kernel-global workqueue if it was not already
  * queued and leaves it in the same position on the kernel-global
  * workqueue otherwise.
  */
static inline bool schedule_work(struct work_struct *work)
{
         return queue_work(system_wq, work);
}

>
> So I wonder if Is there some guarantee that this work queue will not fill
> up memory and is really processed faster than being filled? I.e. can the
> queue overflow?
>
> To reduce this risk, my original implementation strategy was different. The
> update speed was limited by i2c writing. A new register update batch job
> was only scheduled if the previous one is finished. If i2c was blocked/congested,
> the writing worker thread would come to a halt.
>
> All incoming led brightness changes were simply accumulated until a new
> batch job is started, because LEDs would anyways flicker invisibly fast.
>
> Tests with the new driver have shown that it seems not to run into this situation
> on our system but it might depend on factors we have not yet tested (slow i2c,
> other i2c traffic on the same bus, CPU speed, event types choosen).
>
> So I am a little in doubt about this risk. But I may have simply missed
> the reason why the standard approach works and can never overflow.
>
> BR,
> Nikolaus
>
>
>


-- 
Best regards,
Jacek Anaszewski

[toc] | [next] | [standalone]

#1438400 — Re: [PATCH] drivers: led: is31fl319x: 6/9-channel light effect led driver

Hi,

> Am 07.07.2016 um 10:46 schrieb Jacek Anaszewski <j.anaszewski@samsung.com>:
> 
> Hi Nikolaus,
> 
> On 07/06/2016 12:02 PM, H. Nikolaus Schaller wrote:
>> Hi,
>> finally, I found the time to update the driver according to the many comments
>> received a while ago.
>> 
>> Most of them have been worked in, including the regmap idea and
>> brightness_set_blocking().
>> 
>> The driver works on our system, so that I will mail [PATCH v2] as a followup.
>> 
>> There is only one aspect of the new solution I am not sure if it is
>> really better than our old proposal (see below).
>> 
>> 
>>> Am 20.04.2016 um 23:04 schrieb Jacek Anaszewski <jacek.anaszewski@gmail.com>:
>>> 
>>> On 04/19/2016 07:21 PM, H. Nikolaus Schaller wrote:
>>>>> I believe the LEDS core now handles the workqueues generically for
>>>>> blocking operations, so it's no longer needed in the individual drivers.
>>>> 
>>>> We had a lot of trouble with locking and blocking especially if we
>>>> want to indicate CPU or (root) disk activity.
>>> 
>>> What kind of troubles you had? Could you share more details?
>>> Does it mean that current LED core design doesn't fit for your
>>> use cases?
>> 
>> The system started to flicker the LEDs irregularily and sometimes
>> the whole kernel stalled.
>> 
>>> 
>>>> So it is implemented in a way that changes can be requested faster
>>>> than the I2C bus can write new values to the chip.
>>>> 
>>>> Only after one sequence of I2C writes is done, another work function
>>>> can be scheduled. And each group of writes updates as many LEDs
>>>> in parallel if necessary.
>>> 
>>> You can serialize the operations in brightness_set_blocking with
>>> a mutex.
>> 
>> Yes, that works fine in our (incomplete) test setup.
>> 
>> But I think it assumes that the i2c bus is never congested by other i2c traffic.
>> 
>> I have not found code that obviously takes care of the situation if led
>> trigger events (e.g. mmc or cpu triggers) are coming in faster than the
>> i2c (even using regmap) can write out over i2c.
>> 
>> If I understand the led core code correctly, it will just do another schedule_work
>> for every single change of led brightness.
> 
> Please look at schedule_work documentation in include/linux/workqueue.h:
> 
> /**
> * schedule_work - put work task in global workqueue
> * @work: job to be done
> *
> * Returns %false if @work was already on the kernel-global workqueue and
> * %true otherwise.
> *
> * This puts a job in the kernel-global workqueue if it was not already

Ah, ok. I missed that. Thanks!

> * queued and leaves it in the same position on the kernel-global
> * workqueue otherwise.
> */
> static inline bool schedule_work(struct work_struct *work)
> {
>        return queue_work(system_wq, work);
> }

> 
>> 
>> So I wonder if Is there some guarantee that this work queue will not fill
>> up memory and is really processed faster than being filled? I.e. can the
>> queue overflow?
>> 
>> To reduce this risk, my original implementation strategy was different. The
>> update speed was limited by i2c writing. A new register update batch job
>> was only scheduled if the previous one is finished. If i2c was blocked/congested,
>> the writing worker thread would come to a halt.
>> 
>> All incoming led brightness changes were simply accumulated until a new
>> batch job is started, because LEDs would anyways flicker invisibly fast.
>> 
>> Tests with the new driver have shown that it seems not to run into this situation
>> on our system but it might depend on factors we have not yet tested (slow i2c,
>> other i2c traffic on the same bus, CPU speed, event types choosen).
>> 
>> So I am a little in doubt about this risk. But I may have simply missed
>> the reason why the standard approach works and can never overflow.
>> 
>> BR,
>> Nikolaus
>> 
>> 
>> 
> 
> 
> -- 
> Best regards,
> Jacek Anaszewski

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web