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

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

[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 <dmurphy@ti.com> 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
> >> configurable control bank.  
> 
> There are two control banks. You can connect the HVLED outputs to either 
> control bank A or B there is no individual control of the LED strings.
> 
> 
> >> But the DT binding supports 2 DT nodes, one for each control bank
> >> (identified by the `reg` property) and then `led-sources` says which
> >> string should be controlled by given bank.
> >>
> >> But taking in mind that DT should describe how devices are connected to
> >> each other, I think the child nodes in the binding should instead
> >> describe the 3 supported LED strings...  
> 
> The outputs in this case are virtual outputs which are the banks (A and B).
> 
> Since the device is bank controlled the actual current sinks are not 
> defined thus making the the banks the actual outputs.
> 
> This is why the 'reg' property defines the control bank either A or B 
> and the led-sources indicates the strings associated with the control bank.
> 
> Dan
> 

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 this controller should be able to support 3 LED strings via 3
outputs, the device tree binding nodes should, in my opinion, describe
each pin connected string. The nodes should maybe even be called
'led-string@N' where N is from [0, 1, 2].

The fact that the device is bank controlled and there are only two
banks (and it is configurable by which bank each LED string is
controlled) is more relevant to the driver, not as much to device tree
binding.

But yes, I do realize that if we had 3 child nodes, and the driver
created 3 LEDs, then changing brithrness on one of the 3 LEDs would
change brightness on at least another one, which we do not want.

Maybe this driver could parse these 3 `led-string` nodes, each having
defined bank via `led-sources`, and then register LED classdevs for
each bank that is mentioned. This way the device tree would be more
correct, IMO, and the driver would not have the problem mentioned in
the paragraph above.

Marek

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

[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 this controller should be able to support 3 LED strings via 3
> outputs, the device tree binding nodes should, in my opinion, describe
> each pin connected string. The nodes should maybe even be called
> 'led-string@N' where N is from [0, 1, 2].
>
> The fact that the device is bank controlled and there are only two
> banks (and it is configurable by which bank each LED string is
> controlled) is more relevant to the driver, not as much to device tree
> binding.
>
> But yes, I do realize that if we had 3 child nodes, and the driver
> created 3 LEDs, then changing brithrness on one of the 3 LEDs would
> change brightness on at least another one, which we do not want.
>
> Maybe this driver could parse these 3 `led-string` nodes, each having
> defined bank via `led-sources`, and then register LED classdevs for
> each bank that is mentioned. This way the device tree would be more
> correct, IMO, and the driver would not have the problem mentioned in
> the paragraph above.

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 to define the LED out to the bank.

Here is the conversation on how the driver got to where it is.

https://lore.kernel.org/patchwork/patch/972337/

Dan

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

[Marek Behun]

On Tue, 6 Oct 2020 09:57:08 -0500
Dan Murphy <dmurphy@ti.com> 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 to define the LED out to the bank.
> 
> Here is the conversation on how the driver got to where it is.
> 
> https://lore.kernel.org/patchwork/patch/972337/
> 
> Dan
> 

Oh, if this was discussed already, then never mind. Sorry for taking
your time.

Marek

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2256 bytes --]

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 
> > control bank A or B there is no individual control of the LED strings.
> > 
> > 
> > >> But the DT binding supports 2 DT nodes, one for each control bank
> > >> (identified by the `reg` property) and then `led-sources` says which
> > >> string should be controlled by given bank.
> > >>
> > >> But taking in mind that DT should describe how devices are connected to
> > >> each other, I think the child nodes in the binding should instead
> > >> describe the 3 supported LED strings...  
> > 
> > The outputs in this case are virtual outputs which are the banks (A and B).
> > 
> > Since the device is bank controlled the actual current sinks are not 
> > defined thus making the the banks the actual outputs.
> > 
> > This is why the 'reg' property defines the control bank either A or B 
> > and the led-sources indicates the strings associated with the control bank.

> 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.

Well, device tree is not a device schematics...

> If this controller should be able to support 3 LED strings via 3
> outputs, the device tree binding nodes should, in my opinion, describe
> each pin connected string. The nodes should maybe even be called
> 'led-string@N' where N is from [0, 1, 2].
> 
> The fact that the device is bank controlled and there are only two
> banks (and it is configurable by which bank each LED string is
> controlled) is more relevant to the driver, not as much to device tree
> binding.

Seems to me like two independend LEDs, and I'd describe it as
such. The fact that it goes over 3 wires is just a implementation
detail. Lets keep it simple...

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]