Re: [PATCH v8 05/10] pinctrl: eyeq5: add platform driver

From: Andy Shevchenko <andriy.shevchenko@intel.com>
To: "Théo Lebrun" <theo.lebrun@bootlin.com>
Cc: "Gregory CLEMENT" <gregory.clement@bootlin.com>,
	"Michael Turquette" <mturquette@baylibre.com>,
	"Stephen Boyd" <sboyd@kernel.org>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"Conor Dooley" <conor+dt@kernel.org>,
	"Thomas Bogendoerfer" <tsbogend@alpha.franken.de>,
	"Linus Walleij" <linus.walleij@linaro.org>,
	"Rafał Miłecki" <rafal@milecki.pl>,
	"Philipp Zabel" <p.zabel@pengutronix.de>,
	"Vladimir Kondratiev" <vladimir.kondratiev@mobileye.com>,
	linux-mips@vger.kernel.org, linux-clk@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	"Thomas Petazzoni" <thomas.petazzoni@bootlin.com>,
	"Tawfik Bayouk" <tawfik.bayouk@mobileye.com>,
	linux-gpio@vger.kernel.org
Subject: Re: [PATCH v8 05/10] pinctrl: eyeq5: add platform driver
Date: Thu, 29 Feb 2024 17:32:35 +0200	[thread overview]
Message-ID: <ZeCjk9QgtCWb8Ecy@smile.fi.intel.com> (raw)
In-Reply-To: <CZHNS29NK9RR.13G019Y9ZY6IO@bootlin.com>

On Thu, Feb 29, 2024 at 04:13:15PM +0100, Théo Lebrun wrote:
> On Thu Feb 29, 2024 at 12:35 PM CET, Andy Shevchenko wrote:
> > On Wed, Feb 28, 2024 at 07:15:12PM +0100, Théo Lebrun wrote:
> > > On Tue Feb 27, 2024 at 7:14 PM CET, Andy Shevchenko wrote:
> > > > On Tue, Feb 27, 2024 at 03:55:26PM +0100, Théo Lebrun wrote:

...

> > > > > +	bool "Mobileye EyeQ5 pinctrl driver"
> > > >
> > > > Can't be a module?
> > > 
> > > It theory it could, I however do not see why that would be done. Pinctrl
> > > is essential to the platform capabilities. The platform is an embedded
> > > one and performance-oriented; boot-time is important and no user will
> > > ever want to load pinctrl as a module.
> >
> > I can argue. The modularization can give a better granularity in the exactly
> > embedded world when the memory resource (flash/RAM) is limited or fragmented
> > (for one or another reason). Having less weighty kernel at boot makes it smaller
> > to fit, for example, faster read only memory block which is not so uncommon.
> 
> I can argue back. :-) Granularity brought from modules is useful either
> in (1) resource constrained boot context or (2) for peripherals which
> some people might want to do without. We are not in case 1 nor case 2.
> 
> > The rule of thumb is to make modules if, otherwise, it's not so critical for
> > the boot process (and even for some cases we still may have it done as a module
> > with help of deferred probe mechanism).
> 
> I'd call SoC pin control a critical resource for the boot process.
> 
> I also like the simplicity of builtin better for such a resource.
>  - If we tristate pinctrl-eyeq5 and there is a bug, there is a bug (in a
>    context that we have no reason to support).
>  - If we do not allow it and there is a bug, there is no bug.
>    Plus, it makes one less choice for people configuring the kernel.

The problem is that you reduce the flexibility. Nobody prevents you from having
it built-in while tristate. But completely different situation when it's bool.

So my argument still stays. I think new code shouldn't be boolean by default.
The only exceptional cases can do that (like PMIC driver or critical clock one).

[...]

