From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Rini Date: Mon, 25 Jan 2021 12:13:52 -0500 Subject: [PATCH 1/5] net: Introduce DSA class for Ethernet switches In-Reply-To: References: <20210113180526.21797-1-claudiu.manoil@nxp.com> <20210113180526.21797-2-claudiu.manoil@nxp.com> <20210119210011.GW9782@bill-the-cat> Message-ID: <20210125171352.GJ7530@bill-the-cat> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Tue, Jan 19, 2021 at 05:15:26PM -0700, Simon Glass wrote: > Hi Tom, > > On Tue, 19 Jan 2021 at 14:00, Tom Rini wrote: > > > > 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 wrote: > > > > > > > > >-----Original Message----- > > > > >From: Simon Glass > > > > >Sent: Thursday, January 14, 2021 5:42 PM > > > > >To: Claudiu Manoil > > > > >Cc: Joe Hershberger ; Bin Meng > > > > >; Michael Walle ; U-Boot Mailing > > > > >List ; Vladimir Oltean ; > > > > >Alexandru Marginean > > > > >Subject: Re: [PATCH 1/5] net: Introduce DSA class for Ethernet switches > > > > > > > > > [...] > > > > > > > > > >Reviewed-by: Simon Glass > > > > > > > > > >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?" > > Well if you are able to inject code then I suppose you can do > anything. You don't need to worry about existing parameter checking. > > The difference in my mind is whether there is user/input data > involved. If so, then we need lots of checks. If it is just telling > the clock to go a 2GHz, then the checks are probably just bloating the > code. So rather than leave this to individual preference, I am > wondering whether a Kconfig option might be best. Thinking about this more, yes, if we can macro the check, then we can hide it (and potential other checks) under some SANITY_CHECKS option. -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 659 bytes Desc: not available URL: