| Message ID | 20220120204132.17875-4-quic_amelende@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for pm8941-pwrkey.c | expand |
On 1/20/2022 12:41 PM, Anjelique Melendez wrote: > From: David Collins <collinsd@codeaurora.org> > > Add a null check for the pwrkey->data pointer after it is assigned > in pm8941_pwrkey_probe(). This avoids a potential null pointer > dereference when pwrkey->data->has_pon_pbs is accessed later in > the probe function. > > Change-Id: I589c4851e544d79a1863fd110b32a0b45ac03caf > Signed-off-by: David Collins <collinsd@codeaurora.org> > Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com> > --- > drivers/input/misc/pm8941-pwrkey.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c > index 0ce00736e695..ac08ed025802 100644 > --- a/drivers/input/misc/pm8941-pwrkey.c > +++ b/drivers/input/misc/pm8941-pwrkey.c > @@ -263,6 +263,10 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev) > > pwrkey->dev = &pdev->dev; > pwrkey->data = of_device_get_match_data(&pdev->dev); > + if (!pwrkey->data) { > + dev_err(&pdev->dev, "match data not found\n"); > + return -ENODEV; > + } > I don't understand why this patch is 3rd in the series. Isn't it independent from the debounce time? If not, then why it is not fixed as part of the patch which adds this debounce time support? ---Trilok Soni
On 1/20/2022 2:18 PM, Trilok Soni wrote: > On 1/20/2022 12:41 PM, Anjelique Melendez wrote: >> From: David Collins <collinsd@codeaurora.org> >> >> Add a null check for the pwrkey->data pointer after it is assigned >> in pm8941_pwrkey_probe(). This avoids a potential null pointer >> dereference when pwrkey->data->has_pon_pbs is accessed later in >> the probe function. >> >> Change-Id: I589c4851e544d79a1863fd110b32a0b45ac03caf >> Signed-off-by: David Collins <collinsd@codeaurora.org> >> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com> >> --- >> drivers/input/misc/pm8941-pwrkey.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c >> index 0ce00736e695..ac08ed025802 100644 >> --- a/drivers/input/misc/pm8941-pwrkey.c >> +++ b/drivers/input/misc/pm8941-pwrkey.c >> @@ -263,6 +263,10 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev) >> pwrkey->dev = &pdev->dev; >> pwrkey->data = of_device_get_match_data(&pdev->dev); >> + if (!pwrkey->data) { >> + dev_err(&pdev->dev, "match data not found\n"); >> + return -ENODEV; >> + } >> > > I don't understand why this patch is 3rd in the series. Isn't it independent from the debounce time? If not, then why it is not fixed as part of the patch which adds this debounce time support? > > ---Trilok Soni You are correct that this patch is independent from debounce time. In the following version I will move this patch up to be the first patch!
Quoting Bjorn Andersson (2022-01-24 14:26:34) > On Thu 20 Jan 20:18 PST 2022, Stephen Boyd wrote: > > > Quoting Anjelique Melendez (2022-01-20 16:25:26) > > > > > > On 1/20/2022 3:01 PM, Bjorn Andersson wrote: > > > > On Thu 20 Jan 12:41 PST 2022, Anjelique Melendez wrote: > > > > > > > >> From: David Collins <collinsd@codeaurora.org> > > > >> > > > >> Add a null check for the pwrkey->data pointer after it is assigned > > > >> in pm8941_pwrkey_probe(). This avoids a potential null pointer > > > >> dereference when pwrkey->data->has_pon_pbs is accessed later in > > > >> the probe function. > > > >> > > > >> Change-Id: I589c4851e544d79a1863fd110b32a0b45ac03caf > > > >> Signed-off-by: David Collins <collinsd@codeaurora.org> > > > >> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com> > > > >> --- > > > >> drivers/input/misc/pm8941-pwrkey.c | 4 ++++ > > > >> 1 file changed, 4 insertions(+) > > > >> > > > >> diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c > > > >> index 0ce00736e695..ac08ed025802 100644 > > > >> --- a/drivers/input/misc/pm8941-pwrkey.c > > > >> +++ b/drivers/input/misc/pm8941-pwrkey.c > > > >> @@ -263,6 +263,10 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev) > > > >> > > > >> pwrkey->dev = &pdev->dev; > > > >> pwrkey->data = of_device_get_match_data(&pdev->dev); > > > >> + if (!pwrkey->data) { > > > > The only way this can happen is if you add a new compatible and forget > > > > to specify data and when that happens you will get a print in the log > > > > somewhere, which once you realize that you don't have your pwrkey you > > > > might be able to find among all the other prints. > > > > > > > > If you instead don't NULL check this pointer you will get a large splat > > > > in the log, with callstack and all, immediately hinting you that > > > > pwrkey->data is NULL. > > > > > > > > > > > > In other words, there's already a print, a much larger print and I don't > > > > think there's value in handling this mistake gracefully. > > > > > > > > Regards, > > > > Bjorn > > > > > > > > > We would like to the null pointer check in place to avoid static analysis > > > > > > warnings that can be easily fixed. > > > > > > > Many drivers check that their device_get_match_data() returns a valid > > pointer. I'd like to see that API used in addition to checking the > > return value for NULL so that we can keep the static analysis tools > > happy. Yes it's an impossible case assuming the driver writer didn't > > mess up but it shuts SA up and we don't really have a better solution > > to tell tools that device_get_match_data() can't return NULL. > > I'm not saying that device_get_match_data() can't return NULL, Indeed, I wasn't implying that you were saying that. > I'm > saying that in the very specific cases that it would return NULL it's > useful to have a kernel panic - as that's a much faster way to figure > out that something is wrong. I see it as more annoying, but maybe that's my workflow? When my kernel oopses I have to go back to a recovery kernel, which takes me a few more seconds to "repair" my device. If the driver only failed to probe then I'd probably be able to boot far enough to get networking and more easily replace my kernel with a working device. And I'd have userspace access so I could poke around and figure out why the driver failed to probe. Now obviously a big stacktrace would be helpful to know that it's the power key driver that's busted, but it's not like we're calling some internal API here. We're trying to probe a driver and if that oopses because the driver writer failed at their job then it's bad on them for writing a bad patch but also annoying for the integrator who has to deal with the mess they created. I'd rather have a half working system here vs. a totally broken one.
On Mon 24 Jan 17:55 PST 2022, Stephen Boyd wrote: > Quoting Bjorn Andersson (2022-01-24 14:26:34) > > On Thu 20 Jan 20:18 PST 2022, Stephen Boyd wrote: > > > > > Quoting Anjelique Melendez (2022-01-20 16:25:26) > > > > > > > > On 1/20/2022 3:01 PM, Bjorn Andersson wrote: > > > > > On Thu 20 Jan 12:41 PST 2022, Anjelique Melendez wrote: > > > > > > > > > >> From: David Collins <collinsd@codeaurora.org> > > > > >> > > > > >> Add a null check for the pwrkey->data pointer after it is assigned > > > > >> in pm8941_pwrkey_probe(). This avoids a potential null pointer > > > > >> dereference when pwrkey->data->has_pon_pbs is accessed later in > > > > >> the probe function. > > > > >> > > > > >> Change-Id: I589c4851e544d79a1863fd110b32a0b45ac03caf > > > > >> Signed-off-by: David Collins <collinsd@codeaurora.org> > > > > >> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com> > > > > >> --- > > > > >> drivers/input/misc/pm8941-pwrkey.c | 4 ++++ > > > > >> 1 file changed, 4 insertions(+) > > > > >> > > > > >> diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c > > > > >> index 0ce00736e695..ac08ed025802 100644 > > > > >> --- a/drivers/input/misc/pm8941-pwrkey.c > > > > >> +++ b/drivers/input/misc/pm8941-pwrkey.c > > > > >> @@ -263,6 +263,10 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev) > > > > >> > > > > >> pwrkey->dev = &pdev->dev; > > > > >> pwrkey->data = of_device_get_match_data(&pdev->dev); > > > > >> + if (!pwrkey->data) { > > > > > The only way this can happen is if you add a new compatible and forget > > > > > to specify data and when that happens you will get a print in the log > > > > > somewhere, which once you realize that you don't have your pwrkey you > > > > > might be able to find among all the other prints. > > > > > > > > > > If you instead don't NULL check this pointer you will get a large splat > > > > > in the log, with callstack and all, immediately hinting you that > > > > > pwrkey->data is NULL. > > > > > > > > > > > > > > > In other words, there's already a print, a much larger print and I don't > > > > > think there's value in handling this mistake gracefully. > > > > > > > > > > Regards, > > > > > Bjorn > > > > > > > > > > > > We would like to the null pointer check in place to avoid static analysis > > > > > > > > warnings that can be easily fixed. > > > > > > > > > > Many drivers check that their device_get_match_data() returns a valid > > > pointer. I'd like to see that API used in addition to checking the > > > return value for NULL so that we can keep the static analysis tools > > > happy. Yes it's an impossible case assuming the driver writer didn't > > > mess up but it shuts SA up and we don't really have a better solution > > > to tell tools that device_get_match_data() can't return NULL. > > > > I'm not saying that device_get_match_data() can't return NULL, > > Indeed, I wasn't implying that you were saying that. > > > I'm > > saying that in the very specific cases that it would return NULL it's > > useful to have a kernel panic - as that's a much faster way to figure > > out that something is wrong. > > I see it as more annoying, but maybe that's my workflow? When my kernel > oopses I have to go back to a recovery kernel, which takes me a few more > seconds to "repair" my device. If the driver only failed to probe then > I'd probably be able to boot far enough to get networking and more > easily replace my kernel with a working device. And I'd have userspace > access so I could poke around and figure out why the driver failed to > probe. Now obviously a big stacktrace would be helpful to know that it's > the power key driver that's busted, but it's not like we're calling some > internal API here. We're trying to probe a driver and if that oopses > because the driver writer failed at their job then it's bad on them for > writing a bad patch but also annoying for the integrator who has to deal > with the mess they created. I'd rather have a half working system here > vs. a totally broken one. Forgot about your recovery cycle, on most of my boards I just load a new kernel every boot, so there's no cost of recovering from a panic, it might even save me some time if it crashes completely before userspace starts consuming cycles. My only concern is that this "sets" a quite fuzzy precedence. I don't want us to just fix SA warnings all over the place, but I don't want it to be inconvenient to work on the kernel... Regards, Bjorn
On 1/25/2022 10:37 AM, Bjorn Andersson wrote: > On Mon 24 Jan 17:55 PST 2022, Stephen Boyd wrote: > >> Quoting Bjorn Andersson (2022-01-24 14:26:34) >>> On Thu 20 Jan 20:18 PST 2022, Stephen Boyd wrote: >>> >>>> Quoting Anjelique Melendez (2022-01-20 16:25:26) >>>>> On 1/20/2022 3:01 PM, Bjorn Andersson wrote: >>>>>> On Thu 20 Jan 12:41 PST 2022, Anjelique Melendez wrote: >>>>>> >>>>>>> From: David Collins <collinsd@codeaurora.org> >>>>>>> >>>>>>> Add a null check for the pwrkey->data pointer after it is assigned >>>>>>> in pm8941_pwrkey_probe(). This avoids a potential null pointer >>>>>>> dereference when pwrkey->data->has_pon_pbs is accessed later in >>>>>>> the probe function. >>>>>>> >>>>>>> Change-Id: I589c4851e544d79a1863fd110b32a0b45ac03caf >>>>>>> Signed-off-by: David Collins <collinsd@codeaurora.org> >>>>>>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com> >>>>>>> --- >>>>>>> drivers/input/misc/pm8941-pwrkey.c | 4 ++++ >>>>>>> 1 file changed, 4 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c >>>>>>> index 0ce00736e695..ac08ed025802 100644 >>>>>>> --- a/drivers/input/misc/pm8941-pwrkey.c >>>>>>> +++ b/drivers/input/misc/pm8941-pwrkey.c >>>>>>> @@ -263,6 +263,10 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev) >>>>>>> >>>>>>> pwrkey->dev = &pdev->dev; >>>>>>> pwrkey->data = of_device_get_match_data(&pdev->dev); >>>>>>> + if (!pwrkey->data) { >>>>>> The only way this can happen is if you add a new compatible and forget >>>>>> to specify data and when that happens you will get a print in the log >>>>>> somewhere, which once you realize that you don't have your pwrkey you >>>>>> might be able to find among all the other prints. >>>>>> >>>>>> If you instead don't NULL check this pointer you will get a large splat >>>>>> in the log, with callstack and all, immediately hinting you that >>>>>> pwrkey->data is NULL. >>>>>> >>>>>> >>>>>> In other words, there's already a print, a much larger print and I don't >>>>>> think there's value in handling this mistake gracefully. >>>>>> >>>>>> Regards, >>>>>> Bjorn >>>>> >>>>> We would like to the null pointer check in place to avoid static analysis >>>>> >>>>> warnings that can be easily fixed. >>>>> >>>> Many drivers check that their device_get_match_data() returns a valid >>>> pointer. I'd like to see that API used in addition to checking the >>>> return value for NULL so that we can keep the static analysis tools >>>> happy. Yes it's an impossible case assuming the driver writer didn't >>>> mess up but it shuts SA up and we don't really have a better solution >>>> to tell tools that device_get_match_data() can't return NULL. >>> I'm not saying that device_get_match_data() can't return NULL, >> Indeed, I wasn't implying that you were saying that. >> >>> I'm >>> saying that in the very specific cases that it would return NULL it's >>> useful to have a kernel panic - as that's a much faster way to figure >>> out that something is wrong. >> I see it as more annoying, but maybe that's my workflow? When my kernel >> oopses I have to go back to a recovery kernel, which takes me a few more >> seconds to "repair" my device. If the driver only failed to probe then >> I'd probably be able to boot far enough to get networking and more >> easily replace my kernel with a working device. And I'd have userspace >> access so I could poke around and figure out why the driver failed to >> probe. Now obviously a big stacktrace would be helpful to know that it's >> the power key driver that's busted, but it's not like we're calling some >> internal API here. We're trying to probe a driver and if that oopses >> because the driver writer failed at their job then it's bad on them for >> writing a bad patch but also annoying for the integrator who has to deal >> with the mess they created. I'd rather have a half working system here >> vs. a totally broken one. > Forgot about your recovery cycle, on most of my boards I just load a new > kernel every boot, so there's no cost of recovering from a panic, it > might even save me some time if it crashes completely before userspace > starts consuming cycles. > > My only concern is that this "sets" a quite fuzzy precedence. I don't > want us to just fix SA warnings all over the place, but I don't want it > to be inconvenient to work on the kernel... > > Regards, > Bjorn I will drop this patch for now so that further discussion can be had. Can send as a separate patch later.
diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c index 0ce00736e695..ac08ed025802 100644 --- a/drivers/input/misc/pm8941-pwrkey.c +++ b/drivers/input/misc/pm8941-pwrkey.c @@ -263,6 +263,10 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev) pwrkey->dev = &pdev->dev; pwrkey->data = of_device_get_match_data(&pdev->dev); + if (!pwrkey->data) { + dev_err(&pdev->dev, "match data not found\n"); + return -ENODEV; + } parent = pdev->dev.parent; regmap_node = pdev->dev.of_node;