Re: [PATCH] net: stmmac: add sanity check to device_property_read_u32_array call

[Martin Blumenstingl]

On Fri, Jun 28, 2019 at 6:05 PM Martin Blumenstingl
<martin.blumenstingl@xxxxxxxxxxxxxx> wrote:
>
> Hi Colin,
>
> On Fri, Jun 28, 2019 at 10:32 AM Colin Ian King
> <colin.king@xxxxxxxxxxxxx> wrote:
> >
> > On 28/06/2019 05:15, Martin Blumenstingl wrote:
> > > On Tue, Jun 25, 2019 at 9:58 AM Colin Ian King <colin.king@xxxxxxxxxxxxx> wrote:
> > >>
> > >> On 25/06/2019 05:44, Martin Blumenstingl wrote:
> > >>> Hi Colin,
> > >>>
> > >>> On Thu, Jun 20, 2019 at 3:34 AM Martin Blumenstingl
> > >>> <martin.blumenstingl@xxxxxxxxxxxxxx> wrote:
> > >>>>
> > >>>> Hi Colin,
> > >>>>
> > >>>> On Wed, Jun 19, 2019 at 8:55 AM Colin Ian King <colin.king@xxxxxxxxxxxxx> wrote:
> > >>>>>
> > >>>>> On 19/06/2019 06:13, Martin Blumenstingl wrote:
> > >>>>>> Hi Colin,
> > >>>>>>
> > >>>>>>> Currently the call to device_property_read_u32_array is not error checked
> > >>>>>>> leading to potential garbage values in the delays array that are then used
> > >>>>>>> in msleep delays.  Add a sanity check to the property fetching.
> > >>>>>>>
> > >>>>>>> Addresses-Coverity: ("Uninitialized scalar variable")
> > >>>>>>> Signed-off-by: Colin Ian King <colin.king@xxxxxxxxxxxxx>
> > >>>>>> I have also sent a patch [0] to fix initialize the array.
> > >>>>>> can you please look at my patch so we can work out which one to use?
> > >>>>>>
> > >>>>>> my concern is that the "snps,reset-delays-us" property is optional,
> > >>>>>> the current dt-bindings documentation states that it's a required
> > >>>>>> property. in reality it isn't, there are boards (two examples are
> > >>>>>> mentioned in my patch: [0]) without it.
> > >>>>>>
> > >>>>>> so I believe that the resulting behavior has to be:
> > >>>>>> 1. don't delay if this property is missing (instead of delaying for
> > >>>>>>    <garbage value> ms)
> > >>>>>> 2. don't error out if this property is missing
> > >>>>>>
> > >>>>>> your patch covers #1, can you please check whether #2 is also covered?
> > >>>>>> I tested case #2 when submitting my patch and it worked fine (even
> > >>>>>> though I could not reproduce the garbage values which are being read
> > >>>>>> on some boards)
> > >>> in the meantime I have tested your patch.
> > >>> when I don't set the "snps,reset-delays-us" property then I get the
> > >>> following error:
> > >>>   invalid property snps,reset-delays-us
> > >>>
> > >>> my patch has landed in the meantime: [0]
> > >>> how should we proceed with your patch?
> >
> > Your fix is good, so I think we should just drop/forget about my fix.
> thank you for looking at the situation
>
> as far I understand the -net/-net-next tree all commits are immutable
> so if we want to remove your patch we need to send a revert
> do you want me to do that (I can do it on Monday) or will you take care of that?
I just sent the patch: [0]


[0] https://patchwork.ozlabs.org/patch/1125686/