> > > > > +	if (WARN_ON(offset > 31))
> > > > > +		return false;
> > > >
> > > > When this condition can be true?
> > > 
> > > If there is a bug in the code. Defensive programming.
> > > 
> > > There is this subtle conversion of pin numbers => offset inside of a
> > > bank. If one function forgets doing this then eq5p_test_bit() gets
> > > called with a pin number.
> > > 
> > > In this GPIO series I fixed such a bug in a 10 year old driver:
> > > https://lore.kernel.org/lkml/20240228-mbly-gpio-v2-5-3ba757474006@bootlin.com/
> > > 
> > > The whole "if it can happen it will happen" mantra. We'll get a warning
> > > in the logs using pinctrl-eyeq5.
> >
> > My point here that we have mechanisms to avoid such issues, for example in GPIO
> > we have valid_mask field and GPIO library takes care to avoid such conditions
> > from happening. Please, double check that you really need these in your driver.
> > I prefer to avoid them until it's proven that they are real cases.
> 
> Whatever the subsystem does to protect us (like only calling callbacks
> with valid IDs), it will not protect us from bugs inside the driver's
> callbacks.
> 
> I do no see a reason to avoid such code. I do not trust myself to write
> perfect code.

Perfect is enemy of good. ;)

> Its aim is to protect ourselves from our own mistakes. If
> such an issue occurs, understanding that this is what happened would be
> really hard (especially if it occurs on someone else's boards).

Yes, but we usually don't put a dead code into the kernel. So, can you confirm
that warning can appear IRL? If yes, there is another red flag or question:
why WARN()? This is easily becomes a panic and/or reboot (depending to the kernel
command line) and hence may give unresponsive system. Was this considered?

...

> > > > > +static const struct pinctrl_ops eq5p_pinctrl_ops = {
> > > > > +	.get_groups_count	= eq5p_pinctrl_get_groups_count,
> > > > > +	.get_group_name		= eq5p_pinctrl_get_group_name,
> > > > > +	.get_group_pins		= eq5p_pinctrl_get_group_pins,
> > > > > +	.pin_dbg_show		= eq5p_pinctrl_pin_dbg_show,
> > > >
> > > > > +	.dt_node_to_map		= pinconf_generic_dt_node_to_map_pin,
> > > > > +	.dt_free_map		= pinctrl_utils_free_map,
> > > >
> > > > ifdef is missing for these... But the question is, isn't these a default when
> > > > OF is in use?
> > > 
> > > Doesn't look like it is. In drivers/pinctrl/devicetree.c:
> > > 
> > > 	static int dt_to_map_one_config(struct pinctrl *p,
> > > 					struct pinctrl_dev *hog_pctldev,
> > > 					const char *statename,
> > > 					struct device_node *np_config)
> > > 	{
> > > 		// ...
> > > 
> > > 		/*
> > > 		 * Call pinctrl driver to parse device tree node, and
> > > 		 * generate mapping table entries
> > > 		 */
> > > 		ops = pctldev->desc->pctlops;
> > > 		if (!ops->dt_node_to_map) {
> > > 			dev_err(p->dev, "pctldev %s doesn't support DT\n",
> > > 				dev_name(pctldev->dev));
> > > 			return -ENODEV;
> > > 		}
> > > 
> > > 		// ...
> > > 	}
> > > 
> > > And I see nowhere that puts a value if ->dt_node_to_map is empty.
> > > 
> > > For dt_free_map, it is an optional value. If the field is NULL nothing
> > > is done. See dt_free_map() in the same file.
> >
> > If we drop OF dependency, these fields might not be present in the structure
> > (by definition). Compilation won't succeed. Am I mistaken?
> 
> struct pinctrl_ops has both ->dt_node_to_map and ->dt_free_map fields in
> any case. See include/linux/pinctrl/pinctrl.h which declares the
> struct. The function pointers we put are also under no conditional
> compilation.

Indeed, I mixed it with something else (probably GPIO library and one of its
core structures) where it's the case.

-- 
With Best Regards,
Andy Shevchenko