Re: [PATCH] leds: lm3697: Fix out-of-bound access

2020-10-06 Thread Pavel Machek
Hi! > > >> By the way I just realized that the DT binding in this driver seems > > >> incorrect to me. > > >> > > >> The controller logically supports 3 LED strings, each having > > >> configurable control bank. > > > > There are two control banks. You can connect the HVLED outputs to either >

Re: [PATCH] leds: lm3697: Fix out-of-bound access

2020-10-06 Thread Marek Behun
On Tue, 6 Oct 2020 09:57:08 -0500 Dan Murphy wrote: > Unfortunately we cannot and should not change the ABI now. > > Using the led-sources as the bank indicator does not conform to the > definition of the description of the led-sources in the yaml. > > The preference was to use the led-sources

Re: [PATCH] leds: lm3697: Fix out-of-bound access

2020-10-06 Thread Dan Murphy
Marek On 10/6/20 9:41 AM, Marek Behun wrote: Adding Rob to Cc, Rob, could we have your opinion on this? Mine is below. Dan, I looked at the datasheet, I understand this. Nonetheless, device tree should describe how devices are connected to each other. The chip has 3 pins for 3 LED strings. If

Re: [PATCH] leds: lm3697: Fix out-of-bound access

2020-10-06 Thread Marek Behun
Adding Rob to Cc, Rob, could we have your opinion on this? Mine is below. On Tue, 6 Oct 2020 07:21:14 -0500 Dan Murphy wrote: > >> By the way I just realized that the DT binding in this driver seems > >> incorrect to me. > >> > >> The controller logically supports 3 LED strings, each having > >>

Re: [PATCH] leds: lm3697: Fix out-of-bound access

2020-10-06 Thread Dan Murphy
All On 10/6/20 6:59 AM, ultracool...@tutanota.com wrote: While I do agree with you that having the child nodes be led strings make more sense, would it be possible to have, for example, three strings controlled by the same label? Oct 6, 2020, 07:33 by ka...@blackhole.sk: By the way I just r

Re: [PATCH] leds: lm3697: Fix out-of-bound access

2020-10-06 Thread ultracoolguy
While I do agree with you that having the child nodes be led strings make more sense, would it be possible to have, for example, three strings controlled by the same label? Oct 6, 2020, 07:33 by ka...@blackhole.sk: > By the way I just realized that the DT binding in this driver seems > incorrec

Re: [PATCH] leds: lm3697: Fix out-of-bound access

2020-10-06 Thread Marek Behun
By the way I just realized that the DT binding in this driver seems incorrect to me. The controller logically supports 3 LED strings, each having configurable control bank. But the DT binding supports 2 DT nodes, one for each control bank (identified by the `reg` property) and then `led-sources`

Re: [PATCH] leds: lm3697: Fix out-of-bound access

2020-10-05 Thread ultracoolguy
>No problem, sorry for breaking your patch. I moved it near other initializers. No problem :) >Question: is it likely that someone will want to use your device with -stable kernels? Should I mark this for -stable? To be honest, it's unlikely that someone other than me is interested in my device

Re: [PATCH] leds: lm3697: Fix out-of-bound access

2020-10-05 Thread Pavel Machek
On Mon 2020-10-05 20:31:16, ultracool...@tutanota.com wrote: > Otherwise everything works great :) > > (And sorry for sending two emails) No problem, sorry for breaking your patch. I moved it near other initializers. Question: is it likely that someone will want to use your device with -stable k

Re: [PATCH] leds: lm3697: Fix out-of-bound access

2020-10-05 Thread ultracoolguy
Otherwise everything works great :) (And sorry for sending two emails) Oct 5, 2020, 18:29 by ultracool...@tutanota.com: > This: > > led->num_banks = count; > > Has to be below devm_kzalloc. Else, it's NULL. > Oct 5, 2020, 17:32 by pa...@ucw.cz: > >> Hi! >> >>> Agh. I added the Signed-off-by i

Re: [PATCH] leds: lm3697: Fix out-of-bound access

2020-10-05 Thread ultracoolguy
This: led->num_banks = count; Has to be below devm_kzalloc. Else, it's NULL. Oct 5, 2020, 17:32 by pa...@ucw.cz: > Hi! > >> Agh. I added the Signed-off-by in an earlier non-published version of the >> commit, but forgot to add it back. But that doesn't really excuses me. >> >> I attached the

Re: [PATCH] leds: lm3697: Fix out-of-bound access

2020-10-05 Thread Pavel Machek
Hi! > Agh. I added the Signed-off-by in an earlier non-published version of the > commit, but forgot to add it back. But that doesn't really excuses me. > > I attached the (hopefully) final version of this patch.  Pavel, I'll send the > struct rename separately after I submit this. > Thanks,

Re: [PATCH] leds: lm3697: Fix out-of-bound access

2020-10-05 Thread ultracoolguy
Agh. I added the Signed-off-by in an earlier non-published version of the commit, but forgot to add it back. But that doesn't really excuses me. I attached the (hopefully) final version of this patch.  Pavel, I'll send the struct rename separately after I submit this. Oct 5, 2020, 16:48 by p..

Re: [PATCH] leds: lm3697: Fix out-of-bound access

