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

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
To: Colin Ian King <colin.king@canonical.com>
Cc: alexandre.torgue@st.com, netdev@vger.kernel.org,
	kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-stm32@st-md-mailman.stormreply.com, joabreu@synopsys.com,
	mcoquelin.stm32@gmail.com, peppe.cavallaro@st.com,
	davem@davemloft.net, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] net: stmmac: add sanity check to device_property_read_u32_array call
Date: Tue, 2 Jul 2019 00:43:21 +0200	[thread overview]
Message-ID: <CAFBinCCpJLSQiUeqpQTKQDgjy7-ROgjYa913Xe1My_oc6miTzw@mail.gmail.com> (raw)
In-Reply-To: <CAFBinCAx5qrPK1z68bF-tGKpJQfKLnee65qBOxMS4nj8t381+Q@mail.gmail.com>

On Fri, Jun 28, 2019 at 6:05 PM Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
>
> Hi Colin,
>
> On Fri, Jun 28, 2019 at 10:32 AM Colin Ian King
> <colin.king@canonical.com> wrote:
> >
> > On 28/06/2019 05:15, Martin Blumenstingl wrote:
> > > On Tue, Jun 25, 2019 at 9:58 AM Colin Ian King <colin.king@canonical.com> 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@googlemail.com> wrote:
> > >>>>
> > >>>> Hi Colin,
> > >>>>
> > >>>> On Wed, Jun 19, 2019 at 8:55 AM Colin Ian King <colin.king@canonical.com> 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@canonical.com>
> > >>>>>> 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/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel