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 trini@konsulko.com 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 claudiu.manoil@nxp.com wrote:
-----Original Message----- From: Simon Glass sjg@chromium.org Sent: Thursday, January 14, 2021 5:42 PM To: Claudiu Manoil claudiu.manoil@nxp.com Cc: Joe Hershberger joe.hershberger@ni.com; Bin Meng bmeng.cn@gmail.com; Michael Walle michael@walle.cc; U-Boot Mailing List u-boot@lists.denx.de; Vladimir Oltean vladimir.oltean@nxp.com; Alexandru Marginean alexandru.marginean@nxp.com Subject: Re: [PATCH 1/5] net: Introduce DSA class for Ethernet switches
[...]
Reviewed-by: Simon Glass sjg@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?"
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.