2020-10-05 Thread Alexander Dahl
Hei hei, On Mon, Oct 05, 2020 at 05:35:38PM +0200, ultracool...@tutanota.com wrote: > Well, the major benefit I see is that it makes the driver slightly > more readable. However I'm fine with whatever you guys decide. > > I'll attach the patch with the struct renaming removed just in case. Note:

Re: [PATCH] leds: lm3697: Fix out-of-bound access

2020-10-05 Thread Pavel Machek
Hi! > Well, the major benefit I see is that it makes the driver slightly more > readable. However I'm fine with whatever you guys decide. > > I'll attach the patch with the struct renaming removed just in case. Thanks for the patches. Content is pretty good, but I'd really need From + Signed-of

Re: [PATCH] leds: lm3697: Fix out-of-bound access

2020-10-05 Thread Pavel Machek
Hi! > On 10/5/20 8:57 AM, ultracool...@tutanota.com wrote: > > I agree with you. > > > > Attached patch with changes. > > Nack to the patch. No need to do that, without matching From: and Signed-off, I can't really apply it... > The subject says it does one thing but you also unnecessarily cha

Re: [PATCH] leds: lm3697: Fix out-of-bound access

2020-10-05 Thread ultracoolguy
Well, the major benefit I see is that it makes the driver slightly more readable. However I'm fine with whatever you guys decide. I'll attach the patch with the struct renaming removed just in case. Oct 5, 2020, 14:41 by dmur...@ti.com: > Gabriel > > On 10/5/20 9:38 AM, ultracool...@tutanota.

Re: [PATCH] leds: lm3697: Fix out-of-bound access

2020-10-05 Thread Dan Murphy
Gabriel On 10/5/20 9:38 AM, ultracool...@tutanota.com wrote: I understand. So I should leave it like it was and do the rename in another patch? You should do the fix in one patch and leave the structure name alone. The structure naming if fine and has no benefit and actually will make it mo

Re: [PATCH] leds: lm3697: Fix out-of-bound access

2020-10-05 Thread ultracoolguy
I understand. So I should leave it like it was and do the rename in another patch? Oct 5, 2020, 14:33 by dmur...@ti.com: > Marek > > On 10/5/20 8:57 AM, ultracool...@tutanota.com wrote: > >> I agree with you. >> >> Attached patch with changes. >> > > Nack to the patch. > > The subject says it d

Re: [PATCH] leds: lm3697: Fix out-of-bound access

2020-10-05 Thread Dan Murphy
All On 10/5/20 9:33 AM, Dan Murphy wrote: Marek Sorry not Marek but Gabriel I misread the "To" field Dan

Re: [PATCH] leds: lm3697: Fix out-of-bound access

2020-10-05 Thread Dan Murphy
Marek On 10/5/20 8:57 AM, ultracool...@tutanota.com wrote: I agree with you. Attached patch with changes. Nack to the patch. The subject says it does one thing but you also unnecessarily changed the name of the structure. Renaming the structure does not fix the underlying issue Dan

Re: [PATCH] leds: lm3697: Fix out-of-bound access

2020-10-05 Thread ultracoolguy
I agree with you. Attached patch with changes. Oct 5, 2020, 12:13 by ka...@blackhole.sk: > On Sat, 3 Oct 2020 15:02:51 +0200 (CEST) > ultracool...@tutanota.com wrote: > >> From 0dfd5ab647ccbc585c543d702b44d20f0e3fe436 Mon Sep 17 00:00:00 2001 >> From: Ultracoolguy >> Date: Fri, 2 Oct 2020 18

Re: [PATCH] leds: lm3697: Fix out-of-bound access

2020-10-05 Thread Pavel Machek
Hi! > > if (ret) > > dev_err(&priv->client->dev, "Cannot write OUTPUT config\n"); > > > > - for (i = 0; i < LM3697_MAX_CONTROL_BANKS; i++) { > > + for (i = 0; i < priv->num_leds; i++) { > > Ultracoolguy is correct that this for cycle should not iterate > LM3697_MAX_CONTROL_BA

Re: [PATCH] leds: lm3697: Fix out-of-bound access

2020-10-05 Thread Marek Behun
On Sat, 3 Oct 2020 15:02:51 +0200 (CEST) ultracool...@tutanota.com wrote: > From 0dfd5ab647ccbc585c543d702b44d20f0e3fe436 Mon Sep 17 00:00:00 2001 > From: Ultracoolguy > Date: Fri, 2 Oct 2020 18:27:00 -0400 > Subject: [PATCH] leds:lm3697:Fix out-of-bound access > > If both led banks aren't used

Re: [PATCH] leds: lm3697: Fix out-of-bound access

2020-10-03 Thread ultracoolguy
>I'll need your real name to apply a patch. My real name is Gabriel David. >Ok, so I assume this is only problem with certain device trees, and not a problem with dts' in mainline? Yes. Here's the current node I'm using that causes this bug: &i2c_5 {     status = "ok";     ti_lm3697

Re: [PATCH] leds: lm3697: Fix out-of-bound access

2020-10-03 Thread Pavel Machek
Hi! > Signed-off-by: Ultracoolguy I'll need your real name to apply a patch. > Hi, all. This is a patch fixing an out-of-bounds error due to lm3697_init > expecting the device tree to use both control banks.  This fixes it by adding > a new variable that will hold the number of used banks. >