On Wed, 3 Nov 2021 at 10:13, Andy Shevchenko [off-list ref] wrote:

On Tue, Nov 2, 2021 at 10:35 PM Emil Renner Berthing [off-list ref] wrote:

quoted

On Tue, 2 Nov 2021 at 21:02, Andy Shevchenko [off-list ref] wrote:

quoted

On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing [off-list ref] wrote:

quoted

quoted

quoted

+       switch (trigger) {

quoted

quoted

quoted

+       default:

quoted

+               irq_set_handler_locked(d, handle_bad_irq);

Why? You have it already in ->probe(), what's the point?

So last time you asked about this, I explained a situation where
userspace first grabs a GPIO, set the interrupt to edge triggered, and
then later loads a driver that requests an unsupported IRQ type.

I didn't get this scenario. Is it real?

No, it's totally made up, but I mean we even have tools like fuzzing
to help us find bugs that would never happen in real use cases.

quoted

Then
I'd like to set the handler back to handle_bad_irq so we don't get
weird interrupts, but maybe now you know a reason why that doesn't
matter or can't happen?

In ->probe() you set _default_ handler to bad(), what do you mean by
'set the handler back to bad()'? How is it otherwise if you free an
interrupt?

It might not be, but when not sure I thought it better to error on the
safe side.

So, please elaborate with call traces what the scenario / use case you
are talking about. If it's true what you are saying, we have a
situation (plenty of GPIO drivers don't do what you are suggesting
here).

quoted

quoted

quoted

+               return -EINVAL;
+       }

...

quoted

quoted

quoted

+       ret = reset_control_deassert(rst);
+       if (ret)
+               return dev_err_probe(dev, ret, "could not deassert resetd\n");

quoted

+       ret = devm_pinctrl_register_and_init(dev, &starfive_desc, sfp, &sfp->pctl);
+       if (ret)

I don't see who will assert reset here.

No, so originally this driver would first assert and then deassert
reset. I decided against that because in all likelyhood earlier boot
stages would have set pinmux up for a serial port, and we don't want
to interrupt the serial debug output. The only reason I make sure the
reset line is deasserted is in case someone makes a really minimal
bootloader that just does the absolute minimal to load a Linux kernel
and doesn't even log any anything.

By the same token we also don't want to assert reset on error in case
it resets pin muxing for the the serial line that was supposed to log
the error.

Perhaps comment in the code explaining this?

Sure.