[PATCH] pinctrl: pinconf-generic: check error value EOPNOTSUPP

[PATCH] pinctrl: pinconf-generic: check error value EOPNOTSUPP

[Peng Fan (OSS)]

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>
---
 drivers/pinctrl/pinconf-generic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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);
-- 
2.37.1

Re: [PATCH] pinctrl: pinconf-generic: check error value EOPNOTSUPP

[Linus Walleij]

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.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: pinconf-generic: check error value EOPNOTSUPP

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] pinctrl: pinconf-generic: check error value EOPNOTSUPP

[Linus Walleij]

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.

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.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: pinconf-generic: check error value EOPNOTSUPP

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH] pinctrl: pinconf-generic: check error value EOPNOTSUPP

[Peng Fan]

Hi Linus, Andy

> Subject: Re: [PATCH] pinctrl: pinconf-generic: check error value EOPNOTSUPP
> 
> 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.

ok. Got it. 

> > >
> > > 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? 

Sure.
Patch away, we will
> > help you if need be.
Let me try to fix it by myself, that would give me more practice.

Thanks,
Peng.

> 
> Indeed.
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

RE: [PATCH] pinctrl: pinconf-generic: check error value EOPNOTSUPP

[Peng Fan]

Hi Andy,

> Subject: Re: [PATCH] pinctrl: pinconf-generic: check error value EOPNOTSUPP
> 
> 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.

Would you please share me the documentation?

> 
> 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.

BTW: is there any issue if using EOPNOTSUPP here?

Thanks,
Peng.
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

Re: [PATCH] pinctrl: pinconf-generic: check error value EOPNOTSUPP

[Sudeep Holla]

On Fri, Apr 05, 2024 at 02:13:28AM +0000, Peng Fan wrote:
> Hi Andy,
>
> > Subject: Re: [PATCH] pinctrl: pinconf-generic: check error value EOPNOTSUPP
> >
> > 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.
>
> Would you please share me the documentation?
>

+1, I am interested in knowing more about this as I wasn't aware of 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.
>
> And checkpatch.pl reports warning for using ENOTSUPP.
>

Exactly, I do remember changing ENOTSUPP to EOPNOTSUPP based on checkpatch
suggestion. So either the checkpatch.pl or the document you are referring
is inconsistent and needs fixing either way.

--
Regards,
Sudeep

Re: [PATCH] pinctrl: pinconf-generic: check error value EOPNOTSUPP

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] pinctrl: pinconf-generic: check error value EOPNOTSUPP

[Andy Shevchenko]

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.

> > 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.

> 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).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] pinctrl: pinconf-generic: check error value EOPNOTSUPP

[Sudeep Holla]

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.

> > > 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 🙁.

--
Regards,
Sudeep

Re: [PATCH] pinctrl: pinconf-generic: check error value EOPNOTSUPP

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] pinctrl: pinconf-generic: check error value EOPNOTSUPP

[Sudeep Holla]

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

Re: [PATCH] pinctrl: pinconf-generic: check error value EOPNOTSUPP

[Andy Shevchenko]

On Fri, Apr 05, 2024 at 05:07:00PM +0100, Sudeep Holla wrote:
> 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

I (mis?) interpret your words that only GPIO/pin control uses ENOTSUPP internally.

> > > 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.

Yes, I am.

> As the number of occurrence of EOPNOTSUPP in the kernel overall is quite
> large.

Of course that is out of scope of GPIO/pin control design.

> 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.

Sure. Wherever you prefer, the only expectation that GPIO / pin control callbacks
will return into GPIO / pin control _core_ ENOTSUPP.

> 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.

Exactly!

> Thanks for the time and explanation.

Thanks for discussion!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] pinctrl: pinconf-generic: check error value EOPNOTSUPP

[Cristian Marussi]

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....

....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...

Thanks,
Cristian

Re: [PATCH] pinctrl: pinconf-generic: check error value EOPNOTSUPP

[Andy Shevchenko]

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.

For that, I'm going to submit fixes to Intel PMIC GPIO drivers soon.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] pinctrl: pinconf-generic: check error value EOPNOTSUPP

[Cristian Marussi]

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

Re: [PATCH] pinctrl: pinconf-generic: check error value EOPNOTSUPP

[Andy Shevchenko]

On Fri, Apr 05, 2024 at 05:23:31PM +0100, Cristian Marussi wrote:
> On Fri, Apr 05, 2024 at 07:16:56PM +0300, Andy Shevchenko wrote:

...

> 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.

Yes, that's what will work for me, thanks!

-- 
With Best Regards,
Andy Shevchenko