| Message ID | 20240401141031.3106216-1-peng.fan@oss.nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: pinconf-generic: check error value EOPNOTSUPP | expand |
On Thu, Apr 04, 2024 at 01:44:50PM +0200, Linus Walleij wrote: > On Mon, Apr 1, 2024 at 4:02 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: > > > From: Peng Fan <peng.fan@nxp.com> > > > > The SCMI error value SCMI_ERR_SUPPORT maps to linux error value > > '-EOPNOTSUPP', so when dump configs, need check the error value > > EOPNOTSUPP, otherwise there will be log "ERROR READING CONFIG SETTING". > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > (...) > > ret = pin_config_get_for_pin(pctldev, pin, &config); > > /* These are legal errors */ > > - if (ret == -EINVAL || ret == -ENOTSUPP) > > + if (ret == -EINVAL || ret == -ENOTSUPP || ret == -EOPNOTSUPP) > > TBH it's a bit odd to call an in-kernel API such as pin_config_get_for_pin() > and get -EOPNOTSUPP back. But it's not like I care a lot, so patch applied. Hmm... I would like actually to get this being consistent. The documentation explicitly says that in-kernel APIs uses Linux error code and not POSIX one. This check opens a Pandora box. FWIW, it just like dozen or so drivers that needs to be fixed, I prefer to have them being moved to ENOTSUPP, rather this patch.
On Thu, Apr 04, 2024 at 09:03:02PM +0200, Linus Walleij wrote: > On Thu, Apr 4, 2024 at 7:05 PM Andy Shevchenko > <andriy.shevchenko@intel.com> wrote: > > On Thu, Apr 04, 2024 at 01:44:50PM +0200, Linus Walleij wrote: > > > On Mon, Apr 1, 2024 at 4:02 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: > > > > > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > > > The SCMI error value SCMI_ERR_SUPPORT maps to linux error value > > > > '-EOPNOTSUPP', so when dump configs, need check the error value > > > > EOPNOTSUPP, otherwise there will be log "ERROR READING CONFIG SETTING". > > > > > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > > (...) > > > > ret = pin_config_get_for_pin(pctldev, pin, &config); > > > > /* These are legal errors */ > > > > - if (ret == -EINVAL || ret == -ENOTSUPP) > > > > + if (ret == -EINVAL || ret == -ENOTSUPP || ret == -EOPNOTSUPP) > > > > > > TBH it's a bit odd to call an in-kernel API such as pin_config_get_for_pin() > > > and get -EOPNOTSUPP back. But it's not like I care a lot, so patch applied. > > > > Hmm... I would like actually to get this being consistent. The documentation > > explicitly says that in-kernel APIs uses Linux error code and not POSIX one. > > > > This check opens a Pandora box. > > > > FWIW, it just like dozen or so drivers that needs to be fixed, I prefer to > > have them being moved to ENOTSUPP, rather this patch. $ git grep -lw EOPNOTSUPP -- drivers/pinctrl/ drivers/gpio/ drivers/gpio/gpio-crystalcove.c drivers/gpio/gpio-pcie-idio-24.c drivers/gpio/gpio-regmap.c drivers/gpio/gpio-wcove.c // drivers/gpio/gpiolib-cdev.c <<< Here it goes to user space, no need to fix drivers/pinctrl/actions/pinctrl-s500.c drivers/pinctrl/mediatek/mtk-eint.c drivers/pinctrl/mediatek/mtk-eint.h drivers/pinctrl/nxp/pinctrl-s32cc.c drivers/pinctrl/pinctrl-at91-pio4.c // drivers/pinctrl/pinctrl-aw9523.c <<< Should be fixed in Linus' tree by me drivers/pinctrl/pinctrl-ocelot.c drivers/pinctrl/renesas/pinctrl-rzg2l.c drivers/pinctrl/renesas/pinctrl-rzv2m.c drivers/pinctrl/sunplus/sppctl.c drivers/pinctrl/visconti/pinctrl-common.c > Andy is one of the wisest men I know so I have taken out the patch. > > Peng, what about fixing the problem at its source? Patch away, > we will help you if need be. Indeed.
On Fri, Apr 05, 2024 at 10:46:04AM +0100, Sudeep Holla wrote: > On Fri, Apr 05, 2024 at 02:13:28AM +0000, Peng Fan wrote: > > > On Thu, Apr 04, 2024 at 01:44:50PM +0200, Linus Walleij wrote: > > > > On Mon, Apr 1, 2024 at 4:02 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> > > > wrote: ... > > > > > + if (ret == -EINVAL || ret == -ENOTSUPP || ret == > > > > > + -EOPNOTSUPP) > > > > > > > > TBH it's a bit odd to call an in-kernel API such as > > > > pin_config_get_for_pin() and get -EOPNOTSUPP back. But it's not like I care > > > a lot, so patch applied. > > > > > > Hmm... I would like actually to get this being consistent. The documentation > > > explicitly says that in-kernel APIs uses Linux error code and not POSIX one. > > > > Would you please share me the documentation? > > +1, I am interested in knowing more about this as I wasn't aware of this. See my previous reply. > > > This check opens a Pandora box. > > > > > > FWIW, it just like dozen or so drivers that needs to be fixed, I prefer to have > > > them being moved to ENOTSUPP, rather this patch. > > > > I see many patches convert to use EOPNOTSUPP by checking git log. > > > > And checkpatch.pl reports warning for using ENOTSUPP. > > Exactly, I do remember changing ENOTSUPP to EOPNOTSUPP based on checkpatch > suggestion. Sometimes suggestions can be wrong. Checkpatch is famous for false-positives as it's a dumb tool, it can't know about everything. > So either the checkpatch.pl or the document you are referring > is inconsistent and needs fixing either way. True.
On Fri, Apr 05, 2024 at 04:47:19PM +0100, Sudeep Holla wrote: > On Fri, Apr 05, 2024 at 06:38:04PM +0300, Andy Shevchenko wrote: > > On Fri, Apr 05, 2024 at 02:13:28AM +0000, Peng Fan wrote: > > > > On Thu, Apr 04, 2024 at 01:44:50PM +0200, Linus Walleij wrote: > > > > > On Mon, Apr 1, 2024 at 4:02 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> > > > > wrote: ... > > > > This check opens a Pandora box. > > > > > > > > FWIW, it just like dozen or so drivers that needs to be fixed, I prefer to have > > > > them being moved to ENOTSUPP, rather this patch. > > > > > > I see many patches convert to use EOPNOTSUPP by checking git log. > > > > How is that related? You mean for GPIO/pin control drivers? > > > > > And checkpatch.pl reports warning for using ENOTSUPP. > > > > checkpatch has false-positives, this is just one of them. > > Fair enough. > > > > BTW: is there any issue if using EOPNOTSUPP here? > > > > Yes. we don't want to be inconsistent. Using both in one subsystem is asking > > for troubles. If you want EOPNOTSUPP, please convert *all* users and drop > > ENOTSUPP completely (series out of ~100+ patches I believe :-), which probably > > will be not welcome). > > Well, I don't agree with that 100% now since this is GPIO/pinmux sub-system > practice only. git grep -lw ENOTSUPP utterly disagrees with you. > What if we change the source/root error cause(SCMI) in this > case and keep GPIO/pinmux happy today but tomorrow when this needs to be > used in some other subsystem which uses EOPNOTSUPP by default/consistently. This is different case. For that we may shadow error codes with explicit comments. > Now how do we address that then, hence I mentioned I am not 100% in agreement > now while I was before knowing that this is GPIO/pinmux strategy. > > I don't know how to proceed now 🙁. KISS principle? There are only 10+ drivers to fix (I showed a rough list) to use ENOTSUPP instead of 100s+ otherwise.
On Fri, Apr 05, 2024 at 06:55:34PM +0300, Andy Shevchenko wrote: > On Fri, Apr 05, 2024 at 04:47:19PM +0100, Sudeep Holla wrote: > > > > Well, I don't agree with that 100% now since this is GPIO/pinmux sub-system > > practice only. > > git grep -lw ENOTSUPP > > utterly disagrees with you. > /me more confused. Though I haven't dig deeper to chech how many of these EOPNOTSUPP uses are intended for userspace. $git grep -lw ENOTSUPP | wc -l 713 git grep -lw EOPNOTSUPP | wc -l 2946 > > What if we change the source/root error cause(SCMI) in this > > case and keep GPIO/pinmux happy today but tomorrow when this needs to be > > used in some other subsystem which uses EOPNOTSUPP by default/consistently. > > This is different case. For that we may shadow error codes with explicit > comments. > Sure as along as that is acceptable. > > Now how do we address that then, hence I mentioned I am not 100% in agreement > > now while I was before knowing that this is GPIO/pinmux strategy. > > > > I don't know how to proceed now 🙁. > > KISS principle? There are only 10+ drivers to fix (I showed a rough list) > to use ENOTSUPP instead of 100s+ otherwise. > Again I assume you are referring to just GPIO/pinmux subsystem right. As the number of occurrence of EOPNOTSUPP in the kernel overall is quite large. I was thinking of changing the SCMI error map from EOPNOTSUPP to ENOTSUPP, but for now I think it is better to just handle the mapping in the pinmux part of SCMI that pinmux subsystem interacts with. In future if more subsystem expect ENOTSUPP, then we can change it. I hope this aligns with KISS principle as we are just fixing for the case that is know to cause issue rather than changing all probably regressing and then having to fix them all. Thanks for the time and explanation. -- Regards, Sudeep
On Fri, Apr 05, 2024 at 07:16:56PM +0300, Andy Shevchenko wrote: > On Fri, Apr 05, 2024 at 05:06:08PM +0100, Cristian Marussi wrote: > > On Fri, Apr 05, 2024 at 04:47:19PM +0100, Sudeep Holla wrote: > > > On Fri, Apr 05, 2024 at 06:38:04PM +0300, Andy Shevchenko wrote: > > > > On Fri, Apr 05, 2024 at 02:13:28AM +0000, Peng Fan wrote: > > > > > > On Thu, Apr 04, 2024 at 01:44:50PM +0200, Linus Walleij wrote: > > > > > > > On Mon, Apr 1, 2024 at 4:02 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> > > > > > > wrote: > > ... > > > > > > > > > ret = pin_config_get_for_pin(pctldev, pin, &config); > > > > > > > > /* These are legal errors */ > > > > > > > > - if (ret == -EINVAL || ret == -ENOTSUPP) > > > > > > > > + if (ret == -EINVAL || ret == -ENOTSUPP || ret == > > > > > > > > + -EOPNOTSUPP) > > > > > > > > > > > > > > TBH it's a bit odd to call an in-kernel API such as > > > > > > > pin_config_get_for_pin() and get -EOPNOTSUPP back. But it's not like I care > > > > > > a lot, so patch applied. > > > > > > > > > > > > Hmm... I would like actually to get this being consistent. The documentation > > > > > > explicitly says that in-kernel APIs uses Linux error code and not POSIX one. > > > > > > > > > > Would you please share me the documentation? > > > > > > > > Sure. > > > > https://elixir.bootlin.com/linux/latest/source/include/linux/pinctrl/pinconf.h#L24 > > > > https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib.c#L2825 > > > > https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib.c#L2845 > > > > > > > > I admit that this is not the best documented, feel free to produce a proper > > > > documentation. > > > > > > Ah OK, my bad. I assumed you were referring to the entire kernel tree and > > > not just GPIO/pinux. Sorry for that. > > > > ... from this thread, my understanding too was that this forbidden usage of > > POSIX errors for in-kernel API was general to any subsystem...so the ask > > for docs about this... > > > > > > > > This check opens a Pandora box. > > > > > > > > > > > > FWIW, it just like dozen or so drivers that needs to be fixed, I prefer to have > > > > > > them being moved to ENOTSUPP, rather this patch. > > > > > > > > > > I see many patches convert to use EOPNOTSUPP by checking git log. > > > > > > > > How is that related? You mean for GPIO/pin control drivers? > > > > > > > > > And checkpatch.pl reports warning for using ENOTSUPP. > > > > > > > > checkpatch has false-positives, this is just one of them. > > > > > > Fair enough. > > > > > > > > BTW: is there any issue if using EOPNOTSUPP here? > > > > > > > > Yes. we don't want to be inconsistent. Using both in one subsystem is asking > > > > for troubles. If you want EOPNOTSUPP, please convert *all* users and drop > > > > ENOTSUPP completely (series out of ~100+ patches I believe :-), which probably > > > > will be not welcome). > > > > > > Well, I don't agree with that 100% now since this is GPIO/pinmux sub-system > > > practice only. What if we change the source/root error cause(SCMI) in this > > > case and keep GPIO/pinmux happy today but tomorrow when this needs to be > > > used in some other subsystem which uses EOPNOTSUPP by default/consistently. > > > Now how do we address that then, hence I mentioned I am not 100% in agreement > > > now while I was before knowing that this is GPIO/pinmux strategy. > > > > > > I don't know how to proceed now 🙁. > > > > from checkpatch.pl: > > > > # ENOTSUPP is not a standard error code and should be avoided in new patches. > > # Folks usually mean EOPNOTSUPP (also called ENOTSUP), when they type ENOTSUPP. > > # Similarly to ENOSYS warning a small number of false positives is expected. > > > > ...so it seems to me that the this is NOT a false positive BUT Pinctrl/GPIO > > subsystem is an exception in these regards, since this is what is explcitly > > state in checkpatch comments AND there is no generalized doc on this.... > > Checkpatch is false positive _in this case_. > And in practice it's not the first and not the last false positive by checkpatch. > Again, checkpatch is a recommendation, not a letter of law. Documentation is. > > > ....but twe can happily oblige to Pinctrl expectations by remapping to ENOTSUPP > > in the pinctrl protocol layer or driver...just I won't certainly do that at the > > SCMI core layer at all at this point...as Sudeep said ...especially because there > > is NO generalized docs for the above ban of POSIX errors for in-Linux kernel API... > > There is no ban, we use POSIX error codes heavily in kernel. > > If you think GPIO/pin control has a flaw, send patches. I'm not a maintainer there, > perhaps conversion of all GPIO and pin control drivers to POSIX error code will be > warmly welcome, who knows. > > But before that, we want to have ENOTSUPP from callbacks that are used in > GPIO/pin control drivers for the sake of consistency. > Yes, my point was simply to say to fix the retcode of SCMI pinctrl to comply with Pinctrl expectations, and definitely NOT to fix and move to ENOTSUPP all the SCMI originated errors across all protocols, since it is NOT what is expected in general by other susbsystems. Thanks, Cristian
diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c index cada5d18ffae..541c2ac9ffcb 100644 --- a/drivers/pinctrl/pinconf-generic.c +++ b/drivers/pinctrl/pinconf-generic.c @@ -75,7 +75,7 @@ static void pinconf_generic_dump_one(struct pinctrl_dev *pctldev, else ret = pin_config_get_for_pin(pctldev, pin, &config); /* These are legal errors */ - if (ret == -EINVAL || ret == -ENOTSUPP) + if (ret == -EINVAL || ret == -ENOTSUPP || ret == -EOPNOTSUPP) continue; if (ret) { seq_printf(s, "ERROR READING CONFIG SETTING %d ", i);