Hi, > Am 07.07.2016 um 10:46 schrieb Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx>: > > 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@xxxxxxxxx>: >>> >>> 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html