| Message ID | CAPJMGm4StRvJ4zTyrOb7ebo47LrR9bBuZ46p7VOxkDfwWSG=PA@mail.gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] ad7192 driver: fix null pointer dereference in probe when populating adc input ranges | expand |
On Mon, 2023-03-27 at 22:21 +0200, Fabrizio Lamarque wrote: > Allow the use of external clock when mclk clock is defined. > When defining a mclk clock source in device tree with adi,clock-xtal > property, the external crystal oscillator is not turned on. > Without the change, the driver always uses the internal clock even > when mclk clock is defined. > > Current implementation seems to contain a typo, since it expected > st->mclk to be NULL within ad7192_of_clock_select() in order to > select > the external clock, but, if null, external clock cannot loaded > correctly (out of bounds due to invalid mclk) in ad7192_probe(). > > I believe this patch follows the author's intended behavior. > After applying this patch, the external oscillator is started as > expected. > Yes, looks like a valid fix... Just missing a Fixes tag. > I kindly ask your feedback, I may adjust the patch according to your > suggestions. I could also follow up with another patch on > documentation, containing the following (related) issues: > > - adi,int-clock-output-enable is undocumented > - adi,clock-xtal is undocumented > - regulator name avdd and its description is quite misleading, since > this is unrelated to the AVdd pin (#20) of AD7192; it is used instead > as reference voltage (REFIN1 on #15/#16 or REFIN2 on #7/#8). See > int_vref_mv variable within driver implementation. > Don't think the above text belongs to this commit message. That said, it would be great if you could follow up with a couple of patches to document the undocumented properties. As for the regular name, I think it's not so trivial to change it's name because there could be already users using the property like this. So, I guess we have two ways: 1) Just add some description in the property to state what's this regulator really is about. 2) This one is more complex but might be the right one... Deprecrate the current property (you can mark a property as deprecated in the bindings) and add the new one with a proper name. Then, you need to change the driver accordingly keeping in mind that it must still work with the old, deprecrated property. Was lazy to ckeck but I'm assuming the bindings are already in yaml... > Signed-off-by: Fabrizio Lamarque <fl.scratchpad@gmail.com> > --- linux/drivers/iio/adc/ad7192.c 2023-03-13 19:32:42.646239506 Anyways, for this patch and with a proper fixes tag (and with the unrelated text removed from the commit message): Reviewed-by: nuno.sa@analog.com
On Wed, Mar 29, 2023 at 5:15 PM Nuno Sá <noname.nuno@gmail.com> wrote: > > On Mon, 2023-03-27 at 22:21 +0200, Fabrizio Lamarque wrote: > > Allow the use of external clock when mclk clock is defined. > > When defining a mclk clock source in device tree with adi,clock-xtal > > property, the external crystal oscillator is not turned on. > > Without the change, the driver always uses the internal clock even > > when mclk clock is defined. > > > > Current implementation seems to contain a typo, since it expected > > st->mclk to be NULL within ad7192_of_clock_select() in order to > > select > > the external clock, but, if null, external clock cannot loaded > > correctly (out of bounds due to invalid mclk) in ad7192_probe(). > > > > I believe this patch follows the author's intended behavior. > > After applying this patch, the external oscillator is started as > > expected. > > > > Yes, looks like a valid fix... Just missing a Fixes tag. Thank you for having this patch reviewed. Here is a backwards compatibility note I believe you should be aware of. Without this patch, the DT node shall contain (any) clock property for the driver to be loaded properly. The clock frequency is ignored, as it is ignored adi,clock-xtal property. In case clock property is not set, the initialization always fails. There is no way to use an external clock source. As you already verified from source code, this was not intentional. While the proposed patch should make the DT config load as intended, there is a possible side effect on existing implementations relying on internal clock source only, that would be deactivated when clock property is defined in DT. In addition, bindings documentation states clock property is mandatory. I tend to dislike any solution that keeps the existing flow (and do not break compatibility). Nothing that comes to my mind would be as simple as this logical flow correction. In addition, I checked the driver history and it looks like the minor bugs we found in corrent implementation (see previous patch about NULL pointer dereference) have been there for several years. In case backwards compatibility is a real issue, I could rewrite the patch so that an explicit external clock source property could be used instead. If so, I kindly ask you to suggest an acceptable property name. > > I kindly ask your feedback, I may adjust the patch according to your > > suggestions. I could also follow up with another patch on > > documentation, containing the following (related) issues: > > > > - adi,int-clock-output-enable is undocumented > > - adi,clock-xtal is undocumented > > - regulator name avdd and its description is quite misleading, since > > this is unrelated to the AVdd pin (#20) of AD7192; it is used instead > > as reference voltage (REFIN1 on #15/#16 or REFIN2 on #7/#8). See > > int_vref_mv variable within driver implementation. > > > > Don't think the above text belongs to this commit message. That said, > it would be great if you could follow up with a couple of patches to > document the undocumented properties. As for the regular name, I think > it's not so trivial to change it's name because there could be already > users using the property like this. So, I guess we have two ways: > > 1) Just add some description in the property to state what's this > regulator really is about. > 2) This one is more complex but might be the right one... Deprecrate > the current property (you can mark a property as deprecated in the > bindings) and add the new one with a proper name. Then, you need to > change the driver accordingly keeping in mind that it must still work > with the old, deprecrated property. > > Was lazy to ckeck but I'm assuming the bindings are already in yaml... I can confirm bindings are already documented in yaml. I will try to submit a patch with the missing documentation and the proposed solution 2 in the next days. > > > Signed-off-by: Fabrizio Lamarque <fl.scratchpad@gmail.com> > > --- linux/drivers/iio/adc/ad7192.c 2023-03-13 19:32:42.646239506 > > Anyways, for this patch and with a proper fixes tag (and with the > unrelated text removed from the commit message): > > Reviewed-by: nuno.sa@analog.com > >
On Wed, 2023-03-29 at 23:23 +0200, Fabrizio Lamarque wrote: > On Wed, Mar 29, 2023 at 5:15 PM Nuno Sá <noname.nuno@gmail.com> > wrote: > > > > On Mon, 2023-03-27 at 22:21 +0200, Fabrizio Lamarque wrote: > > > Allow the use of external clock when mclk clock is defined. > > > When defining a mclk clock source in device tree with adi,clock- > > > xtal > > > property, the external crystal oscillator is not turned on. > > > Without the change, the driver always uses the internal clock > > > even > > > when mclk clock is defined. > > > > > > Current implementation seems to contain a typo, since it expected > > > st->mclk to be NULL within ad7192_of_clock_select() in order to > > > select > > > the external clock, but, if null, external clock cannot loaded > > > correctly (out of bounds due to invalid mclk) in ad7192_probe(). > > > > > > I believe this patch follows the author's intended behavior. > > > After applying this patch, the external oscillator is started as > > > expected. > > > > > > > Yes, looks like a valid fix... Just missing a Fixes tag. > > Thank you for having this patch reviewed. > Here is a backwards compatibility note I believe you should be aware > of. > > Without this patch, the DT node shall contain (any) clock property > for the driver to be loaded properly. > The clock frequency is ignored, as it is ignored adi,clock-xtal > property. > In case clock property is not set, the initialization always fails. > There is no way to use an external clock source. > As you already verified from source code, this was not intentional. > > While the proposed patch should make the DT config load as intended, > there is a possible side effect on existing implementations relying > on > internal clock source only, that would be deactivated when clock > property > is defined in DT. Hmm, I see. I would argue anyone relying on that should have just stepped forward and fix the real issue as you did :). So, maybe there's no one actually relying on this but that's a big __maybe__... Not really sure how to handle this. I'm not sure we want to keep compatibility with something that's clearly wrong but often we need to "live" with past messes. Jonathan I guess your input will be valuable here :)? > In addition, bindings documentation states clock > property is mandatory. > This looks at least suspicious to me given the fact we have an internal alternative. Clearly looks wrong to me... - Nuno Sá >
On Thu, 30 Mar 2023 09:53:29 +0200 Nuno Sá <noname.nuno@gmail.com> wrote: > On Wed, 2023-03-29 at 23:23 +0200, Fabrizio Lamarque wrote: > > On Wed, Mar 29, 2023 at 5:15 PM Nuno Sá <noname.nuno@gmail.com> > > wrote: > > > > > > On Mon, 2023-03-27 at 22:21 +0200, Fabrizio Lamarque wrote: > > > > Allow the use of external clock when mclk clock is defined. > > > > When defining a mclk clock source in device tree with adi,clock- > > > > xtal > > > > property, the external crystal oscillator is not turned on. > > > > Without the change, the driver always uses the internal clock > > > > even > > > > when mclk clock is defined. > > > > > > > > Current implementation seems to contain a typo, since it expected > > > > st->mclk to be NULL within ad7192_of_clock_select() in order to > > > > select > > > > the external clock, but, if null, external clock cannot loaded > > > > correctly (out of bounds due to invalid mclk) in ad7192_probe(). > > > > > > > > I believe this patch follows the author's intended behavior. > > > > After applying this patch, the external oscillator is started as > > > > expected. > > > > > > > > > > Yes, looks like a valid fix... Just missing a Fixes tag. > > > > Thank you for having this patch reviewed. > > Here is a backwards compatibility note I believe you should be aware > > of. > > > > Without this patch, the DT node shall contain (any) clock property > > for the driver to be loaded properly. > > The clock frequency is ignored, as it is ignored adi,clock-xtal > > property. > > In case clock property is not set, the initialization always fails. > > There is no way to use an external clock source. > > As you already verified from source code, this was not intentional. > > > > While the proposed patch should make the DT config load as intended, > > there is a possible side effect on existing implementations relying > > on > > internal clock source only, that would be deactivated when clock > > property > > is defined in DT. > > Hmm, I see. I would argue anyone relying on that should have just > stepped forward and fix the real issue as you did :). So, maybe there's > no one actually relying on this but that's a big __maybe__... Not > really sure how to handle this. I'm not sure we want to keep > compatibility with something that's clearly wrong but often we need to > "live" with past messes. > > Jonathan I guess your input will be valuable here :)? Rather odd if they have bindings that say there is a clock but 'rely' on the internal clock as opposed to the current bug meaning they aren't using the external clock (but everything would work fine if they were). So I'd expect that we will only see a problem if someone has another bug somewhere else that means that clock isn't working correctly. At that point they will almost certainly want to fix that bug. > > > In addition, bindings documentation states clock > > property is mandatory. > > > > This looks at least suspicious to me given the fact we have an internal > alternative. Clearly looks wrong to me... Agreed. I don't think it should be mandatory in the binding doc. Thanks, Jonathan > > - Nuno Sá > >
On Sat, Apr 1, 2023 at 4:21 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Thu, 30 Mar 2023 09:53:29 +0200 > Nuno Sá <noname.nuno@gmail.com> wrote: > > > On Wed, 2023-03-29 at 23:23 +0200, Fabrizio Lamarque wrote: > > > On Wed, Mar 29, 2023 at 5:15 PM Nuno Sá <noname.nuno@gmail.com> > > > wrote: > > > > > > > > On Mon, 2023-03-27 at 22:21 +0200, Fabrizio Lamarque wrote: > > > > > Allow the use of external clock when mclk clock is defined. > > > > > When defining a mclk clock source in device tree with adi,clock- > > > > > xtal > > > > > property, the external crystal oscillator is not turned on. > > > > > Without the change, the driver always uses the internal clock > > > > > even > > > > > when mclk clock is defined. > > > > > > > > > > Current implementation seems to contain a typo, since it expected > > > > > st->mclk to be NULL within ad7192_of_clock_select() in order to > > > > > select > > > > > the external clock, but, if null, external clock cannot loaded > > > > > correctly (out of bounds due to invalid mclk) in ad7192_probe(). > > > > > > > > > > I believe this patch follows the author's intended behavior. > > > > > After applying this patch, the external oscillator is started as > > > > > expected. > > > > > > > > > > > > > Yes, looks like a valid fix... Just missing a Fixes tag. > > > > > > Thank you for having this patch reviewed. > > > Here is a backwards compatibility note I believe you should be aware > > > of. > > > > > > Without this patch, the DT node shall contain (any) clock property > > > for the driver to be loaded properly. > > > The clock frequency is ignored, as it is ignored adi,clock-xtal > > > property. > > > In case clock property is not set, the initialization always fails. > > > There is no way to use an external clock source. > > > As you already verified from source code, this was not intentional. > > > > > > While the proposed patch should make the DT config load as intended, > > > there is a possible side effect on existing implementations relying > > > on > > > internal clock source only, that would be deactivated when clock > > > property > > > is defined in DT. > > > > Hmm, I see. I would argue anyone relying on that should have just > > stepped forward and fix the real issue as you did :). So, maybe there's > > no one actually relying on this but that's a big __maybe__... Not > > really sure how to handle this. I'm not sure we want to keep > > compatibility with something that's clearly wrong but often we need to > > "live" with past messes. > > > > Jonathan I guess your input will be valuable here :)? > > Rather odd if they have bindings that say there is a clock but 'rely' > on the internal clock as opposed to the current bug meaning they aren't > using the external clock (but everything would work fine if they were). > > So I'd expect that we will only see a problem if someone has another > bug somewhere else that means that clock isn't working correctly. > At that point they will almost certainly want to fix that bug. The bug was introduced in June 2021 from c9ec2cb328e3 iio: adc: ad7192: use devm_clk_get_optional() for mclk, where there is a clearly mistaken logic inversion. Link: https://lore.kernel.org/all/20210513120752.90074-10-aardelean@deviqon.com/#r If you agree, I can send a revised patch set (perhaps within a new thread), with the additional clarifications on clock usage in bindings documentation, and one third patch for undocumented bindings. There is one last issue we are investigating, we have a work-around but we have not found the root cause yet. I will post about it later, in the hope the patch set would include all the changes needed to have the driver operating as expected. Thank you once again for this review. Fabrizio Lamarque > > > > > > In addition, bindings documentation states clock > > > property is mandatory. > > > > > > > This looks at least suspicious to me given the fact we have an internal > > alternative. Clearly looks wrong to me... > > Agreed. I don't think it should be mandatory in the binding doc. > > Thanks, > > Jonathan > > > > > > - Nuno Sá > > > >
On Tue, 4 Apr 2023 09:20:24 +0200 Fabrizio Lamarque <fl.scratchpad@gmail.com> wrote: > On Sat, Apr 1, 2023 at 4:21 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Thu, 30 Mar 2023 09:53:29 +0200 > > Nuno Sá <noname.nuno@gmail.com> wrote: > > > > > On Wed, 2023-03-29 at 23:23 +0200, Fabrizio Lamarque wrote: > > > > On Wed, Mar 29, 2023 at 5:15 PM Nuno Sá <noname.nuno@gmail.com> > > > > wrote: > > > > > > > > > > On Mon, 2023-03-27 at 22:21 +0200, Fabrizio Lamarque wrote: > > > > > > Allow the use of external clock when mclk clock is defined. > > > > > > When defining a mclk clock source in device tree with adi,clock- > > > > > > xtal > > > > > > property, the external crystal oscillator is not turned on. > > > > > > Without the change, the driver always uses the internal clock > > > > > > even > > > > > > when mclk clock is defined. > > > > > > > > > > > > Current implementation seems to contain a typo, since it expected > > > > > > st->mclk to be NULL within ad7192_of_clock_select() in order to > > > > > > select > > > > > > the external clock, but, if null, external clock cannot loaded > > > > > > correctly (out of bounds due to invalid mclk) in ad7192_probe(). > > > > > > > > > > > > I believe this patch follows the author's intended behavior. > > > > > > After applying this patch, the external oscillator is started as > > > > > > expected. > > > > > > > > > > > > > > > > Yes, looks like a valid fix... Just missing a Fixes tag. > > > > > > > > Thank you for having this patch reviewed. > > > > Here is a backwards compatibility note I believe you should be aware > > > > of. > > > > > > > > Without this patch, the DT node shall contain (any) clock property > > > > for the driver to be loaded properly. > > > > The clock frequency is ignored, as it is ignored adi,clock-xtal > > > > property. > > > > In case clock property is not set, the initialization always fails. > > > > There is no way to use an external clock source. > > > > As you already verified from source code, this was not intentional. > > > > > > > > While the proposed patch should make the DT config load as intended, > > > > there is a possible side effect on existing implementations relying > > > > on > > > > internal clock source only, that would be deactivated when clock > > > > property > > > > is defined in DT. > > > > > > Hmm, I see. I would argue anyone relying on that should have just > > > stepped forward and fix the real issue as you did :). So, maybe there's > > > no one actually relying on this but that's a big __maybe__... Not > > > really sure how to handle this. I'm not sure we want to keep > > > compatibility with something that's clearly wrong but often we need to > > > "live" with past messes. > > > > > > Jonathan I guess your input will be valuable here :)? > > > > Rather odd if they have bindings that say there is a clock but 'rely' > > on the internal clock as opposed to the current bug meaning they aren't > > using the external clock (but everything would work fine if they were). > > > > So I'd expect that we will only see a problem if someone has another > > bug somewhere else that means that clock isn't working correctly. > > At that point they will almost certainly want to fix that bug. > > The bug was introduced in June 2021 from c9ec2cb328e3 iio: adc: > ad7192: use devm_clk_get_optional() for mclk, where there is a clearly > mistaken logic inversion. > > Link: https://lore.kernel.org/all/20210513120752.90074-10-aardelean@deviqon.com/#r > > If you agree, I can send a revised patch set (perhaps within a new thread), > with the additional clarifications on clock usage in bindings documentation, > and one third patch for undocumented bindings. Sounds good to me. > > There is one last issue we are investigating, we have a work-around but > we have not found the root cause yet. I will post about it later, in > the hope the > patch set would include all the changes needed to have the driver operating > as expected. Sounds interesting :( > > Thank you once again for this review. NP. Thanks for sending your fixes upstream. Jonathan > > Fabrizio Lamarque > > > > > > > > > > In addition, bindings documentation states clock > > > > property is mandatory. > > > > > > > > > > This looks at least suspicious to me given the fact we have an internal > > > alternative. Clearly looks wrong to me... > > > > Agreed. I don't think it should be mandatory in the binding doc. > > > > Thanks, > > > > Jonathan > > > > > > > > > > - Nuno Sá > > > > > >
--- linux/drivers/iio/adc/ad7192.c 2023-03-13 19:32:42.646239506 +0100 +++ linux/drivers/iio/adc/ad7192.c 2023-03-14 10:19:32.361924242 +0100 @@ -367,7 +367,7 @@ static int ad7192_of_clock_select(struct clock_sel = AD7192_CLK_INT; /* use internal clock */ - if (st->mclk) { + if (!st->mclk) { if (of_property_read_bool(np, "adi,int-clock-output-enable")) clock_sel = AD7192_CLK_INT_CO; } else {
Allow the use of external clock when mclk clock is defined. When defining a mclk clock source in device tree with adi,clock-xtal property, the external crystal oscillator is not turned on. Without the change, the driver always uses the internal clock even when mclk clock is defined. Current implementation seems to contain a typo, since it expected st->mclk to be NULL within ad7192_of_clock_select() in order to select the external clock, but, if null, external clock cannot loaded correctly (out of bounds due to invalid mclk) in ad7192_probe(). I believe this patch follows the author's intended behavior. After applying this patch, the external oscillator is started as expected. I kindly ask your feedback, I may adjust the patch according to your suggestions. I could also follow up with another patch on documentation, containing the following (related) issues: - adi,int-clock-output-enable is undocumented - adi,clock-xtal is undocumented - regulator name avdd and its description is quite misleading, since this is unrelated to the AVdd pin (#20) of AD7192; it is used instead as reference voltage (REFIN1 on #15/#16 or REFIN2 on #7/#8). See int_vref_mv variable within driver implementation. Signed-off-by: Fabrizio Lamarque <fl.scratchpad@gmail.com>