| Message ID | 1596034301-5428-2-git-send-email-claudiu.beznea@microchip.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Eugen Hristev |
| Headers | show |
| Series | clk: at91: add sama7g5 support | expand |
Hi Claudiu, On Wed, 29 Jul 2020 at 08:51, Claudiu Beznea <claudiu.beznea@microchip.com> wrote: > > Check hw and hw->dev before dereference it. > > Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> > --- > drivers/clk/clk.c | 3 +++ > 1 file changed, 3 insertions(+) > Why is this needed? It adds to code size and these situations should not occur. Perhaps use assert()? > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 0f55ba751c0f..9fa18e342eaf 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -57,6 +57,9 @@ ulong clk_generic_get_rate(struct clk *clk) > > const char *clk_hw_get_name(const struct clk *hw) > { > + if (!hw || !hw->dev) > + return NULL; > + > return hw->dev->name; > } > > -- > 2.7.4 > Regards, SImon
On 04.08.2020 05:00, Simon Glass wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi Claudiu, > > On Wed, 29 Jul 2020 at 08:51, Claudiu Beznea > <claudiu.beznea@microchip.com> wrote: >> >> Check hw and hw->dev before dereference it. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> >> --- >> drivers/clk/clk.c | 3 +++ >> 1 file changed, 3 insertions(+) >> > > Why is this needed? It adds to code size and these situations should > not occur. Perhaps use assert()? In my debugging, investigating the issues that patches 03/22, 04/22, 06/22 try to address, I reached also this function and checked these pointers. In the end the issue was not related to them but I though it might be useful to keep these in a patch. I will remove it in the next version. > >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >> index 0f55ba751c0f..9fa18e342eaf 100644 >> --- a/drivers/clk/clk.c >> +++ b/drivers/clk/clk.c >> @@ -57,6 +57,9 @@ ulong clk_generic_get_rate(struct clk *clk) >> >> const char *clk_hw_get_name(const struct clk *hw) >> { >> + if (!hw || !hw->dev) >> + return NULL; >> + >> return hw->dev->name; >> } >> >> -- >> 2.7.4 >> > > Regards, > SImon >
Hi Claudiu, On Tue, 4 Aug 2020 at 01:19, <Claudiu.Beznea@microchip.com> wrote: > > > > On 04.08.2020 05:00, Simon Glass wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > Hi Claudiu, > > > > On Wed, 29 Jul 2020 at 08:51, Claudiu Beznea > > <claudiu.beznea@microchip.com> wrote: > >> > >> Check hw and hw->dev before dereference it. > >> > >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> > >> --- > >> drivers/clk/clk.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > > > > Why is this needed? It adds to code size and these situations should > > not occur. Perhaps use assert()? > > In my debugging, investigating the issues that patches 03/22, 04/22, 06/22 > try to address, I reached also this function and checked these pointers. In > the end the issue was not related to them but I though it might be useful > to keep these in a patch. I will remove it in the next version. IMO we should use assert() to check invariants and catch basic programming errors. But production testing should make sure that the software basically works. Of course it is nice to have these checks, but they add to code size which is always a concern. So I think we should rely on assert() to catch the errors during development, so we are not wasting code checking for things that we know cannot happen. Regards, Simon
Hi Simon, On 04.08.2020 18:08, Simon Glass wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi Claudiu, > > On Tue, 4 Aug 2020 at 01:19, <Claudiu.Beznea@microchip.com> wrote: >> >> >> >> On 04.08.2020 05:00, Simon Glass wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> Hi Claudiu, >>> >>> On Wed, 29 Jul 2020 at 08:51, Claudiu Beznea >>> <claudiu.beznea@microchip.com> wrote: >>>> >>>> Check hw and hw->dev before dereference it. >>>> >>>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> >>>> --- >>>> drivers/clk/clk.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>> >>> Why is this needed? It adds to code size and these situations should >>> not occur. Perhaps use assert()? >> >> In my debugging, investigating the issues that patches 03/22, 04/22, 06/22 >> try to address, I reached also this function and checked these pointers. In >> the end the issue was not related to them but I though it might be useful >> to keep these in a patch. I will remove it in the next version. > > IMO we should use assert() to check invariants and catch basic > programming errors. But production testing should make sure that the > software basically works. > > Of course it is nice to have these checks, but they add to code size > which is always a concern. So I think we should rely on assert() to > catch the errors during development, so we are not wasting code > checking for things that we know cannot happen. OK, I'll switch to assert(). Thank you, Claudiu Beznea > > Regards, > Simon >
Hi Claudiu, On Tue, 4 Aug 2020 at 09:25, <Claudiu.Beznea@microchip.com> wrote: > > Hi Simon, > > On 04.08.2020 18:08, Simon Glass wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > Hi Claudiu, > > > > On Tue, 4 Aug 2020 at 01:19, <Claudiu.Beznea@microchip.com> wrote: > >> > >> > >> > >> On 04.08.2020 05:00, Simon Glass wrote: > >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >>> > >>> Hi Claudiu, > >>> > >>> On Wed, 29 Jul 2020 at 08:51, Claudiu Beznea > >>> <claudiu.beznea@microchip.com> wrote: > >>>> > >>>> Check hw and hw->dev before dereference it. > >>>> > >>>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> > >>>> --- > >>>> drivers/clk/clk.c | 3 +++ > >>>> 1 file changed, 3 insertions(+) > >>>> > >>> > >>> Why is this needed? It adds to code size and these situations should > >>> not occur. Perhaps use assert()? > >> > >> In my debugging, investigating the issues that patches 03/22, 04/22, 06/22 > >> try to address, I reached also this function and checked these pointers. In > >> the end the issue was not related to them but I though it might be useful > >> to keep these in a patch. I will remove it in the next version. > > > > IMO we should use assert() to check invariants and catch basic > > programming errors. But production testing should make sure that the > > software basically works. > > > > Of course it is nice to have these checks, but they add to code size > > which is always a concern. So I think we should rely on assert() to > > catch the errors during development, so we are not wasting code > > checking for things that we know cannot happen. > > OK, I'll switch to assert(). One more point I should have made is that my comments apply mostly to common code that everyone has to use - e.g. the core clock code. So if you want to put dev_err() and other things in your driver and you know about the code-size implications that is less of a concern. But with common code, we should be careful. Regards, Simon
On 04.08.2020 18:29, Simon Glass wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi Claudiu, > > On Tue, 4 Aug 2020 at 09:25, <Claudiu.Beznea@microchip.com> wrote: >> >> Hi Simon, >> >> On 04.08.2020 18:08, Simon Glass wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> Hi Claudiu, >>> >>> On Tue, 4 Aug 2020 at 01:19, <Claudiu.Beznea@microchip.com> wrote: >>>> >>>> >>>> >>>> On 04.08.2020 05:00, Simon Glass wrote: >>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>>>> >>>>> Hi Claudiu, >>>>> >>>>> On Wed, 29 Jul 2020 at 08:51, Claudiu Beznea >>>>> <claudiu.beznea@microchip.com> wrote: >>>>>> >>>>>> Check hw and hw->dev before dereference it. >>>>>> >>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> >>>>>> --- >>>>>> drivers/clk/clk.c | 3 +++ >>>>>> 1 file changed, 3 insertions(+) >>>>>> >>>>> >>>>> Why is this needed? It adds to code size and these situations should >>>>> not occur. Perhaps use assert()? >>>> >>>> In my debugging, investigating the issues that patches 03/22, 04/22, 06/22 >>>> try to address, I reached also this function and checked these pointers. In >>>> the end the issue was not related to them but I though it might be useful >>>> to keep these in a patch. I will remove it in the next version. >>> >>> IMO we should use assert() to check invariants and catch basic >>> programming errors. But production testing should make sure that the >>> software basically works. >>> >>> Of course it is nice to have these checks, but they add to code size >>> which is always a concern. So I think we should rely on assert() to >>> catch the errors during development, so we are not wasting code >>> checking for things that we know cannot happen. >> >> OK, I'll switch to assert(). > > One more point I should have made is that my comments apply mostly to > common code that everyone has to use - e.g. the core clock code. So if > you want to put dev_err() and other things in your driver and you know > about the code-size implications that is less of a concern. But with > common code, we should be careful. Sure! Thank you, Claudiu Beznea > > Regards, > Simon >
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 0f55ba751c0f..9fa18e342eaf 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -57,6 +57,9 @@ ulong clk_generic_get_rate(struct clk *clk) const char *clk_hw_get_name(const struct clk *hw) { + if (!hw || !hw->dev) + return NULL; + return hw->dev->name; }
Check hw and hw->dev before dereference it. Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- drivers/clk/clk.c | 3 +++ 1 file changed, 3 insertions(+)