On Tue, Jan 19, 2021 at 11:06:10AM -0700, Simon Glass wrote: > Hi Claudiu, > > On Fri, 15 Jan 2021 at 09:47, Claudiu Manoil <claudiu.man...@nxp.com> wrote: > > > > >-----Original Message----- > > >From: Simon Glass <s...@chromium.org> > > >Sent: Thursday, January 14, 2021 5:42 PM > > >To: Claudiu Manoil <claudiu.man...@nxp.com> > > >Cc: Joe Hershberger <joe.hershber...@ni.com>; Bin Meng > > ><bmeng...@gmail.com>; Michael Walle <mich...@walle.cc>; U-Boot Mailing > > >List <u-boot@lists.denx.de>; Vladimir Oltean <vladimir.olt...@nxp.com>; > > >Alexandru Marginean <alexandru.margin...@nxp.com> > > >Subject: Re: [PATCH 1/5] net: Introduce DSA class for Ethernet switches > > > > > [...] > > > > > >Reviewed-by: Simon Glass <s...@chromium.org> > > > > > >I don't think it is necessary to have the 'if (!pdev)' checks around > > >the place. We need a way in U-Boot to have checks like that to catch > > >programming errors but to be able to turn them off in production code > > >to reduce size. > > > > > >I suppose a Kconfig would do it, with: > > > > > >if (CONFIG_IS_ENABLED(SAFETY) && !pdev) > > > return log_,msg_ref("safety", -ENODEV) > > > > > >Also note that -ENODEV is used by drive rmodel so it generally isn't > > >safe to return it as a logic error. I think in this case because it > > >never happens, it should be OK. > > > > > > > Thanks for the review, Simon. > > I thought about using assert(pdev) checks, but during development the > > simple "if (!pdev)..." proved more friendly. I like your idea about > > enabling > > the checks at compile time and disabling them in production. > > For now, since this SAFETY flag is not implemented, my understanding is > > that you’re ok with leaving the pdev checks in the code as they are right > > now > > and sometime in the future these will be converted to the "SAFETY" construct > > you mention. > > > > Yes that's fine, you have my review tag. > > +Tom Rini what do you think about CONFIG_SAFETY or similar to allow > these bug checks to be disabled for code-size reasons?
I don't know. Setting aside the name, my first concern is "so we disable certain forms of sanity checks, now assuming a malicious entity somewhere, what's now able to be exploited?" -- Tom
signature.asc
Description: PGP signature